On Wed, 22 May 2019 09:31:35 +0000 Chuanhua Han <chuanhua....@nxp.com> wrote:
> > -----Original Message----- > > From: Lukasz Majewski <lu...@denx.de> > > Sent: 2019年5月22日 16:41 > > To: Chuanhua Han <chuanhua....@nxp.com> > > Cc: h...@denx.de; u-boot@lists.denx.de; Biwen Li <biwen...@nxp.com>; > > s...@chromium.org; Stefano Babic <sba...@denx.de> > > Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read > > wrong time > > > > Hi Chuanhua, > > > > > > -----Original Message----- > > > > From: Lukasz Majewski <lu...@denx.de> > > > > Sent: 2019年5月22日 15:16 > > > > To: Chuanhua Han <chuanhua....@nxp.com> > > > > Cc: h...@denx.de; u-boot@lists.denx.de; Biwen Li > > > > <biwen...@nxp.com>; s...@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...@nxp.com> > > > > > Signed-off-by: Chuanhua Han <chuanhua....@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: lu...@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: > > lu...@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: lu...@denx.de
pgp7tHiZRRDt5.pgp
Description: OpenPGP digital signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot