diff mbox

[v3,3/4] powerpc: NAND: FSL UPM: document new bindings

Message ID 1237975701-23201-4-git-send-email-wg@grandegger.com
State New, archived
Headers show

Commit Message

Wolfgang Grandegger March 25, 2009, 10:08 a.m. UTC
This patch adds documentation for the new NAND FSL UPM bindings for:

 NAND: FSL-UPM: add multi chip support
 NAND: FSL-UPM: Add wait flags to support board/chip specific delays

Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
---
 .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
 1 files changed, 37 insertions(+), 2 deletions(-)

Comments

Anton Vorontsov March 25, 2009, 3:11 p.m. UTC | #1
On Wed, Mar 25, 2009 at 11:08:20AM +0100, Wolfgang Grandegger wrote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
> 
>  NAND: FSL-UPM: add multi chip support
>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
> 
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> ---

To me it looks good.

Acked-by: Anton Vorontsov <avorontsov@ru.mvista.com>

>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
>  - reg : should specify localbus chip select and size used for the chip.
>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>  
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> +  		       UPM pattern (0x1), after writing a data byte (0x2)
> +		       or after writing out a buffer (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
> +	  (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
> +	  required.
> +- chip-delay : chip dependent delay for transfering data from array to
> +	       read registers (tR). Required if property "gpios" is not
> +	       used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> + 		corresponding address lines are used to select the chip.
> +
> +Examples:
>  
>  upm@1,0 {
>  	compatible = "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
>  		};
>  	};
>  };
> +
> +upm@3,0 {
> +	compatible = "fsl,upm-nand";
> +	reg = <3 0x0 0x800>;
> +	fsl,upm-addr-offset = <0x10>;
> +	fsl,upm-cmd-offset = <0x08>;
> +	fsl,upm-wait-flags = <0x5>;
> +	/* Multi-chip device */
> +	num-chips = <2>;
> +	chip-offset = <0x200>;
> +	chip-delay = <25>; // in micro-seconds
> +
> +	nand@0 {
> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +
> +		partition@0 {
> +			    label = "fs";
> +			    reg = <0x00000000 0x10000000>;
> +		};
> +	};
> +};
> -- 
> 1.6.0.6
Grant Likely March 25, 2009, 5:48 p.m. UTC | #2
(cc'ing devicetree-discuss)

On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> This patch adds documentation for the new NAND FSL UPM bindings for:
>
>  NAND: FSL-UPM: add multi chip support
>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>
> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>

Mostly looks good to me; but some comments below.

> ---
>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>  1 files changed, 37 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> index 84a04d5..0272e70 100644
> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
> @@ -5,9 +5,22 @@ Required properties:
>  - reg : should specify localbus chip select and size used for the chip.
>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>
> -Example:
> +Optional properties:
> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
> +                      UPM pattern (0x1), after writing a data byte (0x2)
> +                      or after writing out a buffer (0x4).
> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
> +         (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
> +         required.
> +- chip-delay : chip dependent delay for transfering data from array to
> +              read registers (tR). Required if property "gpios" is not
> +              used (R/B# pins not connected).
> +- num-chips : number of chips per device for multi-chip support.
> +- chip-offset : address offset between chips for multi-chip support. The
> +               corresponding address lines are used to select the chip.

Since these properties (chip-delay, num-chips and chip-offset) are
currently controller specific, it would probably be a good idea to
prefix 'fsl,' onto them.  That way when common NAND controller
properties start getting defined then there won't be any concern about
conflicting with existing meanings.  If you do see these as properties
that other NAND controllers will use, then maybe a 'nand-' prefix is
appropriate (like the SPI binding in booting-without-of).

For the chip offset, it's not clear what the meaning is.  First, does
the UPM controller support access of multiple chips simultaneously?
If so, then can you elaborate in the description on how board design
translates to a chip-offset value.  If it cannot, then it might be
better to have multiple tuples in the 'reg' property for each discrete
chip.  Multiple reg tuples would also remove the need for the
num-chips property.

Cheers,
g.

> +
> +Examples:
>
>  upm@1,0 {
>        compatible = "fsl,upm-nand";
> @@ -26,3 +39,25 @@ upm@1,0 {
>                };
>        };
>  };
> +
> +upm@3,0 {
> +       compatible = "fsl,upm-nand";
> +       reg = <3 0x0 0x800>;
> +       fsl,upm-addr-offset = <0x10>;
> +       fsl,upm-cmd-offset = <0x08>;
> +       fsl,upm-wait-flags = <0x5>;
> +       /* Multi-chip device */
> +       num-chips = <2>;
> +       chip-offset = <0x200>;
> +       chip-delay = <25>; // in micro-seconds
> +
> +       nand@0 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +
> +               partition@0 {
> +                           label = "fs";
> +                           reg = <0x00000000 0x10000000>;
> +               };
> +       };
> +};
> --
> 1.6.0.6
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
Wolfgang Grandegger March 25, 2009, 8:48 p.m. UTC | #3
Grant Likely wrote:
> (cc'ing devicetree-discuss)
> 
> On Wed, Mar 25, 2009 at 4:08 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> This patch adds documentation for the new NAND FSL UPM bindings for:
>>
>>  NAND: FSL-UPM: add multi chip support
>>  NAND: FSL-UPM: Add wait flags to support board/chip specific delays
>>
>> Signed-off-by: Wolfgang Grandegger <wg@grandegger.com>
> 
> Mostly looks good to me; but some comments below.
> 
>> ---
>>  .../powerpc/dts-bindings/fsl/upm-nand.txt          |   39 +++++++++++++++++++-
>>  1 files changed, 37 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> index 84a04d5..0272e70 100644
>> --- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> +++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
>> @@ -5,9 +5,22 @@ Required properties:
>>  - reg : should specify localbus chip select and size used for the chip.
>>  - fsl,upm-addr-offset : UPM pattern offset for the address latch.
>>  - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
>> -- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
>>
>> -Example:
>> +Optional properties:
>> +- fsl,upm-wait-flags : add chip-dependent short delays after running the
>> +                      UPM pattern (0x1), after writing a data byte (0x2)
>> +                      or after writing out a buffer (0x4).
>> +- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
>> +         (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
>> +         required.
>> +- chip-delay : chip dependent delay for transfering data from array to
>> +              read registers (tR). Required if property "gpios" is not
>> +              used (R/B# pins not connected).
>> +- num-chips : number of chips per device for multi-chip support.
>> +- chip-offset : address offset between chips for multi-chip support. The
>> +               corresponding address lines are used to select the chip.
> 
> Since these properties (chip-delay, num-chips and chip-offset) are
> currently controller specific, it would probably be a good idea to
> prefix 'fsl,' onto them.  That way when common NAND controller
> properties start getting defined then there won't be any concern about
> conflicting with existing meanings.  If you do see these as properties
> that other NAND controllers will use, then maybe a 'nand-' prefix is
> appropriate (like the SPI binding in booting-without-of).

The chip-delay is NAND device specific. The proper value can be find in
the data sheet. num-chip is also quite generic. A prefix 'nand-' would
be fine for me. But fsl,upm-chip-offset would be more appropriate than
just chip-offset, indeed.

> For the chip offset, it's not clear what the meaning is.  First, does
> the UPM controller support access of multiple chips simultaneously?

The offset drives the corresponding address lines, which are used to
select the chip. That's how it's done on the TQM8548 board. In
principle, the chips could also be selected through dedicated GPIO pins.
Well, I'm not a hardware guy.

> If so, then can you elaborate in the description on how board design
> translates to a chip-offset value.  If it cannot, then it might be
> better to have multiple tuples in the 'reg' property for each discrete
> chip.  Multiple reg tuples would also remove the need for the
> num-chips property.

The node still describes one device mapping all relevant control
registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
would be more generic and makes num-chips obsolete as well. And the
property would be reserved for that way of implementing the chip select
in hardware.

Wolfgang.
Grant Likely March 26, 2009, 5:09 a.m. UTC | #4
On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> For the chip offset, it's not clear what the meaning is.  First, does
>> the UPM controller support access of multiple chips simultaneously?
>
> The offset drives the corresponding address lines, which are used to
> select the chip. That's how it's done on the TQM8548 board. In
> principle, the chips could also be selected through dedicated GPIO pins.
> Well, I'm not a hardware guy.

Heh.  I mean elaborate in the binding documentation.  :-)

>> If so, then can you elaborate in the description on how board design
>> translates to a chip-offset value.  If it cannot, then it might be
>> better to have multiple tuples in the 'reg' property for each discrete
>> chip.  Multiple reg tuples would also remove the need for the
>> num-chips property.
>
> The node still describes one device mapping all relevant control
> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
> would be more generic and makes num-chips obsolete as well. And the
> property would be reserved for that way of implementing the chip select
> in hardware.

It really sounds like this binding is describing multiple NAND chips
mapped to different base addresses (and looking at the fsm_upm.c
driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
 3 0x400 4>;

It is true that other methods could be used for implementing the chip
select, but that is *not* what the proposed binding describes.  This
proposed binding describes NAND chips selected by address lines
(particular addresses), and in this case I think using reg is the
natural description.

g.
Wolfgang Grandegger March 26, 2009, 7:42 a.m. UTC | #5
Grant Likely wrote:
> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> For the chip offset, it's not clear what the meaning is.  First, does
>>> the UPM controller support access of multiple chips simultaneously?
>> The offset drives the corresponding address lines, which are used to
>> select the chip. That's how it's done on the TQM8548 board. In
>> principle, the chips could also be selected through dedicated GPIO pins.
>> Well, I'm not a hardware guy.
> 
> Heh.  I mean elaborate in the binding documentation.  :-)
> 
>>> If so, then can you elaborate in the description on how board design
>>> translates to a chip-offset value.  If it cannot, then it might be
>>> better to have multiple tuples in the 'reg' property for each discrete
>>> chip.  Multiple reg tuples would also remove the need for the
>>> num-chips property.
>> The node still describes one device mapping all relevant control
>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>> would be more generic and makes num-chips obsolete as well. And the
>> property would be reserved for that way of implementing the chip select
>> in hardware.
> 
> It really sounds like this binding is describing multiple NAND chips
> mapped to different base addresses (and looking at the fsm_upm.c
> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>  3 0x400 4>;

The chip-offset, and not the address, needs to be added to the MAR
register as well before running the pattern:

        mar = cmd << (32 - fun->upm.width);

        if (fun->chip_offset && fun->chip_number > 0)

                mar |= fun->chip_number * fun->chip_offset;

        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);


> It is true that other methods could be used for implementing the chip
> select, but that is *not* what the proposed binding describes.  This
> proposed binding describes NAND chips selected by address lines
> (particular addresses), and in this case I think using reg is the
> natural description.

OK, the chips are selected by accessing a defined address range. Will
prepare a patch using the reg property.

Wolfgang.




> g.
>
Grant Likely March 26, 2009, 2:27 p.m. UTC | #6
On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> Grant Likely wrote:
>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>> the UPM controller support access of multiple chips simultaneously?
>>> The offset drives the corresponding address lines, which are used to
>>> select the chip. That's how it's done on the TQM8548 board. In
>>> principle, the chips could also be selected through dedicated GPIO pins.
>>> Well, I'm not a hardware guy.
>>
>> Heh.  I mean elaborate in the binding documentation.  :-)
>>
>>>> If so, then can you elaborate in the description on how board design
>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>> chip.  Multiple reg tuples would also remove the need for the
>>>> num-chips property.
>>> The node still describes one device mapping all relevant control
>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>> would be more generic and makes num-chips obsolete as well. And the
>>> property would be reserved for that way of implementing the chip select
>>> in hardware.
>>
>> It really sounds like this binding is describing multiple NAND chips
>> mapped to different base addresses (and looking at the fsm_upm.c
>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>  3 0x400 4>;
>
> The chip-offset, and not the address, needs to be added to the MAR
> register as well before running the pattern:
>
>        mar = cmd << (32 - fun->upm.width);
>
>        if (fun->chip_offset && fun->chip_number > 0)
>
>                mar |= fun->chip_number * fun->chip_offset;
>
>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>
>
>> It is true that other methods could be used for implementing the chip
>> select, but that is *not* what the proposed binding describes.  This
>> proposed binding describes NAND chips selected by address lines
>> (particular addresses), and in this case I think using reg is the
>> natural description.
>
> OK, the chips are selected by accessing a defined address range. Will
> prepare a patch using the reg property.

Hold on a sec.  I'm debating from my experience with device tree
bindings, but I'm fairly ignorant about the implementation of NAND on
UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
tell me so and why.  Your comment above about fsl_upm_run_pattern()
makes me doubt my position.

Does using the reg property give the driver enough information to
reliably program the MAR for NAND connections that use the address
line chip select scheme?  Related to that, should the binding include
a property that explicitly states that an address line chip select
scheme is being used?

g.
Wolfgang Grandegger March 26, 2009, 3:33 p.m. UTC | #7
Grant Likely wrote:
> On Thu, Mar 26, 2009 at 1:42 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> On Wed, Mar 25, 2009 at 2:48 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>>> Grant Likely wrote:
>>>>> For the chip offset, it's not clear what the meaning is.  First, does
>>>>> the UPM controller support access of multiple chips simultaneously?
>>>> The offset drives the corresponding address lines, which are used to
>>>> select the chip. That's how it's done on the TQM8548 board. In
>>>> principle, the chips could also be selected through dedicated GPIO pins.
>>>> Well, I'm not a hardware guy.
>>> Heh.  I mean elaborate in the binding documentation.  :-)
>>>
>>>>> If so, then can you elaborate in the description on how board design
>>>>> translates to a chip-offset value.  If it cannot, then it might be
>>>>> better to have multiple tuples in the 'reg' property for each discrete
>>>>> chip.  Multiple reg tuples would also remove the need for the
>>>>> num-chips property.
>>>> The node still describes one device mapping all relevant control
>>>> registers. How about using fsl,upm-chip-offsets = <0x200 0x400>. It
>>>> would be more generic and makes num-chips obsolete as well. And the
>>>> property would be reserved for that way of implementing the chip select
>>>> in hardware.
>>> It really sounds like this binding is describing multiple NAND chips
>>> mapped to different base addresses (and looking at the fsm_upm.c
>>> driver appears to confirm it).  So, does this work?  reg = <3 0x200 4
>>>  3 0x400 4>;
>> The chip-offset, and not the address, needs to be added to the MAR
>> register as well before running the pattern:
>>
>>        mar = cmd << (32 - fun->upm.width);
>>
>>        if (fun->chip_offset && fun->chip_number > 0)
>>
>>                mar |= fun->chip_number * fun->chip_offset;
>>
>>        fsl_upm_run_pattern(&fun->upm, chip->IO_ADDR_R, mar);
>>
>>
>>> It is true that other methods could be used for implementing the chip
>>> select, but that is *not* what the proposed binding describes.  This
>>> proposed binding describes NAND chips selected by address lines
>>> (particular addresses), and in this case I think using reg is the
>>> natural description.
>> OK, the chips are selected by accessing a defined address range. Will
>> prepare a patch using the reg property.
> 
> Hold on a sec.  I'm debating from my experience with device tree

I already started ;-).

> bindings, but I'm fairly ignorant about the implementation of NAND on
> UPM.  It *looks* to me like reg is sufficient, but if I'm wrong then
> tell me so and why.  Your comment above about fsl_upm_run_pattern()
> makes me doubt my position.

It's not sufficient to just map the related space and access it, at least.

> Does using the reg property give the driver enough information to
> reliably program the MAR for NAND connections that use the address
> line chip select scheme?  Related to that, should the binding include

In principle yes:

  if (i > 0)
      offset[i] = resource[i].start - resource[0].start;

> a property that explicitly states that an address line chip select
> scheme is being used?

That's why I'm still in favor of:

  fsl,upm-multi-chip-offsets = <0x200 0x400>

That would state that the address line chip select scheme is used with
the specified offsets. It also allows for a more elegant solution
(code-wise).

Wolfgang.
Grant Likely March 26, 2009, 4:04 p.m. UTC | #8
On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> Does using the reg property give the driver enough information to
>> reliably program the MAR for NAND connections that use the address
>> line chip select scheme?  Related to that, should the binding include
>
> In principle yes:
>
>  if (i > 0)
>      offset[i] = resource[i].start - resource[0].start;

Ewww.  That's ugly.

>> a property that explicitly states that an address line chip select
>> scheme is being used?
>
> That's why I'm still in favor of:
>
>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>
> That would state that the address line chip select scheme is used with
> the specified offsets. It also allows for a more elegant solution
> (code-wise).

Alright.  Then at the very least the property name should reflect that
address lines CS is used to reduce the chance of confusion with
another multi-chip scheme.  Something like
fsl,upm-addr-line-cs-offsets maybe?

Here is another thought.  The binding is describing that address lines
are used to activate CS lines.  Offset for chip access purposes is
derived from the address line, but it doesn't directly describe the
hardware.  The following may be a better description of the hardware.

fsl,upm-addr-line-cs = <9 10>;

g.
Wolfgang Grandegger March 26, 2009, 4:35 p.m. UTC | #9
Grant Likely wrote:
> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Grant Likely wrote:
>>> Does using the reg property give the driver enough information to
>>> reliably program the MAR for NAND connections that use the address
>>> line chip select scheme?  Related to that, should the binding include
>> In principle yes:
>>
>>  if (i > 0)
>>      offset[i] = resource[i].start - resource[0].start;
> 
> Ewww.  That's ugly.

Yep.

>>> a property that explicitly states that an address line chip select
>>> scheme is being used?
>> That's why I'm still in favor of:
>>
>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>
>> That would state that the address line chip select scheme is used with
>> the specified offsets. It also allows for a more elegant solution
>> (code-wise).
> 
> Alright.  Then at the very least the property name should reflect that
> address lines CS is used to reduce the chance of confusion with
> another multi-chip scheme.  Something like
> fsl,upm-addr-line-cs-offsets maybe?


> 
> Here is another thought.  The binding is describing that address lines
> are used to activate CS lines.  Offset for chip access purposes is
> derived from the address line, but it doesn't directly describe the
> hardware.  The following may be a better description of the hardware.
> 
> fsl,upm-addr-line-cs = <9 10>;

The TQM8548 hardware has some logic connected to the two address lines
allowing to select up to 4 chips with two address lines:

 fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

That's the more general solution.

Wolfgang.
Grant Likely March 26, 2009, 5:02 p.m. UTC | #10
On Thu, Mar 26, 2009 at 10:35 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Grant Likely wrote:
>> On Thu, Mar 26, 2009 at 9:33 AM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>>> Grant Likely wrote:
>>>> Does using the reg property give the driver enough information to
>>>> reliably program the MAR for NAND connections that use the address
>>>> line chip select scheme?  Related to that, should the binding include
>>> In principle yes:
>>>
>>>  if (i > 0)
>>>      offset[i] = resource[i].start - resource[0].start;
>>
>> Ewww.  That's ugly.
>
> Yep.
>
>>>> a property that explicitly states that an address line chip select
>>>> scheme is being used?
>>> That's why I'm still in favor of:
>>>
>>>  fsl,upm-multi-chip-offsets = <0x200 0x400>
>>>
>>> That would state that the address line chip select scheme is used with
>>> the specified offsets. It also allows for a more elegant solution
>>> (code-wise).
>>
>> Alright.  Then at the very least the property name should reflect that
>> address lines CS is used to reduce the chance of confusion with
>> another multi-chip scheme.  Something like
>> fsl,upm-addr-line-cs-offsets maybe?
>
>
>>
>> Here is another thought.  The binding is describing that address lines
>> are used to activate CS lines.  Offset for chip access purposes is
>> derived from the address line, but it doesn't directly describe the
>> hardware.  The following may be a better description of the hardware.
>>
>> fsl,upm-addr-line-cs = <9 10>;
>
> The TQM8548 hardware has some logic connected to the two address lines
> allowing to select up to 4 chips with two address lines:
>
>  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>

Ah.  I see.  This is board specific then.  I think it is premature to
try and define a generic solution here because it depends on custom
board hardware and different boards could use very different logic.
The next board could end up doing something completely different.  I'd
rather start to see trends in multiple boards implementing the same
scheme before trying to craft a generic scheme.

In other words, this device is not register-level compatible with the
fsl,upm-nand device.  Give the node a new compatible value
(tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
for the new device.  Use the .data element in the match table to
supply an alternate fun_cmd_ctrl() function for this board (instead of
using a property value do decide which fun_cmd_ctrl() behaviour to
use).  New boards that *do* use the same addressing scheme can claim
compatibility with tqc,tqm8548-upm-nand.

You can still use the property names already discussed, but only
document them as valid for the tqc,tqm8548-upm-nand variant of the
device.

Very little will need to change in your patch to handle it this way.

g.
Anton Vorontsov March 26, 2009, 5:33 p.m. UTC | #11
On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
[]
> >> Here is another thought.  The binding is describing that address lines
> >> are used to activate CS lines.  Offset for chip access purposes is
> >> derived from the address line, but it doesn't directly describe the
> >> hardware.  The following may be a better description of the hardware.
> >>
> >> fsl,upm-addr-line-cs = <9 10>;
> >
> > The TQM8548 hardware has some logic connected to the two address lines
> > allowing to select up to 4 chips with two address lines:
> >
> >  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
> 
> Ah.  I see.  This is board specific then.  I think it is premature to
> try and define a generic solution here because it depends on custom
> board hardware and different boards could use very different logic.
> The next board could end up doing something completely different.  I'd
> rather start to see trends in multiple boards implementing the same
> scheme before trying to craft a generic scheme.
> 
> In other words, this device is not register-level compatible with the
> fsl,upm-nand device.  Give the node a new compatible value
> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
> for the new device.  Use the .data element in the match table to
> supply an alternate fun_cmd_ctrl() function for this board (instead of
> using a property value do decide which fun_cmd_ctrl() behaviour to
> use).  New boards that *do* use the same addressing scheme can claim
> compatibility with tqc,tqm8548-upm-nand.

I don't like this. :-/

UPM is an universal thing, so there are thousands of ways we can
connect NAND to the UPM. Of which only ~10 would be sane (others are
insane, and nobody would do this. If they do, _then_ we'll fall back
to <board>-upm-nand scheme for a particular board).

I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
describe any scheme in "addr lines are cs" connection, it's a common
setup for multi-chip memory, we shouldn't treat it is as something
extraordinary.
Wolfgang Grandegger March 26, 2009, 10:14 p.m. UTC | #12
Anton Vorontsov wrote:
> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
> []
>>>> Here is another thought.  The binding is describing that address lines
>>>> are used to activate CS lines.  Offset for chip access purposes is
>>>> derived from the address line, but it doesn't directly describe the
>>>> hardware.  The following may be a better description of the hardware.
>>>>
>>>> fsl,upm-addr-line-cs = <9 10>;
>>> The TQM8548 hardware has some logic connected to the two address lines
>>> allowing to select up to 4 chips with two address lines:
>>>
>>>  fsl,upm-addr-line-cs-offsets = <0x0 0x200 0x400 0x600>
>> Ah.  I see.  This is board specific then.  I think it is premature to
>> try and define a generic solution here because it depends on custom
>> board hardware and different boards could use very different logic.
>> The next board could end up doing something completely different.  I'd
>> rather start to see trends in multiple boards implementing the same
>> scheme before trying to craft a generic scheme.
>>
>> In other words, this device is not register-level compatible with the
>> fsl,upm-nand device.  Give the node a new compatible value
>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>> for the new device.  Use the .data element in the match table to
>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>> using a property value do decide which fun_cmd_ctrl() behaviour to
>> use).  New boards that *do* use the same addressing scheme can claim
>> compatibility with tqc,tqm8548-upm-nand.
> 
> I don't like this. :-/
> 
> UPM is an universal thing, so there are thousands of ways we can
> connect NAND to the UPM. Of which only ~10 would be sane (others are
> insane, and nobody would do this. If they do, _then_ we'll fall back
> to <board>-upm-nand scheme for a particular board).

Yep.

> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
> describe any scheme in "addr lines are cs" connection, it's a common
> setup for multi-chip memory, we shouldn't treat it is as something
> extraordinary.

I fully agree. I'm going to provide a patch on monday.

Wolfgang.
Grant Likely March 26, 2009, 11:22 p.m. UTC | #13
On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
> Anton Vorontsov wrote:
>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>> In other words, this device is not register-level compatible with the
>>> fsl,upm-nand device.  Give the node a new compatible value
>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>> for the new device.  Use the .data element in the match table to
>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>> use).  New boards that *do* use the same addressing scheme can claim
>>> compatibility with tqc,tqm8548-upm-nand.
>>
>> I don't like this. :-/
>>
>> UPM is an universal thing, so there are thousands of ways we can
>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>> insane, and nobody would do this. If they do, _then_ we'll fall back
>> to <board>-upm-nand scheme for a particular board).
>
> Yep.
>
>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>> describe any scheme in "addr lines are cs" connection, it's a common
>> setup for multi-chip memory, we shouldn't treat it is as something
>> extraordinary.
>
> I fully agree. I'm going to provide a patch on monday.

