Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230116173420.1278704-1-mw@semihalf.com> <20230116173420.1278704-3-mw@semihalf.com>
 <Y8WOVVnFInEoXLVX@shell.armlinux.org.uk> <20230116181618.2iz54jywj7rqzygu@skbuf>
 <Y8XJ3WoP+YKCjTlF@lunn.ch>
In-Reply-To: <Y8XJ3WoP+YKCjTlF@lunn.ch>
From:   Marcin Wojtas <mw@semihalf.com>
Date:   Tue, 17 Jan 2023 17:05:53 +0100
Message-ID: <CAPv3WKc8gfBb7BDf5kwyPCNRxmS_H8AgQKRitbsqvL7ihbP1DA@mail.gmail.com>
Subject: Re: [net-next: PATCH v4 2/8] net: mdio: switch fixed-link PHYs API to fwnode_
To:     Andrew Lunn <andrew@lunn.ch>, Vladimir Oltean <olteanv@gmail.com>
Cc:     "Russell King (Oracle)" <linux@armlinux.org.uk>,
        linux-kernel@vger.kernel.org, linux-acpi@vger.kernel.org,
        netdev@vger.kernel.org, rafael@kernel.org,
        andriy.shevchenko@linux.intel.com, sean.wang@mediatek.com,
        Landen.Chao@mediatek.com, linus.walleij@linaro.org,
        vivien.didelot@gmail.com, f.fainelli@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, hkallweit1@gmail.com, jaz@semihalf.com,
        tn@semihalf.com, Samer.El-Haj-Mahmoud@arm.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Andrew and Vladimir,

pon., 16 sty 2023 o 23:04 Andrew Lunn <andrew@lunn.ch> napisa=C5=82(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 u=
se
> > > 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/e017e69c0=
eda18747029bfe0c335df204670ba59