mx6qsabresd hangs on linux-next

mx6qsabresd hangs on linux-next

[Fabio Estevam]

Hi,

I am running linux-next 20140505 on a mx6qsabresd and I am getting the
following hang after ldb is bound:

imx-ipuv3 2400000.ipu: IPUv3H probed
imx-ipuv3 2800000.ipu: IPUv3H probed
[drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
[drm] No driver support for vblank timestamp query.
imx-drm display-subsystem.11: bound imx-ipuv3-crtc.0 (ops ipu_crtc_ops)
imx-drm display-subsystem.11: bound imx-ipuv3-crtc.1 (ops ipu_crtc_ops)
mmc1: new SDHC card at address e7ac
mmcblk0: mmc1:e7ac SD04G 3.69 GiB
imx-drm display-subsystem.11: bound imx-ipuv3-crtc.2 (ops ipu_crtc_ops)
 mmcblk0: unknown partition table
imx-drm display-subsystem.11: bound imx-ipuv3-crtc.3 (ops ipu_crtc_ops)
imx-hdmi 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
imx-drm display-subsystem.11: bound 120000.hdmi (ops hdmi_ops)
imx-drm display-subsystem.11: bound ldb.10 (ops imx_ldb_ops)

(... system hangs)

If I built the kernel without LVDS support then it boots fine until the prompt.

Haven't started debugging it yet, but in case anyone has any
suggestions, please let me know.

I have also tested with Shawn's for-next and the problem does not
happen with his tree.

Regards,

Fabio Estevam

mx6qsabresd hangs on linux-next

[Shawn Guo]

On Mon, May 05, 2014 at 07:50:24PM -0300, Fabio Estevam wrote:
> Hi,
> 
> I am running linux-next 20140505 on a mx6qsabresd and I am getting the
> following hang after ldb is bound:
> 
> imx-ipuv3 2400000.ipu: IPUv3H probed
> imx-ipuv3 2800000.ipu: IPUv3H probed
> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
> [drm] No driver support for vblank timestamp query.
> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.0 (ops ipu_crtc_ops)
> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.1 (ops ipu_crtc_ops)
> mmc1: new SDHC card at address e7ac
> mmcblk0: mmc1:e7ac SD04G 3.69 GiB
> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.2 (ops ipu_crtc_ops)
>  mmcblk0: unknown partition table
> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.3 (ops ipu_crtc_ops)
> imx-hdmi 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
> imx-drm display-subsystem.11: bound 120000.hdmi (ops hdmi_ops)
> imx-drm display-subsystem.11: bound ldb.10 (ops imx_ldb_ops)
> 
> (... system hangs)

I'm seeing the same issue here.  It seems to be caused by some clk
divider code change.  I'm looking into it further.

Shawn

mx6qsabresd hangs on linux-next

[Fabio Estevam]

On Tue, May 6, 2014 at 11:13 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Mon, May 05, 2014 at 07:50:24PM -0300, Fabio Estevam wrote:
>> Hi,
>>
>> I am running linux-next 20140505 on a mx6qsabresd and I am getting the
>> following hang after ldb is bound:
>>
>> imx-ipuv3 2400000.ipu: IPUv3H probed
>> imx-ipuv3 2800000.ipu: IPUv3H probed
>> [drm] Supports vblank timestamp caching Rev 2 (21.10.2013).
>> [drm] No driver support for vblank timestamp query.
>> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.0 (ops ipu_crtc_ops)
>> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.1 (ops ipu_crtc_ops)
>> mmc1: new SDHC card at address e7ac
>> mmcblk0: mmc1:e7ac SD04G 3.69 GiB
>> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.2 (ops ipu_crtc_ops)
>>  mmcblk0: unknown partition table
>> imx-drm display-subsystem.11: bound imx-ipuv3-crtc.3 (ops ipu_crtc_ops)
>> imx-hdmi 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1
>> imx-drm display-subsystem.11: bound 120000.hdmi (ops hdmi_ops)
>> imx-drm display-subsystem.11: bound ldb.10 (ops imx_ldb_ops)
>>
>> (... system hangs)
>
> I'm seeing the same issue here.  It seems to be caused by some clk
> divider code change.  I'm looking into it further.

Indeed, if I revert:

commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
Author: Maxime COQUELIN <maxime.coquelin@st.com>
Date:   Wed Jan 29 17:24:08 2014 +0100

    clk: divider: Optimize clk_divider_bestdiv loop

    Currently, the for-loop used to try all the different dividers to find the
    one that best fit tries all the values from 1 to max_div,
incrementing by one.
    In case of power-of-two, or table based divider, the loop isn't optimal.

    Instead of incrementing by one, this patch provides directly the
next divider.

    Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
    Signed-off-by: Mike Turquette <mturquette@linaro.org>

Then the board does not hang.

Maxime, Mike

Any suggestion?

mx6qsabresd hangs on linux-next

[Fabio Estevam]

On Tue, May 6, 2014 at 12:36 PM, Fabio Estevam <festevam@gmail.com> wrote:

> Indeed, if I revert:
>
> commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
> Author: Maxime COQUELIN <maxime.coquelin@st.com>
> Date:   Wed Jan 29 17:24:08 2014 +0100
>
>     clk: divider: Optimize clk_divider_bestdiv loop
>
>     Currently, the for-loop used to try all the different dividers to find the
>     one that best fit tries all the values from 1 to max_div,
> incrementing by one.
>     In case of power-of-two, or table based divider, the loop isn't optimal.
>
>     Instead of incrementing by one, this patch provides directly the
> next divider.
>
>     Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>     Signed-off-by: Mike Turquette <mturquette@linaro.org>
>
> Then the board does not hang.

Isn't the increment of i missing?

--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -279,7 +279,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
unsigned long rate,
         */
        maxdiv = min(ULONG_MAX / rate, maxdiv);

-       for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
+       for (i = 1; i <= maxdiv; i +=_next_div(divider, i)) {
                if (!_is_valid_div(divider, i))
                        continue;
                if (rate * i == parent_rate_saved) {

mx6qsabresd hangs on linux-next

[Maxime Coquelin]

Hi Fabio,

On 05/06/2014 05:49 PM, Fabio Estevam wrote:
> On Tue, May 6, 2014 at 12:36 PM, Fabio Estevam <festevam@gmail.com> wrote:
>
>> Indeed, if I revert:
>>
>> commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
>> Author: Maxime COQUELIN <maxime.coquelin@st.com>
>> Date:   Wed Jan 29 17:24:08 2014 +0100
>>
>>      clk: divider: Optimize clk_divider_bestdiv loop
>>
>>      Currently, the for-loop used to try all the different dividers to find the
>>      one that best fit tries all the values from 1 to max_div,
>> incrementing by one.
>>      In case of power-of-two, or table based divider, the loop isn't optimal.
>>
>>      Instead of incrementing by one, this patch provides directly the
>> next divider.
>>
>>      Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>      Signed-off-by: Mike Turquette <mturquette@linaro.org>
>>
>> Then the board does not hang.
>
> Isn't the increment of i missing?

i is incremented in _next_div():

+static int _next_div(struct clk_divider *divider, int div)
+{
+	div++;
+
+	if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
+		return __roundup_pow_of_two(div);
+	if (divider->table)
+		return _round_up_table(divider->table, div);
+
+	return div;
+}

Could you tell me which kind of divider it is? pow2, table, linear?

>
> --- a/drivers/clk/clk-divider.c
> +++ b/drivers/clk/clk-divider.c
> @@ -279,7 +279,7 @@ static int clk_divider_bestdiv(struct clk_hw *hw,
> unsigned long rate,
>           */
>          maxdiv = min(ULONG_MAX / rate, maxdiv);
>
> -       for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
> +       for (i = 1; i <= maxdiv; i +=_next_div(divider, i)) {
This would break power-of-two, and table based dividers.

>                  if (!_is_valid_div(divider, i))
>                          continue;
>                  if (rate * i == parent_rate_saved) {
>

Regards,
Maxime

mx6qsabresd hangs on linux-next

[Fabio Estevam]

On Tue, May 6, 2014 at 1:04 PM, Maxime Coquelin <maxime.coquelin@st.com> wrote:

> i is incremented in _next_div():

Yes, I see it now.

> +static int _next_div(struct clk_divider *divider, int div)
> +{
> +       div++;
> +
> +       if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
> +               return __roundup_pow_of_two(div);
> +       if (divider->table)
> +               return _round_up_table(divider->table, div);
> +
> +       return div;
> +}
>
> Could you tell me which kind of divider it is? pow2, table, linear?

table divider.

Regards,

Fabio Estevam

mx6qsabresd hangs on linux-next

[Sascha Hauer]

On Tue, May 06, 2014 at 06:04:45PM +0200, Maxime Coquelin wrote:
> Hi Fabio,
> 
> On 05/06/2014 05:49 PM, Fabio Estevam wrote:
> >On Tue, May 6, 2014 at 12:36 PM, Fabio Estevam <festevam@gmail.com> wrote:
> >
> >>Indeed, if I revert:
> >>
> >>commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
> >>Author: Maxime COQUELIN <maxime.coquelin@st.com>
> >>Date:   Wed Jan 29 17:24:08 2014 +0100
> >>
> >>     clk: divider: Optimize clk_divider_bestdiv loop
> >>
> >>     Currently, the for-loop used to try all the different dividers to find the
> >>     one that best fit tries all the values from 1 to max_div,
> >>incrementing by one.
> >>     In case of power-of-two, or table based divider, the loop isn't optimal.
> >>
> >>     Instead of incrementing by one, this patch provides directly the
> >>next divider.
> >>
> >>     Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> >>     Signed-off-by: Mike Turquette <mturquette@linaro.org>
> >>
> >>Then the board does not hang.
> >
> >Isn't the increment of i missing?
> 
> i is incremented in _next_div():
> 
> +static int _next_div(struct clk_divider *divider, int div)
> +{
> +	div++;
> +
> +	if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
> +		return __roundup_pow_of_two(div);
> +	if (divider->table)
> +		return _round_up_table(divider->table, div);
> +
> +	return div;
> +}

This cannot work. _round_up_table is implemented like this:

static int _round_up_table(const struct clk_div_table *table, int div)
{
	const struct clk_div_table *clkt;
	int up = _get_table_maxdiv(table);

	for (clkt = table; clkt->div; clkt++) {
		if (clkt->div == div)
			return clkt->div;
		...
	}
	...
}

Here when a table entry matches the input div this function will return
exactly the input div. This means _next_div() will always return the
same value and clk_divider_bestdiv() has an infinite loop:

	for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
		...
	}

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

mx6qsabresd hangs on linux-next

[Shawn Guo]

On Tue, May 06, 2014 at 10:00:29PM +0200, Sascha Hauer wrote:
> On Tue, May 06, 2014 at 06:04:45PM +0200, Maxime Coquelin wrote:
> > Hi Fabio,
> > 
> > On 05/06/2014 05:49 PM, Fabio Estevam wrote:
> > >On Tue, May 6, 2014 at 12:36 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > >
> > >>Indeed, if I revert:
> > >>
> > >>commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
> > >>Author: Maxime COQUELIN <maxime.coquelin@st.com>
> > >>Date:   Wed Jan 29 17:24:08 2014 +0100
> > >>
> > >>     clk: divider: Optimize clk_divider_bestdiv loop
> > >>
> > >>     Currently, the for-loop used to try all the different dividers to find the
> > >>     one that best fit tries all the values from 1 to max_div,
> > >>incrementing by one.
> > >>     In case of power-of-two, or table based divider, the loop isn't optimal.
> > >>
> > >>     Instead of incrementing by one, this patch provides directly the
> > >>next divider.
> > >>
> > >>     Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> > >>     Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > >>
> > >>Then the board does not hang.
> > >
> > >Isn't the increment of i missing?
> > 
> > i is incremented in _next_div():
> > 
> > +static int _next_div(struct clk_divider *divider, int div)
> > +{
> > +	div++;
> > +
> > +	if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
> > +		return __roundup_pow_of_two(div);
> > +	if (divider->table)
> > +		return _round_up_table(divider->table, div);
> > +
> > +	return div;
> > +}
> 
> This cannot work. _round_up_table is implemented like this:
> 
> static int _round_up_table(const struct clk_div_table *table, int div)
> {
> 	const struct clk_div_table *clkt;
> 	int up = _get_table_maxdiv(table);
> 
> 	for (clkt = table; clkt->div; clkt++) {
> 		if (clkt->div == div)
> 			return clkt->div;
> 		...
> 	}
> 	...
> }
> 
> Here when a table entry matches the input div this function will return
> exactly the input div. This means _next_div() will always return the
> same value and clk_divider_bestdiv() has an infinite loop:
> 
> 	for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
> 		...
> 	}

Hmmm, isn't the first thing that _next_div() does to increment the input
div?

Shawn

mx6qsabresd hangs on linux-next

[Shawn Guo]

On Wed, May 07, 2014 at 10:09:51AM +0800, Shawn Guo wrote:
> On Tue, May 06, 2014 at 10:00:29PM +0200, Sascha Hauer wrote:
> > On Tue, May 06, 2014 at 06:04:45PM +0200, Maxime Coquelin wrote:
> > > Hi Fabio,
> > > 
> > > On 05/06/2014 05:49 PM, Fabio Estevam wrote:
> > > >On Tue, May 6, 2014 at 12:36 PM, Fabio Estevam <festevam@gmail.com> wrote:
> > > >
> > > >>Indeed, if I revert:
> > > >>
> > > >>commit e7489693b3a853ab6dfad52f7e6af553ae8d3f28
> > > >>Author: Maxime COQUELIN <maxime.coquelin@st.com>
> > > >>Date:   Wed Jan 29 17:24:08 2014 +0100
> > > >>
> > > >>     clk: divider: Optimize clk_divider_bestdiv loop
> > > >>
> > > >>     Currently, the for-loop used to try all the different dividers to find the
> > > >>     one that best fit tries all the values from 1 to max_div,
> > > >>incrementing by one.
> > > >>     In case of power-of-two, or table based divider, the loop isn't optimal.
> > > >>
> > > >>     Instead of incrementing by one, this patch provides directly the
> > > >>next divider.
> > > >>
> > > >>     Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> > > >>     Signed-off-by: Mike Turquette <mturquette@linaro.org>
> > > >>
> > > >>Then the board does not hang.
> > > >
> > > >Isn't the increment of i missing?
> > > 
> > > i is incremented in _next_div():
> > > 
> > > +static int _next_div(struct clk_divider *divider, int div)
> > > +{
> > > +	div++;
> > > +
> > > +	if (divider->flags & CLK_DIVIDER_POWER_OF_TWO)
> > > +		return __roundup_pow_of_two(div);
> > > +	if (divider->table)
> > > +		return _round_up_table(divider->table, div);
> > > +
> > > +	return div;
> > > +}
> > 
> > This cannot work. _round_up_table is implemented like this:
> > 
> > static int _round_up_table(const struct clk_div_table *table, int div)
> > {
> > 	const struct clk_div_table *clkt;
> > 	int up = _get_table_maxdiv(table);
> > 
> > 	for (clkt = table; clkt->div; clkt++) {
> > 		if (clkt->div == div)
> > 			return clkt->div;
> > 		...
> > 	}
> > 	...
> > }
> > 
> > Here when a table entry matches the input div this function will return
> > exactly the input div. This means _next_div() will always return the
> > same value and clk_divider_bestdiv() has an infinite loop:
> > 
> > 	for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
> > 		...
> > 	}
> 
> Hmmm, isn't the first thing that _next_div() does to increment the input
> div?

I think the infinite loop happens in this case because "i" will never
exceed maxdiv for a table divider.

Shawn

mx6qsabresd hangs on linux-next

[Maxime Coquelin]

Hi Shawn,

On 05/07/2014 05:32 AM, Shawn Guo wrote:
...
>>>
>>> Here when a table entry matches the input div this function will return
>>> exactly the input div. This means _next_div() will always return the
>>> same value and clk_divider_bestdiv() has an infinite loop:
>>>
>>> 	for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
>>> 		...
>>> 	}
>>
>> Hmmm, isn't the first thing that _next_div() does to increment the input
>> div?
>
> I think the infinite loop happens in this case because "i" will never
> exceed maxdiv for a table divider.

You are right. Sorry for the regression.

I thought I tested it, as I had some table-based dividers in my test setup.
After checking again, it appears that I didn't had the 
CLK_SET_RATE_PARENT flag set on these clocks, so I didn't entered the 
for loop...

Could the fix be to initialize "up" variable to INT_MAX in 
_round_up_table (see below)?

I can send the patch if you are fine with it.
I have no hardware to test on this week, I only have compiled this 
patch, not tested it.

Thanks,
Maxime

>
> Shawn
>


diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index b3c8396..cf9114a 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -158,7 +158,7 @@ static bool _is_valid_div(struct clk_divider 
*divider, unsigned int div)
  static int _round_up_table(const struct clk_div_table *table, int div)
  {
         const struct clk_div_table *clkt;
-       int up = _get_table_maxdiv(table);
+       int up = INT_MAX;

         for (clkt = table; clkt->div; clkt++) {
                 if (clkt->div == div)

mx6qsabresd hangs on linux-next

[Fabio Estevam]

Hi Maxime,

On Wed, May 7, 2014 at 5:32 AM, Maxime Coquelin <maxime.coquelin@st.com> wrote:

> You are right. Sorry for the regression.
>
> I thought I tested it, as I had some table-based dividers in my test setup.
> After checking again, it appears that I didn't had the CLK_SET_RATE_PARENT
> flag set on these clocks, so I didn't entered the for loop...
>
> Could the fix be to initialize "up" variable to INT_MAX in _round_up_table
> (see below)?

This works fine for me:

Tested-by: Fabio Estevam <fabio.estevam@freescale.com>

> I can send the patch if you are fine with it.
> I have no hardware to test on this week, I only have compiled this patch,
> not tested it.

Please submit your patch, thanks.

mx6qsabresd hangs on linux-next

[Maxime Coquelin]

Hi Fabio,

On 05/07/2014 04:39 PM, Fabio Estevam wrote:
> Hi Maxime,
>
> On Wed, May 7, 2014 at 5:32 AM, Maxime Coquelin <maxime.coquelin@st.com> wrote:
>
>> You are right. Sorry for the regression.
>>
>> I thought I tested it, as I had some table-based dividers in my test setup.
>> After checking again, it appears that I didn't had the CLK_SET_RATE_PARENT
>> flag set on these clocks, so I didn't entered the for loop...
>>
>> Could the fix be to initialize "up" variable to INT_MAX in _round_up_table
>> (see below)?
>
> This works fine for me:
>
> Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
>
>> I can send the patch if you are fine with it.
>> I have no hardware to test on this week, I only have compiled this patch,
>> not tested it.
>
> Please submit your patch, thanks.
Thanks for testing it, patch sent.

Regards,
Maxime

>

mx6qsabresd hangs on linux-next

[Sascha Hauer]

On Wed, May 07, 2014 at 10:09:51AM +0800, Shawn Guo wrote:
> > Here when a table entry matches the input div this function will return
> > exactly the input div. This means _next_div() will always return the
> > same value and clk_divider_bestdiv() has an infinite loop:
> > 
> > 	for (i = 1; i <= maxdiv; i = _next_div(divider, i)) {
> > 		...
> > 	}
> 
> Hmmm, isn't the first thing that _next_div() does to increment the input
> div?

Sorry, yes. You're right.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |