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

[PATCH v4 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 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            | 217 +++++++++
 drivers/tty/serial/Kconfig                    |  32 +-
 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, 815 insertions(+), 1 deletion(-)
 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 v4 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 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 v4 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>
---

Notes:
    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 cc1d18cb5d18..2f5ede8a08aa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9724,6 +9724,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 v4 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 v4:
    - fixed indent in Kconfig's help section
    - fixed copyright header
    - changed compatible to "litex,soc-controller"
    - simplified litex_soc_ctrl_probe
    - removed unnecessary litex_soc_ctrl_remove

    This commit has been introduced in v3 of the patchset.

    It includes a simplified version of common 'litex.h'
    header introduced in v2 of the patchset.

 MAINTAINERS                        |   2 +
 drivers/soc/Kconfig                |   1 +
 drivers/soc/Makefile               |   1 +
 drivers/soc/litex/Kconfig          |  14 ++
 drivers/soc/litex/Makefile         |   3 +
 drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
 include/linux/litex.h              |  45 ++++++
 7 files changed, 283 insertions(+)
 create mode 100644 drivers/soc/litex/Kconfig
 create mode 100644 drivers/soc/litex/Makefile
 create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
 create mode 100644 include/linux/litex.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 2f5ede8a08aa..a35be1be90d5 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9729,6 +9729,8 @@ M:	Karol Gugala <kgugala@antmicro.com>
 M:	Mateusz Holenko <mholenko@antmicro.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/*/litex,*.yaml
+F:	drivers/soc/litex/litex_soc_ctrl.c
+F:	include/linux/litex.h
 
 LIVE PATCHING
 M:	Josh Poimboeuf <jpoimboe@redhat.com>
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 1778f8c62861..78add2a163be 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/ixp4xx/Kconfig"
+source "drivers/soc/litex/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/renesas/Kconfig"
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 8b49d782a1ab..fd016b51cddd 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
 obj-$(CONFIG_ARCH_MXC)		+= imx/
 obj-$(CONFIG_ARCH_IXP4XX)	+= ixp4xx/
 obj-$(CONFIG_SOC_XWAY)		+= lantiq/
+obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
 obj-y				+= mediatek/
 obj-y				+= amlogic/
 obj-y				+= qcom/
diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
new file mode 100644
index 000000000000..71264c0e1d6c
--- /dev/null
+++ b/drivers/soc/litex/Kconfig
@@ -0,0 +1,14 @@
+# SPDX-License_Identifier: GPL-2.0
+
+menu "Enable LiteX SoC Builder specific drivers"
+
+config LITEX_SOC_CONTROLLER
+	tristate "Enable LiteX SoC Controller driver"
+	help
+	  This option enables the SoC Controller Driver which verifies
+	  LiteX CSR access and provides common litex_get_reg/litex_set_reg
+	  accessors.
+	  All drivers that use functions from litex.h must depend on
+	  LITEX_SOC_CONTROLLER.
+
+endmenu
diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
new file mode 100644
index 000000000000..98ff7325b1c0
--- /dev/null
+++ b/drivers/soc/litex/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License_Identifier: GPL-2.0
+
+obj-$(CONFIG_LITEX_SOC_CONTROLLER)	+= litex_soc_ctrl.o
diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
new file mode 100644
index 000000000000..5defba000fd4
--- /dev/null
+++ b/drivers/soc/litex/litex_soc_ctrl.c
@@ -0,0 +1,217 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LiteX SoC Controller Driver
+ *
+ * Copyright (C) 2020 Antmicro <www.antmicro.com>
+ *
+ */
+
+#include <linux/litex.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+
+/*
+ * The parameters below are true for LiteX SoC
+ * configured for 8-bit CSR Bus, 32-bit aligned.
+ *
+ * Supporting other configurations will require
+ * extending the logic in this header.
+ */
+#define LITEX_REG_SIZE             0x4
+#define LITEX_SUBREG_SIZE          0x1
+#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
+
+static DEFINE_SPINLOCK(csr_lock);
+
+static inline unsigned long read_pointer_with_barrier(
+	const volatile void __iomem *addr)
+{
+	unsigned long val;
+
+	__io_br();
+	val = *(const volatile unsigned long __force *)addr;
+	__io_ar();
+	return val;
+}
+
+static inline void write_pointer_with_barrier(
+	volatile void __iomem *addr, unsigned long val)
+{
+	__io_br();
+	*(volatile unsigned long __force *)addr = val;
+	__io_ar();
+}
+
+/*
+ * 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;
+		write_pointer_with_barrier(
+			reg + (LITEX_REG_SIZE * i), shifted_data);
+	}
+
+	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 = read_pointer_with_barrier(
+			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 (IS_ERR_OR_NULL(soc_ctrl_dev))
+		return -ENOMEM;
+
+	soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR_OR_NULL(soc_ctrl_dev->base))
+		return -EIO;
+
+	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

[PATCH v4 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 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 v4 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:
    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 v4 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko]

On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
>
> From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
>
> This commit adds driver for the FPGA-based LiteX SoC
> Controller from LiteX SoC builder.
>
> Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> ---
>
> Notes:
>     Changes in v4:
>     - fixed indent in Kconfig's help section
>     - fixed copyright header
>     - changed compatible to "litex,soc-controller"
>     - simplified litex_soc_ctrl_probe
>     - removed unnecessary litex_soc_ctrl_remove
>
>     This commit has been introduced in v3 of the patchset.
>
>     It includes a simplified version of common 'litex.h'
>     header introduced in v2 of the patchset.
>
>  MAINTAINERS                        |   2 +
>  drivers/soc/Kconfig                |   1 +
>  drivers/soc/Makefile               |   1 +
>  drivers/soc/litex/Kconfig          |  14 ++
>  drivers/soc/litex/Makefile         |   3 +
>  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
>  include/linux/litex.h              |  45 ++++++
>  7 files changed, 283 insertions(+)
>  create mode 100644 drivers/soc/litex/Kconfig
>  create mode 100644 drivers/soc/litex/Makefile
>  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
>  create mode 100644 include/linux/litex.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2f5ede8a08aa..a35be1be90d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
>  M:     Mateusz Holenko <mholenko@antmicro.com>
>  S:     Maintained
>  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> +F:     drivers/soc/litex/litex_soc_ctrl.c
> +F:     include/linux/litex.h
>
>  LIVE PATCHING
>  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 1778f8c62861..78add2a163be 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/litex/Kconfig"
>  source "drivers/soc/mediatek/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/renesas/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..fd016b51cddd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
>  obj-$(CONFIG_ARCH_MXC)         += imx/
>  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
>  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
>  obj-y                          += mediatek/
>  obj-y                          += amlogic/
>  obj-y                          += qcom/
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> new file mode 100644
> index 000000000000..71264c0e1d6c
> --- /dev/null
> +++ b/drivers/soc/litex/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +menu "Enable LiteX SoC Builder specific drivers"
> +
> +config LITEX_SOC_CONTROLLER
> +       tristate "Enable LiteX SoC Controller driver"
> +       help
> +         This option enables the SoC Controller Driver which verifies
> +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> +         accessors.
> +         All drivers that use functions from litex.h must depend on
> +         LITEX_SOC_CONTROLLER.
> +
> +endmenu
> diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> new file mode 100644
> index 000000000000..98ff7325b1c0
> --- /dev/null
> +++ b/drivers/soc/litex/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> new file mode 100644
> index 000000000000..5defba000fd4
> --- /dev/null
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LiteX SoC Controller Driver
> + *
> + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> + *
> + */
> +
> +#include <linux/litex.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +
> +/*
> + * The parameters below are true for LiteX SoC
> + * configured for 8-bit CSR Bus, 32-bit aligned.
> + *
> + * Supporting other configurations will require
> + * extending the logic in this header.
> + */
> +#define LITEX_REG_SIZE             0x4
> +#define LITEX_SUBREG_SIZE          0x1
> +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> +
> +static DEFINE_SPINLOCK(csr_lock);
> +
> +static inline unsigned long read_pointer_with_barrier(
> +       const volatile void __iomem *addr)
> +{
> +       unsigned long val;
> +
> +       __io_br();
> +       val = *(const volatile unsigned long __force *)addr;
> +       __io_ar();
> +       return val;
> +}
> +
> +static inline void write_pointer_with_barrier(
> +       volatile void __iomem *addr, unsigned long val)
> +{
> +       __io_br();
> +       *(volatile unsigned long __force *)addr = val;
> +       __io_ar();
> +}
> +

I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
order to make sure that a series of reads/writes to a single CSR
register will not be reordered by the compiler.

Does __raw_readl/__raw_writel guarantee this property? If so, I could
drop my functions and use the system ones instead.

> +/*
> + * 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;
> +               write_pointer_with_barrier(
> +                       reg + (LITEX_REG_SIZE * i), shifted_data);
> +       }
> +
> +       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 = read_pointer_with_barrier(
> +                       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 (IS_ERR_OR_NULL(soc_ctrl_dev))
> +               return -ENOMEM;
> +
> +       soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR_OR_NULL(soc_ctrl_dev->base))
> +               return -EIO;
> +
> +       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 v4 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Greg Kroah-Hartman]

On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> >
> > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> >
> > This commit adds driver for the FPGA-based LiteX SoC
> > Controller from LiteX SoC builder.
> >
> > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > ---
> >
> > Notes:
> >     Changes in v4:
> >     - fixed indent in Kconfig's help section
> >     - fixed copyright header
> >     - changed compatible to "litex,soc-controller"
> >     - simplified litex_soc_ctrl_probe
> >     - removed unnecessary litex_soc_ctrl_remove
> >
> >     This commit has been introduced in v3 of the patchset.
> >
> >     It includes a simplified version of common 'litex.h'
> >     header introduced in v2 of the patchset.
> >
> >  MAINTAINERS                        |   2 +
> >  drivers/soc/Kconfig                |   1 +
> >  drivers/soc/Makefile               |   1 +
> >  drivers/soc/litex/Kconfig          |  14 ++
> >  drivers/soc/litex/Makefile         |   3 +
> >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> >  include/linux/litex.h              |  45 ++++++
> >  7 files changed, 283 insertions(+)
> >  create mode 100644 drivers/soc/litex/Kconfig
> >  create mode 100644 drivers/soc/litex/Makefile
> >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> >  create mode 100644 include/linux/litex.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2f5ede8a08aa..a35be1be90d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> >  M:     Mateusz Holenko <mholenko@antmicro.com>
> >  S:     Maintained
> >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> > +F:     drivers/soc/litex/litex_soc_ctrl.c
> > +F:     include/linux/litex.h
> >
> >  LIVE PATCHING
> >  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 1778f8c62861..78add2a163be 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> >  source "drivers/soc/imx/Kconfig"
> >  source "drivers/soc/ixp4xx/Kconfig"
> > +source "drivers/soc/litex/Kconfig"
> >  source "drivers/soc/mediatek/Kconfig"
> >  source "drivers/soc/qcom/Kconfig"
> >  source "drivers/soc/renesas/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 8b49d782a1ab..fd016b51cddd 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
> >  obj-$(CONFIG_ARCH_MXC)         += imx/
> >  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
> >  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> >  obj-y                          += mediatek/
> >  obj-y                          += amlogic/
> >  obj-y                          += qcom/
> > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > new file mode 100644
> > index 000000000000..71264c0e1d6c
> > --- /dev/null
> > +++ b/drivers/soc/litex/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +menu "Enable LiteX SoC Builder specific drivers"
> > +
> > +config LITEX_SOC_CONTROLLER
> > +       tristate "Enable LiteX SoC Controller driver"
> > +       help
> > +         This option enables the SoC Controller Driver which verifies
> > +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > +         accessors.
> > +         All drivers that use functions from litex.h must depend on
> > +         LITEX_SOC_CONTROLLER.
> > +
> > +endmenu
> > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > new file mode 100644
> > index 000000000000..98ff7325b1c0
> > --- /dev/null
> > +++ b/drivers/soc/litex/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > new file mode 100644
> > index 000000000000..5defba000fd4
> > --- /dev/null
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX SoC Controller Driver
> > + *
> > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > + *
> > + */
> > +
> > +#include <linux/litex.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/module.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +
> > +/*
> > + * The parameters below are true for LiteX SoC
> > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > + *
> > + * Supporting other configurations will require
> > + * extending the logic in this header.
> > + */
> > +#define LITEX_REG_SIZE             0x4
> > +#define LITEX_SUBREG_SIZE          0x1
> > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > +
> > +static DEFINE_SPINLOCK(csr_lock);
> > +
> > +static inline unsigned long read_pointer_with_barrier(
> > +       const volatile void __iomem *addr)
> > +{
> > +       unsigned long val;
> > +
> > +       __io_br();
> > +       val = *(const volatile unsigned long __force *)addr;
> > +       __io_ar();
> > +       return val;
> > +}
> > +
> > +static inline void write_pointer_with_barrier(
> > +       volatile void __iomem *addr, unsigned long val)
> > +{
> > +       __io_br();
> > +       *(volatile unsigned long __force *)addr = val;
> > +       __io_ar();
> > +}
> > +
> 
> I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> order to make sure that a series of reads/writes to a single CSR
> register will not be reordered by the compiler.

Please do not do this, there are core kernel calls for this, otherwise
this would be required by every individual driver, which would be crazy.

> Does __raw_readl/__raw_writel guarantee this property? If so, I could
> drop my functions and use the system ones instead.

Try it and see.  What's wrong with the normal iomem read/write
functions?

Also, just writing to a pointer like you did above is not how to do
this, please use the normal function calls, that way your driver will
work properly.

thanks,

greg k-h

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

[Mateusz Holenko]

On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> > On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > >
> > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > >
> > > This commit adds driver for the FPGA-based LiteX SoC
> > > Controller from LiteX SoC builder.
> > >
> > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > ---
> > >
> > > Notes:
> > >     Changes in v4:
> > >     - fixed indent in Kconfig's help section
> > >     - fixed copyright header
> > >     - changed compatible to "litex,soc-controller"
> > >     - simplified litex_soc_ctrl_probe
> > >     - removed unnecessary litex_soc_ctrl_remove
> > >
> > >     This commit has been introduced in v3 of the patchset.
> > >
> > >     It includes a simplified version of common 'litex.h'
> > >     header introduced in v2 of the patchset.
> > >
> > >  MAINTAINERS                        |   2 +
> > >  drivers/soc/Kconfig                |   1 +
> > >  drivers/soc/Makefile               |   1 +
> > >  drivers/soc/litex/Kconfig          |  14 ++
> > >  drivers/soc/litex/Makefile         |   3 +
> > >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> > >  include/linux/litex.h              |  45 ++++++
> > >  7 files changed, 283 insertions(+)
> > >  create mode 100644 drivers/soc/litex/Kconfig
> > >  create mode 100644 drivers/soc/litex/Makefile
> > >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> > >  create mode 100644 include/linux/litex.h
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index 2f5ede8a08aa..a35be1be90d5 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> > >  M:     Mateusz Holenko <mholenko@antmicro.com>
> > >  S:     Maintained
> > >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> > > +F:     drivers/soc/litex/litex_soc_ctrl.c
> > > +F:     include/linux/litex.h
> > >
> > >  LIVE PATCHING
> > >  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > > index 1778f8c62861..78add2a163be 100644
> > > --- a/drivers/soc/Kconfig
> > > +++ b/drivers/soc/Kconfig
> > > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> > >  source "drivers/soc/fsl/Kconfig"
> > >  source "drivers/soc/imx/Kconfig"
> > >  source "drivers/soc/ixp4xx/Kconfig"
> > > +source "drivers/soc/litex/Kconfig"
> > >  source "drivers/soc/mediatek/Kconfig"
> > >  source "drivers/soc/qcom/Kconfig"
> > >  source "drivers/soc/renesas/Kconfig"
> > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > > index 8b49d782a1ab..fd016b51cddd 100644
> > > --- a/drivers/soc/Makefile
> > > +++ b/drivers/soc/Makefile
> > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
> > >  obj-$(CONFIG_ARCH_MXC)         += imx/
> > >  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
> > >  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> > >  obj-y                          += mediatek/
> > >  obj-y                          += amlogic/
> > >  obj-y                          += qcom/
> > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > > new file mode 100644
> > > index 000000000000..71264c0e1d6c
> > > --- /dev/null
> > > +++ b/drivers/soc/litex/Kconfig
> > > @@ -0,0 +1,14 @@
> > > +# SPDX-License_Identifier: GPL-2.0
> > > +
> > > +menu "Enable LiteX SoC Builder specific drivers"
> > > +
> > > +config LITEX_SOC_CONTROLLER
> > > +       tristate "Enable LiteX SoC Controller driver"
> > > +       help
> > > +         This option enables the SoC Controller Driver which verifies
> > > +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > > +         accessors.
> > > +         All drivers that use functions from litex.h must depend on
> > > +         LITEX_SOC_CONTROLLER.
> > > +
> > > +endmenu
> > > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > > new file mode 100644
> > > index 000000000000..98ff7325b1c0
> > > --- /dev/null
> > > +++ b/drivers/soc/litex/Makefile
> > > @@ -0,0 +1,3 @@
> > > +# SPDX-License_Identifier: GPL-2.0
> > > +
> > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> > > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > > new file mode 100644
> > > index 000000000000..5defba000fd4
> > > --- /dev/null
> > > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > > @@ -0,0 +1,217 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * LiteX SoC Controller Driver
> > > + *
> > > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > > + *
> > > + */
> > > +
> > > +#include <linux/litex.h>
> > > +#include <linux/device.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/printk.h>
> > > +#include <linux/module.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/io.h>
> > > +
> > > +/*
> > > + * The parameters below are true for LiteX SoC
> > > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > > + *
> > > + * Supporting other configurations will require
> > > + * extending the logic in this header.
> > > + */
> > > +#define LITEX_REG_SIZE             0x4
> > > +#define LITEX_SUBREG_SIZE          0x1
> > > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > > +
> > > +static DEFINE_SPINLOCK(csr_lock);
> > > +
> > > +static inline unsigned long read_pointer_with_barrier(
> > > +       const volatile void __iomem *addr)
> > > +{
> > > +       unsigned long val;
> > > +
> > > +       __io_br();
> > > +       val = *(const volatile unsigned long __force *)addr;
> > > +       __io_ar();
> > > +       return val;
> > > +}
> > > +
> > > +static inline void write_pointer_with_barrier(
> > > +       volatile void __iomem *addr, unsigned long val)
> > > +{
> > > +       __io_br();
> > > +       *(volatile unsigned long __force *)addr = val;
> > > +       __io_ar();
> > > +}
> > > +
> >
> > I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> > order to make sure that a series of reads/writes to a single CSR
> > register will not be reordered by the compiler.
>
> Please do not do this, there are core kernel calls for this, otherwise
> this would be required by every individual driver, which would be crazy.
>
> > Does __raw_readl/__raw_writel guarantee this property? If so, I could
> > drop my functions and use the system ones instead.
>
> Try it and see.

Since I want to avoid read/write reordering caused by the compiler
optimizations I don't want to rely on a single manual test.
What I mean is that even if it works now for me, it does not guarantee
that it will in the future version of the compiler/using different
compilation flags/etc, right?

> What's wrong with the normal iomem read/write
> functions?

What I want to achieve here is to access the register in the CPU
"native" endianness and make sure that the value I see there is the
same as a predefined pattern.

LiteX is a soft SoC generator - it generates the logic of the whole
SoC (CPU+peripherals) in a form that can be later synthesized and
loaded onto the FPGA/turned into an ASIC/etc. Since it generates the
system as a whole, it gives guarantees on how those elements are
interconnected. It can generate CPUs of different architectures (some
of them being little-, other big-endiann) and I want to have a single
driver to target them all.

In this driver I just want to verify that the interconnection between
CPU and the peripheral is ok - I don't want to adjust dynamically
(i.e., translate endianness in case a mismatch is detected). If what I
see in the register is not what I expect it means that there is
something wrong in the design and the generator should be fixed.

I'm not using ioread32/iowrite32 functions as they reorder bytes
depending on the CPU endianness so the returned value might not
reflect the order of bytes read directly from the peripheral. I could
use ifdefs checking the value of __LITTLE_ENDIAN (and that's in fact
was what we started with), but
(a) it was discouraged in the previous round of the review,
(b) it requires more code - checking __LITTLE_ENDIAN and using
ioread32/ioread32be accordingly.

That's why I ended up with raw pointer access.

>
> Also, just writing to a pointer like you did above is not how to do
> this, please use the normal function calls, that way your driver will
> work properly.

Instead of accessing pointer directly I could call __raw_readl/__raw_writel
- is that what you mean?

>
> thanks,
>
> greg k-h

Thank you very much for the comments!

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

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

[Sam Ravnborg]

Hi Mateusz

A few drive-by comments - the soc area is not something I am well
versed in.

	Sam

On Thu, Apr 02, 2020 at 08:46:10AM +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.
> 
> Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> ---
> 
> Notes:
>     Changes in v4:
>     - fixed indent in Kconfig's help section
>     - fixed copyright header
>     - changed compatible to "litex,soc-controller"
>     - simplified litex_soc_ctrl_probe
>     - removed unnecessary litex_soc_ctrl_remove
> 
>     This commit has been introduced in v3 of the patchset.
> 
>     It includes a simplified version of common 'litex.h'
>     header introduced in v2 of the patchset.
> 
>  MAINTAINERS                        |   2 +
>  drivers/soc/Kconfig                |   1 +
>  drivers/soc/Makefile               |   1 +
>  drivers/soc/litex/Kconfig          |  14 ++
>  drivers/soc/litex/Makefile         |   3 +
>  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
>  include/linux/litex.h              |  45 ++++++
>  7 files changed, 283 insertions(+)
>  create mode 100644 drivers/soc/litex/Kconfig
>  create mode 100644 drivers/soc/litex/Makefile
>  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
>  create mode 100644 include/linux/litex.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2f5ede8a08aa..a35be1be90d5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9729,6 +9729,8 @@ M:	Karol Gugala <kgugala@antmicro.com>
>  M:	Mateusz Holenko <mholenko@antmicro.com>
>  S:	Maintained
>  F:	Documentation/devicetree/bindings/*/litex,*.yaml
> +F:	drivers/soc/litex/litex_soc_ctrl.c
> +F:	include/linux/litex.h
>  
>  LIVE PATCHING
>  M:	Josh Poimboeuf <jpoimboe@redhat.com>
> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> index 1778f8c62861..78add2a163be 100644
> --- a/drivers/soc/Kconfig
> +++ b/drivers/soc/Kconfig
> @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
>  source "drivers/soc/fsl/Kconfig"
>  source "drivers/soc/imx/Kconfig"
>  source "drivers/soc/ixp4xx/Kconfig"
> +source "drivers/soc/litex/Kconfig"
>  source "drivers/soc/mediatek/Kconfig"
>  source "drivers/soc/qcom/Kconfig"
>  source "drivers/soc/renesas/Kconfig"
> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> index 8b49d782a1ab..fd016b51cddd 100644
> --- a/drivers/soc/Makefile
> +++ b/drivers/soc/Makefile
> @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)	+= gemini/
>  obj-$(CONFIG_ARCH_MXC)		+= imx/
>  obj-$(CONFIG_ARCH_IXP4XX)	+= ixp4xx/
>  obj-$(CONFIG_SOC_XWAY)		+= lantiq/
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
>  obj-y				+= mediatek/
>  obj-y				+= amlogic/
>  obj-y				+= qcom/
> diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> new file mode 100644
> index 000000000000..71264c0e1d6c
> --- /dev/null
> +++ b/drivers/soc/litex/Kconfig
> @@ -0,0 +1,14 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +menu "Enable LiteX SoC Builder specific drivers"
> +
> +config LITEX_SOC_CONTROLLER
> +	tristate "Enable LiteX SoC Controller driver"
> +	help
> +	  This option enables the SoC Controller Driver which verifies
> +	  LiteX CSR access and provides common litex_get_reg/litex_set_reg
> +	  accessors.
> +	  All drivers that use functions from litex.h must depend on
> +	  LITEX_SOC_CONTROLLER.
> +
> +endmenu
> diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> new file mode 100644
> index 000000000000..98ff7325b1c0
> --- /dev/null
> +++ b/drivers/soc/litex/Makefile
> @@ -0,0 +1,3 @@
> +# SPDX-License_Identifier: GPL-2.0
> +
> +obj-$(CONFIG_LITEX_SOC_CONTROLLER)	+= litex_soc_ctrl.o
> diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> new file mode 100644
> index 000000000000..5defba000fd4
> --- /dev/null
> +++ b/drivers/soc/litex/litex_soc_ctrl.c
> @@ -0,0 +1,217 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * LiteX SoC Controller Driver
> + *
> + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> + *
> + */
> +
> +#include <linux/litex.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/printk.h>
> +#include <linux/module.h>
> +#include <linux/errno.h>
> +#include <linux/io.h>
> +
> +/*
> + * The parameters below are true for LiteX SoC
> + * configured for 8-bit CSR Bus, 32-bit aligned.
> + *
> + * Supporting other configurations will require
> + * extending the logic in this header.
> + */
> +#define LITEX_REG_SIZE             0x4
> +#define LITEX_SUBREG_SIZE          0x1
> +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> +
> +static DEFINE_SPINLOCK(csr_lock);
> +
> +static inline unsigned long read_pointer_with_barrier(
> +	const volatile void __iomem *addr)
> +{
> +	unsigned long val;
> +
> +	__io_br();
> +	val = *(const volatile unsigned long __force *)addr;
> +	__io_ar();
> +	return val;
> +}
This looks like an open-oced version of readl()
See include/asm-generic/io.h

> +
> +static inline void write_pointer_with_barrier(
> +	volatile void __iomem *addr, unsigned long val)
> +{
> +	__io_br();
> +	*(volatile unsigned long __force *)addr = val;
> +	__io_ar();
> +}
Likewise.

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

The typical linux style is:

void litex_set_reg(void __iomem *reg, unsigned long reg_size, unsigned long val)
{

And if the line is too long, then aling with tabs+spaces right after
first opening paranthese.

General comment for the remaining of the file.

> +	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;
> +		write_pointer_with_barrier(
> +			reg + (LITEX_REG_SIZE * i), shifted_data);
> +	}
> +
> +	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 = read_pointer_with_barrier(
> +			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 (IS_ERR_OR_NULL(soc_ctrl_dev))
> +		return -ENOMEM;
devm_kzalloc() either return NULL or allocated memory.
No need to do IS_ERR_OR_NULL()

> +
> +	soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR_OR_NULL(soc_ctrl_dev->base))
> +		return -EIO;
devm_platform_ioremap_resource does not return NUL on error.
So you loose the original error code here.

	Sam

> +
> +	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 v4 3/5] drivers/soc/litex: add LiteX SoC Controller driver

