Hi Heikki, Christian, > -----Original Message----- > From: Christian A. Ehrhardt <lk@xxxxxxx> > Sent: Saturday, August 31, 2024 3:40 AM > To: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx> > Cc: Gopal, Saranya <saranya.gopal@xxxxxxxxx>; linux- > usb@xxxxxxxxxxxxxxx; gregkh@xxxxxxxxxxxxxxxxxxx; Regupathy, > Rajaram <rajaram.regupathy@xxxxxxxxx> > Subject: Re: [PATCH] usb: typec: ucsi: Do not call ACPI _DSM method > for UCSI read operations > > > 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/ > > Heikki, I see that Christian's other patch is marked as a fix already (https://lore.kernel.org/linux-usb/20240906065853.637205-1-lk@xxxxxxx/T/#u). So, can this patch go in as it is? Please let me know if I need to resubmit with any changes. > > 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> Thanks for the review, Christian. Thanks, Saranya > > > > > > > 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
