Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom

From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>,
	Heikki Krogerus <heikki.krogerus@linux.intel.com>,
	Peter Chen <peter.chen@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	agross@kernel.org, gregkh@linuxfoundation.org,
	jackp@codeaurora.org, wcheng@codeaurora.org,
	linux-usb@vger.kernel.org, linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH 0/3] Implement role-switch notifications from dwc3-drd to dwc3-qcom
Date: Wed, 25 Aug 2021 08:53:29 -0700	[thread overview]
Message-ID: <YSZned9v1+ajzVx0@ripper> (raw)
In-Reply-To: <ce5f12dd-ddc1-6a9c-3dfb-aa44ea166828@linaro.org>

On Wed 25 Aug 01:18 PDT 2021, Bryan O'Donoghue wrote:

> On 25/08/2021 06:51, Felipe Balbi wrote:
> > > Hi Peter, I took a proper look at this again, hoping to find a way to
> > > pass a callback pointer from dwc3-qcom to the dwc3 core, that can be
> > > called from __dwc3_set_mode() to inform the Qualcomm glue about mode
> > > changes.
> > I would rather keep the strict separation between glue and core.
> 
> # 1 __dwc3_set_mode
> Felipe wants to keep a strict separation between core and glue
> 
> # notifier
> Requires the core probe() to complete before the glue probe to work
> reliably. This then would lead us to remaking the dwc3-qcom::probe() to
> facilitate probe deferral.
> 
> We can be sure bugs would be introduced in this process.
> 
> AFAIK Felipe is not opposed to this, Bjorn likes it
> 

Using a notifier or just a direct callback from core to the glue is an
implementation detail, but as you say we need a way for the glue to
register this before the core is fully probed.

> # 2 extcon
> Works but a) is deprecated and b) even if it weren't deprecated has no way
> to layer the messages - that I know of.
> 

Even with extcon, I really don't fancy the fact that we're duplicating
extcon registration in the glue and core - not to mention how it looks
in DT.

> # 3 USB role switch
> Already in-place for the producer {phy, type-c port, usb-gpio typec, google
> ecros} to consumer dwc-core. It already has a layering 1:1 of that array of
> producers to the consumer.
> 
> Unlike extcon though it cannot relay messages to more than one consumer.
> 
> As I see it we can either
> 
> A. Rewrite the dwc3-qcom probe to make it synchronous with dwc3-core probe
> taking the hit of whatever bugs get thrown up as a result of that over the
> next while, potentially years.
> 

The reason for it to be synchronous is that we need the glue to be able
to register it in a way that the core can acquire it when it probes
later.

> B. Use USB role switch in some format.
> 
> Either
> X. as I've submitted here based on a bit of code in dwc3-core or
> 
> Y. maybe trying to hide the "relay" aspect in DTS and USB role-switch core
> 

I don't think it's appropriate to work around the split model in DT.

> It seems to me our choices are notifier + pain and churn - perhaps low,
> perhaps high or USB role switch
> 
> 3.B.X works and is what has been submitted here but, if it is objectionable
> is 3.B.Y viable ?
> 
> As in make USB role switch propigate to multiple consumers via DTS and
> whatever additional work is required in the role-switch layer ?
> 
> + Heikki on that one.
> 

I've not seen the need for multiple consumer of role switching yet (I
don't find this a legit target for it).

But in the case of Type-C altmode several of our boards have either an
external gpio-based SBU-pin-swapper or some redriver on I2C with this
functionality, so we need a way to tell both the PHY and this external
contraption about the orientation.

Regards,
Bjorn