[RESEND PATCH 2/2] usb: dwc3: pci add property to allow user space role switch

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

Signed-off-by: Nehal Bakulchandra Shah <redacted>
---
 drivers/usb/dwc3/drd.c      | 2 ++
 drivers/usb/dwc3/dwc3-pci.c | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 8fcbac10510c..6d579780ffcc 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c

@@ -555,6 +555,8 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 		mode = DWC3_GCTL_PRTCAP_DEVICE;
 	}
 
+	if (device_property_read_bool(dwc->dev, "allow-userspace-role-switch"))
+		dwc3_role_switch.allow_userspace_control = true;
 	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;

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 7ff8fc8f79a9..c1412a6e85b6 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c

@@ -153,6 +153,7 @@ static const struct property_entry dwc3_pci_mr_properties[] = {
 	PROPERTY_ENTRY_STRING("dr_mode", "otg"),
 	PROPERTY_ENTRY_BOOL("usb-role-switch"),
 	PROPERTY_ENTRY_STRING("role-switch-default-mode", "host"),
+	PROPERTY_ENTRY_BOOL("allow-userspace-role-switch"),
 	PROPERTY_ENTRY_BOOL("linux,sysdev_is_parent"),
 	{}
 };

-- 
2.25.1

Hi,

Nehal Bakulchandra Shah [off-list ref] writes:

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

                   usermode ?

Signed-off-by: Nehal Bakulchandra Shah <redacted>

I'm okay with this, just wondering if we need to Document the property
somewhere.

@Heikki, is there a place to document these private properties that's
not on DT binding document?

-- 
balbi

On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:

Hi,

Nehal Bakulchandra Shah [off-list ref] writes:

quoted

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

                   usermode ?

Couldn't you capture that ACPI/EC interrupt in kernel?

quoted

Signed-off-by: Nehal Bakulchandra Shah <redacted>

I'm okay with this, just wondering if we need to Document the property
somewhere.

@Heikki, is there a place to document these private properties that's
not on DT binding document?

The build-in properties are not documented separately. I've always
tried to supply DT bindings for all new properties I've proposed.

In this case though, do we need the new property at all? Why not just
register a normal USB role switch on this platform? It can be either a
dummy role switch that only passes the user space input to dwc3, or,
perhaps ideally, it would also be a driver that captures that ACPI/EC
event/notification and then passes the information from it to dwc3.

thanks,

-- 
heikki

On Wed, Aug 25, 2021 at 10:01:05AM +0300, Heikki Krogerus wrote:

On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:

quoted

Hi,

Nehal Bakulchandra Shah [off-list ref] writes:

quoted

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

                   usermode ?

Couldn't you capture that ACPI/EC interrupt in kernel?

quoted

quoted

Signed-off-by: Nehal Bakulchandra Shah <redacted>

I'm okay with this, just wondering if we need to Document the property
somewhere.

@Heikki, is there a place to document these private properties that's
not on DT binding document?

The build-in properties are not documented separately. I've always
tried to supply DT bindings for all new properties I've proposed.

In this case though, do we need the new property at all? Why not just
register a normal USB role switch on this platform? It can be either a
dummy role switch that only passes the user space input to dwc3, or,
perhaps ideally, it would also be a driver that captures that ACPI/EC
event/notification and then passes the information from it to dwc3.

Please ignore the above question. Sorry. Let me try again...

The question is: why not just capture that ACPI/EC "interrupt" in
kernel and then just use that information to set the dwc3 role switch?
No extra properties needed.


thanks,

-- 
heikki

[Public]

-----Original Message-----
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Sent: Wednesday, August 25, 2021 3:26 AM
To: Felipe Balbi <balbi@kernel.org>
Cc: Shah, Nehal-bakulchandra <redacted>;
gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
kernel@vger.kernel.org; Liu, Kun [off-list ref]; Deucher,
Alexander [off-list ref]
Subject: Re: [RESEND PATCH 2/2] usb: dwc3: pci add property to allow user
space role switch

On Wed, Aug 25, 2021 at 10:01:05AM +0300, Heikki Krogerus wrote:

quoted

On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:

quoted

Hi,

Nehal Bakulchandra Shah [off-list ref] writes:

quoted

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform
is not completely capable of OTG i.e. with type C controller it
does not have PD to support role switching. Hence, based ACPI/EC
interrupt role switch is triggered by the usemode script running in

background.

quoted

quoted

                   usermode ?

Couldn't you capture that ACPI/EC interrupt in kernel?

quoted

quoted

Signed-off-by: Nehal Bakulchandra Shah
[off-list ref]

I'm okay with this, just wondering if we need to Document the
property somewhere.

@Heikki, is there a place to document these private properties
that's not on DT binding document?

The build-in properties are not documented separately. I've always
tried to supply DT bindings for all new properties I've proposed.

In this case though, do we need the new property at all? Why not just
register a normal USB role switch on this platform? It can be either a
dummy role switch that only passes the user space input to dwc3, or,
perhaps ideally, it would also be a driver that captures that ACPI/EC
event/notification and then passes the information from it to dwc3.

Please ignore the above question. Sorry. Let me try again...

The question is: why not just capture that ACPI/EC "interrupt" in kernel and
then just use that information to set the dwc3 role switch?
No extra properties needed.

I'm not a USB expert, but I think the idea was to pop up a message asking the user what role they wanted when they plugged in USB cable?  Then based on their input, the role could be changed.

Alex

Hi Alexander,

On Wed, Aug 25, 2021 at 01:50:48PM +0000, Deucher, Alexander wrote:

I'm not a USB expert, but I think the idea was to pop up a message asking the
user what role they wanted when they plugged in USB cable?  Then based on
their input, the role could be changed.

What exactly is the ACPI/EC interrupt in this case?

Note, that simply selecting one role will only work if the partner
device happens to be in the opposite role at the same time (actually,
even that may not be enough). So for example by selecting host role
will only work if the partner happens to be in device role. If the
parter is also in host role, nothing happens, or both ends just fail
to enumerate each other.

So you always have to negotiate the role with the partner one way or
the other. Now we need to understand how that negotiation is handled
(or is expected to be handled) on this platform.

Which type of connector are we talking about here? Is it USB Type-C,
or is it something else?

thanks,

-- 
heikki

Hi Heikki,
On 8/26/2021 7:36 PM, Heikki Krogerus wrote:

Hi Alexander,

On Wed, Aug 25, 2021 at 01:50:48PM +0000, Deucher, Alexander wrote:

quoted

I'm not a USB expert, but I think the idea was to pop up a message asking the
user what role they wanted when they plugged in USB cable?  Then based on
their input, the role could be changed.

What exactly is the ACPI/EC interrupt in this case?

Note, that simply selecting one role will only work if the partner
device happens to be in the opposite role at the same time (actually,
even that may not be enough). So for example by selecting host role
will only work if the partner happens to be in device role. If the
parter is also in host role, nothing happens, or both ends just fail
to enumerate each other.

So you always have to negotiate the role with the partner one way or
the other. Now we need to understand how that negotiation is handled
(or is expected to be handled) on this platform.

Which type of connector are we talking about here? Is it USB Type-C,
or is it something else?

thanks,

Sorry for the delayed response due to few designed changes. Now we have 
more clarity for the customer platform with respect to usage of DWC3 
controller driver. So it is type C controller which will be using ACPI 
based UCSI driver. As UCSI driver has already role switch support we may 
not need this patch. However we need your input to understand this,

con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);

For this to work, how should be ACPI entry to be defined. Do you have 
sample code, we had discussion on similar point in past but still need 
some clarity if we have sample ACPI ASL Code. I remember something on 
this line from previous discussion with following sample code.

/*
  * I2C1 is the I2C host, and PDC1 is the USB PD Controller (I2C slave 
device).
  */
Scope (\_SB.PCI0.I2C1.PDC1)
{
         /* Each connector should have its own ACPI device entry (node). */
         Device (CON0)
         {
                 Name (_ADR, 0)

                 Name (_DSD, Package () {
                     ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                     Package() {
                         Package () {"usb-role-switch", \_SB.PCI0.DWC3},
                     }
                 })
         }
}

So here is the another question , if we can not achieve this in BIOS , 
Can we register the software node with quirk in DWC3 controller 
something like this

static const struct software_node amd_dwc3_node[] = {
	{ "amd-dwc3-usb-sw", NULL, amd_dwc3_props },
	{},
};

if (dwc->use_sw_node_quirk) {
		ret = software_node_register_nodes(amd_dwc3_node);
		if (ret)
			return ret;
		dwc3_role_switch.fwnode = software_node_fwnode(&amd_dwc3_node[0]);
	} else {
		dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
	}
	

And in UCSI driver again with quirk,

swnode = software_node_find_by_name(NULL, "amd-dwc3-usb-sw");

fwnode = software_node_fwnode(swnode);

con->usb_role_sw = usb_role_switch_find_by_fwnode(fwnode);


Please provide your input that will help us .

Regards
Nehal

Hi,

On Thu, Sep 02, 2021 at 06:15:55PM +0530, Shah, Nehal-bakulchandra wrote:

Hi Heikki,
On 8/26/2021 7:36 PM, Heikki Krogerus wrote:

quoted

Hi Alexander,

On Wed, Aug 25, 2021 at 01:50:48PM +0000, Deucher, Alexander wrote:

quoted

I'm not a USB expert, but I think the idea was to pop up a message asking the
user what role they wanted when they plugged in USB cable?  Then based on
their input, the role could be changed.

What exactly is the ACPI/EC interrupt in this case?

Note, that simply selecting one role will only work if the partner
device happens to be in the opposite role at the same time (actually,
even that may not be enough). So for example by selecting host role
will only work if the partner happens to be in device role. If the
parter is also in host role, nothing happens, or both ends just fail
to enumerate each other.

So you always have to negotiate the role with the partner one way or
the other. Now we need to understand how that negotiation is handled
(or is expected to be handled) on this platform.

Which type of connector are we talking about here? Is it USB Type-C,
or is it something else?

thanks,

Sorry for the delayed response due to few designed changes. Now we have more
clarity for the customer platform with respect to usage of DWC3 controller
driver. So it is type C controller which will be using ACPI based UCSI
driver. As UCSI driver has already role switch support we may not need this
patch. However we need your input to understand this,

con->usb_role_sw = fwnode_usb_role_switch_get(cap->fwnode);

For this to work, how should be ACPI entry to be defined. Do you have sample
code, we had discussion on similar point in past but still need some clarity
if we have sample ACPI ASL Code. I remember something on this line from
previous discussion with following sample code.

/*
 * I2C1 is the I2C host, and PDC1 is the USB PD Controller (I2C slave
device).
 */
Scope (\_SB.PCI0.I2C1.PDC1)
{
        /* Each connector should have its own ACPI device entry (node). */
        Device (CON0)
        {
                Name (_ADR, 0)

                Name (_DSD, Package () {
                    ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
                    Package() {
                        Package () {"usb-role-switch", \_SB.PCI0.DWC3},
                    }
                })
        }
}

In your case, the dwc3 is also the USB host controller, no?

The ACPI guys tell me that in ACPI we should rely on the _PLD
(Physical Location of Device) when determining to which USB Type-C
connector any given USB port (or any other port - like DP) is
connected to. Basically, the connector ACPI device node and the USB
port ACPI device node share the same _PLD, and that's how we know they
are connected. I'm already using that to create a symlink "connector"
for every USB port here: drivers/usb/typec/port-mapper.c

I'm not actually sure how did the ACPI guys think this will work with
USB device controllers, but if your controller is also the USB host
controller, then you will have a separate device node for every
port the host is controlling, and each of those will have the _PLD.

Can you send acpidump so I can take a look what you actually have
under your dwc3 ACPI device node?

        % acpidump -o my_acpi.dump

We most likely do need to update the fwnode_usb_role_switch_get() api
so that it also considers the _PLD, but your ACPI tables maybe
already OK (big maybe).

So here is the another question , if we can not achieve this in BIOS , Can
we register the software node with quirk in DWC3 controller something like
this

static const struct software_node amd_dwc3_node[] = {
	{ "amd-dwc3-usb-sw", NULL, amd_dwc3_props },
	{},
};

if (dwc->use_sw_node_quirk) {
		ret = software_node_register_nodes(amd_dwc3_node);
		if (ret)
			return ret;
		dwc3_role_switch.fwnode = software_node_fwnode(&amd_dwc3_node[0]);
	} else {
		dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
	}
	

And in UCSI driver again with quirk,

swnode = software_node_find_by_name(NULL, "amd-dwc3-usb-sw");

fwnode = software_node_fwnode(swnode);

con->usb_role_sw = usb_role_switch_find_by_fwnode(fwnode);


Please provide your input that will help us .

Let's first check if we can we use _PLD for this.


thanks,

-- 
heikki

Heikki Krogerus [off-list ref] writes:

On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:

quoted

Hi,

Nehal Bakulchandra Shah [off-list ref] writes:

quoted

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

                   usermode ?

Couldn't you capture that ACPI/EC interrupt in kernel?

quoted

quoted

Signed-off-by: Nehal Bakulchandra Shah <redacted>

I'm okay with this, just wondering if we need to Document the property
somewhere.

@Heikki, is there a place to document these private properties that's
not on DT binding document?

The build-in properties are not documented separately. I've always
tried to supply DT bindings for all new properties I've proposed.

In this case though, do we need the new property at all? Why not just
register a normal USB role switch on this platform? It can be either a
dummy role switch that only passes the user space input to dwc3, or,
perhaps ideally, it would also be a driver that captures that ACPI/EC
event/notification and then passes the information from it to dwc3.

I like the actual driver responding to EC IRQ idea.

-- 
balbi

On Wed, Aug 25, 2021 at 12:53:37AM +0530, Nehal Bakulchandra Shah wrote:

For AMD platform there is a requirement to enable user space role
switch from host to device and device to host as customer platform is not
completely capable of OTG i.e. with type C controller it does not have PD
to support role switching. Hence, based ACPI/EC interrupt role switch is
triggered by the usemode script running in background.

Signed-off-by: Nehal Bakulchandra Shah <redacted>
---
 drivers/usb/dwc3/drd.c      | 2 ++
 drivers/usb/dwc3/dwc3-pci.c | 1 +
 2 files changed, 3 insertions(+)

Why is just patch 2/2 resent?

confused,

greg k-h