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

[PATCH v5 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 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                     |  14 +
 drivers/soc/litex/Makefile                    |   3 +
 drivers/soc/litex/litex_soc_ctrl.c            | 197 +++++++++
 drivers/tty/serial/Kconfig                    |  31 ++
 drivers/tty/serial/Makefile                   |   1 +
 drivers/tty/serial/liteuart.c                 | 411 ++++++++++++++++++
 include/linux/litex.h                         |  45 ++
 include/uapi/linux/serial_core.h              |   3 +
 14 files changed, 795 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 v5 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 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 9e67944bec9c..d9107f0fed2e 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -549,6 +549,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 v5 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:
    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..b118ddbf04a4
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/litex/litex,soc-controller.yaml
@@ -0,0 +1,39 @@
+# SPDX-License-Identifier: GPL-2.0
+# 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 = <0x0 0xf0000000 0x0 0xC>;
+        status = "okay";
+    };
+
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 44c1e8b3b3b4..421ede6c4f71 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9815,6 +9815,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

[PATCH v5 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 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          |  14 ++
 drivers/soc/litex/Makefile         |   3 +
 drivers/soc/litex/litex_soc_ctrl.c | 197 +++++++++++++++++++++++++++++
 include/linux/litex.h              |  45 +++++++
 7 files changed, 263 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 421ede6c4f71..1afe7348353b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9820,6 +9820,8 @@ M:	Karol Gugala <kgugala@antmicro.com>
 M:	Mateusz Holenko <mholenko@antmicro.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/*/litex,*.yaml
+F:	drivers/soc/litex/litex_soc_ctrl.c
+F:	include/linux/litex.h
 
 LIVE PATCHING
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 1778f8c62861..78add2a163be 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/ixp4xx/Kconfig"
+source "drivers/soc/litex/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/renesas/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index a39f17cea376..49bbb6ca6d95 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..71264c0e1d6c
--- /dev/null
+++ b/drivers/soc/litex/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License_Identifier: GPL-2.0
+
+menu "Enable LiteX SoC Builder specific drivers"
+
+config LITEX_SOC_CONTROLLER
+	tristate "Enable LiteX SoC Controller driver"
+	help
+	  This option enables the SoC Controller Driver which verifies
+	  LiteX CSR access and provides common litex_get_reg/litex_set_reg
+	  accessors.
+	  All drivers that use functions from litex.h must depend on
+	  LITEX_SOC_CONTROLLER.
+
+endmenu
diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
new file mode 100644
index 000000000000..98ff7325b1c0
--- /dev/null
+++ b/drivers/soc/litex/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License_Identifier: GPL-2.0
+
+obj-$(CONFIG_LITEX_SOC_CONTROLLER)	+= litex_soc_ctrl.o
diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
new file mode 100644
index 000000000000..16f1625836a5
--- /dev/null
+++ b/drivers/soc/litex/litex_soc_ctrl.c
@@ -0,0 +1,197 @@
+// 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/issues/314
+ *
+ * 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;
+
+		__raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
+	}
+
+	spin_unlock_irqrestore(&csr_lock, flags);
+}
+
+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 = __raw_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;
+}
+
+static int accessors_ok;
+
+/*
+ * Check if accessors are safe to be used by other drivers
+ * returns true if yes - false if not
+ */
+int litex_check_accessors(void)
+{
+	return accessors_ok;
+}
+
+#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
+ * and not set `accessors_ok` flag. As a result no other
+ * LiteX driver should access CSR bus.
+ *
+ * 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);
+
+	/* Set flag for other drivers */
+	accessors_ok = 1;
+	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)
+{
+	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);
+
+	return litex_check_csr_access(soc_ctrl_dev->base);
+}
+
+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..f31062436273
--- /dev/null
+++ b/include/linux/litex.h
@@ -0,0 +1,45 @@
+/* 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>
+
+/*
+ * litex_check_accessors is a function implemented in
+ * drivers/soc/litex/litex_soc_controller.c
+ * checking if the common LiteX CSR accessors
+ * are safe to be used by the drivers;
+ * returns true (1) if yes - false (0) if not
+ *
+ * Important: All drivers that use litex_set_reg/litex_get_reg
+ * functions should make sure that LiteX SoC Controller driver
+ * has verified LiteX CSRs read and write operations before
+ * issuing any read/writes to the LiteX peripherals.
+ *
+ * Exemplary snippet that can be used at the beginning
+ * of the driver's probe() function to ensure that LiteX
+ * SoC Controller driver is properely initialized:
+ *
+ * if (!litex_check_accessors())
+ *     return -EPROBE_DEFER;
+ */
+int litex_check_accessors(void);
+
+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 v5 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko]

On Sat, Apr 25, 2020 at 1:42 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>
> ---
>
> Notes:
>     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          |  14 ++
>  drivers/soc/litex/Makefile         |   3 +
>  drivers/soc/litex/litex_soc_ctrl.c | 197 +++++++++++++++++++++++++++++
>  include/linux/litex.h              |  45 +++++++
>  7 files changed, 263 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 421ede6c4f71..1afe7348353b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9820,6 +9820,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
>  M:     Mateusz Holenko <mholenko@antmicro.com>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> +F:     drivers/soc/litex/litex_soc_ctrl.c
> +F:     include/linux/litex.h
>
>  LIVE PATCHING
>  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 1778f8c62861..78add2a163be 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/litex/Kconfig"
>  source "drivers/soc/mediatek/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/renesas/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index a39f17cea376..49bbb6ca6d95 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..71264c0e1d6c
> --- /dev/null
> +++ b/drivers/soc/litex/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +menu "Enable LiteX SoC Builder specific drivers"
> +
> +config LITEX_SOC_CONTROLLER
> +       tristate "Enable LiteX SoC Controller driver"
> +       help
> +         This option enables the SoC Controller Driver which verifies
> +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> +         accessors.
> +         All drivers that use functions from litex.h must depend on
> +         LITEX_SOC_CONTROLLER.
> +
> +endmenu
> diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> new file mode 100644
> index 000000000000..98ff7325b1c0
> --- /dev/null
> +++ b/drivers/soc/litex/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> new file mode 100644
> index 000000000000..16f1625836a5
> --- /dev/null
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -0,0 +1,197 @@
> +// 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/issues/314

