Date:   Tue, 21 Apr 2020 10:29:33 +0200
From:   Mateusz Holenko <mholenko@...micro.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Jiri Slaby <jslaby@...e.com>, devicetree@...r.kernel.org,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Stafford Horne <shorne@...il.com>,
        Karol Gugala <kgugala@...micro.com>,
        Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "Paul E. McKenney" <paulmck@...ux.ibm.com>,
        Filip Kokosinski <fkokosinski@...micro.com>,
        Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>,
        Joel Stanley <joel@....id.au>,
        Jonathan Cameron <Jonathan.Cameron@...wei.com>,
        Maxime Ripard <mripard@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Heiko Stuebner <heiko@...ech.de>,
        Sam Ravnborg <sam@...nborg.org>,
        Icenowy Zheng <icenowy@...c.io>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 3/5] drivers/soc/litex: add LiteX SoC Controller driver

On Thu, Apr 16, 2020 at 4:18 PM Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> 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@...uxfoundation.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@...micro.com> wrote:
> > > > >
> > > > > From: Pawel Czarnecki <pczarnecki@...ernships.antmicro.com>
> > > > >
> > > > > This commit adds driver for the FPGA-based LiteX SoC
> > > > > Controller from LiteX SoC builder.
> > > > >
> > > > > Co-developed-by: Mateusz Holenko <mholenko@...micro.com>
> > > > > Signed-off-by: Mateusz Holenko <mholenko@...micro.com>
> > > > > Signed-off-by: Pawel Czarnecki <pczarnecki@...ernships.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@...micro.com>
> > > > >  M:     Mateusz Holenko <mholenko@...micro.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@...hat.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.

Sure, no doubts here. What I wanted to say though was that I want to
protect the code against compiler optimizations (code reordering) that
I might not be able to detect using just a one-time test. It has nothing to
do with bugs in common functions, only with the compiler itself (and again
it's not a bug).
The way optimizations are handled is dependent on the compiler
and the configuration and I'm manually testing just one of many possible
setups.

I also checked that there are explicit memory barriers used in the
code of readl(),
so I assume __raw_readl()/__raw_writel() does not guarantee ordering alone
(as otherwise __io_br() wouldn't be used in readl()).

> 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 :)

The only reason I ended up with additional
read_pointer_with_barrier()/write_pointer_with_barrier() is because
I couldn't find an in-kernel function for this:
* ioread32()/readl() modifies endianness,
* __raw_readl() does not provide memory barriers.

I have no intention of duplicating the code just for the sake of
having my own copy
- I know more than well that this leads to problems only ;)

read_pointer_with_barrier()/write_pointer_with_barriers() are not
meant to be used
outside the litex_soc_ctrl.c file (that's why they are static) - they
are merely helper
functions for litex_get_reg()/litex_set_reg().
Since they are called only in one context I can just inline them -
litex_set_reg() will
call __raw_writel() surrounded by memory barriers directly. The same
for litex_get_reg().
Would it be less confusing?

An alternative I see is to call in-kernel ioread32()/ioread32be(), but
that would require ifdefing,
i.e., testing the value of __LITTLE_ENDIAN and calling ioread32() OR
ioread32be().
Do you see other options?

I'm fully open for suggestions.

> thanks,
>
> greg k-h

Thanks again for your time and the comments!

Best,
Mateusz

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

Powered by blists - more mailing lists