[PATCH v2 2/7] fdt: Add debugging to fdtdec_get_int/addr()

[PATCH v2 2/7] fdt: Add debugging to fdtdec_get_int/addr()

[Simon Glass]

The new debugging shows the value of integers and addresses read
from the device tree.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 lib/fdtdec.c |   22 ++++++++++++++++------
 1 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/lib/fdtdec.c b/lib/fdtdec.c
index 42c3e89..3885634 100644
--- a/lib/fdtdec.c
+++ b/lib/fdtdec.c
@@ -79,11 +79,16 @@ fdt_addr_t fdtdec_get_addr(const void *blob, int node,
 	const fdt_addr_t *cell;
 	int len;
 
-	debug("get_addr: %s\n", prop_name);
+	debug("%s: %s\n", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
 	if (cell && (len == sizeof(fdt_addr_t) ||
-			len == sizeof(fdt_addr_t) * 2))
-		return fdt_addr_to_cpu(*cell);
+			len == sizeof(fdt_addr_t) * 2)) {
+		fdt_addr_t addr = fdt_addr_to_cpu(*cell);
+
+		debug("%p\n", (void *)addr);
+		return addr;
+	}
+	debug("(not found)\n");
 	return FDT_ADDR_T_NONE;
 }
 
@@ -93,10 +98,15 @@ s32 fdtdec_get_int(const void *blob, int node, const char *prop_name,
 	const s32 *cell;
 	int len;
 
-	debug("get_size: %s\n", prop_name);
+	debug("%s: %s: ", __func__, prop_name);
 	cell = fdt_getprop(blob, node, prop_name, &len);
-	if (cell && len >= sizeof(s32))
-		return fdt32_to_cpu(cell[0]);
+	if (cell && len >= sizeof(s32)) {
+		s32 val = fdt32_to_cpu(cell[0]);
+
+		debug("%#x (%d)\n", val, val);
+		return val;
+	}
+	debug("(not found)\n");
 	return default_val;
 }
 
-- 
1.7.7.3

[PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Add a NAND controller along with a bindings file for review.

Signed-off-by: Simon Glass <sjg@chromium.org>
---
Changes in v2:
- Update NAND binding to add "nvidia," prefix

 arch/arm/dts/tegra20.dtsi                     |    6 ++
 doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
 2 files changed, 74 insertions(+), 0 deletions(-)
 create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index bc64f42..7be0462 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -200,4 +200,10 @@
 		reg = <0x7000f400 0x200>;
 	};
 
+	nand: nand-controller@0x70008000 {
+		#address-cells = <0>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-nand";
+		reg = <0x70008000 0x100>;
+	};
 };
diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
new file mode 100644
index 0000000..b19ce8e
--- /dev/null
+++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
@@ -0,0 +1,68 @@
+NAND Flash
+----------
+
+(there isn't yet a generic binding in Linux, so this describes what is in
+U-Boot)
+
+The device node for a NAND flash device is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Required properties :
+ - compatible : Should be "manufacture,device", "nand-flash"
+ - nvidia,page-data-bytes : Number of bytes in the data area
+ - nvidia,page-spare-bytes : * Number of bytes in spare area
+       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
+			+ tag-ecc-bytes
+ - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
+	(these are typically used for bad block maintenance)
+ - nvidia,data-ecc-bytes : Number of ECC bytes for data area
+ - nvidia,tag-bytes :Number of tag bytes in spare area
+ - nvidia,tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
+
+(replace -bytes with -size or -length?)
+
+This node should sit inside its controller.
+
+
+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as described in the document
+"Open Firmware Recommended Practice : Universal Serial Bus" with the
+following modifications and additions :
+
+Optional properties:
+
+wp-gpio : GPIO of write-protect line, three cells in the format:
+		phandle, parameter, flags
+nvidia,,width : bus width of the NAND device in bits
+
+For now here is something specific to the Nvidia controller, with naming
+based on Nvidia's original (non-fdt) NAND driver:
+
+ - nvidia,nand-timing : Timing parameters for the NAND. Each is in ns.
+	Order is: MAX_TRP_TREA, TWB, Max(tCS, tCH, tALS, tALH),
+	TWHR, Max(tCS, tCH, tALS, tALH), TWH, TWP, TRH, TADL
+
+	MAX_TRP_TREA is:
+		non-EDO mode: Max(tRP, tREA) + 6ns
+		EDO mode: tRP timing
+
+Example:
+
+nand-controller@0x70008000 {
+	compatible = "nvidia,tegra20-nand";
+	wp-gpios = <&gpio 59 0>;		/* PH3 */
+	nvidia,width = <8>;
+	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+	nand@0 {
+		compatible = "hynix,hy27uf4g2b", "nand-flash";
+		nvidia,page-data-bytes = <2048>;
+		nvidia,tag-ecc-bytes = <4>;
+		nvidia,tag-bytes = <20>;
+		nvidia,data-ecc-bytes = <36>;
+		nvidia,skipped-spare-bytes = <4>;
+		nvidia,page-spare-bytes = <64>;
+	};
+};
-- 
1.7.7.3

