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

[PATCH v3] 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 <kbeldan@baylibre.com>.

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
v1 -> v2:
- rebased on top of current drm-next
- removed unnecessary error messages
- removed an extra newline
- added a warning if the effective pixel clock rate differs much from
  the requested rate

v2 -> v3:
- removed WARN_ONCE() in favor of dev_warn()
- added the real and calculated clock rates to the warning message
- tweaked the variable names
- used a local variable to remove redundant 'crtc->mode.clock * 1000'

Retested with

  modetest -M tilcdc -s 26:800x600@RG16
  modetest -M tilcdc -s 26:1024x768@RG16

using some additional work-in-progress changes on top of this patch.

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 57 ++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 52ebe8f..87cfde9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -320,23 +320,68 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
+/*
+ * Calculate the percentage difference between the requested pixel clock rate
+ * and the effective rate resulting from calculating the clock divider value.
+ */
+static unsigned int tilcdc_pclk_diff(unsigned long rate,
+				     unsigned long real_rate)
+{
+	int r = rate / 100, rr = real_rate / 100;
+
+	return (unsigned int)(abs(((rr - r) * 100) / r));
+}
+
 static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
-	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
+	unsigned long clk_rate, real_rate, req_rate;
+	unsigned int clkdiv;
 	int ret;
 
+	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);
+	req_rate = crtc->mode.clock * 1000;
+
+	ret = clk_set_rate(priv->clk, req_rate * clkdiv);
+	clk_rate = clk_get_rate(priv->clk);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
-			crtc->mode.clock);
-		return;
+		/*
+		 * 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.
+		 */
+		if (!clk_rate) {
+			/* Nothing more we can do. Just bail out. */
+			dev_err(dev->dev,
+				"failed to set the pixel clock - unable to read current lcdc clock rate\n");
+			return;
+		}
+
+		clkdiv = DIV_ROUND_CLOSEST(clk_rate, req_rate);
+
+		/*
+		 * Emit a warning if the real clock rate resulting from the
+		 * calculated divider differs much from the requested rate.
+		 *
+		 * 5% is an arbitrary value - LCDs are usually quite tolerant
+		 * about pixel clock rates.
+		 */
+		real_rate = clkdiv * req_rate;
+
+		if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) {
+			dev_warn(dev->dev,
+				 "effective pixel clock rate (%luHz) differs from the calculated rate (%luHz)\n",
+				 clk_rate, real_rate);
+		}
 	}
 
-	tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk);
+	tilcdc_crtc->lcd_fck_rate = clk_rate;
 
 	DBG("lcd_clk=%u, mode clock=%d, div=%u",
 	    tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv);
-- 
2.7.4

[PATCH v3] 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 <kbeldan@baylibre.com>.

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

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
v1 -> v2:
- rebased on top of current drm-next
- removed unnecessary error messages
- removed an extra newline
- added a warning if the effective pixel clock rate differs much from
  the requested rate

v2 -> v3:
- removed WARN_ONCE() in favor of dev_warn()
- added the real and calculated clock rates to the warning message
- tweaked the variable names
- used a local variable to remove redundant 'crtc->mode.clock * 1000'

Retested with

  modetest -M tilcdc -s 26:800x600@RG16
  modetest -M tilcdc -s 26:1024x768@RG16

using some additional work-in-progress changes on top of this patch.

 drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 57 ++++++++++++++++++++++++++++++++----
 1 file changed, 51 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
index 52ebe8f..87cfde9 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
@@ -320,23 +320,68 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
 	return true;
 }
 
+/*
+ * Calculate the percentage difference between the requested pixel clock rate
+ * and the effective rate resulting from calculating the clock divider value.
+ */
+static unsigned int tilcdc_pclk_diff(unsigned long rate,
+				     unsigned long real_rate)
+{
+	int r = rate / 100, rr = real_rate / 100;
+
+	return (unsigned int)(abs(((rr - r) * 100) / r));
+}
+
 static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
 	struct tilcdc_drm_private *priv = dev->dev_private;
 	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
-	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
+	unsigned long clk_rate, real_rate, req_rate;
+	unsigned int clkdiv;
 	int ret;
 
