> -----Original Message----- > From: Lukasz Majewski <lu...@denx.de> > Sent: 2019年5月23日 13:54 > To: Heiko Schocher <h...@denx.de> > Cc: Chuanhua Han <chuanhua....@nxp.com>; u-boot@lists.denx.de; Biwen Li > <biwen...@nxp.com>; s...@chromium.org; Stefano Babic <sba...@denx.de>; > Meng Yi <meng...@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 <lu...@denx.de> > > >> Sent: 2019年5月22日 19:32 > > >> 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 > > >> > > >> 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. > > > 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: > lu...@denx.de _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot