[PATCH 0/4] RFC: OMAP GPMC bindings

[PATCH 0/4] RFC: OMAP GPMC bindings

[Daniel Mack]

This is a series of patches to support GPMC peripherals on OMAP boards.

Depends on Linus' master +
omap-next (branch omap-for-v3.8/cleanup-headers-gpmc)

The only supported peripheral for now is NAND, but other types would be
easy to add.

In order to make the gpmc driver the 'hub' for all sub-nodes, I had to
add some includes. I'm not particularily happy about the fact that there
are mutual references from gpmc.c to gpmc-nand.c now, but that was the
easiest way.

I successfully tested these patches on an AM33xx board to make NAND
work.

Comments welcome!


Thanks,
Daniel


Daniel Mack (4):
  mtd: omap-nand: pass device_node in platform data
  ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs
  ARM: OMAP: gpmc: don't create devices from initcall on DT
  OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

 Documentation/devicetree/bindings/bus/gpmc.txt     |  59 +++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  65 +++++++++
 arch/arm/mach-omap2/gpmc-nand.c                    |   2 +-
 arch/arm/mach-omap2/gpmc.c                         | 146 +++++++++++++++++++++
 drivers/mtd/nand/omap2.c                           |   4 +-
 include/linux/platform_data/mtd-nand-omap2.h       |   2 +
 6 files changed, 276 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/bus/gpmc.txt
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

-- 
1.7.11.7

[PATCH 1/4] mtd: omap-nand: pass device_node in platform data

[Daniel Mack]

Pass an optional device_node pointer in the platform data, which in turn
will be put into a mtd_part_parser_data. This way, code that sets up the
platform devices can pass along the node from DT so that the partitions
can be parsed.

For non-DT boards, this change has no effect.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 drivers/mtd/nand/omap2.c                     | 4 +++-
 include/linux/platform_data/mtd-nand-omap2.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/omap2.c b/drivers/mtd/nand/omap2.c
index 3282b15..a733f15 100644
--- a/drivers/mtd/nand/omap2.c
+++ b/drivers/mtd/nand/omap2.c
@@ -1331,6 +1331,7 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 	dma_cap_mask_t mask;
 	unsigned sig;
 	struct resource			*res;
+	struct mtd_part_parser_data	ppdata = {};
 
 	pdata = pdev->dev.platform_data;
 	if (pdata == NULL) {
@@ -1556,7 +1557,8 @@ static int __devinit omap_nand_probe(struct platform_device *pdev)
 		goto out_release_mem_region;
 	}
 
