| Message ID | 1361899842-30303-7-git-send-email-jon-hunter@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 26, 2013 at 23:00:33, Hunter, Jon wrote: > Convert the OMAP2+ NAND code to use the gpmc_cs_program_settings() > function for configuring the various GPMC options instead of directly > programming the CONFIG1 register. > > This moves the configuration of some GPMC options outside the > nand_gpmc_retime() because these options should only need to be set once > regardless of whether the gpmc timing is changing dynamically at runtime. > The programming of where the wait-pin is also moved slightly, but this > will not have any impact to existing devices as no boards are currently > setting the dev_ready variable. > > Signed-off-by: Jon Hunter <jon-hunter@ti.com> > --- > arch/arm/mach-omap2/gpmc-nand.c | 35 +++++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 12 deletions(-) > > diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c > index afc1e8c..4bdfea2 100644 > --- a/arch/arm/mach-omap2/gpmc-nand.c > +++ b/arch/arm/mach-omap2/gpmc-nand.c > @@ -43,6 +43,10 @@ static struct platform_device gpmc_nand_device = { > .resource = gpmc_nand_resource, > }; > > +static struct gpmc_settings nand_settings = { > + .device_nand = true, > +}; > + Is it possible to make it local variable? It would help GPMC to support NAND device on multiple chip select. Thanks Avinash ... > @@ -140,11 +136,26 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, > dev_err(dev, "Unable to set gpmc timings: %d\n", err); > return err; > } > - } > > - /* Enable RD PIN Monitoring Reg */ > - if (gpmc_nand_data->dev_ready) { > - gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_RDY_BSY, 1); > + /* Enable RD PIN Monitoring Reg */ > + if (gpmc_nand_data->dev_ready) { > + nand_settings.wait_on_read = true; > + nand_settings.wait_on_write = true; > + } > + > + if (gpmc_nand_data->devsize == NAND_BUSWIDTH_16) > + nand_settings.device_width = GPMC_DEVWIDTH_16BIT; > + else > + nand_settings.device_width = GPMC_DEVWIDTH_8BIT; > + > + err = gpmc_cs_program_settings(gpmc_nand_data->cs, > + &nand_settings); > + if (IS_ERR_VALUE(err)) > + goto out_free_cs; > + > + err = gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_WP, 0); > + if (IS_ERR_VALUE(err)) > + goto out_free_cs; > } > > gpmc_update_nand_reg(&gpmc_nand_data->reg, gpmc_nand_data->cs); > -- > 1.7.10.4 > > -- > 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 >
On 02/28/2013 04:38 AM, Philip, Avinash wrote: > On Tue, Feb 26, 2013 at 23:00:33, Hunter, Jon wrote: >> Convert the OMAP2+ NAND code to use the gpmc_cs_program_settings() >> function for configuring the various GPMC options instead of directly >> programming the CONFIG1 register. >> >> This moves the configuration of some GPMC options outside the >> nand_gpmc_retime() because these options should only need to be set once >> regardless of whether the gpmc timing is changing dynamically at runtime. >> The programming of where the wait-pin is also moved slightly, but this >> will not have any impact to existing devices as no boards are currently >> setting the dev_ready variable. >> >> Signed-off-by: Jon Hunter <jon-hunter@ti.com> >> --- >> arch/arm/mach-omap2/gpmc-nand.c | 35 +++++++++++++++++++++++------------ >> 1 file changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c >> index afc1e8c..4bdfea2 100644 >> --- a/arch/arm/mach-omap2/gpmc-nand.c >> +++ b/arch/arm/mach-omap2/gpmc-nand.c >> @@ -43,6 +43,10 @@ static struct platform_device gpmc_nand_device = { >> .resource = gpmc_nand_resource, >> }; >> >> +static struct gpmc_settings nand_settings = { >> + .device_nand = true, >> +}; >> + > > Is it possible to make it local variable? > It would help GPMC to support NAND device on multiple chip select. Well gpmc_nand_init() will be called for each NAND device and so I don't see why this would prevent supporting multiple NANDs on multiple chip-selects. Once migration to device-tree is complete we could definitely make it local as there will be no need for any static initialisations of the structure as all fields would be read from device-tree. I can make it local now if that is preferred and seeing that will be the direction once we have migrated to device-tree, is does make sense. Cheers Jon
On Thu, Feb 28, 2013 at 21:32:01, Hunter, Jon wrote: > > On 02/28/2013 04:38 AM, Philip, Avinash wrote: > > On Tue, Feb 26, 2013 at 23:00:33, Hunter, Jon wrote: > >> Convert the OMAP2+ NAND code to use the gpmc_cs_program_settings() > >> function for configuring the various GPMC options instead of directly > >> programming the CONFIG1 register. > >> > >> This moves the configuration of some GPMC options outside the > >> nand_gpmc_retime() because these options should only need to be set once > >> regardless of whether the gpmc timing is changing dynamically at runtime. > >> The programming of where the wait-pin is also moved slightly, but this > >> will not have any impact to existing devices as no boards are currently > >> setting the dev_ready variable. > >> > >> Signed-off-by: Jon Hunter <jon-hunter@ti.com> > >> --- > >> arch/arm/mach-omap2/gpmc-nand.c | 35 +++++++++++++++++++++++------------ > >> 1 file changed, 23 insertions(+), 12 deletions(-) > >> > >> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c > >> index afc1e8c..4bdfea2 100644 > >> --- a/arch/arm/mach-omap2/gpmc-nand.c > >> +++ b/arch/arm/mach-omap2/gpmc-nand.c > >> @@ -43,6 +43,10 @@ static struct platform_device gpmc_nand_device = { > >> .resource = gpmc_nand_resource, > >> }; > >> > >> +static struct gpmc_settings nand_settings = { > >> + .device_nand = true, > >> +}; > >> + > > > > Is it possible to make it local variable? > > It would help GPMC to support NAND device on multiple chip select. > > Well gpmc_nand_init() will be called for each NAND device and so I don't > see why this would prevent supporting multiple NANDs on multiple > chip-selects. Problem exactly lies here. Here platform device initialized as statically. So multiple requests will points to same platform device. Recently this issue raised in TI e2e forum and solution being proposed. Details can found from http://e2e.ti.com/support/arm/sitara_arm/f/791/p/246997/865379.aspx#865379 I doubt the same is the case for other GPMC client devices also. > > Once migration to device-tree is complete we could definitely make it > local as there will be no need for any static initialisations of the > structure as all fields would be read from device-tree. > > I can make it local now if that is preferred and seeing that will be the > direction once we have migrated to device-tree, is does make sense. As far as I understood, here usage of local variable won't break anything? If that is the case, better solution would be usage of local variable. Thanks Avinash > > Cheers > Jon >
On 02/28/2013 11:40 PM, Philip, Avinash wrote: > On Thu, Feb 28, 2013 at 21:32:01, Hunter, Jon wrote: >> >> On 02/28/2013 04:38 AM, Philip, Avinash wrote: >>> On Tue, Feb 26, 2013 at 23:00:33, Hunter, Jon wrote: >>>> Convert the OMAP2+ NAND code to use the gpmc_cs_program_settings() >>>> function for configuring the various GPMC options instead of directly >>>> programming the CONFIG1 register. >>>> >>>> This moves the configuration of some GPMC options outside the >>>> nand_gpmc_retime() because these options should only need to be set once >>>> regardless of whether the gpmc timing is changing dynamically at runtime. >>>> The programming of where the wait-pin is also moved slightly, but this >>>> will not have any impact to existing devices as no boards are currently >>>> setting the dev_ready variable. >>>> >>>> Signed-off-by: Jon Hunter <jon-hunter@ti.com> >>>> --- >>>> arch/arm/mach-omap2/gpmc-nand.c | 35 +++++++++++++++++++++++------------ >>>> 1 file changed, 23 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c >>>> index afc1e8c..4bdfea2 100644 >>>> --- a/arch/arm/mach-omap2/gpmc-nand.c >>>> +++ b/arch/arm/mach-omap2/gpmc-nand.c >>>> @@ -43,6 +43,10 @@ static struct platform_device gpmc_nand_device = { >>>> .resource = gpmc_nand_resource, >>>> }; >>>> >>>> +static struct gpmc_settings nand_settings = { >>>> + .device_nand = true, >>>> +}; >>>> + >>> >>> Is it possible to make it local variable? >>> It would help GPMC to support NAND device on multiple chip select. >> >> Well gpmc_nand_init() will be called for each NAND device and so I don't >> see why this would prevent supporting multiple NANDs on multiple >> chip-selects. > > Problem exactly lies here. Here platform device initialized as statically. > So multiple requests will points to same platform device. > > Recently this issue raised in TI e2e forum and solution being proposed. > Details can found from > http://e2e.ti.com/support/arm/sitara_arm/f/791/p/246997/865379.aspx#865379 > > I doubt the same is the case for other GPMC client devices also. This structure is only passed to gpmc_cs_program_settings() and not stored in the platform device structure. So I don't think that this is the same problem. Look at what the code is doing with the nand_settings structure. >> Once migration to device-tree is complete we could definitely make it >> local as there will be no need for any static initialisations of the >> structure as all fields would be read from device-tree. >> >> I can make it local now if that is preferred and seeing that will be the >> direction once we have migrated to device-tree, is does make sense. > > As far as I understood, here usage of local variable won't break anything? > If that is the case, better solution would be usage of local variable. It will not. So yes we can. Jon
diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c index afc1e8c..4bdfea2 100644 --- a/arch/arm/mach-omap2/gpmc-nand.c +++ b/arch/arm/mach-omap2/gpmc-nand.c @@ -43,6 +43,10 @@ static struct platform_device gpmc_nand_device = { .resource = gpmc_nand_resource, }; +static struct gpmc_settings nand_settings = { + .device_nand = true, +}; + static int omap2_nand_gpmc_retime( struct omap_nand_platform_data *gpmc_nand_data, struct gpmc_timings *gpmc_t) @@ -74,14 +78,6 @@ static int omap2_nand_gpmc_retime( t.cs_wr_off = gpmc_t->cs_wr_off; t.wr_cycle = gpmc_t->wr_cycle; - /* Configure GPMC */ - if (gpmc_nand_data->devsize == NAND_BUSWIDTH_16) - gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_DEV_SIZE, 1); - else - gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_DEV_SIZE, 0); - gpmc_cs_configure(gpmc_nand_data->cs, - GPMC_CONFIG_DEV_TYPE, GPMC_DEVICETYPE_NAND); - gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_WP, 0); err = gpmc_cs_set_timings(gpmc_nand_data->cs, &t); if (err) return err; @@ -140,11 +136,26 @@ int gpmc_nand_init(struct omap_nand_platform_data *gpmc_nand_data, dev_err(dev, "Unable to set gpmc timings: %d\n", err); return err; } - } - /* Enable RD PIN Monitoring Reg */ - if (gpmc_nand_data->dev_ready) { - gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_RDY_BSY, 1); + /* Enable RD PIN Monitoring Reg */ + if (gpmc_nand_data->dev_ready) { + nand_settings.wait_on_read = true; + nand_settings.wait_on_write = true; + } + + if (gpmc_nand_data->devsize == NAND_BUSWIDTH_16) + nand_settings.device_width = GPMC_DEVWIDTH_16BIT; + else + nand_settings.device_width = GPMC_DEVWIDTH_8BIT; + + err = gpmc_cs_program_settings(gpmc_nand_data->cs, + &nand_settings); + if (IS_ERR_VALUE(err)) + goto out_free_cs; + + err = gpmc_cs_configure(gpmc_nand_data->cs, GPMC_CONFIG_WP, 0); + if (IS_ERR_VALUE(err)) + goto out_free_cs; } gpmc_update_nand_reg(&gpmc_nand_data->reg, gpmc_nand_data->cs);
Convert the OMAP2+ NAND code to use the gpmc_cs_program_settings() function for configuring the various GPMC options instead of directly programming the CONFIG1 register. This moves the configuration of some GPMC options outside the nand_gpmc_retime() because these options should only need to be set once regardless of whether the gpmc timing is changing dynamically at runtime. The programming of where the wait-pin is also moved slightly, but this will not have any impact to existing devices as no boards are currently setting the dev_ready variable. Signed-off-by: Jon Hunter <jon-hunter@ti.com> --- arch/arm/mach-omap2/gpmc-nand.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-)