Message-ID: <CAPv3WKc8gfBb7BDf5kwyPCNRxmS_H8AgQKRitbsqvL7ihbP1DA@mail.gmail.com>
Date:   Tue, 17 Jan 2023 17:05:53 +0100
From:   Marcin Wojtas <mw@...ihalf.com>
To:     Andrew Lunn <andrew@...n.ch>, Vladimir Oltean <olteanv@...il.com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        netdev@...r.kernel.org, rafael@...nel.org,
        andriy.shevchenko@...ux.intel.com, sean.wang@...iatek.com,
        Landen.Chao@...iatek.com, linus.walleij@...aro.org,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, hkallweit1@...il.com, jaz@...ihalf.com,
        tn@...ihalf.com, Samer.El-Haj-Mahmoud@....com
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@...n.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

Powered by blists - more mailing lists