[Mateusz Holenko]

On Thu, Apr 9, 2020 at 9:45 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Mateusz
>
> A few drive-by comments - the soc area is not something I am well
> versed in.
>
>         Sam
>
> On Thu, Apr 02, 2020 at 08:46:10AM +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.
> >
> > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > ---
> >
> > Notes:
> >     Changes in v4:
> >     - fixed indent in Kconfig's help section
> >     - fixed copyright header
> >     - changed compatible to "litex,soc-controller"
> >     - simplified litex_soc_ctrl_probe
> >     - removed unnecessary litex_soc_ctrl_remove
> >
> >     This commit has been introduced in v3 of the patchset.
> >
> >     It includes a simplified version of common 'litex.h'
> >     header introduced in v2 of the patchset.
> >
> >  MAINTAINERS                        |   2 +
> >  drivers/soc/Kconfig                |   1 +
> >  drivers/soc/Makefile               |   1 +
> >  drivers/soc/litex/Kconfig          |  14 ++
> >  drivers/soc/litex/Makefile         |   3 +
> >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> >  include/linux/litex.h              |  45 ++++++
> >  7 files changed, 283 insertions(+)
> >  create mode 100644 drivers/soc/litex/Kconfig
> >  create mode 100644 drivers/soc/litex/Makefile
> >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> >  create mode 100644 include/linux/litex.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 2f5ede8a08aa..a35be1be90d5 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -9729,6 +9729,8 @@ M:      Karol Gugala <kgugala@antmicro.com>
> >  M:   Mateusz Holenko <mholenko@antmicro.com>
> >  S:   Maintained
> >  F:   Documentation/devicetree/bindings/*/litex,*.yaml
> > +F:   drivers/soc/litex/litex_soc_ctrl.c
> > +F:   include/linux/litex.h
> >
> >  LIVE PATCHING
> >  M:   Josh Poimboeuf <jpoimboe@redhat.com>
> > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > index 1778f8c62861..78add2a163be 100644
> > --- a/drivers/soc/Kconfig
> > +++ b/drivers/soc/Kconfig
> > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> >  source "drivers/soc/fsl/Kconfig"
> >  source "drivers/soc/imx/Kconfig"
> >  source "drivers/soc/ixp4xx/Kconfig"
> > +source "drivers/soc/litex/Kconfig"
> >  source "drivers/soc/mediatek/Kconfig"
> >  source "drivers/soc/qcom/Kconfig"
> >  source "drivers/soc/renesas/Kconfig"
> > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > index 8b49d782a1ab..fd016b51cddd 100644
> > --- a/drivers/soc/Makefile
> > +++ b/drivers/soc/Makefile
> > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)   += gemini/
> >  obj-$(CONFIG_ARCH_MXC)               += imx/
> >  obj-$(CONFIG_ARCH_IXP4XX)    += ixp4xx/
> >  obj-$(CONFIG_SOC_XWAY)               += lantiq/
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> >  obj-y                                += mediatek/
> >  obj-y                                += amlogic/
> >  obj-y                                += qcom/
> > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > new file mode 100644
> > index 000000000000..71264c0e1d6c
> > --- /dev/null
> > +++ b/drivers/soc/litex/Kconfig
> > @@ -0,0 +1,14 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +menu "Enable LiteX SoC Builder specific drivers"
> > +
> > +config LITEX_SOC_CONTROLLER
> > +     tristate "Enable LiteX SoC Controller driver"
> > +     help
> > +       This option enables the SoC Controller Driver which verifies
> > +       LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > +       accessors.
> > +       All drivers that use functions from litex.h must depend on
> > +       LITEX_SOC_CONTROLLER.
> > +
> > +endmenu
> > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > new file mode 100644
> > index 000000000000..98ff7325b1c0
> > --- /dev/null
> > +++ b/drivers/soc/litex/Makefile
> > @@ -0,0 +1,3 @@
> > +# SPDX-License_Identifier: GPL-2.0
> > +
> > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)   += litex_soc_ctrl.o
> > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > new file mode 100644
> > index 000000000000..5defba000fd4
> > --- /dev/null
> > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > @@ -0,0 +1,217 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * LiteX SoC Controller Driver
> > + *
> > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > + *
> > + */
> > +
> > +#include <linux/litex.h>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/printk.h>
> > +#include <linux/module.h>
> > +#include <linux/errno.h>
> > +#include <linux/io.h>
> > +
> > +/*
> > + * The parameters below are true for LiteX SoC
> > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > + *
> > + * Supporting other configurations will require
> > + * extending the logic in this header.
> > + */
> > +#define LITEX_REG_SIZE             0x4
> > +#define LITEX_SUBREG_SIZE          0x1
> > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > +
> > +static DEFINE_SPINLOCK(csr_lock);
> > +
> > +static inline unsigned long read_pointer_with_barrier(
> > +     const volatile void __iomem *addr)
> > +{
> > +     unsigned long val;
> > +
> > +     __io_br();
> > +     val = *(const volatile unsigned long __force *)addr;
> > +     __io_ar();
> > +     return val;
> > +}
> This looks like an open-oced version of readl()
> See include/asm-generic/io.h