-	mtd_device_parse_register(&info->mtd, NULL, NULL, pdata->parts,
+	ppdata.of_node = pdata->of_node;
+	mtd_device_parse_register(&info->mtd, NULL, &ppdata, pdata->parts,
 				  pdata->nr_parts);
 
 	platform_set_drvdata(pdev, &info->mtd);
diff --git a/include/linux/platform_data/mtd-nand-omap2.h b/include/linux/platform_data/mtd-nand-omap2.h
index 24d32ca..5217d6e 100644
--- a/include/linux/platform_data/mtd-nand-omap2.h
+++ b/include/linux/platform_data/mtd-nand-omap2.h
@@ -60,6 +60,8 @@ struct omap_nand_platform_data {
 	int			devsize;
 	enum omap_ecc           ecc_opt;
 	struct gpmc_nand_regs	reg;
+	/* for passing the partitions */
+	struct device_node	*of_node;
 };
 
 #endif
-- 
1.7.11.7

[PATCH 2/4] ARM: OMAP: gpmc: enable hwecc for AM33xx SoCs

[Daniel Mack]

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/mach-omap2/gpmc-nand.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 8607735..c3616c6 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -92,7 +92,7 @@ static int omap2_nand_gpmc_retime(
 static bool __init gpmc_hwecc_bch_capable(enum omap_ecc ecc_opt)
 {
 	/* support only OMAP3 class */
-	if (!cpu_is_omap34xx()) {
+	if (!cpu_is_omap34xx() && !soc_is_am33xx()) {
 		pr_err("BCH ecc is not supported on this CPU\n");
 		return 0;
 	}
-- 
1.7.11.7

[PATCH 3/4] ARM: OMAP: gpmc: don't create devices from initcall on DT

[Daniel Mack]

On DT driven boards, the gpmc node will match the driver. Hence, there's
no need to do that unconditionally from the initcall.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 arch/arm/mach-omap2/gpmc.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 60f1cce..1dcb30c 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -844,6 +844,13 @@ static int __init omap_gpmc_init(void)
 	struct platform_device *pdev;
 	char *oh_name = "gpmc";
 
+	/*
+	 * if the board boots up with a populated DT, do not
+	 * manually add the device from this initcall
+	 */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
 	oh = omap_hwmod_lookup(oh_name);
 	if (!oh) {
 		pr_err("Could not look up %s\n", oh_name);
-- 
1.7.11.7

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

This patch adds basic DT bindings for OMAP GPMC.

The actual peripherals are instanciated from child nodes within the GPMC
node, and the only type of device that is currently supported is NAND.

Code was added to parse the generic GPMC timing parameters and some
documentation with examples on how to use them.

Successfully tested on an AM33xx board.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 Documentation/devicetree/bindings/bus/gpmc.txt     |  59 +++++++++
 .../devicetree/bindings/mtd/gpmc-nand.txt          |  65 ++++++++++
 arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
 3 files changed, 263 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/bus/gpmc.txt
 create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt

diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
new file mode 100644
index 0000000..ef1c6e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/bus/gpmc.txt
@@ -0,0 +1,59 @@
+Device tree bindings for OMAP general purpose memory controllers (GPMC)
+
+The actual devices are instantiated from the child nodes of a GPMC node.
+
+Required properties:
+
+ - compatible: Should be set to "ti,gpmc"
+
+Timing properties for child nodes. All are optional and default to 0.
+
+ - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
+
+ Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
+ - gpmc,cs-on:		Assertion time
+ - gpmc,cs-rd-off:	Read deassertion time
+ - gpmc,cs-wr-off:	Write deassertion time
+
+ ADV signal timings corresponding to GPMC_CONFIG3:
+ - gpmc,adv-on:		Assertion time
+ - gpmc,adv-rd-off:	Read deassertion time
+ - gpmc,adv-wr-off:	Write deassertion time
+
+ WE signals timings corresponding to GPMC_CONFIG4:
+ - gpmc,we-on:		Assertion time
+ - gpmc,we-off:		Deassertion time
+
+ OE signals timings corresponding to GPMC_CONFIG4
+ - gpmc,oe-on:		Assertion time
+ - gpmc,oe-off:		Deassertion time
+
+ Access time and cycle time timings corresponding to GPMC_CONFIG5
+ - gpmc,page-burst-access: Multiple access word delay
+ - gpmc,access:		Start-cycle to first data valid delay
+ - gpmc,rd-cycle:	Total read cycle time
+ - gpmc,wr-cycle:	Total write cycle time
+
+The following are only on OMAP3430
+ - gpmc,wr-access
+ - gpmc,wr-data-mux-bus
+
+
+Example for an AM33xx board:
+
+	gpmc: gpmc at 50000000 {
+		compatible = "ti,gpmc";
+		ti,hwmods = "gpmc";
+		reg = <0x50000000 0x1000000>;
+		interrupt-parent = <&intc>;
+		interrupts = <100>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		/* child nodes go here */
+	};
+
+
+
+
+
diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
new file mode 100644
index 0000000..6790fcf
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
@@ -0,0 +1,65 @@
+Device tree bindings for GPMC connected NANDs
+
+GPMC connected NAND (found on OMAP boards) are represented as child nodes of
+the GPMC controller with a name of "nand".
+
+All timing relevant properties are explained in a separate documents - please
+refer to Documentation/devicetree/bindings/bus/gpmc.txt
+
+Required properties:
+
+ - reg: The CS line the peripheral is connected to
+
+Optional properties:
+
+ - ecc:		An integer denoting on of the OMAP_ECC_* values
+ - bus-width:	An integer denoting the bus width of the peripheral. The only
+		value that has any effect is 16. When omitted, a default of
+		8bit is assumed.
+
+For inline partiton table parsing:
+
+ - #address-cells: should be set to 1
+ - #size-cells: should be set to 1
+
+Example for an AM33xx board:
+
+	gpmc: gpmc at 50000000 {
+		compatible = "ti,gpmc";
+		ti,hwmods = "gpmc";
+		reg = <0x50000000 0x1000000>;
+		interrupt-parent = <&intc>;
+		interrupts = <100>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		nand at 0 {
+			reg = <0>; /* CS0 */
+			gpmc,bus-width = <16>;
+			gpmc,ecc = <0>;
+
+			gpmc,sync-clk = <0>;
+			gpmc,cs-on = <0>;
+			gpmc,cs-rd-off = <36>;
+			gpmc,cs-wr-off = <36>;
+			gpmc,adv-on = <6>;
+			gpmc,adv-rd-off = <24>;
+			gpmc,adv-wr-off = <36>;
+			gpmc,we-off = <30>;
+			gpmc,oe-off = <48>;
+			gpmc,access = <54>;
+			gpmc,rd-cycle = <72>;
+			gpmc,wr-cycle = <72>;
+			gpmc,wr-access = <30>;
+			gpmc,wr-data-mux-bus = <0>;
+
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			/* partitions go here */
+		};
+	};
+
+
+
+
diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
index 1dcb30c..2ff919e 100644
--- a/arch/arm/mach-omap2/gpmc.c
+++ b/arch/arm/mach-omap2/gpmc.c
@@ -25,6 +25,9 @@
 #include <linux/module.h>
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/mtd/nand.h>
 
 #include <linux/platform_data/mtd-nand-omap2.h>
 
@@ -37,6 +40,7 @@
 #include "soc.h"
 #include "common.h"
 #include "gpmc.h"
+#include "gpmc-nand.h"
 
 #define	DEVICE_NAME		"omap-gpmc"
 
@@ -751,6 +755,132 @@ static int __devinit gpmc_mem_init(void)
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static struct of_device_id gpmc_dt_ids[] = {
+	{ .compatible = "ti,gpmc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, gpmc_dt_ids);
+
+static void gpmc_read_timings_dt(struct device_node *np,
+				 struct gpmc_timings *gpmc_t)
+{
+	u32 val;
+
+	memset(gpmc_t, 0, sizeof(*gpmc_t));
+
+	/* minimum clock period for syncronous mode */
+	if (!of_property_read_u32(np, "gpmc,sync-clk", &val))
+		gpmc_t->sync_clk = val;
+
+	/* chip select timtings */
+	if (!of_property_read_u32(np, "gpmc,cs-on", &val))
+		gpmc_t->cs_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,cs-rd-off", &val))
+		gpmc_t->cs_rd_off = val;
+
+	if (!of_property_read_u32(np, "gpmc,cs-wr-off", &val))
+		gpmc_t->cs_wr_off = val;
+
+	/* ADV signal timings */
+	if (!of_property_read_u32(np, "gpmc,adv-on", &val))
+		gpmc_t->adv_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,adv-rd-off", &val))
+		gpmc_t->adv_rd_off = val;
+
+	if (!of_property_read_u32(np, "gpmc,adv-wr-off", &val))
+		gpmc_t->adv_wr_off = val;
+
+	/* WE signal timings */
+	if (!of_property_read_u32(np, "gpmc,we-on", &val))
+		gpmc_t->we_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,we-off", &val))
+		gpmc_t->we_off = val;
+
+	/* OE signal timings */
+	if (!of_property_read_u32(np, "gpmc,we-on", &val))
+
+	if (!of_property_read_u32(np, "gpmc,oe-on", &val))
+		gpmc_t->oe_on = val;
+
+	if (!of_property_read_u32(np, "gpmc,oe-off", &val))
+		gpmc_t->oe_off = val;
+
+	/* access and cycle timings */
+	if (!of_property_read_u32(np, "gpmc,page-burst-access", &val))
+		gpmc_t->page_burst_access = val;
+
+	if (!of_property_read_u32(np, "gpmc,access", &val))
+		gpmc_t->access = val;
+
+	if (!of_property_read_u32(np, "gpmc,rd-cycle", &val))
+		gpmc_t->rd_cycle = val;
+
+	if (!of_property_read_u32(np, "gpmc,wr-cycle", &val))
+		gpmc_t->wr_cycle = val;
+
+	/* only for OMAP3430 */
+	if (!of_property_read_u32(np, "gpmc,wr-access", &val))
+		gpmc_t->wr_access = val;
+
+	if (!of_property_read_u32(np, "gpmc,wr-data-mux-bus", &val))
+		gpmc_t->wr_data_mux_bus = val;
+}
+
+static int gpmc_probe_dt(struct platform_device *pdev)
+{
+	u32 val;
+	struct device_node *child;
+	struct gpmc_timings gpmc_t;
+	const struct of_device_id *of_id =
+		of_match_device(gpmc_dt_ids, &pdev->dev);
+
+	if (!of_id)
+		return 0;
+
+	for_each_node_by_name(child, "nand") {
+		struct omap_nand_platform_data *gpmc_nand_data;
+
+		if (of_property_read_u32(child, "reg", &val) < 0) {
+			dev_err(&pdev->dev, "%s has no 'reg' property\n",
+				child->full_name);
+			continue;
+		}
+
+		gpmc_nand_data = devm_kzalloc(&pdev->dev,
+					      sizeof(*gpmc_nand_data),
+					      GFP_KERNEL);
+		if (!gpmc_nand_data) {
+			dev_err(&pdev->dev, "unable to allocate memory?");
+			return -ENOMEM;
+		}
+
+		gpmc_nand_data->cs = val;
+		gpmc_nand_data->of_node = child;
+
+		if (!of_property_read_u32(child, "gpmc,ecc", &val) < 0)
+			gpmc_nand_data->ecc_opt = val;
+
+		if ((!of_property_read_u32(child, "gpmc,bus-width", &val) < 0) &&
+		    val == 16)
+			gpmc_nand_data->devsize = NAND_BUSWIDTH_16;
+
+		gpmc_read_timings_dt(child, &gpmc_t);
+		gpmc_nand_init(gpmc_nand_data, &gpmc_t);
+	}
+
+	return 0;
+}
+#else
+static int gpmc_probe_dt(struct platform_device *pdev)
+{
+	return 0;
+}
+#endif
+
 static __devinit int gpmc_probe(struct platform_device *pdev)
 {
 	int rc;
@@ -804,6 +934,14 @@ static __devinit int gpmc_probe(struct platform_device *pdev)
 	if (IS_ERR_VALUE(gpmc_setup_irq()))
 		dev_warn(gpmc_dev, "gpmc_setup_irq failed\n");
 
+	rc = gpmc_probe_dt(pdev);
+	if (rc < 0) {
+		clk_disable_unprepare(gpmc_l3_clk);
+		clk_put(gpmc_l3_clk);
+		dev_err(gpmc_dev, "failed to probe DT parameters\n");
+		return rc;
+	}
+
 	return 0;
 }
 
@@ -821,6 +959,7 @@ static struct platform_driver gpmc_driver = {
 	.driver		= {
 		.name	= DEVICE_NAME,
 		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(gpmc_dt_ids),
 	},
 };
 
-- 
1.7.11.7

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Tony Lindgren]

* Daniel Mack <zonque@gmail.com> [121022 12:57]:
> This patch adds basic DT bindings for OMAP GPMC.
> 
> The actual peripherals are instanciated from child nodes within the GPMC
> node, and the only type of device that is currently supported is NAND.
> 
> Code was added to parse the generic GPMC timing parameters and some
> documentation with examples on how to use them.
> 
> Successfully tested on an AM33xx board.

That's great, looks good to me. Please repost at least the
binding patch with devicetree-discuss and Rob Herring cc:d
so hopefully we'll get an ack for the binding.

Regards,

Tony

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

On 25.10.2012 01:27, Tony Lindgren wrote:
> * Daniel Mack <zonque@gmail.com> [121022 12:57]:
>> This patch adds basic DT bindings for OMAP GPMC.
>>
>> The actual peripherals are instanciated from child nodes within the GPMC
>> node, and the only type of device that is currently supported is NAND.
>>
>> Code was added to parse the generic GPMC timing parameters and some
>> documentation with examples on how to use them.
>>
>> Successfully tested on an AM33xx board.
> 
> That's great, looks good to me. Please repost at least the
> binding patch with devicetree-discuss and Rob Herring cc:d
> so hopefully we'll get an ack for the binding.

Thanks for the review. I'll wait for feedback from Afzal next week and
then repost. Wanted to see first if that goes in the right direction at
all before bordering the DT people with binding details :)


Regards,
Daniel

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Afzal Mohammed]

Hi Daniel,

On Thursday 25 October 2012 05:01 AM, Daniel Mack wrote:

> Thanks for the review. I'll wait for feedback from Afzal next week and
> then repost. Wanted to see first if that goes in the right direction at
> all before bordering the DT people with binding details :)

I was thinking of a generic approach, where there won't be
any check for peripheral device type.

But going that path would delay achieving DT, may be let's
proceed with your approach to start with so that we can
have a minimal level of DT support for GPMC and probably
we can make it generic later. While adding new properties,
it would be better to keep in mind that we need not change
these later once gpmc DT is made generic.

Regarding the bindings, there are some generic nand
properties like ecc already available, may be that be made
use here. Also perhaps memory size (and offset if
needed) to be mapped for peripherals can go with reg
property of child.

Regards
Afzal

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Afzal,

On 29.10.2012 09:10, Afzal Mohammed wrote:
> On Thursday 25 October 2012 05:01 AM, Daniel Mack wrote:
> 
>> Thanks for the review. I'll wait for feedback from Afzal next week and
>> then repost. Wanted to see first if that goes in the right direction at
>> all before bordering the DT people with binding details :)
> 
> I was thinking of a generic approach, where there won't be
> any check for peripheral device type.
> 
> But going that path would delay achieving DT, may be let's
> proceed with your approach to start with so that we can
> have a minimal level of DT support for GPMC and probably
> we can make it generic later. While adding new properties,
> it would be better to keep in mind that we need not change
> these later once gpmc DT is made generic.
> 
> Regarding the bindings, there are some generic nand
> properties like ecc already available, may be that be made
> use here.

Ah, of_get_nand_ecc_mode() - nice.

> Also perhaps memory size (and offset if
> needed) to be mapped for peripherals can go with reg
> property of child.

Which detail are you referring to here? The only "size" property that is
effective is the one of the generic GPMC block, and there it's in the
"reg"-property.

I'll take the other feedback that I got and quickly do a v2.


Thanks,
Daniel

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Afzal Mohammed]

Hi Daniel,

On Monday 29 October 2012 04:45 PM, Daniel Mack wrote:
> On 29.10.2012 09:10, Afzal Mohammed wrote:

>> Also perhaps memory size (and offset if
>> needed) to be mapped for peripherals can go with reg
>> property of child.

> Which detail are you referring to here? The only "size" property that is
> effective is the one of the generic GPMC block, and there it's in the
> "reg"-property.

I was referring to that of child, now in gpmc_nand_init(),
gpmc_cs_request() is being done, later on if we want to
make it generic and remove gpmc_nand_init(), additional
information that would be required from DT at least is the
memory size to be reserved in gpmc address space for
the connected peripheral (assuming gpmc_cs_request()
would be done by gpmc driver generically later)

What I had in mind was example for external bus in [1],
but I had not looked deep into this aspect yet.

Regards
Afzal

[1] http://devicetree.org/Device_Tree_Usage#Memory_Mapped_Devices

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Afzal,

On 29.10.2012 12:28, Afzal Mohammed wrote:
> On Monday 29 October 2012 04:45 PM, Daniel Mack wrote:
>> On 29.10.2012 09:10, Afzal Mohammed wrote:
> 
>>> Also perhaps memory size (and offset if
>>> needed) to be mapped for peripherals can go with reg
>>> property of child.
> 
>> Which detail are you referring to here? The only "size" property that is
>> effective is the one of the generic GPMC block, and there it's in the
>> "reg"-property.
> 
> I was referring to that of child, now in gpmc_nand_init(),
> gpmc_cs_request() is being done, later on if we want to
> make it generic and remove gpmc_nand_init(), additional
> information that would be required from DT at least is the
> memory size to be reserved in gpmc address space for
> the connected peripheral (assuming gpmc_cs_request()
> would be done by gpmc driver generically later)
> 
> What I had in mind was example for external bus in [1],
> but I had not looked deep into this aspect yet.

Ok, now I see what you mean.

I would say we can use the "reg" property in child node for CS numbers
purely and if we want to get rid of the memory node allocation, we
should use a property in the gpmc top-node for this, something like:

	gpmc: gpmc at 50000000 {
		compatible = "ti,gpmc";
		cs-regs = <0x51000000 0x10000000 ...>;
	};

Changing the meaning of the reg property of children from "cs number" to
"memory sub-region" later is something I would like to avoid.



Daniel

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Afzal Mohammed]

Hi Daniel,

On Monday 29 October 2012 06:02 PM, Daniel Mack wrote:
> On 29.10.2012 12:28, Afzal Mohammed wrote:

>> I was referring to that of child, now in gpmc_nand_init(),
>> gpmc_cs_request() is being done, later on if we want to
>> make it generic and remove gpmc_nand_init(), additional
>> information that would be required from DT at least is the
>> memory size to be reserved in gpmc address space for
>> the connected peripheral (assuming gpmc_cs_request()
>> would be done by gpmc driver generically later)
>>
>> What I had in mind was example for external bus in [1],
>> but I had not looked deep into this aspect yet.

> Ok, now I see what you mean.
>
> I would say we can use the "reg" property in child node for CS numbers
> purely and if we want to get rid of the memory node allocation, we
> should use a property in the gpmc top-node for this, something like:
>
> 	gpmc: gpmc at 50000000 {
> 		compatible = "ti,gpmc";
> 		cs-regs =<0x51000000 0x10000000 ...>;

I think you meant cs-regs = <0x00000000 ..>

0x0 - 0x1fffffff is gpmc external memory address space, while
0x50000000 to plus 16MB is gpmc configuration address space.

You may refer other gpmc peripheral init's that are NOR type.

> Changing the meaning of the reg property of children from "cs number" to
> "memory sub-region" later is something I would like to avoid.

Changing any of the properties later is something we have
to avoid. Let us get feedback from DT maintainers.

Regards
Afzal

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

Hi Daniel,

On 10/22/2012 02:55 PM, Daniel Mack wrote:
> This patch adds basic DT bindings for OMAP GPMC.
> 
> The actual peripherals are instanciated from child nodes within the GPMC
> node, and the only type of device that is currently supported is NAND.
> 
> Code was added to parse the generic GPMC timing parameters and some
> documentation with examples on how to use them.
> 
> Successfully tested on an AM33xx board.

Thanks for sending this and sorry for the delay in responding. Some
comments below ...

> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/bus/gpmc.txt     |  59 +++++++++
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  65 ++++++++++
>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>  3 files changed, 263 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/gpmc.txt
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
> new file mode 100644
> index 0000000..ef1c6e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/gpmc.txt
> @@ -0,0 +1,59 @@
> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
> +
> +The actual devices are instantiated from the child nodes of a GPMC node.
> +
> +Required properties:
> +
> + - compatible: Should be set to "ti,gpmc"

Is this the only required property? I think that "reg" and "ti,hwmods"
are probably also required.

Also given that we are describing the hardware, I am wondering if the
number of chip-selects and wait signals should be defined here too. I
recall that different devices had different number of wait pins available.

> +
> +Timing properties for child nodes. All are optional and default to 0.
> +
> + - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
> +
> + Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
> + - gpmc,cs-on:		Assertion time
> + - gpmc,cs-rd-off:	Read deassertion time
> + - gpmc,cs-wr-off:	Write deassertion time
> +
> + ADV signal timings corresponding to GPMC_CONFIG3:
> + - gpmc,adv-on:		Assertion time
> + - gpmc,adv-rd-off:	Read deassertion time
> + - gpmc,adv-wr-off:	Write deassertion time
> +
> + WE signals timings corresponding to GPMC_CONFIG4:
> + - gpmc,we-on:		Assertion time
> + - gpmc,we-off:		Deassertion time
> +
> + OE signals timings corresponding to GPMC_CONFIG4
> + - gpmc,oe-on:		Assertion time
> + - gpmc,oe-off:		Deassertion time
> +
> + Access time and cycle time timings corresponding to GPMC_CONFIG5
> + - gpmc,page-burst-access: Multiple access word delay
> + - gpmc,access:		Start-cycle to first data valid delay
> + - gpmc,rd-cycle:	Total read cycle time
> + - gpmc,wr-cycle:	Total write cycle time
> +
> +The following are only on OMAP3430
> + - gpmc,wr-access
> + - gpmc,wr-data-mux-bus
> +
> +
> +Example for an AM33xx board:
> +
> +	gpmc: gpmc at 50000000 {
> +		compatible = "ti,gpmc";
> +		ti,hwmods = "gpmc";
> +		reg = <0x50000000 0x1000000>;
> +		interrupt-parent = <&intc>;
> +		interrupts = <100>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +
> +		/* child nodes go here */
> +	};
> +
> +
> +
> +
> +
> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> new file mode 100644
> index 0000000..6790fcf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> @@ -0,0 +1,65 @@
> +Device tree bindings for GPMC connected NANDs
> +
> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
> +the GPMC controller with a name of "nand".
> +
> +All timing relevant properties are explained in a separate documents - please
> +refer to Documentation/devicetree/bindings/bus/gpmc.txt
> +
> +Required properties:
> +
> + - reg: The CS line the peripheral is connected to

Is this the only required property? I would have thought that bus-width
is needed too.

In general, I am wondering if this should be broken into two patches as
you are creating the binding for the gpmc and nand here.

Cheers
Jon

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Jon,

many thanks for your time to look at this.

On 25.10.2012 03:28, Jon Hunter wrote:
> On 10/22/2012 02:55 PM, Daniel Mack wrote:
>> diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
>> new file mode 100644
>> index 0000000..ef1c6e1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/bus/gpmc.txt
>> @@ -0,0 +1,59 @@
>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>> +
>> +The actual devices are instantiated from the child nodes of a GPMC node.
>> +
>> +Required properties:
>> +
>> + - compatible: Should be set to "ti,gpmc"
> 
> Is this the only required property? I think that "reg" and "ti,hwmods"
> are probably also required.

Well yes, but at least "reg" is commonly omitted as it's part of a more
"generic" set of properties. But ok, I can add these.

> Also given that we are describing the hardware, I am wondering if the
> number of chip-selects and wait signals should be defined here too. I
> recall that different devices had different number of wait pins available.

Hmm, that number is currently hard-coded in GPMC_CS_NUM. It would take
some effort to make that dynamic but I agree that this would be a good
thing to have. Afzal?

>> diff --git a/Documentation/devicetree/bindings/mtd/gpmc-nand.txt b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>> new file mode 100644
>> index 0000000..6790fcf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/gpmc-nand.txt
>> @@ -0,0 +1,65 @@
>> +Device tree bindings for GPMC connected NANDs
>> +
>> +GPMC connected NAND (found on OMAP boards) are represented as child nodes of
>> +the GPMC controller with a name of "nand".
>> +
>> +All timing relevant properties are explained in a separate documents - please
>> +refer to Documentation/devicetree/bindings/bus/gpmc.txt
>> +
>> +Required properties:
>> +
>> + - reg: The CS line the peripheral is connected to
> 
> Is this the only required property? I would have thought that bus-width
> is needed too.

I described bus-with in the nand bindings and stated there that it
defaults to 8 and the only meaningful other value us 16. I did that
because the value is in fact parsed in the NAND code, but I can as well
move this around in the documentation.

> In general, I am wondering if this should be broken into two patches as
> you are creating the binding for the gpmc and nand here.

Yes, I thought so too. The thing is that I wanted to keep documentation
and implementation tightly together, and only the generic parser bits in
the code doesn't make much sense without any users. Also, the two
Documentations reference each other, so I thought having them in one
piece could make reviewing easier. But I can of course also split it if
that helps.


Thanks,
Daniel

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 10/25/2012 03:00 AM, Daniel Mack wrote:
> Hi Jon,
> 
> many thanks for your time to look at this.
> 
> On 25.10.2012 03:28, Jon Hunter wrote:
>> On 10/22/2012 02:55 PM, Daniel Mack wrote:
>>> diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
>>> new file mode 100644
>>> index 0000000..ef1c6e1
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/bus/gpmc.txt
>>> @@ -0,0 +1,59 @@
>>> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
>>> +
>>> +The actual devices are instantiated from the child nodes of a GPMC node.
>>> +
>>> +Required properties:
>>> +
>>> + - compatible: Should be set to "ti,gpmc"
>>
>> Is this the only required property? I think that "reg" and "ti,hwmods"
>> are probably also required.
> 
> Well yes, but at least "reg" is commonly omitted as it's part of a more
> "generic" set of properties. But ok, I can add these.
> 
>> Also given that we are describing the hardware, I am wondering if the
>> number of chip-selects and wait signals should be defined here too. I
>> recall that different devices had different number of wait pins available.
> 
> Hmm, that number is currently hard-coded in GPMC_CS_NUM. It would take
> some effort to make that dynamic but I agree that this would be a good
> thing to have. Afzal?

I believe that today all OMAP/AM devices have 8 chip-selects so probably
not a big deal. However, given we are moving to DT it would be nice to
move away from having such #defines for hardware related items.

Cheers
Jon

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Afzal Mohammed]

