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.ha...@samsung.com>
> Cc: Matt Redfearn <matt.redfe...@thinci.com>
> Cc: Xinliang Liu <z.liuxinli...@hisilicon.com>
> Cc: Rongrong Zou <zourongr...@gmail.com>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Neil Armstrong <narmstr...@baylibre.com>
> Cc: Jonas Karlman <jo...@kwiboo.se>
> Cc: Jernej Skrabec <jernej.skra...@siol.net>
> Cc: Thierry Reding <thierry.red...@gmail.com>
> Cc: David Airlie <airl...@linux.ie>,
> Cc: Sean Paul <seanp...@chromium.org>
> Cc: Sam Ravnborg <s...@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.stu...@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)
