[PATCH 1/2] drm/imx: imx-ldb: disable LDB on driver bind

[PATCH 1/2] drm/imx: imx-ldb: disable LDB on driver bind

[Lucas Stach]

The LVDS signal integrity is only guaranteed when the correct enable
sequence (first IPU DI, then LDB) is used. If the LDB display output was
active before the imx-drm driver is loaded (like when a bootsplash was
active) the DI will be disabled by the full IPU reset we do when loading
the driver. The LDB control registers are not part of the IPU range and
thus will remain unchanged.

This leads to the LDB still being active when the DI is getting enabled,
effectively reversing the required enable sequence. Fix this by also
disabling the LDB on driver bind.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 56dd7a9a8e25..17974c0b4be8 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -612,6 +612,9 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		return PTR_ERR(imx_ldb->regmap);
 	}
 
+	/* disable LDB by resetting the control register to POR default */
+	regmap_write(imx_ldb->regmap, IOMUXC_GPR2, 0);
+
 	imx_ldb->dev = dev;
 
 	if (of_id)
-- 
2.16.1

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

[PATCH 2/2] drm/imx: imx-ldb: check if channel is enabled before printing warning

[Lucas Stach]

If the second LVDS channel has been disabled in the DT when using dual-channel
mode we should not print a warning.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/imx-ldb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
index 17974c0b4be8..dd5312b02a8d 100644
--- a/drivers/gpu/drm/imx/imx-ldb.c
+++ b/drivers/gpu/drm/imx/imx-ldb.c
@@ -655,14 +655,14 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
 		if (ret || i < 0 || i > 1)
 			return -EINVAL;
 
+		if (!of_device_is_available(child))
+			continue;
+
 		if (dual && i > 0) {
 			dev_warn(dev, "dual-channel mode, ignoring second output\n");
 			continue;
 		}
 
-		if (!of_device_is_available(child))
-			continue;
-
 		channel = &imx_ldb->channel[i];
 		channel->ldb = imx_ldb;
 		channel->chno = i;
-- 
2.16.1

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

Re: [PATCH 1/2] drm/imx: imx-ldb: disable LDB on driver bind

[Philipp Zabel]

Hi Lucas,

On Wed, 2018-04-11 at 17:31 +0200, Lucas Stach wrote:
> The LVDS signal integrity is only guaranteed when the correct enable
> sequence (first IPU DI, then LDB) is used. If the LDB display output was
> active before the imx-drm driver is loaded (like when a bootsplash was
> active) the DI will be disabled by the full IPU reset we do when loading
> the driver. The LDB control registers are not part of the IPU range and
> thus will remain unchanged.
> 
> This leads to the LDB still being active when the DI is getting enabled,
> effectively reversing the required enable sequence. Fix this by also
> disabling the LDB on driver bind.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-ldb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 56dd7a9a8e25..17974c0b4be8 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -612,6 +612,9 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
>  		return PTR_ERR(imx_ldb->regmap);
>  	}
>  
> +	/* disable LDB by resetting the control register to POR default */
> +	regmap_write(imx_ldb->regmap, IOMUXC_GPR2, 0);
> +

Both patches look good to me in principle, but wouldn't the correct
place to do this be imx_ldb_encoder_funcs.reset:

    struct drm_encoder_funcs {
        /**
         * @reset:
         *
         * Reset encoder hardware and software state to off. This function isn't
         * called by the core directly, only through drm_mode_config_reset().
         * It's not a helper hook only for historical reasons.
         */
        void (*reset)(struct drm_encoder *encoder);
    [...]
    };

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

Re: [PATCH 1/2] drm/imx: imx-ldb: disable LDB on driver bind

[Philipp Zabel]

On Wed, 2018-04-11 at 17:31 +0200, Lucas Stach wrote:
> The LVDS signal integrity is only guaranteed when the correct enable
> sequence (first IPU DI, then LDB) is used. If the LDB display output was
> active before the imx-drm driver is loaded (like when a bootsplash was
> active) the DI will be disabled by the full IPU reset we do when loading
> the driver. The LDB control registers are not part of the IPU range and
> thus will remain unchanged.
> 
> This leads to the LDB still being active when the DI is getting enabled,
> effectively reversing the required enable sequence. Fix this by also
> disabling the LDB on driver bind.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/imx-ldb.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/imx-ldb.c b/drivers/gpu/drm/imx/imx-ldb.c
> index 56dd7a9a8e25..17974c0b4be8 100644
> --- a/drivers/gpu/drm/imx/imx-ldb.c
> +++ b/drivers/gpu/drm/imx/imx-ldb.c
> @@ -612,6 +612,9 @@ static int imx_ldb_bind(struct device *dev, struct device *master, void *data)
>  		return PTR_ERR(imx_ldb->regmap);
>  	}
>  
> +	/* disable LDB by resetting the control register to POR default */
> +	regmap_write(imx_ldb->regmap, IOMUXC_GPR2, 0);
> +

On second thought, this is nice and clear. Moving it into encoder .reset
would unnecessarily complicate things, as both channels share this
register and .reset is called twice, once per channel.

I've applied both patches to imx-drm/next.

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