[PATCH v11 0/5] LiteX SoC controller and LiteUART serial driver

[PATCH v11 0/5] LiteX SoC controller and LiteUART serial driver

[Mateusz Holenko]

This patchset introduces support for LiteX SoC Controller
and LiteUART - serial device from LiteX SoC builder
(https://github.com/enjoy-digital/litex).

In the following patchset I will add
a new mor1kx-based (OpenRISC) platform that
uses this device.

Later I plan to extend this platform by
adding support for more devices from LiteX suite.

Changes in v11:
    - added Reviewed-by tag
    - reformatted some comments
    - switched to WARN instead of BUG on CSR validation fail

Changes in v10:
    - added casting to avoid sparse warnings in the SoC Controller's driver

Changes in v9:
    - fixed the `reg` node notation in the DT example
    - added exporting of the `litex_set_reg`/`litex_get_reg` symbols

Changes in v8:
    - fixed help messages in LiteUART's KConfig
    - removed dependency between LiteUART and LiteX SoC drivers
    - removed `litex_check_accessors()` helper function
    - added crashing (BUG) on the failed LiteX CSR access test

Changes in v7:
    - added missing include directive in UART's driver

Changes in v6:
    - changed accessors in SoC Controller's driver
    - reworked UART driver

Changes in v5:
    - added Reviewed-by tag
    - removed custom accessors from SoC Controller's driver
    - fixed error checking in SoC Controller's driver

Changes in v4:
    - fixed copyright headers
    - fixed SoC Controller's yaml 
    - simplified SoC Controller's driver

Changes in v3:
    - added Acked-by and Reviewed-by tags
    - introduced LiteX SoC Controller driver
    - removed endianness detection (handled now by LiteX SoC Controller driver)
    - modified litex.h header
    - DTS aliases for LiteUART made optional
    - renamed SERIAL_LITEUART_NR_PORTS to SERIAL_LITEUART_MAX_PORTS
    - changed PORT_LITEUART from 122 to 123

Changes in v2:
    - binding description rewritten to a yaml schema file
    - added litex.h header with common register access functions

Filip Kokosinski (3):
  dt-bindings: vendor: add vendor prefix for LiteX
  dt-bindings: serial: document LiteUART bindings
  drivers/tty/serial: add LiteUART driver

Pawel Czarnecki (2):
  dt-bindings: soc: document LiteX SoC Controller bindings
  drivers/soc/litex: add LiteX SoC Controller driver

 .../bindings/serial/litex,liteuart.yaml       |  38 ++
 .../soc/litex/litex,soc-controller.yaml       |  39 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 MAINTAINERS                                   |   9 +
 drivers/soc/Kconfig                           |   1 +
 drivers/soc/Makefile                          |   1 +
 drivers/soc/litex/Kconfig                     |  15 +
 drivers/soc/litex/Makefile                    |   3 +
 drivers/soc/litex/litex_soc_ctrl.c            | 194 +++++++++
 drivers/tty/serial/Kconfig                    |  32 ++
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/liteuart.c                 | 402 ++++++++++++++++++
 include/linux/litex.h                         |  24 ++
 13 files changed, 761 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/litex,liteuart.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
 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 drivers/tty/serial/liteuart.c
 create mode 100644 include/linux/litex.h

-- 
2.25.1

[PATCH v11 1/5] dt-bindings: vendor: add vendor prefix for LiteX

[Mateusz Holenko]

From: Filip Kokosinski <fkokosinski@antmicro.com>

Add vendor prefix for LiteX SoC builder.

Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Acked-by: Rob Herring <robh@kernel.org>
---

Notes:
    No changes in v11.

    No changes in v10.

    No changes in v9.

    No changes in v8.

    No changes in v7.

    No changes in v6.

    No changes in v5.

    No changes in v4.

    Changes in v3:
    - added Acked-by tag
    
    No changes in v2.

 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index d3891386d671..9aae6c56d7a3 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -571,6 +571,8 @@ patternProperties:
     description: Linux-specific binding
   "^linx,.*":
     description: Linx Technologies
+  "^litex,.*":
+    description: LiteX SoC builder
   "^lltc,.*":
     description: Linear Technology Corporation
   "^logicpd,.*":
-- 
2.25.1

[PATCH v11 2/5] dt-bindings: soc: document LiteX SoC Controller bindings

[Mateusz Holenko]

From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>

Add documentation for LiteX SoC Controller bindings.

Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Notes:
    No changes in v11.

    No changes in v10.

    Changes in v9:
        - fixed the `reg` node notation in the DT example

    No changes in v8.

    No changes in v7.

    Changes in v6:
        - fixed license header

    Changes in v5:
        - added reviewed-by tag
    
    Changes in v4:
        - changes compatible to "litex,soc-controller"
        - fixed yaml's header
        - removed unnecessary sections from yaml
        - fixed indentation in yaml
    
    This commit has been introduced in v3 of the patchset.

 .../soc/litex/litex,soc-controller.yaml       | 39 +++++++++++++++++++
 MAINTAINERS                                   |  6 +++
 2 files changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml

diff --git a/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml b/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
new file mode 100644
index 000000000000..53121c1fbe4d
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+# Copyright 2020 Antmicro <www.antmicro.com>
+%YAML 1.2
+---
+$id: "http://devicetree.org/schemas/soc/litex/litex,soc-controller.yaml#"
+$schema: "http://devicetree.org/meta-schemas/core.yaml#"
+
+title: LiteX SoC Controller driver
+
+description: |
+  This is the SoC Controller driver for the LiteX SoC Builder.
+  It's purpose is to verify LiteX CSR (Control&Status Register) access
+  operations and provide function for other drivers to read/write CSRs
+  and to check if those accessors are ready to use.
+
+maintainers:
+  - Karol Gugala <kgugala@antmicro.com>
+  - Mateusz Holenko <mholenko@antmicro.com>
+
+properties:
+  compatible:
+    const: litex,soc-controller
+
+  reg:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    soc_ctrl0: soc-controller@f0000000 {
+        compatible = "litex,soc-controller";
+        reg = <0xf0000000 0xC>;
+        status = "okay";
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b58ca29cc80..39be98db7418 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9835,6 +9835,12 @@ L:	kunit-dev@googlegroups.com
 S:	Maintained
 F:	lib/list-test.c
 
+LITEX PLATFORM
+M:	Karol Gugala <kgugala@antmicro.com>
+M:	Mateusz Holenko <mholenko@antmicro.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/*/litex,*.yaml
+
 LIVE PATCHING
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
 M:	Jiri Kosina <jikos@kernel.org>
-- 
2.25.1

Re: [PATCH v11 2/5] dt-bindings: soc: document LiteX SoC Controller bindings

[Geert Uytterhoeven]

Hi Mateusz,

On Wed, Sep 23, 2020 at 12:09 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
>
> Add documentation for LiteX SoC Controller bindings.
>
> Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
> @@ -0,0 +1,39 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +# Copyright 2020 Antmicro <www.antmicro.com>
> +%YAML 1.2
> +---
> +$id: "http://devicetree.org/schemas/soc/litex/litex,soc-controller.yaml#"
> +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> +
> +title: LiteX SoC Controller driver
> +
> +description: |
> +  This is the SoC Controller driver for the LiteX SoC Builder.
> +  It's purpose is to verify LiteX CSR (Control&Status Register) access

Its

> +  operations and provide function for other drivers to read/write CSRs

functions

> +  and to check if those accessors are ready to use.

be used

> +
> +maintainers:
> +  - Karol Gugala <kgugala@antmicro.com>
> +  - Mateusz Holenko <mholenko@antmicro.com>
> +
> +properties:
> +  compatible:
> +    const: litex,soc-controller
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    soc_ctrl0: soc-controller@f0000000 {
> +        compatible = "litex,soc-controller";
> +        reg = <0xf0000000 0xC>;

Please be consistent w.r.t. lower/upper case: "0xc".

> +        status = "okay";
> +    };

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v11 2/5] dt-bindings: soc: document LiteX SoC Controller bindings

[Mateusz Holenko]

Hi Geert,

On Fri, Sep 25, 2020 at 2:48 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Mateusz,
>
> On Wed, Sep 23, 2020 at 12:09 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> >
> > Add documentation for LiteX SoC Controller bindings.
> >
> > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> Thanks for your patch!

Thanks for your comments!

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
> > @@ -0,0 +1,39 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +# Copyright 2020 Antmicro <www.antmicro.com>
> > +%YAML 1.2
> > +---
> > +$id: "http://devicetree.org/schemas/soc/litex/litex,soc-controller.yaml#"
> > +$schema: "http://devicetree.org/meta-schemas/core.yaml#"
> > +
> > +title: LiteX SoC Controller driver
> > +
> > +description: |
> > +  This is the SoC Controller driver for the LiteX SoC Builder.
> > +  It's purpose is to verify LiteX CSR (Control&Status Register) access
>
> Its
>
> > +  operations and provide function for other drivers to read/write CSRs
>
> functions
>
> > +  and to check if those accessors are ready to use.
>
> be used

I will fix the description as suggested.

> > +
> > +maintainers:
> > +  - Karol Gugala <kgugala@antmicro.com>
> > +  - Mateusz Holenko <mholenko@antmicro.com>
> > +
> > +properties:
> > +  compatible:
> > +    const: litex,soc-controller
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    soc_ctrl0: soc-controller@f0000000 {
> > +        compatible = "litex,soc-controller";
> > +        reg = <0xf0000000 0xC>;
>
> Please be consistent w.r.t. lower/upper case: "0xc".

Sure, I will use lowercase everywhere.

> > +        status = "okay";
> > +    };
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Best regards,
Mateusz


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

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

[Mateusz Holenko]

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 v11:
    - removed an unnecessary comment left over from previous version
    - changed a multi-line comment to comply with the formatting rules
    - use WARN instad of BUG on a failed CSR validation

    Changes in v10:
    - added casting to avoid sparse warnings in the SoC Controller's driver
    
    Changes in v9:
    - added exporting of the `litex_set_reg`/`litex_get_reg` symbols

    Changes in v8:
    - removed `litex_check_accessors()` helper function
    - added crashing (BUG) on the failed LiteX CSR access test

    No changes in v7.

    Changes in v6:
    - added dependency on OF || COMPILE_TEST
    - used le32_to_cpu(readl(addr)) instead of __raw_readl
      and writel(cpu_to_le32(value), addr) instead of __raw_writel
      to take advantage of memory barriers provided by readl/writel

    Changes in v5:
    - removed helper accessors and used __raw_readl/__raw_writel instead
    - fixed checking for errors in litex_soc_ctrl_probe

    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          |  15 +++
 drivers/soc/litex/Makefile         |   3 +
 drivers/soc/litex/litex_soc_ctrl.c | 194 +++++++++++++++++++++++++++++
 include/linux/litex.h              |  24 ++++
 7 files changed, 240 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 39be98db7418..4d70a1b22a87 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9840,6 +9840,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 425ab6f7e375..d097d070f579 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 36452bed86ef..0b16108823ef 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
 obj-y				+= 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..c974ec3846bc
--- /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.
+
+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..08330c9872b0
--- /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
+ * 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);
+
+/*
+ * 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);
+
+unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
+{
+	unsigned long shifted_data, shift, i;
+	unsigned long result = 0;
+	unsigned long flags;
+
+	spin_lock_irqsave(&csr_lock, flags);
+
+	for (i = 0; i < reg_size; ++i) {
+		shifted_data = le32_to_cpu((__le32 __force)readl(reg + (LITEX_REG_SIZE * i)));
+
+		shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
+		result |= (shifted_data << shift);
+	}
+
+	spin_unlock_irqrestore(&csr_lock, flags);
+
+	return result;
+}
+EXPORT_SYMBOL_GPL(litex_get_reg);
+
+#define SCRATCH_REG_OFF         0x04
+#define SCRATCH_REG_SIZE        4
+#define SCRATCH_REG_VALUE       0x12345678
+#define SCRATCH_TEST_VALUE      0xdeadbeef
+
+/*
+ * 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);
+		return -EINVAL;
+	}
+
+	litex_set_reg(reg_addr + SCRATCH_REG_OFF,
+		SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
+	reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
+
+	if (reg != SCRATCH_TEST_VALUE) {
+		panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
+			SCRATCH_TEST_VALUE, reg);
+		return -EINVAL;
+	}
+
+	/* restore original value of the SCRATCH register */
+	litex_set_reg(reg_addr + SCRATCH_REG_OFF,
+		SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
+
+	pr_info("LiteX SoC Controller driver initialized");
+
+	return 0;
+}
+
+struct litex_soc_ctrl_device {
+	void __iomem *base;
+};
+
+static const struct of_device_id litex_soc_ctrl_of_match[] = {
+	{.compatible = "litex,soc-controller"},
+	{},
+};
+
+MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
+
+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;
+
+	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");
+	}
+
+	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_DESCRIPTION("LiteX SoC Controller driver");
+MODULE_AUTHOR("Antmicro <www.antmicro.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/litex.h b/include/linux/litex.h
new file mode 100644
index 000000000000..72061018c172
--- /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);
+
+
+#endif /* _LINUX_LITEX_H */
-- 
2.25.1

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

[Jonathan Cameron]

On Wed, 23 Sep 2020 12:09:06 +0200
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>

A few little things inline, but looks fine in general to me.

Main request is for more formal documentation of parameters to the read
and write functions.

Jonathan
 
> ---
> 
> Notes:
>     Changes in v11:
>     - removed an unnecessary comment left over from previous version
>     - changed a multi-line comment to comply with the formatting rules
>     - use WARN instad of BUG on a failed CSR validation
> 
>     Changes in v10:
>     - added casting to avoid sparse warnings in the SoC Controller's driver
>     
>     Changes in v9:
>     - added exporting of the `litex_set_reg`/`litex_get_reg` symbols
> 
>     Changes in v8:
>     - removed `litex_check_accessors()` helper function
>     - added crashing (BUG) on the failed LiteX CSR access test
> 
>     No changes in v7.
> 
>     Changes in v6:
>     - added dependency on OF || COMPILE_TEST
>     - used le32_to_cpu(readl(addr)) instead of __raw_readl
>       and writel(cpu_to_le32(value), addr) instead of __raw_writel
>       to take advantage of memory barriers provided by readl/writel
> 
>     Changes in v5:
>     - removed helper accessors and used __raw_readl/__raw_writel instead
>     - fixed checking for errors in litex_soc_ctrl_probe
> 
>     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          |  15 +++
>  drivers/soc/litex/Makefile         |   3 +
>  drivers/soc/litex/litex_soc_ctrl.c | 194 +++++++++++++++++++++++++++++
>  include/linux/litex.h              |  24 ++++
>  7 files changed, 240 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 39be98db7418..4d70a1b22a87 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9840,6 +9840,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 425ab6f7e375..d097d070f579 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 36452bed86ef..0b16108823ef 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
>  obj-y				+= 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..c974ec3846bc
> --- /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.
> +
> +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..08330c9872b0
> --- /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>

I'm not seeing anything from this header in use in here yet.

> +#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);
> +
> +/*
> + * 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.
> + */

I would argue in favor of formal kernel-doc for these.
Even with this explanation the exact meaning of parameters
isn't particularly clear.

> +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);
> +
> +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> +{
> +	unsigned long shifted_data, shift, i;
> +	unsigned long result = 0;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&csr_lock, flags);
> +
> +	for (i = 0; i < reg_size; ++i) {
> +		shifted_data = le32_to_cpu((__le32 __force)readl(reg + (LITEX_REG_SIZE * i)));
> +
> +		shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> +		result |= (shifted_data << shift);
> +	}
> +
> +	spin_unlock_irqrestore(&csr_lock, flags);
> +
> +	return result;
> +}
> +EXPORT_SYMBOL_GPL(litex_get_reg);
> +
> +#define SCRATCH_REG_OFF         0x04
> +#define SCRATCH_REG_SIZE        4
> +#define SCRATCH_REG_VALUE       0x12345678
> +#define SCRATCH_TEST_VALUE      0xdeadbeef
> +
> +/*
> + * 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);
> +		return -EINVAL;
> +	}
> +
> +	litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> +		SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
> +	reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> +
> +	if (reg != SCRATCH_TEST_VALUE) {
> +		panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
> +			SCRATCH_TEST_VALUE, reg);
> +		return -EINVAL;
> +	}
> +
> +	/* restore original value of the SCRATCH register */
> +	litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> +		SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
> +
> +	pr_info("LiteX SoC Controller driver initialized");
> +
> +	return 0;
> +}
> +
> +struct litex_soc_ctrl_device {
> +	void __iomem *base;
> +};
> +
> +static const struct of_device_id litex_soc_ctrl_of_match[] = {
> +	{.compatible = "litex,soc-controller"},
> +	{},
> +};
> +
> +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
> +
> +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;
> +
> +	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");
> +	}
> +
> +	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_DESCRIPTION("LiteX SoC Controller driver");
> +MODULE_AUTHOR("Antmicro <www.antmicro.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/litex.h b/include/linux/litex.h
> new file mode 100644
> index 000000000000..72061018c172
> --- /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);
> +

Nitpick. One blank line is almost always enough!

> +
> +#endif /* _LINUX_LITEX_H */

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

[Mateusz Holenko]

Hi Jonathan,

thanks for your review!

On Wed, Sep 23, 2020 at 1:58 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 23 Sep 2020 12:09:06 +0200
> 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>
>
> A few little things inline, but looks fine in general to me.
>
> Main request is for more formal documentation of parameters to the read
> and write functions.

You are right, the parameters should be better documented - I'll take
a look at that.
There are some other comments that suggest to replace the loop-based
generic functions
with a bunch of separate accessors - perhaps going this way would also
make it easier to understand
the code.

> Jonathan
>
> > ---
> >
> > Notes:
> >     Changes in v11:
> >     - removed an unnecessary comment left over from previous version
> >     - changed a multi-line comment to comply with the formatting rules
> >     - use WARN instad of BUG on a failed CSR validation
> >
> >     Changes in v10:
> >     - added casting to avoid sparse warnings in the SoC Controller's driver
> >
> >     Changes in v9:
> >     - added exporting of the `litex_set_reg`/`litex_get_reg` symbols
> >
> >     Changes in v8:
> >     - removed `litex_check_accessors()` helper function
> >     - added crashing (BUG) on the failed LiteX CSR access test
> >
> >     No changes in v7.
> >
> >     Changes in v6:
> >     - added dependency on OF || COMPILE_TEST
> >     - used le32_to_cpu(readl(addr)) instead of __raw_readl
> >       and writel(cpu_to_le32(value), addr) instead of __raw_writel
> >       to take advantage of memory barriers provided by readl/writel
> >
> >     Changes in v5:
> >     - removed helper accessors and used __raw_readl/__raw_writel instead
> >     - fixed checking for errors in litex_soc_ctrl_probe
> >
> >     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          |  15 +++
> >  drivers/soc/litex/Makefile         |   3 +
> >  drivers/soc/litex/litex_soc_ctrl.c | 194 +++++++++++++++++++++++++++++
> >  include/linux/litex.h              |  24 ++++
> >  7 files changed, 240 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 39be98db7418..4d70a1b22a87 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9840,6 +9840,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 425ab6f7e375..d097d070f579 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 36452bed86ef..0b16108823ef 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)   += gemini/
> >  obj-y                                += 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..c974ec3846bc
> > --- /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.
> > +
> > +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..08330c9872b0
> > --- /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>
>
> I'm not seeing anything from this header in use in here yet.

