[PATCH v2 0/2] platform/chrome: typec: xHCI DbC

[PATCH v2 0/2] platform/chrome: typec: xHCI DbC

[Heikki Krogerus]

Hi,

Changed in v2:

The quirk is now wrapped inside an ugly ifdef CONFIG_ACPI :(
I don't have better ideas better ideas for this I'm afraid.

Side note! I will be away for the next three weeks (plus a few days)
starting from Friday (Feb 16).


The original message:

In order to use xHCI DbC we need to allow the USB to be muxed to xHCI
even when the connector is in device role. That's because in DbC mode
the xHCI is the USB device controller.

In the first patch I'm just adding symlinks between the USB role
switches and their USB Type-C connectors. That way the user space can
find the correct role switch simply by following the symlink.

The second patch modifies cros_ec_typec.c. I'm assigning the PLD
(Physical Location of Device) hash of the port to the USB role switch
when it's missing from the ACPI tables. That should make sure the
first patch always works.


Heikki Krogerus (2):
  usb: roles: Link the switch to its connector
  platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

 .../ABI/testing/sysfs-class-usb_role          |  6 +++
 drivers/platform/chrome/cros_ec_typec.c       | 19 +++++++++
 drivers/usb/roles/class.c                     | 40 ++++++++++++++++++-
 3 files changed, 63 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH v2 1/2] usb: roles: Link the switch to its connector

[Heikki Krogerus]

This is probable useful information to have in user space in
general, but it's primarily needed for the xHCI DbC (Debug
Capability). When xHCI DbC is being used, the USB port needs
to be muxed to the xHCI even in device role. In xHCI DbC mode,
the xHCI is the USB device controller.

Tested-by: Uday Bhat <uday.m.bhat@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 .../ABI/testing/sysfs-class-usb_role          |  6 +++
 drivers/usb/roles/class.c                     | 40 ++++++++++++++++++-
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-class-usb_role b/Documentation/ABI/testing/sysfs-class-usb_role
index 3b810a425a52..9fab3f06679e 100644
--- a/Documentation/ABI/testing/sysfs-class-usb_role
+++ b/Documentation/ABI/testing/sysfs-class-usb_role
@@ -19,3 +19,9 @@ Description:
 		- none
 		- host
 		- device
+
+What:		/sys/class/usb_role/<switch>/connector
+Date:		Feb 2024
+Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
+Description:
+		Optional symlink to the USB Type-C connector.
diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
index ae41578bd014..4ad03c93c17f 100644
--- a/drivers/usb/roles/class.c
+++ b/drivers/usb/roles/class.c
@@ -7,6 +7,7 @@
  *         Hans de Goede <hdegoede@redhat.com>
  */
 
+#include <linux/component.h>
 #include <linux/usb/role.h>
 #include <linux/property.h>
 #include <linux/device.h>
