| 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
Hello,
TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST.
I stared at clk-divider.c for some time now given Sascha's failing test
case. I found a fix for the failure (which happens to be what Sascha
suspected).
The other two patches fix problems only present when handling dividers
that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still
heavily broken however. So having a 4bit-divider and a parent clk of
10000 (as in Sascha's test case) requesting
clk_set_rate(clk, 666)
sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the
choice of parent_rate in clk_divider_bestdiv's loop is wrong for
CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is
non-trivial and for sure more than one rate must be tested here. This is
complicated by the fact that clk_round_rate might return a value bigger
than the requested rate which convinces me (once more) that it's a bad
idea to allow that. Even if this was fixed for .round_rate,
clk_divider_set_rate is still broken because it also uses
div = DIV_ROUND_UP(parent_rate, rate);
to calculate the (pretended) best divider to get near rate.
Note this makes at least two reasons to remove support for
CLK_DIVIDER_ROUND_CLOSEST!
Instead I'd favour creating a function
clk_round_rate_nearest
as was suggested some time ago by Soren Brinkmann and me[1] that doesn't
need any clk type specific knowledge. This would mean that not the
divider (or clk in general) would have to know that returning a slightly
bigger rate than requested is OK but the caller which is fine (and even
better) in my eyes. This would simplify clk-divider.c (and probably
others) and give support for "nearest match" for all clock types without
type specific implementation. (Note that it might even make sense to use
a different metric for "nearest", instead of minimizing
abs(target - rate)
you might want to minimize
abs(target / rate - 1)
instead.
Converting the clk framework to 64 bit rates was discussed earlier
already, too, and I wonder if we should fix rounding issues (a bit) in
the same transition such that
clk_set_rate(clk, 333)
allows the clk to be set to 333.3333333333 Hz and let clk_get_rate
return 333 in this case.
Also I'd vote to return 0 or -ESOMETHING if a requested rate is too low
to be set. This would simplify some special casing I think and makes the
request
clk_round_rate(clk, x) <= x
consistent.
Best regards
Uwe
[1] https://lkml.org/lkml/2014/5/14/698
Uwe Kleine-König (3):
clk: divider: fix calculation of maximal parent rate for a given
divider
clk: divider: fix selection of divider when rounding to closest
clk: divider: fix calculation of initial best divider when rounding to
closest
drivers/clk/clk-divider.c | 27 +++++++++++++--------------
1 file changed, 13 insertions(+), 14 deletions(-)
On Sat, Feb 21, 2015 at 11:40:22AM +0100, Uwe Kleine-König wrote: > Hello, > > TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > > I stared at clk-divider.c for some time now given Sascha's failing test > case. I found a fix for the failure (which happens to be what Sascha > suspected). > > The other two patches fix problems only present when handling dividers > that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > heavily broken however. So having a 4bit-divider and a parent clk of > 10000 (as in Sascha's test case) requesting > > clk_set_rate(clk, 666) > > sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > choice of parent_rate in clk_divider_bestdiv's loop is wrong for > CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > non-trivial and for sure more than one rate must be tested here. This is > complicated by the fact that clk_round_rate might return a value bigger > than the requested rate which convinces me (once more) that it's a bad > idea to allow that. Even if this was fixed for .round_rate, > clk_divider_set_rate is still broken because it also uses > > div = DIV_ROUND_UP(parent_rate, rate); > > to calculate the (pretended) best divider to get near rate. > > Note this makes at least two reasons to remove support for > CLK_DIVIDER_ROUND_CLOSEST! > > Instead I'd favour creating a function > > clk_round_rate_nearest Full ack. It's a clock consumer who wants to decide the rounding strategy, not the clock itself and for sure not a specific entity of the clock tree. CLK_DIVIDER_ROUND_CLOSEST should be dropped. Sascha
Quoting Uwe Kleine-König (2015-02-21 02:40:22) > Hello, > > TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > > I stared at clk-divider.c for some time now given Sascha's failing test > case. I found a fix for the failure (which happens to be what Sascha > suspected). > > The other two patches fix problems only present when handling dividers > that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > heavily broken however. So having a 4bit-divider and a parent clk of > 10000 (as in Sascha's test case) requesting > > clk_set_rate(clk, 666) > > sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > choice of parent_rate in clk_divider_bestdiv's loop is wrong for > CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > non-trivial and for sure more than one rate must be tested here. This is > complicated by the fact that clk_round_rate might return a value bigger > than the requested rate which convinces me (once more) that it's a bad > idea to allow that. Even if this was fixed for .round_rate, > clk_divider_set_rate is still broken because it also uses > > div = DIV_ROUND_UP(parent_rate, rate); > > to calculate the (pretended) best divider to get near rate. > > Note this makes at least two reasons to remove support for > CLK_DIVIDER_ROUND_CLOSEST! > > Instead I'd favour creating a function > > clk_round_rate_nearest > > as was suggested some time ago by Soren Brinkmann and me[1] that doesn't Uwe, Thanks for the fixes. I'm thinking of taking all three for 4.0. I also agree on clk_round_rate_nearest (along with a _ceil and _floor version as well). That's something we can do for 4.1 probably. There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST: Loongson QCOM ST So now is probably the right time to remove the flag if we're going to do it. What do you think? Regards, Mike > need any clk type specific knowledge. This would mean that not the > divider (or clk in general) would have to know that returning a slightly > bigger rate than requested is OK but the caller which is fine (and even > better) in my eyes. This would simplify clk-divider.c (and probably > others) and give support for "nearest match" for all clock types without > type specific implementation. (Note that it might even make sense to use > a different metric for "nearest", instead of minimizing > > abs(target - rate) > > you might want to minimize > > abs(target / rate - 1) > > instead. > > Converting the clk framework to 64 bit rates was discussed earlier > already, too, and I wonder if we should fix rounding issues (a bit) in > the same transition such that > > clk_set_rate(clk, 333) > > allows the clk to be set to 333.3333333333 Hz and let clk_get_rate > return 333 in this case. > > Also I'd vote to return 0 or -ESOMETHING if a requested rate is too low > to be set. This would simplify some special casing I think and makes the > request > > clk_round_rate(clk, x) <= x > > consistent. > > Best regards > Uwe > > [1] https://lkml.org/lkml/2014/5/14/698 > > Uwe Kleine-König (3): > clk: divider: fix calculation of maximal parent rate for a given > divider > clk: divider: fix selection of divider when rounding to closest > clk: divider: fix calculation of initial best divider when rounding to > closest > > drivers/clk/clk-divider.c | 27 +++++++++++++-------------- > 1 file changed, 13 insertions(+), 14 deletions(-) > > -- > 2.1.4 >
Hello Mike, On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: > Quoting Uwe Kleine-König (2015-02-21 02:40:22) > > Hello, > > > > TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > > > > I stared at clk-divider.c for some time now given Sascha's failing test > > case. I found a fix for the failure (which happens to be what Sascha > > suspected). > > > > The other two patches fix problems only present when handling dividers > > that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > > heavily broken however. So having a 4bit-divider and a parent clk of > > 10000 (as in Sascha's test case) requesting > > > > clk_set_rate(clk, 666) > > > > sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > > choice of parent_rate in clk_divider_bestdiv's loop is wrong for > > CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > > non-trivial and for sure more than one rate must be tested here. This is > > complicated by the fact that clk_round_rate might return a value bigger > > than the requested rate which convinces me (once more) that it's a bad > > idea to allow that. Even if this was fixed for .round_rate, > > clk_divider_set_rate is still broken because it also uses > > > > div = DIV_ROUND_UP(parent_rate, rate); > > > > to calculate the (pretended) best divider to get near rate. > > > > Note this makes at least two reasons to remove support for > > CLK_DIVIDER_ROUND_CLOSEST! > > > > Instead I'd favour creating a function > > > > clk_round_rate_nearest > > > > as was suggested some time ago by Soren Brinkmann and me[1] that doesn't > > Uwe, > > Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > agree on clk_round_rate_nearest (along with a _ceil and _floor version > as well). That's something we can do for 4.1 probably. I'd say that we make round_rate the _floor version. I guess in most cases that already does the right thing. Also I think 4.1 is very ambitious, so my suggestion for 4.1 is: - add a WARN_ON_ONCE to clk_round_rate catching calls that return a value bigger than requested. - implement clk_round_rate_nearest using clk_round_rate and the assumption that it returns a value that is <= the requested rate. I think without that there are too many special cases to handle and probably not even a reliable way to determine the nearest rate. - while we're at it tightening the requirements for clk_round_rate let's also specify the expected rounding. I'd vote for the mathematical rounding, that is clk_round_rate(someclk, 333) explicitly is allowed to return a rate bigger than 333 as long as it is less than 333.5. At one point while developing patch 1 I had the dividers fixed for the rounding issue. I think I still have that patch somewhere so can post it as RFC. > There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST: > > Loongson > QCOM > ST > > So now is probably the right time to remove the flag if we're going to > do it. "now" is before we have clk_round_rate_nearest, right? So what do you want to do with these three users? Move them to the default implementation? Best regards Uwe
On 03/06/15 11:28, Uwe Kleine-König wrote: > Hello Mike, > > On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: >> Quoting Uwe Kleine-König (2015-02-21 02:40:22) >>> Hello, >>> >>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. >>> >>> I stared at clk-divider.c for some time now given Sascha's failing test >>> case. I found a fix for the failure (which happens to be what Sascha >>> suspected). >>> >>> The other two patches fix problems only present when handling dividers >>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still >>> heavily broken however. So having a 4bit-divider and a parent clk of >>> 10000 (as in Sascha's test case) requesting >>> >>> clk_set_rate(clk, 666) >>> >>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the >>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for >>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is >>> non-trivial and for sure more than one rate must be tested here. This is >>> complicated by the fact that clk_round_rate might return a value bigger >>> than the requested rate which convinces me (once more) that it's a bad >>> idea to allow that. Even if this was fixed for .round_rate, >>> clk_divider_set_rate is still broken because it also uses >>> >>> div = DIV_ROUND_UP(parent_rate, rate); >>> >>> to calculate the (pretended) best divider to get near rate. >>> >>> Note this makes at least two reasons to remove support for >>> CLK_DIVIDER_ROUND_CLOSEST! >>> >>> Instead I'd favour creating a function >>> >>> clk_round_rate_nearest >>> >>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't >> Uwe, >> >> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also >> agree on clk_round_rate_nearest (along with a _ceil and _floor version >> as well). That's something we can do for 4.1 probably. > I'd say that we make round_rate the _floor version. I guess in most > cases that already does the right thing. Also I think 4.1 is very > ambitious, so my suggestion for 4.1 is: > > - add a WARN_ON_ONCE to clk_round_rate catching calls that return a > value bigger than requested. > - implement clk_round_rate_nearest using clk_round_rate and the > assumption that it returns a value that is <= the requested rate. > I think without that there are too many special cases to handle and > probably not even a reliable way to determine the nearest rate. > - while we're at it tightening the requirements for clk_round_rate > let's also specify the expected rounding. I'd vote for the > mathematical rounding, that is > > clk_round_rate(someclk, 333) > > explicitly is allowed to return a rate bigger than 333 as long as it > is less than 333.5. > > At one point while developing patch 1 I had the dividers fixed for the > rounding issue. I think I still have that patch somewhere so can post it > as RFC. > Why do we need clk_round_rate_nearest? We have rate constraints now so drivers should be moving towards requesting a rate that's within a tolerance they can accept if they even care to enforce anything like that. Eventually clk_round_rate() returning a value smaller or larger than what it's called with won't matter as long as what the implementation does fits within the constraints that consumers specify. It may even be possible to remove clk_round_rate() as a consumer API.
On 03/06/15 10:57, Mike Turquette wrote: > > Uwe, > > Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > agree on clk_round_rate_nearest (along with a _ceil and _floor version > as well). That's something we can do for 4.1 probably. > > There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST: > > Loongson > QCOM > ST > > So now is probably the right time to remove the flag if we're going to > do it. > > What do you think? > I may have had to do divider round closest on qcom platforms because of the problem these patches are for. I'll have to go back and look.
On Fri, Mar 06, 2015 at 11:44:05AM -0800, Stephen Boyd wrote: > On 03/06/15 10:57, Mike Turquette wrote: > > > > Uwe, > > > > Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > > agree on clk_round_rate_nearest (along with a _ceil and _floor version > > as well). That's something we can do for 4.1 probably. > > > > There are currently 3 users of CLK_DIVIDER_ROUND_CLOSEST: > > > > Loongson > > QCOM > > ST > > > > So now is probably the right time to remove the flag if we're going to > > do it. > > > > What do you think? > > > > I may have had to do divider round closest on qcom platforms because of > the problem these patches are for. I'll have to go back and look. When you do that, also check if it works for you. I think the implementation is broken for most of the cases. Best regards Uwe
Am Freitag, den 06.03.2015, 11:40 -0800 schrieb Stephen Boyd: > On 03/06/15 11:28, Uwe Kleine-König wrote: > > Hello Mike, > > > > On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: > >> Quoting Uwe Kleine-König (2015-02-21 02:40:22) > >>> Hello, > >>> > >>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > >>> > >>> I stared at clk-divider.c for some time now given Sascha's failing test > >>> case. I found a fix for the failure (which happens to be what Sascha > >>> suspected). > >>> > >>> The other two patches fix problems only present when handling dividers > >>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > >>> heavily broken however. So having a 4bit-divider and a parent clk of > >>> 10000 (as in Sascha's test case) requesting > >>> > >>> clk_set_rate(clk, 666) > >>> > >>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > >>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for > >>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > >>> non-trivial and for sure more than one rate must be tested here. This is > >>> complicated by the fact that clk_round_rate might return a value bigger > >>> than the requested rate which convinces me (once more) that it's a bad > >>> idea to allow that. Even if this was fixed for .round_rate, > >>> clk_divider_set_rate is still broken because it also uses > >>> > >>> div = DIV_ROUND_UP(parent_rate, rate); > >>> > >>> to calculate the (pretended) best divider to get near rate. > >>> > >>> Note this makes at least two reasons to remove support for > >>> CLK_DIVIDER_ROUND_CLOSEST! > >>> > >>> Instead I'd favour creating a function > >>> > >>> clk_round_rate_nearest > >>> > >>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't > >> Uwe, > >> > >> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > >> agree on clk_round_rate_nearest (along with a _ceil and _floor version > >> as well). That's something we can do for 4.1 probably. > > I'd say that we make round_rate the _floor version. I guess in most > > cases that already does the right thing. Also I think 4.1 is very > > ambitious, so my suggestion for 4.1 is: > > > > - add a WARN_ON_ONCE to clk_round_rate catching calls that return a > > value bigger than requested. > > - implement clk_round_rate_nearest using clk_round_rate and the > > assumption that it returns a value that is <= the requested rate. > > I think without that there are too many special cases to handle and > > probably not even a reliable way to determine the nearest rate. > > - while we're at it tightening the requirements for clk_round_rate > > let's also specify the expected rounding. I'd vote for the > > mathematical rounding, that is > > > > clk_round_rate(someclk, 333) > > > > explicitly is allowed to return a rate bigger than 333 as long as it > > is less than 333.5. > > > > At one point while developing patch 1 I had the dividers fixed for the > > rounding issue. I think I still have that patch somewhere so can post it > > as RFC. > > > > Why do we need clk_round_rate_nearest? We have rate constraints now so > drivers should be moving towards requesting a rate that's within a > tolerance they can accept if they even care to enforce anything like > that. Eventually clk_round_rate() returning a value smaller or larger > than what it's called with won't matter as long as what the > implementation does fits within the constraints that consumers specify. > It may even be possible to remove clk_round_rate() as a consumer API. If I have to provide a panel pixel clock I usually want to get a rate as close as possible to the specified typical rate, but within the specified limits. Assume a panel with 70 MHz ideal pixel clock and a valid range of 60 MHz to 80 MHz. If the clock supply supports two frequencies within that range, 60 MHz and 72 MHz, I'd prefer 72 MHz to be chosen over 60 Mhz. regards Philipp
On 03/09/15 02:58, Philipp Zabel wrote: > Am Freitag, den 06.03.2015, 11:40 -0800 schrieb Stephen Boyd: >> On 03/06/15 11:28, Uwe Kleine-König wrote: >>> Hello Mike, >>> >>> On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: >>>> Quoting Uwe Kleine-König (2015-02-21 02:40:22) >>>>> Hello, >>>>> >>>>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. >>>>> >>>>> I stared at clk-divider.c for some time now given Sascha's failing test >>>>> case. I found a fix for the failure (which happens to be what Sascha >>>>> suspected). >>>>> >>>>> The other two patches fix problems only present when handling dividers >>>>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still >>>>> heavily broken however. So having a 4bit-divider and a parent clk of >>>>> 10000 (as in Sascha's test case) requesting >>>>> >>>>> clk_set_rate(clk, 666) >>>>> >>>>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the >>>>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for >>>>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is >>>>> non-trivial and for sure more than one rate must be tested here. This is >>>>> complicated by the fact that clk_round_rate might return a value bigger >>>>> than the requested rate which convinces me (once more) that it's a bad >>>>> idea to allow that. Even if this was fixed for .round_rate, >>>>> clk_divider_set_rate is still broken because it also uses >>>>> >>>>> div = DIV_ROUND_UP(parent_rate, rate); >>>>> >>>>> to calculate the (pretended) best divider to get near rate. >>>>> >>>>> Note this makes at least two reasons to remove support for >>>>> CLK_DIVIDER_ROUND_CLOSEST! >>>>> >>>>> Instead I'd favour creating a function >>>>> >>>>> clk_round_rate_nearest >>>>> >>>>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't >>>> Uwe, >>>> >>>> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also >>>> agree on clk_round_rate_nearest (along with a _ceil and _floor version >>>> as well). That's something we can do for 4.1 probably. >>> I'd say that we make round_rate the _floor version. I guess in most >>> cases that already does the right thing. Also I think 4.1 is very >>> ambitious, so my suggestion for 4.1 is: >>> >>> - add a WARN_ON_ONCE to clk_round_rate catching calls that return a >>> value bigger than requested. >>> - implement clk_round_rate_nearest using clk_round_rate and the >>> assumption that it returns a value that is <= the requested rate. >>> I think without that there are too many special cases to handle and >>> probably not even a reliable way to determine the nearest rate. >>> - while we're at it tightening the requirements for clk_round_rate >>> let's also specify the expected rounding. I'd vote for the >>> mathematical rounding, that is >>> >>> clk_round_rate(someclk, 333) >>> >>> explicitly is allowed to return a rate bigger than 333 as long as it >>> is less than 333.5. >>> >>> At one point while developing patch 1 I had the dividers fixed for the >>> rounding issue. I think I still have that patch somewhere so can post it >>> as RFC. >>> >> Why do we need clk_round_rate_nearest? We have rate constraints now so >> drivers should be moving towards requesting a rate that's within a >> tolerance they can accept if they even care to enforce anything like >> that. Eventually clk_round_rate() returning a value smaller or larger >> than what it's called with won't matter as long as what the >> implementation does fits within the constraints that consumers specify. >> It may even be possible to remove clk_round_rate() as a consumer API. > If I have to provide a panel pixel clock I usually want to get a rate as > close as possible to the specified typical rate, but within the > specified limits. > > Assume a panel with 70 MHz ideal pixel clock and a valid range of 60 MHz > to 80 MHz. If the clock supply supports two frequencies within that > range, 60 MHz and 72 MHz, I'd prefer 72 MHz to be chosen over 60 Mhz. > > Hm.. Maybe we should tweak the arguments to clk_set_range() to have a "typical" rate? So instead of the current API: int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long max) we should have int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned long typical, unsigned long max) with the semantics that we'll set the rate within the min,max constraints and try to get the rate as close to the typical rate as possible? That would match quite a few datasheets out there that specify these triplets.
Hello Stephen, On Mon, Mar 09, 2015 at 12:05:34PM -0700, Stephen Boyd wrote: > On 03/09/15 02:58, Philipp Zabel wrote: > > Am Freitag, den 06.03.2015, 11:40 -0800 schrieb Stephen Boyd: > >> On 03/06/15 11:28, Uwe Kleine-König wrote: > >>> Hello Mike, > >>> > >>> On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: > >>>> Quoting Uwe Kleine-König (2015-02-21 02:40:22) > >>>>> Hello, > >>>>> > >>>>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > >>>>> > >>>>> I stared at clk-divider.c for some time now given Sascha's failing test > >>>>> case. I found a fix for the failure (which happens to be what Sascha > >>>>> suspected). > >>>>> > >>>>> The other two patches fix problems only present when handling dividers > >>>>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > >>>>> heavily broken however. So having a 4bit-divider and a parent clk of > >>>>> 10000 (as in Sascha's test case) requesting > >>>>> > >>>>> clk_set_rate(clk, 666) > >>>>> > >>>>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > >>>>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for > >>>>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > >>>>> non-trivial and for sure more than one rate must be tested here. This is > >>>>> complicated by the fact that clk_round_rate might return a value bigger > >>>>> than the requested rate which convinces me (once more) that it's a bad > >>>>> idea to allow that. Even if this was fixed for .round_rate, > >>>>> clk_divider_set_rate is still broken because it also uses > >>>>> > >>>>> div = DIV_ROUND_UP(parent_rate, rate); > >>>>> > >>>>> to calculate the (pretended) best divider to get near rate. > >>>>> > >>>>> Note this makes at least two reasons to remove support for > >>>>> CLK_DIVIDER_ROUND_CLOSEST! > >>>>> > >>>>> Instead I'd favour creating a function > >>>>> > >>>>> clk_round_rate_nearest > >>>>> > >>>>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't > >>>> Uwe, > >>>> > >>>> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > >>>> agree on clk_round_rate_nearest (along with a _ceil and _floor version > >>>> as well). That's something we can do for 4.1 probably. > >>> I'd say that we make round_rate the _floor version. I guess in most > >>> cases that already does the right thing. Also I think 4.1 is very > >>> ambitious, so my suggestion for 4.1 is: > >>> > >>> - add a WARN_ON_ONCE to clk_round_rate catching calls that return a > >>> value bigger than requested. > >>> - implement clk_round_rate_nearest using clk_round_rate and the > >>> assumption that it returns a value that is <= the requested rate. > >>> I think without that there are too many special cases to handle and > >>> probably not even a reliable way to determine the nearest rate. > >>> - while we're at it tightening the requirements for clk_round_rate > >>> let's also specify the expected rounding. I'd vote for the > >>> mathematical rounding, that is > >>> > >>> clk_round_rate(someclk, 333) > >>> > >>> explicitly is allowed to return a rate bigger than 333 as long as it > >>> is less than 333.5. > >>> > >>> At one point while developing patch 1 I had the dividers fixed for the > >>> rounding issue. I think I still have that patch somewhere so can post it > >>> as RFC. > >>> > >> Why do we need clk_round_rate_nearest? We have rate constraints now so > >> drivers should be moving towards requesting a rate that's within a > >> tolerance they can accept if they even care to enforce anything like > >> that. Eventually clk_round_rate() returning a value smaller or larger > >> than what it's called with won't matter as long as what the > >> implementation does fits within the constraints that consumers specify. > >> It may even be possible to remove clk_round_rate() as a consumer API. > > If I have to provide a panel pixel clock I usually want to get a rate as > > close as possible to the specified typical rate, but within the > > specified limits. > > > > Assume a panel with 70 MHz ideal pixel clock and a valid range of 60 MHz > > to 80 MHz. If the clock supply supports two frequencies within that > > range, 60 MHz and 72 MHz, I'd prefer 72 MHz to be chosen over 60 Mhz. > > > > > > Hm.. Maybe we should tweak the arguments to clk_set_range() to have a > "typical" rate? So instead of the current API: > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > long max) Looking at the current implementation of clk_set_rate_range I wonder how it should be used. The "usual" code flow goes through ret = clk_core_set_rate_nolock(clk->core, clk->core->req_rate); without modifying clk->core->req_rate first. So after clk_set_rate(clk, 50); a call to clk_set_rate_range(clk, 100, 200); returns -EINVAL (at least if determine_rate isn't implemented). Is this intended? This even happens if the clk could provide a rate between 100 and 200. Also if I first call set_rate_range and then set_rate, the clk's members for min and max are unaffected by the latter call, right? > we should have > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > long typical, unsigned long max) > > with the semantics that we'll set the rate within the min,max > constraints and try to get the rate as close to the typical rate as > possible? That would match quite a few datasheets out there that specify > these triplets. And according to which metric do you determine "close"? Consider a clock that can provide 900 Hz and 1110 Hz (but nothing in between). Which one do you consider closer if 1000 Hz are requested? On the first look 900 Hz looks better because abs(1000 - 900) < abs(1000 - 1110) but if you look at the respective cycle times we get: 1 / 900 Hz = 1.111 ms 1 / 1110 Hz = 0.901 ms so 1110 Hz is better according to the metric abs(1 / 1000 - 1 / 1110) < abs(1 / 1000 - 1 / 900) . Maybe even other metrics might be sensible for certain scenarios[1]? That convinces me that it's better to be able to query a clock for possible rates (.round_rate) and get the logic to determine "close" into a single helper (well, one per metric) in the clk core. This is much better because then you have a single complex function instead of one per clk implementation. And then to keep the complexity at a sane level it would be great if you could assume e.g. that .round_rate always rounds down. Best regards Uwe [1] I first thought about minimizing abs(1 - rate / optrate), but that is equivalent to minimizing abs(optrate - rate).
Quoting Stephen Boyd (2015-03-09 12:05:34) > On 03/09/15 02:58, Philipp Zabel wrote: > > Am Freitag, den 06.03.2015, 11:40 -0800 schrieb Stephen Boyd: > >> On 03/06/15 11:28, Uwe Kleine-König wrote: > >>> Hello Mike, > >>> > >>> On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: > >>>> Quoting Uwe Kleine-König (2015-02-21 02:40:22) > >>>>> Hello, > >>>>> > >>>>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > >>>>> > >>>>> I stared at clk-divider.c for some time now given Sascha's failing test > >>>>> case. I found a fix for the failure (which happens to be what Sascha > >>>>> suspected). > >>>>> > >>>>> The other two patches fix problems only present when handling dividers > >>>>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > >>>>> heavily broken however. So having a 4bit-divider and a parent clk of > >>>>> 10000 (as in Sascha's test case) requesting > >>>>> > >>>>> clk_set_rate(clk, 666) > >>>>> > >>>>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > >>>>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for > >>>>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > >>>>> non-trivial and for sure more than one rate must be tested here. This is > >>>>> complicated by the fact that clk_round_rate might return a value bigger > >>>>> than the requested rate which convinces me (once more) that it's a bad > >>>>> idea to allow that. Even if this was fixed for .round_rate, > >>>>> clk_divider_set_rate is still broken because it also uses > >>>>> > >>>>> div = DIV_ROUND_UP(parent_rate, rate); > >>>>> > >>>>> to calculate the (pretended) best divider to get near rate. > >>>>> > >>>>> Note this makes at least two reasons to remove support for > >>>>> CLK_DIVIDER_ROUND_CLOSEST! > >>>>> > >>>>> Instead I'd favour creating a function > >>>>> > >>>>> clk_round_rate_nearest > >>>>> > >>>>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't > >>>> Uwe, > >>>> > >>>> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > >>>> agree on clk_round_rate_nearest (along with a _ceil and _floor version > >>>> as well). That's something we can do for 4.1 probably. > >>> I'd say that we make round_rate the _floor version. I guess in most > >>> cases that already does the right thing. Also I think 4.1 is very > >>> ambitious, so my suggestion for 4.1 is: > >>> > >>> - add a WARN_ON_ONCE to clk_round_rate catching calls that return a > >>> value bigger than requested. > >>> - implement clk_round_rate_nearest using clk_round_rate and the > >>> assumption that it returns a value that is <= the requested rate. > >>> I think without that there are too many special cases to handle and > >>> probably not even a reliable way to determine the nearest rate. > >>> - while we're at it tightening the requirements for clk_round_rate > >>> let's also specify the expected rounding. I'd vote for the > >>> mathematical rounding, that is > >>> > >>> clk_round_rate(someclk, 333) > >>> > >>> explicitly is allowed to return a rate bigger than 333 as long as it > >>> is less than 333.5. > >>> > >>> At one point while developing patch 1 I had the dividers fixed for the > >>> rounding issue. I think I still have that patch somewhere so can post it > >>> as RFC. > >>> > >> Why do we need clk_round_rate_nearest? We have rate constraints now so > >> drivers should be moving towards requesting a rate that's within a > >> tolerance they can accept if they even care to enforce anything like > >> that. Eventually clk_round_rate() returning a value smaller or larger > >> than what it's called with won't matter as long as what the > >> implementation does fits within the constraints that consumers specify. > >> It may even be possible to remove clk_round_rate() as a consumer API. > > If I have to provide a panel pixel clock I usually want to get a rate as > > close as possible to the specified typical rate, but within the > > specified limits. > > > > Assume a panel with 70 MHz ideal pixel clock and a valid range of 60 MHz > > to 80 MHz. If the clock supply supports two frequencies within that > > range, 60 MHz and 72 MHz, I'd prefer 72 MHz to be chosen over 60 Mhz. > > > > > > Hm.. Maybe we should tweak the arguments to clk_set_range() to have a > "typical" rate? So instead of the current API: > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > long max) > > we should have > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > long typical, unsigned long max) > > with the semantics that we'll set the rate within the min,max > constraints and try to get the rate as close to the typical rate as > possible? That would match quite a few datasheets out there that specify > these triplets. This suffers from the same problem that round_rate has today, which is the question of rounding policy. When you say that we want to get as close as possible, how do we decide between equivalent values? We need to make a default policy, document it and stick to it. E.g: clk_set_rate_range(clk, 100, 110, 120); Let's say that round_rate gives us possible values of 108 and 112, both of which are two Hz away from the typical value of 110Hz. One is not closer than the other. Which do we choose? Let's figure out a sane default to the question and document it very loudly in the code. For the sake of consistency I think we should choose the slower value since this is what a normal clk_round_rate should do stay within spec. Obviously either rate (108 or 112) would be in spec, since they are within the min/max range. But if a normal call to clk_round_rate should choose a ceiling value by default (which I think it should) then probably the range stuff should as well, just to keep us from confusing ourselves. Regards, Mike > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Mon, Mar 09, 2015 at 02:07:35PM -0700, Mike Turquette wrote: > Quoting Stephen Boyd (2015-03-09 12:05:34) > > On 03/09/15 02:58, Philipp Zabel wrote: > > > Am Freitag, den 06.03.2015, 11:40 -0800 schrieb Stephen Boyd: > > >> On 03/06/15 11:28, Uwe Kleine-König wrote: > > >>> Hello Mike, > > >>> > > >>> On Fri, Mar 06, 2015 at 10:57:30AM -0800, Mike Turquette wrote: > > >>>> Quoting Uwe Kleine-König (2015-02-21 02:40:22) > > >>>>> Hello, > > >>>>> > > >>>>> TLDR: only apply patch 1 and rip of CLK_DIVIDER_ROUND_CLOSEST. > > >>>>> > > >>>>> I stared at clk-divider.c for some time now given Sascha's failing test > > >>>>> case. I found a fix for the failure (which happens to be what Sascha > > >>>>> suspected). > > >>>>> > > >>>>> The other two patches fix problems only present when handling dividers > > >>>>> that have CLK_DIVIDER_ROUND_CLOSEST set. Note that these are still > > >>>>> heavily broken however. So having a 4bit-divider and a parent clk of > > >>>>> 10000 (as in Sascha's test case) requesting > > >>>>> > > >>>>> clk_set_rate(clk, 666) > > >>>>> > > >>>>> sets the rate to 625 (div=15) instead of 667 (div=16). The reason is the > > >>>>> choice of parent_rate in clk_divider_bestdiv's loop is wrong for > > >>>>> CLK_DIVIDER_ROUND_CLOSEST (with and without patch 1). A fix here is > > >>>>> non-trivial and for sure more than one rate must be tested here. This is > > >>>>> complicated by the fact that clk_round_rate might return a value bigger > > >>>>> than the requested rate which convinces me (once more) that it's a bad > > >>>>> idea to allow that. Even if this was fixed for .round_rate, > > >>>>> clk_divider_set_rate is still broken because it also uses > > >>>>> > > >>>>> div = DIV_ROUND_UP(parent_rate, rate); > > >>>>> > > >>>>> to calculate the (pretended) best divider to get near rate. > > >>>>> > > >>>>> Note this makes at least two reasons to remove support for > > >>>>> CLK_DIVIDER_ROUND_CLOSEST! > > >>>>> > > >>>>> Instead I'd favour creating a function > > >>>>> > > >>>>> clk_round_rate_nearest > > >>>>> > > >>>>> as was suggested some time ago by Soren Brinkmann and me[1] that doesn't > > >>>> Uwe, > > >>>> > > >>>> Thanks for the fixes. I'm thinking of taking all three for 4.0. I also > > >>>> agree on clk_round_rate_nearest (along with a _ceil and _floor version > > >>>> as well). That's something we can do for 4.1 probably. > > >>> I'd say that we make round_rate the _floor version. I guess in most > > >>> cases that already does the right thing. Also I think 4.1 is very > > >>> ambitious, so my suggestion for 4.1 is: > > >>> > > >>> - add a WARN_ON_ONCE to clk_round_rate catching calls that return a > > >>> value bigger than requested. > > >>> - implement clk_round_rate_nearest using clk_round_rate and the > > >>> assumption that it returns a value that is <= the requested rate. > > >>> I think without that there are too many special cases to handle and > > >>> probably not even a reliable way to determine the nearest rate. > > >>> - while we're at it tightening the requirements for clk_round_rate > > >>> let's also specify the expected rounding. I'd vote for the > > >>> mathematical rounding, that is > > >>> > > >>> clk_round_rate(someclk, 333) > > >>> > > >>> explicitly is allowed to return a rate bigger than 333 as long as it > > >>> is less than 333.5. > > >>> > > >>> At one point while developing patch 1 I had the dividers fixed for the > > >>> rounding issue. I think I still have that patch somewhere so can post it > > >>> as RFC. > > >>> > > >> Why do we need clk_round_rate_nearest? We have rate constraints now so > > >> drivers should be moving towards requesting a rate that's within a > > >> tolerance they can accept if they even care to enforce anything like > > >> that. Eventually clk_round_rate() returning a value smaller or larger > > >> than what it's called with won't matter as long as what the > > >> implementation does fits within the constraints that consumers specify. > > >> It may even be possible to remove clk_round_rate() as a consumer API. > > > If I have to provide a panel pixel clock I usually want to get a rate as > > > close as possible to the specified typical rate, but within the > > > specified limits. > > > > > > Assume a panel with 70 MHz ideal pixel clock and a valid range of 60 MHz > > > to 80 MHz. If the clock supply supports two frequencies within that > > > range, 60 MHz and 72 MHz, I'd prefer 72 MHz to be chosen over 60 Mhz. > > > > > > > > > > Hm.. Maybe we should tweak the arguments to clk_set_range() to have a > > "typical" rate? So instead of the current API: > > > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > > long max) > > > > we should have > > > > int clk_set_rate_range(struct clk *clk, unsigned long min, unsigned > > long typical, unsigned long max) > > > > with the semantics that we'll set the rate within the min,max > > constraints and try to get the rate as close to the typical rate as > > possible? That would match quite a few datasheets out there that specify > > these triplets. > > This suffers from the same problem that round_rate has today, which is > the question of rounding policy. When you say that we want to get as > close as possible, how do we decide between equivalent values? We need > to make a default policy, document it and stick to it. E.g: > > clk_set_rate_range(clk, 100, 110, 120); > > Let's say that round_rate gives us possible values of 108 and 112, both > of which are two Hz away from the typical value of 110Hz. One is not > closer than the other. Which do we choose? Let's figure out a sane > default to the question and document it very loudly in the code. > > For the sake of consistency I think we should choose the slower value > since this is what a normal clk_round_rate should do stay within spec. > Obviously either rate (108 or 112) would be in spec, since they are > within the min/max range. But if a normal call to clk_round_rate should > choose a ceiling value by default (which I think it should) then > probably the range stuff should as well, just to keep us from confusing > ourselves. If you see round_rate(110) = 108 it would be fortunate to know if you get 108 because the next available greater rate is > 112 or because the implementation rounds down always (which would mean that 111 is possible, too). For the "easy" consumers this probably doesn't matter much, but if you do things that affects a considerable part of the clock tree, you really want to know more about the behaviour of round_rate to effectively work with its results. So yes, please let us pick ceiling for round_rate (i.e. a fixed policy for all clks) and then it should even be possible to make clk_set_rate_range a generic function that doesn't need the min and max members in the clk struct and the respective parameters to determine_rate. What should a clock that can only provide 100 Hz return on clk_round_rate(clk, 60); ? 0? -ESOMETHING (for SOMETHING = ...?)? Best regards Uwe
On 03/09/15 14:58, Uwe Kleine-König wrote: > > If you see > > round_rate(110) = 108 > > it would be fortunate to know if you get 108 because the next available > greater rate is > 112 or because the implementation rounds down always > (which would mean that 111 is possible, too). For the "easy" consumers > this probably doesn't matter much, but if you do things that affects > a considerable part of the clock tree, you really want to know more > about the behaviour of round_rate to effectively work with its results. > > So yes, please let us pick ceiling for round_rate (i.e. a fixed policy > for all clks) and then it should even be possible to make > clk_set_rate_range a generic function that doesn't need the min and max > members in the clk struct and the respective parameters to > determine_rate. > > What should a clock that can only provide 100 Hz return on > > clk_round_rate(clk, 60); > > ? 0? -ESOMETHING (for SOMETHING = ...?)? > Do you have any real world use cases, or is this just all theoretical? At least in Philipp's panel case we can discuss how to make an API that works properly. These other examples are either completely theoretical or taken out of context and so it's unclear how they matter in practice. Ideally I'd like an API to exist that doesn't require going back and forth with the framework (i.e. it's "atomic" and doesn't require calling clk_round_rate() in a loop) and that allows consumers to properly express what they want. Right now we have a way to say min/max and a typical rate is in the works. If we need to declare some sort of clock provider rounding policy then we've failed to provide an API that properly expresses all the requirements that the consumer has. It probably means we're missing some key parameter that consumers know but we don't accept. Maybe some more concrete examples will help clarify what this is.
Hello Stephen, On Mon, Mar 09, 2015 at 03:40:29PM -0700, Stephen Boyd wrote: > On 03/09/15 14:58, Uwe Kleine-König wrote: > > If you see > > > > round_rate(110) = 108 > > > > it would be fortunate to know if you get 108 because the next available > > greater rate is > 112 or because the implementation rounds down always > > (which would mean that 111 is possible, too). For the "easy" consumers > > this probably doesn't matter much, but if you do things that affects > > a considerable part of the clock tree, you really want to know more > > about the behaviour of round_rate to effectively work with its results. > > > > So yes, please let us pick ceiling for round_rate (i.e. a fixed policy > > for all clks) and then it should even be possible to make > > clk_set_rate_range a generic function that doesn't need the min and max > > members in the clk struct and the respective parameters to > > determine_rate. > > > > What should a clock that can only provide 100 Hz return on > > > > clk_round_rate(clk, 60); > > > > ? 0? -ESOMETHING (for SOMETHING = ...?)? > > > > Do you have any real world use cases, or is this just all theoretical? The question about clk_round_rate(fixed_clk_100hz, 60) is an implementation detail that we must handle after agreeing that clk_round_rate should always round down. I faced that when I tried to implement this rounding requirement for dividers. > At least in Philipp's panel case we can discuss how to make an API that > works properly. These other examples are either completely theoretical > or taken out of context and so it's unclear how they matter in practice. We can stick to Philipp's panel case if you want. Philipp wants to find a rate between 100 Hz and 120 Hz and likes 110 Hz most. And the lower abs(1 / 110 - 1 / r) the better. Let's assume the clk is provided by a fixed clk with 10000 Hz that goes through two 4bit-dividers. (So no, that's not a real world use case, but I imagine that something like that can occur and should definitely be possible to handle.) Something similar happens if you have for example an i2c bus device that has a built-in divider. For the lowest consumer the situation is easy most of the time: It wants a certain frequency to update a panel, or it wants at most 100 kHz on the i2c bus. But already for the divider one step up the clk tree it's not that easy any more. > Ideally I'd like an API to exist that doesn't require going back and > forth with the framework (i.e. it's "atomic" and doesn't require calling > clk_round_rate() in a loop) and that allows consumers to properly Why is calling "clk_round_rate() in a loop" bad? In some situations you won't be able to do something different. > express what they want. Right now we have a way to say min/max and a > typical rate is in the works. If we need to declare some sort of clock > provider rounding policy then we've failed to provide an API that > properly expresses all the requirements that the consumer has. It I think you don't want a way to express: "I want a frequency that I can divide down to 110 Hz with a divider picked from [1 ... 16].". And even if we somehow manage to create something like that in a sane way, I think the only reliable and maintainable way to get there is to not ask all clock types to implement this functionality. That is, I want the complexity at a single place in the framework and only ask easy things from the clk type implementors. A .round_rate callback is easy for most clk types. .determine_rate a bit less and it already promotes boilerplate because each implementation has to check for min_rate and max_rate. And .determine_rate as it is today doesn't even support the typical value yet. > probably means we're missing some key parameter that consumers know but > we don't accept. Maybe some more concrete examples will help clarify > what this is. Best regards Uwe
On 03/09/15 16:34, Uwe Kleine-König wrote: > Hello Stephen, > > On Mon, Mar 09, 2015 at 03:40:29PM -0700, Stephen Boyd wrote: >> On 03/09/15 14:58, Uwe Kleine-König wrote: >>> If you see >>> >>> round_rate(110) = 108 >>> >>> it would be fortunate to know if you get 108 because the next available >>> greater rate is > 112 or because the implementation rounds down always >>> (which would mean that 111 is possible, too). For the "easy" consumers >>> this probably doesn't matter much, but if you do things that affects >>> a considerable part of the clock tree, you really want to know more >>> about the behaviour of round_rate to effectively work with its results. >>> >>> So yes, please let us pick ceiling for round_rate (i.e. a fixed policy >>> for all clks) and then it should even be possible to make >>> clk_set_rate_range a generic function that doesn't need the min and max >>> members in the clk struct and the respective parameters to >>> determine_rate. >>> >>> What should a clock that can only provide 100 Hz return on >>> >>> clk_round_rate(clk, 60); >>> >>> ? 0? -ESOMETHING (for SOMETHING = ...?)? >>> >> Do you have any real world use cases, or is this just all theoretical? > The question about clk_round_rate(fixed_clk_100hz, 60) is an > implementation detail that we must handle after agreeing that > clk_round_rate should always round down. I faced that when I tried to > implement this rounding requirement for dividers. > >> At least in Philipp's panel case we can discuss how to make an API that >> works properly. These other examples are either completely theoretical >> or taken out of context and so it's unclear how they matter in practice. > We can stick to Philipp's panel case if you want. Philipp wants to find > a rate between 100 Hz and 120 Hz and likes 110 Hz most. Why does Philipp like 110Hz the most? Where is the desire for that rate coming from? > And the lower > abs(1 / 110 - 1 / r) the better. Similarly, where is this requirement coming from? Some datasheet? Or is it just some arbitrary decision we've made that may not hold true for all consumers? > Let's assume the clk is provided by a > fixed clk with 10000 Hz that goes through two 4bit-dividers. (So no, > that's not a real world use case, but I imagine that something like that > can occur and should definitely be possible to handle.) Something > similar happens if you have for example an i2c bus device that has a > built-in divider. For the lowest consumer the situation is easy most of > the time: It wants a certain frequency to update a panel, or it wants at > most 100 kHz on the i2c bus. But already for the divider one step up > the clk tree it's not that easy any more. > >> Ideally I'd like an API to exist that doesn't require going back and >> forth with the framework (i.e. it's "atomic" and doesn't require calling >> clk_round_rate() in a loop) and that allows consumers to properly > Why is calling "clk_round_rate() in a loop" bad? In some situations you > won't be able to do something different. The problem is open coding that loop all over the kernel in slightly different ways. Plus it doesn't even work for some platforms where the parent for a clock could change between the call to clk_round_rate() and clk_set_rate() and then clk_set_rate() would do something completely different. It would be nice if clk_round_rate() was considered "stable" but unfortunately on these platforms it isn't. This is the "atomic" problem I mentioned. > >> express what they want. Right now we have a way to say min/max and a >> typical rate is in the works. If we need to declare some sort of clock >> provider rounding policy then we've failed to provide an API that >> properly expresses all the requirements that the consumer has. It > I think you don't want a way to express: "I want a frequency that I > can divide down to 110 Hz with a divider picked from [1 ... 16].". > And even if we somehow manage to create something like that in a sane > way, I think the only reliable and maintainable way to get there is to > not ask all clock types to implement this functionality. No we don't want anything like consumers asking for a rate with a set of division ratios. > That is, I want the complexity at a single place in the framework and > only ask easy things from the clk type implementors. A .round_rate > callback is easy for most clk types. .determine_rate a bit less and it > already promotes boilerplate because each implementation has to check > for min_rate and max_rate. And .determine_rate as it is today doesn't > even support the typical value yet. Now we seem to be talking about framework internal details. We needed to push the min/max (and typical if/when we add it) into the .determine_rate ops because .determine_rate is 1) the replacement for .round_rate and 2) may need to query a number of parent clocks to figure out what rate is supported and make sure it fits within the constraints it requires. To consolidate all this logic into one place on top of .round_rate would completely miss the 'mux' capabilities of clocks. We could have added min/max to .round_rate but that would have been hugely invasive so it was deemed easier to fix up the handful of .determine_rate ops for this new feature. I don't see how .round_rate or .determine_rate is any harder to implement than the other but I'm probably biased. Most likely we should make the arguments to .determine_rate into a struct so that it doesn't keep requiring a flag day to change parameters passed there. We can probably allow .determine_rate to return values outside min/max too and check it in the framework when we get the rate back if that's a concern. I don't see a way around the min/max check if an op calls __clk_determine_rate() on its parent though.
Hi Stephen, Am Mittwoch, den 11.03.2015, 18:21 -0700 schrieb Stephen Boyd: [...] > Why does Philipp like 110Hz the most? Where is the desire for that rate > coming from? > > > And the lower > > abs(1 / 110 - 1 / r) the better. > > Similarly, where is this requirement coming from? Some datasheet? Or is > it just some arbitrary decision we've made that may not hold true for > all consumers? In my panel example the datasheet usually documents the typical pixel clock and vertical and horizontal timings that exactly result in the nominal panel refresh rate, currently most often 60 Hz. In this use case, the driver doesn't want the pixel clock to stay below a hard frequency limit, but to get as close as possible to the target frequency, either above or below, so the relative error to the nominal panel refresh rate stays as small as possible. Thus for a fictional target rate of 110 Hz, I'd like to minimize abs((round_rate / 110) - 1). regards Philipp
On Thu, Mar 12, 2015 at 09:57:53AM +0100, Philipp Zabel wrote: > Am Mittwoch, den 11.03.2015, 18:21 -0700 schrieb Stephen Boyd: > [...] > > Why does Philipp like 110Hz the most? Where is the desire for that rate > > coming from? > > > > > And the lower > > > abs(1 / 110 - 1 / r) the better. > > > > Similarly, where is this requirement coming from? Some datasheet? Or is > > it just some arbitrary decision we've made that may not hold true for > > all consumers? It's not comming from a datasheet. But that's what I guess is the right metric for quite some cases. E.g. an UART sample rate and I also wouldn't be surprised if Philipp's panel example would call for this metric, too. For an UART running with say 38400 Bd you want to sample with a freqency of 38400 Hz (not considering oversampling, but that is only a factor that doesn't makes my reasoning wrong). If you now consider 38401 Hz and 38399 Hz the respective deltas are 1 Hz. But if you look at the time between two samples we have: 38401 Hz -> 26.04098852 us -> delta: 0.6781507 ns 38400 Hz -> 26.04166667 us 38399 Hz -> 26.04234485 us -> delta: 0.6781861 ns So with 38401 it takes a little longer until the slightly deviating rate results in sampling the wrong bit. > In this use case, the driver doesn't want the pixel clock to stay below > a hard frequency limit, but to get as close as possible to the target > frequency, either above or below, so the relative error to the nominal > panel refresh rate stays as small as possible. Thus for a fictional > target rate of 110 Hz, I'd like to minimize abs((round_rate / 110) - 1). Note that minimizing abs((round_rate / 110) - 1) is equivalent to minimizing abs(round_rate - 110) . Best regards Uwe
Am Freitag, den 13.03.2015, 08:50 +0100 schrieb Uwe Kleine-König: > On Thu, Mar 12, 2015 at 09:57:53AM +0100, Philipp Zabel wrote: > > Am Mittwoch, den 11.03.2015, 18:21 -0700 schrieb Stephen Boyd: > > [...] > > > Why does Philipp like 110Hz the most? Where is the desire for that rate > > > coming from? > > > > > > > And the lower > > > > abs(1 / 110 - 1 / r) the better. > > > > > > Similarly, where is this requirement coming from? Some datasheet? Or is > > > it just some arbitrary decision we've made that may not hold true for > > > all consumers? > It's not comming from a datasheet. But that's what I guess is the right > metric for quite some cases. E.g. an UART sample rate and I also > wouldn't be surprised if Philipp's panel example would call for this > metric, too. > > For an UART running with say 38400 Bd you want to sample with a freqency > of 38400 Hz (not considering oversampling, but that is only a factor > that doesn't makes my reasoning wrong). If you now consider 38401 Hz and > 38399 Hz the respective deltas are 1 Hz. But if you look at the time > between two samples we have: > > 38401 Hz -> 26.04098852 us -> delta: 0.6781507 ns > 38400 Hz -> 26.04166667 us > 38399 Hz -> 26.04234485 us -> delta: 0.6781861 ns > > So with 38401 it takes a little longer until the slightly deviating rate > results in sampling the wrong bit. > > > In this use case, the driver doesn't want the pixel clock to stay below > > a hard frequency limit, but to get as close as possible to the target > > frequency, either above or below, so the relative error to the nominal > > panel refresh rate stays as small as possible. Thus for a fictional > > target rate of 110 Hz, I'd like to minimize abs((round_rate / 110) - 1). > Note that minimizing > > abs((round_rate / 110) - 1) > > is equivalent to minimizing > > abs(round_rate - 110) Of course, and you're right, I should want to minimize the delta of the interval time, not of the rate so that if playing back a video stream at exactly the nominal frequency, it takes as long as possible until I have to drop or duplicate a frame to stay in sync. regards Philipp
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(-)