Date:   Tue, 28 Jun 2022 17:09:22 +0200
From:   Michael Walle <michael@...le.cc>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
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

Am 2022-06-28 16:36, schrieb Krzysztof Kozlowski:
> On 28/06/2022 16:22, Michael Walle wrote:
>>>> I can't follow you here. Please note, that you need the actual
>>>> physical port number. It's not a made up number, but corresponds
>>>> to a physical port on that ethernet switch. So you can't just skip
>>>> the disabled ones. port@2 must have port number 2.
>>> 
>>> The number "2" you get from the reg property, so where is exactly the
>>> problem?
>> 
>> That you need to get the total number of ports of the hardware (which
>> is also used for things beyond validation, eg. during switch init
>> all ports will are disabled). And right now, that is done by counting
>> all the nodes - which is bad, I guess we agree here.
> 
> 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.

>> 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. 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.

>> 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";

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).

-michael

> 
>>   (4) keep generic compatible if the hardware can be read out to get
>>       the number of ports.
>> 
>> I think (1) isn't the way to go. (2) was my try until I noticed
>> that odd fwnode behavior. But judging by this thread, I don't think
>> thats possible. I don't know if (4) is possible at all. If not we'd
>> be left with (3).

Powered by blists - more mailing lists