| Message ID | 1384343884-29622-1-git-send-email-shc_work@mail.ru |
|---|---|
| State | New, archived |
| Headers | show |
+ Pekon, Ezequiel Hi Alexander, We're dealing with a similar issue in other drivers currently, and I think it's worth straightening out the issue for all systems. On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: > This patch adds a property to automatically determine the NAND > bus width by CFI/ONFI information from chip. This property works > if the bus width is not specified explicitly. This issue brings up a few questions in my mind, which are relevant to device tree in general. First of all, do we need a device tree property at all, for something that is auto-detectable? Related: is a device tree consumer (like Linux) supposed to be a validator, or simply a best-effort? I'm considering the following case: if Linux is allowed to auto-detect some property which also has a device tree binding (e.g., "nand-bus-width", in Documentation/devicetree/bindings/mtd/nand.txt), what should happen if the binding happens to be incorrect? IOW, what if the device tree specifies buswidth is x16, but Linux correctly detects it as x8? Shouldn't we make the best effort to bring the hardware up, regardless of what the device tree says? So for something like this GPIO driver, I'm thinking that Linux should just use NAND_BUSWIDTH_AUTO all the time [*]. But (as I'm hinting above) that would allow DTB firmware implementors to be lazier and have a technically-incorrect "nand-bus-width" or "bank-width" binding, since they know it can reliably be detected and overridden by Linux. [*] Except where resource_size(res) < 2, as Alexander already has in this patch. > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- > .../devicetree/bindings/mtd/gpio-control-nand.txt | 3 +++ > drivers/mtd/nand/gpio.c | 16 ++++++++++++---- > 2 files changed, 15 insertions(+), 4 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > index 36ef07d..fe4e960 100644 > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > @@ -19,6 +19,9 @@ Optional properties: > defaults to 1 byte. > - chip-delay : chip dependent delay for transferring data from array to > read registers (tR). If not present then a default of 20us is used. > +- gpio-control-nand,bank-width-auto : Device bus width is determined > + automatically by CFI/ONFI information from chip if "bank-width" > + parameter is omitted (Boolean). If we do resort to a new binding for auto-buswidth, it should be a generic one that all NAND drivers can use. Maybe a separate boolean "nand-bus-width-auto" and if it is present, then it overrules the presence (or lack) of the "nand-bus-width" boolean property? Or is it possible to extend "nand-bus-width" to allow the value of 0 to mean automatic? You may want to modify the of_get_nand_bus_width() helper (or add a new one, of_nand_bus_width_auto()?) to drivers/of/of_mtd.c to assist with this. ...BTW, it looks like we have a duplicate binding here: GPIO NAND defines "bank-width" where generic NAND defines "nand-bus-width". Aren't these essentially duplications? Can we support the generic binding in gpio.c and discourage "bank-width"? Or is that just unnecessary effort? > - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read > location used to guard against bus reordering with regards to accesses to > the GPIO's and the NAND flash data bus. If present, then after changing [...] Brian
On Tue, Nov 26, 2013 at 05:21:58PM -0800, Brian Norris wrote: > + Pekon, Ezequiel + Pekon, Ezequiel for real this time! Sorry... Everything else intact /Brian > > Hi Alexander, > > We're dealing with a similar issue in other drivers currently, and I > think it's worth straightening out the issue for all systems. > > On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: > > This patch adds a property to automatically determine the NAND > > bus width by CFI/ONFI information from chip. This property works > > if the bus width is not specified explicitly. > > This issue brings up a few questions in my mind, which are relevant to > device tree in general. > > First of all, do we need a device tree property at all, for something > that is auto-detectable? > > Related: is a device tree consumer (like Linux) supposed to be a > validator, or simply a best-effort? I'm considering the following case: > if Linux is allowed to auto-detect some property which also has a device > tree binding (e.g., "nand-bus-width", in > Documentation/devicetree/bindings/mtd/nand.txt), what should happen if > the binding happens to be incorrect? IOW, what if the device tree > specifies buswidth is x16, but Linux correctly detects it as x8? > Shouldn't we make the best effort to bring the hardware up, regardless > of what the device tree says? > > So for something like this GPIO driver, I'm thinking that Linux should > just use NAND_BUSWIDTH_AUTO all the time [*]. But (as I'm hinting above) > that would allow DTB firmware implementors to be lazier and have a > technically-incorrect "nand-bus-width" or "bank-width" binding, since > they know it can reliably be detected and overridden by Linux. > > [*] Except where resource_size(res) < 2, as Alexander already has in > this patch. > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > .../devicetree/bindings/mtd/gpio-control-nand.txt | 3 +++ > > drivers/mtd/nand/gpio.c | 16 ++++++++++++---- > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > index 36ef07d..fe4e960 100644 > > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > @@ -19,6 +19,9 @@ Optional properties: > > defaults to 1 byte. > > - chip-delay : chip dependent delay for transferring data from array to > > read registers (tR). If not present then a default of 20us is used. > > +- gpio-control-nand,bank-width-auto : Device bus width is determined > > + automatically by CFI/ONFI information from chip if "bank-width" > > + parameter is omitted (Boolean). > > If we do resort to a new binding for auto-buswidth, it should be a > generic one that all NAND drivers can use. Maybe a separate boolean > "nand-bus-width-auto" and if it is present, then it overrules the > presence (or lack) of the "nand-bus-width" boolean property? > Or is it possible to extend "nand-bus-width" to allow the value of 0 to > mean automatic? > > You may want to modify the of_get_nand_bus_width() helper (or add a new > one, of_nand_bus_width_auto()?) to drivers/of/of_mtd.c to assist with > this. > > ...BTW, it looks like we have a duplicate binding here: GPIO NAND > defines "bank-width" where generic NAND defines "nand-bus-width". Aren't > these essentially duplications? Can we support the generic binding in > gpio.c and discourage "bank-width"? Or is that just unnecessary effort? > > > - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read > > location used to guard against bus reordering with regards to accesses to > > the GPIO's and the NAND flash data bus. If present, then after changing > > [...] > > Brian
... > We're dealing with a similar issue in other drivers currently, and I > think it's worth straightening out the issue for all systems. > > On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: > > This patch adds a property to automatically determine the NAND > > bus width by CFI/ONFI information from chip. This property works > > if the bus width is not specified explicitly. > > This issue brings up a few questions in my mind, which are relevant to > device tree in general. > > First of all, do we need a device tree property at all, for something > that is auto-detectable? > > Related: is a device tree consumer (like Linux) supposed to be a > validator, or simply a best-effort? I'm considering the following case: > if Linux is allowed to auto-detect some property which also has a device > tree binding (e.g., "nand-bus-width", in > Documentation/devicetree/bindings/mtd/nand.txt), what should happen if > the binding happens to be incorrect? IOW, what if the device tree > specifies buswidth is x16, but Linux correctly detects it as x8? > Shouldn't we make the best effort to bring the hardware up, regardless > of what the device tree says? > > So for something like this GPIO driver, I'm thinking that Linux should > just use NAND_BUSWIDTH_AUTO all the time [*]. But (as I'm hinting above) > that would allow DTB firmware implementors to be lazier and have a > technically-incorrect "nand-bus-width" or "bank-width" binding, since > they know it can reliably be detected and overridden by Linux. > > [*] Except where resource_size(res) < 2, as Alexander already has in > this patch. > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > > --- > > .../devicetree/bindings/mtd/gpio-control-nand.txt | 3 +++ > > drivers/mtd/nand/gpio.c | 16 ++++++++++++---- > > 2 files changed, 15 insertions(+), 4 deletions(-) > > > > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > index 36ef07d..fe4e960 100644 > > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > > @@ -19,6 +19,9 @@ Optional properties: > > defaults to 1 byte. > > - chip-delay : chip dependent delay for transferring data from array to > > read registers (tR). If not present then a default of 20us is used. > > +- gpio-control-nand,bank-width-auto : Device bus width is determined > > + automatically by CFI/ONFI information from chip if "bank-width" > > + parameter is omitted (Boolean). > > If we do resort to a new binding for auto-buswidth, it should be a > generic one that all NAND drivers can use. Maybe a separate boolean > "nand-bus-width-auto" and if it is present, then it overrules the > presence (or lack) of the "nand-bus-width" boolean property? > Or is it possible to extend "nand-bus-width" to allow the value of 0 to > mean automatic? This was be my first implementation of this feature, but rejected... http://permalink.gmane.org/gmane.linux.drivers.mtd/47302 ---
Hi Alexander, On Tue, Nov 26, 2013 at 8:21 PM, Alexander Shiyan <shc_work@mail.ru> wrote: > ... >> We're dealing with a similar issue in other drivers currently, and I >> think it's worth straightening out the issue for all systems. >> >> On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: >> > This patch adds a property to automatically determine the NAND >> > bus width by CFI/ONFI information from chip. This property works >> > if the bus width is not specified explicitly. >> >> This issue brings up a few questions in my mind, which are relevant to >> device tree in general. >> >> First of all, do we need a device tree property at all, for something >> that is auto-detectable? >> >> Related: is a device tree consumer (like Linux) supposed to be a >> validator, or simply a best-effort? I'm considering the following case: >> if Linux is allowed to auto-detect some property which also has a device >> tree binding (e.g., "nand-bus-width", in >> Documentation/devicetree/bindings/mtd/nand.txt), what should happen if >> the binding happens to be incorrect? IOW, what if the device tree >> specifies buswidth is x16, but Linux correctly detects it as x8? >> Shouldn't we make the best effort to bring the hardware up, regardless >> of what the device tree says? >> >> So for something like this GPIO driver, I'm thinking that Linux should >> just use NAND_BUSWIDTH_AUTO all the time [*]. But (as I'm hinting above) >> that would allow DTB firmware implementors to be lazier and have a >> technically-incorrect "nand-bus-width" or "bank-width" binding, since >> they know it can reliably be detected and overridden by Linux. >> >> [*] Except where resource_size(res) < 2, as Alexander already has in >> this patch. >> >> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> >> > --- >> > .../devicetree/bindings/mtd/gpio-control-nand.txt | 3 +++ >> > drivers/mtd/nand/gpio.c | 16 ++++++++++++---- >> > 2 files changed, 15 insertions(+), 4 deletions(-) >> > >> > diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > index 36ef07d..fe4e960 100644 >> > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt >> > @@ -19,6 +19,9 @@ Optional properties: >> > defaults to 1 byte. >> > - chip-delay : chip dependent delay for transferring data from array to >> > read registers (tR). If not present then a default of 20us is used. >> > +- gpio-control-nand,bank-width-auto : Device bus width is determined >> > + automatically by CFI/ONFI information from chip if "bank-width" >> > + parameter is omitted (Boolean). >> >> If we do resort to a new binding for auto-buswidth, it should be a >> generic one that all NAND drivers can use. Maybe a separate boolean >> "nand-bus-width-auto" and if it is present, then it overrules the >> presence (or lack) of the "nand-bus-width" boolean property? >> Or is it possible to extend "nand-bus-width" to allow the value of 0 to >> mean automatic? > > This was be my first implementation of this feature, but rejected... > http://permalink.gmane.org/gmane.linux.drivers.mtd/47302 Huh? Your original patch was not correct (it removed properties) and it was not comprehensive in its description -- it didn't cover any of the topics in my questions above. It wasn't rejected for any reason related to my questions here. My question is whether we can leave the documented bus-width/bank-width binding in place, but just change the Linux implementation to perform auto-detection instead of just using the binding directly. And if the answer is "no", then we need a new binding like your current patch, except that it should be usable for all NAND. Brian
Hi Alexander, > From: Alexander Shiyan > This patch adds a property to automatically determine the NAND > bus width by CFI/ONFI information from chip. This property works > if the bus width is not specified explicitly. > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru> > --- [...] > --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt > @@ -19,6 +19,9 @@ Optional properties: > defaults to 1 byte. > - chip-delay : chip dependent delay for transferring data from array to > read registers (tR). If not present then a default of 20us is used. > +- gpio-control-nand,bank-width-auto : Device bus width is determined > + automatically by CFI/ONFI information from chip if "bank-width" > + parameter is omitted (Boolean). > There is one important thing to understand here is that when a READ_ID or READ_PARAM command is issued to NAND device, the read_data is is transmitted only on lower 8-bit data lines. This is as per ONFI standard. Therefore, irrespective of whether the NAND device is x8 or x16, ONFI parameter page is always read in 8-bit mode.. -------------------------- [*] Reference: ONFI spec version 3.1 (section 3.5.3. Target Initialization) "The Read ID and Read Parameter Page commands only use the lower 8-bits of the data bus. The host shall not issue commands that use a word data width on x16 devices until the host determines the device supports a 16-bit data bus width in the parameter page." -------------------------- Now this forms the basis of NAND_BUSWIDTH_AUTO, which allows ONFI parameter parsing '@@nand_flash_detect_onfi()' only in 8-bit mode. And once the actual width is known, then it re-configures the driver while returning from nand_scan_ident(). Thus, in summary: To read on-chip ONFI parameters, your driver should always be in 8-bit mode. Using this as basis, we tried eliminate NAND_BUSWIDTH_AUTO and instead implemented this requirement directly as part of generic NAND driver in nand_base.c: nand_flash_detect_onfi(). Please refer following patch and subsequent discussion on same. (It might help you understand that we don't need to explicitly set have NAND_BUSWIDTH_AUTO, instead we can make it implicit in our code while scanning NAND devices) http://lists.infradead.org/pipermail/linux-mtd/2013-November/050252.html with regards, pekon
> Hi Alexander, > > We're dealing with a similar issue in other drivers currently, and I > think it's worth straightening out the issue for all systems. > > On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: > > This patch adds a property to automatically determine the NAND > > bus width by CFI/ONFI information from chip. This property works > > if the bus width is not specified explicitly. ... Hello Brian. Can at least part [2/3] "mtd: nand: gpio: Use devm_gpio_request_one() where possible" be applied at this time? Thanks. ---
Hi Brian, On Tue, Nov 26, 2013 at 05:23:38PM -0800, Brian Norris wrote: [..] > > > > If we do resort to a new binding for auto-buswidth, it should be a > > generic one that all NAND drivers can use. Why do we need yet another binding to describe something that's completely discoverable? I'm working on *removing* any need to set the bus width, either from the driver or from the DT, so I see this patch as step backwards. Can anyone help me understand if there's *any* valid use case where we want to specify a-priori the bus width, considering it's completely discoverable at run-time?
> Hi Brian, > > On Tue, Nov 26, 2013 at 05:23:38PM -0800, Brian Norris wrote: > [..] > > > > > > If we do resort to a new binding for auto-buswidth, it should be a > > > generic one that all NAND drivers can use. > > Why do we need yet another binding to describe something that's > completely discoverable? > > I'm working on *removing* any need to set the bus width, either from the > driver or from the DT, so I see this patch as step backwards. > > Can anyone help me understand if there's *any* valid use case where we > want to specify a-priori the bus width, considering it's completely > discoverable at run-time? Look at my previous attempt: http://permalink.gmane.org/gmane.linux.drivers.mtd/47411 http://permalink.gmane.org/gmane.linux.drivers.mtd/47413 ---
On Fri, Nov 29, 2013 at 04:35:04PM +0400, Alexander Shiyan wrote: > > Hi Brian, > > > > On Tue, Nov 26, 2013 at 05:23:38PM -0800, Brian Norris wrote: > > [..] > > > > > > > > If we do resort to a new binding for auto-buswidth, it should be a > > > > generic one that all NAND drivers can use. > > > > Why do we need yet another binding to describe something that's > > completely discoverable? > > > > I'm working on *removing* any need to set the bus width, either from the > > driver or from the DT, so I see this patch as step backwards. > > > > Can anyone help me understand if there's *any* valid use case where we > > want to specify a-priori the bus width, considering it's completely > > discoverable at run-time? > > Look at my previous attempt: > http://permalink.gmane.org/gmane.linux.drivers.mtd/47411 > http://permalink.gmane.org/gmane.linux.drivers.mtd/47413 > I think we're mixing (again) the NAND device bus width with the controller bus width. The former is discoverable and doesn't belong at DT (or anywhere), the latter goes beyond NAND (as memory controller's can handle other kinds of flashes) and is usually required to be specifiedat the DT. Hence, I think only the latter needs a DT binding.
On Fri, Nov 29, 2013 at 09:25:52AM -0300, Ezequiel Garcia wrote: > On Tue, Nov 26, 2013 at 05:23:38PM -0800, Brian Norris wrote: > [..] > > > > > > If we do resort to a new binding for auto-buswidth, it should be a > > > generic one that all NAND drivers can use. > > Why do we need yet another binding to describe something that's > completely discoverable? The DT property is not specified only for the sake of the flash chip itself, but for the sake of the controller which supports it. I suppose we've kind of overloaded its usage, but it is not entirely auto-detectable. > I'm working on *removing* any need to set the bus width, either from the > driver or from the DT, so I see this patch as step backwards. Well, I disagree with the removal ;) > Can anyone help me understand if there's *any* valid use case where we > want to specify a-priori the bus width, considering it's completely > discoverable at run-time? I think the primary use case should be to reflect a limitation in the hardware (besides just the flash chip). It can mean that the controller itself only supports one bus width, or that the board is only wired up for x8, for instance. Brian
On Fri, Nov 29, 2013 at 12:56:04PM +0400, Alexander Shiyan wrote: > > Hi Alexander, > > > > We're dealing with a similar issue in other drivers currently, and I > > think it's worth straightening out the issue for all systems. > > > > On Wed, Nov 13, 2013 at 03:58:02PM +0400, Alexander Shiyan wrote: > > > This patch adds a property to automatically determine the NAND > > > bus width by CFI/ONFI information from chip. This property works > > > if the bus width is not specified explicitly. > ... > > Hello Brian. > Can at least part [2/3] "mtd: nand: gpio: Use devm_gpio_request_one() where possible" > be applied at this time? Probably. I was avoiding taking things out of order in the series for now, but I'll take another look. If it looks good and applies independently, then I'll take it. Brian
Brian, On Sat, Nov 30, 2013 at 01:15:56AM -0800, Brian Norris wrote: > > > Can anyone help me understand if there's *any* valid use case where we > > want to specify a-priori the bus width, considering it's completely > > discoverable at run-time? > > I think the primary use case should be to reflect a limitation in the > hardware (besides just the flash chip). It can mean that the controller > itself only supports one bus width, or that the board is only wired up > for x8, for instance. > What do you mean by "reflect a limitation" of the hardware? Maybe I'm not really following you, but it sounds as you're mixing up the NAND device buswidth and the memory controller buswidth. At least I consider them as two different parameters for two different pieces of hardware. Consider OMAP, just as an example. We currently have two DT properties: 1. nand-bus-width: not related to OMAP but kernel wide. It's supposed to specify the devices buswidth. 2. gpmc,device-width: It specifies how the controller should be configured (it doesn't really specify hardware, but configuration, sadly). I now realise maybe this piece of DT binding is not a good example of DT. Anyway, once again: Why would we need to set "nand-bus-width" to specify the flash device width given we can discover that as soon as the NAND is detected? Notice, that this discussion is independent of the discussion about removing the NAND_BUSWIDTH_AUTO. It's just about removing a DT property for a parameter that's runtime configurable.
On Sat, Nov 30, 2013 at 08:17:22AM -0300, Ezequiel Garcia wrote: > Brian, > > On Sat, Nov 30, 2013 at 01:15:56AM -0800, Brian Norris wrote: > > > > > Can anyone help me understand if there's *any* valid use case where we > > > want to specify a-priori the bus width, considering it's completely > > > discoverable at run-time? > > > > I think the primary use case should be to reflect a limitation in the > > hardware (besides just the flash chip). It can mean that the controller > > itself only supports one bus width, or that the board is only wired up > > for x8, for instance. > > > > What do you mean by "reflect a limitation" of the hardware? Read the following sentence. Limitations: the hardware could be hooked up only for 8 data lines; or the controller could support only one particular bus width. Either of these can only be reflected in device tree. > Maybe I'm not really following you, but it sounds as you're mixing up > the NAND device buswidth and the memory controller buswidth. At least > I consider them as two different parameters for two different pieces > of hardware. Yes, maybe I'm combining two properties, but we only have one way to express this for now. This should be resolved. > Consider OMAP, just as an example. We currently have two DT properties: > > 1. nand-bus-width: not related to OMAP but kernel wide. It's supposed > to specify the devices buswidth. > > 2. gpmc,device-width: It specifies how the controller should be > configured (it doesn't really specify hardware, but configuration, > sadly). > > I now realise maybe this piece of DT binding is not a good example of DT. > > Anyway, once again: Why would we need to set "nand-bus-width" to specify > the flash device width given we can discover that as soon as the NAND > is detected? You've asked two different questions. Question 1: Can anyone help me understand if there's *any* valid use case where we want to specify a-priori the bus width, considering it's completely discoverable at run-time? Question 2: Why would we need to set "nand-bus-width" to specify the flash device width given we can discover that as soon as the NAND is detected? The first question is about the NAND core in general, where I strongly believe that a NAND driver has the right to specify "a-priori" what its supported bus width(s) is/are. The second question is specifically about the nand-bus-width DT property. I think the property is somewhat broken, since it describes the flash chip and not exactly the board wiring or controller capabilities. So to keep this thread focused on Alexander's patch: +- gpio-control-nand,bank-width-auto : Device bus width is determined + automatically by CFI/ONFI information from chip if "bank-width" + parameter is omitted (Boolean). This description fits squarely into describing the flash chip (not the board wiring or controller limitations), and so it seems superfluous (in agreement with Ezequiel here, right?). But we have this property already for GPIO: - bank-width : Width (in bytes) of the device. If not present, the width defaults to 1 byte. This seems to describe the flash chip as well, and paints us into a corner where we can't determine if it was used because of board/controller limitations or because of the flash chip. We can get out of this through the use of an additional property of some kind. I think to describe the controller/board properly, we probably need something like this for some drivers: <manufacturer>,supported-bus-widths = <LIST>; Where <LIST> can be one or more of 8 or 16, and it reflects any hardware limitations outside of the flash chip (e.g., # of data lines; controller limitations). And if <manufacturer>,supported-bus-widths is missing, then we default to say that the hardware *only* supports the bus width found in "nand-bus-width"/"bank-width"/similar. So we have this: [I] <manufacturer>,supported-bus-widths = <8>, <16>; nand-bus-width = <don't care>; ===> This means the NAND driver could allow auto-detect of the flash buswidth. The "nand-bus-width" (or "bank-width") can actually be "deprecated" here. [II] <manufacturer>,supported-bus-widths = <X>; // X = 8 or 16 nand-bus-width = <Y>; // Y = 8 or 16 or missing ===> This means the NAND driver should only configure bus width to X. If Y is missing, we're OK. If X != Y, then the device tree is malformed. [III] <manufacturer>,supported-bus-widths is missing; nand-bus-width = <Y>; // Y = 8 or 16 or missing ===> This means the NAND driver should only configure bus width to Y. This covers the most general DT description, I think. But in the GPIO case, the "controller" is so dead simple that we can probably represent the "supported-bus-widths" simply through the # of connected data lines: gpio,data-lines = <X>; where X = 8 or 16. If X=16, that means it can support either x8 or x16 (and hence, auto-detection in the NAND core), but if X=8, we only support x8 and no autodetection. Brian
On Sat, Nov 30, 2013 at 01:17:38AM -0800, Brian Norris wrote: > On Fri, Nov 29, 2013 at 12:56:04PM +0400, Alexander Shiyan wrote: > > Can at least part [2/3] "mtd: nand: gpio: Use devm_gpio_request_one() where possible" > > be applied at this time? > > Probably. I was avoiding taking things out of order in the series for > now, but I'll take another look. If it looks good and applies > independently, then I'll take it. Patch 2 can't be applied independently. You can rebase and resend if you want it applied. We still haven't resolved patch 1. It doesn't seem like quite the right binding. Brian
> On Sat, Nov 30, 2013 at 01:17:38AM -0800, Brian Norris wrote: > > On Fri, Nov 29, 2013 at 12:56:04PM +0400, Alexander Shiyan wrote: > > > Can at least part [2/3] "mtd: nand: gpio: Use devm_gpio_request_one() where possible" > > > be applied at this time? > > > > Probably. I was avoiding taking things out of order in the series for > > now, but I'll take another look. If it looks good and applies > > independently, then I'll take it. > > Patch 2 can't be applied independently. You can rebase and resend if you > want it applied. > > We still haven't resolved patch 1. It doesn't seem like quite the right > binding. I am again inclined to believe that it is necessary to completely remove the width parameter for the driver and always determine the bus width automatically when the window size is more than 1 byte. ---
diff --git a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt index 36ef07d..fe4e960 100644 --- a/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt +++ b/Documentation/devicetree/bindings/mtd/gpio-control-nand.txt @@ -19,6 +19,9 @@ Optional properties: defaults to 1 byte. - chip-delay : chip dependent delay for transferring data from array to read registers (tR). If not present then a default of 20us is used. +- gpio-control-nand,bank-width-auto : Device bus width is determined + automatically by CFI/ONFI information from chip if "bank-width" + parameter is omitted (Boolean). - gpio-control-nand,io-sync-reg : A 64-bit physical address for a read location used to guard against bus reordering with regards to accesses to the GPIO's and the NAND flash data bus. If present, then after changing diff --git a/drivers/mtd/nand/gpio.c b/drivers/mtd/nand/gpio.c index e826f89..8ec731d 100644 --- a/drivers/mtd/nand/gpio.c +++ b/drivers/mtd/nand/gpio.c @@ -116,6 +116,9 @@ static int gpio_nand_get_config_of(const struct device *dev, dev_err(dev, "invalid bank-width %u\n", val); return -EINVAL; } + } else if (of_property_read_bool(dev->of_node, + "gpio-control-nand,bank-width-auto")) { + plat->options |= NAND_BUSWIDTH_AUTO; } plat->gpio_rdy = of_get_gpio(dev->of_node, 0); @@ -223,6 +226,15 @@ static int gpio_nand_probe(struct platform_device *pdev) if (IS_ERR(chip->IO_ADDR_R)) return PTR_ERR(chip->IO_ADDR_R); + ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); + if (ret) + return ret; + + /* Only 8-bit bus wide is possible if size is 1 */ + if (resource_size(res) < 2) + gpiomtd->plat.options &= ~(NAND_BUSWIDTH_16 | + NAND_BUSWIDTH_AUTO); + res = gpio_nand_get_io_sync(pdev); if (res) { gpiomtd->io_sync = devm_ioremap_resource(&pdev->dev, res); @@ -230,10 +242,6 @@ static int gpio_nand_probe(struct platform_device *pdev) return PTR_ERR(gpiomtd->io_sync); } - ret = gpio_nand_get_config(&pdev->dev, &gpiomtd->plat); - if (ret) - return ret; - ret = devm_gpio_request(&pdev->dev, gpiomtd->plat.gpio_nce, "NAND NCE"); if (ret) return ret;
This patch adds a property to automatically determine the NAND bus width by CFI/ONFI information from chip. This property works if the bus width is not specified explicitly. Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- .../devicetree/bindings/mtd/gpio-control-nand.txt | 3 +++ drivers/mtd/nand/gpio.c | 16 ++++++++++++---- 2 files changed, 15 insertions(+), 4 deletions(-)