[PATCH v2 5/7] tegra: fdt: Add NAND definitions to fdt

[Simon Glass]

Add a flash node to handle the NAND, including memory timings and
page / block size information.

Signed-off-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
---
Changes in v2:
- Update NAND binding to add "nvidia," prefix

 board/nvidia/dts/tegra2-seaboard.dts |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/board/nvidia/dts/tegra2-seaboard.dts b/board/nvidia/dts/tegra2-seaboard.dts
index 08ada8c..a11bceb 100644
--- a/board/nvidia/dts/tegra2-seaboard.dts
+++ b/board/nvidia/dts/tegra2-seaboard.dts
@@ -126,4 +126,19 @@
 				0x00000000 0x00000000 0x00000000 0x00000000 >;
 		};
 	};
+
+	nand-controller@0x70008000 {
+		wp-gpios = <&gpio 59 0>;		/* PH3 */
+		nvidia,width = <8>;
+		nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+		nand@0 {
+			compatible = "hynix,hy27uf4g2b", "nand-flash";
+			nvidia,page-data-bytes = <2048>;
+			nvidia,tag-ecc-bytes = <4>;
+			nvidia,tag-bytes = <20>;
+			nvidia,data-ecc-bytes = <36>;
+			nvidia,skipped-spare-bytes = <4>;
+			nvidia,page-spare-bytes = <64>;
+		};
+	};
 };
-- 
1.7.7.3

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/13/2012 01:29 PM, Simon Glass wrote:
> Add a NAND controller along with a bindings file for review.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> Changes in v2:
> - Update NAND binding to add "nvidia," prefix
> 
>  arch/arm/dts/tegra20.dtsi                     |    6 ++
>  doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 0 deletions(-)
>  create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt
> 
> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
> index bc64f42..7be0462 100644
> --- a/arch/arm/dts/tegra20.dtsi
> +++ b/arch/arm/dts/tegra20.dtsi
> @@ -200,4 +200,10 @@
>  		reg = <0x7000f400 0x200>;
>  	};
>  
> +	nand: nand-controller@0x70008000 {

s/nand-controller@/flash@/ (or "nand@" if you really want -- there's
enough of that in use already)

> +		#address-cells = <0>;
> +		#size-cells = <0>;
> +		compatible = "nvidia,tegra20-nand";
> +		reg = <0x70008000 0x100>;
> +	};
>  };
> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
> new file mode 100644
> index 0000000..b19ce8e
> --- /dev/null
> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
> @@ -0,0 +1,68 @@
> +NAND Flash
> +----------
> +
> +(there isn't yet a generic binding in Linux, so this describes what is in
> +U-Boot)

Ideally the binding should not be Linux-specific or U-Boot specific --
it's just the binding that describes this hardware.

> +The device node for a NAND flash device is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Required properties :
> + - compatible : Should be "manufacture,device", "nand-flash"

s/manufacture/manufacturer/

No "nand-flash" compatible, as there's no standard "nand-flash" binding.
 You might want something like "nvidia,tegra20-nand-chip".

> + - nvidia,page-data-bytes : Number of bytes in the data area
> + - nvidia,page-spare-bytes : * Number of bytes in spare area
> +       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
> +			+ tag-ecc-bytes
> + - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
> +	(these are typically used for bad block maintenance)
> + - nvidia,data-ecc-bytes : Number of ECC bytes for data area
> + - nvidia,tag-bytes :Number of tag bytes in spare area
> + - nvidia,tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
> +
> +(replace -bytes with -size or -length?)

I like bytes -- makes the unit clear.

