diff mbox

[U-Boot,v3,4/7] tegra: fdt: Add NAND controller binding and definitions

Message ID 1334688614-4977-5-git-send-email-sjg@chromium.org
State Superseded, archived
Headers show

Commit Message

Simon Glass April 17, 2012, 6:50 p.m. UTC 
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

Changes in v3:
- Add reg property for unit address (should be used for chip select)
- Change note in fdt binding about the need for a hardware-specific binding
- Fix up typos in fdt binding, and rename the file
- Update fdt binding to make everything Nvidia-specific

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

Comments

Scott Wood April 17, 2012, 7:06 p.m. UTC | #1 
On 04/17/2012 01:50 PM, Simon Glass wrote:
> diff --git a/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt b/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt
> new file mode 100644
> index 0000000..2484556
> --- /dev/null
> +++ b/doc/device-tree-bindings/nand/nvidia,tegra20-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. There should not be Linux-specific or U-Boot specific binding, just
> +a binding that describes this hardware. But agreeing a binding in Linux in
> +the absence of a driver may be beyond my powers.)
> +
> +The device node for a NAND flash device is as follows:
> +
> +Required properties :
> + - compatible : Should be "manufacturer,device", "nand-flash"

Again, "nand-flash" is not an appropriate compatible.  There is no
generic nand-flash binding.

> + - 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

Do you really need this stuff to be in the device tree?  You should be
able to determine this information from the ID table.

> + - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
> +	(these are typically used for bad block maintenance)

So this binding can't deal with the bad block marker being somewhere
other than the beginning of the spare area (e.g. 8-bit small page NAND)?

> + - nvidia,data-ecc-bytes : Number of ECC bytes for data area

Number of ECC bytes per page?  Number of ECC bytes per ECC block?
Number of data bytes per ECC block?

> + - nvidia,tag-bytes :Number of tag bytes in spare area

What are tag bytes?

> +Nvidia NAND Controller
> +----------------------
> +
> +The device node for a NAND flash controller is as follows:
> +
> +Optional properties:
> +
> +nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
> +		phandle, parameter, flags

Doesn't the number of cells depend on the GPIO controller binding?

> +nvidia,nand-width : bus width of the NAND device in bits
> +
> + - 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

Might want to point out that there's one cell per timing parameter.

-Scott
Simon Glass April 17, 2012, 8:18 p.m. UTC | #2 
+Jim, who wrote the driver originally

Hi Scott,

On Tue, Apr 17, 2012 at 12:06 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/17/2012 01:50 PM, Simon Glass wrote:
>> diff --git a/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt b/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt
>> new file mode 100644
>> index 0000000..2484556
>> --- /dev/null
>> +++ b/doc/device-tree-bindings/nand/nvidia,tegra20-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. There should not be Linux-specific or U-Boot specific binding, just
>> +a binding that describes this hardware. But agreeing a binding in Linux in
>> +the absence of a driver may be beyond my powers.)
>> +
>> +The device node for a NAND flash device is as follows:
>> +
>> +Required properties :
>> + - compatible : Should be "manufacturer,device", "nand-flash"
>
> Again, "nand-flash" is not an appropriate compatible.  There is no
> generic nand-flash binding.

OK, so I should just drop this.

>
>> + - 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
>
> Do you really need this stuff to be in the device tree?  You should be
> able to determine this information from the ID table.

I suspect so - the driver originally had a lot of CONFIGs for this.
Maybe someone who wants to take it further could do this as part of
supporting ONFI?

I will see if Jim Lin can take another look.

>
>> + - nvidia,skipped-spare-bytes : Number of bytes to skip at start of spare area
>> +     (these are typically used for bad block maintenance)
>
> So this binding can't deal with the bad block marker being somewhere
> other than the beginning of the spare area (e.g. 8-bit small page NAND)?

I suppose it depends on what you want to put afterwards.

