[PATCH 1/2] memory: omap-gpmc: dump "before" state before first modification

[PATCH 1/2] memory: omap-gpmc: dump "before" state before first modification

[Uwe Kleine-König]

When gpmc_cs_show_timings is called in gpmc_cs_set_timings()
gpmc_cs_program_settings() was already run which modifies the CONFIG1
register. So to be more useful do the "before" dump earlier.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/memory/omap-gpmc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 32ac049f2bc4..6515dfc2b805 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -696,7 +696,6 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t,
 	int div;
 	u32 l;
 
-	gpmc_cs_show_timings(cs, "before gpmc_cs_set_timings");
 	div = gpmc_calc_divider(t->sync_clk);
 	if (div < 0)
 		return div;
@@ -1988,6 +1987,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
 	if (ret < 0)
 		goto err;
 
+	gpmc_cs_show_timings(cs, "before gpmc_cs_program_settings");
 	ret = gpmc_cs_program_settings(cs, &gpmc_s);
 	if (ret < 0)
 		goto err;
-- 
2.6.0

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Uwe Kleine-König]

Most register values for the chip select setup depend on the frequency
of the fck clock.
So add a hint that the values setup by the bootloader might differ from
the right setup for Linux if the bootloader uses a different frequency.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/memory/Kconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index c6a644b22af4..1414dd53be57 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -64,6 +64,9 @@ config OMAP_GPMC_DEBUG
 	  Enables verbose debugging mostly to decode the bootloader provided
 	  timings. Enable this during development to configure devices
 	  connected to the GPMC bus.
+	  Note that you cannot just tweak your device tree until the registers
+	  setup by linux match what the bootloader did because that one might
+	  use a different fck frequency influencing most register settings.
 
 config MVEBU_DEVBUS
 	bool "Marvell EBU Device Bus Controller"
-- 
2.6.0

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Roger Quadros]

On 06/10/15 23:07, Uwe Kleine-K?nig wrote:
> Most register values for the chip select setup depend on the frequency
> of the fck clock.
> So add a hint that the values setup by the bootloader might differ from
> the right setup for Linux if the bootloader uses a different frequency.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/memory/Kconfig | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index c6a644b22af4..1414dd53be57 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -64,6 +64,9 @@ config OMAP_GPMC_DEBUG
>  	  Enables verbose debugging mostly to decode the bootloader provided
>  	  timings. Enable this during development to configure devices
>  	  connected to the GPMC bus.
> +	  Note that you cannot just tweak your device tree until the registers
> +	  setup by linux match what the bootloader did because that one might
> +	  use a different fck frequency influencing most register settings.

Looks like we can't know for sure the GPMC fclk used at the bootloader
else we could have just printed the GPMC fclk pre and post gpmc settings.

How about this instead?

NOTE: Apart from matching the register setup with the bootloader you also need to
match the GPMC FCLK frequency used by the bootloader else the GPMC timings
won't be identical with the bootloader timings.

Also you might need to build this patch on top of
http://article.gmane.org/gmane.linux.kernel/2054796

>  
>  config MVEBU_DEVBUS
>  	bool "Marvell EBU Device Bus Controller"
> 

cheers,
-roger

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Uwe Kleine-König]

Hello Roger,

On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
> On 06/10/15 23:07, Uwe Kleine-K?nig wrote:
> > Most register values for the chip select setup depend on the frequency
> > of the fck clock.
> > So add a hint that the values setup by the bootloader might differ from
> > the right setup for Linux if the bootloader uses a different frequency.
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> >  drivers/memory/Kconfig | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> > index c6a644b22af4..1414dd53be57 100644
> > --- a/drivers/memory/Kconfig
> > +++ b/drivers/memory/Kconfig
> > @@ -64,6 +64,9 @@ config OMAP_GPMC_DEBUG
> >  	  Enables verbose debugging mostly to decode the bootloader provided
> >  	  timings. Enable this during development to configure devices
> >  	  connected to the GPMC bus.
> > +	  Note that you cannot just tweak your device tree until the registers
> > +	  setup by linux match what the bootloader did because that one might
> > +	  use a different fck frequency influencing most register settings.
> 
> Looks like we can't know for sure the GPMC fclk used at the bootloader
> else we could have just printed the GPMC fclk pre and post gpmc settings.
> 
> How about this instead?
> 
> NOTE: Apart from matching the register setup with the bootloader you also need to
> match the GPMC FCLK frequency used by the bootloader else the GPMC timings
> won't be identical with the bootloader timings.
Yeah, sounds better, thanks.

