Re: [PATCH v3 3/3] usb: ucsi_acpi: Quirk to ack a connector change ack cmd

[Heikki Krogerus]

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