Re: [PATCH v3 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko]

Hi Maxime,

On Tue, Feb 25, 2020 at 10:25 AM Maxime Ripard <maxime@xxxxxxxxxx> wrote:
>
> On Tue, Feb 25, 2020 at 09:46:57AM +0100, Mateusz Holenko wrote:
> > From: Pawel Czarnecki <pczarnecki@xxxxxxxxxxxxxxxxxxxxxxxx>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
> >
> > Co-developed-by: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > Signed-off-by: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > Signed-off-by: Pawel Czarnecki <pczarnecki@xxxxxxxxxxxxxxxxxxxxxxxx>
> > ---
> >
> > Notes:
> >     This commit has been introduced in v3 of the patchset.
> >
> >     It includes a simplified version of common 'litex.h'
> >     header introduced in v2 of the patchset.
> >
> >  MAINTAINERS                        |   2 +
> >  drivers/soc/Kconfig                |   1 +
> >  drivers/soc/Makefile               |   1 +
> >  drivers/soc/litex/Kconfig          |  14 ++
> >  drivers/soc/litex/Makefile         |   3 +
> >  drivers/soc/litex/litex_soc_ctrl.c | 233 +++++++++++++++++++++++++++++
> >  include/linux/litex.h              |  45 ++++++
> >  7 files changed, 299 insertions(+)
> >  create mode 100644 drivers/soc/litex/Kconfig
> >  create mode 100644 drivers/soc/litex/Makefile
> >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> >  create mode 100644 include/linux/litex.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index ec925c081dd2..22a67514ace3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9730,6 +9730,8 @@ M:      Karol Gugala <kgugala@xxxxxxxxxxxx>
> >  M:   Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/*/litex,*.yaml
> > +F:   drivers/soc/litex/litex_soc_ctrl.c
> > +F:   include/linux/litex.h
> >
> >  LIVE PATCHING
> >  M:   Josh Poimboeuf <jpoimboe@xxxxxxxxxx>
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 1778f8c62861..78add2a163be 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> >  source "drivers/soc/imx/Kconfig"
> >  source "drivers/soc/ixp4xx/Kconfig"
> > +source "drivers/soc/litex/Kconfig"
> >  source "drivers/soc/mediatek/Kconfig"
> >  source "drivers/soc/qcom/Kconfig"
> >  source "drivers/soc/renesas/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 8b49d782a1ab..fd016b51cddd 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)   += gemini/
> >  obj-$(CONFIG_ARCH_MXC)               += imx/
> >  obj-$(CONFIG_ARCH_IXP4XX)    += ixp4xx/
> >  obj-$(CONFIG_SOC_XWAY)               += lantiq/
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> >  obj-y                                += mediatek/
> >  obj-y                                += amlogic/
> >  obj-y                                += qcom/
> > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > new file mode 100644
> > index 000000000000..22c78cda0b83
> > --- /dev/null
> > +++ b/drivers/soc/litex/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +menu "Enable LiteX SoC Builder specific drivers"
> > +
> > +config LITEX_SOC_CONTROLLER
> > +     tristate "Enable LiteX SoC Controller driver"
> > +     help
> > +     This option enables the SoC Controller Driver which verifies
> > +     LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > +     accessors.
> > +     All drivers that use functions from litex.h must depend on
> > +     LITEX_SOC_CONTROLLER
> > +
> > +endmenu
> > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > new file mode 100644
> > index 000000000000..98ff7325b1c0
> > --- /dev/null
> > +++ b/drivers/soc/litex/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)   += litex_soc_ctrl.o
> > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > new file mode 100644
> > index 000000000000..b810782da3a5
> > --- /dev/null
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -0,0 +1,233 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX SoC Controller Driver
> > + *
> > + * Copyright (C) 2020 Antmicro
> > + *
> > + */
> > +
> > +#include <linux/litex.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/module.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +
> > +/*
> > + * The parameters below are true for LiteX SoC
> > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > + *
> > + * Supporting other configurations will require
> > + * extending the logic in this header.
> > + */
> > +#define LITEX_REG_SIZE             0x4
> > +#define LITEX_SUBREG_SIZE          0x1
> > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > +
> > +static DEFINE_SPINLOCK(csr_lock);
> > +
> > +static inline unsigned long read_pointer_with_barrier(
> > +     const volatile void __iomem *addr)
> > +{
> > +     unsigned long val;
> > +
> > +     __io_br();
> > +     val = *(const volatile unsigned long __force *)addr;
> > +     __io_ar();
> > +     return val;
> > +}
> > +
> > +static inline void write_pointer_with_barrier(
> > +     volatile void __iomem *addr, unsigned long val)
> > +{
> > +     __io_br();
> > +     *(volatile unsigned long __force *)addr = val;
> > +     __io_ar();
> > +}
>
> What's wrong with readl/writel ?
>

readl/writel use a fixed endianness. What I want to do here is
to access bytes in the CPU native ordering - there is a comment
below explaining how CSRs are handled in LiteX.

