[U-Boot] [PATCH] Revert "fdt: Fix fdtdec_get_addr_size() for 64-bit"

[Simon Glass]

Hi,

On 14 August 2015 at 03:08, Bin Meng <bmeng.cn at gmail.com> wrote:
> Hi Michal,
>
> On Fri, Aug 14, 2015 at 5:01 PM, Michal Suchanek <hramrach at gmail.com> wrote:
>> On 14 August 2015 at 10:10, Bin Meng <bmeng.cn at gmail.com> wrote:
>>
>>> Do we have any conclusion about commit 5b34436? Today I started to
>>> check the pre-relocatoin DM PCI UART issue, but found it is now broken
>>> due to this commit. The broken part is at
>>> ns16550_serial_ofdata_to_platdata() in drivers/serial/ns16550.c, in
>>> which it has:
>>>
>>>     addr = fdtdec_get_addr(gd->fdt_blob, dev->of_offset, "reg");
>>> #ifdef CONFIG_PCI
>>>     if (addr == FDT_ADDR_T_NONE) {
>>> ...
>>>
>>> Before commit 5b34436, the old behavior is that the call to
>>> fdtdec_get_addr() returns FDT_ADDR_T_NONE so that we can trap into the
>>> PCI logic. But with commit 5b34436, addr is now zero which just bypass
>>> this logic.
>>>
>>> As for why addr is now zero, this is because fdtdec_get_number() can
>>> only handle a 64-bit number at most. However for PCI reg, it has 3
>>> cells. So if I have the following encoding:
>>>
>>> reg = <0x00025100 0x0 0x0 0x0 0x0
>>>            0x01025110 0x0 0x0 0x0 0x0>;
>>>
>>> The addr will be assigned to zero after two rounds of left shift by 32-bit.
>>>
>>> I can certainly change ns16550 driver to test the return value against
>>> 0 now, but I think this fdtdec_get_addr() does not cover all cases.
>>> Please advise.
>>>
>>
>> Hello,
>>
>> What do you expect the fdtdec_get_addr to do?
>>
>> Any 32bit (or 64bit number on 64bit archs) is valid return value so
>> there is no possibility to return an error. This is probably a problem
>> with the interface. If there is more than can fit or there is an error
>> you will never know.
>
> Agreed.
>
>>
>> eg. Linux has this code for decoding numbers which can have 1-2 cells:
>>
>>                 reg = of_get_property(pp, "reg", &len);
>>                 if (!reg) {
>> blah
>>                 }
>>
>>                 a_cells = of_n_addr_cells(pp); /* scan parents for
>> #address-cells */
>>                 s_cells = of_n_size_cells(pp);
>>                 (*pparts)[i].offset = of_read_number(reg, a_cells);
>>                 (*pparts)[i].size = of_read_number(reg + a_cells, s_cells);
>>
>>
>> If you read an address that can be 3 cells then you need a function
>> that returns cell array rather than a single integer. Or you can check
>> the address length and decide if you can fit that into an integer on
>> your platform.
>>
>
> Yep, we have an API fdtdec_get_pci_addr() to parse a PCI reg. I was
> raising this because I see this broke the existing codes and there was
> discussion on reverting this commit.

I plan to apply this revert to u-boot-x86 (where SPI is currently
broken) and (once it has a bit more testing) also this patch which I
think makes the change in a safer way:

https://patchwork.ozlabs.org/patch/504918/

At present that patch breaks at least one x86 board and I have not dug
into it yet.

The revert should not break tegra, according to Stephen.

Regards,
Simon