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

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

[Bryan O'Donoghue]

This is a topic we have been discussing for some time, initially in the
context of gpio usb-c-connector role-switching.

https://lore.kernel.org/linux-usb/20200311191501.8165-1-bryan.odonoghue@linaro.org

Hardware availability constraints limited scope to finish that off.

Thankfully Wesley Cheng made a new set of USB role-switch related patches
for dwc3-qcom, this time in conjunction with the qcom pm8150b type-c
silicon.

https://lore.kernel.org/linux-usb/20201009082843.28503-1-wcheng@codeaurora.org

For the RB5 project we picked Wesley's changes and developed them further,
around a type-c port manager.

As a precursor to that TCPM I reposted Wesley's patches
https://lore.kernel.org/linux-usb/20210629144449.2550737-1-bryan.odonoghue@linaro.org

Bjorn pointed out that having the role-switch triggered from dwc3-qcom to
dwc3-drd is not the right way around, indicating a preference for the
original notifier from dwc3-drd to dwc3-qcom.

There are two approaches I considred and prototyped to accomplish the
desired dwc3-drd -> dwc3-qcom messaging.

#1 Using a notifier in dwc3-drd to trigger dwc3-qcom

   This would be nice since it would accomplish the desired layering
   dwc3-drd -> dwc3-qcom.

   However:
   a) It would be a real mess as dwc3-qcom is the parent device of
      dwc3-core so, if the child-device dwc3-core deferred probing for
      whatever reason we would have to detect this and retry the parent's
      probe again. The point in time that dwc3-qcom could potentially parse
      such a deferral in the child device is late. It would also be weird
      and messy to try to roll back the parent's probe because of a child
      device deferral.

      I considered making some sort of worker in the parent to check for
      child device probe but, again this seemed like an atrocious hack so,
      I didn't even try to prototype that.

   b) One potential solution was using "__weak" linkage in a function
      provided by dwc3-drd that a wrapper such as dwc3-qcom could then
      over-ride.

      If a wrapper such as dwc3-qcom then implemented a function with
      regular linkage it would over-ride the __weak function and provide a
      method for the dwc3-drd code to call into dwc3-qcom when probing was
      complete, thus allowing registration of the notifier when the child
      was ready.

      This would work up until the point that you tried to compile two
      implementations of a dwc3 wrapper into the one kernel module or the
      one kernel image say dwc3-qcom and a similar implementation in
      dwc3-meson. At that point you would get linker breakage.

#2 Using USB role switching for the notification

   Wesley's implementation took the approach dwc3-qcom -> dwc3-drd, whereas
   the approach I'm proposing here is dwc3-drd -> dwc3-qcom, which is also
   what we discussed on the list.

   Having implemented it, I think USB role-switching in the direction
   dwc3-drd -> dwc3-qcom is also a much cleaner solution for several
   reasons.

   a) Handling probe deferral is built into Linux' USB role switching today
      so we don't have to re-invent that wheel, unlike with the original
      notifier model.

   b) There is no "wiring up" or traversing the graph tree for the wrapper
      layer to determine if the parent device has a compliant type-c
      connector associated with it, unlike in the dwc3-qcom -> dwc3-drd
      model.

      All that has to happen is "usb-role-switch" is declared in the parent
      dwc3-qcom node and the role-switch API takes care of the rest.

      That means its possible to use a usb-c-connector, qcom type-c pm8150b
      driver, a USCI, a tps659x, a fusb302 or something like ChromeOS
      cros_ec to notify dwc3-drd without dwc3-qcom having to have
      the slighest clue which type of device is sending the signal.

      All dwc3-qcom needs to do is waggle UTMI signals in a register when a
      role-switch happens.

   c) It "feels" like a layering violation to have the dwc3-qcom SoC
      wrapper receive the event and trigger the dwc3-drd core.

      The standard model of parent/child role switching or remote-endpoint
      traversal that USB role switching already has works just fine for
      dwc3-drd, we just need to trigger dwc3-qcom for the role-switch in a
      non-vendor and non-SoC specific way.

   d) Less code. It turns out there's less code implementing as a
      role-switch interface in the direction dwc3-drd -> dwc3-qcom.

   e) Portability. The mechanism used for dwc3-drd -> dwc3 qcom can be
      reused for any other similar wrapper which models the wrapper as a
      parent of the dwc3-drd.

For all of those reasons I've opted to use USB role-switch notification
from dwc3-drd to dwc3-qcom.

git add bod git://git.linaro.org/people/bryan.odonoghue/kernel.git
git fetch bod
git diff usb-next-5.13.rcx-rb5-tcpm..usb-next-5.13.rcx-rb5-tcpm-v2

Bryan O'Donoghue (2):
  usb: dwc3: Add role switch relay support
  usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient

Wesley Cheng (1):
  usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API

 drivers/usb/dwc3/core.h      |  2 +
 drivers/usb/dwc3/drd.c       | 10 +++++
 drivers/usb/dwc3/dwc3-qcom.c | 77 ++++++++++++++++++++++++++++++++++--
 3 files changed, 85 insertions(+), 4 deletions(-)

-- 
2.30.1

[PATCH 1/3] usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API

[Bryan O'Donoghue]

From: Wesley Cheng <wcheng@codeaurora.org>

There was an extra character in the dwc3_qcom_vbus_override_enable()
function.  Removed the extra character.

Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 49e6ca94486d..2e61302e3e91 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -115,7 +115,7 @@ static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
 	readl(base + offset);
 }
 
-static void dwc3_qcom_vbus_overrride_enable(struct dwc3_qcom *qcom, bool enable)
+static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
 {
 	if (enable) {
 		dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_SS_PHY_CTRL,
@@ -136,7 +136,7 @@ static int dwc3_qcom_vbus_notifier(struct notifier_block *nb,
 	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, vbus_nb);
 
 	/* enable vbus override for device mode */
-	dwc3_qcom_vbus_overrride_enable(qcom, event);
+	dwc3_qcom_vbus_override_enable(qcom, event);
 	qcom->mode = event ? USB_DR_MODE_PERIPHERAL : USB_DR_MODE_HOST;
 
 	return NOTIFY_DONE;
@@ -148,7 +148,7 @@ static int dwc3_qcom_host_notifier(struct notifier_block *nb,
 	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, host_nb);
 
 	/* disable vbus override in host mode */
-	dwc3_qcom_vbus_overrride_enable(qcom, !event);
+	dwc3_qcom_vbus_override_enable(qcom, !event);
 	qcom->mode = event ? USB_DR_MODE_HOST : USB_DR_MODE_PERIPHERAL;
 
 	return NOTIFY_DONE;
@@ -811,7 +811,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 
 	/* enable vbus override for device mode */
 	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
-		dwc3_qcom_vbus_overrride_enable(qcom, true);
+		dwc3_qcom_vbus_override_enable(qcom, true);
 
 	/* register extcon to override sw_vbus on Vbus change later */
 	ret = dwc3_qcom_register_extcon(qcom);
-- 
2.30.1

Re: [PATCH 1/3] usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API

[Bjorn Andersson]

On Sat 03 Jul 20:33 CDT 2021, Bryan O'Donoghue wrote:

> From: Wesley Cheng <wcheng@codeaurora.org>
> 
> There was an extra character in the dwc3_qcom_vbus_override_enable()
> function.  Removed the extra character.
> 
> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 49e6ca94486d..2e61302e3e91 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -115,7 +115,7 @@ static inline void dwc3_qcom_clrbits(void __iomem *base, u32 offset, u32 val)
>  	readl(base + offset);
>  }
>  
> -static void dwc3_qcom_vbus_overrride_enable(struct dwc3_qcom *qcom, bool enable)
> +static void dwc3_qcom_vbus_override_enable(struct dwc3_qcom *qcom, bool enable)
>  {
>  	if (enable) {
>  		dwc3_qcom_setbits(qcom->qscratch_base, QSCRATCH_SS_PHY_CTRL,
> @@ -136,7 +136,7 @@ static int dwc3_qcom_vbus_notifier(struct notifier_block *nb,
>  	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, vbus_nb);
>  
>  	/* enable vbus override for device mode */
> -	dwc3_qcom_vbus_overrride_enable(qcom, event);
> +	dwc3_qcom_vbus_override_enable(qcom, event);
>  	qcom->mode = event ? USB_DR_MODE_PERIPHERAL : USB_DR_MODE_HOST;
>  
>  	return NOTIFY_DONE;
> @@ -148,7 +148,7 @@ static int dwc3_qcom_host_notifier(struct notifier_block *nb,
>  	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, host_nb);
>  
>  	/* disable vbus override in host mode */
> -	dwc3_qcom_vbus_overrride_enable(qcom, !event);
> +	dwc3_qcom_vbus_override_enable(qcom, !event);
>  	qcom->mode = event ? USB_DR_MODE_HOST : USB_DR_MODE_PERIPHERAL;
>  
>  	return NOTIFY_DONE;
> @@ -811,7 +811,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  
>  	/* enable vbus override for device mode */
>  	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
> -		dwc3_qcom_vbus_overrride_enable(qcom, true);
> +		dwc3_qcom_vbus_override_enable(qcom, true);
>  
>  	/* register extcon to override sw_vbus on Vbus change later */
>  	ret = dwc3_qcom_register_extcon(qcom);
> -- 
> 2.30.1
> 

[PATCH 2/3] usb: dwc3: Add role switch relay support

[Bryan O'Donoghue]

Add the ability to relay a role switch message from the core to the parent
device of the controller.

This is useful for the qcom-dwc3 wrapper which wants to receive role-switch
notifications in order to waggle internal SoC UTMI signals.

Having the core trigger the parent wrapper has the advantage of keeping the
connector mechanism agnostic from dwc3 wrapper code and means that any
other wrapper implementation on any other SoC or architecture need not
write custom code to find various types of Type-C role switch mechanisms.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/core.h |  2 ++
 drivers/usb/dwc3/drd.c  | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index dccdf13b5f9e..974104cc16f7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -993,6 +993,7 @@ struct dwc3_scratchpad_array {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_sw_relay: usb_role_switch relay handle
  * @role_switch_default_mode: default operation mode of controller while
  *			usb role is USB_ROLE_NONE.
  * @usb_psy: pointer to power supply interface.
@@ -1136,6 +1137,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	struct usb_role_switch	*role_sw_relay;
 	enum usb_dr_mode	role_switch_default_mode;
 
 	struct power_supply	*usb_psy;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 8fcbac10510c..74293861bd8f 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -507,6 +507,9 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
 	}
 
 	dwc3_set_mode(dwc, mode);
+	if (dwc->role_sw_relay)
+		usb_role_switch_set_role(dwc->role_sw_relay, role);
+
 	return 0;
 }
 
@@ -563,6 +566,10 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 	if (IS_ERR(dwc->role_sw))
 		return PTR_ERR(dwc->role_sw);
 
+	dwc->role_sw_relay = usb_role_switch_get(dwc->dev);
+	if (IS_ERR(dwc->role_sw_relay))
+		return PTR_ERR(dwc->role_sw_relay);
+
 	dwc3_set_mode(dwc, mode);
 	return 0;
 }
@@ -630,6 +637,9 @@ void dwc3_drd_exit(struct dwc3 *dwc)
 {
 	unsigned long flags;
 
+	if (dwc->role_sw_relay)
+		usb_role_switch_put(dwc->role_sw_relay);
+
 	if (dwc->role_sw)
 		usb_role_switch_unregister(dwc->role_sw);
 
-- 
2.30.1

Re: [PATCH 2/3] usb: dwc3: Add role switch relay support

[Peter Chen]

On 21-07-04 02:33:13, Bryan O'Donoghue wrote:
> Add the ability to relay a role switch message from the core to the parent
> device of the controller.
> 
> This is useful for the qcom-dwc3 wrapper which wants to receive role-switch
> notifications in order to waggle internal SoC UTMI signals.
> 
> Having the core trigger the parent wrapper has the advantage of keeping the
> connector mechanism agnostic from dwc3 wrapper code and means that any
> other wrapper implementation on any other SoC or architecture need not
> write custom code to find various types of Type-C role switch mechanisms.

If I understand correctly, the call trace like below:

Connector (GPIO/Type-C, usb_role_switch_set_role) -> dwc3 core (func: dwc3_usb_role_switch_set)
-> dwc3 qcom glue (func: dwc3_qcom_usb_role_switch_set)

And, at dts, the property "usb-role-switch" will be at both dwc3 glue and core node
dwc3 core is the connector (GPIO/Type-C)'s role switch 
dwc3 glue is the dwc3 core's role switch

right?

Peter

> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/usb/dwc3/core.h |  2 ++
>  drivers/usb/dwc3/drd.c  | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index dccdf13b5f9e..974104cc16f7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -993,6 +993,7 @@ struct dwc3_scratchpad_array {
>   *		- USBPHY_INTERFACE_MODE_UTMI
>   *		- USBPHY_INTERFACE_MODE_UTMIW
>   * @role_sw: usb_role_switch handle
> + * @role_sw_relay: usb_role_switch relay handle
>   * @role_switch_default_mode: default operation mode of controller while
>   *			usb role is USB_ROLE_NONE.
>   * @usb_psy: pointer to power supply interface.
> @@ -1136,6 +1137,7 @@ struct dwc3 {
>  	struct notifier_block	edev_nb;
>  	enum usb_phy_interface	hsphy_mode;
>  	struct usb_role_switch	*role_sw;
> +	struct usb_role_switch	*role_sw_relay;
>  	enum usb_dr_mode	role_switch_default_mode;
>  
>  	struct power_supply	*usb_psy;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 8fcbac10510c..74293861bd8f 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -507,6 +507,9 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
>  	}
>  
>  	dwc3_set_mode(dwc, mode);
> +	if (dwc->role_sw_relay)
> +		usb_role_switch_set_role(dwc->role_sw_relay, role);
> +
>  	return 0;
>  }
>  
> @@ -563,6 +566,10 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  	if (IS_ERR(dwc->role_sw))
>  		return PTR_ERR(dwc->role_sw);
>  
> +	dwc->role_sw_relay = usb_role_switch_get(dwc->dev);
> +	if (IS_ERR(dwc->role_sw_relay))
> +		return PTR_ERR(dwc->role_sw_relay);
> +
>  	dwc3_set_mode(dwc, mode);
>  	return 0;
>  }
> @@ -630,6 +637,9 @@ void dwc3_drd_exit(struct dwc3 *dwc)
>  {
>  	unsigned long flags;
>  
> +	if (dwc->role_sw_relay)
> +		usb_role_switch_put(dwc->role_sw_relay);
> +
>  	if (dwc->role_sw)
>  		usb_role_switch_unregister(dwc->role_sw);
>  
> -- 
> 2.30.1
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 2/3] usb: dwc3: Add role switch relay support

[Bryan O'Donoghue]

On 06/07/2021 03:51, Peter Chen wrote:
> If I understand correctly, the call trace like below:
> 
> Connector (GPIO/Type-C, usb_role_switch_set_role) -> dwc3 core (func: dwc3_usb_role_switch_set)
> -> dwc3 qcom glue (func: dwc3_qcom_usb_role_switch_set)

Yes with dwc3_qcom_usb_role_switch_set() then toggling 
dwc3_qcom_vbus_override_enable() which is the missing piece right now

> And, at dts, the property "usb-role-switch" will be at both dwc3 glue and core node
> dwc3 core is the connector (GPIO/Type-C)'s role switch
> dwc3 glue is the dwc3 core's role switch
> 
> right?

Yes, since the glue is modeled as the parent device of the core all that 
has to happen between dwc3 core and dwc3-qcom is "usb-role-swtich" 
declared in the parent dwc3-qcom entry

---
bod

Re: [PATCH 2/3] usb: dwc3: Add role switch relay support

[Bjorn Andersson]

On Sat 03 Jul 20:33 CDT 2021, Bryan O'Donoghue wrote:

> Add the ability to relay a role switch message from the core to the parent
> device of the controller.
> 
> This is useful for the qcom-dwc3 wrapper which wants to receive role-switch
> notifications in order to waggle internal SoC UTMI signals.
> 
> Having the core trigger the parent wrapper has the advantage of keeping the
> connector mechanism agnostic from dwc3 wrapper code and means that any
> other wrapper implementation on any other SoC or architecture need not
> write custom code to find various types of Type-C role switch mechanisms.
> 
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/usb/dwc3/core.h |  2 ++
>  drivers/usb/dwc3/drd.c  | 10 ++++++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index dccdf13b5f9e..974104cc16f7 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -993,6 +993,7 @@ struct dwc3_scratchpad_array {
>   *		- USBPHY_INTERFACE_MODE_UTMI
>   *		- USBPHY_INTERFACE_MODE_UTMIW
>   * @role_sw: usb_role_switch handle
> + * @role_sw_relay: usb_role_switch relay handle
>   * @role_switch_default_mode: default operation mode of controller while
>   *			usb role is USB_ROLE_NONE.
>   * @usb_psy: pointer to power supply interface.
> @@ -1136,6 +1137,7 @@ struct dwc3 {
>  	struct notifier_block	edev_nb;
>  	enum usb_phy_interface	hsphy_mode;
>  	struct usb_role_switch	*role_sw;
> +	struct usb_role_switch	*role_sw_relay;
>  	enum usb_dr_mode	role_switch_default_mode;
>  
>  	struct power_supply	*usb_psy;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 8fcbac10510c..74293861bd8f 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -507,6 +507,9 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
>  	}
>  
>  	dwc3_set_mode(dwc, mode);
> +	if (dwc->role_sw_relay)
> +		usb_role_switch_set_role(dwc->role_sw_relay, role);
> +
>  	return 0;
>  }
>  
> @@ -563,6 +566,10 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  	if (IS_ERR(dwc->role_sw))
>  		return PTR_ERR(dwc->role_sw);
>  
> +	dwc->role_sw_relay = usb_role_switch_get(dwc->dev);

So you're going to rely on DT to create a role-switch link between the
dwc3 core driver and the qcom wrapper?

As I said the other day, this is just a hack to get around the fact that
of_platform_populate() may return before the dwc3 core has probed.

Another such case can be seen in [1], where Wesley is patching the DT
node in runtime in order to pass a boolean parameter between the two
driver parts.

If we fix that we don't need these kinds of workarounds.

[1] https://lore.kernel.org/linux-arm-msm/1625218655-14180-6-git-send-email-wcheng@codeaurora.org/

Regards,
Bjorn

> +	if (IS_ERR(dwc->role_sw_relay))
> +		return PTR_ERR(dwc->role_sw_relay);
> +
>  	dwc3_set_mode(dwc, mode);
>  	return 0;
>  }
> @@ -630,6 +637,9 @@ void dwc3_drd_exit(struct dwc3 *dwc)
>  {
>  	unsigned long flags;
>  
> +	if (dwc->role_sw_relay)
> +		usb_role_switch_put(dwc->role_sw_relay);
> +
>  	if (dwc->role_sw)
>  		usb_role_switch_unregister(dwc->role_sw);
>  
> -- 
> 2.30.1
> 

Re: [PATCH 2/3] usb: dwc3: Add role switch relay support

[Bryan O'Donoghue]

On 07/07/2021 06:14, Bjorn Andersson wrote:
> So you're going to rely on DT to create a role-switch link between the
> dwc3 core driver and the qcom wrapper?
> 
> As I said the other day, this is just a hack to get around the fact that
> of_platform_populate() may return before the dwc3 core has probed.

A beautiful hack though

> Another such case can be seen in [1], where Wesley is patching the DT
> node in runtime in order to pass a boolean parameter between the two
> driver parts.
> 
> If we fix that we don't need these kinds of workarounds.

OK, I understand/agree

Lets look at changing the way dwc3-qcom launches dwc3-core and make it that

1. Probe deferral in dwc3-core either defers the dwc3-qcom code
2. Or that the dwc3-qcom code waits for dwc3-core

either way instead of avoiding the fact that dwc3-core can defer we 
should make it that dwc3-qcom::probe() exits only when the dwc3-core is done

---
bod

Re: [PATCH 2/3] usb: dwc3: Add role switch relay support

[Bryan O'Donoghue]

On 07/07/2021 10:49, Bryan O'Donoghue wrote:
> On 07/07/2021 06:14, Bjorn Andersson wrote:
>> So you're going to rely on DT to create a role-switch link between the
>> dwc3 core driver and the qcom wrapper?
>>
>> As I said the other day, this is just a hack to get around the fact that
>> of_platform_populate() may return before the dwc3 core has probed.

> either way instead of avoiding the fact that dwc3-core can defer we 
> should make it that dwc3-qcom::probe() exits* only when the dwc3-core is 

*completes

[PATCH 3/3] usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient

[Bryan O'Donoghue]

When switching role from host to peripheral or peripheral to host we need
to set SoC UTMI signal logic in software since this is not done
automatically by the PHY or DWC3 core.

We have existing extcon code in dwc3-qcom which already implements the
right logic for extcon based systems, however, as we move to USB
role-switching we need to similarly facilitate the same UTMI switch
notification.

Setting the dwc3-qcom wrapper up as a USB role switch signal recipient
allows us to replicate the extcon logic with the role-switch API by
receiving the set_role() from dwc3-core and calling the existing VBUS
extcon code.

Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 69 ++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 2e61302e3e91..1aec387a8537 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -20,6 +20,7 @@
 #include <linux/usb/of.h>
 #include <linux/reset.h>
 #include <linux/iopoll.h>
+#include <linux/usb/role.h>
 
 #include "core.h"
 
@@ -81,6 +82,7 @@ struct dwc3_qcom {
 	struct extcon_dev	*host_edev;
 	struct notifier_block	vbus_nb;
 	struct notifier_block	host_nb;
+	struct usb_role_switch	*role_sw;
 
 	const struct dwc3_acpi_pdata *acpi_pdata;
 
@@ -154,6 +156,66 @@ static int dwc3_qcom_host_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
+static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw,
+					 enum usb_role role)
+{
+	struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw);
+	bool enable;
+
+	switch (role) {
+	case USB_ROLE_DEVICE:
+		qcom->mode = USB_DR_MODE_PERIPHERAL;
+		enable = true;
+		break;
+	case USB_ROLE_HOST:
+	default:
+		qcom->mode = USB_DR_MODE_HOST;
+		enable = false;
+		break;
+	}
+
+	dwc3_qcom_vbus_override_enable(qcom, enable);
+	return 0;
+}
+
+static enum usb_role dwc3_qcom_usb_role_switch_get(struct usb_role_switch *sw)
+{
+	struct dwc3_qcom *qcom = usb_role_switch_get_drvdata(sw);
+	enum usb_role role;
+
+	switch (qcom->mode) {
+	case USB_DR_MODE_PERIPHERAL:
+		role = USB_ROLE_DEVICE;
+		break;
+	case  USB_DR_MODE_HOST:
+	default:
+		role = USB_ROLE_HOST;
+		break;
+	}
+
+	return role;
+}
+
+static int dwc3_qcom_setup_role_switch(struct dwc3_qcom *qcom)
+{
+	struct usb_role_switch_desc dwc3_qcom_role_switch = {NULL};
+
+	dwc3_qcom_role_switch.fwnode = dev_fwnode(qcom->dev);
+	dwc3_qcom_role_switch.set = dwc3_qcom_usb_role_switch_set;
+	dwc3_qcom_role_switch.get = dwc3_qcom_usb_role_switch_get;
+	dwc3_qcom_role_switch.driver_data = qcom;
+	qcom->role_sw = usb_role_switch_register(qcom->dev,
+						 &dwc3_qcom_role_switch);
+	if (IS_ERR(qcom->role_sw))
+		return PTR_ERR(qcom->role_sw);
+
+	return 0;
+}
+#else
+#define dwc3_qcom_setup_role_switch(x) 0
+#endif
+
 static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
 {
 	struct device		*dev = qcom->dev;
@@ -818,6 +880,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
+	ret = dwc3_qcom_setup_role_switch(qcom);
+	if (ret)
+		goto interconnect_exit;
+
 	device_init_wakeup(&pdev->dev, 1);
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
@@ -850,6 +916,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	if (qcom->role_sw)
+		usb_role_switch_unregister(qcom->role_sw);
+
 	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 
-- 
2.30.1

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

[Peter Chen]

On 21-07-04 02:33:11, Bryan O'Donoghue wrote:
> This is a topic we have been discussing for some time, initially in the
> context of gpio usb-c-connector role-switching.
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-1-bryan.odonoghue@linaro.org
> 
> Hardware availability constraints limited scope to finish that off.
> 
> Thankfully Wesley Cheng made a new set of USB role-switch related patches
> for dwc3-qcom, this time in conjunction with the qcom pm8150b type-c
> silicon.
> 
> https://lore.kernel.org/linux-usb/20201009082843.28503-1-wcheng@codeaurora.org
> 
> For the RB5 project we picked Wesley's changes and developed them further,
> around a type-c port manager.
> 
> As a precursor to that TCPM I reposted Wesley's patches
> https://lore.kernel.org/linux-usb/20210629144449.2550737-1-bryan.odonoghue@linaro.org
> 
> Bjorn pointed out that having the role-switch triggered from dwc3-qcom to
> dwc3-drd is not the right way around, indicating a preference for the
> original notifier from dwc3-drd to dwc3-qcom.
> 
> There are two approaches I considred and prototyped to accomplish the
> desired dwc3-drd -> dwc3-qcom messaging.
> 
> #1 Using a notifier in dwc3-drd to trigger dwc3-qcom
> 
>    This would be nice since it would accomplish the desired layering
>    dwc3-drd -> dwc3-qcom.
> 
>    However:
>    a) It would be a real mess as dwc3-qcom is the parent device of
>       dwc3-core so, if the child-device dwc3-core deferred probing for
>       whatever reason we would have to detect this and retry the parent's
>       probe again.

Why do you think we need to retry the parent's probe again? And why using
a notifier need to concern core's deferral probe? I know there are some
downstream code which using this way, I would like to know the shortcoming
for it.

Peter

>	The point in time that dwc3-qcom could potentially parse
>       such a deferral in the child device is late. It would also be weird
>       and messy to try to roll back the parent's probe because of a child
>       device deferral.
> 
>       I considered making some sort of worker in the parent to check for
>       child device probe but, again this seemed like an atrocious hack so,
>       I didn't even try to prototype that.
> 
>    b) One potential solution was using "__weak" linkage in a function
>       provided by dwc3-drd that a wrapper such as dwc3-qcom could then
>       over-ride.
> 
>       If a wrapper such as dwc3-qcom then implemented a function with
>       regular linkage it would over-ride the __weak function and provide a
>       method for the dwc3-drd code to call into dwc3-qcom when probing was
>       complete, thus allowing registration of the notifier when the child
>       was ready.
> 
>       This would work up until the point that you tried to compile two
>       implementations of a dwc3 wrapper into the one kernel module or the
>       one kernel image say dwc3-qcom and a similar implementation in
>       dwc3-meson. At that point you would get linker breakage.
> 
> #2 Using USB role switching for the notification
> 
>    Wesley's implementation took the approach dwc3-qcom -> dwc3-drd, whereas
>    the approach I'm proposing here is dwc3-drd -> dwc3-qcom, which is also
>    what we discussed on the list.
> 
>    Having implemented it, I think USB role-switching in the direction
>    dwc3-drd -> dwc3-qcom is also a much cleaner solution for several
>    reasons.
> 
>    a) Handling probe deferral is built into Linux' USB role switching today
>       so we don't have to re-invent that wheel, unlike with the original
>       notifier model.
> 
>    b) There is no "wiring up" or traversing the graph tree for the wrapper
>       layer to determine if the parent device has a compliant type-c
>       connector associated with it, unlike in the dwc3-qcom -> dwc3-drd
>       model.
> 
>       All that has to happen is "usb-role-switch" is declared in the parent
>       dwc3-qcom node and the role-switch API takes care of the rest.
> 
>       That means its possible to use a usb-c-connector, qcom type-c pm8150b
>       driver, a USCI, a tps659x, a fusb302 or something like ChromeOS
>       cros_ec to notify dwc3-drd without dwc3-qcom having to have
>       the slighest clue which type of device is sending the signal.
> 
>       All dwc3-qcom needs to do is waggle UTMI signals in a register when a
>       role-switch happens.
> 
>    c) It "feels" like a layering violation to have the dwc3-qcom SoC
>       wrapper receive the event and trigger the dwc3-drd core.
> 
>       The standard model of parent/child role switching or remote-endpoint
>       traversal that USB role switching already has works just fine for
>       dwc3-drd, we just need to trigger dwc3-qcom for the role-switch in a
>       non-vendor and non-SoC specific way.
> 
>    d) Less code. It turns out there's less code implementing as a
>       role-switch interface in the direction dwc3-drd -> dwc3-qcom.
> 
>    e) Portability. The mechanism used for dwc3-drd -> dwc3 qcom can be
>       reused for any other similar wrapper which models the wrapper as a
>       parent of the dwc3-drd.
> 
> For all of those reasons I've opted to use USB role-switch notification
> from dwc3-drd to dwc3-qcom.
> 
> git add bod git://git.linaro.org/people/bryan.odonoghue/kernel.git
> git fetch bod
> git diff usb-next-5.13.rcx-rb5-tcpm..usb-next-5.13.rcx-rb5-tcpm-v2
> 
> Bryan O'Donoghue (2):
>   usb: dwc3: Add role switch relay support
>   usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient
> 
> Wesley Cheng (1):
>   usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API
> 
>  drivers/usb/dwc3/core.h      |  2 +
>  drivers/usb/dwc3/drd.c       | 10 +++++
>  drivers/usb/dwc3/dwc3-qcom.c | 77 ++++++++++++++++++++++++++++++++++--
>  3 files changed, 85 insertions(+), 4 deletions(-)
> 
> -- 
> 2.30.1
> 

