diff mbox series

[v4,09/18] usb: dwc3: qcom: Override VBUS when using gpio_usb_connector

Message ID 20200207015907.242991-10-bryan.odonoghue@linaro.org (mailing list archive)
State Superseded
Headers show
Series Enable Qualcomm QCS 404 HS/SS USB | expand

Commit Message

Bryan O'Donoghue Feb. 7, 2020, 1:58 a.m. UTC 
Using the gpio_usb_connector driver also means that we are not supplying
VBUS via the SoC but by an external PMIC directly.

This patch searches for a gpio_usb_connector as a child node of the core
DWC3 block and if found switches on the VBUS over-ride, leaving it up to
the role-switching code in gpio-usb-connector to switch off and on VBUS.

Cc: Andy Gross <agross@kernel.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

Comments

Jack Pham Feb. 7, 2020, 8:07 a.m. UTC | #1 
Hi Bryan,

On Fri, Feb 07, 2020 at 01:58:58AM +0000, Bryan O'Donoghue wrote:
> Using the gpio_usb_connector driver also means that we are not supplying
> VBUS via the SoC but by an external PMIC directly.
> 
> This patch searches for a gpio_usb_connector as a child node of the core
> DWC3 block and if found switches on the VBUS over-ride, leaving it up to
> the role-switching code in gpio-usb-connector to switch off and on VBUS.
 
<snip>

>  static int dwc3_qcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*np = pdev->dev.of_node;
> @@ -557,7 +572,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	struct dwc3_qcom	*qcom;
>  	struct resource		*res, *parent_res = NULL;
>  	int			ret, i;
> -	bool			ignore_pipe_clk;
> +	bool			ignore_pipe_clk, gpio_usb_conn;
>  
>  	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
>  	if (!qcom)
> @@ -649,9 +664,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	}
>  
>  	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
> +	gpio_usb_conn = dwc3_qcom_find_gpio_usb_connector(qcom->dwc3);
>  
> -	/* enable vbus override for device mode */
> -	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
> +	/* enable vbus override for device mode or GPIO USB connector mode */
> +	if (qcom->mode == USB_DR_MODE_PERIPHERAL || gpio_usb_conn)
>  		dwc3_qcom_vbus_overrride_enable(qcom, true);

This doesn't seem right. It looks like you are doing the vbus_override
only once on probe() and keeping it that way regardless of the dynamic
state of the connector, i.e. even after VBUS is physically removed
and/or ID pin is low.

>  	/* register extcon to override sw_vbus on Vbus change later */

As suggested by this comment, if you look at the extcon handling, it
intercepts the VBUS state toggling in dwc3_qcom_vbus_notifier() and
calls vbus_override() accordingly. That way it should only be true when
the role==USB_ROLE_DEVICE and disabled otherwise (USB_ROLE_HOST/NONE).

To me the gpio-b connector + usb-role-switch is attempting to be an
alternative to extcon. But to correctly mimic the vbus_override()
behavior I think we need a way to intercept when the connector child
driver calls usb_role_switch_set_role() to the dwc3 device, but somehow
be able to do it from up here in the parent/glue layer. Unfortunately I
don't have a good idea of how to do that, short of shoehorning an
"upcall" notification from drd.c to the glue, something I don't think
Felipe would be a fan of.

Could the usb_role_switch class somehow be enhanced to support chaining
multiple "consumers" to support this case? Such that when the gpio-b
driver calls set_role() it could get handled both by drd.c and
dwc3-qcom.c?

Jack
Bryan O'Donoghue Feb. 7, 2020, 10:36 a.m. UTC | #2 
On 07/02/2020 08:07, Jack Pham wrote:
> Hi Bryan,
> 
> On Fri, Feb 07, 2020 at 01:58:58AM +0000, Bryan O'Donoghue wrote:
>> Using the gpio_usb_connector driver also means that we are not supplying
>> VBUS via the SoC but by an external PMIC directly.
>>
>> This patch searches for a gpio_usb_connector as a child node of the core
>> DWC3 block and if found switches on the VBUS over-ride, leaving it up to
>> the role-switching code in gpio-usb-connector to switch off and on VBUS.
>   
> <snip>
> 
>>   static int dwc3_qcom_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node	*np = pdev->dev.of_node;
>> @@ -557,7 +572,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	struct dwc3_qcom	*qcom;
>>   	struct resource		*res, *parent_res = NULL;
>>   	int			ret, i;
>> -	bool			ignore_pipe_clk;
>> +	bool			ignore_pipe_clk, gpio_usb_conn;
>>   
>>   	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
>>   	if (!qcom)
>> @@ -649,9 +664,10 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>>   	}
>>   
>>   	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
>> +	gpio_usb_conn = dwc3_qcom_find_gpio_usb_connector(qcom->dwc3);
>>   
>> -	/* enable vbus override for device mode */
>> -	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
>> +	/* enable vbus override for device mode or GPIO USB connector mode */
>> +	if (qcom->mode == USB_DR_MODE_PERIPHERAL || gpio_usb_conn)
>>   		dwc3_qcom_vbus_overrride_enable(qcom, true);
> 
> This doesn't seem right. It looks like you are doing the vbus_override
> only once on probe() and keeping it that way regardless of the dynamic
> state of the connector, i.e. even after VBUS is physically removed
> and/or ID pin is low.
> 

