| Message ID | 20240213130018.3029991-3-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 56403220577b95276b8080e8480adc7e1f9d43ec |
| Headers | show |
| Series | platform/chrome: typec: xHCI DbC | expand |
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,
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,
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 --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;