[U-Boot] [PATCH v2 1/6] i2c: rcar_i2c: Setup SCL/SDA delay at rcar_i2c_set_speed

[U-Boot] [PATCH v2 1/6] i2c: rcar_i2c: Setup SCL/SDA delay at rcar_i2c_set_speed

[Ismael Luceno Cortes]

It only needs to be done once.

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---
 drivers/i2c/rcar_i2c.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 10b0f8bad4..74643b085e 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -116,9 +116,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
 	writel(0, priv->base + RCAR_I2C_ICMSR);
 	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
 
-	if (priv->type == RCAR_I2C_TYPE_GEN3)
-		writel(RCAR_I2C_ICFBSCR_TCYC17, priv->base + RCAR_I2C_ICFBSCR);
-
+	/* Wait for the bus */
 	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
 				RCAR_I2C_ICMCR_FSDA, false, 2, true);
 	if (ret) {
@@ -304,6 +302,10 @@ scgd_find:
 	priv->icccr = (scgd << RCAR_I2C_ICCCR_SCGD_OFF) | cdf;
 	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
 
+	if (priv->type == RCAR_I2C_TYPE_GEN3)
+		/* Set SCL/SDA delay */
+		writel(RCAR_I2C_ICFBSCR_TCYC17, priv->base + RCAR_I2C_ICFBSCR);
+
 	return 0;
 }
 

base-commit: f08023c07d826fbc8e62fdd3367961b2f0b06844
prerequisite-patch-id: 9e5b0458bc15640eb483ccad91dbe85150f9f7be
-- 
2.19.1

[U-Boot] [PATCH v2 2/6] i2c: rcar_i2c: Add comments about registers & values

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---

Notes:
    Changes since v1:
    - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")

 drivers/i2c/rcar_i2c.c | 45 +++++++++++++++++++++---------------------
 1 file changed, 23 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 74643b085e..c1a233b6e9 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -18,35 +18,36 @@
 #include <asm/io.h>
 #include <wait_bit.h>
 
-#define RCAR_I2C_ICSCR			0x00
-#define RCAR_I2C_ICMCR			0x04
-#define RCAR_I2C_ICMCR_MDBS		BIT(7)
-#define RCAR_I2C_ICMCR_FSCL		BIT(6)
-#define RCAR_I2C_ICMCR_FSDA		BIT(5)
-#define RCAR_I2C_ICMCR_OBPC		BIT(4)
-#define RCAR_I2C_ICMCR_MIE		BIT(3)
+#define RCAR_I2C_ICSCR			0x00   /* slave ctrl */
+#define RCAR_I2C_ICMCR			0x04   /* master ctrl */
+#define RCAR_I2C_ICMCR_MDBS		BIT(7) /* non-fifo mode switch */
+#define RCAR_I2C_ICMCR_FSCL		BIT(6) /* override SCL pin */
+#define RCAR_I2C_ICMCR_FSDA		BIT(5) /* override SDA pin */
+#define RCAR_I2C_ICMCR_OBPC		BIT(4) /* override pins */
+#define RCAR_I2C_ICMCR_MIE		BIT(3) /* master if enable */
 #define RCAR_I2C_ICMCR_TSBE		BIT(2)
-#define RCAR_I2C_ICMCR_FSB		BIT(1)
-#define RCAR_I2C_ICMCR_ESG		BIT(0)
-#define RCAR_I2C_ICSSR			0x08
-#define RCAR_I2C_ICMSR			0x0c
+#define RCAR_I2C_ICMCR_FSB		BIT(1) /* force stop bit */
+#define RCAR_I2C_ICMCR_ESG		BIT(0) /* enable start bit gen */
+#define RCAR_I2C_ICSSR			0x08   /* slave status */
+#define RCAR_I2C_ICMSR			0x0c   /* master status */
 #define RCAR_I2C_ICMSR_MASK		0x7f
-#define RCAR_I2C_ICMSR_MNR		BIT(6)
-#define RCAR_I2C_ICMSR_MAL		BIT(5)
-#define RCAR_I2C_ICMSR_MST		BIT(4)
+#define RCAR_I2C_ICMSR_MNR		BIT(6) /* Nack */
+#define RCAR_I2C_ICMSR_MAL		BIT(5) /* Arbitration lost */
+#define RCAR_I2C_ICMSR_MST		BIT(4) /* Stop */
 #define RCAR_I2C_ICMSR_MDE		BIT(3)
 #define RCAR_I2C_ICMSR_MDT		BIT(2)
 #define RCAR_I2C_ICMSR_MDR		BIT(1)
 #define RCAR_I2C_ICMSR_MAT		BIT(0)
