[PATCH] drm: tilcdc: add a workaround for failed clk_set_rate()

[Tomi Valkeinen]

On 28/09/16 14:43, Bartosz Golaszewski wrote:

> how far is far enough to emit a warning? On da850 the requested rate
> is 22800 Hz, while the calculated divider is 6, which results in
> the real rate of 22500 Hz. This is less than 1% difference -
> should we take this value as reference?

Good question, and I don't have a clear answer. But I think the point is
just to inform the user that it's likely that his LCD won't work
properly, and often LCDs are quite tolerant about the pclk. So diff less
than 1% is just fine. Even 5% sounds still fine to me, perhaps even 10%.
I think we just have to try something out, and adjust it if people get
the message too often.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH] drm: tilcdc: add a workaround for failed clk_set_rate()

[Tomi Valkeinen]

Hi,

On 27/09/16 18:29, Bartosz Golaszewski wrote:
> Some architectures don't use the common clock framework and don't
> implement all the clk interfaces for every clock. This is the case
> for da850-lcdk where clk_set_rate() only works for PLL0 and PLL1.
> 
> Trying to set the clock rate for the LCDC clock results in -EINVAL
> being returned.
> 
> As a workaround for that: if the call to clk_set_rate() fails, fall
> back to adjusting the clock divider instead. Proper divider value is
> calculated by dividing the current clock rate by the required pixel
> clock rate in HZ.
> 
> This code is based on a hack initially developed internally for
> baylibre by Karl Beldan .
> 
> Tested with a da850-lcdk with an LCD display connected over VGA.
> 
> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 31 ---
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 2087689..f2ff3b1 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -636,22 +636,40 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
>   struct drm_device *dev = crtc->dev;
>   struct tilcdc_drm_private *priv = dev->dev_private;
>   unsigned long lcd_clk;
> - const unsigned clkdiv = 2; /* using a fixed divider of 2 */
> + unsigned int clkdiv;
>   int ret;
>  
>   pm_runtime_get_sync(dev->dev);
>  
>   tilcdc_crtc_disable(crtc);
>  
> + clkdiv = 2; /* first try using a standard divider of 2 */
> +
>   /* mode.clock is in KHz, set_rate wants parameter in Hz */
>   ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv);
> + lcd_clk = clk_get_rate(priv->clk);
>   if (ret < 0) {
> - dev_err(dev->dev, "failed to set display clock rate to: %d\n",
> - crtc->mode.clock);
> - goto out;
> - }
> + /*
> +  * If we fail to set the clock rate (some architectures don't
> +  * use the common clock framework yet and may not implement
> +  * all the clk API calls for every clock), try the next best
> +  * thing: adjusting the clock divider, unless clk_get_rate()
> +  * failed as well.
> +  */
> + dev_err(dev->dev,
> + "failed to set display clock rate to: %d\n",
> + crtc->mode.clock);

This code path is not an error, is it? No reason to print anything here.

> + if (!lcd_clk) {
> + /* Nothing more we can do. Just bail out. */
> + dev_err(dev->dev,
> + "failed to read the display clock rate\n");

This is an error, but maybe the error message should say something else.
I mean, "failed to _read_ display clk rate", when the user tries to set
pixel clock, doesn't quite make sense =).

> + goto out;
> + }
>  
> - lcd_clk = clk_get_rate(priv->clk);
> + dev_info(dev->dev,
> +  "falling back to adjusting the clock divisor\n");

I don't see a reason for this print either. This is normal if the
platform doesn't support changing the lcdc's input clock.

> + clkdiv = DIV_ROUND_CLOSEST(lcd_clk, (crtc->mode.clock * 1000));
> + }

If the clock divider path easily creates pixel clocks that are quite far
from the requested ones (which I think it does), I think it would make
sense to have a print here if the final pixel clock is far enough from
the requested one. And that's a valid print for the current code path
too, as it's not clear whether lcdc's input clock can be set to an exact
value.

>   DBG("lcd_clk=%lu, mode clock=%d, div=%u",
>   lcd_clk, crtc->mode.clock, clkdiv);
> @@ -664,7 +682,6 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
>   tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
>   LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
>   LCDC_V2_CORE_CLK_EN);
> -

Extra change here.

 Tomi

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: 

[PATCH] drm: tilcdc: add a workaround for failed clk_set_rate()

[Jyri Sarha]

On 09/27/16 18:29, Bartosz Golaszewski wrote:
> Some architectures don't use the common clock framework and don't
> implement all the clk interfaces for every clock. This is the case
> for da850-lcdk where clk_set_rate() only works for PLL0 and PLL1.
> 
> Trying to set the clock rate for the LCDC clock results in -EINVAL
> being returned.
> 
> As a workaround for that: if the call to clk_set_rate() fails, fall
> back to adjusting the clock divider instead. Proper divider value is
> calculated by dividing the current clock rate by the required pixel
> clock rate in HZ.
> 
> This code is based on a hack initially developed internally for
> baylibre by Karl Beldan .
> 
> Tested with a da850-lcdk with an LCD display connected over VGA.
> 

Could you rebase the fix on top of latest drm-next[1] (or my latest pull
request tag[2])?

The conflict is so big that it is better the check that the fix still
works after rebasing.

Best regards,
Jyri

[1] git://people.freedesktop.org/~airlied/linux drm-next
[2] https://github.com/jsarha/linux tags/tilcdc-4.9-3.1

> Signed-off-by: Bartosz Golaszewski 
> ---
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 31 ---
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
> b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 2087689..f2ff3b1 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -636,22 +636,40 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
>   struct drm_device *dev = crtc->dev;
>   struct tilcdc_drm_private *priv = dev->dev_private;
>   unsigned long lcd_clk;
> - const unsigned clkdiv = 2; /* using a fixed divider of 2 */
> + unsigned int clkdiv;
>   int ret;
>  
>   pm_runtime_get_sync(dev->dev);
>  
>   tilcdc_crtc_disable(crtc);
>  
> + clkdiv = 2; /* first try using a standard divider of 2 */
> +
>   /* mode.clock is in KHz, set_rate wants parameter in Hz */
>   ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv);
> + lcd_clk = clk_get_rate(priv->clk);
>   if (ret < 0) {
> - dev_err(dev->dev, "failed to set display clock rate to: %d\n",
> - crtc->mode.clock);
> - goto out;
> - }
> + /*
> +  * If we fail to set the clock rate (some architectures don't
> +  * use the common clock framework yet and may not implement
> +  * all the clk API calls for every clock), try the next best
> +  * thing: adjusting the clock divider, unless clk_get_rate()
> +  * failed as well.
> +  */
> + dev_err(dev->dev,
> + "failed to set display clock rate to: %d\n",
> + crtc->mode.clock);
> + if (!lcd_clk) {
> + /* Nothing more we can do. Just bail out. */
> + dev_err(dev->dev,
> + "failed to read the display clock rate\n");
> + goto out;
> + }
>  
> - lcd_clk = clk_get_rate(priv->clk);
> + dev_info(dev->dev,
> +  "falling back to adjusting the clock divisor\n");
> + clkdiv = DIV_ROUND_CLOSEST(lcd_clk, (crtc->mode.clock * 1000));
> + }
>  
>   DBG("lcd_clk=%lu, mode clock=%d, div=%u",
>   lcd_clk, crtc->mode.clock, clkdiv);
> @@ -664,7 +682,6 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
>   tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
>   LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
>   LCDC_V2_CORE_CLK_EN);
> -
>   if (tilcdc_crtc_is_on(crtc))
>   tilcdc_crtc_enable(crtc);
>  
> 

[PATCH] drm: tilcdc: add a workaround for failed clk_set_rate()

[Bartosz Golaszewski]

+ Sekhar

2016-09-28 13:19 GMT+02:00 Tomi Valkeinen :
> Hi,
>
> On 27/09/16 18:29, Bartosz Golaszewski wrote:
>> Some architectures don't use the common clock framework and don't
>> implement all the clk interfaces for every clock. This is the case
>> for da850-lcdk where clk_set_rate() only works for PLL0 and PLL1.
>>
>> Trying to set the clock rate for the LCDC clock results in -EINVAL
>> being returned.
>>
>> As a workaround for that: if the call to clk_set_rate() fails, fall
>> back to adjusting the clock divider instead. Proper divider value is
>> calculated by dividing the current clock rate by the required pixel
>> clock rate in HZ.
>>
>> This code is based on a hack initially developed internally for
>> baylibre by Karl Beldan .
>>
>> Tested with a da850-lcdk with an LCD display connected over VGA.
>>
>> Signed-off-by: Bartosz Golaszewski 

[snip]

>
> If the clock divider path easily creates pixel clocks that are quite far
> from the requested ones (which I think it does), I think it would make
> sense to have a print here if the final pixel clock is far enough from
> the requested one. And that's a valid print for the current code path
> too, as it's not clear whether lcdc's input clock can be set to an exact
> value.
>
>>   DBG("lcd_clk=%lu, mode clock=%d, div=%u",
>>   lcd_clk, crtc->mode.clock, clkdiv);
>> @@ -664,7 +682,6 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
>>   tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
>>   LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
>>   LCDC_V2_CORE_CLK_EN);
>> -

Hi Tomi,

how far is far enough to emit a warning? On da850 the requested rate
is 22800 Hz, while the calculated divider is 6, which results in
the real rate of 22500 Hz. This is less than 1% difference -
should we take this value as reference?

I'll apply all other requested changes.

Best regards,
Bartosz Golaszewski

[PATCH] drm: tilcdc: add a workaround for failed clk_set_rate()

[Bartosz Golaszewski]

Some architectures don't use the common clock framework and don't
implement all the clk interfaces for every clock. This is the case
for da850-lcdk where clk_set_rate() only works for PLL0 and PLL1.

Trying to set the clock rate for the LCDC clock results in -EINVAL
being returned.

As a workaround for that: if the call to clk_set_rate() fails, fall
back to adjusting the clock divider instead. Proper divider value is
calculated by dividing the current clock rate by the required pixel
clock rate in HZ.

This code is based on a hack initially developed internally for
baylibre by Karl Beldan .

Tested with a da850-lcdk with an LCD display connected over VGA.

Signed-off-by: Bartosz Golaszewski 
---
 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 31 ---
 1 file changed, 24 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c 
b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 2087689..f2ff3b1 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -636,22 +636,40 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
struct drm_device *dev = crtc->dev;
struct tilcdc_drm_private *priv = dev->dev_private;
unsigned long lcd_clk;
-   const unsigned clkdiv = 2; /* using a fixed divider of 2 */
+   unsigned int clkdiv;
int ret;

pm_runtime_get_sync(dev->dev);

tilcdc_crtc_disable(crtc);

+   clkdiv = 2; /* first try using a standard divider of 2 */
+
/* mode.clock is in KHz, set_rate wants parameter in Hz */
ret = clk_set_rate(priv->clk, crtc->mode.clock * 1000 * clkdiv);
+   lcd_clk = clk_get_rate(priv->clk);
if (ret < 0) {
-   dev_err(dev->dev, "failed to set display clock rate to: %d\n",
-   crtc->mode.clock);
-   goto out;
-   }
+   /*
+* If we fail to set the clock rate (some architectures don't
+* use the common clock framework yet and may not implement
+* all the clk API calls for every clock), try the next best
+* thing: adjusting the clock divider, unless clk_get_rate()
+* failed as well.
+*/
+   dev_err(dev->dev,
+   "failed to set display clock rate to: %d\n",
+   crtc->mode.clock);
+   if (!lcd_clk) {
+   /* Nothing more we can do. Just bail out. */
+   dev_err(dev->dev,
+   "failed to read the display clock rate\n");
+   goto out;
+   }

-   lcd_clk = clk_get_rate(priv->clk);
+   dev_info(dev->dev,
+"falling back to adjusting the clock divisor\n");
+   clkdiv = DIV_ROUND_CLOSEST(lcd_clk, (crtc->mode.clock * 1000));
+   }

DBG("lcd_clk=%lu, mode clock=%d, div=%u",
lcd_clk, crtc->mode.clock, clkdiv);
@@ -664,7 +682,6 @@ void tilcdc_crtc_update_clk(struct drm_crtc *crtc)
tilcdc_set(dev, LCDC_CLK_ENABLE_REG,
LCDC_V2_DMA_CLK_EN | LCDC_V2_LIDD_CLK_EN |
LCDC_V2_CORE_CLK_EN);
-
if (tilcdc_crtc_is_on(crtc))
tilcdc_crtc_enable(crtc);

-- 
2.7.4