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

[Lukasz Majewski <lukma@denx.de>]

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>