On Sun, Jan 21, 2024 at 09:41:23PM +0100, Christian A. Ehrhardt wrote:
> The PPM on some Dell laptops seems to expect that the ACK_CC_CI
> command to clear the connector change notification is in turn
> followed by another ACK_CC_CI to acknowledge the ACK_CC_CI command
> itself. This is in violation of the UCSI spec that states:
>
> "The only notification that is not acknowledged by the OPM is
> the command completion notification for the ACK_CC_CI or the
> PPM_RESET command."
>
> Add a quirk to send this ack anyway.
> Apply the quirk to all Dell systems.
>
> On the first command that acks a connector change send a dummy
> command to determine if it runs into a timeout. Only activate
> the quirk if it does. This ensure that we do not break Dell
> systems that do not need the quirk.
>
> Signed-off-by: Christian A. Ehrhardt <lk@xxxxxxx>
Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> ---
> drivers/usb/typec/ucsi/ucsi_acpi.c | 71 ++++++++++++++++++++++++++++--
> 1 file changed, 68 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index fa222080887d..928eacbeb21a 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -25,6 +25,8 @@ struct ucsi_acpi {
> unsigned long flags;
> guid_t guid;
> u64 cmd;
> + bool dell_quirk_probed;
> + bool dell_quirk_active;
> };
>
> static int ucsi_acpi_dsm(struct ucsi_acpi *ua, int func)
> @@ -126,12 +128,73 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
> .async_write = ucsi_acpi_async_write
> };
>
> -static const struct dmi_system_id zenbook_dmi_id[] = {
> +/*
> + * Some Dell laptops expect that an ACK command with the
> + * UCSI_ACK_CONNECTOR_CHANGE bit set is followed by a (separate)
> + * ACK command that only has the UCSI_ACK_COMMAND_COMPLETE bit set.
> + * If this is not done events are not delivered to OSPM and
> + * subsequent commands will timeout.
> + */
> +static int
> +ucsi_dell_sync_write(struct ucsi *ucsi, unsigned int offset,
> + const void *val, size_t val_len)
> +{
> + struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> + u64 cmd = *(u64 *)val, ack = 0;
> + int ret;
> +
> + if (UCSI_COMMAND(cmd) == UCSI_ACK_CC_CI &&
> + cmd & UCSI_ACK_CONNECTOR_CHANGE)
> + ack = UCSI_ACK_CC_CI | UCSI_ACK_COMMAND_COMPLETE;
> +
> + ret = ucsi_acpi_sync_write(ucsi, offset, val, val_len);
> + if (ret != 0)
> + return ret;
> + if (ack == 0)
> + return ret;
> +
> + if (!ua->dell_quirk_probed) {
> + ua->dell_quirk_probed = true;
> +
> + cmd = UCSI_GET_CAPABILITY;
> + ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd,
> + sizeof(cmd));
> + if (ret == 0)
> + return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL,
> + &ack, sizeof(ack));
> + if (ret != -ETIMEDOUT)
> + return ret;
> +
> + ua->dell_quirk_active = true;
> + dev_err(ua->dev, "Firmware bug: Additional ACK required after ACKing a connector change.\n");
> + dev_err(ua->dev, "Firmware bug: Enabling workaround\n");
> + }
> +
> + if (!ua->dell_quirk_active)
> + return ret;
> +
> + return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &ack, sizeof(ack));
> +}
> +
> +static const struct ucsi_operations ucsi_dell_ops = {
> + .read = ucsi_acpi_read,
> + .sync_write = ucsi_dell_sync_write,
> + .async_write = ucsi_acpi_async_write
> +};
> +
> +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, "Dell Inc."),
> + },
> + .driver_data = (void *)&ucsi_dell_ops,
> },
> { }
> };
> @@ -160,6 +223,7 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
> {
> struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> const struct ucsi_operations *ops = &ucsi_acpi_ops;
> + const struct dmi_system_id *id;
> struct ucsi_acpi *ua;
> struct resource *res;
> acpi_status status;
> @@ -189,8 +253,9 @@ static int ucsi_acpi_probe(struct platform_device *pdev)
> init_completion(&ua->complete);
> ua->dev = &pdev->dev;
>
> - if (dmi_check_system(zenbook_dmi_id))
> - ops = &ucsi_zenbook_ops;
> + id = dmi_first_match(ucsi_acpi_quirks);
> + if (id)
> + ops = id->driver_data;
>
> ua->ucsi = ucsi_create(&pdev->dev, ops);
> if (IS_ERR(ua->ucsi))
> --
> 2.40.1
--
heikki