-- 

Thanks,
Peter Chen

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

[Bjorn Andersson]

On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:

> On 21-07-04 02:33:11, Bryan O'Donoghue wrote:
> > This is a topic we have been discussing for some time, initially in the
> > context of gpio usb-c-connector role-switching.
> > 
> > https://lore.kernel.org/linux-usb/20200311191501.8165-1-bryan.odonoghue@linaro.org
> > 
> > Hardware availability constraints limited scope to finish that off.
> > 
> > Thankfully Wesley Cheng made a new set of USB role-switch related patches
> > for dwc3-qcom, this time in conjunction with the qcom pm8150b type-c
> > silicon.
> > 
> > https://lore.kernel.org/linux-usb/20201009082843.28503-1-wcheng@codeaurora.org
> > 
> > For the RB5 project we picked Wesley's changes and developed them further,
> > around a type-c port manager.
> > 
> > As a precursor to that TCPM I reposted Wesley's patches
> > https://lore.kernel.org/linux-usb/20210629144449.2550737-1-bryan.odonoghue@linaro.org
> > 
> > Bjorn pointed out that having the role-switch triggered from dwc3-qcom to
> > dwc3-drd is not the right way around, indicating a preference for the
> > original notifier from dwc3-drd to dwc3-qcom.
> > 
> > There are two approaches I considred and prototyped to accomplish the
> > desired dwc3-drd -> dwc3-qcom messaging.
> > 
> > #1 Using a notifier in dwc3-drd to trigger dwc3-qcom
> > 
> >    This would be nice since it would accomplish the desired layering
> >    dwc3-drd -> dwc3-qcom.
> > 
> >    However:
> >    a) It would be a real mess as dwc3-qcom is the parent device of
> >       dwc3-core so, if the child-device dwc3-core deferred probing for
> >       whatever reason we would have to detect this and retry the parent's
> >       probe again.
> 

Allow me to reorder your two questions:

> And why using a notifier need to concern core's deferral probe?

The problem at hand calls for the core for somehow invoking
dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.

This means that dwc3-qcom somehow needs to inform the dwc3-core about
this (and stash the pointer). And this can't be done until dwc3-core
actually exist, which it won't until dwc3_probe() has completed
successfully (or in particular allocated struct dwc).

> Why do you think we need to retry the parent's probe again?

There's four options here:

0) Hope that of_platform_populate() always succeeds in invoking
dwc3_probe() on the first attempt, so that it is available when
of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of
the other platform's drivers).

1) Ensure that the operations performed by dwc3_probe() happens
synchronously and return a failure to dwc3-qcom, which depending on how
dwc3_probe() failed can propagate that failure - i.e. either probe defer
or clean up its resources if the failure from dwc3-core is permanent.

2) Register the dwc3-core and then return from dwc3_qcom_probe() in some
half-initialized state and through some yet to be invented notification
mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has
finished. But I know of no such notification mechanism in place today
and we can just register a callback, because of 1).
Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is
done probing - which might never happen.

3) Make drvdata of all the platform drivers be some known struct that
dwc3-core can retrieve and dereference - containing the optional
callback to the role_switch callback.


We've tried the option 0) for a few years now. Option 2) is a variant of
what we have today, where we patch one problem up and hope that nothing
unexpected happens until things has fully probed. We're doing 3) in
various other places, but in my view it's abusing a void * and has to be
kept synchronized between all the possible parent drivers.

Left is 1), which will take some refactoring, but will leave the
interaction between the two drivers in a state that's very easy to
reason about.

> I know there are some downstream code which using this way, I would
> like to know the shortcoming for it.
> 

The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe()
"manually" and then returning -EPROBE_DEFER if the dwc3-core's resources
aren't yet available is that we're wasting some time tearing down the
dwc3-qcom state and then re-initialize it next time this is attempted.

But as this is the idiomatic way to deal with the problem of "resources
not yet ready" there are mitigation being put in place to reduce the
number of such attempts being made.

Regards,
Bjorn

> Peter
> 
> >	The point in time that dwc3-qcom could potentially parse
> >       such a deferral in the child device is late. It would also be weird
> >       and messy to try to roll back the parent's probe because of a child
> >       device deferral.
> > 
> >       I considered making some sort of worker in the parent to check for
> >       child device probe but, again this seemed like an atrocious hack so,
> >       I didn't even try to prototype that.
> > 
> >    b) One potential solution was using "__weak" linkage in a function
> >       provided by dwc3-drd that a wrapper such as dwc3-qcom could then
> >       over-ride.
> > 
> >       If a wrapper such as dwc3-qcom then implemented a function with
> >       regular linkage it would over-ride the __weak function and provide a
> >       method for the dwc3-drd code to call into dwc3-qcom when probing was
> >       complete, thus allowing registration of the notifier when the child
> >       was ready.
> > 
> >       This would work up until the point that you tried to compile two
> >       implementations of a dwc3 wrapper into the one kernel module or the
> >       one kernel image say dwc3-qcom and a similar implementation in
> >       dwc3-meson. At that point you would get linker breakage.
> > 
> > #2 Using USB role switching for the notification
> > 
> >    Wesley's implementation took the approach dwc3-qcom -> dwc3-drd, whereas
> >    the approach I'm proposing here is dwc3-drd -> dwc3-qcom, which is also
> >    what we discussed on the list.
> > 
> >    Having implemented it, I think USB role-switching in the direction
> >    dwc3-drd -> dwc3-qcom is also a much cleaner solution for several
> >    reasons.
> > 
> >    a) Handling probe deferral is built into Linux' USB role switching today
> >       so we don't have to re-invent that wheel, unlike with the original
> >       notifier model.
> > 
> >    b) There is no "wiring up" or traversing the graph tree for the wrapper
> >       layer to determine if the parent device has a compliant type-c
> >       connector associated with it, unlike in the dwc3-qcom -> dwc3-drd
> >       model.
> > 
> >       All that has to happen is "usb-role-switch" is declared in the parent
> >       dwc3-qcom node and the role-switch API takes care of the rest.
> > 
> >       That means its possible to use a usb-c-connector, qcom type-c pm8150b
> >       driver, a USCI, a tps659x, a fusb302 or something like ChromeOS
> >       cros_ec to notify dwc3-drd without dwc3-qcom having to have
> >       the slighest clue which type of device is sending the signal.
> > 
> >       All dwc3-qcom needs to do is waggle UTMI signals in a register when a
> >       role-switch happens.
> > 
> >    c) It "feels" like a layering violation to have the dwc3-qcom SoC
> >       wrapper receive the event and trigger the dwc3-drd core.
> > 
> >       The standard model of parent/child role switching or remote-endpoint
> >       traversal that USB role switching already has works just fine for
> >       dwc3-drd, we just need to trigger dwc3-qcom for the role-switch in a
> >       non-vendor and non-SoC specific way.
> > 
> >    d) Less code. It turns out there's less code implementing as a
> >       role-switch interface in the direction dwc3-drd -> dwc3-qcom.
> > 
> >    e) Portability. The mechanism used for dwc3-drd -> dwc3 qcom can be
> >       reused for any other similar wrapper which models the wrapper as a
> >       parent of the dwc3-drd.
> > 
> > For all of those reasons I've opted to use USB role-switch notification
> > from dwc3-drd to dwc3-qcom.
> > 
> > git add bod git://git.linaro.org/people/bryan.odonoghue/kernel.git
> > git fetch bod
> > git diff usb-next-5.13.rcx-rb5-tcpm..usb-next-5.13.rcx-rb5-tcpm-v2
> > 
> > Bryan O'Donoghue (2):
> >   usb: dwc3: Add role switch relay support
> >   usb: dwc3: dwc3-qcom: Make dwc3-qcom a role-switch signal recipient
> > 
> > Wesley Cheng (1):
> >   usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API
> > 
> >  drivers/usb/dwc3/core.h      |  2 +
> >  drivers/usb/dwc3/drd.c       | 10 +++++
> >  drivers/usb/dwc3/dwc3-qcom.c | 77 ++++++++++++++++++++++++++++++++++--
> >  3 files changed, 85 insertions(+), 4 deletions(-)
> > 
> > -- 
> > 2.30.1
> > 
> 
> -- 
> 
> Thanks,
> Peter Chen
> 

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

[Peter Chen]

On 21-07-07 14:03:19, Bjorn Andersson wrote:
> On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> 
> Allow me to reorder your two questions:
> 
> > And why using a notifier need to concern core's deferral probe?
> 
> The problem at hand calls for the core for somehow invoking
> dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> 
> This means that dwc3-qcom somehow needs to inform the dwc3-core about
> this (and stash the pointer). And this can't be done until dwc3-core
> actually exist, which it won't until dwc3_probe() has completed
> successfully (or in particular allocated struct dwc).

Maybe you misunderstood the notifier I meant previous, my pointer was
calling glue layer API directly.

Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
allocated successfully, you could call glue layer notifier at function
dwc3_usb_role_switch_set directly.
Some references of my idea [1] [2]

[1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
[2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205

> 
> > Why do you think we need to retry the parent's probe again?
> 
> There's four options here:
> 
> 0) Hope that of_platform_populate() always succeeds in invoking
> dwc3_probe() on the first attempt, so that it is available when
> of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of
> the other platform's drivers).
> 
> 1) Ensure that the operations performed by dwc3_probe() happens
> synchronously and return a failure to dwc3-qcom, which depending on how
> dwc3_probe() failed can propagate that failure - i.e. either probe defer
> or clean up its resources if the failure from dwc3-core is permanent.
> 
> 2) Register the dwc3-core and then return from dwc3_qcom_probe() in some
> half-initialized state and through some yet to be invented notification
> mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has
> finished. But I know of no such notification mechanism in place today
> and we can just register a callback, because of 1).
> Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is
> done probing - which might never happen.
> 
> 3) Make drvdata of all the platform drivers be some known struct that
> dwc3-core can retrieve and dereference - containing the optional
> callback to the role_switch callback.
> 
> 
> We've tried the option 0) for a few years now. Option 2) is a variant of
> what we have today, where we patch one problem up and hope that nothing
> unexpected happens until things has fully probed. We're doing 3) in
> various other places, but in my view it's abusing a void * and has to be
> kept synchronized between all the possible parent drivers.
> 
> Left is 1), which will take some refactoring, but will leave the
> interaction between the two drivers in a state that's very easy to
> reason about.

Function of_find_device_by_node() invoked at glue layer is usually successfully,
The dwc3_probe failure doesn't affect it, unless you enable auto-suspend,
and glue layer's runtime suspend routine depends on dwc3 core's runtime suspend
routine. Would you please describe more about dwc3-core probe failure causes
dwc3-qcom's probe has failed or in half-initialized state you said?

> 
> > I know there are some downstream code which using this way, I would
> > like to know the shortcoming for it.
> > 
> 
> The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe()
> "manually" and then returning -EPROBE_DEFER if the dwc3-core's resources
> aren't yet available is that we're wasting some time tearing down the
> dwc3-qcom state and then re-initialize it next time this is attempted.

Like above, would you explain more about it?

-- 

Thanks,
Peter Chen

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

[Bjorn Andersson]

On Wed 07 Jul 22:06 CDT 2021, Peter Chen wrote:

