[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

Re: [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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

Re: [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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

Re: [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

[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/  |

Re: [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"

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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"

Re: [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

[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/  |

Re: [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

[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

Re: [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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

Re: [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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[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

please strip MIME-Version and Content-T{ype,ransfer-Encoding} in git am --scissors

[Uwe Kleine-König]

Hello,

when applying the mail below (without the '> ' prefix) using git am
--scissors the result looks like:

	$ git show
	commit 26ef0606927cc1979faa4166d7f9f3584b5cdc61
	Author: Tony Lindgren <tony@atomide.com>
	Date:   Tue Oct 6 05:36:17 2015 -0700

	    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.
	    
	[...]

	$ git version
	git version 2.6.0

The obvious improvement is to strip all headers like git am does without
--scissors.

If someone wants a bounce of the original mail, just ask per PM.

Best regards
Uwe


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?
> 
> 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"
> 

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

Re: please strip MIME-Version and Content-T{ype,ransfer-Encoding} in git am --scissors

[Junio C Hamano]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

> Hello,
>
> when applying the mail below (without the '> ' prefix) using git am
> --scissors the result looks like:
>
> 	$ git show
> 	commit 26ef0606927cc1979faa4166d7f9f3584b5cdc61
> 	Author: Tony Lindgren <tony@atomide.com>
> 	Date:   Tue Oct 6 05:36:17 2015 -0700
>
> 	    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.
> 	    
> 	[...]
>
> 	$ git version
> 	git version 2.6.0
>
> The obvious improvement is to strip all headers like git am does without
> --scissors.

Does this have anything to do with scissors, though?  If you remove
everything before "8< ---" in the body of Tony's message (i.e. keep
the in-body headers starting with "From:" and ending with CTE) and
try again, I would suspect that you will get the same result.

I also think that the "MIME-Version" thing is what gives this;
mailinfo and am do not really use it, and consider that the in-body
header ends there.

The right approach to tweak mailinfo to cope with this better would
be to keep a bit more state inside mailinfo.c::handle_commit_msg()
so that if we are (1) using in-body headers, (2) have already seen
_some_ valid in-body header like "Subject:" and "From: ", and (3)
have not seen a blank line, discard lines that we do not care about
(e.g. "MIME-VERSION: 1.0").


> If someone wants a bounce of the original mail, just ask per PM.

I have no idea what you are talking about here...


> 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?
>> 
>> 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
>> ...

Re: please strip MIME-Version and Content-T{ype,ransfer-Encoding} in git am --scissors

[Uwe Kleine-König]

Hello Junio,

On Thu, Oct 08, 2015 at 12:28:46PM -0700, Junio C Hamano wrote:
> Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
> 
> > Hello,
> >
> > when applying the mail below (without the '> ' prefix) using git am
> > --scissors the result looks like:
> >
> > 	$ git show
> > 	commit 26ef0606927cc1979faa4166d7f9f3584b5cdc61
> > 	Author: Tony Lindgren <tony@atomide.com>
> > 	Date:   Tue Oct 6 05:36:17 2015 -0700
> >
> > 	    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.
> > 	    
> > 	[...]
> >
> > 	$ git version
> > 	git version 2.6.0
> >
> > The obvious improvement is to strip all headers like git am does without
> > --scissors.
> 
> Does this have anything to do with scissors, though?  If you remove
> everything before "8< ---" in the body of Tony's message (i.e. keep
> the in-body headers starting with "From:" and ending with CTE) and
> try again, I would suspect that you will get the same result.
No, you're wrong here:

ukl@dude.ptx:~/gsrc/linux$ head ~/tmp/1444332661.3982_89.ptx\:2\,RS 
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.

ukl@dude.ptx:~/gsrc/linux$ git am ~/tmp/1444332661.3982_89.ptx\:2\,RS
Applying: memory: omap-gpmc: Fix unselectable debug option for GPMC
ukl@dude.ptx:~/gsrc/linux$ git cat-file commit HEAD
tree bab01e3e0d0bdd715b86cf7d5c9e8bb9768a30dc
parent c6fa8e6de3dc420cba092bf155b2ed25bcd537f7
author Tony Lindgren <tony@atomide.com> 1444134977 -0700
committer Uwe Kleine-König <u.kleine-koenig@pengutronix.de> 1444332782 +0200

memory: omap-gpmc: Fix unselectable debug option for GPMC

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>

> I also think that the "MIME-Version" thing is what gives this;
> mailinfo and am do not really use it, and consider that the in-body
> header ends there.

I failed to follow you here.

> The right approach to tweak mailinfo to cope with this better would
> be to keep a bit more state inside mailinfo.c::handle_commit_msg()
> so that if we are (1) using in-body headers, (2) have already seen
> _some_ valid in-body header like "Subject:" and "From: ", and (3)
> have not seen a blank line, discard lines that we do not care about
> (e.g. "MIME-VERSION: 1.0").

That sound's right.
 
> > If someone wants a bounce of the original mail, just ask per PM.
> 
> I have no idea what you are talking about here...

The result would be that a copy of the original mail would hit your mailbox if
you asked per private mail (PM).

Best regards
Uwe

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

Re: please strip MIME-Version and Content-T{ype,ransfer-Encoding} in git am --scissors

[Junio C Hamano]

Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:

>> Does this have anything to do with scissors, though?  If you remove
>> everything before "8< ---" in the body of Tony's message (i.e. keep
>> the in-body headers starting with "From:" and ending with CTE) and
>> try again, I would suspect that you will get the same result.
> No, you're wrong here:
>
> ukl@dude.ptx:~/gsrc/linux$ head ~/tmp/1444332661.3982_89.ptx\:2\,RS 
> 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.

I think you are the one who misread my question.  I said "keep the
in-body headers", didn't I?  If you did the "head", then you would
see something like this:


> ukl@dude.ptx:~/gsrc/linux$ head ~/tmp/1444332661.3982_89.ptx\:2\,RS 
> 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
> ... probably Received: and all other junk from your mailbox ...
>
> 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.

>> I also think that the "MIME-Version" thing is what gives this;
>> mailinfo and am do not really use it, and consider that the in-body
>> header ends there.
>
> I failed to follow you here.

I think if you tried the example with in-body header, you will see
what I meant.

>
>> The right approach to tweak mailinfo to cope with this better would
>> be to keep a bit more state inside mailinfo.c::handle_commit_msg()
>> so that if we are (1) using in-body headers, (2) have already seen
>> _some_ valid in-body header like "Subject:" and "From: ", and (3)
>> have not seen a blank line, discard lines that we do not care about
>> (e.g. "MIME-VERSION: 1.0").
>
> That sound's right.

Re: please strip MIME-Version and Content-T{ype,ransfer-Encoding} in git am --scissors

[Uwe Kleine-König]

Hello Junio,

On Thu, Oct 08, 2015 at 01:04:01PM -0700, Junio C Hamano wrote:
> Uwe Kleine-König  <u.kleine-koenig@pengutronix.de> writes:
> 
> >> Does this have anything to do with scissors, though?  If you remove
> >> everything before "8< ---" in the body of Tony's message (i.e. keep
> >> the in-body headers starting with "From:" and ending with CTE) and
> >> try again, I would suspect that you will get the same result.
> > No, you're wrong here:
> >
> > ukl@dude.ptx:~/gsrc/linux$ head ~/tmp/1444332661.3982_89.ptx\:2\,RS 
> > 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.
> 
> I think you are the one who misread my question.  I said "keep the
> in-body headers", didn't I?  If you did the "head", then you would
> see something like this:

Ah got it. Yes, you're right. (Subject and Date are actually different
between real and in-body headers, but that's not important. git am picks
up the in-body headers.)
 
> > ukl@dude.ptx:~/gsrc/linux$ head ~/tmp/1444332661.3982_89.ptx\:2\,RS 
> > 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
> > ... probably Received: and all other junk from your mailbox ...
> >
> > 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.
> 
> >> I also think that the "MIME-Version" thing is what gives this;
> >> mailinfo and am do not really use it, and consider that the in-body
> >> header ends there.
> >
> > I failed to follow you here.
> 
> I think if you tried the example with in-body header, you will see
> what I meant.
> 
> >
> >> The right approach to tweak mailinfo to cope with this better would
> >> be to keep a bit more state inside mailinfo.c::handle_commit_msg()
> >> so that if we are (1) using in-body headers, (2) have already seen
> >> _some_ valid in-body header like "Subject:" and "From: ", and (3)
> >> have not seen a blank line, discard lines that we do not care about
> >> (e.g. "MIME-VERSION: 1.0").

The right thing should also happen if MIME-Version comes above Subject
in the body but other than that I'm with you here.

Best regards
Uwe

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

[PATCH] mailinfo: ignore in-body header that we do not care about

[Junio C Hamano]

"git mailinfo" (hence "git am") understands some well-known headers,
like "Subject: ", "Date: " and "From: ", placed at the beginning of
the message body (and the "--scissors" can discard the part of the
body before a scissors-mark).  However, some people throw other
kinds of header-looking things there, expecting them to be
discarded.

Finding and discarding anything that looks like RFC2822 header is
not a right solution.  The body of the message may start with a line
that begins with a word followed by a colon that is a legitimate
part of the message that should not be discarded.

Instead, keep reading non-blank lines once we see an in-body header
at the beginning and discard them.  Nobody will be insane enough to
reorder the headers to read like this:

    Garbage-non-in-body-header: here
    Subject: in-body subject

    Here is the body of the commit log.

but it is common for lazy or misguided people to leave non-header
materials in-body like this:

    From: Junio C Hamano <gitster@pobox.com>
    Date: Mon, 28 Sep 2015 19:19:27 -0700
    Subject: [PATCH] Git 2.6.1
    MIME-Version: 1.0

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 I think it is wrong for the in-body header codepath to pay
 attention to content-transfer-encodings and stuff, but that is a
 separate issue.

 Also if you remove the "does the line even look like a header?"
 check, some tests in t5100 starts failing.  E.g.

    From nobody Mon Sep 17 00:00:00 2001
    From: A U Thor <a.u.thor@example.com>
    Subject: check bogus body header (from)
    Date: Fri, 9 Jun 2006 00:44:16 -0700

    From: bogosity
      - a list
      - of stuff

 wants to make sure the list of two bulletted-items are in the
 commit log, and the in-body From: line gets used.

 So I dunno.  I am not entirely convinced that this is a good
 change.

 builtin/mailinfo.c  | 34 +++++++++++++++++++++++++++++++---
 t/t5100-mailinfo.sh |  3 ++-
 t/t5100/info0018    |  5 +++++
 t/t5100/msg0018     |  2 ++
 t/t5100/patch0018   |  6 ++++++
 t/t5100/sample.mbox | 18 ++++++++++++++++++
 6 files changed, 64 insertions(+), 4 deletions(-)
 create mode 100644 t/t5100/info0018
 create mode 100644 t/t5100/msg0018
 create mode 100644 t/t5100/patch0018

diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
index 999a525..169ee54 100644
--- a/builtin/mailinfo.c
+++ b/builtin/mailinfo.c
@@ -787,18 +787,46 @@ static int is_scissors_line(const struct strbuf *line)
 
 static int handle_commit_msg(struct strbuf *line)
 {
+	/*
+	 * Are we still scanning and discarding in-body headers?
+	 * It is initially set to 1, set to 2 when we do see a
+	 * valid in-body header.
+	 */
 	static int still_looking = 1;
+	int is_empty_line;
 
 	if (!cmitmsg)
 		return 0;
 
-	if (still_looking) {
-		if (!line->len || (line->len == 1 && line->buf[0] == '\n'))
+	is_empty_line = (!line->len || (line->len == 1 && line->buf[0] == '\n'));
+	if (still_looking == 1) {
+		/*
+		 * Haven't seen a known in-body header; discard an empty line.
+		 */
+		if (is_empty_line)
 			return 0;
 	}
 
 	if (use_inbody_headers && still_looking) {
-		still_looking = check_header(line, s_hdr_data, 0);
+		int is_known_header = check_header(line, s_hdr_data, 0);
+
+		if (still_looking == 2) {
+			/*
+			 * an empty line after the in-body header block,
+			 * or a line obviously not an attempt to invent
+			 * an unsupported in-body header.
+			 */
+			if (is_empty_line || !is_rfc2822_header(line))
+				still_looking = 0;
+			if (is_empty_line)
+				return 0;
+			/* otherwise do not discard the line, but keep going */
+		} else if (is_known_header) {
+			still_looking = 2;
+		} else if (still_looking != 2) {
+			still_looking = 0;
+		}
+
 		if (still_looking)
 			return 0;
 	} else
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e97cfb2..3ce041b 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,8 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 17'
+	test `cat last` = 18
+'
 
 check_mailinfo () {
 	mail=$1 opt=$2
diff --git a/t/t5100/info0018 b/t/t5100/info0018
new file mode 100644
index 0000000..ec671fc
--- /dev/null
+++ b/t/t5100/info0018
@@ -0,0 +1,5 @@
+Author: A U Thor
+Email: a.u.thor@example.com
+Subject: A E I O U
+Date: Mon, 17 Sep 2012 14:23:49 -0700
+
diff --git a/t/t5100/msg0018 b/t/t5100/msg0018
new file mode 100644
index 0000000..2ee0900
--- /dev/null
+++ b/t/t5100/msg0018
@@ -0,0 +1,2 @@
+New content here
+
diff --git a/t/t5100/patch0018 b/t/t5100/patch0018
new file mode 100644
index 0000000..35cf84c
--- /dev/null
+++ b/t/t5100/patch0018
@@ -0,0 +1,6 @@
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++New content
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 8b2ae06..d7c5878 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -406,6 +406,7 @@ Subject: re: [PATCH] another patch
 
 From: A U Thor <a.u.thor@example.com>
 Subject: [PATCH] another patch
+
 >Here is an empty patch from A U Thor.
 
 Hey you forgot the patch!
@@ -699,3 +700,20 @@ index e69de29..d95f3ad 100644
 +++ b/foo
 @@ -0,0 +1 @@
 +New content
+From nobody Mon Sep 17 00:00:00 2001
+From: A U Thor <a.u.thor@example.com>
+Subject: Re: some discussion title
+Date: Mon, 17 Sep 2012 14:23:49 -0700
+
+Subject: A E I O U
+MIME-VERSION: 1.0
+Garbage: Not a valid in-body header
+
+New content here
+
+diff --git a/foo b/foo
+index e69de29..d95f3ad 100644
+--- a/foo
++++ b/foo
+@@ -0,0 +1 @@
++New content
-- 
2.6.1-296-ge15092e