> +Nvidia NAND Controller
> +----------------------
> +
> +The device node for a NAND flash controller is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
> +following modifications and additions :
> +
> +Optional properties:
> +
> +wp-gpio : GPIO of write-protect line, three cells in the format:
> +		phandle, parameter, flags

nvidia,nand-wp-gpio

> +nvidia,,width : bus width of the NAND device in bits

s/,,width/,nand-width/

> +For now here is something specific to the Nvidia controller, 

Isn't this whole file specific to the nvidia controller?

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 11:43 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/13/2012 01:29 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>> Changes in v2:
>> - Update NAND binding to add "nvidia," prefix
>>
>>  arch/arm/dts/tegra20.dtsi                     |    6 ++
>>  doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
>>  2 files changed, 74 insertions(+), 0 deletions(-)
>>  create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt
>>
>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>> index bc64f42..7be0462 100644
>> --- a/arch/arm/dts/tegra20.dtsi
>> +++ b/arch/arm/dts/tegra20.dtsi
>> @@ -200,4 +200,10 @@
>>               reg = <0x7000f400 0x200>;
>>       };
>>
>> +     nand: nand-controller@0x70008000 {
>
> s/nand-controller@/flash@/ (or "nand@" if you really want -- there's
> enough of that in use already)

Changed to flash@.

I am a little concerned that we are co-mingling the controller with
the device, but I think this is ok.

>
>> +             #address-cells = <0>;
>> +             #size-cells = <0>;
>> +             compatible = "nvidia,tegra20-nand";
>> +             reg = <0x70008000 0x100>;
>> +     };
>>  };
>> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
>> new file mode 100644
>> index 0000000..b19ce8e
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>> @@ -0,0 +1,68 @@
>> +NAND Flash
>> +----------
>> +
>> +(there isn't yet a generic binding in Linux, so this describes what is in
>> +U-Boot)
>
> Ideally the binding should not be Linux-specific or U-Boot specific --
> it's just the binding that describes this hardware.

Agreed, but trying to agree a binding in Linux in the absence of a
driver may be beyond my powers. Let's see if anyone picks up on this,
I could be wrong. I will add a comment in the meantime.

>
>> +The device node for a NAND flash device is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Required properties :
>> + - compatible : Should be "manufacture,device", "nand-flash"
>
> s/manufacture/manufacturer/

Done

>
> No "nand-flash" compatible, as there's no standard "nand-flash" binding.
>  You might want something like "nvidia,tegra20-nand-chip".
>
>> + - nvidia,page-data-bytes : Number of bytes in the data area
>> + - nvidia,page-spare-bytes : * Number of bytes in spare area
>> +       spare area = skipped-spare-bytes + data-ecc-bytes + tag-bytes
>> +                     + tag-ecc-bytes
>> + - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
>> +     (these are typically used for bad block maintenance)
>> + - nvidia,data-ecc-bytes : Number of ECC bytes for data area
>> + - nvidia,tag-bytes :Number of tag bytes in spare area
>> + - nvidia,tag-ecc-bytes : Number ECC bytes to be generated for tag bytes
>> +
>> +(replace -bytes with -size or -length?)
>
> I like bytes -- makes the unit clear.

OK, will remove comment.

>
>> +Nvidia NAND Controller
>> +----------------------
>> +
>> +The device node for a NAND flash controller is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>> +following modifications and additions :
>> +
>> +Optional properties:
>> +
>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>> +             phandle, parameter, flags
>
> nvidia,nand-wp-gpio

Done, nvidia,nand-wp-gpios which seems to the the standard in Linux.

>
>> +nvidia,,width : bus width of the NAND device in bits
>
> s/,,width/,nand-width/

Done

>
>> +For now here is something specific to the Nvidia controller,
>
> Isn't this whole file specific to the nvidia controller?

Yes, removed.

>
> -Scott
>

