[PATCH V2 0/4] staging: nvec: improve robustnes during initialization

[PATCH V2 0/4] staging: nvec: improve robustnes during initialization

[Marc Dietrich]

This series against 6.9-rc2 improves robustnes of the keyboard and
touchpad initialization with the goal to eliminate the ugly delay
in the I2C client controller ISR.

Changes since initial submission:
- address comments from Dan regarding commit messages and function
  documentation
- address comments from Thierry by introducing responseless sync writes

Marc Dietrich (5):
  staging: nvec: add ability to ignore EC responses in sync writes
  staging: nvec: make keyboard init synchronous
  staging: nvec: make touchpad init synchronous
  staging: nvec: make i2c controller register writes robust
  staging: nvec: update TODO

 drivers/staging/nvec/TODO       |  1 -
 drivers/staging/nvec/nvec.c     | 50 ++++++++++++++++++++-------------
 drivers/staging/nvec/nvec_kbd.c |  9 +++---
 drivers/staging/nvec/nvec_ps2.c | 31 +++++++++++++-------
 4 files changed, 57 insertions(+), 34 deletions(-)

--
2.43.0

[PATCH 1/5] staging: nvec: add ability to ignore EC responses in sync writes

[Marc Dietrich]

In case we just want to submit a message to the EC but are not
interested in its response, we can free the response buffer early.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 282a664c9176..45df190c2f94 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -300,7 +300,9 @@ int nvec_write_sync(struct nvec_chip *nvec,
 {
 	mutex_lock(&nvec->sync_write_mutex);

-	*msg = NULL;
+	if (msg != NULL)
+		*msg = NULL;
+
 	nvec->sync_write_pending = (data[1] << 8) + data[0];

 	if (nvec_write_async(nvec, data, size) < 0) {
@@ -320,7 +322,10 @@ int nvec_write_sync(struct nvec_chip *nvec,

 	dev_dbg(nvec->dev, "nvec_sync_write: pong!\n");

-	*msg = nvec->last_sync_msg;
+	if (msg != NULL)
+		*msg = nvec->last_sync_msg;
+	else
+		nvec_msg_free(nvec, nvec->last_sync_msg);

 	mutex_unlock(&nvec->sync_write_mutex);

--
2.43.0

[PATCH 2/5] staging: nvec: make keyboard init synchronous

[Marc Dietrich]

Currently, we are constantly sending commands to the EC without waiting for them to be
executed. This can lead to confusion, especially if we initialize several different devices
one after the other.
To avoid this, we are switching from asynchronous to synchronous command transmission.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec_kbd.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/nvec/nvec_kbd.c b/drivers/staging/nvec/nvec_kbd.c
index f9a1da952c0a..d0259c80f810 100644
--- a/drivers/staging/nvec/nvec_kbd.c
+++ b/drivers/staging/nvec/nvec_kbd.c
@@ -148,15 +148,16 @@ static int nvec_kbd_probe(struct platform_device *pdev)
 	nvec_register_notifier(nvec, &keys_dev.notifier, 0);

 	/* Enable keyboard */
-	nvec_write_async(nvec, enable_kbd, 2);
+	nvec_write_sync(nvec, enable_kbd, 2, NULL);

 	/* configures wake on special keys */
-	nvec_write_async(nvec, cnfg_wake, 4);
+	nvec_write_sync(nvec, cnfg_wake, 4, NULL);
+
 	/* enable wake key reporting */
-	nvec_write_async(nvec, cnfg_wake_key_reporting, 3);
+	nvec_write_sync(nvec, cnfg_wake_key_reporting, 3, NULL);

 	/* Disable caps lock LED */
-	nvec_write_async(nvec, clear_leds, sizeof(clear_leds));
+	nvec_write_sync(nvec, clear_leds, sizeof(clear_leds), NULL);

 	return 0;
 }
--
2.43.0

[PATCH 3/5] staging: nvec: make touchpad init synchronous

[Marc Dietrich]

Currently, we are constantly sending commands to the EC without waiting for them to be
executed. For the touchpad initialization this only worked because we were waiting 200 µs
between each submitted command byte, so the EC had enough time to execute. In the furture
we like to avoid this delay, so we need to wait for each command to be executed first.
Do this by switching from asynchronous to synchronous command transmission.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec_ps2.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/nvec/nvec_ps2.c b/drivers/staging/nvec/nvec_ps2.c
index cb6d71b8dc83..f34016c4a26b 100644
--- a/drivers/staging/nvec/nvec_ps2.c
+++ b/drivers/staging/nvec/nvec_ps2.c
@@ -60,16 +60,6 @@ static void ps2_stopstreaming(struct serio *ser_dev)
 	nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
 }

-static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
-{
-	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
-
-	buf[2] = cmd & 0xff;
-
-	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
-	return nvec_write_async(ps2_dev.nvec, buf, sizeof(buf));
-}
-
 static int nvec_ps2_notifier(struct notifier_block *nb,
 			     unsigned long event_type, void *data)
 {
@@ -98,6 +88,27 @@ static int nvec_ps2_notifier(struct notifier_block *nb,
 	return NOTIFY_DONE;
 }

+static int ps2_sendcommand(struct serio *ser_dev, unsigned char cmd)
+{
+	unsigned char buf[] = { NVEC_PS2, SEND_COMMAND, ENABLE_MOUSE, 1 };
+	struct nvec_msg *msg;
+	int ret;
+
+	buf[2] = cmd & 0xff;
+
+	dev_dbg(&ser_dev->dev, "Sending ps2 cmd %02x\n", cmd);
+
+	ret = nvec_write_sync(ps2_dev.nvec, buf, sizeof(buf), &msg);
+	if (ret < 0)
+		return ret;
+
+	nvec_ps2_notifier(NULL, NVEC_PS2, msg->data);
+
+	nvec_msg_free(ps2_dev.nvec, msg);
+
+	return 0;
+}
+
 static int nvec_mouse_probe(struct platform_device *pdev)
 {
 	struct nvec_chip *nvec = dev_get_drvdata(pdev->dev.parent);
--
2.43.0

[PATCH 4/5] staging: nvec: make i2c controller register writes robust

[Marc Dietrich]

The i2c controller needs to read back the data written to its registers.
This way we can avoid the long delay in the interrupt handler.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 45df190c2f94..214839f51048 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -570,6 +570,22 @@ static void nvec_tx_set(struct nvec_chip *nvec)
 		(uint)nvec->tx->size, nvec->tx->data[1]);
 }

+/**
+ * i2c_writel - safely write to an I2C client controller register
+ * @val: value to be written
+ * @reg: register to write to
+ *
+ * A write to an I2C controller register needs to be read back to make sure
+ * that the value has arrived.
+ */
+static void i2c_writel(u32 val, void *reg)
+{
+	writel_relaxed(val, reg);
+
+	/* read back register to make sure that register writes completed */
+	readl_relaxed(reg);
+}
+
 /**
  * nvec_interrupt - Interrupt handler
  * @irq: The IRQ
@@ -604,7 +620,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 	if ((status & RNW) == 0) {
 		received = readl(nvec->base + I2C_SL_RCVD);
 		if (status & RCVD)
-			writel(0, nvec->base + I2C_SL_RCVD);
+			i2c_writel(0, nvec->base + I2C_SL_RCVD);
 	}

 	if (status == (I2C_SL_IRQ | RCVD))
@@ -696,7 +712,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)

 	/* Send data if requested, but not on end of transmission */
 	if ((status & (RNW | END_TRANS)) == RNW)
-		writel(to_send, nvec->base + I2C_SL_RCVD);
+		i2c_writel(to_send, nvec->base + I2C_SL_RCVD);

 	/* If we have send the first byte */
 	if (status == (I2C_SL_IRQ | RNW | RCVD))
@@ -713,15 +729,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 		status & RCVD ? " RCVD" : "",
 		status & RNW ? " RNW" : "");

-	/*
-	 * TODO: replace the udelay with a read back after each writel above
-	 * in order to work around a hardware issue, see i2c-tegra.c
-	 *
-	 * Unfortunately, this change causes an intialisation issue with the
-	 * touchpad, which needs to be fixed first.
-	 */
-	udelay(100);
-
 	return IRQ_HANDLED;
 }

@@ -737,15 +744,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)

 	val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
 	    (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
-	writel(val, nvec->base + I2C_CNFG);
+	i2c_writel(val, nvec->base + I2C_CNFG);

 	clk_set_rate(nvec->i2c_clk, 8 * 80000);

-	writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
-	writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
+	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
+	i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);

-	writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
-	writel(0, nvec->base + I2C_SL_ADDR2);
+	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
+	i2c_writel(0, nvec->base + I2C_SL_ADDR2);

 	enable_irq(nvec->irq);
 }
@@ -754,7 +761,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
 static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
 {
 	disable_irq(nvec->irq);
-	writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
+	i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
 	clk_disable_unprepare(nvec->i2c_clk);
 }
 #endif
--
2.43.0

Re: [PATCH 4/5] staging: nvec: make i2c controller register writes robust

[Greg KH]

On Sat, Apr 06, 2024 at 02:31:22PM +0200, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.
> 
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
>  drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
>  1 file changed, 24 insertions(+), 17 deletions(-)

This change did not apply to the tree, can you rebase it and resend?

thanks,
greg k-h

[PATCH V2 4/5] staging: nvec: make i2c controller register writes robust

[Marc Dietrich]

The i2c controller needs to read back the data written to its registers.
This way we can avoid the long delay in the interrupt handler.

V2: rebase against staging-next

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/nvec.c | 41 ++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
index 45df190c2f94..214839f51048 100644
--- a/drivers/staging/nvec/nvec.c
+++ b/drivers/staging/nvec/nvec.c
@@ -570,6 +570,22 @@ static void nvec_tx_set(struct nvec_chip *nvec)
 		(uint)nvec->tx->size, nvec->tx->data[1]);
 }

+/**
+ * i2c_writel - safely write to an I2C client controller register
+ * @val: value to be written
+ * @reg: register to write to
+ *
+ * A write to an I2C controller register needs to be read back to make sure
+ * that the value has arrived.
+ */
+static void i2c_writel(u32 val, void *reg)
+{
+	writel_relaxed(val, reg);
+
+	/* read back register to make sure that register writes completed */
+	readl_relaxed(reg);
+}
+
 /**
  * nvec_interrupt - Interrupt handler
  * @irq: The IRQ
@@ -604,7 +620,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 	if ((status & RNW) == 0) {
 		received = readl(nvec->base + I2C_SL_RCVD);
 		if (status & RCVD)
-			writel(0, nvec->base + I2C_SL_RCVD);
+			i2c_writel(0, nvec->base + I2C_SL_RCVD);
 	}

 	if (status == (I2C_SL_IRQ | RCVD))
@@ -696,7 +712,7 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)

 	/* Send data if requested, but not on end of transmission */
 	if ((status & (RNW | END_TRANS)) == RNW)
-		writel(to_send, nvec->base + I2C_SL_RCVD);
+		i2c_writel(to_send, nvec->base + I2C_SL_RCVD);

 	/* If we have send the first byte */
 	if (status == (I2C_SL_IRQ | RNW | RCVD))
@@ -713,15 +729,6 @@ static irqreturn_t nvec_interrupt(int irq, void *dev)
 		status & RCVD ? " RCVD" : "",
 		status & RNW ? " RNW" : "");

-	/*
-	 * TODO: replace the udelay with a read back after each writel above
-	 * in order to work around a hardware issue, see i2c-tegra.c
-	 *
-	 * Unfortunately, this change causes an initialisation issue with the
-	 * touchpad, which needs to be fixed first.
-	 */
-	udelay(100);
-
 	return IRQ_HANDLED;
 }

@@ -737,15 +744,15 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)

 	val = I2C_CNFG_NEW_MASTER_SFM | I2C_CNFG_PACKET_MODE_EN |
 	    (0x2 << I2C_CNFG_DEBOUNCE_CNT_SHIFT);
-	writel(val, nvec->base + I2C_CNFG);
+	i2c_writel(val, nvec->base + I2C_CNFG);

 	clk_set_rate(nvec->i2c_clk, 8 * 80000);

-	writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
-	writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);
+	i2c_writel(I2C_SL_NEWSL, nvec->base + I2C_SL_CNFG);
+	i2c_writel(0x1E, nvec->base + I2C_SL_DELAY_COUNT);

-	writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
-	writel(0, nvec->base + I2C_SL_ADDR2);
+	i2c_writel(nvec->i2c_addr >> 1, nvec->base + I2C_SL_ADDR1);
+	i2c_writel(0, nvec->base + I2C_SL_ADDR2);

 	enable_irq(nvec->irq);
 }
@@ -754,7 +761,7 @@ static void tegra_init_i2c_slave(struct nvec_chip *nvec)
 static void nvec_disable_i2c_slave(struct nvec_chip *nvec)
 {
 	disable_irq(nvec->irq);
-	writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
+	i2c_writel(I2C_SL_NEWSL | I2C_SL_NACK, nvec->base + I2C_SL_CNFG);
 	clk_disable_unprepare(nvec->i2c_clk);
 }
 #endif
--
2.43.0

Re: [PATCH V2 4/5] staging: nvec: make i2c controller register writes robust

[Dan Carpenter]

On Sun, Apr 14, 2024 at 05:09:37PM +0200, Marc Dietrich wrote:
> The i2c controller needs to read back the data written to its registers.
> This way we can avoid the long delay in the interrupt handler.
> 
> V2: rebase against staging-next
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This needs to go below the cut off line.

> 
> Signed-off-by: Marc Dietrich <marvin24@gmx.de>
> ---
  ^^^
Here.

Greg applied all the other patches in the series, so sure, you're
resending 4/5 but probably it's less confusing to just call it [PATCH]
and leave off that it was part of a series...  Just put that in the note
under the cut off line.

Send it as a new thread as well.

(I wouldn't have commented on half this stuff except that the v2 comment
was in the wrong place so you're going to need to resend anyway.  I
think DRM puts their v2 comments there but no one else does because it's
weird and it doesn't need to be in the permanent git log).

regards,
dan carpenter

[PATCH 5/5] staging: nvec: update TODO

[Marc Dietrich]

Remove isr delay from the bill.

Signed-off-by: Marc Dietrich <marvin24@gmx.de>
---
 drivers/staging/nvec/TODO | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/staging/nvec/TODO b/drivers/staging/nvec/TODO
index 8afde3ccc960..33f9ebe6d59b 100644
--- a/drivers/staging/nvec/TODO
+++ b/drivers/staging/nvec/TODO
@@ -1,5 +1,4 @@
 ToDo list (incomplete, unordered)
 	- move the driver to the new i2c slave framework
 	- finish suspend/resume support
-	- fix udelay in the isr
 	- add atomic ops in order to fix shutoff/reboot problems
--
2.43.0