diff mbox series

[7/7] usb: dwc3: qcom: Enable gpio-usb-conn based role-switching

Message ID 20200311191501.8165-8-bryan.odonoghue@linaro.org (mailing list archive)
State New, archived
Headers show
Series DWC3/Qualcomm connector based role-switching | expand

Commit Message

Bryan O'Donoghue March 11, 2020, 7:15 p.m. UTC 
This patch adds the ability to receive a notification from the DRD code for
role-switch events and in doing so it introduces a disjunction between
gpio-usb-conn or extcon mode.

This is what we want to do, since the two methods are mutually exclusive.

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: Jack Pham <jackp@codeaurora.org>
Cc: linux-arm-msm@vger.kernel.org
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Bjorn Andersson March 17, 2020, 6:31 a.m. UTC | #1 
On Wed 11 Mar 12:15 PDT 2020, Bryan O'Donoghue wrote:

> This patch adds the ability to receive a notification from the DRD code for
> role-switch events and in doing so it introduces a disjunction between
> gpio-usb-conn or extcon mode.
> 
> This is what we want to do, since the two methods are mutually exclusive.
> 
> 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: Jack Pham <jackp@codeaurora.org>
> Cc: linux-arm-msm@vger.kernel.org
> Cc: linux-usb@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 6f4b2b3cffce..f6a7ede5953e 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -571,6 +571,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	struct device		*dev = &pdev->dev;
>  	struct dwc3_qcom	*qcom;
>  	struct resource		*res, *parent_res = NULL;
> +	struct dwc3		*dwc;
>  	int			ret, i;
>  	bool			ignore_pipe_clk;
>  
> @@ -669,8 +670,16 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
>  		dwc3_qcom_vbus_overrride_enable(qcom, true);
>  
> -	/* register extcon to override sw_vbus on Vbus change later */
> -	ret = dwc3_qcom_register_extcon(qcom);
> +	if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {
> +		/* Using gpio-usb-conn register a notifier for VBUS */
> +		dwc = platform_get_drvdata(qcom->dwc3);

As I was testing some other things on my qcs404 board this
suddenly failed.

The of_platform_populate() in dwc3_qcom_of_register_core() will create a
struct platform_device and attempt to probe this. But as my PHY(s) isn't
ready that returns with -EPROBE_DEFER - i.e. it will not reach the
platform_set_drvdata().

The check in dwc3_qcom_of_register_core() successfully resolves the
struct platform_device (it's sitting there waiting to be reprobed
later).

So qcom->dwc3 will be valid, but dwc here will be NULL.

> +		qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier;
> +		ret = dwc3_role_switch_notifier_register(dwc, &qcom->vbus_nb);

So here we pass NULL to dwc3_role_switch_notifier_register(), which
dereferences it and we get an oops.


I don't yet have a sane suggestion on how to redesign the dependency
between the two drivers in order to avoid this, but it's at least not
possible to access the child's state data from dwc3_qcom_probe().

Regards,
Bjorn

> +	} else {
> +		/* register extcon to override sw_vbus on Vbus change later */
> +		ret = dwc3_qcom_register_extcon(qcom);
> +	}
> +
>  	if (ret)
>  		goto depopulate;
>  
> @@ -702,8 +711,11 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  {
>  	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>  	struct device *dev = &pdev->dev;
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  	int i;
>  
> +	dwc3_role_switch_notifier_unregister(dwc, &qcom->vbus_nb);
> +
>  	of_platform_depopulate(dev);
>  
>  	for (i = qcom->num_clocks - 1; i >= 0; i--) {
> -- 
> 2.25.1
>
Bryan O'Donoghue March 17, 2020, 3:22 p.m. UTC | #2 
On 17/03/2020 06:31, Bjorn Andersson wrote:
> I don't yet have a sane suggestion on how to redesign the dependency
> between the two drivers in order to avoid this, but it's at least not
> possible to access the child's state data from dwc3_qcom_probe().

yep, this should be modeled as the dwc3 registering with the parent 
role-switch, like gpio-usb-conn does with dwc3.

I have an idea for a patch, I'll v2 this.
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 6f4b2b3cffce..f6a7ede5953e 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -571,6 +571,7 @@  static int dwc3_qcom_probe(struct platform_device *pdev)
 	struct device		*dev = &pdev->dev;
 	struct dwc3_qcom	*qcom;
 	struct resource		*res, *parent_res = NULL;
+	struct dwc3		*dwc;
 	int			ret, i;
 	bool			ignore_pipe_clk;
 
@@ -669,8 +670,16 @@  static int dwc3_qcom_probe(struct platform_device *pdev)
 	if (qcom->mode == USB_DR_MODE_PERIPHERAL)
 		dwc3_qcom_vbus_overrride_enable(qcom, true);
 
-	/* register extcon to override sw_vbus on Vbus change later */
-	ret = dwc3_qcom_register_extcon(qcom);
+	if (dwc3_qcom_find_gpio_usb_connector(qcom->dwc3)) {
+		/* Using gpio-usb-conn register a notifier for VBUS */
+		dwc = platform_get_drvdata(qcom->dwc3);
+		qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier;
+		ret = dwc3_role_switch_notifier_register(dwc, &qcom->vbus_nb);
+	} else {
+		/* register extcon to override sw_vbus on Vbus change later */
+		ret = dwc3_qcom_register_extcon(qcom);
+	}
+
 	if (ret)
 		goto depopulate;
 
@@ -702,8 +711,11 @@  static int dwc3_qcom_remove(struct platform_device *pdev)
 {
 	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
 	struct device *dev = &pdev->dev;
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 	int i;
 
+	dwc3_role_switch_notifier_unregister(dwc, &qcom->vbus_nb);
+
 	of_platform_depopulate(dev);
 
 	for (i = qcom->num_clocks - 1; i >= 0; i--) {