>
>> + - nvidia,data-ecc-bytes : Number of ECC bytes for data area
>
> Number of ECC bytes per page?  Number of ECC bytes per ECC block?
> Number of data bytes per ECC block?
>
>> + - nvidia,tag-bytes :Number of tag bytes in spare area
>
> What are tag bytes?

I know that term from YAFFS, but other than that I am not sure.

>
>> +Nvidia NAND Controller
>> +----------------------
>> +
>> +The device node for a NAND flash controller is as follows:
>> +
>> +Optional properties:
>> +
>> +nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
>> +             phandle, parameter, flags
>
> Doesn't the number of cells depend on the GPIO controller binding?

Yes, but this is the binding Tegra uses.

>
>> +nvidia,nand-width : bus width of the NAND device in bits
>> +
>> + - 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
>
> Might want to point out that there's one cell per timing parameter.

OK.

I'm going to wait and see if Jim can pick up this driver and make the
device-tree change you requested.

>
> -Scott
>

Regards,
Simon
Scott Wood April 17, 2012, 8:31 p.m. UTC | #3 
On 04/17/2012 03:18 PM, Simon Glass wrote:
> +Jim, who wrote the driver originally
> 
> Hi Scott,
> 
> On Tue, Apr 17, 2012 at 12:06 PM, Scott Wood <scottwood@freescale.com> wrote:
>>> + - 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
>>
>> Do you really need this stuff to be in the device tree?  You should be
>> able to determine this information from the ID table.
> 
> I suspect so - the driver originally had a lot of CONFIGs for this.
> Maybe someone who wants to take it further could do this as part of
> supporting ONFI?
> 
> I will see if Jim Lin can take another look.

You don't need ONFI to get the page/spare size out of the ID table.

The generic NAND code should already be doing this for you (fills in
mtd->writesize and mtd->oobsize).  If you need it during setup, we now
have CONFIG_SYS_NAND_SELF_INIT that allows splitting up
nand_scan_ident() from nand_scan_tail().

>>> +Nvidia NAND Controller
>>> +----------------------
>>> +
>>> +The device node for a NAND flash controller is as follows:
>>> +
>>> +Optional properties:
>>> +
>>> +nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
>>> +             phandle, parameter, flags
>>
>> Doesn't the number of cells depend on the GPIO controller binding?
> 
> Yes, but this is the binding Tegra uses.

Still, it doesn't belong in the NAND binding.  Maybe a future chip wants
to use this NAND binding but a different GPIO binding.  If nothing else,
people tend to copy-and-paste such descriptions.  We've still got people
adding bindings for Freescale devices saying interrupts are encoded as a
pair of cells, even though the interrupt controller now uses four cells
per interrupt.

-Scott
Simon Glass April 17, 2012, 8:36 p.m. UTC | #4 
Hi Scott,

On Tue, Apr 17, 2012 at 1:31 PM, Scott Wood <scottwood@freescale.com> wrote:
> On 04/17/2012 03:18 PM, Simon Glass wrote:
>> +Jim, who wrote the driver originally
>>
>> Hi Scott,
>>
>> On Tue, Apr 17, 2012 at 12:06 PM, Scott Wood <scottwood@freescale.com> wrote:
>>>> + - 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
>>>
>>> Do you really need this stuff to be in the device tree?  You should be
>>> able to determine this information from the ID table.
>>
>> I suspect so - the driver originally had a lot of CONFIGs for this.
>> Maybe someone who wants to take it further could do this as part of
>> supporting ONFI?
>>
>> I will see if Jim Lin can take another look.
>
> You don't need ONFI to get the page/spare size out of the ID table.
>
> The generic NAND code should already be doing this for you (fills in
> mtd->writesize and mtd->oobsize).  If you need it during setup, we now
> have CONFIG_SYS_NAND_SELF_INIT that allows splitting up
> nand_scan_ident() from nand_scan_tail().

OK, sounds good. It should just need to be changed over to calculate
these values from the nand layer.