Regards,
Simon

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/13/2012 02:01 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Fri, Apr 13, 2012 at 11:43 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/13/2012 01:29 PM, Simon Glass wrote:
>>> Add a NAND controller along with a bindings file for review.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>> Changes in v2:
>>> - Update NAND binding to add "nvidia," prefix
>>>
>>>  arch/arm/dts/tegra20.dtsi                     |    6 ++
>>>  doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
>>>  2 files changed, 74 insertions(+), 0 deletions(-)
>>>  create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt
>>>
>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>> index bc64f42..7be0462 100644
>>> --- a/arch/arm/dts/tegra20.dtsi
>>> +++ b/arch/arm/dts/tegra20.dtsi
>>> @@ -200,4 +200,10 @@
>>>               reg = <0x7000f400 0x200>;
>>>       };
>>>
>>> +     nand: nand-controller@0x70008000 {
>>
>> s/nand-controller@/flash@/ (or "nand@" if you really want -- there's
>> enough of that in use already)
> 
> Changed to flash@.
> 
> I am a little concerned that we are co-mingling the controller with
> the device, but I think this is ok.

No, you're right -- it should be something like nand-controller@.  For
some reason I didn't notice the node split when I wrote that.

>>> +             #address-cells = <0>;
>>> +             #size-cells = <0>;
>>> +             compatible = "nvidia,tegra20-nand";
>>> +             reg = <0x70008000 0x100>;
>>> +     };
>>>  };
>>> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>> new file mode 100644
>>> index 0000000..b19ce8e
>>> --- /dev/null
>>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>> @@ -0,0 +1,68 @@
>>> +NAND Flash
>>> +----------
>>> +
>>> +(there isn't yet a generic binding in Linux, so this describes what is in
>>> +U-Boot)
>>
>> Ideally the binding should not be Linux-specific or U-Boot specific --
>> it's just the binding that describes this hardware.
> 
> Agreed, but trying to agree a binding in Linux in the absence of a
> driver may be beyond my powers.

It shouldn't be, and if it is then we should move on to a better binding
repository (Grant set up devicetree.org for this a while back, but I'm
not sure what the process is for considering a binding there to be final).

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Scott,

On Fri, Apr 13, 2012 at 12:07 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/13/2012 02:01 PM, Simon Glass wrote:
>> Hi Scott,
>>
>> On Fri, Apr 13, 2012 at 11:43 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 04/13/2012 01:29 PM, Simon Glass wrote:
>>>> Add a NAND controller along with a bindings file for review.
>>>>
>>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>>> ---
>>>> Changes in v2:
>>>> - Update NAND binding to add "nvidia," prefix
>>>>
>>>>  arch/arm/dts/tegra20.dtsi                     |    6 ++
>>>>  doc/device-tree-bindings/nand/nvidia-nand.txt |   68 +++++++++++++++++++++++++
>>>>  2 files changed, 74 insertions(+), 0 deletions(-)
>>>>  create mode 100644 doc/device-tree-bindings/nand/nvidia-nand.txt
>>>>
>>>> diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
>>>> index bc64f42..7be0462 100644
>>>> --- a/arch/arm/dts/tegra20.dtsi
>>>> +++ b/arch/arm/dts/tegra20.dtsi
>>>> @@ -200,4 +200,10 @@
>>>>               reg = <0x7000f400 0x200>;
>>>>       };
>>>>
>>>> +     nand: nand-controller@0x70008000 {
>>>
>>> s/nand-controller@/flash@/ (or "nand@" if you really want -- there's
>>> enough of that in use already)
>>
>> Changed to flash@.
>>
>> I am a little concerned that we are co-mingling the controller with
>> the device, but I think this is ok.
>
> No, you're right -- it should be something like nand-controller@.  For
> some reason I didn't notice the node split when I wrote that.

OK, changed it back.

>
>>>> +             #address-cells = <0>;
>>>> +             #size-cells = <0>;
>>>> +             compatible = "nvidia,tegra20-nand";
>>>> +             reg = <0x70008000 0x100>;
>>>> +     };
>>>>  };
>>>> diff --git a/doc/device-tree-bindings/nand/nvidia-nand.txt b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>>> new file mode 100644
>>>> index 0000000..b19ce8e
>>>> --- /dev/null
>>>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>>> @@ -0,0 +1,68 @@
>>>> +NAND Flash
>>>> +----------
>>>> +
>>>> +(there isn't yet a generic binding in Linux, so this describes what is in
>>>> +U-Boot)
>>>
>>> Ideally the binding should not be Linux-specific or U-Boot specific --
>>> it's just the binding that describes this hardware.
>>
>> Agreed, but trying to agree a binding in Linux in the absence of a
>> driver may be beyond my powers.
>
> It shouldn't be, and if it is then we should move on to a better binding
> repository (Grant set up devicetree.org for this a while back, but I'm
> not sure what the process is for considering a binding there to be final).

Well we probably agree there should be a new repo for this. This is
going to the right mailing list (Devicetree Discuss), so people can
chime in as needed.

>
> -Scott
>

