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

[Heiko Schocher]

Hello Lukasz,

Am 23.05.2019 um 07:54 schrieb Lukasz Majewski:
> 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.

Exactly.

>>>>>>>> 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.

If possible, yes! I wonder: if this driver is in mainline, it must
at least worked for Meng Yi (as driver author) ... unfortunately
the email address is not valid ... so removed him from cc list.

bye,
Heiko
> 
>>
>> 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
> 

-- 
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