Well, I still don't think it is the wisest choice.  My position is
that it is better to be conservative and pedantic now because it is
easy to relax the rules from that point.  If it turns out after some
experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
serious flaw, then the impact is contained.  On the other side, if it
is confirmed and useful and correct, it is a trivial change to make it
available to everything that claims compatibility with fsl,upm-nand.

That said, I won't oppose it if you go this route.  However at the
very least, please change the nand node's compatible list to be:

compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";

The custom glue logic makes it something unique, so "tqc,..." should
be at the start of the list to describe it as such, even if the driver
only ever uses "fsl,upm-nand".

g.
Anton Vorontsov March 26, 2009, 11:32 p.m. UTC | #14
On Thu, Mar 26, 2009 at 05:22:36PM -0600, Grant Likely wrote:
[...]
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";

Yeah, that's definitely a good idea.
Wolfgang Grandegger March 27, 2009, 8:07 a.m. UTC | #15
Grant Likely wrote:
> On Thu, Mar 26, 2009 at 4:14 PM, Wolfgang Grandegger <wg@grandegger.com> wrote:
>> Anton Vorontsov wrote:
>>> On Thu, Mar 26, 2009 at 11:02:06AM -0600, Grant Likely wrote:
>>>> In other words, this device is not register-level compatible with the
>>>> fsl,upm-nand device.  Give the node a new compatible value
>>>> (tqc,tqm8548-upm-nand) and add another entry to the of_fun_match table
>>>> for the new device.  Use the .data element in the match table to
>>>> supply an alternate fun_cmd_ctrl() function for this board (instead of
>>>> using a property value do decide which fun_cmd_ctrl() behaviour to
>>>> use).  New boards that *do* use the same addressing scheme can claim
>>>> compatibility with tqc,tqm8548-upm-nand.
>>> I don't like this. :-/
>>>
>>> UPM is an universal thing, so there are thousands of ways we can
>>> connect NAND to the UPM. Of which only ~10 would be sane (others are
>>> insane, and nobody would do this. If they do, _then_ we'll fall back
>>> to <board>-upm-nand scheme for a particular board).
>> Yep.
>>
>>> I don't see any problem with fsl,upm-addr-line-cs-offsets. It can
>>> describe any scheme in "addr lines are cs" connection, it's a common
>>> setup for multi-chip memory, we shouldn't treat it is as something
>>> extraordinary.
>> I fully agree. I'm going to provide a patch on monday.
> 
> Well, I still don't think it is the wisest choice.  My position is
> that it is better to be conservative and pedantic now because it is
> easy to relax the rules from that point.  If it turns out after some
> experience with "fsl,upm-addr-line-cs-offset" that the scheme has a
> serious flaw, then the impact is contained.  On the other side, if it
> is confirmed and useful and correct, it is a trivial change to make it
> available to everything that claims compatibility with fsl,upm-nand.
> 
> That said, I won't oppose it if you go this route.  However at the
> very least, please change the nand node's compatible list to be:
> 
> compatible = "tqc,tqm8548-upm-nand", "fsl,upm-nand";
> 
> The custom glue logic makes it something unique, so "tqc,..." should
> be at the start of the list to describe it as such, even if the driver
> only ever uses "fsl,upm-nand".