Hi Daniel,

On Thursday 25 October 2012 01:30 PM, Daniel Mack wrote:

> Hmm, that number is currently hard-coded in GPMC_CS_NUM. It would take
> some effort to make that dynamic but I agree that this would be a good
> thing to have. Afzal?

It would be good to have (to the best of my knowledge
am335x has only 7 pinned out, even though IP has 8)

Regards
Afzal

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 10/22/2012 02:55 PM, Daniel Mack wrote:
> This patch adds basic DT bindings for OMAP GPMC.
> 
> The actual peripherals are instanciated from child nodes within the GPMC
> node, and the only type of device that is currently supported is NAND.
> 
> Code was added to parse the generic GPMC timing parameters and some
> documentation with examples on how to use them.
> 
> Successfully tested on an AM33xx board.
> 
> Signed-off-by: Daniel Mack <zonque@gmail.com>
> ---
>  Documentation/devicetree/bindings/bus/gpmc.txt     |  59 +++++++++
>  .../devicetree/bindings/mtd/gpmc-nand.txt          |  65 ++++++++++
>  arch/arm/mach-omap2/gpmc.c                         | 139 +++++++++++++++++++++
>  3 files changed, 263 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/bus/gpmc.txt
>  create mode 100644 Documentation/devicetree/bindings/mtd/gpmc-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/bus/gpmc.txt b/Documentation/devicetree/bindings/bus/gpmc.txt
> new file mode 100644
> index 0000000..ef1c6e1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/bus/gpmc.txt
> @@ -0,0 +1,59 @@
> +Device tree bindings for OMAP general purpose memory controllers (GPMC)
> +
> +The actual devices are instantiated from the child nodes of a GPMC node.
> +
> +Required properties:
> +
> + - compatible: Should be set to "ti,gpmc"
> +
> +Timing properties for child nodes. All are optional and default to 0.
> +
> + - gpmc,sync-clk:	Minimum clock period for synchronous mode, in picoseconds
> +
> + Chip-select signal timings corresponding to GPMC_CS_CONFIG2:
> + - gpmc,cs-on:		Assertion time
> + - gpmc,cs-rd-off:	Read deassertion time
> + - gpmc,cs-wr-off:	Write deassertion time
> +
> + ADV signal timings corresponding to GPMC_CONFIG3:
> + - gpmc,adv-on:		Assertion time
> + - gpmc,adv-rd-off:	Read deassertion time
> + - gpmc,adv-wr-off:	Write deassertion time
> +
> + WE signals timings corresponding to GPMC_CONFIG4:
> + - gpmc,we-on:		Assertion time
> + - gpmc,we-off:		Deassertion time
> +
> + OE signals timings corresponding to GPMC_CONFIG4
> + - gpmc,oe-on:		Assertion time
> + - gpmc,oe-off:		Deassertion time
> +
> + Access time and cycle time timings corresponding to GPMC_CONFIG5
> + - gpmc,page-burst-access: Multiple access word delay
> + - gpmc,access:		Start-cycle to first data valid delay
> + - gpmc,rd-cycle:	Total read cycle time
> + - gpmc,wr-cycle:	Total write cycle time
> +
> +The following are only on OMAP3430
> + - gpmc,wr-access
> + - gpmc,wr-data-mux-bus
> +
> +
> +Example for an AM33xx board:
> +
> +	gpmc: gpmc at 50000000 {
> +		compatible = "ti,gpmc";
> +		ti,hwmods = "gpmc";
> +		reg = <0x50000000 0x1000000>;

Nit-pick, that size is quite large for a register range. I recommend
looking at the HWMOD data file
(arch/arm/mach-omap2/omap_hwmod_33xx_data.c) and see how much space is
allocated for the registers (see structure am33xx_gpmc_addr_space).

Cheers
Jon

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Daniel Mack]