This could link to the wiki
(https://github.com/enjoy-digital/litex/wiki/CSR-Bus) directly.
(Suggested by Gabriel Somlo <gsomlo@gmail.com>)

> + *
> + * 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;
> +
> +               __raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));

As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
readl/writel/ioread/iowrite() standard functions providing memory
barriers *and* have values in CPU native endianness by using the
following constructs:

`le32_to_cpu(readl(addr))`

and

`writel(cpu_to_le32(value), addr)`

as le32_to_cpu/cpu_to_le32():
- does nothing on LE CPUs and
- reorders bytes on BE CPUs which in turn reverts swapping made by
readl() resulting in returning the original value.

> +       }
> +
> +       spin_unlock_irqrestore(&csr_lock, flags);
> +}
> +
> +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 = __raw_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;
> +}
> +
> +static int accessors_ok;
> +
> +/*
> + * Check if accessors are safe to be used by other drivers
> + * returns true if yes - false if not
> + */
> +int litex_check_accessors(void)
> +{
> +       return accessors_ok;
> +}
> +
> +#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
> + * and not set `accessors_ok` flag. As a result no other
> + * LiteX driver should access CSR bus.
> + *
> + * 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);
> +
> +       /* Set flag for other drivers */
> +       accessors_ok = 1;
> +       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)
> +{
> +       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);
> +
> +       return litex_check_csr_access(soc_ctrl_dev->base);
> +}
> +
> +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..f31062436273
> --- /dev/null
> +++ b/include/linux/litex.h
> @@ -0,0 +1,45 @@
> +/* 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>
> +
> +/*
> + * litex_check_accessors is a function implemented in
> + * drivers/soc/litex/litex_soc_controller.c
> + * checking if the common LiteX CSR accessors
> + * are safe to be used by the drivers;
> + * returns true (1) if yes - false (0) if not
> + *
> + * Important: All drivers that use litex_set_reg/litex_get_reg
> + * functions should make sure that LiteX SoC Controller driver
> + * has verified LiteX CSRs read and write operations before
> + * issuing any read/writes to the LiteX peripherals.
> + *
> + * Exemplary snippet that can be used at the beginning
> + * of the driver's probe() function to ensure that LiteX
> + * SoC Controller driver is properely initialized:
> + *
> + * if (!litex_check_accessors())
> + *     return -EPROBE_DEFER;
> + */
> +int litex_check_accessors(void);
> +
> +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
>


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

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

[Benjamin Herrenschmidt]

On Mon, 2020-04-27 at 11:13 +0200, Mateusz Holenko wrote:
> As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
> readl/writel/ioread/iowrite() standard functions providing memory
> barriers *and* have values in CPU native endianness by using the
> following constructs:
> 
> `le32_to_cpu(readl(addr))`
> 
> and
> 
> `writel(cpu_to_le32(value), addr)`
> 
> as le32_to_cpu/cpu_to_le32():
> - does nothing on LE CPUs and
> - reorders bytes on BE CPUs which in turn reverts swapping made by
> readl() resulting in returning the original value.

It's a bit sad... I don't understand why you need this. The HW has a
fied endian has you have mentioned earlier (and that is a good design).

The fact that you are trying to shove things into a "smaller pipe" than
the actual register shouldn't affect at what address the MSB and LSB
reside. And readl/writel (or ioread32/iowrite32) will always be LE as
well, so will match the HW layout. Thus I don't see why you need to
play swapping games here.

This however would be avoided completely if the HW was a tiny bit
smarter and would do the multi-beat access for you which shouldn't be
terribly hard to implement.

That said, it would be even clearer if you just open coded the 2 or 3
useful cases: 32/8, 32/16 and 32/32. The loop with calculated shifts
(and no masks) makes the code hard to understand.

Cheers,
Ben.

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

[Gabriel L. Somlo]

Hi Ben,

On Wed, Apr 29, 2020 at 01:21:11PM +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2020-04-27 at 11:13 +0200, Mateusz Holenko wrote:
> > As Gabriel Somlo <gsomlo@gmail.com> suggested to me, I could still use
> > readl/writel/ioread/iowrite() standard functions providing memory
> > barriers *and* have values in CPU native endianness by using the
> > following constructs:
> > 
> > `le32_to_cpu(readl(addr))`
> > 
> > and
> > 
> > `writel(cpu_to_le32(value), addr)`
> > 
> > as le32_to_cpu/cpu_to_le32():
> > - does nothing on LE CPUs and
> > - reorders bytes on BE CPUs which in turn reverts swapping made by
> > readl() resulting in returning the original value.
> 
> It's a bit sad... I don't understand why you need this. The HW has a
> fied endian has you have mentioned earlier (and that is a good design).
> 
> The fact that you are trying to shove things into a "smaller pipe" than
> the actual register shouldn't affect at what address the MSB and LSB
> reside. And readl/writel (or ioread32/iowrite32) will always be LE as
> well, so will match the HW layout. Thus I don't see why you need to
> play swapping games here.
> 
> This however would be avoided completely if the HW was a tiny bit
> smarter and would do the multi-beat access for you which shouldn't be
> terribly hard to implement.
> 
> That said, it would be even clearer if you just open coded the 2 or 3
> useful cases: 32/8, 32/16 and 32/32. The loop with calculated shifts
> (and no masks) makes the code hard to understand.

