Message-ID: <Y0VkeSxv9IkJPHJj@lunn.ch>
Date:   Tue, 11 Oct 2022 14:41:29 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Soha Jin <soha@...u.info>
Cc:     'Giuseppe Cavallaro' <peppe.cavallaro@...com>,
        'Alexandre Torgue' <alexandre.torgue@...s.st.com>,
        'Jose Abreu' <joabreu@...opsys.com>,
        "'David S. Miller'" <davem@...emloft.net>,
        'Eric Dumazet' <edumazet@...gle.com>,
        'Jakub Kicinski' <kuba@...nel.org>,
        'Paolo Abeni' <pabeni@...hat.com>,
        'Yangyu Chen' <cyy@...self.name>, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/3] net: stmmac: use fwnode instead of of to configure
 driver

On Tue, Oct 11, 2022 at 11:12:45AM +0800, Soha Jin wrote:
> Hi Andrew,
> 
> > From: Andrew Lunn <andrew@...n.ch>
> > Sent: Tuesday, October 11, 2022 4:38 AM
> > 
> > None of these are documented as being valid in ACPI. Do you need to ensure
> > they only come from DT, or you document them for ACPI, and get the ACPI
> > maintainers to ACK that they are O.K.
> 
> There is _DSD object in ACPI which is used to define Device Specific Data,
> and provide additional properties and information to the driver. With
> specific UUID listed in _DSD, a package can be used like Device Tree (a
> string key associated with a value), and this is also the object
> fwnode_property_* will parse with.
> 
> I have tested some of properties with a device describing stmmac device in
> ACPI, and it works. These properties should be the configuration to the
> driver, and is not related to the way configuring it. Moreover, these are
> described in _DSD and not a part of ACPI standard, there seems no need to
> ask ACPI maintainers.
> 
> Also, as described in Documentation/firmware-guide/acpi/enumeration.rst,
> there is a Device Tree Namespace Link HID (PRP0001) in kernel. PRP0001 can
> be used to describe a Device Tree in ACPI's _DSD object, and it just put DT
> properties in a _DSD package with the specific UUID I said above. But to
> utilize this feature, the driver seems need to use fwnode APIs.

The problem i see with ACPI is that everybody does it differently.
Each driver defines its own set of properties, which are different to
every other driver. You end up with snowflakes, each driver is unique,
making it harder to understand, you cannot transfer knowledge from one
driver to another. This is fine in the closed sources world of binary
blob drivers, you don't get to see other drivers that much. But for
Linux, everything is open, and we want you to look at other drivers,
copy the good ideas from other driver, make drivers look similar, so
they are easy to maintain. So ACPI snowflakes are bad.

DT on the other hand, through DT maintainers and general good
practices, splits its properties into driver unique and generic
properties. If there is a generic property for what you want to
express, you us it. Otherwise, you define a vendor property. If you
see the same vendor property a few times, you add a generic property,
since it is a concept which is shared by a number of drivers.  DT
documents all its properties.

The other problem is that people just seem to think they can stuff DT
properties into ACPI and call it done. But ACPI is not DT. DT has a
lot of history, things done wrong initially, and then corrected. We
don't want to just copy this history into ACPI. We want a clean
design, throwing away the history of things done wrongly. So i don't
expect ACPI to have any backwards compatibility hacks.

As you say, the ACPI standard says nothing about networking, or MDIO
busses, or PHYs. Which means we the Linux community can defines how
ACPI is used within networking, and we can say snowflakes are not
wanted. We can follow the good practices of DT, and document
everything.

So, please read
Documentation/firmware-guide/acpi/dsd/phy.rst. Anything generic to do
with PHYs, MDIO, etc should be documented in there. And your driver
should not handle any generic properties which are not listed in
there. Note that document says nothing about backwards compatibility.

Please add an
Documentation/firmware-guide/acpi/dsd/ethernet-controller.rst for all
the generic properties to do with the MAC driver, similar to the DT
document ethernet-controller.rst.

I would also suggest you add a document specific to this MAC driver,
documenting properties specific to it. Any DT properties which are
listed as deprecated, i don't expect to be seen implemented in ACPI.
The reset GPIO is a good example of this. Look at its horrible
history, how it is totally messed up in DT. Don't copy that mess into
ACPI.

> 
> > Backward compatibility only applies to DT. Anybody using ACPI should not
> > expect any backwards compatibility, they should be documented mandatory
> > properties right from the beginning.
> 
> Just do not want to mix the use of OF and fwnode APIs, I simply changed all
> OF property APIs with fwnode APIs. If you really think the backward
> compatibility should not exist in ACPI, I will change these compatible
> codes back to OF APIs.

Because ACPI is not DT, sometimes you do need to handle them
differently. If you have the exact same property in DT and ACPI, you
are just stuffing DT into ACPI, yes, you can use the fwnode APIs. But
when ACPI and DT differ, you need to use the lower level APIs to deal
with these differences.

     Andrew

Powered by blists - more mailing lists