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

[Heiko Schocher]

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

> 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.
>>
>> 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?
>>
>> or reverse asked, why does this bit may not pop up for you Pali?
>>
>> I have not yet time to check this... also I am unsure if I have a hw
>> where I can try ...
>>
>> bye,
>> Heiko
> 
> Regards,
> Ivo
> 

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