LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
To: "Shah, Nehal-bakulchandra" <nehal-bakulchandra.shah@amd.com>
Cc: "Deucher, Alexander" <Alexander.Deucher@amd.com>,
	Felipe Balbi <balbi@kernel.org>,
	"gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Liu, Kun" <Kun.Liu2@amd.com>
Subject: Re: [RESEND PATCH 2/2] usb: dwc3: pci add property to allow user space role switch
Date: Thu, 9 Sep 2021 15:55:15 +0300	[thread overview]
Message-ID: <YToEM3NLebXmLNrY@kuha.fi.intel.com> (raw)
In-Reply-To: <a06b1b50-771c-312d-c91e-b6707c4b9401@amd.com>

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

  reply	other threads:[~2021-09-09 13:35 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-24 19:23 Nehal Bakulchandra Shah
2021-08-25  5:55 ` Felipe Balbi
2021-08-25  7:01   ` Heikki Krogerus
2021-08-25  7:26     ` Heikki Krogerus
2021-08-25 13:50       ` Deucher, Alexander
2021-08-26 14:06         ` Heikki Krogerus
2021-09-02 12:45           ` Shah, Nehal-bakulchandra
2021-09-09 12:55             ` Heikki Krogerus [this message]
2021-08-25  7:43     ` Felipe Balbi
2021-08-26 11:13 ` Greg KH

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YToEM3NLebXmLNrY@kuha.fi.intel.com \
    --to=heikki.krogerus@linux.intel.com \
    --cc=Alexander.Deucher@amd.com \
    --cc=Kun.Liu2@amd.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nehal-bakulchandra.shah@amd.com \
    --subject='Re: [RESEND PATCH 2/2] usb: dwc3: pci add property to allow user space role switch' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).