[U-Boot] [PATCH] i2c: pcf2127: fix bug that read wrong time

[U-Boot] [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

Because i2c driver does not generate a stop bit when reading registers
of pcf2127

Signed-off-by: Biwen Li <biwen.li@nxp.com>
Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
---
 drivers/i2c/i2c-uclass.c |  2 ++
 drivers/i2c/mxc_i2c.c    | 70 +++++++++++++++++++++++++++++++++++-----
 drivers/rtc/pcf2127.c    | 18 ++++++-----
 include/i2c.h            |  2 ++
 4 files changed, 76 insertions(+), 16 deletions(-)

diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
index e47abf1833..18f7364d72 100644
--- a/drivers/i2c/i2c-uclass.c
+++ b/drivers/i2c/i2c-uclass.c
@@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset, uint8_t *buffer, int len)
 	if (len) {
 		ptr->addr = chip->chip_addr;
 		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
+		ptr->flags |= chip->flags & DM_I2C_CHIP_RD_NEED_STOP_BIT ?
+			      I2C_M_RD_NEED_STOP_BIT : 0;
 		ptr->flags |= I2C_M_RD;
 		ptr->len = len;
 		ptr->buf = buffer;
diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
index 73b9807598..241367bac8 100644
--- a/drivers/i2c/mxc_i2c.c
+++ b/drivers/i2c/mxc_i2c.c
@@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 	int reg_shift = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
 		VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
 	int read_mode;
+	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true : false;
+	unsigned int temp;
 
 	/* Here address len is set to -1 to not send any address at first.
 	 * Otherwise i2c_init_transfer will send the chip address with write
@@ -976,6 +978,7 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 	read_mode = -1; /* So it's always different on the first message */
 	for (; nmsgs > 0; nmsgs--, msg++) {
 		const int msg_is_read = !!(msg->flags & I2C_M_RD);
+		bool next_is_read = nmsgs > 1 && (msg[1].flags & I2C_M_RD);
 
 		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n", msg->addr,
 		      msg->len, msg_is_read ? 'R' : 'W');
@@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 		if (msg_is_read != read_mode) {
 			/* Send repeated start if not 1st message */
 			if (read_mode != -1) {
-				debug("i2c_xfer: [RSTART]\n");
-				ret = readb(base + (I2CR << reg_shift));
-				ret |= I2CR_RSTA;
-				writeb(ret, base + (I2CR << reg_shift));
+				if (!(msg[1].flags & I2C_M_RD_NEED_STOP_BIT)) {
+					debug("i2c_xfer: [RSTART]\n");
+					ret = readb(base + (I2CR << reg_shift));
+					ret |= I2CR_RSTA;
+					writeb(ret, base + (I2CR << reg_shift));
+				}
 			}
 			debug("i2c_xfer: [ADDR %02x | %c]\n", msg->addr,
 			      msg_is_read ? 'R' : 'W');
+
 			ret = tx_byte(i2c_bus, (msg->addr << 1) | msg_is_read);
 			if (ret < 0) {
 				debug("i2c_xfer: [STOP]\n");
@@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
 			read_mode = msg_is_read;
 		}
 
-		if (msg->flags & I2C_M_RD)
+		if (msg->flags & I2C_M_RD) {
 			ret = i2c_read_data(i2c_bus, msg->addr, msg->buf,
 					    msg->len, nmsgs == 1 ||
 						      (msg->flags & I2C_M_STOP));
-		else
-			ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
-					     msg->len);
+			if (ret < 0)
+				break;
+			continue;
+		}
 
+		 /* Write message */
+		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
+				     msg->len);
 		if (ret < 0)
 			break;
+
+		if (!next_is_read)
+			continue;
+
+		/* Read message following write message */
+		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
+			/* Generate a stop bit */
+			i2c_imx_stop(i2c_bus);
+			/* Reset i2c slave */
+			i2c_force_reset_slave();
+
+			/* Enable I2C controller */
+			if (quirk)
+				ret = readb(base + (I2CR << reg_shift))
+					& I2CR_IDIS;
+			else
+				ret = !(readb(base + (I2CR << reg_shift))
+						& I2CR_IEN);
+			if (ret) {
+				writeb(I2CR_IEN, base + (I2CR << reg_shift));
+				/* Wait for controller to be stable */
+				udelay(50);
+			}
+
+			/* Clear interrupt bit */
+			writeb(I2SR_IIF_CLEAR, base + (I2SR << reg_shift));
+			ret = wait_for_sr_state(i2c_bus, ST_BUS_IDLE);
+			if (ret < 0)
+				return ret;
+
+			/* Start I2C transaction */
+			temp = readb(base + (I2CR << reg_shift));
+			temp |= I2CR_MSTA;
+			writeb(temp, base + (I2CR << reg_shift));
+
+			ret = wait_for_sr_state(i2c_bus, ST_BUS_BUSY);
+			if (ret < 0)
+				return ret;
+
+			/* Enter transfer mode */
+			temp |= I2CR_MTX | I2CR_TX_NO_AK;
+			writeb(temp, base + (I2CR << reg_shift));
+			udelay(50);
+		}
 	}
 
 	if (ret)
diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
index dcf0340b4d..010c45ecbf 100644
--- a/drivers/rtc/pcf2127.c
+++ b/drivers/rtc/pcf2127.c
@@ -24,12 +24,9 @@
 
 static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 {
-	uchar buf[8];
+	uchar buf[7] = {0};
 	int i = 0, ret;
 
-	/* start register address */
-	buf[i++] = PCF2127_REG_SC;
-
 	/* hours, minutes and seconds */
 	buf[i++] = bin2bcd(tm->tm_sec);
 	buf[i++] = bin2bcd(tm->tm_min);
@@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time *tm)
 	buf[i++] = bin2bcd(tm->tm_year % 100);
 
 	/* write register's data */
-	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
+	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
 
 	return ret;
 }
@@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
 	int ret = 0;
 	uchar buf[10] = { PCF2127_REG_CTRL1 };
 
-	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
-	if (ret < 0)
-		return ret;
 	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;
@@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice *dev)
 	return 0;
 }
 
+static int pcf2127_probe(struct udevice *dev)
+{
+	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
+
+	return 0;
+}
+
 static const struct rtc_ops pcf2127_rtc_ops = {
 	.get = pcf2127_rtc_get,
 	.set = pcf2127_rtc_set,
@@ -104,6 +105,7 @@ static const struct udevice_id pcf2127_rtc_ids[] = {
 U_BOOT_DRIVER(rtc_pcf2127) = {
 	.name	= "rtc-pcf2127",
 	.id	= UCLASS_RTC,
+	.probe	= pcf2127_probe,
 	.of_match = pcf2127_rtc_ids,
 	.ops	= &pcf2127_rtc_ops,
 };
diff --git a/include/i2c.h b/include/i2c.h
index a5c760c711..beaa028349 100644
--- a/include/i2c.h
+++ b/include/i2c.h
@@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
 	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit addressing */
 	DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send address for each read byte */
 	DM_I2C_CHIP_WR_ADDRESS	= 1 << 2, /* Send address for each write byte */
+	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate stop bit */
 };
 
 struct udevice;
@@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
 	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK */
 	I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit on reads */
 	I2C_M_RECV_LEN		= 0x0400, /* length is first received byte */
+	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit */
 };
 
 /**
-- 
2.17.1

[U-Boot] [PATCH] i2c: pcf2127: fix bug that read wrong time

[Lukasz Majewski]

Hi Chuanhua,

> Because i2c driver does not generate a stop bit when reading registers
> of pcf2127

The change (in the common i2c code) is rather large when considering the
description above.

Could you write a more detailed patch description? Is this only the
problem with i2c in DM?

Is the same code (as introduced in this commit) available in Linux
kernel?
How the error is reproduced? What are the symptoms of it?

In which way the pcf2127 needs special treatment (non I2C compatible)?
How does it differ from other RTC devices?

> 
> Signed-off-by: Biwen Li <biwen.li@nxp.com>
> Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> ---
>  drivers/i2c/i2c-uclass.c |  2 ++
>  drivers/i2c/mxc_i2c.c    | 70
> +++++++++++++++++++++++++++++++++++----- drivers/rtc/pcf2127.c    |
> 18 ++++++----- include/i2c.h            |  2 ++
>  4 files changed, 76 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> index e47abf1833..18f7364d72 100644
> --- a/drivers/i2c/i2c-uclass.c
> +++ b/drivers/i2c/i2c-uclass.c
> @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset,
> uint8_t *buffer, int len) if (len) {
>  		ptr->addr = chip->chip_addr;
>  		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> I2C_M_TEN : 0;
> +		ptr->flags |= chip->flags &
> DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> +			      I2C_M_RD_NEED_STOP_BIT : 0;
>  		ptr->flags |= I2C_M_RD;
>  		ptr->len = len;
>  		ptr->buf = buffer;
> diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> index 73b9807598..241367bac8 100644
> --- a/drivers/i2c/mxc_i2c.c
> +++ b/drivers/i2c/mxc_i2c.c
> @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) int reg_shift = i2c_bus->driver_data
> & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
>  	int read_mode;
> +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true :
> false;
> +	unsigned int temp;
>  
>  	/* Here address len is set to -1 to not send any address at
> first.
>  	 * Otherwise i2c_init_transfer will send the chip address
> with write @@ -976,6 +978,7 @@ static int mxc_i2c_xfer(struct udevice
> *bus, struct i2c_msg *msg, int nmsgs) read_mode = -1; /* So it's
> always different on the first message */ for (; nmsgs > 0; nmsgs--,
> msg++) { const int msg_is_read = !!(msg->flags & I2C_M_RD);
> +		bool next_is_read = nmsgs > 1 && (msg[1].flags &
> I2C_M_RD); 
>  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> @@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) if (msg_is_read != read_mode) {
>  			/* Send repeated start if not 1st message */
>  			if (read_mode != -1) {
> -				debug("i2c_xfer: [RSTART]\n");
> -				ret = readb(base + (I2CR <<
> reg_shift));
> -				ret |= I2CR_RSTA;
> -				writeb(ret, base + (I2CR <<
> reg_shift));
> +				if (!(msg[1].flags &
> I2C_M_RD_NEED_STOP_BIT)) {
> +					debug("i2c_xfer:
> [RSTART]\n");
> +					ret = readb(base + (I2CR <<
> reg_shift));
> +					ret |= I2CR_RSTA;
> +					writeb(ret, base + (I2CR <<
> reg_shift));
> +				}
>  			}
>  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> msg->addr, msg_is_read ? 'R' : 'W');
> +
>  			ret = tx_byte(i2c_bus, (msg->addr << 1) |
> msg_is_read); if (ret < 0) {
>  				debug("i2c_xfer: [STOP]\n");
> @@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice *bus,
> struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
>  		}
>  
> -		if (msg->flags & I2C_M_RD)
> +		if (msg->flags & I2C_M_RD) {
>  			ret = i2c_read_data(i2c_bus, msg->addr,
> msg->buf, msg->len, nmsgs == 1 ||
>  						      (msg->flags &
> I2C_M_STOP));
> -		else
> -			ret = i2c_write_data(i2c_bus, msg->addr,
> msg->buf,
> -					     msg->len);
> +			if (ret < 0)
> +				break;
> +			continue;
> +		}
>  
> +		 /* Write message */
> +		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> +				     msg->len);
>  		if (ret < 0)
>  			break;
> +
> +		if (!next_is_read)
> +			continue;
> +
> +		/* Read message following write message */
> +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> +			/* Generate a stop bit */
> +			i2c_imx_stop(i2c_bus);
> +			/* Reset i2c slave */
> +			i2c_force_reset_slave();
> +
> +			/* Enable I2C controller */
> +			if (quirk)
> +				ret = readb(base + (I2CR <<
> reg_shift))
> +					& I2CR_IDIS;
> +			else
> +				ret = !(readb(base + (I2CR <<
> reg_shift))
> +						& I2CR_IEN);
> +			if (ret) {
> +				writeb(I2CR_IEN, base + (I2CR <<
> reg_shift));
> +				/* Wait for controller to be stable
> */
> +				udelay(50);
> +			}
> +
> +			/* Clear interrupt bit */
> +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> reg_shift));
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_IDLE);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Start I2C transaction */
> +			temp = readb(base + (I2CR << reg_shift));
> +			temp |= I2CR_MSTA;
> +			writeb(temp, base + (I2CR << reg_shift));
> +
> +			ret = wait_for_sr_state(i2c_bus,
> ST_BUS_BUSY);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Enter transfer mode */
> +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> +			writeb(temp, base + (I2CR << reg_shift));
> +			udelay(50);
> +		}
>  	}
>  
>  	if (ret)
> diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
> index dcf0340b4d..010c45ecbf 100644
> --- a/drivers/rtc/pcf2127.c
> +++ b/drivers/rtc/pcf2127.c
> @@ -24,12 +24,9 @@
>  
>  static int pcf2127_rtc_set(struct udevice *dev, const struct
> rtc_time *tm) {
> -	uchar buf[8];
> +	uchar buf[7] = {0};
>  	int i = 0, ret;
>  
> -	/* start register address */
> -	buf[i++] = PCF2127_REG_SC;
> -
>  	/* hours, minutes and seconds */
>  	buf[i++] = bin2bcd(tm->tm_sec);
>  	buf[i++] = bin2bcd(tm->tm_min);
> @@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev,
> const struct rtc_time *tm) buf[i++] = bin2bcd(tm->tm_year % 100);
>  
>  	/* write register's data */
> -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
> +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
>  
>  	return ret;
>  }
> @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev,
> struct rtc_time *tm) int ret = 0;
>  	uchar buf[10] = { PCF2127_REG_CTRL1 };
>  
> -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> -	if (ret < 0)
> -		return ret;
>  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
>  	if (ret < 0)
>  		return ret;
> @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice *dev)
>  	return 0;
>  }
>  
> +static int pcf2127_probe(struct udevice *dev)
> +{
> +	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
> +
> +	return 0;
> +}
> +
>  static const struct rtc_ops pcf2127_rtc_ops = {
>  	.get = pcf2127_rtc_get,
>  	.set = pcf2127_rtc_set,
> @@ -104,6 +105,7 @@ static const struct udevice_id pcf2127_rtc_ids[]
> = { U_BOOT_DRIVER(rtc_pcf2127) = {
>  	.name	= "rtc-pcf2127",
>  	.id	= UCLASS_RTC,
> +	.probe	= pcf2127_probe,
>  	.of_match = pcf2127_rtc_ids,
>  	.ops	= &pcf2127_rtc_ops,
>  };
> diff --git a/include/i2c.h b/include/i2c.h
> index a5c760c711..beaa028349 100644
> --- a/include/i2c.h
> +++ b/include/i2c.h
> @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
>  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit addressing
> */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send address for each
> read byte */ DM_I2C_CHIP_WR_ADDRESS	= 1 << 2, /* Send address
> for each write byte */
> +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate
> stop bit */ };
>  
>  struct udevice;
> @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
>  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK */
>  	I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit
> on reads */ I2C_M_RECV_LEN		= 0x0400, /* length is
> first received byte */
> +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit
> */ };
>  
>  /**




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190522/4f80dec4/attachment.sig>

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年5月22日 15:16
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> sjg at chromium.org
> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> Hi Chuanhua,
> 
> > Because i2c driver does not generate a stop bit when reading registers
> > of pcf2127
> 
> The change (in the common i2c code) is rather large when considering the
> description above.
> 
> Could you write a more detailed patch description? Is this only the problem
> with i2c in DM?
OK, This is a problem with the i2c transport framework. After writing the register
address that you want to read, the stop signal is missing before reading the register data.
> 
> Is the same code (as introduced in this commit) available in Linux kernel?
The kernel does not have such a problem
> How the error is reproduced? What are the symptoms of it?
You can use the i2c command to read the register data of pcf2127. 
You will find that you want to read the address 0 of the register, 
but the data of the 1 address is read, and the data read later will be offset.
> 
> In which way the pcf2127 needs special treatment (non I2C compatible)?
> How does it differ from other RTC devices?
According to the pcf2127 chip manual, when you need to read the internal
register data, you need to write the register address first and then       
need stop bit(it is necessary).
> 
> >
> > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > ---
> >  drivers/i2c/i2c-uclass.c |  2 ++
> >  drivers/i2c/mxc_i2c.c    | 70
> > +++++++++++++++++++++++++++++++++++----- drivers/rtc/pcf2127.c    |
> > 18 ++++++----- include/i2c.h            |  2 ++
> >  4 files changed, 76 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c index
> > e47abf1833..18f7364d72 100644
> > --- a/drivers/i2c/i2c-uclass.c
> > +++ b/drivers/i2c/i2c-uclass.c
> > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint offset,
> > uint8_t *buffer, int len) if (len) {
> >  		ptr->addr = chip->chip_addr;
> >  		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> > I2C_M_TEN : 0;
> > +		ptr->flags |= chip->flags &
> > DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > +			      I2C_M_RD_NEED_STOP_BIT : 0;
> >  		ptr->flags |= I2C_M_RD;
> >  		ptr->len = len;
> >  		ptr->buf = buffer;
> > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
> > 73b9807598..241367bac8 100644
> > --- a/drivers/i2c/mxc_i2c.c
> > +++ b/drivers/i2c/mxc_i2c.c
> > @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > struct i2c_msg *msg, int nmsgs) int reg_shift = i2c_bus->driver_data &
> > I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT : IMX_I2C_REGSHIFT;
> >  	int read_mode;
> > +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ? true :
> > false;
> > +	unsigned int temp;
> >
> >  	/* Here address len is set to -1 to not send any address at first.
> >  	 * Otherwise i2c_init_transfer will send the chip address with write
> > @@ -976,6 +978,7 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > struct i2c_msg *msg, int nmsgs) read_mode = -1; /* So it's always
> > different on the first message */ for (; nmsgs > 0; nmsgs--,
> > msg++) { const int msg_is_read = !!(msg->flags & I2C_M_RD);
> > +		bool next_is_read = nmsgs > 1 && (msg[1].flags &
> > I2C_M_RD);
> >  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> > msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> > @@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > struct i2c_msg *msg, int nmsgs) if (msg_is_read != read_mode) {
> >  			/* Send repeated start if not 1st message */
> >  			if (read_mode != -1) {
> > -				debug("i2c_xfer: [RSTART]\n");
> > -				ret = readb(base + (I2CR <<
> > reg_shift));
> > -				ret |= I2CR_RSTA;
> > -				writeb(ret, base + (I2CR <<
> > reg_shift));
> > +				if (!(msg[1].flags &
> > I2C_M_RD_NEED_STOP_BIT)) {
> > +					debug("i2c_xfer:
> > [RSTART]\n");
> > +					ret = readb(base + (I2CR <<
> > reg_shift));
> > +					ret |= I2CR_RSTA;
> > +					writeb(ret, base + (I2CR <<
> > reg_shift));
> > +				}
> >  			}
> >  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> > msg->addr, msg_is_read ? 'R' : 'W');
> > +
> >  			ret = tx_byte(i2c_bus, (msg->addr << 1) | msg_is_read); if (ret <
> > 0) {
> >  				debug("i2c_xfer: [STOP]\n");
> > @@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
> >  		}
> >
> > -		if (msg->flags & I2C_M_RD)
> > +		if (msg->flags & I2C_M_RD) {
> >  			ret = i2c_read_data(i2c_bus, msg->addr,
> > msg->buf, msg->len, nmsgs == 1 ||
> >  						      (msg->flags &
> > I2C_M_STOP));
> > -		else
> > -			ret = i2c_write_data(i2c_bus, msg->addr,
> > msg->buf,
> > -					     msg->len);
> > +			if (ret < 0)
> > +				break;
> > +			continue;
> > +		}
> >
> > +		 /* Write message */
> > +		ret = i2c_write_data(i2c_bus, msg->addr, msg->buf,
> > +				     msg->len);
> >  		if (ret < 0)
> >  			break;
> > +
> > +		if (!next_is_read)
> > +			continue;
> > +
> > +		/* Read message following write message */
> > +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> > +			/* Generate a stop bit */
> > +			i2c_imx_stop(i2c_bus);
> > +			/* Reset i2c slave */
> > +			i2c_force_reset_slave();
> > +
> > +			/* Enable I2C controller */
> > +			if (quirk)
> > +				ret = readb(base + (I2CR <<
> > reg_shift))
> > +					& I2CR_IDIS;
> > +			else
> > +				ret = !(readb(base + (I2CR <<
> > reg_shift))
> > +						& I2CR_IEN);
> > +			if (ret) {
> > +				writeb(I2CR_IEN, base + (I2CR <<
> > reg_shift));
> > +				/* Wait for controller to be stable
> > */
> > +				udelay(50);
> > +			}
> > +
> > +			/* Clear interrupt bit */
> > +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> > reg_shift));
> > +			ret = wait_for_sr_state(i2c_bus,
> > ST_BUS_IDLE);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			/* Start I2C transaction */
> > +			temp = readb(base + (I2CR << reg_shift));
> > +			temp |= I2CR_MSTA;
> > +			writeb(temp, base + (I2CR << reg_shift));
> > +
> > +			ret = wait_for_sr_state(i2c_bus,
> > ST_BUS_BUSY);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			/* Enter transfer mode */
> > +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> > +			writeb(temp, base + (I2CR << reg_shift));
> > +			udelay(50);
> > +		}
> >  	}
> >
> >  	if (ret)
> > diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c index
> > dcf0340b4d..010c45ecbf 100644
> > --- a/drivers/rtc/pcf2127.c
> > +++ b/drivers/rtc/pcf2127.c
> > @@ -24,12 +24,9 @@
> >
> >  static int pcf2127_rtc_set(struct udevice *dev, const struct rtc_time
> > *tm) {
> > -	uchar buf[8];
> > +	uchar buf[7] = {0};
> >  	int i = 0, ret;
> >
> > -	/* start register address */
> > -	buf[i++] = PCF2127_REG_SC;
> > -
> >  	/* hours, minutes and seconds */
> >  	buf[i++] = bin2bcd(tm->tm_sec);
> >  	buf[i++] = bin2bcd(tm->tm_min);
> > @@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev,
> > const struct rtc_time *tm) buf[i++] = bin2bcd(tm->tm_year % 100);
> >
> >  	/* write register's data */
> > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
> > +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
> >
> >  	return ret;
> >  }
> > @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev,
> > struct rtc_time *tm) int ret = 0;
> >  	uchar buf[10] = { PCF2127_REG_CTRL1 };
> >
> > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> > -	if (ret < 0)
> > -		return ret;
> >  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
> >  	if (ret < 0)
> >  		return ret;
> > @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice *dev)
> >  	return 0;
> >  }
> >
> > +static int pcf2127_probe(struct udevice *dev) {
> > +	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct rtc_ops pcf2127_rtc_ops = {
> >  	.get = pcf2127_rtc_get,
> >  	.set = pcf2127_rtc_set,
> > @@ -104,6 +105,7 @@ static const struct udevice_id pcf2127_rtc_ids[] =
> > { U_BOOT_DRIVER(rtc_pcf2127) = {
> >  	.name	= "rtc-pcf2127",
> >  	.id	= UCLASS_RTC,
> > +	.probe	= pcf2127_probe,
> >  	.of_match = pcf2127_rtc_ids,
> >  	.ops	= &pcf2127_rtc_ops,
> >  };
> > diff --git a/include/i2c.h b/include/i2c.h index
> > a5c760c711..beaa028349 100644
> > --- a/include/i2c.h
> > +++ b/include/i2c.h
> > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> >  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit addressing
> > */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send address for each
> > read byte */ DM_I2C_CHIP_WR_ADDRESS	= 1 << 2, /* Send address
> > for each write byte */
> > +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need generate
> > stop bit */ };
> >
> >  struct udevice;
> > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> >  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK */
> >  	I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit
> > on reads */ I2C_M_RECV_LEN		= 0x0400, /* length is
> > first received byte */
> > +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop bit
> > */ };
> >
> >  /**
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Lukasz Majewski]

Hi Chuanhua,

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年5月22日 15:16
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> > sjg at chromium.org
> > Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> > time
> > 
> > Hi Chuanhua,
> >   
> > > Because i2c driver does not generate a stop bit when reading
> > > registers of pcf2127  
> > 
> > The change (in the common i2c code) is rather large when
> > considering the description above.
> > 
> > Could you write a more detailed patch description? Is this only the
> > problem with i2c in DM?  
> OK, This is a problem with the i2c transport framework. After writing
> the register address that you want to read, the stop signal is
> missing before reading the register data.
> > 
> > Is the same code (as introduced in this commit) available in Linux
> > kernel?  
> The kernel does not have such a problem

Do you know maybe why such issue is not present on Linux kernel?

> > How the error is reproduced? What are the symptoms of it?  
> You can use the i2c command to read the register data of pcf2127. 

So the problem is with using "i2c ..." commands from U-Boot prompt?

> You will find that you want to read the address 0 of the register, 
> but the data of the 1 address is read, and the data read later will
> be offset.

As fair as I can tell the pcf2127 has its own DM aware driver at
driver/rtc/pcf2127.c.

Is this driver broken so you need to modify the generic i.MX I2C code?
Have you tried to test this driver on your setup?

> > 
> > In which way the pcf2127 needs special treatment (non I2C
> > compatible)? How does it differ from other RTC devices?  
> According to the pcf2127 chip manual, when you need to read the
> internal register data, you need to write the register address first
> and then need stop bit(it is necessary).
> >   
> > >
> > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > ---
> > >  drivers/i2c/i2c-uclass.c |  2 ++
> > >  drivers/i2c/mxc_i2c.c    | 70
> > > +++++++++++++++++++++++++++++++++++----- drivers/rtc/pcf2127.c
> > > | 18 ++++++----- include/i2c.h            |  2 ++
> > >  4 files changed, 76 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > > index e47abf1833..18f7364d72 100644
> > > --- a/drivers/i2c/i2c-uclass.c
> > > +++ b/drivers/i2c/i2c-uclass.c
> > > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint
> > > offset, uint8_t *buffer, int len) if (len) {
> > >  		ptr->addr = chip->chip_addr;
> > >  		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> > > I2C_M_TEN : 0;
> > > +		ptr->flags |= chip->flags &
> > > DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > > +			      I2C_M_RD_NEED_STOP_BIT : 0;
> > >  		ptr->flags |= I2C_M_RD;
> > >  		ptr->len = len;
> > >  		ptr->buf = buffer;
> > > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
> > > 73b9807598..241367bac8 100644
> > > --- a/drivers/i2c/mxc_i2c.c
> > > +++ b/drivers/i2c/mxc_i2c.c
> > > @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > > struct i2c_msg *msg, int nmsgs) int reg_shift =
> > > i2c_bus->driver_data & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT :
> > > IMX_I2C_REGSHIFT; int read_mode;
> > > +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
> > > true : false;
> > > +	unsigned int temp;
> > >
> > >  	/* Here address len is set to -1 to not send any address
> > > at first.
> > >  	 * Otherwise i2c_init_transfer will send the chip
> > > address with write @@ -976,6 +978,7 @@ static int
> > > mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
> > > read_mode = -1; /* So it's always different on the first message
> > > */ for (; nmsgs > 0; nmsgs--, msg++) { const int msg_is_read
> > > = !!(msg->flags & I2C_M_RD);
> > > +		bool next_is_read = nmsgs > 1 && (msg[1].flags &
> > > I2C_M_RD);
> > >  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> > > msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> > > @@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > > struct i2c_msg *msg, int nmsgs) if (msg_is_read != read_mode) {
> > >  			/* Send repeated start if not 1st
> > > message */ if (read_mode != -1) {
> > > -				debug("i2c_xfer: [RSTART]\n");
> > > -				ret = readb(base + (I2CR <<
> > > reg_shift));
> > > -				ret |= I2CR_RSTA;
> > > -				writeb(ret, base + (I2CR <<
> > > reg_shift));
> > > +				if (!(msg[1].flags &
> > > I2C_M_RD_NEED_STOP_BIT)) {
> > > +					debug("i2c_xfer:
> > > [RSTART]\n");
> > > +					ret = readb(base + (I2CR
> > > << reg_shift));
> > > +					ret |= I2CR_RSTA;
> > > +					writeb(ret, base + (I2CR
> > > << reg_shift));
> > > +				}
> > >  			}
> > >  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> > > msg->addr, msg_is_read ? 'R' : 'W');
> > > +
> > >  			ret = tx_byte(i2c_bus, (msg->addr << 1)
> > > | msg_is_read); if (ret < 0) {
> > >  				debug("i2c_xfer: [STOP]\n");
> > > @@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice
> > > *bus, struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
> > >  		}
> > >
> > > -		if (msg->flags & I2C_M_RD)
> > > +		if (msg->flags & I2C_M_RD) {
> > >  			ret = i2c_read_data(i2c_bus, msg->addr,
> > > msg->buf, msg->len, nmsgs == 1 ||
> > >  						      (msg->flags
> > > & I2C_M_STOP));
> > > -		else
> > > -			ret = i2c_write_data(i2c_bus, msg->addr,
> > > msg->buf,
> > > -					     msg->len);
> > > +			if (ret < 0)
> > > +				break;
> > > +			continue;
> > > +		}
> > >
> > > +		 /* Write message */
> > > +		ret = i2c_write_data(i2c_bus, msg->addr,
> > > msg->buf,
> > > +				     msg->len);
> > >  		if (ret < 0)
> > >  			break;
> > > +
> > > +		if (!next_is_read)
> > > +			continue;
> > > +
> > > +		/* Read message following write message */
> > > +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> > > +			/* Generate a stop bit */
> > > +			i2c_imx_stop(i2c_bus);
> > > +			/* Reset i2c slave */
> > > +			i2c_force_reset_slave();
> > > +
> > > +			/* Enable I2C controller */
> > > +			if (quirk)
> > > +				ret = readb(base + (I2CR <<
> > > reg_shift))
> > > +					& I2CR_IDIS;
> > > +			else
> > > +				ret = !(readb(base + (I2CR <<
> > > reg_shift))
> > > +						& I2CR_IEN);
> > > +			if (ret) {
> > > +				writeb(I2CR_IEN, base + (I2CR <<
> > > reg_shift));
> > > +				/* Wait for controller to be
> > > stable */
> > > +				udelay(50);
> > > +			}
> > > +
> > > +			/* Clear interrupt bit */
> > > +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> > > reg_shift));
> > > +			ret = wait_for_sr_state(i2c_bus,
> > > ST_BUS_IDLE);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			/* Start I2C transaction */
> > > +			temp = readb(base + (I2CR << reg_shift));
> > > +			temp |= I2CR_MSTA;
> > > +			writeb(temp, base + (I2CR << reg_shift));
> > > +
> > > +			ret = wait_for_sr_state(i2c_bus,
> > > ST_BUS_BUSY);
> > > +			if (ret < 0)
> > > +				return ret;
> > > +
> > > +			/* Enter transfer mode */
> > > +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> > > +			writeb(temp, base + (I2CR << reg_shift));
> > > +			udelay(50);
> > > +		}
> > >  	}
> > >
> > >  	if (ret)
> > > diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c index
> > > dcf0340b4d..010c45ecbf 100644
> > > --- a/drivers/rtc/pcf2127.c
> > > +++ b/drivers/rtc/pcf2127.c
> > > @@ -24,12 +24,9 @@
> > >
> > >  static int pcf2127_rtc_set(struct udevice *dev, const struct
> > > rtc_time *tm) {
> > > -	uchar buf[8];
> > > +	uchar buf[7] = {0};
> > >  	int i = 0, ret;
> > >
> > > -	/* start register address */
> > > -	buf[i++] = PCF2127_REG_SC;
> > > -
> > >  	/* hours, minutes and seconds */
> > >  	buf[i++] = bin2bcd(tm->tm_sec);
> > >  	buf[i++] = bin2bcd(tm->tm_min);
> > > @@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev,
> > > const struct rtc_time *tm) buf[i++] = bin2bcd(tm->tm_year % 100);
> > >
> > >  	/* write register's data */
> > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf,
> > > sizeof(buf));
> > > +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
> > >
> > >  	return ret;
> > >  }
> > > @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev,
> > > struct rtc_time *tm) int ret = 0;
> > >  	uchar buf[10] = { PCF2127_REG_CTRL1 };
> > >
> > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> > > -	if (ret < 0)
> > > -		return ret;
> > >  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> > > sizeof(buf)); if (ret < 0)
> > >  		return ret;
> > > @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice
> > > *dev) return 0;
> > >  }
> > >
> > > +static int pcf2127_probe(struct udevice *dev) {
> > > +	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static const struct rtc_ops pcf2127_rtc_ops = {
> > >  	.get = pcf2127_rtc_get,
> > >  	.set = pcf2127_rtc_set,
> > > @@ -104,6 +105,7 @@ static const struct udevice_id
> > > pcf2127_rtc_ids[] = { U_BOOT_DRIVER(rtc_pcf2127) = {
> > >  	.name	= "rtc-pcf2127",
> > >  	.id	= UCLASS_RTC,
> > > +	.probe	= pcf2127_probe,
> > >  	.of_match = pcf2127_rtc_ids,
> > >  	.ops	= &pcf2127_rtc_ops,
> > >  };
> > > diff --git a/include/i2c.h b/include/i2c.h index
> > > a5c760c711..beaa028349 100644
> > > --- a/include/i2c.h
> > > +++ b/include/i2c.h
> > > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> > >  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit
> > > addressing */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send
> > > address for each read byte */ DM_I2C_CHIP_WR_ADDRESS	= 1
> > > << 2, /* Send address for each write byte */
> > > +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need
> > > generate stop bit */ };
> > >
> > >  struct udevice;
> > > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> > >  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK
> > > */ I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit
> > > on reads */ I2C_M_RECV_LEN		= 0x0400, /* length is
> > > first received byte */
> > > +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop
> > > bit */ };
> > >
> > >  /**  
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190522/9c0c877a/attachment.sig>

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年5月22日 16:41
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> Hi Chuanhua,
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年5月22日 15:16
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> > > sjg at chromium.org
> > > Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> > > time
> > >
> > > Hi Chuanhua,
> > >
> > > > Because i2c driver does not generate a stop bit when reading
> > > > registers of pcf2127
> > >
> > > The change (in the common i2c code) is rather large when considering
> > > the description above.
> > >
> > > Could you write a more detailed patch description? Is this only the
> > > problem with i2c in DM?
> > OK, This is a problem with the i2c transport framework. After writing
> > the register address that you want to read, the stop signal is missing
> > before reading the register data.
> > >
> > > Is the same code (as introduced in this commit) available in Linux
> > > kernel?
> > The kernel does not have such a problem
> 
> Do you know maybe why such issue is not present on Linux kernel?
The kernel code is encapsulated when reading the pcf2127 register, which separates the read and write, 
thus generating a stop signal before reading the register.
Eg: Here is the wrapper made by the kernel code:
static int pcf2127_i2c_read(void *context, const void *reg, size_t reg_size,
                                void *val, size_t val_size)
{
        struct device *dev = context;
        struct i2c_client *client = to_i2c_client(dev);
        int ret;

        if (WARN_ON(reg_size != 1)) 
                return -EINVAL;

        ret = i2c_master_send(client, reg, 1); 
        if (ret != 1)
                return ret < 0 ? ret : -EIO;

        ret = i2c_master_recv(client, val, val_size);
        if (ret != val_size)
                return ret < 0 ? ret : -EIO;

        return 0;
}
> 
> > > How the error is reproduced? What are the symptoms of it?
> > You can use the i2c command to read the register data of pcf2127.
> 
> So the problem is with using "i2c ..." commands from U-Boot prompt?
Yes,but adding debugging to the rtc driver is also the same problem
> 
> > You will find that you want to read the address 0 of the register,
> > but the data of the 1 address is read, and the data read later will
> > be offset.
> 
> As fair as I can tell the pcf2127 has its own DM aware driver at
> driver/rtc/pcf2127.c.
> 
> Is this driver broken so you need to modify the generic i.MX I2C code?
> Have you tried to test this driver on your setup?
Pcf2127 driver also has problems, has been modified, need i2c general code to support
> 
> > >
> > > In which way the pcf2127 needs special treatment (non I2C
> > > compatible)? How does it differ from other RTC devices?
> > According to the pcf2127 chip manual, when you need to read the
> > internal register data, you need to write the register address first
> > and then need stop bit(it is necessary).
> > >
> > > >
> > > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > ---
> > > >  drivers/i2c/i2c-uclass.c |  2 ++
> > > >  drivers/i2c/mxc_i2c.c    | 70
> > > > +++++++++++++++++++++++++++++++++++----- drivers/rtc/pcf2127.c
> > > > | 18 ++++++----- include/i2c.h            |  2 ++
> > > >  4 files changed, 76 insertions(+), 16 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/i2c-uclass.c b/drivers/i2c/i2c-uclass.c
> > > > index e47abf1833..18f7364d72 100644
> > > > --- a/drivers/i2c/i2c-uclass.c
> > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > offset, uint8_t *buffer, int len) if (len) {
> > > >  		ptr->addr = chip->chip_addr;
> > > >  		ptr->flags = chip->flags & DM_I2C_CHIP_10BIT ?
> > > > I2C_M_TEN : 0;
> > > > +		ptr->flags |= chip->flags &
> > > > DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > > > +			      I2C_M_RD_NEED_STOP_BIT : 0;
> > > >  		ptr->flags |= I2C_M_RD;
> > > >  		ptr->len = len;
> > > >  		ptr->buf = buffer;
> > > > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c index
> > > > 73b9807598..241367bac8 100644
> > > > --- a/drivers/i2c/mxc_i2c.c
> > > > +++ b/drivers/i2c/mxc_i2c.c
> > > > @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > > > struct i2c_msg *msg, int nmsgs) int reg_shift =
> > > > i2c_bus->driver_data & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT :
> > > > IMX_I2C_REGSHIFT; int read_mode;
> > > > +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
> > > > true : false;
> > > > +	unsigned int temp;
> > > >
> > > >  	/* Here address len is set to -1 to not send any address
> > > > at first.
> > > >  	 * Otherwise i2c_init_transfer will send the chip
> > > > address with write @@ -976,6 +978,7 @@ static int
> > > > mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int nmsgs)
> > > > read_mode = -1; /* So it's always different on the first message
> > > > */ for (; nmsgs > 0; nmsgs--, msg++) { const int msg_is_read
> > > > = !!(msg->flags & I2C_M_RD);
> > > > +		bool next_is_read = nmsgs > 1 && (msg[1].flags &
> > > > I2C_M_RD);
> > > >  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> > > > msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> > > > @@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice *bus,
> > > > struct i2c_msg *msg, int nmsgs) if (msg_is_read != read_mode) {
> > > >  			/* Send repeated start if not 1st
> > > > message */ if (read_mode != -1) {
> > > > -				debug("i2c_xfer: [RSTART]\n");
> > > > -				ret = readb(base + (I2CR <<
> > > > reg_shift));
> > > > -				ret |= I2CR_RSTA;
> > > > -				writeb(ret, base + (I2CR <<
> > > > reg_shift));
> > > > +				if (!(msg[1].flags &
> > > > I2C_M_RD_NEED_STOP_BIT)) {
> > > > +					debug("i2c_xfer:
> > > > [RSTART]\n");
> > > > +					ret = readb(base + (I2CR
> > > > << reg_shift));
> > > > +					ret |= I2CR_RSTA;
> > > > +					writeb(ret, base + (I2CR
> > > > << reg_shift));
> > > > +				}
> > > >  			}
> > > >  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> > > > msg->addr, msg_is_read ? 'R' : 'W');
> > > > +
> > > >  			ret = tx_byte(i2c_bus, (msg->addr << 1)
> > > > | msg_is_read); if (ret < 0) {
> > > >  				debug("i2c_xfer: [STOP]\n");
> > > > @@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice
> > > > *bus, struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
> > > >  		}
> > > >
> > > > -		if (msg->flags & I2C_M_RD)
> > > > +		if (msg->flags & I2C_M_RD) {
> > > >  			ret = i2c_read_data(i2c_bus, msg->addr,
> > > > msg->buf, msg->len, nmsgs == 1 ||
> > > >  						      (msg->flags
> > > > & I2C_M_STOP));
> > > > -		else
> > > > -			ret = i2c_write_data(i2c_bus, msg->addr,
> > > > msg->buf,
> > > > -					     msg->len);
> > > > +			if (ret < 0)
> > > > +				break;
> > > > +			continue;
> > > > +		}
> > > >
> > > > +		 /* Write message */
> > > > +		ret = i2c_write_data(i2c_bus, msg->addr,
> > > > msg->buf,
> > > > +				     msg->len);
> > > >  		if (ret < 0)
> > > >  			break;
> > > > +
> > > > +		if (!next_is_read)
> > > > +			continue;
> > > > +
> > > > +		/* Read message following write message */
> > > > +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> > > > +			/* Generate a stop bit */
> > > > +			i2c_imx_stop(i2c_bus);
> > > > +			/* Reset i2c slave */
> > > > +			i2c_force_reset_slave();
> > > > +
> > > > +			/* Enable I2C controller */
> > > > +			if (quirk)
> > > > +				ret = readb(base + (I2CR <<
> > > > reg_shift))
> > > > +					& I2CR_IDIS;
> > > > +			else
> > > > +				ret = !(readb(base + (I2CR <<
> > > > reg_shift))
> > > > +						& I2CR_IEN);
> > > > +			if (ret) {
> > > > +				writeb(I2CR_IEN, base + (I2CR <<
> > > > reg_shift));
> > > > +				/* Wait for controller to be
> > > > stable */
> > > > +				udelay(50);
> > > > +			}
> > > > +
> > > > +			/* Clear interrupt bit */
> > > > +			writeb(I2SR_IIF_CLEAR, base + (I2SR <<
> > > > reg_shift));
> > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > ST_BUS_IDLE);
> > > > +			if (ret < 0)
> > > > +				return ret;
> > > > +
> > > > +			/* Start I2C transaction */
> > > > +			temp = readb(base + (I2CR << reg_shift));
> > > > +			temp |= I2CR_MSTA;
> > > > +			writeb(temp, base + (I2CR << reg_shift));
> > > > +
> > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > ST_BUS_BUSY);
> > > > +			if (ret < 0)
> > > > +				return ret;
> > > > +
> > > > +			/* Enter transfer mode */
> > > > +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> > > > +			writeb(temp, base + (I2CR << reg_shift));
> > > > +			udelay(50);
> > > > +		}
> > > >  	}
> > > >
> > > >  	if (ret)
> > > > diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c index
> > > > dcf0340b4d..010c45ecbf 100644
> > > > --- a/drivers/rtc/pcf2127.c
> > > > +++ b/drivers/rtc/pcf2127.c
> > > > @@ -24,12 +24,9 @@
> > > >
> > > >  static int pcf2127_rtc_set(struct udevice *dev, const struct
> > > > rtc_time *tm) {
> > > > -	uchar buf[8];
> > > > +	uchar buf[7] = {0};
> > > >  	int i = 0, ret;
> > > >
> > > > -	/* start register address */
> > > > -	buf[i++] = PCF2127_REG_SC;
> > > > -
> > > >  	/* hours, minutes and seconds */
> > > >  	buf[i++] = bin2bcd(tm->tm_sec);
> > > >  	buf[i++] = bin2bcd(tm->tm_min);
> > > > @@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice *dev,
> > > > const struct rtc_time *tm) buf[i++] = bin2bcd(tm->tm_year % 100);
> > > >
> > > >  	/* write register's data */
> > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf,
> > > > sizeof(buf));
> > > > +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
> > > >
> > > >  	return ret;
> > > >  }
> > > > @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice *dev,
> > > > struct rtc_time *tm) int ret = 0;
> > > >  	uchar buf[10] = { PCF2127_REG_CTRL1 };
> > > >
> > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> > > > -	if (ret < 0)
> > > > -		return ret;
> > > >  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> > > > sizeof(buf)); if (ret < 0)
> > > >  		return ret;
> > > > @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice
> > > > *dev) return 0;
> > > >  }
> > > >
> > > > +static int pcf2127_probe(struct udevice *dev) {
> > > > +	i2c_set_chip_flags(dev, DM_I2C_CHIP_RD_NEED_STOP_BIT);
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  static const struct rtc_ops pcf2127_rtc_ops = {
> > > >  	.get = pcf2127_rtc_get,
> > > >  	.set = pcf2127_rtc_set,
> > > > @@ -104,6 +105,7 @@ static const struct udevice_id
> > > > pcf2127_rtc_ids[] = { U_BOOT_DRIVER(rtc_pcf2127) = {
> > > >  	.name	= "rtc-pcf2127",
> > > >  	.id	= UCLASS_RTC,
> > > > +	.probe	= pcf2127_probe,
> > > >  	.of_match = pcf2127_rtc_ids,
> > > >  	.ops	= &pcf2127_rtc_ops,
> > > >  };
> > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > a5c760c711..beaa028349 100644
> > > > --- a/include/i2c.h
> > > > +++ b/include/i2c.h
> > > > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> > > >  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit
> > > > addressing */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send
> > > > address for each read byte */ DM_I2C_CHIP_WR_ADDRESS	= 1
> > > > << 2, /* Send address for each write byte */
> > > > +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need
> > > > generate stop bit */ };
> > > >
> > > >  struct udevice;
> > > > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> > > >  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after NAK
> > > > */ I2C_M_NO_RD_ACK		= 0x0800, /* skip the Ack bit
> > > > on reads */ I2C_M_RECV_LEN		= 0x0400, /* length is
> > > > first received byte */
> > > > +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate stop
> > > > bit */ };
> > > >
> > > >  /**
> > >
> > >
> > >
> > >
> > > Best regards,
> > >
> > > Lukasz Majewski
> > >
> > > --
> > >
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > > lukma at denx.de
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Lukasz Majewski]

On Wed, 22 May 2019 09:31:35 +0000
Chuanhua Han <chuanhua.han@nxp.com> wrote:

> > -----Original Message-----
> > From: Lukasz Majewski <lukma@denx.de>
> > Sent: 2019年5月22日 16:41
> > To: Chuanhua Han <chuanhua.han@nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> > sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> > Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> > wrong time
> > 
> > Hi Chuanhua,
> >   
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma@denx.de>
> > > > Sent: 2019年5月22日 15:16
> > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li@nxp.com>; sjg at chromium.org
> > > > Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> > > > time
> > > >
> > > > Hi Chuanhua,
> > > >  
> > > > > Because i2c driver does not generate a stop bit when reading
> > > > > registers of pcf2127  
> > > >
> > > > The change (in the common i2c code) is rather large when
> > > > considering the description above.
> > > >
> > > > Could you write a more detailed patch description? Is this only
> > > > the problem with i2c in DM?  
> > > OK, This is a problem with the i2c transport framework. After
> > > writing the register address that you want to read, the stop
> > > signal is missing before reading the register data.  
> > > >
> > > > Is the same code (as introduced in this commit) available in
> > > > Linux kernel?  
> > > The kernel does not have such a problem  
> > 
> > Do you know maybe why such issue is not present on Linux kernel?  
> The kernel code is encapsulated when reading the pcf2127 register,
> which separates the read and write, thus generating a stop signal
> before reading the register. Eg: Here is the wrapper made by the
> kernel code: static int pcf2127_i2c_read(void *context, const void
> *reg, size_t reg_size, void *val, size_t val_size)
> {
>         struct device *dev = context;
>         struct i2c_client *client = to_i2c_client(dev);
>         int ret;
> 
>         if (WARN_ON(reg_size != 1)) 
>                 return -EINVAL;
> 
>         ret = i2c_master_send(client, reg, 1); 
>         if (ret != 1)
>                 return ret < 0 ? ret : -EIO;
> 
>         ret = i2c_master_recv(client, val, val_size);
>         if (ret != val_size)
>                 return ret < 0 ? ret : -EIO;
> 
>         return 0;
> }

That was my point - the same shall be done in the pcf2127 driver. This
is a slow device, so we can afford for this.

> >   
> > > > How the error is reproduced? What are the symptoms of it?  
> > > You can use the i2c command to read the register data of
> > > pcf2127.  
> > 
> > So the problem is with using "i2c ..." commands from U-Boot
> > prompt?  
> Yes,but adding debugging to the rtc driver is also the same problem
> >   
> > > You will find that you want to read the address 0 of the register,
> > > but the data of the 1 address is read, and the data read later
> > > will be offset.  
> > 
> > As fair as I can tell the pcf2127 has its own DM aware driver at
> > driver/rtc/pcf2127.c.
> > 
> > Is this driver broken so you need to modify the generic i.MX I2C
> > code? Have you tried to test this driver on your setup?  
> Pcf2127 driver also has problems, has been modified, need i2c general
> code to support

Just one remark the mxc_i2c.c is IMX specific I2C code. Not the generic
one.

Moreover, it looks like the same approach (first send, then read) is
performed in the pcf2127 driver at pcf2127_rtc_get() function.

I think that the driver shall be first thoroughly checked, then fixes
shall be added to it.

> >   
> > > >
> > > > In which way the pcf2127 needs special treatment (non I2C
> > > > compatible)? How does it differ from other RTC devices?  
> > > According to the pcf2127 chip manual, when you need to read the
> > > internal register data, you need to write the register address
> > > first and then need stop bit(it is necessary).  
> > > >  
> > > > >
> > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > ---
> > > > >  drivers/i2c/i2c-uclass.c |  2 ++
> > > > >  drivers/i2c/mxc_i2c.c    | 70
> > > > > +++++++++++++++++++++++++++++++++++----- drivers/rtc/pcf2127.c
> > > > > | 18 ++++++----- include/i2c.h            |  2 ++
> > > > >  4 files changed, 76 insertions(+), 16 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..18f7364d72 100644
> > > > > --- a/drivers/i2c/i2c-uclass.c
> > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > > offset, uint8_t *buffer, int len) if (len) {
> > > > >  		ptr->addr = chip->chip_addr;
> > > > >  		ptr->flags = chip->flags &
> > > > > DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
> > > > > +		ptr->flags |= chip->flags &
> > > > > DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > > > > +			      I2C_M_RD_NEED_STOP_BIT : 0;
> > > > >  		ptr->flags |= I2C_M_RD;
> > > > >  		ptr->len = len;
> > > > >  		ptr->buf = buffer;
> > > > > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> > > > > index 73b9807598..241367bac8 100644
> > > > > --- a/drivers/i2c/mxc_i2c.c
> > > > > +++ b/drivers/i2c/mxc_i2c.c
> > > > > @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice
> > > > > *bus, struct i2c_msg *msg, int nmsgs) int reg_shift =
> > > > > i2c_bus->driver_data & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT :
> > > > > IMX_I2C_REGSHIFT; int read_mode;
> > > > > +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
> > > > > true : false;
> > > > > +	unsigned int temp;
> > > > >
> > > > >  	/* Here address len is set to -1 to not send any
> > > > > address at first.
> > > > >  	 * Otherwise i2c_init_transfer will send the chip
> > > > > address with write @@ -976,6 +978,7 @@ static int
> > > > > mxc_i2c_xfer(struct udevice *bus, struct i2c_msg *msg, int
> > > > > nmsgs) read_mode = -1; /* So it's always different on the
> > > > > first message */ for (; nmsgs > 0; nmsgs--, msg++) { const
> > > > > int msg_is_read = !!(msg->flags & I2C_M_RD);
> > > > > +		bool next_is_read = nmsgs > 1 &&
> > > > > (msg[1].flags & I2C_M_RD);
> > > > >  		debug("i2c_xfer: chip=0x%x, len=0x%x,
> > > > > dir=%c\n", msg->addr, msg->len, msg_is_read ? 'R' : 'W');
> > > > > @@ -983,13 +986,16 @@ static int mxc_i2c_xfer(struct udevice
> > > > > *bus, struct i2c_msg *msg, int nmsgs) if (msg_is_read !=
> > > > > read_mode) { /* Send repeated start if not 1st
> > > > > message */ if (read_mode != -1) {
> > > > > -				debug("i2c_xfer:
> > > > > [RSTART]\n");
> > > > > -				ret = readb(base + (I2CR <<
> > > > > reg_shift));
> > > > > -				ret |= I2CR_RSTA;
> > > > > -				writeb(ret, base + (I2CR <<
> > > > > reg_shift));
> > > > > +				if (!(msg[1].flags &
> > > > > I2C_M_RD_NEED_STOP_BIT)) {
> > > > > +					debug("i2c_xfer:
> > > > > [RSTART]\n");
> > > > > +					ret = readb(base +
> > > > > (I2CR << reg_shift));
> > > > > +					ret |= I2CR_RSTA;
> > > > > +					writeb(ret, base +
> > > > > (I2CR << reg_shift));
> > > > > +				}
> > > > >  			}
> > > > >  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> > > > > msg->addr, msg_is_read ? 'R' : 'W');
> > > > > +
> > > > >  			ret = tx_byte(i2c_bus, (msg->addr <<
> > > > > 1) | msg_is_read); if (ret < 0) {
> > > > >  				debug("i2c_xfer: [STOP]\n");
> > > > > @@ -999,16 +1005,64 @@ static int mxc_i2c_xfer(struct udevice
> > > > > *bus, struct i2c_msg *msg, int nmsgs) read_mode = msg_is_read;
> > > > >  		}
> > > > >
> > > > > -		if (msg->flags & I2C_M_RD)
> > > > > +		if (msg->flags & I2C_M_RD) {
> > > > >  			ret = i2c_read_data(i2c_bus,
> > > > > msg->addr, msg->buf, msg->len, nmsgs == 1 ||
> > > > >  						      (msg->flags
> > > > > & I2C_M_STOP));
> > > > > -		else
> > > > > -			ret = i2c_write_data(i2c_bus,
> > > > > msg->addr, msg->buf,
> > > > > -					     msg->len);
> > > > > +			if (ret < 0)
> > > > > +				break;
> > > > > +			continue;
> > > > > +		}
> > > > >
> > > > > +		 /* Write message */
> > > > > +		ret = i2c_write_data(i2c_bus, msg->addr,
> > > > > msg->buf,
> > > > > +				     msg->len);
> > > > >  		if (ret < 0)
> > > > >  			break;
> > > > > +
> > > > > +		if (!next_is_read)
> > > > > +			continue;
> > > > > +
> > > > > +		/* Read message following write message */
> > > > > +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> > > > > +			/* Generate a stop bit */
> > > > > +			i2c_imx_stop(i2c_bus);
> > > > > +			/* Reset i2c slave */
> > > > > +			i2c_force_reset_slave();
> > > > > +
> > > > > +			/* Enable I2C controller */
> > > > > +			if (quirk)
> > > > > +				ret = readb(base + (I2CR <<
> > > > > reg_shift))
> > > > > +					& I2CR_IDIS;
> > > > > +			else
> > > > > +				ret = !(readb(base + (I2CR <<
> > > > > reg_shift))
> > > > > +						& I2CR_IEN);
> > > > > +			if (ret) {
> > > > > +				writeb(I2CR_IEN, base +
> > > > > (I2CR << reg_shift));
> > > > > +				/* Wait for controller to be
> > > > > stable */
> > > > > +				udelay(50);
> > > > > +			}
> > > > > +
> > > > > +			/* Clear interrupt bit */
> > > > > +			writeb(I2SR_IIF_CLEAR, base + (I2SR
> > > > > << reg_shift));
> > > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > > ST_BUS_IDLE);
> > > > > +			if (ret < 0)
> > > > > +				return ret;
> > > > > +
> > > > > +			/* Start I2C transaction */
> > > > > +			temp = readb(base + (I2CR <<
> > > > > reg_shift));
> > > > > +			temp |= I2CR_MSTA;
> > > > > +			writeb(temp, base + (I2CR <<
> > > > > reg_shift)); +
> > > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > > ST_BUS_BUSY);
> > > > > +			if (ret < 0)
> > > > > +				return ret;
> > > > > +
> > > > > +			/* Enter transfer mode */
> > > > > +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> > > > > +			writeb(temp, base + (I2CR <<
> > > > > reg_shift));
> > > > > +			udelay(50);
> > > > > +		}
> > > > >  	}
> > > > >
> > > > >  	if (ret)
> > > > > diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
> > > > > index dcf0340b4d..010c45ecbf 100644
> > > > > --- a/drivers/rtc/pcf2127.c
> > > > > +++ b/drivers/rtc/pcf2127.c
> > > > > @@ -24,12 +24,9 @@
> > > > >
> > > > >  static int pcf2127_rtc_set(struct udevice *dev, const struct
> > > > > rtc_time *tm) {
> > > > > -	uchar buf[8];
> > > > > +	uchar buf[7] = {0};
> > > > >  	int i = 0, ret;
> > > > >
> > > > > -	/* start register address */
> > > > > -	buf[i++] = PCF2127_REG_SC;
> > > > > -
> > > > >  	/* hours, minutes and seconds */
> > > > >  	buf[i++] = bin2bcd(tm->tm_sec);
> > > > >  	buf[i++] = bin2bcd(tm->tm_min);
> > > > > @@ -44,7 +41,7 @@ static int pcf2127_rtc_set(struct udevice
> > > > > *dev, const struct rtc_time *tm) buf[i++] =
> > > > > bin2bcd(tm->tm_year % 100);
> > > > >
> > > > >  	/* write register's data */
> > > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf,
> > > > > sizeof(buf));
> > > > > +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
> > > > >
> > > > >  	return ret;
> > > > >  }
> > > > > @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice
> > > > > *dev, struct rtc_time *tm) int ret = 0;
> > > > >  	uchar buf[10] = { PCF2127_REG_CTRL1 };
> > > > >
> > > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> > > > > -	if (ret < 0)
> > > > > -		return ret;
> > > > >  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> > > > > sizeof(buf)); if (ret < 0)
> > > > >  		return ret;
> > > > > @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice
> > > > > *dev) return 0;
> > > > >  }
> > > > >
> > > > > +static int pcf2127_probe(struct udevice *dev) {
> > > > > +	i2c_set_chip_flags(dev,
> > > > > DM_I2C_CHIP_RD_NEED_STOP_BIT); +
> > > > > +	return 0;
> > > > > +}
> > > > > +
> > > > >  static const struct rtc_ops pcf2127_rtc_ops = {
> > > > >  	.get = pcf2127_rtc_get,
> > > > >  	.set = pcf2127_rtc_set,
> > > > > @@ -104,6 +105,7 @@ static const struct udevice_id
> > > > > pcf2127_rtc_ids[] = { U_BOOT_DRIVER(rtc_pcf2127) = {
> > > > >  	.name	= "rtc-pcf2127",
> > > > >  	.id	= UCLASS_RTC,
> > > > > +	.probe	= pcf2127_probe,
> > > > >  	.of_match = pcf2127_rtc_ids,
> > > > >  	.ops	= &pcf2127_rtc_ops,
> > > > >  };
> > > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > > a5c760c711..beaa028349 100644
> > > > > --- a/include/i2c.h
> > > > > +++ b/include/i2c.h
> > > > > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> > > > >  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit
> > > > > addressing */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send
> > > > > address for each read byte */ DM_I2C_CHIP_WR_ADDRESS	=
> > > > > 1 << 2, /* Send address for each write byte */
> > > > > +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need
> > > > > generate stop bit */ };
> > > > >
> > > > >  struct udevice;
> > > > > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> > > > >  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after
> > > > > NAK */ I2C_M_NO_RD_ACK		= 0x0800, /* skip the
> > > > > Ack bit on reads */ I2C_M_RECV_LEN		=
> > > > > 0x0400, /* length is first received byte */
> > > > > +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate
> > > > > stop bit */ };
> > > > >
> > > > >  /**  
> > > >
> > > >
> > > >
> > > >
> > > > Best regards,
> > > >
> > > > Lukasz Majewski
> > > >
> > > > --
> > > >
> > > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> > > > Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> > > > (+49)-8142-66989-80 Email: lukma at denx.de  
> > 
> > 
> > 
> > 
> > Best regards,
> > 
> > Lukasz Majewski
> > 
> > --
> > 
> > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > lukma at denx.de  




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190522/4c4da1ef/attachment.sig>

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年5月22日 19:32
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> On Wed, 22 May 2019 09:31:35 +0000
> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> 
> > > -----Original Message-----
> > > From: Lukasz Majewski <lukma@denx.de>
> > > Sent: 2019年5月22日 16:41
> > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> > > sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> > > Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> > > time
> > >
> > > Hi Chuanhua,
> > >
> > > > > -----Original Message-----
> > > > > From: Lukasz Majewski <lukma@denx.de>
> > > > > Sent: 2019年5月22日 15:16
> > > > > To: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > > <biwen.li@nxp.com>; sjg at chromium.org
> > > > > Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> > > > > time
> > > > >
> > > > > Hi Chuanhua,
> > > > >
> > > > > > Because i2c driver does not generate a stop bit when reading
> > > > > > registers of pcf2127
> > > > >
> > > > > The change (in the common i2c code) is rather large when
> > > > > considering the description above.
> > > > >
> > > > > Could you write a more detailed patch description? Is this only
> > > > > the problem with i2c in DM?
> > > > OK, This is a problem with the i2c transport framework. After
> > > > writing the register address that you want to read, the stop
> > > > signal is missing before reading the register data.
> > > > >
> > > > > Is the same code (as introduced in this commit) available in
> > > > > Linux kernel?
> > > > The kernel does not have such a problem
> > >
> > > Do you know maybe why such issue is not present on Linux kernel?
> > The kernel code is encapsulated when reading the pcf2127 register,
> > which separates the read and write, thus generating a stop signal
> > before reading the register. Eg: Here is the wrapper made by the
> > kernel code: static int pcf2127_i2c_read(void *context, const void
> > *reg, size_t reg_size, void *val, size_t val_size) {
> >         struct device *dev = context;
> >         struct i2c_client *client = to_i2c_client(dev);
> >         int ret;
> >
> >         if (WARN_ON(reg_size != 1))
> >                 return -EINVAL;
> >
> >         ret = i2c_master_send(client, reg, 1);
> >         if (ret != 1)
> >                 return ret < 0 ? ret : -EIO;
> >
> >         ret = i2c_master_recv(client, val, val_size);
> >         if (ret != val_size)
> >                 return ret < 0 ? ret : -EIO;
> >
> >         return 0;
> > }
> 
> That was my point - the same shall be done in the pcf2127 driver. This is a slow
> device, so we can afford for this.
There is no similar api in the uboot code to split the read data into the address of the send register and receive the data.
Uboot encapsulates the transmit register address and the read data, so no stop signal is generated.
> 
> > >
> > > > > How the error is reproduced? What are the symptoms of it?
> > > > You can use the i2c command to read the register data of pcf2127.
> > >
> > > So the problem is with using "i2c ..." commands from U-Boot prompt?
> > Yes,but adding debugging to the rtc driver is also the same problem
> > >
> > > > You will find that you want to read the address 0 of the register,
> > > > but the data of the 1 address is read, and the data read later
> > > > will be offset.
> > >
> > > As fair as I can tell the pcf2127 has its own DM aware driver at
> > > driver/rtc/pcf2127.c.
> > >
> > > Is this driver broken so you need to modify the generic i.MX I2C
> > > code? Have you tried to test this driver on your setup?
> > Pcf2127 driver also has problems, has been modified, need i2c general
> > code to support
> 
> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the generic one.
ok
> 
> Moreover, it looks like the same approach (first send, then read) is performed
> in the pcf2127 driver at pcf2127_rtc_get() function.
> 
> I think that the driver shall be first thoroughly checked, then fixes shall be
> added to it.
> 
> > >
> > > > >
> > > > > In which way the pcf2127 needs special treatment (non I2C
> > > > > compatible)? How does it differ from other RTC devices?
> > > > According to the pcf2127 chip manual, when you need to read the
> > > > internal register data, you need to write the register address
> > > > first and then need stop bit(it is necessary).
> > > > >
> > > > > >
> > > > > > Signed-off-by: Biwen Li <biwen.li@nxp.com>
> > > > > > Signed-off-by: Chuanhua Han <chuanhua.han@nxp.com>
> > > > > > ---
> > > > > >  drivers/i2c/i2c-uclass.c |  2 ++
> > > > > >  drivers/i2c/mxc_i2c.c    | 70
> > > > > > +++++++++++++++++++++++++++++++++++-----
> drivers/rtc/pcf2127.c
> > > > > > | 18 ++++++----- include/i2c.h            |  2 ++
> > > > > >  4 files changed, 76 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/i2c/i2c-uclass.c
> > > > > > b/drivers/i2c/i2c-uclass.c index e47abf1833..18f7364d72 100644
> > > > > > --- a/drivers/i2c/i2c-uclass.c
> > > > > > +++ b/drivers/i2c/i2c-uclass.c
> > > > > > @@ -141,6 +141,8 @@ int dm_i2c_read(struct udevice *dev, uint
> > > > > > offset, uint8_t *buffer, int len) if (len) {
> > > > > >  		ptr->addr = chip->chip_addr;
> > > > > >  		ptr->flags = chip->flags &
> > > > > > DM_I2C_CHIP_10BIT ? I2C_M_TEN : 0;
> > > > > > +		ptr->flags |= chip->flags &
> > > > > > DM_I2C_CHIP_RD_NEED_STOP_BIT ?
> > > > > > +			      I2C_M_RD_NEED_STOP_BIT : 0;
> > > > > >  		ptr->flags |= I2C_M_RD;
> > > > > >  		ptr->len = len;
> > > > > >  		ptr->buf = buffer;
> > > > > > diff --git a/drivers/i2c/mxc_i2c.c b/drivers/i2c/mxc_i2c.c
> > > > > > index 73b9807598..241367bac8 100644
> > > > > > --- a/drivers/i2c/mxc_i2c.c
> > > > > > +++ b/drivers/i2c/mxc_i2c.c
> > > > > > @@ -962,6 +962,8 @@ static int mxc_i2c_xfer(struct udevice
> > > > > > *bus, struct i2c_msg *msg, int nmsgs) int reg_shift =
> > > > > > i2c_bus->driver_data & I2C_QUIRK_FLAG ? VF610_I2C_REGSHIFT :
> > > > > > IMX_I2C_REGSHIFT; int read_mode;
> > > > > > +	bool quirk = i2c_bus->driver_data & I2C_QUIRK_FLAG ?
> > > > > > true : false;
> > > > > > +	unsigned int temp;
> > > > > >
> > > > > >  	/* Here address len is set to -1 to not send any address at
> > > > > > first.
> > > > > >  	 * Otherwise i2c_init_transfer will send the chip address
> > > > > > with write @@ -976,6 +978,7 @@ static int mxc_i2c_xfer(struct
> > > > > > udevice *bus, struct i2c_msg *msg, int
> > > > > > nmsgs) read_mode = -1; /* So it's always different on the
> > > > > > first message */ for (; nmsgs > 0; nmsgs--, msg++) { const int
> > > > > > msg_is_read = !!(msg->flags & I2C_M_RD);
> > > > > > +		bool next_is_read = nmsgs > 1 &&
> > > > > > (msg[1].flags & I2C_M_RD);
> > > > > >  		debug("i2c_xfer: chip=0x%x, len=0x%x, dir=%c\n",
> msg->addr,
> > > > > > msg->len, msg_is_read ? 'R' : 'W'); @@ -983,13 +986,16 @@
> > > > > > static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg
> > > > > > *msg, int nmsgs) if (msg_is_read !=
> > > > > > read_mode) { /* Send repeated start if not 1st message */ if
> > > > > > (read_mode != -1) {
> > > > > > -				debug("i2c_xfer:
> > > > > > [RSTART]\n");
> > > > > > -				ret = readb(base + (I2CR <<
> > > > > > reg_shift));
> > > > > > -				ret |= I2CR_RSTA;
> > > > > > -				writeb(ret, base + (I2CR <<
> > > > > > reg_shift));
> > > > > > +				if (!(msg[1].flags &
> > > > > > I2C_M_RD_NEED_STOP_BIT)) {
> > > > > > +					debug("i2c_xfer:
> > > > > > [RSTART]\n");
> > > > > > +					ret = readb(base +
> > > > > > (I2CR << reg_shift));
> > > > > > +					ret |= I2CR_RSTA;
> > > > > > +					writeb(ret, base +
> > > > > > (I2CR << reg_shift));
> > > > > > +				}
> > > > > >  			}
> > > > > >  			debug("i2c_xfer: [ADDR %02x | %c]\n",
> > > > > > msg->addr, msg_is_read ? 'R' : 'W');
> > > > > > +
> > > > > >  			ret = tx_byte(i2c_bus, (msg->addr <<
> > > > > > 1) | msg_is_read); if (ret < 0) {
> > > > > >  				debug("i2c_xfer: [STOP]\n"); @@ -999,16
> +1005,64 @@
> > > > > > static int mxc_i2c_xfer(struct udevice *bus, struct i2c_msg
> > > > > > *msg, int nmsgs) read_mode = msg_is_read;
> > > > > >  		}
> > > > > >
> > > > > > -		if (msg->flags & I2C_M_RD)
> > > > > > +		if (msg->flags & I2C_M_RD) {
> > > > > >  			ret = i2c_read_data(i2c_bus,
> > > > > > msg->addr, msg->buf, msg->len, nmsgs == 1 ||
> > > > > >  						      (msg->flags
> > > > > > & I2C_M_STOP));
> > > > > > -		else
> > > > > > -			ret = i2c_write_data(i2c_bus,
> > > > > > msg->addr, msg->buf,
> > > > > > -					     msg->len);
> > > > > > +			if (ret < 0)
> > > > > > +				break;
> > > > > > +			continue;
> > > > > > +		}
> > > > > >
> > > > > > +		 /* Write message */
> > > > > > +		ret = i2c_write_data(i2c_bus, msg->addr,
> > > > > > msg->buf,
> > > > > > +				     msg->len);
> > > > > >  		if (ret < 0)
> > > > > >  			break;
> > > > > > +
> > > > > > +		if (!next_is_read)
> > > > > > +			continue;
> > > > > > +
> > > > > > +		/* Read message following write message */
> > > > > > +		if (msg[1].flags & I2C_M_RD_NEED_STOP_BIT) {
> > > > > > +			/* Generate a stop bit */
> > > > > > +			i2c_imx_stop(i2c_bus);
> > > > > > +			/* Reset i2c slave */
> > > > > > +			i2c_force_reset_slave();
> > > > > > +
> > > > > > +			/* Enable I2C controller */
> > > > > > +			if (quirk)
> > > > > > +				ret = readb(base + (I2CR <<
> > > > > > reg_shift))
> > > > > > +					& I2CR_IDIS;
> > > > > > +			else
> > > > > > +				ret = !(readb(base + (I2CR <<
> > > > > > reg_shift))
> > > > > > +						& I2CR_IEN);
> > > > > > +			if (ret) {
> > > > > > +				writeb(I2CR_IEN, base +
> > > > > > (I2CR << reg_shift));
> > > > > > +				/* Wait for controller to be
> > > > > > stable */
> > > > > > +				udelay(50);
> > > > > > +			}
> > > > > > +
> > > > > > +			/* Clear interrupt bit */
> > > > > > +			writeb(I2SR_IIF_CLEAR, base + (I2SR
> > > > > > << reg_shift));
> > > > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > > > ST_BUS_IDLE);
> > > > > > +			if (ret < 0)
> > > > > > +				return ret;
> > > > > > +
> > > > > > +			/* Start I2C transaction */
> > > > > > +			temp = readb(base + (I2CR <<
> > > > > > reg_shift));
> > > > > > +			temp |= I2CR_MSTA;
> > > > > > +			writeb(temp, base + (I2CR <<
> > > > > > reg_shift)); +
> > > > > > +			ret = wait_for_sr_state(i2c_bus,
> > > > > > ST_BUS_BUSY);
> > > > > > +			if (ret < 0)
> > > > > > +				return ret;
> > > > > > +
> > > > > > +			/* Enter transfer mode */
> > > > > > +			temp |= I2CR_MTX | I2CR_TX_NO_AK;
> > > > > > +			writeb(temp, base + (I2CR <<
> > > > > > reg_shift));
> > > > > > +			udelay(50);
> > > > > > +		}
> > > > > >  	}
> > > > > >
> > > > > >  	if (ret)
> > > > > > diff --git a/drivers/rtc/pcf2127.c b/drivers/rtc/pcf2127.c
> > > > > > index dcf0340b4d..010c45ecbf 100644
> > > > > > --- a/drivers/rtc/pcf2127.c
> > > > > > +++ b/drivers/rtc/pcf2127.c
> > > > > > @@ -24,12 +24,9 @@
> > > > > >
> > > > > >  static int pcf2127_rtc_set(struct udevice *dev, const struct
> > > > > > rtc_time *tm) {
> > > > > > -	uchar buf[8];
> > > > > > +	uchar buf[7] = {0};
> > > > > >  	int i = 0, ret;
> > > > > >
> > > > > > -	/* start register address */
> > > > > > -	buf[i++] = PCF2127_REG_SC;
> > > > > > -
> > > > > >  	/* hours, minutes and seconds */
> > > > > >  	buf[i++] = bin2bcd(tm->tm_sec);
> > > > > >  	buf[i++] = bin2bcd(tm->tm_min); @@ -44,7 +41,7 @@ static int
> > > > > > pcf2127_rtc_set(struct udevice *dev, const struct rtc_time
> > > > > > *tm) buf[i++] = bin2bcd(tm->tm_year % 100);
> > > > > >
> > > > > >  	/* write register's data */
> > > > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf,
> > > > > > sizeof(buf));
> > > > > > +	ret = dm_i2c_write(dev, PCF2127_REG_SC, buf, i);
> > > > > >
> > > > > >  	return ret;
> > > > > >  }
> > > > > > @@ -54,9 +51,6 @@ static int pcf2127_rtc_get(struct udevice
> > > > > > *dev, struct rtc_time *tm) int ret = 0;
> > > > > >  	uchar buf[10] = { PCF2127_REG_CTRL1 };
> > > > > >
> > > > > > -	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> > > > > > -	if (ret < 0)
> > > > > > -		return ret;
> > > > > >  	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
> > > > > > if (ret < 0)
> > > > > >  		return ret;
> > > > > > @@ -90,6 +84,13 @@ static int pcf2127_rtc_reset(struct udevice
> > > > > > *dev) return 0;
> > > > > >  }
> > > > > >
> > > > > > +static int pcf2127_probe(struct udevice *dev) {
> > > > > > +	i2c_set_chip_flags(dev,
> > > > > > DM_I2C_CHIP_RD_NEED_STOP_BIT); +
> > > > > > +	return 0;
> > > > > > +}
> > > > > > +
> > > > > >  static const struct rtc_ops pcf2127_rtc_ops = {
> > > > > >  	.get = pcf2127_rtc_get,
> > > > > >  	.set = pcf2127_rtc_set,
> > > > > > @@ -104,6 +105,7 @@ static const struct udevice_id
> > > > > > pcf2127_rtc_ids[] = { U_BOOT_DRIVER(rtc_pcf2127) = {
> > > > > >  	.name	= "rtc-pcf2127",
> > > > > >  	.id	= UCLASS_RTC,
> > > > > > +	.probe	= pcf2127_probe,
> > > > > >  	.of_match = pcf2127_rtc_ids,
> > > > > >  	.ops	= &pcf2127_rtc_ops,
> > > > > >  };
> > > > > > diff --git a/include/i2c.h b/include/i2c.h index
> > > > > > a5c760c711..beaa028349 100644
> > > > > > --- a/include/i2c.h
> > > > > > +++ b/include/i2c.h
> > > > > > @@ -28,6 +28,7 @@ enum dm_i2c_chip_flags {
> > > > > >  	DM_I2C_CHIP_10BIT	= 1 << 0, /* Use 10-bit
> > > > > > addressing */ DM_I2C_CHIP_RD_ADDRESS	= 1 << 1, /* Send
> > > > > > address for each read byte */ DM_I2C_CHIP_WR_ADDRESS	=
> > > > > > 1 << 2, /* Send address for each write byte */
> > > > > > +	DM_I2C_CHIP_RD_NEED_STOP_BIT    = 1 << 3, /* Need
> > > > > > generate stop bit */ };
> > > > > >
> > > > > >  struct udevice;
> > > > > > @@ -87,6 +88,7 @@ enum dm_i2c_msg_flags {
> > > > > >  	I2C_M_IGNORE_NAK	= 0x1000, /* continue after
> > > > > > NAK */ I2C_M_NO_RD_ACK		= 0x0800, /* skip the
> > > > > > Ack bit on reads */ I2C_M_RECV_LEN		=
> > > > > > 0x0400, /* length is first received byte */
> > > > > > +	I2C_M_RD_NEED_STOP_BIT  = 0x0002, /* need generate
> > > > > > stop bit */ };
> > > > > >
> > > > > >  /**
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > Best regards,
> > > > >
> > > > > Lukasz Majewski
> > > > >
> > > > > --
> > > > >
> > > > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194
> > > > > Groebenzell, Germany Phone: (+49)-8142-66989-59 Fax:
> > > > > (+49)-8142-66989-80 Email: lukma at denx.de
> > >
> > >
> > >
> > >
> > > Best regards,
> > >
> > > Lukasz Majewski
> > >
> > > --
> > >
> > > DENX Software Engineering GmbH,      Managing Director: Wolfgang
> > > Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell,
> > > Germany Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> > > lukma at denx.de
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Heiko Schocher]

Hello Chuanhua Han,

Am 22.05.2019 um 14:45 schrieb Chuanhua Han:
> 
> 
>> -----Original Message-----
>> From: Lukasz Majewski <lukma@denx.de>
>> Sent: 2019年5月22日 19:32
>> To: Chuanhua Han <chuanhua.han@nxp.com>
>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
>> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
>> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
>>
>> On Wed, 22 May 2019 09:31:35 +0000
>> Chuanhua Han <chuanhua.han@nxp.com> wrote:
>>
>>>> -----Original Message-----
>>>> From: Lukasz Majewski <lukma@denx.de>
>>>> Sent: 2019年5月22日 16:41
>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
>>>> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
>>>> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
>>>> time
>>>>
>>>> Hi Chuanhua,
>>>>
>>>>>> -----Original Message-----
>>>>>> From: Lukasz Majewski <lukma@denx.de>
>>>>>> Sent: 2019年5月22日 15:16
>>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
>>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
>>>>>> <biwen.li@nxp.com>; sjg at chromium.org
>>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
>>>>>> time
>>>>>>
>>>>>> Hi Chuanhua,
>>>>>>
>>>>>>> Because i2c driver does not generate a stop bit when reading
>>>>>>> registers of pcf2127
>>>>>>
>>>>>> The change (in the common i2c code) is rather large when
>>>>>> considering the description above.
>>>>>>
>>>>>> Could you write a more detailed patch description? Is this only
>>>>>> the problem with i2c in DM?
>>>>> OK, This is a problem with the i2c transport framework. After
>>>>> writing the register address that you want to read, the stop
>>>>> signal is missing before reading the register data.
>>>>>>
>>>>>> Is the same code (as introduced in this commit) available in
>>>>>> Linux kernel?
>>>>> The kernel does not have such a problem
>>>>
>>>> Do you know maybe why such issue is not present on Linux kernel?
>>> The kernel code is encapsulated when reading the pcf2127 register,
>>> which separates the read and write, thus generating a stop signal
>>> before reading the register. Eg: Here is the wrapper made by the
>>> kernel code: static int pcf2127_i2c_read(void *context, const void
>>> *reg, size_t reg_size, void *val, size_t val_size) {
>>>          struct device *dev = context;
>>>          struct i2c_client *client = to_i2c_client(dev);
>>>          int ret;
>>>
>>>          if (WARN_ON(reg_size != 1))
>>>                  return -EINVAL;
>>>
>>>          ret = i2c_master_send(client, reg, 1);
>>>          if (ret != 1)
>>>                  return ret < 0 ? ret : -EIO;
>>>
>>>          ret = i2c_master_recv(client, val, val_size);
>>>          if (ret != val_size)
>>>                  return ret < 0 ? ret : -EIO;
>>>
>>>          return 0;
>>> }
>>
>> That was my point - the same shall be done in the pcf2127 driver. This is a slow
>> device, so we can afford for this.
> There is no similar api in the uboot code to split the read data into the address of the send register and receive the data.
> Uboot encapsulates the transmit register address and the read data, so no stop signal is generated.

If this API is missing, please introduce it. But not in one big patch
instead split it into:

- introduce I2C_M_RD_NEED_STOP_BIT

- support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c

   or may we need a more common approach to this?

- pcf2127 driver changes

Also, as Lukasz mentioned, provide commit messages with more
information, what you do and why.

>>
>>>>
>>>>>> How the error is reproduced? What are the symptoms of it?
>>>>> You can use the i2c command to read the register data of pcf2127.
>>>>
>>>> So the problem is with using "i2c ..." commands from U-Boot prompt?
>>> Yes,but adding debugging to the rtc driver is also the same problem
>>>>
>>>>> You will find that you want to read the address 0 of the register,
>>>>> but the data of the 1 address is read, and the data read later
>>>>> will be offset.
>>>>
>>>> As fair as I can tell the pcf2127 has its own DM aware driver at
>>>> driver/rtc/pcf2127.c.
>>>>
>>>> Is this driver broken so you need to modify the generic i.MX I2C
>>>> code? Have you tried to test this driver on your setup?
>>> Pcf2127 driver also has problems, has been modified, need i2c general
>>> code to support
>>
>> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the generic one.
> ok
>>
>> Moreover, it looks like the same approach (first send, then read) is performed
>> in the pcf2127 driver at pcf2127_rtc_get() function.
>>
>> I think that the driver shall be first thoroughly checked, then fixes shall be
>> added to it.

I have no such device, so hard to say ... and as Lukasz alread mentioned
the driver seems to make such an approach:

  52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
  53 {
  54         int ret = 0;
  55         uchar buf[10] = { PCF2127_REG_CTRL1 };
  56
  57         ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
  58         if (ret < 0)
  59                 return ret;
  60         ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
  61         if (ret < 0)
  62                 return ret;


It seems there are currently no real users of this driver:

pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
./drivers/rtc/Kconfig
./drivers/rtc/Makefile
pollux:u-boot hs [master] $

I added Meng Yi to cc, as he is the author of this driver. May he
can say here more... at last I hope, the driver worked for him.

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

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Lukasz Majewski]

Hi Heiko,

> Hello Chuanhua Han,
> 
> Am 22.05.2019 um 14:45 schrieb Chuanhua Han:
> > 
> >   
> >> -----Original Message-----
> >> From: Lukasz Majewski <lukma@denx.de>
> >> Sent: 2019年5月22日 19:32
> >> To: Chuanhua Han <chuanhua.han@nxp.com>
> >> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> >> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> >> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> >> wrong time
> >>
> >> On Wed, 22 May 2019 09:31:35 +0000
> >> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >>  
> >>>> -----Original Message-----
> >>>> From: Lukasz Majewski <lukma@denx.de>
> >>>> Sent: 2019年5月22日 16:41
> >>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> >>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> >>>> <biwen.li@nxp.com>; sjg at chromium.org; Stefano Babic
> >>>> <sbabic@denx.de> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127:
> >>>> fix bug that read wrong time
> >>>>
> >>>> Hi Chuanhua,
> >>>>  
> >>>>>> -----Original Message-----
> >>>>>> From: Lukasz Majewski <lukma@denx.de>
> >>>>>> Sent: 2019年5月22日 15:16
> >>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> >>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> >>>>>> <biwen.li@nxp.com>; sjg at chromium.org
> >>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> >>>>>> wrong time
> >>>>>>
> >>>>>> Hi Chuanhua,
> >>>>>>  
> >>>>>>> Because i2c driver does not generate a stop bit when reading
> >>>>>>> registers of pcf2127  
> >>>>>>
> >>>>>> The change (in the common i2c code) is rather large when
> >>>>>> considering the description above.
> >>>>>>
> >>>>>> Could you write a more detailed patch description? Is this only
> >>>>>> the problem with i2c in DM?  
> >>>>> OK, This is a problem with the i2c transport framework. After
> >>>>> writing the register address that you want to read, the stop
> >>>>> signal is missing before reading the register data.  
> >>>>>>
> >>>>>> Is the same code (as introduced in this commit) available in
> >>>>>> Linux kernel?  
> >>>>> The kernel does not have such a problem  
> >>>>
> >>>> Do you know maybe why such issue is not present on Linux
> >>>> kernel?  
> >>> The kernel code is encapsulated when reading the pcf2127 register,
> >>> which separates the read and write, thus generating a stop signal
> >>> before reading the register. Eg: Here is the wrapper made by the
> >>> kernel code: static int pcf2127_i2c_read(void *context, const void
> >>> *reg, size_t reg_size, void *val, size_t val_size) {
> >>>          struct device *dev = context;
> >>>          struct i2c_client *client = to_i2c_client(dev);
> >>>          int ret;
> >>>
> >>>          if (WARN_ON(reg_size != 1))
> >>>                  return -EINVAL;
> >>>
> >>>          ret = i2c_master_send(client, reg, 1);
> >>>          if (ret != 1)
> >>>                  return ret < 0 ? ret : -EIO;
> >>>
> >>>          ret = i2c_master_recv(client, val, val_size);
> >>>          if (ret != val_size)
> >>>                  return ret < 0 ? ret : -EIO;
> >>>
> >>>          return 0;
> >>> }  
> >>
> >> That was my point - the same shall be done in the pcf2127 driver.
> >> This is a slow device, so we can afford for this.  
> > There is no similar api in the uboot code to split the read data
> > into the address of the send register and receive the data. Uboot
> > encapsulates the transmit register address and the read data, so no
> > stop signal is generated.  
> 
> If this API is missing, please introduce it. But not in one big patch
> instead split it into:
> 
> - introduce I2C_M_RD_NEED_STOP_BIT
> 
> - support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c
> 
>    or may we need a more common approach to this?
> 
> - pcf2127 driver changes
> 
> Also, as Lukasz mentioned, provide commit messages with more
> information, what you do and why.

Verbose commit messages help with understanding the _real_ problem
being solved by the patch. Also are helpful in the future as a
documentation and reference.

> 
> >>  
> >>>>  
> >>>>>> How the error is reproduced? What are the symptoms of it?  
> >>>>> You can use the i2c command to read the register data of
> >>>>> pcf2127.  
> >>>>
> >>>> So the problem is with using "i2c ..." commands from U-Boot
> >>>> prompt?  
> >>> Yes,but adding debugging to the rtc driver is also the same
> >>> problem  
> >>>>  
> >>>>> You will find that you want to read the address 0 of the
> >>>>> register, but the data of the 1 address is read, and the data
> >>>>> read later will be offset.  
> >>>>
> >>>> As fair as I can tell the pcf2127 has its own DM aware driver at
> >>>> driver/rtc/pcf2127.c.
> >>>>
> >>>> Is this driver broken so you need to modify the generic i.MX I2C
> >>>> code? Have you tried to test this driver on your setup?  
> >>> Pcf2127 driver also has problems, has been modified, need i2c
> >>> general code to support  
> >>
> >> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the
> >> generic one.  
> > ok  
> >>
> >> Moreover, it looks like the same approach (first send, then read)
> >> is performed in the pcf2127 driver at pcf2127_rtc_get() function.
> >>
> >> I think that the driver shall be first thoroughly checked, then
> >> fixes shall be added to it.  
> 
> I have no such device, so hard to say ... and as Lukasz alread
> mentioned the driver seems to make such an approach:
> 
>   52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time
> *tm) 53 {
>   54         int ret = 0;
>   55         uchar buf[10] = { PCF2127_REG_CTRL1 };
>   56
>   57         ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
>   58         if (ret < 0)
>   59                 return ret;
>   60         ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> sizeof(buf)); 61         if (ret < 0)
>   62                 return ret;
> 

I would prefer to fix the issue in the driver itself. Only when it is
not possible we shall introduce extra flags and modify the common I2C
code.

> 
> It seems there are currently no real users of this driver:
> 
> pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
> ./drivers/rtc/Kconfig
> ./drivers/rtc/Makefile
> pollux:u-boot hs [master] $
> 
> I added Meng Yi to cc, as he is the author of this driver. May he
> can say here more... at last I hope, the driver worked for him.
> 
> bye,
> Heiko




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190523/91a1ce26/attachment.sig>

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Heiko Schocher]

Hello Lukasz,

Am 23.05.2019 um 07:54 schrieb Lukasz Majewski:
> Hi Heiko,
> 
>> Hello Chuanhua Han,
>>
>> Am 22.05.2019 um 14:45 schrieb Chuanhua Han:
>>>
>>>    
>>>> -----Original Message-----
>>>> From: Lukasz Majewski <lukma@denx.de>
>>>> Sent: 2019年5月22日 19:32
>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
>>>> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
>>>> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
>>>> wrong time
>>>>
>>>> On Wed, 22 May 2019 09:31:35 +0000
>>>> Chuanhua Han <chuanhua.han@nxp.com> wrote:
>>>>   
>>>>>> -----Original Message-----
>>>>>> From: Lukasz Majewski <lukma@denx.de>
>>>>>> Sent: 2019年5月22日 16:41
>>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
>>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
>>>>>> <biwen.li@nxp.com>; sjg at chromium.org; Stefano Babic
>>>>>> <sbabic@denx.de> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127:
>>>>>> fix bug that read wrong time
>>>>>>
>>>>>> Hi Chuanhua,
>>>>>>   
>>>>>>>> -----Original Message-----
>>>>>>>> From: Lukasz Majewski <lukma@denx.de>
>>>>>>>> Sent: 2019年5月22日 15:16
>>>>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
>>>>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
>>>>>>>> <biwen.li@nxp.com>; sjg at chromium.org
>>>>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
>>>>>>>> wrong time
>>>>>>>>
>>>>>>>> Hi Chuanhua,
>>>>>>>>   
>>>>>>>>> Because i2c driver does not generate a stop bit when reading
>>>>>>>>> registers of pcf2127
>>>>>>>>
>>>>>>>> The change (in the common i2c code) is rather large when
>>>>>>>> considering the description above.
>>>>>>>>
>>>>>>>> Could you write a more detailed patch description? Is this only
>>>>>>>> the problem with i2c in DM?
>>>>>>> OK, This is a problem with the i2c transport framework. After
>>>>>>> writing the register address that you want to read, the stop
>>>>>>> signal is missing before reading the register data.
>>>>>>>>
>>>>>>>> Is the same code (as introduced in this commit) available in
>>>>>>>> Linux kernel?
>>>>>>> The kernel does not have such a problem
>>>>>>
>>>>>> Do you know maybe why such issue is not present on Linux
>>>>>> kernel?
>>>>> The kernel code is encapsulated when reading the pcf2127 register,
>>>>> which separates the read and write, thus generating a stop signal
>>>>> before reading the register. Eg: Here is the wrapper made by the
>>>>> kernel code: static int pcf2127_i2c_read(void *context, const void
>>>>> *reg, size_t reg_size, void *val, size_t val_size) {
>>>>>           struct device *dev = context;
>>>>>           struct i2c_client *client = to_i2c_client(dev);
>>>>>           int ret;
>>>>>
>>>>>           if (WARN_ON(reg_size != 1))
>>>>>                   return -EINVAL;
>>>>>
>>>>>           ret = i2c_master_send(client, reg, 1);
>>>>>           if (ret != 1)
>>>>>                   return ret < 0 ? ret : -EIO;
>>>>>
>>>>>           ret = i2c_master_recv(client, val, val_size);
>>>>>           if (ret != val_size)
>>>>>                   return ret < 0 ? ret : -EIO;
>>>>>
>>>>>           return 0;
>>>>> }
>>>>
>>>> That was my point - the same shall be done in the pcf2127 driver.
>>>> This is a slow device, so we can afford for this.
>>> There is no similar api in the uboot code to split the read data
>>> into the address of the send register and receive the data. Uboot
>>> encapsulates the transmit register address and the read data, so no
>>> stop signal is generated.
>>
>> If this API is missing, please introduce it. But not in one big patch
>> instead split it into:
>>
>> - introduce I2C_M_RD_NEED_STOP_BIT
>>
>> - support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c
>>
>>     or may we need a more common approach to this?
>>
>> - pcf2127 driver changes
>>
>> Also, as Lukasz mentioned, provide commit messages with more
>> information, what you do and why.
> 
> Verbose commit messages help with understanding the _real_ problem
> being solved by the patch. Also are helpful in the future as a
> documentation and reference.

Exactly.

>>>>>>>> How the error is reproduced? What are the symptoms of it?
>>>>>>> You can use the i2c command to read the register data of
>>>>>>> pcf2127.
>>>>>>
>>>>>> So the problem is with using "i2c ..." commands from U-Boot
>>>>>> prompt?
>>>>> Yes,but adding debugging to the rtc driver is also the same
>>>>> problem
>>>>>>   
>>>>>>> You will find that you want to read the address 0 of the
>>>>>>> register, but the data of the 1 address is read, and the data
>>>>>>> read later will be offset.
>>>>>>
>>>>>> As fair as I can tell the pcf2127 has its own DM aware driver at
>>>>>> driver/rtc/pcf2127.c.
>>>>>>
>>>>>> Is this driver broken so you need to modify the generic i.MX I2C
>>>>>> code? Have you tried to test this driver on your setup?
>>>>> Pcf2127 driver also has problems, has been modified, need i2c
>>>>> general code to support
>>>>
>>>> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the
>>>> generic one.
>>> ok
>>>>
>>>> Moreover, it looks like the same approach (first send, then read)
>>>> is performed in the pcf2127 driver at pcf2127_rtc_get() function.
>>>>
>>>> I think that the driver shall be first thoroughly checked, then
>>>> fixes shall be added to it.
>>
>> I have no such device, so hard to say ... and as Lukasz alread
>> mentioned the driver seems to make such an approach:
>>
>>    52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time
>> *tm) 53 {
>>    54         int ret = 0;
>>    55         uchar buf[10] = { PCF2127_REG_CTRL1 };
>>    56
>>    57         ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
>>    58         if (ret < 0)
>>    59                 return ret;
>>    60         ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
>> sizeof(buf)); 61         if (ret < 0)
>>    62                 return ret;
>>
> 
> I would prefer to fix the issue in the driver itself. Only when it is
> not possible we shall introduce extra flags and modify the common I2C
> code.

If possible, yes! I wonder: if this driver is in mainline, it must
at least worked for Meng Yi (as driver author) ... unfortunately
the email address is not valid ... so removed him from cc list.

bye,
Heiko
> 
>>
>> It seems there are currently no real users of this driver:
>>
>> pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
>> ./drivers/rtc/Kconfig
>> ./drivers/rtc/Makefile
>> pollux:u-boot hs [master] $
>>
>> I added Meng Yi to cc, as he is the author of this driver. May he
>> can say here more... at last I hope, the driver worked for him.
>>
>> bye,
>> Heiko
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
> 

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

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

> -----Original Message-----
> From: Heiko Schocher <hs@denx.de>
> Sent: 2019年5月23日 11:29
> To: Chuanhua Han <chuanhua.han@nxp.com>
> Cc: Lukasz Majewski <lukma@denx.de>; u-boot at lists.denx.de; Biwen Li
> <biwen.li@nxp.com>; sjg at chromium.org; Stefano Babic <sbabic@denx.de>;
> Meng Yi <meng.yi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> Caution: EXT Email
> 
> Hello Chuanhua Han,
> 
> Am 22.05.2019 um 14:45 schrieb Chuanhua Han:
> >
> >
> >> -----Original Message-----
> >> From: Lukasz Majewski <lukma@denx.de>
> >> Sent: 2019年5月22日 19:32
> >> To: Chuanhua Han <chuanhua.han@nxp.com>
> >> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> >> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> >> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> >> time
> >>
> >> On Wed, 22 May 2019 09:31:35 +0000
> >> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> >>
> >>>> -----Original Message-----
> >>>> From: Lukasz Majewski <lukma@denx.de>
> >>>> Sent: 2019年5月22日 16:41
> >>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> >>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> >>>> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> >>>> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> >>>> wrong time
> >>>>
> >>>> Hi Chuanhua,
> >>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Lukasz Majewski <lukma@denx.de>
> >>>>>> Sent: 2019年5月22日 15:16
> >>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> >>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> >>>>>> <biwen.li@nxp.com>; sjg at chromium.org
> >>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong
> >>>>>> time
> >>>>>>
> >>>>>> Hi Chuanhua,
> >>>>>>
> >>>>>>> Because i2c driver does not generate a stop bit when reading
> >>>>>>> registers of pcf2127
> >>>>>>
> >>>>>> The change (in the common i2c code) is rather large when
> >>>>>> considering the description above.
> >>>>>>
> >>>>>> Could you write a more detailed patch description? Is this only
> >>>>>> the problem with i2c in DM?
> >>>>> OK, This is a problem with the i2c transport framework. After
> >>>>> writing the register address that you want to read, the stop
> >>>>> signal is missing before reading the register data.
> >>>>>>
> >>>>>> Is the same code (as introduced in this commit) available in
> >>>>>> Linux kernel?
> >>>>> The kernel does not have such a problem
> >>>>
> >>>> Do you know maybe why such issue is not present on Linux kernel?
> >>> The kernel code is encapsulated when reading the pcf2127 register,
> >>> which separates the read and write, thus generating a stop signal
> >>> before reading the register. Eg: Here is the wrapper made by the
> >>> kernel code: static int pcf2127_i2c_read(void *context, const void
> >>> *reg, size_t reg_size, void *val, size_t val_size) {
> >>>          struct device *dev = context;
> >>>          struct i2c_client *client = to_i2c_client(dev);
> >>>          int ret;
> >>>
> >>>          if (WARN_ON(reg_size != 1))
> >>>                  return -EINVAL;
> >>>
> >>>          ret = i2c_master_send(client, reg, 1);
> >>>          if (ret != 1)
> >>>                  return ret < 0 ? ret : -EIO;
> >>>
> >>>          ret = i2c_master_recv(client, val, val_size);
> >>>          if (ret != val_size)
> >>>                  return ret < 0 ? ret : -EIO;
> >>>
> >>>          return 0;
> >>> }
> >>
> >> That was my point - the same shall be done in the pcf2127 driver.
> >> This is a slow device, so we can afford for this.
> > There is no similar api in the uboot code to split the read data into the
> address of the send register and receive the data.
> > Uboot encapsulates the transmit register address and the read data, so no
> stop signal is generated.
> 
> If this API is missing, please introduce it. But not in one big patch instead split
> it into:
> 
> - introduce I2C_M_RD_NEED_STOP_BIT
> 
> - support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c
> 
>    or may we need a more common approach to this?
> 
> - pcf2127 driver changes
> 
> Also, as Lukasz mentioned, provide commit messages with more information,
> what you do and why.
Ok, Next I will split the patch and provide a more detailed description messages.
> 
> >>
> >>>>
> >>>>>> How the error is reproduced? What are the symptoms of it?
> >>>>> You can use the i2c command to read the register data of pcf2127.
> >>>>
> >>>> So the problem is with using "i2c ..." commands from U-Boot prompt?
> >>> Yes,but adding debugging to the rtc driver is also the same problem
> >>>>
> >>>>> You will find that you want to read the address 0 of the register,
> >>>>> but the data of the 1 address is read, and the data read later
> >>>>> will be offset.
> >>>>
> >>>> As fair as I can tell the pcf2127 has its own DM aware driver at
> >>>> driver/rtc/pcf2127.c.
> >>>>
> >>>> Is this driver broken so you need to modify the generic i.MX I2C
> >>>> code? Have you tried to test this driver on your setup?
> >>> Pcf2127 driver also has problems, has been modified, need i2c
> >>> general code to support
> >>
> >> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the generic one.
> > ok
> >>
> >> Moreover, it looks like the same approach (first send, then read) is
> >> performed in the pcf2127 driver at pcf2127_rtc_get() function.
> >>
> >> I think that the driver shall be first thoroughly checked, then fixes
> >> shall be added to it.
> 
> I have no such device, so hard to say ... and as Lukasz alread mentioned the
> driver seems to make such an approach:
> 
>   52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time *tm)
>   53 {
>   54         int ret = 0;
>   55         uchar buf[10] = { PCF2127_REG_CTRL1 };
>   56
>   57         ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
>   58         if (ret < 0)
>   59                 return ret;
>   60         ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
>   61         if (ret < 0)
>   62                 return ret;
> 
> 
Although this can be done logically, the dm_i2c_read function is designed to put the address of the write register 
and the read data in a call to the ops->xfer(bus, msg, msg_count) function, so that we can't get us. Expected effect 
(there is no stop signal after the address of the write register but the restart signal)
> It seems there are currently no real users of this driver:
> 
> pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
> ./drivers/rtc/Kconfig
> ./drivers/rtc/Makefile
> pollux:u-boot hs [master] $
> 
> I added Meng Yi to cc, as he is the author of this driver. May he can say here
> more... at last I hope, the driver worked for him.
True, currently only the NXP layerscape platform board users the pcf2127 chip, but Meng Yi has now left the company.
 Now I will fix the patch and send the next version.
> 
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[U-Boot] [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time

[Chuanhua Han]

> -----Original Message-----
> From: Lukasz Majewski <lukma@denx.de>
> Sent: 2019年5月23日 13:54
> To: Heiko Schocher <hs@denx.de>
> Cc: Chuanhua Han <chuanhua.han@nxp.com>; u-boot at lists.denx.de; Biwen Li
> <biwen.li@nxp.com>; sjg at chromium.org; Stefano Babic <sbabic@denx.de>;
> Meng Yi <meng.yi@nxp.com>
> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> Hi Heiko,
> 
> > Hello Chuanhua Han,
> >
> > Am 22.05.2019 um 14:45 schrieb Chuanhua Han:
> > >
> > >
> > >> -----Original Message-----
> > >> From: Lukasz Majewski <lukma@denx.de>
> > >> Sent: 2019年5月22日 19:32
> > >> To: Chuanhua Han <chuanhua.han@nxp.com>
> > >> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li@nxp.com>;
> > >> sjg at chromium.org; Stefano Babic <sbabic@denx.de>
> > >> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> > >> wrong time
> > >>
> > >> On Wed, 22 May 2019 09:31:35 +0000
> > >> Chuanhua Han <chuanhua.han@nxp.com> wrote:
> > >>
> > >>>> -----Original Message-----
> > >>>> From: Lukasz Majewski <lukma@denx.de>
> > >>>> Sent: 2019年5月22日 16:41
> > >>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> > >>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > >>>> <biwen.li@nxp.com>; sjg at chromium.org; Stefano Babic
> > >>>> <sbabic@denx.de> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127:
> > >>>> fix bug that read wrong time
> > >>>>
> > >>>> Hi Chuanhua,
> > >>>>
> > >>>>>> -----Original Message-----
> > >>>>>> From: Lukasz Majewski <lukma@denx.de>
> > >>>>>> Sent: 2019年5月22日 15:16
> > >>>>>> To: Chuanhua Han <chuanhua.han@nxp.com>
> > >>>>>> Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > >>>>>> <biwen.li@nxp.com>; sjg at chromium.org
> > >>>>>> Subject: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> > >>>>>> wrong time
> > >>>>>>
> > >>>>>> Hi Chuanhua,
> > >>>>>>
> > >>>>>>> Because i2c driver does not generate a stop bit when reading
> > >>>>>>> registers of pcf2127
> > >>>>>>
> > >>>>>> The change (in the common i2c code) is rather large when
> > >>>>>> considering the description above.
> > >>>>>>
> > >>>>>> Could you write a more detailed patch description? Is this only
> > >>>>>> the problem with i2c in DM?
> > >>>>> OK, This is a problem with the i2c transport framework. After
> > >>>>> writing the register address that you want to read, the stop
> > >>>>> signal is missing before reading the register data.
> > >>>>>>
> > >>>>>> Is the same code (as introduced in this commit) available in
> > >>>>>> Linux kernel?
> > >>>>> The kernel does not have such a problem
> > >>>>
> > >>>> Do you know maybe why such issue is not present on Linux kernel?
> > >>> The kernel code is encapsulated when reading the pcf2127 register,
> > >>> which separates the read and write, thus generating a stop signal
> > >>> before reading the register. Eg: Here is the wrapper made by the
> > >>> kernel code: static int pcf2127_i2c_read(void *context, const void
> > >>> *reg, size_t reg_size, void *val, size_t val_size) {
> > >>>          struct device *dev = context;
> > >>>          struct i2c_client *client = to_i2c_client(dev);
> > >>>          int ret;
> > >>>
> > >>>          if (WARN_ON(reg_size != 1))
> > >>>                  return -EINVAL;
> > >>>
> > >>>          ret = i2c_master_send(client, reg, 1);
> > >>>          if (ret != 1)
> > >>>                  return ret < 0 ? ret : -EIO;
> > >>>
> > >>>          ret = i2c_master_recv(client, val, val_size);
> > >>>          if (ret != val_size)
> > >>>                  return ret < 0 ? ret : -EIO;
> > >>>
> > >>>          return 0;
> > >>> }
> > >>
> > >> That was my point - the same shall be done in the pcf2127 driver.
> > >> This is a slow device, so we can afford for this.
> > > There is no similar api in the uboot code to split the read data
> > > into the address of the send register and receive the data. Uboot
> > > encapsulates the transmit register address and the read data, so no
> > > stop signal is generated.
> >
> > If this API is missing, please introduce it. But not in one big patch
> > instead split it into:
> >
> > - introduce I2C_M_RD_NEED_STOP_BIT
> >
> > - support flag I2C_M_RD_NEED_STOP_BIT in driver drivers/i2c/mxc_i2c.c
> >
> >    or may we need a more common approach to this?
> >
> > - pcf2127 driver changes
> >
> > Also, as Lukasz mentioned, provide commit messages with more
> > information, what you do and why.
> 
> Verbose commit messages help with understanding the _real_ problem being
> solved by the patch. Also are helpful in the future as a documentation and
> reference.
I'll sort out the details in the next patch
> 
> >
> > >>
> > >>>>
> > >>>>>> How the error is reproduced? What are the symptoms of it?
> > >>>>> You can use the i2c command to read the register data of
> > >>>>> pcf2127.
> > >>>>
> > >>>> So the problem is with using "i2c ..." commands from U-Boot
> > >>>> prompt?
> > >>> Yes,but adding debugging to the rtc driver is also the same
> > >>> problem
> > >>>>
> > >>>>> You will find that you want to read the address 0 of the
> > >>>>> register, but the data of the 1 address is read, and the data
> > >>>>> read later will be offset.
> > >>>>
> > >>>> As fair as I can tell the pcf2127 has its own DM aware driver at
> > >>>> driver/rtc/pcf2127.c.
> > >>>>
> > >>>> Is this driver broken so you need to modify the generic i.MX I2C
> > >>>> code? Have you tried to test this driver on your setup?
> > >>> Pcf2127 driver also has problems, has been modified, need i2c
> > >>> general code to support
> > >>
> > >> Just one remark the mxc_i2c.c is IMX specific I2C code. Not the
> > >> generic one.
> > > ok
> > >>
> > >> Moreover, it looks like the same approach (first send, then read)
> > >> is performed in the pcf2127 driver at pcf2127_rtc_get() function.
> > >>
> > >> I think that the driver shall be first thoroughly checked, then
> > >> fixes shall be added to it.
> >
> > I have no such device, so hard to say ... and as Lukasz alread
> > mentioned the driver seems to make such an approach:
> >
> >   52 static int pcf2127_rtc_get(struct udevice *dev, struct rtc_time
> > *tm) 53 {
> >   54         int ret = 0;
> >   55         uchar buf[10] = { PCF2127_REG_CTRL1 };
> >   56
> >   57         ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 1);
> >   58         if (ret < 0)
> >   59                 return ret;
> >   60         ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf,
> > sizeof(buf)); 61         if (ret < 0)
> >   62                 return ret;
> >
> 
> I would prefer to fix the issue in the driver itself. Only when it is not possible
> we shall introduce extra flags and modify the common I2C code.
Because the pcf2127 chip is special, it must have a stop signal after writing 
the register to perform the following operations of reading the register.


	/***********************
	 *Generate the first part timings of reading from registers.
	 *                           write bit                       STOP
	 *                               |                          | 
	 *    ----------------------------------------------------------------------------------------------------------------
	 *    | S | 1  0  1  0  0  0  1  0 | A |      00h-1Dh      | A | P |
	 *    ----------------------------------------------------------------------------------------------------------------
	 *       | slave address = 0x51  |    |  | register address  |   | 
	 *                                |                      |
	 *                               ACK from slave    ACK from slave
	 **/
	ret = dm_i2c_write(dev, PCF2127_REG_CTRL1, buf, 0);
 	if (ret < 0)
 		return ret;

	/***********************
	 *Generate the second part timings of reading from registers.
	 *                             read bit                                                  STOP
	 *                                |                                                     |
	 *    ----------------------------------------------------------------------------------------------------------------------------------------------------------
	 *    | S |  1  0  1  0  0  0  1  1 | A |      data byte(0-n)      | A |     last data byte | NA | P |
	 *    ------------------------------------------------------------------------------------------------------------------------------------------------------------
	 *        |slave address = 0x51 |     | |     data from slave      |  |                      |
	 *                                 |                          |              		 |	
	 *                             ACK from slave                 ACK from master         No ACK
	 */
 	ret = dm_i2c_read(dev, PCF2127_REG_CTRL1, buf, sizeof(buf));
 	if (ret < 0)
 		return ret;> 
> >
> > It seems there are currently no real users of this driver:
> >
> > pollux:u-boot hs [master] $ grep -lr RTC_PCF2127 .
> > ./drivers/rtc/Kconfig
> > ./drivers/rtc/Makefile
> > pollux:u-boot hs [master] $
> >
> > I added Meng Yi to cc, as he is the author of this driver. May he can
> > say here more... at last I hope, the driver worked for him.
> >
> > bye,
> > Heiko
> 
> 
> 
> 
> Best regards,
> 
> Lukasz Majewski
> 
> --
> 
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email:
> lukma at denx.de