[PATCH 0/4] drm/sun4i: rgb and dotclock misc fixes and improvements

[PATCH 0/4] drm/sun4i: rgb and dotclock misc fixes and improvements

[Chen-Yu Tsai]

Hi Maxime,

Here are a few small fixes and improvements to the sun4i drm driver.

Patch 1 declares the LCD panel RGB interface encoder and connector types
as MIPI DPI. AFAIK DPI is the parallel RGB variant.

Patch 2 fixes the weird clock rates I was getting on the dot clock when
testing the RGB-to-VGA bridge patches. You may want to queue this as a
fix for 4.8.

Patch 3 increases the dot clock divider upper bound by 1 to 127. AFAIK
127 is a valid divider. I doubt we will ever use it, since the parents
will go way higher than they are supposed to, but getting it right is
nicer.

Patch 4 changes the dot clock's behavior to make it round to the closest
clock rate. I think this would make it easier to match the LCD panel's
timings. More on the LCD timings in a later patch set.


Regards
ChenYu

Chen-Yu Tsai (4):
  drm/sun4i: rgb: Declare RGB encoder and connector as MIPI DPI
  drm/sun4i: dotclock: Fix clock rate read back calcation
  drm/sun4i: dotclock: Allow divider = 127
  drm/sun4i: dotclock: Round to closest clock rate

 drivers/gpu/drm/sun4i/sun4i_dotclock.c | 7 ++++---
 drivers/gpu/drm/sun4i/sun4i_rgb.c      | 4 ++--
 2 files changed, 6 insertions(+), 5 deletions(-)

-- 
2.9.3

[PATCH 1/4] drm/sun4i: rgb: Declare RGB encoder and connector as MIPI DPI

[Chen-Yu Tsai]

The 18 or 24 bit parallel RGB LCD panel interface found on Allwinner
SoCs matches the description of MIPI DPI. Declare the RGB encoder and
connector as MIPI DPI.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_rgb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c
index c3ff10f559cc..8b520d9f5bd9 100644
--- a/drivers/gpu/drm/sun4i/sun4i_rgb.c
+++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c
@@ -240,7 +240,7 @@ int sun4i_rgb_init(struct drm_device *drm)
 	ret = drm_encoder_init(drm,
 			       &rgb->encoder,
 			       &sun4i_rgb_enc_funcs,
-			       DRM_MODE_ENCODER_NONE,
+			       DRM_MODE_ENCODER_DPI,
 			       NULL);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't initialise the rgb encoder\n");