+	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);
+	req_rate = crtc->mode.clock * 1000;
+
+	ret = clk_set_rate(priv->clk, req_rate * clkdiv);
+	clk_rate = clk_get_rate(priv->clk);
 	if (ret < 0) {
-		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
-			crtc->mode.clock);
-		return;
+		/*
+		 * 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.
+		 */
+		if (!clk_rate) {
+			/* Nothing more we can do. Just bail out. */
+			dev_err(dev->dev,
+				"failed to set the pixel clock - unable to read current lcdc clock rate\n");
+			return;
+		}
+
+		clkdiv = DIV_ROUND_CLOSEST(clk_rate, req_rate);
+
+		/*
+		 * Emit a warning if the real clock rate resulting from the
+		 * calculated divider differs much from the requested rate.
+		 *
+		 * 5% is an arbitrary value - LCDs are usually quite tolerant
+		 * about pixel clock rates.
+		 */
+		real_rate = clkdiv * req_rate;
+
+		if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) {
+			dev_warn(dev->dev,
+				 "effective pixel clock rate (%luHz) differs from the calculated rate (%luHz)\n",
+				 clk_rate, real_rate);
+		}
 	}
 
-	tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk);
+	tilcdc_crtc->lcd_fck_rate = clk_rate;
 
 	DBG("lcd_clk=%u, mode clock=%d, div=%u",
 	    tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv);
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jyri Sarha]

On 09/29/16 19:43, 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 <kbeldan@baylibre.com>.
> 
> Tested with a da850-lcdk with an LCD display connected over VGA.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Forgot to mention, but I've picked this up too, but I won't send a pull
request for 4.9 anymore.

BR,
Jyri

> ---
> v1 -> v2:
> - rebased on top of current drm-next
> - removed unnecessary error messages
> - removed an extra newline
> - added a warning if the effective pixel clock rate differs much from
>   the requested rate
> 
> v2 -> v3:
> - removed WARN_ONCE() in favor of dev_warn()
> - added the real and calculated clock rates to the warning message
> - tweaked the variable names
> - used a local variable to remove redundant 'crtc->mode.clock * 1000'
> 
> Retested with
> 
>   modetest -M tilcdc -s 26:800x600@RG16
>   modetest -M tilcdc -s 26:1024x768@RG16
> 
> using some additional work-in-progress changes on top of this patch.
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 57 ++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 52ebe8f..87cfde9 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -320,23 +320,68 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +/*
> + * Calculate the percentage difference between the requested pixel clock rate
> + * and the effective rate resulting from calculating the clock divider value.
> + */
> +static unsigned int tilcdc_pclk_diff(unsigned long rate,
> +				     unsigned long real_rate)
> +{
> +	int r = rate / 100, rr = real_rate / 100;
> +
> +	return (unsigned int)(abs(((rr - r) * 100) / r));
> +}
> +
>  static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct tilcdc_drm_private *priv = dev->dev_private;
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> -	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
> +	unsigned long clk_rate, real_rate, req_rate;
> +	unsigned int clkdiv;
>  	int ret;
>  
> +	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);
> +	req_rate = crtc->mode.clock * 1000;
> +
> +	ret = clk_set_rate(priv->clk, req_rate * clkdiv);
> +	clk_rate = clk_get_rate(priv->clk);
>  	if (ret < 0) {
> -		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
> -			crtc->mode.clock);
> -		return;
> +		/*
> +		 * 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.
> +		 */
> +		if (!clk_rate) {
> +			/* Nothing more we can do. Just bail out. */
> +			dev_err(dev->dev,
> +				"failed to set the pixel clock - unable to read current lcdc clock rate\n");
> +			return;
> +		}
> +
> +		clkdiv = DIV_ROUND_CLOSEST(clk_rate, req_rate);
> +
> +		/*
> +		 * Emit a warning if the real clock rate resulting from the
> +		 * calculated divider differs much from the requested rate.
> +		 *
> +		 * 5% is an arbitrary value - LCDs are usually quite tolerant
> +		 * about pixel clock rates.
> +		 */
> +		real_rate = clkdiv * req_rate;
> +
> +		if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) {
> +			dev_warn(dev->dev,
> +				 "effective pixel clock rate (%luHz) differs from the calculated rate (%luHz)\n",
> +				 clk_rate, real_rate);
> +		}
>  	}
>  
> -	tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk);
> +	tilcdc_crtc->lcd_fck_rate = clk_rate;
>  
>  	DBG("lcd_clk=%u, mode clock=%d, div=%u",
>  	    tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv);
> 

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