Hmm, I don't see anything much in the documentation that flags why we 
want or need to toggle this.

>>   	/* register extcon to override sw_vbus on Vbus change later */
> 
> As suggested by this comment, if you look at the extcon handling, it
> intercepts the VBUS state toggling in dwc3_qcom_vbus_notifier() and
> calls vbus_override() accordingly. That way it should only be true when
> the role==USB_ROLE_DEVICE and disabled otherwise (USB_ROLE_HOST/NONE).
> 
> To me the gpio-b connector + usb-role-switch is attempting to be an
> alternative to extcon. But to correctly mimic the vbus_override()
> behavior I think we need a way to intercept when the connector child
> driver calls usb_role_switch_set_role() to the dwc3 device, but somehow
> be able to do it from up here in the parent/glue layer. Unfortunately I
> don't have a good idea of how to do that, short of shoehorning an
> "upcall" notification from drd.c to the glue, something I don't think
> Felipe would be a fan of.
> 
> Could the usb_role_switch class somehow be enhanced to support chaining
> multiple "consumers" to support this case? Such that when the gpio-b
> driver calls set_role() it could get handled both by drd.c and
> dwc3-qcom.c?

It is probably necessary eventually, but, per my reading of the 
documents and working with the hardware, I couldn't justify the 
additional work.

However if you think this patchset needs the toggle, I can look into 
getting the indicator to toggle here too.

We'd need to add some sort of linked list of notifiers to the role 
switching logic and toggle them in order.

Similar to what is done in extcon now for the various notifer hooks.

---
bod
Bryan O'Donoghue Feb. 7, 2020, 10:50 a.m. UTC | #3 
On 07/02/2020 10:36, Bryan O'Donoghue wrote:
> On 07/02/2020 08:07, Jack Pham wrote:
>> Could the usb_role_switch class somehow be enhanced to support chaining
>> multiple "consumers" to support this case? Such that when the gpio-b
>> driver calls set_role() it could get handled both by drd.c and
>> dwc3-qcom.c?
> 
> It is probably necessary eventually, but, per my reading of the 
> documents and working with the hardware, I couldn't justify the 
> additional work.
> 
> However if you think this patchset needs the toggle, I can look into 
> getting the indicator to toggle here too.
> 
> We'd need to add some sort of linked list of notifiers to the role 
> switching logic and toggle them in order.
> 
> Similar to what is done in extcon now for the various notifer hooks.

Maybe I'm wrong...

Looking a bit closer at the role-switch code it might be possible to 
register multiple devices _as-is_ so long as you have a pointer to the 
relevant parent...

---
bod
Bryan O'Donoghue Feb. 7, 2020, 3:04 p.m. UTC | #4 
On 07/02/2020 10:50, Bryan O'Donoghue wrote:
> 
> Maybe I'm wrong...
> 
> Looking a bit closer at the role-switch code it might be possible to 
> register multiple devices _as-is_ so long as you have a pointer to the 
> relevant parent...

Soo... its possible to create a new role-switch device relatively easily 
@usb_role_switch_register() but, the drivers calling the role-switch 
callback have a 1:1 mapping between role-switch call and receiver.

Doing something inside DWC3 <=> DWC3::QCOM looks like less of a rewrite.
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 261af9e38ddd..b2d20b474029 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -550,6 +550,21 @@  static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
 	.ss_phy_irq_index = 2
 };
 
+static bool dwc3_qcom_find_gpio_usb_connector(struct platform_device *pdev)
+{
+	struct device_node	*np;
+	bool			retval = false;
+
+	np = of_get_child_by_name(pdev->dev.of_node, "connector");
+	if (np) {
+		if (of_device_is_compatible(np, "gpio-usb-b-connector"))
+			retval = true;
+	}
+	of_node_put(np);
+
+	return retval;
+}
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node;
@@ -557,7 +572,7 @@  static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct dwc3_qcom	*qcom;
 	struct resource		*res, *parent_res = NULL;
 	int			ret, i;
-	bool			ignore_pipe_clk;
+	bool			ignore_pipe_clk, gpio_usb_conn;
 
 	qcom = devm_kzalloc(&pdev->dev, sizeof(*qcom), GFP_KERNEL);
 	if (!qcom)
@@ -649,9 +664,10 @@  static int dwc3_qcom_probe(struct platform_device *pdev)
 	}
 
 	qcom->mode = usb_get_dr_mode(&qcom->dwc3->dev);
+	gpio_usb_conn = dwc3_qcom_find_gpio_usb_connector(qcom->dwc3);
 
-	/* enable vbus override for device mode */
-	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
+	/* enable vbus override for device mode or GPIO USB connector mode */
+	if (qcom->mode == USB_DR_MODE_PERIPHERAL || gpio_usb_conn)
 		dwc3_qcom_vbus_overrride_enable(qcom, true);
 
 	/* register extcon to override sw_vbus on Vbus change later */