@@ -255,7 +255,7 @@ int sun4i_rgb_init(struct drm_device *drm)
 					 &sun4i_rgb_con_helper_funcs);
 		ret = drm_connector_init(drm, &rgb->connector,
 					 &sun4i_rgb_con_funcs,
-					 DRM_MODE_CONNECTOR_Unknown);
+					 DRM_MODE_CONNECTOR_DPI);
 		if (ret) {
 			dev_err(drm->dev, "Couldn't initialise the rgb connector\n");
 			goto err_cleanup_connector;
-- 
2.9.3

[PATCH 2/4] drm/sun4i: dotclock: Fix clock rate read back calcation

[Chen-Yu Tsai]

When reading back the divider set in the register, we mask off the
bits that aren't part of the divider. Unfortunately the mask used
here was not converted from the field width.

Fix this by converting the field width to a proper bit mask.

Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_dotclock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
index 4332da48b1b3..1b6c2253192e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
@@ -62,7 +62,7 @@ static unsigned long sun4i_dclk_recalc_rate(struct clk_hw *hw,
 	regmap_read(dclk->regmap, SUN4I_TCON0_DCLK_REG, &val);
 
 	val >>= SUN4I_TCON0_DCLK_DIV_SHIFT;
-	val &= SUN4I_TCON0_DCLK_DIV_WIDTH;
+	val &= (1 << SUN4I_TCON0_DCLK_DIV_WIDTH) - 1;
 
 	if (!val)
 		val = 1;
-- 
2.9.3

[PATCH 3/4] drm/sun4i: dotclock: Allow divider = 127

[Chen-Yu Tsai]

The dot clock divider is 7 bits wide, and the divider range is 1 ~ 127,
or 6 ~ 127 if phase offsets are used. The 0 register value also
represents a divider of 1 or bypass.

Make the end condition of the for loop inclusive of 127 in the
round_rate callback.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_dotclock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
index 1b6c2253192e..3eb99784f371 100644
--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
@@ -77,7 +77,7 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
 	u8 best_div = 1;
 	int i;
 
-	for (i = 6; i < 127; i++) {
+	for (i = 6; i <= 127; i++) {
 		unsigned long ideal = rate * i;
 		unsigned long rounded;
 
-- 
2.9.3

[PATCH 4/4] drm/sun4i: dotclock: Round to closest clock rate

[Chen-Yu Tsai]

With display pixel clocks we want to have the closest possible clock
rate, to minimize timing and refresh rate skews. Whether the actual
clock rate is higher or lower than the requested rate is less important.

Also check candidates against the requested rate, rather than the
ideal parent rate, the varying dividers also influence the difference
between the requested rate and the rounded rate.

Signed-off-by: Chen-Yu Tsai <wens@csie.org>
---
 drivers/gpu/drm/sun4i/sun4i_dotclock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
index 3eb99784f371..d401156490f3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
+++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
@@ -90,7 +90,8 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
 			goto out;
 		}
 
-		if ((rounded < ideal) && (rounded > best_parent)) {
+		if (abs(rate - rounded / i) <
+		    abs(rate - best_parent / best_div)) {
 			best_parent = rounded;
 			best_div = i;
 		}
-- 
2.9.3

Re: [PATCH 1/4] drm/sun4i: rgb: Declare RGB encoder and connector as MIPI DPI

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 515 bytes --]

Hi,

On Thu, Sep 15, 2016 at 11:13:59PM +0800, Chen-Yu Tsai wrote:
> The 18 or 24 bit parallel RGB LCD panel interface found on Allwinner
> SoCs matches the description of MIPI DPI. Declare the RGB encoder and
> connector as MIPI DPI.

Unfortunately, even it that patch might be true (is there a public
spec for that standard?), this breaks the user-space, so there's no
way we change this.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/4] drm/sun4i: dotclock: Allow divider = 127

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 522 bytes --]

On Thu, Sep 15, 2016 at 11:14:01PM +0800, Chen-Yu Tsai wrote:
> The dot clock divider is 7 bits wide, and the divider range is 1 ~ 127,
> or 6 ~ 127 if phase offsets are used. The 0 register value also
> represents a divider of 1 or bypass.
> 
> Make the end condition of the for loop inclusive of 127 in the
> round_rate callback.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/4] drm/sun4i: dotclock: Fix clock rate read back calcation

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On Thu, Sep 15, 2016 at 11:14:00PM +0800, Chen-Yu Tsai wrote:
> When reading back the divider set in the register, we mask off the
> bits that aren't part of the divider. Unfortunately the mask used
> here was not converted from the field width.
> 
> Fix this by converting the field width to a proper bit mask.
> 
> Fixes: 9026e0d122ac ("drm: Add Allwinner A10 Display Engine support")
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] drm/sun4i: dotclock: Round to closest clock rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1342 bytes --]

Hi,

On Thu, Sep 15, 2016 at 11:14:02PM +0800, Chen-Yu Tsai wrote:
> With display pixel clocks we want to have the closest possible clock
> rate, to minimize timing and refresh rate skews. Whether the actual
> clock rate is higher or lower than the requested rate is less important.
> 
> Also check candidates against the requested rate, rather than the
> ideal parent rate, the varying dividers also influence the difference
> between the requested rate and the rounded rate.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> ---
>  drivers/gpu/drm/sun4i/sun4i_dotclock.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> index 3eb99784f371..d401156490f3 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> @@ -90,7 +90,8 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
>  			goto out;
>  		}
>  
> -		if ((rounded < ideal) && (rounded > best_parent)) {
> +		if (abs(rate - rounded / i) <
> +		    abs(rate - best_parent / best_div)) {

I'm not sure what you're trying to do here. Why is the divider involved?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/4] drm/sun4i: rgb: Declare RGB encoder and connector as MIPI DPI

[Chen-Yu Tsai]

On Mon, Sep 19, 2016 at 3:12 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 11:13:59PM +0800, Chen-Yu Tsai wrote:
>> The 18 or 24 bit parallel RGB LCD panel interface found on Allwinner
>> SoCs matches the description of MIPI DPI. Declare the RGB encoder and
>> connector as MIPI DPI.
>
> Unfortunately, even it that patch might be true (is there a public
> spec for that standard?), this breaks the user-space, so there's no

Not that I know of. I basically googled the term and looked at other
datasheets and asked on #linux-arm-kernel.

> way we change this.

:(

ChenYu

Re: [PATCH 4/4] drm/sun4i: dotclock: Round to closest clock rate

[Chen-Yu Tsai]

On Mon, Sep 19, 2016 at 3:16 AM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Thu, Sep 15, 2016 at 11:14:02PM +0800, Chen-Yu Tsai wrote:
>> With display pixel clocks we want to have the closest possible clock
>> rate, to minimize timing and refresh rate skews. Whether the actual
>> clock rate is higher or lower than the requested rate is less important.
>>
>> Also check candidates against the requested rate, rather than the
>> ideal parent rate, the varying dividers also influence the difference
>> between the requested rate and the rounded rate.
>>
>> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
>> ---
>>  drivers/gpu/drm/sun4i/sun4i_dotclock.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
>> index 3eb99784f371..d401156490f3 100644
>> --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
>> +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
>> @@ -90,7 +90,8 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
>>                       goto out;
>>               }
>>
>> -             if ((rounded < ideal) && (rounded > best_parent)) {
>> +             if (abs(rate - rounded / i) <
>> +                 abs(rate - best_parent / best_div)) {
>
> I'm not sure what you're trying to do here. Why is the divider involved?

Say you want the dotclock at X, so you try Y = 6 ~ 127 for the divider.
Now you're asking the CCF to round (X*Y).

In the original code, you were comparing the result of rounding (X * Y).

                if ((rounded < ideal) && (rounded > best_parent)) {
                        best_parent = rounded;
                        best_div = i;
                }

where ideal = X * Y (i in the code). Given the divider increases in
the loop, you are actually not closing in on the best divider, but the
highest divider that doesn't give a higher rate than the ideal rate.

Including the divider makes it compare the actual dot clock frequency
if a given divider was used.

Does this makes sense? Explaining this kind of makes my head spin...

Regards
ChenYu

>
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Re: [PATCH 4/4] drm/sun4i: dotclock: Round to closest clock rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 663 bytes --]

On Thu, Sep 15, 2016 at 11:14:02PM +0800, Chen-Yu Tsai wrote:
> With display pixel clocks we want to have the closest possible clock
> rate, to minimize timing and refresh rate skews. Whether the actual
> clock rate is higher or lower than the requested rate is less important.
> 
> Also check candidates against the requested rate, rather than the
> ideal parent rate, the varying dividers also influence the difference
> between the requested rate and the rounded rate.
> 
> Signed-off-by: Chen-Yu Tsai <wens@csie.org>

Applied, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/4] drm/sun4i: dotclock: Round to closest clock rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2533 bytes --]

On Mon, Sep 19, 2016 at 11:36:18PM +0800, Chen-Yu Tsai wrote:
> On Mon, Sep 19, 2016 at 3:16 AM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Thu, Sep 15, 2016 at 11:14:02PM +0800, Chen-Yu Tsai wrote:
> >> With display pixel clocks we want to have the closest possible clock
> >> rate, to minimize timing and refresh rate skews. Whether the actual
> >> clock rate is higher or lower than the requested rate is less important.
> >>
> >> Also check candidates against the requested rate, rather than the
> >> ideal parent rate, the varying dividers also influence the difference
> >> between the requested rate and the rounded rate.
> >>
> >> Signed-off-by: Chen-Yu Tsai <wens@csie.org>
> >> ---
> >>  drivers/gpu/drm/sun4i/sun4i_dotclock.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/sun4i/sun4i_dotclock.c b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> >> index 3eb99784f371..d401156490f3 100644
> >> --- a/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> >> +++ b/drivers/gpu/drm/sun4i/sun4i_dotclock.c
> >> @@ -90,7 +90,8 @@ static long sun4i_dclk_round_rate(struct clk_hw *hw, unsigned long rate,
> >>                       goto out;
> >>               }
> >>
> >> -             if ((rounded < ideal) && (rounded > best_parent)) {
> >> +             if (abs(rate - rounded / i) <
> >> +                 abs(rate - best_parent / best_div)) {
> >
> > I'm not sure what you're trying to do here. Why is the divider involved?
> 
> Say you want the dotclock at X, so you try Y = 6 ~ 127 for the divider.
> Now you're asking the CCF to round (X*Y).
> 
> In the original code, you were comparing the result of rounding (X * Y).
> 
>                 if ((rounded < ideal) && (rounded > best_parent)) {
>                         best_parent = rounded;
>                         best_div = i;
>                 }
> 
> where ideal = X * Y (i in the code). Given the divider increases in
> the loop, you are actually not closing in on the best divider, but the
> highest divider that doesn't give a higher rate than the ideal rate.
> 
> Including the divider makes it compare the actual dot clock frequency
> if a given divider was used.
> 
> Does this makes sense? Explaining this kind of makes my head spin...

Yes, sorry, I didn't remember rounded was actually the rounded parent
rate.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]