* [PATCH 0/3] usb: typec: ucsi: Ack connector change early
@ 2024-03-27 22:45 Christian A. Ehrhardt
2024-03-27 22:45 ` [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h Christian A. Ehrhardt
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Christian A. Ehrhardt @ 2024-03-27 22:45 UTC (permalink / raw
To: linux-kernel, Dmitry Baryshkov, Diogo Ivo
Cc: Christian A. Ehrhardt, Heikki Krogerus, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
As briefly discussed here
https://lore.kernel.org/lkml/Zf1XUrG1UbVJWzoz@kuha.fi.intel.com/
acknowledge connector change events along with the first command
in ucsi_handle_connector_change(). The connector lock should be
sufficient to protect the rest of the function and the partner
tasks.
This allows us to remove the Dell quirk in ucsi_acpi.c.
Additionally, this reduces the number of commands that are sent
with an un-acknowleged connector change event.
Christian A. Ehrhardt (3):
usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h
usb: typec: ucsi: Never send a lone connector change ack
usb: typec: ucsi_acpi: Remove Dell quirk
drivers/usb/typec/ucsi/ucsi.c | 48 ++++++++++-------------
drivers/usb/typec/ucsi/ucsi.h | 2 -
drivers/usb/typec/ucsi/ucsi_acpi.c | 56 ++-------------------------
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
4 files changed, 25 insertions(+), 82 deletions(-)
--
2.40.1
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h
2024-03-27 22:45 [PATCH 0/3] usb: typec: ucsi: Ack connector change early Christian A. Ehrhardt
@ 2024-03-27 22:45 ` Christian A. Ehrhardt
2024-04-03 10:39 ` Heikki Krogerus
2024-03-27 22:45 ` [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack Christian A. Ehrhardt
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Christian A. Ehrhardt @ 2024-03-27 22:45 UTC (permalink / raw
To: linux-kernel, Dmitry Baryshkov, Diogo Ivo
Cc: Christian A. Ehrhardt, Heikki Krogerus, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
In ucsi.h there are flag definitions for the ->flags field
in struct ucsi. Some implementations abuse these bits for
their private ->flags fields e.g. in struct ucsi_acpi.
Move the definitions into the backend implementations that
still need them. While there fix one instance where the flag
name was not converted in a previous change.
No semantic change intended.
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
drivers/usb/typec/ucsi/ucsi.h | 2 --
drivers/usb/typec/ucsi/ucsi_acpi.c | 3 ++-
drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
index 0e7c92eb1b22..591f734d457b 100644
--- a/drivers/usb/typec/ucsi/ucsi.h
+++ b/drivers/usb/typec/ucsi/ucsi.h
@@ -403,8 +403,6 @@ struct ucsi {
/* PPM communication flags */
unsigned long flags;
#define EVENT_PENDING 0
-#define COMMAND_PENDING 1
-#define ACK_PENDING 2
unsigned long quirks;
#define UCSI_NO_PARTNER_PDOS BIT(0) /* Don't read partner's PDOs */
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index 7b3ac133ef86..cc03a49c589c 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -203,7 +203,8 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
!test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
- if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &ua->flags))
+ if (cci & UCSI_CCI_ACK_COMPLETE &&
+ test_bit(UCSI_ACPI_ACK_PENDING, &ua->flags))
complete(&ua->complete);
if (cci & UCSI_CCI_COMMAND_COMPLETE &&
test_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags))
diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
index 93d7806681cf..ac48b7763114 100644
--- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
+++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
@@ -64,6 +64,7 @@ struct ucsi_stm32g0 {
struct completion complete;
struct device *dev;
unsigned long flags;
+#define COMMAND_PENDING 1
const char *fw_name;
struct ucsi *ucsi;
bool suspended;
--
2.40.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack
2024-03-27 22:45 [PATCH 0/3] usb: typec: ucsi: Ack connector change early Christian A. Ehrhardt
2024-03-27 22:45 ` [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h Christian A. Ehrhardt
@ 2024-03-27 22:45 ` Christian A. Ehrhardt
2024-04-03 10:40 ` Heikki Krogerus
2024-03-27 22:45 ` [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk Christian A. Ehrhardt
2024-03-29 5:56 ` [PATCH 0/3] usb: typec: ucsi: Ack connector change early Dmitry Baryshkov
3 siblings, 1 reply; 8+ messages in thread
From: Christian A. Ehrhardt @ 2024-03-27 22:45 UTC (permalink / raw
To: linux-kernel, Dmitry Baryshkov, Diogo Ivo
Cc: Christian A. Ehrhardt, Heikki Krogerus, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
Some PPM implementation do not like UCSI_ACK_CONNECTOR_CHANGE
without UCSI_ACK_COMMAND_COMPLETE. Moreover, doing this is racy
as it requires sending two UCSI_ACK_CC_CI commands in a row and
the second one will be started with UCSI_CCI_ACK_COMPLETE already
set in CCI.
Bundle the UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
for the UCSI_GET_CONNECTOR_STATUS command that is sent while
handling a connector change event.
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
drivers/usb/typec/ucsi/ucsi.c | 48 +++++++++++++++--------------------
1 file changed, 21 insertions(+), 27 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
index 31d8a46ae5e7..48f093a1dc09 100644
--- a/drivers/usb/typec/ucsi/ucsi.c
+++ b/drivers/usb/typec/ucsi/ucsi.c
@@ -49,22 +49,16 @@ static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
}
-static int ucsi_acknowledge_command(struct ucsi *ucsi)
+static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
{
u64 ctrl;
ctrl = UCSI_ACK_CC_CI;
ctrl |= UCSI_ACK_COMMAND_COMPLETE;
-
- return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
-}
-
-static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
-{
- u64 ctrl;
-
- ctrl = UCSI_ACK_CC_CI;
- ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
+ if (conn_ack) {
+ clear_bit(EVENT_PENDING, &ucsi->flags);
+ ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
+ }
return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
}
@@ -77,7 +71,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
int ret;
/* Acknowledge the command that failed */
- ret = ucsi_acknowledge_command(ucsi);
+ ret = ucsi_acknowledge(ucsi, false);
if (ret)
return ret;
@@ -89,7 +83,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
if (ret)
return ret;
- ret = ucsi_acknowledge_command(ucsi);
+ ret = ucsi_acknowledge(ucsi, false);
if (ret)
return ret;
@@ -152,7 +146,7 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
return -EIO;
if (cci & UCSI_CCI_NOT_SUPPORTED) {
- if (ucsi_acknowledge_command(ucsi) < 0)
+ if (ucsi_acknowledge(ucsi, false) < 0)
dev_err(ucsi->dev,
"ACK of unsupported command failed\n");
return -EOPNOTSUPP;
@@ -165,15 +159,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
}
if (cmd == UCSI_CANCEL && cci & UCSI_CCI_CANCEL_COMPLETE) {
- ret = ucsi_acknowledge_command(ucsi);
+ ret = ucsi_acknowledge(ucsi, false);
return ret ? ret : -EBUSY;
}
return UCSI_CCI_LENGTH(cci);
}
-int ucsi_send_command(struct ucsi *ucsi, u64 command,
- void *data, size_t size)
+static int ucsi_send_command_common(struct ucsi *ucsi, u64 command,
+ void *data, size_t size, bool conn_ack)
{
u8 length;
int ret;
@@ -192,7 +186,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
goto out;
}
- ret = ucsi_acknowledge_command(ucsi);
+ ret = ucsi_acknowledge(ucsi, conn_ack);
if (ret)
goto out;
@@ -201,6 +195,12 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
mutex_unlock(&ucsi->ppm_lock);
return ret;
}
+
+int ucsi_send_command(struct ucsi *ucsi, u64 command,
+ void *data, size_t size)
+{
+ return ucsi_send_command_common(ucsi, command, data, size, false);
+}
EXPORT_SYMBOL_GPL(ucsi_send_command);
/* -------------------------------------------------------------------------- */
@@ -1168,7 +1168,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
mutex_lock(&con->lock);
command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
- ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status));
+
+ ret = ucsi_send_command_common(ucsi, command, &con->status,
+ sizeof(con->status), true);
if (ret < 0) {
dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
__func__, ret);
@@ -1225,14 +1227,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
- mutex_lock(&ucsi->ppm_lock);
- clear_bit(EVENT_PENDING, &con->ucsi->flags);
- ret = ucsi_acknowledge_connector_change(ucsi);
- mutex_unlock(&ucsi->ppm_lock);
-
- if (ret)
- dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
-
out_unlock:
mutex_unlock(&con->lock);
}
--
2.40.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk
2024-03-27 22:45 [PATCH 0/3] usb: typec: ucsi: Ack connector change early Christian A. Ehrhardt
2024-03-27 22:45 ` [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h Christian A. Ehrhardt
2024-03-27 22:45 ` [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack Christian A. Ehrhardt
@ 2024-03-27 22:45 ` Christian A. Ehrhardt
2024-04-03 10:40 ` Heikki Krogerus
2024-03-29 5:56 ` [PATCH 0/3] usb: typec: ucsi: Ack connector change early Dmitry Baryshkov
3 siblings, 1 reply; 8+ messages in thread
From: Christian A. Ehrhardt @ 2024-03-27 22:45 UTC (permalink / raw
To: linux-kernel, Dmitry Baryshkov, Diogo Ivo
Cc: Christian A. Ehrhardt, Heikki Krogerus, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
The Dell quirk from ucsi_acpi.c. The quirk is no longer
necessary as we no longer send lone connector change acks.
Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
---
drivers/usb/typec/ucsi/ucsi_acpi.c | 53 +-----------------------------
1 file changed, 1 insertion(+), 52 deletions(-)
diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
index cc03a49c589c..8d112c3edae5 100644
--- a/drivers/usb/typec/ucsi/ucsi_acpi.c
+++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
@@ -23,7 +23,6 @@ struct ucsi_acpi {
void *base;
struct completion complete;
unsigned long flags;
-#define UCSI_ACPI_SUPPRESS_EVENT 0
#define UCSI_ACPI_COMMAND_PENDING 1
#define UCSI_ACPI_ACK_PENDING 2
guid_t guid;
@@ -129,49 +128,6 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
.async_write = ucsi_acpi_async_write
};
-/*
- * Some Dell laptops don't like ACK commands with the
- * UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
- * bit set. To work around this send a dummy command and bundle the
- * UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
- * for the dummy command.
- */
-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;
- u64 dummycmd = UCSI_GET_CAPABILITY;
- int ret;
-
- if (cmd == (UCSI_ACK_CC_CI | UCSI_ACK_CONNECTOR_CHANGE)) {
- cmd |= UCSI_ACK_COMMAND_COMPLETE;
-
- /*
- * The UCSI core thinks it is sending a connector change ack
- * and will accept new connector change events. We don't want
- * this to happen for the dummy command as its response will
- * still report the very event that the core is trying to clear.
- */
- set_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
- ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &dummycmd,
- sizeof(dummycmd));
- clear_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
-
- if (ret < 0)
- return ret;
- }
-
- return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
-}
-
-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 = {
@@ -180,12 +136,6 @@ static const struct dmi_system_id ucsi_acpi_quirks[] = {
},
.driver_data = (void *)&ucsi_zenbook_ops,
},
- {
- .matches = {
- DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
- },
- .driver_data = (void *)&ucsi_dell_ops,
- },
{ }
};
@@ -199,8 +149,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
if (ret)
return;
- if (UCSI_CCI_CONNECTOR(cci) &&
- !test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
+ if (UCSI_CCI_CONNECTOR(cci))
ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
if (cci & UCSI_CCI_ACK_COMPLETE &&
--
2.40.1
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] usb: typec: ucsi: Ack connector change early
2024-03-27 22:45 [PATCH 0/3] usb: typec: ucsi: Ack connector change early Christian A. Ehrhardt
` (2 preceding siblings ...)
2024-03-27 22:45 ` [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk Christian A. Ehrhardt
@ 2024-03-29 5:56 ` Dmitry Baryshkov
3 siblings, 0 replies; 8+ messages in thread
From: Dmitry Baryshkov @ 2024-03-29 5:56 UTC (permalink / raw
To: Christian A. Ehrhardt
Cc: linux-kernel, Diogo Ivo, Heikki Krogerus, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
On Thu, 28 Mar 2024 at 00:46, Christian A. Ehrhardt <lk@c--e.de> wrote:
>
> As briefly discussed here
> https://lore.kernel.org/lkml/Zf1XUrG1UbVJWzoz@kuha.fi.intel.com/
> acknowledge connector change events along with the first command
> in ucsi_handle_connector_change(). The connector lock should be
> sufficient to protect the rest of the function and the partner
> tasks.
>
> This allows us to remove the Dell quirk in ucsi_acpi.c.
> Additionally, this reduces the number of commands that are sent
> with an un-acknowleged connector change event.
>
> Christian A. Ehrhardt (3):
> usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h
> usb: typec: ucsi: Never send a lone connector change ack
> usb: typec: ucsi_acpi: Remove Dell quirk
>
> drivers/usb/typec/ucsi/ucsi.c | 48 ++++++++++-------------
> drivers/usb/typec/ucsi/ucsi.h | 2 -
> drivers/usb/typec/ucsi/ucsi_acpi.c | 56 ++-------------------------
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
> 4 files changed, 25 insertions(+), 82 deletions(-)
Tested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> #
SM8450-HDK, sc8180x-primus
--
With best wishes
Dmitry
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h
2024-03-27 22:45 ` [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h Christian A. Ehrhardt
@ 2024-04-03 10:39 ` Heikki Krogerus
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Krogerus @ 2024-04-03 10:39 UTC (permalink / raw
To: Christian A. Ehrhardt
Cc: linux-kernel, Dmitry Baryshkov, Diogo Ivo, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
On Wed, Mar 27, 2024 at 11:45:52PM +0100, Christian A. Ehrhardt wrote:
> In ucsi.h there are flag definitions for the ->flags field
> in struct ucsi. Some implementations abuse these bits for
> their private ->flags fields e.g. in struct ucsi_acpi.
>
> Move the definitions into the backend implementations that
> still need them. While there fix one instance where the flag
> name was not converted in a previous change.
>
> No semantic change intended.
>
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/ucsi/ucsi.h | 2 --
> drivers/usb/typec/ucsi/ucsi_acpi.c | 3 ++-
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 0e7c92eb1b22..591f734d457b 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -403,8 +403,6 @@ struct ucsi {
> /* PPM communication flags */
> unsigned long flags;
> #define EVENT_PENDING 0
> -#define COMMAND_PENDING 1
> -#define ACK_PENDING 2
>
> unsigned long quirks;
> #define UCSI_NO_PARTNER_PDOS BIT(0) /* Don't read partner's PDOs */
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index 7b3ac133ef86..cc03a49c589c 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -203,7 +203,8 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
> !test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
> ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
>
> - if (cci & UCSI_CCI_ACK_COMPLETE && test_bit(ACK_PENDING, &ua->flags))
> + if (cci & UCSI_CCI_ACK_COMPLETE &&
> + test_bit(UCSI_ACPI_ACK_PENDING, &ua->flags))
> complete(&ua->complete);
> if (cci & UCSI_CCI_COMMAND_COMPLETE &&
> test_bit(UCSI_ACPI_COMMAND_PENDING, &ua->flags))
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 93d7806681cf..ac48b7763114 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -64,6 +64,7 @@ struct ucsi_stm32g0 {
> struct completion complete;
> struct device *dev;
> unsigned long flags;
> +#define COMMAND_PENDING 1
> const char *fw_name;
> struct ucsi *ucsi;
> bool suspended;
> --
> 2.40.1
--
heikki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack
2024-03-27 22:45 ` [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack Christian A. Ehrhardt
@ 2024-04-03 10:40 ` Heikki Krogerus
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Krogerus @ 2024-04-03 10:40 UTC (permalink / raw
To: Christian A. Ehrhardt
Cc: linux-kernel, Dmitry Baryshkov, Diogo Ivo, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
On Wed, Mar 27, 2024 at 11:45:53PM +0100, Christian A. Ehrhardt wrote:
> Some PPM implementation do not like UCSI_ACK_CONNECTOR_CHANGE
> without UCSI_ACK_COMMAND_COMPLETE. Moreover, doing this is racy
> as it requires sending two UCSI_ACK_CC_CI commands in a row and
> the second one will be started with UCSI_CCI_ACK_COMPLETE already
> set in CCI.
>
> Bundle the UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
> for the UCSI_GET_CONNECTOR_STATUS command that is sent while
> handling a connector change event.
>
> Signed-off-by: Christian A. Ehrhardt <lk@c--e.de>
Reviewed-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 48 +++++++++++++++--------------------
> 1 file changed, 21 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 31d8a46ae5e7..48f093a1dc09 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -49,22 +49,16 @@ static int ucsi_read_message_in(struct ucsi *ucsi, void *buf,
> return ucsi->ops->read(ucsi, UCSI_MESSAGE_IN, buf, buf_size);
> }
>
> -static int ucsi_acknowledge_command(struct ucsi *ucsi)
> +static int ucsi_acknowledge(struct ucsi *ucsi, bool conn_ack)
> {
> u64 ctrl;
>
> ctrl = UCSI_ACK_CC_CI;
> ctrl |= UCSI_ACK_COMMAND_COMPLETE;
> -
> - return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> -}
> -
> -static int ucsi_acknowledge_connector_change(struct ucsi *ucsi)
> -{
> - u64 ctrl;
> -
> - ctrl = UCSI_ACK_CC_CI;
> - ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
> + if (conn_ack) {
> + clear_bit(EVENT_PENDING, &ucsi->flags);
> + ctrl |= UCSI_ACK_CONNECTOR_CHANGE;
> + }
>
> return ucsi->ops->sync_write(ucsi, UCSI_CONTROL, &ctrl, sizeof(ctrl));
> }
> @@ -77,7 +71,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
> int ret;
>
> /* Acknowledge the command that failed */
> - ret = ucsi_acknowledge_command(ucsi);
> + ret = ucsi_acknowledge(ucsi, false);
> if (ret)
> return ret;
>
> @@ -89,7 +83,7 @@ static int ucsi_read_error(struct ucsi *ucsi)
> if (ret)
> return ret;
>
> - ret = ucsi_acknowledge_command(ucsi);
> + ret = ucsi_acknowledge(ucsi, false);
> if (ret)
> return ret;
>
> @@ -152,7 +146,7 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
> return -EIO;
>
> if (cci & UCSI_CCI_NOT_SUPPORTED) {
> - if (ucsi_acknowledge_command(ucsi) < 0)
> + if (ucsi_acknowledge(ucsi, false) < 0)
> dev_err(ucsi->dev,
> "ACK of unsupported command failed\n");
> return -EOPNOTSUPP;
> @@ -165,15 +159,15 @@ static int ucsi_exec_command(struct ucsi *ucsi, u64 cmd)
> }
>
> if (cmd == UCSI_CANCEL && cci & UCSI_CCI_CANCEL_COMPLETE) {
> - ret = ucsi_acknowledge_command(ucsi);
> + ret = ucsi_acknowledge(ucsi, false);
> return ret ? ret : -EBUSY;
> }
>
> return UCSI_CCI_LENGTH(cci);
> }
>
> -int ucsi_send_command(struct ucsi *ucsi, u64 command,
> - void *data, size_t size)
> +static int ucsi_send_command_common(struct ucsi *ucsi, u64 command,
> + void *data, size_t size, bool conn_ack)
> {
> u8 length;
> int ret;
> @@ -192,7 +186,7 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
> goto out;
> }
>
> - ret = ucsi_acknowledge_command(ucsi);
> + ret = ucsi_acknowledge(ucsi, conn_ack);
> if (ret)
> goto out;
>
> @@ -201,6 +195,12 @@ int ucsi_send_command(struct ucsi *ucsi, u64 command,
> mutex_unlock(&ucsi->ppm_lock);
> return ret;
> }
> +
> +int ucsi_send_command(struct ucsi *ucsi, u64 command,
> + void *data, size_t size)
> +{
> + return ucsi_send_command_common(ucsi, command, data, size, false);
> +}
> EXPORT_SYMBOL_GPL(ucsi_send_command);
>
> /* -------------------------------------------------------------------------- */
> @@ -1168,7 +1168,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> mutex_lock(&con->lock);
>
> command = UCSI_GET_CONNECTOR_STATUS | UCSI_CONNECTOR_NUMBER(con->num);
> - ret = ucsi_send_command(ucsi, command, &con->status, sizeof(con->status));
> +
> + ret = ucsi_send_command_common(ucsi, command, &con->status,
> + sizeof(con->status), true);
> if (ret < 0) {
> dev_err(ucsi->dev, "%s: GET_CONNECTOR_STATUS failed (%d)\n",
> __func__, ret);
> @@ -1225,14 +1227,6 @@ static void ucsi_handle_connector_change(struct work_struct *work)
> if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
> ucsi_partner_task(con, ucsi_check_altmodes, 1, 0);
>
> - mutex_lock(&ucsi->ppm_lock);
> - clear_bit(EVENT_PENDING, &con->ucsi->flags);
> - ret = ucsi_acknowledge_connector_change(ucsi);
> - mutex_unlock(&ucsi->ppm_lock);
> -
> - if (ret)
> - dev_err(ucsi->dev, "%s: ACK failed (%d)", __func__, ret);
> -
> out_unlock:
> mutex_unlock(&con->lock);
> }
> --
> 2.40.1
--
heikki
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk
2024-03-27 22:45 ` [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk Christian A. Ehrhardt
@ 2024-04-03 10:40 ` Heikki Krogerus
0 siblings, 0 replies; 8+ messages in thread
From: Heikki Krogerus @ 2024-04-03 10:40 UTC (permalink / raw
To: Christian A. Ehrhardt
Cc: linux-kernel, Dmitry Baryshkov, Diogo Ivo, Greg Kroah-Hartman,
Prashant Malani, Jameson Thies, Abhishek Pandit-Subedi,
Neil Armstrong, Uwe Kleine-König, Samuel Čavoj,
linux-usb
On Wed, Mar 27, 2024 at 11:45:54PM +0100, Christian A. Ehrhardt wrote:
> The Dell quirk from ucsi_acpi.c. The quirk is no longer
> necessary as we no longer send lone connector change acks.
>
> 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 | 53 +-----------------------------
> 1 file changed, 1 insertion(+), 52 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index cc03a49c589c..8d112c3edae5 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -23,7 +23,6 @@ struct ucsi_acpi {
> void *base;
> struct completion complete;
> unsigned long flags;
> -#define UCSI_ACPI_SUPPRESS_EVENT 0
> #define UCSI_ACPI_COMMAND_PENDING 1
> #define UCSI_ACPI_ACK_PENDING 2
> guid_t guid;
> @@ -129,49 +128,6 @@ static const struct ucsi_operations ucsi_zenbook_ops = {
> .async_write = ucsi_acpi_async_write
> };
>
> -/*
> - * Some Dell laptops don't like ACK commands with the
> - * UCSI_ACK_CONNECTOR_CHANGE but not the UCSI_ACK_COMMAND_COMPLETE
> - * bit set. To work around this send a dummy command and bundle the
> - * UCSI_ACK_CONNECTOR_CHANGE with the UCSI_ACK_COMMAND_COMPLETE
> - * for the dummy command.
> - */
> -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;
> - u64 dummycmd = UCSI_GET_CAPABILITY;
> - int ret;
> -
> - if (cmd == (UCSI_ACK_CC_CI | UCSI_ACK_CONNECTOR_CHANGE)) {
> - cmd |= UCSI_ACK_COMMAND_COMPLETE;
> -
> - /*
> - * The UCSI core thinks it is sending a connector change ack
> - * and will accept new connector change events. We don't want
> - * this to happen for the dummy command as its response will
> - * still report the very event that the core is trying to clear.
> - */
> - set_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
> - ret = ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &dummycmd,
> - sizeof(dummycmd));
> - clear_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags);
> -
> - if (ret < 0)
> - return ret;
> - }
> -
> - return ucsi_acpi_sync_write(ucsi, UCSI_CONTROL, &cmd, sizeof(cmd));
> -}
> -
> -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 = {
> @@ -180,12 +136,6 @@ static const struct dmi_system_id ucsi_acpi_quirks[] = {
> },
> .driver_data = (void *)&ucsi_zenbook_ops,
> },
> - {
> - .matches = {
> - DMI_MATCH(DMI_SYS_VENDOR, "Dell Inc."),
> - },
> - .driver_data = (void *)&ucsi_dell_ops,
> - },
> { }
> };
>
> @@ -199,8 +149,7 @@ static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
> if (ret)
> return;
>
> - if (UCSI_CCI_CONNECTOR(cci) &&
> - !test_bit(UCSI_ACPI_SUPPRESS_EVENT, &ua->flags))
> + if (UCSI_CCI_CONNECTOR(cci))
> ucsi_connector_change(ua->ucsi, UCSI_CCI_CONNECTOR(cci));
>
> if (cci & UCSI_CCI_ACK_COMPLETE &&
> --
> 2.40.1
--
heikki
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-03 10:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-03-27 22:45 [PATCH 0/3] usb: typec: ucsi: Ack connector change early Christian A. Ehrhardt
2024-03-27 22:45 ` [PATCH 1/3] usb: typec: ucsi: Stop abuse of bit definitions from ucsi.h Christian A. Ehrhardt
2024-04-03 10:39 ` Heikki Krogerus
2024-03-27 22:45 ` [PATCH 2/3] usb: typec: ucsi: Never send a lone connector change ack Christian A. Ehrhardt
2024-04-03 10:40 ` Heikki Krogerus
2024-03-27 22:45 ` [PATCH 3/3] usb: typec: ucsi_acpi: Remove Dell quirk Christian A. Ehrhardt
2024-04-03 10:40 ` Heikki Krogerus
2024-03-29 5:56 ` [PATCH 0/3] usb: typec: ucsi: Ack connector change early Dmitry Baryshkov
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).