Date:   Tue, 28 Jun 2022 17:17:05 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Michael Walle <michael@...le.cc>
Cc:     Andy Shevchenko <andy.shevchenko@...il.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
        devicetree <devicetree@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Horatiu Vultur <horatiu.vultur@...rochip.com>
Subject: Re: fwnode_for_each_child_node() and OF backend discrepancy

On 28/06/2022 17:09, Michael Walle wrote:
>> It's bad also from another reason - the DT node was explicitly 
>> disabled,
>> but you perform some operation on actual hardware representing this
>> node. I would assume that a disabled DT node means it is not
>> operational, e.g. hardware not present or missing clocks, so you should
>> not treat it as another meaning - power down/unused.
> 
> Mh. Assume a SoC with an integrated ethernet switch. Some ports
> are externally connected, some don't. I'd think they should be disabled,
> no? Until now, all bindings I know, treat them as disabled. But OTOH
> you still need to do some configurations on them, like disable port
> forwarding, disable them or whatever. So the hardware is present, but
> it is not connected to anything.

I see your point and the meaning is okay... except that drivers don't
touch disabled nodes. If a device (with some address space) is disabled,
you do not write there "please be power off". Here the case is a bit
different, because I think ports do not have their own address space.
Yet it contradicts the logic - something is disabled in DT and you
expect to perform actual operations on it.

> 
>>> But it works,
>>> as long as no ports are disabled and all ports are described in the
>>> device tree. But I have device trees where some are disabled.
>>
>> I am not sure if I follow this. You have devices which
>> 1. have unused ports, but all DT nodes are available/okay,
>> 2. have unused ports, which are in DT status=disabled?
>>
>> Doesn't case 2 break the bindings? If so, we don't care about such
>> out-of-tree users. We cannot support all of possible weird combinations
>> in out-of-tree DTS files...
> 
> Case 1 is invalid I think.
> 
> How does case 2 break the binding? It breaks the driver, yes. But not
> the binding.

The binding asks to describe all the ports, not describe and disable them.

> I agree on the out-of-tree argument, *but* isn't that
> what the binding is for, that out-of-tree device trees gonna work as
> long as they follow the binding? And I don't see where it dictates that 
> all
> nodes must be enabled; nor that it must either be 2 or 8 children nodes.

True, that's not specific, but as with any incomplete hardware
description in DTS, the binding cannot guarantee you
backwards-compatibility.

The hardware should be described fully in DTS and bindings expect that
as well.

> 
>>> I assume, you cannot read the hardware itself to get the number of
>>> physical ports; and we have the compatible "microchip,lan966x-switch",
>>> which is the generic one, so it could be the LAN9668 (with 8 ports)
>>> or the LAN9662 (with 2 ports).
>>
>> I'll keep that argument for future when I see again patches adding such
>> wildcard compatible. :) I had to discuss with some folks...
>>
>> Although the compatible difference does not have to be important here,
>> because one could say the 9662 and 9668 programming model is exaclty 
>> the
>> same and they differ by number of ports. This number of ports can be a
>> dedicated property or counted from the children (if they were all
>> available).
> 
> Mh. Rob was always in favor of dedicated compatible strings. And I
> think there are also subtle differences. Eg. the LAN9662 has some kind
> of accelerating engine, if I'm not mistaken.
> 
> So what do you prefer:
> 
>    compatible = "microchip,lan9668";
> and
>    compatible = "microchip,lan9662";

This one.

> 
> or
> 
>    compatible = "microchip,lan966x";
>    microchip,num-phys-ports = <8>;
> and
>    compatible = "microchip,lan966x";
>    microchip,num-phys-ports = <2>;
>    microchip,accelerating-engine;
>    ..
> 
> The argument here was always, we don't want too much properties if
> it can be deduced by the compatible string.
> 
>>> We somehow have to retain backwards
>>> compatibility. Thus my idea was to at least make the handling slightly
>>> better and count *any* child nodes. So it doesn't fall apart with
>>> disabled
>>> nodes. Then introduce proper compatible strings
>>> "microchip,lan9668-switch"
>>> and use that to hardcode the num_phys_ports to 8. But there will be
>>> device trees with microchip,lan966x-switch out there, which we do want
>>> to support.
>>>
>>> I see the following options:
>>>   (1) just don't care and get rid of the "microchip,lan966x-switch"
>>>       compatible
>>>   (2) quick fix for the older kernels by counting all the nodes and
>>>       proper fix for the newer kernels with dedicated compatibles
>>>   (3) no fix for older kernels, introduce new compatibles for new
>>>       kernels
>>
>> I propose this one. I would not care about out-of-tree DTSes which
>> decided to disable random stuff and expect things working. :)
> 
> I'd argue, that is the usual case for all the switch bindings I
> know of; not some unusual config. E.g. the SoC dtsi disables all
> ports by default and only the ones which are actually connected
> by the board are then enabled in the board dts, see
> arch/arm/boot/dts/lan966x.dtsi
> arch/arm64/boot/dts/freescale/fsl-ls1028a.dtsi
> 
> That being said, I don't care too much about the older kernels.
> So I'm fine with (3).


Best regards,
Krzysztof

Powered by blists - more mailing lists