#size-cells = <0> in a bus node, and kernel messages complaining about this

[Mitch Bradley]

On 6/28/2012 8:49 AM, Mitch Bradley wrote:
> On 6/28/2012 8:21 AM, Mitch Bradley wrote:
>> On 6/28/2012 7:50 AM, Stephen Warren wrote:
>>> On 06/27/2012 06:57 PM, Mitch Bradley wrote:
>>>> On 6/27/2012 11:26 AM, Stephen Warren wrote:
>>>>> I believe I've seen the following construct bandied about as the
>>>>> correct
>>>>> way of representing a bunch of nodes that have the same name (since
>>>>> they
>>>>> represent the same type of object) within device-tree.
>>>>>
>>>>>      regulators {
>>>>>          compatible = "simple-bus";
>>>>>          #address-cells = <1>;
>>>>>          #size-cells = <0>;
>>>>>
>>>>>          regulator at 0 {
>>>>>              compatible = "regulator-fixed";
>>>>>              reg = <0>;
>>>>> ...
>>>>>          };
>>>>>
>>>>>          regulator at 1 {
>>>>>              compatible = "regulator-fixed";
>>>>>              reg = <1>;
>>>>> ...
>>>>>          };
>>>>>      };
>>>>>
>>>>> However, when the kernel parses that, it issues messages such as:
>>>>>
>>>>> prom_parse: Bad cell count for /regulators/regulator at 0
>>>>> prom_parse: Bad cell count for /regulators/regulator at 1
>>>>
>>>> The message comes from __of_translate_address(), which has the comment:
>>>>
>>>>   * Note: We consider that crossing any level with #size-cells == 0
>>>> to mean
>>>>   * that translation is impossible (that is we are not dealing with a
>>>> value
>>>>   * that can be mapped to a cpu physical address). This is not really
>>>> specified
>>>>   * that way, but this is traditionally the way IBM at least do things
>>>>
>>>> So it seems that the problem only occurs if something tries to
>>>> translate
>>>> the regulator's "reg" address to a CPU address.  Is that
>>>> possible/meaningful
>>>> in your case?
>>>
>>> That's quite likely.
>>>
>>> Note that the regulators node is compatible = "simple-bus", and I'm
>>> doing that so that the child regulator nodes are automatically recursed
>>> into, and a platform device created for each. Part of creating those
>>> platform devices is to convert the reg and interrupts properties to
>>> platform device resources, which is almost certainly what's calling
>>> __of_translate_address(). This is a compatibility thing on ARM; I guess
>>> pure OF-style drivers call something like of_get_address()/of_iomap()
>>> themselves in their probe() function if appropriate, rather than relying
>>> on calling platform_get_resource(), and hence forcing the DT parsing
>>> code to convert resources beforehand in all cases, even if not used.
>>>
>>> Perhaps simple-bus could be enhanced to detect when size-cells==0 and
>>> know that this means the bus is really just a container/grouping of
>>> non-addressed objects, and hence not provide the memory resources. Or,
>>> perhaps we should introduce a new compatible value that only triggers
>>> child instantiation but not resource setup, or something like that?
>>
>> ISTM that if you want different semantics, a different compatible value
>> is the best approach.  My mental model is that "simple-bus" has
>> memory-mapped children. Perhaps something like "enumerated-bus" for
>> non-memory-mapped ?
>
>
> I just looked at the code in platform.c that calls
> of_translate_address() (in of_device_make_bus_id()).  It appears that
> the reason for translating the address is to create a unique device name
> via:
>
>      dev_set_name(dev, "%llx.%s", addr, node->name);
>
> Failure of_translate_address() is not treated as an error, rather the
> code falls back to naming the device with an incrementing number:
>
>      dev_set_name(dev, "%s.%d", node->name, magic - 1);
>
> Perhaps the solution is add an additional step to the heuristic.  If
> of_translate_address() fails, try to use the untranslated reg property
> value to make the name unique, generating a name like:
>
>      name.N,M
>
> where N,M is a list of #address-cells numbers from the first entry of
> the reg property value.
>
> That would be better than the existing heuristic (which includes the
> phrase "(and pray ...)" in its commentary), and fully consistent with
> device tree dogma.
>
> One could still add a new compatible value to distinguish that from a
> "simple-bus", as platform.c has a match table that includes simple-bus
> and others.  The heuristic for making the unique name is not contingent
> upon a specific bus name.


Hmmm.

1) The "KERN_ERR "prom_parse: Bad cell count ..." message is 
presumptuous, if one treats failure of of_translate_address() other than 
as a hard failure.  So if the device naming heuristic were to be 
improved as suggested above, that message should probably be removed, or 
at least downgraded to a warning or debug message.

2) of_get_address() also uses OF_CHECK_COUNTS(), which is unhappy with 
#size-cells == 0.  That's not right.  #size-cells==0 is perfectly 
reasonable for enumerated children and should be supported.

I'm reasonably certain that the right fix is simply to remove:

	if (!OF_CHECK_COUNTS(na, ns))
		return NULL;

from of_get_address().  I looked at all of the places that use 
of_get_address().  With two exceptions, of_get_address() is followed by 
of_translate_address(), which will (correctly) fail if #size-cells is 0. 
  The two exceptions are for devices that can only exist below specific 
buses, which presumably are known to have nonzero #size-cells.

>
>
>>
>>>
>>
>> _______________________________________________
>> devicetree-discuss mailing list
>> devicetree-discuss at lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/devicetree-discuss
>>
>
> _______________________________________________
> devicetree-discuss mailing list
> devicetree-discuss at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>