Hi Heikki, Hi Saranya, On Fri, Aug 30, 2024 at 11:44:33AM +0300, Heikki Krogerus wrote: > On Fri, Aug 30, 2024 at 02:13:42PM +0530, Saranya Gopal wrote: > > ACPI _DSM methods are needed only for UCSI write operations and for reading > > CCI during RESET_PPM operation. So, remove _DSM calls from other places. > > While there, remove the Zenbook quirk also since the default behavior > > now aligns with the Zenbook quirk. With this change, GET_CONNECTOR_STATUS > > returns at least 6 seconds faster than before in Arrowlake-S platforms. > > > > Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > > Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx> > > Maybe this should be marked as a fix. I think this covers: > https://lore.kernel.org/linux-usb/20240829100109.562429-2-lk@xxxxxxx/ > > Christian, can you check this? The change certainly looks like the correct thing to do and would remove the need for the zenbook quirk. I'll try to get that combination tested by the original reporter of https://bugzilla.kernel.org/show_bug.cgi?id=219108 > > --- > > drivers/usb/typec/ucsi/ucsi_acpi.c | 56 +++--------------------------- > > 1 file changed, 5 insertions(+), 51 deletions(-) > > > > diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c > > index 7a5dff8d9cc6..accf15ff1306 100644 > > --- a/drivers/usb/typec/ucsi/ucsi_acpi.c > > +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c > > @@ -61,9 +61,11 @@ static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci) > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > int ret; > > > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > - if (ret) > > - return ret; > > + if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { > > + ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > + if (ret) > > + return ret; > > + } This is slightly incorrect because we wait for the completion of at least one other command (UCSI_SET_NOTIFICATION_ENABLE) by polling cci. However, this is a very minor corner case. It could be fixed by adding an optional ->poll() method or similar that is NULL on other implementations and does the DSM READ on ACPI. We could then call this before read_cci when polling for completion. If this is done ->read_cci() would never call the DSM method. However, the change in its current state is a definitive improvement, and looks good to me. Thus feel free to add Reviewed-by: Christian A. Ehrhardt <lk@xxxxxxx> > > > > memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); > > > > @@ -73,11 +75,6 @@ static int ucsi_acpi_read_cci(struct ucsi *ucsi, u32 *cci) > > static int ucsi_acpi_read_message_in(struct ucsi *ucsi, void *val, size_t val_len) > > { > > struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > - int ret; > > - > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > - if (ret) > > - return ret; > > > > memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len); > > > > @@ -102,42 +99,6 @@ static const struct ucsi_operations ucsi_acpi_ops = { > > .async_control = ucsi_acpi_async_control > > }; > > > > -static int > > -ucsi_zenbook_read_cci(struct ucsi *ucsi, u32 *cci) > > -{ > > - struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > - int ret; > > - > > - if (UCSI_COMMAND(ua->cmd) == UCSI_PPM_RESET) { > > - ret = ucsi_acpi_dsm(ua, UCSI_DSM_FUNC_READ); > > - if (ret) > > - return ret; > > - } > > - > > - memcpy(cci, ua->base + UCSI_CCI, sizeof(*cci)); > > - > > - return 0; > > -} > > - > > -static int > > -ucsi_zenbook_read_message_in(struct ucsi *ucsi, void *val, size_t val_len) > > -{ > > - struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi); > > - > > - /* UCSI_MESSAGE_IN is never read for PPM_RESET, return stored data */ > > - memcpy(val, ua->base + UCSI_MESSAGE_IN, val_len); > > - > > - return 0; > > -} > > - > > -static const struct ucsi_operations ucsi_zenbook_ops = { > > - .read_version = ucsi_acpi_read_version, > > - .read_cci = ucsi_zenbook_read_cci, > > - .read_message_in = ucsi_zenbook_read_message_in, > > - .sync_control = ucsi_sync_control_common, > > - .async_control = ucsi_acpi_async_control > > -}; > > - > > static int ucsi_gram_read_message_in(struct ucsi *ucsi, void *val, size_t val_len) > > { > > u16 bogus_change = UCSI_CONSTAT_POWER_LEVEL_CHANGE | > > @@ -190,13 +151,6 @@ static const struct ucsi_operations ucsi_gram_ops = { > > }; > > > > static const struct dmi_system_id ucsi_acpi_quirks[] = { > > - { > > - .matches = { > > - DMI_MATCH(DMI_SYS_VENDOR, "ASUSTeK COMPUTER INC."), > > - DMI_MATCH(DMI_PRODUCT_NAME, "ZenBook UX325UA_UM325UA"), > > - }, > > - .driver_data = (void *)&ucsi_zenbook_ops, > > - }, > > { > > .matches = { > > DMI_MATCH(DMI_SYS_VENDOR, "LG Electronics"), > > -- > > 2.34.1 Best regards, Christian
