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