A "compound" LiteX MMIO register of 32 bits total, starting at address
0x80000004, containing value 0x12345678, is spread across 4 8-bit
subregisters aligned at ulong in the MMIO space like this on LE:

0x82000000  00 00 00 00 12 00 00 00 34 00 00 00 56 00 00 00  ........4...V...
                        ^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^
0x82000010  78 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  x...............
            ^^^^^^^^^^^

and like this on BE:

0x82000000  00 00 00 00 00 00 00 12 00 00 00 34 00 00 00 56  ...........4...V
                        ^^^^^^^^^^^ ^^^^^^^^^^^ ^^^^^^^^^^^
0x82000010  00 00 00 78 00 00 00 00 00 00 00 00 00 00 00 00  ...x............
            ^^^^^^^^^^^

LiteX can be optionally built to use larger than 8-bit subregisters,
here's an example with 16-bit subregisters (also aligned at ulong),
for the same "compound" register:

on LE:
0x82000000  00 00 00 00 34 12 00 00 78 56 00 00 00 00 00 00  ....4...xV......
                        ^^^^^^^^^^^ ^^^^^^^^^^^

and on BE:
0x82000000  00 00 00 00 00 00 12 34 00 00 56 78 00 00 00 00  .......4..Vx....
                        ^^^^^^^^^^^ ^^^^^^^^^^^

Essentially (back to the more common 8-bit subregister size), a compound
register foo = 0x12345678 is stored as

	ulong foo[4] = {0x12, 0x34, 0x56, 0x78};

in the CPU's native endianness, aligned at the CPU's native word width
(hence "ulong").

With 16-bit subregisters that would then be:

	ulong foo[2] = {0x1234, 0x5678};

Trouble with readl() and writel() is that they convert everything to LE
internally, which on BE would get us something different *within* each
subregister (i.e., 0x12000000 instead of 0x12, or 0x34120000 instead of
0x1234).

The cleanest way (IMHO) to accomplish an endian-agnostic readl() (that
preserves both barriers AND native endianness) is to undo the internal
__le32_to_cpu() using:

	cpu_to_le32(readl(addr))

This keeps us away from using any '__' internals directly (e.g.,
__raw_readl()), or open-coding our own `litex_readl()`, e.g.:

	static inline u32 litex_readl(const volatile void __iomem *addr)
	{
		u32 val;
		__io_br();
		val = __raw_readl(addr)); /* No le32 byteswap here! */
		__io_ar(val);
		return val;
	}

... which is something that was strongly advised against in earlier
revisions of this series.

Cheers,
--Gabriel

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

[Benjamin Herrenschmidt]

On Sat, 2020-04-25 at 13:42 +0200, Mateusz Holenko wrote:
> From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> 
> This commit adds driver for the FPGA-based LiteX SoC
> Controller from LiteX SoC builder.

Sorry for jumping in late, Joel only just pointed me to this :)

> + * 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;
> +
> +		__raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
> +	}
> +
> +	spin_unlock_irqrestore(&csr_lock, flags);
> +}
> +
> +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 = __raw_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;
> +}

I really don't like the fact that the register sizes & sub sizes are
#defined. As your comment explains, this makes it harder to support
other configurations. This geometry should come from the device-tree
instead.

Also this while thing is rather gross (and the lock will not help
performance). Why can't CSRs be normally memory mapped always instead ?

Even when transporting them on a HW bus that's smaller, the HW bus
conversion should be able to do the break-down into a multi-breat
transfer rather than doing that in SW.

Or at least have a fast-path if the register size is no larger than the
sub size, so you can use a normal ioread32/iowrite32.

Also I wonder ... last I played with LiteX, it would re-generate the
register layout (including the bit layout inside registers potentially)
rather enthousiastically, making it pretty hard to have a fixed
register layout for use by a kernel driver. Was this addressed ?

Cheers,
Ben.

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

[Mateusz Holenko]

On Wed, Apr 29, 2020 at 5:12 AM Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> On Sat, 2020-04-25 at 13:42 +0200, Mateusz Holenko wrote:
> > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
>
> Sorry for jumping in late, Joel only just pointed me to this :)
>
> > + * 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;
> > +
> > +             __raw_writel(shifted_data, reg + (LITEX_REG_SIZE * i));
> > +     }
> > +
> > +     spin_unlock_irqrestore(&csr_lock, flags);
> > +}
> > +
> > +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 = __raw_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;
> > +}
>
> I really don't like the fact that the register sizes & sub sizes are
> #defined. As your comment explains, this makes it harder to support
> other configurations. This geometry should come from the device-tree
> instead.

This is a valid point - putting those parameters into DT would indeed allow
for more flexibility. Currently we are focusing on supporting a single LiteX
configuration (32-bit/8-bit bus) and that's why those parameters got fixed.

Adding support for other configurations however is not just changing those
two parameters (which should be fairly easy by itself), but also
handling different
registers layout for a single peripheral (in different configurations
CSRs offsets and sizes may differ).

Since this adds another layer of complexity we want to start with a simpler
version that can be extended in the future.

> Also this while thing is rather gross (and the lock will not help
> performance). Why can't CSRs be normally memory mapped always instead ?

Using a different LiteX configuration 32-bit/32-bit bus would solve
the problem -
a single LiteX CSR would map nicely to a single 32-bit memory pointer and no
loop/locks would be needed.

In the default configuration (32-bit/8-bit bus) there are gaps between bytes
(as Gabriel Somlo already explained in his mail) which need to be handled
"manually".

> Even when transporting them on a HW bus that's smaller, the HW bus
> conversion should be able to do the break-down into a multi-breat
> transfer rather than doing that in SW.
>
> Or at least have a fast-path if the register size is no larger than the
> sub size, so you can use a normal ioread32/iowrite32.

Again - this is possible, but using a non-default 32-bit/32-bit bus LiteX
configuration.

> Also I wonder ... last I played with LiteX, it would re-generate the
> register layout (including the bit layout inside registers potentially)
> rather enthousiastically, making it pretty hard to have a fixed
> register layout for use by a kernel driver. Was this addressed ?

TBH I never experienced bit layout inside a register to change by itself,
but I agree that using different bus width configurations causes CSRs
to be splitted into 4/2/1 32-bit registers (changing de facto the layout
from the SW perspective) - that's why we provide helper functions
in this file.

It is possible to have different configurations of a peripheral
in LiteX that e.g, turns features on/off - this might cause some CSRs
to shift and result in incompatibilities. There are ways in LiteX
to avoid such problems if the model is properly designed, though.

Another aspect of LiteX is that the order in which separate peripherals
(modules) are created results in a different memory map of the whole SoC.
This, however, is easily addressed by using a dynamically generated DT
and do not require the code of drivers to be altered in any way.

> Cheers,
> Ben.
>
>

Thanks for your comments!


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

