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