> On 21-07-07 14:03:19, Bjorn Andersson wrote:
> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> > 
> > Allow me to reorder your two questions:
> > 
> > > And why using a notifier need to concern core's deferral probe?
> > 
> > The problem at hand calls for the core for somehow invoking
> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> > 
> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
> > this (and stash the pointer). And this can't be done until dwc3-core
> > actually exist, which it won't until dwc3_probe() has completed
> > successfully (or in particular allocated struct dwc).
> 
> Maybe you misunderstood the notifier I meant previous, my pointer was
> calling glue layer API directly.
> 
> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
> allocated successfully, you could call glue layer notifier at function
> dwc3_usb_role_switch_set directly.
> Some references of my idea [1] [2]
> 

It's probably 5+ years since I ran into something using platform_data,
had totally forgotten about it.

Defining a dwc3_platdata to allow the glue drivers to pass a function
pointer (and Wesley's bool) to the core driver sounds like a possible
way out of this.

> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
> 
> > 
> > > Why do you think we need to retry the parent's probe again?
> > 
> > There's four options here:
> > 
> > 0) Hope that of_platform_populate() always succeeds in invoking
> > dwc3_probe() on the first attempt, so that it is available when
> > of_find_device_by_node() is invoked in dwc3_qcom_probe() (and a few of
> > the other platform's drivers).
> > 
> > 1) Ensure that the operations performed by dwc3_probe() happens
> > synchronously and return a failure to dwc3-qcom, which depending on how
> > dwc3_probe() failed can propagate that failure - i.e. either probe defer
> > or clean up its resources if the failure from dwc3-core is permanent.
> > 
> > 2) Register the dwc3-core and then return from dwc3_qcom_probe() in some
> > half-initialized state and through some yet to be invented notification
> > mechanism continue the tail of dwc3_qcom_probe() once dwc3_probe() has
> > finished. But I know of no such notification mechanism in place today
> > and we can just register a callback, because of 1).
> > Furthermore we'd leave dwc3-qcom half-initialized until the dwc3-core is
> > done probing - which might never happen.
> > 
> > 3) Make drvdata of all the platform drivers be some known struct that
> > dwc3-core can retrieve and dereference - containing the optional
> > callback to the role_switch callback.
> > 
> > 
> > We've tried the option 0) for a few years now. Option 2) is a variant of
> > what we have today, where we patch one problem up and hope that nothing
> > unexpected happens until things has fully probed. We're doing 3) in
> > various other places, but in my view it's abusing a void * and has to be
> > kept synchronized between all the possible parent drivers.
> > 
> > Left is 1), which will take some refactoring, but will leave the
> > interaction between the two drivers in a state that's very easy to
> > reason about.
> 
> Function of_find_device_by_node() invoked at glue layer is usually successfully,

Went spelunking in drivers/base again, and I think you're right.

of_find_device_by_node() looks for devices on the platform_bus's klist
of devices, so if of_platform_populate() ends up successfully getting
through device_add() the we will find something. It might not have
probed yet, but as long as we don't rely on that we should be good...

> The dwc3_probe failure doesn't affect it, unless you enable auto-suspend,
> and glue layer's runtime suspend routine depends on dwc3 core's runtime suspend
> routine.

Right, if we hit qcom_dwc3_resume_irq() before the core driver has
probed it certainly looks like we're going to hit a NULL pointer.

> Would you please describe more about dwc3-core probe failure causes
> dwc3-qcom's probe has failed or in half-initialized state you said?
> 

Bryan had a previous patch where the glue layer was notified about role
switching (iirc) and as soon as we hit a probe deferal in the core
driver we'd dereference some pointer in the glue layer. I don't find the
patch right now, but I suspect it might have been caused by the same
platform_get_drvdata() as we see in qcom_dwc3_resume_irq().

> > 
> > > I know there are some downstream code which using this way, I would
> > > like to know the shortcoming for it.
> > > 
> > 
> > The shortcoming of having dwc3_qcom_probe() invoke dwc3_probe()
> > "manually" and then returning -EPROBE_DEFER if the dwc3-core's resources
> > aren't yet available is that we're wasting some time tearing down the
> > dwc3-qcom state and then re-initialize it next time this is attempted.
> 
> Like above, would you explain more about it?
> 

I could, but I guess if we use platform_data to pass the callbacks to
the core it doesn't matter if the core driver probes synchronously or in
a week (except if the glue hits qcom_dwc3_resume_irq(), but if that can
happen it can be fixed separately)...

Regards,
Bjorn

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

[Bryan O'Donoghue]

On 08/07/2021 04:54, Bjorn Andersson wrote:
> Bryan had a previous patch where the glue layer was notified about role
> switching (iirc) and as soon as we hit a probe deferal in the core
> driver we'd dereference some pointer in the glue layer. I don't find the
> patch right now, but I suspect it might have been caused by the same
> platform_get_drvdata() as we see in qcom_dwc3_resume_irq().

Here

https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/

and here

https://lore.kernel.org/linux-usb/20200311191501.8165-8-bryan.odonoghue@linaro.org/

one thing about that I don't think is right now in retrospect is having 
to find a DT connector in the core, meaning it incorrectly assumes you 
have a node named "connector" as a child of dwc3-core

https://lore.kernel.org/linux-usb/158463604559.152100.9219030962819234620@swboyd.mtv.corp.google.com/

Having done some work with TCPM on pm8150b silicon I see what Stephen 
was saying about that

That's one solid reason I like the USB role-switch API - because it gets 
you out of the business of trying to discern from dwc3-qcom if dwc3-core 
has role-switching turned on by iterating through its range of DT nodes 
and looking for a special one named "connector"

The initial and imperfect solution I had for that looked like this

if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {}

Wesley had another iteration on that that was a little better

https://lore.kernel.org/linux-usb/20201009082843.28503-4-wcheng@codeaurora.org/

+static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode)
+{
+	if (fwnode && (!fwnode_property_match_string(fwnode, "compatible",
+						     "gpio-usb-b-connector") ||
+	    !fwnode_property_match_string(fwnode, "compatible",
+					  "usb-c-connector")))
+		return 1;
+
+	return 0;
+}

All we are really doing there is replicating functionality that the 
role-switch API already provides


---
bod

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

[Bjorn Andersson]

On Thu 08 Jul 03:17 PDT 2021, Bryan O'Donoghue wrote:

> On 08/07/2021 04:54, Bjorn Andersson wrote:
> > Bryan had a previous patch where the glue layer was notified about role
> > switching (iirc) and as soon as we hit a probe deferal in the core
> > driver we'd dereference some pointer in the glue layer. I don't find the
> > patch right now, but I suspect it might have been caused by the same
> > platform_get_drvdata() as we see in qcom_dwc3_resume_irq().
> 
> Here
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/
> 
> and here
> 
> https://lore.kernel.org/linux-usb/20200311191501.8165-8-bryan.odonoghue@linaro.org/
> 

Now that I dug through the code I remembered why this didn't work.

You do:
	dwc = platform_get_drvdata(qcom->dwc3);

In order to be able to register the callback in the notifier chain that
you added to struct dwc3, but while qcom->dwc3 is a valid struct
platform_device, it might not have probed yet and "dwc" becomes NULL,
which you then dereferenced in dwc3_role_switch_notifier_register().

So we need a mechanism that passes that callback/notifier that has a
life cycle matching that of the glue device.

> one thing about that I don't think is right now in retrospect is having to
> find a DT connector in the core, meaning it incorrectly assumes you have a
> node named "connector" as a child of dwc3-core
> 
> https://lore.kernel.org/linux-usb/158463604559.152100.9219030962819234620@swboyd.mtv.corp.google.com/
> 
> Having done some work with TCPM on pm8150b silicon I see what Stephen was
> saying about that
> 
> That's one solid reason I like the USB role-switch API - because it gets you
> out of the business of trying to discern from dwc3-qcom if dwc3-core has
> role-switching turned on by iterating through its range of DT nodes and
> looking for a special one named "connector"
> 
> The initial and imperfect solution I had for that looked like this
> 
> if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {}
> 
> Wesley had another iteration on that that was a little better
> 
> https://lore.kernel.org/linux-usb/20201009082843.28503-4-wcheng@codeaurora.org/
> 
> +static int dwc3_qcom_connector_check(struct fwnode_handle *fwnode)
> +{
> +	if (fwnode && (!fwnode_property_match_string(fwnode, "compatible",
> +						     "gpio-usb-b-connector") ||
> +	    !fwnode_property_match_string(fwnode, "compatible",
> +					  "usb-c-connector")))
> +		return 1;
> +
> +	return 0;
> +}
> 
> All we are really doing there is replicating functionality that the
> role-switch API already provides
> 

But isn't this role switching interaction (both B and C) already part of
the core/drd, so if we can just get the drd to invoke
dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the
extcon code from the qcom glue as well)?

Regards,
Bjorn

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

[Bryan O'Donoghue]

On 25/08/2021 00:52, Bjorn Andersson wrote:
> But isn't this role switching interaction (both B and C) already part of
> the core/drd, so if we can just get the drd to invoke
> dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the
> extcon code from the qcom glue as well)?

Provided we have an acceptable way of triggering when a role-switch 
happens - then USB role switching itself is a NOP here, its really just 
a convenience to invoke the callback.

---
bod

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

[Bjorn Andersson]

On Tue 24 Aug 16:58 PDT 2021, Bryan O'Donoghue wrote:

> On 25/08/2021 00:52, Bjorn Andersson wrote:
> > But isn't this role switching interaction (both B and C) already part of
> > the core/drd, so if we can just get the drd to invoke
> > dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the
> > extcon code from the qcom glue as well)?
> 
> Provided we have an acceptable way of triggering when a role-switch happens
> - then USB role switching itself is a NOP here, its really just a
> convenience to invoke the callback.
> 

Thanks for confirming. Then let's come up with an acceptable way, rather
than duplicating yet another feature already implemented in the core.

Regards,
Bjorn

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