-#define RCAR_I2C_ICSIER			0x10
-#define RCAR_I2C_ICMIER			0x14
-#define RCAR_I2C_ICCCR			0x18
+#define RCAR_I2C_ICSIER			0x10   /* slave irq enable */
+#define RCAR_I2C_ICMIER			0x14   /* master irq enable */
+#define RCAR_I2C_ICCCR			0x18   /* clock dividers */
 #define RCAR_I2C_ICCCR_SCGD_OFF		3
-#define RCAR_I2C_ICSAR			0x1c
-#define RCAR_I2C_ICMAR			0x20
-#define RCAR_I2C_ICRXD_ICTXD		0x24
-#define RCAR_I2C_ICFBSCR		0x38
-#define RCAR_I2C_ICFBSCR_TCYC17		0x0f
+#define RCAR_I2C_ICSAR			0x1c   /* slave address */
+#define RCAR_I2C_ICMAR			0x20   /* master address */
+#define RCAR_I2C_ICRXD_ICTXD		0x24   /* data port */
+#define RCAR_I2C_ICFBSCR		0x38   /* First Bit Setup Cycle (Gen3) */
+#define RCAR_I2C_ICFBSCR_TCYC17		0x0f   /* 17*Tcyc delay 1st bit
+						  between SDA and SCL */
 
 enum rcar_i2c_type {
 	RCAR_I2C_TYPE_GEN2,
-- 
2.19.1

[U-Boot] [PATCH v2 3/6] i2c: rcar_i2c: Fix sending of slave addresses

[Ismael Luceno Cortes]

Do the reset before clearing the MSR, otherwise it may result in a read
or write operation instead if the start condition is repeated.

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---

Notes:
    Changes since v1:
    - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")

 drivers/i2c/rcar_i2c.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index c1a233b6e9..7131f0c994 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -130,9 +130,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
 	}
 
 	writel((chip << 1) | read, priv->base + RCAR_I2C_ICMAR);
-	writel(0, priv->base + RCAR_I2C_ICMSR);
+	/* Reset */
 	writel(RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE | RCAR_I2C_ICMCR_ESG,
 	       priv->base + RCAR_I2C_ICMCR);
+	/* Clear Status */
+	writel(0, priv->base + RCAR_I2C_ICMSR);
 
 	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMSR, mask,
 				true, 100, true);
-- 
2.19.1

[U-Boot] [PATCH v2 4/6] i2c: rcar_i2c: Don't mask errors with EREMOTEIO at rcar_i2c_xfer

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---

Notes:
    Changes since v1:
    - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")

 drivers/i2c/rcar_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 7131f0c994..d6b0418290 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -217,7 +217,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 			ret = rcar_i2c_write_common(dev, msg);
 
 		if (ret)
-			return -EREMOTEIO;
+			return ret;
 	}
 
 	return ret;
-- 
2.19.1

[U-Boot] [PATCH v2 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Ismael Luceno Cortes]

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---

Notes:
    Changes since v1:
    - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
    - Fixed masking of return value from rcar_i2c_set_addr

 drivers/i2c/rcar_i2c.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index d6b0418290..4556b115bd 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -154,10 +154,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
 	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
 	int i, ret = -EREMOTEIO;
 
-	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
-	if (ret)
-		return ret;
-
 	for (i = 0; i < msg->len; i++) {
 		if (msg->len - 1 == i)
 			icmcr |= RCAR_I2C_ICMCR_FSB;
@@ -184,10 +180,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
 	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
 	int i, ret = -EREMOTEIO;
 
-	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
-	if (ret)
-		return ret;
-
 	for (i = 0; i < msg->len; i++) {
 		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
 		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
@@ -211,6 +203,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 	int ret;
 
 	for (; nmsgs > 0; nmsgs--, msg++) {
+		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
+		if (ret)
+			return ret;
+
 		if (msg->flags & I2C_M_RD)
 			ret = rcar_i2c_read_common(dev, msg);
 		else
@@ -220,7 +216,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
 			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int rcar_i2c_probe_chip(struct udevice *dev, uint addr, uint flags)
-- 
2.19.1

[U-Boot] [PATCH v2 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Ismael Luceno Cortes]

Cosmetic change.  Any call to the recover function would need to do the
same check afterwards, so it's sensible to make it part of the function.

Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
---

Notes:
    Changes since v1:
    - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
    - Explained the change
    - Replaced C99-style variable declaration

 drivers/i2c/rcar_i2c.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
index 4556b115bd..21bcf09101 100644
--- a/drivers/i2c/rcar_i2c.c
+++ b/drivers/i2c/rcar_i2c.c
@@ -77,12 +77,13 @@ static int rcar_i2c_finish(struct udevice *dev)
 	return ret;
 }
 
-static void rcar_i2c_recover(struct udevice *dev)
+static int rcar_i2c_recover(struct udevice *dev)
 {
 	struct rcar_i2c_priv *priv = dev_get_priv(dev);
 	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
 	u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
 	int i;
+	u32 ret;
 
 	/* Send 9 SCL pulses */
 	for (i = 0; i < 9; i++) {
@@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
 	udelay(5);
 	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
 	udelay(5);
+
+	ret = readl(priv->base + RCAR_I2C_ICMSR);
+	return ret & RCAR_I2C_ICMCR_FSDA;
 }
 
 static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
@@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
 	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
 				RCAR_I2C_ICMCR_FSDA, false, 2, true);
 	if (ret) {
-		rcar_i2c_recover(dev);
-		val = readl(priv->base + RCAR_I2C_ICMSR);
-		if (val & RCAR_I2C_ICMCR_FSDA) {
+		if (rcar_i2c_recover(dev)) {
 			dev_err(dev, "Bus busy, aborting\n");
 			return ret;
 		}
-- 
2.19.1

[U-Boot] [PATCH v2 1/6] i2c: rcar_i2c: Setup SCL/SDA delay at rcar_i2c_set_speed

[Marek Vasut]

On 3/5/19 12:16 PM, Ismael Luceno Cortes wrote:
> It only needs to be done once.

As much as I hate to do it, I need to ask you to reword the commit
message again.

When I look at just the commit message and I read "It only needs to be
done once.", I literally have no clue what the change does. I need to
look into the patch.

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
>  drivers/i2c/rcar_i2c.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 10b0f8bad4..74643b085e 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -116,9 +116,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>  	writel(0, priv->base + RCAR_I2C_ICMSR);
>  	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
>  
> -	if (priv->type == RCAR_I2C_TYPE_GEN3)
> -		writel(RCAR_I2C_ICFBSCR_TCYC17, priv->base + RCAR_I2C_ICFBSCR);
> -
> +	/* Wait for the bus */
>  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
>  				RCAR_I2C_ICMCR_FSDA, false, 2, true);
>  	if (ret) {
> @@ -304,6 +302,10 @@ scgd_find:
>  	priv->icccr = (scgd << RCAR_I2C_ICCCR_SCGD_OFF) | cdf;
>  	writel(priv->icccr, priv->base + RCAR_I2C_ICCCR);
>  
> +	if (priv->type == RCAR_I2C_TYPE_GEN3)
> +		/* Set SCL/SDA delay */
> +		writel(RCAR_I2C_ICFBSCR_TCYC17, priv->base + RCAR_I2C_ICFBSCR);

Please add brackets around the multi-line code, so it's obvious what is
in the if conditional and what is not.

>  	return 0;
>  }
>  
> 
> base-commit: f08023c07d826fbc8e62fdd3367961b2f0b06844
> prerequisite-patch-id: 9e5b0458bc15640eb483ccad91dbe85150f9f7be
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 2/6] i2c: rcar_i2c: Add comments about registers & values

[Marek Vasut]

On 3/5/19 12:16 PM, Ismael Luceno Cortes wrote:

Commit message is missing :(

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
> 
> Notes:
>     Changes since v1:
>     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")

Thanks!

>  drivers/i2c/rcar_i2c.c | 45 +++++++++++++++++++++---------------------
>  1 file changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 74643b085e..c1a233b6e9 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -18,35 +18,36 @@
>  #include <asm/io.h>
>  #include <wait_bit.h>
>  
> -#define RCAR_I2C_ICSCR			0x00
> -#define RCAR_I2C_ICMCR			0x04
> -#define RCAR_I2C_ICMCR_MDBS		BIT(7)
> -#define RCAR_I2C_ICMCR_FSCL		BIT(6)
> -#define RCAR_I2C_ICMCR_FSDA		BIT(5)
> -#define RCAR_I2C_ICMCR_OBPC		BIT(4)
> -#define RCAR_I2C_ICMCR_MIE		BIT(3)
> +#define RCAR_I2C_ICSCR			0x00   /* slave ctrl */
> +#define RCAR_I2C_ICMCR			0x04   /* master ctrl */
> +#define RCAR_I2C_ICMCR_MDBS		BIT(7) /* non-fifo mode switch */
> +#define RCAR_I2C_ICMCR_FSCL		BIT(6) /* override SCL pin */
> +#define RCAR_I2C_ICMCR_FSDA		BIT(5) /* override SDA pin */
> +#define RCAR_I2C_ICMCR_OBPC		BIT(4) /* override pins */
> +#define RCAR_I2C_ICMCR_MIE		BIT(3) /* master if enable */
>  #define RCAR_I2C_ICMCR_TSBE		BIT(2)
> -#define RCAR_I2C_ICMCR_FSB		BIT(1)
> -#define RCAR_I2C_ICMCR_ESG		BIT(0)
> -#define RCAR_I2C_ICSSR			0x08
> -#define RCAR_I2C_ICMSR			0x0c
> +#define RCAR_I2C_ICMCR_FSB		BIT(1) /* force stop bit */
> +#define RCAR_I2C_ICMCR_ESG		BIT(0) /* enable start bit gen */
> +#define RCAR_I2C_ICSSR			0x08   /* slave status */
> +#define RCAR_I2C_ICMSR			0x0c   /* master status */
>  #define RCAR_I2C_ICMSR_MASK		0x7f
> -#define RCAR_I2C_ICMSR_MNR		BIT(6)
> -#define RCAR_I2C_ICMSR_MAL		BIT(5)
> -#define RCAR_I2C_ICMSR_MST		BIT(4)
> +#define RCAR_I2C_ICMSR_MNR		BIT(6) /* Nack */
> +#define RCAR_I2C_ICMSR_MAL		BIT(5) /* Arbitration lost */
> +#define RCAR_I2C_ICMSR_MST		BIT(4) /* Stop */
>  #define RCAR_I2C_ICMSR_MDE		BIT(3)
>  #define RCAR_I2C_ICMSR_MDT		BIT(2)
>  #define RCAR_I2C_ICMSR_MDR		BIT(1)
>  #define RCAR_I2C_ICMSR_MAT		BIT(0)
> -#define RCAR_I2C_ICSIER			0x10
> -#define RCAR_I2C_ICMIER			0x14
> -#define RCAR_I2C_ICCCR			0x18
> +#define RCAR_I2C_ICSIER			0x10   /* slave irq enable */
> +#define RCAR_I2C_ICMIER			0x14   /* master irq enable */
> +#define RCAR_I2C_ICCCR			0x18   /* clock dividers */
>  #define RCAR_I2C_ICCCR_SCGD_OFF		3
> -#define RCAR_I2C_ICSAR			0x1c
> -#define RCAR_I2C_ICMAR			0x20
> -#define RCAR_I2C_ICRXD_ICTXD		0x24
> -#define RCAR_I2C_ICFBSCR		0x38
> -#define RCAR_I2C_ICFBSCR_TCYC17		0x0f
> +#define RCAR_I2C_ICSAR			0x1c   /* slave address */
> +#define RCAR_I2C_ICMAR			0x20   /* master address */
> +#define RCAR_I2C_ICRXD_ICTXD		0x24   /* data port */
> +#define RCAR_I2C_ICFBSCR		0x38   /* First Bit Setup Cycle (Gen3) */
> +#define RCAR_I2C_ICFBSCR_TCYC17		0x0f   /* 17*Tcyc delay 1st bit
> +						  between SDA and SCL */
>  
>  enum rcar_i2c_type {
>  	RCAR_I2C_TYPE_GEN2,

Checkpatch is complaining:

WARNING: line over 80 characters
#137: FILE: drivers/i2c/rcar_i2c.c:47:
+#define RCAR_I2C_ICFBSCR               0x38   /* First Bit Setup Cycle
(Gen3) */

WARNING: Block comments use * on subsequent lines
#139: FILE: drivers/i2c/rcar_i2c.c:49:
+#define RCAR_I2C_ICFBSCR_TCYC17                0x0f   /* 17*Tcyc delay
1st bit
+                                                 between SDA and SCL */

WARNING: Block comments use a trailing */ on a separate line
#139: FILE: drivers/i2c/rcar_i2c.c:49:
+                                                 between SDA and SCL */

total: 1 errors, 3 warnings, 0 checks, 60 lines checked

Please fix and run scripts/checkpatch.pl on the patches before posting them.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 3/6] i2c: rcar_i2c: Fix sending of slave addresses

[Marek Vasut]

On 3/5/19 12:16 PM, Ismael Luceno Cortes wrote:
> Do the reset before clearing the MSR, otherwise it may result in a read
> or write operation instead if the start condition is repeated.
> 
> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>

Reviewed-by: Marek Vasut <marek.vasut+renesas@gmail.com>

> ---
> 
> Notes:
>     Changes since v1:
>     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
> 
>  drivers/i2c/rcar_i2c.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index c1a233b6e9..7131f0c994 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -130,9 +130,11 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>  	}
>  
>  	writel((chip << 1) | read, priv->base + RCAR_I2C_ICMAR);
> -	writel(0, priv->base + RCAR_I2C_ICMSR);
> +	/* Reset */
>  	writel(RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE | RCAR_I2C_ICMCR_ESG,
>  	       priv->base + RCAR_I2C_ICMCR);
> +	/* Clear Status */
> +	writel(0, priv->base + RCAR_I2C_ICMSR);
>  
>  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMSR, mask,
>  				true, 100, true);
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 4/6] i2c: rcar_i2c: Don't mask errors with EREMOTEIO at rcar_i2c_xfer

[Marek Vasut]

On 3/5/19 12:16 PM, Ismael Luceno Cortes wrote:

Commit message is missing :-(

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
> 
> Notes:
>     Changes since v1:
>     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
> 
>  drivers/i2c/rcar_i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 7131f0c994..d6b0418290 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -217,7 +217,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>  			ret = rcar_i2c_write_common(dev, msg);
>  
>  		if (ret)
> -			return -EREMOTEIO;
> +			return ret;
>  	}
>  
>  	return ret;
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 5/6] i2c: rcar_i2c: Set the slave address from rcar_i2c_xfer

[Marek Vasut]

On 3/5/19 12:16 PM, Ismael Luceno Cortes wrote:

Commit message is missing :-(

The patch itself is fine though. Can you fill the commit message and do
a V3 of the series ?

> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
> 
> Notes:
>     Changes since v1:
>     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
>     - Fixed masking of return value from rcar_i2c_set_addr
> 
>  drivers/i2c/rcar_i2c.c | 14 +++++---------
>  1 file changed, 5 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index d6b0418290..4556b115bd 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -154,10 +154,6 @@ static int rcar_i2c_read_common(struct udevice *dev, struct i2c_msg *msg)
>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>  	int i, ret = -EREMOTEIO;
>  
> -	ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> -	if (ret)
> -		return ret;
> -
>  	for (i = 0; i < msg->len; i++) {
>  		if (msg->len - 1 == i)
>  			icmcr |= RCAR_I2C_ICMCR_FSB;
> @@ -184,10 +180,6 @@ static int rcar_i2c_write_common(struct udevice *dev, struct i2c_msg *msg)
>  	u32 icmcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_MIE;
>  	int i, ret = -EREMOTEIO;
>  
> -	ret = rcar_i2c_set_addr(dev, msg->addr, 0);
> -	if (ret)
> -		return ret;
> -
>  	for (i = 0; i < msg->len; i++) {
>  		writel(msg->buf[i], priv->base + RCAR_I2C_ICRXD_ICTXD);
>  		writel(icmcr, priv->base + RCAR_I2C_ICMCR);
> @@ -211,6 +203,10 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>  	int ret;
>  
>  	for (; nmsgs > 0; nmsgs--, msg++) {
> +		ret = rcar_i2c_set_addr(dev, msg->addr, 1);
> +		if (ret)
> +			return ret;
> +
>  		if (msg->flags & I2C_M_RD)
>  			ret = rcar_i2c_read_common(dev, msg);
>  		else
> @@ -220,7 +216,7 @@ static int rcar_i2c_xfer(struct udevice *dev, struct i2c_msg *msg, int nmsgs)
>  			return ret;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int rcar_i2c_probe_chip(struct udevice *dev, uint addr, uint flags)
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Marek Vasut]

On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote:
> Cosmetic change.  Any call to the recover function would need to do the
> same check afterwards, so it's sensible to make it part of the function.
> 
> Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> ---
> 
> Notes:
>     Changes since v1:
>     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
>     - Explained the change
>     - Replaced C99-style variable declaration
> 
>  drivers/i2c/rcar_i2c.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> index 4556b115bd..21bcf09101 100644
> --- a/drivers/i2c/rcar_i2c.c
> +++ b/drivers/i2c/rcar_i2c.c
> @@ -77,12 +77,13 @@ static int rcar_i2c_finish(struct udevice *dev)
>  	return ret;
>  }
>  
> -static void rcar_i2c_recover(struct udevice *dev)
> +static int rcar_i2c_recover(struct udevice *dev)
>  {
>  	struct rcar_i2c_priv *priv = dev_get_priv(dev);
>  	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
>  	u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
>  	int i;
> +	u32 ret;

Can you change this to "reg" or something like that ? $ret is usually a
signed int return value, so this could be confusing.

>  
>  	/* Send 9 SCL pulses */
>  	for (i = 0; i < 9; i++) {
> @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
>  	udelay(5);
>  	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
>  	udelay(5);
> +
> +	ret = readl(priv->base + RCAR_I2C_ICMSR);
> +	return ret & RCAR_I2C_ICMCR_FSDA;

What about doing this instead

reg = readl...
if (reg & ...FSDA)
	return -EBUSY;

return 0;

That way, the code will use standard errno.h return values.

>  }
>  
>  static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
> @@ -121,9 +125,7 @@ static int rcar_i2c_set_addr(struct udevice *dev, u8 chip, u8 read)
>  	ret = wait_for_bit_le32(priv->base + RCAR_I2C_ICMCR,
>  				RCAR_I2C_ICMCR_FSDA, false, 2, true);
>  	if (ret) {
> -		rcar_i2c_recover(dev);
> -		val = readl(priv->base + RCAR_I2C_ICMSR);
> -		if (val & RCAR_I2C_ICMCR_FSDA) {
> +		if (rcar_i2c_recover(dev)) {
>  			dev_err(dev, "Bus busy, aborting\n");
>  			return ret;
>  		}
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH v2 6/6] i2c: rcar_i2c: Move FSDA check to rcar_i2c_recover

[Ismael Luceno Cortes]

On 05/Mar/2019 19:32, Marek Vasut wrote:
> On 3/5/19 12:23 PM, Ismael Luceno Cortes wrote:
> > Cosmetic change.  Any call to the recover function would need to do the
> > same check afterwards, so it's sensible to make it part of the function.
> > 
> > Signed-off-by: Ismael Luceno <ismael.luceno@silicon-gears.com>
> > ---
> > 
> > Notes:
> >     Changes since v1:
> >     - Rebased on top of patch 1050650 ("i2c: rcar_i2c: Add Gen3 SoC support")
> >     - Explained the change
> >     - Replaced C99-style variable declaration
> > 
> >  drivers/i2c/rcar_i2c.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/i2c/rcar_i2c.c b/drivers/i2c/rcar_i2c.c
> > index 4556b115bd..21bcf09101 100644
> > --- a/drivers/i2c/rcar_i2c.c
> > +++ b/drivers/i2c/rcar_i2c.c
> > @@ -77,12 +77,13 @@ static int rcar_i2c_finish(struct udevice *dev)
> >  	return ret;
> >  }
> >  
> > -static void rcar_i2c_recover(struct udevice *dev)
> > +static int rcar_i2c_recover(struct udevice *dev)
> >  {
> >  	struct rcar_i2c_priv *priv = dev_get_priv(dev);
> >  	u32 mcr = RCAR_I2C_ICMCR_MDBS | RCAR_I2C_ICMCR_OBPC;
> >  	u32 mcra = mcr | RCAR_I2C_ICMCR_FSDA;
> >  	int i;
> > +	u32 ret;
> 
> Can you change this to "reg" or something like that ? $ret is usually a
> signed int return value, so this could be confusing.

Ok.
 
> >  
> >  	/* Send 9 SCL pulses */
> >  	for (i = 0; i < 9; i++) {
> > @@ -102,6 +103,9 @@ static void rcar_i2c_recover(struct udevice *dev)
> >  	udelay(5);
> >  	writel(mcra | RCAR_I2C_ICMCR_FSCL, priv->base + RCAR_I2C_ICMCR);
> >  	udelay(5);
> > +
> > +	ret = readl(priv->base + RCAR_I2C_ICMSR);
> > +	return ret & RCAR_I2C_ICMCR_FSDA;
> 
> What about doing this instead
> 
> reg = readl...
> if (reg & ...FSDA)
> 	return -EBUSY;
> 
> return 0;
> 
> That way, the code will use standard errno.h return values.

Makes sense.