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

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/

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