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 03:50:34PM +0200, Mateusz Holenko wrote:
> On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > 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.
> 
> Since I want to avoid read/write reordering caused by the compiler
> optimizations I don't want to rely on a single manual test.
> What I mean is that even if it works now for me, it does not guarantee
> that it will in the future version of the compiler/using different
> compilation flags/etc, right?

No, if the common functions stop working, then they will be fixed.  If
you try to roll your own and they stop working in the future, no one
will notice.

Please use the common in-kernel functions for this, it's not ok for
drivers to try to do it themselves for basic things like this, no matter
what platform they think they are designed for :)

thanks,

greg k-h