diff mbox series

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

Message ID 20240121204123.275441-4-lk@c--e.de (mailing list archive)
State Accepted
Commit f3be347ea42dbb0358cd8b2d8dc543a23b70a976
Headers show
Series UCSI fixes | expand

Commit Message

Christian A. Ehrhardt Jan. 21, 2024, 8:41 p.m. UTC 
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@c--e.de>
---
 drivers/usb/typec/ucsi/ucsi_acpi.c | 71 ++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

Comments

Heikki Krogerus Jan. 24, 2024, 8:11 a.m. UTC | #1 
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@c--e.de>

Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

> ---
>  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
diff mbox series

Patch

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))