Hi Jonathan,

thanks for your review!

On Wed, Sep 23, 2020 at 1:58 PM Jonathan Cameron
[off-list ref] wrote:

On Wed, 23 Sep 2020 12:09:06 +0200
Mateusz Holenko [off-list ref] wrote:

quoted

From: Pawel Czarnecki <redacted>

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

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

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

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

Jonathan

quoted

---

Notes:
    Changes in v11:
    - removed an unnecessary comment left over from previous version
    - changed a multi-line comment to comply with the formatting rules
    - use WARN instad of BUG on a failed CSR validation

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

    Changes in v9:
    - added exporting of the `litex_set_reg`/`litex_get_reg` symbols

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

    No changes in v7.

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

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

    Changes in v4:
    - fixed indent in Kconfig's help section
    - fixed copyright header
    - changed compatible to "litex,soc-controller"
    - simplified litex_soc_ctrl_probe
    - removed unnecessary litex_soc_ctrl_remove

    This commit has been introduced in v3 of the patchset.

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

 MAINTAINERS                        |   2 +
 drivers/soc/Kconfig                |   1 +
 drivers/soc/Makefile               |   1 +
 drivers/soc/litex/Kconfig          |  15 +++
 drivers/soc/litex/Makefile         |   3 +
 drivers/soc/litex/litex_soc_ctrl.c | 194 +++++++++++++++++++++++++++++
 include/linux/litex.h              |  24 ++++
 7 files changed, 240 insertions(+)
 create mode 100644 drivers/soc/litex/Kconfig
 create mode 100644 drivers/soc/litex/Makefile
 create mode 100644 drivers/soc/litex/litex_soc_ctrl.c
 create mode 100644 include/linux/litex.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 39be98db7418..4d70a1b22a87 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -9840,6 +9840,8 @@ M:      Karol Gugala <kgugala@antmicro.com>
 M:   Mateusz Holenko <mholenko@antmicro.com>
 S:   Maintained
 F:   Documentation/devicetree/bindings/*/litex,*.yaml
+F:   drivers/soc/litex/litex_soc_ctrl.c
+F:   include/linux/litex.h

 LIVE PATCHING
 M:   Josh Poimboeuf <jpoimboe@redhat.com>

diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 425ab6f7e375..d097d070f579 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig

@@ -9,6 +9,7 @@ source "drivers/soc/bcm/Kconfig"
 source "drivers/soc/fsl/Kconfig"
 source "drivers/soc/imx/Kconfig"
 source "drivers/soc/ixp4xx/Kconfig"
+source "drivers/soc/litex/Kconfig"
 source "drivers/soc/mediatek/Kconfig"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/renesas/Kconfig"

diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 36452bed86ef..0b16108823ef 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile

@@ -14,6 +14,7 @@ obj-$(CONFIG_ARCH_GEMINI)   += gemini/
 obj-y                                += imx/
 obj-$(CONFIG_ARCH_IXP4XX)    += ixp4xx/
 obj-$(CONFIG_SOC_XWAY)               += lantiq/
+obj-$(CONFIG_LITEX_SOC_CONTROLLER) += litex/
 obj-y                                += mediatek/
 obj-y                                += amlogic/
 obj-y                                += qcom/

diff --git a/drivers/soc/litex/Kconfig b/drivers/soc/litex/Kconfig
new file mode 100644
index 000000000000..c974ec3846bc
--- /dev/null
+++ b/drivers/soc/litex/Kconfig

@@ -0,0 +1,15 @@
+# SPDX-License_Identifier: GPL-2.0
+
+menu "Enable LiteX SoC Builder specific drivers"
+
+config LITEX_SOC_CONTROLLER
+     tristate "Enable LiteX SoC Controller driver"
+     depends on OF || COMPILE_TEST
+     help
+       This option enables the SoC Controller Driver which verifies
+       LiteX CSR access and provides common litex_get_reg/litex_set_reg
+       accessors.
+       All drivers that use functions from litex.h must depend on
+       LITEX_SOC_CONTROLLER.
+
+endmenu

diff --git a/drivers/soc/litex/Makefile b/drivers/soc/litex/Makefile
new file mode 100644
index 000000000000..98ff7325b1c0
--- /dev/null
+++ b/drivers/soc/litex/Makefile

@@ -0,0 +1,3 @@
+# SPDX-License_Identifier: GPL-2.0
+
+obj-$(CONFIG_LITEX_SOC_CONTROLLER)   += litex_soc_ctrl.o

diff --git a/drivers/soc/litex/litex_soc_ctrl.c b/drivers/soc/litex/litex_soc_ctrl.c
new file mode 100644
index 000000000000..08330c9872b0
--- /dev/null
+++ b/drivers/soc/litex/litex_soc_ctrl.c

@@ -0,0 +1,194 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * LiteX SoC Controller Driver
+ *
+ * Copyright (C) 2020 Antmicro <www.antmicro.com>
+ *
+ */
+
+#include <linux/litex.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>

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

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

