[PATCH v2] dm: Fix OF_BAD_ADDR definition

[Jan Kiszka]

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.chotard at 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 at 8000/noxlatebus at 3,300/dev at 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 at 40610000
>>>> MMC:   mmc at 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 at 285c0000: ret=0 tmp=00000000bdf10750

Jan

-- 
Siemens AG, Technology
Competence Center Embedded Linux