That's right as this function is designed after readl().

The difference is that I need to read the "raw" value,
i.e., I don't want to pass it through __le32_to_cpu()
- as described in previous comments I want to verify
endianness of the data read from the register.

I could use __raw_readl(), but this one alone does
not provide memory barriers.

> > +
> > +static inline void write_pointer_with_barrier(
> > +     volatile void __iomem *addr, unsigned long val)
> > +{
> > +     __io_br();
> > +     *(volatile unsigned long __force *)addr = val;
> > +     __io_ar();
> > +}
> Likewise.
>
> > +
> > +/*
> > + * 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)
> > +{
>
> The typical linux style is:
>
> void litex_set_reg(void __iomem *reg, unsigned long reg_size, unsigned long val)
> {
>
> And if the line is too long, then aling with tabs+spaces right after
> first opening paranthese.

Right. This notation was because I wanted to fit into the line-length limit.
I'll change it to:

void litex_set_reg(void __iomem *reg,
                             unsigned long reg_size, unsigned long val)
{

> General comment for the remaining of the file.
>
> > +     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;
> > +             write_pointer_with_barrier(
> > +                     reg + (LITEX_REG_SIZE * i), shifted_data);
> > +     }
> > +
> > +     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 = read_pointer_with_barrier(
> > +                     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 (IS_ERR_OR_NULL(soc_ctrl_dev))
> > +             return -ENOMEM;
> devm_kzalloc() either return NULL or allocated memory.
> No need to do IS_ERR_OR_NULL()

I'll simplify this.

> > +
> > +     soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
> > +     if (IS_ERR_OR_NULL(soc_ctrl_dev->base))
> > +             return -EIO;
> devm_platform_ioremap_resource does not return NUL on error.
> So you loose the original error code here.

I'll change this to:

if (IS_ERR(soc_ctrl_dev->base))
    return PTR_ERR(soc_ctrl_dev->base);


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

Thanks for all the comments!


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

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

[Rob Herring]

On Thu, 2 Apr 2020 08:45:54 +0200, Mateusz Holenko 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>
> ---
> 
> Notes:
>     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
> 

Reviewed-by: Rob Herring <robh@kernel.org>

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

[Greg Kroah-Hartman]

On Thu, Apr 02, 2020 at 03:50:34PM +0200, Mateusz Holenko wrote:
> On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> > > On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > >
> > > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > >
> > > > This commit adds driver for the FPGA-based LiteX SoC
> > > > Controller from LiteX SoC builder.
> > > >
> > > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > > ---
> > > >
> > > > Notes:
> > > >     Changes in v4:
> > > >     - fixed indent in Kconfig's help section
> > > >     - fixed copyright header
> > > >     - changed compatible to "litex,soc-controller"
> > > >     - simplified litex_soc_ctrl_probe
> > > >     - removed unnecessary litex_soc_ctrl_remove
> > > >
> > > >     This commit has been introduced in v3 of the patchset.
> > > >
> > > >     It includes a simplified version of common 'litex.h'
> > > >     header introduced in v2 of the patchset.
> > > >
> > > >  MAINTAINERS                        |   2 +
> > > >  drivers/soc/Kconfig                |   1 +
> > > >  drivers/soc/Makefile               |   1 +
> > > >  drivers/soc/litex/Kconfig          |  14 ++
> > > >  drivers/soc/litex/Makefile         |   3 +
> > > >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> > > >  include/linux/litex.h              |  45 ++++++
> > > >  7 files changed, 283 insertions(+)
> > > >  create mode 100644 drivers/soc/litex/Kconfig
> > > >  create mode 100644 drivers/soc/litex/Makefile
> > > >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> > > >  create mode 100644 include/linux/litex.h
> > > >
> > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > index 2f5ede8a08aa..a35be1be90d5 100644
> > > > --- a/MAINTAINERS
> > > > +++ b/MAINTAINERS
> > > > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> > > >  M:     Mateusz Holenko <mholenko@antmicro.com>
> > > >  S:     Maintained
> > > >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> > > > +F:     drivers/soc/litex/litex_soc_ctrl.c
> > > > +F:     include/linux/litex.h
> > > >
> > > >  LIVE PATCHING
> > > >  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> > > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > > > index 1778f8c62861..78add2a163be 100644
> > > > --- a/drivers/soc/Kconfig
> > > > +++ b/drivers/soc/Kconfig
> > > > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> > > >  source "drivers/soc/fsl/Kconfig"
> > > >  source "drivers/soc/imx/Kconfig"
> > > >  source "drivers/soc/ixp4xx/Kconfig"
> > > > +source "drivers/soc/litex/Kconfig"
> > > >  source "drivers/soc/mediatek/Kconfig"
> > > >  source "drivers/soc/qcom/Kconfig"
> > > >  source "drivers/soc/renesas/Kconfig"
> > > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > > > index 8b49d782a1ab..fd016b51cddd 100644
> > > > --- a/drivers/soc/Makefile
> > > > +++ b/drivers/soc/Makefile
> > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
> > > >  obj-$(CONFIG_ARCH_MXC)         += imx/
> > > >  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
> > > >  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> > > >  obj-y                          += mediatek/
> > > >  obj-y                          += amlogic/
> > > >  obj-y                          += qcom/
> > > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > > > new file mode 100644
> > > > index 000000000000..71264c0e1d6c
> > > > --- /dev/null
> > > > +++ b/drivers/soc/litex/Kconfig
> > > > @@ -0,0 +1,14 @@
> > > > +# SPDX-License_Identifier: GPL-2.0
> > > > +
> > > > +menu "Enable LiteX SoC Builder specific drivers"
> > > > +
> > > > +config LITEX_SOC_CONTROLLER
> > > > +       tristate "Enable LiteX SoC Controller driver"
> > > > +       help
> > > > +         This option enables the SoC Controller Driver which verifies
> > > > +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > > > +         accessors.
> > > > +         All drivers that use functions from litex.h must depend on
> > > > +         LITEX_SOC_CONTROLLER.
> > > > +
> > > > +endmenu
> > > > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > > > new file mode 100644
> > > > index 000000000000..98ff7325b1c0
> > > > --- /dev/null
> > > > +++ b/drivers/soc/litex/Makefile
> > > > @@ -0,0 +1,3 @@
> > > > +# SPDX-License_Identifier: GPL-2.0
> > > > +
> > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> > > > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > > > new file mode 100644
> > > > index 000000000000..5defba000fd4
> > > > --- /dev/null
> > > > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > > > @@ -0,0 +1,217 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/*
> > > > + * LiteX SoC Controller Driver
> > > > + *
> > > > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > > > + *
> > > > + */
> > > > +
> > > > +#include <linux/litex.h>
> > > > +#include <linux/device.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/printk.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/errno.h>
> > > > +#include <linux/io.h>
> > > > +
> > > > +/*
> > > > + * The parameters below are true for LiteX SoC
> > > > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > > > + *
> > > > + * Supporting other configurations will require
> > > > + * extending the logic in this header.
> > > > + */
> > > > +#define LITEX_REG_SIZE             0x4
> > > > +#define LITEX_SUBREG_SIZE          0x1
> > > > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > > > +
> > > > +static DEFINE_SPINLOCK(csr_lock);
> > > > +
> > > > +static inline unsigned long read_pointer_with_barrier(
> > > > +       const volatile void __iomem *addr)
> > > > +{
> > > > +       unsigned long val;
> > > > +
> > > > +       __io_br();
> > > > +       val = *(const volatile unsigned long __force *)addr;
> > > > +       __io_ar();
> > > > +       return val;
> > > > +}
> > > > +
> > > > +static inline void write_pointer_with_barrier(
> > > > +       volatile void __iomem *addr, unsigned long val)
> > > > +{
> > > > +       __io_br();
> > > > +       *(volatile unsigned long __force *)addr = val;
> > > > +       __io_ar();
> > > > +}
> > > > +
> > >
> > > I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> > > order to make sure that a series of reads/writes to a single CSR
> > > register will not be reordered by the compiler.
> >
> > Please do not do this, there are core kernel calls for this, otherwise
> > this would be required by every individual driver, which would be crazy.
> >
> > > Does __raw_readl/__raw_writel guarantee this property? If so, I could
> > > drop my functions and use the system ones instead.
> >
> > Try it and see.
> 
> Since I want to avoid read/write reordering caused by the compiler
> optimizations I don't want to rely on a single manual test.
> What I mean is that even if it works now for me, it does not guarantee
> that it will in the future version of the compiler/using different
> compilation flags/etc, right?

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

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

