| Message ID | 1400267276-16001-1-git-send-email-pekon@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello Pekon, On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote: > This patch enables 'wait-pin' monitoring in NAND driver if following properties > are present under NAND DT node > gpmc,wait-pin = <wait-pin number> > gpmc,wait-on-read; > gpmc,wait-on-write; > As NAND generic framework uses common path nand_chip->dev_ready() for monitoring > completion of Read and Write status, so wait-pin monitoring is enabled only when > both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified. > I see that the GPMC driver checks if "gpmc,wait-pin" property is defined and only in that case tries to read both "gpmc,wait-on-read" and "gpmc,wait-on-write" and prints a "read/write wait monitoring not enabled!" warning if one of those is not defined. So my question is, it's a requirement and these 3 properties need to always be defined together? If that is the case then I wonder why there are 3 different properties and why not just defining "gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ? Or if is valid to define "gpmc-wait-pin" without "gpmc-wait-on-{read,write}" or only one those then why that scary warning is printed? Whatever is the case I think that the GPMC DT binding documentation (Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more clear on this regard. > Signed-off-by: Pekon Gupta <pekon@ti.com> > --- > arch/arm/mach-omap2/gpmc-nand.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c > index 4349e82..7dc742d 100644 > --- a/arch/arm/mach-omap2/gpmc-nand.c > +++ b/arch/arm/mach-omap2/gpmc-nand.c > @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, > } > } > > - if (gpmc_nand_data->of_node) > + if (gpmc_nand_data->of_node) { > gpmc_read_settings_dt(gpmc_nand_data->of_node, &s); > - else > + if (s.wait_on_read && s.wait_on_write) You said that wait-on-pin monitoring is only enabled if both "gpmc,wait-on-read" and "gpmc,wait-on-write" are specified. But in that case I think we should clear the wait_pin on gpmc_read_settings_dt() if either "gpmc,wait-on-read" or "gpmc,wait-on-write" were not specified and check s.wait_pin instead. Or is this semantic only for NAND and other devices connected to the GPMC behave differently wrt "gpmc,wait-pin"? > + gpmc_nand_data->dev_ready = "true"; You should really use gpmc_nand_data->dev_ready = true here. The C99 standard allows you to assign a string literal to a _Bool type and will be evaluated to 1 but that is confusing and I haven't seen that used in other part of the kernel. > + } else { > gpmc_set_legacy(gpmc_nand_data, &s); > - > + } > s.device_nand = true; > > err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s); > -- > 1.8.5.1.163.gd7aced9 > Best regards, Javier -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Javier, >From: Javier Martinez Canillas [mailto:javier@dowhile0.org] > >Hello Pekon, > >On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote: >> This patch enables 'wait-pin' monitoring in NAND driver if following properties >> are present under NAND DT node >> gpmc,wait-pin = <wait-pin number> >> gpmc,wait-on-read; >> gpmc,wait-on-write; >> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring >> completion of Read and Write status, so wait-pin monitoring is enabled only when >> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified. >> > >I see that the GPMC driver checks if "gpmc,wait-pin" property is >defined and only in that case tries to read both "gpmc,wait-on-read" >and "gpmc,wait-on-write" and prints a "read/write wait monitoring not >enabled!" warning if one of those is not defined. > >So my question is, it's a requirement and these 3 properties need to >always be defined together? If that is the case then I wonder why >there are 3 different properties and why not just defining >"gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ? > GPMC as a hardware engine allows selection of wait-pin independently for both Read and Write paths, but NAND generic framework uses single common function nand_chip->dev_ready() which is used for both Read and Write paths to poll wait-pin. So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' should be simultaneously defined to enable wait-pin monitoring. But GPMC being generic hardware engine for NOR, OneNAND and other parallel interfaces like Camera, Ethernet the two separate bindings allow flexibility to use wait-pin monitoring for only one of the paths {Read | Write}. Therefore this check is added in gpmc_nand_init(), and not in gpmc_read_settings_dt() which is common for all type of GPMC devices (NAND, NOR, .. ) >Or if is valid to define "gpmc-wait-pin" without >"gpmc-wait-on-{read,write}" or only one those then why that scary >warning is printed? > >Whatever is the case I think that the GPMC DT binding documentation >(Documentation/devicetree/bindings/bus/ti-gpmc.txt) should be more >clear on this regard. > Agree. But I think this clarification fits better in NAND specific documentation Documentation/devicetree/bindings/mtd/gpmc-nand.txt >> Signed-off-by: Pekon Gupta <pekon@ti.com> >> --- >> arch/arm/mach-omap2/gpmc-nand.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c >> index 4349e82..7dc742d 100644 >> --- a/arch/arm/mach-omap2/gpmc-nand.c >> +++ b/arch/arm/mach-omap2/gpmc-nand.c >> @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, >> } >> } >> >> - if (gpmc_nand_data->of_node) >> + if (gpmc_nand_data->of_node) { >> gpmc_read_settings_dt(gpmc_nand_data->of_node, &s); >> - else >> + if (s.wait_on_read && s.wait_on_write) > >You said that wait-on-pin monitoring is only enabled if both >"gpmc,wait-on-read" and "gpmc,wait-on-write" are specified. But in >that case I think we should clear the wait_pin on >gpmc_read_settings_dt() if either "gpmc,wait-on-read" or >"gpmc,wait-on-write" were not specified and check s.wait_pin instead. > >Or is this semantic only for NAND and other devices connected to the >GPMC behave differently wrt "gpmc,wait-pin"? > True, this is only for NAND. Other device drivers may use wait-pin either 'only for Read' or 'only for Write' access. However I doubt why anyone would do so, unless there is some design bug | constrain. >> + gpmc_nand_data->dev_ready = "true"; > >You should really use gpmc_nand_data->dev_ready = true here. The C99 >standard allows you to assign a string literal to a _Bool type and >will be evaluated to 1 but that is confusing and I haven't seen that >used in other part of the kernel. > Sorry, my bad. I over looked it. I'll fix it in next version along with Documentation update. With regards, pekon
On 05/19/2014 09:26 PM, Gupta, Pekon wrote: > Hi Javier, > >> From: Javier Martinez Canillas [mailto:javier@dowhile0.org] >> >> Hello Pekon, >> >> On Fri, May 16, 2014 at 9:07 PM, Pekon Gupta <pekon@ti.com> wrote: >>> This patch enables 'wait-pin' monitoring in NAND driver if following properties >>> are present under NAND DT node >>> gpmc,wait-pin = <wait-pin number> >>> gpmc,wait-on-read; >>> gpmc,wait-on-write; >>> As NAND generic framework uses common path nand_chip->dev_ready() for monitoring >>> completion of Read and Write status, so wait-pin monitoring is enabled only when >>> both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified. >>> >> >> I see that the GPMC driver checks if "gpmc,wait-pin" property is >> defined and only in that case tries to read both "gpmc,wait-on-read" >> and "gpmc,wait-on-write" and prints a "read/write wait monitoring not >> enabled!" warning if one of those is not defined. >> >> So my question is, it's a requirement and these 3 properties need to >> always be defined together? If that is the case then I wonder why >> there are 3 different properties and why not just defining >> "gpmc,wait-pin" implies "gpmc,wait-on-{read,write}" ? >> > GPMC as a hardware engine allows selection of wait-pin independently > for both Read and Write paths, but NAND generic framework uses single > common function nand_chip->dev_ready() which is used for both > Read and Write paths to poll wait-pin. > So, in case of NAND both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' > should be simultaneously defined to enable wait-pin monitoring. > > But GPMC being generic hardware engine for NOR, OneNAND and other > parallel interfaces like Camera, Ethernet the two separate bindings allow > flexibility to use wait-pin monitoring for only one of the paths {Read | Write}. > > Therefore this check is added in gpmc_nand_init(), and not in gpmc_read_settings_dt() > which is common for all type of GPMC devices (NAND, NOR, .. ) The behavior of wait pin is slightly different when it is a NAND device when compared to other NOR like devices. On NAND, the pin is not used for inserting wait states in the bus access cycle, but instead it is used for waiting for the device to be ready after a particular command. So this wait logic must be implemented in the NAND driver software. GPMC will only forward the wait pin state via a status register, or at best give you an interrupt. For NAND use case, the GPMC hardware WAIT pin monitoring logic needs to be disabled (see NOTE in section 10.1.5.14.2 NAND Device-Ready Pin). The NAND driver only needs to know which wait pin the NAND chips device ready is connected to so that it can monitor it via the GPMC.STATUS register. The current omap_dev_ready() is so fragile that it will break if NAND device is not on CS0. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c index 4349e82..7dc742d 100644 --- a/arch/arm/mach-omap2/gpmc-nand.c +++ b/arch/arm/mach-omap2/gpmc-nand.c @@ -123,11 +123,13 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, } } - if (gpmc_nand_data->of_node) + if (gpmc_nand_data->of_node) { gpmc_read_settings_dt(gpmc_nand_data->of_node, &s); - else + if (s.wait_on_read && s.wait_on_write) + gpmc_nand_data->dev_ready = "true"; + } else { gpmc_set_legacy(gpmc_nand_data, &s); - + } s.device_nand = true; err = gpmc_cs_program_settings(gpmc_nand_data->cs, &s);
This patch enables 'wait-pin' monitoring in NAND driver if following properties are present under NAND DT node gpmc,wait-pin = <wait-pin number> gpmc,wait-on-read; gpmc,wait-on-write; As NAND generic framework uses common path nand_chip->dev_ready() for monitoring completion of Read and Write status, so wait-pin monitoring is enabled only when both 'gpmc,wait-on-read' and 'gpmc,wait-on-write' are specified. Signed-off-by: Pekon Gupta <pekon@ti.com> --- arch/arm/mach-omap2/gpmc-nand.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)