[5/7] phy: qcom-qmp-combo: Introduce drm_bridge

  The QMP combo PHY sits in an of_graph connected between the DisplayPort
controller and a USB Type-C connector (or possibly a redriver).

The TCPM needs to be able to convey the HPD signal to the DisplayPort
controller, but no directly link is provided by DeviceTree so the signal
needs to "pass through" the QMP combo phy.

Handle this by introducing a drm_bridge which upon initialization finds
the next bridge (i.e. the usb-c-connector) and chain this together. This
way HPD changes in the connector will propagate to the DisplayPort
driver.

The connector bridge is resolved lazily, as the TCPM is expected to be
able to resolve the typec mux and switch at probe time, so the QMP combo
phy will probe before the TCPM.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
 1 file changed, 36 insertions(+)
  

On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> The QMP combo PHY sits in an of_graph connected between the DisplayPort
> controller and a USB Type-C connector (or possibly a redriver).
>
> The TCPM needs to be able to convey the HPD signal to the DisplayPort
> controller, but no directly link is provided by DeviceTree so the signal
> needs to "pass through" the QMP combo phy.
>
> Handle this by introducing a drm_bridge which upon initialization finds
> the next bridge (i.e. the usb-c-connector) and chain this together. This
> way HPD changes in the connector will propagate to the DisplayPort
> driver.
>
> The connector bridge is resolved lazily, as the TCPM is expected to be
> able to resolve the typec mux and switch at probe time, so the QMP combo
> phy will probe before the TCPM.
>
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 5d6d6ef3944b..84bc08002537 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -22,6 +22,8 @@
>  #include <linux/usb/typec.h>
>  #include <linux/usb/typec_mux.h>
>
> +#include <drm/drm_bridge.h>
> +
>  #include <dt-bindings/phy/phy-qcom-qmp.h>
>
>  #include "phy-qcom-qmp.h"
> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>         struct clk_hw dp_link_hw;
>         struct clk_hw dp_pixel_hw;
>
> +       struct drm_bridge bridge;
> +
>         struct typec_switch_dev *sw;
>         enum typec_orientation orientation;
>  };
> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
>         return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
>  }
>
> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> +                                  enum drm_bridge_attach_flags flags)
> +{
> +       struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> +       struct drm_bridge *next_bridge;
> +
> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> +               return -EINVAL;
> +
> +       next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> +       if (IS_ERR(next_bridge))
> +               return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> +
> +       return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +}
> +
> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> +       .attach = qmp_combo_bridge_attach,
> +};
> +
> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> +{
> +       qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> +       qmp->bridge.of_node = qmp->dev->of_node;
> +
> +       return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> +}
> +
>  static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
>  {
>         struct device *dev = qmp->dev;
> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
>         if (ret)
>                 return ret;
>
> +       ret = qmp_combo_dp_register_bridge(qmp);
> +       if (ret)
> +               return ret;
> +
>         /* Check for legacy binding with child nodes. */
>         usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>         if (usb_np) {
> --
> 2.39.2
>

You need to add some or all of these
       select DRM_DISPLAY_DP_HELPER
       select DRM_DISPLAY_HELPER
       select DRM_DP_AUX_BUS
       select DRM_KMS_HELPER
       select DRM_MIPI_DSI
       select DRM_PANEL


/opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
Unexpected GOT/PLT entries detected!
/opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
Unexpected run-time procedure linkages detected!
drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
`qmp_combo_bridge_attach':
phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
`devm_drm_of_get_bridge'
phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
`devm_drm_bridge_add'

---
bod
  

On 26/04/2023 12:33, Bryan O'Donoghue wrote:
> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
>>
>> The QMP combo PHY sits in an of_graph connected between the DisplayPort
>> controller and a USB Type-C connector (or possibly a redriver).
>>
>> The TCPM needs to be able to convey the HPD signal to the DisplayPort
>> controller, but no directly link is provided by DeviceTree so the signal
>> needs to "pass through" the QMP combo phy.
>>
>> Handle this by introducing a drm_bridge which upon initialization finds
>> the next bridge (i.e. the usb-c-connector) and chain this together. This
>> way HPD changes in the connector will propagate to the DisplayPort
>> driver.
>>
>> The connector bridge is resolved lazily, as the TCPM is expected to be
>> able to resolve the typec mux and switch at probe time, so the QMP combo
>> phy will probe before the TCPM.
>>
>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>>   1 file changed, 36 insertions(+)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> index 5d6d6ef3944b..84bc08002537 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>> @@ -22,6 +22,8 @@
>>   #include <linux/usb/typec.h>
>>   #include <linux/usb/typec_mux.h>
>>
>> +#include <drm/drm_bridge.h>
>> +
>>   #include <dt-bindings/phy/phy-qcom-qmp.h>
>>
>>   #include "phy-qcom-qmp.h"
>> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>>          struct clk_hw dp_link_hw;
>>          struct clk_hw dp_pixel_hw;
>>
>> +       struct drm_bridge bridge;
>> +
>>          struct typec_switch_dev *sw;
>>          enum typec_orientation orientation;
>>   };
>> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
>>          return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
>>   }
>>
>> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
>> +                                  enum drm_bridge_attach_flags flags)
>> +{
>> +       struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
>> +       struct drm_bridge *next_bridge;
>> +
>> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>> +               return -EINVAL;
>> +
>> +       next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
>> +       if (IS_ERR(next_bridge))
>> +               return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
>> +
>> +       return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>> +}
>> +
>> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
>> +       .attach = qmp_combo_bridge_attach,
>> +};
>> +
>> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
>> +{
>> +       qmp->bridge.funcs = &qmp_combo_bridge_funcs;
>> +       qmp->bridge.of_node = qmp->dev->of_node;
>> +
>> +       return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
>> +}
>> +
>>   static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
>>   {
>>          struct device *dev = qmp->dev;
>> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
>>          if (ret)
>>                  return ret;
>>
>> +       ret = qmp_combo_dp_register_bridge(qmp);
>> +       if (ret)
>> +               return ret;

I think the DRM part should be only built if CONFIG_DRM is enabled, I don't
have a strong opinion on this, I think Vinod could help here.

>> +
>>          /* Check for legacy binding with child nodes. */
>>          usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>>          if (usb_np) {
>> --
>> 2.39.2
>>
> 
> You need to add some or all of these
>         select DRM_DISPLAY_DP_HELPER
>         select DRM_DISPLAY_HELPER
>         select DRM_DP_AUX_BUS
>         select DRM_KMS_HELPER
>         select DRM_MIPI_DSI
>         select DRM_PANEL
> 
> 
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected GOT/PLT entries detected!
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected run-time procedure linkages detected!
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> `qmp_combo_bridge_attach':
> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> `devm_drm_of_get_bridge'
> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> `devm_drm_bridge_add'

I think CONFIG_DRM_PANEL_BRIDGE in addition to CONFIG_DRM. should be enough.

With this config added and my drm-bridge hat:

Acked-by: Neil Armstrong <neil.armstrong@linaro.org>

Neil


> 
> ---
> bod
  

On 27/04/2023 16:11, Neil Armstrong wrote:
> On 26/04/2023 12:33, Bryan O'Donoghue wrote:
>> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
>> <quic_bjorande@quicinc.com> wrote:
>>>
>>> The QMP combo PHY sits in an of_graph connected between the DisplayPort
>>> controller and a USB Type-C connector (or possibly a redriver).
>>>
>>> The TCPM needs to be able to convey the HPD signal to the DisplayPort
>>> controller, but no directly link is provided by DeviceTree so the signal
>>> needs to "pass through" the QMP combo phy.
>>>
>>> Handle this by introducing a drm_bridge which upon initialization finds
>>> the next bridge (i.e. the usb-c-connector) and chain this together. This
>>> way HPD changes in the connector will propagate to the DisplayPort
>>> driver.
>>>
>>> The connector bridge is resolved lazily, as the TCPM is expected to be
>>> able to resolve the typec mux and switch at probe time, so the QMP combo
>>> phy will probe before the TCPM.
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>>>   1 file changed, 36 insertions(+)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c 
>>> b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> index 5d6d6ef3944b..84bc08002537 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>>> @@ -22,6 +22,8 @@
>>>   #include <linux/usb/typec.h>
>>>   #include <linux/usb/typec_mux.h>
>>>
>>> +#include <drm/drm_bridge.h>
>>> +
>>>   #include <dt-bindings/phy/phy-qcom-qmp.h>
>>>
>>>   #include "phy-qcom-qmp.h"
>>> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>>>          struct clk_hw dp_link_hw;
>>>          struct clk_hw dp_pixel_hw;
>>>
>>> +       struct drm_bridge bridge;
>>> +
>>>          struct typec_switch_dev *sw;
>>>          enum typec_orientation orientation;
>>>   };
>>> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct 
>>> qmp_combo *qmp, struct device_node *
>>>          return devm_add_action_or_reset(qmp->dev, 
>>> phy_clk_release_provider, dp_np);
>>>   }
>>>
>>> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
>>> +                                  enum drm_bridge_attach_flags flags)
>>> +{
>>> +       struct qmp_combo *qmp = container_of(bridge, struct 
>>> qmp_combo, bridge);
>>> +       struct drm_bridge *next_bridge;
>>> +
>>> +       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
>>> +               return -EINVAL;
>>> +
>>> +       next_bridge = devm_drm_of_get_bridge(qmp->dev, 
>>> qmp->dev->of_node, 0, 0);
>>> +       if (IS_ERR(next_bridge))
>>> +               return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), 
>>> "failed to acquire drm_bridge\n");
>>> +
>>> +       return drm_bridge_attach(bridge->encoder, next_bridge, 
>>> bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>> +}
>>> +
>>> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
>>> +       .attach = qmp_combo_bridge_attach,
>>> +};
>>> +
>>> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
>>> +{
>>> +       qmp->bridge.funcs = &qmp_combo_bridge_funcs;
>>> +       qmp->bridge.of_node = qmp->dev->of_node;
>>> +
>>> +       return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
>>> +}
>>> +
>>>   static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, 
>>> struct device_node *np)
>>>   {
>>>          struct device *dev = qmp->dev;
>>> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct 
>>> platform_device *pdev)
>>>          if (ret)
>>>                  return ret;
>>>
>>> +       ret = qmp_combo_dp_register_bridge(qmp);
>>> +       if (ret)
>>> +               return ret;
> 
> I think the DRM part should be only built if CONFIG_DRM is enabled, I don't
> have a strong opinion on this, I think Vinod could help here.
> 
>>> +
>>>          /* Check for legacy binding with child nodes. */
>>>          usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>>>          if (usb_np) {
>>> -- 
>>> 2.39.2
>>>
>>
>> You need to add some or all of these
>>         select DRM_DISPLAY_DP_HELPER
>>         select DRM_DISPLAY_HELPER
>>         select DRM_DP_AUX_BUS
>>         select DRM_KMS_HELPER
>>         select DRM_MIPI_DSI
>>         select DRM_PANEL
>>
>>
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected GOT/PLT entries detected!
>> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
>> Unexpected run-time procedure linkages detected!
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
>> `qmp_combo_bridge_attach':
>> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
>> `devm_drm_of_get_bridge'
>> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to 
>> `drm_bridge_attach'
>> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
>> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
>> `devm_drm_bridge_add'
> 
> I think CONFIG_DRM_PANEL_BRIDGE in addition to CONFIG_DRM. should be 
> enough.
> 

I'd say DRM_PANEL_BRIDGE || !DRM_PANEL_BRIDGE in addition to DRM. And we 
probably should fix the  devm_drm_of_get_bridge() stub to use 
drm_of_find_panel_or_bridge() with panel = NULL.

> With this config added and my drm-bridge hat:
> 
> Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
> 
> Neil
> 
> 
>>
>> ---
>> bod
>
  

On Wed, Apr 26, 2023 at 11:33:40AM +0100, Bryan O'Donoghue wrote:
> On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> <quic_bjorande@quicinc.com> wrote:
> >
[..]
> You need to add some or all of these
>        select DRM_DISPLAY_DP_HELPER
>        select DRM_DISPLAY_HELPER
>        select DRM_DP_AUX_BUS
>        select DRM_KMS_HELPER
>        select DRM_MIPI_DSI
>        select DRM_PANEL
> 
> 
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected GOT/PLT entries detected!
> /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> Unexpected run-time procedure linkages detected!
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> `qmp_combo_bridge_attach':
> phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> `devm_drm_of_get_bridge'
> phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> `devm_drm_bridge_add'
> 

You're correct, and TYPEC. Realized that I forgot these once I had
posted the patches. Will figure out the actual set for v2.

Thanks,
Bjorn
  

On Thu, Apr 27, 2023 at 8:56 PM Bjorn Andersson
<quic_bjorande@quicinc.com> wrote:
>
> On Wed, Apr 26, 2023 at 11:33:40AM +0100, Bryan O'Donoghue wrote:
> > On Tue, Apr 25, 2023 at 4:40 AM Bjorn Andersson
> > <quic_bjorande@quicinc.com> wrote:
> > >
> [..]
> > You need to add some or all of these
> >        select DRM_DISPLAY_DP_HELPER
> >        select DRM_DISPLAY_HELPER
> >        select DRM_DP_AUX_BUS
> >        select DRM_KMS_HELPER
> >        select DRM_MIPI_DSI
> >        select DRM_PANEL
> >
> >
> > /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> > Unexpected GOT/PLT entries detected!
> > /opt/linaro/gcc-linaro-7.5.0-2019.12-x86_64_aarch64-linux-gnu/bin/aarch64-linux-gnu-ld:
> > Unexpected run-time procedure linkages detected!
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function
> > `qmp_combo_bridge_attach':
> > phy-qcom-qmp-combo.c:(.text+0xb50): undefined reference to
> > `devm_drm_of_get_bridge'
> > phy-qcom-qmp-combo.c:(.text+0xb6c): undefined reference to `drm_bridge_attach'
> > drivers/phy/qualcomm/phy-qcom-qmp-combo.o: In function `qmp_combo_probe':
> > phy-qcom-qmp-combo.c:(.text+0x13fc): undefined reference to
> > `devm_drm_bridge_add'
> >
>
> You're correct, and TYPEC. Realized that I forgot these once I had
> posted the patches. Will figure out the actual set for v2.
>
> Thanks,
> Bjorn

So I added CONFIG_DRM to Kconfig for the combo phy and then replaced
the old patch we had with your series.

Works for me with my TCPM set with zero changes - aside from slotting
the old PHY patch with your expanded series on SM8250

Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

https://git.codelinaro.org/bryan.odonoghue/kernel/-/tree/linux-next-23-04-28-pm8150b-tcpm-qcom-wrapper-typec-mux-bjorn
  

On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> The QMP combo PHY sits in an of_graph connected between the DisplayPort
> controller and a USB Type-C connector (or possibly a redriver).
> 
> The TCPM needs to be able to convey the HPD signal to the DisplayPort
> controller, but no directly link is provided by DeviceTree so the signal
> needs to "pass through" the QMP combo phy.
> 
> Handle this by introducing a drm_bridge which upon initialization finds
> the next bridge (i.e. the usb-c-connector) and chain this together. This
> way HPD changes in the connector will propagate to the DisplayPort
> driver.
> 
> The connector bridge is resolved lazily, as the TCPM is expected to be
> able to resolve the typec mux and switch at probe time, so the QMP combo
> phy will probe before the TCPM.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
>  1 file changed, 36 insertions(+)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> index 5d6d6ef3944b..84bc08002537 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> @@ -22,6 +22,8 @@
>  #include <linux/usb/typec.h>
>  #include <linux/usb/typec_mux.h>
>  
> +#include <drm/drm_bridge.h>
> +
>  #include <dt-bindings/phy/phy-qcom-qmp.h>
>  
>  #include "phy-qcom-qmp.h"
> @@ -1332,6 +1334,8 @@ struct qmp_combo {
>  	struct clk_hw dp_link_hw;
>  	struct clk_hw dp_pixel_hw;
>  
> +	struct drm_bridge bridge;
> +
>  	struct typec_switch_dev *sw;
>  	enum typec_orientation orientation;
>  };
> @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
>  	return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
>  }
>  
> +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> +				   enum drm_bridge_attach_flags flags)
> +{
> +	struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> +	struct drm_bridge *next_bridge;
> +
> +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> +		return -EINVAL;
> +
> +	next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> +	if (IS_ERR(next_bridge))
> +		return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");

Using dev_err_probe() in an attach callback looks wrong as these
functions should not be returning -EPROBE_DEFER (and this is not a probe
function).

> +
> +	return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);