[PATCH v5 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 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..87bf846b170a
--- /dev/null
+++ b/Documentation/devicetree/bindings/serial/litex,liteuart.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0
+
+%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

[PATCH v5 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>
---

Notes:
    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       |  31 +++
 drivers/tty/serial/Makefile      |   1 +
 drivers/tty/serial/liteuart.c    | 411 +++++++++++++++++++++++++++++++
 include/uapi/linux/serial_core.h |   3 +
 5 files changed, 447 insertions(+)
 create mode 100644 drivers/tty/serial/liteuart.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a35be1be90d5..9e4e70b6b51d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9731,6 +9731,7 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/*/litex,*.yaml
 F:	drivers/soc/litex/litex_soc_ctrl.c
 F:	include/linux/litex.h
+F:	drivers/tty/serial/liteuart.c
 
 LIVE PATCHING
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
diff --git a/drivers/tty/serial/Kconfig b/drivers/tty/serial/Kconfig
index 52eaac21ff9f..2fabe0df774d 100644
--- a/drivers/tty/serial/Kconfig
+++ b/drivers/tty/serial/Kconfig
@@ -1572,6 +1572,37 @@ 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
+	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' 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' 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..b294b20967e4
--- /dev/null
+++ b/drivers/tty/serial/liteuart.c
@@ -0,0 +1,411 @@
+// 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/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/timer.h>
+#include <linux/tty_flip.h>
+#include <linux/litex.h>
+
+/* module-related defines */
+#define DRIVER_NAME	"liteuart"
+#define DRIVER_MAJOR	0
+#define DRIVER_MINOR	0
+#define DEV_NAME	"ttyLXU"
+
+/*
+ * 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 SIZE_RXTX	1
+#define OFF_TXFULL	0x04
+#define SIZE_TXFULL	1
+#define OFF_RXEMPTY	0x08
+#define SIZE_RXEMPTY	1
+#define OFF_EV_STATUS	0x0c
+#define SIZE_EV_STATUS	1
+#define OFF_EV_PENDING	0x10
+#define SIZE_EV_PENDING	1
+#define OFF_EV_ENABLE	0x14
+#define SIZE_EV_ENABLE	1
+
+/* 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 struct liteuart_port liteuart_ports[CONFIG_SERIAL_LITEUART_MAX_PORTS];
+static DECLARE_BITMAP(liteuart_ports_in_use, CONFIG_SERIAL_LITEUART_MAX_PORTS);
+
+#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+static struct console liteuart_console;
+#endif
+
+static struct uart_driver liteuart_driver = {
+	.owner = THIS_MODULE,
+	.driver_name = DRIVER_NAME,
+	.dev_name = DEV_NAME,
+	.major = DRIVER_MAJOR,
+	.minor = DRIVER_MINOR,
+	.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,
+			SIZE_RXEMPTY)) == 1) {
+		ch = litex_get_reg(membase + OFF_RXTX, SIZE_RXTX);
+		port->icount.rx++;
+
+		/* necessary for RXEMPTY to refresh its value */
+		litex_set_reg(membase + OFF_EV_PENDING,
+			SIZE_EV_PENDING, 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, SIZE_TXFULL))
+		cpu_relax();
+
+	litex_set_reg(port->membase + OFF_RXTX, SIZE_RXTX, 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, SIZE_TXFULL))
+		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,
+			SIZE_RXTX, 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, SIZE_EV_ENABLE, 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 (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
+}
+
+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)
+{
+	if (flags & UART_CONFIG_TYPE)
+		port->type = PORT_LITEUART;
+}
+
+static int liteuart_verify_port(struct uart_port *port,
+				struct serial_struct *ser)
+{
+	if (port->type != PORT_UNKNOWN && ser->type != PORT_LITEUART)
+		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;
+	int dev_id;
+
+	if (!litex_check_accessors())
+		return -EPROBE_DEFER;
+
+	/* 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)
+		dev_id = find_first_zero_bit(liteuart_ports_in_use,
+					     CONFIG_SERIAL_LITEUART_MAX_PORTS);
+
+	if (dev_id >= CONFIG_SERIAL_LITEUART_MAX_PORTS)
+		return -ENODEV;
+
+	if (test_and_set_bit(dev_id, liteuart_ports_in_use))
+		return -EBUSY;
+
+	uart = &liteuart_ports[dev_id];
+	port = &uart->port;
+
+	/* get {map,mem}base */
+	port->mapbase = platform_get_resource(pdev, IORESOURCE_MEM, 0)->start;
+	port->membase = of_iomap(np, 0);
+	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;
+
+	return uart_add_one_port(&liteuart_driver,
+				 &liteuart_ports[dev_id].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 = DRIVER_NAME,
+		.of_match_table = of_match_ptr(liteuart_of_match),
+	},
+};
+
+#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
+
+static void liteuart_console_write(struct console *co, const char *s,
+	unsigned int count)
+{
+	struct uart_port *port = &liteuart_ports[co->index].port;
+	unsigned long flags;
+
+	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 uart_port *port;
+	int baud = 115200;
+	int bits = 8;
+	int parity = 'n';
+	int flow = 'n';
+
+	port = &liteuart_ports[co->index].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 = DRIVER_NAME,
+	.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:" DRIVER_NAME);
diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
index 8ec3dd742ea4..449b8fe9273c 100644
--- a/include/uapi/linux/serial_core.h
+++ b/include/uapi/linux/serial_core.h
@@ -293,4 +293,7 @@
 /* Freescale LINFlexD UART */
 #define PORT_LINFLEXUART	122
 
+/* LiteUART */
+#define PORT_LITEUART	123
+
 #endif /* _UAPILINUX_SERIAL_CORE_H */
-- 
2.25.1

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

[Andy Shevchenko]

On Sat, Apr 25, 2020 at 2:45 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>

Co-developed-by?

...

> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9731,6 +9731,7 @@ S:        Maintained
>  F:     Documentation/devicetree/bindings/*/litex,*.yaml
>  F:     drivers/soc/litex/litex_soc_ctrl.c
>  F:     include/linux/litex.h
> +F:     drivers/tty/serial/liteuart.c

Ordering issue, run latest checkpatch.pl and parse-maintaners.pl to fix.

...

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

...

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

Can you use some like 76 characters per line?

...

> +#define OFF_RXTX       0x00
> +#define SIZE_RXTX      1
> +#define OFF_TXFULL     0x04
> +#define SIZE_TXFULL    1
> +#define OFF_RXEMPTY    0x08
> +#define SIZE_RXEMPTY   1
> +#define OFF_EV_STATUS  0x0c
> +#define SIZE_EV_STATUS 1
> +#define OFF_EV_PENDING 0x10
> +#define SIZE_EV_PENDING        1
> +#define OFF_EV_ENABLE  0x14
> +#define SIZE_EV_ENABLE 1

Why do you need all those SIZE_*?

...

> +static struct uart_driver liteuart_driver = {
> +       .owner = THIS_MODULE,
> +       .driver_name = DRIVER_NAME,
> +       .dev_name = DEV_NAME,

Much easier to see if any name collisions are happen by grepping
similar struct definitions, but these macros are making life harder.

> +       .major = DRIVER_MAJOR,
> +       .minor = DRIVER_MINOR,

Ditto.

> +       .nr = CONFIG_SERIAL_LITEUART_MAX_PORTS,

> +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> +       .cons = &liteuart_console,
> +#endif

> +};

...

> +static const char *liteuart_type(struct uart_port *port)
> +{
> +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> +}

Do we need this check? Do we need a port type at all?

...

> +static int liteuart_probe(struct platform_device *pdev)
> +{
> +       struct device_node *np = pdev->dev.of_node;
> +       struct liteuart_port *uart;
> +       struct uart_port *port;
> +       int dev_id;
> +
> +       if (!litex_check_accessors())
> +               return -EPROBE_DEFER;
> +

> +       /* no device tree */
> +       if (!np)
> +               return -ENODEV;

I guess it should go first, otherwise potentially you may end up with
deferred module above.

> +       /* look for aliases; auto-enumerate for free index if not found */
> +       dev_id = of_alias_get_id(np, "serial");
> +       if (dev_id < 0)
> +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);

Racy.

> +       /* get {map,mem}base */
> +       port->mapbase = platform_get_resource(pdev, IORESOURCE_MEM, 0)->start;
> +       port->membase = of_iomap(np, 0);

Can't you use devm_platform_get_and_ioremap_resource() ?

> +       if (!port->membase)
> +               return -ENXIO;

> +}

...

> +static struct platform_driver liteuart_platform_driver = {
> +       .probe = liteuart_probe,
> +       .remove = liteuart_remove,
> +       .driver = {
> +               .name = DRIVER_NAME,

> +               .of_match_table = of_match_ptr(liteuart_of_match),

of_match_ptr() makes no sense (you have depends on OF).

> +       },
> +};

...


> +static int __init liteuart_console_init(void)
> +{

Missed spin lock initialization.

> +       register_console(&liteuart_console);
> +
> +       return 0;
> +}

> +

Extra blank line.

> +console_initcall(liteuart_console_init);

...

> +/* LiteUART */
> +#define PORT_LITEUART  123

We have holes in the list, use them.

And again why we need this?

-- 
With Best Regards,
Andy Shevchenko

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

[Mateusz Holenko]

On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Apr 25, 2020 at 2:45 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>
>
> Co-developed-by?

Most of the coding here is done by Filip Kokosinski - I'm responsible
for managing the patches and sending to LKML so I don't think I
qualify as a co-developer :)

> ...
>
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9731,6 +9731,7 @@ S:        Maintained
> >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> >  F:     drivers/soc/litex/litex_soc_ctrl.c
> >  F:     include/linux/litex.h
> > +F:     drivers/tty/serial/liteuart.c
>
> Ordering issue, run latest checkpatch.pl and parse-maintaners.pl to fix.

We'll check that.

> ...
>
> > +config SERIAL_LITEUART
> > +       tristate "LiteUART serial port support"
> > +       depends on HAS_IOMEM
>
> > +       depends on OF
>
> || COMPILE_TEST ?

Sure, we'll add that.

> > +       depends on LITEX_SOC_CONTROLLER
> > +       select SERIAL_CORE
>
> ...
>
> > +/*
> > + * 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
> > + */
>
> Can you use some like 76 characters per line?
>

We'll reformat the code to match 76 chars.

> ...
>
> > +#define OFF_RXTX       0x00
> > +#define SIZE_RXTX      1
> > +#define OFF_TXFULL     0x04
> > +#define SIZE_TXFULL    1
> > +#define OFF_RXEMPTY    0x08
> > +#define SIZE_RXEMPTY   1
> > +#define OFF_EV_STATUS  0x0c
> > +#define SIZE_EV_STATUS 1
> > +#define OFF_EV_PENDING 0x10
> > +#define SIZE_EV_PENDING        1
> > +#define OFF_EV_ENABLE  0x14
> > +#define SIZE_EV_ENABLE 1
>
> Why do you need all those SIZE_*?
>
> ...

This is related to how LiteX peripherals (LiteUART being one of them)
handle register access.
The LiteX HW splits a classic 32-bit register into 4 32-bit registers,
each one containing only 8-bit part of it.

SIZE in this context means how many of those "subregisters" (still
32-bit wide, but with only 8-bit of meaningful data) to read/write.
The "litex.h" header (patch 3 of this patchset) provides common
functions for doing it, but it must know the size for each register.

>
> > +static struct uart_driver liteuart_driver = {
> > +       .owner = THIS_MODULE,
> > +       .driver_name = DRIVER_NAME,
> > +       .dev_name = DEV_NAME,
>
> Much easier to see if any name collisions are happen by grepping
> similar struct definitions, but these macros are making life harder.

Do you mean to avoid indirection caused by defines and write e.g.,
`.driver_name = "liteuart"`?

OK, but the reason we have defines in the first place is because we
use the same name in many places and we want to avoid inconsistencies
(typos, partial rename, etc.).
What's more, looking at other serial drivers I see the notation is not
consistent - many of them use defines for name/major/minor as well.

> > +       .major = DRIVER_MAJOR,
> > +       .minor = DRIVER_MINOR,
>
> Ditto.
>
> > +       .nr = CONFIG_SERIAL_LITEUART_MAX_PORTS,
>
> > +#ifdef CONFIG_SERIAL_LITEUART_CONSOLE
> > +       .cons = &liteuart_console,
> > +#endif
>
> > +};
>
> ...
>
> > +static const char *liteuart_type(struct uart_port *port)
> > +{
> > +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > +}
>
> Do we need this check? Do we need a port type at all?
>
> ...

This is inspired by serial_core.c and other serial drivers.
We don't support any alternative `port->types` values so it's probably
not necessary for us, but it seems that this is how other serial
drivers are written too.

> > +static int liteuart_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *np = pdev->dev.of_node;
> > +       struct liteuart_port *uart;
> > +       struct uart_port *port;
> > +       int dev_id;
> > +
> > +       if (!litex_check_accessors())
> > +               return -EPROBE_DEFER;
> > +
>
> > +       /* no device tree */
> > +       if (!np)
> > +               return -ENODEV;
>
> I guess it should go first, otherwise potentially you may end up with
> deferred module above.

You are right. We'll reorder the initialization.

> > +       /* look for aliases; auto-enumerate for free index if not found */
> > +       dev_id = of_alias_get_id(np, "serial");
> > +       if (dev_id < 0)
> > +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> > +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);
>
> Racy.

We'll protect it with a mutex to avoid race conditions.

> > +       /* get {map,mem}base */
> > +       port->mapbase = platform_get_resource(pdev, IORESOURCE_MEM, 0)->start;
> > +       port->membase = of_iomap(np, 0);
>
> Can't you use devm_platform_get_and_ioremap_resource() ?

This indeed can be simplified.

> > +       if (!port->membase)
> > +               return -ENXIO;
>
> > +}
>
> ...
>
> > +static struct platform_driver liteuart_platform_driver = {
> > +       .probe = liteuart_probe,
> > +       .remove = liteuart_remove,
> > +       .driver = {
> > +               .name = DRIVER_NAME,
>
> > +               .of_match_table = of_match_ptr(liteuart_of_match),
>
> of_match_ptr() makes no sense (you have depends on OF).

You mean that `of_match_ptr(X)` resolves simply to `X` when
`CONFIG_OF` is defined?
In this context it surely can be simplified.

> > +       },
> > +};
>
> ...
>
>
> > +static int __init liteuart_console_init(void)
> > +{
>
> Missed spin lock initialization.

We'll fix this.

> > +       register_console(&liteuart_console);
> > +
> > +       return 0;
> > +}
>
> > +
>
> Extra blank line.

You mean we should remove an empty line between the definition of
liteuart_console_init() and the call to console_initcall()? It seems
to be inconsistent across different drivers, but sure - no problem.

> > +console_initcall(liteuart_console_init);
>
> ...
>
> > +/* LiteUART */
> > +#define PORT_LITEUART  123
>
> We have holes in the list, use them.
>
> And again why we need this?

This is inspired by other serial drivers that also reserves
identifiers in this file and handles them the same way we do. We
simply followed the convention.

