The current logic to deal with old DT missing the LVDS properties doesn't take into account whether the LVDS output is supported in the first place, resulting in spurious error messages on SoCs where it doesn't even matter.
Introduce a new TCON flag to list if LVDS is supported at all to prevent this from happening.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_tcon.c | 86 ++++++++++++++++++++------------------ drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 3c15cf24b503..6ff0e2e84e81 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -870,52 +870,56 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, return ret; }
- /* - * This can only be made optional since we've had DT nodes - * without the LVDS reset properties. - * - * If the property is missing, just disable LVDS, and print a - * warning. - */ - tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds"); - if (IS_ERR(tcon->lvds_rst)) { - dev_err(dev, "Couldn't get our reset line\n"); - return PTR_ERR(tcon->lvds_rst); - } else if (tcon->lvds_rst) { - has_lvds_rst = true; - reset_control_reset(tcon->lvds_rst); - } else { - has_lvds_rst = false; - } + if (tcon->quirks->supports_lvds) { + /* + * This can only be made optional since we've had DT + * nodes without the LVDS reset properties. + * + * If the property is missing, just disable LVDS, and + * print a warning. + */ + tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds"); + if (IS_ERR(tcon->lvds_rst)) { + dev_err(dev, "Couldn't get our reset line\n"); + return PTR_ERR(tcon->lvds_rst); + } else if (tcon->lvds_rst) { + has_lvds_rst = true; + reset_control_reset(tcon->lvds_rst); + } else { + has_lvds_rst = false; + }
- /* - * This can only be made optional since we've had DT nodes - * without the LVDS reset properties. - * - * If the property is missing, just disable LVDS, and print a - * warning. - */ - if (tcon->quirks->has_lvds_alt) { - tcon->lvds_pll = devm_clk_get(dev, "lvds-alt"); - if (IS_ERR(tcon->lvds_pll)) { - if (PTR_ERR(tcon->lvds_pll) == -ENOENT) { - has_lvds_alt = false; + /* + * This can only be made optional since we've had DT + * nodes without the LVDS reset properties. + * + * If the property is missing, just disable LVDS, and + * print a warning. + */ + if (tcon->quirks->has_lvds_alt) { + tcon->lvds_pll = devm_clk_get(dev, "lvds-alt"); + if (IS_ERR(tcon->lvds_pll)) { + if (PTR_ERR(tcon->lvds_pll) == -ENOENT) { + has_lvds_alt = false; + } else { + dev_err(dev, "Couldn't get the LVDS PLL\n"); + return PTR_ERR(tcon->lvds_pll); + } } else { - dev_err(dev, "Couldn't get the LVDS PLL\n"); - return PTR_ERR(tcon->lvds_pll); + has_lvds_alt = true; } - } else { - has_lvds_alt = true; } - }
- if (!has_lvds_rst || (tcon->quirks->has_lvds_alt && !has_lvds_alt)) { - dev_warn(dev, - "Missing LVDS properties, Please upgrade your DT\n"); - dev_warn(dev, "LVDS output disabled\n"); + if (!has_lvds_rst || + (tcon->quirks->has_lvds_alt && !has_lvds_alt)) { + dev_warn(dev, "Missing LVDS properties, Please upgrade your DT\n"); + dev_warn(dev, "LVDS output disabled\n"); + can_lvds = false; + } else { + can_lvds = true; + } + } else { can_lvds = false; - } else { - can_lvds = true; }
ret = sun4i_tcon_init_clocks(dev, tcon); @@ -1134,7 +1138,7 @@ static const struct sun4i_tcon_quirks sun8i_a33_quirks = { };
static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = { - /* nothing is supported */ + .supports_lvds = true, };
static const struct sun4i_tcon_quirks sun8i_v3s_quirks = { diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index b761c7b823c5..278700c7bf9f 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -175,6 +175,7 @@ struct sun4i_tcon_quirks { bool has_channel_1; /* a33 does not have channel 1 */ bool has_lvds_alt; /* Does the LVDS clock have a parent other than the TCON clock? */ bool needs_de_be_mux; /* sun6i needs mux to select backend */ + bool supports_lvds; /* Does the TCON support an LVDS output? */
/* callback to handle tcon muxing options */ int (*set_mux)(struct sun4i_tcon *, const struct drm_encoder *);
In the case where mode_valid callback of our RGB connector was called before mode_set was being called, the range of dividers would not be set, resulting in a division by zero later on in the clk_round_rate logic.
Set the range of dividers before calling clk_round_rate to fix this.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index 832f8f9bc47f..b8da5a50a61d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -92,6 +92,8 @@ static int sun4i_rgb_mode_valid(struct drm_connector *connector,
DRM_DEBUG_DRIVER("Vertical parameters OK\n");
+ tcon->dclk_min_div = 6; + tcon->dclk_max_div = 127; rounded_rate = clk_round_rate(tcon->dclk, rate); if (rounded_rate < rate) return MODE_CLOCK_LOW;
Il 21/02/2018 13:57, Maxime Ripard ha scritto:
In the case where mode_valid callback of our RGB connector was called before mode_set was being called, the range of dividers would not be set, resulting in a division by zero later on in the clk_round_rate logic.
Set the range of dividers before calling clk_round_rate to fix this.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Tested-by: Giulio Benetti giulio.benetti@micronovasrl.com
Before crtc couldn't be added because of min=0 and max=0. Now they're initiliazed and crtc is added correctly.
drivers/gpu/drm/sun4i/sun4i_rgb.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_rgb.c b/drivers/gpu/drm/sun4i/sun4i_rgb.c index 832f8f9bc47f..b8da5a50a61d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_rgb.c +++ b/drivers/gpu/drm/sun4i/sun4i_rgb.c @@ -92,6 +92,8 @@ static int sun4i_rgb_mode_valid(struct drm_connector *connector,
DRM_DEBUG_DRIVER("Vertical parameters OK\n");
- tcon->dclk_min_div = 6;
- tcon->dclk_max_div = 127; rounded_rate = clk_round_rate(tcon->dclk, rate); if (rounded_rate < rate) return MODE_CLOCK_LOW;
On Wed, Feb 21, 2018 at 8:57 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:
In the case where mode_valid callback of our RGB connector was called before mode_set was being called, the range of dividers would not be set, resulting in a division by zero later on in the clk_round_rate logic.
Set the range of dividers before calling clk_round_rate to fix this.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Make sure that the CRTC code will call the enable/disable_vblank hooks.
Otherwise, since the refcounting will be off, we might end up in a situation where the vblank management functions are called while the CRTC is off.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_crtc.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 5decae0069d0..78cbc3145e44 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -93,6 +93,8 @@ static void sun4i_crtc_atomic_disable(struct drm_crtc *crtc,
DRM_DEBUG_DRIVER("Disabling the CRTC\n");
+ drm_crtc_vblank_off(crtc); + sun4i_tcon_set_status(scrtc->tcon, encoder, false);
if (crtc->state->event && !crtc->state->active) { @@ -113,6 +115,8 @@ static void sun4i_crtc_atomic_enable(struct drm_crtc *crtc, DRM_DEBUG_DRIVER("Enabling the CRTC\n");
sun4i_tcon_set_status(scrtc->tcon, encoder, true); + + drm_crtc_vblank_on(crtc); }
static void sun4i_crtc_mode_set_nofb(struct drm_crtc *crtc)
On Wed, Feb 21, 2018 at 8:57 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:
Make sure that the CRTC code will call the enable/disable_vblank hooks.
Otherwise, since the refcounting will be off, we might end up in a situation where the vblank management functions are called while the CRTC is off.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
On Wed, Feb 21, 2018 at 8:57 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:
The current logic to deal with old DT missing the LVDS properties doesn't take into account whether the LVDS output is supported in the first place, resulting in spurious error messages on SoCs where it doesn't even matter.
Introduce a new TCON flag to list if LVDS is supported at all to prevent this from happening.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
drivers/gpu/drm/sun4i/sun4i_tcon.c | 86 ++++++++++++++++++++------------------ drivers/gpu/drm/sun4i/sun4i_tcon.h | 1 + 2 files changed, 46 insertions(+), 41 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.c b/drivers/gpu/drm/sun4i/sun4i_tcon.c index 3c15cf24b503..6ff0e2e84e81 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.c +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.c @@ -870,52 +870,56 @@ static int sun4i_tcon_bind(struct device *dev, struct device *master, return ret; }
/** This can only be made optional since we've had DT nodes* without the LVDS reset properties.** If the property is missing, just disable LVDS, and print a* warning.*/tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");if (IS_ERR(tcon->lvds_rst)) {dev_err(dev, "Couldn't get our reset line\n");return PTR_ERR(tcon->lvds_rst);} else if (tcon->lvds_rst) {has_lvds_rst = true;reset_control_reset(tcon->lvds_rst);} else {has_lvds_rst = false;}
if (tcon->quirks->supports_lvds) {/** This can only be made optional since we've had DT* nodes without the LVDS reset properties.** If the property is missing, just disable LVDS, and* print a warning.*/tcon->lvds_rst = devm_reset_control_get_optional(dev, "lvds");if (IS_ERR(tcon->lvds_rst)) {dev_err(dev, "Couldn't get our reset line\n");return PTR_ERR(tcon->lvds_rst);} else if (tcon->lvds_rst) {has_lvds_rst = true;reset_control_reset(tcon->lvds_rst);} else {has_lvds_rst = false;}
/** This can only be made optional since we've had DT nodes* without the LVDS reset properties.** If the property is missing, just disable LVDS, and print a* warning.*/if (tcon->quirks->has_lvds_alt) {tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");if (IS_ERR(tcon->lvds_pll)) {if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {has_lvds_alt = false;
/** This can only be made optional since we've had DT* nodes without the LVDS reset properties.
This comment doesn't match the code following it. Otherwise,
Reviewed-by: Chen-Yu Tsai wens@csie.org
** If the property is missing, just disable LVDS, and* print a warning.*/if (tcon->quirks->has_lvds_alt) {tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");if (IS_ERR(tcon->lvds_pll)) {if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {has_lvds_alt = false;} else {dev_err(dev, "Couldn't get the LVDS PLL\n");return PTR_ERR(tcon->lvds_pll);} } else {
dev_err(dev, "Couldn't get the LVDS PLL\n");return PTR_ERR(tcon->lvds_pll);
has_lvds_alt = true; }
} else {has_lvds_alt = true; }}if (!has_lvds_rst || (tcon->quirks->has_lvds_alt && !has_lvds_alt)) {dev_warn(dev,"Missing LVDS properties, Please upgrade your DT\n");dev_warn(dev, "LVDS output disabled\n");
if (!has_lvds_rst ||(tcon->quirks->has_lvds_alt && !has_lvds_alt)) {dev_warn(dev, "Missing LVDS properties, Please upgrade your DT\n");dev_warn(dev, "LVDS output disabled\n");can_lvds = false;} else {can_lvds = true;}} else { can_lvds = false;
} else {can_lvds = true; } ret = sun4i_tcon_init_clocks(dev, tcon);@@ -1134,7 +1138,7 @@ static const struct sun4i_tcon_quirks sun8i_a33_quirks = { };
static const struct sun4i_tcon_quirks sun8i_a83t_lcd_quirks = {
/* nothing is supported */
.supports_lvds = true,};
static const struct sun4i_tcon_quirks sun8i_v3s_quirks = { diff --git a/drivers/gpu/drm/sun4i/sun4i_tcon.h b/drivers/gpu/drm/sun4i/sun4i_tcon.h index b761c7b823c5..278700c7bf9f 100644 --- a/drivers/gpu/drm/sun4i/sun4i_tcon.h +++ b/drivers/gpu/drm/sun4i/sun4i_tcon.h @@ -175,6 +175,7 @@ struct sun4i_tcon_quirks { bool has_channel_1; /* a33 does not have channel 1 */ bool has_lvds_alt; /* Does the LVDS clock have a parent other than the TCON clock? */ bool needs_de_be_mux; /* sun6i needs mux to select backend */
bool supports_lvds; /* Does the TCON support an LVDS output? */ /* callback to handle tcon muxing options */ int (*set_mux)(struct sun4i_tcon *, const struct drm_encoder *);-- 2.14.3
Hi,
On Tue, Mar 06, 2018 at 03:38:25PM +0800, Chen-Yu Tsai wrote:
/** This can only be made optional since we've had DT nodes* without the LVDS reset properties.** If the property is missing, just disable LVDS, and print a* warning.*/if (tcon->quirks->has_lvds_alt) {tcon->lvds_pll = devm_clk_get(dev, "lvds-alt");if (IS_ERR(tcon->lvds_pll)) {if (PTR_ERR(tcon->lvds_pll) == -ENOENT) {has_lvds_alt = false;
/** This can only be made optional since we've had DT* nodes without the LVDS reset properties.This comment doesn't match the code following it. Otherwise,
It was there before, and it was a typo. I'll send a followup patch
Reviewed-by: Chen-Yu Tsai wens@csie.org
And I've applied all three patches.
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Giulio Benetti -
Maxime Ripard