I'll check and remove if this is not needed.

> > +#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);
> > +
> > +/*
> > + * 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.
> > + */
>
> I would argue in favor of formal kernel-doc for these.
> Even with this explanation the exact meaning of parameters
> isn't particularly clear.

As stated above, I'll make it clearer - either by adding a formal doc
or by splitting this function into several easier-to-understand ones.

> > +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);
> > +
> > +unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
> > +{
> > +     unsigned long shifted_data, shift, i;
> > +     unsigned long result = 0;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&csr_lock, flags);
> > +
> > +     for (i = 0; i < reg_size; ++i) {
> > +             shifted_data = le32_to_cpu((__le32 __force)readl(reg + (LITEX_REG_SIZE * i)));
> > +
> > +             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
> > +             result |= (shifted_data << shift);
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +
> > +     return result;
> > +}
> > +EXPORT_SYMBOL_GPL(litex_get_reg);
> > +
> > +#define SCRATCH_REG_OFF         0x04
> > +#define SCRATCH_REG_SIZE        4
> > +#define SCRATCH_REG_VALUE       0x12345678
> > +#define SCRATCH_TEST_VALUE      0xdeadbeef
> > +
> > +/*
> > + * 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);
> > +             return -EINVAL;
> > +     }
> > +
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
> > +     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
> > +
> > +     if (reg != SCRATCH_TEST_VALUE) {
> > +             panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
> > +                     SCRATCH_TEST_VALUE, reg);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* restore original value of the SCRATCH register */
> > +     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
> > +             SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
> > +
> > +     pr_info("LiteX SoC Controller driver initialized");
> > +
> > +     return 0;
> > +}
> > +
> > +struct litex_soc_ctrl_device {
> > +     void __iomem *base;
> > +};
> > +
> > +static const struct of_device_id litex_soc_ctrl_of_match[] = {
> > +     {.compatible = "litex,soc-controller"},
> > +     {},
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
> > +
> > +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;
> > +
> > +     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");
> > +     }
> > +
> > +     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_DESCRIPTION("LiteX SoC Controller driver");
> > +MODULE_AUTHOR("Antmicro <www.antmicro.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/include/linux/litex.h b/include/linux/litex.h
> > new file mode 100644
> > index 000000000000..72061018c172
> > --- /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);
> > +
>
> Nitpick. One blank line is almost always enough!

Right, one blank line ought to be enough for anyone.

> > +
> > +#endif /* _LINUX_LITEX_H */
>
>

Best regards,
Mateusz


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

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

[Geert Uytterhoeven]

Hi Mateusz,

On Wed, Sep 23, 2020 at 12:10 PM 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>

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"?

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?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Gabriel L. Somlo]

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

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

[Geert Uytterhoeven]

Hi Gabriel,

On Fri, Sep 25, 2020 at 5:06 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote:
> On Fri, Sep 25, 2020 at 03:16:02PM +0200, Geert Uytterhoeven wrote:
> > On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > + */
> > > +#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.

That sounds like a good idea to me.
Having 8-bit accesses may be worthwhile on a small microcontroller, but a
full-fledge Linux system can use more and wider MMIO.

> 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.

Can these be different than 64-bit (and 128-bit)?
That's not unlike accessors on other 32-bit platforms.
Still, no loop needed, just doing two (or four) 32-bit accesses in a row
is fine (but requires using inlines instead of your current single
out-of-line function).

> > > --- /dev/null
> > > +++ b/include/linux/litex.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)

Yes, you definitely want the 32-bit and 64-bit ports to agree ;-)
Note that these are using the "old" "bwlq" convention (with "l"
predating 64-bit long on 64-bit platforms) instead of the more modern
explicit {8,16,32,64}, but that's a minor detail.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Mateusz Holenko]

