Messages in this thread | | | From | Marcin Wojtas <> | Date | Tue, 17 Jan 2023 17:05:53 +0100 | Subject | Re: [net-next: PATCH v4 2/8] net: mdio: switch fixed-link PHYs API to fwnode_ |
| |
Hi Andrew and Vladimir,
pon., 16 sty 2023 o 23:04 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote: > > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote: > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > > fixed-link PHYs API is used by DSA and a number of drivers > > > > and was depending on of_. Switch to fwnode_ so to make it > > > > hardware description agnostic and allow to be used in ACPI > > > > world as well. > > > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > > the more flexible fixed link implementation in phylink? > > > > Would it be even better if DSA had some driver-level prerequisites to > > impose for ACPI support - like phylink support rather than adjust_link - > > and we would simply branch off to a dsa_shared_port_link_register_acpi() > > function, leaving the current dsa_shared_port_link_register_of() alone, > > with all its workarounds and hacks? I don't believe that carrying all > > that logic over to a common fwnode based API is the proper way forward.
In the past couple of years, a number of subsystems have migrated to a more generic HW description abstraction (e.g. a big chunk of network, pinctrl, gpio). ACPI aside, with this patchset one can even try to describe the switch topology with the swnode (I haven't tried that though). I fully agree that there should be no 0-day baggage in the DSA ACPI binding (FYI the more fwnode- version of the dsa_shared_port_validate_of() cought one issue in the WIP ACPI description in my setup). On the other hand, I find fwnode_/device_ APIs really helpful for most of the cases - ACPI/OF/swnode differences can be hidden to a generic layer and the need of maintaining separate code paths related to the hardware description on the driver/subsystem level is minimized. An example could be found in v1 of this series, the last 4 patches in [1] show that it can be done in a simple / seamless way, especially given the ACPI (fwnode) PHY description in phylink is already settled and widely used. I am aware at the end of the day, after final review all this can be more complex.
I expect that the actual DSA ACPI support acceptance will require a lot of discussions and decisions, on whether certain solutions are worth migrating from OF world or require spec modification. For now my goal was to migrate to a more generic HW description API, and so to allow possible follow-up ACPI-related modifications, and additions to be extracted and better tracked.
> > I agree with you there, here is little attempt to make a clean ACPI > binding. Most of the attempts to add ACPI support seem to try to take > the short cut for just search/replace of_ with fwnode_. And we then > have to push back and say no, and generally it then goes quiet.
In most cases, the devices' description is pretty straightforward: * a node (single or with some children), resources (mem, irqs), mmio address space, optionally address on a bus and a couple of properties The DSDT/SSDT tables are very well suited for this. In case of separate, contained drivers that is also really easy to maintain.
However, I fully understand your concerns and caution before blessing any change related to subsystem/generic code. Therefore ACPI support addition was split after v1 (refer to discussion in [1]) and will require ACPI maintainers' input and guidelines.
> > Marcin, please approach this from the other end. Please document in > Documentation/firmware-guide/acpi/dsd what a clean binding should look > like, and then try to implement it. >
This is how I initially approached this (original submission: [2]; a bit updated version, working on top of the current patchset: [3]). We then agreed that in order to remove a bit hacky mitigation of the double ACPI scan problem, an MDIOSerialBus _CRS method should be defined in the ACPI spec, similar to the I2CSerialBus/SPISerialBus/UARTSerialBus. I am going to submit the first version for review in the coming days. The DSA purely ACPI-related changes would be updated and submitted, once the method is accepted.
Best regards, Marcin
[1] https://www.spinics.net/lists/netdev/msg827337.html [2] https://www.spinics.net/lists/netdev/msg827345.html [3] https://github.com/semihalf-wojtas-marcin/Linux-Kernel/commit/e017e69c0eda18747029bfe0c335df204670ba59
| |