| Message ID | 1423720903-24806-2-git-send-email-Ying.Liu@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > If no best divider is normally found, we will try to use the maximum divider. > We should not set the parent clock rate to be 1Hz by force for being rounded. > Instead, we should take the maximum divider as a base and calculate a correct > parent clock rate for being rounded. Please add an explanation why you think the current code is wrong and what this actually fixes, maybe an example? > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > index c0a842b..f641d4b 100644 > --- a/drivers/clk/clk-divider.c > +++ b/drivers/clk/clk-divider.c > @@ -311,7 +311,8 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, > > if (!bestdiv) { > bestdiv = _get_maxdiv(divider); > - *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), 1); > + *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), > + MULT_ROUND_UP(rate, bestdiv)); When getting into the if(!bestdiv) it means that the lowest possible rate we can archieve is still higher than the target rate, so setting the parent rate as low as possible seems sane to me. Why do you think this is wrong? In which case this even makes a difference? Sascha
On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > If no best divider is normally found, we will try to use the maximum divider. > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > Instead, we should take the maximum divider as a base and calculate a correct > > parent clock rate for being rounded. > > Please add an explanation why you think the current code is wrong and > what this actually fixes, maybe an example? The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on the MX6DL SabreSD board. These are the clock tree summaries with or without the patch applied: 1) With the patch applied: pll5_bypass_src 1 1 24000000 0 0 pll5 1 1 844800048 0 0 pll5_bypass 1 1 844800048 0 0 pll5_video 1 1 844800048 0 0 pll5_post_div 1 1 211200012 0 0 pll5_video_div 1 1 211200012 0 0 ipu1_di0_pre_sel 1 1 211200012 0 0 ipu1_di0_pre 1 1 26400002 0 0 ipu1_di0_sel 1 1 26400002 0 0 ipu1_di0 1 1 26400002 0 0 2) Without the patch applied: pll5_bypass_src 1 1 24000000 0 0 pll5 1 1 648000000 0 0 pll5_bypass 1 1 648000000 0 0 pll5_video 1 1 648000000 0 0 pll5_post_div 1 1 162000000 0 0 pll5_video_div 1 1 40500000 0 0 ipu1_di0_pre_sel 1 1 40500000 0 0 ipu1_di0_pre 1 1 20250000 0 0 ipu1_di0_sel 1 1 20250000 0 0 ipu1_di0 1 1 20250000 0 0 > > > diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c > > index c0a842b..f641d4b 100644 > > --- a/drivers/clk/clk-divider.c > > +++ b/drivers/clk/clk-divider.c > > @@ -311,7 +311,8 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, > > > > if (!bestdiv) { > > bestdiv = _get_maxdiv(divider); > > - *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), 1); > > + *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), > > + MULT_ROUND_UP(rate, bestdiv)); > > When getting into the if(!bestdiv) it means that the lowest possible > rate we can archieve is still higher than the target rate, so setting > the parent rate as low as possible seems sane to me. Why do you think > this is wrong? In which case this even makes a difference? We still should take the little left chance to get a closest target clock rate we want(26.4MHz, in the example case above), so it looks that the lowest parent clock rate for being rounded should be MULT_ROUND_UP(rate, bestdiv) instead of 1Hz. Regards, Liu Ying > > 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 |
On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > If no best divider is normally found, we will try to use the maximum divider. > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > Instead, we should take the maximum divider as a base and calculate a correct > > > parent clock rate for being rounded. > > > > Please add an explanation why you think the current code is wrong and > > what this actually fixes, maybe an example? > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > the MX6DL SabreSD board. > > These are the clock tree summaries with or without the patch applied: > 1) With the patch applied: > pll5_bypass_src 1 1 24000000 0 0 > pll5 1 1 844800048 0 0 > pll5_bypass 1 1 844800048 0 0 > pll5_video 1 1 844800048 0 0 > pll5_post_div 1 1 211200012 0 0 > pll5_video_div 1 1 211200012 0 0 > ipu1_di0_pre_sel 1 1 211200012 0 0 > ipu1_di0_pre 1 1 26400002 0 0 > ipu1_di0_sel 1 1 26400002 0 0 > ipu1_di0 1 1 26400002 0 0 > > 2) Without the patch applied: > pll5_bypass_src 1 1 24000000 0 0 > pll5 1 1 648000000 0 0 > pll5_bypass 1 1 648000000 0 0 > pll5_video 1 1 648000000 0 0 > pll5_post_div 1 1 162000000 0 0 > pll5_video_div 1 1 40500000 0 0 > ipu1_di0_pre_sel 1 1 40500000 0 0 > ipu1_di0_pre 1 1 20250000 0 0 > ipu1_di0_sel 1 1 20250000 0 0 > ipu1_di0 1 1 20250000 0 0 This seems to be broken since: | commit b11d282dbea27db1788893115dfca8a7856bf205 | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> | Date: Thu Feb 13 12:03:59 2014 +0200 | | clk: divider: fix rate calculation for fractional rates This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted in a lower frequency than clk_round_rate(rate) returned. Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to the rest of the divider. Maybe this should be a simple rate * i now, but I'm unsure what side effects this has. I think your patch only fixes the behaviour in your case by accident, it's not a correct fix for this issue. Sascha
On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > parent clock rate for being rounded. > > > > > > Please add an explanation why you think the current code is wrong and > > > what this actually fixes, maybe an example? > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > the MX6DL SabreSD board. > > > > These are the clock tree summaries with or without the patch applied: > > 1) With the patch applied: > > pll5_bypass_src 1 1 24000000 0 0 > > pll5 1 1 844800048 0 0 > > pll5_bypass 1 1 844800048 0 0 > > pll5_video 1 1 844800048 0 0 > > pll5_post_div 1 1 211200012 0 0 > > pll5_video_div 1 1 211200012 0 0 > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > ipu1_di0_pre 1 1 26400002 0 0 > > ipu1_di0_sel 1 1 26400002 0 0 > > ipu1_di0 1 1 26400002 0 0 > > > > 2) Without the patch applied: > > pll5_bypass_src 1 1 24000000 0 0 > > pll5 1 1 648000000 0 0 > > pll5_bypass 1 1 648000000 0 0 > > pll5_video 1 1 648000000 0 0 > > pll5_post_div 1 1 162000000 0 0 > > pll5_video_div 1 1 40500000 0 0 > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > ipu1_di0_pre 1 1 20250000 0 0 > > ipu1_di0_sel 1 1 20250000 0 0 > > ipu1_di0 1 1 20250000 0 0 > > This seems to be broken since: > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > | Date: Thu Feb 13 12:03:59 2014 +0200 > | > | clk: divider: fix rate calculation for fractional rates > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > in a lower frequency than clk_round_rate(rate) returned. > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > the rest of the divider. Maybe this should be a simple rate * i now, but > I'm unsure what side effects this has. > > I think your patch only fixes the behaviour in your case by accident, > it's not a correct fix for this issue. Well, it's defined that: new_rate = clk_round_rate(clk, rate); returns the rate which you would get if you did: clk_set_rate(clk, rate); new_rate = clk_get_rate(clk); The reasoning here is that clk_round_rate() gives you a way to query what rate you would get if you were to ask for the rate to be set, without effecting a change in the hardware. The idea that you should call clk_round_rate() first before clk_set_rate() and pass the returned rounded rate into clk_set_rate() is really idiotic given that. Please don't do it, and please remove code which does it, and in review comment on it. Thanks.
On Thu, Feb 12, 2015 at 12:56:46PM +0000, Russell King - ARM Linux wrote: > On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > > parent clock rate for being rounded. > > > > > > > > Please add an explanation why you think the current code is wrong and > > > > what this actually fixes, maybe an example? > > > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > > the MX6DL SabreSD board. > > > > > > These are the clock tree summaries with or without the patch applied: > > > 1) With the patch applied: > > > pll5_bypass_src 1 1 24000000 0 0 > > > pll5 1 1 844800048 0 0 > > > pll5_bypass 1 1 844800048 0 0 > > > pll5_video 1 1 844800048 0 0 > > > pll5_post_div 1 1 211200012 0 0 > > > pll5_video_div 1 1 211200012 0 0 > > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > > ipu1_di0_pre 1 1 26400002 0 0 > > > ipu1_di0_sel 1 1 26400002 0 0 > > > ipu1_di0 1 1 26400002 0 0 > > > > > > 2) Without the patch applied: > > > pll5_bypass_src 1 1 24000000 0 0 > > > pll5 1 1 648000000 0 0 > > > pll5_bypass 1 1 648000000 0 0 > > > pll5_video 1 1 648000000 0 0 > > > pll5_post_div 1 1 162000000 0 0 > > > pll5_video_div 1 1 40500000 0 0 > > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > > ipu1_di0_pre 1 1 20250000 0 0 > > > ipu1_di0_sel 1 1 20250000 0 0 > > > ipu1_di0 1 1 20250000 0 0 > > > > This seems to be broken since: > > > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > > | Date: Thu Feb 13 12:03:59 2014 +0200 > > | > > | clk: divider: fix rate calculation for fractional rates > > > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > > in a lower frequency than clk_round_rate(rate) returned. > > > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > > the rest of the divider. Maybe this should be a simple rate * i now, but > > I'm unsure what side effects this has. > > > > I think your patch only fixes the behaviour in your case by accident, > > it's not a correct fix for this issue. > > Well, it's defined that: > > new_rate = clk_round_rate(clk, rate); > > returns the rate which you would get if you did: > > clk_set_rate(clk, rate); > new_rate = clk_get_rate(clk); > > The reasoning here is that clk_round_rate() gives you a way to query what > rate you would get if you were to ask for the rate to be set, without > effecting a change in the hardware. > > The idea that you should call clk_round_rate() first before clk_set_rate() > and pass the returned rounded rate into clk_set_rate() is really idiotic > given that. Please don't do it, and please remove code which does it, and > in review comment on it. Thanks. Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) is equal to clk_round_rate(rate). So when this assumption is wrong then it should simply be reverted. So Liu, could you test if reverting Tomis patch fixes your problem? Sascha
On Thu, Feb 12, 2015 at 02:41:31PM +0100, Sascha Hauer wrote: > On Thu, Feb 12, 2015 at 12:56:46PM +0000, Russell King - ARM Linux wrote: > > On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > > > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > > > parent clock rate for being rounded. > > > > > > > > > > Please add an explanation why you think the current code is wrong and > > > > > what this actually fixes, maybe an example? > > > > > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > > > the MX6DL SabreSD board. > > > > > > > > These are the clock tree summaries with or without the patch applied: > > > > 1) With the patch applied: > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > pll5 1 1 844800048 0 0 > > > > pll5_bypass 1 1 844800048 0 0 > > > > pll5_video 1 1 844800048 0 0 > > > > pll5_post_div 1 1 211200012 0 0 > > > > pll5_video_div 1 1 211200012 0 0 > > > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > > > ipu1_di0_pre 1 1 26400002 0 0 > > > > ipu1_di0_sel 1 1 26400002 0 0 > > > > ipu1_di0 1 1 26400002 0 0 > > > > > > > > 2) Without the patch applied: > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > pll5 1 1 648000000 0 0 > > > > pll5_bypass 1 1 648000000 0 0 > > > > pll5_video 1 1 648000000 0 0 > > > > pll5_post_div 1 1 162000000 0 0 > > > > pll5_video_div 1 1 40500000 0 0 > > > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > > > ipu1_di0_pre 1 1 20250000 0 0 > > > > ipu1_di0_sel 1 1 20250000 0 0 > > > > ipu1_di0 1 1 20250000 0 0 > > > > > > This seems to be broken since: > > > > > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > > > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > | Date: Thu Feb 13 12:03:59 2014 +0200 > > > | > > > | clk: divider: fix rate calculation for fractional rates > > > > > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > > > in a lower frequency than clk_round_rate(rate) returned. > > > > > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > > > the rest of the divider. Maybe this should be a simple rate * i now, but > > > I'm unsure what side effects this has. > > > > > > I think your patch only fixes the behaviour in your case by accident, > > > it's not a correct fix for this issue. > > > > Well, it's defined that: > > > > new_rate = clk_round_rate(clk, rate); > > > > returns the rate which you would get if you did: > > > > clk_set_rate(clk, rate); > > new_rate = clk_get_rate(clk); > > > > The reasoning here is that clk_round_rate() gives you a way to query what > > rate you would get if you were to ask for the rate to be set, without > > effecting a change in the hardware. > > > > The idea that you should call clk_round_rate() first before clk_set_rate() > > and pass the returned rounded rate into clk_set_rate() is really idiotic > > given that. Please don't do it, and please remove code which does it, and > > in review comment on it. Thanks. > > Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) > is equal to clk_round_rate(rate). So when this assumption is wrong then > it should simply be reverted. > So Liu, could you test if reverting Tomis patch fixes your problem? Yes, I'll test tomorrow when I have access to my board. Regards, Liu Ying > > 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 |
On Thu, Feb 12, 2015 at 10:06:27PM +0800, Liu Ying wrote: > On Thu, Feb 12, 2015 at 02:41:31PM +0100, Sascha Hauer wrote: > > On Thu, Feb 12, 2015 at 12:56:46PM +0000, Russell King - ARM Linux wrote: > > > On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > > > > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > > > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > > > > parent clock rate for being rounded. > > > > > > > > > > > > Please add an explanation why you think the current code is wrong and > > > > > > what this actually fixes, maybe an example? > > > > > > > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > > > > the MX6DL SabreSD board. > > > > > > > > > > These are the clock tree summaries with or without the patch applied: > > > > > 1) With the patch applied: > > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > > pll5 1 1 844800048 0 0 > > > > > pll5_bypass 1 1 844800048 0 0 > > > > > pll5_video 1 1 844800048 0 0 > > > > > pll5_post_div 1 1 211200012 0 0 > > > > > pll5_video_div 1 1 211200012 0 0 > > > > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > > > > ipu1_di0_pre 1 1 26400002 0 0 > > > > > ipu1_di0_sel 1 1 26400002 0 0 > > > > > ipu1_di0 1 1 26400002 0 0 > > > > > > > > > > 2) Without the patch applied: > > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > > pll5 1 1 648000000 0 0 > > > > > pll5_bypass 1 1 648000000 0 0 > > > > > pll5_video 1 1 648000000 0 0 > > > > > pll5_post_div 1 1 162000000 0 0 > > > > > pll5_video_div 1 1 40500000 0 0 > > > > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > > > > ipu1_di0_pre 1 1 20250000 0 0 > > > > > ipu1_di0_sel 1 1 20250000 0 0 > > > > > ipu1_di0 1 1 20250000 0 0 > > > > > > > > This seems to be broken since: > > > > > > > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > > > > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > | Date: Thu Feb 13 12:03:59 2014 +0200 > > > > | > > > > | clk: divider: fix rate calculation for fractional rates > > > > > > > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > > > > in a lower frequency than clk_round_rate(rate) returned. > > > > > > > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > > > > the rest of the divider. Maybe this should be a simple rate * i now, but > > > > I'm unsure what side effects this has. > > > > > > > > I think your patch only fixes the behaviour in your case by accident, > > > > it's not a correct fix for this issue. > > > > > > Well, it's defined that: > > > > > > new_rate = clk_round_rate(clk, rate); > > > > > > returns the rate which you would get if you did: > > > > > > clk_set_rate(clk, rate); > > > new_rate = clk_get_rate(clk); > > > > > > The reasoning here is that clk_round_rate() gives you a way to query what > > > rate you would get if you were to ask for the rate to be set, without > > > effecting a change in the hardware. > > > > > > The idea that you should call clk_round_rate() first before clk_set_rate() > > > and pass the returned rounded rate into clk_set_rate() is really idiotic > > > given that. Please don't do it, and please remove code which does it, and > > > in review comment on it. Thanks. > > > > Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) > > is equal to clk_round_rate(rate). So when this assumption is wrong then > > it should simply be reverted. > > So Liu, could you test if reverting Tomis patch fixes your problem? > > Yes, I'll test tomorrow when I have access to my board. Tomi's patch cannot be reverted directly because of conflicts with the later patches. So, I revert all the clock divider driver patches on top of that. And, yes, after reverting Tomi's patch, I may get the correct 26.4MHz pixel clock rate. Regards, Liu Ying > > Regards, > Liu Ying > > > > > 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 | > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
Travis liked your message with Boxer for Android. On Feb 12, 2015 8:58 PM, Liu Ying <Ying.Liu@freescale.com> wrote: > > On Thu, Feb 12, 2015 at 10:06:27PM +0800, Liu Ying wrote: > > On Thu, Feb 12, 2015 at 02:41:31PM +0100, Sascha Hauer wrote: > > > On Thu, Feb 12, 2015 at 12:56:46PM +0000, Russell King - ARM Linux wrote: > > > > On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > > > > > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > > > > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > > > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > > > > > parent clock rate for being rounded. > > > > > > > > > > > > > > Please add an explanation why you think the current code is wrong and > > > > > > > what this actually fixes, maybe an example? > > > > > > > > > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > > > > > the MX6DL SabreSD board. > > > > > > > > > > > > These are the clock tree summaries with or without the patch applied: > > > > > > 1) With the patch applied: > > > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > > > pll5 1 1 844800048 0 0 > > > > > > pll5_bypass 1 1 844800048 0 0 > > > > > > pll5_video 1 1 844800048 0 0 > > > > > > pll5_post_div 1 1 211200012 0 0 > > > > > > pll5_video_div 1 1 211200012 0 0 > > > > > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > > > > > ipu1_di0_pre 1 1 26400002 0 0 > > > > > > ipu1_di0_sel 1 1 26400002 0 0 > > > > > > ipu1_di0 1 1 26400002 0 0 > > > > > > > > > > > > 2) Without the patch applied: > > > > > > pll5_bypass_src 1 1 24000000 0 0 > > > > > > pll5 1 1 648000000 0 0 > > > > > > pll5_bypass 1 1 648000000 0 0 > > > > > > pll5_video 1 1 648000000 0 0 > > > > > > pll5_post_div 1 1 162000000 0 0 > > > > > > pll5_video_div 1 1 40500000 0 0 > > > > > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > > > > > ipu1_di0_pre 1 1 20250000 0 0 > > > > > > ipu1_di0_sel 1 1 20250000 0 0 > > > > > > ipu1_di0 1 1 20250000 0 0 > > > > > > > > > > This seems to be broken since: > > > > > > > > > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > > > > > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > > > > > | Date: Thu Feb 13 12:03:59 2014 +0200 > > > > > | > > > > > | clk: divider: fix rate calculation for fractional rates > > > > > > > > > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > > > > > in a lower frequency than clk_round_rate(rate) returned. > > > > > > > > > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > > > > > the rest of the divider. Maybe this should be a simple rate * i now, but > > > > > I'm unsure what side effects this has. > > > > > > > > > > I think your patch only fixes the behaviour in your case by accident, > > > > > it's not a correct fix for this issue. > > > > > > > > Well, it's defined that: > > > > > > > > new_rate = clk_round_rate(clk, rate); > > > > > > > > returns the rate which you would get if you did: > > > > > > > > clk_set_rate(clk, rate); > > > > new_rate = clk_get_rate(clk); > > > > > > > > The reasoning here is that clk_round_rate() gives you a way to query what > > > > rate you would get if you were to ask for the rate to be set, without > > > > effecting a change in the hardware. > > > > > > > > The idea that you should call clk_round_rate() first before clk_set_rate() > > > > and pass the returned rounded rate into clk_set_rate() is really idiotic > > > > given that. Please don't do it, and please remove code which does it, and > > > > in review comment on it. Thanks. > > > > > > Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) > > > is equal to clk_round_rate(rate). So when this assumption is wrong then > > > it should simply be reverted. > > > So Liu, could you test if reverting Tomis patch fixes your problem? > > > > Yes, I'll test tomorrow when I have access to my board. > > Tomi's patch cannot be reverted directly because of conflicts with the later > patches. So, I revert all the clock divider driver patches on top of that. > And, yes, after reverting Tomi's patch, I may get the correct 26.4MHz pixel > clock rate. > > Regards, > Liu Ying > > > > > Regards, > > Liu Ying > > > > > > > > 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 | > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
On 12/02/15 15:41, Sascha Hauer wrote: > Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) > is equal to clk_round_rate(rate). So when this assumption is wrong then > it should simply be reverted. When is it not equal? I agree that doing clk_set_rate(clk, clk_round_rate(clk, rate)) is pointless, but shouldn't it still work? And we can forget about clk_round_rate. Without my patch, this would behave oddly also: rate = clk_get_rate(clk); clk_set_rate(clk, rate); The end result could be something else than 'rate'. Tomi
On Fri, Feb 13, 2015 at 04:35:36PM +0200, Tomi Valkeinen wrote: > On 12/02/15 15:41, Sascha Hauer wrote: > > > Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) > > is equal to clk_round_rate(rate). So when this assumption is wrong then > > it should simply be reverted. > > When is it not equal? > > I agree that doing clk_set_rate(clk, clk_round_rate(clk, rate)) is > pointless, but shouldn't it still work? > > And we can forget about clk_round_rate. Without my patch, this would > behave oddly also: > > rate = clk_get_rate(clk); > clk_set_rate(clk, rate); > > The end result could be something else than 'rate'. I agree that it's a bit odd, but I think it has to be like this. Consider that you request a rate of 100Hz, but the clock can only produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. Now when you request 99Hz from clk_set_rate() the 99.5Hz value can't be used because it's too high. Sascha
On 13/02/15 20:57, Sascha Hauer wrote: > On Fri, Feb 13, 2015 at 04:35:36PM +0200, Tomi Valkeinen wrote: >> On 12/02/15 15:41, Sascha Hauer wrote: >> >>> Tomis patch is based on the assumption that clk_set_rate(clk_round_rate(rate)) >>> is equal to clk_round_rate(rate). So when this assumption is wrong then >>> it should simply be reverted. >> >> When is it not equal? >> >> I agree that doing clk_set_rate(clk, clk_round_rate(clk, rate)) is >> pointless, but shouldn't it still work? >> >> And we can forget about clk_round_rate. Without my patch, this would >> behave oddly also: >> >> rate = clk_get_rate(clk); >> clk_set_rate(clk, rate); >> >> The end result could be something else than 'rate'. > > I agree that it's a bit odd, but I think it has to be like this. > Consider that you request a rate of 100Hz, but the clock can only > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. > Now when you request 99Hz from clk_set_rate() the 99.5Hz value > can't be used because it's too high. Would that problem better be fixed by changing the clock driver so that when asked for 99Hz, it would look for rates less than 100Hz? I think the old behavior was so odd that I would call it broken, so I hope the current problems can be fixed via some other ways than breaking it again. Tomi
On Fri, Feb 13, 2015 at 07:57:13PM +0100, Sascha Hauer wrote: > I agree that it's a bit odd, but I think it has to be like this. > Consider that you request a rate of 100Hz, but the clock can only > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. > Now when you request 99Hz from clk_set_rate() the 99.5Hz value > can't be used because it's too high. Math rounding rules normally state that anything of .5 and greater should be rounded up, not rounded down. So, for 99.5Hz, you really ought to be returning 100Hz, not 99Hz. However, you do have a point for 99.4Hz, which would be returned as 99Hz, and when set, it would result in something which isn't 99.4Hz.
Quoting Russell King - ARM Linux (2015-02-16 03:27:24) > On Fri, Feb 13, 2015 at 07:57:13PM +0100, Sascha Hauer wrote: > > I agree that it's a bit odd, but I think it has to be like this. > > Consider that you request a rate of 100Hz, but the clock can only > > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. > > Now when you request 99Hz from clk_set_rate() the 99.5Hz value > > can't be used because it's too high. > > Math rounding rules normally state that anything of .5 and greater > should be rounded up, not rounded down. So, for 99.5Hz, you really > ought to be returning 100Hz, not 99Hz. > > However, you do have a point for 99.4Hz, which would be returned as > 99Hz, and when set, it would result in something which isn't 99.4Hz. More practically, this again raises the issue of whether or not unsigned long rate should be in millihertz or something other than hertz. And then that question again raises the issue of making rate 64-bit... Regards, Mike > > -- > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up > according to speedtest.net.
On Fri, Feb 20, 2015 at 11:13:06AM -0800, Mike Turquette wrote: > Quoting Russell King - ARM Linux (2015-02-16 03:27:24) > > On Fri, Feb 13, 2015 at 07:57:13PM +0100, Sascha Hauer wrote: > > > I agree that it's a bit odd, but I think it has to be like this. > > > Consider that you request a rate of 100Hz, but the clock can only > > > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. > > > Now when you request 99Hz from clk_set_rate() the 99.5Hz value > > > can't be used because it's too high. > > > > Math rounding rules normally state that anything of .5 and greater > > should be rounded up, not rounded down. So, for 99.5Hz, you really > > ought to be returning 100Hz, not 99Hz. > > > > However, you do have a point for 99.4Hz, which would be returned as > > 99Hz, and when set, it would result in something which isn't 99.4Hz. > > More practically, this again raises the issue of whether or not unsigned > long rate should be in millihertz or something other than hertz. You still get the same issue if it's millihertz. Take 10 2/3rds Hz. Is 10.666 Hz less than 10 2/3rds Hz?
Quoting Russell King - ARM Linux (2015-02-20 11:20:43) > On Fri, Feb 20, 2015 at 11:13:06AM -0800, Mike Turquette wrote: > > Quoting Russell King - ARM Linux (2015-02-16 03:27:24) > > > On Fri, Feb 13, 2015 at 07:57:13PM +0100, Sascha Hauer wrote: > > > > I agree that it's a bit odd, but I think it has to be like this. > > > > Consider that you request a rate of 100Hz, but the clock can only > > > > produce 99.5Hz, so due to rounding clk_round_rate() returns 99Hz. > > > > Now when you request 99Hz from clk_set_rate() the 99.5Hz value > > > > can't be used because it's too high. > > > > > > Math rounding rules normally state that anything of .5 and greater > > > should be rounded up, not rounded down. So, for 99.5Hz, you really > > > ought to be returning 100Hz, not 99Hz. > > > > > > However, you do have a point for 99.4Hz, which would be returned as > > > 99Hz, and when set, it would result in something which isn't 99.4Hz. > > > > More practically, this again raises the issue of whether or not unsigned > > long rate should be in millihertz or something other than hertz. > > You still get the same issue if it's millihertz. Take 10 2/3rds Hz. > Is 10.666 Hz less than 10 2/3rds Hz? Millihertz won't solve rounding problems, I agree. But we currently do not support any fractional rates and we should. Regards, Mike > > -- > FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up > according to speedtest.net.
Hello, On Thu, Feb 12, 2015 at 01:24:05PM +0100, Sascha Hauer wrote: > On Thu, Feb 12, 2015 at 06:39:45PM +0800, Liu Ying wrote: > > On Thu, Feb 12, 2015 at 10:33:56AM +0100, Sascha Hauer wrote: > > > On Thu, Feb 12, 2015 at 02:01:24PM +0800, Liu Ying wrote: > > > > If no best divider is normally found, we will try to use the maximum divider. > > > > We should not set the parent clock rate to be 1Hz by force for being rounded. > > > > Instead, we should take the maximum divider as a base and calculate a correct > > > > parent clock rate for being rounded. > > > > > > Please add an explanation why you think the current code is wrong and > > > what this actually fixes, maybe an example? > > > > The MIPI DSI panel's pixel clock rate is 26.4MHz and it's derived from PLL5 on > > the MX6DL SabreSD board. > > > > These are the clock tree summaries with or without the patch applied: > > 1) With the patch applied: > > pll5_bypass_src 1 1 24000000 0 0 > > pll5 1 1 844800048 0 0 > > pll5_bypass 1 1 844800048 0 0 > > pll5_video 1 1 844800048 0 0 > > pll5_post_div 1 1 211200012 0 0 > > pll5_video_div 1 1 211200012 0 0 > > ipu1_di0_pre_sel 1 1 211200012 0 0 > > ipu1_di0_pre 1 1 26400002 0 0 > > ipu1_di0_sel 1 1 26400002 0 0 > > ipu1_di0 1 1 26400002 0 0 > > > > 2) Without the patch applied: > > pll5_bypass_src 1 1 24000000 0 0 > > pll5 1 1 648000000 0 0 > > pll5_bypass 1 1 648000000 0 0 > > pll5_video 1 1 648000000 0 0 > > pll5_post_div 1 1 162000000 0 0 > > pll5_video_div 1 1 40500000 0 0 > > ipu1_di0_pre_sel 1 1 40500000 0 0 > > ipu1_di0_pre 1 1 20250000 0 0 > > ipu1_di0_sel 1 1 20250000 0 0 > > ipu1_di0 1 1 20250000 0 0 > > This seems to be broken since: > > | commit b11d282dbea27db1788893115dfca8a7856bf205 > | Author: Tomi Valkeinen <tomi.valkeinen@ti.com> > | Date: Thu Feb 13 12:03:59 2014 +0200 > | > | clk: divider: fix rate calculation for fractional rates > > This patch fixed a case when clk_set_rate(clk_round_rate(rate)) resulted > in a lower frequency than clk_round_rate(rate) returned. > > Since then the MULT_ROUND_UP in clk_divider_bestdiv() is inconsistent to > the rest of the divider. Maybe this should be a simple rate * i now, but > I'm unsure what side effects this has. I think this is the right fix that I found, too, while fixing all problems that your test code with the three dividers shows (at least the subset of all problem I was able to identify). > I think your patch only fixes the behaviour in your case by accident, > it's not a correct fix for this issue. Right. I will follow up with a patch (and two more that fix different things). Best regards Uwe
diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c index c0a842b..f641d4b 100644 --- a/drivers/clk/clk-divider.c +++ b/drivers/clk/clk-divider.c @@ -311,7 +311,8 @@ static int clk_divider_bestdiv(struct clk_hw *hw, unsigned long rate, if (!bestdiv) { bestdiv = _get_maxdiv(divider); - *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), 1); + *best_parent_rate = __clk_round_rate(__clk_get_parent(hw->clk), + MULT_ROUND_UP(rate, bestdiv)); } return bestdiv;
If no best divider is normally found, we will try to use the maximum divider. We should not set the parent clock rate to be 1Hz by force for being rounded. Instead, we should take the maximum divider as a base and calculate a correct parent clock rate for being rounded. Signed-off-by: Liu Ying <Ying.Liu@freescale.com> --- v8->v9: * Rebase onto the imx-drm/next branch of Philipp Zabel's open git repository. v7->v8: * None. v6->v7: * None. v5->v6: * None. v4->v5: * None. v3->v4: * None. v2->v3: * None. v1->v2: * None. drivers/clk/clk-divider.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)