Re: How to proceed: usci_acpi: PPM init failed (-110)

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

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@cavoj.net>
> 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@cavoj.net>
> ---
>  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