[PATCH v1 0/2] i2c: microchip: misc minor fixes

[PATCH v1 0/2] i2c: microchip: misc minor fixes

[Conor Dooley]

Hey all,
Pair of fixes here for some of the logic in the microchip i2c driver.
Both were reported against the Linux driver, which formed the basis for
the U-Boot driver & the issues are present here too.
Thanks,
Conor.

Conor Dooley (2):
  i2c: microchip: fix ack sending logic
  i2c: microchip: fix erroneous late ack send

 drivers/i2c/i2c-microchip.c | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

-- 
2.38.0

[PATCH v1 1/2] i2c: microchip: fix ack sending logic

[Conor Dooley]

"Master receive mode" was not correctly sending ACKs/NACKs in the
interrupt handler. Bring the handling of M_SLAR_ACK, M_RX_DATA_ACKED &
M_RX_DATA_NACKED in line with the Linux driver.

Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
Reported-by: Shravan Chippa <shravan.chippa@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/i2c/i2c-microchip.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-microchip.c
index 12f65d0af7..3a27459386 100644
--- a/drivers/i2c/i2c-microchip.c
+++ b/drivers/i2c/i2c-microchip.c
@@ -2,8 +2,9 @@
 /*
  * Microchip I2C controller driver
  *
- * Copyright (C) 2021 Microchip Technology Inc.
+ * Copyright (C) 2021-2022 Microchip Technology Inc.
  * Padmarao Begari <padmarao.begari@microchip.com>
+ * Conor Dooley <conor.dooley@microchip.com>
  */
 #include <common.h>
 #include <clk.h>
@@ -265,16 +266,27 @@ static int mpfs_i2c_service_handler(struct mpfs_i2c_bus *bus)
 		}
 		break;
 	case STATUS_M_SLAR_ACK:
-		ctrl = readl(bus->base + MPFS_I2C_CTRL);
-		ctrl |= CTRL_AA;
-		writel(ctrl, bus->base + MPFS_I2C_CTRL);
-		if (bus->msg_len == 0) {
+		if (bus->msg_len > 1u) {
+			ctrl = readl(bus->base + MPFS_I2C_CTRL);
+			ctrl |= CTRL_AA;
+			writel(ctrl, bus->base + MPFS_I2C_CTRL);
+		} else if (bus->msg_len == 1u) {
+			ctrl = readl(bus->base + MPFS_I2C_CTRL);
+			ctrl &= ~CTRL_AA;
+			writel(ctrl, bus->base + MPFS_I2C_CTRL);
+		} else {
+			ctrl = readl(bus->base + MPFS_I2C_CTRL);
+			ctrl |= CTRL_AA;
+			writel(ctrl, bus->base + MPFS_I2C_CTRL);
 			/* On the last byte to be transmitted, send STOP */
 			mpfs_i2c_stop(bus);
 			finish = true;
 		}
 		break;
 	case STATUS_M_RX_DATA_ACKED:
+		mpfs_i2c_empty_rx(bus);
+		break;
+	case STATUS_M_RX_DATA_NACKED:
 		mpfs_i2c_empty_rx(bus);
 		if (bus->msg_len == 0) {
 			/* On the last byte to be transmitted, send STOP */
@@ -283,7 +295,6 @@ static int mpfs_i2c_service_handler(struct mpfs_i2c_bus *bus)
 		}
 		break;
 	case STATUS_M_TX_DATA_NACK:
-	case STATUS_M_RX_DATA_NACKED:
 	case STATUS_M_SLAR_NACK:
 	case STATUS_M_SLAW_NACK:
 		bus->msg_err = -ENXIO;
-- 
2.38.0

[PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Conor Dooley]

A late ack is currently being sent at the end of a transfer due to
incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert Ack
bit is being written to the controller's control reg after the last
byte has been received, causing it to sent another byte with the ack.
Instead, the AA flag should be written to the control register when
the penultimate byte is read so it is sent out for the last byte.

Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/i2c/i2c-microchip.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-microchip.c
index 3a27459386..d82b80f535 100644
--- a/drivers/i2c/i2c-microchip.c
+++ b/drivers/i2c/i2c-microchip.c
@@ -224,7 +224,7 @@ static void mpfs_i2c_empty_rx(struct mpfs_i2c_bus *bus)
 		bus->msg_len--;
 	}
 
-	if (bus->msg_len == 0) {
+	if (bus->msg_len <= 1) {
 		ctrl = readl(bus->base + MPFS_I2C_CTRL);
 		ctrl &= ~CTRL_AA;
 		writel(ctrl, bus->base + MPFS_I2C_CTRL);
-- 
2.38.0

Re: [PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Conor.Dooley]

On 26/10/2022 08:49, Conor Dooley wrote:
> A late ack is currently being sent at the end of a transfer due to
> incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert Ack
> bit is being written to the controller's control reg after the last
> byte has been received, causing it to sent another byte with the ack.
> Instead, the AA flag should be written to the control register when
> the penultimate byte is read so it is sent out for the last byte.
> 
> Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")

I had removed this fixes tag but I must have aborted the rebase
in which I did. If nothing else needs changing, please drop it,
otherwise I'll remove it if/when I send a v2.

Thanks,
Conor.

> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>   drivers/i2c/i2c-microchip.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-microchip.c
> index 3a27459386..d82b80f535 100644
> --- a/drivers/i2c/i2c-microchip.c
> +++ b/drivers/i2c/i2c-microchip.c
> @@ -224,7 +224,7 @@ static void mpfs_i2c_empty_rx(struct mpfs_i2c_bus *bus)
>   		bus->msg_len--;
>   	}
>   
> -	if (bus->msg_len == 0) {
> +	if (bus->msg_len <= 1) {
>   		ctrl = readl(bus->base + MPFS_I2C_CTRL);
>   		ctrl &= ~CTRL_AA;
>   		writel(ctrl, bus->base + MPFS_I2C_CTRL);

Re: [PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Padmarao.Begari]

> On Wed, 2022-10-26 at 08:49 +0100, Conor Dooley wrote:
> A late ack is currently being sent at the end of a transfer due to
> incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert Ack
> bit is being written to the controller's control reg after the last
> byte has been received, causing it to sent another byte with the ack.
> Instead, the AA flag should be written to the control register when
> the penultimate byte is read so it is sent out for the last byte.
> 
> Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-
> microchip.c
> index 3a27459386..d82b80f535 100644
> --- a/drivers/i2c/i2c-microchip.c
> +++ b/drivers/i2c/i2c-microchip.c
> @@ -224,7 +224,7 @@ static void mpfs_i2c_empty_rx(struct mpfs_i2c_bus
> *bus)
>  		bus->msg_len--;
>  	}
>  
> -	if (bus->msg_len == 0) {
> +	if (bus->msg_len <= 1) {
>  		ctrl = readl(bus->base + MPFS_I2C_CTRL);
>  		ctrl &= ~CTRL_AA;
>  		writel(ctrl, bus->base + MPFS_I2C_CTRL);

Reviewed-by: Padmarao Begari <padmarao.begari@microchip.com>

Re: [PATCH v1 1/2] i2c: microchip: fix ack sending logic

[Padmarao.Begari]

> On Wed, 2022-10-26 at 08:49 +0100, Conor Dooley wrote:
> "Master receive mode" was not correctly sending ACKs/NACKs in the
> interrupt handler. Bring the handling of M_SLAR_ACK, M_RX_DATA_ACKED
> &
> M_RX_DATA_NACKED in line with the Linux driver.
> 
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Reported-by: Shravan Chippa <shravan.chippa@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-
> microchip.c
> index 12f65d0af7..3a27459386 100644
> --- a/drivers/i2c/i2c-microchip.c
> +++ b/drivers/i2c/i2c-microchip.c
> @@ -2,8 +2,9 @@
>  /*
>   * Microchip I2C controller driver
>   *
> - * Copyright (C) 2021 Microchip Technology Inc.
> + * Copyright (C) 2021-2022 Microchip Technology Inc.
>   * Padmarao Begari <padmarao.begari@microchip.com>
> + * Conor Dooley <conor.dooley@microchip.com>
>   */
>  #include <common.h>
>  #include <clk.h>
> @@ -265,16 +266,27 @@ static int mpfs_i2c_service_handler(struct
> mpfs_i2c_bus *bus)
>  		}
>  		break;
>  	case STATUS_M_SLAR_ACK:
> -		ctrl = readl(bus->base + MPFS_I2C_CTRL);
> -		ctrl |= CTRL_AA;
> -		writel(ctrl, bus->base + MPFS_I2C_CTRL);
> -		if (bus->msg_len == 0) {
> +		if (bus->msg_len > 1u) {
> +			ctrl = readl(bus->base + MPFS_I2C_CTRL);
> +			ctrl |= CTRL_AA;
> +			writel(ctrl, bus->base + MPFS_I2C_CTRL);
> +		} else if (bus->msg_len == 1u) {
> +			ctrl = readl(bus->base + MPFS_I2C_CTRL);
> +			ctrl &= ~CTRL_AA;
> +			writel(ctrl, bus->base + MPFS_I2C_CTRL);
> +		} else {
> +			ctrl = readl(bus->base + MPFS_I2C_CTRL);
> +			ctrl |= CTRL_AA;
> +			writel(ctrl, bus->base + MPFS_I2C_CTRL);
>  			/* On the last byte to be transmitted, send
> STOP */
>  			mpfs_i2c_stop(bus);
>  			finish = true;
>  		}
>  		break;
>  	case STATUS_M_RX_DATA_ACKED:
> +		mpfs_i2c_empty_rx(bus);
> +		break;
> +	case STATUS_M_RX_DATA_NACKED:
>  		mpfs_i2c_empty_rx(bus);
>  		if (bus->msg_len == 0) {
>  			/* On the last byte to be transmitted, send
> STOP */
> @@ -283,7 +295,6 @@ static int mpfs_i2c_service_handler(struct
> mpfs_i2c_bus *bus)
>  		}
>  		break;
>  	case STATUS_M_TX_DATA_NACK:
> -	case STATUS_M_RX_DATA_NACKED:
>  	case STATUS_M_SLAR_NACK:
>  	case STATUS_M_SLAW_NACK:
>  		bus->msg_err = -ENXIO;

Reviewed-by: Padmarao Begari <padmarao.begari@microchip.com>

Re: [PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Padmarao.Begari]

Hi Conor,
> On Wed, 2022-10-26 at 07:54 +0000, Conor Dooley - M52691 wrote:
> On 26/10/2022 08:49, Conor Dooley wrote:
> > A late ack is currently being sent at the end of a transfer due to
> > incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert
> > Ack
> > bit is being written to the controller's control reg after the last
> > byte has been received, causing it to sent another byte with the
> > ack.
> > Instead, the AA flag should be written to the control register when
> > the penultimate byte is read so it is sent out for the last byte.
> > 
> > Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
> > Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> > Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")
> 
> I had removed this fixes tag but I must have aborted the rebase
> in which I did. If nothing else needs changing, please drop it,
> otherwise I'll remove it if/when I send a v2.
> 
Yes you can remove it because the patch check is showing warning for
this fixes tag

Regards
Padmarao
> Thanks,
> Conor.
> 
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > ---
> >   drivers/i2c/i2c-microchip.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/i2c/i2c-microchip.c b/drivers/i2c/i2c-
> > microchip.c
> > index 3a27459386..d82b80f535 100644
> > --- a/drivers/i2c/i2c-microchip.c
> > +++ b/drivers/i2c/i2c-microchip.c
> > @@ -224,7 +224,7 @@ static void mpfs_i2c_empty_rx(struct
> > mpfs_i2c_bus *bus)
> >   		bus->msg_len--;
> >   	}
> >   
> > -	if (bus->msg_len == 0) {
> > +	if (bus->msg_len <= 1) {
> >   		ctrl = readl(bus->base + MPFS_I2C_CTRL);
> >   		ctrl &= ~CTRL_AA;
> >   		writel(ctrl, bus->base + MPFS_I2C_CTRL);

Re: [PATCH v1 1/2] i2c: microchip: fix ack sending logic

[Heiko Schocher]

Hello Conor,

On 26.10.22 09:49, Conor Dooley wrote:
> "Master receive mode" was not correctly sending ACKs/NACKs in the
> interrupt handler. Bring the handling of M_SLAR_ACK, M_RX_DATA_ACKED &
> M_RX_DATA_NACKED in line with the Linux driver.
> 
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Reported-by: Shravan Chippa <shravan.chippa@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)


Thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Heiko Schocher]

Hello Conor,

On 26.10.22 09:49, Conor Dooley wrote:
> A late ack is currently being sent at the end of a transfer due to
> incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert Ack
> bit is being written to the controller's control reg after the last
> byte has been received, causing it to sent another byte with the ack.
> Instead, the AA flag should be written to the control register when
> the penultimate byte is read so it is sent out for the last byte.
> 
> Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)


Thanks!

Reviewed-by: Heiko Schocher <hs@denx.de>

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v1 1/2] i2c: microchip: fix ack sending logic

[Heiko Schocher]

Hello Conor,

On 26.10.22 09:49, Conor Dooley wrote:
> "Master receive mode" was not correctly sending ACKs/NACKs in the
> interrupt handler. Bring the handling of M_SLAR_ACK, M_RX_DATA_ACKED &
> M_RX_DATA_NACKED in line with the Linux driver.
> 
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Reported-by: Shravan Chippa <shravan.chippa@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)

Applied to u-boot-i2c.git master

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de

Re: [PATCH v1 2/2] i2c: microchip: fix erroneous late ack send

[Heiko Schocher]

Hello Conor,

On 26.10.22 09:49, Conor Dooley wrote:
> A late ack is currently being sent at the end of a transfer due to
> incorrect logic in mchp_corei2c_empty_rx(). Currently the Assert Ack
> bit is being written to the controller's control reg after the last
> byte has been received, causing it to sent another byte with the ack.
> Instead, the AA flag should be written to the control register when
> the penultimate byte is read so it is sent out for the last byte.
> 
> Reported-by: Andreas Buerkler <andreas.buerkler@enclustra.com>
> Fixes: 0dc0d1e094 ("i2c: Add Microchip PolarFire SoC I2C driver")
> Fixes: 0190d48488 ("i2c: microchip: fix ack sending logic")
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  drivers/i2c/i2c-microchip.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied after removing wrong Fixes tag to u-boot-i2c.git master

Thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs@denx.de