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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> On Thu, Apr 2, 2020 at 8:46 AM 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>
> > ---
> >
> > Notes:
> >     Changes in v4:
> >     - fixed indent in Kconfig's help section
> >     - fixed copyright header
> >     - changed compatible to "litex,soc-controller"
> >     - simplified litex_soc_ctrl_probe
> >     - removed unnecessary litex_soc_ctrl_remove
> >
> >     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 | 217 +++++++++++++++++++++++++++++
> >  include/linux/litex.h              |  45 ++++++
> >  7 files changed, 283 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 2f5ede8a08aa..a35be1be90d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> >  M:     Mateusz Holenko <mholenko@antmicro.com>
> >  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@redhat.com>
> > 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..71264c0e1d6c
> > --- /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..5defba000fd4
> > --- /dev/null
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -0,0 +1,217 @@
> > +// 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
> > + * 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();
> > +}
> > +
> 
> I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> order to make sure that a series of reads/writes to a single CSR
> register will not be reordered by the compiler.

Please do not do this, there are core kernel calls for this, otherwise
this would be required by every individual driver, which would be crazy.

> Does __raw_readl/__raw_writel guarantee this property? If so, I could
> drop my functions and use the system ones instead.

Try it and see.  What's wrong with the normal iomem read/write
functions?

Also, just writing to a pointer like you did above is not how to do
this, please use the normal function calls, that way your driver will
work properly.

thanks,

greg k-h