Hi Jan
On 2/15/22 21:36, Jan Kiszka wrote:
> On 15.02.22 14:49, Jan Kiszka wrote:
>> On 15.02.22 14:34, Patrice CHOTARD wrote:
>>> Hi Jan
>>>
>>> On 2/15/22 14:00, Jan Kiszka wrote:
>>>> On 15.02.22 12:56, Patrice CHOTARD wrote:
>>>>> Hi Jan
>>>>>
>>>>> On 2/14/22 16:21, Jan Kiszka wrote:
>>>>>> On 04.01.22 08:42, Patrice Chotard wrote:
>>>>>>> When OF_LIVE flag is enabled on a 64 bits platform, there is an
>>>>>>> issue when dev_read_addr() is called and need to perform an address
>>>>>>> translation using __of_translate_address().
>>>>>>>
>>>>>>> In case of error, __of_translate_address() return's value is OF_BAD_ADDR
>>>>>>> (wich is defined in include/dm/of.h to ((u64)-1) = 0xffffffffffffffff).
>>>>>>> The return value of dev_read_addr() is often compared to FDT_ADDR_T_NONE
>>>>>>> which is defined as (-1U) = 0xffffffff.
>>>>>>> In this case the comparison is always false.
>>>>>>>
>>>>>>> To fix this issue, define FDT_ADDR_T_NONE to (ulong)(-1) in case of
>>>>>>> AARCH64. Update accordingly related tests.
>>>>>>>
>>>>>>> Signed-off-by: Patrice Chotard <patrice.chot...@foss.st.com>
>>>>>>>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes in v2:
>>>>>>> - define FDT_ADDR_T_NONE as ((ulong)(-1)) and keep OF_BAD_ADDR
>>>>>>> unchanged
>>>>>>>
>>>>>>> include/fdtdec.h | 5 ++++-
>>>>>>> test/dm/ofnode.c | 2 +-
>>>>>>> test/dm/pci.c | 4 ++--
>>>>>>> test/dm/test-fdt.c | 2 +-
>>>>>>> 4 files changed, 8 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/fdtdec.h b/include/fdtdec.h
>>>>>>> index 6c7ab887b2..e9e2916d6e 100644
>>>>>>> --- a/include/fdtdec.h
>>>>>>> +++ b/include/fdtdec.h
>>>>>>> @@ -24,16 +24,19 @@
>>>>>>> typedef phys_addr_t fdt_addr_t;
>>>>>>> typedef phys_size_t fdt_size_t;
>>>>>>>
>>>>>>> -#define FDT_ADDR_T_NONE (-1U)
>>>>>>> #define FDT_SIZE_T_NONE (-1U)
>>>>>>>
>>>>>>> #ifdef CONFIG_PHYS_64BIT
>>>>>>> +#define FDT_ADDR_T_NONE ((ulong)(-1))
>>>>>>> +
>>>>>>> #define fdt_addr_to_cpu(reg) be64_to_cpu(reg)
>>>>>>> #define fdt_size_to_cpu(reg) be64_to_cpu(reg)
>>>>>>> #define cpu_to_fdt_addr(reg) cpu_to_be64(reg)
>>>>>>> #define cpu_to_fdt_size(reg) cpu_to_be64(reg)
>>>>>>> typedef fdt64_t fdt_val_t;
>>>>>>> #else
>>>>>>> +#define FDT_ADDR_T_NONE (-1U)
>>>>>>> +
>>>>>>> #define fdt_addr_to_cpu(reg) be32_to_cpu(reg)
>>>>>>> #define fdt_size_to_cpu(reg) be32_to_cpu(reg)
>>>>>>> #define cpu_to_fdt_addr(reg) cpu_to_be32(reg)
>>>>>>> diff --git a/test/dm/ofnode.c b/test/dm/ofnode.c
>>>>>>> index cea0746bb3..e6c925eba6 100644
>>>>>>> --- a/test/dm/ofnode.c
>>>>>>> +++ b/test/dm/ofnode.c
>>>>>>> @@ -286,7 +286,7 @@ static int dm_test_ofnode_get_reg(struct
>>>>>>> unit_test_state *uts)
>>>>>>> ut_assert(ofnode_valid(node));
>>>>>>> addr = ofnode_get_addr(node);
>>>>>>> size = ofnode_get_size(node);
>>>>>>> - ut_asserteq(FDT_ADDR_T_NONE, addr);
>>>>>>> + ut_asserteq_64(FDT_ADDR_T_NONE, addr);
>>>>>>> ut_asserteq(FDT_SIZE_T_NONE, size);
>>>>>>>
>>>>>>> node =
>>>>>>> ofnode_path("/translation-test@8000/noxlatebus@3,300/dev@42");
>>>>>>> diff --git a/test/dm/pci.c b/test/dm/pci.c
>>>>>>> index fa2e4a8559..00e4440a9d 100644
>>>>>>> --- a/test/dm/pci.c
>>>>>>> +++ b/test/dm/pci.c
>>>>>>> @@ -331,10 +331,10 @@ static int dm_test_pci_addr_live(struct
>>>>>>> unit_test_state *uts)
>>>>>>> struct udevice *swap1f, *swap1;
>>>>>>>
>>>>>>> ut_assertok(dm_pci_bus_find_bdf(PCI_BDF(0, 0x1f, 0), &swap1f));
>>>>>>> - ut_asserteq(FDT_ADDR_T_NONE, dev_read_addr_pci(swap1f));
>>>>>>> + ut_asserteq_64(FDT_ADDR_T_NONE, dev_read_addr_pci(swap1f));
>>>>>>>
>>>>>>> ut_assertok(dm_pci_bus_find_bdf(PCI_BDF(0, 0x1, 0), &swap1));
>>>>>>> - ut_asserteq(FDT_ADDR_T_NONE, dev_read_addr_pci(swap1));
>>>>>>> + ut_asserteq_64(FDT_ADDR_T_NONE, dev_read_addr_pci(swap1));
>>>>>>>
>>>>>>> return 0;
>>>>>>> }
>>>>>>> diff --git a/test/dm/test-fdt.c b/test/dm/test-fdt.c
>>>>>>> index 8866d4d959..e1de066226 100644
>>>>>>> --- a/test/dm/test-fdt.c
>>>>>>> +++ b/test/dm/test-fdt.c
>>>>>>> @@ -768,7 +768,7 @@ static int dm_test_fdt_livetree_writing(struct
>>>>>>> unit_test_state *uts)
>>>>>>> /* Test setting generic properties */
>>>>>>>
>>>>>>> /* Non-existent in DTB */
>>>>>>> - ut_asserteq(FDT_ADDR_T_NONE, dev_read_addr(dev));
>>>>>>> + ut_asserteq_64(FDT_ADDR_T_NONE, dev_read_addr(dev));
>>>>>>> /* reg = 0x42, size = 0x100 */
>>>>>>> ut_assertok(ofnode_write_prop(node, "reg", 8,
>>>>>>>
>>>>>>> "\x00\x00\x00\x42\x00\x00\x01\x00"));
>>>>>>
>>>>>> Since this commit, I'm getting this dev_get_priv warning:
>>>>>>
>>>>>> [...]
>>>>>> U-Boot 2022.01-00766-g9876ae7db6d-dirty (Feb 14 2022 - 16:15:21 +0100)
>>>>>>
>>>>>> Model: SIMATIC IOT2050 Basic
>>>>>> DRAM: 1 GiB
>>>>>> Core: 94 devices, 28 uclasses, devicetree: separate
>>>>>> WDT: Not starting watchdog@40610000
>>>>>> MMC: mmc@4fa0000: 0
>>>>>> Loading Environment from SPIFlash... SF: Detected w25q128 with page size
>>>>>> 256 Bytes, erase size 64 KiB, total 16 MiB
>>>>>> drivers/dma/ti/k3-udma.c:1744 __dev_get_priv: null device
>>>>>> drivers/dma/ti/k3-udma.c:1744 __dev_get_priv: null device
>>>>>> OK
>>>>>> In: serial
>>>>>> Out: serial
>>>>>> Err: serial
>>>>>> Net: No ethernet found.
>>>>>> Hit any key to stop autoboot: 0
>>>>>>
>>>>>> (I've instrumented dev_get_priv to tell me file:line)
>>>>>>
>>>>>> Is that a sleeping problem surfaced by the commit or a real regression?
>>>>>> I can boot, though.
>>>>>>
>>>>>> Jan
>>>>>>
>>>>>
>>>>> It should be interesting to understand why uclass_get_device_by_phandle()
>>>>> return tmp = NULL.
>>>>
>>>> Yep.
>>>>
>>>>> What's the return value of uclass_get_device_by_phandle() ?
>>>>>
>>>>
>>>> -22, EINVAL.
>>>
>>> As EINVAL is one of the more "common" error value, i think you have to go
>>> deeper
>>> to see where the uclass_get_device_by_phandle() is failing.
>>>
>>
>> Sigh, I was hoping to get around debugging this myself.
>>
>> In any case: With this patch revert, the related code path is still
>> taken, just successfully:
>>
>> ti-udma dma-controller@285c0000: ret=0 tmp=00000000bdf10750
>>
>
> To conclude this thread: The patch does what it is supposed to do, i.e.
> define the right FDT_ADDR_T_NONE so that comparisons finally work
> correctly on 64-bit archs.
>
> As they work correctly now, in this case in dev_remap_addr_name, they
> reveal that k3_nav_ringacc_init() tries to get a non-existent
> configuration register "cfg". So far it got -1LL as result, != NULL, and
> likely used that happily. The missing register came from a missing
> u-boot specific fragment in our board dts, compare to the TI reference
> board. Working on a fix.
>
> Jan
>
Thanks for the feedback ;-)
Patrice