> Also you might need to build this patch on top of
> http://article.gmane.org/gmane.linux.kernel/2054796
I talked to Tony about this patch yesterday on irc, but I didn't find it
in the archives yet when I sent my mail.

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Tony Lindgren]

* Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> [151007 00:57]:
> On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
> > 
> > How about this instead?
> > 
> > NOTE: Apart from matching the register setup with the bootloader you also need to
> > match the GPMC FCLK frequency used by the bootloader else the GPMC timings
> > won't be identical with the bootloader timings.
> Yeah, sounds better, thanks.
> 
> > Also you might need to build this patch on top of
> > http://article.gmane.org/gmane.linux.kernel/2054796
> I talked to Tony about this patch yesterday on irc, but I didn't find it
> in the archives yet when I sent my mail.

Yes sorry here's a repost with your and Roger's changes folded in and
edited a bit. Probably best to keep them together with this patch.

Does the following look OK to you guys?

Regards,

Tony

8< ----------------
From: Tony Lindgren <tony@atomide.com>
Date: Tue, 6 Oct 2015 05:36:17 -0700
Subject: [PATCH] memory: omap-gpmc: Fix unselectable debug option for GPMC
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Commit 63aa945b1013 ("memory: omap-gpmc: Add Kconfig option for debug")
added a debug option for GPMC, but somehow managed to keep it unselectable.

This probably happened because I had some uncommitted changes and the
GPMC option is selected in the platform specific Kconfig.

Let's also update the description a bit, it does not mention that
enabling the debug option also disables the reset of GPMC controller
during the init as pointed out by Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> and Roger Quadros <rogerq@ti.com>.

Fixes: 63aa945b1013 ("memory: omap-gpmc: Add Kconfig option for debug")
Reported-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: Tony Lindgren <tony@atomide.com>

--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -58,12 +58,18 @@ config OMAP_GPMC
 	  memory drives like NOR, NAND, OneNAND, SRAM.
 
 config OMAP_GPMC_DEBUG
-	bool
+	bool "Enable GPMC debug output and skip reset of GPMC during init"
 	depends on OMAP_GPMC
 	help
 	  Enables verbose debugging mostly to decode the bootloader provided
-	  timings. Enable this during development to configure devices
-	  connected to the GPMC bus.
+	  timings. To preserve the bootloader provided timings, the reset
+	  of GPMC is skipped during init. Enable this during development to
+	  configure devices connected to the GPMC bus.
+
+	  NOTE: In addition to matching the register setup with the bootloader
+	  you also need to match the GPMC FCLK frequency used by the
+	  bootloader or else the GPMC timings won't be identical with the
+	  bootloader timings.
 
 config MVEBU_DEVBUS
 	bool "Marvell EBU Device Bus Controller"

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Uwe Kleine-König]

Hello Tony,

On Wed, Oct 07, 2015 at 03:41:19AM -0700, Tony Lindgren wrote:
> * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> [151007 00:57]:
> > On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
> > > 
> > > How about this instead?
> > > 
> > > NOTE: Apart from matching the register setup with the bootloader you also need to
> > > match the GPMC FCLK frequency used by the bootloader else the GPMC timings
> > > won't be identical with the bootloader timings.
> > Yeah, sounds better, thanks.
> > 
> > > Also you might need to build this patch on top of
> > > http://article.gmane.org/gmane.linux.kernel/2054796
> > I talked to Tony about this patch yesterday on irc, but I didn't find it
> > in the archives yet when I sent my mail.
> 
> Yes sorry here's a repost with your and Roger's changes folded in and
> edited a bit. Probably best to keep them together with this patch.
> 
> Does the following look OK to you guys?
Yes,
Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Roger Quadros]

On 07/10/15 14:02, Uwe Kleine-K?nig wrote:
> Hello Tony,
> 
> On Wed, Oct 07, 2015 at 03:41:19AM -0700, Tony Lindgren wrote:
>> * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> [151007 00:57]:
>>> On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
>>>>
>>>> How about this instead?
>>>>
>>>> NOTE: Apart from matching the register setup with the bootloader you also need to
>>>> match the GPMC FCLK frequency used by the bootloader else the GPMC timings
>>>> won't be identical with the bootloader timings.
>>> Yeah, sounds better, thanks.
>>>
>>>> Also you might need to build this patch on top of
>>>> http://article.gmane.org/gmane.linux.kernel/2054796
>>> I talked to Tony about this patch yesterday on irc, but I didn't find it
>>> in the archives yet when I sent my mail.
>>
>> Yes sorry here's a repost with your and Roger's changes folded in and
>> edited a bit. Probably best to keep them together with this patch.
>>
>> Does the following look OK to you guys?
> Yes,
> Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Looks good to me too.

