| Message ID | 20210824192337.3100288-1-Nehal-Bakulchandra.shah@amd.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
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 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.
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
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: >> 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: > > 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
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"), {} };
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(+)