[PATCH 0/2] dwc3-qcom: Prepare the ground for pm8150b tcpm

[PATCH 0/2] dwc3-qcom: Prepare the ground for pm8150b tcpm

[Bryan O'Donoghue]

These two patches from Wesley prepare the ground for addition of a full
typec port manager upstream.

We took a bunch of these patches into the RB5 development tree, which I
have subsequently split into a USB/TCPM specific staging branch for
upstream.

https://git.linaro.org/people/bryan.odonoghue/kernel.git/log/?h=usb-next-5.13.rcx-rb5-tcpm

Some of the original patches from Wesley for basic type-c have landed
upstream and some have not. The type-c patches are subsumed into a pm8150b
tcpm driver but, these two specific patches have not changed and are still
required.

I'd like to resubmit them here with my Tested-by and Signed-off-by appended.

https://patchwork.kernel.org/project/linux-arm-msm/cover/20201009082843.28503-1-wcheng@codeaurora.org/

Wesley Cheng (2):
  usb: dwc3: dwc3-qcom: Find USB connector and register role switch
  usb: dwc3: dwc3-qcom: Fix typo in the dwc3 vbus override API

 drivers/usb/dwc3/dwc3-qcom.c | 126 +++++++++++++++++++++++++++++++++--
 1 file changed, 120 insertions(+), 6 deletions(-)

-- 
2.30.1

[PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

From: Wesley Cheng <wcheng@codeaurora.org>

If registering a USB typeC connector, the connector node may not be a child
of the DWC3 QCOM device.  Utilize devcon graph search to lookup if any
remote endpoints contain the connector.  If a connector is present, the
DWC3 QCOM will register a USB role switch to receive role change events, as
well as attain a reference to the DWC3 core role switch to pass the event
down.

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

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 49e6ca94486d..4491704503ab 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/fwnode.h>
+#include <linux/usb/role.h>
 
 #include "core.h"
 
@@ -82,6 +84,9 @@ struct dwc3_qcom {
 	struct notifier_block	vbus_nb;
 	struct notifier_block	host_nb;
 
+	struct usb_role_switch *role_sw;
+	struct usb_role_switch *dwc3_drd_sw;
+
 	const struct dwc3_acpi_pdata *acpi_pdata;
 
 	enum usb_dr_mode	mode;
@@ -296,6 +301,73 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
 	icc_put(qcom->icc_path_apps);
 }
 
+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);
+	struct fwnode_handle *child;
+	bool enable = false;
+
+	if (!qcom->dwc3_drd_sw) {
+		child = device_get_next_child_node(qcom->dev, NULL);
+		if (child) {
+			qcom->dwc3_drd_sw = usb_role_switch_find_by_fwnode(child);
+			fwnode_handle_put(child);
+			if (IS_ERR(qcom->dwc3_drd_sw)) {
+				qcom->dwc3_drd_sw = NULL;
+				return 0;
+			}
+		}
+	}
+
+	usb_role_switch_set_role(qcom->dwc3_drd_sw, role);
+
+	if (role == USB_ROLE_DEVICE)
+		enable = true;
+	else
+		enable = false;
+
+	qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
+					       USB_DR_MODE_PERIPHERAL;
+	dwc3_qcom_vbus_overrride_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_HOST:
+		role = USB_ROLE_HOST;
+		break;
+	case USB_DR_MODE_PERIPHERAL:
+		role = USB_ROLE_DEVICE;
+		break;
+	default:
+		role = USB_ROLE_DEVICE;
+		break;
+	}
+
+	return role;
+}
+
+static int dwc3_qcom_setup_role_switch(struct dwc3_qcom *qcom)
+{
+	struct usb_role_switch_desc dwc3_role_switch = {NULL};
+
+	dwc3_role_switch.fwnode = dev_fwnode(qcom->dev);
+	dwc3_role_switch.set = dwc3_qcom_usb_role_switch_set;
+	dwc3_role_switch.get = dwc3_qcom_usb_role_switch_get;
+	dwc3_role_switch.driver_data = qcom;
+	qcom->role_sw = usb_role_switch_register(qcom->dev, &dwc3_role_switch);
+	if (IS_ERR(qcom->role_sw))
+		return PTR_ERR(qcom->role_sw);
+
+	return 0;
+}
+
 static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
 {
 	if (qcom->hs_phy_irq) {
@@ -698,6 +770,40 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
 	return acpi_create_platform_device(adev, NULL);
 }
 
+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;
+}
+
+static void *dwc3_qcom_find_usb_connector_match(struct fwnode_handle *fwnode,
+						const char *id, void *data)
+{
+	/* Check if the "connector" node is the parent of the remote endpoint */
+	if (dwc3_qcom_connector_check(fwnode))
+		return fwnode;
+
+	/* else, check if it is a child node */
+	fwnode = fwnode_get_named_child_node(fwnode, "connector");
+	if (dwc3_qcom_connector_check(fwnode))
+		return fwnode;
+
+	return 0;
+}
+
+static bool dwc3_qcom_find_usb_connector(struct platform_device *pdev)
+{
+	struct fwnode_handle *fwnode = pdev->dev.fwnode;
+
+	return fwnode_connection_find_match(fwnode, "connector", NULL,
+					    dwc3_qcom_find_usb_connector_match);
+}
+
 static int dwc3_qcom_probe(struct platform_device *pdev)
 {
 	struct device_node	*np = pdev->dev.of_node;
@@ -813,8 +919,13 @@ 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_usb_connector(pdev)) {
+		ret = dwc3_qcom_setup_role_switch(qcom);
+	} else {
+		/* register extcon to override sw_vbus on Vbus change later */
+		ret = dwc3_qcom_register_extcon(qcom);
+	}
+
 	if (ret)
 		goto interconnect_exit;
 
