* [PATCH 1/4] of: Add device tree bindings for Evatronix @ 2016-06-02 7:47 Ricard Wanderlof 2016-06-03 14:22 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Ricard Wanderlof @ 2016-06-02 7:47 UTC (permalink / raw) To: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, David Woodhouse Cc: Linux mtd, devicetree, linux-kernel Devicetree bindings for the driver for the Evatronix NANDFLASH-CTRL NAND flash controller IP. This controller is used in the Axis ARTPEC-6 SoC. The driver supports BCH ECC using the controller's hardware, but there is also an option to use software BCH ECC. However, the ECC layouts are not compatible so it's not possible to mix them. The main advantage to using software ECC is that there are more OOB bytes free, as the hardware is slightly wasteful on OOB space. BCH ECC from 4 to 32 bits over 256, 512 or 1024 byte ECC blocks is supported. Only large-page flash chips are supported, using 4 or 5 address cycles. Signed-off-by: Ricard Wanderlof <ricardw@axis.com> --- .../devicetree/bindings/mtd/evatronix-nand.txt | 44 ++++++++++++++++++++ .../devicetree/bindings/vendor-prefixes.txt | 1 + 2 files changed, 45 insertions(+) create mode 100644 Documentation/devicetree/bindings/mtd/evatronix-nand.txt diff --git a/Documentation/devicetree/bindings/mtd/evatronix-nand.txt b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt new file mode 100644 index 0000000..7ceb95a --- /dev/null +++ b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt @@ -0,0 +1,44 @@ +Evatronix NANDFLASH-CTRL NAND flash controller + +Required properties: +- compatible : "evatronix,nandflash-ctrl" +- reg : specify bus address and register area size. +- interrupts : controller interrupt number and irq type. +- nand-ecc-mode : See nand.txt. Supported values "hw", "soft". +- nand-ecc-algo : See nand.txt. Supported value "bch". +- nand-ecc-strength : See nand.txt. Supported values: 4, 8, 16, 24, 32. +- nand-ecc-step-size : See nand.txt. Supported values: 256, 512, 1024. + +Optional properties: +- nand-on-flash-bbt: See nand.txt. +- #address-cells, #size-cells: See partition.txt. +- evatronix,use-bank-select : Use controller bank select function to access + multiple chips, rather than chip enable. +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are + connected using a wired-AND topology rather than + individually. +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. + +Example: + +nand: nand@f801e000 { + compatible = "evatronix,nandflash-ctrl"; + #address-cells = <1>; + #size-cells = <1>; + reg = <0xf801e000 0x0200>; + interrupts = <0 139 IRQ_TYPE_LEVEL_HIGH>; + /* ONFi mode 0 timing, assuming 100 MHz clock. */ + /* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN, + * TIME_GEN_SEQ_0, _1, _2, _3 */ + evatronix,timings = <0x0d151533 0x000b0515 0x00000046 + 0x00150000 0x00000000 0x00000005 0x00000015>; + nand-ecc-mode = "hw"; + nand-ecc-algo = "bch"; + nand-on-flash-bbt; + nand-ecc-strength = <8>; + nand-ecc-step-size = <512>; + evatronix,use-bank-select; + evatronix,rb-wired-and; +}; diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt index 86740d4..4018162 100644 --- a/Documentation/devicetree/bindings/vendor-prefixes.txt +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt @@ -83,6 +83,7 @@ epson Seiko Epson Corp. est ESTeem Wireless Modems ettus NI Ettus Research eukrea Eukréa Electromatique +evatronix Evatronix SA everest Everest Semiconductor Co. Ltd. everspin Everspin Technologies, Inc. excito Excito -- 1.7.10.4 -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-02 7:47 [PATCH 1/4] of: Add device tree bindings for Evatronix Ricard Wanderlof @ 2016-06-03 14:22 ` Boris Brezillon 2016-06-07 15:01 ` Ricard Wanderlof 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2016-06-03 14:22 UTC (permalink / raw) To: Ricard Wanderlof Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel Hi Ricard, On Thu, 2 Jun 2016 09:47:18 +0200 Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > Devicetree bindings for the driver for the Evatronix NANDFLASH-CTRL NAND flash > controller IP. This controller is used in the Axis ARTPEC-6 SoC. > > The driver supports BCH ECC using the controller's hardware, but there is > also an option to use software BCH ECC. However, the ECC layouts are not > compatible so it's not possible to mix them. The main advantage to using > software ECC is that there are more OOB bytes free, as the hardware is > slightly wasteful on OOB space. > > BCH ECC from 4 to 32 bits over 256, 512 or 1024 byte ECC blocks is supported. > > Only large-page flash chips are supported, using 4 or 5 address cycles. > > Signed-off-by: Ricard Wanderlof <ricardw@axis.com> > --- > .../devicetree/bindings/mtd/evatronix-nand.txt | 44 ++++++++++++++++++++ > .../devicetree/bindings/vendor-prefixes.txt | 1 + > 2 files changed, 45 insertions(+) > create mode 100644 Documentation/devicetree/bindings/mtd/evatronix-nand.txt > > diff --git a/Documentation/devicetree/bindings/mtd/evatronix-nand.txt b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt > new file mode 100644 > index 0000000..7ceb95a > --- /dev/null > +++ b/Documentation/devicetree/bindings/mtd/evatronix-nand.txt > @@ -0,0 +1,44 @@ > +Evatronix NANDFLASH-CTRL NAND flash controller > + > +Required properties: > +- compatible : "evatronix,nandflash-ctrl" > +- reg : specify bus address and register area size. > +- interrupts : controller interrupt number and irq type. > +- nand-ecc-mode : See nand.txt. Supported values "hw", "soft". > +- nand-ecc-algo : See nand.txt. Supported value "bch". > +- nand-ecc-strength : See nand.txt. Supported values: 4, 8, 16, 24, 32. > +- nand-ecc-step-size : See nand.txt. Supported values: 256, 512, 1024. > + > +Optional properties: > +- nand-on-flash-bbt: See nand.txt. > +- #address-cells, #size-cells: See partition.txt. > +- evatronix,use-bank-select : Use controller bank select function to access > + multiple chips, rather than chip enable. You mean, using a dedicated logic to control the CS lines rather than a GPIO (controlled by the SW using gpio_set_value())? It that's the case then I suggest doing it the other way around. When you want to use a plain GPIO you should define something like: cs-gpios = <&pioC X>; If it's not there the controller should use it's internal logic to control the CS line. BTW, you seem to support controlling several CS using the same controller, which means you'll have to specify which CS the NAND chip is connected to (see [1]). > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are > + connected using a wired-AND topology rather than > + individually. Hm, is that really required? If the R/B line is shared among several NAND chips, it should be transparent, you just have to specific which chip is connected to which GPIO (or dedicated R/B pin). > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, > + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, > + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. Can this be extracted from the timing mode exposed by the NAND chip. IMO it shouldn't be defined in the DT. > + > +Example: > + > +nand: nand@f801e000 { > + compatible = "evatronix,nandflash-ctrl"; > + #address-cells = <1>; > + #size-cells = <1>; > + reg = <0xf801e000 0x0200>; > + interrupts = <0 139 IRQ_TYPE_LEVEL_HIGH>; > + /* ONFi mode 0 timing, assuming 100 MHz clock. */ > + /* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN, > + * TIME_GEN_SEQ_0, _1, _2, _3 */ > + evatronix,timings = <0x0d151533 0x000b0515 0x00000046 > + 0x00150000 0x00000000 0x00000005 0x00000015>; > + nand-ecc-mode = "hw"; > + nand-ecc-algo = "bch"; > + nand-on-flash-bbt; > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; > + evatronix,use-bank-select; > + evatronix,rb-wired-and; > +}; We recently added more constraints on the 'NAND controller/NAND chip' representation in the DT [1]. You should rework your binding (and your code) to match these constraints, even if you controller is only able to interface with a single NAND chip. Thanks, Boris [1]https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/nand.txt > diff --git a/Documentation/devicetree/bindings/vendor-prefixes.txt b/Documentation/devicetree/bindings/vendor-prefixes.txt > index 86740d4..4018162 100644 > --- a/Documentation/devicetree/bindings/vendor-prefixes.txt > +++ b/Documentation/devicetree/bindings/vendor-prefixes.txt > @@ -83,6 +83,7 @@ epson Seiko Epson Corp. > est ESTeem Wireless Modems > ettus NI Ettus Research > eukrea Eukréa Electromatique > +evatronix Evatronix SA > everest Everest Semiconductor Co. Ltd. > everspin Everspin Technologies, Inc. > excito Excito -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-03 14:22 ` Boris Brezillon @ 2016-06-07 15:01 ` Ricard Wanderlof 2016-06-08 15:50 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Ricard Wanderlof @ 2016-06-07 15:01 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel Hi Boris, First of all, thanks for reviewing this. On Fri, 3 Jun 2016, Boris Brezillon wrote: > > + > > +Optional properties: > > +- nand-on-flash-bbt: See nand.txt. > > +- #address-cells, #size-cells: See partition.txt. > > +- evatronix,use-bank-select : Use controller bank select function to access > > + multiple chips, rather than chip enable. > > You mean, using a dedicated logic to control the CS lines rather than a > GPIO (controlled by the SW using gpio_set_value())? No. In the SoC on which this has been developed, the nand flash controller has dedicated I/O pins for all its functions. The controller has the concept of 'banks' vs. 'devices'. Essentially, a 'bank' is a group of multiple chips of the same geometry etc, and the controller can send commands in parallel to all devices in the same bank. In contrast, different 'banks' can have completely different devices, but all devices must be idle before the controller can switch 'banks'. The usecase for the former would be to increase performance and the latter case would be if there were for instance one small NAND flash for program storage and a large one for data. The IP that the driver was tested on has two available chip selects, and the DT property controls whether they refer to two devices in the same bank vs. two separate banks. > BTW, you seem to support controlling several CS using the same > controller, which means you'll have to specify which CS the NAND chip > is connected to (see [1]). That is true. The driver as it stands currently only supports a single CS, however, the IP as configured in the SoC in question can handle two CS lines. My plan is to initially provide support for only the single-CS case (there are places in the code where I felt it practical to prepare for the multiple-CS case though). As such, I realize that the previously mentioned property (evatronix,use-bank-select) won't really have any meaning in the single-CS case, so perhaps it should be omitted at this point completely, and added in the future when it is really needed? > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are > > + connected using a wired-AND topology rather than > > + individually. > > Hm, is that really required? If the R/B line is shared among several > NAND chips, it should be transparent, you just have to specific which > chip is connected to which GPIO (or dedicated R/B pin). For the SoC in question, the wired-and vs individual R/B lines was a choice made during the chip design, which is why I exported this choice as a DT property, as it reflects a hardware choice done outside the IP but inside the SoC. I didn't really want to have the added flexibility (and complexity) of being able to use any R/B line for any connected flash chip. It seems an unnecessary complication for the driver without much gain. But it would certainly be doable, as the R/B stuff is handled completely in software. One would still need a new property that the child nand nodes can use to select the R/B line (e.g. evatronix,rb-line). (In the configuration we have, as with the CS lines, the R/B line has its own dedicated pin, so it's not a GPIO). So it would still require a DT property to manage it. Again, this property doesn't really have any meaning in the single-CS case, so should I omit it completely? > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, > > + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, > > + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. > > > Can this be extracted from the timing mode exposed by the NAND chip. > IMO it shouldn't be defined in the DT. I agree, Ideally one would like to read timing parameters from the NAND chip (ONFi or JEDEC parameter pages) and determine all the controller IP timings from that. There are a couple of reasons I haven't gone that route. Mainly, the reason is that the part of the timing settings are influenced by delays within the SoC which are not part of the controller itself, thus they cannot be part of the driver code and must come from somewhere else. The other reason is that the resulting calculations would be rather complex to express, given the general case of different controller operating frequencies, different types of parameter pages (ONFi vs. JEDEC), and a number of settable timing modes (ONFi mode 0, 1, 2, ...). It would be a nightmare to test and probably even worse to make future changes. At some point, the resulting register values will have to be analyzed manually anyway to verify them, checking them against the timings for the nand flash chips in question, so one might as well put those values into the configuration to start with. One route to go would be to have a number of 'timings' parameters in the .dtsi file for the SoC in question, where the register values are defined for different timing modes. The board (.dts) file could then either select one of the .dtsi-defined timing modes, or it could be selected automatically for those cases where it would be possible (i.e. ONFi timing modes). > > + /* ONFi mode 0 timing, assuming 100 MHz clock. */ > > + /* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN, > > + * TIME_GEN_SEQ_0, _1, _2, _3 */ > > + evatronix,timings = <0x0d151533 0x000b0515 0x00000046 > > + 0x00150000 0x00000000 0x00000005 0x00000015>; > > + nand-ecc-mode = "hw"; > > + nand-ecc-algo = "bch"; > > + nand-on-flash-bbt; > > + nand-ecc-strength = <8>; > > + nand-ecc-step-size = <512>; > > + evatronix,use-bank-select; > > + evatronix,rb-wired-and; > > +}; > > We recently added more constraints on the 'NAND controller/NAND chip' > representation in the DT [1]. > You should rework your binding (and your code) to match these > constraints, even if you controller is only able to interface with a > single NAND chip. Just so I'm clear on this: what you're referring to here is that the generic mtd properties (nand-*) are now per chip rather than per controller? I will have to rework this. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-07 15:01 ` Ricard Wanderlof @ 2016-06-08 15:50 ` Boris Brezillon 2016-06-10 15:35 ` Ricard Wanderlof 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2016-06-08 15:50 UTC (permalink / raw) To: Ricard Wanderlof Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Tue, 7 Jun 2016 17:01:41 +0200 Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > Hi Boris, > > First of all, thanks for reviewing this. > > On Fri, 3 Jun 2016, Boris Brezillon wrote: > > > > + > > > +Optional properties: > > > +- nand-on-flash-bbt: See nand.txt. > > > +- #address-cells, #size-cells: See partition.txt. > > > +- evatronix,use-bank-select : Use controller bank select function to access > > > + multiple chips, rather than chip enable. > > > > You mean, using a dedicated logic to control the CS lines rather than a > > GPIO (controlled by the SW using gpio_set_value())? > > No. In the SoC on which this has been developed, the nand flash controller > has dedicated I/O pins for all its functions. > > The controller has the concept of 'banks' vs. 'devices'. Essentially, a > 'bank' is a group of multiple chips of the same geometry etc, and the > controller can send commands in parallel to all devices in the same bank. > In contrast, different 'banks' can have completely different devices, but > all devices must be idle before the controller can switch 'banks'. > > The usecase for the former would be to increase performance and the latter > case would be if there were for instance one small NAND flash for program > storage and a large one for data. > > The IP that the driver was tested on has two available chip selects, and > the DT property controls whether they refer to two devices in the same > bank vs. two separate banks. Still don't see how it can be used in real world (or maybe I don't understand how it works). Say the CS are both selected at the same time, this means you'll send the same set of data to the 2 chips, right. I see two use cases for this: 1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE lines and you want to mirror data written on chip0 on chip1 2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but chip0 is using the lower 8bits of the bus, and chip1 is using the remaining 8bits. This way you fake an 16bits bus Both cases are unsafe, because of NAND unreliability. Say you're writing on a block in chip0 this block in chip1 is bad. One of your write will succeed, the other one will fail. Could you give a real use case where this 'bank-select' method is really usefull (and safe)? If you can't, then drop this mode for now, and always operate in 'chip-enable' mode. > > > BTW, you seem to support controlling several CS using the same > > controller, which means you'll have to specify which CS the NAND chip > > is connected to (see [1]). > > That is true. The driver as it stands currently only supports a single CS, > however, the IP as configured in the SoC in question can handle two CS > lines. My plan is to initially provide support for only the single-CS > case (there are places in the code where I felt it practical to prepare > for the multiple-CS case though). > > As such, I realize that the previously mentioned property > (evatronix,use-bank-select) won't really have any meaning in the single-CS > case, so perhaps it should be omitted at this point completely, and added > in the future when it is really needed? Yes, but I'd still like to see a proper NAND controller/NAND chip separation in you binding. > > > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are > > > + connected using a wired-AND topology rather than > > > + individually. > > > > Hm, is that really required? If the R/B line is shared among several > > NAND chips, it should be transparent, you just have to specific which > > chip is connected to which GPIO (or dedicated R/B pin). > > For the SoC in question, the wired-and vs individual R/B lines was a > choice made during the chip design, which is why I exported this choice as > a DT property, as it reflects a hardware choice done outside the IP but > inside the SoC. Okay, maybe it's worth keeping this property if the AND logic is integrated in the SoC. > > I didn't really want to have the added flexibility (and complexity) of > being able to use any R/B line for any connected flash chip. It seems an > unnecessary complication for the driver without much gain. Not really, at least if you properly separate the chip and controller objects it's quite easy to deal with, and I'll ask you to do this clean separation anyway (even if you say you only have a single chip per controller) :P. > > But it would certainly be doable, as the R/B stuff is handled completely > in software. One would still need a new property that the child nand nodes > can use to select the R/B line (e.g. evatronix,rb-line). (In the > configuration we have, as with the CS lines, the R/B line has its own > dedicated pin, so it's not a GPIO). So it would still require a DT > property to manage it. Yep. The sunxi_nand driver is using allwinner,rb = <X>. > > Again, this property doesn't really have any meaning in the single-CS > case, so should I omit it completely? Keep in mind that you're defining a DT binding, and this binding is submitting to the 'stable ABI' rule, so you shouldn't think in term of what your driver supports now, but what you're IP is capable of, and it's definitely capable of handling several R/B pins. > > > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, > > > + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, > > > + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. > > > > > > Can this be extracted from the timing mode exposed by the NAND chip. > > IMO it shouldn't be defined in the DT. > > I agree, Ideally one would like to read timing parameters from the NAND > chip (ONFi or JEDEC parameter pages) and determine all the controller IP > timings from that. There are a couple of reasons I haven't gone that > route. > > Mainly, the reason is that the part of the timing settings are influenced > by delays within the SoC which are not part of the controller itself, thus > they cannot be part of the driver code and must come from somewhere else. Then you should have a way to retrieve these informations (proper clk drivers, specific bus drivers, or any other means). > > The other reason is that the resulting calculations would be rather > complex to express, given the general case of different controller > operating frequencies, different types of parameter pages (ONFi vs. > JEDEC), and a number of settable timing modes (ONFi mode 0, 1, 2, ...). It > would be a nightmare to test and probably even worse to make future > changes. At some point, the resulting register values will have to be > analyzed manually anyway to verify them, checking them against the timings > for the nand flash chips in question, so one might as well put those > values into the configuration to start with. And my answer is no :). Just because it's complicated to extract these information at the driver level level doesn't mean you should put it in the DT. That's exactly the opposite: the DT is supposed to encode the hardware representation, not how you want to configure it. > > One route to go would be to have a number of 'timings' parameters in the > .dtsi file for the SoC in question, where the register values are defined > for different timing modes. The board (.dts) file could then either select > one of the .dtsi-defined timing modes, or it could be selected > automatically for those cases where it would be possible (i.e. ONFi timing > modes). Nope, they should not be defined in the DT at all. How about using a default/safe timing mode for now (ONFI timing mode 0 should work for all NANDs). The only thing you'll have to do is retrieve the source clk rate and calculate timing register values accordingly. Or you can even assume you always have a 100MHz source clk and hardcode it in your driver. > > > > + /* ONFi mode 0 timing, assuming 100 MHz clock. */ > > > + /* Order is TIME_SEQ_0, TIME_SEQ_1, TIMINGS_ASYN, > > > + * TIME_GEN_SEQ_0, _1, _2, _3 */ > > > + evatronix,timings = <0x0d151533 0x000b0515 0x00000046 > > > + 0x00150000 0x00000000 0x00000005 0x00000015>; > > > + nand-ecc-mode = "hw"; > > > + nand-ecc-algo = "bch"; > > > + nand-on-flash-bbt; > > > + nand-ecc-strength = <8>; > > > + nand-ecc-step-size = <512>; > > > + evatronix,use-bank-select; > > > + evatronix,rb-wired-and; > > > +}; > > > > We recently added more constraints on the 'NAND controller/NAND chip' > > representation in the DT [1]. > > You should rework your binding (and your code) to match these > > constraints, even if you controller is only able to interface with a > > single NAND chip. > > Just so I'm clear on this: what you're referring to here is that the > generic mtd properties (nand-*) are now per chip rather than per > controller? I will have to rework this. Yes, and your NAND chip should be a sub-node of the NAND controller node (and not mixed together as you've done here). -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-08 15:50 ` Boris Brezillon @ 2016-06-10 15:35 ` Ricard Wanderlof 2016-06-10 15:54 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Ricard Wanderlof @ 2016-06-10 15:35 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Wed, 8 Jun 2016, Boris Brezillon wrote: > > > > +Optional properties: > > > > +- nand-on-flash-bbt: See nand.txt. > > > > +- #address-cells, #size-cells: See partition.txt. > > > > +- evatronix,use-bank-select : Use controller bank select function to access > > > > + multiple chips, rather than chip enable. > > > > > > You mean, using a dedicated logic to control the CS lines rather than a > > > GPIO (controlled by the SW using gpio_set_value())? > > > > No. In the SoC on which this has been developed, the nand flash controller > > has dedicated I/O pins for all its functions. > > > > The controller has the concept of 'banks' vs. 'devices'. Essentially, a > > 'bank' is a group of multiple chips of the same geometry etc, and the > > controller can send commands in parallel to all devices in the same bank. > > In contrast, different 'banks' can have completely different devices, but > > all devices must be idle before the controller can switch 'banks'. > > > > The usecase for the former would be to increase performance and the latter > > case would be if there were for instance one small NAND flash for program > > storage and a large one for data. > > > > The IP that the driver was tested on has two available chip selects, and > > the DT property controls whether they refer to two devices in the same > > bank vs. two separate banks. > > Still don't see how it can be used in real world (or maybe I don't > understand how it works). > > Say the CS are both selected at the same time, this means you'll send > the same set of data to the 2 chips, right. I see two use cases for > this: > 1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE > lines and you want to mirror data written on chip0 on chip1 > 2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but > chip0 is using the lower 8bits of the bus, and chip1 is using the > remaining 8bits. This way you fake an 16bits bus > > Both cases are unsafe, because of NAND unreliability. Say you're > writing on a block in chip0 this block in chip1 is bad. One of your > write will succeed, the other one will fail. > > Could you give a real use case where this 'bank-select' method is > really usefull (and safe)? > If you can't, then drop this mode for now, and always operate in > 'chip-enable' mode. The use cases as I see are as follows: a) Two identical chips sharing all but the CS lines, in order to implement a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a 4 GB area). In this case, for certain operations, the controller does not have to wait for one device to complete before issuing a command to another. I'm not sure how the controller keeps track of the two devices though. b) Two different chips with the same connection, which provide disjunct functions, e.g. one (small) flash for program storage and one (large) for data. But your questions have convinced me to leave this part of the driver for now, and as you say it can be added in when the need arises. > > > BTW, you seem to support controlling several CS using the same > > > controller, which means you'll have to specify which CS the NAND chip > > > is connected to (see [1]). > > > > That is true. The driver as it stands currently only supports a single CS, > > however, the IP as configured in the SoC in question can handle two CS > > lines. My plan is to initially provide support for only the single-CS > > case (there are places in the code where I felt it practical to prepare > > for the multiple-CS case though). > > > > As such, I realize that the previously mentioned property > > (evatronix,use-bank-select) won't really have any meaning in the single-CS > > case, so perhaps it should be omitted at this point completely, and added > > in the future when it is really needed? > > Yes, but I'd still like to see a proper NAND controller/NAND chip > separation in you binding. Yes, I understand that. > > > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are > > > > + connected using a wired-AND topology rather than > > > > + individually. > > > > > > Hm, is that really required? If the R/B line is shared among several > > > NAND chips, it should be transparent, you just have to specific which > > > chip is connected to which GPIO (or dedicated R/B pin). > > > > For the SoC in question, the wired-and vs individual R/B lines was a > > choice made during the chip design, which is why I exported this choice as > > a DT property, as it reflects a hardware choice done outside the IP but > > inside the SoC. > > Okay, maybe it's worth keeping this property if the AND logic is > integrated in the SoC. Ok. > > I didn't really want to have the added flexibility (and complexity) of > > being able to use any R/B line for any connected flash chip. It seems an > > unnecessary complication for the driver without much gain. > > Not really, at least if you properly separate the chip and controller > objects it's quite easy to deal with, and I'll ask you to do this clean > separation anyway (even if you say you only have a single chip per > controller) :P. Yes, the separation of chip and controller is needed, but the R/B line flexibility requires an additional mapping functionality within the driver. Not rocket science of course, but I can't see much point in it (other than to cover up a potential routing mistake done by a PCB designer). > > > > Again, this property doesn't really have any meaning in the single-CS > > case, so should I omit it completely? > > Keep in mind that you're defining a DT binding, and this binding is > submitting to the 'stable ABI' rule, so you shouldn't think in term of > what your driver supports now, but what you're IP is capable of, and > it's definitely capable of handling several R/B pins. Yes, I understand about the stable API rule, but at the same time that R/B mapping could be added without violating that rule at a later date if someone really needed it, couldn't it? I mean, an IP may be capable of a lot of things, and we don't necessarily want to implement them all in the driver to start with and have DT propertes for them all do we? > > > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, > > > > + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, > > > > + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. > > > > > > > > > Can this be extracted from the timing mode exposed by the NAND chip. > > > IMO it shouldn't be defined in the DT. > > > > I agree, Ideally one would like to read timing parameters from the NAND > > chip (ONFi or JEDEC parameter pages) and determine all the controller IP > > timings from that. There are a couple of reasons I haven't gone that > > route. > > I think what I'm worried about is that say we add the functionality to take the appropriate parameters and create a set of timing values. We can verify those values only under a very limited set of circumstances (a certain clock frequency, certain pad delays on a given SoC, limited range of flash chips etc). At a later date, the IP is used in a different SoC and the original calculation turns out to be wrong in that case. Updating the timing calcuation function to work with the original SoC and the new one would be a bit of a nightmare I. Since at some point a set of specific timing values need to be calculated for a given SoC anyway, we might as well use those values as they are. > And my answer is no :). :-) > Just because it's complicated to extract these information at the driver > level level doesn't mean you should put it in the DT. That's exactly the > opposite: the DT is supposed to encode the hardware representation, not > how you want to configure it. I would say that the complexity and error-proneness of automatic timing calculation outweighs the benefit, but I see I'm fighting a loosing battle here... > How about using a default/safe timing mode for now (ONFI timing mode 0 > should work for all NANDs). > The only thing you'll have to do is retrieve the source clk rate and > calculate timing register values accordingly. Or you can even assume > you always have a 100MHz source clk and hardcode it in your driver. Yes, that is certainly possible of course, and the driver already has a hard-coded default setup for this case. In that case though the driver could have pre-set setups for other ONFi modes, and we could have an _optional_ DT property to select them, the reason for that being in order to handle non-ONFi flashes whose timing cannot be gleaned from the device itself. I.e. something like evatronix,onfi-timing-mode = <2>; Would that be acceptable? /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-10 15:35 ` Ricard Wanderlof @ 2016-06-10 15:54 ` Boris Brezillon 2016-06-10 16:46 ` Ricard Wanderlof 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2016-06-10 15:54 UTC (permalink / raw) To: Ricard Wanderlof Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Fri, 10 Jun 2016 17:35:24 +0200 Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > On Wed, 8 Jun 2016, Boris Brezillon wrote: > > > > > > +Optional properties: > > > > > +- nand-on-flash-bbt: See nand.txt. > > > > > +- #address-cells, #size-cells: See partition.txt. > > > > > +- evatronix,use-bank-select : Use controller bank select function to access > > > > > + multiple chips, rather than chip enable. > > > > > > > > You mean, using a dedicated logic to control the CS lines rather than a > > > > GPIO (controlled by the SW using gpio_set_value())? > > > > > > No. In the SoC on which this has been developed, the nand flash controller > > > has dedicated I/O pins for all its functions. > > > > > > The controller has the concept of 'banks' vs. 'devices'. Essentially, a > > > 'bank' is a group of multiple chips of the same geometry etc, and the > > > controller can send commands in parallel to all devices in the same bank. > > > In contrast, different 'banks' can have completely different devices, but > > > all devices must be idle before the controller can switch 'banks'. > > > > > > The usecase for the former would be to increase performance and the latter > > > case would be if there were for instance one small NAND flash for program > > > storage and a large one for data. > > > > > > The IP that the driver was tested on has two available chip selects, and > > > the DT property controls whether they refer to two devices in the same > > > bank vs. two separate banks. > > > > Still don't see how it can be used in real world (or maybe I don't > > understand how it works). > > > > Say the CS are both selected at the same time, this means you'll send > > the same set of data to the 2 chips, right. I see two use cases for > > this: > > 1/ You have 2 chips connected to the same data bus, CS, RB and CLE/ALE > > lines and you want to mirror data written on chip0 on chip1 > > 2/ You have 2 chips connected to the same CS, RB, CLE/ALE lines, but > > chip0 is using the lower 8bits of the bus, and chip1 is using the > > remaining 8bits. This way you fake an 16bits bus > > > > Both cases are unsafe, because of NAND unreliability. Say you're > > writing on a block in chip0 this block in chip1 is bad. One of your > > write will succeed, the other one will fail. > > > > Could you give a real use case where this 'bank-select' method is > > really usefull (and safe)? > > If you can't, then drop this mode for now, and always operate in > > 'chip-enable' mode. > > The use cases as I see are as follows: > > a) Two identical chips sharing all but the CS lines, in order to implement > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a > 4 GB area). In this case, for certain operations, the controller does not > have to wait for one device to complete before issuing a command to > another. I'm not sure how the controller keeps track of the two devices > though. I think it's the chip-enable use case, isn't it? > > b) Two different chips with the same connection, which provide disjunct > functions, e.g. one (small) flash for program storage and one (large) for > data. Then they can't share the CS line (or be actived at the same time), at least it doesn't make any sense to me. > > But your questions have convinced me to leave this part of the driver for > now, and as you say it can be added in when the need arises. > > > > > BTW, you seem to support controlling several CS using the same > > > > controller, which means you'll have to specify which CS the NAND chip > > > > is connected to (see [1]). > > > > > > That is true. The driver as it stands currently only supports a single CS, > > > however, the IP as configured in the SoC in question can handle two CS > > > lines. My plan is to initially provide support for only the single-CS > > > case (there are places in the code where I felt it practical to prepare > > > for the multiple-CS case though). > > > > > > As such, I realize that the previously mentioned property > > > (evatronix,use-bank-select) won't really have any meaning in the single-CS > > > case, so perhaps it should be omitted at this point completely, and added > > > in the future when it is really needed? > > > > Yes, but I'd still like to see a proper NAND controller/NAND chip > > separation in you binding. > > Yes, I understand that. > > > > > > +- evatronix,rb-wired-and: Assume ready/busy signal from all flash chips are > > > > > + connected using a wired-AND topology rather than > > > > > + individually. > > > > > > > > Hm, is that really required? If the R/B line is shared among several > > > > NAND chips, it should be transparent, you just have to specific which > > > > chip is connected to which GPIO (or dedicated R/B pin). > > > > > > For the SoC in question, the wired-and vs individual R/B lines was a > > > choice made during the chip design, which is why I exported this choice as > > > a DT property, as it reflects a hardware choice done outside the IP but > > > inside the SoC. > > > > Okay, maybe it's worth keeping this property if the AND logic is > > integrated in the SoC. > > Ok. > > > > I didn't really want to have the added flexibility (and complexity) of > > > being able to use any R/B line for any connected flash chip. It seems an > > > unnecessary complication for the driver without much gain. > > > > Not really, at least if you properly separate the chip and controller > > objects it's quite easy to deal with, and I'll ask you to do this clean > > separation anyway (even if you say you only have a single chip per > > controller) :P. > > Yes, the separation of chip and controller is needed, but the R/B line > flexibility requires an additional mapping functionality within the > driver. Not rocket science of course, but I can't see much point in it > (other than to cover up a potential routing mistake done by a PCB > designer). You seem to argue on all the minor things I'm asking. Honestly, it should be hard or error-prone to do it. And let's say you don't support it in your driver, you should still think about that when designing your bindings. > > > > > > > Again, this property doesn't really have any meaning in the single-CS > > > case, so should I omit it completely? > > > > Keep in mind that you're defining a DT binding, and this binding is > > submitting to the 'stable ABI' rule, so you shouldn't think in term of > > what your driver supports now, but what you're IP is capable of, and > > it's definitely capable of handling several R/B pins. > > Yes, I understand about the stable API rule, but at the same time that R/B > mapping could be added without violating that rule at a later date if > someone really needed it, couldn't it? > > I mean, an IP may be capable of a lot of things, and we don't necessarily > want to implement them all in the driver to start with and have DT > propertes for them all do we? Hm, you should at least take the capabilities you know about into account when defining a new binding, and you already know that some SoCs might decide to expose 2 or more R/B pins, so it should already be designed this way. > > > > > > +- evatronix,timings: Seven 32-bit values for initializing the TIME_SEQ_0, > > > > > + TIME_SEQ_1, TIMINGS_ASYN, TIME_GEN_SEQ_0, TIME_GEN_SEQ_1, > > > > > + TIME_GEN_SEQ_2 and TIME_GEN_SEQ_3 registers, respectively. > > > > > > > > > > > > Can this be extracted from the timing mode exposed by the NAND chip. > > > > IMO it shouldn't be defined in the DT. > > > > > > I agree, Ideally one would like to read timing parameters from the NAND > > > chip (ONFi or JEDEC parameter pages) and determine all the controller IP > > > timings from that. There are a couple of reasons I haven't gone that > > > route. > > > > > I think what I'm worried about is that say we add the functionality to > take the appropriate parameters and create a set of timing values. We can > verify those values only under a very limited set of circumstances (a > certain clock frequency, certain pad delays on a given SoC, limited range > of flash chips etc). At a later date, the IP is used in a different SoC > and the original calculation turns out to be wrong in that case. Updating > the timing calcuation function to work with the original SoC and the new > one would be a bit of a nightmare I. Since at some point a set of specific > timing values need to be calculated for a given SoC anyway, we might as > well use those values as they are. > > > And my answer is no :). > > :-) > > > Just because it's complicated to extract these information at the driver > > level level doesn't mean you should put it in the DT. That's exactly the > > opposite: the DT is supposed to encode the hardware representation, not > > how you want to configure it. > > I would say that the complexity and error-proneness of automatic timing > calculation outweighs the benefit, but I see I'm fighting a loosing battle > here... > > > How about using a default/safe timing mode for now (ONFI timing mode 0 > > should work for all NANDs). > > The only thing you'll have to do is retrieve the source clk rate and > > calculate timing register values accordingly. Or you can even assume > > you always have a 100MHz source clk and hardcode it in your driver. > > Yes, that is certainly possible of course, and the driver already has a > hard-coded default setup for this case. > > In that case though the driver could have pre-set setups for other ONFi > modes, and we could have an _optional_ DT property to select them, the > reason for that being in order to handle non-ONFi flashes whose timing > cannot be gleaned from the device itself. > > I.e. something like > > evatronix,onfi-timing-mode = <2>; > > Would that be acceptable? Actually this has been added to the nand_flash_dev structure, and it's called onfi_timing_mode_default [1]. If you need a different timing, define a full ID entry for your NAND. Note that I'm also reworking the NAND detection code to let vendor specific code initialize a few things [2], which should allow us to get rid of the full-id entries. This timing mode tweaking could/should be done at this level. I'm opposed to the idea of putting information that can be automatically deduced in the DT. For this specific case, the main reason is that a board vendor can decide to use different NAND chips on the same design, and they might not all share the same capabilities (and you don't want to have a dts for each NAND). [1]http://lxr.free-electrons.com/source/include/linux/mtd/nand.h#L871 [2]https://lkml.org/lkml/2016/5/27/264 -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-10 15:54 ` Boris Brezillon @ 2016-06-10 16:46 ` Ricard Wanderlof 2016-06-10 17:03 ` Boris Brezillon 0 siblings, 1 reply; 9+ messages in thread From: Ricard Wanderlof @ 2016-06-10 16:46 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Fri, 10 Jun 2016, Boris Brezillon wrote: > > The use cases as I see are as follows: > > > > a) Two identical chips sharing all but the CS lines, in order to implement > > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a > > 4 GB area). In this case, for certain operations, the controller does not > > have to wait for one device to complete before issuing a command to > > another. I'm not sure how the controller keeps track of the two devices > > though. > > I think it's the chip-enable use case, isn't it? > > > > > b) Two different chips with the same connection, which provide disjunct > > functions, e.g. one (small) flash for program storage and one (large) for > > data. > > Then they can't share the CS line (or be actived at the same time), at > least it doesn't make any sense to me. Sorry if I haven't been clear enough, but in neither case are the CS lines shared between the devices. There is still a separate CS for each flash chip. The question is how the controller handles the CS lines. Basically, in the general case, the controller can handle a matrix of nand flash chips. There can be a number of banks, each of which can have a number of individual CS lines. For the (in this case academic) case of 3 banks and 4 chip selects per bank, there would be a total of 3 x 4 = 12 CS lines. For the IP configuration the driver was written for, there are only 2 CS lines, and we can configure if they are to be viewed by the controller as 2 CS lines within the same single bank, or 2 separate banks with one CS each. This is what the DT property is intended to express. It basically translates directly into a register write in the IP. But, as you are driving at below, the bindings should really cover the more general case, for which a simple use-bank-select property doesn't really cut it. Since the driver only supports a single CS at the time being, I'm really proposing to drop it completely, alternatively we could have two: 'banks' and 'devices-per-bank', which reflect what the general IP case would be able to handle. For the version of the IP in use these would have the permitted values of 1 and 2, with some combinations being illegal. Unfortunately, the IP configuration cannot be read out (neither the version of it), so it's not possible for the driver to verify the DT settings against the actual IP configuration. I don't really know how to solve that. > > > > I didn't really want to have the added flexibility (and complexity) of > > > > being able to use any R/B line for any connected flash chip. It seems an > > > > unnecessary complication for the driver without much gain. > > > > > > Not really, at least if you properly separate the chip and controller > > > objects it's quite easy to deal with, and I'll ask you to do this clean > > > separation anyway (even if you say you only have a single chip per > > > controller) :P. > > > > Yes, the separation of chip and controller is needed, but the R/B line > > flexibility requires an additional mapping functionality within the > > driver. Not rocket science of course, but I can't see much point in it > > (other than to cover up a potential routing mistake done by a PCB > > designer). > > You seem to argue on all the minor things I'm asking. Honestly, it > should be hard or error-prone to do it. And let's say you don't support > it in your driver, you should still think about that when designing > your bindings. Sorry, it's just that over the years I've seen too much code that introduces various flexibilities just because someone thought it might be 'nice to have at some point' or 'because the hardware supports it', but which in reality is never used and still must be maintained. Sure, one small mapping function is certainly not going to break the bank, far from it, but over time the matrix of things that can be configured can grow to awkward proportions, and often something thought of as a minor issue can turn out to be complex to support in the end. And given a stable API rule, it's too late to simplify things once they have been implemented. Of course, the code should not be written in a way to limit future expansion either. > > I mean, an IP may be capable of a lot of things, and we don't necessarily > > want to implement them all in the driver to start with and have DT > > propertes for them all do we? > > Hm, you should at least take the capabilities you know about into > account when defining a new binding, and you already know that some > SoCs might decide to expose 2 or more R/B pins, so it should already be > designed this way. Ok. I'll give it some more thought then. > I'm opposed to the idea of putting information that can be > automatically deduced in the DT. For this specific case, the main > reason is that a board vendor can decide to use different NAND chips > on the same design, and they might not all share the same > capabilities (and you don't want to have a dts for each NAND). Yes, that makes sense of course, but what if someone would want to override the automatic settings, for whatever reason, using an optional DT property? I can think of several reasons either way, that's why I'm asking. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-10 16:46 ` Ricard Wanderlof @ 2016-06-10 17:03 ` Boris Brezillon 2016-06-10 17:14 ` Ricard Wanderlof 0 siblings, 1 reply; 9+ messages in thread From: Boris Brezillon @ 2016-06-10 17:03 UTC (permalink / raw) To: Ricard Wanderlof Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Fri, 10 Jun 2016 18:46:24 +0200 Ricard Wanderlof <ricard.wanderlof@axis.com> wrote: > On Fri, 10 Jun 2016, Boris Brezillon wrote: > > > > The use cases as I see are as follows: > > > > > > a) Two identical chips sharing all but the CS lines, in order to implement > > > a seemingly-larger address space. (e.g. two 256 Mbit chips implementing a > > > 4 GB area). In this case, for certain operations, the controller does not > > > have to wait for one device to complete before issuing a command to > > > another. I'm not sure how the controller keeps track of the two devices > > > though. > > > > I think it's the chip-enable use case, isn't it? > > > > > > > > b) Two different chips with the same connection, which provide disjunct > > > functions, e.g. one (small) flash for program storage and one (large) for > > > data. > > > > Then they can't share the CS line (or be actived at the same time), at > > least it doesn't make any sense to me. > > Sorry if I haven't been clear enough, but in neither case are the CS lines > shared between the devices. There is still a separate CS for each flash > chip. The question is how the controller handles the CS lines. > > Basically, in the general case, the controller can handle a matrix of nand > flash chips. There can be a number of banks, each of which can have a > number of individual CS lines. For the (in this case academic) case of 3 > banks and 4 chip selects per bank, there would be a total of 3 x 4 = 12 CS > lines. > > For the IP configuration the driver was written for, there are only 2 CS > lines, and we can configure if they are to be viewed by the controller as > 2 CS lines within the same single bank, or 2 separate banks with one CS > each. This is what the DT property is intended to express. It basically > translates directly into a register write in the IP. Okay, got it. I guess we should just expose the chip select in a linear way (3 banks of 4 CS means the controller should support up to 12 chips), unless you really have a way to change the CS pins routing internally. > > But, as you are driving at below, the bindings should really cover the > more general case, for which a simple use-bank-select property doesn't > really cut it. Since the driver only supports a single CS at the time > being, I'm really proposing to drop it completely, alternatively we could > have two: 'banks' and 'devices-per-bank', which reflect what the general > IP case would be able to handle. For the version of the IP in use these > would have the permitted values of 1 and 2, with some combinations being > illegal. Unfortunately, the IP configuration cannot be read out (neither > the version of it), so it's not possible for the driver to verify the > DT settings against the actual IP configuration. I don't really know how > to solve that. Yes, let's drop the property for now. Just trying to understand how the IP works ;). > > > > > > I didn't really want to have the added flexibility (and complexity) of > > > > > being able to use any R/B line for any connected flash chip. It seems an > > > > > unnecessary complication for the driver without much gain. > > > > > > > > Not really, at least if you properly separate the chip and controller > > > > objects it's quite easy to deal with, and I'll ask you to do this clean > > > > separation anyway (even if you say you only have a single chip per > > > > controller) :P. > > > > > > Yes, the separation of chip and controller is needed, but the R/B line > > > flexibility requires an additional mapping functionality within the > > > driver. Not rocket science of course, but I can't see much point in it > > > (other than to cover up a potential routing mistake done by a PCB > > > designer). > > > > You seem to argue on all the minor things I'm asking. Honestly, it > > should be hard or error-prone to do it. And let's say you don't support > > it in your driver, you should still think about that when designing > > your bindings. > > Sorry, it's just that over the years I've seen too much code that > introduces various flexibilities just because someone thought it might be > 'nice to have at some point' or 'because the hardware supports it', but > which in reality is never used and still must be maintained. Sure, one > small mapping function is certainly not going to break the bank, far from > it, but over time the matrix of things that can be configured can grow to > awkward proportions, and often something thought of as a minor issue can > turn out to be complex to support in the end. And given a stable API rule, > it's too late to simplify things once they have been implemented. > > Of course, the code should not be written in a way to limit future > expansion either. > > > > I mean, an IP may be capable of a lot of things, and we don't necessarily > > > want to implement them all in the driver to start with and have DT > > > propertes for them all do we? > > > > Hm, you should at least take the capabilities you know about into > > account when defining a new binding, and you already know that some > > SoCs might decide to expose 2 or more R/B pins, so it should already be > > designed this way. > > Ok. I'll give it some more thought then. > > > I'm opposed to the idea of putting information that can be > > automatically deduced in the DT. For this specific case, the main > > reason is that a board vendor can decide to use different NAND chips > > on the same design, and they might not all share the same > > capabilities (and you don't want to have a dts for each NAND). > > Yes, that makes sense of course, but what if someone would want to > override the automatic settings, for whatever reason, using an optional DT > property? I can think of several reasons either way, that's why I'm > asking. You mean reducing the timings because the board design prevents using the highest supported mode for example? That would actually be a valid use case, and I guess we could make a generic property for that (without the vendor prefix). -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/4] of: Add device tree bindings for Evatronix 2016-06-10 17:03 ` Boris Brezillon @ 2016-06-10 17:14 ` Ricard Wanderlof 0 siblings, 0 replies; 9+ messages in thread From: Ricard Wanderlof @ 2016-06-10 17:14 UTC (permalink / raw) To: Boris Brezillon Cc: Brian Norris, David Woodhouse, Benoit Cousson, Tony Lindgren, devicetree, Linux mtd, linux-kernel On Fri, 10 Jun 2016, Boris Brezillon wrote: > > Basically, in the general case, the controller can handle a matrix of > > nand flash chips. There can be a number of banks, each of which can > > have a number of individual CS lines. For the (in this case academic) > > case of 3 banks and 4 chip selects per bank, there would be a total of > > 3 x 4 = 12 CS lines. > > > > For the IP configuration the driver was written for, there are only 2 > > CS lines, and we can configure if they are to be viewed by the > > controller as 2 CS lines within the same single bank, or 2 separate > > banks with one CS each. This is what the DT property is intended to > > express. It basically translates directly into a register write in the > > IP. > > Okay, got it. I guess we should just expose the chip select in a linear > way (3 banks of 4 CS means the controller should support up to 12 > chips), unless you really have a way to change the CS pins routing > internally. When I go through the driver I will also revisit this and give it some more thought if there's a set of bindings which would make sense both for the case we have now and for a general configuration of the IP. > > Yes, that makes sense of course, but what if someone would want to > > override the automatic settings, for whatever reason, using an > > optional DT property? I can think of several reasons either way, > > that's why I'm asking. > > You mean reducing the timings because the board design prevents using > the highest supported mode for example? That would actually be a valid > use case, and I guess we could make a generic property for that > (without the vendor prefix). Yes, either that, or if the automatic selection fails for some reason, say in a given case we know that the chips we are using support mode 2 timing, but one of them gets misidentified as mode 0. Sure, that is a bug and should be fixed of course, but I can imagine commercial situations where it may not be feasable to update the kernel, but where a new DT would be ok. /Ricard -- Ricard Wolf Wanderlöf ricardw(at)axis.com Axis Communications AB, Lund, Sweden www.axis.com Phone +46 46 272 2016 Fax +46 46 13 61 30 ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-06-10 17:14 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-02 7:47 [PATCH 1/4] of: Add device tree bindings for Evatronix Ricard Wanderlof 2016-06-03 14:22 ` Boris Brezillon 2016-06-07 15:01 ` Ricard Wanderlof 2016-06-08 15:50 ` Boris Brezillon 2016-06-10 15:35 ` Ricard Wanderlof 2016-06-10 15:54 ` Boris Brezillon 2016-06-10 16:46 ` Ricard Wanderlof 2016-06-10 17:03 ` Boris Brezillon 2016-06-10 17:14 ` Ricard Wanderlof
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).