Hi Gabriel,

On Fri, Sep 25, 2020 at 5:06 PM Gabriel L. Somlo <gsomlo@gmail.com> wrote:
>
> 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@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>
> >
> > 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.

I agree that having LITEX_SUBREG_SIZE equal to 4 would make the code
much simpler.

I believe, however, that at the moment LiteX (for 32-bit CPUs) and
both main 32-bit Linux capable LiteX-based platforms
(https://github.com/litex-hub/linux-on-litex-vexriscv and
https://github.com/timvideos/litex-buildenv) by default generate SoC
with 8-bit data width and that's why the driver currently targets this
configuration.

> 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.

I'm totally open for extending/changing the default LITEX_SUBREG_SIZE
value for this driver once the default configuration for LiteX
platforms changes.

> 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.

There are also situations (like the GPIO controller) where the layout
of registers is dynamic and depends on the LiteX configuration (number
of supported pins).
In this situation we need to read the configuration from DT and keep
the driver flexible by using dynamic, loop-based CSR accessors.

That's why I believe we could have both - fast, non-loop based
accessors for common registers widths 8/32/64/etc. and the current
implementation where runtime flexibility is required (at the cost of
performance).

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

Best regards,
Mateusz

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

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

[Mateusz Holenko]

Hi Geert,

On Fri, Sep 25, 2020 at 3:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Mateusz,
>
> On Wed, Sep 23, 2020 at 12:10 PM 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>
>
> Thanks for your patch!

Thanks for your review!

> > --- /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.

But then if other drivers depend just on LITEX, it would not automatically
mean that the LITEX_SOC_CONTROLLER is selected, right?. And if it's not selected
litex_{g,s}et_reg() are not available and the compilation would fail.

I could move the implementation of those functions directly to the
litex.h header
and avoid this KConfig dependency, but I'm not sure if they are not
too big to become a static inline.
What do you think?

> > --- /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

Right, I will fix that.

> > + * 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.

It's not - you are correct. I will rephrase it to "extending the logic
in this 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"?

Since the amount of possible `reg_size` is practically limited we could
add explicit 8/32/64/128 accessors to eliminate loops.
As for multiple writel/readl I don't really see an option to avoid
them for the 8-bit bus width.

> The spinlock access will probably become the source of lock contention
> later, especially when considering SMP variants.

Do you have any suggestions on how to handle this?
Dropping locks could lead to the situation when two cores write at the
same time leaving a wrong (mixed) value in the CSR.

> > +/*
> > + * 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)

On UART most probably not, as broken CSRs mean broken UART driver as well.
But I believe you can retrieve logs from the memory and analyze them
post-mortem, isn't that right?

> > +               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.

Right, this check is not necessary - I will remove it.

> > +
> > +       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.

You are right - in both cases (read/write) we call panic before
returning a non-0 value from litex_check_csr_access().
In this context additional WARN is probably not necessary. I'll extend
the panic message to indicate that read/write failure means a broken
system and drop this WARN.

> > +       }
> > +
> > +       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...

Yes, this is an interesting point.
We have already covered it in some previous version of the patchset by testing
if the LiteX SoC Controller driver was probed and returning -EDEFER in
the UART driver otherwise
(to make sure CSR access is verified before UART uses it).
With this approach, however, all future LiteX drivers would have to
follow the pattern and we would require the LiteX SoC Controller
to be part of the system (which is not the case for the Microwatt
platform for example - it uses just an eth core from LiteX).

That's why we finally decided to go with the relaxed version - where
we allow the SoC controller driver not to be the first LiteX driver to
be probed (and possibly detect a problem after other drivers encounter
it).

> > --- /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?

Yes, this is definitely doable.

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Best regards,
Mateusz

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

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

[Geert Uytterhoeven]

Hi Mateusz,

On Tue, Oct 6, 2020 at 10:02 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> On Fri, Sep 25, 2020 at 3:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Sep 23, 2020 at 12:10 PM 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>

> > > --- /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.
>
> But then if other drivers depend just on LITEX, it would not automatically
> mean that the LITEX_SOC_CONTROLLER is selected, right?. And if it's not selected
> litex_{g,s}et_reg() are not available and the compilation would fail.

As the LITEX config symbol above uses plain "bool", without a
description, it is invisible.  Hence it cannot be enabled by the user,
only be selected by other symbols.
If LITEX_SOC_CONTROLLER is the only symbol selecting LITEX, the
dependency is met.

> I could move the implementation of those functions directly to the
> litex.h header
> and avoid this KConfig dependency, but I'm not sure if they are not
> too big to become a static inline.
> What do you think?

With the spinlock and the loop, they're too large to be inlined, IMHO.

> > > --- /dev/null
> > > +++ b/drivers/soc/litex/litex_soc_ctrl.c

> > > + */
> > > +#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"?
>
> Since the amount of possible `reg_size` is practically limited we could
> add explicit 8/32/64/128 accessors to eliminate loops.

Good.

(assuming 32-bit physical reg accesses below)

> As for multiple writel/readl I don't really see an option to avoid
> them for the 8-bit bus width.

Sure, 64-bit register accesses consist of two 32-bit accesses on other
32-bit platforms, too.

> > The spinlock access will probably become the source of lock contention
> > later, especially when considering SMP variants.
>
> Do you have any suggestions on how to handle this?
> Dropping locks could lead to the situation when two cores write at the
> same time leaving a wrong (mixed) value in the CSR.

Is this due to the CSR bus or due to the CSR register?
I mean can two 64-bit accesses to different CSR registers be done as
four interleaved 32-bit accesses, or must they not be interleaved?

If (hopefully) they can be interleaved, you just need serialization of
accesses to the same 64-bit register.  As the same register is usually
not accessed from multiple drivers, you can handle the serialization
inside the driver, if it can ever happen at all (e.g. main driver
operation and interrupt handler accessing the same register).
That avoids the need for the spinlock in the generic register accessors.

If they must not be interleaved, you indeed need serialization at the
bus level, but only for the 64-bit accesses?  And I would strongly
suggest to look into changing the CSR bus behavior at the hardware
level, if possible...

> > > +/*
> > > + * 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)
>
> On UART most probably not, as broken CSRs mean broken UART driver as well.
> But I believe you can retrieve logs from the memory and analyze them
> post-mortem, isn't that right?

Sure. Been there, done that ;-)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Mateusz Holenko]

On Tue, Oct 6, 2020 at 10:38 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Mateusz,
>
> On Tue, Oct 6, 2020 at 10:02 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > On Fri, Sep 25, 2020 at 3:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Wed, Sep 23, 2020 at 12:10 PM 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>
>
> > > > --- /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.
> >
> > But then if other drivers depend just on LITEX, it would not automatically
> > mean that the LITEX_SOC_CONTROLLER is selected, right?. And if it's not selected
> > litex_{g,s}et_reg() are not available and the compilation would fail.
>
> As the LITEX config symbol above uses plain "bool", without a
> description, it is invisible.  Hence it cannot be enabled by the user,
> only be selected by other symbols.
> If LITEX_SOC_CONTROLLER is the only symbol selecting LITEX, the
> dependency is met.
>
> > I could move the implementation of those functions directly to the
> > litex.h header
> > and avoid this KConfig dependency, but I'm not sure if they are not
> > too big to become a static inline.
> > What do you think?
>
> With the spinlock and the loop, they're too large to be inlined, IMHO.

Sure.

> > > > --- /dev/null
> > > > +++ b/drivers/soc/litex/litex_soc_ctrl.c
>
> > > > + */
> > > > +#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"?
> >
> > Since the amount of possible `reg_size` is practically limited we could
> > add explicit 8/32/64/128 accessors to eliminate loops.
>
> Good.
>
> (assuming 32-bit physical reg accesses below)
>
> > As for multiple writel/readl I don't really see an option to avoid
> > them for the 8-bit bus width.
>
> Sure, 64-bit register accesses consist of two 32-bit accesses on other
> 32-bit platforms, too.
>
> > > The spinlock access will probably become the source of lock contention
> > > later, especially when considering SMP variants.
> >
> > Do you have any suggestions on how to handle this?
> > Dropping locks could lead to the situation when two cores write at the
> > same time leaving a wrong (mixed) value in the CSR.
>
> Is this due to the CSR bus or due to the CSR register?
> I mean can two 64-bit accesses to different CSR registers be done as
> four interleaved 32-bit accesses, or must they not be interleaved?
>
> If (hopefully) they can be interleaved, you just need serialization of
> accesses to the same 64-bit register.  As the same register is usually
> not accessed from multiple drivers, you can handle the serialization
> inside the driver, if it can ever happen at all (e.g. main driver
> operation and interrupt handler accessing the same register).
> That avoids the need for the spinlock in the generic register accessors.
> If they must not be interleaved, you indeed need serialization at the
> bus level, but only for the 64-bit accesses?  And I would strongly
> suggest to look into changing the CSR bus behavior at the hardware
> level, if possible...

Good point. I was thinking about the single CSR register consistency,
not the whole bus.

Changing the CSR behaviour at the LiteX level is something that is
currently discussed by the developers, but I'm not aware of any
timeline.
I believe we can always simplify the driver's code in the future, once
the HW becomes better suited for Linux support in the default
configuration.

> > > > +/*
> > > > + * 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)
> >
> > On UART most probably not, as broken CSRs mean broken UART driver as well.
> > But I believe you can retrieve logs from the memory and analyze them
> > post-mortem, isn't that right?
>
> Sure. Been there, done that ;-)
>
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Best,
Mateusz

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

[PATCH v11 4/5] dt-bindings: serial: document LiteUART bindings

[Mateusz Holenko]

From: Filip Kokosinski <fkokosinski@antmicro.com>

Add documentation for LiteUART devicetree bindings.

Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Notes:
    No changes in v11.

    No changes in v10.

    No changes in v9.

    No changes in v8.

    No changes in v7.

    Changes in v6:
    - fixed license header

    No changes in v5.

    No changes in v4.

    Changes in v3:
    - added Reviewed-by tag
    - patch number changed from 3 to 4
    - removed changes in MAINTAINERS file (moved to patch #2)
    
    Changes in v2:
    - binding description rewritten to a yaml schema file
    - added interrupt line
    - fixed unit address
    - patch number changed from 2 to 3

 .../bindings/serial/litex,liteuart.yaml       | 38 +++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/serial/litex,liteuart.yaml

diff --git a/Documentation/devicetree/bindings/serial/litex,liteuart.yaml b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml
new file mode 100644
index 000000000000..69acb222bb57
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/serial/litex,liteuart.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: LiteUART serial controller
+
+maintainers:
+  - Karol Gugala <kgugala@antmicro.com>
+  - Mateusz Holenko <mholenko@antmicro.com>
+
+description: |
+  LiteUART serial controller is a part of LiteX FPGA SoC builder. It supports
+  multiple CPU architectures, currently including e.g. OpenRISC and RISC-V.
+
+properties:
+  compatible:
+    const: litex,liteuart
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    uart0: serial@e0001800 {
+      compatible = "litex,liteuart";
+      reg = <0xe0001800 0x100>;
+      interrupts = <2>;
+    };
-- 
2.25.1

Re: [PATCH v11 4/5] dt-bindings: serial: document LiteUART bindings

[Geert Uytterhoeven]

Hi Mateusz,

On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> From: Filip Kokosinski <fkokosinski@antmicro.com>
>
> Add documentation for LiteUART devicetree bindings.
>
> Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

> --- /dev/null
> +++ b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml
> @@ -0,0 +1,38 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/serial/litex,liteuart.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: LiteUART serial controller
> +
> +maintainers:
> +  - Karol Gugala <kgugala@antmicro.com>
> +  - Mateusz Holenko <mholenko@antmicro.com>
> +
> +description: |
> +  LiteUART serial controller is a part of LiteX FPGA SoC builder. It supports

part of the

> +  multiple CPU architectures, currently including e.g. OpenRISC and RISC-V.
> +
> +properties:
> +  compatible:
> +    const: litex,liteuart

Have you already decided how to handle future LiteUART variants that add
new features (e.g. CTS/RTS, DMA)?


> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    uart0: serial@e0001800 {
> +      compatible = "litex,liteuart";
> +      reg = <0xe0001800 0x100>;
> +      interrupts = <2>;
> +    };
> --
> 2.25.1
>


--
Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v11 4/5] dt-bindings: serial: document LiteUART bindings

[Mateusz Holenko]

Hi Geert,

On Fri, Sep 25, 2020 at 3:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Mateusz,
>
> On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > From: Filip Kokosinski <fkokosinski@antmicro.com>
> >
> > Add documentation for LiteUART devicetree bindings.
> >
> > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > Reviewed-by: Rob Herring <robh@kernel.org>
>
> Thanks for your patch!

Thanks for your review!

>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml
> > @@ -0,0 +1,38 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/serial/litex,liteuart.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: LiteUART serial controller
> > +
> > +maintainers:
> > +  - Karol Gugala <kgugala@antmicro.com>
> > +  - Mateusz Holenko <mholenko@antmicro.com>
> > +
> > +description: |
> > +  LiteUART serial controller is a part of LiteX FPGA SoC builder. It supports
>
> part of the

Right, will fix that.

> > +  multiple CPU architectures, currently including e.g. OpenRISC and RISC-V.
> > +
> > +properties:
> > +  compatible:
> > +    const: litex,liteuart
>
> Have you already decided how to handle future LiteUART variants that add
> new features (e.g. CTS/RTS, DMA)?

We were thinking of adding KConfig options, like

[ ] LiteUART serial port support
< >     LiteUART DMA support

and using ifdefs in the code.

The other option could be to extend LiteX itself so that the UART core
provides information about its configuration via the capabilities register.
That way the driver could configure itself automatically at runtime.

This is, however, not decided yet.

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    uart0: serial@e0001800 {
> > +      compatible = "litex,liteuart";
> > +      reg = <0xe0001800 0x100>;
> > +      interrupts = <2>;
> > +    };
> > --
> > 2.25.1
> >
>
>
> --
> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Best regards,
Mateusz


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

Re: [PATCH v11 4/5] dt-bindings: serial: document LiteUART bindings

[Geert Uytterhoeven]

Hi Mateusz,

On Tue, Oct 6, 2020 at 9:01 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> On Fri, Sep 25, 2020 at 3:16 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Wed, Sep 23, 2020 at 12:10 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > From: Filip Kokosinski <fkokosinski@antmicro.com>
> > >
> > > Add documentation for LiteUART devicetree bindings.
> > >
> > > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > Reviewed-by: Rob Herring <robh@kernel.org>

> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml

> > > +properties:
> > > +  compatible:
> > > +    const: litex,liteuart
> >
> > Have you already decided how to handle future LiteUART variants that add
> > new features (e.g. CTS/RTS, DMA)?
>
> We were thinking of adding KConfig options, like
>
> [ ] LiteUART serial port support
> < >     LiteUART DMA support
>
> and using ifdefs in the code.

That is the driver part, not the DT part.
If enabled, the driver still needs to know if the feature is present and
to be used, or not.

> The other option could be to extend LiteX itself so that the UART core
> provides information about its configuration via the capabilities register.
> That way the driver could configure itself automatically at runtime.
>
> This is, however, not decided yet.

A capabilities register sounds good to me.
That means everything is handled automatically by the driver
However, it does mean the DT schema checker cannot validate the
use of optional DT properties related to optional features, if any.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v11 5/5] drivers/tty/serial: add LiteUART driver

[Mateusz Holenko]

From: Filip Kokosinski <fkokosinski@antmicro.com>

This commit adds driver for the FPGA-based LiteUART serial controller
from LiteX SoC builder.

The current implementation supports LiteUART configured
for 32 bit data width and 8 bit CSR bus width.

It does not support IRQ.

Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---

Notes:
    Changes in v11:
    - added Reviewed-by tag

    No changes in v10.

    No changes in v9.

    Changes in v8:
    - fixed help messages in LiteUART's KConfig
    - removed dependency between LiteUART and LiteX SoC drivers

    Changed in v7:
    - added missing include directive

    Changes in v6:
    - LiteUART ports now stored in xArray
    - removed PORT_LITEUART
    - fixed formatting
    - removed some unnecessary defines

    No changes in v5.

    Changes in v4:
    - fixed copyright header
    - removed a wrong dependency on UARTLITE from Kconfig
    - added a dependency on LITEX_SOC_CONTROLLER to LITEUART in Kconfig

    Changes in v3:
    - aliases made optional
    - used litex_get_reg/litex_set_reg functions instead of macros
    - SERIAL_LITEUART_NR_PORTS renamed to SERIAL_LITEUART_MAX_PORTS
    - PORT_LITEUART changed from 122 to 123
    - added dependency on LITEX_SOC_CONTROLLER
    - patch number changed from 4 to 5

    No changes in v2.

 MAINTAINERS                   |   1 +
 drivers/tty/serial/Kconfig    |  32 +++
 drivers/tty/serial/Makefile   |   1 +
 drivers/tty/serial/liteuart.c | 402 ++++++++++++++++++++++++++++++++++
 4 files changed, 436 insertions(+)
 create mode 100644 drivers/tty/serial/liteuart.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4d70a1b22a87..1387cefc63ce 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9841,6 +9841,7 @@ M:	Mateusz Holenko <mholenko@antmicro.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/*/litex,*.yaml
 F:	drivers/soc/litex/litex_soc_ctrl.c
+F:	drivers/tty/serial/liteuart.c
 F:	include/linux/litex.h
 
 LIVE PATCHING
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index adf9e80e7dc9..17aaf0afb27a 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1562,6 +1562,38 @@ config SERIAL_MILBEAUT_USIO_CONSOLE
 	  receives all kernel messages and warnings and which allows logins in
 	  single user mode).
 