This line is over 100 chars, but there should be no reason not to break
it before 80 here.

> +}
> +
> +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> +	.attach	= qmp_combo_bridge_attach,
> +};
> +
> +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> +{
> +	qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> +	qmp->bridge.of_node = qmp->dev->of_node;
> +
> +	return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> +}

Guess you need a dummy function also for qmp_combo_dp_register_bridge()
in case of !CONFIG_DRM.

> +
>  static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
>  {
>  	struct device *dev = qmp->dev;
> @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = qmp_combo_dp_register_bridge(qmp);
> +	if (ret)
> +		return ret;
> +
>  	/* Check for legacy binding with child nodes. */
>  	usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
>  	if (usb_np) {

Johan
  

On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > controller and a USB Type-C connector (or possibly a redriver).
> > 
> > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > controller, but no directly link is provided by DeviceTree so the signal
> > needs to "pass through" the QMP combo phy.
> > 
> > Handle this by introducing a drm_bridge which upon initialization finds
> > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > way HPD changes in the connector will propagate to the DisplayPort
> > driver.
> > 
> > The connector bridge is resolved lazily, as the TCPM is expected to be
> > able to resolve the typec mux and switch at probe time, so the QMP combo
> > phy will probe before the TCPM.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > index 5d6d6ef3944b..84bc08002537 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > @@ -22,6 +22,8 @@
> >  #include <linux/usb/typec.h>
> >  #include <linux/usb/typec_mux.h>
> >  
> > +#include <drm/drm_bridge.h>
> > +
> >  #include <dt-bindings/phy/phy-qcom-qmp.h>
> >  
> >  #include "phy-qcom-qmp.h"
> > @@ -1332,6 +1334,8 @@ struct qmp_combo {
> >  	struct clk_hw dp_link_hw;
> >  	struct clk_hw dp_pixel_hw;
> >  
> > +	struct drm_bridge bridge;
> > +
> >  	struct typec_switch_dev *sw;
> >  	enum typec_orientation orientation;
> >  };
> > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> >  	return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> >  }
> >  
> > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > +				   enum drm_bridge_attach_flags flags)
> > +{
> > +	struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > +	struct drm_bridge *next_bridge;
> > +
> > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > +		return -EINVAL;
> > +
> > +	next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > +	if (IS_ERR(next_bridge))
> > +		return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> 
> Using dev_err_probe() in an attach callback looks wrong as these
> functions should not be returning -EPROBE_DEFER (and this is not a probe
> function).
> 

The problem is that this might return EPROBE_DEFER, and at least today
propagates out to returning EPROBE_DEFER from our DP controller's
bind().

This is not optimal, but unfortunately we have a two way dependency
across the of_graph, so we need to make one of the sides lazy...

> > +
> > +	return drm_bridge_attach(bridge->encoder, next_bridge, bridge, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> 
> This line is over 100 chars, but there should be no reason not to break
> it before 80 here.
> 
> > +}
> > +
> > +static const struct drm_bridge_funcs qmp_combo_bridge_funcs = {
> > +	.attach	= qmp_combo_bridge_attach,
> > +};
> > +
> > +static int qmp_combo_dp_register_bridge(struct qmp_combo *qmp)
> > +{
> > +	qmp->bridge.funcs = &qmp_combo_bridge_funcs;
> > +	qmp->bridge.of_node = qmp->dev->of_node;
> > +
> > +	return devm_drm_bridge_add(qmp->dev, &qmp->bridge);
> > +}
> 
> Guess you need a dummy function also for qmp_combo_dp_register_bridge()
> in case of !CONFIG_DRM.
> 

Right, missed that dependency. Will wrap this and provide a dummy.

Thanks,
Bjorn

> > +
> >  static int qmp_combo_parse_dt_lecacy_dp(struct qmp_combo *qmp, struct device_node *np)
> >  {
> >  	struct device *dev = qmp->dev;
> > @@ -3459,6 +3491,10 @@ static int qmp_combo_probe(struct platform_device *pdev)
> >  	if (ret)
> >  		return ret;
> >  
> > +	ret = qmp_combo_dp_register_bridge(qmp);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* Check for legacy binding with child nodes. */
> >  	usb_np = of_get_child_by_name(dev->of_node, "usb3-phy");
> >  	if (usb_np) {
> 
> Johan
  

On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > controller and a USB Type-C connector (or possibly a redriver).
> > > 
> > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > controller, but no directly link is provided by DeviceTree so the signal
> > > needs to "pass through" the QMP combo phy.
> > > 
> > > Handle this by introducing a drm_bridge which upon initialization finds
> > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > way HPD changes in the connector will propagate to the DisplayPort
> > > driver.
> > > 
> > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > phy will probe before the TCPM.
> > > 
> > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > ---
> > >  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > >  1 file changed, 36 insertions(+)
> > > 
> > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > index 5d6d6ef3944b..84bc08002537 100644
> > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c

> > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > >  	return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > >  }
> > >  
> > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > +				   enum drm_bridge_attach_flags flags)
> > > +{
> > > +	struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > +	struct drm_bridge *next_bridge;
> > > +
> > > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > +		return -EINVAL;
> > > +
> > > +	next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > +	if (IS_ERR(next_bridge))
> > > +		return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> > 
> > Using dev_err_probe() in an attach callback looks wrong as these
> > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > function).
> 
> The problem is that this might return EPROBE_DEFER, and at least today
> propagates out to returning EPROBE_DEFER from our DP controller's
> bind().

Due to the known issue with the MSM driver panel lookup, or due to some
more fundamental problem with the stack?

At least in the former case, I don't think we should hide the fact that
we have an unresolved issue with the MSM driver this way even if it
means printing an extra error message until it has been resolved (cf.
the panel lookup errors that we've intentionally kept in place).

> This is not optimal, but unfortunately we have a two way dependency
> across the of_graph, so we need to make one of the sides lazy...

But this comments seems to suggest this is a bigger issue than the panel
lookup.

Could you describe the issue in some more detail (e.g. when would you
see -EPROBE_DEFER here)?

Johan
  

On Thu, 4 May 2023 at 11:38, Johan Hovold <johan@kernel.org> wrote:
>
> On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> > On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > > controller and a USB Type-C connector (or possibly a redriver).
> > > >
> > > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > > controller, but no directly link is provided by DeviceTree so the signal
> > > > needs to "pass through" the QMP combo phy.
> > > >
> > > > Handle this by introducing a drm_bridge which upon initialization finds
> > > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > > way HPD changes in the connector will propagate to the DisplayPort
> > > > driver.
> > > >
> > > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > > phy will probe before the TCPM.
> > > >
> > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > ---
> > > >  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > >
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > index 5d6d6ef3944b..84bc08002537 100644
> > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
>
> > > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > > >   return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > > >  }
> > > >
> > > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > > +                            enum drm_bridge_attach_flags flags)
> > > > +{
> > > > + struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > > + struct drm_bridge *next_bridge;
> > > > +
> > > > + if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > > +         return -EINVAL;
> > > > +
> > > > + next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > > + if (IS_ERR(next_bridge))
> > > > +         return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> > >
> > > Using dev_err_probe() in an attach callback looks wrong as these
> > > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > > function).
> >
> > The problem is that this might return EPROBE_DEFER, and at least today
> > propagates out to returning EPROBE_DEFER from our DP controller's
> > bind().
>
> Due to the known issue with the MSM driver panel lookup, or due to some
> more fundamental problem with the stack?

