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@xxxxxxxxxxxx> 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> > > 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
