LinuxLists.cc - [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> Describe the Distributed Switch Architecture (DSA) - compliant
> MDIO devices. In ACPI world they are represented as children
> of the MDIO busses, which are responsible for their enumeration
> based on the standard _ADR fields and description in _DSD objects
> under device properties UUID [1].

I would say this is too limiting. In the DT world, they are not
limited to MDIO children. They can be I2C children, SPI children
etc. There are plenty of I2C switches and SPI switches. This is
actually something we got wrong with the first DT binding. We simply
translated the platform data in DT, and at that time, there was only
MDIO switches supported. That was a real blocker to I2C, SPI and MMIO
devices until we discarded the DT binding and had a second go.

DSA switches are just devices on a bus, any sort of bus.

Look at Documentation/devicetree/binding/net/dsa/dsa.yaml. There is no
reference to MDIO.

I would expect the same with ACPI. Somehow the bus enumerates and
instantiates a device on the bus. The device then registers itself
with the DSA core. The DSA core does not care what sort of bus it is
on, that is the drivers problem.

Andrew

2022-06-20 20:06:48

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

> +In DSDT/SSDT the scope of switch device is extended by the front-panel
> +and one or more so called 'CPU' switch ports. Additionally
> +subsequent MDIO busses with attached PHYs can be described.

Humm, dsa.yaml says nothing about MDIO busses with attached PHYs.
That is up to each individual DSA drivers binding.

Please spilt this into a generic DSA binding, similar to dsa.yaml, and
a Marvell specific binding, similar to marvell.txt. It might be you
also need a generic MDIO binding, since the marvell device is just an
MDIO device, and inherits some of its properties from MDIO.

> +
> +This document presents the switch description with the required subnodes
> +and _DSD properties.
> +
> +These properties are defined in accordance with the "Device
> +Properties UUID For _DSD" [dsd-guide] document and the
> +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device
> +Data Descriptors containing them.
> +
> +Switch device
> +=============
> +
> +The switch device is represented as a child node of the MDIO bus.
> +It must comprise the _HID (and optionally _CID) field, so to allow matching
> +with appropriate driver via ACPI ID. The other obligatory field is
> +_ADR with the device address on the MDIO bus [adr]. Below example
> +shows 'SWI0' switch device at address 0x4 on the 'SMI0' bus.
>
> +.. code-block:: none
> +
> + Scope (\_SB.SMI0)
> + {
> + Name (_HID, "MRVL0100")
> + Name (_UID, 0x00)
> + Method (_STA)
> + {
> + Return (0xF)
> + }
> + Name (_CRS, ResourceTemplate ()
> + {
> + Memory32Fixed (ReadWrite,
> + 0xf212a200,
> + 0x00000010,

What do these magic numbers mean?

> + )
> + })
> + Device (SWI0)
> + {
> + Name (_HID, "MRVL0120")
> + Name (_UID, 0x00)
> + Name (_ADR, 0x4)
> + <...>
> + }

I guess it is not normal for ACPI, but could you add some comments
which explain this. In DT we have

properties:
reg:
minimum: 0
maximum: 31
description:
The ID number for the device.

which i guess what this _ADR property is, but it would be nice if it
actually described what it is supposed to mean. You have a lot of
undocumented properties here.

> +label
> +-----
> +A property with a string value describing port's name in the OS. In case the
> +port is connected to the MAC ('CPU' port), its value should be set to "cpu".

Each port is a MAC, so "is connected to the MAC" is a bit
meaningless. "CPU Port" is well defined in DSA, and is a DSA concept,
not a DT concept, so you might as well just use it here.

> +
> +phy-handle
> +----------
> +For each MAC node, a device property "phy-handle" is used to reference
> +the PHY that is registered on an MDIO bus. This is mandatory for
> +network interfaces that have PHYs connected to MAC via MDIO bus.

It is not mandatory. The DSA core will assume that port 0 has a PHY
using address 0, port 1 has a PHY using address 1, etc. You only need
a phy-handle when this assumption does not work.

> +See [phy] for more details.
> +
> +ethernet
> +--------
> +A property valid for the so called 'CPU' port and should comprise a reference
> +to the MAC object declared in the DSDT/SSDT.

Is "MAC" an ACPI term? Because this does not seem very descriptive to
me. DT says:

Should be a phandle to a valid Ethernet device node. This host
device is what the switch port is connected to

> +
> +fixed-link
> +----------
> +The 'fixed-link' is described by a data-only subnode of the
> +port, which is linked in the _DSD package via
> +hierarchical data extension (UUID dbb8e3e6-5886-4ba6-8795-1319f52a966b
> +in accordance with [dsd-guide] "_DSD Implementation Guide" document).
> +The subnode should comprise a required property ("speed") and
> +possibly the optional ones - complete list of parameters and
> +their values are specified in [ethernet-controller].

You appear to be cut/pasting
Documentation/firmware-guide/acpi/dsd/phy.txt. Please just reference
it.

> +Below example comprises MDIO bus ('SMI0') with a PHY at address 0x0 ('PHY0')
> +and a switch ('SWI0') at 0x4. The so called 'CPU' port ('PRT5') is connected to
> +the SoC's MAC (\_SB.PP20.ETH2). 'PRT2' port is configured as 1G fixed-link.

This is ACPI, so it is less likely to be a SoC. The hosts CPU port
could well be an external PCIe device for example. Yes, there are AMD
devices with built in MACs, but in the ACPI world, they don't happen
so often.

I assume you have 3 different 'compatible' strings for the nv88e6xxx
driver? You should document them somewhere and say how they map to
different marvell switches,

Andrew

2022-06-20 23:49:53

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

pon., 20 cze 2022 o 21:47 Andrew Lunn <[email protected]> napisał(a):
>
> > +In DSDT/SSDT the scope of switch device is extended by the front-panel
> > +and one or more so called 'CPU' switch ports. Additionally
> > +subsequent MDIO busses with attached PHYs can be described.
>
> Humm, dsa.yaml says nothing about MDIO busses with attached PHYs.
> That is up to each individual DSA drivers binding.
>
> Please spilt this into a generic DSA binding, similar to dsa.yaml, and
> a Marvell specific binding, similar to marvell.txt. It might be you
> also need a generic MDIO binding, since the marvell device is just an
> MDIO device, and inherits some of its properties from MDIO.
>
> > +
> > +This document presents the switch description with the required subnodes
> > +and _DSD properties.
> > +
> > +These properties are defined in accordance with the "Device
> > +Properties UUID For _DSD" [dsd-guide] document and the
> > +daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device
> > +Data Descriptors containing them.
> > +
> > +Switch device
> > +=============
> > +
> > +The switch device is represented as a child node of the MDIO bus.
> > +It must comprise the _HID (and optionally _CID) field, so to allow matching
> > +with appropriate driver via ACPI ID. The other obligatory field is
> > +_ADR with the device address on the MDIO bus [adr]. Below example
> > +shows 'SWI0' switch device at address 0x4 on the 'SMI0' bus.
> >
> > +.. code-block:: none
> > +
> > + Scope (\_SB.SMI0)
> > + {
> > + Name (_HID, "MRVL0100")
> > + Name (_UID, 0x00)
> > + Method (_STA)
> > + {
> > + Return (0xF)
> > + }
> > + Name (_CRS, ResourceTemplate ()
> > + {
> > + Memory32Fixed (ReadWrite,
> > + 0xf212a200,
> > + 0x00000010,
>
> What do these magic numbers mean?
>
> > + )
> > + })
> > + Device (SWI0)
> > + {
> > + Name (_HID, "MRVL0120")
> > + Name (_UID, 0x00)
> > + Name (_ADR, 0x4)
> > + <...>
> > + }
>
> I guess it is not normal for ACPI, but could you add some comments
> which explain this. In DT we have
>
> properties:
> reg:
> minimum: 0
> maximum: 31
> description:
> The ID number for the device.
>
> which i guess what this _ADR property is, but it would be nice if it
> actually described what it is supposed to mean. You have a lot of
> undocumented properties here.
>
>
> > +label
> > +-----
> > +A property with a string value describing port's name in the OS. In case the
> > +port is connected to the MAC ('CPU' port), its value should be set to "cpu".
>
> Each port is a MAC, so "is connected to the MAC" is a bit
> meaningless. "CPU Port" is well defined in DSA, and is a DSA concept,
> not a DT concept, so you might as well just use it here.
>
> > +
> > +phy-handle
> > +----------
> > +For each MAC node, a device property "phy-handle" is used to reference
> > +the PHY that is registered on an MDIO bus. This is mandatory for
> > +network interfaces that have PHYs connected to MAC via MDIO bus.
>
> It is not mandatory. The DSA core will assume that port 0 has a PHY
> using address 0, port 1 has a PHY using address 1, etc. You only need
> a phy-handle when this assumption does not work.
>
> > +See [phy] for more details.
> > +
> > +ethernet
> > +--------
> > +A property valid for the so called 'CPU' port and should comprise a reference
> > +to the MAC object declared in the DSDT/SSDT.
>
> Is "MAC" an ACPI term? Because this does not seem very descriptive to
> me. DT says:
>
> Should be a phandle to a valid Ethernet device node. This host
> device is what the switch port is connected to
>
> > +
> > +fixed-link
> > +----------
> > +The 'fixed-link' is described by a data-only subnode of the
> > +port, which is linked in the _DSD package via
> > +hierarchical data extension (UUID dbb8e3e6-5886-4ba6-8795-1319f52a966b
> > +in accordance with [dsd-guide] "_DSD Implementation Guide" document).
> > +The subnode should comprise a required property ("speed") and
> > +possibly the optional ones - complete list of parameters and
> > +their values are specified in [ethernet-controller].
>
> You appear to be cut/pasting
> Documentation/firmware-guide/acpi/dsd/phy.txt. Please just reference
> it.
>
> > +Below example comprises MDIO bus ('SMI0') with a PHY at address 0x0 ('PHY0')
> > +and a switch ('SWI0') at 0x4. The so called 'CPU' port ('PRT5') is connected to
> > +the SoC's MAC (\_SB.PP20.ETH2). 'PRT2' port is configured as 1G fixed-link.
>
> This is ACPI, so it is less likely to be a SoC. The hosts CPU port
> could well be an external PCIe device for example. Yes, there are AMD
> devices with built in MACs, but in the ACPI world, they don't happen
> so often.
>
> I assume you have 3 different 'compatible' strings for the nv88e6xxx
> driver? You should document them somewhere and say how they map to
> different marvell switches,
>

Thank you for the remarks, I'll address all after the consensus about
the binding is established.

Best regards,
Marcin

2022-06-21 00:16:44

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

pon., 20 cze 2022 o 20:19 Andrew Lunn <[email protected]> napisał(a):
>
> On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > Describe the Distributed Switch Architecture (DSA) - compliant
> > MDIO devices. In ACPI world they are represented as children
> > of the MDIO busses, which are responsible for their enumeration
> > based on the standard _ADR fields and description in _DSD objects
> > under device properties UUID [1].
>
> I would say this is too limiting. In the DT world, they are not
> limited to MDIO children. They can be I2C children, SPI children
> etc. There are plenty of I2C switches and SPI switches. This is
> actually something we got wrong with the first DT binding. We simply
> translated the platform data in DT, and at that time, there was only
> MDIO switches supported. That was a real blocker to I2C, SPI and MMIO
> devices until we discarded the DT binding and had a second go.
>
> DSA switches are just devices on a bus, any sort of bus.
>
> Look at Documentation/devicetree/binding/net/dsa/dsa.yaml. There is no
> reference to MDIO.
>
> I would expect the same with ACPI. Somehow the bus enumerates and
> instantiates a device on the bus. The device then registers itself
> with the DSA core. The DSA core does not care what sort of bus it is
> on, that is the drivers problem.
>

Thanks for mentioning the other options. It makes things easier and
the MDIO as current strict dependency will be dropped.

Best regards,
Marcin

2022-06-21 10:04:19

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> Describe the Distributed Switch Architecture (DSA) - compliant
> MDIO devices. In ACPI world they are represented as children
> of the MDIO busses, which are responsible for their enumeration
> based on the standard _ADR fields and description in _DSD objects
> under device properties UUID [1].
>
> [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
> Signed-off-by: Marcin Wojtas <[email protected]>
> ---
> Documentation/firmware-guide/acpi/dsd/dsa.rst | 359 ++++++++++++++++++++
> Documentation/firmware-guide/acpi/index.rst | 1 +
> 2 files changed, 360 insertions(+)
> create mode 100644 Documentation/firmware-guide/acpi/dsd/dsa.rst
>
> diff --git a/Documentation/firmware-guide/acpi/dsd/dsa.rst b/Documentation/firmware-guide/acpi/dsd/dsa.rst
> new file mode 100644
> index 000000000000..dba76d89f4e6
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/dsa.rst

Why is this document part of Linux code base ?
How will the other OSes be aware of this ?
I assume there was some repository to maintain such DSDs so that it
is accessible for other OSes. I am not agreeing or disagreeing on the
change itself, but I am concerned about this present in the kernel
code.

--
Regards,
Sudeep

2022-06-21 11:15:56

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:

...

> > + Name (_CRS, ResourceTemplate ()
> > + {
> > + Memory32Fixed (ReadWrite,
> > + 0xf212a200,
> > + 0x00000010,
>
> What do these magic numbers mean?

Address + Length, it's all described in the ACPI specification. Or if you asked
why the values there are the particular numbers? I guess it's fined to have
anything sane in the example. OTOH a comment may be added.

> > + )
> > + })

...

> > + Device (SWI0)
> > + {
> > + Name (_HID, "MRVL0120")
> > + Name (_UID, 0x00)
> > + Name (_ADR, 0x4)
> > + <...>
> > + }
>
> I guess it is not normal for ACPI, but could you add some comments
> which explain this. In DT we have
>
> properties:
> reg:
> minimum: 0
> maximum: 31
> description:
> The ID number for the device.
>
> which i guess what this _ADR property is, but it would be nice if it
> actually described what it is supposed to mean. You have a lot of
> undocumented properties here.

Btw, you are right, _ADR mustn't go together with _HID/_CID.

--
With Best Regards,
Andy Shevchenko

2022-06-21 11:22:07

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:
>
> ...
>
> > > + Name (_CRS, ResourceTemplate ()
> > > + {
> > > + Memory32Fixed (ReadWrite,
> > > + 0xf212a200,
> > > + 0x00000010,
> >
> > What do these magic numbers mean?
>
> Address + Length, it's all described in the ACPI specification.

The address+plus length of what? This device is on an MDIO bus. As
such, there is no memory! It probably makes sense to somebody who
knows ACPI, but to me i have no idea what it means.

> Or if you asked
> why the values there are the particular numbers? I guess it's fined to have
> anything sane in the example. OTOH a comment may be added.
>
> > > + )
> > > + })
>
> ...
>
> > > + Device (SWI0)
> > > + {
> > > + Name (_HID, "MRVL0120")
> > > + Name (_UID, 0x00)
> > > + Name (_ADR, 0x4)
> > > + <...>
> > > + }
> >
> > I guess it is not normal for ACPI, but could you add some comments
> > which explain this. In DT we have
> >
> > properties:
> > reg:
> > minimum: 0
> > maximum: 31
> > description:
> > The ID number for the device.
> >
> > which i guess what this _ADR property is, but it would be nice if it
> > actually described what it is supposed to mean. You have a lot of
> > undocumented properties here.
>
> Btw, you are right, _ADR mustn't go together with _HID/_CID.

Does ACPI have anything like .yaml to describe the binding and tools
to validate it?

Andrew

2022-06-21 11:31:13

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > Describe the Distributed Switch Architecture (DSA) - compliant
> > MDIO devices. In ACPI world they are represented as children
> > of the MDIO busses, which are responsible for their enumeration
> > based on the standard _ADR fields and description in _DSD objects
> > under device properties UUID [1].
> >
> > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf

> Why is this document part of Linux code base ?

It's fine, but your are right with your latter questions.

> How will the other OSes be aware of this ?

Should be a standard somewhere.

> I assume there was some repository to maintain such DSDs so that it
> is accessible for other OSes. I am not agreeing or disagreeing on the
> change itself, but I am concerned about this present in the kernel
> code.

I dunno we have a such, but the closest I may imagine is MIPI standardization,
that we have at least for cameras and sound.

I would suggest to go and work with MIPI for network / DSA / etc area, so
everybody else will be aware of the standard.

--
With Best Regards,
Andy Shevchenko

2022-06-21 11:43:36

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:

...

> > > > + Name (_CRS, ResourceTemplate ()
> > > > + {
> > > > + Memory32Fixed (ReadWrite,
> > > > + 0xf212a200,
> > > > + 0x00000010,
> > >
> > > What do these magic numbers mean?
> >
> > Address + Length, it's all described in the ACPI specification.
>
> The address+plus length of what? This device is on an MDIO bus. As
> such, there is no memory! It probably makes sense to somebody who
> knows ACPI, but to me i have no idea what it means.

I see what you mean. Honestly I dunno what the device this description is for.
For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
MDIOSerialBus() resources type is what would be good to have in ACPI
specification.

> > Or if you asked
> > why the values there are the particular numbers? I guess it's fined to have
> > anything sane in the example. OTOH a comment may be added.
> >
> > > > + )
> > > > + })

...

> > > > + Device (SWI0)
> > > > + {
> > > > + Name (_HID, "MRVL0120")
> > > > + Name (_UID, 0x00)
> > > > + Name (_ADR, 0x4)
> > > > + <...>
> > > > + }
> > >
> > > I guess it is not normal for ACPI, but could you add some comments
> > > which explain this. In DT we have
> > >
> > > properties:
> > > reg:
> > > minimum: 0
> > > maximum: 31
> > > description:
> > > The ID number for the device.
> > >
> > > which i guess what this _ADR property is, but it would be nice if it
> > > actually described what it is supposed to mean. You have a lot of
> > > undocumented properties here.
> >
> > Btw, you are right, _ADR mustn't go together with _HID/_CID.
>
> Does ACPI have anything like .yaml to describe the binding and tools
> to validate it?

ACPI is a language, the "bindings" (in a way how you probably use this term)
are a small subset and usually refers to the format whoever provides them. I.e.
_DSD with a certain UUID is a part of the DT bindings, so DT validation is what
is expected to be performed. Otherwise for other UUIDs it may or may be no such
validators exist, I don't know.

--
With Best Regards,
Andy Shevchenko

2022-06-21 11:56:38

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > MDIO devices. In ACPI world they are represented as children
> > > of the MDIO busses, which are responsible for their enumeration
> > > based on the standard _ADR fields and description in _DSD objects
> > > under device properties UUID [1].
> > >
> > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
>
> > Why is this document part of Linux code base ?
>
> It's fine, but your are right with your latter questions.
>
> > How will the other OSes be aware of this ?
>
> Should be a standard somewhere.
>
> > I assume there was some repository to maintain such DSDs so that it
> > is accessible for other OSes. I am not agreeing or disagreeing on the
> > change itself, but I am concerned about this present in the kernel
> > code.
>
> I dunno we have a such, but the closest I may imagine is MIPI standardization,
> that we have at least for cameras and sound.
>
> I would suggest to go and work with MIPI for network / DSA / etc area, so
> everybody else will be aware of the standard.

It is the same argument as for DT. Other OSes and bootloaders seem to
manage digging around in Linux for DT binding documentation. I don't
see why bootloaders and other OSes can not also dig around in Linux
for ACPI binding documentations.

Ideally, somebody will submit all this for acceptance into ACPI, but
into somebody does, i suspect it will just remain a defacto standard
in Linux.

Andrew

2022-06-21 12:04:11

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 02:46:05PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> > On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
>
> ...
>
> > > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > > that we have at least for cameras and sound.
> > >
> > > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > > everybody else will be aware of the standard.
> >
> > It is the same argument as for DT. Other OSes and bootloaders seem to
> > manage digging around in Linux for DT binding documentation. I don't
> > see why bootloaders and other OSes can not also dig around in Linux
> > for ACPI binding documentations.
> >
> > Ideally, somebody will submit all this for acceptance into ACPI, but
> > into somebody does, i suspect it will just remain a defacto standard
> > in Linux.
>
> The "bindings" are orthogonal to ACPI specification. It's a vendor / OS / ...
> specific from ACPI p.o.v. It has an UUID field and each UUID may or may not
> be a part of any standard.

We want to avoid snowflakes, each driver doing its own thing,
different to every other driver. So we push as much as possible into
the core, meaning the driver have no choice. So i expect the MDIO part
to look the same for every MDIO bus in Linux using ACPI. I expect the
PHY part to look the same, for every PHY using ACPI in Linux, the DSA
part to look the same, for every DSA switch using linux, because all
the ACPI is in the core of each of these subsystems. The driver only
gets to implement its own properties for anything which is not in one
of these cores.

So you say these bindings are vendor/OS specific, which is great. We
are defining how Linux does this. We are fully open, any other OS or
bootloader can copy it, but it also suggests we don't need to care
about other OSes and bootloaders? That actually seems opposite to DT,
were we do try to share it, and avoid being vendor or OS specific!

Andrew

2022-06-21 12:13:14

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:

...

> > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > that we have at least for cameras and sound.
> >
> > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > everybody else will be aware of the standard.
>
> It is the same argument as for DT. Other OSes and bootloaders seem to
> manage digging around in Linux for DT binding documentation. I don't
> see why bootloaders and other OSes can not also dig around in Linux
> for ACPI binding documentations.
>
> Ideally, somebody will submit all this for acceptance into ACPI, but
> into somebody does, i suspect it will just remain a defacto standard
> in Linux.

The "bindings" are orthogonal to ACPI specification. It's a vendor / OS / ...
specific from ACPI p.o.v. It has an UUID field and each UUID may or may not
be a part of any standard.

--
With Best Regards,
Andy Shevchenko

2022-06-21 13:38:11

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > > MDIO devices. In ACPI world they are represented as children
> > > > of the MDIO busses, which are responsible for their enumeration
> > > > based on the standard _ADR fields and description in _DSD objects
> > > > under device properties UUID [1].
> > > >
> > > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> >
> > > Why is this document part of Linux code base ?
> >
> > It's fine, but your are right with your latter questions.
> >
> > > How will the other OSes be aware of this ?
> >
> > Should be a standard somewhere.
> >
> > > I assume there was some repository to maintain such DSDs so that it
> > > is accessible for other OSes. I am not agreeing or disagreeing on the
> > > change itself, but I am concerned about this present in the kernel
> > > code.
> >
> > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > that we have at least for cameras and sound.
> >
> > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > everybody else will be aware of the standard.
>
> It is the same argument as for DT. Other OSes and bootloaders seem to
> manage digging around in Linux for DT binding documentation. I don't
> see why bootloaders and other OSes can not also dig around in Linux
> for ACPI binding documentations.
>

Theoretically you are right. But in DT case majority of non-standard(by
standard I am referring to the one's in Open Firmware specification) are
in the kernel. But that is not true for ACPI. And that is the reason for
objecting it. One of the main other OS using ACPI may not look here for
any ACPI bindings(we may not care, but still OS neutral place is better
for this).

> Ideally, somebody will submit all this for acceptance into ACPI, but
> into somebody does, i suspect it will just remain a defacto standard
> in Linux.
>

DSD is not integral part of ACPI spec, so the process is never clear.
However there is this project[1], IIUC it is just guidance and doesn't
include any bindings IIUC. But we need something similar here for better
visibility and to remain OS agnostic. Even with DT, there is a strong
desire to separate it out, but it has grown so much that it is getting
harder to do that with every release. I was just trying to avoid getting
into that situation.

--
Regards,
Sudeep

[1] https://github.com/UEFI/DSD-Guide

2022-06-21 15:40:49

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 05:23:30PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 21, 2022 at 3:28 PM Sudeep Holla <[email protected]> wrote:
> >
> > On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> > > On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > > > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > > > > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > > > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > > > > MDIO devices. In ACPI world they are represented as children
> > > > > > of the MDIO busses, which are responsible for their enumeration
> > > > > > based on the standard _ADR fields and description in _DSD objects
> > > > > > under device properties UUID [1].
> > > > > >
> > > > > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > > >
> > > > > Why is this document part of Linux code base ?
> > > >
> > > > It's fine, but your are right with your latter questions.
> > > >
> > > > > How will the other OSes be aware of this ?
> > > >
> > > > Should be a standard somewhere.
> > > >
> > > > > I assume there was some repository to maintain such DSDs so that it
> > > > > is accessible for other OSes. I am not agreeing or disagreeing on the
> > > > > change itself, but I am concerned about this present in the kernel
> > > > > code.
> > > >
> > > > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > > > that we have at least for cameras and sound.
> > > >
> > > > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > > > everybody else will be aware of the standard.
> > >
> > > It is the same argument as for DT. Other OSes and bootloaders seem to
> > > manage digging around in Linux for DT binding documentation. I don't
> > > see why bootloaders and other OSes can not also dig around in Linux
> > > for ACPI binding documentations.
> > >
> >
> > Theoretically you are right. But in DT case majority of non-standard(by
> > standard I am referring to the one's in Open Firmware specification) are
> > in the kernel. But that is not true for ACPI. And that is the reason for
> > objecting it. One of the main other OS using ACPI may not look here for
> > any ACPI bindings(we may not care, but still OS neutral place is better
> > for this).
> >
> > > Ideally, somebody will submit all this for acceptance into ACPI, but
> > > into somebody does, i suspect it will just remain a defacto standard
> > > in Linux.
> > >
> >
> > DSD is not integral part of ACPI spec, so the process is never clear.
> > However there is this project[1], IIUC it is just guidance and doesn't
> > include any bindings IIUC. But we need something similar here for better
> > visibility and to remain OS agnostic. Even with DT, there is a strong
> > desire to separate it out, but it has grown so much that it is getting
> > harder to do that with every release. I was just trying to avoid getting
> > into that situation.
> >
> > [1] https://github.com/UEFI/DSD-Guide
>
> Here's my personal take on this.
>
> This patch series essentially makes the kernel recognize a few generic
> (that is, not tied on any specific device ID) device properties
> supplied by the firmware via _DSD. They are generic, because there is
> some library code in the kernel that can consume them and that library
> code is used in multiple places (and it is better to supply data from
> the firmware directly to it).
>
> If we all agree that it is a good idea for the kernel to allow these
> properties to be supplied via _DSD this way, there is no reason to
> avoid admitting that fact in the kernel documentation.
>
> IMV, there's nothing wrong with stating officially that these
> properties are recognized by the kernel and what they are used for and
> it has no bearing on whether or not they are also used by someone
> else.

Good point. I was also suggested to make properties have prefix "linux-"
similar to "uefi-" in the set of DSD properties list @[1]. In that case
it makes more sense to maintain in the kernel. If they add "uefi-" prefix,
I was also told that it can be hosted @[1] as specific in section 3.1.4 @[2]

I just sent an update to Documentation with the link to[1]. I can also
update the same to mention about the process as described in section 3.1.4
if that helps and we are happy to follow that in the kernel.

--
Regards,
Sudeep

[1] https://github.com/UEFI/DSD-Guide
[2] https://github.com/UEFI/DSD-Guide/blob/main/src/dsd-guide.adoc#314-adding-uefi-device-properties

2022-06-21 16:05:18

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 5:37 PM Sudeep Holla <[email protected]> wrote:
>
> On Tue, Jun 21, 2022 at 05:23:30PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jun 21, 2022 at 3:28 PM Sudeep Holla <[email protected]> wrote:
> > >
> > > On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> > > > On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > > > > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > > > > > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > > > > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > > > > > MDIO devices. In ACPI world they are represented as children
> > > > > > > of the MDIO busses, which are responsible for their enumeration
> > > > > > > based on the standard _ADR fields and description in _DSD objects
> > > > > > > under device properties UUID [1].
> > > > > > >
> > > > > > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > > > >
> > > > > > Why is this document part of Linux code base ?
> > > > >
> > > > > It's fine, but your are right with your latter questions.
> > > > >
> > > > > > How will the other OSes be aware of this ?
> > > > >
> > > > > Should be a standard somewhere.
> > > > >
> > > > > > I assume there was some repository to maintain such DSDs so that it
> > > > > > is accessible for other OSes. I am not agreeing or disagreeing on the
> > > > > > change itself, but I am concerned about this present in the kernel
> > > > > > code.
> > > > >
> > > > > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > > > > that we have at least for cameras and sound.
> > > > >
> > > > > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > > > > everybody else will be aware of the standard.
> > > >
> > > > It is the same argument as for DT. Other OSes and bootloaders seem to
> > > > manage digging around in Linux for DT binding documentation. I don't
> > > > see why bootloaders and other OSes can not also dig around in Linux
> > > > for ACPI binding documentations.
> > > >
> > >
> > > Theoretically you are right. But in DT case majority of non-standard(by
> > > standard I am referring to the one's in Open Firmware specification) are
> > > in the kernel. But that is not true for ACPI. And that is the reason for
> > > objecting it. One of the main other OS using ACPI may not look here for
> > > any ACPI bindings(we may not care, but still OS neutral place is better
> > > for this).
> > >
> > > > Ideally, somebody will submit all this for acceptance into ACPI, but
> > > > into somebody does, i suspect it will just remain a defacto standard
> > > > in Linux.
> > > >
> > >
> > > DSD is not integral part of ACPI spec, so the process is never clear.
> > > However there is this project[1], IIUC it is just guidance and doesn't
> > > include any bindings IIUC. But we need something similar here for better
> > > visibility and to remain OS agnostic. Even with DT, there is a strong
> > > desire to separate it out, but it has grown so much that it is getting
> > > harder to do that with every release. I was just trying to avoid getting
> > > into that situation.
> > >
> > > [1] https://github.com/UEFI/DSD-Guide
> >
> > Here's my personal take on this.
> >
> > This patch series essentially makes the kernel recognize a few generic
> > (that is, not tied on any specific device ID) device properties
> > supplied by the firmware via _DSD. They are generic, because there is
> > some library code in the kernel that can consume them and that library
> > code is used in multiple places (and it is better to supply data from
> > the firmware directly to it).
> >
> > If we all agree that it is a good idea for the kernel to allow these
> > properties to be supplied via _DSD this way, there is no reason to
> > avoid admitting that fact in the kernel documentation.
> >
> > IMV, there's nothing wrong with stating officially that these
> > properties are recognized by the kernel and what they are used for and
> > it has no bearing on whether or not they are also used by someone
> > else.
>
> Good point. I was also suggested to make properties have prefix "linux-"
> similar to "uefi-" in the set of DSD properties list @[1]. In that case
> it makes more sense to maintain in the kernel. If they add "uefi-" prefix,
> I was also told that it can be hosted @[1] as specific in section 3.1.4 @[2]

Well, the point here is to use the same property names on both the DT
and ACPI ends IIUC and there's certain value in doing that.

The library in question already uses these names with DT and there is
no real need to change that.

Of course, if the platform firmware supplies these properties in a way
described in the document in question, it will be a provision
specifically for Linux and nothing else (unless that hypothetical
other thing decides to follow Linux in this respect, that is). As
long as that is clear, I don't see why it would be better to introduce
different property names just for _DSD.

> I just sent an update to Documentation with the link to[1].

Thanks!

> I can also
> update the same to mention about the process as described in section 3.1.4
> if that helps and we are happy to follow that in the kernel.
>
> [1] https://github.com/UEFI/DSD-Guide
> [2] https://github.com/UEFI/DSD-Guide/blob/main/src/dsd-guide.adoc#314-adding-uefi-device-properties

IMV it would be useful to add that information, but IMO the process is
mostly relevant for new use cases, when someone wants to introduce an
entirely new property that is not yet covered by the DT bindings.

In the cases when the existing DT properties are the closest thing to
a standard way of supplying the OS with the information in question it
is most appealing to use the property names that are in use already.

2022-06-21 16:40:40

by Rafael J. Wysocki

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 3:28 PM Sudeep Holla <[email protected]> wrote:
>
> On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> > On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > > > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > > > MDIO devices. In ACPI world they are represented as children
> > > > > of the MDIO busses, which are responsible for their enumeration
> > > > > based on the standard _ADR fields and description in _DSD objects
> > > > > under device properties UUID [1].
> > > > >
> > > > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > >
> > > > Why is this document part of Linux code base ?
> > >
> > > It's fine, but your are right with your latter questions.
> > >
> > > > How will the other OSes be aware of this ?
> > >
> > > Should be a standard somewhere.
> > >
> > > > I assume there was some repository to maintain such DSDs so that it
> > > > is accessible for other OSes. I am not agreeing or disagreeing on the
> > > > change itself, but I am concerned about this present in the kernel
> > > > code.
> > >
> > > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > > that we have at least for cameras and sound.
> > >
> > > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > > everybody else will be aware of the standard.
> >
> > It is the same argument as for DT. Other OSes and bootloaders seem to
> > manage digging around in Linux for DT binding documentation. I don't
> > see why bootloaders and other OSes can not also dig around in Linux
> > for ACPI binding documentations.
> >
>
> Theoretically you are right. But in DT case majority of non-standard(by
> standard I am referring to the one's in Open Firmware specification) are
> in the kernel. But that is not true for ACPI. And that is the reason for
> objecting it. One of the main other OS using ACPI may not look here for
> any ACPI bindings(we may not care, but still OS neutral place is better
> for this).
>
> > Ideally, somebody will submit all this for acceptance into ACPI, but
> > into somebody does, i suspect it will just remain a defacto standard
> > in Linux.
> >
>
> DSD is not integral part of ACPI spec, so the process is never clear.
> However there is this project[1], IIUC it is just guidance and doesn't
> include any bindings IIUC. But we need something similar here for better
> visibility and to remain OS agnostic. Even with DT, there is a strong
> desire to separate it out, but it has grown so much that it is getting
> harder to do that with every release. I was just trying to avoid getting
> into that situation.
>
> [1] https://github.com/UEFI/DSD-Guide

Here's my personal take on this.

This patch series essentially makes the kernel recognize a few generic
(that is, not tied on any specific device ID) device properties
supplied by the firmware via _DSD. They are generic, because there is
some library code in the kernel that can consume them and that library
code is used in multiple places (and it is better to supply data from
the firmware directly to it).

If we all agree that it is a good idea for the kernel to allow these
properties to be supplied via _DSD this way, there is no reason to
avoid admitting that fact in the kernel documentation.

IMV, there's nothing wrong with stating officially that these
properties are recognized by the kernel and what they are used for and
it has no bearing on whether or not they are also used by someone
else.

2022-06-21 18:29:17

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Tue, Jun 21, 2022 at 06:00:42PM +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 21, 2022 at 5:37 PM Sudeep Holla <[email protected]> wrote:
> >
> > On Tue, Jun 21, 2022 at 05:23:30PM +0200, Rafael J. Wysocki wrote:
> > > On Tue, Jun 21, 2022 at 3:28 PM Sudeep Holla <[email protected]> wrote:
> > > >
> > > > On Tue, Jun 21, 2022 at 01:24:51PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Jun 21, 2022 at 02:15:41PM +0300, Andy Shevchenko wrote:
> > > > > > On Tue, Jun 21, 2022 at 10:45:56AM +0100, Sudeep Holla wrote:
> > > > > > > On Mon, Jun 20, 2022 at 05:02:22PM +0200, Marcin Wojtas wrote:
> > > > > > > > Describe the Distributed Switch Architecture (DSA) - compliant
> > > > > > > > MDIO devices. In ACPI world they are represented as children
> > > > > > > > of the MDIO busses, which are responsible for their enumeration
> > > > > > > > based on the standard _ADR fields and description in _DSD objects
> > > > > > > > under device properties UUID [1].
> > > > > > > >
> > > > > > > > [1] http://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf
> > > > > >
> > > > > > > Why is this document part of Linux code base ?
> > > > > >
> > > > > > It's fine, but your are right with your latter questions.
> > > > > >
> > > > > > > How will the other OSes be aware of this ?
> > > > > >
> > > > > > Should be a standard somewhere.
> > > > > >
> > > > > > > I assume there was some repository to maintain such DSDs so that it
> > > > > > > is accessible for other OSes. I am not agreeing or disagreeing on the
> > > > > > > change itself, but I am concerned about this present in the kernel
> > > > > > > code.
> > > > > >
> > > > > > I dunno we have a such, but the closest I may imagine is MIPI standardization,
> > > > > > that we have at least for cameras and sound.
> > > > > >
> > > > > > I would suggest to go and work with MIPI for network / DSA / etc area, so
> > > > > > everybody else will be aware of the standard.
> > > > >
> > > > > It is the same argument as for DT. Other OSes and bootloaders seem to
> > > > > manage digging around in Linux for DT binding documentation. I don't
> > > > > see why bootloaders and other OSes can not also dig around in Linux
> > > > > for ACPI binding documentations.
> > > > >
> > > >
> > > > Theoretically you are right. But in DT case majority of non-standard(by
> > > > standard I am referring to the one's in Open Firmware specification) are
> > > > in the kernel. But that is not true for ACPI. And that is the reason for
> > > > objecting it. One of the main other OS using ACPI may not look here for
> > > > any ACPI bindings(we may not care, but still OS neutral place is better
> > > > for this).
> > > >
> > > > > Ideally, somebody will submit all this for acceptance into ACPI, but
> > > > > into somebody does, i suspect it will just remain a defacto standard
> > > > > in Linux.
> > > > >
> > > >
> > > > DSD is not integral part of ACPI spec, so the process is never clear.
> > > > However there is this project[1], IIUC it is just guidance and doesn't
> > > > include any bindings IIUC. But we need something similar here for better
> > > > visibility and to remain OS agnostic. Even with DT, there is a strong
> > > > desire to separate it out, but it has grown so much that it is getting
> > > > harder to do that with every release. I was just trying to avoid getting
> > > > into that situation.
> > > >
> > > > [1] https://github.com/UEFI/DSD-Guide
> > >
> > > Here's my personal take on this.
> > >
> > > This patch series essentially makes the kernel recognize a few generic
> > > (that is, not tied on any specific device ID) device properties
> > > supplied by the firmware via _DSD. They are generic, because there is
> > > some library code in the kernel that can consume them and that library
> > > code is used in multiple places (and it is better to supply data from
> > > the firmware directly to it).
> > >
> > > If we all agree that it is a good idea for the kernel to allow these
> > > properties to be supplied via _DSD this way, there is no reason to
> > > avoid admitting that fact in the kernel documentation.
> > >
> > > IMV, there's nothing wrong with stating officially that these
> > > properties are recognized by the kernel and what they are used for and
> > > it has no bearing on whether or not they are also used by someone
> > > else.
> >
> > Good point. I was also suggested to make properties have prefix "linux-"
> > similar to "uefi-" in the set of DSD properties list @[1]. In that case
> > it makes more sense to maintain in the kernel. If they add "uefi-" prefix,
> > I was also told that it can be hosted @[1] as specific in section 3.1.4 @[2]
>
> Well, the point here is to use the same property names on both the DT
> and ACPI ends IIUC and there's certain value in doing that.
>
> The library in question already uses these names with DT and there is
> no real need to change that.
>

Make sense and I agree.

> Of course, if the platform firmware supplies these properties in a way
> described in the document in question, it will be a provision
> specifically for Linux and nothing else (unless that hypothetical
> other thing decides to follow Linux in this respect, that is). As
> long as that is clear, I don't see why it would be better to introduce
> different property names just for _DSD.
>

Understood and very valid point.

> > I just sent an update to Documentation with the link to[1].
>
> Thanks!
>
> > I can also
> > update the same to mention about the process as described in section 3.1.4
> > if that helps and we are happy to follow that in the kernel.
> >
> > [1] https://github.com/UEFI/DSD-Guide
> > [2] https://github.com/UEFI/DSD-Guide/blob/main/src/dsd-guide.adoc#314-adding-uefi-device-properties
>
> IMV it would be useful to add that information, but IMO the process is
> mostly relevant for new use cases, when someone wants to introduce an
> entirely new property that is not yet covered by the DT bindings.
>
> In the cases when the existing DT properties are the closest thing to
> a standard way of supplying the OS with the information in question it
> is most appealing to use the property names that are in use already.

Agreed, I wasn't aware that the properties discussed in $subject are already
DT properties. I agree on the point that we don't want to have a different
one(with prefix) if the DT properties are already supported.

--
Regards,
Sudeep

2022-06-22 09:37:05

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

wt., 21 cze 2022 o 13:42 Andy Shevchenko
<[email protected]> napisał(a):
>
> On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> > On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:
>
> ...
>
> > > > > + Name (_CRS, ResourceTemplate ()
> > > > > + {
> > > > > + Memory32Fixed (ReadWrite,
> > > > > + 0xf212a200,
> > > > > + 0x00000010,
> > > >
> > > > What do these magic numbers mean?
> > >
> > > Address + Length, it's all described in the ACPI specification.
> >
> > The address+plus length of what? This device is on an MDIO bus. As
> > such, there is no memory! It probably makes sense to somebody who
> > knows ACPI, but to me i have no idea what it means.
>
> I see what you mean. Honestly I dunno what the device this description is for.
> For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
> MDIOSerialBus() resources type is what would be good to have in ACPI
> specification.
>

It's not device on MDIO bus, but the MDIO controller's register itself
(this _CSR belongs to the parent, subnodes do not refer to it in any
way). The child device requires only _ADR (or whatever else is needed
for the case the DSA device is attached to SPI/I2C controllers).

Best regards,
Marcin

2022-06-22 10:14:36

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote:
> wt., 21 cze 2022 o 13:42 Andy Shevchenko
> <[email protected]> napisał(a):
> >
> > On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> > > On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > > > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:
> >
> > ...
> >
> > > > > > + Name (_CRS, ResourceTemplate ()
> > > > > > + {
> > > > > > + Memory32Fixed (ReadWrite,
> > > > > > + 0xf212a200,
> > > > > > + 0x00000010,
> > > > >
> > > > > What do these magic numbers mean?
> > > >
> > > > Address + Length, it's all described in the ACPI specification.
> > >
> > > The address+plus length of what? This device is on an MDIO bus. As
> > > such, there is no memory! It probably makes sense to somebody who
> > > knows ACPI, but to me i have no idea what it means.
> >
> > I see what you mean. Honestly I dunno what the device this description is for.
> > For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
> > MDIOSerialBus() resources type is what would be good to have in ACPI
> > specification.
> >
>
> It's not device on MDIO bus, but the MDIO controller's register itself

Ah. So this is equivalent to

CP11X_LABEL(mdio): mdio@12a200 {
#address-cells = <1>;
#size-cells = <0>;
compatible = "marvell,orion-mdio";
reg = <0x12a200 0x10>;
clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>,
<&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
status = "disabled";
};

DT seems a lot more readable, "marvell,orion-mdio" is a good hint that
device this is. But maybe it is more readable because that is what i'm
used to.

Please could you add a lot more comments. Given that nobody currently
actually does networking via ACPI, we have to assume everybody trying
to use it is a newbie, and more comments are better than less.

Thanks
Andrew

2022-06-22 10:47:28

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

śr., 22 cze 2022 o 11:24 Andrew Lunn <[email protected]> napisał(a):
>
> On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote:
> > wt., 21 cze 2022 o 13:42 Andy Shevchenko
> > <[email protected]> napisał(a):
> > >
> > > On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> > > > On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:
> > >
> > > ...
> > >
> > > > > > > + Name (_CRS, ResourceTemplate ()
> > > > > > > + {
> > > > > > > + Memory32Fixed (ReadWrite,
> > > > > > > + 0xf212a200,
> > > > > > > + 0x00000010,
> > > > > >
> > > > > > What do these magic numbers mean?
> > > > >
> > > > > Address + Length, it's all described in the ACPI specification.
> > > >
> > > > The address+plus length of what? This device is on an MDIO bus. As
> > > > such, there is no memory! It probably makes sense to somebody who
> > > > knows ACPI, but to me i have no idea what it means.
> > >
> > > I see what you mean. Honestly I dunno what the device this description is for.
> > > For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
> > > MDIOSerialBus() resources type is what would be good to have in ACPI
> > > specification.
> > >
> >
> > It's not device on MDIO bus, but the MDIO controller's register itself
>
> Ah. So this is equivalent to
>
> CP11X_LABEL(mdio): mdio@12a200 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "marvell,orion-mdio";
> reg = <0x12a200 0x10>;
> clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>,
> <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
> status = "disabled";
> };
>
> DT seems a lot more readable, "marvell,orion-mdio" is a good hint that
> device this is. But maybe it is more readable because that is what i'm
> used to.

No worries, this reaction is not uncommon (including myself), I agree
it becomes more readable, the longer you work with it :).

IMO the ACPI node of orion-mdio looks very similar. Please take a look:

Device (SMI0)
{
Name (_HID, "MRVL0100") // _HID: Hardware ID
Name (_UID, 0x00) // _UID: Unique ID
Method (_STA) // _STA: Device status
{
Return (0xF)
}
Name (_CRS, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
0xf212a200, // Address Base
0x00000010, // Address Length
)
})
}

You can "map" the objects/methods to what you know from DT farly easily:
_HID -> compatible string
_STA -> 'status' property
_CRS & Memory32Fixed -> 'reg' property (_CRS can also comprise IRQs
and other kind of resources, you can check [1] for more details).

Clocks are configured by firmware, so they are not referenced in the
tables and touched by the orion-mdio driver.

>
> Please could you add a lot more comments. Given that nobody currently
> actually does networking via ACPI, we have to assume everybody trying
> to use it is a newbie, and more comments are better than less.

I can add more verbose description of the example and probably a
reference to https://www.kernel.org/doc/Documentation/firmware-guide/acpi/dsd/phy.rst
("DSDT entry for MDIO node").

[1] https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#crs-current-resource-settings

Best regards,
Marcin

>
> Thanks
> Andrew

2022-06-22 10:48:51

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 12:22:23PM +0200, Marcin Wojtas wrote:
> śr., 22 cze 2022 o 11:24 Andrew Lunn <[email protected]> napisał(a):
> >
> > On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote:
> > > wt., 21 cze 2022 o 13:42 Andy Shevchenko
> > > <[email protected]> napisał(a):
> > > >
> > > > On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > > > > > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:
> > > >
> > > > ...
> > > >
> > > > > > > > + Name (_CRS, ResourceTemplate ()
> > > > > > > > + {
> > > > > > > > + Memory32Fixed (ReadWrite,
> > > > > > > > + 0xf212a200,
> > > > > > > > + 0x00000010,
> > > > > > >
> > > > > > > What do these magic numbers mean?
> > > > > >
> > > > > > Address + Length, it's all described in the ACPI specification.
> > > > >
> > > > > The address+plus length of what? This device is on an MDIO bus. As
> > > > > such, there is no memory! It probably makes sense to somebody who
> > > > > knows ACPI, but to me i have no idea what it means.
> > > >
> > > > I see what you mean. Honestly I dunno what the device this description is for.
> > > > For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
> > > > MDIOSerialBus() resources type is what would be good to have in ACPI
> > > > specification.
> > > >
> > >
> > > It's not device on MDIO bus, but the MDIO controller's register itself
> >
> > Ah. So this is equivalent to
> >
> > CP11X_LABEL(mdio): mdio@12a200 {
> > #address-cells = <1>;
> > #size-cells = <0>;
> > compatible = "marvell,orion-mdio";
> > reg = <0x12a200 0x10>;
> > clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>,
> > <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
> > status = "disabled";
> > };
> >
> > DT seems a lot more readable, "marvell,orion-mdio" is a good hint that
> > device this is. But maybe it is more readable because that is what i'm
> > used to.
>
> No worries, this reaction is not uncommon (including myself), I agree
> it becomes more readable, the longer you work with it :).
>
> IMO the ACPI node of orion-mdio looks very similar. Please take a look:
>
> Device (SMI0)
> {
> Name (_HID, "MRVL0100") // _HID: Hardware ID
> Name (_UID, 0x00) // _UID: Unique ID
> Method (_STA) // _STA: Device status
> {
> Return (0xF)
> }
> Name (_CRS, ResourceTemplate ()
> {
> Memory32Fixed (ReadWrite,
> 0xf212a200, // Address Base
> 0x00000010, // Address Length
> )
> })
> }
>
> You can "map" the objects/methods to what you know from DT farly easily:
> _HID -> compatible string

MRVL0100 is pretty meaningless, but marvell,orion-mdio gives you a
much better idea what the device is. That i would say is the key of
the problem here. Without knowing what MRVL0100 means, it is hard to
guess the rest.

Andrew

2022-06-22 11:10:35

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote:
> wt., 21 cze 2022 o 13:42 Andy Shevchenko
> <[email protected]> napisał(a):

...

> It's not device on MDIO bus, but the MDIO controller's register itself
> (this _CSR belongs to the parent, subnodes do not refer to it in any
> way). The child device requires only _ADR (or whatever else is needed
> for the case the DSA device is attached to SPI/I2C controllers).

More and more the idea of standardizing the MDIOSerialBus() resource looks
plausible. The _ADR() usage is a bit grey area in ACPI specification. Maybe
someone can also make it descriptive, so Microsoft and others won't utilize
_ADR() in any level of weirdness.

--
With Best Regards,
Andy Shevchenko

2022-06-22 11:13:36

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 12:37:46PM +0200, Andrew Lunn wrote:
> On Wed, Jun 22, 2022 at 12:22:23PM +0200, Marcin Wojtas wrote:

...

> MRVL0100 is pretty meaningless, but marvell,orion-mdio gives you a
> much better idea what the device is. That i would say is the key of
> the problem here. Without knowing what MRVL0100 means, it is hard to
> guess the rest.

As I pointed out they may use _DDN which will be part of the DSDT independenly
of the ACPICA version (`iasl` may add comments on the devices it knows) or any
other ACPI tools. The cons of this is the unused bulk in the ACPI tables,
meaning wasted space in firmware.

--
With Best Regards,
Andy Shevchenko

2022-06-22 11:40:32

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 11:24:07AM +0200, Andrew Lunn wrote:
> On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote:
> > wt., 21 cze 2022 o 13:42 Andy Shevchenko
> > <[email protected]> napisał(a):
> > > On Tue, Jun 21, 2022 at 01:18:38PM +0200, Andrew Lunn wrote:
> > > > On Tue, Jun 21, 2022 at 02:09:14PM +0300, Andy Shevchenko wrote:
> > > > > On Mon, Jun 20, 2022 at 09:47:31PM +0200, Andrew Lunn wrote:

...

> > > > > > > + Memory32Fixed (ReadWrite,
> > > > > > > + 0xf212a200,
> > > > > > > + 0x00000010,
> > > > > >
> > > > > > What do these magic numbers mean?
> > > > >
> > > > > Address + Length, it's all described in the ACPI specification.
> > > >
> > > > The address+plus length of what? This device is on an MDIO bus. As
> > > > such, there is no memory! It probably makes sense to somebody who
> > > > knows ACPI, but to me i have no idea what it means.
> > >
> > > I see what you mean. Honestly I dunno what the device this description is for.
> > > For the DSA that's behind MDIO bus? Then it's definitely makes no sense and
> > > MDIOSerialBus() resources type is what would be good to have in ACPI
> > > specification.
> >
> > It's not device on MDIO bus, but the MDIO controller's register itself
>
> Ah. So this is equivalent to
>
> CP11X_LABEL(mdio): mdio@12a200 {
> #address-cells = <1>;
> #size-cells = <0>;
> compatible = "marvell,orion-mdio";
> reg = <0x12a200 0x10>;
> clocks = <&CP11X_LABEL(clk) 1 9>, <&CP11X_LABEL(clk) 1 5>,
> <&CP11X_LABEL(clk) 1 6>, <&CP11X_LABEL(clk) 1 18>;
> status = "disabled";
> };
>
> DT seems a lot more readable, "marvell,orion-mdio" is a good hint that
> device this is. But maybe it is more readable because that is what i'm
> used to.

In ACPI we have _HID and _DDN. _DDN may put a descriptive string.

> Please could you add a lot more comments. Given that nobody currently
> actually does networking via ACPI, we have to assume everybody trying
> to use it is a newbie, and more comments are better than less.

--
With Best Regards,
Andy Shevchenko

2022-06-22 11:42:45

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

> > It's not device on MDIO bus, but the MDIO controller's register itself
> > (this _CSR belongs to the parent, subnodes do not refer to it in any
> > way). The child device requires only _ADR (or whatever else is needed
> > for the case the DSA device is attached to SPI/I2C controllers).
>
> More and more the idea of standardizing the MDIOSerialBus() resource looks
> plausible. The _ADR() usage is a bit grey area in ACPI specification. Maybe
> someone can also make it descriptive, so Microsoft and others won't utilize
> _ADR() in any level of weirdness.

I don't know if it makes any difference, but there are two protocols
spoken over MDIO, c22 and c45, specified in clause 22 and clause 45 of
the 802.3 specification. In some conditions, you need to specify which
protocol to speak to a device at a particular address. In DT we
indicate this with the compatible string, when maybe it should really
be considered as an extension of the address.

If somebody does produce a draft for MDIOSerialBus() i'm happy to
review it.

Andrew

2022-06-22 14:44:09

[permalink] [raw]

Subject: Re: [net-next: PATCH 09/12] Documentation: ACPI: DSD: introduce DSA description

On Wed, Jun 22, 2022 at 01:22:15PM +0200, Andrew Lunn wrote:
> > > It's not device on MDIO bus, but the MDIO controller's register itself
> > > (this _CSR belongs to the parent, subnodes do not refer to it in any
> > > way). The child device requires only _ADR (or whatever else is needed
> > > for the case the DSA device is attached to SPI/I2C controllers).
> >
> > More and more the idea of standardizing the MDIOSerialBus() resource looks
> > plausible. The _ADR() usage is a bit grey area in ACPI specification. Maybe
> > someone can also make it descriptive, so Microsoft and others won't utilize
> > _ADR() in any level of weirdness.
>
> I don't know if it makes any difference, but there are two protocols
> spoken over MDIO, c22 and c45, specified in clause 22 and clause 45 of
> the 802.3 specification. In some conditions, you need to specify which
> protocol to speak to a device at a particular address. In DT we
> indicate this with the compatible string, when maybe it should really
> be considered as an extension of the address.
>
> If somebody does produce a draft for MDIOSerialBus() i'm happy to
> review it.

I also can review it. Marcin, would it be hard for you to prepare a formal
proposal for ACPI specification?

--
With Best Regards,
Andy Shevchenko

2022-06-22 15:54:45