Patchwork [2/2] mtd: fsl_ifc_nand: Probe partitions OF node

login
register
mail settings
Submitter Aaron Sierra
Date Aug. 16, 2014, 12:47 a.m.
Message ID <251532766.70336.1408150059026.JavaMail.zimbra@xes-inc.com>
Download mbox | patch
Permalink /patch/380443/
State Rejected
Headers show

Comments

Aaron Sierra - Aug. 16, 2014, 12:47 a.m.
Previously, the OF node defining the IFC NAND controller was being
passed to mtd_device_parse_register(), not the node defining the
partitions. This resulted in no OF-defined partitions being created.

Signed-off-by: Aaron Sierra <asierra@xes-inc.com>
---
 drivers/mtd/nand/fsl_ifc_nand.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
Scott Wood - Aug. 20, 2014, 11:26 p.m.
On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> Previously, the OF node defining the IFC NAND controller was being
> passed to mtd_device_parse_register(), not the node defining the
> partitions. This resulted in no OF-defined partitions being created.

This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
getting the controller node?

What does the device tree look like in which you're seeing this happen?

-Scott
Aaron Sierra - Aug. 20, 2014, 11:45 p.m.
----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Wednesday, August 20, 2014 6:26:25 PM
> 
> On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > Previously, the OF node defining the IFC NAND controller was being
> > passed to mtd_device_parse_register(), not the node defining the
> > partitions. This resulted in no OF-defined partitions being created.
> 
> This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> getting the controller node?
> 
> What does the device tree look like in which you're seeing this happen?
> 
> -Scott
> 

This is the node that is defined in my T1042 device tree:

		nand0@4,0 {
			#address-cells = <0>;
			#size-cells = <0>;
			compatible = "fsl,ifc-nand";
                        reg = <4 0x0 0x040000>;

			nand@0 {
				#address-cells = <1>;
				#size-cells = <2>;
				compatible = "micron,mt29f32g08";

				partition@0 {
					label = "NAND Filesystem";
					reg = <0 0x1 0x00000000>;
				};
			};
		};

It is based on a node used previously with the fsl_upm NAND driver on
a P5020:

		upm@4,0 {
			#address-cells = <0>;
			#size-cells = <0>;
			compatible = "fsl,upm-nand";
                        reg = <4 0x0 0x080000>;
                        fsl,upm-addr-offset = <0x10>;
                        fsl,upm-cmd-offset = <0x08>;
                        fsl,upm-wait-flags = <0x1>;
                        chip-delay = <50>;

			nand@0 {
				#address-cells = <1>;
				#size-cells = <2>;
				compatible = "micron,mt29f32g08";

				partition@0 {
					label = "NAND Filesystem";
					reg = <0 0x1 0x00000000>;
				};
			};
		};

The patch that I submitted is based on code from the fsl_upm driver:

	flash_np = of_get_next_child(upm_np, NULL);
	if (!flash_np)
		return -ENODEV;

[snip]

	ppdata.of_node = flash_np;
	ret = mtd_device_parse_register(&fun->mtd, NULL, &ppdata, NULL, 0);
err:
	of_node_put(flash_np);

-Aaron
Scott Wood - Aug. 20, 2014, 11:59 p.m.
On Wed, 2014-08-20 at 18:45 -0500, Aaron Sierra wrote:
> ----- Original Message -----
> > From: "Scott Wood" <scottwood@freescale.com>
> > Sent: Wednesday, August 20, 2014 6:26:25 PM
> > 
> > On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > > Previously, the OF node defining the IFC NAND controller was being
> > > passed to mtd_device_parse_register(), not the node defining the
> > > partitions. This resulted in no OF-defined partitions being created.
> > 
> > This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> > getting the controller node?
> > 
> > What does the device tree look like in which you're seeing this happen?
> > 
> > -Scott
> > 
> 
> This is the node that is defined in my T1042 device tree:
> 
> 		nand0@4,0 {
> 			#address-cells = <0>;
> 			#size-cells = <0>;
> 			compatible = "fsl,ifc-nand";
>                         reg = <4 0x0 0x040000>;
> 
> 			nand@0 {
> 				#address-cells = <1>;
> 				#size-cells = <2>;
> 				compatible = "micron,mt29f32g08";
> 
> 				partition@0 {
> 					label = "NAND Filesystem";
> 					reg = <0 0x1 0x00000000>;
> 				};
> 			};
> 		};

This is wrong.  You shouldn't have a separate nand@0 node.

Your patch will break partition detection on all of the existing IFC
device trees.

> It is based on a node used previously with the fsl_upm NAND driver on
> a P5020:

Why would you base it on a upm node rather than on an ifc node, or on
the IFC binding document
(Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt)?

-Scott
Aaron Sierra - Aug. 21, 2014, 12:15 a.m.
----- Original Message -----
> From: "Scott Wood" <scottwood@freescale.com>
> Sent: Wednesday, August 20, 2014 6:59:56 PM
> 
> On Wed, 2014-08-20 at 18:45 -0500, Aaron Sierra wrote:
> > ----- Original Message -----
> > > From: "Scott Wood" <scottwood@freescale.com>
> > > Sent: Wednesday, August 20, 2014 6:26:25 PM
> > > 
> > > On Fri, 2014-08-15 at 19:47 -0500, Aaron Sierra wrote:
> > > > Previously, the OF node defining the IFC NAND controller was being
> > > > passed to mtd_device_parse_register(), not the node defining the
> > > > partitions. This resulted in no OF-defined partitions being created.
> > > 
> > > This driver probes on "fsl,ifc-nand", not "fsl,ifc".  So how is it
> > > getting the controller node?
> > > 
> > > What does the device tree look like in which you're seeing this happen?
> > > 
> > > -Scott
> > > 
> > 
> > This is the node that is defined in my T1042 device tree:
> > 
> > 		nand0@4,0 {
> > 			#address-cells = <0>;
> > 			#size-cells = <0>;
> > 			compatible = "fsl,ifc-nand";
> >                         reg = <4 0x0 0x040000>;
> > 
> > 			nand@0 {
> > 				#address-cells = <1>;
> > 				#size-cells = <2>;
> > 				compatible = "micron,mt29f32g08";
> > 
> > 				partition@0 {
> > 					label = "NAND Filesystem";
> > 					reg = <0 0x1 0x00000000>;
> > 				};
> > 			};
> > 		};
> 
> This is wrong.  You shouldn't have a separate nand@0 node.
> 
> Your patch will break partition detection on all of the existing IFC
> device trees.
> 
> > It is based on a node used previously with the fsl_upm NAND driver on
> > a P5020:
> 
> Why would you base it on a upm node rather than on an ifc node, or on
> the IFC binding document
> (Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt)?
> 
> -Scott
> 

Ah, I was under the wrong impression that there was a common MTD
partitioning format. I didn't realize it was defined by each
driver. Thanks for clearing that up.

-Aaron
Scott Wood - Aug. 21, 2014, 12:17 a.m.
On Wed, 2014-08-20 at 19:15 -0500, Aaron Sierra wrote:
> Ah, I was under the wrong impression that there was a common MTD
> partitioning format. I didn't realize it was defined by each
> driver. Thanks for clearing that up.

The partition nodes should take the same form regardless of controller
type, but not the node that contains the partition nodes.

-Scott

Patch

diff --git a/drivers/mtd/nand/fsl_ifc_nand.c b/drivers/mtd/nand/fsl_ifc_nand.c
index 7861909..9aeb146 100644
--- a/drivers/mtd/nand/fsl_ifc_nand.c
+++ b/drivers/mtd/nand/fsl_ifc_nand.c
@@ -1029,7 +1029,6 @@  static int fsl_ifc_nand_probe(struct platform_device *dev)
 	struct device_node *node = dev->dev.of_node;
 	struct mtd_part_parser_data ppdata;
 
-	ppdata.of_node = dev->dev.of_node;
 	if (!fsl_ifc_ctrl_dev || !fsl_ifc_ctrl_dev->regs)
 		return -ENODEV;
 	ifc = fsl_ifc_ctrl_dev->regs;
@@ -1126,9 +1125,13 @@  static int fsl_ifc_nand_probe(struct platform_device *dev)
 
 	/* First look for RedBoot table or partitions on the command
 	 * line, these take precedence over device tree information */
+	ppdata.of_node = of_get_next_child(dev->dev.of_node, NULL);
 	mtd_device_parse_register(&priv->mtd, part_probe_types, &ppdata,
 						NULL, 0);
 
+	if (ppdata.of_node)
+		of_node_put(ppdata.of_node);
+
 	dev_info(priv->dev, "IFC NAND device at 0x%llx, bank %d\n",
 		 (unsigned long long)res.start, priv->bank);
 	return 0;