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

Chuanhua Han chuanhua.han at nxp.com
Thu May 23 10:28:38 UTC 2019



> -----Original Message-----
> From: Lukasz Majewski <lukma at denx.de>
> Sent: 2019年5月23日 13:54
> To: Heiko Schocher <hs at denx.de>
> Cc: Chuanhua Han <chuanhua.han at nxp.com>; u-boot at lists.denx.de; Biwen Li
> <biwen.li at nxp.com>; sjg at chromium.org; Stefano Babic <sbabic at denx.de>;
> Meng Yi <meng.yi at 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 at denx.de>
> > >> Sent: 2019年5月22日 19:32
> > >> 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
> > >>
> > >> 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.
> > > 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


More information about the U-Boot mailing list