[PATCH 1/2] dt-bindings: mfd: Add Baikal-T1 Boot Controller bindings

[PATCH 1/2] dt-bindings: mfd: Add Baikal-T1 Boot Controller bindings

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

From Linux point of view Baikal-T1 Boot Controller is a multi-function
memory-mapped device, which provides an access to three memory-mapped
ROMs and to an embedded DW APB SSI-based SPI controller. It's refelected
in the be,bt1-boot-ctl bindings file. So the device must be added to
the system dts-file as an ordinary memory-mapped device node with
a single clocks source phandle declared and with also memory-mapped
spi/mtd-rom sub-devices.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 .../bindings/mfd/be,bt1-boot-ctl.yaml         | 89 +++++++++++++++++++
 1 file changed, 89 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml

diff --git a/Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml b/Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml
new file mode 100644
index 000000000000..bb95a236d231
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml
@@ -0,0 +1,89 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/be,bt1-boot-ctl.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Baikal-T1 Boot Controller bindings
+
+description: |
+  Baikal-T1 SoC Boot Controller is a vendor-specific module responsible
+  for the CPU primary booting up. Mainly it is a special block, which
+  task is to properly start the SoC and then pass the control to the CPU
+  cores. It also provides a MMIO-based interface to a bootable memory
+  devices with an executable code pre-installed for the system to
+  start. The controller includes the next functions:
+  1) Pysically mapped ROMs to transparently access a SoC' internal firmware
+     and SPI Boot flash.
+  2) DW APB SSI-based embedded SPI controller.
+
+maintainers:
+  - Serge Semin <fancer.lancer@gmail.com>
+
+properties:
+  compatible:
+    const: be,bt1-boot-ctl
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    description: APB interface clock source.
+    maxItems: 1
+
+  clock-names:
+    items:
+      - const: pclk
+
+  "#address-cells": true
+
+  "#size-cells": true
+
+  ranges: true
+
+patternProperties:
+  "^(rom|spi)@[0-9a-fA-F]+$":
+    type: object
+
+    properties:
+      compatible:
+        anyOf:
+          - description: Memory mapped boot ROMs.
+            items:
+             - enum:
+               - be,bt1-int-rom
+               - be,bt1-ssi-rom
+               - be,bt1-boot-rom
+             - const: mtd-rom
+          - description: DW APB SSI-based boot SPI controller.
+            const: be,bt1-boot-ssi
+
+    required:
+      - compatible
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - "#address-cells"
+  - "#size-cells"
+  - ranges
+  - clocks
+  - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/bt1-ccu.h>
+
+    boot: boot@1F040000 {
+      compatible = "be,bt1-boot-ctl";
+      reg = <0x1F040000 0x100>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+      ranges;
+
+      clocks = <&ccu_sys CCU_SYS_APB_CLK>;
+      clock-names = "pclk";
+    };
+...
-- 
2.25.1

[PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey.Semin]

From: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Baikal-T1 Boot Controller is an IP block embedded into the SoC and
responsible for the chip proper pre-initialization and further
booting up from some memory device. From the Linux kernel point of view
it's just a multi-functional device, which exports three physically mapped
ROMs and a single SPI controller.

Primarily the ROMs are intended to be used for transparent access of
the memory devices with system bootup code. First ROM device is an
embedded into the SoC firmware, which is supposed to be used just for
the chip debug and tests. Second ROM device provides a MMIO-based
access to an external SPI flash. Third ROM mirrors either the Internal
or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
chip pins selecting the system boot device.

External SPI flash can be also accessed by the DW APB SSI SPI controller
embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
SPI flash region shouldn't be accessed.

Taking into account all the peculiarities described above, we created
an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
OF-based sub-device registration it also provides a simple API to
serialize an access to the external SPI flash from either the MMIO-based
SPI interface or embedded SPI controller.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: Paul Burton <paulburton@kernel.org>
Cc: Ralf Baechle <ralf@linux-mips.org>
---
 drivers/mfd/Kconfig              |  13 ++
 drivers/mfd/Makefile             |   1 +
 drivers/mfd/bt1-boot-ctl.c       | 345 +++++++++++++++++++++++++++++++
 include/linux/mfd/bt1-boot-ctl.h |  46 +++++
 4 files changed, 405 insertions(+)
 create mode 100644 drivers/mfd/bt1-boot-ctl.c
 create mode 100644 include/linux/mfd/bt1-boot-ctl.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2b203290e7b9..769320dfe25d 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -211,6 +211,19 @@ config MFD_AXP20X_RSB
 	  components like regulators or the PEK (Power Enable Key) under the
 	  corresponding menus.
 
+config MFD_BT1_BOOT
+	tristate "Baikal-T1 Boot Controller support"
+	select MFD_CORE
+	depends on (MIPS_BAIKAL_T1 || COMPILE_TEST) && OF
+	help
+	  If you say Y here you get support for Baikal-T1 SoC Boot Controller
+	  block embedded into the chip. It enables an access to the memory
+	  mapped Internal ROM, SPI ROM, Bootup ROM (this memory region content
+	  depends on the bootup mode) and DW APB SSI-based SPI interface.
+	  Aside from registering the sub-devices this driver provides an APIs,
+	  to switch the Boot SPI interface being handled by the memory mapped
+	  SPI ROM or the DW APB SSI modules.
+
 config MFD_CROS_EC_DEV
 	tristate "ChromeOS Embedded Controller multifunction device"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b83f172545e1..4c282973090b 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
+obj-$(CONFIG_MFD_BT1_BOOT)	+= bt1-boot-ctl.o
 obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
 
diff --git a/drivers/mfd/bt1-boot-ctl.c b/drivers/mfd/bt1-boot-ctl.c
new file mode 100644
index 000000000000..9e3cd47a2e7a
--- /dev/null
+++ b/drivers/mfd/bt1-boot-ctl.c
@@ -0,0 +1,345 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Authors:
+ *   Serge Semin <Sergey.Semin@baikalelectronics.ru>
+ *
+ * Baikal-T1 Boot Controller driver.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/io.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/clk.h>
+#include <linux/mutex.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/bt1-boot-ctl.h>
+
+/*
+ * Baikal-T1 boot controller control registers. These are vendor-specific
+ * registers, which can be used to detect the CPU boot mode and toggle the
+ * Boot SPI flash access modes - either over SPI controller registers
+ * or transparently over a 16 MB memory region mapped at the 0x1C000000
+ * base address.
+ */
+#define BC_CSR				0x00
+#define BC_CSR_MODE_FLD			0
+#define BC_CSR_MODE_MASK		GENMASK(1, BC_CSR_MODE_FLD)
+#define BC_CSR_MODE_ROM			0
+#define BC_CSR_MODE_SRAM		1
+#define BC_CSR_MODE_FLASH		2
+#define BC_CSR_SPI_RDA			BIT(8)
+#define BC_MAR				0x04
+#define BC_MAR_BSAB			BIT(0)
+#define BC_DRID				0x08
+#define BC_VID				0x0C
+#define BC_SPI				0x100
+#define BC_SPI_SSIENR			0x108
+#define BC_SPI_DR_START			0x160
+#define BC_SPI_DR_END			0x1EC
+
+#define BC_GET_FLD(_name, _data) \
+	(((u32)(_data) & BC_ ##_name## _MASK) >> BC_ ##_name## _FLD)
+
+/*
+ * struct bt1_bc - Baikal-T1 Boot Controller private data.
+ * @pdev: platform device instance of the boot controller.
+ * @dev: pointer to the corresponding device structure.
+ * @regs: memory mapped control registers.
+ * @spi_mode: current SPI interface mode.
+ * @spi_mtx: mutex to serialize access to the SPI interfae.
+ * @boot_mode: current SoC boot mode.
+ * @pclk: APB clock.
+ */
+struct bt1_bc {
+	struct platform_device *pdev;
+	struct device *dev;
+
+	void __iomem *regs;
+	enum bt1_bc_spi_mode spi_mode;
+	struct mutex spi_mtx;
+
+	int boot_mode;
+
+	struct clk *pclk;
+};
+
+/*
+ * Baikal-T1 Boot SPI registers state expected to be specified so the
+ * physically mapped ROM interface would work. If DW APB SSI controller
+ * configuration differs from them, the APB-terminator will report the
+ * slave access timeout error.
+ * Note we speed the transfer up to half the DW APB SSI reference clock
+ * instead of leaving a sixth part of it. This increases the interface
+ * performace threefold.
+ */
+static const u32 bc_spi_dump[] = {
+	0x00000307, 0x00000003, 0x00000001, 0x00000000,
+	0x00000001, 0x00000002, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000006, 0x0000003f,
+	0x00000001, 0x00000001, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00100012, 0x3332322a,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000,
+	0x00000000, 0x00000000, 0x00000000, 0x00000000
+};
+
+static inline u32 bc_read(void __iomem *reg)
+{
+	return readl(reg);
+}
+
+static inline void bc_write(void __iomem *reg, u32 data)
+{
+	writel(data, reg);
+}
+
+static inline u32 bc_update(void __iomem *reg, u32 mask, u32 data)
+{
+	u32 old;
+
+	old = readl_relaxed(reg);
+	writel((old & ~mask) | (data & mask), reg);
+
+	return old & mask;
+}
+
+static const char *bc_boot_mode(int mode)
+{
+	switch (mode) {
+	case BC_CSR_MODE_ROM:
+		return "Internal ROM";
+	case BC_CSR_MODE_SRAM:
+		return "Static RAM";
+	case BC_CSR_MODE_FLASH:
+		return "SPI Flash";
+	}
+
+	return "Unknown";
+}
+
+static void bc_spi_map_config(struct bt1_bc *bc)
+{
+	int i, addr;
+
+	bc_write(bc->regs + BC_SPI_SSIENR, 0x0);
+	for (i = 0; i < ARRAY_SIZE(bc_spi_dump); ++i) {
+		addr = BC_SPI + 4*i;
+		if ((addr < BC_SPI_DR_START || addr >= BC_SPI_DR_END) &&
+		    addr != BC_SPI_SSIENR) {
+			bc_write(bc->regs + addr, bc_spi_dump[i]);
+		}
+	}
+	bc_write(bc->regs + BC_SPI_SSIENR, 0x1);
+}
+
+/*
+ * __bt1_bc_spi_lock() - lock Boot Controller SPI interface.
+ * @bc: bt1_bc structure.
+ * @mode: SPI interface mode.
+ *
+ * Exclusive lock primitive to serialize access to the shared SPI flash
+ * by acquiring the mutex and enabling the requested mode.
+ */
+int __bt1_bc_spi_lock(struct bt1_bc *bc, enum bt1_bc_spi_mode mode)
+{
+	int ret;
+
+	if (mode != BT1_BC_SPI_MAP && mode != BT1_BC_SPI_DW)
+		return -EINVAL;
+
+	ret = mutex_lock_interruptible(&bc->spi_mtx);
+	if (ret)
+		return ret;
+
+	if (mode == BT1_BC_SPI_MAP) {
+		if (bc->spi_mode == BT1_BC_SPI_DW)
+			bc_spi_map_config(bc);
+		bc_update(bc->regs + BC_CSR, BC_CSR_SPI_RDA, 0);
+	} else {
+		bc_update(bc->regs + BC_CSR, BC_CSR_SPI_RDA, BC_CSR_SPI_RDA);
+	}
+
+	bc->spi_mode = mode;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(__bt1_bc_spi_lock);
+
+/*
+ * bt1_bc_spi_unlock() - unlock Boot Controller SPI interface.
+ * @bc: bt1_bc structure.
+ * @mode: SPI interface mode.
+ *
+ * Exclusive unlock primitive to release the shared SPI flash resource.
+ */
+void bt1_bc_spi_unlock(struct bt1_bc *bc)
+{
+	mutex_unlock(&bc->spi_mtx);
+}
+EXPORT_SYMBOL_GPL(bt1_bc_spi_unlock);
+
+static struct bt1_bc *bc_create_data(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct bt1_bc *bc;
+
+	bc = devm_kzalloc(dev, sizeof(*bc), GFP_KERNEL);
+	if (!bc)
+		return ERR_PTR(-ENOMEM);
+
+	bc->pdev = pdev;
+	bc->dev = dev;
+
+	return bc;
+}
+
+static int bc_request_regs(struct bt1_bc *bc)
+{
+	bc->regs = devm_platform_ioremap_resource(bc->pdev, 0);
+	if (IS_ERR(bc->regs)) {
+		dev_err(bc->dev, "Couldn't map the controller registers\n");
+		return PTR_ERR(bc->regs);
+	}
+
+	return 0;
+}
+
+static void bc_disable_clk(void *data)
+{
+	struct bt1_bc *bc = data;
+
+	clk_disable_unprepare(bc->pclk);
+}
+
+static int bc_request_clk(struct bt1_bc *bc)
+{
+	int ret;
+
+	bc->pclk = devm_clk_get(bc->dev, "pclk");
+	if (IS_ERR(bc->pclk)) {
+		dev_err(bc->dev, "Couldn't get the APB clock descriptor\n");
+		return PTR_ERR(bc->pclk);
+	}
+
+	ret = clk_prepare_enable(bc->pclk);
+	if (ret) {
+		dev_err(bc->dev, "Couldn't enable the APB clock\n");
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(bc->dev, bc_disable_clk, bc);
+	if (ret) {
+		dev_err(bc->dev, "Can't add the APB clock disable action\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static void bc_clear_iface(void *data)
+{
+	struct bt1_bc *bc = data;
+
+	mutex_destroy(&bc->spi_mtx);
+	platform_set_drvdata(bc->pdev, NULL);
+}
+
+static int bc_init_iface(struct bt1_bc *bc)
+{
+	u32 data;
+	int ret;
+
+	/*
+	 * Make sure the interface is initialized in a way so SRAM and SPI
+	 * DW would be accessible by default.
+	 */
+	bc_write(bc->regs + BC_MAR, 0);
+	data = bc_read(bc->regs + BC_CSR);
+	bc_write(bc->regs + BC_CSR, data | BC_CSR_SPI_RDA);
+
+	bc->spi_mode = BT1_BC_SPI_DW;
+	bc->boot_mode = BC_GET_FLD(CSR_MODE, data);
+
+	platform_set_drvdata(bc->pdev, bc);
+	mutex_init(&bc->spi_mtx);
+
+	ret = devm_add_action_or_reset(bc->dev, bc_clear_iface, bc);
+	if (ret) {
+		dev_err(bc->dev, "Can't add the interface clearance action\n");
+		return ret;
+	}
+
+	dev_info(bc->dev, "System loader started from %s\n",
+		 bc_boot_mode(bc->boot_mode));
+
+	return 0;
+}
+
+static int bc_register_devices(struct bt1_bc *bc)
+{
+	int ret;
+
+	ret = devm_of_platform_populate(bc->dev);
+	if (ret)
+		dev_err(bc->dev, "Failed to add sub-devices\n");
+
+	return ret;
+}
+
+static int bc_probe(struct platform_device *pdev)
+{
+	struct bt1_bc *bc;
+	int ret;
+
+	bc = bc_create_data(pdev);
+	if (IS_ERR(bc))
+		return PTR_ERR(bc);
+
+	ret = bc_request_regs(bc);
+	if (ret)
+		return ret;
+
+	ret = bc_request_clk(bc);
+	if (ret)
+		return ret;
+
+	ret = bc_init_iface(bc);
+	if (ret)
+		return ret;
+
+	return bc_register_devices(bc);
+}
+
+static const struct of_device_id bc_of_match[] = {
+	{ .compatible = "be,bt1-boot-ctl" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, bc_of_match);
+
+static struct platform_driver bc_driver = {
+	.probe = bc_probe,
+	.driver = {
+		.name = "bt1-boot-ctl",
+		.of_match_table = of_match_ptr(bc_of_match)
+	}
+};
+module_platform_driver(bc_driver);
+
+MODULE_AUTHOR("Serge Semin <Sergey.Semin@baikalelectronics.ru>");
+MODULE_DESCRIPTION("Baikal-T1 Boot Controller driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/bt1-boot-ctl.h b/include/linux/mfd/bt1-boot-ctl.h
new file mode 100644
index 000000000000..2c2dcc01a015
--- /dev/null
+++ b/include/linux/mfd/bt1-boot-ctl.h
@@ -0,0 +1,46 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
+ *
+ * Baikal-T1 Boot Controller driver.
+ */
+#ifndef __MFD_BT1_BOOT_CTL_H__
+#define __MFD_BT1_BOOT_CTL_H__
+
+struct bt1_bc;
+
+/*
+ * enum bt1_bc_spi_mode - Boot Controller SPI interface modes.
+ * @BT1_BC_SPI_MAP: memory mapped SPI Flash.
+ * @BT1_BC_SPI_DW: DW APB SSI-based access.
+ */
+enum bt1_bc_spi_mode {
+	BT1_BC_SPI_MAP,
+	BT1_BC_SPI_DW
+};
+
+extern int __bt1_bc_spi_lock(struct bt1_bc *bc, enum bt1_bc_spi_mode mode);
+extern void bt1_bc_spi_unlock(struct bt1_bc *bc);
+
+/*
+ * bt1_bc_spi_map_lock() - Lock Boot SPI Controller in the trasparent mode.
+ * @__bc: bt1_bc structure.
+ *
+ * Helper macro to lock the Baikal-T1 Boot SPI interface in the mode,
+ * when the SPI Flash is directly mapped to a memory region in the
+ * read-only mode.
+ */
+#define bt1_bc_spi_map_lock(__bc) \
+	__bt1_bc_spi_lock(__bc, BT1_BC_SPI_MAP)
+
+/*
+ * bt1_bc_spi_dw_lock() - Lock Boot SPI Controller in the DW SSI mode.
+ * @__bc: bt1_bc structure.
+ *
+ * Helper macro to lock the Baikal-T1 Boot SPI interface in the
+ * DW SSI controller mode.
+ */
+#define bt1_bc_spi_dw_lock(__bc) \
+	__bt1_bc_spi_lock(__bc, BT1_BC_SPI_DW)
+
+#endif /* __MFD_BT1_BOOT_CTL_H__ */
-- 
2.25.1

Re: [PATCH 1/2] dt-bindings: mfd: Add Baikal-T1 Boot Controller bindings

[Rob Herring]

On Fri, 6 Mar 2020 16:05:27 +0300, <Sergey.Semin@baikalelectronics.ru> wrote:
> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> >From Linux point of view Baikal-T1 Boot Controller is a multi-function
> memory-mapped device, which provides an access to three memory-mapped
> ROMs and to an embedded DW APB SSI-based SPI controller. It's refelected
> in the be,bt1-boot-ctl bindings file. So the device must be added to
> the system dts-file as an ordinary memory-mapped device node with
> a single clocks source phandle declared and with also memory-mapped
> spi/mtd-rom sub-devices.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  .../bindings/mfd/be,bt1-boot-ctl.yaml         | 89 +++++++++++++++++++
>  1 file changed, 89 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dts:17:10: fatal error: dt-bindings/clock/bt1-ccu.h: No such file or directory
 #include <dt-bindings/clock/bt1-ccu.h>
          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
compilation terminated.
scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dt.yaml' failed
make[1]: *** [Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dt.yaml] Error 1
Makefile:1262: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1250277
Please check and re-submit.

Re: [PATCH 0/2] mfd: Add Baikal-T1 SoC Boot Controller driver

[Sergey Semin]

On Fri, Mar 06, 2020 at 04:05:26PM +0300, Sergey.Semin@baikalelectronics.ru wrote:
> From: Serge Semin <fancer.lancer@gmail.com>
> 
> Baikal-T1 Boot Controller is an IP block embedded into the SoC and responsible
> for the chip proper pre-initialization and further booting up from selected
> memory mapped device. From the Linux kernel point of view it's just a multi-
> functional device, which exports three physically mapped ROMs and a single SPI
> controller interface.
> 
> Baikal-T1 can boot either from an external SPI-flash or from an embedded into
> it firmware. So when the bootup from the SPI-flash is selected the flash memory
> can be accessed either directly via the embedded into the Boot Controller DW
> APB SSI controller registers or via a physically mapped ROM (which is just an
> FSM IP-core interacting with the DW APB SSI controller by itself). Since both
> of these interfaces are using the same SPI interface they can't be utilized
> simultaneously. Instead the Boot Controller provides the access switching
> functionality by means of the control register flag. That's why we need the
> Boot Controller MFD driver provided by this patchset - in order to multiplex the
> access to the DW APB SSI controller and SPI interface from two different
> subsystems.
> 
> After this patchset is integrated into the kernel we'll submit two more
> patchsets with physically mapped ROMs (due to some peculiarities we can't have
> the already available in the kernel mtd-rom drivers) and SPI controller
> (similarly the available in the kernel DW APB SSI driver isn't suitable for
> our version of the SPI controller) drivers will be submitted for integration
> into the mainline Linux kernel.
> 
> This patchset is rebased and tested on the mainline Linux kernel 5.6-rc4:
> commit 98d54f81e36b ("Linux 5.6-rc4").
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Maxim Kaurkin <Maxim.Kaurkin@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Ramil Zaripov <Ramil.Zaripov@baikalelectronics.ru>
> Cc: Ekaterina Skachko <Ekaterina.Skachko@baikalelectronics.ru>
> Cc: Vadim Vlasov <V.Vlasov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (2):
>   dt-bindings: mfd: Add Baikal-T1 Boot Controller bindings
>   mfd: Add Baikal-T1 Boot Controller driver
> 
>  .../bindings/mfd/be,bt1-boot-ctl.yaml         |  89 +++++
>  drivers/mfd/Kconfig                           |  13 +
>  drivers/mfd/Makefile                          |   1 +
>  drivers/mfd/bt1-boot-ctl.c                    | 345 ++++++++++++++++++
>  include/linux/mfd/bt1-boot-ctl.h              |  46 +++
>  5 files changed, 494 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml
>  create mode 100644 drivers/mfd/bt1-boot-ctl.c
>  create mode 100644 include/linux/mfd/bt1-boot-ctl.h
> 
> -- 
> 2.25.1
> 

Folks,

It appears our corporate email server changes the Message-Id field of 
messages passing through it. Due to that the emails threading gets to be
broken. I'll resubmit the properly structured v2 patchset as soon as our system
administrator fixes the problem. Sorry for the inconvenience caused by it.

Regards,
-Sergey

Re: [PATCH 1/2] dt-bindings: mfd: Add Baikal-T1 Boot Controller bindings

[Sergey Semin]

On Mon, Mar 09, 2020 at 01:07:28PM -0500, Rob Herring wrote:
> On Fri, 6 Mar 2020 16:05:27 +0300, <Sergey.Semin@baikalelectronics.ru> wrote:
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > >From Linux point of view Baikal-T1 Boot Controller is a multi-function
> > memory-mapped device, which provides an access to three memory-mapped
> > ROMs and to an embedded DW APB SSI-based SPI controller. It's refelected
> > in the be,bt1-boot-ctl bindings file. So the device must be added to
> > the system dts-file as an ordinary memory-mapped device node with
> > a single clocks source phandle declared and with also memory-mapped
> > spi/mtd-rom sub-devices.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  .../bindings/mfd/be,bt1-boot-ctl.yaml         | 89 +++++++++++++++++++
> >  1 file changed, 89 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dts:17:10: fatal error: dt-bindings/clock/bt1-ccu.h: No such file or directory
>  #include <dt-bindings/clock/bt1-ccu.h>
>           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> compilation terminated.
> scripts/Makefile.lib:311: recipe for target 'Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dt.yaml' failed
> make[1]: *** [Documentation/devicetree/bindings/mfd/be,bt1-boot-ctl.example.dt.yaml] Error 1
> Makefile:1262: recipe for target 'dt_binding_check' failed
> make: *** [dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1250277
> Please check and re-submit.

Rob,
I'll fix this and also take into account the comments you added to the
hwmon patch. Then I'll resend this patchset. So don't bother with review
for now.

Regards,
-Sergey

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Lee Jones]

On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:

> From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> responsible for the chip proper pre-initialization and further
> booting up from some memory device. From the Linux kernel point of view
> it's just a multi-functional device, which exports three physically mapped
> ROMs and a single SPI controller.
> 
> Primarily the ROMs are intended to be used for transparent access of
> the memory devices with system bootup code. First ROM device is an
> embedded into the SoC firmware, which is supposed to be used just for
> the chip debug and tests. Second ROM device provides a MMIO-based
> access to an external SPI flash. Third ROM mirrors either the Internal
> or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> chip pins selecting the system boot device.
> 
> External SPI flash can be also accessed by the DW APB SSI SPI controller
> embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> SPI flash region shouldn't be accessed.
> 
> Taking into account all the peculiarities described above, we created
> an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> OF-based sub-device registration it also provides a simple API to
> serialize an access to the external SPI flash from either the MMIO-based
> SPI interface or embedded SPI controller.

Not sure why this is being classified as an MFD.

This is clearly 'just' a memory device.

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Paul Burton <paulburton@kernel.org>
> Cc: Ralf Baechle <ralf@linux-mips.org>
> ---
>  drivers/mfd/Kconfig              |  13 ++
>  drivers/mfd/Makefile             |   1 +
>  drivers/mfd/bt1-boot-ctl.c       | 345 +++++++++++++++++++++++++++++++
>  include/linux/mfd/bt1-boot-ctl.h |  46 +++++
>  4 files changed, 405 insertions(+)
>  create mode 100644 drivers/mfd/bt1-boot-ctl.c
>  create mode 100644 include/linux/mfd/bt1-boot-ctl.h

[...]

> diff --git a/drivers/mfd/bt1-boot-ctl.c b/drivers/mfd/bt1-boot-ctl.c
> new file mode 100644
> index 000000000000..9e3cd47a2e7a
> --- /dev/null
> +++ b/drivers/mfd/bt1-boot-ctl.c
> @@ -0,0 +1,345 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> + *
> + * Authors:
> + *   Serge Semin <Sergey.Semin@baikalelectronics.ru>
> + *
> + * Baikal-T1 Boot Controller driver.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/clk.h>
> +#include <linux/mutex.h>
> +#include <linux/mfd/core.h>

Despite including the MFD API, I don't see it being used at all.

> +#include <linux/mfd/bt1-boot-ctl.h>

[...]

> +static inline u32 bc_read(void __iomem *reg)
> +{
> +	return readl(reg);
> +}
> +
> +static inline void bc_write(void __iomem *reg, u32 data)
> +{
> +	writel(data, reg);
> +}

Abstraction for the sake of abstraction is generally discouraged.

[...]

> +static int bc_register_devices(struct bt1_bc *bc)
> +{
> +	int ret;
> +
> +	ret = devm_of_platform_populate(bc->dev);
> +	if (ret)
> +		dev_err(bc->dev, "Failed to add sub-devices\n");
> +
> +	return ret;
> +}

You can call devm_of_platform_populate() from anywhere.

Doesn't have to be an MFD.

[...]

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey Semin]

Hello Lee,

On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> 
> > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > 
> > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > responsible for the chip proper pre-initialization and further
> > booting up from some memory device. From the Linux kernel point of view
> > it's just a multi-functional device, which exports three physically mapped
> > ROMs and a single SPI controller.
> > 
> > Primarily the ROMs are intended to be used for transparent access of
> > the memory devices with system bootup code. First ROM device is an
> > embedded into the SoC firmware, which is supposed to be used just for
> > the chip debug and tests. Second ROM device provides a MMIO-based
> > access to an external SPI flash. Third ROM mirrors either the Internal
> > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > chip pins selecting the system boot device.
> > 
> > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > SPI flash region shouldn't be accessed.
> > 
> > Taking into account all the peculiarities described above, we created
> > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > OF-based sub-device registration it also provides a simple API to
> > serialize an access to the external SPI flash from either the MMIO-based
> > SPI interface or embedded SPI controller.
> 
> Not sure why this is being classified as an MFD.
> 
> This is clearly 'just' a memory device.
> 

Hm, I see this as a normal MFD device. The Boot controller provides a
set of physically mapped ROMs and a DW APB SSI-based embedded SPI
controller. Yes, the SPI controller is normally utilized to access an external
flash device, and at boot stage it is used for it. But still it's a SPI
controller which driver belongs to the kernel SPI subsystem. Moreover
nothing prevents a platform designer from using it together with custom
GPIO-based chip-selects to have additional devices on the SPI bus.

As I said the problem is that an SPI flash might be accessed either with
use of a physically mapped ROM or via the normal DW APB SPI controller.
These two interfaces can't be used simultaneously, because the ROM is
just an rtl state-machine, which is built to translate MMIO operations
through the SPI controller registers to an external SPI-nor at CS0 of
the interface. That's why first I need to make sure the interface is locked,
then being enabled, then the corresponding driver can use it, then get
to unlock. That's the point of having the __bt1_bc_spi_lock() and
bt1_bc_spi_unlock() methods exported from the driver.

I've got two drivers for MTD ROM and SPI controller based on that
methods. But I haven't submitted them yet, because they belong to two
different subsystems and I need to have this one being accepted first.

Recently I've sent an RFC regarding a different question, but it
concerns the Baikal-T1 system controller and the boot controller as being part
of it:

https://lkml.org/lkml/2020/3/22/393

Regards,
-Sergey

> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > Signed-off-by: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> > Cc: Paul Burton <paulburton@kernel.org>
> > Cc: Ralf Baechle <ralf@linux-mips.org>
> > ---
> >  drivers/mfd/Kconfig              |  13 ++
> >  drivers/mfd/Makefile             |   1 +
> >  drivers/mfd/bt1-boot-ctl.c       | 345 +++++++++++++++++++++++++++++++
> >  include/linux/mfd/bt1-boot-ctl.h |  46 +++++
> >  4 files changed, 405 insertions(+)
> >  create mode 100644 drivers/mfd/bt1-boot-ctl.c
> >  create mode 100644 include/linux/mfd/bt1-boot-ctl.h
> 
> [...]
> 
> > diff --git a/drivers/mfd/bt1-boot-ctl.c b/drivers/mfd/bt1-boot-ctl.c
> > new file mode 100644
> > index 000000000000..9e3cd47a2e7a
> > --- /dev/null
> > +++ b/drivers/mfd/bt1-boot-ctl.c
> > @@ -0,0 +1,345 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2020 BAIKAL ELECTRONICS, JSC
> > + *
> > + * Authors:
> > + *   Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > + *
> > + * Baikal-T1 Boot Controller driver.
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
> > +#include <linux/device.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/io.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/clk.h>
> > +#include <linux/mutex.h>
> > +#include <linux/mfd/core.h>
> 
> Despite including the MFD API, I don't see it being used at all.
> 
> > +#include <linux/mfd/bt1-boot-ctl.h>
> 
> [...]
> 
> > +static inline u32 bc_read(void __iomem *reg)
> > +{
> > +	return readl(reg);
> > +}
> > +
> > +static inline void bc_write(void __iomem *reg, u32 data)
> > +{
> > +	writel(data, reg);
> > +}
> 
> Abstraction for the sake of abstraction is generally discouraged.
> 
> [...]
> 
> > +static int bc_register_devices(struct bt1_bc *bc)
> > +{
> > +	int ret;
> > +
> > +	ret = devm_of_platform_populate(bc->dev);
> > +	if (ret)
> > +		dev_err(bc->dev, "Failed to add sub-devices\n");
> > +
> > +	return ret;
> > +}
> 
> You can call devm_of_platform_populate() from anywhere.
> 
> Doesn't have to be an MFD.
> 
> [...]
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Lee Jones]

On Wed, 25 Mar 2020, Sergey Semin wrote:

> Hello Lee,
> 
> On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > 
> > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > 
> > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > responsible for the chip proper pre-initialization and further
> > > booting up from some memory device. From the Linux kernel point of view
> > > it's just a multi-functional device, which exports three physically mapped
> > > ROMs and a single SPI controller.
> > > 
> > > Primarily the ROMs are intended to be used for transparent access of
> > > the memory devices with system bootup code. First ROM device is an
> > > embedded into the SoC firmware, which is supposed to be used just for
> > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > chip pins selecting the system boot device.
> > > 
> > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > SPI flash region shouldn't be accessed.
> > > 
> > > Taking into account all the peculiarities described above, we created
> > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > OF-based sub-device registration it also provides a simple API to
> > > serialize an access to the external SPI flash from either the MMIO-based
> > > SPI interface or embedded SPI controller.
> > 
> > Not sure why this is being classified as an MFD.
> > 
> > This is clearly 'just' a memory device.
> > 
> 
> Hm, I see this as a normal MFD device. The Boot controller provides a
> set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> controller. Yes, the SPI controller is normally utilized to access an external
> flash device, and at boot stage it is used for it. But still it's a SPI
> controller which driver belongs to the kernel SPI subsystem. Moreover
> nothing prevents a platform designer from using it together with custom
> GPIO-based chip-selects to have additional devices on the SPI bus.
> 
> As I said the problem is that an SPI flash might be accessed either with
> use of a physically mapped ROM or via the normal DW APB SPI controller.
> These two interfaces can't be used simultaneously, because the ROM is
> just an rtl state-machine, which is built to translate MMIO operations
> through the SPI controller registers to an external SPI-nor at CS0 of
> the interface. That's why first I need to make sure the interface is locked,
> then being enabled, then the corresponding driver can use it, then get
> to unlock. That's the point of having the __bt1_bc_spi_lock() and
> bt1_bc_spi_unlock() methods exported from the driver.
> 
> I've got two drivers for MTD ROM and SPI controller based on that
> methods. But I haven't submitted them yet, because they belong to two
> different subsystems and I need to have this one being accepted first.

This is not a totally unique device/situation.  I've seen (and NACKed)
this type of device before.  You need to explain this to the MTD
(SPI-NOR?) maintainers.  They should be in a good position to provide
guidance.

> Recently I've sent an RFC regarding a different question, but it
> concerns the Baikal-T1 system controller and the boot controller as being part
> of it:
> 
> https://lkml.org/lkml/2020/3/22/393

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey Semin]

Michael, Richard, Vignesh and MTD mailing list are Cc'ed to have their
comments on the issue.

My answers on the previous email are below.

On Thu, Mar 26, 2020 at 09:13:13AM +0000, Lee Jones wrote:
> On Wed, 25 Mar 2020, Sergey Semin wrote:
> 
> > Hello Lee,
> > 
> > On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> > > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > > 
> > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > 
> > > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > > responsible for the chip proper pre-initialization and further
> > > > booting up from some memory device. From the Linux kernel point of view
> > > > it's just a multi-functional device, which exports three physically mapped
> > > > ROMs and a single SPI controller.
> > > > 
> > > > Primarily the ROMs are intended to be used for transparent access of
> > > > the memory devices with system bootup code. First ROM device is an
> > > > embedded into the SoC firmware, which is supposed to be used just for
> > > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > > chip pins selecting the system boot device.
> > > > 
> > > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > > SPI flash region shouldn't be accessed.
> > > > 
> > > > Taking into account all the peculiarities described above, we created
> > > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > > OF-based sub-device registration it also provides a simple API to
> > > > serialize an access to the external SPI flash from either the MMIO-based
> > > > SPI interface or embedded SPI controller.
> > > 
> > > Not sure why this is being classified as an MFD.
> > > 
> > > This is clearly 'just' a memory device.
> > > 
> > 
> > Hm, I see this as a normal MFD device. The Boot controller provides a
> > set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> > controller. Yes, the SPI controller is normally utilized to access an external
> > flash device, and at boot stage it is used for it. But still it's a SPI
> > controller which driver belongs to the kernel SPI subsystem. Moreover
> > nothing prevents a platform designer from using it together with custom
> > GPIO-based chip-selects to have additional devices on the SPI bus.
> > 
> > As I said the problem is that an SPI flash might be accessed either with
> > use of a physically mapped ROM or via the normal DW APB SPI controller.
> > These two interfaces can't be used simultaneously, because the ROM is
> > just an rtl state-machine, which is built to translate MMIO operations
> > through the SPI controller registers to an external SPI-nor at CS0 of
> > the interface. That's why first I need to make sure the interface is locked,
> > then being enabled, then the corresponding driver can use it, then get
> > to unlock. That's the point of having the __bt1_bc_spi_lock() and
> > bt1_bc_spi_unlock() methods exported from the driver.
> > 
> > I've got two drivers for MTD ROM and SPI controller based on that
> > methods. But I haven't submitted them yet, because they belong to two
> > different subsystems and I need to have this one being accepted first.
> 
> This is not a totally unique device/situation.  I've seen (and NACKed)
> this type of device before.  You need to explain this to the MTD
> (SPI-NOR?) maintainers.  They should be in a good position to provide
> guidance.
> 

Sorry, I don't really understand your justification. The boot controller
exports two types of devices: physically mapped ROMs and an DW APB
SSI-based SPI controller. Aside from being able to access an externally
attached SPI flash the SPI controller has normal SPI interface which in
general can be used to access any other SPI-slave. The complexity of
the case is that one of physically mapped ROM RTL uses the DW APB SSI
controller to access an external SPI flash, which when done makes the
DW APB SSI registers unusable on direct way. That's why I implemented the
boot controller as an MFD. An alternation caused by this peculiarity
will be submitted to drivers/mtd/maps/physmap-{core.c,baikal-t1-rom.c}
later after this change is reviewed and accepted. Similar situation
concerns a driver of DW APB SSI-based SPI controller. So according to
the current design the boot controller with it' sub-devices will be
declared in dts as follows:

boot: boot@1f040000 {
	compatible = "be,bt1-boot-ctl";
	reg = <0x1f040000 0x100>;
	#address-cells = <1>;
	#size-cells = <1>;
	ranges;

	clocks = <&ccu_sys CCU_SYS_APB_CLK>;
	clock-names = "pclk";

	int_rom: rom@1bfc0000 {
		compatible = "be,bt1-int-rom", "mtd-rom";
		reg = <0x1bfc0000 0x10000>;
		...
	};

	spi_rom: rom@1c000000 {
		compatible = "be,bt1-ssi-rom", "mtd-rom";
		reg = <0x1c000000 0x1000000>;
		...
	};

	spi0: spi@1f040100 {
		compatible = "be,bt1-boot-ssi";
		reg = <0x1f040100 0xf00>;
		#address-cells = <1>;
		#size-cells = <0>;

		clocks = <&ccu_sys CCU_SYS_SPI_CLK>;
		clock-names = "ref";

		...
	};

	boot_rom: rom@1fc00000 {
		compatible = "be,bt1-boot-rom", "mtd-rom";
		reg = <0x1fc00000 0x400000>;
		...
	};
};

I dare to assume, that by saying: "Despite including the MFD API, I don't
see it being used at all." you meant that since the driver doesn't
redistribute any resource by declaring mfd_cell'es, doesn't
register mfd-based sub-devices, and primary use-case of the boot
controller is to access flash-devices, it should be just moved to the MTD
subsystem. I don't think it would be better than to have a common part 
defined here in MFD while ROM-specific part - in MTD, and SPI-specific - in
the SPI subsystems. I would consider Baikal-T1 Boot Controller being similar
to drivers/mfd/qcom-spmi-pmic.c, drivers/mfd/atmel-flexcom.c, etc, but
instead of having GPIO, RTC, UART, i2c, etc sub-devices (which are also
fully defied in dts), it exports MMIO-based ROMs and SPI-controller.

Lee, explain me please what is the difference between these MFDs and
Baikal-T1 Boot Controller, that makes one simple MFDs suitable for the
MFD subsystem, while another isn't?

Miquel, Richard, Vignesh and everyone from MTD, who can help could you
join this discussion and clarify whether the Baikal-T1 Boot Controller
driver is supposed to be moved to the MTD subsystem? If so, then what is
the better place to put it within the drivers/mtd/ directory tree?

Regards,
-Sergey

> > Recently I've sent an RFC regarding a different question, but it
> > concerns the Baikal-T1 system controller and the boot controller as being part
> > of it:
> > 
> > https://lkml.org/lkml/2020/3/22/393
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Lee Jones]

On Thu, 26 Mar 2020, Sergey Semin wrote:

> Michael, Richard, Vignesh and MTD mailing list are Cc'ed to have their
> comments on the issue.
> 
> My answers on the previous email are below.
> 
> On Thu, Mar 26, 2020 at 09:13:13AM +0000, Lee Jones wrote:
> > On Wed, 25 Mar 2020, Sergey Semin wrote:
> > 
> > > Hello Lee,
> > > 
> > > On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> > > > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > > > 
> > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > 
> > > > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > > > responsible for the chip proper pre-initialization and further
> > > > > booting up from some memory device. From the Linux kernel point of view
> > > > > it's just a multi-functional device, which exports three physically mapped
> > > > > ROMs and a single SPI controller.
> > > > > 
> > > > > Primarily the ROMs are intended to be used for transparent access of
> > > > > the memory devices with system bootup code. First ROM device is an
> > > > > embedded into the SoC firmware, which is supposed to be used just for
> > > > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > > > chip pins selecting the system boot device.
> > > > > 
> > > > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > > > SPI flash region shouldn't be accessed.
> > > > > 
> > > > > Taking into account all the peculiarities described above, we created
> > > > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > > > OF-based sub-device registration it also provides a simple API to
> > > > > serialize an access to the external SPI flash from either the MMIO-based
> > > > > SPI interface or embedded SPI controller.
> > > > 
> > > > Not sure why this is being classified as an MFD.
> > > > 
> > > > This is clearly 'just' a memory device.
> > > > 
> > > 
> > > Hm, I see this as a normal MFD device. The Boot controller provides a
> > > set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> > > controller. Yes, the SPI controller is normally utilized to access an external
> > > flash device, and at boot stage it is used for it. But still it's a SPI
> > > controller which driver belongs to the kernel SPI subsystem. Moreover
> > > nothing prevents a platform designer from using it together with custom
> > > GPIO-based chip-selects to have additional devices on the SPI bus.
> > > 
> > > As I said the problem is that an SPI flash might be accessed either with
> > > use of a physically mapped ROM or via the normal DW APB SPI controller.
> > > These two interfaces can't be used simultaneously, because the ROM is
> > > just an rtl state-machine, which is built to translate MMIO operations
> > > through the SPI controller registers to an external SPI-nor at CS0 of
> > > the interface. That's why first I need to make sure the interface is locked,
> > > then being enabled, then the corresponding driver can use it, then get
> > > to unlock. That's the point of having the __bt1_bc_spi_lock() and
> > > bt1_bc_spi_unlock() methods exported from the driver.
> > > 
> > > I've got two drivers for MTD ROM and SPI controller based on that
> > > methods. But I haven't submitted them yet, because they belong to two
> > > different subsystems and I need to have this one being accepted first.
> > 
> > This is not a totally unique device/situation.  I've seen (and NACKed)
> > this type of device before.  You need to explain this to the MTD
> > (SPI-NOR?) maintainers.  They should be in a good position to provide
> > guidance.
> > 
> 
> Sorry, I don't really understand your justification. The boot controller
> exports two types of devices: physically mapped ROMs and an DW APB
> SSI-based SPI controller. Aside from being able to access an externally
> attached SPI flash the SPI controller has normal SPI interface which in
> general can be used to access any other SPI-slave. The complexity of
> the case is that one of physically mapped ROM RTL uses the DW APB SSI
> controller to access an external SPI flash, which when done makes the
> DW APB SSI registers unusable on direct way. That's why I implemented the
> boot controller as an MFD. An alternation caused by this peculiarity
> will be submitted to drivers/mtd/maps/physmap-{core.c,baikal-t1-rom.c}
> later after this change is reviewed and accepted. Similar situation
> concerns a driver of DW APB SSI-based SPI controller. So according to
> the current design the boot controller with it' sub-devices will be
> declared in dts as follows:
> 
> boot: boot@1f040000 {
> 	compatible = "be,bt1-boot-ctl";
> 	reg = <0x1f040000 0x100>;
> 	#address-cells = <1>;
> 	#size-cells = <1>;
> 	ranges;

What control does this device offer in those 0x100 registers? 

> 	clocks = <&ccu_sys CCU_SYS_APB_CLK>;
> 	clock-names = "pclk";
> 
> 	int_rom: rom@1bfc0000 {
> 		compatible = "be,bt1-int-rom", "mtd-rom";
> 		reg = <0x1bfc0000 0x10000>;
> 		...
> 	};
> 
> 	spi_rom: rom@1c000000 {
> 		compatible = "be,bt1-ssi-rom", "mtd-rom";
> 		reg = <0x1c000000 0x1000000>;
> 		...
> 	};
> 
> 	spi0: spi@1f040100 {
> 		compatible = "be,bt1-boot-ssi";
> 		reg = <0x1f040100 0xf00>;
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		clocks = <&ccu_sys CCU_SYS_SPI_CLK>;
> 		clock-names = "ref";
> 
> 		...
> 	};
> 
> 	boot_rom: rom@1fc00000 {
> 		compatible = "be,bt1-boot-rom", "mtd-rom";
> 		reg = <0x1fc00000 0x400000>;
> 		...
> 	};
> };
> 
> I dare to assume, that by saying: "Despite including the MFD API, I don't
> see it being used at all." you meant that since the driver doesn't
> redistribute any resource by declaring mfd_cell'es, doesn't
> register mfd-based sub-devices, and primary use-case of the boot
> controller is to access flash-devices, it should be just moved to the MTD
> subsystem. I don't think it would be better than to have a common part 
> defined here in MFD while ROM-specific part - in MTD, and SPI-specific - in
> the SPI subsystems. I would consider Baikal-T1 Boot Controller being similar
> to drivers/mfd/qcom-spmi-pmic.c, drivers/mfd/atmel-flexcom.c, etc, but
> instead of having GPIO, RTC, UART, i2c, etc sub-devices (which are also
> fully defied in dts), it exports MMIO-based ROMs and SPI-controller.

Are the ROMs all controlled via SPI?

> Lee, explain me please what is the difference between these MFDs and
> Baikal-T1 Boot Controller, that makes one simple MFDs suitable for the
> MFD subsystem, while another isn't?

Complexity.

[NB: Please Don't assume that just because I accepted a driver into
     MFD 6 years ago, that it would be accepted again today.]

To me this looks like an MTD device which is controlled via SPI.

The way I'm reading this currently, it might serve well to make the
MTD portion(s) children of the SPI controller.  I still do not see a
compelling reason to warrant adding a superfluous MFD driver if at all
avoidable.

> Miquel, Richard, Vignesh and everyone from MTD, who can help could you
> join this discussion and clarify whether the Baikal-T1 Boot Controller
> driver is supposed to be moved to the MTD subsystem? If so, then what is
> the better place to put it within the drivers/mtd/ directory tree?
> 
> > > Recently I've sent an RFC regarding a different question, but it
> > > concerns the Baikal-T1 system controller and the boot controller as being part
> > > of it:
> > > 
> > > https://lkml.org/lkml/2020/3/22/393
> > 

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Miquel Raynal]

Hi Lee, Sergey,

Lee Jones <lee.jones@linaro.org> wrote on Fri, 27 Mar 2020 09:01:39
+0000:

> On Thu, 26 Mar 2020, Sergey Semin wrote:
> 
> > Michael, Richard, Vignesh and MTD mailing list are Cc'ed to have their
> > comments on the issue.
> > 
> > My answers on the previous email are below.
> > 
> > On Thu, Mar 26, 2020 at 09:13:13AM +0000, Lee Jones wrote:  
> > > On Wed, 25 Mar 2020, Sergey Semin wrote:
> > >   
> > > > Hello Lee,
> > > > 
> > > > On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:  
> > > > > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > > > >   
> > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > > 
> > > > > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > > > > responsible for the chip proper pre-initialization and further
> > > > > > booting up from some memory device. From the Linux kernel point of view
> > > > > > it's just a multi-functional device, which exports three physically mapped
> > > > > > ROMs and a single SPI controller.
> > > > > > 
> > > > > > Primarily the ROMs are intended to be used for transparent access of
> > > > > > the memory devices with system bootup code. First ROM device is an
> > > > > > embedded into the SoC firmware, which is supposed to be used just for
> > > > > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > > > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > > > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > > > > chip pins selecting the system boot device.
> > > > > > 
> > > > > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > > > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > > > > SPI flash region shouldn't be accessed.
> > > > > > 
> > > > > > Taking into account all the peculiarities described above, we created
> > > > > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > > > > OF-based sub-device registration it also provides a simple API to
> > > > > > serialize an access to the external SPI flash from either the MMIO-based
> > > > > > SPI interface or embedded SPI controller.  
> > > > > 
> > > > > Not sure why this is being classified as an MFD.
> > > > > 
> > > > > This is clearly 'just' a memory device.
> > > > >   
> > > > 
> > > > Hm, I see this as a normal MFD device. The Boot controller provides a
> > > > set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> > > > controller. Yes, the SPI controller is normally utilized to access an external
> > > > flash device, and at boot stage it is used for it. But still it's a SPI
> > > > controller which driver belongs to the kernel SPI subsystem. Moreover
> > > > nothing prevents a platform designer from using it together with custom
> > > > GPIO-based chip-selects to have additional devices on the SPI bus.
> > > > 
> > > > As I said the problem is that an SPI flash might be accessed either with
> > > > use of a physically mapped ROM or via the normal DW APB SPI controller.
> > > > These two interfaces can't be used simultaneously, because the ROM is
> > > > just an rtl state-machine, which is built to translate MMIO operations
> > > > through the SPI controller registers to an external SPI-nor at CS0 of
> > > > the interface. That's why first I need to make sure the interface is locked,
> > > > then being enabled, then the corresponding driver can use it, then get
> > > > to unlock. That's the point of having the __bt1_bc_spi_lock() and
> > > > bt1_bc_spi_unlock() methods exported from the driver.
> > > > 
> > > > I've got two drivers for MTD ROM and SPI controller based on that
> > > > methods. But I haven't submitted them yet, because they belong to two
> > > > different subsystems and I need to have this one being accepted first.  
> > > 
> > > This is not a totally unique device/situation.  I've seen (and NACKed)
> > > this type of device before.  You need to explain this to the MTD
> > > (SPI-NOR?) maintainers.  They should be in a good position to provide
> > > guidance.
> > >   
> > 
> > Sorry, I don't really understand your justification. The boot controller
> > exports two types of devices: physically mapped ROMs and an DW APB
> > SSI-based SPI controller. Aside from being able to access an externally
> > attached SPI flash the SPI controller has normal SPI interface which in
> > general can be used to access any other SPI-slave. The complexity of
> > the case is that one of physically mapped ROM RTL uses the DW APB SSI
> > controller to access an external SPI flash, which when done makes the
> > DW APB SSI registers unusable on direct way. That's why I implemented the
> > boot controller as an MFD. An alternation caused by this peculiarity
> > will be submitted to drivers/mtd/maps/physmap-{core.c,baikal-t1-rom.c}
> > later after this change is reviewed and accepted. Similar situation
> > concerns a driver of DW APB SSI-based SPI controller. So according to
> > the current design the boot controller with it' sub-devices will be
> > declared in dts as follows:
> > 
> > boot: boot@1f040000 {
> > 	compatible = "be,bt1-boot-ctl";
> > 	reg = <0x1f040000 0x100>;
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	ranges;  
> 
> What control does this device offer in those 0x100 registers? 
> 
> > 	clocks = <&ccu_sys CCU_SYS_APB_CLK>;
> > 	clock-names = "pclk";
> > 
> > 	int_rom: rom@1bfc0000 {
> > 		compatible = "be,bt1-int-rom", "mtd-rom";
> > 		reg = <0x1bfc0000 0x10000>;
> > 		...
> > 	};
> > 
> > 	spi_rom: rom@1c000000 {
> > 		compatible = "be,bt1-ssi-rom", "mtd-rom";
> > 		reg = <0x1c000000 0x1000000>;
> > 		...
> > 	};
> > 
> > 	spi0: spi@1f040100 {
> > 		compatible = "be,bt1-boot-ssi";
> > 		reg = <0x1f040100 0xf00>;
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		clocks = <&ccu_sys CCU_SYS_SPI_CLK>;
> > 		clock-names = "ref";
> > 
> > 		...
> > 	};
> > 
> > 	boot_rom: rom@1fc00000 {
> > 		compatible = "be,bt1-boot-rom", "mtd-rom";
> > 		reg = <0x1fc00000 0x400000>;
> > 		...
> > 	};
> > };
> > 
> > I dare to assume, that by saying: "Despite including the MFD API, I don't
> > see it being used at all." you meant that since the driver doesn't
> > redistribute any resource by declaring mfd_cell'es, doesn't
> > register mfd-based sub-devices, and primary use-case of the boot
> > controller is to access flash-devices, it should be just moved to the MTD
> > subsystem. I don't think it would be better than to have a common part 
> > defined here in MFD while ROM-specific part - in MTD, and SPI-specific - in
> > the SPI subsystems. I would consider Baikal-T1 Boot Controller being similar
> > to drivers/mfd/qcom-spmi-pmic.c, drivers/mfd/atmel-flexcom.c, etc, but
> > instead of having GPIO, RTC, UART, i2c, etc sub-devices (which are also
> > fully defied in dts), it exports MMIO-based ROMs and SPI-controller.  
> 
> Are the ROMs all controlled via SPI?
> 
> > Lee, explain me please what is the difference between these MFDs and
> > Baikal-T1 Boot Controller, that makes one simple MFDs suitable for the
> > MFD subsystem, while another isn't?  
> 
> Complexity.
> 
> [NB: Please Don't assume that just because I accepted a driver into
>      MFD 6 years ago, that it would be accepted again today.]
> 
> To me this looks like an MTD device which is controlled via SPI.
> 
> The way I'm reading this currently, it might serve well to make the
> MTD portion(s) children of the SPI controller.  I still do not see a
> compelling reason to warrant adding a superfluous MFD driver if at all
> avoidable.
> 
> > Miquel, Richard, Vignesh and everyone from MTD, who can help could you
> > join this discussion and clarify whether the Baikal-T1 Boot Controller
> > driver is supposed to be moved to the MTD subsystem? If so, then what is
> > the better place to put it within the drivers/mtd/ directory tree?
> >   

Thank you for the detailed explanation. I'll try to bring useful
information to sort this out. IIUC, this bloc exports:

1/ One ROM located in the SoC
2/ The access to a possible second ROM over SPI

And also:

3/ Access to the SPI controller itself
4/ Access to 1/ or 2/ depending on an external switch.

IMHO:

1/ Might be seen as a random MTD device, its driver should be in
   /drivers/mtd/devices I guess.
3/ Is a SPI controller, its driver should be memory agnostic and
   located in /driver/spi/. However, it should probably implement
   the spi-mem infrastructure *and* the direct-mapping infrastructure
   which would automatically cover 2/ as well. The reg property of 2/
   should probably be part of 3/.

For 4/ I don't know what is the right thing to do yet.

What do you think?
 
> > > > Recently I've sent an RFC regarding a different question, but it
> > > > concerns the Baikal-T1 system controller and the boot controller as being part
> > > > of it:
> > > > 
> > > > https://lkml.org/lkml/2020/3/22/393  


Thanks,
Miquèl

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey Semin]

On Fri, Mar 27, 2020 at 09:01:39AM +0000, Lee Jones wrote:
> On Thu, 26 Mar 2020, Sergey Semin wrote:
> 
> > Michael, Richard, Vignesh and MTD mailing list are Cc'ed to have their
> > comments on the issue.
> > 
> > My answers on the previous email are below.
> > 
> > On Thu, Mar 26, 2020 at 09:13:13AM +0000, Lee Jones wrote:
> > > On Wed, 25 Mar 2020, Sergey Semin wrote:
> > > 
> > > > Hello Lee,
> > > > 
> > > > On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:
> > > > > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > > > > 
> > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > > 
> > > > > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > > > > responsible for the chip proper pre-initialization and further
> > > > > > booting up from some memory device. From the Linux kernel point of view
> > > > > > it's just a multi-functional device, which exports three physically mapped
> > > > > > ROMs and a single SPI controller.
> > > > > > 
> > > > > > Primarily the ROMs are intended to be used for transparent access of
> > > > > > the memory devices with system bootup code. First ROM device is an
> > > > > > embedded into the SoC firmware, which is supposed to be used just for
> > > > > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > > > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > > > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > > > > chip pins selecting the system boot device.
> > > > > > 
> > > > > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > > > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > > > > SPI flash region shouldn't be accessed.
> > > > > > 
> > > > > > Taking into account all the peculiarities described above, we created
> > > > > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > > > > OF-based sub-device registration it also provides a simple API to
> > > > > > serialize an access to the external SPI flash from either the MMIO-based
> > > > > > SPI interface or embedded SPI controller.
> > > > > 
> > > > > Not sure why this is being classified as an MFD.
> > > > > 
> > > > > This is clearly 'just' a memory device.
> > > > > 
> > > > 
> > > > Hm, I see this as a normal MFD device. The Boot controller provides a
> > > > set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> > > > controller. Yes, the SPI controller is normally utilized to access an external
> > > > flash device, and at boot stage it is used for it. But still it's a SPI
> > > > controller which driver belongs to the kernel SPI subsystem. Moreover
> > > > nothing prevents a platform designer from using it together with custom
> > > > GPIO-based chip-selects to have additional devices on the SPI bus.
> > > > 
> > > > As I said the problem is that an SPI flash might be accessed either with
> > > > use of a physically mapped ROM or via the normal DW APB SPI controller.
> > > > These two interfaces can't be used simultaneously, because the ROM is
> > > > just an rtl state-machine, which is built to translate MMIO operations
> > > > through the SPI controller registers to an external SPI-nor at CS0 of
> > > > the interface. That's why first I need to make sure the interface is locked,
> > > > then being enabled, then the corresponding driver can use it, then get
> > > > to unlock. That's the point of having the __bt1_bc_spi_lock() and
> > > > bt1_bc_spi_unlock() methods exported from the driver.
> > > > 
> > > > I've got two drivers for MTD ROM and SPI controller based on that
> > > > methods. But I haven't submitted them yet, because they belong to two
> > > > different subsystems and I need to have this one being accepted first.
> > > 
> > > This is not a totally unique device/situation.  I've seen (and NACKed)
> > > this type of device before.  You need to explain this to the MTD
> > > (SPI-NOR?) maintainers.  They should be in a good position to provide
> > > guidance.
> > > 
> > 
> > Sorry, I don't really understand your justification. The boot controller
> > exports two types of devices: physically mapped ROMs and an DW APB
> > SSI-based SPI controller. Aside from being able to access an externally
> > attached SPI flash the SPI controller has normal SPI interface which in
> > general can be used to access any other SPI-slave. The complexity of
> > the case is that one of physically mapped ROM RTL uses the DW APB SSI
> > controller to access an external SPI flash, which when done makes the
> > DW APB SSI registers unusable on direct way. That's why I implemented the
> > boot controller as an MFD. An alternation caused by this peculiarity
> > will be submitted to drivers/mtd/maps/physmap-{core.c,baikal-t1-rom.c}
> > later after this change is reviewed and accepted. Similar situation
> > concerns a driver of DW APB SSI-based SPI controller. So according to
> > the current design the boot controller with it' sub-devices will be
> > declared in dts as follows:
> > 
> > boot: boot@1f040000 {
> > 	compatible = "be,bt1-boot-ctl";
> > 	reg = <0x1f040000 0x100>;
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	ranges;
> 
> What control does this device offer in those 0x100 registers? 
> 

The main functionality of these registers is to switch the DW APB SSI
controller registers utilization: either make them being normally mapped
to the spi0:reg MMIO space (see spi0 node below) or set them being utilized
by the SPI ROM RTL, which is mapped to the spi_rom:reg ROM space. So in
first case the SPI-interface is handled by a generic SPI-controller,
while in the second one - it's purely subject of the SPI-flash access.

> > 	clocks = <&ccu_sys CCU_SYS_APB_CLK>;
> > 	clock-names = "pclk";
> > 
> > 	int_rom: rom@1bfc0000 {
> > 		compatible = "be,bt1-int-rom", "mtd-rom";
> > 		reg = <0x1bfc0000 0x10000>;
> > 		...
> > 	};
> > 

BTW Normally I add an SPI-flash subnode to the SPI-controller, which
will refer to the same SPI-flash as spi_rom:

> > 	spi_rom: rom@1c000000 {
> > 		compatible = "be,bt1-ssi-rom", "mtd-rom";
> > 		reg = <0x1c000000 0x1000000>;
> > 		...
> > 	};
> > 
> > 	spi0: spi@1f040100 {
> > 		compatible = "be,bt1-boot-ssi";
> > 		reg = <0x1f040100 0xf00>;
> > 		#address-cells = <1>;
> > 		#size-cells = <0>;
> > 
> > 		clocks = <&ccu_sys CCU_SYS_SPI_CLK>;
> > 		clock-names = "ref";
> > 
		spi_flash: flash@0 {
			compatible = "jedec,spi-nor";
			#address-cells = <1>;
			#size-cells = <1>;
			reg = <0>;

			spi-max-frequency = <25000000>;
		};
> > 	};
> > 

Now both spi_rom and spi_flash devices can be used to access the same
memory device. But in first case it would be the read-only access in
4-bytes chunks, while in the second case - read-write in up to the page
chunks. But in addition to the spi_flash, I could place there any other
SPI-slave device, like SPI-UART controller or SPI-SD selected by a GPIO
chip-select line, and normally this should work.

NOTE also that by adding the SPI-flash sub-node of spi0 in addition to
the spi_rom I totally follow the device tree requirement to reflect the
real hardware devices structure. So from DT point of view such
construction is totally allowable and even required.

> > 	boot_rom: rom@1fc00000 {
> > 		compatible = "be,bt1-boot-rom", "mtd-rom";
> > 		reg = <0x1fc00000 0x400000>;
> > 		...
> > 	};
> > };
> > 
> > I dare to assume, that by saying: "Despite including the MFD API, I don't
> > see it being used at all." you meant that since the driver doesn't
> > redistribute any resource by declaring mfd_cell'es, doesn't
> > register mfd-based sub-devices, and primary use-case of the boot
> > controller is to access flash-devices, it should be just moved to the MTD
> > subsystem. I don't think it would be better than to have a common part 
> > defined here in MFD while ROM-specific part - in MTD, and SPI-specific - in
> > the SPI subsystems. I would consider Baikal-T1 Boot Controller being similar
> > to drivers/mfd/qcom-spmi-pmic.c, drivers/mfd/atmel-flexcom.c, etc, but
> > instead of having GPIO, RTC, UART, i2c, etc sub-devices (which are also
> > fully defied in dts), it exports MMIO-based ROMs and SPI-controller.
> 
> Are the ROMs all controlled via SPI?
> 

No. int_rom is embedded into the Boot Controller firmware. It has
nothing concerning SPI. boot_rom is just a mirror of either spi_rom or int_rom
depending on the system bootup mode (selected by external pins settings).
So if SPI BOOT mode is enabled at the system startup, then boot_rom =
spi_rom (at first 4MB range), if ROM BOOT mode is enabled, then boot_rom = int_rom.

> > Lee, explain me please what is the difference between these MFDs and
> > Baikal-T1 Boot Controller, that makes one simple MFDs suitable for the
> > MFD subsystem, while another isn't?
> 
> Complexity.
> 
> [NB: Please Don't assume that just because I accepted a driver into
>      MFD 6 years ago, that it would be accepted again today.]
> 

Does this mean that accepting Qcom SPMI PMIC and Atmel flexcom drivers
to the MFD would be a mistake from current MFD point of view? What
changed since then?

> To me this looks like an MTD device which is controlled via SPI.
> 

If it was like this I wouldn't have had it submitted to the MFD subsystem.
Boot Controller has multiple sub-blocks embedded not only ROMs. By
saying so I refer to the DW APB SSI-based SPI-controller and it'
sub-nodes. See the next comment for details.

> The way I'm reading this currently, it might serve well to make the
> MTD portion(s) children of the SPI controller.  I still do not see a
> compelling reason to warrant adding a superfluous MFD driver if at all
> avoidable.
> 

Here what the Boot Controller is from hardware point of view. It's an RTL,
which includes the next functionality/subblocks:
1) Basic system initialization at the chip startup stage, when it's
powered on (like resetting the subblocks, strapping the boot mode, etc).
2) Internal ROM with a default firmware, which is embedded into the chip
and is run at the system startup when the corresponding boot mode is enabled.
Firmware is mapped to the int_rom:reg registers.
3) DW APB SSI-based SPI-controller, though with very limited resources
(no IRQ, no DMA, just 8 bytes FIFO). So it's just a normal SPI-controller.
4) SPI ROM RTL, which allows to access an externally attached 16MB SPI-nor at
SPI chip-select #0 via the spi_rom:reg memory space. When the
corresponding flag in the Boot Controller register is set the DW APB SSI
registers are acquired by this RTL and unattached from the spi0:reg
space to access the external SPI flash.
5) Boot ROM memory space, which is just a mirror of either SPI ROM or
Internal ROM depending on the boot mode pins state strapped at the chip
startup stage.

NOTE SPI ROM RTL (spi_rom) allows just READ-only access to the SPI-flash
and makes it in 4-bytes data chunks at a time which isn't very efficient.
While by means of the direct DW APB SSI controller usage (via spi0) the
SPI-flash can be read in much bigger chunks and, which is crucial, can be
written. Moreover if GPIO chip-select is available the SPI-controller can be
attached to any other SPI-slave (not only SPI-flash).

So, yes, Baikal-T1 Boot Controller is less complex than Qcom SPMI or
Atmel Flexcom, but as I see it, the controller still has multi-function
nature: ROMs + a specific SPI-controller.

If you are still in doubts whether the Boot Controller should be
identified as MFD and don't agree to have the driver here, then where
do you think it is supposed to go? To an SPI subsystem? No, because
it has embedded ROMs. To MTD? No, It has the SPI-controller. I thought,
that MFD is for any device, which includes blocks of different
functionalities (I2C, GPIO, SPI, ROMs, RTC, UART, etc). Which as I see it
Baikal-T1 Boot Controller is.

-Sergey

> > Miquel, Richard, Vignesh and everyone from MTD, who can help could you
> > join this discussion and clarify whether the Baikal-T1 Boot Controller
> > driver is supposed to be moved to the MTD subsystem? If so, then what is
> > the better place to put it within the drivers/mtd/ directory tree?
> > 
> > > > Recently I've sent an RFC regarding a different question, but it
> > > > concerns the Baikal-T1 system controller and the boot controller as being part
> > > > of it:
> > > > 
> > > > https://lkml.org/lkml/2020/3/22/393
> > > 
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 2/2] mfd: Add Baikal-T1 Boot Controller driver

[Sergey Semin]

Hello Miquel,

Thanks for clarifications. My comments are blow.

On Fri, Mar 27, 2020 at 11:25:30AM +0100, Miquel Raynal wrote:
> Hi Lee, Sergey,
> 
> Lee Jones <lee.jones@linaro.org> wrote on Fri, 27 Mar 2020 09:01:39
> +0000:
> 
> > On Thu, 26 Mar 2020, Sergey Semin wrote:
> > 
> > > Michael, Richard, Vignesh and MTD mailing list are Cc'ed to have their
> > > comments on the issue.
> > > 
> > > My answers on the previous email are below.
> > > 
> > > On Thu, Mar 26, 2020 at 09:13:13AM +0000, Lee Jones wrote:  
> > > > On Wed, 25 Mar 2020, Sergey Semin wrote:
> > > >   
> > > > > Hello Lee,
> > > > > 
> > > > > On Wed, Mar 25, 2020 at 10:09:40AM +0000, Lee Jones wrote:  
> > > > > > On Fri, 06 Mar 2020, Sergey.Semin@baikalelectronics.ru wrote:
> > > > > >   
> > > > > > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > > > > > > 
> > > > > > > Baikal-T1 Boot Controller is an IP block embedded into the SoC and
> > > > > > > responsible for the chip proper pre-initialization and further
> > > > > > > booting up from some memory device. From the Linux kernel point of view
> > > > > > > it's just a multi-functional device, which exports three physically mapped
> > > > > > > ROMs and a single SPI controller.
> > > > > > > 
> > > > > > > Primarily the ROMs are intended to be used for transparent access of
> > > > > > > the memory devices with system bootup code. First ROM device is an
> > > > > > > embedded into the SoC firmware, which is supposed to be used just for
> > > > > > > the chip debug and tests. Second ROM device provides a MMIO-based
> > > > > > > access to an external SPI flash. Third ROM mirrors either the Internal
> > > > > > > or SPI ROM region, depending on the state of the external BOOTCFG_{0,1}
> > > > > > > chip pins selecting the system boot device.
> > > > > > > 
> > > > > > > External SPI flash can be also accessed by the DW APB SSI SPI controller
> > > > > > > embedded into the Baikal-T1 Boot Controller. In this case the memory mapped
> > > > > > > SPI flash region shouldn't be accessed.
> > > > > > > 
> > > > > > > Taking into account all the peculiarities described above, we created
> > > > > > > an MFD-based driver for the Baikal-T1 controller. Aside from ordinary
> > > > > > > OF-based sub-device registration it also provides a simple API to
> > > > > > > serialize an access to the external SPI flash from either the MMIO-based
> > > > > > > SPI interface or embedded SPI controller.  
> > > > > > 
> > > > > > Not sure why this is being classified as an MFD.
> > > > > > 
> > > > > > This is clearly 'just' a memory device.
> > > > > >   
> > > > > 
> > > > > Hm, I see this as a normal MFD device. The Boot controller provides a
> > > > > set of physically mapped ROMs and a DW APB SSI-based embedded SPI
> > > > > controller. Yes, the SPI controller is normally utilized to access an external
> > > > > flash device, and at boot stage it is used for it. But still it's a SPI
> > > > > controller which driver belongs to the kernel SPI subsystem. Moreover
> > > > > nothing prevents a platform designer from using it together with custom
> > > > > GPIO-based chip-selects to have additional devices on the SPI bus.
> > > > > 
> > > > > As I said the problem is that an SPI flash might be accessed either with
> > > > > use of a physically mapped ROM or via the normal DW APB SPI controller.
> > > > > These two interfaces can't be used simultaneously, because the ROM is
> > > > > just an rtl state-machine, which is built to translate MMIO operations
> > > > > through the SPI controller registers to an external SPI-nor at CS0 of
> > > > > the interface. That's why first I need to make sure the interface is locked,
> > > > > then being enabled, then the corresponding driver can use it, then get
> > > > > to unlock. That's the point of having the __bt1_bc_spi_lock() and
> > > > > bt1_bc_spi_unlock() methods exported from the driver.
> > > > > 
> > > > > I've got two drivers for MTD ROM and SPI controller based on that
> > > > > methods. But I haven't submitted them yet, because they belong to two
> > > > > different subsystems and I need to have this one being accepted first.  
> > > > 
> > > > This is not a totally unique device/situation.  I've seen (and NACKed)
> > > > this type of device before.  You need to explain this to the MTD
> > > > (SPI-NOR?) maintainers.  They should be in a good position to provide
> > > > guidance.
> > > >   
> > > 
> > > Sorry, I don't really understand your justification. The boot controller
> > > exports two types of devices: physically mapped ROMs and an DW APB
> > > SSI-based SPI controller. Aside from being able to access an externally
> > > attached SPI flash the SPI controller has normal SPI interface which in
> > > general can be used to access any other SPI-slave. The complexity of
> > > the case is that one of physically mapped ROM RTL uses the DW APB SSI
> > > controller to access an external SPI flash, which when done makes the
> > > DW APB SSI registers unusable on direct way. That's why I implemented the
> > > boot controller as an MFD. An alternation caused by this peculiarity
> > > will be submitted to drivers/mtd/maps/physmap-{core.c,baikal-t1-rom.c}
> > > later after this change is reviewed and accepted. Similar situation
> > > concerns a driver of DW APB SSI-based SPI controller. So according to
> > > the current design the boot controller with it' sub-devices will be
> > > declared in dts as follows:
> > > 
> > > boot: boot@1f040000 {
> > > 	compatible = "be,bt1-boot-ctl";
> > > 	reg = <0x1f040000 0x100>;
> > > 	#address-cells = <1>;
> > > 	#size-cells = <1>;
> > > 	ranges;  
> > 
> > What control does this device offer in those 0x100 registers? 
> > 
> > > 	clocks = <&ccu_sys CCU_SYS_APB_CLK>;
> > > 	clock-names = "pclk";
> > > 
> > > 	int_rom: rom@1bfc0000 {
> > > 		compatible = "be,bt1-int-rom", "mtd-rom";
> > > 		reg = <0x1bfc0000 0x10000>;
> > > 		...
> > > 	};
> > > 
> > > 	spi_rom: rom@1c000000 {
> > > 		compatible = "be,bt1-ssi-rom", "mtd-rom";
> > > 		reg = <0x1c000000 0x1000000>;
> > > 		...
> > > 	};
> > > 
> > > 	spi0: spi@1f040100 {
> > > 		compatible = "be,bt1-boot-ssi";
> > > 		reg = <0x1f040100 0xf00>;
> > > 		#address-cells = <1>;
> > > 		#size-cells = <0>;
> > > 
> > > 		clocks = <&ccu_sys CCU_SYS_SPI_CLK>;
> > > 		clock-names = "ref";
> > > 
> > > 		...
> > > 	};
> > > 
> > > 	boot_rom: rom@1fc00000 {
> > > 		compatible = "be,bt1-boot-rom", "mtd-rom";
> > > 		reg = <0x1fc00000 0x400000>;
> > > 		...
> > > 	};
> > > };
> > > 
> > > I dare to assume, that by saying: "Despite including the MFD API, I don't
> > > see it being used at all." you meant that since the driver doesn't
> > > redistribute any resource by declaring mfd_cell'es, doesn't
> > > register mfd-based sub-devices, and primary use-case of the boot
> > > controller is to access flash-devices, it should be just moved to the MTD
> > > subsystem. I don't think it would be better than to have a common part 
> > > defined here in MFD while ROM-specific part - in MTD, and SPI-specific - in
> > > the SPI subsystems. I would consider Baikal-T1 Boot Controller being similar
> > > to drivers/mfd/qcom-spmi-pmic.c, drivers/mfd/atmel-flexcom.c, etc, but
> > > instead of having GPIO, RTC, UART, i2c, etc sub-devices (which are also
> > > fully defied in dts), it exports MMIO-based ROMs and SPI-controller.  
> > 
> > Are the ROMs all controlled via SPI?
> > 
> > > Lee, explain me please what is the difference between these MFDs and
> > > Baikal-T1 Boot Controller, that makes one simple MFDs suitable for the
> > > MFD subsystem, while another isn't?  
> > 
> > Complexity.
> > 
> > [NB: Please Don't assume that just because I accepted a driver into
> >      MFD 6 years ago, that it would be accepted again today.]
> > 
> > To me this looks like an MTD device which is controlled via SPI.
> > 
> > The way I'm reading this currently, it might serve well to make the
> > MTD portion(s) children of the SPI controller.  I still do not see a
> > compelling reason to warrant adding a superfluous MFD driver if at all
> > avoidable.
> > 
> > > Miquel, Richard, Vignesh and everyone from MTD, who can help could you
> > > join this discussion and clarify whether the Baikal-T1 Boot Controller
> > > driver is supposed to be moved to the MTD subsystem? If so, then what is
> > > the better place to put it within the drivers/mtd/ directory tree?
> > >   
> 
> Thank you for the detailed explanation. I'll try to bring useful
> information to sort this out. IIUC, this bloc exports:
> 

> 1/ One ROM located in the SoC
> 2/ The access to a possible second ROM over SPI
> 

Absolutely right. In this case these are the physically mapped ROMs in
the mtd subsystem notation. So they are the subjects of
drivers/mtd/maps/physmap-* driver. Though these ROMs require a
word-aligned access, so I had to implement a dedicated
driver/mtd/maps/physmap-bt1-rom.c code. I'll submit it after we
finish with this patchset, settle the issues raised in its framework.

> And also:
> 
> 3/ Access to the SPI controller itself

Absolutely right. It's a set of DW APB SSI registers, so normally it
would be a subject of drivers/spi/{spi-dw-mmio.c,spi-dw.c} driver, but
due to very limited resources (no DMA, no IRQ, just 8 bytes FIFO, a
single native chip-select) and a racy access from 2/, that code won't
work, but would only after a serious refactoring, so I had to create
a dedicated driver for it.

> 4/ Access to 1/ or 2/ depending on an external switch.
> 

Yes, it's another physically mapped ROM, which is a mirror of either 1/
or 2/ depending on the SoC boot mode strapped at the chip startup.

So to speak 1/ is always mapped (it's Internal ROM), 2/ is a physically
mapped SPI ROM, 3/ is an SPI controller itself primarily dedicated to
access the SPI ROM, and 4/ is a mirror of 1/ or 2/. The problem is in
simultaneous access to 2/ and 3/ and to 4/ when the SPI-boot mode is enabled.
Here is the Boot Controller structure together with comments of how my
current drivers design is using it:

Boot Controller (0x1f040000 0x100) - root MFD device.
|
+---+-> 1/ Internal ROM (0x1bfc0000 0x10000) - seen as /dev/mtdX (64KB of internal firmware).
|  \
+---+-> 4/ Boot ROM (0x1fc00000 0x400000) - seen as /dev/mtdY (4MB mirror of either Internal ROM or SPI ROM depending on the SoC boot mode strapped at the chip startup).
|  /
+---+-> 3/ SPI ROM (0x1c000000 0x1000000) - seen as /dev/mtdZ.
|   |
|  \  - This is a switch, which is handled by a flag in the Boot Controller register 0x1f040000.
+-- +-> 2/ DW APB SSI registers (0x1f040100 0xf00) - seen as SPI-dev +------> Connected to CS0 external SPI-nor flash - seen as mtdN
                                                                     |
                                                                     +------> Might be connected to any other SPI-slave with using GPIO-based chip-select.

So as you can see, a system software can access either SPI/Boot ROM or DW APB SSI
registers at a time, not simultaneously. That's why I needed a dedicated
API, which would serialize an access to the DW APB SSI registers. This
is what the MFD driver submitted by this patchset is intended for. It
provides a lock, which makes sure that either SPI/Boot ROM or DW APB SSI
is enabled while lock is held. In addition the MFD driver is responsible
for the sub-devices population.

See the https://lkml.org/lkml/2020/3/27/273 message for some more
details.

> IMHO:
> 
> 1/ Might be seen as a random MTD device, its driver should be in
>    /drivers/mtd/devices I guess.

Agreed. I've got a driver for it, but it will be
drivers/mtd/maps/physmap-bt1-rom.c (will use the common code
drivers/mtd/maps/physmap-core.c), since it's just a ROM. Alas the ROM
will have no direct mapping due to the word-aligned access restriction
(no-unaligned-direct-access property). Due to this restriction I also had
to create a dedicated code in drivers/mtd/maps/physmap-bt1-rom.c instead
of using generic "mtd-rom" compatible binding.

> 3/ Is a SPI controller, its driver should be memory agnostic and
>    located in /driver/spi/. However, it should probably implement
>    the spi-mem infrastructure *and* the direct-mapping infrastructure
>    which would automatically cover 2/ as well. The reg property of 2/
>    should probably be part of 3/.
> 

My new SPI-controller driver does implement the spi-mem infrastructure,
but it also implements a generic SPI callbacks so to allow any SPI-slave
(not only the Boot SPI-flash) being connected to the SPI bus. The
generic SPI callbacks are also required if GPIO-based chip-select is
available for the SPI-flash (don't really know why you permitted this
restriction in spi_mem_exec_op() instead of enabling the slave device at
the time of mem_op-based communications). Since 2/ and 4/ are handled by
drivers/mtd/maps/physmap-bt1-rom.c, there is no direct-mapping
implemented for this SPI-controller.

> For 4/ I don't know what is the right thing to do yet.
> 

That's one of the reason why I implemented it in a way it's implemented.
1/, 2/ and 4/ are the physically mapped ROMs handled by the
drivers/mtd/maps/physmap-bt1-rom.c driver, while 3/ is just an SPI-controller.
And all of them are parts of a single MFD - Baikal-T1 Boot Controller.
Moreover current Boot Controller DT-nodes structure is as much as possible
close to the structure of the device and its subnodes from hardware
point of view. Such design looked neat and elegant for me. It also
covered all parts of the RTL block.

> What do you think?
>  

Yes, your design was my first thought. I also had in mind to implement
2/ as a direct mapping of 3/ . But then I didn't find spi-nor using the
direct mapping interface, I also didn't come up with a solution for 4/
and seeing 1/, 2/ and 4/ having the same properties I decided that it
would be better to make them being exported as a dedicated mtd'es.

So, Miquèl, do you still think that my decision was wrong and it would be
better to create your version of the design? If so, folks, what to do with 4/
then, just drop?

Regards,
-Sergey

> > > > > Recently I've sent an RFC regarding a different question, but it
> > > > > concerns the Baikal-T1 system controller and the boot controller as being part
> > > > > of it:
> > > > > 
> > > > > https://lkml.org/lkml/2020/3/22/393  
> 
> 
> Thanks,
> Miquèl