@@ -850,6 +961,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	usb_role_switch_unregister(qcom->role_sw);
+	usb_role_switch_put(qcom->dwc3_drd_sw);
+
 	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 
-- 
2.30.1

[PATCH 2/2] 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 | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 4491704503ab..541e2626294a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -120,7 +120,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,
@@ -141,7 +141,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;
@@ -153,7 +153,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;
@@ -329,7 +329,7 @@ static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw,
 
 	qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
 					       USB_DR_MODE_PERIPHERAL;
-	dwc3_qcom_vbus_overrride_enable(qcom, enable);
+	dwc3_qcom_vbus_override_enable(qcom, enable);
 	return 0;
 }
 
@@ -917,7 +917,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);
 
 	if (dwc3_qcom_find_usb_connector(pdev)) {
 		ret = dwc3_qcom_setup_role_switch(qcom);
-- 
2.30.1

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bjorn Andersson]

On Tue 29 Jun 09:44 CDT 2021, Bryan O'Donoghue wrote:

> From: Wesley Cheng <wcheng@codeaurora.org>
> 
> If registering a USB typeC connector, the connector node may not be a child
> of the DWC3 QCOM device.  Utilize devcon graph search to lookup if any
> remote endpoints contain the connector.  If a connector is present, the
> DWC3 QCOM will register a USB role switch to receive role change events, as
> well as attain a reference to the DWC3 core role switch to pass the event
> down.
> 

What's wrong with the switch that dwc3_setup_role_switch() sets up?

That's what I've been using in my UCSI hacking on Snapdragon 888 and it
seems to work...

Regards,
Bjorn

> Signed-off-by: Wesley Cheng <wcheng@codeaurora.org>
> Tested-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> Signed-off-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 118 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 116 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 49e6ca94486d..4491704503ab 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/fwnode.h>
> +#include <linux/usb/role.h>
>  
>  #include "core.h"
>  
> @@ -82,6 +84,9 @@ struct dwc3_qcom {
>  	struct notifier_block	vbus_nb;
>  	struct notifier_block	host_nb;
>  
> +	struct usb_role_switch *role_sw;
> +	struct usb_role_switch *dwc3_drd_sw;
> +
>  	const struct dwc3_acpi_pdata *acpi_pdata;
>  
>  	enum usb_dr_mode	mode;
> @@ -296,6 +301,73 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
>  	icc_put(qcom->icc_path_apps);
>  }
>  
> +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);
> +	struct fwnode_handle *child;
> +	bool enable = false;
> +
> +	if (!qcom->dwc3_drd_sw) {
> +		child = device_get_next_child_node(qcom->dev, NULL);
> +		if (child) {
> +			qcom->dwc3_drd_sw = usb_role_switch_find_by_fwnode(child);
> +			fwnode_handle_put(child);
> +			if (IS_ERR(qcom->dwc3_drd_sw)) {
> +				qcom->dwc3_drd_sw = NULL;
> +				return 0;
> +			}
> +		}
> +	}
> +
> +	usb_role_switch_set_role(qcom->dwc3_drd_sw, role);
> +
> +	if (role == USB_ROLE_DEVICE)
> +		enable = true;
> +	else
> +		enable = false;
> +
> +	qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
> +					       USB_DR_MODE_PERIPHERAL;
> +	dwc3_qcom_vbus_overrride_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_HOST:
> +		role = USB_ROLE_HOST;
> +		break;
> +	case USB_DR_MODE_PERIPHERAL:
> +		role = USB_ROLE_DEVICE;
> +		break;
> +	default:
> +		role = USB_ROLE_DEVICE;
> +		break;
> +	}
> +
> +	return role;
> +}
> +
> +static int dwc3_qcom_setup_role_switch(struct dwc3_qcom *qcom)
> +{
> +	struct usb_role_switch_desc dwc3_role_switch = {NULL};
> +
> +	dwc3_role_switch.fwnode = dev_fwnode(qcom->dev);
> +	dwc3_role_switch.set = dwc3_qcom_usb_role_switch_set;
> +	dwc3_role_switch.get = dwc3_qcom_usb_role_switch_get;
> +	dwc3_role_switch.driver_data = qcom;
> +	qcom->role_sw = usb_role_switch_register(qcom->dev, &dwc3_role_switch);
> +	if (IS_ERR(qcom->role_sw))
> +		return PTR_ERR(qcom->role_sw);
> +
> +	return 0;
> +}
> +
>  static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>  {
>  	if (qcom->hs_phy_irq) {
> @@ -698,6 +770,40 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
>  	return acpi_create_platform_device(adev, NULL);
>  }
>  
> +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;
> +}
> +
> +static void *dwc3_qcom_find_usb_connector_match(struct fwnode_handle *fwnode,
> +						const char *id, void *data)
> +{
> +	/* Check if the "connector" node is the parent of the remote endpoint */
> +	if (dwc3_qcom_connector_check(fwnode))
> +		return fwnode;
> +
> +	/* else, check if it is a child node */
> +	fwnode = fwnode_get_named_child_node(fwnode, "connector");
> +	if (dwc3_qcom_connector_check(fwnode))
> +		return fwnode;
> +
> +	return 0;
> +}
> +
> +static bool dwc3_qcom_find_usb_connector(struct platform_device *pdev)
> +{
> +	struct fwnode_handle *fwnode = pdev->dev.fwnode;
> +
> +	return fwnode_connection_find_match(fwnode, "connector", NULL,
> +					    dwc3_qcom_find_usb_connector_match);
> +}
> +
>  static int dwc3_qcom_probe(struct platform_device *pdev)
>  {
>  	struct device_node	*np = pdev->dev.of_node;
> @@ -813,8 +919,13 @@ 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_usb_connector(pdev)) {
> +		ret = dwc3_qcom_setup_role_switch(qcom);
> +	} else {
> +		/* register extcon to override sw_vbus on Vbus change later */
> +		ret = dwc3_qcom_register_extcon(qcom);
> +	}
> +
>  	if (ret)
>  		goto interconnect_exit;
>  
> @@ -850,6 +961,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int i;
>  
> +	usb_role_switch_unregister(qcom->role_sw);
> +	usb_role_switch_put(qcom->dwc3_drd_sw);
> +
>  	device_remove_software_node(&qcom->dwc3->dev);
>  	of_platform_depopulate(dev);
>  
> -- 
> 2.30.1
> 

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

[Bjorn Andersson]

On Tue 29 Jun 09:44 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>

But I think you should have sent the two patches in opposite order,
because this could have been merged, but now it depends on patch 1.

Regards,
Bjorn

> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 4491704503ab..541e2626294a 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -120,7 +120,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,
> @@ -141,7 +141,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;
> @@ -153,7 +153,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;
> @@ -329,7 +329,7 @@ static int dwc3_qcom_usb_role_switch_set(struct usb_role_switch *sw,
>  
>  	qcom->mode = (role == USB_ROLE_HOST) ? USB_DR_MODE_HOST :
>  					       USB_DR_MODE_PERIPHERAL;
> -	dwc3_qcom_vbus_overrride_enable(qcom, enable);
> +	dwc3_qcom_vbus_override_enable(qcom, enable);
>  	return 0;
>  }
>  
> @@ -917,7 +917,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);
>  
>  	if (dwc3_qcom_find_usb_connector(pdev)) {
>  		ret = dwc3_qcom_setup_role_switch(qcom);
> -- 
> 2.30.1
> 

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

On 29/06/2021 16:48, Bjorn Andersson wrote:
> What's wrong with the switch that dwc3_setup_role_switch() sets up?
> 
> That's what I've been using in my UCSI hacking on Snapdragon 888 and it
> seems to work...
> 
> Regards,
> Bjorn

A good question, which as soon as you asked it made me completely doubt 
if I'd tested the tree without the patch applied.

I reverted both on my working tree and indeed it breaks role-switch 
detection.

In TCPM the connector is a child node of TCPM

https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm 
line 1396

We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303

Not entirely sure what the graph tree looks like in your USCI case but I 
guess the connector is a child node of the controller ?

But I think your question is why bother with the role-switch in dwc3-qcom

dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought 
to be in the PHY but for whatever reason ended up being buried in the 
qcom-dwc3 wrapper.

Certainly we want that for qcs405 if you remember - I'm assuming for the 
sm8x50 too.

Even if we shouldn't twiddle these bits on sm8x50 I believe its wanted 
on qcs405.

I'm open to correction on that one though

---
bod

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Jack Pham]

On Tue, Jun 29, 2021 at 08:23:46PM +0100, Bryan O'Donoghue wrote:
> On 29/06/2021 16:48, Bjorn Andersson wrote:
> > What's wrong with the switch that dwc3_setup_role_switch() sets up?
> > 
> > That's what I've been using in my UCSI hacking on Snapdragon 888 and it
> > seems to work...

Bjorn are you exercising dual-role or just host mode?

> A good question, which as soon as you asked it made me completely doubt if
> I'd tested the tree without the patch applied.
> 
> I reverted both on my working tree and indeed it breaks role-switch
> detection.
> 
> In TCPM the connector is a child node of TCPM
> 
> https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm
> line 1396
> 
> We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303
> 
> Not entirely sure what the graph tree looks like in your USCI case but I
> guess the connector is a child node of the controller ?
> 
> But I think your question is why bother with the role-switch in dwc3-qcom
> 
> dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to
> be in the PHY but for whatever reason ended up being buried in the qcom-dwc3
> wrapper.

This. When switching dwc3 into peripheral mode we also need
dwc3_qcom_vbus_override_enable() to write those registers to manually
drive the UTMI VBUS valid signal high to the controller since our SoCs
are never physically wired to the connector's VBUS. These registers
(QSCRATCH_{HS,SS}_PHY_CTRL) however don't belong to the PHYs but are
part of our dwc3 wrapper's IO map and hence dwc3-qcom is the only
appropriate place to handle them since dwc3-qcom has over the years
paired with several different PHYs depending on SoC.

Wesley's approach in $SUBJECT was to designate dwc3-qcom itself as a
usb-role-switch-able device so that in the DT the connector graph
endpoints would wire to the dwc3-qcom device itself instead of its dwc3
core child. The idea would be for dwc3-qcom would intercept the
role_switch_set call from TCPM/UCSI, call the vbus_override_enable() and
then pass on the notification to the child to let dwc3/drd.c handle the
rest.

IIRC there had been an alternate proposal to keep the role switch
connection only at the dwc3 core but in order to handle the vbus
override business an additional notification would have needed to be
done either from the usb_role_switch class driver or as an "upcall" from
dwc3 core -> glue. (found it, it was by you Bryan [1])

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

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

On 29/06/2021 21:02, Jack Pham wrote:
> IIRC there had been an alternate proposal to keep the role switch
> connection only at the dwc3 core but in order to handle the vbus
> override business an additional notification would have needed to be
> done either from the usb_role_switch class driver or as an "upcall" from
> dwc3 core -> glue. (found it, it was by you Bryan [1])
> 
> [1]https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/

Which had a bug :( Bjorn found

My excuse for not following up is I had to hand back my hardware and got 
sucked into doing something else.

I think Wesley's approach here is a good one so, that's also why I'm 
re-posting.

I don't have a functional qcs405 setup but, I have validated it on sm8250.

---
bod

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bjorn Andersson]

On Tue 29 Jun 14:23 CDT 2021, Bryan O'Donoghue wrote:

> On 29/06/2021 16:48, Bjorn Andersson wrote:
> > What's wrong with the switch that dwc3_setup_role_switch() sets up?
> > 
> > That's what I've been using in my UCSI hacking on Snapdragon 888 and it
> > seems to work...
> > 
> > Regards,
> > Bjorn
> 
> A good question, which as soon as you asked it made me completely doubt if
> I'd tested the tree without the patch applied.
> 
> I reverted both on my working tree and indeed it breaks role-switch
> detection.
> 
> In TCPM the connector is a child node of TCPM
> 
> https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm
> line 1396
> 
> We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303
> 

That's expected, because the dwc3 role switching logic registers on
&usb_1_dwc3 instead of &usb_1.

> Not entirely sure what the graph tree looks like in your USCI case but I
> guess the connector is a child node of the controller ?
> 

No, the connector currently a child of a completely different rpmsg
device, i.e. in system terms it's the same as your design. One thing
detecting the change in parameters and then invoking the change on the
dwc3 controller (not the qcom variant thereof).

> But I think your question is why bother with the role-switch in dwc3-qcom
> 
> dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to
> be in the PHY but for whatever reason ended up being buried in the qcom-dwc3
> wrapper.
> 

I missed this part when I wired up my role switcher and things works
fine, but I presume there's a good reason for the vbus override and it's
not unlikely that I missed something in my testing.

I do however think that it would be appropriate to use the generic dwc3
role switcher and then make that somehow poke the vbus_override in the
qcom instance.

> Certainly we want that for qcs405 if you remember - I'm assuming for the
> sm8x50 too.
> 
> Even if we shouldn't twiddle these bits on sm8x50 I believe its wanted on
> qcs405.
> 

Can you remind me about the purpose of these bits?

Regards,
Bjorn

> I'm open to correction on that one though
> 
> ---
> bod

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bjorn Andersson]

On Tue 29 Jun 15:02 CDT 2021, Jack Pham wrote:

> On Tue, Jun 29, 2021 at 08:23:46PM +0100, Bryan O'Donoghue wrote:
> > On 29/06/2021 16:48, Bjorn Andersson wrote:
> > > What's wrong with the switch that dwc3_setup_role_switch() sets up?
> > > 
> > > That's what I've been using in my UCSI hacking on Snapdragon 888 and it
> > > seems to work...
> 
> Bjorn are you exercising dual-role or just host mode?
> 

I remember testing switching between host and peripheral, but given
your explanation below I doubt that I actually validated that peripheral
mode was functional.

> > A good question, which as soon as you asked it made me completely doubt if
> > I'd tested the tree without the patch applied.
> > 
> > I reverted both on my working tree and indeed it breaks role-switch
> > detection.
> > 
> > In TCPM the connector is a child node of TCPM
> > 
> > https://git.linaro.org/people/bryan.odonoghue/kernel.git/tree/arch/arm64/boot/dts/qcom/qrb5165-rb5.dts?h=usb-next-5.13.rcx-rb5-tcpm
> > line 1396
> > 
> > We use a remote endpoint inside of TCPM to hook up into &usb_1 line 1303
> > 
> > Not entirely sure what the graph tree looks like in your USCI case but I
> > guess the connector is a child node of the controller ?
> > 
> > But I think your question is why bother with the role-switch in dwc3-qcom
> > 
> > dwc3_qcom_vbus_override_enable(){} is switching bits that probably ought to
> > be in the PHY but for whatever reason ended up being buried in the qcom-dwc3
> > wrapper.
> 
> This. When switching dwc3 into peripheral mode we also need
> dwc3_qcom_vbus_override_enable() to write those registers to manually
> drive the UTMI VBUS valid signal high to the controller since our SoCs
> are never physically wired to the connector's VBUS. These registers
> (QSCRATCH_{HS,SS}_PHY_CTRL) however don't belong to the PHYs but are
> part of our dwc3 wrapper's IO map and hence dwc3-qcom is the only
> appropriate place to handle them since dwc3-qcom has over the years
> paired with several different PHYs depending on SoC.
> 
> Wesley's approach in $SUBJECT was to designate dwc3-qcom itself as a
> usb-role-switch-able device so that in the DT the connector graph
> endpoints would wire to the dwc3-qcom device itself instead of its dwc3
> core child. The idea would be for dwc3-qcom would intercept the
> role_switch_set call from TCPM/UCSI, call the vbus_override_enable() and
> then pass on the notification to the child to let dwc3/drd.c handle the
> rest.
> 

I really do object against us adding another role switch implementation
just to, once again, circumvent the annoying split between the platform
specific and core part of the dwc3 driver.

> IIRC there had been an alternate proposal to keep the role switch
> connection only at the dwc3 core but in order to handle the vbus
> override business an additional notification would have needed to be
> done either from the usb_role_switch class driver or as an "upcall" from
> dwc3 core -> glue. (found it, it was by you Bryan [1])
> 
> [1] https://lore.kernel.org/linux-usb/20200311191501.8165-7-bryan.odonoghue@linaro.org/

I liked this, and it worked when I tested it, but iirc it suffered from
the problem that the core's probe may or may not have finished
successfully at the time that of_platform_populate() returns.

But fixing this problem would save us quite a bit of headache.

Regards,
Bjorn

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

On 29/06/2021 21:30, Bjorn Andersson wrote:
> I liked this, and it worked when I tested it, but iirc it suffered from
> the problem that the core's probe may or may not have finished
> successfully at the time that of_platform_populate() returns.
> 
> But fixing this problem would save us quite a bit of headache.

OK.

I will take a look at resurrecting the old patches either fixing the 
probe order - or perhaps using something like Wesley's role-switch to 
have dwc3 core optionally trigger dwc3-qcom

Binding tcpm into &usb_1_dwc3 instead of &usb_1

---
bod

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

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

On 29/06/2021 22:57, Bryan O'Donoghue wrote:
> On 29/06/2021 21:30, Bjorn Andersson wrote:
>> I liked this, and it worked when I tested it, but iirc it suffered from
>> the problem that the core's probe may or may not have finished
>> successfully at the time that of_platform_populate() returns.
>>
>> But fixing this problem would save us quite a bit of headache.
> 
> OK.
> 
> I will take a look at resurrecting the old patches either fixing the 
> probe order - or perhaps using something like Wesley's role-switch to 
> have dwc3 core optionally trigger dwc3-qcom
> 
> Binding tcpm into &usb_1_dwc3 instead of &usb_1
> 
> ---
> bod

So here's a potential way forward. Not technically breaking my "no 
patches at 3am rule"

Basically we can fix the probe order problem if we have dwc3 drd call 
into dwc3-qcom.

In order to make that not be a problem for all non qcom platforms - use 
a function with weak binding in drd - which a wrapper - in this case the 
qcom wrapper can over ride..

---
bod

[-- Attachment #2: 0001-usb-dwc3-Add-support-for-a-role-switch-notifier.patch --]
[-- Type: text/x-patch, Size: 2623 bytes --]

From 329ffacea542163fe7ac798b77db7cb3599a65ac Mon Sep 17 00:00:00 2001
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Date: Wed, 11 Mar 2020 19:15:00 +0000
Subject: [PATCH 1/2] usb: dwc3: Add support for a role-switch notifier

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.

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

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index dccdf13b5f9e..7f81ee3a9657 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -995,6 +995,7 @@ struct dwc3_scratchpad_array {
  * @role_sw: usb_role_switch handle
  * @role_switch_default_mode: default operation mode of controller while
  *			usb role is USB_ROLE_NONE.
+ * @role_sw_nl: role switch notifier list
  * @usb_psy: pointer to power supply interface.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
@@ -1136,6 +1137,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;
 	enum usb_dr_mode	role_switch_default_mode;
 
 	struct power_supply	*usb_psy;
@@ -1586,4 +1588,10 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
 { }
 #endif
 
+#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
+void __weak
+dwc3_set_parent_role_switch_notifier(struct device *parent,
+				     struct raw_notifier_head *role_sw_nl) {}
+#endif
+
 #endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 8fcbac10510c..7ae09730a319 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -507,6 +507,8 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
 	}
 
 	dwc3_set_mode(dwc, mode);
+	raw_notifier_call_chain(&dwc->role_sw_nl, mode, NULL);
+
 	return 0;
 }
 
@@ -563,6 +565,9 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 	if (IS_ERR(dwc->role_sw))
 		return PTR_ERR(dwc->role_sw);
 
+	RAW_INIT_NOTIFIER_HEAD(&dwc->role_sw_nl);
+	dwc3_set_parent_role_switch_notifier(dwc->dev->parent,
+					     &dwc->role_sw_nl);
 	dwc3_set_mode(dwc, mode);
 	return 0;
 }
-- 
2.30.1


[-- Attachment #3: 0002-usb-dwc3-qcom-Implement-VBUS-role-switch-notifier-ho.patch --]
[-- Type: text/x-patch, Size: 2417 bytes --]

From 9d2b50b448f7efd8b3891a2e8f52440794ed2958 Mon Sep 17 00:00:00 2001
From: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Date: Wed, 30 Jun 2021 03:10:49 +0100
Subject: [PATCH 2/2] usb: dwc3: qcom: Implement VBUS role-switch notifier
 hooks

Implement dwc3_set_parent_role_switch_notifier() in dwc3-qcom allowing the
core drd code to call into the parent device when it has successfully setup
all necessary role-switching logic.

The qcom-dwc3 binding reuses the existing VBUS notifier for extcon
receiving notifications from the role-switch layer instead of extcon.

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

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 49e6ca94486d..1b5aa345d025 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -81,6 +81,7 @@ struct dwc3_qcom {
 	struct extcon_dev	*host_edev;
 	struct notifier_block	vbus_nb;
 	struct notifier_block	host_nb;
+	struct raw_notifier_head *role_sw_nl;
 
 	const struct dwc3_acpi_pdata *acpi_pdata;
 
@@ -154,6 +155,20 @@ static int dwc3_qcom_host_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }
 
+#if IS_ENABLED(CONFIG_USB_ROLE_SWITCH)
+void dwc3_set_parent_role_switch_notifier(struct device *dev,
+					  struct raw_notifier_head *role_sw_nl)
+{
+	struct platform_device *pdev = container_of(dev,
+						    struct platform_device, dev);
+	struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
+
+	qcom->vbus_nb.notifier_call = dwc3_qcom_vbus_notifier;
+	if (!raw_notifier_chain_register(role_sw_nl, &qcom->vbus_nb))
+		qcom->role_sw_nl = role_sw_nl;
+}
+#endif
+
 static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
 {
 	struct device		*dev = qcom->dev;
@@ -829,6 +844,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 interconnect_exit:
 	dwc3_qcom_interconnect_exit(qcom);
 depopulate:
+	if (qcom->role_sw_nl)
+		raw_notifier_chain_unregister(qcom->role_sw_nl, &qcom->vbus_nb);
 	if (np)
 		of_platform_depopulate(&pdev->dev);
 	else
@@ -850,6 +867,9 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	if (qcom->role_sw_nl)
+		raw_notifier_chain_unregister(qcom->role_sw_nl, &qcom->vbus_nb);
+
 	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 
-- 
2.30.1

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Jack Pham]

On Wed, Jun 30, 2021 at 03:21:46AM +0100, Bryan O'Donoghue wrote:
> On 29/06/2021 22:57, Bryan O'Donoghue wrote:
> > On 29/06/2021 21:30, Bjorn Andersson wrote:
> > > I liked this, and it worked when I tested it, but iirc it suffered from
> > > the problem that the core's probe may or may not have finished
> > > successfully at the time that of_platform_populate() returns.
> > > 
> > > But fixing this problem would save us quite a bit of headache.
> > 
> > OK.
> > 
> > I will take a look at resurrecting the old patches either fixing the
> > probe order - or perhaps using something like Wesley's role-switch to
> > have dwc3 core optionally trigger dwc3-qcom
> > 
> > Binding tcpm into &usb_1_dwc3 instead of &usb_1
> > 
> > ---
> > bod
> 
> So here's a potential way forward. Not technically breaking my "no patches
> at 3am rule"
> 
> Basically we can fix the probe order problem if we have dwc3 drd call into
> dwc3-qcom.
> 
> In order to make that not be a problem for all non qcom platforms - use a
> function with weak binding in drd - which a wrapper - in this case the qcom
> wrapper can over ride..

I'm afraid I'm not too familiar with weak symbols. Would that still work
if dwc3 & dwc3-qcom are built as modules? Also is it supported with
Clang/LLVM?

Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 1/2] usb: dwc3: dwc3-qcom: Find USB connector and register role switch

[Bryan O'Donoghue]

On 01/07/2021 02:12, Jack Pham wrote:
> I'm afraid I'm not too familiar with weak symbols. Would that still work
> if dwc3 & dwc3-qcom are built as modules? Also is it supported with
> Clang/LLVM?

__weak would work fine, until you tried to have two strong implementations.

Your linker should choke if dwc3-meson-g12a, dwc3-qcom and dwc3-drd 
linked together with both wrappers implementing normal linkage.

However, I do think its possible to use role switching to have dwc3-drd 
trigger dwc3-qcom.

The role switch code is resilient to deferral so we don't have to solve 
the problem we had with get_drvdata() in the notifier solution and it 
gets us out of the business of having dwc3-qcom relay the role-switch 
onto the core - or indeed care about what sort of connector is attached.

dwc3-qcom shouldn't have to know or care what sort of connector is 
attached to it, ecros, gpio-typec, tcpm or a raw type-c driver and like 
Bjorn said, it shouldn't be the case that the wrapper relays onto the core.

Anyway I'm playing with that prototype now