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

Lukasz Majewski lukma at denx.de
Wed May 22 11:31:40 UTC 2019


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

> > -----Original Message-----
> > From: Lukasz Majewski <lukma at denx.de>
> > Sent: 2019年5月22日 16:41
> > To: Chuanhua Han <chuanhua.han at nxp.com>
> > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li <biwen.li at nxp.com>;
> > sjg at chromium.org; Stefano Babic <sbabic at denx.de>
> > Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read
> > wrong time
> > 
> > Hi Chuanhua,
> >   
> > > > -----Original Message-----
> > > > From: Lukasz Majewski <lukma at denx.de>
> > > > Sent: 2019年5月22日 15:16
> > > > To: Chuanhua Han <chuanhua.han at nxp.com>
> > > > Cc: hs at denx.de; u-boot at lists.denx.de; Biwen Li
> > > > <biwen.li at 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 at nxp.com>
> > > > > Signed-off-by: Chuanhua Han <chuanhua.han at 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>


More information about the U-Boot mailing list