cheers,
-roger

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Tony Lindgren]

* Roger Quadros <rogerq@ti.com> [151007 04:12]:
> On 07/10/15 14:02, Uwe Kleine-K?nig wrote:
> > Hello Tony,
> > 
> > On Wed, Oct 07, 2015 at 03:41:19AM -0700, Tony Lindgren wrote:
> >> * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> [151007 00:57]:
> >>> On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
> >>>>
> >>>> How about this instead?
> >>>>
> >>>> NOTE: Apart from matching the register setup with the bootloader you also need to
> >>>> match the GPMC FCLK frequency used by the bootloader else the GPMC timings
> >>>> won't be identical with the bootloader timings.
> >>> Yeah, sounds better, thanks.
> >>>
> >>>> Also you might need to build this patch on top of
> >>>> http://article.gmane.org/gmane.linux.kernel/2054796
> >>> I talked to Tony about this patch yesterday on irc, but I didn't find it
> >>> in the archives yet when I sent my mail.
> >>
> >> Yes sorry here's a repost with your and Roger's changes folded in and
> >> edited a bit. Probably best to keep them together with this patch.
> >>
> >> Does the following look OK to you guys?
> > Yes,
> > Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> 
> Looks good to me too.

Are you OK if I use your Acked-by from the previous version on
this updated version?

Regards,

Tony

[PATCH 2/2] memory: omap-gpmc: expand the description of the debug facility

[Roger Quadros]

On 07/10/15 16:40, Tony Lindgren wrote:
> * Roger Quadros <rogerq@ti.com> [151007 04:12]:
>> On 07/10/15 14:02, Uwe Kleine-K?nig wrote:
>>> Hello Tony,
>>>
>>> On Wed, Oct 07, 2015 at 03:41:19AM -0700, Tony Lindgren wrote:
>>>> * Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> [151007 00:57]:
>>>>> On Wed, Oct 07, 2015 at 10:45:50AM +0300, Roger Quadros wrote:
>>>>>>
>>>>>> How about this instead?
>>>>>>
>>>>>> NOTE: Apart from matching the register setup with the bootloader you also need to
>>>>>> match the GPMC FCLK frequency used by the bootloader else the GPMC timings
>>>>>> won't be identical with the bootloader timings.
>>>>> Yeah, sounds better, thanks.
>>>>>
>>>>>> Also you might need to build this patch on top of
>>>>>> http://article.gmane.org/gmane.linux.kernel/2054796
>>>>> I talked to Tony about this patch yesterday on irc, but I didn't find it
>>>>> in the archives yet when I sent my mail.
>>>>
>>>> Yes sorry here's a repost with your and Roger's changes folded in and
>>>> edited a bit. Probably best to keep them together with this patch.
>>>>
>>>> Does the following look OK to you guys?
>>> Yes,
>>> Acked-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>>
>> Looks good to me too.
> 
> Are you OK if I use your Acked-by from the previous version on
> this updated version?

Yes please.

cheers,
-roger

[PATCH 1/2] memory: omap-gpmc: dump "before" state before first modification

[Roger Quadros]

On 06/10/15 23:07, Uwe Kleine-K?nig wrote:
> When gpmc_cs_show_timings is called in gpmc_cs_set_timings()
> gpmc_cs_program_settings() was already run which modifies the CONFIG1
> register. So to be more useful do the "before" dump earlier.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Acked-by: Roger Quadros <rogerq@ti.com>

> ---
>  drivers/memory/omap-gpmc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 32ac049f2bc4..6515dfc2b805 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -696,7 +696,6 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t,
>  	int div;
>  	u32 l;
>  
> -	gpmc_cs_show_timings(cs, "before gpmc_cs_set_timings");
>  	div = gpmc_calc_divider(t->sync_clk);
>  	if (div < 0)
>  		return div;
> @@ -1988,6 +1987,7 @@ static int gpmc_probe_generic_child(struct platform_device *pdev,
>  	if (ret < 0)
>  		goto err;
>  
> +	gpmc_cs_show_timings(cs, "before gpmc_cs_program_settings");
>  	ret = gpmc_cs_program_settings(cs, &gpmc_s);
>  	if (ret < 0)
>  		goto err;
> 

cheers,
-roger