Hi Jon,

On 25.10.2012 03:53, Jon Hunter wrote:
> 
> On 10/22/2012 02:55 PM, Daniel Mack wrote:

>> +Example for an AM33xx board:
>> +
>> +	gpmc: gpmc at 50000000 {
>> +		compatible = "ti,gpmc";
>> +		ti,hwmods = "gpmc";
>> +		reg = <0x50000000 0x1000000>;
> 
> Nit-pick, that size is quite large for a register range. I recommend
> looking at the HWMOD data file
> (arch/arm/mach-omap2/omap_hwmod_33xx_data.c) and see how much space is
> allocated for the registers (see structure am33xx_gpmc_addr_space).

Yeah but reserving the entire memory as per the reference manual also
prvents other from poking around in the same register space. Is there a
scenario in which it would of disadvantage to reserve all that memory?



Daniel

[PATCH 4/4] OMAP: mtd: gpmc: add DT bindings for GPMC timings and NAND

[Jon Hunter]

On 10/25/2012 04:43 AM, Daniel Mack wrote:
> Hi Jon,
> 
> On 25.10.2012 03:53, Jon Hunter wrote:
>>
>> On 10/22/2012 02:55 PM, Daniel Mack wrote:
> 
>>> +Example for an AM33xx board:
>>> +
>>> +	gpmc: gpmc at 50000000 {
>>> +		compatible = "ti,gpmc";
>>> +		ti,hwmods = "gpmc";
>>> +		reg = <0x50000000 0x1000000>;
>>
>> Nit-pick, that size is quite large for a register range. I recommend
>> looking at the HWMOD data file
>> (arch/arm/mach-omap2/omap_hwmod_33xx_data.c) and see how much space is
>> allocated for the registers (see structure am33xx_gpmc_addr_space).
> 
> Yeah but reserving the entire memory as per the reference manual also
> prvents other from poking around in the same register space. Is there a
> scenario in which it would of disadvantage to reserve all that memory?

1. It chews up a large chunk of virtual memory space unnecessarily. For
   devices not using HIGHMEM and wish to use say 512MB of RAM, virtual
   memory space can be constrained.
2. We don't do that today probably because of #1.

Cheers
Jon