Hi Andrew and Vladimir,

pon., 16 sty 2023 o 23:04 Andrew Lunn [off-list ref] napisał(a):

On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote:

quoted

On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote:

quoted

On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote:

quoted

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