[U-Boot] [PATCH 031/080] serial: ns16550: Fix address translation

[Mario Six]

Hi,

On Mon, Oct 9, 2017 at 4:09 PM, Simon Glass <sjg at chromium.org> wrote:
> Hi,
>
> On 9 October 2017 at 06:55, Dr. Philipp Tomsich
> <philipp.tomsich at theobroma-systems.com> wrote:
>>
>>> On 9 Oct 2017, at 14:45, Mario Six <mario.six at gdsys.cc> wrote:
>>>
>>> (adding Philipp since he converted lots of drivers to livetree recently)
>>>
>>> On Mon, Oct 9, 2017 at 6:48 AM, Simon Glass <sjg at chromium.org> wrote:
>>>> Hi Mario,
>>>>
>>>> On 29 September 2017 at 06:51, Mario Six <mario.six at gdsys.cc> wrote:
>>>>> The dev_read_addr function does not do any bus translations, and just
>>>>> returns the raw address read from the device tree, which makes the
>>>>> driver not work on systems that need bus translations to get the actual
>>>>> memory address of the device's register space.
>>>>
>>>> Aside from any current functionality, what is the correct thing for
>>>> dev_read_addr() to do? I worry that the two parts (live/flat tree)
>>>> might do different things.
>>>>
>>>
>>> I took a closer look, and indeed, the two parts of dev_read_addr behave
>>> differently: dev_read_addr calls dev_read_addr_index, which calls either
>>> ofnode_get_addr_index when live tree is active, or devfdt_get_addr_index when
>>> it's not. devfdt_get_addr_index applies bus translations, but
>>> ofnode_get_addr_index returns the untranslated address using of_read_number
>>> (the else part doesn't run, since we have an active live tree if
>>> ofnode_get_addr_index was called).
>>
>> Good point. I would expect the livetree case to use translated addresses
>> during runtime, as dev_read_addr_index calls either ofnode_get_addr_index
>> or devfdt_get_addr_index.
>>
>> In other words: are we missing an address translation from ofnode_get_addr_index
>> or should the address retrieved via ofnode_get_addr_index have been
>> translated by earlier processing?
>
> Thank you both. Yes it seems like the right answer is to add the
> missing translation in. There is a CONFIG_OF_TRANSLATE which enables
> this. I think most boards don't use it, which is probably why there
> are no problems without it, but I think it is becoming more common as
> boards become more complex.
>

OK, I'll send a separate patch this week. Thanks for taking a look!

>>
>>>
>>> We could fix this by running of_translate_address on the value returned by
>>> of_read_number, so that dev_get_addr would then always return a translated
>>> address.
>>>
>>> But what I think is strange is that most live tree conversion patches (e.g.
>>> a9d3037 ("usb: dwc2: convert to livetree") or 4aac33f ("dm: mmc: fsl_esdhc:
>>> Update to support livetree")) simply replaced devfdt_get_addr (which does apply
>>> bus translations) with dev_read_addr (which does not apply bus translations in
>>> the live tree case). Shouldn't the converted driver have failed in the live
>>> tree case? Or were all drivers converted until now not depending on any bus
>>> translations?
>>>
>>>> In any case, we should not compound the problem if dev_read_addr() is wrong.
>>>>
>>>>>
>>>>> Since the dev_read_addr function is widely used, we refrain from
>>>>> modifying it, and instead read the raw address from the device tree, and
>>>>> apply the bus translations using the recently introduced
>>>>> dev_translate_address function.
>>>>>
>>>>> Signed-off-by: Mario Six <mario.six at gdsys.cc>
>>>>> ---
>>>>> drivers/serial/ns16550.c | 5 +++--
>>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> REgards,
>>>> Simon
>>>
>>> Best regards,
>>>
>>> Mario
>>
>
> Regards,
> Simon
>
Best regards,

Mario