+config SERIAL_LITEUART
+	tristate "LiteUART serial port support"
+	depends on HAS_IOMEM
+	depends on OF || COMPILE_TEST
+	depends on LITEX_SOC_CONTROLLER
+	select SERIAL_CORE
+	help
+	  This driver is for the FPGA-based LiteUART serial controller from LiteX
+	  SoC builder.
+
+	  Say 'Y' or 'M' here if you wish to use the LiteUART serial controller.
+	  Otherwise, say 'N'.
+
+config SERIAL_LITEUART_MAX_PORTS
+	int "Maximum number of LiteUART ports"
+	depends on SERIAL_LITEUART
+	default "1"
+	help
+	  Set this to the maximum number of serial ports you want the driver
+	  to support.
+
+config SERIAL_LITEUART_CONSOLE
+	bool "LiteUART serial port console support"
+	depends on SERIAL_LITEUART=y
+	select SERIAL_CORE_CONSOLE
+	help
+	  Say 'Y' or 'M' here if you wish to use the FPGA-based LiteUART serial
+	  controller from LiteX SoC builder as the system console
+	  (the system console is the device which receives all kernel messages
+	  and warnings and which allows logins in single user mode).
+	  Otherwise, say 'N'.
+
 endmenu
 
 config SERIAL_MCTRL_GPIO
