RE: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Jon, On Thu, Jun 14, 2012 at 23:23:48, Hunter, Jon wrote: > On 06/14/2012 12:40 AM, Mohammed, Afzal wrote: > > During gpmc driver probe, it will configure all the connected peripherals, > > if configuration details are not present at that point of time, gpmc driver > > will cry out saying that configuration & timings has not been configured, > > (please see holler if no configuration patch). > > Sorry, I am not sure if I am missing something here, but isn't the > chip-select requested during the gpmc probe? If so then we should not be > programming the gpmc registers at all until the chip-select has been > allocated. Hence, after the probe seems more appropriate. This patch by itself has no much meaning other than preparing for driver conversion so that driver conversion series would be simpler, if you read it with gpmc driver conversion series and adapting peripherals to gpmc driver series, hopefully you can make out what I am trying to express with gpmc driver, first chip select request happens, then programming gpmc registers happen, but note that both happen in probe > >> I am not convinced we need to. Furthermore with your change you do not > >> actually set async mode in the onenand until _set_sync() is called. > > > > Yes, setting async mode in onenand is done in set_sync function, and it is > > always called by onenand driver indirectly. > > > > Seems if setting async mode in onenand is taken out of set_sync & placed > > it before set_sync invocation in gpmc_onenand_setup, intention will be > > clear, right ? (even though sequence wise same thing is happening now) > > Exactly. Ok, v2 will have this change > >> Yes but as far as I can see, it seems that this is the intent of the > >> onenand_setup() function to perform the necessary initialisation. > > > > I believe doing it in gpmc_onenand_init is better, due to the reasons > > mentioned above as well as because function name will correspond to what > > it is doing, i.e. initialization > > If the chip-select is allocating during the probe, then I don't agree. I believe you were referring to gpmc driver probe, not onenand probe, the changes has been done such that both old and new driver interface can make use of most of existing code. And only old interface will call gpmc_onenand_init, and with that gpmc driver interface is not coming into picture, while with new gpmc driver interface, function used would be different one as in [1] > > Yes, on omap3evm, with help of local patches (as mainline doesn't have > > those). It was mentioned in cover letter of gpmc driver conversion series > > Great. Sorry but with 20+ patch spread across 3 series it is not always > easy to find the details. So ideally it should be mentioned in this > cover letter too. I am sorry for that Regards Afzal [1] http://www.mail-archive.com/linux-omap@vger.kernel.org/msg69919.html -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Afzal, On 06/14/2012 12:40 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Wed, Jun 13, 2012 at 22:08:47, Hunter, Jon wrote: >> On 06/13/2012 12:03 AM, Mohammed, Afzal wrote: > >>> As gpmc_onenand_setup is a callback by onenand driver, we would have >>> lost the opportunity to configure onenand before driver is probed. >> >> Is that a problem? Looks like it is called early in the probe and so I >> would hope no one is attempting to access the onenand itself before the >> probe has completed. > > During gpmc driver probe, it will configure all the connected peripherals, > if configuration details are not present at that point of time, gpmc driver > will cry out saying that configuration & timings has not been configured, > (please see holler if no configuration patch). Sorry, I am not sure if I am missing something here, but isn't the chip-select requested during the gpmc probe? If so then we should not be programming the gpmc registers at all until the chip-select has been allocated. Hence, after the probe seems more appropriate. > And I do not see any reason > why gpmc driver should not be fed with async mode configuration initially, > as it has to be done always. It is more of where you are doing it. I am not against putting in async mode to begin with. >> >>> This would cause requirement of double GPMC configuring and we lost >>> the opportunity to configure GPMC before driver is probed. >> >> I am not convinced we need to. Furthermore with your change you do not >> actually set async mode in the onenand until _set_sync() is called. > > Yes, setting async mode in onenand is done in set_sync function, and it is > always called by onenand driver indirectly. > > Seems if setting async mode in onenand is taken out of set_sync & placed > it before set_sync invocation in gpmc_onenand_setup, intention will be > clear, right ? (even though sequence wise same thing is happening now) Exactly. >> >>> And the first step for onenand configuration is always to set it >>> to async mode (with the way it is done now), so it seems reasonable >>> to rely on normal GPMC configuration for async & then do reconfigure >>> for sync. >> >> Yes but as far as I can see, it seems that this is the intent of the >> onenand_setup() function to perform the necessary initialisation. > > I believe doing it in gpmc_onenand_init is better, due to the reasons > mentioned above as well as because function name will correspond to what > it is doing, i.e. initialization If the chip-select is allocating during the probe, then I don't agree. >> Have you tested onenand? Do you have a board with onenand? > > Yes, on omap3evm, with help of local patches (as mainline doesn't have > those). It was mentioned in cover letter of gpmc driver conversion series Great. Sorry but with 20+ patch spread across 3 series it is not always easy to find the details. So ideally it should be mentioned in this cover letter too. Thanks Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Jon, On Wed, Jun 13, 2012 at 22:08:47, Hunter, Jon wrote: > On 06/13/2012 12:03 AM, Mohammed, Afzal wrote: > > As gpmc_onenand_setup is a callback by onenand driver, we would have > > lost the opportunity to configure onenand before driver is probed. > > Is that a problem? Looks like it is called early in the probe and so I > would hope no one is attempting to access the onenand itself before the > probe has completed. During gpmc driver probe, it will configure all the connected peripherals, if configuration details are not present at that point of time, gpmc driver will cry out saying that configuration & timings has not been configured, (please see holler if no configuration patch). And I do not see any reason why gpmc driver should not be fed with async mode configuration initially, as it has to be done always. > > > This would cause requirement of double GPMC configuring and we lost > > the opportunity to configure GPMC before driver is probed. > > I am not convinced we need to. Furthermore with your change you do not > actually set async mode in the onenand until _set_sync() is called. Yes, setting async mode in onenand is done in set_sync function, and it is always called by onenand driver indirectly. Seems if setting async mode in onenand is taken out of set_sync & placed it before set_sync invocation in gpmc_onenand_setup, intention will be clear, right ? (even though sequence wise same thing is happening now) > > > And the first step for onenand configuration is always to set it > > to async mode (with the way it is done now), so it seems reasonable > > to rely on normal GPMC configuration for async & then do reconfigure > > for sync. > > Yes but as far as I can see, it seems that this is the intent of the > onenand_setup() function to perform the necessary initialisation. I believe doing it in gpmc_onenand_init is better, due to the reasons mentioned above as well as because function name will correspond to what it is doing, i.e. initialization > > Have you tested onenand? Do you have a board with onenand? Yes, on omap3evm, with help of local patches (as mainline doesn't have those). It was mentioned in cover letter of gpmc driver conversion series Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Afzal, On 06/13/2012 12:03 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 23:00:48, Hunter, Jon wrote: > >> On 06/12/2012 01:16 AM, Mohammed, Afzal wrote: >>> With the existing code, set_async was done as part of set_sync, hence >>> requiring GPMC to be configured twice after driver takes control, with >>> your suggestion too, GPMC would have to be configured twice. >> >> I am just suggesting that you place the call to set_async_mode in the >> gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the >> calls from set_sync (like you have done). So I don't see that these >> would configure the GPMC twice. > > As gpmc_onenand_setup is a callback by onenand driver, we would have > lost the opportunity to configure onenand before driver is probed. Is that a problem? Looks like it is called early in the probe and so I would hope no one is attempting to access the onenand itself before the probe has completed. > This would cause requirement of double GPMC configuring and we lost > the opportunity to configure GPMC before driver is probed. I am not convinced we need to. Furthermore with your change you do not actually set async mode in the onenand until _set_sync() is called. > And the first step for onenand configuration is always to set it > to async mode (with the way it is done now), so it seems reasonable > to rely on normal GPMC configuration for async & then do reconfigure > for sync. Yes but as far as I can see, it seems that this is the intent of the onenand_setup() function to perform the necessary initialisation. Have you tested onenand? Do you have a board with onenand? Cheers Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Jon, On Tue, Jun 12, 2012 at 23:00:48, Hunter, Jon wrote: > On 06/12/2012 01:16 AM, Mohammed, Afzal wrote: > > With the existing code, set_async was done as part of set_sync, hence > > requiring GPMC to be configured twice after driver takes control, with > > your suggestion too, GPMC would have to be configured twice. > > I am just suggesting that you place the call to set_async_mode in the > gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the > calls from set_sync (like you have done). So I don't see that these > would configure the GPMC twice. As gpmc_onenand_setup is a callback by onenand driver, we would have lost the opportunity to configure onenand before driver is probed. This would cause requirement of double GPMC configuring and we lost the opportunity to configure GPMC before driver is probed. And the first step for onenand configuration is always to set it to async mode (with the way it is done now), so it seems reasonable to rely on normal GPMC configuration for async & then do reconfigure for sync. Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
On 06/12/2012 01:16 AM, Mohammed, Afzal wrote: > Hi Jon, > > On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote: > >> I agree with getting rid of the first instance at the beginning of >> _set_async_mode, but why get rid of the above one? Are you assuming that >> by default it is in async mode? Could be nice to keep it to be explicit. > > Second one is achieved below Hmmm ... it seems odd to put the commands for setting async in the set sync. I see what you are doing but surely someone should call set async and the set sync. >>> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct >>> omap_onenand_platform_data *cfg, >>> u32 reg; >>> bool clk_dep = false; >>> >>> + /* Ensure sync read and sync write are disabled */ >>> + reg = readw(onenand_base + ONENAND_REG_SYS_CFG1); >>> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; >>> + writew(reg, onenand_base + ONENAND_REG_SYS_CFG1); >>> + >> >> Should we only set these after we have checked the flags and depending >> on which flags are set? > > Even for sync mode, setting async mode initially is required > >>> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct >>> omap_onenand_platform_data *_onenand_data) >>> gpmc_onenand_resource.end = gpmc_onenand_resource.start + >>> ONENAND_IO_SIZE - 1; >>> >>> + omap2_onenand_set_async_mode(gpmc_onenand_data->cs); >>> + >> >> What about putting this in the gpmc_onenand_setup() function before the >> call to _set_sync_mode? Seems nice to have these together. > > Intention of this patch is to setup GPMC async mode before driver starts > its job so that reconfiguring GPMC needs to be to be done only once (this > helps in avoiding issues when it has to work with gpmc driver). > > With the existing code, set_async was done as part of set_sync, hence > requiring GPMC to be configured twice after driver takes control, with > your suggestion too, GPMC would have to be configured twice. I am just suggesting that you place the call to set_async_mode in the gpmc_onenand_setup() instead of the gpmc_onenand_init() and remove the calls from set_sync (like you have done). So I don't see that these would configure the GPMC twice. Jon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Jon, On Tue, Jun 12, 2012 at 00:06:30, Hunter, Jon wrote: > I agree with getting rid of the first instance at the beginning of > _set_async_mode, but why get rid of the above one? Are you assuming that > by default it is in async mode? Could be nice to keep it to be explicit. Second one is achieved below > > @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct > > omap_onenand_platform_data *cfg, > > u32 reg; > > bool clk_dep = false; > > > > + /* Ensure sync read and sync write are disabled */ > > + reg = readw(onenand_base + ONENAND_REG_SYS_CFG1); > > + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE; > > + writew(reg, onenand_base + ONENAND_REG_SYS_CFG1); > > + > > Should we only set these after we have checked the flags and depending > on which flags are set? Even for sync mode, setting async mode initially is required > > @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct > > omap_onenand_platform_data *_onenand_data) > > gpmc_onenand_resource.end = gpmc_onenand_resource.start + > > ONENAND_IO_SIZE - 1; > > > > + omap2_onenand_set_async_mode(gpmc_onenand_data->cs); > > + > > What about putting this in the gpmc_onenand_setup() function before the > call to _set_sync_mode? Seems nice to have these together. Intention of this patch is to setup GPMC async mode before driver starts its job so that reconfiguring GPMC needs to be to be done only once (this helps in avoiding issues when it has to work with gpmc driver). With the existing code, set_async was done as part of set_sync, hence requiring GPMC to be configured twice after driver takes control, with your suggestion too, GPMC would have to be configured twice. Regards Afzal -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/3] ARM: OMAP2+: onenand: cleanup for gpmc driver conversion
Hi Afzal,
On 06/11/2012 09:01 AM, Afzal Mohammed wrote:
> Reorganize gpmc-onenand initialization so that changes
> required for gpmc driver migration can be made smooth.
>
> Ensuring sync read/write are disabled in onenand cannot be
> expect to work properly unless GPMC is setup, this has been
> removed.
So I think I see what you are saying, but the above is not 100% clear. I
think that what you are saying is that omap2_onenand_set_async_mode() is
attempting to configure the onenand registers before configuring the
gpmc interface and hence this is not correct.
> Two instances of doing the same has been clubbed
> into one as onenand driver always calls indirectly
> "omap2_onenand_set_sync_mode", and has placed such that
> configuring onenand for async read/write would happen once
> GPMC is setup for async mode (which happens even for sync
> mode, before configuring to sync mode).
It may be clearer to say that _set_sync_mode is always called during
onenand setup and this will call _set_async_mode. So instead of calling
_set_async_mode from _set_sync_mode, just call it directly during the
gpmc_onenand_init().
> Signed-off-by: Afzal Mohammed
> ---
> arch/arm/mach-omap2/gpmc-onenand.c | 24 +---
> 1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/gpmc-onenand.c
> b/arch/arm/mach-omap2/gpmc-onenand.c
> index 71d7c07..fd4c48d 100644
> --- a/arch/arm/mach-omap2/gpmc-onenand.c
> +++ b/arch/arm/mach-omap2/gpmc-onenand.c
> @@ -38,10 +38,9 @@ static struct platform_device gpmc_onenand_device = {
> .resource = &gpmc_onenand_resource,
> };
>
> -static int omap2_onenand_set_async_mode(int cs, void __iomem *onenand_base)
> +static int omap2_onenand_set_async_mode(int cs)
> {
> struct gpmc_timings t;
> - u32 reg;
> int err;
>
> const int t_cer = 15;
> @@ -55,11 +54,6 @@ static int omap2_onenand_set_async_mode(int cs, void
> __iomem *onenand_base)
> const int t_wpl = 40;
> const int t_wph = 30;
>
> - /* Ensure sync read and sync write are disabled */
> - reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
> - reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
> - writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
> -
> memset(&t, 0, sizeof(t));
> t.sync_clk = 0;
> t.cs_on = 0;
> @@ -95,10 +89,6 @@ static int omap2_onenand_set_async_mode(int cs, void
> __iomem *onenand_base)
> if (err)
> return err;
>
> - /* Ensure sync read and sync write are disabled */
> - reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
> - reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
> - writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
I agree with getting rid of the first instance at the beginning of
_set_async_mode, but why get rid of the above one? Are you assuming that
by default it is in async mode? Could be nice to keep it to be explicit.
> return 0;
> }
> @@ -191,19 +181,21 @@ static int omap2_onenand_set_sync_mode(struct
> omap_onenand_platform_data *cfg,
> u32 reg;
> bool clk_dep = false;
>
> + /* Ensure sync read and sync write are disabled */
> + reg = readw(onenand_base + ONENAND_REG_SYS_CFG1);
> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
> + writew(reg, onenand_base + ONENAND_REG_SYS_CFG1);
> +
Should we only set these after we have checked the flags and depending
on which flags are set?
> if (cfg->flags & ONENAND_SYNC_READ) {
> sync_read = 1;
> } else if (cfg->flags & ONENAND_SYNC_READWRITE) {
> sync_read = 1;
> sync_write = 1;
> } else
> - return omap2_onenand_set_async_mode(cs, onenand_base);
> + return 0;
>
> if (!freq) {
> /* Very first call freq is not known */
> - err = omap2_onenand_set_async_mode(cs, onenand_base);
> - if (err)
> - return err;
> freq = omap2_onenand_get_freq(cfg, onenand_base, &clk_dep);
> first_time = 1;
> }
> @@ -421,6 +413,8 @@ void __init gpmc_onenand_init(struct
> omap_onenand_platform_data *_onenand_data)
> gpmc_onenand_resource.end = gpmc_onenand_resource.start +
> ONENAND_IO_SIZE - 1;
>
> + omap2_onenand_set_async_mode(gpmc_onenand_data->cs);
> +
What about putting this in the gpmc_onenand_setup() function before the
call to _set_sync_mode? Seems nice to have these together.
Cheers
Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