[Bryan O'Donoghue]

On 25/08/2021 01:01, Bjorn Andersson wrote:
> On Tue 24 Aug 16:58 PDT 2021, Bryan O'Donoghue wrote:
> 
>> On 25/08/2021 00:52, Bjorn Andersson wrote:
>>> But isn't this role switching interaction (both B and C) already part of
>>> the core/drd, so if we can just get the drd to invoke
>>> dwc3_qcom_vbus_overrride_enable() we're done (and can remove all the
>>> extcon code from the qcom glue as well)?
>>
>> Provided we have an acceptable way of triggering when a role-switch happens
>> - then USB role switching itself is a NOP here, its really just a
>> convenience to invoke the callback.
>>
> 
> Thanks for confirming. Then let's come up with an acceptable way, rather
> than duplicating yet another feature already implemented in the core.
> 
> Regards,
> Bjorn
> 

The only other thing to say about USB role-switching is it appears to be 
very much 1:1.

Extcon allows a virtually unlimited number of consumers of an even.

Is it envisaged that a role-switch could or should be consumed by say 3 
or even 4 separate pieces of logic and if not, why not ?

What if we had a magic black box that needed to sit ontop of the QCOM 
layer and further consume a role switch ?

notifier/platform pointer + callback ?

---
bod

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

[Bjorn Andersson]

On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:

> On 21-07-07 14:03:19, Bjorn Andersson wrote:
> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> > 
> > Allow me to reorder your two questions:
> > 
> > > And why using a notifier need to concern core's deferral probe?
> > 
> > The problem at hand calls for the core for somehow invoking
> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> > 
> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
> > this (and stash the pointer). And this can't be done until dwc3-core
> > actually exist, which it won't until dwc3_probe() has completed
> > successfully (or in particular allocated struct dwc).
> 
> Maybe you misunderstood the notifier I meant previous, my pointer was
> calling glue layer API directly.
> 
> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
> allocated successfully, you could call glue layer notifier at function
> dwc3_usb_role_switch_set directly.
> Some references of my idea [1] [2]
> 
> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
> 

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.

This looks quite promising and I think we should be able to get rid of
some duplicated extcon code from the Qualcomm glue as well...


I reworked the existing code to replace of_platform_populate() with
of_platform_device_create_pdata() to register the core device and pass a
struct with a function pointer and this works fine.

But then I searched the list archives and found this:
https://lore.kernel.org/lkml/CAL_JsqJ5gsctd7L3VOhTO1JdUqmMmSJRpos1XQyfxzmGO7wauw@mail.gmail.com/

In other words, per Rob's NACK this seems like a dead end.


@Rob, for your understanding, the dwc3 platform glue is implemented in a
set of platform_drivers, registering the core as a separate
platform_device and we need to make a function call from the core dwc3
code into the glue code.

My initial proposal was that we provide some helper function that the
glue code would use to allocate and register the core device, along
which we can pass such callback information.
But this turns out to pretty much be a re-implementation of
of_platform_device_create_pdata().  Perhaps that's still preferable?

Regards,
Bjorn

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

[Felipe Balbi]

Hi,

Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
>
>> On 21-07-07 14:03:19, Bjorn Andersson wrote:
>> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
>> > 
>> > Allow me to reorder your two questions:
>> > 
>> > > And why using a notifier need to concern core's deferral probe?
>> > 
>> > The problem at hand calls for the core for somehow invoking
>> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
>> > 
>> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
>> > this (and stash the pointer). And this can't be done until dwc3-core
>> > actually exist, which it won't until dwc3_probe() has completed
>> > successfully (or in particular allocated struct dwc).
>> 
>> Maybe you misunderstood the notifier I meant previous, my pointer was
>> calling glue layer API directly.
>> 
>> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
>> allocated successfully, you could call glue layer notifier at function
>> dwc3_usb_role_switch_set directly.
>> Some references of my idea [1] [2]
>> 
>> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
>> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
>> 
>
> 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.

-- 
balbi

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

[Bryan O'Donoghue]

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

# 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.

# 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.

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

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.

---
bod

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

[Bjorn Andersson]

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

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

[Heikki Krogerus]

On Wed, Aug 25, 2021 at 08:53:29AM -0700, Bjorn Andersson wrote:
> 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

Notifiers were proposed for the USB role switches already some time
ago [1], and I don't think anybody was against them, but in the end I
don't think there were any users for those notifier, so they were
never added.

If something needs to only react to the role changes like I think in
this case, then I would just add those notifiers to the USB role
switches.

[1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/

> 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.

There was an idea to add bind and unbind callbacks to the software
nodes (callbacks that would be called when a device is bind to the
node) at one point in order to solve this kind of problems.

In this case it would work so that you would supply a software node
for the role switch in your glue driver (that part is not a problem),
and then if the bind of that software node is called, you know the
role switch was registered, and you can acquire the handle to it
safely and start listening notifications from it.

But I don't know if that's a very sophisticated solution.

thanks,

-- 
heikki

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

[Bjorn Andersson]

On Wed 25 Aug 09:43 PDT 2021, Heikki Krogerus wrote:

> On Wed, Aug 25, 2021 at 08:53:29AM -0700, Bjorn Andersson wrote:
> > 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
> 
> Notifiers were proposed for the USB role switches already some time
> ago [1], and I don't think anybody was against them, but in the end I
> don't think there were any users for those notifier, so they were
> never added.
> 
> If something needs to only react to the role changes like I think in
> this case, then I would just add those notifiers to the USB role
> switches.
> 
> [1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/
> 

Afaict this would end up pretty much identical to the notification chain
that Bryan proposed earlier; the dwc3 drd code registers a
usb_role_switch and the glue code somehow needs to get hold of that
resource to register the notification.

But the glue code has no way to know when the core/drd code is done
registering, so it has no way to know when there is a notification chain
to register with.

Regards,
Bjorn

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

[Bryan O'Donoghue]

On 25/08/2021 16:53, Bjorn Andersson wrote:
> 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.

Its a very similar problem to orientation switch

As an example

- redriver may need to fix up signal integrity for
   lane switching

- PHY needs to toggle lanes from one IP block to another

I don't think off the top of my head a USB controller or DPU cares much 
about the orientation switch but for argument sake you could add one to 
that list.

I _think_ the type-c mux layer handles this though, as in what we did on 
RB5 has PHY and redriver receiving and reacting to a type-c orientation 
switch both with the existing type-c port driver and the new tcpm.

+ Dmitry - he did the mux work on the PHY and the redriver

Seems to me that the type-c mux way of diseminating to more than one 
place might fight role-switching well too.

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

[Bjorn Andersson]

On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote:

> On 25/08/2021 16:53, Bjorn Andersson wrote:
> > 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.
> 
> Its a very similar problem to orientation switch
> 
> As an example
> 
> - redriver may need to fix up signal integrity for
>   lane switching
> 
> - PHY needs to toggle lanes from one IP block to another
> 

Right, conceptually the problem is similar, but IMHO there's a big
difference in that the redriver and PHY are two physically separate
entities - on different buses. The dwc3 glue and core represent the same
piece of hardware.

> I don't think off the top of my head a USB controller or DPU cares much
> about the orientation switch but for argument sake you could add one to that
> list.
> 

Right, downstream the DPU driver is involved in the orientation
switching in the PHY, but upstream this moved into the PHY driver.

> I _think_ the type-c mux layer handles this though, as in what we did on RB5
> has PHY and redriver receiving and reacting to a type-c orientation switch
> both with the existing type-c port driver and the new tcpm.
> 
> + Dmitry - he did the mux work on the PHY and the redriver
> 
> Seems to me that the type-c mux way of diseminating to more than one place
> might fight role-switching well too.
> 

Both works by you the controller using the of_graph to acquire the
handle to _the_ consumer. I'm not aware of any support that would allow
us to signal two separate entities about the mux, orientation or role.

But as I said, for the orientation (at least) we do need to signal two
separate pieces of hardware (and drivers) about the change. Perhaps the
notifier mechanism that Heikki linked to earlier would be sufficient
though (I don't see a problem with probe deferring the redriver until
the type-c controller is registered).

But I don't like the idea of duplicating the rather clumsy of_graph
definition on both the glue node and the core node in DT. Similar to how
we previously had to do extcon in both nodes, and we kept getting that
wrong.

Regards,
Bjorn

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

[Felipe Balbi]

Hi,

Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote:
>
>> On 25/08/2021 16:53, Bjorn Andersson wrote:
>> > 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.
>> 
>> Its a very similar problem to orientation switch
>> 
>> As an example
>> 
>> - redriver may need to fix up signal integrity for
>>   lane switching
>> 
>> - PHY needs to toggle lanes from one IP block to another
>> 
>
> Right, conceptually the problem is similar, but IMHO there's a big
> difference in that the redriver and PHY are two physically separate
> entities - on different buses. The dwc3 glue and core represent the same
> piece of hardware.

no they don't. The glue is a real piece of HW that adapts the "generic"
synopsys IP to a given SoC. OMAP, for example, was adapting Synopsys'
proprietary interface to the Sonics interconnect, while some others may
adapt it to AXI or PCI or whatever.

They are different HW blocks, the glue (in many cases) has its own IRQ,
its own address space, its own register file, and so on. Granted, the
glue also serves as an access port from CPU to the synopsys core, but
that's handled by `ranges' DT property.

-- 
balbi

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

[Bjorn Andersson]

On Thu 26 Aug 01:15 CDT 2021, Felipe Balbi wrote:

> 
> Hi,
> 
> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> > On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote:
> >
> >> On 25/08/2021 16:53, Bjorn Andersson wrote:
> >> > 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.
> >> 
> >> Its a very similar problem to orientation switch
> >> 
> >> As an example
> >> 
> >> - redriver may need to fix up signal integrity for
> >>   lane switching
> >> 
> >> - PHY needs to toggle lanes from one IP block to another
> >> 
> >
> > Right, conceptually the problem is similar, but IMHO there's a big
> > difference in that the redriver and PHY are two physically separate
> > entities - on different buses. The dwc3 glue and core represent the same
> > piece of hardware.
> 
> no they don't. The glue is a real piece of HW that adapts the "generic"
> synopsys IP to a given SoC. OMAP, for example, was adapting Synopsys'
> proprietary interface to the Sonics interconnect, while some others may
> adapt it to AXI or PCI or whatever.
> 
> They are different HW blocks, the glue (in many cases) has its own IRQ,
> its own address space, its own register file, and so on. Granted, the
> glue also serves as an access port from CPU to the synopsys core, but
> that's handled by `ranges' DT property.
> 

So what you're saying is that the "Qualcomm glue" is the hardware, and
the core is just the basis for that design?

Regardless, on the Qualcomm platforms we have need to inform both
devices about role changes, how do we do this? (Without platform_data
and preferably without having to duplicate the typec code in the glue
and core and the two device nodes in DT)

Regards,
Bjorn

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

[Heikki Krogerus]

[-- Attachment #1: Type: text/plain, Size: 2886 bytes --]

On Wed, Sep 15, 2021 at 08:53:35AM -0500, Bjorn Andersson wrote:
> On Thu 26 Aug 01:15 CDT 2021, Felipe Balbi wrote:
> 
> > 
> > Hi,
> > 
> > Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> > > On Wed 25 Aug 10:59 PDT 2021, Bryan O'Donoghue wrote:
> > >
> > >> On 25/08/2021 16:53, Bjorn Andersson wrote:
> > >> > 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.
> > >> 
> > >> Its a very similar problem to orientation switch
> > >> 
> > >> As an example
> > >> 
> > >> - redriver may need to fix up signal integrity for
> > >>   lane switching
> > >> 
> > >> - PHY needs to toggle lanes from one IP block to another
> > >> 
> > >
> > > Right, conceptually the problem is similar, but IMHO there's a big
> > > difference in that the redriver and PHY are two physically separate
> > > entities - on different buses. The dwc3 glue and core represent the same
> > > piece of hardware.
> > 
> > no they don't. The glue is a real piece of HW that adapts the "generic"
> > synopsys IP to a given SoC. OMAP, for example, was adapting Synopsys'
> > proprietary interface to the Sonics interconnect, while some others may
> > adapt it to AXI or PCI or whatever.
> > 
> > They are different HW blocks, the glue (in many cases) has its own IRQ,
> > its own address space, its own register file, and so on. Granted, the
> > glue also serves as an access port from CPU to the synopsys core, but
> > that's handled by `ranges' DT property.
> > 
> 
> So what you're saying is that the "Qualcomm glue" is the hardware, and
> the core is just the basis for that design?
> 
> Regardless, on the Qualcomm platforms we have need to inform both
> devices about role changes, how do we do this? (Without platform_data
> and preferably without having to duplicate the typec code in the glue
> and core and the two device nodes in DT)

That part can be handled by simply adding the notifiers to the USB
role switch class as said before [1], so I think the actual problem
here is that your glue layer depends on core that your glue creates
(or a resource that the core driver creates).

I think the dependency on dwc3 core, and what ever it creates, issue
you should be able to solve by using the component framework. I'll
attach you a patch showing how it should probable look like (not even
compile tested).

So the dwc3 core is the aggregate driver and the glue is the only
component in that example (that should be enough for your needs), but
it should not be any problem to later register also the child devices
(xHCI, USB role switch, etc.) as components if needed.

[1] https://lore.kernel.org/linux-usb/20191002231617.3670-3-john.stultz@linaro.org/

thanks,

-- 
heikki

[-- Attachment #2: dwc3.diff --]
[-- Type: text/plain, Size: 4550 bytes --]

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 0104a80b185e1..7cc49611af74f 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -26,6 +26,7 @@
 #include <linux/acpi.h>
 #include <linux/pinctrl/consumer.h>
 #include <linux/reset.h>
+#include <linux/component.h>
 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
@@ -1521,11 +1522,32 @@ static void dwc3_check_params(struct dwc3 *dwc)
 	}
 }
 
+static int dwc3_aggregate_bind(struct device *dev)
+{
+	return component_bind_all(dev, NULL);
+}
+
+static void dwc3_aggregate_unbind(struct device *dev)
+{
+	component_unbind_all(dev, NULL);
+}
+
+static const struct component_master_ops dwc3_aggregate_ops = {
+	.bind = dwc3_aggregate_bind,
+	.unbind = dwc3_aggregate_unbind,
+};
+
+static int dwc3_compare(struct device *dev, void *data)
+{
+	return dev == data;
+}
+
 static int dwc3_probe(struct platform_device *pdev)
 {
 	struct device		*dev = &pdev->dev;
 	struct resource		*res, dwc_res;
 	struct dwc3		*dwc;
+	struct component_match	*match = NULL;
 
 	int			ret;
 
@@ -1646,10 +1668,21 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (ret)
 		goto err5;
 
+	/* Add component match entry for the glue. */
+	component_match_add(dwc->dev, &match, dwc3_compare, dwc->dev->parent);
+
+	/* Everything is ready now. Declare this driver as the aggregate. */
+	ret = component_master_add_with_match(dwc->dev, &dwc3_aggregate_ops, match);
+	if (ret)
+		goto err6;
+
 	pm_runtime_put(dev);
 
 	return 0;
 
+err6:
+	dwc3_core_exit_mode(dwc);
+
 err5:
 	dwc3_debugfs_exit(dwc);
 	dwc3_event_buffers_cleanup(dwc);
@@ -1696,6 +1729,8 @@ static int dwc3_remove(struct platform_device *pdev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
+	component_master_del(dwc->dev, &dwc3_aggregate_ops);
+
 	dwc3_core_exit_mode(dwc);
 	dwc3_debugfs_exit(dwc);
 
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 9abbd01028c5f..ffe585344d6a8 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -20,6 +20,8 @@
 #include <linux/usb/of.h>
 #include <linux/reset.h>
 #include <linux/iopoll.h>
+#include <linux/usb/role.h>
+#include <linux/component.h>
 
 #include "core.h"
 
@@ -81,6 +83,7 @@ struct dwc3_qcom {
 	struct extcon_dev	*host_edev;
 	struct notifier_block	vbus_nb;
 	struct notifier_block	host_nb;
+	struct notifier_block	role_nb;
 
 	const struct dwc3_acpi_pdata *acpi_pdata;
 
@@ -717,6 +720,51 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 	return acpi_create_platform_device(adev, NULL);
 }
 
+static int dwc3_qcom_role_notifier(struct notifier_block *nb,
+				   unsigned long event, void *ptr)
+{
+	struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, role_nb);
+
+	/* Do what ever you need to do... */
+
+	return NOTIFY_DONE;
+}
+
+static int dwc3_qcom_bind(struct device *dev, struct device *dwc, void *data)
+{
+	struct usb_role_switch *sw = usb_role_switch_find_by_fwnode(dev_fwnode(dwc));
+	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+	/*
+	 * Time to finalize initilization.
+	 *
+	 * Our aggregate driver - dwc3 core - is guaranteed to be ready when
+	 * this is called. That means USB role switch "sw" is also now ready.
+	 */
+
+	/* Register role switch notifier */
+	qcom->role_nb.notifier_call = dwc3_qcom_role_notifier;
+	usb_role_switch_register_notifier(sw, qcom->role_nb);
+	usb_role_switch_put(sw);
+
+	return 0;
+}
+
+static void dwc3_qcom_unbind(struct device *dev, struct device *dwc, void *data)
+{
+	struct usb_role_switch *sw = usb_role_switch_find_by_fwnode(dev_fwnode(dwc));
+	struct dwc3_qcom *qcom = dev_get_drvdata(dev);
+
+	/* Unregister role switch notifier */
+	usb_role_switch_unregister_notifier(sw, qcom->role_nb);
+	usb_role_switch_put(sw);
+}
+
+static const struct component_ops dwc3_qcom_component_ops = {
+	.bind = dwc3_qcom_bind,
+	.unbind = dwc3_qcom_unbind,
+};
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node;
@@ -837,6 +885,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (ret)
 		goto interconnect_exit;
 
+	ret = component_add(dev, &dwc3_qcom_component_ops);
+	if (ret)
+		goto interconnect_exit;
+
 	device_init_wakeup(&pdev->dev, 1);
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
@@ -869,6 +921,7 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	component_del(dev, &dwc3_qcom_component_ops);
 	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 

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

[Dmitry Baryshkov]

Hi,

On Wed, 25 Aug 2021 at 20:57, Bryan O'Donoghue
<bryan.odonoghue@linaro.org> wrote:
>
> On 25/08/2021 16:53, Bjorn Andersson wrote:
> > 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.
>
> Its a very similar problem to orientation switch
>
> As an example
>
> - redriver may need to fix up signal integrity for
>    lane switching
>
> - PHY needs to toggle lanes from one IP block to another
>
> I don't think off the top of my head a USB controller or DPU cares much
> about the orientation switch but for argument sake you could add one to
> that list.
>
> I _think_ the type-c mux layer handles this though, as in what we did on
> RB5 has PHY and redriver receiving and reacting to a type-c orientation
> switch both with the existing type-c port driver and the new tcpm.
>
> + Dmitry - he did the mux work on the PHY and the redriver

For the RB5 case I ended up with the redriver acting as a client for
the type-c port orientation events, and then it would act as a source
for the event being sent to the DP PHY. This chained approach is far
from being ideal, but it allowed me to use the current framework
without applying significant changes. I've had some ideas on how to
improve the type-c framework, but I never had enough time to
materialize them.

> Seems to me that the type-c mux way of diseminating to more than one
> place might fight role-switching well too.

-- 
With best wishes
Dmitry

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

[Rob Herring]

On Wed, Aug 25, 2021 at 12:52 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
> >
> >> On 21-07-07 14:03:19, Bjorn Andersson wrote:
> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> >> >
> >> > Allow me to reorder your two questions:
> >> >
> >> > > And why using a notifier need to concern core's deferral probe?
> >> >
> >> > The problem at hand calls for the core for somehow invoking
> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> >> >
> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
> >> > this (and stash the pointer). And this can't be done until dwc3-core
> >> > actually exist, which it won't until dwc3_probe() has completed
> >> > successfully (or in particular allocated struct dwc).
> >>
> >> Maybe you misunderstood the notifier I meant previous, my pointer was
> >> calling glue layer API directly.
> >>
> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
> >> allocated successfully, you could call glue layer notifier at function
> >> dwc3_usb_role_switch_set directly.
> >> Some references of my idea [1] [2]
> >>
> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
> >>
> >
> > 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.

On the surface that seems nice, but obviously there are issues with
the approach. It's also not how pretty much every other instance of
licensed IP blocks are structured.

The specific need here seems to be multiple entities needing role
switch notifications. Seems like that should be solved in a generic
way.

Rob

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

[Felipe Balbi]

Hi,

Rob Herring <robh+dt@kernel.org> writes:
>> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
>> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
>> >
>> >> On 21-07-07 14:03:19, Bjorn Andersson wrote:
>> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
>> >> >
>> >> > Allow me to reorder your two questions:
>> >> >
>> >> > > And why using a notifier need to concern core's deferral probe?
>> >> >
>> >> > The problem at hand calls for the core for somehow invoking
>> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
>> >> >
>> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
>> >> > this (and stash the pointer). And this can't be done until dwc3-core
>> >> > actually exist, which it won't until dwc3_probe() has completed
>> >> > successfully (or in particular allocated struct dwc).
>> >>
>> >> Maybe you misunderstood the notifier I meant previous, my pointer was
>> >> calling glue layer API directly.
>> >>
>> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
>> >> allocated successfully, you could call glue layer notifier at function
>> >> dwc3_usb_role_switch_set directly.
>> >> Some references of my idea [1] [2]
>> >>
>> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
>> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
>> >>
>> >
>> > 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.
>
> On the surface that seems nice, but obviously there are issues with
> the approach. It's also not how pretty much every other instance of
> licensed IP blocks are structured.
>
> The specific need here seems to be multiple entities needing role
> switch notifications. Seems like that should be solved in a generic
> way.

right, solve it generically without breaking the module isolation ;-)

-- 
balbi

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

[Bjorn Andersson]

On Tue 24 Aug 22:51 PDT 2021, Felipe Balbi wrote:

> 
> Hi,
> 
> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
> >
> >> On 21-07-07 14:03:19, Bjorn Andersson wrote:
> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> >> > 
> >> > Allow me to reorder your two questions:
> >> > 
> >> > > And why using a notifier need to concern core's deferral probe?
> >> > 
> >> > The problem at hand calls for the core for somehow invoking
> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> >> > 
> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
> >> > this (and stash the pointer). And this can't be done until dwc3-core
> >> > actually exist, which it won't until dwc3_probe() has completed
> >> > successfully (or in particular allocated struct dwc).
> >> 
> >> Maybe you misunderstood the notifier I meant previous, my pointer was
> >> calling glue layer API directly.
> >> 
> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
> >> allocated successfully, you could call glue layer notifier at function
> >> dwc3_usb_role_switch_set directly.
> >> Some references of my idea [1] [2]
> >> 
> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
> >> 
> >
> > 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.
> 

I'm okay with that goal, but the result is that both the OMAP and
Qualcomm driver duplicates the extcon interface already present in the
DRD, and the Meson driver duplicates the usb_role_switch. In addition to
the code duplication this manifest itself in the need for us to link
extcon to both the glue and core nodes in DeviceTree.

In order to function in a USB-C based setup we now need to register a 
usb_role_switch from the Qualcomm glue and we need to evolve the
usb_role_switch implementation to allow for the Type-C controller to
notify more than a single role-switcher.

So we're facing the need to introduce another bunch of duplication and
the DT will be quite ugly with both glue and core having to set up an
of_graph with the Type-C controller.


I really would like for us to come up with a way where the core can
notify the glue that role switching is occurring, so that we instead of
adding more duplication could aim to, over time, remove the extcon and
usb_role_switch logic from the Qualcomm, OMAP and Meson drivers.

Regards,
Bjorn

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

[Felipe Balbi]

Hi,

Bjorn Andersson <bjorn.andersson@linaro.org> writes:
>> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
>> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
>> >
>> >> On 21-07-07 14:03:19, Bjorn Andersson wrote:
>> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
>> >> > 
>> >> > Allow me to reorder your two questions:
>> >> > 
>> >> > > And why using a notifier need to concern core's deferral probe?
>> >> > 
>> >> > The problem at hand calls for the core for somehow invoking
>> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
>> >> > 
>> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
>> >> > this (and stash the pointer). And this can't be done until dwc3-core
>> >> > actually exist, which it won't until dwc3_probe() has completed
>> >> > successfully (or in particular allocated struct dwc).
>> >> 
>> >> Maybe you misunderstood the notifier I meant previous, my pointer was
>> >> calling glue layer API directly.
>> >> 
>> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
>> >> allocated successfully, you could call glue layer notifier at function
>> >> dwc3_usb_role_switch_set directly.
>> >> Some references of my idea [1] [2]
>> >> 
>> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
>> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
>> >> 
>> >
>> > 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.
>> 
>
> I'm okay with that goal, but the result is that both the OMAP and
> Qualcomm driver duplicates the extcon interface already present in the
> DRD, and the Meson driver duplicates the usb_role_switch. In addition to
> the code duplication this manifest itself in the need for us to link
> extcon to both the glue and core nodes in DeviceTree.
>
> In order to function in a USB-C based setup we now need to register a 
> usb_role_switch from the Qualcomm glue and we need to evolve the
> usb_role_switch implementation to allow for the Type-C controller to
> notify more than a single role-switcher.
>
> So we're facing the need to introduce another bunch of duplication and
> the DT will be quite ugly with both glue and core having to set up an
> of_graph with the Type-C controller.
>
>
> I really would like for us to come up with a way where the core can
> notify the glue that role switching is occurring, so that we instead of
> adding more duplication could aim to, over time, remove the extcon and
> usb_role_switch logic from the Qualcomm, OMAP and Meson drivers.

We can make a comparison between clk rate notifiers. Anyone can
subscribe to a clk rate notification and react to the notification. A
generic dual role notification system should allow for something
similar. I really don't get why folks want to treat a glue and core
driver differently in this case.

Why do we want to pass function pointers around instead of letting
whatever role notification mechanism to be able to notify more than one
user?

Also keep in mind that we have dwc3 implementations which are dual role
capable but don't ship the synopsys DRD block. Rather, there's a
peripheral-only dwc3 instance and a separate xhci with custom logic
handling role swap.

If we were to provide a dwc3-specific role swap function-pointer based
interface, we would just create yet another special case for this. A
better approach would be to start consolidating all of these different
role-swap mechanisms in a generic layer that "knows-it-all". If dwc3 is
generating the role notification or a separate type-c controller or even
some EC IRQ, that shouldn't matter for the listeners.

-- 
balbi

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

[Bjorn Andersson]

On Wed 25 Aug 08:22 PDT 2021, Felipe Balbi wrote:

> 
> Hi,
> 
> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> >> Bjorn Andersson <bjorn.andersson@linaro.org> writes:
> >> > On Wed 07 Jul 20:06 PDT 2021, Peter Chen wrote:
> >> >
> >> >> On 21-07-07 14:03:19, Bjorn Andersson wrote:
> >> >> > On Tue 06 Jul 20:57 CDT 2021, Peter Chen wrote:
> >> >> > 
> >> >> > Allow me to reorder your two questions:
> >> >> > 
> >> >> > > And why using a notifier need to concern core's deferral probe?
> >> >> > 
> >> >> > The problem at hand calls for the core for somehow invoking
> >> >> > dwc3_qcom_vbus_overrride_enable() with a pointer to dwc3_qcom passed.
> >> >> > 
> >> >> > This means that dwc3-qcom somehow needs to inform the dwc3-core about
> >> >> > this (and stash the pointer). And this can't be done until dwc3-core
> >> >> > actually exist, which it won't until dwc3_probe() has completed
> >> >> > successfully (or in particular allocated struct dwc).
> >> >> 
> >> >> Maybe you misunderstood the notifier I meant previous, my pointer was
> >> >> calling glue layer API directly.
> >> >> 
> >> >> Role switch is from dwc3-core, when it occurs, it means structure dwc3 has
> >> >> allocated successfully, you could call glue layer notifier at function
> >> >> dwc3_usb_role_switch_set directly.
> >> >> Some references of my idea [1] [2]
> >> >> 
> >> >> [1] Function ci_hdrc_msm_notify_event at ci_hdrc_msm_notify_event
> >> >> [2] https://source.codeaurora.org/external/imx/linux-imx/tree/drivers/usb/dwc3/core.c?h=lf-5.10.y#n205
> >> >> 
> >> >
> >> > 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.
> >> 
> >
> > I'm okay with that goal, but the result is that both the OMAP and
> > Qualcomm driver duplicates the extcon interface already present in the
> > DRD, and the Meson driver duplicates the usb_role_switch. In addition to
> > the code duplication this manifest itself in the need for us to link
> > extcon to both the glue and core nodes in DeviceTree.
> >
> > In order to function in a USB-C based setup we now need to register a 
> > usb_role_switch from the Qualcomm glue and we need to evolve the
> > usb_role_switch implementation to allow for the Type-C controller to
> > notify more than a single role-switcher.
> >
> > So we're facing the need to introduce another bunch of duplication and
> > the DT will be quite ugly with both glue and core having to set up an
> > of_graph with the Type-C controller.
> >
> >
> > I really would like for us to come up with a way where the core can
> > notify the glue that role switching is occurring, so that we instead of
> > adding more duplication could aim to, over time, remove the extcon and
> > usb_role_switch logic from the Qualcomm, OMAP and Meson drivers.
> 
> We can make a comparison between clk rate notifiers. Anyone can
> subscribe to a clk rate notification and react to the notification. A
> generic dual role notification system should allow for something
> similar. I really don't get why folks want to treat a glue and core
> driver differently in this case.
> 
> Why do we want to pass function pointers around instead of letting
> whatever role notification mechanism to be able to notify more than one
> user?
> 
> Also keep in mind that we have dwc3 implementations which are dual role
> capable but don't ship the synopsys DRD block. Rather, there's a
> peripheral-only dwc3 instance and a separate xhci with custom logic
> handling role swap.
> 

So you're suggesting that we invent a 3rd mechanism (in addition to the
already existing duplication between extcon and usb_role_switch) for
propagating role switching notifications through the kernel?

> If we were to provide a dwc3-specific role swap function-pointer based
> interface, we would just create yet another special case for this. A
> better approach would be to start consolidating all of these different
> role-swap mechanisms in a generic layer that "knows-it-all". If dwc3 is
> generating the role notification or a separate type-c controller or even
> some EC IRQ, that shouldn't matter for the listeners.
> 

I was under the impression that usb_role_switch is the attempt to
replace extcon as the one solution. The problem in the dwc3 case is that
the same piece of hardware (i.e. _the_ USB controller) needs to
implement and wired up as two separate consumers of that message.

I recognize the complexity caused by the flexibility in DWC3 based
designs, but I would like to see whatever combination be seen as a
single entity to the rest of the system - rather than building the
notification plumbing between the two pieces using DeviceTree.

Regards,
Bjorn