[U-Boot] [PATCH] fdt: Allow fdt_translate_address() to work with buses

[Stephen Warren]

On 01/07/2016 04:43 AM, Przemyslaw Marczak wrote:
> Hello,
>
> On 01/05/2016 06:26 PM, Stephen Warren wrote:
>> On 01/05/2016 08:47 AM, Przemyslaw Marczak wrote:
>>> Hello,
>>>
>>> On 01/05/2016 02:00 AM, Simon Glass wrote:
>>>> Hi Stephen,
>>>>
>>>> On 4 January 2016 at 13:15, Stephen Warren <swarren at wwwdotorg.org>
>>>> wrote:
>>>>> On 01/03/2016 04:04 PM, Simon Glass wrote:
>>>>>>
>>>>>> It is common for I2C and SPI buses to have a single-cell address
>>>>>> and a
>>>>>> size
>>>>>> of 0. These produce a warning at present. For example on snow:
>>>>>>
>>>>>> __of_translate_address: Bad cell count for gpc4
>>>>>> __of_translate_address: Bad cell count for gpx0
>>>>>> __of_translate_address: Bad cell count for gpv2
>>>>>> __of_translate_address: Bad cell count for gpv4
>>>>>>
>>>>>> One of the nodes in question looks like this in part:
>>>>>>
>>>>>>          pinctrl_2: pinctrl at 10d10000 {
>>>>>>                  #address-cells = <1>;
>>>>>>                  #size-cells = <0>;
>>>>>>                  gpv2: gpv2 {
>>>>>>                          reg = <0x060>;
>>>>>>                  };
>>>>>>                  gpv4: gpv4 {
>>>>>>                          reg = <0xc0>;
>>>>>>                  };
>>>>>>          };
>>>>>>
>>>>>> This is clearly valid so it looks like the conversion to use
>>>>>> fdt_translate_address() in dev_get_addr() is not currently a good
>>>>>> move.
>>>>>
>>>>>
>>>>> To disable that, why not simply turn off CONFIG_OF_TRANSLATE on the
>>>>> affected
>>>>> platforms? That's precisely why that config option was introduced
>>>>> when the
>>>>> call to fdt_translate_address() was added to dev_get_addr()?
>>>>>
>>>>> That would prevent this patch from affecting platforms that don't
>>>>> trigger
>>>>> this issue, this leaving the valid check in place.
>>>>
>>>> But since this breaks normal behaviour we don't know what platforms
>>>> are affected. We have made CONFIG_OF_TRANSLATE the default. So this
>>>> approach doesn't seem (in effect) any better than Przemyslaw's newer
>>>> series, below.
>>>>
>>>>>
>>>>>> Przemyslaw Marczak sent three patches to resolve this for exynos
>>>>>> boards:
>>>>>>
>>>>>> https://patchwork.ozlabs.org/patch/557008/
>>>>>> https://patchwork.ozlabs.org/patch/557010/
>>>>>> https://patchwork.ozlabs.org/patch/557009/
>>>>>>
>>>>>> But this involves creating a new function, and everyone will need to
>>>>>> know
>>>>>> when to use which one. Also the problem may affect other boards.
>>>>>
>>>>>
>>>>> I suggest adding an extra parameter to dev_get_addr() (or whatever
>>>>> calls it)
>>>>> that indicates the root of the address space. The check on #size-cells
>>>>> should be skipped for that one node (or level of translation) but
>>>>> enabled
>>>>> for all other levels. This way, there would be no need for anyone to
>>>>> choose
>>>>> between functions; there'd only be one. Most cases (i.e. translation
>>>>> of MMIO
>>>>> addresses) would simply pass 0 as the extra parameter (for the root
>>>>> node),
>>>>> but in special cases where it's known translation is not expected to
>>>>> reach
>>>>> the root MMIO space (e.g. I2C, SPI controllers), the controller node
>>>>> would
>>>>> be passed in.
>>>>
>>>> How would the caller know this root? It sounds plausible, but I do
>>>> want to avoid complex rules. I think you are saying that buses that
>>>> use their own address mechanism (i.e. not MMIO) must do something
>>>> special. The current dev_get_addr() is really simple.
>>>
>>> Simon, Stephen
>>>
>>> As a summary of the issue, please tell me your opinion about this:
>>>
>>> Since the __of_translate_address() is called always with the "ranges" as
>>> an argument, it looks reasonable to check it at the function beginning,
>>> that the "ranges" property exists in the parent node.
>>>
>>> If not exists - then don't check the size-cells count and this should
>>> fix the problem with additional argument.
>>> This is simple and correct from specification point of view - which says
>>> ranges and #size-cells property's - are not required [ePAPR v1.1].
>>
>> It /might/ indeed be reasonable to skip the check on #size-cells if
>> there is no ranges property; a missing ranges property is already
>> treated as a 1:1 translation as described by the comment at the top of
>> of_translate_one(). Still, there are likely some edge-cases w.r.t. the
>> "length" fields in the ranges property that would need to be thought
>> through in more detail before I'd say for certain that this change does
>> absolutely make sense.
>>
>> Either way though, making that change wouldn't solve the problem at all.
>>
>> It's simply not possible to translate an I2C device address beyond the
>> root of the I2C address space; there is no equivalent of an I2C device's
>> address in MMIO address space. The root of the problem is that the code
>> is attempting to perform an I2C->MMIO address translation. If we prevent
>> that (by capping any such translation at the root of the I2C address
>> space for example), the problem is solved at that point. Once that's
>> solved, the issue of translating across a boundary with #size-cells=0
>> will never appear, and hence does not need to be solved.
>>
>>
>
> The problem with I2C and I2C->MMIO address translation is solved by my
> new patch. The specification is clear for such case, if reg can't be
> mapped to it's parent address space, then don't use 'ranges' and this
> can be easy solved by additional check.

Logic dictates that such a case (I2C->MMIO) doesn't make sense, so the 
specification doesn't cover it at all; see my upcoming comments on that 
patch.