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

Thu May 23 10:14:24 UTC 2019


> -----Original Message-----
> From: Heiko Schocher <hs at denx.de>
> Sent: 2019年5月23日 11:29
> To: Chuanhua Han <chuanhua.han at nxp.com>
> Cc: Lukasz Majewski <lukma 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>;
> Meng Yi <meng.yi at nxp.com>
> Subject: Re: [EXT] Re: [PATCH] i2c: pcf2127: fix bug that read wrong time
> 
> Caution: EXT Email
> 
> 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.
Ok, Next I will split the patch and provide a more detailed description messages.
> 
> >>
> >>>>
> >>>>>> 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;
> 
> 
Although this can be done logically, the dm_i2c_read function is designed to put the address of the write register 
and the read data in a call to the ops->xfer(bus, msg, msg_count) function, so that we can't get us. Expected effect 
(there is no stop signal after the address of the write register but the restart signal)
> 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.
True, currently only the NXP layerscape platform board users the pcf2127 chip, but Meng Yi has now left the company.
 Now I will fix the patch and send the next version.
> 
> bye,
> Heiko
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de