[Jyri Sarha]

On 09/29/16 19:43, 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 <kbeldan@baylibre.com>.
> 
> Tested with a da850-lcdk with an LCD display connected over VGA.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Forgot to mention, but I've picked this up too, but I won't send a pull
request for 4.9 anymore.

BR,
Jyri

> ---
> v1 -> v2:
> - rebased on top of current drm-next
> - removed unnecessary error messages
> - removed an extra newline
> - added a warning if the effective pixel clock rate differs much from
>   the requested rate
> 
> v2 -> v3:
> - removed WARN_ONCE() in favor of dev_warn()
> - added the real and calculated clock rates to the warning message
> - tweaked the variable names
> - used a local variable to remove redundant 'crtc->mode.clock * 1000'
> 
> Retested with
> 
>   modetest -M tilcdc -s 26:800x600@RG16
>   modetest -M tilcdc -s 26:1024x768@RG16
> 
> using some additional work-in-progress changes on top of this patch.
> 
>  drivers/gpu/drm/tilcdc/tilcdc_crtc.c | 57 ++++++++++++++++++++++++++++++++----
>  1 file changed, 51 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> index 52ebe8f..87cfde9 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_crtc.c
> @@ -320,23 +320,68 @@ static bool tilcdc_crtc_mode_fixup(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +/*
> + * Calculate the percentage difference between the requested pixel clock rate
> + * and the effective rate resulting from calculating the clock divider value.
> + */
> +static unsigned int tilcdc_pclk_diff(unsigned long rate,
> +				     unsigned long real_rate)
> +{
> +	int r = rate / 100, rr = real_rate / 100;
> +
> +	return (unsigned int)(abs(((rr - r) * 100) / r));
> +}
> +
>  static void tilcdc_crtc_set_clk(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct tilcdc_drm_private *priv = dev->dev_private;
>  	struct tilcdc_crtc *tilcdc_crtc = to_tilcdc_crtc(crtc);
> -	const unsigned clkdiv = 2; /* using a fixed divider of 2 */
> +	unsigned long clk_rate, real_rate, req_rate;
> +	unsigned int clkdiv;
>  	int ret;
>  
> +	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);
> +	req_rate = crtc->mode.clock * 1000;
> +
> +	ret = clk_set_rate(priv->clk, req_rate * clkdiv);
> +	clk_rate = clk_get_rate(priv->clk);
>  	if (ret < 0) {
> -		dev_err(dev->dev, "failed to set display clock rate to: %d\n",
> -			crtc->mode.clock);
> -		return;
> +		/*
> +		 * 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.
> +		 */
> +		if (!clk_rate) {
> +			/* Nothing more we can do. Just bail out. */
> +			dev_err(dev->dev,
> +				"failed to set the pixel clock - unable to read current lcdc clock rate\n");
> +			return;
> +		}
> +
> +		clkdiv = DIV_ROUND_CLOSEST(clk_rate, req_rate);
> +
> +		/*
> +		 * Emit a warning if the real clock rate resulting from the
> +		 * calculated divider differs much from the requested rate.
> +		 *
> +		 * 5% is an arbitrary value - LCDs are usually quite tolerant
> +		 * about pixel clock rates.
> +		 */
> +		real_rate = clkdiv * req_rate;
> +
> +		if (tilcdc_pclk_diff(clk_rate, real_rate) > 5) {
> +			dev_warn(dev->dev,
> +				 "effective pixel clock rate (%luHz) differs from the calculated rate (%luHz)\n",
> +				 clk_rate, real_rate);
> +		}
>  	}
>  
> -	tilcdc_crtc->lcd_fck_rate = clk_get_rate(priv->clk);
> +	tilcdc_crtc->lcd_fck_rate = clk_rate;
>  
>  	DBG("lcd_clk=%u, mode clock=%d, div=%u",
>  	    tilcdc_crtc->lcd_fck_rate, crtc->mode.clock, clkdiv);
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel