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(-)
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;
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
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
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;
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
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;
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
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; }
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
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
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
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
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Maxime Ripard