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

["Gabriel L. Somlo" <gsomlo@gmail.com>]

Hi Geert, Mateusz,

On Fri, Sep 25, 2020 at 03:16:02PM +0200, Geert Uytterhoeven wrote:
> Hi Mateusz,
> 
> On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
> >
> > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> 
> Thanks for your patch!
> 
> > --- /dev/null
> > +++ b/drivers/soc/litex/Kconfig
> > @@ -0,0 +1,15 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +menu "Enable LiteX SoC Builder specific drivers"
> > +
> > +config LITEX_SOC_CONTROLLER
> > +       tristate "Enable LiteX SoC Controller driver"
> > +       depends on OF || COMPILE_TEST
> > +       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.
> 
> I'm wondering if it makes sense to have them depend on a "simpler"
> symbol instead, e.g. LITEX?
> 
> Currently the SoC controller is limited to I/O accessors and a simple
> register compatibility check, but you may want to extend it with more
> features later, so you probably want to keep the LITEX_SOC_CONTROLLER.
> Hence you could add
> 
>     config LITEX
>         bool
> 
> and let LITEX_SOC_CONTROLLER select LITEX.
> 
> > --- /dev/null
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -0,0 +1,194 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX SoC Controller Driver
> > + *
> > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > + *
> > + */
> > +
> > +#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
> 
> SoCs
> 
> > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > + *
> > + * Supporting other configurations will require
> > + * extending the logic in this header.
> 
> This is no longer a header file.
> 
> > + */
> > +#define LITEX_REG_SIZE             0x4
> > +#define LITEX_SUBREG_SIZE          0x1
> > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > +
> > +static DEFINE_SPINLOCK(csr_lock);
> > +
> > +/*
> > + * 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/wiki/CSR-Bus
> > + *
> > + * 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;
> > +
> > +               writel((u32 __force)cpu_to_le32(shifted_data), reg + (LITEX_REG_SIZE * i));
> > +       }
> > +
> > +       spin_unlock_irqrestore(&csr_lock, flags);
> > +}
> > +EXPORT_SYMBOL_GPL(litex_set_reg);
> 
> I'm still wondering about the overhead of loops and multiple accesses,
> and the need for them (see also BenH's earlier comment).
> If e.g. the register widths change for LiteUART (currently they're
> hardcoded to one), would you still consider it using the same
> programming interface, and thus compatible with "litex,liteuart"?

There's been talk within the LiteX dev community to standardize on a
LITEX_SUBREG_SIZE of 0x4 (i.e., using all 32 bits of a 32-bit
(LITEX_REG_SIZE) aligned MMIO location). Early 32-bit (vexriscv based)
Linux capable LiteX designs started out with only the 8 LSBits used
within a 32-bit MMIO location, but 64-bit (Rocket chip) based LiteX SoCs
use 4-byte aligned, fully populated MMIO registers (i.e., both
LITEX_SUBREG_SIZE *and* LITEX_REG_SIZE are 4). There's also been talk of
deprecating LITEX_SUBREG_SIZE == 0x1 for "linux-capable LiteX builds",
but nothing definitive yet AFAIK.

As long as adding LITEX_SUBREG_SIZE 0x4 (either as a config option, or
as a hard-coded default in a subsequent version) won't break things, we
should be safe going forward afaict.

Geert: note that LiteX has wider-than-32-bit registers spread across
multiple 32-bit aligned, 8- or 32-bit wide "subregisters", so looping
and shifting will still be necessary, even with LITEX_SUBREG_SIZE 0x4.

> The spinlock access will probably become the source of lock contention
> later, especially when considering SMP variants.
> 
> > +/*
> > + * 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.
> > + *
> > + * 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.
> > + */
> > +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);
> 
> Do you think the user will ever see this panic message? (see below)
> 
> > +               return -EINVAL;
> 
> Good ;-)  All of BUG()/WARN()/panic() may be compiled out, depending on
> config options, so the system may continue running beyond the panic()
> call.
> 
> > +static int litex_soc_ctrl_probe(struct platform_device *pdev)
> > +{
> > +       int result;
> > +       struct device *dev;
> > +       struct device_node *node;
> > +       struct litex_soc_ctrl_device *soc_ctrl_dev;
> > +
> > +       dev = &pdev->dev;
> > +       node = dev->of_node;
> > +       if (!node)
> > +               return -ENODEV;
> 
> FYI, this cannot happen.
> 
> > +
> > +       soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
> > +       if (!soc_ctrl_dev)
> > +               return -ENOMEM;
> > +
> > +       soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> > +       if (IS_ERR(soc_ctrl_dev->base))
> > +               return PTR_ERR(soc_ctrl_dev->base);
> > +
> > +       result = litex_check_csr_access(soc_ctrl_dev->base);
> > +       if (result) {
> > +               /* LiteX CSRs access is broken which means that
> > +                * none of LiteX drivers will most probably
> > +                * operate correctly
> > +                */
> > +               WARN(1, "Failed to validate CSR registers, the system is probably broken.\n");
> 
> WARN(result, ...)
> 
> But is this WARN() needed? You have already called panic() before.
> 
> > +       }
> > +
> > +       return result;
> > +}
> > +
> > +static struct platform_driver litex_soc_ctrl_driver = {
> > +       .driver = {
> > +               .name = "litex-soc-controller",
> > +               .of_match_table = of_match_ptr(litex_soc_ctrl_of_match)
> > +       },
> > +       .probe = litex_soc_ctrl_probe,
> > +};
> > +
> > +module_platform_driver(litex_soc_ctrl_driver);
> 
> module_platform_driver() means this driver is probed quite late in the
> boot sequence.  Currently the only other LiteX driver is liteuart, which
> is probed at more or less the same time, but I can envision more early
> drivers to be added later (typically interrupt/clock controllers and
> timers not integrated into the main CPU core).
> Note that even liteuart will run earlier, and thus access CSR registers
> before the check has run, when using e.g. earlycon...
> 
> > --- /dev/null
> > +++ b/include/linux/litex.h
> > @@ -0,0 +1,24 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Common LiteX header providing
> > + * helper functions for accessing CSRs.
> > + *
> > + * Implementation of the functions is provided by
> > + * the LiteX SoC Controller driver.
> > + *
> > + * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
> > + */
> > +
> > +#ifndef _LINUX_LITEX_H
> > +#define _LINUX_LITEX_H
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
> > +#include <linux/compiler_types.h>
> > +
> > +void litex_set_reg(void __iomem *reg, unsigned long reg_sz, unsigned long val);
> > +
> > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_sz);
> 
> Perhaps you can add static inline litex_{read,write}{8,16,32}() wrappers,
> so drivers don't have to pass the reg_sz parameter explicitly,
> and to make it look more like accessors of other bus types?

Seconded -- perhaps simply cut'n'paste and/or adapt from
https://github.com/litex-hub/linux/blob/litex-rocket-rebase/include/linux/litex.h#L78
(from the 64-bit port of the LiteX linux patch set)
 
Cheers,
--Gabriel