Re: [RFC][PATCH] drm: kirin: Fix dsi probe/attach logic

[Andrzej Hajda <a.hajda@samsung.com>]

On 29.08.2019 08:05, John Stultz wrote:
> Since commit 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI
> host at probe time") landed in -next the HiKey board would fail
> to boot, looping:
>
>   adv7511 2-0039: failed to find dsi host
>
> messages over and over. Andrzej Hajda suggested this is due to a
> circular dependency issue, and that the adv7511 change is
> correcting the improper order used earlier.
>
> Unfortunately this means the DSI drivers that use adv7511 need
> to also need to be updated to use the proper ordering to
> continue to work.
>
> This patch tries to reorder the initialization to register the
> dsi_host first, and then call component_add via dsi_host_attach,
> instead of doing that at probe time.
>
> This seems to resolve the issue with the HiKey board.
>
> Cc: Andrzej Hajda <a.hajda@samsung.com>
> Cc: Matt Redfearn <matt.redfearn@thinci.com>
> Cc: Xinliang Liu <z.liuxinliang@hisilicon.com>
> Cc: Rongrong Zou <zourongrong@gmail.com>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skrabec@siol.net>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: David Airlie <airlied@linux.ie>,
> Cc: Sean Paul <seanpaul@chromium.org>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>
> Fixes: 83f35bc3a852 ("drm/bridge: adv7511: Attach to DSI host at probe time")
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> Note: I'm really not super familiar with the DSI code here,
> and am mostly just trying to refactor the existing code in a
> similar fashion to the suggested dw-mipi-dsi-rockchip.c
> implementation. Careful review would be greatly appreciated!
>
> Also there is an outstanding regression on the db410c since it
> similarly uses the adv7511 and probably needs a similar rework.
> ---
>  drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c | 111 ++++++++++---------
>  1 file changed, 56 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> index 5bf8138941de..696cee1a1219 100644
> --- a/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> +++ b/drivers/gpu/drm/hisilicon/kirin/dw_drm_dsi.c
> @@ -79,6 +79,7 @@ struct dsi_hw_ctx {
>  };
>  
>  struct dw_dsi {
> +	struct device *dev;
>  	struct drm_encoder encoder;
>  	struct drm_bridge *bridge;
>  	struct mipi_dsi_host host;
> @@ -724,51 +725,6 @@ static int dw_drm_encoder_init(struct device *dev,
>  	return 0;
>  }
>  
> -static int dsi_host_attach(struct mipi_dsi_host *host,
> -			   struct mipi_dsi_device *mdsi)
> -{
> -	struct dw_dsi *dsi = host_to_dsi(host);
> -
> -	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> -		DRM_ERROR("dsi device params invalid\n");
> -		return -EINVAL;
> -	}
> -
> -	dsi->lanes = mdsi->lanes;
> -	dsi->format = mdsi->format;
> -	dsi->mode_flags = mdsi->mode_flags;
> -
> -	return 0;
> -}
> -
> -static int dsi_host_detach(struct mipi_dsi_host *host,
> -			   struct mipi_dsi_device *mdsi)
> -{
> -	/* do nothing */
> -	return 0;
> -}
> -
> -static const struct mipi_dsi_host_ops dsi_host_ops = {
> -	.attach = dsi_host_attach,
> -	.detach = dsi_host_detach,
> -};
> -
> -static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> -{
> -	struct mipi_dsi_host *host = &dsi->host;
> -	int ret;
> -
> -	host->dev = dev;
> -	host->ops = &dsi_host_ops;
> -	ret = mipi_dsi_host_register(host);
> -	if (ret) {
> -		DRM_ERROR("failed to register dsi host\n");
> -		return ret;
> -	}
> -
> -	return 0;
> -}
> -
>  static int dsi_bridge_init(struct drm_device *dev, struct dw_dsi *dsi)
>  {
>  	struct drm_encoder *encoder = &dsi->encoder;
> @@ -796,10 +752,6 @@ static int dsi_bind(struct device *dev, struct device *master, void *data)
>  	if (ret)
>  		return ret;
>  
> -	ret = dsi_host_init(dev, dsi);
> -	if (ret)
> -		return ret;
> -
>  	ret = dsi_bridge_init(drm_dev, dsi);
>  	if (ret)
>  		return ret;
> @@ -817,13 +769,22 @@ static const struct component_ops dsi_ops = {
>  	.unbind	= dsi_unbind,
>  };
>  
> -static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +static int dsi_host_attach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *mdsi)
>  {
> -	struct dsi_hw_ctx *ctx = dsi->ctx;
> -	struct device_node *np = pdev->dev.of_node;
> -	struct resource *res;
> +	struct dw_dsi *dsi = host_to_dsi(host);
> +	struct device_node *np = dsi->dev->of_node;
>  	int ret;
>  
> +	if (mdsi->lanes < 1 || mdsi->lanes > 4) {
> +		DRM_ERROR("dsi device params invalid\n");
> +		return -EINVAL;
> +	}
> +
> +	dsi->lanes = mdsi->lanes;
> +	dsi->format = mdsi->format;
> +	dsi->mode_flags = mdsi->mode_flags;
> +
>  	/*
>  	 * Get the endpoint node. In our case, dsi has one output port1
>  	 * to which the external HDMI bridge is connected.
> @@ -832,6 +793,42 @@ static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
>  	if (ret)
>  		return ret;
>  
> +	return component_add(dsi->dev, &dsi_ops);
> +}
> +
> +static int dsi_host_detach(struct mipi_dsi_host *host,
> +			   struct mipi_dsi_device *mdsi)
> +{
> +	/* do nothing */
> +	return 0;
> +}
> +
> +static const struct mipi_dsi_host_ops dsi_host_ops = {
> +	.attach = dsi_host_attach,
> +	.detach = dsi_host_detach,
> +};
> +
> +static int dsi_host_init(struct device *dev, struct dw_dsi *dsi)
> +{
> +	struct mipi_dsi_host *host = &dsi->host;
> +	int ret;
> +
> +	host->dev = dev;
> +	host->ops = &dsi_host_ops;
> +	ret = mipi_dsi_host_register(host);
> +	if (ret) {
> +		DRM_ERROR("failed to register dsi host\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int dsi_parse_dt(struct platform_device *pdev, struct dw_dsi *dsi)
> +{
> +	struct dsi_hw_ctx *ctx = dsi->ctx;
> +	struct resource *res;
> +
>  	ctx->pclk = devm_clk_get(&pdev->dev, "pclk");
>  	if (IS_ERR(ctx->pclk)) {
>  		DRM_ERROR("failed to get pclk clock\n");
> @@ -862,15 +859,19 @@ static int dsi_probe(struct platform_device *pdev)
>  	}
>  	dsi = &data->dsi;
>  	ctx = &data->ctx;
> +	dsi->dev = &pdev->dev;
>  	dsi->ctx = ctx;
>  
>  	ret = dsi_parse_dt(pdev, dsi);
>  	if (ret)
>  		return ret;
>  
> -	platform_set_drvdata(pdev, data);
> +	ret = dsi_host_init(&pdev->dev, dsi);
> +	if (ret)
> +		return ret;
>  
> -	return component_add(&pdev->dev, &dsi_ops);
> +	platform_set_drvdata(pdev, data);


I suspect platform_set_drvdata should be before dsi_host_init.

Imagine dsi_probe calls dsi_host_init it registers dsi_host and
instantly dsi_device appears, so dsi_host_attach is called, it calls
component_add and dsi_bind is called, which requires valid drvdata.


Beside this it looks correct, but it would be good to test it.

So after fixing above you can add my R-B.


Regards

Andrzej


> +	return 0;
>  }
>  
>  static int dsi_remove(struct platform_device *pdev)