* [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT()
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
@ 2025-02-22 13:38 ` Sven Peter via B4 Relay
2025-03-20 0:23 ` Andi Shyti
2025-02-22 13:38 ` [PATCH 2/4] i2c: pasemi: Improve error recovery Sven Peter via B4 Relay
` (4 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Sven Peter via B4 Relay @ 2025-02-22 13:38 UTC (permalink / raw)
To: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Andi Shyti
Cc: linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Sven Peter, Hector Martin
From: Sven Peter <sven@svenpeter.dev>
Add the missing register bits to the defines and also switch
those to use the BIT macro which is much more readable than
using hardcoded masks
Co-developed-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
drivers/i2c/busses/i2c-pasemi-core.c | 40 ++++++++++++++++++++++--------------
1 file changed, 25 insertions(+), 15 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
index dac694a9d781..bd128ab2e2eb 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.c
+++ b/drivers/i2c/busses/i2c-pasemi-core.c
@@ -5,6 +5,7 @@
* SMBus host driver for PA Semi PWRficient
*/
+#include <linux/bitfield.h>
#include <linux/module.h>
#include <linux/pci.h>
#include <linux/kernel.h>
@@ -26,21 +27,30 @@
#define REG_REV 0x28
/* Register defs */
-#define MTXFIFO_READ 0x00000400
-#define MTXFIFO_STOP 0x00000200
-#define MTXFIFO_START 0x00000100
-#define MTXFIFO_DATA_M 0x000000ff
-
-#define MRXFIFO_EMPTY 0x00000100
-#define MRXFIFO_DATA_M 0x000000ff
-
-#define SMSTA_XEN 0x08000000
-#define SMSTA_MTN 0x00200000
-
-#define CTL_MRR 0x00000400
-#define CTL_MTR 0x00000200
-#define CTL_EN 0x00000800
-#define CTL_CLK_M 0x000000ff
+#define MTXFIFO_READ BIT(10)
+#define MTXFIFO_STOP BIT(9)
+#define MTXFIFO_START BIT(8)
+#define MTXFIFO_DATA_M GENMASK(7, 0)
+
+#define MRXFIFO_EMPTY BIT(8)
+#define MRXFIFO_DATA_M GENMASK(7, 0)
+
+#define SMSTA_XIP BIT(28)
+#define SMSTA_XEN BIT(27)
+#define SMSTA_JMD BIT(25)
+#define SMSTA_JAM BIT(24)
+#define SMSTA_MTO BIT(23)
+#define SMSTA_MTA BIT(22)
+#define SMSTA_MTN BIT(21)
+#define SMSTA_MRNE BIT(19)
+#define SMSTA_MTE BIT(16)
+#define SMSTA_TOM BIT(6)
+
+#define CTL_EN BIT(11)
+#define CTL_MRR BIT(10)
+#define CTL_MTR BIT(9)
+#define CTL_UJM BIT(8)
+#define CTL_CLK_M GENMASK(7, 0)
static inline void reg_write(struct pasemi_smbus *smbus, int reg, int val)
{
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT()
2025-02-22 13:38 ` [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT() Sven Peter via B4 Relay
@ 2025-03-20 0:23 ` Andi Shyti
2025-03-22 13:23 ` Andy Shevchenko
0 siblings, 1 reply; 12+ messages in thread
From: Andi Shyti @ 2025-03-20 0:23 UTC (permalink / raw)
To: sven
Cc: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
Hi Sven,
On Sat, Feb 22, 2025 at 01:38:33PM +0000, Sven Peter via B4 Relay wrote:
> From: Sven Peter <sven@svenpeter.dev>
>
> Add the missing register bits to the defines and also switch
> those to use the BIT macro which is much more readable than
> using hardcoded masks
>
> Co-developed-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
Just this patch merged to i2c/i2c-host.
Patch 3 and 4 look good, just some comments on patch 2.
Thanks,
Andi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT()
2025-03-20 0:23 ` Andi Shyti
@ 2025-03-22 13:23 ` Andy Shevchenko
2025-03-30 8:50 ` Sven Peter
0 siblings, 1 reply; 12+ messages in thread
From: Andy Shevchenko @ 2025-03-22 13:23 UTC (permalink / raw)
To: Andi Shyti
Cc: sven, Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
Thu, Mar 20, 2025 at 01:23:25AM +0100, Andi Shyti kirjoitti:
> Hi Sven,
>
> On Sat, Feb 22, 2025 at 01:38:33PM +0000, Sven Peter via B4 Relay wrote:
> > From: Sven Peter <sven@svenpeter.dev>
> >
> > Add the missing register bits to the defines and also switch
> > those to use the BIT macro which is much more readable than
> > using hardcoded masks
> >
> > Co-developed-by: Hector Martin <marcan@marcan.st>
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
>
> Just this patch merged to i2c/i2c-host.
This needs an update as well. The proper header for BIT() et al. is bits.h.
bitfield.h is for FIELD_*() et al.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT()
2025-03-22 13:23 ` Andy Shevchenko
@ 2025-03-30 8:50 ` Sven Peter
0 siblings, 0 replies; 12+ messages in thread
From: Sven Peter @ 2025-03-30 8:50 UTC (permalink / raw)
To: Andy Shevchenko, Andi Shyti
Cc: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
On Sat, Mar 22, 2025, at 14:23, Andy Shevchenko wrote:
> Thu, Mar 20, 2025 at 01:23:25AM +0100, Andi Shyti kirjoitti:
>> Hi Sven,
>>
>> On Sat, Feb 22, 2025 at 01:38:33PM +0000, Sven Peter via B4 Relay wrote:
>> > From: Sven Peter <sven@svenpeter.dev>
>> >
>> > Add the missing register bits to the defines and also switch
>> > those to use the BIT macro which is much more readable than
>> > using hardcoded masks
>> >
>> > Co-developed-by: Hector Martin <marcan@marcan.st>
>> > Signed-off-by: Hector Martin <marcan@marcan.st>
>> > Signed-off-by: Sven Peter <sven@svenpeter.dev>
>>
>> Just this patch merged to i2c/i2c-host.
>
> This needs an update as well. The proper header for BIT() et al. is bits.h.
> bitfield.h is for FIELD_*() et al.
Ugh, good catch. Since this commit is already on the way upstream I'll send a fix
(and another one to sort the headers alphabetically while I'm at it anyway).
Thanks,
Sven
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 2/4] i2c: pasemi: Improve error recovery
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
2025-02-22 13:38 ` [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT() Sven Peter via B4 Relay
@ 2025-02-22 13:38 ` Sven Peter via B4 Relay
2025-03-20 0:17 ` Andi Shyti
2025-02-22 13:38 ` [PATCH 3/4] i2c: pasemi: Enable the unjam machine Sven Peter via B4 Relay
` (3 subsequent siblings)
5 siblings, 1 reply; 12+ messages in thread
From: Sven Peter via B4 Relay @ 2025-02-22 13:38 UTC (permalink / raw)
To: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Andi Shyti
Cc: linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Sven Peter, Hector Martin
From: Hector Martin <marcan@marcan.st>
The hardware (supposedly) has a 25ms timeout for clock stretching
and the driver uses 100ms which should be plenty. The error
reocvery itself is however lacking.
Add handling for all the missing error condition, and better recovery in
pasemi_smb_clear(). Also move the timeout to a #define since it's used
in multiple places now.
Signed-off-by: Hector Martin <marcan@marcan.st>
[sven: adjusted commit message after splitting the commit into two]
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
drivers/i2c/busses/i2c-pasemi-core.c | 70 +++++++++++++++++++++++++++++-------
1 file changed, 58 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
index bd128ab2e2eb..770b86b92a10 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.c
+++ b/drivers/i2c/busses/i2c-pasemi-core.c
@@ -52,6 +52,8 @@
#define CTL_UJM BIT(8)
#define CTL_CLK_M GENMASK(7, 0)
+#define TRANSFER_TIMEOUT_MS 100
+
static inline void reg_write(struct pasemi_smbus *smbus, int reg, int val)
{
dev_dbg(smbus->dev, "smbus write reg %x val %08x\n", reg, val);
@@ -80,23 +82,45 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
reinit_completion(&smbus->irq_completion);
}
-static void pasemi_smb_clear(struct pasemi_smbus *smbus)
+static int pasemi_smb_clear(struct pasemi_smbus *smbus)
{
unsigned int status;
+ int timeout = TRANSFER_TIMEOUT_MS;
status = reg_read(smbus, REG_SMSTA);
+
+ /* First wait for the bus to go idle */
+ while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
+ msleep(1);
+ status = reg_read(smbus, REG_SMSTA);
+ }
+
+ if (timeout < 0) {
+ dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
+ return -EIO;
+ }
+
+ /* If any badness happened or there is data in the FIFOs, reset the FIFOs */
+ if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
+ !(status & SMSTA_MTE))
+ pasemi_reset(smbus);
+
+ /* Clear the flags */
reg_write(smbus, REG_SMSTA, status);
+
+ return 0;
}
static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
{
- int timeout = 100;
+ int timeout = TRANSFER_TIMEOUT_MS;
unsigned int status;
if (smbus->use_irq) {
reinit_completion(&smbus->irq_completion);
- reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
- wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
+ /* XEN should be set when a transaction terminates, whether due to error or not */
+ reg_write(smbus, REG_IMASK, SMSTA_XEN);
+ wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(timeout));
reg_write(smbus, REG_IMASK, 0);
status = reg_read(smbus, REG_SMSTA);
} else {
@@ -107,16 +131,36 @@ static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
}
}
- /* Got NACK? */
- if (status & SMSTA_MTN)
- return -ENXIO;
+ /* Controller timeout? */
+ if (status & SMSTA_TOM) {
+ dev_warn(smbus->dev, "Controller timeout, status 0x%08x\n", status);
+ return -EIO;
+ }
- if (timeout < 0) {
- dev_warn(smbus->dev, "Timeout, status 0x%08x\n", status);
- reg_write(smbus, REG_SMSTA, status);
+ /* Peripheral timeout? */
+ if (status & SMSTA_MTO) {
+ dev_warn(smbus->dev, "Peripheral timeout, status 0x%08x\n", status);
return -ETIME;
}
+ /* Still stuck in a transaction? */
+ if (status & SMSTA_XIP) {
+ dev_warn(smbus->dev, "Bus stuck, status 0x%08x\n", status);
+ return -EIO;
+ }
+
+ /* Arbitration loss? */
+ if (status & SMSTA_MTA) {
+ dev_warn(smbus->dev, "Arbitration loss, status 0x%08x\n", status);
+ return -EBUSY;
+ }
+
+ /* Got NACK? */
+ if (status & SMSTA_MTN) {
+ dev_warn(smbus->dev, "NACK, status 0x%08x\n", status);
+ return -ENXIO;
+ }
+
/* Clear XEN */
reg_write(smbus, REG_SMSTA, SMSTA_XEN);
@@ -177,7 +221,8 @@ static int pasemi_i2c_xfer(struct i2c_adapter *adapter,
struct pasemi_smbus *smbus = adapter->algo_data;
int ret, i;
- pasemi_smb_clear(smbus);
+ if (pasemi_smb_clear(smbus))
+ return -EIO;
ret = 0;
@@ -200,7 +245,8 @@ static int pasemi_smb_xfer(struct i2c_adapter *adapter,
addr <<= 1;
read_flag = read_write == I2C_SMBUS_READ;
- pasemi_smb_clear(smbus);
+ if (pasemi_smb_clear(smbus))
+ return -EIO;
switch (size) {
case I2C_SMBUS_QUICK:
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] i2c: pasemi: Improve error recovery
2025-02-22 13:38 ` [PATCH 2/4] i2c: pasemi: Improve error recovery Sven Peter via B4 Relay
@ 2025-03-20 0:17 ` Andi Shyti
2025-03-22 10:25 ` Sven Peter
0 siblings, 1 reply; 12+ messages in thread
From: Andi Shyti @ 2025-03-20 0:17 UTC (permalink / raw)
To: sven
Cc: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
Hi Sven,
On Sat, Feb 22, 2025 at 01:38:34PM +0000, Sven Peter via B4 Relay wrote:
> The hardware (supposedly) has a 25ms timeout for clock stretching
> and the driver uses 100ms which should be plenty.
Can we add this lines as a comment to the define you are adding?
> The error
> reocvery itself is however lacking.
...
> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
> {
> unsigned int status;
> + int timeout = TRANSFER_TIMEOUT_MS;
>
> status = reg_read(smbus, REG_SMSTA);
> +
> + /* First wait for the bus to go idle */
> + while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
> + msleep(1);
Please, use usleep_range for 1 millisecond timeout.
> + status = reg_read(smbus, REG_SMSTA);
> + }
You could use here readx_poll_timeout() here.
> +
> + if (timeout < 0) {
> + dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
if it's an error, please use an error.
> + return -EIO;
> + }
> +
> + /* If any badness happened or there is data in the FIFOs, reset the FIFOs */
> + if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
> + !(status & SMSTA_MTE))
Please, fixe the alignment here.
> + pasemi_reset(smbus);
> +
> + /* Clear the flags */
> reg_write(smbus, REG_SMSTA, status);
> +
> + return 0;
> }
>
> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
> {
> - int timeout = 100;
> + int timeout = TRANSFER_TIMEOUT_MS;
> unsigned int status;
>
> if (smbus->use_irq) {
> reinit_completion(&smbus->irq_completion);
> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
> - wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
> + /* XEN should be set when a transaction terminates, whether due to error or not */
> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
> + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(timeout));
what happens if the timeout expires?
> reg_write(smbus, REG_IMASK, 0);
> status = reg_read(smbus, REG_SMSTA);
> } else {
...
> struct pasemi_smbus *smbus = adapter->algo_data;
> int ret, i;
>
> - pasemi_smb_clear(smbus);
> + if (pasemi_smb_clear(smbus))
> + return -EIO;
Can we use
ret = ...
if (ret)
return ret;
This way we return whatever comes from pasemi_smb_clear().
>
> ret = 0;
This way we can remove this line, as well.
Andi
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 2/4] i2c: pasemi: Improve error recovery
2025-03-20 0:17 ` Andi Shyti
@ 2025-03-22 10:25 ` Sven Peter
0 siblings, 0 replies; 12+ messages in thread
From: Sven Peter @ 2025-03-22 10:25 UTC (permalink / raw)
To: Andi Shyti
Cc: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
Hi Andi,
Thanks for the review! Will send a v2 after -rc1 is out.
On Thu, Mar 20, 2025, at 01:17, Andi Shyti wrote:
> Hi Sven,
>
> On Sat, Feb 22, 2025 at 01:38:34PM +0000, Sven Peter via B4 Relay wrote:
>> The hardware (supposedly) has a 25ms timeout for clock stretching
>> and the driver uses 100ms which should be plenty.
>
> Can we add this lines as a comment to the define you are adding?
Sure.
>
>> The error
>> reocvery itself is however lacking.
>
> ...
>
>> -static void pasemi_smb_clear(struct pasemi_smbus *smbus)
>> +static int pasemi_smb_clear(struct pasemi_smbus *smbus)
>> {
>> unsigned int status;
>> + int timeout = TRANSFER_TIMEOUT_MS;
>>
>> status = reg_read(smbus, REG_SMSTA);
>> +
>> + /* First wait for the bus to go idle */
>> + while ((status & (SMSTA_XIP | SMSTA_JAM)) && timeout--) {
>> + msleep(1);
>
> Please, use usleep_range for 1 millisecond timeout.
Ack.
>
>> + status = reg_read(smbus, REG_SMSTA);
>> + }
>
> You could use here readx_poll_timeout() here.
Yup, that should work.
>
>> +
>> + if (timeout < 0) {
>> + dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
>
> if it's an error, please use an error.
Ack.
>
>> + return -EIO;
>> + }
>> +
>> + /* If any badness happened or there is data in the FIFOs, reset the FIFOs */
>> + if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
>> + !(status & SMSTA_MTE))
>
> Please, fixe the alignment here.
Ok.
>
>> + pasemi_reset(smbus);
>> +
>> + /* Clear the flags */
>> reg_write(smbus, REG_SMSTA, status);
>> +
>> + return 0;
>> }
>>
>> static int pasemi_smb_waitready(struct pasemi_smbus *smbus)
>> {
>> - int timeout = 100;
>> + int timeout = TRANSFER_TIMEOUT_MS;
>> unsigned int status;
>>
>> if (smbus->use_irq) {
>> reinit_completion(&smbus->irq_completion);
>> - reg_write(smbus, REG_IMASK, SMSTA_XEN | SMSTA_MTN);
>> - wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(100));
>> + /* XEN should be set when a transaction terminates, whether due to error or not */
>> + reg_write(smbus, REG_IMASK, SMSTA_XEN);
>> + wait_for_completion_timeout(&smbus->irq_completion, msecs_to_jiffies(timeout));
>
> what happens if the timeout expires?
I think that can only happen if the hardware is seriously broken because
it's always supposed to set XEN. I'll make sure to catch that case in v2
though and print a separate error message similar to how the polling case
below is taken care of right now.
>
>> reg_write(smbus, REG_IMASK, 0);
>> status = reg_read(smbus, REG_SMSTA);
>> } else {
>
> ...
>
>> struct pasemi_smbus *smbus = adapter->algo_data;
>> int ret, i;
>>
>> - pasemi_smb_clear(smbus);
>> + if (pasemi_smb_clear(smbus))
>> + return -EIO;
>
> Can we use
>
> ret = ...
> if (ret)
> return ret;
>
> This way we return whatever comes from pasemi_smb_clear().
>
>>
>> ret = 0;
>
> This way we can remove this line, as well.
Sure, will do both for v2.
Thanks,
Sven
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 3/4] i2c: pasemi: Enable the unjam machine
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
2025-02-22 13:38 ` [PATCH 1/4] i2c: pasemi: Add registers bits and switch to BIT() Sven Peter via B4 Relay
2025-02-22 13:38 ` [PATCH 2/4] i2c: pasemi: Improve error recovery Sven Peter via B4 Relay
@ 2025-02-22 13:38 ` Sven Peter via B4 Relay
2025-02-22 13:38 ` [PATCH 4/4] i2c: pasemi: Log bus reset causes Sven Peter via B4 Relay
` (2 subsequent siblings)
5 siblings, 0 replies; 12+ messages in thread
From: Sven Peter via B4 Relay @ 2025-02-22 13:38 UTC (permalink / raw)
To: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Andi Shyti
Cc: linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Sven Peter, Hector Martin
From: Hector Martin <marcan@marcan.st>
The I2C bus can get stuck under some conditions (desync between
controller and device). The pasemi controllers include an unjam feature
that is enabled on reset, but was being disabled by the driver. Keep it
enabled by explicitly setting the UJM bit in the CTL register. This
should help recover the bus from certain conditions, which would
otherwise remain stuck forever.
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
drivers/i2c/busses/i2c-pasemi-core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
index 770b86b92a10..8f0ba975172f 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.c
+++ b/drivers/i2c/busses/i2c-pasemi-core.c
@@ -73,7 +73,7 @@ static inline int reg_read(struct pasemi_smbus *smbus, int reg)
static void pasemi_reset(struct pasemi_smbus *smbus)
{
- u32 val = (CTL_MTR | CTL_MRR | (smbus->clk_div & CTL_CLK_M));
+ u32 val = (CTL_MTR | CTL_MRR | CTL_UJM | (smbus->clk_div & CTL_CLK_M));
if (smbus->hw_rev >= 6)
val |= CTL_EN;
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 4/4] i2c: pasemi: Log bus reset causes
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
` (2 preceding siblings ...)
2025-02-22 13:38 ` [PATCH 3/4] i2c: pasemi: Enable the unjam machine Sven Peter via B4 Relay
@ 2025-02-22 13:38 ` Sven Peter via B4 Relay
2025-02-24 18:14 ` [PATCH 0/4] Apple/PASemi i2c error recovery fixes Alyssa Rosenzweig
2025-02-24 21:48 ` Neal Gompa
5 siblings, 0 replies; 12+ messages in thread
From: Sven Peter via B4 Relay @ 2025-02-22 13:38 UTC (permalink / raw)
To: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Andi Shyti
Cc: linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Sven Peter, Hector Martin
From: Hector Martin <marcan@marcan.st>
This ensures we get all information we need to debug issues when users
forward us their logs.
Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Sven Peter <sven@svenpeter.dev>
---
drivers/i2c/busses/i2c-pasemi-core.c | 13 ++++++++++---
1 file changed, 10 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/busses/i2c-pasemi-core.c b/drivers/i2c/busses/i2c-pasemi-core.c
index 8f0ba975172f..ae0181a76470 100644
--- a/drivers/i2c/busses/i2c-pasemi-core.c
+++ b/drivers/i2c/busses/i2c-pasemi-core.c
@@ -21,6 +21,7 @@
/* Register offsets */
#define REG_MTXFIFO 0x00
#define REG_MRXFIFO 0x04
+#define REG_XFSTA 0x0c
#define REG_SMSTA 0x14
#define REG_IMASK 0x18
#define REG_CTL 0x1c
@@ -84,7 +85,7 @@ static void pasemi_reset(struct pasemi_smbus *smbus)
static int pasemi_smb_clear(struct pasemi_smbus *smbus)
{
- unsigned int status;
+ unsigned int status, xfstatus;
int timeout = TRANSFER_TIMEOUT_MS;
status = reg_read(smbus, REG_SMSTA);
@@ -95,15 +96,21 @@ static int pasemi_smb_clear(struct pasemi_smbus *smbus)
status = reg_read(smbus, REG_SMSTA);
}
+ xfstatus = reg_read(smbus, REG_XFSTA);
+
if (timeout < 0) {
- dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x)\n", status);
+ dev_warn(smbus->dev, "Bus is still stuck (status 0x%08x xfstatus 0x%08x)\n",
+ status, xfstatus);
return -EIO;
}
/* If any badness happened or there is data in the FIFOs, reset the FIFOs */
if ((status & (SMSTA_MRNE | SMSTA_JMD | SMSTA_MTO | SMSTA_TOM | SMSTA_MTN | SMSTA_MTA)) ||
- !(status & SMSTA_MTE))
+ !(status & SMSTA_MTE)) {
+ dev_warn(smbus->dev, "Issuing reset due to status 0x%08x (xfstatus 0x%08x)\n",
+ status, xfstatus);
pasemi_reset(smbus);
+ }
/* Clear the flags */
reg_write(smbus, REG_SMSTA, status);
--
2.34.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 0/4] Apple/PASemi i2c error recovery fixes
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
` (3 preceding siblings ...)
2025-02-22 13:38 ` [PATCH 4/4] i2c: pasemi: Log bus reset causes Sven Peter via B4 Relay
@ 2025-02-24 18:14 ` Alyssa Rosenzweig
2025-02-24 21:48 ` Neal Gompa
5 siblings, 0 replies; 12+ messages in thread
From: Alyssa Rosenzweig @ 2025-02-24 18:14 UTC (permalink / raw)
To: sven
Cc: Janne Grunau, Madhavan Srinivasan, Michael Ellerman,
Nicholas Piggin, Christophe Leroy, Naveen N Rao, Andi Shyti,
linuxppc-dev, asahi, linux-arm-kernel, linux-i2c, linux-kernel,
Hector Martin
Reviewed-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
Le Sat , Feb 22, 2025 at 01:38:32PM +0000, Sven Peter via B4 Relay a écrit :
> Hi,
>
> This series adds a few fixes/improvements to the error recovery for
> Apple/PASemi i2c controllers.
> The patches have been in our downstream tree and were originally used
> to debug a rare glitch caused by clock strechting but are useful in
> general. We haven't seen the controller misbehave since adding these.
>
> Best,
>
> Sven
>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
> Hector Martin (3):
> i2c: pasemi: Improve error recovery
> i2c: pasemi: Enable the unjam machine
> i2c: pasemi: Log bus reset causes
>
> Sven Peter (1):
> i2c: pasemi: Add registers bits and switch to BIT()
>
> drivers/i2c/busses/i2c-pasemi-core.c | 121 ++++++++++++++++++++++++++---------
> 1 file changed, 92 insertions(+), 29 deletions(-)
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250220-pasemi-fixes-916cb77404ba
>
> Best regards,
> --
> Sven Peter <sven@svenpeter.dev>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 0/4] Apple/PASemi i2c error recovery fixes
2025-02-22 13:38 [PATCH 0/4] Apple/PASemi i2c error recovery fixes Sven Peter via B4 Relay
` (4 preceding siblings ...)
2025-02-24 18:14 ` [PATCH 0/4] Apple/PASemi i2c error recovery fixes Alyssa Rosenzweig
@ 2025-02-24 21:48 ` Neal Gompa
5 siblings, 0 replies; 12+ messages in thread
From: Neal Gompa @ 2025-02-24 21:48 UTC (permalink / raw)
To: sven
Cc: Janne Grunau, Alyssa Rosenzweig, Madhavan Srinivasan,
Michael Ellerman, Nicholas Piggin, Christophe Leroy, Naveen N Rao,
Andi Shyti, linuxppc-dev, asahi, linux-arm-kernel, linux-i2c,
linux-kernel, Hector Martin
On Sat, Feb 22, 2025 at 8:38 AM Sven Peter via B4 Relay
<devnull+sven.svenpeter.dev@kernel.org> wrote:
>
> Hi,
>
> This series adds a few fixes/improvements to the error recovery for
> Apple/PASemi i2c controllers.
> The patches have been in our downstream tree and were originally used
> to debug a rare glitch caused by clock strechting but are useful in
> general. We haven't seen the controller misbehave since adding these.
>
> Best,
>
> Sven
>
> Signed-off-by: Sven Peter <sven@svenpeter.dev>
> ---
> Hector Martin (3):
> i2c: pasemi: Improve error recovery
> i2c: pasemi: Enable the unjam machine
> i2c: pasemi: Log bus reset causes
>
> Sven Peter (1):
> i2c: pasemi: Add registers bits and switch to BIT()
>
> drivers/i2c/busses/i2c-pasemi-core.c | 121 ++++++++++++++++++++++++++---------
> 1 file changed, 92 insertions(+), 29 deletions(-)
> ---
> base-commit: 2014c95afecee3e76ca4a56956a936e23283f05b
> change-id: 20250220-pasemi-fixes-916cb77404ba
>
> Best regards,
> --
> Sven Peter <sven@svenpeter.dev>
>
>
>
LGTM.
Reviewed-by: Neal Gompa <neal@gompa.dev>
--
真実はいつも一つ!/ Always, there's only one truth!
^ permalink raw reply [flat|nested] 12+ messages in thread