> --
> With Best Regards,
> Andy Shevchenko

Thanks for your time and the comments! We'll address them in the next
version of the patchset.

Best regards,
Mateusz Hołenko

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

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

[Andy Shevchenko]

On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:

...

> > > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> >
> > Co-developed-by?
>
> Most of the coding here is done by Filip Kokosinski - I'm responsible
> for managing the patches and sending to LKML so I don't think I
> qualify as a co-developer :)

I see.

...

> > > +#define OFF_RXTX       0x00
> > > +#define SIZE_RXTX      1
> > > +#define OFF_TXFULL     0x04
> > > +#define SIZE_TXFULL    1
> > > +#define OFF_RXEMPTY    0x08
> > > +#define SIZE_RXEMPTY   1
> > > +#define OFF_EV_STATUS  0x0c
> > > +#define SIZE_EV_STATUS 1
> > > +#define OFF_EV_PENDING 0x10
> > > +#define SIZE_EV_PENDING        1
> > > +#define OFF_EV_ENABLE  0x14
> > > +#define SIZE_EV_ENABLE 1
> >
> > Why do you need all those SIZE_*?

> This is related to how LiteX peripherals (LiteUART being one of them)
> handle register access.
> The LiteX HW splits a classic 32-bit register into 4 32-bit registers,
> each one containing only 8-bit part of it.
>
> SIZE in this context means how many of those "subregisters" (still
> 32-bit wide, but with only 8-bit of meaningful data) to read/write.
> The "litex.h" header (patch 3 of this patchset) provides common
> functions for doing it, but it must know the size for each register.

So, can't you simple use them as is? I still didn't get how SIZE helps here.

...

> > > +static struct uart_driver liteuart_driver = {
> > > +       .owner = THIS_MODULE,
> > > +       .driver_name = DRIVER_NAME,
> > > +       .dev_name = DEV_NAME,
> >
> > Much easier to see if any name collisions are happen by grepping
> > similar struct definitions, but these macros are making life harder.
>
> Do you mean to avoid indirection caused by defines and write e.g.,
> `.driver_name = "liteuart"`?
>
> OK, but the reason we have defines in the first place is because we
> use the same name in many places and we want to avoid inconsistencies
> (typos, partial rename, etc.).
> What's more, looking at other serial drivers I see the notation is not
> consistent - many of them use defines for name/major/minor as well.

The problem here that .driver_name is a part of user visible
interface, so, when you rename it it will affect the module alias.
How DEV_NAME is shared? It should not be, otherwise it will collide
with other drivers.

> > > +       .major = DRIVER_MAJOR,
> > > +       .minor = DRIVER_MINOR,
> >
> > Ditto.

Ditto.

> > > +};

...

> > > +static const char *liteuart_type(struct uart_port *port)
> > > +{
> > > +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > > +}
> >
> > Do we need this check? Do we need a port type at all?

> This is inspired by serial_core.c and other serial drivers.
> We don't support any alternative `port->types` values so it's probably
> not necessary for us, but it seems that this is how other serial
> drivers are written too.

Legacy drivers are not the best example to take. So, if you can
survive without UART type, please go with it. Otherwise commit message
should point out why it's needed so eagerly.

...

> > > +       /* look for aliases; auto-enumerate for free index if not found */
> > > +       dev_id = of_alias_get_id(np, "serial");
> > > +       if (dev_id < 0)
> > > +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> > > +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);
> >
> > Racy.
>
> We'll protect it with a mutex to avoid race conditions.

Rather consider to use xArray API.

...

> > > +               .of_match_table = of_match_ptr(liteuart_of_match),
> >
> > of_match_ptr() makes no sense (you have depends on OF).
>
> You mean that `of_match_ptr(X)` resolves simply to `X` when
> `CONFIG_OF` is defined?
> In this context it surely can be simplified.

Yes.

...

> > > +static int __init liteuart_console_init(void)
> > > +{

> > > +}
> >
> > > +
> >
> > Extra blank line.
>
> You mean we should remove an empty line between the definition of
> liteuart_console_init() and the call to console_initcall()? It seems
> to be inconsistent across different drivers, but sure - no problem.

Less LOCs is good (but keep common sense applied).

...

> > > +/* LiteUART */
> > > +#define PORT_LITEUART  123
> >
> > We have holes in the list, use them.
> >
> > And again why we need this?
>
> This is inspired by other serial drivers that also reserves
> identifiers in this file and handles them the same way we do. We
> simply followed the convention.

See above. This ID is a part of UAPI which is kinda redundant nowadays.
You need to provide a good argument for that. Otherwise, get rid of it.

-- 
With Best Regards,
Andy Shevchenko

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

[Mateusz Holenko]