> > +/*
> > + * LiteX SoC Generator, depending on the configuration,
> > + * can split a single logical CSR (Control & Status Register)
> > + * into a series of consecutive physical registers.
> > + *
> > + * For example, in the configuration with 8-bit CSR Bus,
> > + * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit
> > + * logical CSR will be generated as four 32-bit physical registers,
> > + * each one containing one byte of meaningful data.
> > + *
> > + * For details see: https://github.com/enjoy-digital/litex/issues/314
> > + *
> > + * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
> > + * the logic of writing to/reading from the LiteX CSR in a single
> > + * place that can be then reused by all LiteX drivers.
> > + */
> > +void litex_set_reg(
> > +     void __iomem *reg, unsigned long reg_size, unsigned long val)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             shifted_data = val >> shift;
> > +             write_pointer_with_barrier(
> > +                     reg + (LITEX_REG_SIZE * i), shifted_data);
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +}
> > +
> > +unsigned long litex_get_reg(
> > +     void __iomem *reg, unsigned long reg_size)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long result = 0;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shifted_data = read_pointer_with_barrier(
> > +                     reg + (LITEX_REG_SIZE * i));
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             result |= (shifted_data << shift);
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +
> > +     return result;
> > +}
>
> I'm not sure what's supposed to be in that register, but usually it
> will either be abstracted away through a framework since letting each
> and every driver poke into the same register is not really ideal, or
> if you really have to do it, using a syscon.

Those litex_set_reg/ltex_get_reg are generic functions allowing to access
any CSR generated by LiteX (addressed with reg argument). Each peripheral
has its own set of CSRs and each driver should access only CSRs from
a single peripheral.

The reason for having a common accessor is to have the logic of doing
a single CSR write (implemented internally as many writes) and a single
CSR read (implemented internally as many reads) just in one place
to avoid code duplication.

> > +static int accessors_ok;
> > +
> > +/*
> > + * Check if accessors are safe to be used by other drivers
> > + * returns true if yes - false if not
> > + */
> > +int litex_check_accessors(void)
> > +{
> > +     return accessors_ok;
> > +}
> > +
> > +#define SCRATCH_REG_OFF         0x04
> > +#define SCRATCH_REG_SIZE        4
> > +#define SCRATCH_REG_VALUE       0x12345678
> > +#define SCRATCH_TEST_VALUE      0xdeadbeef
> > +
> > +/*
> > + * Check LiteX CSR read/write access
> > + *
> > + * This function reads and writes a scratch register in order
> > + * to verify if CSR access works.
> > + *
> > + * In case any problems are detected, the driver should panic
> > + * and not set `accessors_ok` flag. As a result no other
> > + * LiteX driver should access CSR bus.
> > + *
> > + * Access to the LiteX CSR is, by design, done in CPU native
> > + * endianness. The driver should not dynamically configure
> > + * access functions when the endianness mismatch is detected.
> > + * Such situation indicates problems in the soft SoC design
> > + * and should be solved at the LiteX generator level,
> > + * not in the software.
>
> I guess that's why you don't use readl/writel. Then
> __raw_readl/__raw_writel?

Do __raw_readl/__raw_writel provide memory barriers? I want to make sure
that CSR's subregisters are read in a specific order.

> > + */
> > +static int litex_check_csr_access(void __iomem *reg_addr)
> > +{
> > +     unsigned long reg;
> > +
> > +     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> > +
> > +     if (reg != SCRATCH_REG_VALUE) {
> > +             panic("Scratch register read error! Expected: 0x%x but got: 0x%lx",
> > +                     SCRATCH_REG_VALUE, reg);
> > +             return -EINVAL;
> > +     }
> > +
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
> > +     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> > +
> > +     if (reg != SCRATCH_TEST_VALUE) {
> > +             panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
> > +                     SCRATCH_TEST_VALUE, reg);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* restore original value of the SCRATCH register */
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
> > +
> > +     /* Set flag for other drivers */
> > +     accessors_ok = 1;
> > +     pr_info("LiteX SoC Controller driver initialized");
> > +
> > +     return 0;
> > +}
> > +
> > +struct litex_soc_ctrl_device {
> > +     void __iomem *base;
> > +};
> > +
> > +static const struct of_device_id litex_soc_ctrl_of_match[] = {
> > +     {.compatible = "litex,soc_controller"},
> > +     {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
> > +
> > +static int litex_soc_ctrl_probe(struct platform_device *pdev)
> > +{
> > +     struct device *dev;
> > +     struct device_node *node;
> > +     const struct of_device_id *id;
> > +     struct litex_soc_ctrl_device *soc_ctrl_dev;
> > +     struct resource *res;
> > +
> > +     dev = &pdev->dev;
> > +     node = dev->of_node;
> > +     if (!node)
> > +             return -ENODEV;
> > +
> > +     id = of_match_node(litex_soc_ctrl_of_match, node);
> > +     if (!id)
> > +             return -ENODEV;
>
> That's pretty much guaranteed.

You are right, I'll remove this fragment.

> > +     soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
> > +     if (IS_ERR_OR_NULL(soc_ctrl_dev))
> > +             return -ENOMEM;
> > +
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     if (IS_ERR_OR_NULL(res))
> > +             return -EBUSY;
> > +
> > +     soc_ctrl_dev->base = devm_of_iomap(dev, node, 0, &res->end);
> > +     if (IS_ERR_OR_NULL(soc_ctrl_dev->base))
> > +             return -EIO;
>
> devm_platform_ioremap_resource ?

I'll simplify this fragment by using devm_platform_ioremap_resource as
suggested.

> > +     return litex_check_csr_access(soc_ctrl_dev->base);
> > +}
> > +
> > +static int litex_soc_ctrl_remove(struct platform_device *pdev)
> > +{
> > +     return 0;
> > +}
>
> You can leave the remove hook out entirely if you don't have anything
> in it.

Right.

> Maxime

Thanks for the comments!

I'll refactor the code to address your suggestions.



--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland