[PATCH v5 0/7] Xilinx Zynq OCM support

[PATCH v5 0/7] Xilinx Zynq OCM support

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 5364 bytes --]

Hi,

this is the next attepmt to add support for On Chip Memory
configuration via On Chip Memory Controller.
OCM can be divided into 4 independend blocks and placed to
two locations which this driver detects.

For everybody on-chip SRAM driver "mmio-sram"
is missing parity IRQ handling not sure how to write
in generic way and also the memory layout
can be changed at run time (not currently supported by this driver)

smp-sram trampoline allocation can be used from mmio-sram
and size allocated via DT but currently no reason for using
it.

Creating mmio-sram node with setting at run time based on current setting
is possible but it won't look good.
One way how to do it is here
"ARM: mvebu: Add quirk for i2c for the OpenBlocks AX3-4 board"
(sha1: 85e618a1be2b2092318178d1d66bdad49cbbeeeb)
but creating nodes at run-time or changing compact strings
will be just more hacky code.

Using device-tree overlays is another option but SMP
trampoline code is placed there and it will be used by PM code
that's why driver should be there before user-space.

Next option is to create driver which just create platform device
at run-time. This is doable but I expect sram driver has to be
updated.

Regarding reading SLCR OCM configuration there is an option to use
regmap and not to use zynq_slcr_get_ocm_config.
This is how that code will look like when regmap is used.

+       struct regmap *syscon;
+
+       syscon = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+                                                "regmap");
+       if (IS_ERR(syscon)) {
+               dev_err(&pdev->dev, "regmap property not found\n");
+               return PTR_ERR(syscon);
+       }

-       ocm_config = zynq_slcr_get_ocm_config();
+       ret = regmap_read(syscon, SLCR_OCM_CFG_OFFSET,
+                         &ocm_config);
+       if (ret)
+               return -ENODEV;

Not sure which option is better but this is also working fine.
With this change patch
"ARM: zynq: Extend SLCR driver to read OCM configuration"
is not needed.

Regarding ifdef CONFIG_SMP driver allocates just size which
is used by the kernel - there could be an option to allocate
fixed size but it is IMHO worse then allocate actual needed
part.
With this simplification there is no need to refactor
common.h but it doesn't mean that refactoring is not good
to do.

Thanks,
Michal


Changes in v5:
- Separate binding from the driver
- Fix commit message reported by Linus W
- Use SZ_64K instead size in HEX reported by Linus W
- Fix dev_dbg message for allocate reset vector table
- Detect start address not base for SMP trampoline allocation
- Move binding to separate patch

Changes in v4:
- New patch in this series
- New patch in this series
- Move only slcr.h and smp.h and keep common.h in platform
  which is not needed by OCMC driver
  Based on Arnd request: https://lkml.org/lkml/2014/10/20/268
  Not all symbols from slcr.h/smp.h will be used by OCMC driver
  - no problem to remove them if it is needed
- Add record to MAINTAINERS file
- slcr.h has moved to soc/include/ folder. Move definition there too.
- Use }; instead of } ; in doc
- Use memory-controller@... instead of ocmc@...
- Create Kconfig entry for OCMC driver - enable GENERIC_ALLOCATOR here
- Add entry to MAINTAINERS file
- Use memory-controller@... instead of ocmc@...

Changes in v3:
- Move OCM to drivers/soc
- Update year
- Extract DTS node to be able to apply it out of driver
- Remove generic allocator enabling
- Extract SLCR part
- Use const in of_device_id
- OCM->OCMC
- Use ocmc-1.0 compatible string
- Extract from OCM driver

Changes in v2:
- Update pm.c added in 3.17 too - Soren pointed on it
- Change compatibility string to be in xilinx format
- Fix kernel-doc format

Michal Simek (7):
  ARM: zynq: Extract smp related functions out of common.h
  ARM: zynq: Extract slcr related functions out of common.h
  ARM: zynq: Move slcr.h and smp.h to generic location
  ARM: zynq: Extend SLCR driver to read OCM configuration
  devicetree: bindings: Add zynq ocmc node description
  ARM: zynq: Add OCM controller driver
  ARM: zynq: DT: Add OCM controller node

 .../bindings/arm/zynq/xlnx,zynq-ocmc.txt           |  17 ++
 MAINTAINERS                                        |   2 +
 arch/arm/boot/dts/zynq-7000.dtsi                   |   7 +
 arch/arm/mach-zynq/common.c                        |   2 +
 arch/arm/mach-zynq/common.h                        |  19 --
 arch/arm/mach-zynq/platsmp.c                       |   3 +
 arch/arm/mach-zynq/slcr.c                          |  17 ++
 drivers/soc/Kconfig                                |   1 +
 drivers/soc/Makefile                               |   1 +
 drivers/soc/zynq/Kconfig                           |  13 ++
 drivers/soc/zynq/Makefile                          |   1 +
 drivers/soc/zynq/zynq_ocmc.c                       | 249 +++++++++++++++++++++
 include/soc/zynq/slcr.h                            |  30 +++
 include/soc/zynq/smp.h                             |  30 +++
 14 files changed, 373 insertions(+), 19 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt
 create mode 100644 drivers/soc/zynq/Kconfig
 create mode 100644 drivers/soc/zynq/Makefile
 create mode 100644 drivers/soc/zynq/zynq_ocmc.c
 create mode 100644 include/soc/zynq/slcr.h
 create mode 100644 include/soc/zynq/smp.h

--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v5 5/7] devicetree: bindings: Add zynq ocmc node description

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 1331 bytes --]

Add binding for Zynq On Chip Memory controller driver
which is taking care about On Chip Memory.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>

---

Changes in v5:
- Separate binding from the driver

Changes in v4: None
Changes in v3: None
Changes in v2: None

 .../devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt     | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt

diff --git a/Documentation/devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt b/Documentation/devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt
new file mode 100644
index 000000000000..dcf221e8a16e
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/zynq/xlnx,zynq-ocmc.txt
@@ -0,0 +1,17 @@
+Device tree bindings for Zynq's OCM controller
+
+The OCM is divided to 4 64kB segments which can be separately configured
+to low or high location. Location is controlled via SLCR.
+
+Required properties:
+ compatible: Compatibility string. Must be "xlnx,zynq-ocmc-1.0".
+ reg: Specify the base and size of the OCMC registers in the memory map.
+      E.g.: reg = <0xf800c000 0x1000>;
+
+Example:
+ocmc: memory-controller@f800c000 {
+	compatible =  "xlnx,zynq-ocmc-1.0";
+	interrupt-parent = <&intc>;
+	interrupts = <0 3 4>;
+	reg = <0xf800c000 0x1000>;
+};
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v5 6/7] ARM: zynq: Add OCM controller driver

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 10562 bytes --]

The driver provide memory allocator which can
be used by others drivers to allocate memory inside OCM.
All locations for 64kB blocks are supported
and driver is trying to allocate the largest continuous
block of memory.

Checking mpcore addressing filterring is not done here
but could be added in future.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>

---

Changes in v5:
- Fix commit message reported by Linus W
- Use SZ_64K instead size in HEX reported by Linus W
- Fix dev_dbg message for allocate reset vector table
- Detect start address not base for SMP trampoline allocation
- Move binding to separate patch

Changes in v4:
- Use }; instead of } ; in doc
- Use memory-controller@... instead of ocmc@...
- Create Kconfig entry for OCMC driver - enable GENERIC_ALLOCATOR here
- Add entry to MAINTAINERS file

Changes in v3:
- Move OCM to drivers/soc
- Update year
- Extract DTS node to be able to apply it out of driver
- Remove generic allocator enabling
- Extract SLCR part
- Use const in of_device_id
- OCM->OCMC
- Use ocmc-1.0 compatible string

Changes in v2:
- Change compatibility string to be in xilinx format
- Fix kernel-doc format

 MAINTAINERS                  |   1 +
 drivers/soc/Kconfig          |   1 +
 drivers/soc/Makefile         |   1 +
 drivers/soc/zynq/Kconfig     |  13 +++
 drivers/soc/zynq/Makefile    |   1 +
 drivers/soc/zynq/zynq_ocmc.c | 249 +++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 266 insertions(+)
 create mode 100644 drivers/soc/zynq/Kconfig
 create mode 100644 drivers/soc/zynq/Makefile
 create mode 100644 drivers/soc/zynq/zynq_ocmc.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 7b6ed1f667ad..ad56d68273fe 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1550,6 +1550,7 @@ S:	Supported
 F:	arch/arm/mach-zynq/
 F:	drivers/cpuidle/cpuidle-zynq.c
 F:	drivers/block/xsysace.c
+F:	drivers/soc/zynq/
 F:	include/soc/zynq/
 N:	zynq
 N:	xilinx
diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
index 76d6bd4da138..25cc114a982d 100644
--- a/drivers/soc/Kconfig
+++ b/drivers/soc/Kconfig
@@ -3,5 +3,6 @@ menu "SOC (System On Chip) specific Drivers"
 source "drivers/soc/qcom/Kconfig"
 source "drivers/soc/ti/Kconfig"
 source "drivers/soc/versatile/Kconfig"
+source "drivers/soc/zynq/Kconfig"

 endmenu
diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
index 063113d0bd38..77a7865a3367 100644
--- a/drivers/soc/Makefile
+++ b/drivers/soc/Makefile
@@ -6,3 +6,4 @@ obj-$(CONFIG_ARCH_QCOM)		+= qcom/
 obj-$(CONFIG_ARCH_TEGRA)	+= tegra/
 obj-$(CONFIG_SOC_TI)		+= ti/
 obj-$(CONFIG_PLAT_VERSATILE)	+= versatile/
+obj-$(CONFIG_ARCH_ZYNQ)		+= zynq/
diff --git a/drivers/soc/zynq/Kconfig b/drivers/soc/zynq/Kconfig
new file mode 100644
index 000000000000..0163c54acbc6
--- /dev/null
+++ b/drivers/soc/zynq/Kconfig
@@ -0,0 +1,13 @@
+#
+# Xilnx Zynq SoC drivers
+#
+
+if ARCH_ZYNQ
+
+config ZYNQ_OCMC
+	def_bool y
+	select GENERIC_ALLOCATOR
+	help
+	  Xilinx Zynq On Chip Memory Controller driver
+
+endif
diff --git a/drivers/soc/zynq/Makefile b/drivers/soc/zynq/Makefile
new file mode 100644
index 000000000000..807c9b83850d
--- /dev/null
+++ b/drivers/soc/zynq/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_ZYNQ_OCMC) += zynq_ocmc.o
diff --git a/drivers/soc/zynq/zynq_ocmc.c b/drivers/soc/zynq/zynq_ocmc.c
new file mode 100644
index 000000000000..1237ff702ad0
--- /dev/null
+++ b/drivers/soc/zynq/zynq_ocmc.c
@@ -0,0 +1,249 @@
+/*
+ * Copyright (C) 2013 - 2014 Xilinx
+ *
+ * Based on "Generic on-chip SRAM allocation driver"
+ *
+ * Copyright (C) 2012 Philipp Zabel, Pengutronix
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/err.h>
+#include <linux/genalloc.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of_irq.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+
+#include <soc/zynq/slcr.h>
+#include <soc/zynq/smp.h>
+
+#define ZYNQ_OCMC_HIGHADDR	0xfffc0000
+#define ZYNQ_OCMC_LOWADDR	0x0
+#define ZYNQ_OCMC_BLOCK_SIZE	SZ_64K
+#define ZYNQ_OCMC_BLOCKS		4
+#define ZYNQ_OCMC_GRANULARITY	32
+
+#define ZYNQ_OCMC_PARITY_CTRL	0x0
+#define ZYNQ_OCMC_PARITY_ENABLE	0x1e
+
+#define ZYNQ_OCMC_PARITY_ERRADDRESS	0x4
+
+#define ZYNQ_OCMC_IRQ_STS		0x8
+#define ZYNQ_OCMC_IRQ_STS_ERR_MASK	0x7
+
+struct zynq_ocmc_dev {
+	void __iomem *base;
+	int irq;
+	struct gen_pool *pool;
+	struct resource res[ZYNQ_OCMC_BLOCKS];
+};
+
+/**
+ * zynq_ocmc_irq_handler - Interrupt service routine of the OCM controller
+ * @irq:	IRQ number
+ * @data:	Pointer to the zynq_ocmc_dev structure
+ *
+ * Return:     IRQ_HANDLED always
+ */
+static irqreturn_t zynq_ocmc_irq_handler(int irq, void *data)
+{
+	u32 sts;
+	u32 err_addr;
+	struct zynq_ocmc_dev *zynq_ocmc = data;
+
+	/* check status */
+	sts = readl(zynq_ocmc->base + ZYNQ_OCMC_IRQ_STS);
+	if (sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK) {
+		/* check error address */
+		err_addr = readl(zynq_ocmc->base + ZYNQ_OCMC_PARITY_ERRADDRESS);
+		pr_err("%s: OCM err intr generated at 0x%04x (stat: 0x%08x).",
+		       __func__, err_addr, sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK);
+	}
+	pr_warn("%s: Interrupt generated by OCM, but no error is found.",
+		__func__);
+
+	return IRQ_HANDLED;
+}
+
+/**
+ * zynq_ocmc_probe - Probe method for the OCM driver
+ * @pdev:	Pointer to the platform_device structure
+ *
+ * This function initializes the driver data structures and the hardware.
+ *
+ * Return:	0 on success and error value on failure
+ */
+static int zynq_ocmc_probe(struct platform_device *pdev)
+{
+	int ret;
+	struct zynq_ocmc_dev *zynq_ocmc;
+	u32 i, ocm_config, curr;
+	struct resource *res;
+
+	ocm_config = zynq_slcr_get_ocm_config();
+
+	zynq_ocmc = devm_kzalloc(&pdev->dev, sizeof(*zynq_ocmc), GFP_KERNEL);
+	if (!zynq_ocmc)
+		return -ENOMEM;
+
+	zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
+					       ilog2(ZYNQ_OCMC_GRANULARITY),
+					       -1);
+	if (!zynq_ocmc->pool)
+		return -ENOMEM;
+
+	curr = 0; /* For storing current struct resource for OCM */
+	for (i = 0; i < ZYNQ_OCMC_BLOCKS; i++) {
+		u32 base, start, end;
+
+		/* Setup base address for 64kB OCM block */
+		if (ocm_config & BIT(i))
+			base = ZYNQ_OCMC_HIGHADDR;
+		else
+			base = ZYNQ_OCMC_LOWADDR;
+
+		/* Calculate start and end block addresses */
+		start = i * ZYNQ_OCMC_BLOCK_SIZE + base;
+		end = start + (ZYNQ_OCMC_BLOCK_SIZE - 1);
+
+		/* Concatenate OCM blocks together to get bigger pool */
+		if (i > 0 && start == (zynq_ocmc->res[curr - 1].end + 1)) {
+			zynq_ocmc->res[curr - 1].end = end;
+		} else {
+#ifdef CONFIG_SMP
+			/*
+			 * OCM block if placed at 0x0 has special meaning
+			 * for SMP because jump trampoline is added there.
+			 * Ensure that this address won't be allocated.
+			 */
+			if (!start) {
+				u32 trampoline_code_size =
+					&zynq_secondary_trampoline_end -
+					&zynq_secondary_trampoline;
+				dev_dbg(&pdev->dev,
+					"Allocate reset vector table %dBytes\n",
+					trampoline_code_size);
+				/* Postpone start offset */
+				start += trampoline_code_size;
+			}
+#endif
+			/* First resource is always initialized */
+			zynq_ocmc->res[curr].start = start;
+			zynq_ocmc->res[curr].end = end;
+			zynq_ocmc->res[curr].flags = IORESOURCE_MEM;
+			curr++; /* Increment curr value */
+		}
+		dev_dbg(&pdev->dev, "OCM block %d, start %x, end %x\n",
+			i, start, end);
+	}
+
+	/*
+	 * Separate pool allocation from OCM block detection to ensure
+	 * the biggest possible pool.
+	 */
+	for (i = 0; i < ZYNQ_OCMC_BLOCKS; i++) {
+		unsigned long size;
+		void __iomem *virt_base;
+
+		/* Skip all zero size resources */
+		if (zynq_ocmc->res[i].end == 0)
+			break;
+		dev_dbg(&pdev->dev, "OCM resources %d, start %x, end %x\n",
+			i, zynq_ocmc->res[i].start, zynq_ocmc->res[i].end);
+		size = resource_size(&zynq_ocmc->res[i]);
+		virt_base = devm_ioremap_resource(&pdev->dev,
+						  &zynq_ocmc->res[i]);
+		if (IS_ERR(virt_base))
+			return PTR_ERR(virt_base);
+
+		ret = gen_pool_add_virt(zynq_ocmc->pool,
+					(unsigned long)virt_base,
+					zynq_ocmc->res[i].start, size, -1);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Gen pool failed\n");
+			return ret;
+		}
+		dev_info(&pdev->dev, "ZYNQ OCM pool: %ld KiB @ 0x%p\n",
+			 size / 1024, virt_base);
+	}
+
+	/* Get OCM config space */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	zynq_ocmc->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(zynq_ocmc->base))
+		return PTR_ERR(zynq_ocmc->base);
+
+	/* Allocate OCM parity IRQ */
+	zynq_ocmc->irq = platform_get_irq(pdev, 0);
+	if (zynq_ocmc->irq < 0) {
+		dev_err(&pdev->dev, "irq resource not found\n");
+		return zynq_ocmc->irq;
+	}
+	ret = devm_request_irq(&pdev->dev, zynq_ocmc->irq,
+			       zynq_ocmc_irq_handler,
+			       0, pdev->name, zynq_ocmc);
+	if (ret != 0) {
+		dev_err(&pdev->dev, "request_irq failed\n");
+		return ret;
+	}
+
+	/* Enable parity errors */
+	writel(ZYNQ_OCMC_PARITY_ENABLE,
+	       zynq_ocmc->base + ZYNQ_OCMC_PARITY_CTRL);
+
+	platform_set_drvdata(pdev, zynq_ocmc);
+
+	return 0;
+}
+
+/**
+ * zynq_ocmc_remove - Remove method for the OCM driver
+ * @pdev:	Pointer to the platform_device structure
+ *
+ * Return:	0 on success and error value on failure
+ *
+ * This function is called if a device is physically removed from the system or
+ * if the driver module is being unloaded. It frees all resources allocated to
+ * the device.
+ */
+static int zynq_ocmc_remove(struct platform_device *pdev)
+{
+	struct zynq_ocmc_dev *zynq_ocmc = platform_get_drvdata(pdev);
+
+	dev_info(&pdev->dev, "Removed\n");
+	if (gen_pool_avail(zynq_ocmc->pool) < gen_pool_size(zynq_ocmc->pool))
+		dev_dbg(&pdev->dev, "removed while SRAM allocated\n");
+
+	return 0;
+}
+
+static const struct of_device_id zynq_ocmc_dt_ids[] = {
+	{ .compatible = "xlnx,zynq-ocmc-1.0" },
+	{ /* end of table */ }
+};
+
+static struct platform_driver zynq_ocmc_driver = {
+	.driver = {
+		.name = "zynq_ocmc",
+		.of_match_table = zynq_ocmc_dt_ids,
+	},
+	.probe = zynq_ocmc_probe,
+	.remove = zynq_ocmc_remove,
+};
+
+static int __init zynq_ocmc_init(void)
+{
+	return platform_driver_register(&zynq_ocmc_driver);
+}
+
+arch_initcall(zynq_ocmc_init);
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v5 7/7] ARM: zynq: DT: Add OCM controller node

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

Add the on-chip-memory controller node to the Zynq devicetree.

Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---

Changes in v5: None
Changes in v4:
- Use memory-controller@... instead of ocmc@...

Changes in v3:
- Extract from OCM driver

Changes in v2: None

 arch/arm/boot/dts/zynq-7000.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi
index 24036c440440..040f95e817c8 100644
--- a/arch/arm/boot/dts/zynq-7000.dtsi
+++ b/arch/arm/boot/dts/zynq-7000.dtsi
@@ -151,6 +151,13 @@
 			reg = <0xf8006000 0x1000>;
 		} ;

+		ocmc: memory-controller@f800c000 {
+			compatible = "xlnx,zynq-ocmc-1.0";
+			interrupt-parent = <&intc>;
+			interrupts = <0 3 4>;
+			reg = <0xf800c000 0x1000>;
+		};
+
 		uart0: serial@e0000000 {
 			compatible = "xlnx,xuartps", "cdns,uart-r1p8";
 			status = "disabled";
--
1.8.2.3


[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v5 6/7] ARM: zynq: Add OCM controller driver

[Arnd Bergmann]

On Monday 01 December 2014 14:24:57 Michal Simek wrote:
> The driver provide memory allocator which can
> be used by others drivers to allocate memory inside OCM.
> All locations for 64kB blocks are supported
> and driver is trying to allocate the largest continuous
> block of memory.
> 
> Checking mpcore addressing filterring is not done here
> but could be added in future.
> 
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

The driver doesn't actually have any interface as far as I can tell,
so I wonder how you expect it to be used.

Do you have a list of drivers that would be using it?

How does it relate to the generic "mmio-sram" binding? Is this
meant as a specialization?

> +/**
> + * zynq_ocmc_irq_handler - Interrupt service routine of the OCM controller
> + * @irq:	IRQ number
> + * @data:	Pointer to the zynq_ocmc_dev structure
> + *
> + * Return:     IRQ_HANDLED always
> + */
> +static irqreturn_t zynq_ocmc_irq_handler(int irq, void *data)
> +{
> +	u32 sts;
> +	u32 err_addr;
> +	struct zynq_ocmc_dev *zynq_ocmc = data;
> +
> +	/* check status */
> +	sts = readl(zynq_ocmc->base + ZYNQ_OCMC_IRQ_STS);
> +	if (sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK) {
> +		/* check error address */
> +		err_addr = readl(zynq_ocmc->base + ZYNQ_OCMC_PARITY_ERRADDRESS);
> +		pr_err("%s: OCM err intr generated at 0x%04x (stat: 0x%08x).",
> +		       __func__, err_addr, sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK);
> +	}
> +	pr_warn("%s: Interrupt generated by OCM, but no error is found.",
> +		__func__);
> +
> +	return IRQ_HANDLED;
> +}

I'm also puzzled by this: you don't really do anything here but print
a message. What is the purpose of this interrupt? As this looks unrelated
to the rest of the driver, maybe this should be part of drivers/edac
instead?

> +static int zynq_ocmc_probe(struct platform_device *pdev)
> +{
> +       int ret;
> +       struct zynq_ocmc_dev *zynq_ocmc;
> +       u32 i, ocm_config, curr;
> +       struct resource *res;
> +
> +       ocm_config = zynq_slcr_get_ocm_config();
> +
> +       zynq_ocmc = devm_kzalloc(&pdev->dev, sizeof(*zynq_ocmc),
> GFP_KERNEL);
> +       if (!zynq_ocmc)
> +               return -ENOMEM;
> +
> +       zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
> +                                              ilog2(ZYNQ_OCMC_GRANULARITY),
> +                                              -1);
> +       if (!zynq_ocmc->pool)
> +               return -ENOMEM;
> +
> +       curr = 0; /* For storing current struct resource for OCM */
> +       for (i = 0; i < ZYNQ_OCMC_BLOCKS; i++) {
> +               u32 base, start, end;
> +
> +               /* Setup base address for 64kB OCM block */
> +               if (ocm_config & BIT(i))
> +                       base = ZYNQ_OCMC_HIGHADDR;
> +               else
> +                       base = ZYNQ_OCMC_LOWADDR;

You write in the introductory mail that you want to support detecting the
address from the slcr, and possibly even allow changing it at runtime,
but I don't understand what that would be good for.

It's not uncommon to describe in DT settings that one can also find
out from hardware but that are set up by the boot loader in a particular
way. Why not this one? For all I can tell, that would let you simply
use one driver for the EDAC and the generic mmio-sram from the memory,
without the need to do anything further.

	Arnd

Re: [PATCH v5 6/7] ARM: zynq: Add OCM controller driver

[Michal Simek]

On 12/01/2014 04:31 PM, Arnd Bergmann wrote:
> On Monday 01 December 2014 14:24:57 Michal Simek wrote:
>> The driver provide memory allocator which can
>> be used by others drivers to allocate memory inside OCM.
>> All locations for 64kB blocks are supported
>> and driver is trying to allocate the largest continuous
>> block of memory.
>>
>> Checking mpcore addressing filterring is not done here
>> but could be added in future.
>>
>> Signed-off-by: Michal Simek <michal.simek-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org>
> 
> The driver doesn't actually have any interface as far as I can tell,
> so I wonder how you expect it to be used.
> 
> Do you have a list of drivers that would be using it?

For supporting suspend you have to save content to OCM memory
and you need any driver which is providing generic functionality
to allocate that memory for it.

Here is example - it is not upstream because we need to get OCM driver
to mainline first.
https://github.com/Xilinx/linux-xlnx/blob/master/arch/arm/mach-zynq/pm.c

The second example is to use OCM memory for storing BD for ethernet controller.
OCM is fast memory accessible IRC in one cycle which increase bandwidth.

For all of these you need the driver which provide you the way how to allocate
memory from OCM.

> How does it relate to the generic "mmio-sram" binding? Is this
> meant as a specialization?

This driver was based on mmio-sram which does the same. Provide you a way
how to allocate memory space in any sram memory on chip.
It is design for static case but not for dynamic one like this one.

As I pointed in cover letter there could be an option to use mmio-sram driver
but there must be any way how to create DT description at run-time or choose
configuration at run-time.

>> +/**
>> + * zynq_ocmc_irq_handler - Interrupt service routine of the OCM controller
>> + * @irq:	IRQ number
>> + * @data:	Pointer to the zynq_ocmc_dev structure
>> + *
>> + * Return:     IRQ_HANDLED always
>> + */
>> +static irqreturn_t zynq_ocmc_irq_handler(int irq, void *data)
>> +{
>> +	u32 sts;
>> +	u32 err_addr;
>> +	struct zynq_ocmc_dev *zynq_ocmc = data;
>> +
>> +	/* check status */
>> +	sts = readl(zynq_ocmc->base + ZYNQ_OCMC_IRQ_STS);
>> +	if (sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK) {
>> +		/* check error address */
>> +		err_addr = readl(zynq_ocmc->base + ZYNQ_OCMC_PARITY_ERRADDRESS);
>> +		pr_err("%s: OCM err intr generated at 0x%04x (stat: 0x%08x).",
>> +		       __func__, err_addr, sts & ZYNQ_OCMC_IRQ_STS_ERR_MASK);
>> +	}
>> +	pr_warn("%s: Interrupt generated by OCM, but no error is found.",
>> +		__func__);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
> I'm also puzzled by this: you don't really do anything here but print
> a message. What is the purpose of this interrupt? As this looks unrelated
> to the rest of the driver, maybe this should be part of drivers/edac
> instead?

yes - it is just showing that there is error in OCM.
It can be added to edac - no problem to do so but the driver will be
really simple that's why not sure if make sense to do it.

> 
>> +static int zynq_ocmc_probe(struct platform_device *pdev)
>> +{
>> +       int ret;
>> +       struct zynq_ocmc_dev *zynq_ocmc;
>> +       u32 i, ocm_config, curr;
>> +       struct resource *res;
>> +
>> +       ocm_config = zynq_slcr_get_ocm_config();
>> +
>> +       zynq_ocmc = devm_kzalloc(&pdev->dev, sizeof(*zynq_ocmc),
>> GFP_KERNEL);
>> +       if (!zynq_ocmc)
>> +               return -ENOMEM;
>> +
>> +       zynq_ocmc->pool = devm_gen_pool_create(&pdev->dev,
>> +                                              ilog2(ZYNQ_OCMC_GRANULARITY),
>> +                                              -1);
>> +       if (!zynq_ocmc->pool)
>> +               return -ENOMEM;
>> +
>> +       curr = 0; /* For storing current struct resource for OCM */
>> +       for (i = 0; i < ZYNQ_OCMC_BLOCKS; i++) {
>> +               u32 base, start, end;
>> +
>> +               /* Setup base address for 64kB OCM block */
>> +               if (ocm_config & BIT(i))
>> +                       base = ZYNQ_OCMC_HIGHADDR;
>> +               else
>> +                       base = ZYNQ_OCMC_LOWADDR;
> 
> You write in the introductory mail that you want to support detecting the
> address from the slcr, and possibly even allow changing it at runtime,
> but I don't understand what that would be good for.

Note: PL means programmable logic

On zynq you can use memory from fixed part which starts from 0 to ddr size.
That's one configuration.
The second is to add memory controller just to PL - the use case can be that
memory controller in PL will provide better bandwidth (because of customization)
for your application and this memory can't started from 0. Most of the time
it starts from 0x40000000. But you need to have memory at 0 for adding reset vectors
or standalone application running on the second ARM core that's why you can
place there OCM.

It means it is up to users what configuration they want/need to use that's why
OCM is divided to 4 64k blocks which you can place to any location you like.
it is 16 combinations.

Chip can be setup to use any of this combination without any capable bootloader
that's why relating on bootloader is not solution. Also problematic for upgrading
the system.

> It's not uncommon to describe in DT settings that one can also find
> out from hardware but that are set up by the boot loader in a particular
> way. Why not this one? 

It is 16 combinations how these 4 blocks can be placed together but only
one reliable way is to check SLCR configuration and based on reg setup.

I can add all these 16 combinations to DTS and detect configuration in SLCR
and then change any status property for that particular configuration
but it will be more ugly than this solution.

Note: Just place there 4/8 blocks with low and high locations is not good
because the maximum allocated size will be just 64k.

> For all I can tell, that would let you simply
> use one driver for the EDAC and the generic mmio-sram from the memory,
> without the need to do anything further.

EDAC driver - no problem to do it and make sense.
Using mmio-sram for one fixed configuration is easy but not sure if this is
the right thing to do.
Listing all combinations in DTS file is quite easy to do but mechanism
to select one combination is the key for it.

Thanks,
Michal

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html