[PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[Matt Redfearn]

In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
which attach to the DSI host via mipi_dsi_attach() at probe time, the
ADV7533 bridge device does not. Instead it defers this to the point that
the upstream device connects to its bridge via drm_bridge_attach().
The generic Synopsys MIPI DSI host driver does not register it's own
drm_bridge until the MIPI DSI has attached. But it does not call
drm_bridge_attach() on the downstream device until the upstream device
has attached. This leads to a chicken and the egg failure and the DRM
pipeline does not complete.
Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
the Synopsys MIPI DSI host registers it's bridge such that it is
available for the upstream device to connect to.

Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>

---

Changes in v2:
Cleanup if adv7533_attach_dsi fails.

 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index e7ddd3e3db9..807827bd910 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
 				 &adv7511_connector_helper_funcs);
 	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
 
-	if (adv->type == ADV7533)
-		ret = adv7533_attach_dsi(adv);
-
 	if (adv->i2c_main->irq)
 		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
 			     ADV7511_INT0_HPD);
@@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
 	drm_bridge_add(&adv7511->bridge);
 
 	adv7511_audio_init(dev, adv7511);
+
+	if (adv7511->type == ADV7533) {
+		ret = adv7533_attach_dsi(adv7511);
+		if (ret)
+			goto err_remove_bridge;
+	}
+
 	return 0;
 
+err_remove_bridge:
+	drm_bridge_remove(&adv7511->bridge);
 err_unregister_cec:
 	i2c_unregister_device(adv7511->i2c_cec);
 	if (adv7511->cec_clk)
-- 
2.17.1

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[Andrzej Hajda]

On 27.06.2019 17:18, Matt Redfearn wrote:
> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
> which attach to the DSI host via mipi_dsi_attach() at probe time, the
> ADV7533 bridge device does not. Instead it defers this to the point that
> the upstream device connects to its bridge via drm_bridge_attach().
> The generic Synopsys MIPI DSI host driver does not register it's own
> drm_bridge until the MIPI DSI has attached. But it does not call
> drm_bridge_attach() on the downstream device until the upstream device
> has attached. This leads to a chicken and the egg failure and the DRM
> pipeline does not complete.
> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
> the Synopsys MIPI DSI host registers it's bridge such that it is
> available for the upstream device to connect to.
>
> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>

Queued to drm-misc-next.


Regards

Andrzej

>
> ---
>
> Changes in v2:
> Cleanup if adv7533_attach_dsi fails.
>
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index e7ddd3e3db9..807827bd910 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
>  				 &adv7511_connector_helper_funcs);
>  	drm_connector_attach_encoder(&adv->connector, bridge->encoder);
>  
> -	if (adv->type == ADV7533)
> -		ret = adv7533_attach_dsi(adv);
> -
>  	if (adv->i2c_main->irq)
>  		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
>  			     ADV7511_INT0_HPD);
> @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>  	drm_bridge_add(&adv7511->bridge);
>  
>  	adv7511_audio_init(dev, adv7511);
> +
> +	if (adv7511->type == ADV7533) {
> +		ret = adv7533_attach_dsi(adv7511);
> +		if (ret)
> +			goto err_remove_bridge;
> +	}
> +
>  	return 0;
>  
> +err_remove_bridge:
> +	drm_bridge_remove(&adv7511->bridge);
>  err_unregister_cec:
>  	i2c_unregister_device(adv7511->i2c_cec);
>  	if (adv7511->cec_clk)

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[John Stultz]

On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote:
>
> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
> which attach to the DSI host via mipi_dsi_attach() at probe time, the
> ADV7533 bridge device does not. Instead it defers this to the point that
> the upstream device connects to its bridge via drm_bridge_attach().
> The generic Synopsys MIPI DSI host driver does not register it's own
> drm_bridge until the MIPI DSI has attached. But it does not call
> drm_bridge_attach() on the downstream device until the upstream device
> has attached. This leads to a chicken and the egg failure and the DRM
> pipeline does not complete.
> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
> the Synopsys MIPI DSI host registers it's bridge such that it is
> available for the upstream device to connect to.
>
> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>
>
> ---
>
> Changes in v2:
> Cleanup if adv7533_attach_dsi fails.
>
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index e7ddd3e3db9..807827bd910 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
>                                  &adv7511_connector_helper_funcs);
>         drm_connector_attach_encoder(&adv->connector, bridge->encoder);
>
> -       if (adv->type == ADV7533)
> -               ret = adv7533_attach_dsi(adv);
> -
>         if (adv->i2c_main->irq)
>                 regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
>                              ADV7511_INT0_HPD);
> @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
>         drm_bridge_add(&adv7511->bridge);
>
>         adv7511_audio_init(dev, adv7511);
> +
> +       if (adv7511->type == ADV7533) {
> +               ret = adv7533_attach_dsi(adv7511);
> +               if (ret)
> +                       goto err_remove_bridge;
> +       }
> +
>         return 0;
>
> +err_remove_bridge:
> +       drm_bridge_remove(&adv7511->bridge);
>  err_unregister_cec:
>         i2c_unregister_device(adv7511->i2c_cec);
>         if (adv7511->cec_clk)
> --

As a heads up, I just did some testing on drm-misc-next and this patch
seems to be breaking the HiKey board.  On bootup, I'm seeing:
[    4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using
dummy regulator
[    4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using
dummy regulator
[    4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using
dummy regulator
[    4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using
dummy regulator
[    4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using
dummy regulator
[    4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using
dummy regulator
[    4.272970] adv7511 2-0039: failed to find dsi host

over and over.  The dummy regulator messages are normal, but usually
[    4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops)

Starts up right afterward.

Reverting the change above seems to get things working again.  I've
not had much time to dig as to whats going wrong, but will keep
looking and wanted to raise the issue in the meantime.

thanks
-john

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[John Stultz]

On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote:
> >
> > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
> > which attach to the DSI host via mipi_dsi_attach() at probe time, the
> > ADV7533 bridge device does not. Instead it defers this to the point that
> > the upstream device connects to its bridge via drm_bridge_attach().
> > The generic Synopsys MIPI DSI host driver does not register it's own
> > drm_bridge until the MIPI DSI has attached. But it does not call
> > drm_bridge_attach() on the downstream device until the upstream device
> > has attached. This leads to a chicken and the egg failure and the DRM
> > pipeline does not complete.
> > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
> > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
> > the Synopsys MIPI DSI host registers it's bridge such that it is
> > available for the upstream device to connect to.
> >
> > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>
> >
> > ---
> >
> > Changes in v2:
> > Cleanup if adv7533_attach_dsi fails.
> >
> >  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > index e7ddd3e3db9..807827bd910 100644
> > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> > @@ -874,9 +874,6 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
> >                                  &adv7511_connector_helper_funcs);
> >         drm_connector_attach_encoder(&adv->connector, bridge->encoder);
> >
> > -       if (adv->type == ADV7533)
> > -               ret = adv7533_attach_dsi(adv);
> > -
> >         if (adv->i2c_main->irq)
> >                 regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
> >                              ADV7511_INT0_HPD);
> > @@ -1222,8 +1219,17 @@ static int adv7511_probe(struct i2c_client *i2c, const struct i2c_device_id *id)
> >         drm_bridge_add(&adv7511->bridge);
> >
> >         adv7511_audio_init(dev, adv7511);
> > +
> > +       if (adv7511->type == ADV7533) {
> > +               ret = adv7533_attach_dsi(adv7511);
> > +               if (ret)
> > +                       goto err_remove_bridge;
> > +       }
> > +
> >         return 0;
> >
> > +err_remove_bridge:
> > +       drm_bridge_remove(&adv7511->bridge);
> >  err_unregister_cec:
> >         i2c_unregister_device(adv7511->i2c_cec);
> >         if (adv7511->cec_clk)
> > --
>
> As a heads up, I just did some testing on drm-misc-next and this patch
> seems to be breaking the HiKey board.  On bootup, I'm seeing:
> [    4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using
> dummy regulator
> [    4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using
> dummy regulator
> [    4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using
> dummy regulator
> [    4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using
> dummy regulator
> [    4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using
> dummy regulator
> [    4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using
> dummy regulator
> [    4.272970] adv7511 2-0039: failed to find dsi host
>
> over and over.

I also just got a report that our testing is seeing this on the db410c
board as well:
  https://bugs.linaro.org/show_bug.cgi?id=5345

It seems like the change is making it try to attach to the dsi too
early before the dsi has registered?

thanks
-john

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[John Stultz]

On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote:
> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote:
> >
> > In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
> > which attach to the DSI host via mipi_dsi_attach() at probe time, the
> > ADV7533 bridge device does not. Instead it defers this to the point that
> > the upstream device connects to its bridge via drm_bridge_attach().
> > The generic Synopsys MIPI DSI host driver does not register it's own
> > drm_bridge until the MIPI DSI has attached. But it does not call
> > drm_bridge_attach() on the downstream device until the upstream device
> > has attached. This leads to a chicken and the egg failure and the DRM
> > pipeline does not complete.
> > Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
> > probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
> > the Synopsys MIPI DSI host registers it's bridge such that it is
> > available for the upstream device to connect to.
> >
> > Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>
> >
> > ---
>
> As a heads up, I just did some testing on drm-misc-next and this patch
> seems to be breaking the HiKey board.  On bootup, I'm seeing:
> [    4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using
> dummy regulator
> [    4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using
> dummy regulator
> [    4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using
> dummy regulator
> [    4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using
> dummy regulator
> [    4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using
> dummy regulator
> [    4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using
> dummy regulator
> [    4.272970] adv7511 2-0039: failed to find dsi host
>
> over and over.  The dummy regulator messages are normal, but usually
> [    4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops)
>
> Starts up right afterward.

Hey Matt, Andrzej,
  I just wanted to follow up on this as this patch is breaking a
couple of boards. Any sense of what might be missing, or is this
something we should revert?

I'm happy to test any patch ideas you have.

thanks
-john

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[Andrzej Hajda]

On 27.08.2019 22:03, John Stultz wrote:
> On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote:
>> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote:
>>> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
>>> which attach to the DSI host via mipi_dsi_attach() at probe time, the
>>> ADV7533 bridge device does not. Instead it defers this to the point that
>>> the upstream device connects to its bridge via drm_bridge_attach().
>>> The generic Synopsys MIPI DSI host driver does not register it's own
>>> drm_bridge until the MIPI DSI has attached. But it does not call
>>> drm_bridge_attach() on the downstream device until the upstream device
>>> has attached. This leads to a chicken and the egg failure and the DRM
>>> pipeline does not complete.
>>> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
>>> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
>>> the Synopsys MIPI DSI host registers it's bridge such that it is
>>> available for the upstream device to connect to.
>>>
>>> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>
>>>
>>> ---
>> As a heads up, I just did some testing on drm-misc-next and this patch
>> seems to be breaking the HiKey board.  On bootup, I'm seeing:
>> [    4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using
>> dummy regulator
>> [    4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using
>> dummy regulator
>> [    4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using
>> dummy regulator
>> [    4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using
>> dummy regulator
>> [    4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using
>> dummy regulator
>> [    4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using
>> dummy regulator
>> [    4.272970] adv7511 2-0039: failed to find dsi host
>>
>> over and over.  The dummy regulator messages are normal, but usually
>> [    4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops)
>>
>> Starts up right afterward.
> Hey Matt, Andrzej,
>   I just wanted to follow up on this as this patch is breaking a
> couple of boards. Any sense of what might be missing, or is this
> something we should revert?
>
> I'm happy to test any patch ideas you have.


I guess this is circular dependency issue:

- adv waits for dsi-host, then it creates drm_bridge,

- dsi-driver waits for drm_bridge, then it creates dsi host.


The patch introduces proper order:

- 1st we should register devices buses,

- then we should wait for drm components.


So the best solution would be to fix f4107800.dsi driver - it shouldn't
look for drm_bridge in probe, instead it should register dsi_host, and
in dsi host attach callback look for drm_bridge, then call component_add
(if all required resources are gathered), see
dw_mipi_dsi_rockchip_host_attach for example.


Regards

Andrzej



>
> thanks
> -john
>
>

Re: [PATCH v2] drm/bridge: adv7511: Attach to DSI host at probe time

[Matt Redfearn]

On 28/08/2019 08:05, Andrzej Hajda wrote:
> On 27.08.2019 22:03, John Stultz wrote:
>> On Mon, Aug 19, 2019 at 3:27 PM John Stultz <john.stultz@linaro.org> wrote:
>>> On Thu, Jun 27, 2019 at 8:18 AM Matt Redfearn <matt.redfearn@thinci.com> wrote:
>>>> In contrast to all of the DSI panel drivers in drivers/gpu/drm/panel
>>>> which attach to the DSI host via mipi_dsi_attach() at probe time, the
>>>> ADV7533 bridge device does not. Instead it defers this to the point that
>>>> the upstream device connects to its bridge via drm_bridge_attach().
>>>> The generic Synopsys MIPI DSI host driver does not register it's own
>>>> drm_bridge until the MIPI DSI has attached. But it does not call
>>>> drm_bridge_attach() on the downstream device until the upstream device
>>>> has attached. This leads to a chicken and the egg failure and the DRM
>>>> pipeline does not complete.
>>>> Since all other mipi_dsi_device drivers call mipi_dsi_attach() in
>>>> probe(), make the adv7533 mipi_dsi_device do the same. This ensures that
>>>> the Synopsys MIPI DSI host registers it's bridge such that it is
>>>> available for the upstream device to connect to.
>>>>
>>>> Signed-off-by: Matt Redfearn <matt.redfearn@thinci.com>
>>>>
>>>> ---
>>> As a heads up, I just did some testing on drm-misc-next and this patch
>>> seems to be breaking the HiKey board.  On bootup, I'm seeing:
>>> [    4.209615] adv7511 2-0039: 2-0039 supply avdd not found, using
>>> dummy regulator
>>> [    4.217075] adv7511 2-0039: 2-0039 supply dvdd not found, using
>>> dummy regulator
>>> [    4.224453] adv7511 2-0039: 2-0039 supply pvdd not found, using
>>> dummy regulator
>>> [    4.231804] adv7511 2-0039: 2-0039 supply a2vdd not found, using
>>> dummy regulator
>>> [    4.239242] adv7511 2-0039: 2-0039 supply v3p3 not found, using
>>> dummy regulator
>>> [    4.246615] adv7511 2-0039: 2-0039 supply v1p2 not found, using
>>> dummy regulator
>>> [    4.272970] adv7511 2-0039: failed to find dsi host
>>>
>>> over and over.  The dummy regulator messages are normal, but usually
>>> [    4.444315] kirin-drm f4100000.ade: bound f4107800.dsi (ops dsi_ops)
>>>
>>> Starts up right afterward.
>> Hey Matt, Andrzej,
>>    I just wanted to follow up on this as this patch is breaking a
>> couple of boards. Any sense of what might be missing, or is this
>> something we should revert?
>>
>> I'm happy to test any patch ideas you have.
> 
> 
> I guess this is circular dependency issue:
> 
> - adv waits for dsi-host, then it creates drm_bridge,
> 
> - dsi-driver waits for drm_bridge, then it creates dsi host.
> 
> 
> The patch introduces proper order:
> 
> - 1st we should register devices buses,
> 
> - then we should wait for drm components.
> 
> 
> So the best solution would be to fix f4107800.dsi driver - it shouldn't
> look for drm_bridge in probe, instead it should register dsi_host, and
> in dsi host attach callback look for drm_bridge, then call component_add
> (if all required resources are gathered), see
> dw_mipi_dsi_rockchip_host_attach for example.

Hi all,

Sorry that my patch seems to have broken things. It was using the 
generic synopsys dsi driver 
(drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c) that I found the 
circular dependency fixed by my patch.

It appears that the patch has broken things for the platform specific 
synopsys dsi driver for Kirin. So another approach (though a lot more 
work) would be to switch Kirin over to using the generic synopsys dsi 
driver so that we have less redundancy in the tree.

Thanks,
Matt

> 
> 
> Regards
> 
> Andrzej
> 
> 
> 
>>
>> thanks
>> -john
>>
>>
>