diff mbox series

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

Message ID 20240213130018.3029991-3-heikki.krogerus@linux.intel.com
State New
Headers
Series platform/chrome: typec: xHCI DbC |

Commit Message

Heikki Krogerus Feb. 13, 2024, 1 p.m. UTC 
  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(+)

Comments

Prashant Malani Feb. 13, 2024, 4:55 p.m. UTC | #1 
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/
Heikki Krogerus Feb. 14, 2024, 10:22 a.m. UTC | #2 
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 Krogerus Feb. 14, 2024, 11:59 a.m. UTC | #3 
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,
Prashant Malani Feb. 14, 2024, 6:11 p.m. UTC | #4 
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,
diff mbox series

Patch

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;