[PATCH] OMAP: fix gpmc nand setup when no timings supplied

[PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Mike Rapoport]

The gpmc nand infrastructure crashes when there no timing structure
supplied in the omap_nand_platform_data. Adding check for
gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows to
continue nand initialization without modifying gpmc timings.

Signed-off-by: Mike Rapoport <mike@compulab.co.il>
---
 arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
index 64d74f0..3629da3 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
 {
 	struct device *dev = &gpmc_nand_device.dev;
 
+	if (!gpmc_nand_data->gpmc_t) {
+		dev_info(dev, "Keeping gpmc timings\n");
+		return 0;
+	}
+
 	/* Set timings in GPMC */
 	if (omap2_nand_gpmc_retime() < 0) {
 		dev_err(dev, "Unable to set gpmc timings\n");
-- 
1.6.6.2

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Mike Rapoport]

Any comments on this?

Mike Rapoport wrote:
> The gpmc nand infrastructure crashes when there no timing structure
> supplied in the omap_nand_platform_data. Adding check for
> gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows to
> continue nand initialization without modifying gpmc timings.
> 
> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> ---
>  arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
>  1 files changed, 5 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-nand.c
> index 64d74f0..3629da3 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
>  {
>  	struct device *dev = &gpmc_nand_device.dev;
>  
> +	if (!gpmc_nand_data->gpmc_t) {
> +		dev_info(dev, "Keeping gpmc timings\n");
> +		return 0;
> +	}
> +
>  	/* Set timings in GPMC */
>  	if (omap2_nand_gpmc_retime() < 0) {
>  		dev_err(dev, "Unable to set gpmc timings\n");


-- 
Sincerely yours,
Mike.

RE: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Ghorai, Sukumar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> owner@vger.kernel.org] On Behalf Of Mike Rapoport
> Sent: 2010-04-22 10:50
> To: linux-omap@vger.kernel.org
> Cc: Tony Lindgren; Mike Rapoport
> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
> 
> Any comments on this?
> 
> Mike Rapoport wrote:
> > The gpmc nand infrastructure crashes when there no timing structure
> > supplied in the omap_nand_platform_data. Adding check for
> > gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows to
> > continue nand initialization without modifying gpmc timings.
> >
> > Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> > ---
> >  arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
> >  1 files changed, 5 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-
> nand.c
> > index 64d74f0..3629da3 100644
> > --- a/arch/arm/mach-omap2/gpmc-nand.c
> > +++ b/arch/arm/mach-omap2/gpmc-nand.c
> > @@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
> >  {
> >  	struct device *dev = &gpmc_nand_device.dev;
> >
> > +	if (!gpmc_nand_data->gpmc_t) {
> > +		dev_info(dev, "Keeping gpmc timings\n");
> > +		return 0;
> > +	}
[Ghorai] This is the only time its setup the gpmc timings for NAND. And it should return as error.

> > +
> >  	/* Set timings in GPMC */
> >  	if (omap2_nand_gpmc_retime() < 0) {
> >  		dev_err(dev, "Unable to set gpmc timings\n");
> 
> 
> --
> Sincerely yours,
> Mike.
> --
> 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

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Mike Rapoport]

Ghorai, Sukumar wrote:
> 
>> -----Original Message-----
>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>> owner@vger.kernel.org] On Behalf Of Mike Rapoport
>> Sent: 2010-04-22 10:50
>> To: linux-omap@vger.kernel.org
>> Cc: Tony Lindgren; Mike Rapoport
>> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
>>
>> Any comments on this?
>>
>> Mike Rapoport wrote:
>>> The gpmc nand infrastructure crashes when there no timing structure
>>> supplied in the omap_nand_platform_data. Adding check for
>>> gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows to
>>> continue nand initialization without modifying gpmc timings.
>>>
>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>> ---
>>>  arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-omap2/gpmc-
>> nand.c
>>> index 64d74f0..3629da3 100644
>>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>>> @@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
>>>  {
>>>  	struct device *dev = &gpmc_nand_device.dev;
>>>
>>> +	if (!gpmc_nand_data->gpmc_t) {
>>> +		dev_info(dev, "Keeping gpmc timings\n");
>>> +		return 0;
>>> +	}
> [Ghorai] This is the only time its setup the gpmc timings for NAND. And it should return as error.

Somehow I was under the impression that X-Loader sets up the NAND 
timings even if booted from OneNAND or MMC. Apparently I'm wrong :).

What do you think about adding a flag to omap_nand_platform_data that 
will allow to keep timings if they were already configured by the 
bootloader? This is really useful when the board can be assembled with 
different kinds of NAND flashes.

>>> +
>>>  	/* Set timings in GPMC */
>>>  	if (omap2_nand_gpmc_retime() < 0) {
>>>  		dev_err(dev, "Unable to set gpmc timings\n");
>>
>> --
>> Sincerely yours,
>> Mike.
>> --
>> 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


-- 
Sincerely yours,
Mike.

RE: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Ghorai, Sukumar]

> -----Original Message-----
> From: Mike Rapoport [mailto:mike@compulab.co.il]
> Sent: 2010-04-22 12:29
> To: Ghorai, Sukumar
> Cc: linux-omap@vger.kernel.org; Tony Lindgren
> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
> 
> Ghorai, Sukumar wrote:
> >
> >> -----Original Message-----
> >> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
> >> owner@vger.kernel.org] On Behalf Of Mike Rapoport
> >> Sent: 2010-04-22 10:50
> >> To: linux-omap@vger.kernel.org
> >> Cc: Tony Lindgren; Mike Rapoport
> >> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
> >>
> >> Any comments on this?
> >>
> >> Mike Rapoport wrote:
> >>> The gpmc nand infrastructure crashes when there no timing structure
> >>> supplied in the omap_nand_platform_data. Adding check for
> >>> gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows
> to
> >>> continue nand initialization without modifying gpmc timings.
> >>>
> >>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
> >>> ---
> >>>  arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
> >>>  1 files changed, 5 insertions(+), 0 deletions(-)
> >>>
> >>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-
> omap2/gpmc-
> >> nand.c
> >>> index 64d74f0..3629da3 100644
> >>> --- a/arch/arm/mach-omap2/gpmc-nand.c
> >>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> >>> @@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
> >>>  {
> >>>  	struct device *dev = &gpmc_nand_device.dev;
> >>>
> >>> +	if (!gpmc_nand_data->gpmc_t) {
> >>> +		dev_info(dev, "Keeping gpmc timings\n");
> >>> +		return 0;
> >>> +	}
> > [Ghorai] This is the only time its setup the gpmc timings for NAND. And
> it should return as error.
> 
> Somehow I was under the impression that X-Loader sets up the NAND
> timings even if booted from OneNAND or MMC. Apparently I'm wrong :).
> 
> What do you think about adding a flag to omap_nand_platform_data that
> will allow to keep timings if they were already configured by the
> bootloader? This is really useful when the board can be assembled with
> different kinds of NAND flashes.
[Ghorai] firstly we should not depend on u-boot/ x-loader for this GPMC configuration. I think each board will have only one device finally. 
> 
> >>> +
> >>>  	/* Set timings in GPMC */
> >>>  	if (omap2_nand_gpmc_retime() < 0) {
> >>>  		dev_err(dev, "Unable to set gpmc timings\n");
> >>
> >> --
> >> Sincerely yours,
> >> Mike.
> >> --
> >> 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
> 
> 
> --
> Sincerely yours,
> Mike.

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Mike Rapoport]

Ghorai, Sukumar wrote:
> 
>> -----Original Message-----
>> From: Mike Rapoport [mailto:mike@compulab.co.il]
>> Sent: 2010-04-22 12:29
>> To: Ghorai, Sukumar
>> Cc: linux-omap@vger.kernel.org; Tony Lindgren
>> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
>>
>> Ghorai, Sukumar wrote:
>>>> -----Original Message-----
>>>> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-
>>>> owner@vger.kernel.org] On Behalf Of Mike Rapoport
>>>> Sent: 2010-04-22 10:50
>>>> To: linux-omap@vger.kernel.org
>>>> Cc: Tony Lindgren; Mike Rapoport
>>>> Subject: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied
>>>>
>>>> Any comments on this?
>>>>
>>>> Mike Rapoport wrote:
>>>>> The gpmc nand infrastructure crashes when there no timing structure
>>>>> supplied in the omap_nand_platform_data. Adding check for
>>>>> gpmc_nand_data->gpmc_t pointer validity resolves the crash and allows
>> to
>>>>> continue nand initialization without modifying gpmc timings.
>>>>>
>>>>> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
>>>>> ---
>>>>>  arch/arm/mach-omap2/gpmc-nand.c |    5 +++++
>>>>>  1 files changed, 5 insertions(+), 0 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-omap2/gpmc-nand.c b/arch/arm/mach-
>> omap2/gpmc-
>>>> nand.c
>>>>> index 64d74f0..3629da3 100644
>>>>> --- a/arch/arm/mach-omap2/gpmc-nand.c
>>>>> +++ b/arch/arm/mach-omap2/gpmc-nand.c
>>>>> @@ -83,6 +83,11 @@ static int gpmc_nand_setup(void)
>>>>>  {
>>>>>  	struct device *dev = &gpmc_nand_device.dev;
>>>>>
>>>>> +	if (!gpmc_nand_data->gpmc_t) {
>>>>> +		dev_info(dev, "Keeping gpmc timings\n");
>>>>> +		return 0;
>>>>> +	}
>>> [Ghorai] This is the only time its setup the gpmc timings for NAND. And
>> it should return as error.
>>
>> Somehow I was under the impression that X-Loader sets up the NAND
>> timings even if booted from OneNAND or MMC. Apparently I'm wrong :).
>>
>> What do you think about adding a flag to omap_nand_platform_data that
>> will allow to keep timings if they were already configured by the
>> bootloader? This is really useful when the board can be assembled with
>> different kinds of NAND flashes.
> [Ghorai] firstly we should not depend on u-boot/ x-loader for this GPMC configuration.
 > I think each board will have only one device finally.

CM-T35, for instance can be assembled with different NAND flash chips. 
Besides, boards that use NAND as primary boot device, we anyway depend 
on proper GPMC configuration in the bootloader chain. Having ability to 
define GPMC timings in the kernel and keep the settings made by the 
bootloader adds flexibility level for board designers.

>>>>> +
>>>>>  	/* Set timings in GPMC */
>>>>>  	if (omap2_nand_gpmc_retime() < 0) {
>>>>>  		dev_err(dev, "Unable to set gpmc timings\n");
>>>> --
>>>> Sincerely yours,
>>>> Mike.
>>>> --
>>>> 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
>>
>> --
>> Sincerely yours,
>> Mike.


-- 
Sincerely yours,
Mike.

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Tony Lindgren]

* Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
> Ghorai, Sukumar wrote:
> 
> CM-T35, for instance can be assembled with different NAND flash
> chips. Besides, boards that use NAND as primary boot device, we
> anyway depend on proper GPMC configuration in the bootloader chain.
> Having ability to define GPMC timings in the kernel and keep the
> settings made by the bootloader adds flexibility level for board
> designers.

Not implementing the retime function for GPMC will cause issues
with PM as you cannot scale the L3 frequency without breaking
your GPMC timings.

Tony

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Vimal Singh]

On Mon, Apr 26, 2010 at 11:44 PM, Tony Lindgren <tony@atomide.com> wrote:
> * Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
>> Ghorai, Sukumar wrote:
>>
>> CM-T35, for instance can be assembled with different NAND flash
>> chips. Besides, boards that use NAND as primary boot device, we
>> anyway depend on proper GPMC configuration in the bootloader chain.
>> Having ability to define GPMC timings in the kernel and keep the
>> settings made by the bootloader adds flexibility level for board
>> designers.
>
> Not implementing the retime function for GPMC will cause issues
> with PM as you cannot scale the L3 frequency without breaking
> your GPMC timings.

BTW, there is a patch from Stanley.Miao already in LO:
https://patchwork.kernel.org/patch/93613/

But it is still advisable to implement the retime function as stated by Tony.

-- 
Regards,
Vimal Singh

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Mike Rapoport]

Tony Lindgren wrote:
> * Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
>> Ghorai, Sukumar wrote:
>>
>> CM-T35, for instance can be assembled with different NAND flash
>> chips. Besides, boards that use NAND as primary boot device, we
>> anyway depend on proper GPMC configuration in the bootloader chain.
>> Having ability to define GPMC timings in the kernel and keep the
>> settings made by the bootloader adds flexibility level for board
>> designers.
> 
> Not implementing the retime function for GPMC will cause issues
> with PM as you cannot scale the L3 frequency without breaking
> your GPMC timings.

I agree that without retime function scaling the frequency will break 
the GPMC timings. But my point was that there should be an _option_ to 
keep the timings defined by the bootloader rather than enforce board 
files to specify timings.
Since skipping the retime function will break gpmc timings in PM-enabled 
  kernel, we need to implement this option in smarter way. E.g. 
something like:

diff --git a/arch/arm/mach-omap2/gpmc-nand.c 
b/arch/arm/mach-omap2/gpmc-nand.c
index 64d74f0..65ac0d0 100644
--- a/arch/arm/mach-omap2/gpmc-nand.c
+++ b/arch/arm/mach-omap2/gpmc-nand.c
@@ -34,6 +34,12 @@ static struct platform_device gpmc_nand_device = {
  	.resource	= &gpmc_nand_resource,
  };

+static int gpmc_nand_detect_timings(void)
+{
+	/* FIXME: implement timings detection */
+	return -EINVAL;
+}
+
  static int omap2_nand_gpmc_retime(void)
  {
  	struct gpmc_timings t;
@@ -109,6 +115,14 @@ int __init gpmc_nand_init(struct 
omap_nand_platform_data *_nand_data)
  		return err;
  	}

+	if (gpmc_nand_data->keep_timings) {
+		err = gpmc_nand_detect_timings();
+		if (err < 0) {
+			dev_err(dev, "Cannot detect GPMC timings\n");
+			return err;
+		}
+	}
+
  	err = gpmc_nand_setup();
  	if (err < 0) {
  		dev_err(dev, "NAND platform setup failed: %d\n", err);
diff --git a/arch/arm/plat-omap/include/plat/nand.h 
b/arch/arm/plat-omap/include/plat/nand.h
index 6ba88d2..cf05d2d 100644
--- a/arch/arm/plat-omap/include/plat/nand.h
+++ b/arch/arm/plat-omap/include/plat/nand.h
@@ -24,6 +24,7 @@ struct omap_nand_platform_data {
  	void __iomem		*gpmc_cs_baseaddr;
  	void __iomem		*gpmc_baseaddr;
  	int			devsize;
+	bool			keep_timings;
  };

  /* size (4 KiB) for IO mapping */



> Tony


-- 
Sincerely yours,
Mike.

Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied

[Tony Lindgren]

* Mike Rapoport <mike@compulab.co.il> [100427 00:40]:
> Tony Lindgren wrote:
> >* Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
> >>Ghorai, Sukumar wrote:
> >>
> >>CM-T35, for instance can be assembled with different NAND flash
> >>chips. Besides, boards that use NAND as primary boot device, we
> >>anyway depend on proper GPMC configuration in the bootloader chain.
> >>Having ability to define GPMC timings in the kernel and keep the
> >>settings made by the bootloader adds flexibility level for board
> >>designers.
> >
> >Not implementing the retime function for GPMC will cause issues
> >with PM as you cannot scale the L3 frequency without breaking
> >your GPMC timings.
> 
> I agree that without retime function scaling the frequency will
> break the GPMC timings. But my point was that there should be an
> _option_ to keep the timings defined by the bootloader rather than
> enforce board files to specify timings.

Sure. Can you please check one more time your patch and what is
still missing after Stanley's fix? That's now in omap-fixes and master
branches as commit 11e1ef2d105900a302b7ca92bcaf96a96d0274a1.

> Since skipping the retime function will break gpmc timings in
> PM-enabled  kernel, we need to implement this option in smarter way.
> E.g. something like:

Yeah we should print some warning if the retime function is not
implemented as it can cause mysterious bugs later on. I guess
implementing a dummy retime function would be best as then the
warning would be related to the actual L3 rate change.

Regards,

Tony
 
> diff --git a/arch/arm/mach-omap2/gpmc-nand.c
> b/arch/arm/mach-omap2/gpmc-nand.c
> index 64d74f0..65ac0d0 100644
> --- a/arch/arm/mach-omap2/gpmc-nand.c
> +++ b/arch/arm/mach-omap2/gpmc-nand.c
> @@ -34,6 +34,12 @@ static struct platform_device gpmc_nand_device = {
>  	.resource	= &gpmc_nand_resource,
>  };
> 
> +static int gpmc_nand_detect_timings(void)
> +{
> +	/* FIXME: implement timings detection */
> +	return -EINVAL;
> +}
> +
>  static int omap2_nand_gpmc_retime(void)
>  {
>  	struct gpmc_timings t;
> @@ -109,6 +115,14 @@ int __init gpmc_nand_init(struct
> omap_nand_platform_data *_nand_data)
>  		return err;
>  	}
> 
> +	if (gpmc_nand_data->keep_timings) {
> +		err = gpmc_nand_detect_timings();
> +		if (err < 0) {
> +			dev_err(dev, "Cannot detect GPMC timings\n");
> +			return err;
> +		}
> +	}
> +
>  	err = gpmc_nand_setup();
>  	if (err < 0) {
>  		dev_err(dev, "NAND platform setup failed: %d\n", err);
> diff --git a/arch/arm/plat-omap/include/plat/nand.h
> b/arch/arm/plat-omap/include/plat/nand.h
> index 6ba88d2..cf05d2d 100644
> --- a/arch/arm/plat-omap/include/plat/nand.h
> +++ b/arch/arm/plat-omap/include/plat/nand.h
> @@ -24,6 +24,7 @@ struct omap_nand_platform_data {
>  	void __iomem		*gpmc_cs_baseaddr;
>  	void __iomem		*gpmc_baseaddr;
>  	int			devsize;
> +	bool			keep_timings;
>  };
> 
>  /* size (4 KiB) for IO mapping */
> 
> 
> 
> >Tony
> 
> 
> -- 
> Sincerely yours,
> Mike.

Bug in omap2_nand_gpmc_retime? (was: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied)

[Mike Rapoport]

Tony Lindgren wrote:
> * Mike Rapoport <mike@compulab.co.il> [100427 00:40]:
>> Tony Lindgren wrote:
>>> * Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
>>>> Ghorai, Sukumar wrote:
>>>>
>>>> CM-T35, for instance can be assembled with different NAND flash
>>>> chips. Besides, boards that use NAND as primary boot device, we
>>>> anyway depend on proper GPMC configuration in the bootloader chain.
>>>> Having ability to define GPMC timings in the kernel and keep the
>>>> settings made by the bootloader adds flexibility level for board
>>>> designers.
>>> Not implementing the retime function for GPMC will cause issues
>>> with PM as you cannot scale the L3 frequency without breaking
>>> your GPMC timings.
>> I agree that without retime function scaling the frequency will
>> break the GPMC timings. But my point was that there should be an
>> _option_ to keep the timings defined by the bootloader rather than
>> enforce board files to specify timings.
> 
> Sure. Can you please check one more time your patch and what is
> still missing after Stanley's fix? That's now in omap-fixes and master
> branches as commit 11e1ef2d105900a302b7ca92bcaf96a96d0274a1.
> 
>> Since skipping the retime function will break gpmc timings in
>> PM-enabled  kernel, we need to implement this option in smarter way.
>> E.g. something like:
> 
> Yeah we should print some warning if the retime function is not
> implemented as it can cause mysterious bugs later on. I guess
> implementing a dummy retime function would be best as then the
> warning would be related to the actual L3 rate change.

While working on implementation of gpmc_nand_detect_timings I've seen 
that omap2_nand_gpmc_retime converts the timings supplied by the 
platform to ticks and passes it to gpmc_cs_set_timings which in turn 
converts the timings to ticks. Is it a bug or am I missing something?

> Regards,
> 
> Tony
>  
-- 
Sincerely yours,
Mike.

Re: Bug in omap2_nand_gpmc_retime? (was: Re: [PATCH] OMAP: fix gpmc nand setup when no timings supplied)

[Vimal Singh]

On Wed, Apr 28, 2010 at 8:35 PM, Mike Rapoport <mike@compulab.co.il> wrote:
> Tony Lindgren wrote:
>>
>> * Mike Rapoport <mike@compulab.co.il> [100427 00:40]:
>>>
>>> Tony Lindgren wrote:
>>>>
>>>> * Mike Rapoport <mike@compulab.co.il> [100422 01:41]:
>>>>>
>>>>> Ghorai, Sukumar wrote:
>>>>>
>>>>> CM-T35, for instance can be assembled with different NAND flash
>>>>> chips. Besides, boards that use NAND as primary boot device, we
>>>>> anyway depend on proper GPMC configuration in the bootloader chain.
>>>>> Having ability to define GPMC timings in the kernel and keep the
>>>>> settings made by the bootloader adds flexibility level for board
>>>>> designers.
>>>>
>>>> Not implementing the retime function for GPMC will cause issues
>>>> with PM as you cannot scale the L3 frequency without breaking
>>>> your GPMC timings.
>>>
>>> I agree that without retime function scaling the frequency will
>>> break the GPMC timings. But my point was that there should be an
>>> _option_ to keep the timings defined by the bootloader rather than
>>> enforce board files to specify timings.
>>
>> Sure. Can you please check one more time your patch and what is
>> still missing after Stanley's fix? That's now in omap-fixes and master
>> branches as commit 11e1ef2d105900a302b7ca92bcaf96a96d0274a1.
>>
>>> Since skipping the retime function will break gpmc timings in
>>> PM-enabled  kernel, we need to implement this option in smarter way.
>>> E.g. something like:
>>
>> Yeah we should print some warning if the retime function is not
>> implemented as it can cause mysterious bugs later on. I guess
>> implementing a dummy retime function would be best as then the
>> warning would be related to the actual L3 rate change.
>
> While working on implementation of gpmc_nand_detect_timings I've seen that
> omap2_nand_gpmc_retime converts the timings supplied by the platform to
> ticks and passes it to gpmc_cs_set_timings which in turn converts the
> timings to ticks. Is it a bug or am I missing something?

No, 'omap2_nand_gpmc_retime' does not convert timings supplied by the
platform to tick. Rather it rounds the timings passed by the platform
to timings in units of 'tick' period.

-- 
Regards,
Vimal Singh
--
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

Re: Bug in omap2_nand_gpmc_retime?

[Mike Rapoport]

Vimal Singh wrote:
> On Wed, Apr 28, 2010 at 8:35 PM, Mike Rapoport <mike@compulab.co.il> wrote:

>> While working on implementation of gpmc_nand_detect_timings I've seen that
>> omap2_nand_gpmc_retime converts the timings supplied by the platform to
>> ticks and passes it to gpmc_cs_set_timings which in turn converts the
>> timings to ticks. Is it a bug or am I missing something?
> 
> No, 'omap2_nand_gpmc_retime' does not convert timings supplied by the
> platform to tick. Rather it rounds the timings passed by the platform
> to timings in units of 'tick' period.

Thanks for the clarification. I still wonder why is this necessary. 
Anyway gpmc_set_cs_timings will do the conversion?



-- 
Sincerely yours,
Mike.