[PATCH 0/6] Fix MEI command timeout issue following warm reboot

[PATCH 0/6] Fix MEI command timeout issue following warm reboot

[Wentong Wu]

While enabling the IVSC on certain recent commercial products, the chipset
may occasionally enter an unknown state following a warm reboot. This issue
can cause the firmware to fail in responding to the MEI command from the
host, despite the firmware being re-downloaded. To resolve this, the current
patch set incorporates reset logic during system shutdown to ensure that the
IVSC chipset remains in a valid state after a warm reboot.

Furthermore, after the firmware download is complete, the firmware requires
some time to become operational. To address this, additional sleep time has
been introduced before the initial read operation to prevent a confusing
timeout error in vsc_tp_xfer().

Additionally, this patch set includes several enhancements as well:
1) constructing the SPI transfer command as per a specific request;
2) utilizing the appropriate byte order swap function for data received
from the ROM;
3) correcting a spelling error in a comment.

Wentong Wu (6):
  mei: vsc: Enhance IVSC chipset reset toggling
  mei: vsc: Enhance IVSC chipset stability during warm reboot
  mei: vsc: Enhance SPI transfer of IVSC rom
  mei: vsc: Utilize the appropriate byte order swap function
  mei: vsc: Prevent timeout error with added delay post-firmware
    download
  mei: vsc: Fix spelling error

 drivers/misc/mei/platform-vsc.c  |  4 ++--
 drivers/misc/mei/vsc-fw-loader.c |  2 +-
 drivers/misc/mei/vsc-tp.c        | 20 ++++++++++++++++++--
 3 files changed, 21 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/6] mei: vsc: Enhance IVSC chipset reset toggling

[Wentong Wu]

Implementing the hardware recommendation to toggle the chipset reset.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/vsc-tp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index e6a98dba8a73..dcab5174bf00 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -350,6 +350,8 @@ void vsc_tp_reset(struct vsc_tp *tp)
 	disable_irq(tp->spi->irq);
 
 	/* toggle reset pin */
+	gpiod_set_value_cansleep(tp->resetfw, 1);
+	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
 	gpiod_set_value_cansleep(tp->resetfw, 0);
 	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
 	gpiod_set_value_cansleep(tp->resetfw, 1);
-- 
2.34.1

[PATCH 2/6] mei: vsc: Enhance IVSC chipset stability during warm reboot

[Wentong Wu]

During system shutdown, incorporate reset logic to ensure the IVSC
chipset remains in a valid state. This adjustment guarantees that
the IVSC chipset operates in a known state following a warm reboot.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/vsc-tp.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index dcab5174bf00..4595b1a25536 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -570,6 +570,19 @@ static void vsc_tp_remove(struct spi_device *spi)
 	free_irq(spi->irq, tp);
 }
 
+static void vsc_tp_shutdown(struct spi_device *spi)
+{
+	struct vsc_tp *tp = spi_get_drvdata(spi);
+
+	platform_device_unregister(tp->pdev);
+
+	mutex_destroy(&tp->mutex);
+
+	vsc_tp_reset(tp);
+
+	free_irq(spi->irq, tp);
+}
+
 static const struct acpi_device_id vsc_tp_acpi_ids[] = {
 	{ "INTC1009" }, /* Raptor Lake */
 	{ "INTC1058" }, /* Tiger Lake */
@@ -582,6 +595,7 @@ MODULE_DEVICE_TABLE(acpi, vsc_tp_acpi_ids);
 static struct spi_driver vsc_tp_driver = {
 	.probe = vsc_tp_probe,
 	.remove = vsc_tp_remove,
+	.shutdown = vsc_tp_shutdown,
 	.driver = {
 		.name = "vsc-tp",
 		.acpi_match_table = vsc_tp_acpi_ids,
-- 
2.34.1

[PATCH 3/6] mei: vsc: Enhance SPI transfer of IVSC rom

[Wentong Wu]

Constructing the SPI transfer command as per the specific request.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/vsc-tp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 4595b1a25536..7a89e4e5d553 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -331,7 +331,7 @@ int vsc_tp_rom_xfer(struct vsc_tp *tp, const void *obuf, void *ibuf, size_t len)
 		return ret;
 	}
 
-	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, tp->rx_buf, len);
+	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, ibuf ? tp->rx_buf : ibuf, len);
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH 4/6] mei: vsc: Utilize the appropriate byte order swap function

[Wentong Wu]

Switch from cpu_to_be32_array() to be32_to_cpu_array() for the
received rom data.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/vsc-tp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
index 7a89e4e5d553..381d7ba4f98a 100644
--- a/drivers/misc/mei/vsc-tp.c
+++ b/drivers/misc/mei/vsc-tp.c
@@ -336,7 +336,7 @@ int vsc_tp_rom_xfer(struct vsc_tp *tp, const void *obuf, void *ibuf, size_t len)
 		return ret;
 
 	if (ibuf)
-		cpu_to_be32_array(ibuf, tp->rx_buf, words);
+		be32_to_cpu_array(ibuf, tp->rx_buf, words);
 
 	return ret;
 }
-- 
2.34.1

[PATCH 5/6] mei: vsc: Prevent timeout error with added delay post-firmware download

[Wentong Wu]

After completing the firmware download, the firmware requires some
time to become functional. This change introduces additional sleep
time before the first read operation to prevent a confusing timeout
error in vsc_tp_xfer().

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/platform-vsc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/misc/mei/platform-vsc.c b/drivers/misc/mei/platform-vsc.c
index 1ec65d87488a..d02f6e881139 100644
--- a/drivers/misc/mei/platform-vsc.c
+++ b/drivers/misc/mei/platform-vsc.c
@@ -28,8 +28,8 @@
 
 #define MEI_VSC_MAX_MSG_SIZE		512
 
-#define MEI_VSC_POLL_DELAY_US		(50 * USEC_PER_MSEC)
-#define MEI_VSC_POLL_TIMEOUT_US		(200 * USEC_PER_MSEC)
+#define MEI_VSC_POLL_DELAY_US		(100 * USEC_PER_MSEC)
+#define MEI_VSC_POLL_TIMEOUT_US		(400 * USEC_PER_MSEC)
 
 #define mei_dev_to_vsc_hw(dev)		((struct mei_vsc_hw *)((dev)->hw))
 
-- 
2.34.1

[PATCH 6/6] mei: vsc: Fix spelling error

[Wentong Wu]

Fix a spelling error in a comment.

Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
Cc: stable@vger.kernel.org # for 6.8+
Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Tested-by: Jason Chen <jason.z.chen@intel.com>
---
 drivers/misc/mei/vsc-fw-loader.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/misc/mei/vsc-fw-loader.c b/drivers/misc/mei/vsc-fw-loader.c
index 596a9d695dfc..084d0205f97d 100644
--- a/drivers/misc/mei/vsc-fw-loader.c
+++ b/drivers/misc/mei/vsc-fw-loader.c
@@ -204,7 +204,7 @@ struct vsc_img_frag {
 
 /**
  * struct vsc_fw_loader - represent vsc firmware loader
- * @dev: device used to request fimware
+ * @dev: device used to request firmware
  * @tp: transport layer used with the firmware loader
  * @csi: CSI image
  * @ace: ACE image
-- 
2.34.1

Re: [PATCH 1/6] mei: vsc: Enhance IVSC chipset reset toggling

[Sakari Ailus]

Hi Wentong,

Thanks for the set.

On Sun, Jun 23, 2024 at 05:30:51PM +0800, Wentong Wu wrote:
> Implementing the hardware recommendation to toggle the chipset reset.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Cc: stable@vger.kernel.org # for 6.8+
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> Tested-by: Jason Chen <jason.z.chen@intel.com>
> ---
>  drivers/misc/mei/vsc-tp.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index e6a98dba8a73..dcab5174bf00 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -350,6 +350,8 @@ void vsc_tp_reset(struct vsc_tp *tp)
>  	disable_irq(tp->spi->irq);
>  
>  	/* toggle reset pin */
> +	gpiod_set_value_cansleep(tp->resetfw, 1);
> +	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
>  	gpiod_set_value_cansleep(tp->resetfw, 0);
>  	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
>  	gpiod_set_value_cansleep(tp->resetfw, 1);

Looking at the patch, the driver appears to leave the reset signal enabled.
As it currently works, also the polarity appears to be wrong.

Could you addrss this, after this patch?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 6/6] mei: vsc: Fix spelling error

[Sakari Ailus]

Hi Wentong,

On Sun, Jun 23, 2024 at 05:30:56PM +0800, Wentong Wu wrote:
> Fix a spelling error in a comment.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Cc: stable@vger.kernel.org # for 6.8+

There's hardly a need to cc this to stable.

> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> Tested-by: Jason Chen <jason.z.chen@intel.com>
> ---
>  drivers/misc/mei/vsc-fw-loader.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/vsc-fw-loader.c b/drivers/misc/mei/vsc-fw-loader.c
> index 596a9d695dfc..084d0205f97d 100644
> --- a/drivers/misc/mei/vsc-fw-loader.c
> +++ b/drivers/misc/mei/vsc-fw-loader.c
> @@ -204,7 +204,7 @@ struct vsc_img_frag {
>  
>  /**
>   * struct vsc_fw_loader - represent vsc firmware loader
> - * @dev: device used to request fimware
> + * @dev: device used to request firmware
>   * @tp: transport layer used with the firmware loader
>   * @csi: CSI image
>   * @ace: ACE image

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/6] mei: vsc: Enhance SPI transfer of IVSC rom

[Sakari Ailus]

Hi Wentong,

On Sun, Jun 23, 2024 at 05:30:53PM +0800, Wentong Wu wrote:
> Constructing the SPI transfer command as per the specific request.
> 
> Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> Cc: stable@vger.kernel.org # for 6.8+
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> Tested-by: Jason Chen <jason.z.chen@intel.com>
> ---
>  drivers/misc/mei/vsc-tp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> index 4595b1a25536..7a89e4e5d553 100644
> --- a/drivers/misc/mei/vsc-tp.c
> +++ b/drivers/misc/mei/vsc-tp.c
> @@ -331,7 +331,7 @@ int vsc_tp_rom_xfer(struct vsc_tp *tp, const void *obuf, void *ibuf, size_t len)
>  		return ret;
>  	}
>  
> -	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, tp->rx_buf, len);
> +	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, ibuf ? tp->rx_buf : ibuf, len);

Is this correct? I.e. use ibuf when it's NULL, otherwise use tp->rx_buf?

>  	if (ret)
>  		return ret;
>  

-- 
Kind regards,

Sakari Ailus

RE: [PATCH 6/6] mei: vsc: Fix spelling error

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Hi Wentong,
> 
> On Sun, Jun 23, 2024 at 05:30:56PM +0800, Wentong Wu wrote:
> > Fix a spelling error in a comment.
> >
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Cc: stable@vger.kernel.org # for 6.8+
> 
> There's hardly a need to cc this to stable.

Ack, thanks

BR,
Wentong
> 
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > Tested-by: Jason Chen <jason.z.chen@intel.com>
> > ---
> >  drivers/misc/mei/vsc-fw-loader.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/vsc-fw-loader.c b/drivers/misc/mei/vsc-fw-
> loader.c
> > index 596a9d695dfc..084d0205f97d 100644
> > --- a/drivers/misc/mei/vsc-fw-loader.c
> > +++ b/drivers/misc/mei/vsc-fw-loader.c
> > @@ -204,7 +204,7 @@ struct vsc_img_frag {
> >
> >  /**
> >   * struct vsc_fw_loader - represent vsc firmware loader
> > - * @dev: device used to request fimware
> > + * @dev: device used to request firmware
> >   * @tp: transport layer used with the firmware loader
> >   * @csi: CSI image
> >   * @ace: ACE image
> 
> --
> Kind regards,
> 
> Sakari Ailus

RE: [PATCH 3/6] mei: vsc: Enhance SPI transfer of IVSC rom

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Hi Wentong,
> 
> On Sun, Jun 23, 2024 at 05:30:53PM +0800, Wentong Wu wrote:
> > Constructing the SPI transfer command as per the specific request.
> >
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Cc: stable@vger.kernel.org # for 6.8+
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > Tested-by: Jason Chen <jason.z.chen@intel.com>
> > ---
> >  drivers/misc/mei/vsc-tp.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> > index 4595b1a25536..7a89e4e5d553 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -331,7 +331,7 @@ int vsc_tp_rom_xfer(struct vsc_tp *tp, const void
> *obuf, void *ibuf, size_t len)
> >  		return ret;
> >  	}
> >
> > -	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, tp->rx_buf, len);
> > +	ret = vsc_tp_dev_xfer(tp, tp->tx_buf, ibuf ? tp->rx_buf : ibuf,
> > +len);
> 
> Is this correct? I.e. use ibuf when it's NULL, otherwise use tp->rx_buf?

Yes, the SPI framework will adjust this. If ibuf is NULL, but we give tp->rx_buf,
the actual transfer will tx more data than needed.

BR,
Wentong
> 
> >  	if (ret)
> >  		return ret;
> >
> 
> --
> Kind regards,
> 
> Sakari Ailus

RE: [PATCH 1/6] mei: vsc: Enhance IVSC chipset reset toggling

[Wu, Wentong]

> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> Hi Wentong,
> 
> Thanks for the set.
>
Hi Sakari,

Thanks for your review

> On Sun, Jun 23, 2024 at 05:30:51PM +0800, Wentong Wu wrote:
> > Implementing the hardware recommendation to toggle the chipset reset.
> >
> > Fixes: 566f5ca97680 ("mei: Add transport driver for IVSC device")
> > Cc: stable@vger.kernel.org # for 6.8+
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > Tested-by: Jason Chen <jason.z.chen@intel.com>
> > ---
> >  drivers/misc/mei/vsc-tp.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/misc/mei/vsc-tp.c b/drivers/misc/mei/vsc-tp.c
> > index e6a98dba8a73..dcab5174bf00 100644
> > --- a/drivers/misc/mei/vsc-tp.c
> > +++ b/drivers/misc/mei/vsc-tp.c
> > @@ -350,6 +350,8 @@ void vsc_tp_reset(struct vsc_tp *tp)
> >  	disable_irq(tp->spi->irq);
> >
> >  	/* toggle reset pin */
> > +	gpiod_set_value_cansleep(tp->resetfw, 1);
> > +	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
> >  	gpiod_set_value_cansleep(tp->resetfw, 0);
> >  	msleep(VSC_TP_RESET_PIN_TOGGLE_INTERVAL_MS);
> >  	gpiod_set_value_cansleep(tp->resetfw, 1);
> 
> Looking at the patch, the driver appears to leave the reset signal enabled.
> As it currently works, also the polarity appears to be wrong.

Ok, the reset pin is RISING trigger. Probably we can remove this.

> 
> Could you addrss this, after this patch?

ack, I will send v2 patch set.

BR,
Wentong
> 
> --
> Kind regards,
> 
> Sakari Ailus