diff --git a/drivers/tty/serial/Makefile b/drivers/tty/serial/Makefile
index d056ee6cca33..9f8ba419ff3b 100644
--- a/drivers/tty/serial/Makefile
+++ b/drivers/tty/serial/Makefile
@@ -89,6 +89,7 @@ obj-$(CONFIG_SERIAL_OWL)	+= owl-uart.o
 obj-$(CONFIG_SERIAL_RDA)	+= rda-uart.o
 obj-$(CONFIG_SERIAL_MILBEAUT_USIO) += milbeaut_usio.o
 obj-$(CONFIG_SERIAL_SIFIVE)	+= sifive.o
+obj-$(CONFIG_SERIAL_LITEUART) += liteuart.o
 
 # GPIOLIB helpers for modem control lines
 obj-$(CONFIG_SERIAL_MCTRL_GPIO)	+= serial_mctrl_gpio.o
diff --git a/drivers/tty/serial/liteuart.c b/drivers/tty/serial/liteuart.c
new file mode 100644
index 000000000000..b087e6a32106
--- /dev/null
+++ b/drivers/tty/serial/liteuart.c
@@ -0,0 +1,402 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LiteUART serial controller (LiteX) Driver
+ *
+ * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
+ */
+
+#include <linux/console.h>
+#include <linux/litex.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/serial.h>
+#include <linux/serial_core.h>
+#include <linux/slab.h>
+#include <linux/timer.h>
+#include <linux/tty_flip.h>
+#include <linux/xarray.h>
+
+/*
+ * CSRs definitions (base address offsets + width)
+ *
+ * The definitions below are true for LiteX SoC configured for 8-bit CSR Bus,
+ * 32-bit aligned.
+ *
+ * Supporting other configurations might require new definitions or a more
+ * generic way of indexing the LiteX CSRs.
+ *
+ * For more details on how CSRs are defined and handled in LiteX, see comments
+ * in the LiteX SoC Driver: drivers/soc/litex/litex_soc_ctrl.c
+ */
+#define OFF_RXTX	0x00
+#define OFF_TXFULL	0x04
+#define OFF_RXEMPTY	0x08
+#define OFF_EV_STATUS	0x0c
+#define OFF_EV_PENDING	0x10
+#define OFF_EV_ENABLE	0x14
+
+/* events */
+#define EV_TX		0x1
+#define EV_RX		0x2
+
+struct liteuart_port {
+	struct uart_port port;
+	struct timer_list timer;
+};
+
+#define to_liteuart_port(port)	container_of(port, struct liteuart_port, port)
+
+static DEFINE_XARRAY_FLAGS(liteuart_array, XA_FLAGS_ALLOC);
+
+#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+static struct console liteuart_console;
+#endif
+
+static struct uart_driver liteuart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = "liteuart",
+	.dev_name = "ttyLXU",
+	.major = 0,
+	.minor = 0,
+	.nr = CONFIG_SERIAL_LITEUART_MAX_PORTS,
+#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+	.cons = &liteuart_console,
+#endif
+};
+
+static void liteuart_timer(struct timer_list *t)
+{
+	struct liteuart_port *uart = from_timer(uart, t, timer);
+	struct uart_port *port = &uart->port;
+	unsigned char __iomem *membase = port->membase;
+	unsigned int flg = TTY_NORMAL;
+	int ch;
+	unsigned long status;
+
+	while ((status = !litex_get_reg(membase + OFF_RXEMPTY, 1)) == 1) {
+		ch = litex_get_reg(membase + OFF_RXTX, 1);
+		port->icount.rx++;
+
+		/* necessary for RXEMPTY to refresh its value */
+		litex_set_reg(membase + OFF_EV_PENDING, 1, EV_TX | EV_RX);
+
+		/* no overflow bits in status */
+		if (!(uart_handle_sysrq_char(port, ch)))
+			uart_insert_char(port, status, 0, ch, flg);
+
+		tty_flip_buffer_push(&port->state->port);
+	}
+
+	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+}
+
+static void liteuart_putchar(struct uart_port *port, int ch)
+{
+	while (litex_get_reg(port->membase + OFF_TXFULL, 1))
+		cpu_relax();
+
+	litex_set_reg(port->membase + OFF_RXTX, 1, ch);
+}
+
+static unsigned int liteuart_tx_empty(struct uart_port *port)
+{
+	/* not really tx empty, just checking if tx is not full */
+	if (!litex_get_reg(port->membase + OFF_TXFULL, 1))
+		return TIOCSER_TEMT;
+
+	return 0;
+}
+
+static void liteuart_set_mctrl(struct uart_port *port, unsigned int mctrl)
+{
+	/* modem control register is not present in LiteUART */
+}
+
+static unsigned int liteuart_get_mctrl(struct uart_port *port)
+{
+	return TIOCM_CTS | TIOCM_DSR | TIOCM_CAR;
+}
+
+static void liteuart_stop_tx(struct uart_port *port)
+{
+}
+
+static void liteuart_start_tx(struct uart_port *port)
+{
+	struct circ_buf *xmit = &port->state->xmit;
+	unsigned char ch;
+
+	if (unlikely(port->x_char)) {
+		litex_set_reg(port->membase + OFF_RXTX, 1, port->x_char);
+		port->icount.tx++;
+		port->x_char = 0;
+	} else if (!uart_circ_empty(xmit)) {
+		while (xmit->head != xmit->tail) {
+			ch = xmit->buf[xmit->tail];
+			xmit->tail = (xmit->tail + 1) & (UART_XMIT_SIZE - 1);
+			port->icount.tx++;
+			liteuart_putchar(port, ch);
+		}
+	}
+
+	if (uart_circ_chars_pending(xmit) < WAKEUP_CHARS)
+		uart_write_wakeup(port);
+}
+
+static void liteuart_stop_rx(struct uart_port *port)
+{
+	struct liteuart_port *uart = to_liteuart_port(port);
+
+	/* just delete timer */
+	del_timer(&uart->timer);
+}
+
+static void liteuart_break_ctl(struct uart_port *port, int break_state)
+{
+	/* LiteUART doesn't support sending break signal */
+}
+
+static int liteuart_startup(struct uart_port *port)
+{
+	struct liteuart_port *uart = to_liteuart_port(port);
+
+	/* disable events */
+	litex_set_reg(port->membase + OFF_EV_ENABLE, 1, 0);
+
+	/* prepare timer for polling */
+	timer_setup(&uart->timer, liteuart_timer, 0);
+	mod_timer(&uart->timer, jiffies + uart_poll_timeout(port));
+
+	return 0;
+}
+
+static void liteuart_shutdown(struct uart_port *port)
+{
+}
+
+static void liteuart_set_termios(struct uart_port *port, struct ktermios *new,
+				 struct ktermios *old)
+{
+	unsigned int baud;
+	unsigned long flags;
+
+	spin_lock_irqsave(&port->lock, flags);
+
+	/* update baudrate */
+	baud = uart_get_baud_rate(port, new, old, 0, 460800);
+	uart_update_timeout(port, new->c_cflag, baud);
+
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static const char *liteuart_type(struct uart_port *port)
+{
+	return "liteuart";
+}
+
+static void liteuart_release_port(struct uart_port *port)
+{
+}
+
+static int liteuart_request_port(struct uart_port *port)
+{
+	return 0;
+}
+
+static void liteuart_config_port(struct uart_port *port, int flags)
+{
+	/*
+	 * Driver core for serial ports forces a non-zero value for port type.
+	 * Write an arbitrary value here to accommodate the serial core driver,
+	 * as ID part of UAPI is redundant.
+	 */
+	port->type = 1;
+}
+
+static int liteuart_verify_port(struct uart_port *port,
+				struct serial_struct *ser)
+{
+	if (port->type != PORT_UNKNOWN && ser->type != 1)
+		return -EINVAL;
+
+	return 0;
+}
+
+static const struct uart_ops liteuart_ops = {
+	.tx_empty	= liteuart_tx_empty,
+	.set_mctrl	= liteuart_set_mctrl,
+	.get_mctrl	= liteuart_get_mctrl,
+	.stop_tx	= liteuart_stop_tx,
+	.start_tx	= liteuart_start_tx,
+	.stop_rx	= liteuart_stop_rx,
+	.break_ctl	= liteuart_break_ctl,
+	.startup	= liteuart_startup,
+	.shutdown	= liteuart_shutdown,
+	.set_termios	= liteuart_set_termios,
+	.type		= liteuart_type,
+	.release_port	= liteuart_release_port,
+	.request_port	= liteuart_request_port,
+	.config_port	= liteuart_config_port,
+	.verify_port	= liteuart_verify_port,
+};
+
+static int liteuart_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct liteuart_port *uart;
+	struct uart_port *port;
+	struct xa_limit limit;
+	int dev_id, ret;
+
+	/* no device tree */
+	if (!np)
+		return -ENODEV;
+
+	/* look for aliases; auto-enumerate for free index if not found */
+	dev_id = of_alias_get_id(np, "serial");
+	if (dev_id < 0)
+		limit = XA_LIMIT(0, CONFIG_SERIAL_LITEUART_MAX_PORTS);
+	else
+		limit = XA_LIMIT(dev_id, dev_id);
+
+	uart = kzalloc(sizeof(struct liteuart_port), GFP_KERNEL);
+	if (!uart)
+		return -ENOMEM;
+
+	ret = xa_alloc(&liteuart_array, &dev_id, uart, limit, GFP_KERNEL);
+	if (ret)
+		return ret;
+
+	port = &uart->port;
+
+	/* get membase */
+	port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
+	if (!port->membase)
+		return -ENXIO;
+
+	/* values not from device tree */
+	port->dev = &pdev->dev;
+	port->iotype = UPIO_MEM;
+	port->flags = UPF_BOOT_AUTOCONF;
+	port->ops = &liteuart_ops;
+	port->regshift = 2;
+	port->fifosize = 16;
+	port->iobase = 1;
+	port->type = PORT_UNKNOWN;
+	port->line = dev_id;
+	spin_lock_init(&port->lock);
+
+	return uart_add_one_port(&liteuart_driver, &uart->port);
+}
+
+static int liteuart_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static const struct of_device_id liteuart_of_match[] = {
+	{ .compatible = "litex,liteuart" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, liteuart_of_match);
+
+static struct platform_driver liteuart_platform_driver = {
+	.probe = liteuart_probe,
+	.remove = liteuart_remove,
+	.driver = {
+		.name = "liteuart",
+		.of_match_table = liteuart_of_match,
+	},
+};
+
+#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+
+static void liteuart_console_write(struct console *co, const char *s,
+	unsigned int count)
+{
+	struct liteuart_port *uart;
+	struct uart_port *port;
+	unsigned long flags;
+
+	uart = (struct liteuart_port *)xa_load(&liteuart_array, co->index);
+	port = &uart->port;
+
+	spin_lock_irqsave(&port->lock, flags);
+	uart_console_write(port, s, count, liteuart_putchar);
+	spin_unlock_irqrestore(&port->lock, flags);
+}
+
+static int liteuart_console_setup(struct console *co, char *options)
+{
+	struct liteuart_port *uart;
+	struct uart_port *port;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	uart = (struct liteuart_port *)xa_load(&liteuart_array, co->index);
+	if (!uart)
+		return -ENODEV;
+
+	port = &uart->port;
+	if (!port->membase)
+		return -ENODEV;
+
+	if (options)
+		uart_parse_options(options, &baud, &parity, &bits, &flow);
+
+	return uart_set_options(port, co, baud, parity, bits, flow);
+}
+
+static struct console liteuart_console = {
+	.name = "liteuart",
+	.write = liteuart_console_write,
+	.device = uart_console_device,
+	.setup = liteuart_console_setup,
+	.flags = CON_PRINTBUFFER,
+	.index = -1,
+	.data = &liteuart_driver,
+};
+
+static int __init liteuart_console_init(void)
+{
+	register_console(&liteuart_console);
+
+	return 0;
+}
+console_initcall(liteuart_console_init);
+#endif /* CONFIG_SERIAL_LITEUART_CONSOLE */
+
+static int __init liteuart_init(void)
+{
+	int res;
+
+	res = uart_register_driver(&liteuart_driver);
+	if (res)
+		return res;
+
+	res = platform_driver_register(&liteuart_platform_driver);
+	if (res) {
+		uart_unregister_driver(&liteuart_driver);
+		return res;
+	}
+
+	return 0;
+}
+
+static void __exit liteuart_exit(void)
+{
+	platform_driver_unregister(&liteuart_platform_driver);
+	uart_unregister_driver(&liteuart_driver);
+}
+
+module_init(liteuart_init);
+module_exit(liteuart_exit);
+
+MODULE_AUTHOR("Antmicro <www.antmicro.com>");
+MODULE_DESCRIPTION("LiteUART serial driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform: liteuart");
-- 
2.25.1

Re: [PATCH v11 5/5] drivers/tty/serial: add LiteUART driver

[Geert Uytterhoeven]

Hi Mateusz,

On Wed, Sep 23, 2020 at 12:12 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> From: Filip Kokosinski <fkokosinski@antmicro.com>
>
> This commit adds driver for the FPGA-based LiteUART serial controller
> from LiteX SoC builder.
>
> The current implementation supports LiteUART configured
> for 32 bit data width and 8 bit CSR bus width.
>
> It does not support IRQ.
>
> Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>

Thanks for your patch!

> --- /dev/null
> +++ b/drivers/tty/serial/liteuart.c

> +static int liteuart_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct liteuart_port *uart;
> +       struct uart_port *port;
> +       struct xa_limit limit;
> +       int dev_id, ret;
> +
> +       /* no device tree */
> +       if (!np)
> +               return -ENODEV;
> +
> +       /* look for aliases; auto-enumerate for free index if not found */
> +       dev_id = of_alias_get_id(np, "serial");
> +       if (dev_id < 0)
> +               limit = XA_LIMIT(0, CONFIG_SERIAL_LITEUART_MAX_PORTS);
> +       else
> +               limit = XA_LIMIT(dev_id, dev_id);
> +
> +       uart = kzalloc(sizeof(struct liteuart_port), GFP_KERNEL);

Who frees this memory? Use devm_kzalloc()?

> +       if (!uart)
> +               return -ENOMEM;
> +
> +       ret = xa_alloc(&liteuart_array, &dev_id, uart, limit, GFP_KERNEL);

Who frees this entry?

> +       if (ret)
> +               return ret;
> +
> +       port = &uart->port;
> +
> +       /* get membase */
> +       port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> +       if (!port->membase)
> +               return -ENXIO;
> +
> +       /* values not from device tree */
> +       port->dev = &pdev->dev;
> +       port->iotype = UPIO_MEM;
> +       port->flags = UPF_BOOT_AUTOCONF;
> +       port->ops = &liteuart_ops;
> +       port->regshift = 2;
> +       port->fifosize = 16;
> +       port->iobase = 1;
> +       port->type = PORT_UNKNOWN;
> +       port->line = dev_id;
> +       spin_lock_init(&port->lock);
> +
> +       return uart_add_one_port(&liteuart_driver, &uart->port);
> +}

