On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> 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@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>
> > > ---
> > >
> > > 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@xxxxxxxxxxxx>
> > > M: Mateusz Holenko <mholenko@xxxxxxxxxxxx>
> > > 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@xxxxxxxxxx>
> > > 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?
> What's wrong with the normal iomem read/write
> functions?
What I want to achieve here is to access the register in the CPU
"native" endianness and make sure that the value I see there is the
same as a predefined pattern.
LiteX is a soft SoC generator - it generates the logic of the whole
SoC (CPU+peripherals) in a form that can be later synthesized and
loaded onto the FPGA/turned into an ASIC/etc. Since it generates the
system as a whole, it gives guarantees on how those elements are
interconnected. It can generate CPUs of different architectures (some
of them being little-, other big-endiann) and I want to have a single
driver to target them all.
In this driver I just want to verify that the interconnection between
CPU and the peripheral is ok - I don't want to adjust dynamically
(i.e., translate endianness in case a mismatch is detected). If what I
see in the register is not what I expect it means that there is
something wrong in the design and the generator should be fixed.
I'm not using ioread32/iowrite32 functions as they reorder bytes
depending on the CPU endianness so the returned value might not
reflect the order of bytes read directly from the peripheral. I could
use ifdefs checking the value of __LITTLE_ENDIAN (and that's in fact
was what we started with), but
(a) it was discouraged in the previous round of the review,
(b) it requires more code - checking __LITTLE_ENDIAN and using
ioread32/ioread32be accordingly.
That's why I ended up with raw pointer access.
>
> 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.
Instead of accessing pointer directly I could call __raw_readl/__raw_writel
- is that what you mean?
>
> thanks,
>
> greg k-h
Thank you very much for the comments!
--
Mateusz Holenko
Antmicro Ltd | www.antmicro.com
Roosevelta 22, 60-829 Poznan, Poland
