[PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[AngeloGioacchino Del Regno]

Entering suspend while the display attached to this bridge is still on
makes the resume sequence to resume the bridge first, display last:
when this happens, we get a timeout while resuming the bridge, as its
MCU will get stuck due to the display being unpowered.

On the other hand, on mt8173-elm, closing the lid makes the display to
get powered off first, bridge last, so at resume time the sequence is
swapped (compared to the first example) and everything just works
as expected.

Add a stateless device link to the DRM device that this bridge belongs
to, ensuring a correct resume sequence and solving the unability to
correctly resume bridge operation in the first mentioned example.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 45100edd745b..191cc196c9d1 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -100,6 +100,7 @@ struct ps8640 {
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
+	struct device_link *link;
 	bool powered;
 };
 
@@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 		goto err_aux_register;
 	}
 
+	ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
+	if (!ps_bridge->link) {
+		dev_err(dev, "failed to create device link");
+		ret = -EINVAL;
+		goto err_devlink;
+	}
+
 	/* Attach the panel-bridge to the dsi bridge */
-	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
+	ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
+	if (ret)
+		goto err_bridge_attach;
 
+err_bridge_attach:
+	device_link_del(ps_bridge->link);
+err_devlink:
+	drm_dp_aux_unregister(&ps_bridge->aux);
 err_aux_register:
 	mipi_dsi_detach(dsi);
 err_dsi_attach:
@@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 
 static void ps8640_bridge_detach(struct drm_bridge *bridge)
 {
-	drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
+	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+
+	drm_dp_aux_unregister(&ps_bridge->aux);
+	if (ps_bridge->link)
+		device_link_del(ps_bridge->link);
 }
 
 static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
-- 
2.33.1

[PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[AngeloGioacchino Del Regno]

Entering suspend while the display attached to this bridge is still on
makes the resume sequence to resume the bridge first, display last:
when this happens, we get a timeout while resuming the bridge, as its
MCU will get stuck due to the display being unpowered.

On the other hand, on mt8173-elm, closing the lid makes the display to
get powered off first, bridge last, so at resume time the sequence is
swapped (compared to the first example) and everything just works
as expected.

Add a stateless device link to the DRM device that this bridge belongs
to, ensuring a correct resume sequence and solving the unability to
correctly resume bridge operation in the first mentioned example.

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 45100edd745b..191cc196c9d1 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -100,6 +100,7 @@ struct ps8640 {
 	struct regulator_bulk_data supplies[2];
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
+	struct device_link *link;
 	bool powered;
 };
 
@@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 		goto err_aux_register;
 	}
 
+	ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
+	if (!ps_bridge->link) {
+		dev_err(dev, "failed to create device link");
+		ret = -EINVAL;
+		goto err_devlink;
+	}
+
 	/* Attach the panel-bridge to the dsi bridge */
-	return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
+	ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
 				 &ps_bridge->bridge, flags);
+	if (ret)
+		goto err_bridge_attach;
 
+err_bridge_attach:
+	device_link_del(ps_bridge->link);
+err_devlink:
+	drm_dp_aux_unregister(&ps_bridge->aux);
 err_aux_register:
 	mipi_dsi_detach(dsi);
 err_dsi_attach:
@@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
 
 static void ps8640_bridge_detach(struct drm_bridge *bridge)
 {
-	drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
+	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
+
+	drm_dp_aux_unregister(&ps_bridge->aux);
+	if (ps_bridge->link)
+		device_link_del(ps_bridge->link);
 }
 
 static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
-- 
2.33.1

Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[Robert Foss]

Hey AngeloGioacchino,

On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Entering suspend while the display attached to this bridge is still on
> makes the resume sequence to resume the bridge first, display last:
> when this happens, we get a timeout while resuming the bridge, as its
> MCU will get stuck due to the display being unpowered.
>
> On the other hand, on mt8173-elm, closing the lid makes the display to
> get powered off first, bridge last, so at resume time the sequence is
> swapped (compared to the first example) and everything just works
> as expected.
>
> Add a stateless device link to the DRM device that this bridge belongs
> to, ensuring a correct resume sequence and solving the unability to
> correctly resume bridge operation in the first mentioned example.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 45100edd745b..191cc196c9d1 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -100,6 +100,7 @@ struct ps8640 {
>         struct regulator_bulk_data supplies[2];
>         struct gpio_desc *gpio_reset;
>         struct gpio_desc *gpio_powerdown;
> +       struct device_link *link;
>         bool powered;
>  };
>
> @@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>                 goto err_aux_register;
>         }
>
> +       ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
> +       if (!ps_bridge->link) {
> +               dev_err(dev, "failed to create device link");
> +               ret = -EINVAL;
> +               goto err_devlink;
> +       }
> +
>         /* Attach the panel-bridge to the dsi bridge */
> -       return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
> +       ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>                                  &ps_bridge->bridge, flags);
> +       if (ret)
> +               goto err_bridge_attach;
>
> +err_bridge_attach:
> +       device_link_del(ps_bridge->link);
> +err_devlink:
> +       drm_dp_aux_unregister(&ps_bridge->aux);
>  err_aux_register:
>         mipi_dsi_detach(dsi);
>  err_dsi_attach:
> @@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>
>  static void ps8640_bridge_detach(struct drm_bridge *bridge)
>  {
> -       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
> +       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +
> +       drm_dp_aux_unregister(&ps_bridge->aux);
> +       if (ps_bridge->link)
> +               device_link_del(ps_bridge->link);
>  }
>
>  static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> --
> 2.33.1
>

This patch does not apply on drm-misc-next, could you rebase it on the
current branch?

Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[Robert Foss]

Hey AngeloGioacchino,

On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Entering suspend while the display attached to this bridge is still on
> makes the resume sequence to resume the bridge first, display last:
> when this happens, we get a timeout while resuming the bridge, as its
> MCU will get stuck due to the display being unpowered.
>
> On the other hand, on mt8173-elm, closing the lid makes the display to
> get powered off first, bridge last, so at resume time the sequence is
> swapped (compared to the first example) and everything just works
> as expected.
>
> Add a stateless device link to the DRM device that this bridge belongs
> to, ensuring a correct resume sequence and solving the unability to
> correctly resume bridge operation in the first mentioned example.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
> index 45100edd745b..191cc196c9d1 100644
> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
> @@ -100,6 +100,7 @@ struct ps8640 {
>         struct regulator_bulk_data supplies[2];
>         struct gpio_desc *gpio_reset;
>         struct gpio_desc *gpio_powerdown;
> +       struct device_link *link;
>         bool powered;
>  };
>
> @@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>                 goto err_aux_register;
>         }
>
> +       ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
> +       if (!ps_bridge->link) {
> +               dev_err(dev, "failed to create device link");
> +               ret = -EINVAL;
> +               goto err_devlink;
> +       }
> +
>         /* Attach the panel-bridge to the dsi bridge */
> -       return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
> +       ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>                                  &ps_bridge->bridge, flags);
> +       if (ret)
> +               goto err_bridge_attach;
>
> +err_bridge_attach:
> +       device_link_del(ps_bridge->link);
> +err_devlink:
> +       drm_dp_aux_unregister(&ps_bridge->aux);
>  err_aux_register:
>         mipi_dsi_detach(dsi);
>  err_dsi_attach:
> @@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>
>  static void ps8640_bridge_detach(struct drm_bridge *bridge)
>  {
> -       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
> +       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
> +
> +       drm_dp_aux_unregister(&ps_bridge->aux);
> +       if (ps_bridge->link)
> +               device_link_del(ps_bridge->link);
>  }
>
>  static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
> --
> 2.33.1
>

This patch does not apply on drm-misc-next, could you rebase it on the
current branch?

Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[AngeloGioacchino Del Regno]

Il 04/01/22 15:22, Robert Foss ha scritto:
> Hey AngeloGioacchino,
> 
> On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Entering suspend while the display attached to this bridge is still on
>> makes the resume sequence to resume the bridge first, display last:
>> when this happens, we get a timeout while resuming the bridge, as its
>> MCU will get stuck due to the display being unpowered.
>>
>> On the other hand, on mt8173-elm, closing the lid makes the display to
>> get powered off first, bridge last, so at resume time the sequence is
>> swapped (compared to the first example) and everything just works
>> as expected.
>>
>> Add a stateless device link to the DRM device that this bridge belongs
>> to, ensuring a correct resume sequence and solving the unability to
>> correctly resume bridge operation in the first mentioned example.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index 45100edd745b..191cc196c9d1 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -100,6 +100,7 @@ struct ps8640 {
>>          struct regulator_bulk_data supplies[2];
>>          struct gpio_desc *gpio_reset;
>>          struct gpio_desc *gpio_powerdown;
>> +       struct device_link *link;
>>          bool powered;
>>   };
>>
>> @@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>                  goto err_aux_register;
>>          }
>>
>> +       ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
>> +       if (!ps_bridge->link) {
>> +               dev_err(dev, "failed to create device link");
>> +               ret = -EINVAL;
>> +               goto err_devlink;
>> +       }
>> +
>>          /* Attach the panel-bridge to the dsi bridge */
>> -       return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>> +       ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>>                                   &ps_bridge->bridge, flags);
>> +       if (ret)
>> +               goto err_bridge_attach;
>>
>> +err_bridge_attach:
>> +       device_link_del(ps_bridge->link);
>> +err_devlink:
>> +       drm_dp_aux_unregister(&ps_bridge->aux);
>>   err_aux_register:
>>          mipi_dsi_detach(dsi);
>>   err_dsi_attach:
>> @@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>
>>   static void ps8640_bridge_detach(struct drm_bridge *bridge)
>>   {
>> -       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
>> +       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +
>> +       drm_dp_aux_unregister(&ps_bridge->aux);
>> +       if (ps_bridge->link)
>> +               device_link_del(ps_bridge->link);
>>   }
>>
>>   static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>> --
>> 2.33.1
>>
> 
> This patch does not apply on drm-misc-next, could you rebase it on the
> current branch?
> 

Sure, I'll rebase it asap.

Re: [PATCH] drm/bridge: parade-ps8640: Link device to ensure suspend/resume order

[AngeloGioacchino Del Regno]

Il 04/01/22 15:22, Robert Foss ha scritto:
> Hey AngeloGioacchino,
> 
> On Tue, 2 Nov 2021 at 14:08, AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
>>
>> Entering suspend while the display attached to this bridge is still on
>> makes the resume sequence to resume the bridge first, display last:
>> when this happens, we get a timeout while resuming the bridge, as its
>> MCU will get stuck due to the display being unpowered.
>>
>> On the other hand, on mt8173-elm, closing the lid makes the display to
>> get powered off first, bridge last, so at resume time the sequence is
>> swapped (compared to the first example) and everything just works
>> as expected.
>>
>> Add a stateless device link to the DRM device that this bridge belongs
>> to, ensuring a correct resume sequence and solving the unability to
>> correctly resume bridge operation in the first mentioned example.
>>
>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
>> ---
>>   drivers/gpu/drm/bridge/parade-ps8640.c | 22 ++++++++++++++++++++--
>>   1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
>> index 45100edd745b..191cc196c9d1 100644
>> --- a/drivers/gpu/drm/bridge/parade-ps8640.c
>> +++ b/drivers/gpu/drm/bridge/parade-ps8640.c
>> @@ -100,6 +100,7 @@ struct ps8640 {
>>          struct regulator_bulk_data supplies[2];
>>          struct gpio_desc *gpio_reset;
>>          struct gpio_desc *gpio_powerdown;
>> +       struct device_link *link;
>>          bool powered;
>>   };
>>
>> @@ -460,10 +461,23 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>                  goto err_aux_register;
>>          }
>>
>> +       ps_bridge->link = device_link_add(bridge->dev->dev, dev, DL_FLAG_STATELESS);
>> +       if (!ps_bridge->link) {
>> +               dev_err(dev, "failed to create device link");
>> +               ret = -EINVAL;
>> +               goto err_devlink;
>> +       }
>> +
>>          /* Attach the panel-bridge to the dsi bridge */
>> -       return drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>> +       ret = drm_bridge_attach(bridge->encoder, ps_bridge->panel_bridge,
>>                                   &ps_bridge->bridge, flags);
>> +       if (ret)
>> +               goto err_bridge_attach;
>>
>> +err_bridge_attach:
>> +       device_link_del(ps_bridge->link);
>> +err_devlink:
>> +       drm_dp_aux_unregister(&ps_bridge->aux);
>>   err_aux_register:
>>          mipi_dsi_detach(dsi);
>>   err_dsi_attach:
>> @@ -473,7 +487,11 @@ static int ps8640_bridge_attach(struct drm_bridge *bridge,
>>
>>   static void ps8640_bridge_detach(struct drm_bridge *bridge)
>>   {
>> -       drm_dp_aux_unregister(&bridge_to_ps8640(bridge)->aux);
>> +       struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
>> +
>> +       drm_dp_aux_unregister(&ps_bridge->aux);
>> +       if (ps_bridge->link)
>> +               device_link_del(ps_bridge->link);
>>   }
>>
>>   static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
>> --
>> 2.33.1
>>
> 
> This patch does not apply on drm-misc-next, could you rebase it on the
> current branch?
> 

Sure, I'll rebase it asap.