thanks,

greg k-h

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

[Mateusz Holenko]

On Thu, Apr 16, 2020 at 4:18 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, Apr 02, 2020 at 03:50:34PM +0200, Mateusz Holenko wrote:
> > On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> > > > On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > > >
> > > > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > > >
> > > > > This commit adds driver for the FPGA-based LiteX SoC
> > > > > Controller from LiteX SoC builder.
> > > > >
> > > > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > > > ---
> > > > >
> > > > > Notes:
> > > > >     Changes in v4:
> > > > >     - fixed indent in Kconfig's help section
> > > > >     - fixed copyright header
> > > > >     - changed compatible to "litex,soc-controller"
> > > > >     - simplified litex_soc_ctrl_probe
> > > > >     - removed unnecessary litex_soc_ctrl_remove
> > > > >
> > > > >     This commit has been introduced in v3 of the patchset.
> > > > >
> > > > >     It includes a simplified version of common 'litex.h'
> > > > >     header introduced in v2 of the patchset.
> > > > >
> > > > >  MAINTAINERS                        |   2 +
> > > > >  drivers/soc/Kconfig                |   1 +
> > > > >  drivers/soc/Makefile               |   1 +
> > > > >  drivers/soc/litex/Kconfig          |  14 ++
> > > > >  drivers/soc/litex/Makefile         |   3 +
> > > > >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> > > > >  include/linux/litex.h              |  45 ++++++
> > > > >  7 files changed, 283 insertions(+)
> > > > >  create mode 100644 drivers/soc/litex/Kconfig
> > > > >  create mode 100644 drivers/soc/litex/Makefile
> > > > >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> > > > >  create mode 100644 include/linux/litex.h
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > index 2f5ede8a08aa..a35be1be90d5 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> > > > >  M:     Mateusz Holenko <mholenko@antmicro.com>
> > > > >  S:     Maintained
> > > > >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> > > > > +F:     drivers/soc/litex/litex_soc_ctrl.c
> > > > > +F:     include/linux/litex.h
> > > > >
> > > > >  LIVE PATCHING
> > > > >  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > > > > index 1778f8c62861..78add2a163be 100644
> > > > > --- a/drivers/soc/Kconfig
> > > > > +++ b/drivers/soc/Kconfig
> > > > > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> > > > >  source "drivers/soc/fsl/Kconfig"
> > > > >  source "drivers/soc/imx/Kconfig"
> > > > >  source "drivers/soc/ixp4xx/Kconfig"
> > > > > +source "drivers/soc/litex/Kconfig"
> > > > >  source "drivers/soc/mediatek/Kconfig"
> > > > >  source "drivers/soc/qcom/Kconfig"
> > > > >  source "drivers/soc/renesas/Kconfig"
> > > > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > > > > index 8b49d782a1ab..fd016b51cddd 100644
> > > > > --- a/drivers/soc/Makefile
> > > > > +++ b/drivers/soc/Makefile
> > > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
> > > > >  obj-$(CONFIG_ARCH_MXC)         += imx/
> > > > >  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
> > > > >  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> > > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> > > > >  obj-y                          += mediatek/
> > > > >  obj-y                          += amlogic/
> > > > >  obj-y                          += qcom/
> > > > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > > > > new file mode 100644
> > > > > index 000000000000..71264c0e1d6c
> > > > > --- /dev/null
> > > > > +++ b/drivers/soc/litex/Kconfig
> > > > > @@ -0,0 +1,14 @@
> > > > > +# SPDX-License_Identifier: GPL-2.0
> > > > > +
> > > > > +menu "Enable LiteX SoC Builder specific drivers"
> > > > > +
> > > > > +config LITEX_SOC_CONTROLLER
> > > > > +       tristate "Enable LiteX SoC Controller driver"
> > > > > +       help
> > > > > +         This option enables the SoC Controller Driver which verifies
> > > > > +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > > > > +         accessors.
> > > > > +         All drivers that use functions from litex.h must depend on
> > > > > +         LITEX_SOC_CONTROLLER.
> > > > > +
> > > > > +endmenu
> > > > > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > > > > new file mode 100644
> > > > > index 000000000000..98ff7325b1c0
> > > > > --- /dev/null
> > > > > +++ b/drivers/soc/litex/Makefile
> > > > > @@ -0,0 +1,3 @@
> > > > > +# SPDX-License_Identifier: GPL-2.0
> > > > > +
> > > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> > > > > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > > > > new file mode 100644
> > > > > index 000000000000..5defba000fd4
> > > > > --- /dev/null
> > > > > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > > > > @@ -0,0 +1,217 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/*
> > > > > + * LiteX SoC Controller Driver
> > > > > + *
> > > > > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > > > > + *
> > > > > + */
> > > > > +
> > > > > +#include <linux/litex.h>
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/errno.h>
> > > > > +#include <linux/of.h>
> > > > > +#include <linux/of_platform.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/printk.h>
> > > > > +#include <linux/module.h>
> > > > > +#include <linux/errno.h>
> > > > > +#include <linux/io.h>
> > > > > +
> > > > > +/*
> > > > > + * The parameters below are true for LiteX SoC
> > > > > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > > > > + *
> > > > > + * Supporting other configurations will require
> > > > > + * extending the logic in this header.
> > > > > + */
> > > > > +#define LITEX_REG_SIZE             0x4
> > > > > +#define LITEX_SUBREG_SIZE          0x1
> > > > > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > > > > +
> > > > > +static DEFINE_SPINLOCK(csr_lock);
> > > > > +
> > > > > +static inline unsigned long read_pointer_with_barrier(
> > > > > +       const volatile void __iomem *addr)
> > > > > +{
> > > > > +       unsigned long val;
> > > > > +
> > > > > +       __io_br();
> > > > > +       val = *(const volatile unsigned long __force *)addr;
> > > > > +       __io_ar();
> > > > > +       return val;
> > > > > +}
> > > > > +
> > > > > +static inline void write_pointer_with_barrier(
> > > > > +       volatile void __iomem *addr, unsigned long val)
> > > > > +{
> > > > > +       __io_br();
> > > > > +       *(volatile unsigned long __force *)addr = val;
> > > > > +       __io_ar();
> > > > > +}
> > > > > +
> > > >
> > > > I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> > > > order to make sure that a series of reads/writes to a single CSR
> > > > register will not be reordered by the compiler.
> > >
> > > Please do not do this, there are core kernel calls for this, otherwise
> > > this would be required by every individual driver, which would be crazy.
> > >
> > > > Does __raw_readl/__raw_writel guarantee this property? If so, I could
> > > > drop my functions and use the system ones instead.
> > >
> > > Try it and see.
> >
> > Since I want to avoid read/write reordering caused by the compiler
> > optimizations I don't want to rely on a single manual test.
> > What I mean is that even if it works now for me, it does not guarantee
> > that it will in the future version of the compiler/using different
> > compilation flags/etc, right?
>
> No, if the common functions stop working, then they will be fixed.  If
> you try to roll your own and they stop working in the future, no one
> will notice.

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

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

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

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

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

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

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

I'm fully open for suggestions.

> thanks,
>
> greg k-h

Thanks again for your time and the comments!

Best,
Mateusz

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

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

[Greg Kroah-Hartman]

On Tue, Apr 21, 2020 at 10:29:33AM +0200, Mateusz Holenko wrote:
> On Thu, Apr 16, 2020 at 4:18 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, Apr 02, 2020 at 03:50:34PM +0200, Mateusz Holenko wrote:
> > > On Thu, Apr 2, 2020 at 9:43 AM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, Apr 02, 2020 at 08:50:40AM +0200, Mateusz Holenko wrote:
> > > > > On Thu, Apr 2, 2020 at 8:46 AM Mateusz Holenko <mholenko@antmicro.com> wrote:
> > > > > >
> > > > > > From: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > > > >
> > > > > > This commit adds driver for the FPGA-based LiteX SoC
> > > > > > Controller from LiteX SoC builder.
> > > > > >
> > > > > > Co-developed-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > > > Signed-off-by: Mateusz Holenko <mholenko@antmicro.com>
> > > > > > Signed-off-by: Pawel Czarnecki <pczarnecki@internships.antmicro.com>
> > > > > > ---
> > > > > >
> > > > > > Notes:
> > > > > >     Changes in v4:
> > > > > >     - fixed indent in Kconfig's help section
> > > > > >     - fixed copyright header
> > > > > >     - changed compatible to "litex,soc-controller"
> > > > > >     - simplified litex_soc_ctrl_probe
> > > > > >     - removed unnecessary litex_soc_ctrl_remove
> > > > > >
> > > > > >     This commit has been introduced in v3 of the patchset.
> > > > > >
> > > > > >     It includes a simplified version of common 'litex.h'
> > > > > >     header introduced in v2 of the patchset.
> > > > > >
> > > > > >  MAINTAINERS                        |   2 +
> > > > > >  drivers/soc/Kconfig                |   1 +
> > > > > >  drivers/soc/Makefile               |   1 +
> > > > > >  drivers/soc/litex/Kconfig          |  14 ++
> > > > > >  drivers/soc/litex/Makefile         |   3 +
> > > > > >  drivers/soc/litex/litex_soc_ctrl.c | 217 +++++++++++++++++++++++++++++
> > > > > >  include/linux/litex.h              |  45 ++++++
> > > > > >  7 files changed, 283 insertions(+)
> > > > > >  create mode 100644 drivers/soc/litex/Kconfig
> > > > > >  create mode 100644 drivers/soc/litex/Makefile
> > > > > >  create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
> > > > > >  create mode 100644 include/linux/litex.h
> > > > > >
> > > > > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > > > > index 2f5ede8a08aa..a35be1be90d5 100644
> > > > > > --- a/MAINTAINERS
> > > > > > +++ b/MAINTAINERS
> > > > > > @@ -9729,6 +9729,8 @@ M:        Karol Gugala <kgugala@antmicro.com>
> > > > > >  M:     Mateusz Holenko <mholenko@antmicro.com>
> > > > > >  S:     Maintained
> > > > > >  F:     Documentation/devicetree/bindings/*/litex,*.yaml
> > > > > > +F:     drivers/soc/litex/litex_soc_ctrl.c
> > > > > > +F:     include/linux/litex.h
> > > > > >
> > > > > >  LIVE PATCHING
> > > > > >  M:     Josh Poimboeuf <jpoimboe@redhat.com>
> > > > > > diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
> > > > > > index 1778f8c62861..78add2a163be 100644
> > > > > > --- a/drivers/soc/Kconfig
> > > > > > +++ b/drivers/soc/Kconfig
> > > > > > @@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
> > > > > >  source "drivers/soc/fsl/Kconfig"
> > > > > >  source "drivers/soc/imx/Kconfig"
> > > > > >  source "drivers/soc/ixp4xx/Kconfig"
> > > > > > +source "drivers/soc/litex/Kconfig"
> > > > > >  source "drivers/soc/mediatek/Kconfig"
> > > > > >  source "drivers/soc/qcom/Kconfig"
> > > > > >  source "drivers/soc/renesas/Kconfig"
> > > > > > diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
> > > > > > index 8b49d782a1ab..fd016b51cddd 100644
> > > > > > --- a/drivers/soc/Makefile
> > > > > > +++ b/drivers/soc/Makefile
> > > > > > @@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)     += gemini/
> > > > > >  obj-$(CONFIG_ARCH_MXC)         += imx/
> > > > > >  obj-$(CONFIG_ARCH_IXP4XX)      += ixp4xx/
> > > > > >  obj-$(CONFIG_SOC_XWAY)         += lantiq/
> > > > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
> > > > > >  obj-y                          += mediatek/
> > > > > >  obj-y                          += amlogic/
> > > > > >  obj-y                          += qcom/
> > > > > > diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
> > > > > > new file mode 100644
> > > > > > index 000000000000..71264c0e1d6c
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/soc/litex/Kconfig
> > > > > > @@ -0,0 +1,14 @@
> > > > > > +# SPDX-License_Identifier: GPL-2.0
> > > > > > +
> > > > > > +menu "Enable LiteX SoC Builder specific drivers"
> > > > > > +
> > > > > > +config LITEX_SOC_CONTROLLER
> > > > > > +       tristate "Enable LiteX SoC Controller driver"
> > > > > > +       help
> > > > > > +         This option enables the SoC Controller Driver which verifies
> > > > > > +         LiteX CSR access and provides common litex_get_reg/litex_set_reg
> > > > > > +         accessors.
> > > > > > +         All drivers that use functions from litex.h must depend on
> > > > > > +         LITEX_SOC_CONTROLLER.
> > > > > > +
> > > > > > +endmenu
> > > > > > diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
> > > > > > new file mode 100644
> > > > > > index 000000000000..98ff7325b1c0
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/soc/litex/Makefile
> > > > > > @@ -0,0 +1,3 @@
> > > > > > +# SPDX-License_Identifier: GPL-2.0
> > > > > > +
> > > > > > +obj-$(CONFIG_LITEX_SOC_CONTROLLER)     += litex_soc_ctrl.o
> > > > > > diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..5defba000fd4
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/soc/litex/litex_soc_ctrl.c
> > > > > > @@ -0,0 +1,217 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/*
> > > > > > + * LiteX SoC Controller Driver
> > > > > > + *
> > > > > > + * Copyright (C) 2020 Antmicro <www.antmicro.com>
> > > > > > + *
> > > > > > + */
> > > > > > +
> > > > > > +#include <linux/litex.h>
> > > > > > +#include <linux/device.h>
> > > > > > +#include <linux/errno.h>
> > > > > > +#include <linux/of.h>
> > > > > > +#include <linux/of_platform.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/printk.h>
> > > > > > +#include <linux/module.h>
> > > > > > +#include <linux/errno.h>
> > > > > > +#include <linux/io.h>
> > > > > > +
> > > > > > +/*
> > > > > > + * The parameters below are true for LiteX SoC
> > > > > > + * configured for 8-bit CSR Bus, 32-bit aligned.
> > > > > > + *
> > > > > > + * Supporting other configurations will require
> > > > > > + * extending the logic in this header.
> > > > > > + */
> > > > > > +#define LITEX_REG_SIZE             0x4
> > > > > > +#define LITEX_SUBREG_SIZE          0x1
> > > > > > +#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
> > > > > > +
> > > > > > +static DEFINE_SPINLOCK(csr_lock);
> > > > > > +
> > > > > > +static inline unsigned long read_pointer_with_barrier(
> > > > > > +       const volatile void __iomem *addr)
> > > > > > +{
> > > > > > +       unsigned long val;
> > > > > > +
> > > > > > +       __io_br();
> > > > > > +       val = *(const volatile unsigned long __force *)addr;
> > > > > > +       __io_ar();
> > > > > > +       return val;
> > > > > > +}
> > > > > > +
> > > > > > +static inline void write_pointer_with_barrier(
> > > > > > +       volatile void __iomem *addr, unsigned long val)
> > > > > > +{
> > > > > > +       __io_br();
> > > > > > +       *(volatile unsigned long __force *)addr = val;
> > > > > > +       __io_ar();
> > > > > > +}
> > > > > > +
> > > > >
> > > > > I'm defining read_pointer_with_barrier/write_pointer_with_barrier in
> > > > > order to make sure that a series of reads/writes to a single CSR
> > > > > register will not be reordered by the compiler.
> > > >
> > > > Please do not do this, there are core kernel calls for this, otherwise
> > > > this would be required by every individual driver, which would be crazy.
> > > >
> > > > > Does __raw_readl/__raw_writel guarantee this property? If so, I could
> > > > > drop my functions and use the system ones instead.
> > > >
> > > > Try it and see.
> > >
> > > Since I want to avoid read/write reordering caused by the compiler
> > > optimizations I don't want to rely on a single manual test.
> > > What I mean is that even if it works now for me, it does not guarantee
> > > that it will in the future version of the compiler/using different
> > > compilation flags/etc, right?
> >
> > No, if the common functions stop working, then they will be fixed.  If
> > you try to roll your own and they stop working in the future, no one
> > will notice.
> 
> Sure, no doubts here. What I wanted to say though was that I want to
> protect the code against compiler optimizations (code reordering) that
> I might not be able to detect using just a one-time test. It has nothing to
> do with bugs in common functions, only with the compiler itself (and again
> it's not a bug).
> The way optimizations are handled is dependent on the compiler
> and the configuration and I'm manually testing just one of many possible
> setups.
> 
> I also checked that there are explicit memory barriers used in the
> code of readl(),
> so I assume __raw_readl()/__raw_writel() does not guarantee ordering alone
> (as otherwise __io_br() wouldn't be used in readl()).

Correct, you have "open coded" the existing readX() functions here, just
use them.

thanks,

greg k-h