quoted

+#include <linux/platform_device.h>
+#include <linux/printk.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/io.h>
+
+/*
+ * The parameters below are true for LiteX SoC
+ * configured for 8-bit CSR Bus, 32-bit aligned.
+ *
+ * Supporting other configurations will require
+ * extending the logic in this header.
+ */
+#define LITEX_REG_SIZE             0x4
+#define LITEX_SUBREG_SIZE          0x1
+#define LITEX_SUBREG_SIZE_BIT      (LITEX_SUBREG_SIZE * 8)
+
+static DEFINE_SPINLOCK(csr_lock);
+
+/*
+ * LiteX SoC Generator, depending on the configuration,
+ * can split a single logical CSR (Control & Status Register)
+ * into a series of consecutive physical registers.
+ *
+ * For example, in the configuration with 8-bit CSR Bus,
+ * 32-bit aligned (the default one for 32-bit CPUs) a 32-bit
+ * logical CSR will be generated as four 32-bit physical registers,
+ * each one containing one byte of meaningful data.
+ *
+ * For details see: https://github.com/enjoy-digital/litex/wiki/CSR-Bus
+ *
+ * The purpose of `litex_set_reg`/`litex_get_reg` is to implement
+ * the logic of writing to/reading from the LiteX CSR in a single
+ * place that can be then reused by all LiteX drivers.
+ */

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

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

quoted

+void litex_set_reg(void __iomem *reg, unsigned long reg_size,
+                 unsigned long val)
+{
+     unsigned long shifted_data, shift, i;
+     unsigned long flags;
+
+     spin_lock_irqsave(&csr_lock, flags);
+
+     for (i = 0; i < reg_size; ++i) {
+             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
+             shifted_data = val >> shift;
+
+             writel((u32 __force)cpu_to_le32(shifted_data), reg + (LITEX_REG_SIZE * i));
+     }
+
+     spin_unlock_irqrestore(&csr_lock, flags);
+}
+EXPORT_SYMBOL_GPL(litex_set_reg);
+
+unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_size)
+{
+     unsigned long shifted_data, shift, i;
+     unsigned long result = 0;
+     unsigned long flags;
+
+     spin_lock_irqsave(&csr_lock, flags);
+
+     for (i = 0; i < reg_size; ++i) {
+             shifted_data = le32_to_cpu((__le32 __force)readl(reg + (LITEX_REG_SIZE * i)));
+
+             shift = ((reg_size - i - 1) * LITEX_SUBREG_SIZE_BIT);
+             result |= (shifted_data << shift);
+     }
+
+     spin_unlock_irqrestore(&csr_lock, flags);
+
+     return result;
+}
+EXPORT_SYMBOL_GPL(litex_get_reg);
+
+#define SCRATCH_REG_OFF         0x04
+#define SCRATCH_REG_SIZE        4
+#define SCRATCH_REG_VALUE       0x12345678
+#define SCRATCH_TEST_VALUE      0xdeadbeef
+
+/*
+ * Check LiteX CSR read/write access
+ *
+ * This function reads and writes a scratch register in order
+ * to verify if CSR access works.
+ *
+ * In case any problems are detected, the driver should panic.
+ *
+ * Access to the LiteX CSR is, by design, done in CPU native
+ * endianness. The driver should not dynamically configure
+ * access functions when the endianness mismatch is detected.
+ * Such situation indicates problems in the soft SoC design
+ * and should be solved at the LiteX generator level,
+ * not in the software.
+ */
+static int litex_check_csr_access(void __iomem *reg_addr)
+{
+     unsigned long reg;
+
+     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
+
+     if (reg != SCRATCH_REG_VALUE) {
+             panic("Scratch register read error! Expected: 0x%x but got: 0x%lx",
+                     SCRATCH_REG_VALUE, reg);
+             return -EINVAL;
+     }
+
+     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
+             SCRATCH_REG_SIZE, SCRATCH_TEST_VALUE);
+     reg = litex_get_reg(reg_addr + SCRATCH_REG_OFF, SCRATCH_REG_SIZE);
+
+     if (reg != SCRATCH_TEST_VALUE) {
+             panic("Scratch register write error! Expected: 0x%x but got: 0x%lx",
+                     SCRATCH_TEST_VALUE, reg);
+             return -EINVAL;
+     }
+
+     /* restore original value of the SCRATCH register */
+     litex_set_reg(reg_addr + SCRATCH_REG_OFF,
+             SCRATCH_REG_SIZE, SCRATCH_REG_VALUE);
+
+     pr_info("LiteX SoC Controller driver initialized");
+
+     return 0;
+}
+
+struct litex_soc_ctrl_device {
+     void __iomem *base;
+};
+
+static const struct of_device_id litex_soc_ctrl_of_match[] = {
+     {.compatible = "litex,soc-controller"},
+     {},
+};
+
+MODULE_DEVICE_TABLE(of, litex_soc_ctrl_of_match);
+
+static int litex_soc_ctrl_probe(struct platform_device *pdev)
+{
+     int result;
+     struct device *dev;
+     struct device_node *node;
+     struct litex_soc_ctrl_device *soc_ctrl_dev;
+
+     dev = &pdev->dev;
+     node = dev->of_node;
+     if (!node)
+             return -ENODEV;
+
+     soc_ctrl_dev = devm_kzalloc(dev, sizeof(*soc_ctrl_dev), GFP_KERNEL);
+     if (!soc_ctrl_dev)
+             return -ENOMEM;
+
+     soc_ctrl_dev->base = devm_platform_ioremap_resource(pdev, 0);
+     if (IS_ERR(soc_ctrl_dev->base))
+             return PTR_ERR(soc_ctrl_dev->base);
+
+     result = litex_check_csr_access(soc_ctrl_dev->base);
+     if (result) {
+             /* LiteX CSRs access is broken which means that
+              * none of LiteX drivers will most probably
+              * operate correctly
+              */
+             WARN(1, "Failed to validate CSR registers, the system is probably broken.\n");
+     }
+
+     return result;
+}
+
+static struct platform_driver litex_soc_ctrl_driver = {
+     .driver = {
+             .name = "litex-soc-controller",
+             .of_match_table = of_match_ptr(litex_soc_ctrl_of_match)
+     },
+     .probe = litex_soc_ctrl_probe,
+};
+
+module_platform_driver(litex_soc_ctrl_driver);
+MODULE_DESCRIPTION("LiteX SoC Controller driver");
+MODULE_AUTHOR("Antmicro <www.antmicro.com>");
+MODULE_LICENSE("GPL v2");

diff --git a/include/linux/litex.h b/include/linux/litex.h
new file mode 100644
index 000000000000..72061018c172
--- /dev/null
+++ b/include/linux/litex.h

@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Common LiteX header providing
+ * helper functions for accessing CSRs.
+ *
+ * Implementation of the functions is provided by
+ * the LiteX SoC Controller driver.
+ *
+ * Copyright (C) 2019-2020 Antmicro <www.antmicro.com>
+ */
+
+#ifndef _LINUX_LITEX_H
+#define _LINUX_LITEX_H
+
+#include <linux/io.h>
+#include <linux/types.h>
+#include <linux/compiler_types.h>
+
+void litex_set_reg(void __iomem *reg, unsigned long reg_sz, unsigned long val);
+
+unsigned long litex_get_reg(void __iomem *reg, unsigned long reg_sz);
+

Nitpick. One blank line is almost always enough!

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

quoted

+
+#endif /* _LINUX_LITEX_H */

Best regards,
Mateusz


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