On Tue, May 5, 2020 at 4:02 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, May 4, 2020 at 4:44 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > On Tue, Apr 28, 2020 at 5:50 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Sat, Apr 25, 2020 at 2:45 PM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> ...
>
> > > > Signed-off-by: Filip Kokosinski <fkokosinski@antmicro.com>
> > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > >
> > > Co-developed-by?
> >
> > Most of the coding here is done by Filip Kokosinski - I'm responsible
> > for managing the patches and sending to LKML so I don't think I
> > qualify as a co-developer :)
>
> I see.
>
> ...
>
> > > > +#define OFF_RXTX       0x00
> > > > +#define SIZE_RXTX      1
> > > > +#define OFF_TXFULL     0x04
> > > > +#define SIZE_TXFULL    1
> > > > +#define OFF_RXEMPTY    0x08
> > > > +#define SIZE_RXEMPTY   1
> > > > +#define OFF_EV_STATUS  0x0c
> > > > +#define SIZE_EV_STATUS 1
> > > > +#define OFF_EV_PENDING 0x10
> > > > +#define SIZE_EV_PENDING        1
> > > > +#define OFF_EV_ENABLE  0x14
> > > > +#define SIZE_EV_ENABLE 1
> > >
> > > Why do you need all those SIZE_*?
>
> > This is related to how LiteX peripherals (LiteUART being one of them)
> > handle register access.
> > The LiteX HW splits a classic 32-bit register into 4 32-bit registers,
> > each one containing only 8-bit part of it.
> >
> > SIZE in this context means how many of those "subregisters" (still
> > 32-bit wide, but with only 8-bit of meaningful data) to read/write.
> > The "litex.h" header (patch 3 of this patchset) provides common
> > functions for doing it, but it must know the size for each register.
>
> So, can't you simple use them as is? I still didn't get how SIZE helps here.
>
> ...

Do you mean to call litex_get_reg() with 1 directly as a second argument instead
of using a defined value (which in the context of uart driver is 1 for
every register anyway)?

Sure, this is doable. We just thought a named define will explain
better what's going on.

In general case, register's offsets and sizes might differ between
LiteX configurations
(LiteX is an SoC generator capable of creating different setups) so
having a dynamic
size in litex_get_reg() is useful.
With this patchset we are targeting a single (default) configuration
(so it's enough for
OFF_ and SIZE_ to be fixed), but this could be extended to be more
dynamic in the future.

> > > > +static struct uart_driver liteuart_driver = {
> > > > +       .owner = THIS_MODULE,
> > > > +       .driver_name = DRIVER_NAME,
> > > > +       .dev_name = DEV_NAME,
> > >
> > > Much easier to see if any name collisions are happen by grepping
> > > similar struct definitions, but these macros are making life harder.
> >
> > Do you mean to avoid indirection caused by defines and write e.g.,
> > `.driver_name = "liteuart"`?
> >
> > OK, but the reason we have defines in the first place is because we
> > use the same name in many places and we want to avoid inconsistencies
> > (typos, partial rename, etc.).
> > What's more, looking at other serial drivers I see the notation is not
> > consistent - many of them use defines for name/major/minor as well.
>
> The problem here that .driver_name is a part of user visible
> interface, so, when you rename it it will affect the module alias.
> How DEV_NAME is shared? It should not be, otherwise it will collide
> with other drivers.

I meant that DRIVER_NAME define is used in the file in many places:
* liteuart_driver.driver_name
* liteuart_type()
* liteuart_platfrom_driver.driver.name
* liteuart_console.name
* MODULE_ALIAS
It's not shared with other drivers, but used multiple times in the
code of this one.

DEV_NAME/DRIVER_MAJOR/DRIVER_MINOR are indeed referenced only once so
it's no problem to write those values directly and get rid of their
defines.

> > > > +       .major = DRIVER_MAJOR,
> > > > +       .minor = DRIVER_MINOR,
> > >
> > > Ditto.
>
> Ditto.
>
> > > > +};
>
> ...

What do you mean by '...' ?

>
> > > > +static const char *liteuart_type(struct uart_port *port)
> > > > +{
> > > > +       return (port->type == PORT_LITEUART) ? DRIVER_NAME : NULL;
> > > > +}
> > >
> > > Do we need this check? Do we need a port type at all?
>
> > This is inspired by serial_core.c and other serial drivers.
> > We don't support any alternative `port->types` values so it's probably
> > not necessary for us, but it seems that this is how other serial
> > drivers are written too.
>
> Legacy drivers are not the best example to take. So, if you can
> survive without UART type, please go with it. Otherwise commit message
> should point out why it's needed so eagerly.
>
> ...

I guess we'll be good without the UART type.

> > > > +       /* look for aliases; auto-enumerate for free index if not found */
> > > > +       dev_id = of_alias_get_id(np, "serial");
> > > > +       if (dev_id < 0)
> > > > +               dev_id = find_first_zero_bit(liteuart_ports_in_use,
> > > > +                                            CONFIG_SERIAL_LITEUART_MAX_PORTS);
> > >
> > > Racy.
> >
> > We'll protect it with a mutex to avoid race conditions.
>
> Rather consider to use xArray API.
>
> ...

We'll check that up.

> > > > +               .of_match_table = of_match_ptr(liteuart_of_match),
> > >
> > > of_match_ptr() makes no sense (you have depends on OF).
> >
> > You mean that `of_match_ptr(X)` resolves simply to `X` when
> > `CONFIG_OF` is defined?
> > In this context it surely can be simplified.
>
> Yes.
>
> ...

OK.

> > > > +static int __init liteuart_console_init(void)
> > > > +{
>
> > > > +}
> > >
> > > > +
> > >
> > > Extra blank line.
> >
> > You mean we should remove an empty line between the definition of
> > liteuart_console_init() and the call to console_initcall()? It seems
> > to be inconsistent across different drivers, but sure - no problem.
>
> Less LOCs is good (but keep common sense applied).
>
> ...

Sure.

> > > > +/* LiteUART */
> > > > +#define PORT_LITEUART  123
> > >
> > > We have holes in the list, use them.
> > >
> > > And again why we need this?
> >
> > This is inspired by other serial drivers that also reserves
> > identifiers in this file and handles them the same way we do. We
> > simply followed the convention.
>
> See above. This ID is a part of UAPI which is kinda redundant nowadays.
> You need to provide a good argument for that. Otherwise, get rid of it.

We'll remove this and see if everything works fine.

> --
> With Best Regards,
> Andy Shevchenko

Thanks for the discussion!


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