| Message ID | 1237975701-23201-4-git-send-email-wg@grandegger.com |
|---|---|
| State | New, archived |
| Headers | show |
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
(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 >
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.
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.
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. >
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.
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.
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.
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.
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.
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.
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.
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.
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.
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 --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>; + }; + }; +};
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(-)