> +static int __init liteuart_init(void)
> +{
> +       int res;
> +
> +       res = uart_register_driver(&liteuart_driver);
> +       if (res)
> +               return res;
> +
> +       res = platform_driver_register(&liteuart_platform_driver);
> +       if (res) {
> +               uart_unregister_driver(&liteuart_driver);
> +               return res;
> +       }
> +
> +       return 0;
> +}
> +
> +static void __exit liteuart_exit(void)
> +{
> +       platform_driver_unregister(&liteuart_platform_driver);
> +       uart_unregister_driver(&liteuart_driver);
> +}
> +
> +module_init(liteuart_init);
> +module_exit(liteuart_exit);

Several drivers call uart_{,un}register_driver() from their .probe()
resp. .remove() callbacks, so they can use module_platform_driver()
instead of the above boilerplate.  Greg, what's your stance on that?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v11 5/5] drivers/tty/serial: add LiteUART driver

[Mateusz Holenko]

Hi Geert,

On Fri, Sep 25, 2020 at 3:41 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
>
> Hi Mateusz,
>
> On Wed, Sep 23, 2020 at 12:12 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > From: Filip Kokosinski <fkokosinski@antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteUART serial controller
> > from LiteX SoC builder.
> >
> > The current implementation supports LiteUART configured
> > for 32 bit data width and 8 bit CSR bus width.
> >
> > It does not support IRQ.
> >
> > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
>
> Thanks for your patch!

