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

[Bjorn Andersson]

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@xxxxxxxxxxx>
> > > > ---
> > > >  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