[PATCH 0/7] DWC3/Qualcomm connector based role-switching

[PATCH 0/7] DWC3/Qualcomm connector based role-switching

[Bryan O'Donoghue]

This set of patches adds the ability to use usb-gpio-connectors with
role-switching to the DWC3 controller.

Additional Qualcomm specific logic is included in the Qualcomm DWC3 wrapper
to facilitate PHY related updates on role-switch and thus VBUS state
toggle.

These patches have been through seven review cycles already and have a
number of Review-by and Ack-by. For the pusposes of making it easier to
merge this set focuses on the USB role-switching related stuff alone.

The last set for can be found here:
https://lkml.org/lkml/2020/3/3/807

Bryan O'Donoghue (6):
  dt-bindings: usb: dwc3: Add a gpio-usb-connector example
  dt-bindings: usb: dwc3: Add a usb-role-switch to the example
  usb: dwc3: qcom: Add support for usb-conn-gpio connectors
  usb: dwc3: Add support for usb-conn-gpio connectors
  usb: dwc3: Add support for a role-switch notifier
  usb: dwc3: qcom: Enable gpio-usb-conn based role-switching

Yu Chen (1):
  usb: dwc3: Registering a role switch in the DRD code.

 .../devicetree/bindings/usb/dwc3.txt          |   9 ++
 drivers/usb/dwc3/core.h                       |  22 ++++
 drivers/usb/dwc3/drd.c                        | 120 +++++++++++++++++-
 drivers/usb/dwc3/dwc3-qcom.c                  |  31 ++++-
 4 files changed, 179 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/7] usb: dwc3: Registering a role switch in the DRD code.

