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

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

[Nehal Bakulchandra Shah]

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 <Nehal-Bakulchandra.shah@amd.com>
---
 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

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

[Felipe Balbi]

Hi,

Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com> 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 <Nehal-Bakulchandra.shah@amd.com>

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

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

[Heikki Krogerus]

On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com> 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 ?

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

> > Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> 
> 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

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

[Heikki Krogerus]

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:
> > 
> > Hi,
> > 
> > Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com> 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 ?
> 
> Couldn't you capture that ACPI/EC interrupt in kernel?
> 
> > > Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
> > 
> > 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

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

[Felipe Balbi]

Heikki Krogerus <heikki.krogerus@linux.intel.com> writes:

> On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com> 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 ?
>
> Couldn't you capture that ACPI/EC interrupt in kernel?
>
>> > Signed-off-by: Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com>
>> 
>> 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

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

[Deucher, Alexander]

[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 <Nehal-bakulchandra.Shah@amd.com>;
> gregkh@linuxfoundation.org; linux-usb@vger.kernel.org; linux-
> kernel@vger.kernel.org; Liu, Kun <Kun.Liu2@amd.com>; Deucher,
> Alexander <Alexander.Deucher@amd.com>
> 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:
> > On Wed, Aug 25, 2021 at 08:55:41AM +0300, Felipe Balbi wrote:
> > >
> > > Hi,
> > >
> > > Nehal Bakulchandra Shah <Nehal-Bakulchandra.shah@amd.com> 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 ?
> >
> > Couldn't you capture that ACPI/EC interrupt in kernel?
> >
> > > > Signed-off-by: Nehal Bakulchandra Shah
> > > > <Nehal-Bakulchandra.shah@amd.com>
> > >
> > > 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

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

[Greg KH]

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 <Nehal-Bakulchandra.shah@amd.com>
> ---
>  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

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

[Heikki Krogerus]

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

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

[Shah, Nehal-bakulchandra]

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:
>> 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

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

[Heikki Krogerus]

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:
> > 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,
> > 
> 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