>
>>>> +Nvidia NAND Controller
>>>> +----------------------
>>>> +
>>>> +The device node for a NAND flash controller is as follows:
>>>> +
>>>> +Optional properties:
>>>> +
>>>> +nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
>>>> +             phandle, parameter, flags
>>>
>>> Doesn't the number of cells depend on the GPIO controller binding?
>>
>> Yes, but this is the binding Tegra uses.
>
> Still, it doesn't belong in the NAND binding.  Maybe a future chip wants
> to use this NAND binding but a different GPIO binding.  If nothing else,
> people tend to copy-and-paste such descriptions.  We've still got people
> adding bindings for Freescale devices saying interrupts are encoded as a
> pair of cells, even though the interrupt controller now uses four cells
> per interrupt.

OK I see - are you are saying that we should just say something like:

"nvidia,wp-gpios : GPIO of write-protect line, as defined by gpio bindings"

>
> -Scott
>

Regards,
Simon
Scott Wood April 17, 2012, 8:49 p.m. UTC | #5 
On 04/17/2012 03:36 PM, Simon Glass wrote:
> Hi Scott,
> 
> On Tue, Apr 17, 2012 at 1:31 PM, Scott Wood <scottwood@freescale.com> wrote:
>> On 04/17/2012 03:18 PM, Simon Glass wrote:
>>> On Tue, Apr 17, 2012 at 12:06 PM, Scott Wood <scottwood@freescale.com> wrote:
>>>> Doesn't the number of cells depend on the GPIO controller binding?
>>>
>>> Yes, but this is the binding Tegra uses.
>>
>> Still, it doesn't belong in the NAND binding.  Maybe a future chip wants
>> to use this NAND binding but a different GPIO binding.  If nothing else,
>> people tend to copy-and-paste such descriptions.  We've still got people
>> adding bindings for Freescale devices saying interrupts are encoded as a
>> pair of cells, even though the interrupt controller now uses four cells
>> per interrupt.
> 
> OK I see - are you are saying that we should just say something like:
> 
> "nvidia,wp-gpios : GPIO of write-protect line, as defined by gpio bindings"

Yes.  If there were more than one GPIO line, you'd specify which one is
which, similar to reg and interrupts.

-Scott
diff mbox

Patch

diff --git a/arch/arm/dts/tegra20.dtsi b/arch/arm/dts/tegra20.dtsi
index bc64f42..018a3c8 100644
--- a/arch/arm/dts/tegra20.dtsi
+++ b/arch/arm/dts/tegra20.dtsi
@@ -200,4 +200,10 @@ 
 		reg = <0x7000f400 0x200>;
 	};
 
+	nand: nand-controller@70008000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "nvidia,tegra20-nand";
+		reg = <0x70008000 0x100>;
+	};
 };
diff --git a/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt b/doc/device-tree-bindings/nand/nvidia,tegra20-nand.txt
new file mode 100644
index 0000000..2484556
--- /dev/null
+++ b/doc/device-tree-bindings/nand/nvidia,tegra20-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. There should not be Linux-specific or U-Boot specific binding, just
+a binding that describes this hardware. But agreeing a binding in Linux in
+the absence of a driver may be beyond my powers.)
+
+The device node for a NAND flash device is as follows:
+
+Required properties :
+ - compatible : Should be "manufacturer,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
+
+This node should sit inside its controller.
+
+
+Nvidia NAND Controller
+----------------------
+
+The device node for a NAND flash controller is as follows:
+
+Optional properties:
+
+nvidia,wp-gpios : GPIO of write-protect line, three cells in the format:
+		phandle, parameter, flags
+nvidia,nand-width : bus width of the NAND device in bits
+
+ - 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
+
+The 'reg' property should provide the chip select used by the flash chip.
+
+
+Example
+-------
+
+nand-controller@0x70008000 {
+	compatible = "nvidia,tegra20-nand";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	nvidia,wp-gpios = <&gpio 59 0>;		/* PH3 */
+	nvidia,nand-width = <8>;
+	nvidia,timing = <26 100 20 80 20 10 12 10 70>;
+	nand@0 {
+		reg = <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>;
+	};
+};