U-Boot i2c bus num 1 is broken on Nokia N900

[Heiko Schocher]

Hello Ivaylo,

Am 30.10.2020 um 08:00 schrieb Ivaylo Dimitrov:
> Hi,
> 
> On 29.10.20 г. 11:32 ч., Heiko Schocher wrote:
>> Hello Ivaylo,
>>
>> Am 29.10.2020 um 08:51 schrieb Ivaylo Dimitrov:
>>> Hi,
>>>
>>> On 28.10.20 г. 7:42 ч., Heiko Schocher wrote:
>>>> Hello Pali,
>>>>
>>>> sorry for late response ...
>>>>
>>>> Am 26.10.2020 um 22:48 schrieb Pali Rohár:
>>>>> On Monday 27 April 2020 09:03:13 Heiko Schocher wrote:
>>>>>> Hello Pali,
>>>>>>
>>>>>> Am 26.04.2020 um 01:54 schrieb Pali Rohár:
>>>>>>> Adding Hannes and Heiko to the loop, please look at this problem.
>>>>>>>
>>>>>>> On Saturday 25 April 2020 14:11:32 Pali Rohár wrote:
>>>>>>>> On Saturday 25 April 2020 07:00:58 Adam Ford wrote:
>>>>>>>>> On Sat, Apr 25, 2020 at 6:50 AM Pali Rohár <pali at kernel.org> wrote:
>>>>>>>>>>
>>>>>>>>>> On Saturday 25 April 2020 06:36:58 Adam Ford wrote:
>>>>>>>>>>> On Sat, Apr 25, 2020 at 5:42 AM Pali Rohár <pali at kernel.org> wrote:
>>>>>>>>>>>>
>>>>>>>>>>>> On Thursday 02 April 2020 20:42:31 Pali Rohár wrote:
>>>>>>>>>>>>> On Wednesday 01 April 2020 12:32:29 Merlijn Wajer wrote:
>>>>>>>>>>>>>> Hi,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 01/04/2020 00:42, Pali Rohár wrote:
>>>>>>>>>>>>>>> On Wednesday 01 April 2020 00:35:07 Pali Rohár wrote:
>>>>>>>>>>>>>>>> This patch series contain fixes for Nokia RX-51 board (aka N900).
>>>>>>>>>>>>>>>> After these changes it is possible to run U-Boot in qemu emulator again.
>>>>>>>>>>>>>>>> And U-Boot can boot kernel image from RAM, eMMC or OneNAND memory without
>>>>>>>>>>>>>>>> problem.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> But on real Nokia N900 device is U-Boot crashing in reboot loop.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> I do not have serial console for Nokia N900 to debug this issue, but
>>>>>>>>>>>>>>> seems that it is related to OMAP I2C and OMAP HS MMC code. Problem is
>>>>>>>>>>>>>>> that there is no crash and even no error in qemu emulator so I cannot
>>>>>>>>>>>>>>> debug this issue.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> First problem is around /* reset lp5523 led */ code in rx51.c. On real
>>>>>>>>>>>>>>> N900 device it generates repeating messages:
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>     Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>>>     Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> When I commented that few lines all these messages disappeared. So
>>>>>>>>>>>>>>> problem is with OMAP I2C.
>>>>>>> ...
>>>>>>>>>>>>>>> I remember that somebody had serial jig for Nokia N900, could somebody
>>>>>>>>>>>>>>> look at this reboot loop problem?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> And any idea how should be OMAP I2C configured in U-Boot to correctly
>>>>>>>>>>>>>>> work?
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Maybe I will try to find some time to git bisect which change broke
>>>>>>>>>>>>>>> U-Boot on real N900 hardware.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Took latest u-boot master, applied patches and this is the result on
>>>>>>>>>>>>>> serial (first part is NOLO booting, I think that can be ignored) [1].
>>>>>>>>>>>>>
>>>>>>>>>>>>> ...
>>>>>>>>>>>>>
>>>>>>>>>>>>>> U-Boot 2020.04-rc4-00033-g7dbafe0634-dirty (Apr 01 2020 - 12:15:47 +0200)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> OMAP3530-HS ES3.1, CPU-OPP2, L3-165MHz, Max CPU Clock 600 MHz
>>>>>>>>>>>>>> Nokia RX-51 + LPDDR/OneNAND
>>>>>>>>>>>>>> I2C:   ready
>>>>>>>>>>>>>> DRAM:  256 MiB
>>>>>>>>>>>>>> NAND:  0 Bytes
>>>>>>>>>>>>>
>>>>>>>>>>>>> Looks like that something with NAND is broken.
>>>>>>
>>>>>> The board code in U-Boot is in a very old state... :-(
>>>>
>>>> :-(
>>>>
>>>>>>>>>>>>>> MMC:   OMAP SD/MMC: 0, OMAP SD/MMC: 1
>>>>>>>>>>>>>> In:    vga
>>>>>>>>>>>>>> Out:   vga
>>>>>>>>>>>>>> Err:   vga
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0100
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> Timed out in wait_for_event: status=0000
>>>>>>>>>>>>>> Check if pads/pull-ups of bus are properly configured
>>>>>>>>>>>>>> i2c_read (addr phase): pads on bus probably not configured (status=0x10)
> 
> How is that trace even possible? I built and tested u-boot here on my devices and I see the same 
> message, but unless I am becoming blind, i2c_read() is never called from within i2c_write(). This is 
> really suspicious.

Puh..

> 
>>>>>>>>>>>>>> i2c_write: timed out writig last byte!
>>>>>>>>>>>>>
>>>>>>>>>>>>> These i2c errors are caused by
>>>>>>>>>>>>>
>>>>>>>>>>>>>         /* reset lp5523 led */
>>>>>>>>>>>>>         i2c_set_bus_num(1);
>>>>>>
>>>>>> deprecated ... the board code needs cleanup ...
>>>>
>>>> Yes, my first thought too!
>>>>
>>>> This board needs a complete cleanup.
>>>>
>>>>> I converted code to CONFIG_DM_I2C and nothing was changed, issue is
>>>>> still there...
>>>>
>>>> Ok.
>>>>
>>>>>>>>>>>>>         state = 0xff;
>>>>>>>>>>>>>         i2c_write(0x32, 0x3d, 1, &state, 1);
>>>>>>>>>>>>>         i2c_set_bus_num(0);
>>>>>>>>>>>>>
>>>>>>>>>>>>> Is there anything which needs to be done to initialize i2c bus 1?
>>>>>>>>>>>>> Because this code is working fine on older U-Boot version.
>>>>>>>>>>>>
>>>>>>>>>>>> Above code worked fine for U-Boot 2013.04, but in git version from
>>>>>>>>>>>> January 2015 it prints above error messages.
>>>>>>>>>>>>
>>>>>>>>>>>> On on internet forums I see these error messages also from other OMAP3
>>>>>>>>>>>> board, e.g. beagle board.
>>>>>>>>>>>>
>>>>>>>>>>>> Has somebody some working OMAP3 board? And can test if it works with
>>>>>>>>>>>> recent version of U-Boot? I guess that above i2c problem would happen
>>>>>>>>>>>> also on other OMAP3 boards.
>>>>>>>>>>>
>>>>>>>>>>> There was a conversion a while ago to dm_i2c, and I converted my board
>>>>>>>>>>> to support using the device tree during the SPL phase, and whenever I
>>>>>>>>>>> am aware any driver has driver model (DM) support, I try to convert my
>>>>>>>>>>> board.
>>>>>>>>>>>
>>>>>>>>>>> I have a DM3730 and the last check I did was 2020.04-rc1, and it was working
>>>>>>>>>>
>>>>>>>>>> Ok, so it either OMAP3430 specific problem or N900 board specific
>>>>>>>>>> problem. N900 does not use driver model.
>>>>>>>>>
>>>>>>>>> i have an OMAP3530 which is basically a 3430, and it works too. I am
>>>>>>>>> guessing the issue is unique to the N900 or the fact that it's
>>>>>>>>> high-security.  Neither of my boards are HS parts.  They are both GP.
>>>>>>>>
>>>>>>>> N900 is HS device, but I guess that should be caused by GP vs HS
>>>>>>>> difference. Working i2c bus 0 and non-working i2c bus 1 could not be
>>>>>>>> caused by GP vs HS difference. Also I guess that omap hs mmc would be
>>>>>>>> same on GP and HS boards.
>>>>>>> ...
>>>>>>>>>> Before calling i2c_write(0x32, ...) I tried to call i2c_probe(0x32) and
>>>>>>>>>> it returned error.
>>>>>>>>>>
>>>>>>>>>> If I tried to call "i2c dev 1" in U-Boot console, I got tons of errors
>>>>>>>>>> and basically U-Boot stopped responding.
>>>>>>>>>>
>>>>>>>>>> So by above observation it looks like I2C bus num 1 does not work, but
>>>>>>>>>> I2C bus num 0 works fine.
>>>>>>>>>>
>>>>>>>>>> Do I need to call i2c_probe(...) before calling i2c_write(...)?
>>>>>>>>>>
>>>>>>>>>> And is something special needed for initializing omap i2c bus num 1?
>>>>>>>>>> Because from my above observation it looks like that something is
>>>>>>>>>> missing for bus 1 which in older u-boot version was not needed.
>>>>>>>
>>>>>>> Now I was able to find commit which is causing above i2c problems:
>>>>>>> "Check if pads/pull-ups of bus are properly configured"
>>>>>>>
>>>>>>> It is d5243359e1afc957acd373dbbde1cf6c70ee5485:
>>>>>>>
>>>>>>>       OMAP24xx I2C: Add support for set-speed
>>>>>>>       Adds support for set-speed on the OMAP24xx I2C Adapter.
>>>>>>>       Changes to omap24_i2c_write(...) for polling ARDY Bit from IRQ-Status.
>>>>>>>       Otherwise on a subsequent call the transfer of last byte from the
>>>>>>>       predecessor is aborted and therefore lost. For exmaple when
>>>>>>>       i2c_write(...) is followed by a i2c_setspeed(...) (which has to
>>>>>>>       deactivate and activate master for changing psc,...).
>>>>>>>       Minor cosmetical changes.
>>>>>>>       Signed-off-by: Hannes Petermaier <oe5hpm at oevsv.at>
>>>>>>>       Cc: Heiko Schocher <hs at denx.de>
>>>>>>>
>>>>>>> U-Boot version prior this command does not report those i2c errors.
>>>>>>>
>>>>>>> Hannes, any idea how your patch could broke omap i2c i2c bus num 1 on
>>>>>>> Nokia N900?
>>>>>>
>>>>>> Hard to say here anything useful, as I do not have the hardware...
>>>>>>
>>>>>> The above commit changes:
>>>>>>
>>>>>> -               udelay(I2C_WAIT);
>>>>>> +               udelay(adap->waitdelay);
>>>>>>
>>>>>> May you can check, if adap->waitdelay is the same as I2C_WAIT ?
>>>>>
>>>>> Yes, it is the same value.
>>>>
>>>> Ok, fine.
>>>>
>>>>> Anyway, I have deeply looked at that commit again and it just adds
>>>>> support for omap24_i2c_setspeed and into omap24_i2c_write adds following
>>>>> change:
>>>>>
>>>>> @@ -464,6 +502,15 @@ static int omap24_i2c_write(struct i2c_adapter *adap, uchar chip, uint addr,
>>>>>               goto wr_exit;
>>>>>           }
>>>>>       }
>>>>> +    /*
>>>>> +     * poll ARDY bit for making sure that last byte really has been
>>>>> +     * transferred on the bus.
>>>>> +     */
>>>>> +    do {
>>>>> +        status = wait_for_event(adap);
>>>>> +    } while (!(status & I2C_STAT_ARDY) && timeout--);
>>>>> +    if (timeout <= 0)
>>>>> +        printf("i2c_write: timed out writig last byte!\n");
>>>>>   wr_exit:
>>>>>       flush_fifo(adap);
>>>>>
>>>>> And this change is causing that non-functional i2c bus.
>>>>
>>>> Ok, this part definetely changes behaviour in timing...
>>>>
>>>>> I applied revert of above change on top of the master u-boot branch and
>>>>> i2c bus num 1 (second) started working on N900 hw:
>>>>>
>>>>> diff --git a/drivers/i2c/omap24xx_i2c.c b/drivers/i2c/omap24xx_i2c.c
>>>>> index 0af4e333c4..a49cf89712 100644
>>>>> --- a/drivers/i2c/omap24xx_i2c.c
>>>>> +++ b/drivers/i2c/omap24xx_i2c.c
>>>>> @@ -820,16 +820,6 @@ static int __omap24_i2c_write(void __iomem *i2c_base, int ip_rev, int 
>>>>> waitdelay,
>>>>>           }
>>>>>       }
>>>>> -    /*
>>>>> -     * poll ARDY bit for making sure that last byte really has been
>>>>> -     * transferred on the bus.
>>>>> -     */
>>>>> -    do {
>>>>> -        status = wait_for_event(i2c_base, ip_rev, waitdelay);
>>>>> -    } while (!(status & I2C_STAT_ARDY) && timeout--);
>>>>> -    if (timeout <= 0)
>>>>> -        printf("i2c_write: timed out writig last byte!\n");
>>>>> -
>>>>>   wr_exit:
>>>>>       flush_fifo(i2c_base, ip_rev);
>>>>>       omap_i2c_write_reg(i2c_base, ip_rev, 0xFFFF, OMAP_I2C_STAT_REG);
>>>>>
>>>>> I have looked into i2c-omap.c linux kernel driver and its transfer
>>>>> function does not have any such code for waiting ARDY bit.
>>>>
>>>
>>> Correct, but this waiting seems to be described in the TRM (see Figure 18-46 and Figure 18-47), 
>>> albeit for the polling mode. Though, in general flow diagrams (Figure 18-29 and Figure 18-31), 
>>> there is no such loop.
>>
>> Could you provide a link to the TRM?
>>
> 
> https://www.ti.com/pdfs/wtbu/OMAP34xx_ES3.1x_PUBLIC_TRM_vZN.pdf

thanks! Seems I was blind ...

>>> However, by looking to __omap24_i2c_init(), it is not clear to me what mode uboot uses as it 
>>> enables almost all interrupt bits in OMAP_I2C_IE_REG and loops waiting for events at the same 
>>> time. Could someone confirm if uboot uses interrupt or polling mode? As this changes the things 
>>> in regards to ARDY bit a lot, IIUC.
>>
>> I think, u-boot only polls the registers, while enabling all
>> interrupt bits ...
>>
>>>> Ok...
>>>>
>>>>> Why it is there? I have not able to find any information and that
>>>>> comment is strange... it looks like it was incomplete/broken? workaround
>>>>> about other issue.
>>>>
>>>> Hmm.. ARDY bit means:
>>>> """
>>>> The current transaction is finished and the module registers
>>>> can be accessed.
>>>> """
>>>>
>>>
>>> But it seems there is something weird about ARDY bit, at least in interrupt mode, see linux 
>>> kernel commits cb527ede1b and 4cdbf7d346. So it seems ARDY bit shall be cleared twice.
>>
>> Hmm.. yes.
>>
>>>> Seems not to bad to me to check this bit! ... but may missing
>>>> on transaction start some prerequisite is missing for triggering
>>>> this bit? And so, this additional check only leads in a loop
>>>> going into timeout?
>>>>
>>>>> As you can see in log, at the first call status flags contains value
>>>>> 0x0100 and on all other calls it contains just 0x000 status flags.
>>>>>
>>>>> Therefore ARDY bit is never set, but i2c transfer works fine.
>>>>
>>>
>>> What looks wrong to me is that __omap24_i2c_init() enables all interrupts in OMAP_I2C_IE_REG, 
>>> however, the precondition for polling mode according to Figure 18-29 is that all interrupts shall 
>>> be disabled.
>>>
>>>> Hmm... so the question why does this bit not pop up on transfer
>>>> end?
>>>>
>>>
>>> It seems it never pops out. Moreover, if we look at the logs, the first wait_for_event() seems to 
>>
>> yes.
>>
>>> timeout with status=0100, that is with BF bit set. What looks suspicions here, is that the only 
>>> bit we ever see set, is the bit we don't have interrupt bit enabled for.
>>>
>>> Pali, maybe you should try to comment the code that sets interrupt bits in __omap24_i2c_init() 
>>> (the block that starts with "if (ip_rev == OMAP_I2C_REV_V1)") and see if it makes any difference. 
>>> Also, maybe add more traces in __omap24_i2c_write() to see which exactly wait_for_event() call 
>>> times out.
>>
>> May worth a try.
>>
>> More mystically is, that it works fine for Pali on bus 0 but
>> makes problems on bus 1 ... !?
>>
>> Pali: Do you have also on bus 0 i2c writes?
>>
>>>> I just can speculate that adding this polling ARDY bit loop
>>>> changes the timing... and fixed an underlying bug, yes...
>>>>
>>>> but if this bit never pop up, there must come the printf
>>>> "i2c_write: timed out writig last byte!" for each i2c transfer.
>>>>
> 
> And this is what happens, we have that once, as we write only one byte to bus 1.
> 
>>>> Hannes may you can check if this is the case for you?
>>>>
>>>> why does nobody claimed that this message pops up in the last 5 years?
>>>>
> 
> I can confirm I see it on the 2 devices I tested here.
> 
> What is worse, is that writing on bus 1 does not fail every time. I increased I2C_TIMEOUT to 100000 
> (the value from the TRM) and it seems now after power-cycle, write succeeds almost every time, 
> however, after a reset command from u-boot, it usually fails. And with that increased timeout,when 
> it fails I see:
> 
> Check if pads/pull-ups of bus are properly configured
> i2c_read (addr phase): pads on bus probably not configured (status=0x10)
> 
> message 5 times during the failing write.
> 
> How we end up there, is a mystery to me.

Yes...

Ok, on page 2671 is described when this ARDY event is triggered, so
if we wait for it, we must first check, if the prerequisite is met...

Hmm.. the ARDY bit is also for interrupt mode described, see page 2778
Figure 18-31 ... so I think it is correct to check it ... but
I do not see, why we should wait for it in a loop and print
an error if bit does not come up

But I have no time to dig into this ...

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