Ideally MSM DP driver should call component_add() only when the next
bridge is available. This is how we handle it for the DSI case.
However I'm yet to see the changes to dp_display_probe() which make
actual use of the done_probing callback. And even that will only fix
the eDP case. For the normal DP case we have no way of being properly
notified when the next bridge becomes available. So the driver will
try to drm_bridge_attach() from the component's bind() callback and
return an error if the chain is not (yet) fully available.

>
> At least in the former case, I don't think we should hide the fact that
> we have an unresolved issue with the MSM driver this way even if it
> means printing an extra error message until it has been resolved (cf.
> the panel lookup errors that we've intentionally kept in place).
>
> > This is not optimal, but unfortunately we have a two way dependency
> > across the of_graph, so we need to make one of the sides lazy...
>
> But this comments seems to suggest this is a bigger issue than the panel
> lookup.
>
> Could you describe the issue in some more detail (e.g. when would you
> see -EPROBE_DEFER here)?
>
> Johan
  

On Thu, May 04, 2023 at 10:38:54AM +0200, Johan Hovold wrote:
> On Wed, May 03, 2023 at 08:13:54PM -0700, Bjorn Andersson wrote:
> > On Tue, May 02, 2023 at 02:05:53PM +0200, Johan Hovold wrote:
> > > On Mon, Apr 24, 2023 at 08:40:08PM -0700, Bjorn Andersson wrote:
> > > > The QMP combo PHY sits in an of_graph connected between the DisplayPort
> > > > controller and a USB Type-C connector (or possibly a redriver).
> > > > 
> > > > The TCPM needs to be able to convey the HPD signal to the DisplayPort
> > > > controller, but no directly link is provided by DeviceTree so the signal
> > > > needs to "pass through" the QMP combo phy.
> > > > 
> > > > Handle this by introducing a drm_bridge which upon initialization finds
> > > > the next bridge (i.e. the usb-c-connector) and chain this together. This
> > > > way HPD changes in the connector will propagate to the DisplayPort
> > > > driver.
> > > > 
> > > > The connector bridge is resolved lazily, as the TCPM is expected to be
> > > > able to resolve the typec mux and switch at probe time, so the QMP combo
> > > > phy will probe before the TCPM.
> > > > 
> > > > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > > > ---
> > > >  drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 36 +++++++++++++++++++++++
> > > >  1 file changed, 36 insertions(+)
> > > > 
> > > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > index 5d6d6ef3944b..84bc08002537 100644
> > > > --- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> > > > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
> 
> > > > @@ -3196,6 +3200,34 @@ static int qmp_combo_register_clocks(struct qmp_combo *qmp, struct device_node *
> > > >  	return devm_add_action_or_reset(qmp->dev, phy_clk_release_provider, dp_np);
> > > >  }
> > > >  
> > > > +static int qmp_combo_bridge_attach(struct drm_bridge *bridge,
> > > > +				   enum drm_bridge_attach_flags flags)
> > > > +{
> > > > +	struct qmp_combo *qmp = container_of(bridge, struct qmp_combo, bridge);
> > > > +	struct drm_bridge *next_bridge;
> > > > +
> > > > +	if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> > > > +		return -EINVAL;
> > > > +
> > > > +	next_bridge = devm_drm_of_get_bridge(qmp->dev, qmp->dev->of_node, 0, 0);
> > > > +	if (IS_ERR(next_bridge))
> > > > +		return dev_err_probe(qmp->dev, PTR_ERR(next_bridge), "failed to acquire drm_bridge\n");
> > > 
> > > Using dev_err_probe() in an attach callback looks wrong as these
> > > functions should not be returning -EPROBE_DEFER (and this is not a probe
> > > function).
> > 
> > The problem is that this might return EPROBE_DEFER, and at least today
> > propagates out to returning EPROBE_DEFER from our DP controller's
> > bind().
> 
> Due to the known issue with the MSM driver panel lookup, or due to some
> more fundamental problem with the stack?
> 

No, but looks for the drm_bridge in the connector.

> At least in the former case, I don't think we should hide the fact that
> we have an unresolved issue with the MSM driver this way even if it
> means printing an extra error message until it has been resolved (cf.
> the panel lookup errors that we've intentionally kept in place).
> 
> > This is not optimal, but unfortunately we have a two way dependency
> > across the of_graph, so we need to make one of the sides lazy...
> 
> But this comments seems to suggest this is a bigger issue than the panel
> lookup.
> 
> Could you describe the issue in some more detail (e.g. when would you
> see -EPROBE_DEFER here)?
> 

pmic_glink needs to look up the typec_switch_dev through the of_graph,
which won't be present until the QMP phy is probed. And the QMP phy is
looking for the connector, which won't be present until pmic_glink has
found the QMP phy.

So what I'm saying is that either pmic_glink or QMP needs to look up the
other side lazily.


The attach happens during bind of the msm_drm component, so at least
today it's a consistent path to return EPROBE_DEFER in the DP
controller...

Regards,
Bjorn