That's a good idea in case we need it lateron. For the time being, I
prefer to make the driver as generic as possible. Currently there are
only two boards using the driver, the MPC8360RTDK and the TQM8548. and
it's unlikely that there will be much more in the future.

Wolfgang.
diff mbox

Patch

diff --git a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
index 84a04d5..0272e70 100644
--- a/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
+++ b/Documentation/powerpc/dts-bindings/fsl/upm-nand.txt
@@ -5,9 +5,22 @@  Required properties:
 - reg : should specify localbus chip select and size used for the chip.
 - fsl,upm-addr-offset : UPM pattern offset for the address latch.
 - fsl,upm-cmd-offset : UPM pattern offset for the command latch.
-- gpios : may specify optional GPIO connected to the Ready-Not-Busy pin.
 
-Example:
+Optional properties:
+- fsl,upm-wait-flags : add chip-dependent short delays after running the
+  		       UPM pattern (0x1), after writing a data byte (0x2)
+		       or after writing out a buffer (0x4).
+- gpios : may specify optional GPIOs connected to the Ready-Not-Busy pins
+	  (R/B#). For multi-chip devices, "num-chips" GPIO definitions are
+	  required.
+- chip-delay : chip dependent delay for transfering data from array to
+	       read registers (tR). Required if property "gpios" is not
+	       used (R/B# pins not connected).
+- num-chips : number of chips per device for multi-chip support.
+- chip-offset : address offset between chips for multi-chip support. The
+ 		corresponding address lines are used to select the chip.
+
+Examples:
 
 upm@1,0 {
 	compatible = "fsl,upm-nand";
@@ -26,3 +39,25 @@  upm@1,0 {
 		};
 	};
 };
+
+upm@3,0 {
+	compatible = "fsl,upm-nand";
+	reg = <3 0x0 0x800>;
+	fsl,upm-addr-offset = <0x10>;
+	fsl,upm-cmd-offset = <0x08>;
+	fsl,upm-wait-flags = <0x5>;
+	/* Multi-chip device */
+	num-chips = <2>;
+	chip-offset = <0x200>;
+	chip-delay = <25>; // in micro-seconds
+
+	nand@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			    label = "fs";
+			    reg = <0x00000000 0x10000000>;
+		};
+	};
+};