@@ -34,6 +35,32 @@ struct usb_role_switch {
 
 #define to_role_switch(d)	container_of(d, struct usb_role_switch, dev)
 
+static int connector_bind(struct device *dev, struct device *connector, void *data)
+{
+	int ret;
+
+	ret = sysfs_create_link(&dev->kobj, &connector->kobj, "connector");
+	if (ret)
+		return ret;
+
+	ret = sysfs_create_link(&connector->kobj, &dev->kobj, "usb-role-switch");
+	if (ret)
+		sysfs_remove_link(&dev->kobj, "connector");
+
+	return ret;
+}
+
+static void connector_unbind(struct device *dev, struct device *connector, void *data)
+{
+	sysfs_remove_link(&connector->kobj, "usb-role-switch");
+	sysfs_remove_link(&dev->kobj, "connector");
+}
+
+static const struct component_ops connector_ops = {
+	.bind = connector_bind,
+	.unbind = connector_unbind,
+};
+
 /**
  * usb_role_switch_set_role - Set USB role for a switch
  * @sw: USB role switch
@@ -352,6 +379,12 @@ usb_role_switch_register(struct device *parent,
 		return ERR_PTR(ret);
 	}
 
+	if (dev_fwnode(&sw->dev)) {
+		ret = component_add(&sw->dev, &connector_ops);
+		if (ret)
+			dev_warn(&sw->dev, "failed to add component\n");
+	}
+
 	/* TODO: Symlinks for the host port and the device controller. */
 
 	return sw;
@@ -366,8 +399,11 @@ EXPORT_SYMBOL_GPL(usb_role_switch_register);
  */
 void usb_role_switch_unregister(struct usb_role_switch *sw)
 {
-	if (!IS_ERR_OR_NULL(sw))
-		device_unregister(&sw->dev);
+	if (IS_ERR_OR_NULL(sw))
+		return;
+	if (dev_fwnode(&sw->dev))
+		component_del(&sw->dev, &connector_ops);
+	device_unregister(&sw->dev);
 }
 EXPORT_SYMBOL_GPL(usb_role_switch_unregister);
 
-- 
2.43.0

[PATCH v2 2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

[Heikki Krogerus]

The USB role switch does not always have the _PLD (Physical
Location of Device) in ACPI tables. If it's missing,
assigning the PLD hash of the port to the switch. That
should guarantee that the USB Type-C port mapping code is
always able to find the connection between the two (the port
and the switch).

Tested-by: Uday Bhat <uday.m.bhat@intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/platform/chrome/cros_ec_typec.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
index 2b2f14a1b711..4d305876ec08 100644
--- a/drivers/platform/chrome/cros_ec_typec.c
+++ b/drivers/platform/chrome/cros_ec_typec.c
@@ -24,6 +24,23 @@
 #define DP_PORT_VDO	(DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
 				DP_CAP_DFP_D | DP_CAP_RECEPTACLE)
 
+static void cros_typec_role_switch_quirk(struct fwnode_handle *fwnode)
+{
+#ifdef CONFIG_ACPI
+	struct fwnode_handle *switch_fwnode;
+
+	/* Supply the USB role switch with the correct pld_crc if it's missing. */
+	switch_fwnode = fwnode_find_reference(fwnode, "usb-role-switch", 0);
+	if (!IS_ERR_OR_NULL(switch_fwnode)) {
+		struct acpi_device *adev = to_acpi_device_node(switch_fwnode);
+
+		if (adev && !adev->pld_crc)
+			adev->pld_crc = to_acpi_device_node(fwnode)->pld_crc;
+		fwnode_handle_put(switch_fwnode);
+	}
+#endif
+}
+
 static int cros_typec_parse_port_props(struct typec_capability *cap,
 				       struct fwnode_handle *fwnode,
 				       struct device *dev)
@@ -66,6 +83,8 @@ static int cros_typec_parse_port_props(struct typec_capability *cap,
 		cap->prefer_role = ret;
 	}
 
+	cros_typec_role_switch_quirk(fwnode);
+
 	cap->fwnode = fwnode;
 
 	return 0;
-- 
2.43.0

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

[Prashant Malani]

Hi Heikki,

On Tue, Feb 13, 2024 at 5:00 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> The USB role switch does not always have the _PLD (Physical
> Location of Device) in ACPI tables. If it's missing,
> assigning the PLD hash of the port to the switch. That
> should guarantee that the USB Type-C port mapping code is
> always able to find the connection between the two (the port
> and the switch).
>
> Tested-by: Uday Bhat <uday.m.bhat@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/platform/chrome/cros_ec_typec.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> index 2b2f14a1b711..4d305876ec08 100644
> --- a/drivers/platform/chrome/cros_ec_typec.c
> +++ b/drivers/platform/chrome/cros_ec_typec.c
> @@ -24,6 +24,23 @@
>  #define DP_PORT_VDO    (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
>                                 DP_CAP_DFP_D | DP_CAP_RECEPTACLE)
>
> +static void cros_typec_role_switch_quirk(struct fwnode_handle *fwnode)
> +{
> +#ifdef CONFIG_ACPI
> +       struct fwnode_handle *switch_fwnode;
> +
> +       /* Supply the USB role switch with the correct pld_crc if it's missing. */
> +       switch_fwnode = fwnode_find_reference(fwnode, "usb-role-switch", 0);
> +       if (!IS_ERR_OR_NULL(switch_fwnode)) {
> +               struct acpi_device *adev = to_acpi_device_node(switch_fwnode);
> +
> +               if (adev && !adev->pld_crc)
> +                       adev->pld_crc = to_acpi_device_node(fwnode)->pld_crc;
> +               fwnode_handle_put(switch_fwnode);
> +       }
> +#endif
> +}
> +

