[2/4] drm/tegra: dsi: Clear enable register if powered by bootloader

In cases where the DSI module is left on by the bootloader
some panels may fail to initialize if the enable register is not cleared
before the panel's initialization sequence. Clear it and add an optional
device tree property to inform the driver if this is the case.

Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
 drivers/gpu/drm/tegra/dsi.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> In cases where the DSI module is left on by the bootloader
> some panels may fail to initialize if the enable register is not cleared
> before the panel's initialization sequence. Clear it and add an optional
> device tree property to inform the driver if this is the case.
> 
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
>  drivers/gpu/drm/tegra/dsi.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index de1333dc0d86..f66514379913 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -78,6 +78,8 @@ struct tegra_dsi {
>  	unsigned int video_fifo_depth;
>  	unsigned int host_fifo_depth;
>  
> +	bool enabled;

There should be no need to track this. DRM/KMS internally already knows,
so we should make use of the built-in mechanisms as much as possible.

> +
>  	/* for ganged-mode support */
>  	struct tegra_dsi *master;
>  	struct tegra_dsi *slave;
> @@ -460,6 +462,8 @@ static void tegra_dsi_enable(struct tegra_dsi *dsi)
>  	value |= DSI_POWER_CONTROL_ENABLE;
>  	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>  
> +	dsi->enabled = true;
> +
>  	if (dsi->slave)
>  		tegra_dsi_enable(dsi->slave);
>  }
> @@ -737,6 +741,8 @@ static void tegra_dsi_disable(struct tegra_dsi *dsi)
>  	value &= ~DSI_POWER_CONTROL_ENABLE;
>  	tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>  
> +	dsi->enabled = false;
> +
>  	if (dsi->slave)
>  		tegra_dsi_disable(dsi->slave);
>  
> @@ -912,6 +918,27 @@ static void tegra_dsi_encoder_enable(struct drm_encoder *encoder)
>  	u32 value;
>  	int err;
>  
> +	/* If the bootloader enabled DSI it needs to be disabled
> +	 * in order for the panel initialization commands to be
> +	 * properly sent.
> +	 */
> +	if (dsi->enabled) {
> +		if (dc) {
> +			value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> +			value &= ~DSI_ENABLE;
> +			tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
> +
> +			tegra_dc_commit(dc);
> +		}
> +
> +		err = tegra_dsi_wait_idle(dsi, 100);
> +		if (err < 0)
> +			dev_dbg(dsi->dev, "failed to idle DSI: %d\n", err);
> +
> +		/* disable DSI controller */
> +		tegra_dsi_disable(dsi);
> +	}

This is suboptimal because it is largely a duplication of what we
already have in tegra_dsi_disable(). Also, see below.

> +
>  	err = tegra_dsi_prepare(dsi);
>  	if (err < 0) {
>  		dev_err(dsi->dev, "failed to prepare: %d\n", err);
> @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  
>  	dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
>  
> +	/* Check if the DSI module was left on by bootloader. */
> +	dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");

The isn't a documented property. But before you go and add this, are
there no alternative ways to detect that the DSI controller is active?
Could we not read one of the registers to find out?

DRM/KMS has built-in mechanisms to read back hardware state on boot, so
I wonder if we can hook that up. It'd make the most sense if all sub-
drivers did this, because then we could eventually inherit the
bootloader configuration and transition to the kernel display driver
seamlessly, but doing this in DSI first may help prepare for that more
extended use-case.

A slightly simpler alternative would be to add the reset code to the
encoder's or connector's ->reset() implementation. This is called at the
right time (i.e. when the mode configuration is first reset), so you can
run the workaround from tegra_dsi_encoder_enable() there. That's better
than having this guarded by the dsi->enabled flag so that it is run only
once.

Thierry

On Fri, Sep 30, 2022 at 01:11:10PM +0200, Thierry Reding wrote:
> On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> > +
> >  	err = tegra_dsi_prepare(dsi);
> >  	if (err < 0) {
> >  		dev_err(dsi->dev, "failed to prepare: %d\n", err);
> > @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
> >  
> >  	dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
> >  
> > +	/* Check if the DSI module was left on by bootloader. */
> > +	dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");
> 
> The isn't a documented property. But before you go and add this, are
> there no alternative ways to detect that the DSI controller is active?
> Could we not read one of the registers to find out?

Hello, thank you for your feedback.

You are correct, it is possible to simply read a register to obtain
this information, and this property is not needed.

> DRM/KMS has built-in mechanisms to read back hardware state on boot, so
> I wonder if we can hook that up. It'd make the most sense if all sub-
> drivers did this, because then we could eventually inherit the
> bootloader configuration and transition to the kernel display driver
> seamlessly, but doing this in DSI first may help prepare for that more
> extended use-case.

I have only recently started digging in the DRM/KMS subsystem, could
you point out what those mechanisms are? That end goal seems like
something worth pursuing.

> A slightly simpler alternative would be to add the reset code to the
> encoder's or connector's ->reset() implementation. This is called at the
> right time (i.e. when the mode configuration is first reset), so you can
> run the workaround from tegra_dsi_encoder_enable() there. That's better
> than having this guarded by the dsi->enabled flag so that it is run only
> once.
> 
> Thierry

Regarding the placement of the workaround, I placed it in encoder_enable()
since my attempts of placing it in other functions (such as the connector's
->reset() method) resulted in a kernel hang, and I have no solution for this.
I'm assuming this is due to some part of the DSI hardware not being fully
initialized, but I haven't been able to confirm this.

Best regards,

Diogo