Hi Sam,
Sorry to keep you waiting.
On Thu, Dec 22, 2022 at 09:18:27PM +0100, Samuel Čavoj wrote:
> Hi Heikki,
>
> I gave this a hard look and figured out the issue. Long story short, the
> firmware is clearing the CCI on EC RAM after copying from EC RAM to
> system memory. This happens both when notifications are delivered and
> when a read operation is explicitly performed via _DSM(read). What the
> driver currently does after receiving a notification is performing an
> explicit read. However by this time the CCI in EC RAM has been cleared
> by the AML and the information is lost.
>
> Details:
>
> 1. _DSM(read) of the UCSI device:
>
> Copies all relevant fields of the mailbox data structure from EC RAM
> to a SystemMemory OperationRegion. The last field to be copied is the
> CCI:
>
> [...]
> CCI0 = \_SB.PCI0.SBRG.EC0.ECRD (RefOf (\_SB.PCI0.SBRG.EC0.CCI0))
> CCI1 = \_SB.PCI0.SBRG.EC0.ECRD (RefOf (\_SB.PCI0.SBRG.EC0.CCI1))
> CCI2 = \_SB.PCI0.SBRG.EC0.ECRD (RefOf (\_SB.PCI0.SBRG.EC0.CCI2))
> CCI3 = \_SB.PCI0.SBRG.EC0.ECRD (RefOf (\_SB.PCI0.SBRG.EC0.CCI3))
> [...]
>
> However, for some reason, this is followed by another two operations,
> which zero-out half of the CCI:
>
> [...]
> \_SB.PCI0.SBRG.EC0.ECWT (Zero, RefOf (\_SB.PCI0.SBRG.EC0.CCI0))
> \_SB.PCI0.SBRG.EC0.ECWT (Zero, RefOf (\_SB.PCI0.SBRG.EC0.CCI3))
>
> I don't know why this is present. This does not cause the problem,
> however, only leads to issues if two explicit reads are performed
> consecutively. What does cause problems with the current driver
> implementation is:
>
> 2. The event handler (_Q79)
>
> The _Q79 event handler on my machine is responsible for the UCSI
> notifications. It performs a copy from EC RAM to system memory and
> triggers the 0x80 notification on the UCSI device (called UBTC).
> The final instructions of this handler are:
>
> [...]
> ^^^^UBTC.CCI0 = CCI0 /* \_SB_.PCI0.SBRG.EC0_.CCI0 */
> ^^^^UBTC.CCI1 = CCI1 /* \_SB_.PCI0.SBRG.EC0_.CCI1 */
> ^^^^UBTC.CCI2 = CCI2 /* \_SB_.PCI0.SBRG.EC0_.CCI2 */
> ^^^^UBTC.CCI3 = CCI3 /* \_SB_.PCI0.SBRG.EC0_.CCI3 */
> USGC = 0xF1
> CCI0 = Zero // These two lines are the culprit
> CCI3 = Zero
> Local1 = ((Timer - Local0) / 0x2710)
> Notify (UBTC, 0x80) // Status Change
> Release (ECMT)
>
>
> A side note:
> I figured this out by booting up a Windows installation and convincing
> the local kernel debugger to dump ACPI trace information to a file. The file
> contained an outrageous amount of information with inconsistent formatting
> (missing commas and stuff) for which I wrote an extremely janky parser in
> Python. This let me examine the exact steps performed by the Windows driver.
> And the difference I noticed is that the Linux driver was issuing a
> _DSM(read)
> after every notification, reading the already-zeroed-out CCI.
>
> Patching the AML to remove the zeroing-out instructions seemed to work as
> well, but I suppose this is not a good general solution.
>
> Following is a prototype-grade patch, in essence performing soft-reads most
> of
> the time (i.e. just reading from the OpRegion and not calling _DSM) and
> explicit
> reads when necessary. I am unfortunately not familiar with the spec and the
> hardware in the wild and I understand that it is possible that some devices
> on
> the other hand do not synchronize the mailbox when notifying and it needs to
> be
> done explicitly. I suppose we'd need a parameter to configure this behaviour
> in
> that case with a quirk system. The patch works on my system. Some other
> issues
> surface later, but I think they are related to a particular cheap dongle I
> have
> because they don't seem to occur without it.
>
> What do you think about this situation? Is the patch reasonable, or does it
> need
> a significant re-think?
This is great! Thank you so much for figuring this one out!
I think your patch looks totally reasonable, but let me test it with
a couple of different systems that I have. I'll check the event
handlers from the ACPI tables. If that is how Windows works in
general, then this is what has to be done.
Thanks!
> From 45a29c149a29da989fbdd843c7f040a4454c3a33 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Samuel=20=C4=8Cavoj?= <samuel@xxxxxxxxx>
> Date: Fri, 11 Nov 2022 00:06:24 +0100
> Subject: [PATCH] TODO: ucsi: introduce read_explicit
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
>
> Signed-off-by: Samuel Čavoj <samuel@xxxxxxxxx>
> ---
> drivers/usb/typec/ucsi/ucsi.c | 9 +++++----
> drivers/usb/typec/ucsi/ucsi.h | 3 +++
> drivers/usb/typec/ucsi/ucsi_acpi.c | 11 +++++++++++
> drivers/usb/typec/ucsi/ucsi_ccg.c | 1 +
> drivers/usb/typec/ucsi/ucsi_stm32g0.c | 1 +
> 5 files changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index e690fa50ffce..c2fce8b27821 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -884,7 +884,7 @@ static int ucsi_reset_ppm(struct ucsi *ucsi)
> goto out;
> }
>
> - ret = ucsi->ops->read(ucsi, UCSI_CCI, &cci, sizeof(cci));
> + ret = ucsi->ops->read_explicit(ucsi, UCSI_CCI, &cci, sizeof(cci));
> if (ret)
> goto out;
>
> @@ -1351,7 +1351,8 @@ struct ucsi *ucsi_create(struct device *dev, const
> struct ucsi_operations *ops)
> {
> struct ucsi *ucsi;
>
> - if (!ops || !ops->read || !ops->sync_write || !ops->async_write)
> + if (!ops || !ops->read || !ops->read_explicit || !ops->sync_write ||
> + !ops->async_write)
> return ERR_PTR(-EINVAL);
>
> ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
> @@ -1385,8 +1386,8 @@ int ucsi_register(struct ucsi *ucsi)
> {
> int ret;
>
> - ret = ucsi->ops->read(ucsi, UCSI_VERSION, &ucsi->version,
> - sizeof(ucsi->version));
> + ret = ucsi->ops->read_explicit(ucsi, UCSI_VERSION, &ucsi->version,
> + sizeof(ucsi->version));
> if (ret)
> return ret;
>
> diff --git a/drivers/usb/typec/ucsi/ucsi.h b/drivers/usb/typec/ucsi/ucsi.h
> index 8eb391e3e592..e961ec1f92a0 100644
> --- a/drivers/usb/typec/ucsi/ucsi.h
> +++ b/drivers/usb/typec/ucsi/ucsi.h
> @@ -37,6 +37,7 @@ struct ucsi_altmode;
> /**
> * struct ucsi_operations - UCSI I/O operations
> * @read: Read operation
> + * @read_explicit: Read operation with explicit poll if applicable
> * @sync_write: Blocking write operation
> * @async_write: Non-blocking write operation
> * @update_altmodes: Squashes duplicate DP altmodes
> @@ -48,6 +49,8 @@ struct ucsi_altmode;
> struct ucsi_operations {
> int (*read)(struct ucsi *ucsi, unsigned int offset,
> void *val, size_t val_len);
> + int (*read_explicit)(struct ucsi *ucsi, unsigned int offset,
> + void *val, size_t val_len);
> int (*sync_write)(struct ucsi *ucsi, unsigned int offset,
> const void *val, size_t val_len);
> int (*async_write)(struct ucsi *ucsi, unsigned int offset,
> diff --git a/drivers/usb/typec/ucsi/ucsi_acpi.c
> b/drivers/usb/typec/ucsi/ucsi_acpi.c
> index a0cd8c4ebe31..20432f4313c9 100644
> --- a/drivers/usb/typec/ucsi/ucsi_acpi.c
> +++ b/drivers/usb/typec/ucsi/ucsi_acpi.c
> @@ -52,6 +52,16 @@ static int ucsi_acpi_read(struct ucsi *ucsi, unsigned int
> offset,
> void *val, size_t val_len)
> {
> struct ucsi_acpi *ua = ucsi_get_drvdata(ucsi);
> +
> + memcpy(val, ua->base + offset, val_len);
> +
> + return 0;
> +}
> +
> +static int ucsi_acpi_read_explicit(struct ucsi *ucsi, unsigned int offset,
> + 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);
> @@ -101,6 +111,7 @@ static int ucsi_acpi_sync_write(struct ucsi *ucsi,
> unsigned int offset,
>
> static const struct ucsi_operations ucsi_acpi_ops = {
> .read = ucsi_acpi_read,
> + .read_explicit = ucsi_acpi_read_explicit,
> .sync_write = ucsi_acpi_sync_write,
> .async_write = ucsi_acpi_async_write
> };
> diff --git a/drivers/usb/typec/ucsi/ucsi_ccg.c
> b/drivers/usb/typec/ucsi/ucsi_ccg.c
> index 5c0bf48be766..c1d2db3a7363 100644
> --- a/drivers/usb/typec/ucsi/ucsi_ccg.c
> +++ b/drivers/usb/typec/ucsi/ucsi_ccg.c
> @@ -593,6 +593,7 @@ static int ucsi_ccg_sync_write(struct ucsi *ucsi,
> unsigned int offset,
>
> static const struct ucsi_operations ucsi_ccg_ops = {
> .read = ucsi_ccg_read,
> + .read_explicit = ucsi_ccg_read,
> .sync_write = ucsi_ccg_sync_write,
> .async_write = ucsi_ccg_async_write,
> .update_altmodes = ucsi_ccg_update_altmodes
> diff --git a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> index 061551d464f1..274b5f016dfb 100644
> --- a/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> +++ b/drivers/usb/typec/ucsi/ucsi_stm32g0.c
> @@ -437,6 +437,7 @@ static irqreturn_t ucsi_stm32g0_irq_handler(int irq,
> void *data)
>
> static const struct ucsi_operations ucsi_stm32g0_ops = {
> .read = ucsi_stm32g0_read,
> + .read_explicit = ucsi_stm32g0_read,
> .sync_write = ucsi_stm32g0_sync_write,
> .async_write = ucsi_stm32g0_async_write,
> };
Br,
--
heikki