Regards,
Simon

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Stephen Warren]

On 04/13/2012 12:43 PM, Scott Wood wrote:
> On 04/13/2012 01:29 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>

>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt

>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>> +		phandle, parameter, flags
> 
> nvidia,nand-wp-gpio

I'm not convinced about this. For example many SDHCI bindings use just
"wp-gpios" not "shdci-wp-gpios". Is there really a need to keep the
property names unique across all bindings, even though a given node only
relies on one binding?

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Stephen Warren]

On 04/13/2012 12:29 PM, Simon Glass wrote:
> Add a NAND controller along with a bindings file for review.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt

I'd prefer this be called nvidia,tegra20-nand.txt so filenames are named
according to compatible value. This makes it easier to look things up.

> +The device node for a NAND flash device is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the

This is really based on USB?

> +Required properties :
> + - compatible : Should be "manufacture,device", "nand-flash"
> + - nvidia,page-data-bytes : Number of bytes in the data area
> + - nvidia,page-spare-bytes : * Number of bytes in spare area

Not sure what that "*" is?

> +Nvidia NAND Controller
> +----------------------
> +
> +The device node for a NAND flash controller is as described in the document
> +"Open Firmware Recommended Practice : Universal Serial Bus" with the

USB again?

> +nand-controller@0x70008000 {
> +	compatible = "nvidia,tegra20-nand";
> +	wp-gpios = <&gpio 59 0>;		/* PH3 */
> +	nvidia,width = <8>;
> +	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
> +	nand@0 {
> +		compatible = "hynix,hy27uf4g2b", "nand-flash";

The TRM says there can be up to 8 chip selects. Don't the NAND device
sub-nodes need a reg property to indicate which chip-select they're on?

Also, the TRM mentions async vs. ONFI devices. Don't we need properties
somewhere to configure that kind of thing?

> +		nvidia,page-data-bytes = <2048>;
> +		nvidia,tag-ecc-bytes = <4>;
> +		nvidia,tag-bytes = <20>;
> +		nvidia,data-ecc-bytes = <36>;
> +		nvidia,skipped-spare-bytes = <4>;
> +		nvidia,page-spare-bytes = <64>;
> +	};
> +};

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/13/2012 04:05 PM, Stephen Warren wrote:
> On 04/13/2012 12:29 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>>
>> Signed-off-by: Simon Glass<sjg@chromium.org>
>
>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>
> I'd prefer this be called nvidia,tegra20-nand.txt so filenames are named
> according to compatible value. This makes it easier to look things up.

Could be awkward if additional compatibles are added, though.  Grep can 
find compatibles within the text.

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/13/2012 03:58 PM, Stephen Warren wrote:
> On 04/13/2012 12:43 PM, Scott Wood wrote:
>> On 04/13/2012 01:29 PM, Simon Glass wrote:
>>> Add a NAND controller along with a bindings file for review.
>>>
>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>
>>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>
>>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>>> +		phandle, parameter, flags
>>
>> nvidia,nand-wp-gpio
>
> I'm not convinced about this. For example many SDHCI bindings use just
> "wp-gpios" not "shdci-wp-gpios". Is there really a need to keep the
> property names unique across all bindings, even though a given node only
> relies on one binding?
>

Yeah, there's a lot of bad practice in the existing trees.  But the 
general recommendation for a while now has been to namespace properties 
that aren't defined in standardized, device-indpendent way.  That way we 
don't get conflicts if we want to use that name for a standard property 
in the future, and there's less confusion if multiple people use the 
same name in different devices with different semantics.

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Stephen Warren]

On 04/13/2012 03:21 PM, Scott Wood wrote:
> On 04/13/2012 03:58 PM, Stephen Warren wrote:
>> On 04/13/2012 12:43 PM, Scott Wood wrote:
>>> On 04/13/2012 01:29 PM, Simon Glass wrote:
>>>> Add a NAND controller along with a bindings file for review.
>>>>
>>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>
>>>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>
>>>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>>>> +        phandle, parameter, flags
>>>
>>> nvidia,nand-wp-gpio
>>
>> I'm not convinced about this. For example many SDHCI bindings use just
>> "wp-gpios" not "shdci-wp-gpios". Is there really a need to keep the
>> property names unique across all bindings, even though a given node only
>> relies on one binding?
>>
> 
> Yeah, there's a lot of bad practice in the existing trees.  But the
> general recommendation for a while now has been to namespace properties
> that aren't defined in standardized, device-indpendent way.  That way we
> don't get conflicts if we want to use that name for a standard property
> in the future, and there's less confusion if multiple people use the
> same name in different devices with different semantics.

I thought that's what the "nvidia," vendor prefix was for. Presumably
standardized properties wouldn't have that?

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/13/2012 04:22 PM, Stephen Warren wrote:
> On 04/13/2012 03:21 PM, Scott Wood wrote:
>> On 04/13/2012 03:58 PM, Stephen Warren wrote:
>>> On 04/13/2012 12:43 PM, Scott Wood wrote:
>>>> On 04/13/2012 01:29 PM, Simon Glass wrote:
>>>>> Add a NAND controller along with a bindings file for review.
>>>>>
>>>>> Signed-off-by: Simon Glass<sjg@chromium.org>
>>>
>>>>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>>>
>>>>> +wp-gpio : GPIO of write-protect line, three cells in the format:
>>>>> +        phandle, parameter, flags
>>>>
>>>> nvidia,nand-wp-gpio
>>>
>>> I'm not convinced about this. For example many SDHCI bindings use just
>>> "wp-gpios" not "shdci-wp-gpios". Is there really a need to keep the
>>> property names unique across all bindings, even though a given node only
>>> relies on one binding?
>>>
>>
>> Yeah, there's a lot of bad practice in the existing trees.  But the
>> general recommendation for a while now has been to namespace properties
>> that aren't defined in standardized, device-indpendent way.  That way we
>> don't get conflicts if we want to use that name for a standard property
>> in the future, and there's less confusion if multiple people use the
>> same name in different devices with different semantics.
>
> I thought that's what the "nvidia," vendor prefix was for.

Yes, and it applies to non-standard properties too.

> Presumably standardized properties wouldn't have that?

Right.

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Stephen,

On Fri, Apr 13, 2012 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/13/2012 12:29 PM, Simon Glass wrote:
>> Add a NAND controller along with a bindings file for review.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
>> +++ b/doc/device-tree-bindings/nand/nvidia-nand.txt
>
> I'd prefer this be called nvidia,tegra20-nand.txt so filenames are named
> according to compatible value. This makes it easier to look things up.

OK, changed

>
>> +The device node for a NAND flash device is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>
> This is really based on USB?

Well just in that it follows the conventions. I will remove this since
I'm not sure we use anything that isn't in the ePAPR.

>
>> +Required properties :
>> + - compatible : Should be "manufacture,device", "nand-flash"
>> + - nvidia,page-data-bytes : Number of bytes in the data area
>> + - nvidia,page-spare-bytes : * Number of bytes in spare area
>
> Not sure what that "*" is?

Removed

>
>> +Nvidia NAND Controller
>> +----------------------
>> +
>> +The device node for a NAND flash controller is as described in the document
>> +"Open Firmware Recommended Practice : Universal Serial Bus" with the
>
> USB again?

Removed

>
>> +nand-controller@0x70008000 {
>> +     compatible = "nvidia,tegra20-nand";
>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>> +     nvidia,width = <8>;
>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>> +     nand@0 {
>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>
> The TRM says there can be up to 8 chip selects. Don't the NAND device
> sub-nodes need a reg property to indicate which chip-select they're on?

We don't have driver support for this at present.

>
> Also, the TRM mentions async vs. ONFI devices. Don't we need properties
> somewhere to configure that kind of thing?

We don't have driver support for this at present, either :-(

>
>> +             nvidia,page-data-bytes = <2048>;
>> +             nvidia,tag-ecc-bytes = <4>;
>> +             nvidia,tag-bytes = <20>;
>> +             nvidia,data-ecc-bytes = <36>;
>> +             nvidia,skipped-spare-bytes = <4>;
>> +             nvidia,page-spare-bytes = <64>;
>> +     };
>> +};

Regards,
Simon

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/17/2012 01:33 PM, Simon Glass wrote:
> Hi Stephen,
> 
> On Fri, Apr 13, 2012 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 04/13/2012 12:29 PM, Simon Glass wrote:
>>> +nand-controller@0x70008000 {
>>> +     compatible = "nvidia,tegra20-nand";
>>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>>> +     nvidia,width = <8>;
>>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>>> +     nand@0 {
>>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>>
>> The TRM says there can be up to 8 chip selects. Don't the NAND device
>> sub-nodes need a reg property to indicate which chip-select they're on?
> 
> We don't have driver support for this at present.

That shouldn't matter.  The device tree is about describing the
hardware.  Ideally the device tree shouldn't have to change if in the
future you do get driver support for it.

Also, unit addresses should only be present if reg is present, and they
should match.

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi,

On Tue, Apr 17, 2012 at 11:38 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/17/2012 01:33 PM, Simon Glass wrote:
>> Hi Stephen,
>>
>> On Fri, Apr 13, 2012 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>> On 04/13/2012 12:29 PM, Simon Glass wrote:
>>>> +nand-controller@0x70008000 {
>>>> +     compatible = "nvidia,tegra20-nand";
>>>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>>>> +     nvidia,width = <8>;
>>>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>>>> +     nand@0 {
>>>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>>>
>>> The TRM says there can be up to 8 chip selects. Don't the NAND device
>>> sub-nodes need a reg property to indicate which chip-select they're on?
>>
>> We don't have driver support for this at present.
>
> That shouldn't matter.  The device tree is about describing the
> hardware.  Ideally the device tree shouldn't have to change if in the
> future you do get driver support for it.
>
> Also, unit addresses should only be present if reg is present, and they
> should match.

OK I will leave @0 in there, and add a reg property to the node.

>
> -Scott
>

Regards,
Simon

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Scott Wood]

On 04/17/2012 01:44 PM, Simon Glass wrote:
> Hi,
> 
> On Tue, Apr 17, 2012 at 11:38 AM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/17/2012 01:33 PM, Simon Glass wrote:
>>> Hi Stephen,
>>>
>>> On Fri, Apr 13, 2012 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>> On 04/13/2012 12:29 PM, Simon Glass wrote:
>>>>> +nand-controller@0x70008000 {
>>>>> +     compatible = "nvidia,tegra20-nand";
>>>>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>>>>> +     nvidia,width = <8>;
>>>>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>>>>> +     nand@0 {
>>>>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>>>>
>>>> The TRM says there can be up to 8 chip selects. Don't the NAND device
>>>> sub-nodes need a reg property to indicate which chip-select they're on?
>>>
>>> We don't have driver support for this at present.
>>
>> That shouldn't matter.  The device tree is about describing the
>> hardware.  Ideally the device tree shouldn't have to change if in the
>> future you do get driver support for it.
>>
>> Also, unit addresses should only be present if reg is present, and they
>> should match.
> 
> OK I will leave @0 in there, and add a reg property to the node.

Also set #address-cells = <1> and #size-cells = <0> in the controller node.

-Scott

Re: [PATCH v2 4/7] tegra: fdt: Add NAND controller binding and definitions

[Simon Glass]

Hi Scott,

On Tue, Apr 17, 2012 at 11:45 AM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/17/2012 01:44 PM, Simon Glass wrote:
>> Hi,
>>
>> On Tue, Apr 17, 2012 at 11:38 AM, Scott Wood <scottwood@freescale.com> wrote:
>>> On 04/17/2012 01:33 PM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On Fri, Apr 13, 2012 at 2:05 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>> On 04/13/2012 12:29 PM, Simon Glass wrote:
>>>>>> +nand-controller@0x70008000 {
>>>>>> +     compatible = "nvidia,tegra20-nand";
>>>>>> +     wp-gpios = <&gpio 59 0>;                /* PH3 */
>>>>>> +     nvidia,width = <8>;
>>>>>> +     nvidia,timing = <26 100 20 80 20 10 12 10 70>;
>>>>>> +     nand@0 {
>>>>>> +             compatible = "hynix,hy27uf4g2b", "nand-flash";
>>>>>
>>>>> The TRM says there can be up to 8 chip selects. Don't the NAND device
>>>>> sub-nodes need a reg property to indicate which chip-select they're on?
>>>>
>>>> We don't have driver support for this at present.
>>>
>>> That shouldn't matter.  The device tree is about describing the
>>> hardware.  Ideally the device tree shouldn't have to change if in the
>>> future you do get driver support for it.
>>>
>>> Also, unit addresses should only be present if reg is present, and they
>>> should match.
>>
>> OK I will leave @0 in there, and add a reg property to the node.
>
> Also set #address-cells = <1> and #size-cells = <0> in the controller node.

Thanks, yes spotted that - dtc crashes if you have address cells as zero.

Will send a new series out in a minute.

>
> -Scott
>

Regards,
Simon