[Bryan O'Donoghue]

From: Yu Chen <chenyu56@huawei.com>

The Type-C drivers use USB role switch API to inform the
system about the negotiated data role, so registering a role
switch in the DRD code in order to support platforms with
USB Type-C connectors.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Guillaume Gardet <Guillaume.Gardet@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/core.h |  3 ++
 drivers/usb/dwc3/drd.c  | 78 ++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 80 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 77c4a9abe365..a99e57636172 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -25,6 +25,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/role.h>
 #include <linux/ulpi/interface.h>
 
 #include <linux/phy/phy.h>
@@ -953,6 +954,7 @@ struct dwc3_scratchpad_array {
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
+ * @role_sw: usb_role_switch handle
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -1086,6 +1088,7 @@ struct dwc3 {
 	struct extcon_dev	*edev;
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
+	struct usb_role_switch	*role_sw;
 
 	u32			fladj;
 	u32			irq_gadget;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index c946d64142ad..ba0f6cdc4c71 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -476,6 +476,74 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	return edev;
 }
 
+#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
+#define ROLE_SWITCH 1
+static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role role)
+{
+	struct dwc3 *dwc = usb_role_switch_get_drvdata(sw);
+	u32 mode;
+
+	switch (role) {
+	case USB_ROLE_HOST:
+		mode = DWC3_GCTL_PRTCAP_HOST;
+		break;
+	case USB_ROLE_DEVICE:
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		break;
+	default:
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		break;
+	}
+
+	dwc3_set_mode(dwc, mode);
+	return 0;
+}
+
+static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
+{
+	struct dwc3 *dwc = usb_role_switch_get_drvdata(sw);
+	unsigned long flags;
+	enum usb_role role;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		role = USB_ROLE_HOST;
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		role = USB_ROLE_DEVICE;
+		break;
+	case DWC3_GCTL_PRTCAP_OTG:
+		role = dwc->current_otg_role;
+		break;
+	default:
+		role = USB_ROLE_DEVICE;
+		break;
+	}
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	return role;
+}
+
+static int dwc3_setup_role_switch(struct dwc3 *dwc)
+{
+	struct usb_role_switch_desc dwc3_role_switch = {NULL};
+
+	dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
+	dwc3_role_switch.set = dwc3_usb_role_switch_set;
+	dwc3_role_switch.get = dwc3_usb_role_switch_get;
+	dwc3_role_switch.driver_data = dwc;
+	dwc->role_sw = usb_role_switch_register(dwc->dev, &dwc3_role_switch);
+	if (IS_ERR(dwc->role_sw))
+		return PTR_ERR(dwc->role_sw);
+
+	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	return 0;
+}
+#else
+#define ROLE_SWITCH 0
+#define dwc3_setup_role_switch(x) 0
+#endif
+
 int dwc3_drd_init(struct dwc3 *dwc)
 {
 	int ret, irq;
@@ -484,7 +552,12 @@ int dwc3_drd_init(struct dwc3 *dwc)
 	if (IS_ERR(dwc->edev))
 		return PTR_ERR(dwc->edev);
 
-	if (dwc->edev) {
+	if (ROLE_SWITCH &&
+	    device_property_read_bool(dwc->dev, "usb-role-switch")) {
+		ret = dwc3_setup_role_switch(dwc);
+		if (ret < 0)
+			return ret;
+	} else if (dwc->edev) {
 		dwc->edev_nb.notifier_call = dwc3_drd_notifier;
 		ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
 					       &dwc->edev_nb);
@@ -531,6 +604,9 @@ void dwc3_drd_exit(struct dwc3 *dwc)
 {
 	unsigned long flags;
 
+	if (dwc->role_sw)
+		usb_role_switch_unregister(dwc->role_sw);
+
 	if (dwc->edev)
 		extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
 					   &dwc->edev_nb);
-- 
2.25.1

[PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Bryan O'Donoghue]

A USB connector should be a child node of the USB controller
connector/usb-connector.txt. This patch adds an example of how to do this
to the dwc3 binding descriptions.

It is necessary to declare a connector as a child-node of a USB controller
for role-switching to work, so this example should be helpful to others
implementing that.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Acked-by: Felipe Balbi <balbi@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 66780a47ad85..4e1e4afccee6 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -121,4 +121,12 @@ dwc3@4a030000 {
 	interrupts = <0 92 4>
 	usb-phy = <&usb2_phy>, <&usb3,phy>;
 	snps,incr-burst-type-adjustment = <1>, <4>, <8>, <16>;
+
+	usb_con: connector {
+		compatible = "gpio-usb-b-connector";
+		id-gpios = <&tlmm 116 GPIO_ACTIVE_HIGH>;
+		vbus-supply = <&usb3_vbus_reg>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&usb3_id_pin>, <&usb3_vbus_pin>;
+	};
 };
-- 
2.25.1

Re: [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Stephen Boyd]

Quoting Bryan O'Donoghue (2020-03-11 12:14:56)
> A USB connector should be a child node of the USB controller
> connector/usb-connector.txt. This patch adds an example of how to do this
> to the dwc3 binding descriptions.

I read that as a child of the USB interface controller, which is not the
same as the USB controller. For example, we're talking about having the
usb connector be a child of the EC on ChromeOS devices because that
manages the connector

> 
> It is necessary to declare a connector as a child-node of a USB controller
> for role-switching to work, so this example should be helpful to others
> implementing that.

Maybe it should be a virtual node at the root of the DT if it's GPIO
controlled? And then the phy can be connected to the usb connector
through the graph binding.

> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Acked-by: Felipe Balbi <balbi@kernel.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 66780a47ad85..4e1e4afccee6 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -121,4 +121,12 @@ dwc3@4a030000 {
>         interrupts = <0 92 4>
>         usb-phy = <&usb2_phy>, <&usb3,phy>;

Weird that there's a comma here        ^

Not a problem with this patch, just noticing it while reading the diff.

>         snps,incr-burst-type-adjustment = <1>, <4>, <8>, <16>;
> +
> +       usb_con: connector {
> +               compatible = "gpio-usb-b-connector";
> +               id-gpios = <&tlmm 116 GPIO_ACTIVE_HIGH>;
> +               vbus-supply = <&usb3_vbus_reg>;
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&usb3_id_pin>, <&usb3_vbus_pin>;
> +       };

Re: [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Bryan O'Donoghue]

On 19/03/2020 01:08, Stephen Boyd wrote:
> Quoting Bryan O'Donoghue (2020-03-11 12:14:56)
>> A USB connector should be a child node of the USB controller
>> connector/usb-connector.txt. This patch adds an example of how to do this
>> to the dwc3 binding descriptions.
> 
> I read that as a child of the USB interface controller, which is not the
> same as the USB controller. For example, we're talking about having the
> usb connector be a child of the EC on ChromeOS devices because that
> manages the connector
> 
>>
>> It is necessary to declare a connector as a child-node of a USB controller
>> for role-switching to work, so this example should be helpful to others
>> implementing that.
> 
> Maybe it should be a virtual node at the root of the DT if it's GPIO
> controlled? And then the phy can be connected to the usb connector
> through the graph binding.

Graph binding can probably work.

Re: the PHY.

For myself the hardware model is

Connector -> PHY -> Host controller -> Host controller wrapper

Only

Connector -> Host controller -> Host controller wrapper

care about the USB role though.

If your PHY did care about the role, you'd really need to write a 
connector/phy type-c type driver, to detect the state and toggle your 
PHY bits before doing usb_role_switch_set_role() back to DWC3.

At least that's my understanding.

---
bod

Re: [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Stephen Boyd]

Quoting Bryan O'Donoghue (2020-03-19 08:22:14)
> On 19/03/2020 01:08, Stephen Boyd wrote:
> > 
> > Maybe it should be a virtual node at the root of the DT if it's GPIO
> > controlled? And then the phy can be connected to the usb connector
> > through the graph binding.
> 
> Graph binding can probably work.
> 
> Re: the PHY.
> 
> For myself the hardware model is
> 
> Connector -> PHY -> Host controller -> Host controller wrapper
> 
> Only
> 
> Connector -> Host controller -> Host controller wrapper
> 
> care about the USB role though.
> 
> If your PHY did care about the role, you'd really need to write a 
> connector/phy type-c type driver, to detect the state and toggle your 
> PHY bits before doing usb_role_switch_set_role() back to DWC3.
> 

Yes some PHYs do care about the role. Sometimes they have to toggle some
bit to switch between host and gadget mode for example. I haven't fully
read this patch series but maybe the PHY can be the one that controls
the gpio for the connector?

We (ChromeOS) need to integrate the type-c connector class, etc. on
sc7180 with the dwc3 driver and the current thinking has the type-c
connectors underneath the cros_ec node because the EC is the type-c
manager. The EC will have a type-c driver associated with it.

Re: [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Bryan O'Donoghue]

On 19/03/2020 16:40, Stephen Boyd wrote:
> Quoting Bryan O'Donoghue (2020-03-19 08:22:14)
>> On 19/03/2020 01:08, Stephen Boyd wrote:
>>>
>>> Maybe it should be a virtual node at the root of the DT if it's GPIO
>>> controlled? And then the phy can be connected to the usb connector
>>> through the graph binding.
>>
>> Graph binding can probably work.
>>
>> Re: the PHY.
>>
>> For myself the hardware model is
>>
>> Connector -> PHY -> Host controller -> Host controller wrapper
>>
>> Only
>>
>> Connector -> Host controller -> Host controller wrapper
>>
>> care about the USB role though.
>>
>> If your PHY did care about the role, you'd really need to write a
>> connector/phy type-c type driver, to detect the state and toggle your
>> PHY bits before doing usb_role_switch_set_role() back to DWC3.
>>
> 
> Yes some PHYs do care about the role. Sometimes they have to toggle some
> bit to switch between host and gadget mode for example. I haven't fully
> read this patch series but maybe the PHY can be the one that controls
> the gpio for the connector?

Previous version of the PHY from 2019 had extcon toggling vbus.

Since extcon is going away, we moved go usb-gpio

https://lwn.net/ml/devicetree/20190905175802.GA19599@jackp-linux.qualcomm.com/

https://lwn.net/ml/devicetree/5d71edf5.1c69fb81.1f307.fdd6@mx.google.com/

usb-gpio-conn handle VBUS and notifies via the USB role switch API.

Which if the connector is a child of the controller "just works" but, 
maybe with a little bit of work DT <port> references could do the same 
thing and the connector wouldn't need to be declared as a child.

> We (ChromeOS) need to integrate the type-c connector class, etc. on
> sc7180 with the dwc3 driver and the current thinking has the type-c
> connectors underneath the cros_ec node because the EC is the type-c
> manager. The EC will have a type-c driver associated with it.

right and you don't want, doesn't work or doesn't make sense, to declare 
cros_ec as a child of DWC3, fair enough.

I guess a DT remote-endpoint{} will do the job.

Something like:
arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi

---
bod

Re: [PATCH 2/7] dt-bindings: usb: dwc3: Add a gpio-usb-connector example

[Stephen Boyd]

Quoting Bryan O'Donoghue (2020-03-19 11:03:58)
> On 19/03/2020 16:40, Stephen Boyd wrote:
> > the gpio for the connector?
> 
> Previous version of the PHY from 2019 had extcon toggling vbus.
> 
> Since extcon is going away, we moved go usb-gpio
> 
> https://lwn.net/ml/devicetree/20190905175802.GA19599@jackp-linux.qualcomm.com/
> 
> https://lwn.net/ml/devicetree/5d71edf5.1c69fb81.1f307.fdd6@mx.google.com/
> 
> usb-gpio-conn handle VBUS and notifies via the USB role switch API.
> 
> Which if the connector is a child of the controller "just works" but, 
> maybe with a little bit of work DT <port> references could do the same 
> thing and the connector wouldn't need to be declared as a child.
> 
> > We (ChromeOS) need to integrate the type-c connector class, etc. on
> > sc7180 with the dwc3 driver and the current thinking has the type-c
> > connectors underneath the cros_ec node because the EC is the type-c
> > manager. The EC will have a type-c driver associated with it.
> 
> right and you don't want, doesn't work or doesn't make sense, to declare 
> cros_ec as a child of DWC3, fair enough.
> 
> I guess a DT remote-endpoint{} will do the job.
> 
> Something like:
> arch/arm64/boot/dts/freescale/imx8mn-evk.dtsi
> 

Yes something like that, but it looks like that dtsi file has the usb
host controller connected through a remote-endpoint to the type-c
connector. I was under the impression that it would only be the phy that
is connected this way because it's possible for there to be multiple
different phys that drive data out through one connector. For example,
in type-c the DP phy can be different from the USB2 phy or USB3 phy and
there could even be other things like an HDMI phy too that all go to the
same connector.

[PATCH 3/7] dt-bindings: usb: dwc3: Add a usb-role-switch to the example

[Bryan O'Donoghue]

This patch adds usb-role-switch to the example dwc3 given in the file.

Documentation/devicetree/bindings/usb/generic.txt makes this a valid
declaration for dwc3 this patch gives an example of how to use it.

Reviewed-by: Rob Herring <robh@kernel.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 4e1e4afccee6..8c6c7b355356 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -121,6 +121,7 @@ dwc3@4a030000 {
 	interrupts = <0 92 4>
 	usb-phy = <&usb2_phy>, <&usb3,phy>;
 	snps,incr-burst-type-adjustment = <1>, <4>, <8>, <16>;
+	usb-role-switch;
 
 	usb_con: connector {
 		compatible = "gpio-usb-b-connector";
-- 
2.25.1

[PATCH 4/7] usb: dwc3: qcom: Add support for usb-conn-gpio connectors

[Bryan O'Donoghue]

This patch adds a routine to find a usb-conn-gpio in the main DWC3 code.
This will be useful in a subsequent patch where we will reuse the current
extcon VBUS notifier with usb-conn-gpio.

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>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 1dfd024cd06b..6f4b2b3cffce 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;
-- 
2.25.1

[PATCH 5/7] usb: dwc3: Add support for usb-conn-gpio connectors

[Bryan O'Donoghue]

This patch adds the ability to probe and enumerate a connector based on
usb-conn-gpio.

You would use usb-conn-gpio when a regulator in your system provides VBUS
directly to the connector instead of supplying via the USB PHY.

The parent device must have the "usb-role-switch" property, so that when
the usb-conn-gpio driver calls usb_role_switch_set_role() the notification
in dwc3 will run and the block registers will be updated to match the state
detected at the connector.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/drd.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index ba0f6cdc4c71..2705871ec95e 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -11,6 +11,7 @@
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/property.h>
+#include <linux/of_platform.h>
 
 #include "debug.h"
 #include "core.h"
@@ -539,9 +540,30 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 	return 0;
 }
+
+static int dwc3_register_gpio_usb_connector(struct dwc3 *dwc)
+{
+	struct device		*dev = dwc->dev;
+	struct device_node	*np = dev->of_node, *conn_np;
+	int			ret = 0;
+
+	conn_np = of_get_child_by_name(np, "connector");
+	if (!conn_np) {
+		dev_dbg(dev, "no connector child node specified\n");
+		goto done;
+	}
+
+	if (of_device_is_compatible(conn_np, "gpio-usb-b-connector"))
+		ret = of_platform_populate(np, NULL, NULL, dev);
+done:
+	of_node_put(conn_np);
+	return ret;
+}
+
 #else
 #define ROLE_SWITCH 0
 #define dwc3_setup_role_switch(x) 0
+#define dwc3_register_gpio_usb_connector(x) 0
 #endif
 
 int dwc3_drd_init(struct dwc3 *dwc)
@@ -557,6 +579,9 @@ int dwc3_drd_init(struct dwc3 *dwc)
 		ret = dwc3_setup_role_switch(dwc);
 		if (ret < 0)
 			return ret;
+		ret = dwc3_register_gpio_usb_connector(dwc);
+		if (ret < 0)
+			return ret;
 	} else if (dwc->edev) {
 		dwc->edev_nb.notifier_call = dwc3_drd_notifier;
 		ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
-- 
2.25.1

[PATCH 6/7] usb: dwc3: Add support for a role-switch notifier

[Bryan O'Donoghue]

Role-switching is a 1:1 mapping between a producer and a consumer. For DWC3
we have some vendor specific wrappers, notably the qcom wrapper that want
to toggle some PHY related bits on a USB role switch.

This patch adds a role-switch notifier to the dwc3 drd code. When the USB
role-switch set() routine runs, the notifier will fire passing the notified
mode to the consumer, thus allowing vendor specific fix-ups to toggle from
the role-switching events.

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
Reviewed-by: Jack Pham <jackp@codeaurora.org>
Tested-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
---
 drivers/usb/dwc3/core.h | 19 +++++++++++++++++++
 drivers/usb/dwc3/drd.c  | 17 +++++++++++++++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index a99e57636172..c2e85f587674 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -955,6 +955,7 @@ struct dwc3_scratchpad_array {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_sw_nl: role switch notifier list
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -1089,6 +1090,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	struct raw_notifier_head role_sw_nl;
 
 	u32			fladj;
 	u32			irq_gadget;
@@ -1499,4 +1501,21 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
 { }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
+int dwc3_role_switch_notifier_register(struct dwc3 *dwc,
+				       struct notifier_block *nb);
+int dwc3_role_switch_notifier_unregister(struct dwc3 *dwc,
+					 struct notifier_block *nb);
+#else
+static inline int
+dwc3_role_switch_notifier_register(struct dwc3 *dwc,
+				   struct notifier_block *nb)
+{ return 0; }
+
+static inline int
+dwc3_role_switch_notifier_unregister(struct dwc3 *dwc,
+				     struct notifier_block *nb)
+{ return 0; }
+#endif
+
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 2705871ec95e..789e93dd93b4 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -497,6 +497,8 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role ro
 	}
 
 	dwc3_set_mode(dwc, mode);
+	raw_notifier_call_chain(&dwc->role_sw_nl, mode, NULL);
+
 	return 0;
 }
 
@@ -560,6 +562,18 @@ static int dwc3_register_gpio_usb_connector(struct dwc3 *dwc)
 	return ret;
 }
 
+int dwc3_role_switch_notifier_register(struct dwc3 *dwc,
+				       struct notifier_block *nb)
+{
+	return raw_notifier_chain_register(&dwc->role_sw_nl, nb);
+}
+
+int dwc3_role_switch_notifier_unregister(struct dwc3 *dwc,
+					 struct notifier_block *nb)
+{
+	return raw_notifier_chain_unregister(&dwc->role_sw_nl, nb);
+}
+
 #else
 #define ROLE_SWITCH 0
 #define dwc3_setup_role_switch(x) 0
@@ -582,6 +596,9 @@ int dwc3_drd_init(struct dwc3 *dwc)
 		ret = dwc3_register_gpio_usb_connector(dwc);
 		if (ret < 0)
 			return ret;
+
+		RAW_INIT_NOTIFIER_HEAD(&dwc->role_sw_nl);
+
 	} else if (dwc->edev) {
 		dwc->edev_nb.notifier_call = dwc3_drd_notifier;
 		ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
-- 
2.25.1

Re: [PATCH 6/7] usb: dwc3: Add support for a role-switch notifier

[Vincent Whitchurch]

On Wed, Mar 11, 2020 at 07:15:00PM +0000, Bryan O'Donoghue wrote:
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 2705871ec95e..789e93dd93b4 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -497,6 +497,8 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw, enum usb_role ro
>  	}
>  
>  	dwc3_set_mode(dwc, mode);
> +	raw_notifier_call_chain(&dwc->role_sw_nl, mode, NULL);
> +
>  	return 0;
>  }

dwc3_set_mode() is called from a bunch of other places too, is it
sufficient to call the notifier only from here?  Also, dwc3_set_mode()
performs the mode set asynchronously so the mode switch can race with
this notifier call, is that OK?

Mike Looijmans proposed the control of a vbus regulator from
__dwc3_set_mode(), and that would take care of both the points above.
Perhaps this notifier call can be moved to the same place or perhaps
Mike's patch could even work for you?  The only problem is that your
switching code in dwc3-qcom.c would have to be modelled as a reulator:

 https://lore.kernel.org/linux-usb/20200619142512.19824-1-mike.looijmans@topic.nl/

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

[Bryan O'Donoghue]

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);
+		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--) {
-- 
2.25.1

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

[Bjorn Andersson]

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
> 

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

[Bryan O'Donoghue]

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.