Thanks for your review!

>
> > --- /dev/null
> > +++ b/drivers/tty/serial/liteuart.c
>
> > +static int liteuart_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct liteuart_port *uart;
> > +       struct uart_port *port;
> > +       struct xa_limit limit;
> > +       int dev_id, ret;
> > +
> > +       /* no device tree */
> > +       if (!np)
> > +               return -ENODEV;
> > +
> > +       /* look for aliases; auto-enumerate for free index if not found */
> > +       dev_id = of_alias_get_id(np, "serial");
> > +       if (dev_id < 0)
> > +               limit = XA_LIMIT(0, CONFIG_SERIAL_LITEUART_MAX_PORTS);
> > +       else
> > +               limit = XA_LIMIT(dev_id, dev_id);
> > +
> > +       uart = kzalloc(sizeof(struct liteuart_port), GFP_KERNEL);
>
> Who frees this memory? Use devm_kzalloc()?

You are right - it leaks right now. We'll switch to devm_kzalloc().

> > +       if (!uart)
> > +               return -ENOMEM;
> > +
> > +       ret = xa_alloc(&liteuart_array, &dev_id, uart, limit, GFP_KERNEL);
>
> Who frees this entry?

We'll add a call to xa_erase() when removing the driver.

> > +       if (ret)
> > +               return ret;
> > +
> > +       port = &uart->port;
> > +
> > +       /* get membase */
> > +       port->membase = devm_platform_get_and_ioremap_resource(pdev, 0, NULL);
> > +       if (!port->membase)
> > +               return -ENXIO;
> > +
> > +       /* values not from device tree */
> > +       port->dev = &pdev->dev;
> > +       port->iotype = UPIO_MEM;
> > +       port->flags = UPF_BOOT_AUTOCONF;
> > +       port->ops = &liteuart_ops;
> > +       port->regshift = 2;
> > +       port->fifosize = 16;
> > +       port->iobase = 1;
> > +       port->type = PORT_UNKNOWN;
> > +       port->line = dev_id;
> > +       spin_lock_init(&port->lock);
> > +
> > +       return uart_add_one_port(&liteuart_driver, &uart->port);
> > +}
>
> > +static int __init liteuart_init(void)
> > +{
> > +       int res;
> > +
> > +       res = uart_register_driver(&liteuart_driver);
> > +       if (res)
> > +               return res;
> > +
> > +       res = platform_driver_register(&liteuart_platform_driver);
> > +       if (res) {
> > +               uart_unregister_driver(&liteuart_driver);
> > +               return res;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void __exit liteuart_exit(void)
> > +{
> > +       platform_driver_unregister(&liteuart_platform_driver);
> > +       uart_unregister_driver(&liteuart_driver);
> > +}
> > +
> > +module_init(liteuart_init);
> > +module_exit(liteuart_exit);
>
> Several drivers call uart_{,un}register_driver() from their .probe()
> resp. .remove() callbacks, so they can use module_platform_driver()
> instead of the above boilerplate.  Greg, what's your stance on that?

I don't have much experience here and can't tell which version is the
preferred one.

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Best regards,
Mateusz

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