I'll reiterate my comment[ 1] from v1: can this be in the
common Type-C code, i.e typec_register_port() ?

I don't see anything in this implementation which is Chrome OS specific.

Thanks,

-Prashant

[1] https://lore.kernel.org/chrome-platform/CACeCKaffZBPA0Q_Bqs1hjKJB4HCj=VKrqO21dXj4AF5C5VwtVQ@mail.gmail.com/

Re: [PATCH v2 1/2] usb: roles: Link the switch to its connector

[Prashant Malani]

On Tue, Feb 13, 2024 at 5:00 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> This is probable useful information to have in user space in
> general, but it's primarily needed for the xHCI DbC (Debug
> Capability). When xHCI DbC is being used, the USB port needs
> to be muxed to the xHCI even in device role. In xHCI DbC mode,
> the xHCI is the USB device controller.
>
> Tested-by: Uday Bhat <uday.m.bhat@intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
FWIW:
Reviewed-by: Prashant Malani <pmalani@chromium.org>

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

[Heikki Krogerus]

On Tue, Feb 13, 2024 at 08:55:20AM -0800, Prashant Malani wrote:
> Hi Heikki,
> 
> On Tue, Feb 13, 2024 at 5:00 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
> >
> > The USB role switch does not always have the _PLD (Physical
> > Location of Device) in ACPI tables. If it's missing,
> > assigning the PLD hash of the port to the switch. That
> > should guarantee that the USB Type-C port mapping code is
> > always able to find the connection between the two (the port
> > and the switch).
> >
> > Tested-by: Uday Bhat <uday.m.bhat@intel.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > ---
> >  drivers/platform/chrome/cros_ec_typec.c | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c
> > index 2b2f14a1b711..4d305876ec08 100644
> > --- a/drivers/platform/chrome/cros_ec_typec.c
> > +++ b/drivers/platform/chrome/cros_ec_typec.c
> > @@ -24,6 +24,23 @@
> >  #define DP_PORT_VDO    (DP_CONF_SET_PIN_ASSIGN(BIT(DP_PIN_ASSIGN_C) | BIT(DP_PIN_ASSIGN_D)) | \
> >                                 DP_CAP_DFP_D | DP_CAP_RECEPTACLE)
> >
> > +static void cros_typec_role_switch_quirk(struct fwnode_handle *fwnode)
> > +{
> > +#ifdef CONFIG_ACPI
> > +       struct fwnode_handle *switch_fwnode;
> > +
> > +       /* Supply the USB role switch with the correct pld_crc if it's missing. */
> > +       switch_fwnode = fwnode_find_reference(fwnode, "usb-role-switch", 0);
> > +       if (!IS_ERR_OR_NULL(switch_fwnode)) {
> > +               struct acpi_device *adev = to_acpi_device_node(switch_fwnode);
> > +
> > +               if (adev && !adev->pld_crc)
> > +                       adev->pld_crc = to_acpi_device_node(fwnode)->pld_crc;
> > +               fwnode_handle_put(switch_fwnode);
> > +       }
> > +#endif
> > +}
> > +
> 
> I'll reiterate my comment[ 1] from v1: can this be in the
> common Type-C code, i.e typec_register_port() ?
>
> I don't see anything in this implementation which is Chrome OS specific.

I'm sorry Prashant, I failed to notice your comment.

This is only needed for Chrome OS. The problem affects only certain
Chromebooks.

I do not want to put quirks to the generic subsystem code unless we
have to. If there are more device drivers that need the same quirk,
then we can move it there, but not before that.

This solution is in any case a hack regardless of where we put it, and
I don't like it because of that. But I don't have any better ideas
unfortunately.

thanks,

-- 
heikki

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

[Heikki Krogerus]

Hi Emilie,

On Wed, Feb 14, 2024 at 12:04:22PM +0100, Emilie Roberts wrote:
> My understanding is that this is related to the wiring spec and not
> ChromeOS specific. It seems possible that OEMs making non-ChromeOS devices
> may have this same issue. Or are we certain that only Chromebooks will ever
> see this?

Non-ChromeOS platforms do not have this issue.

The issue is with the ACPI tables - the USB role switch ACPI device
nodes don't have the _PLD object on these systems. Ideally this could
be fixed there by simply adding the _PLD to those ACPI device objects,
but I understood that that is not an option.

But maybe I misunderstood... Can the ACPI tables on these platforms
still be updated?

thanks,

-- 
heikki

Re: [PATCH v2 2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD

[Prashant Malani]

Hi Heikki,

On Wed, Feb 14, 2024 at 3:59 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi Emilie,
>
> On Wed, Feb 14, 2024 at 12:04:22PM +0100, Emilie Roberts wrote:
> > My understanding is that this is related to the wiring spec and not
> > ChromeOS specific. It seems possible that OEMs making non-ChromeOS devices
> > may have this same issue. Or are we certain that only Chromebooks will ever
> > see this?
>
> Non-ChromeOS platforms do not have this issue.
>
> The issue is with the ACPI tables - the USB role switch ACPI device
> nodes don't have the _PLD object on these systems. Ideally this could
> be fixed there by simply adding the _PLD to those ACPI device objects,
> but I understood that that is not an option.
>
> But maybe I misunderstood... Can the ACPI tables on these platforms
> still be updated?

Since it's just a _PLD update to, it should be possible to do a "light" firmware
update on the relevant boards. Shyam/Emilie/Won, how practical is this?

I'd much prefer this to be fixed properly in the ACPI table than relying
on this quirk.

IAC, if we absolutely *have* to use this quirk:
Acked-by: Prashant Malani <pmalani@chromium.org>

Thanks,

Re: [PATCH v2 0/2] platform/chrome: typec: xHCI DbC

[patchwork-bot+chrome-platform]

Hello:

This series was applied to chrome-platform/linux.git (for-kernelci)
by Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

On Tue, 13 Feb 2024 15:00:16 +0200 you wrote:
> Hi,
> 
> Changed in v2:
> 
> The quirk is now wrapped inside an ugly ifdef CONFIG_ACPI :(
> I don't have better ideas better ideas for this I'm afraid.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] usb: roles: Link the switch to its connector
    https://git.kernel.org/chrome-platform/c/9a270ec7bfb0
  - [v2,2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD
    https://git.kernel.org/chrome-platform/c/56403220577b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH v2 0/2] platform/chrome: typec: xHCI DbC

[patchwork-bot+chrome-platform]

Hello:

This series was applied to chrome-platform/linux.git (for-next)
by Greg Kroah-Hartman <gregkh@linuxfoundation.org>:

On Tue, 13 Feb 2024 15:00:16 +0200 you wrote:
> Hi,
> 
> Changed in v2:
> 
> The quirk is now wrapped inside an ugly ifdef CONFIG_ACPI :(
> I don't have better ideas better ideas for this I'm afraid.
> 
> [...]

Here is the summary with links:
  - [v2,1/2] usb: roles: Link the switch to its connector
    https://git.kernel.org/chrome-platform/c/9a270ec7bfb0
  - [v2,2/2] platform/chrome: cros_ec_typec: Make sure the USB role switch has PLD
    https://git.kernel.org/chrome-platform/c/56403220577b

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html