diff mbox series

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

Message ID 20210704013314.200951-3-bryan.odonoghue@linaro.org
State New
Headers show
Series Implement role-switch notifications from dwc3-drd to dwc3-qcom | expand

Commit Message

Bryan O'Donoghue July 4, 2021, 1:33 a.m. UTC
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(+)

Comments

Peter Chen July 6, 2021, 2:51 a.m. UTC | #1
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
>
Bryan O'Donoghue July 6, 2021, 10:07 a.m. UTC | #2
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
Bjorn Andersson July 7, 2021, 5:14 a.m. UTC | #3
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
>
Bryan O'Donoghue July 7, 2021, 9:49 a.m. UTC | #4
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
diff mbox series

Patch

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);