| Message ID | 20220620150225.1307946-10-mw@semihalf.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI support for DSA | expand |
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
> +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
pon., 20 cze 2022 o 20:19 Andrew Lunn <andrew@lunn.ch> 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
pon., 20 cze 2022 o 21:47 Andrew Lunn <andrew@lunn.ch> 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
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 <mw@semihalf.com> > --- > 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
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.
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.
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
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
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.
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.
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
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.
On Tue, Jun 21, 2022 at 3:28 PM Sudeep Holla <sudeep.holla@arm.com> 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.
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 <sudeep.holla@arm.com> 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
On Tue, Jun 21, 2022 at 5:37 PM Sudeep Holla <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> 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.
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 <sudeep.holla@arm.com> 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 <sudeep.holla@arm.com> 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
wt., 21 cze 2022 o 13:42 Andy Shevchenko <andriy.shevchenko@linux.intel.com> 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
On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote: > wt., 21 cze 2022 o 13:42 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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
śr., 22 cze 2022 o 11:24 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote: > > wt., 21 cze 2022 o 13:42 Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> 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
On Wed, Jun 22, 2022 at 12:22:23PM +0200, Marcin Wojtas wrote: > śr., 22 cze 2022 o 11:24 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote: > > > wt., 21 cze 2022 o 13:42 Andy Shevchenko > > > <andriy.shevchenko@linux.intel.com> 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
On Wed, Jun 22, 2022 at 11:08:13AM +0200, Marcin Wojtas wrote: > wt., 21 cze 2022 o 13:42 Andy Shevchenko > <andriy.shevchenko@linux.intel.com> 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.
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 > > <andriy.shevchenko@linux.intel.com> 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.
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.
> > 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
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?
śr., 22 cze 2022 o 16:20 Andy Shevchenko <andriy.shevchenko@linux.intel.com> napisał(a): > > 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? > I've just consulted this to get an understanding of the process. * I will initiate it with the code-first ECR using the linux-acpi mailing list, where all the technical review will take place. * At the same time a ticket and the formal process for this will be triggered within UEFI Forum. * Once everything gets approved, an official confirmation will be provided and from that moment, it would allow us to proceed with implementation without need of waiting months for another ACPI Specification release. Unless anyone objects, I will include this thread recipients to take part in review of the proposed MDIOSerialBus _CRS resource macro contents, so it contains all relevant information. Note: Once this hopefully gets accepted one day and allow us proceed with Linux handling, it should be easy to satisfy backward compatibility with current users of MDIO+PHY in ACPI. Best regards, Marcin
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 @@ -0,0 +1,359 @@ +.. SPDX-License-Identifier: GPL-2.0 + +=========== +DSA in ACPI +=========== + +The **Distributed Switch Architecture (DSA)** devices on an MDIO bus [dsa] +are enumerated using fwnode_mdiobus_register_device() and later probed +by a dedicated driver based on the ACPI ID match result. + +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. + +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, + ) + }) + Device (SWI0) + { + Name (_HID, "MRVL0120") + Name (_UID, 0x00) + Name (_ADR, 0x4) + <...> + } + } + +Switch MDIO bus +=============== + +A switch internal MDIO bus, please refer to 'MDIO bus and PHYs in ACPI' [phy] +document for more details. Its name must be set to **MDIO** for proper +enumeration by net/dsa API. + +Switch MDIO bus declaration example: +------------------------------------ + +.. code-block:: none + + Scope (\_SB.SMI0.SWI0) + { + Name (_HID, "MRVL0120") + Name (_UID, 0x00) + Name (_ADR, 0x4) + Device (MDIO) { + Name (_ADR, 0x0) + Device (S0P0) + { + Name (_ADR, 0x11) + } + Device (S0P1) + { + Name (_ADR, 0x12) + } + Device (S0P2) + { + Name (_ADR, 0x13) + } + Device (S0P3) + { + Name (_ADR, 0x14) + } + } + <...> + } + +Switch ports +============ + +The ports must be grouped under **PRTS** switch child device. They +should comprise a _ADR field with a port enumerator [adr] and +other properties in a standard _DSD object [dsa-properties]. + +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". + +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. +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. + +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]. +See [phy] for more details. + +Switch ports' description example: +---------------------------------- + +.. code-block:: none + + Scope (\_SB.SMI0.SWI0) + { + Name (_HID, "MRVL0120") + Name (_UID, 0x00) + Name (_ADR, 0x4) + Device (PRTS) { + Name (_ADR, 0x0) + Device (PRT1) + { + Name (_ADR, 0x1) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan2"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P0}, + } + }) + } + Device (PRT2) + { + Name (_ADR, 0x2) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan1"}, + }, + ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), + Package () { + Package () {"fixed-link", "LNK0"} + } + }) + Name (LNK0, Package(){ // Data-only subnode of port + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () {"speed", 1000}, + Package () {"full-duplex", 1} + } + }) + } + Device (PRT3) + { + Name (_ADR, 0x3) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan4"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P2}, + } + }) + } + Device (PRT4) + { + Name (_ADR, 0x4) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan3"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P3}, + } + }) + } + Device (PRT5) + { + Name (_ADR, 0x5) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "cpu"}, + Package () { "ethernet", \_SB.PP20.ETH2}, + } + }) + } + } + <...> + } + +Full DSA description example +============================ + +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. + +.. code-block:: none + + Scope (\_SB.SMI0) + { + Name (_HID, "MRVL0100") + Name (_UID, 0x00) + Method (_STA) + { + Return (0xF) + } + Name (_CRS, ResourceTemplate () + { + Memory32Fixed (ReadWrite, + 0xf212a200, + 0x00000010, + ) + }) + Device (PHY0) + { + Name (_ADR, 0x0) + } + Device (SWI0) + { + Name (_HID, "MRVL0120") + Name (_UID, 0x00) + Name (_ADR, 0x4) + Device (PRTS) { + Name (_ADR, 0x0) + Device (PRT1) + { + Name (_ADR, 0x1) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan2"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P0}, + } + }) + } + Device (PRT2) + { + Name (_ADR, 0x2) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan1"}, + }, + ToUUID("dbb8e3e6-5886-4ba6-8795-1319f52a966b"), + Package () { + Package () {"fixed-link", "LNK0"} + } + }) + Name (LNK0, Package(){ // Data-only subnode of port + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () {"speed", 1000}, + Package () {"full-duplex", 1} + } + }) + } + Device (PRT3) + { + Name (_ADR, 0x3) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan4"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P2}, + } + }) + } + Device (PRT4) + { + Name (_ADR, 0x4) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "lan3"}, + Package () { "phy-handle", \_SB.SMI0.SWI0.MDIO.S0P3}, + } + }) + } + Device (PRT5) + { + Name (_ADR, 0x5) + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () { "label", "cpu"}, + Package () { "ethernet", \_SB.PP20.ETH2}, + } + }) + } + } + Device (MDIO) { + Name (_ADR, 0x0) + Device (S0P0) + { + Name (_ADR, 0x11) + } + Device (S0P2) + { + Name (_ADR, 0x13) + } + Device (S0P3) + { + Name (_ADR, 0x14) + } + } + } + } + +TODO +==== + +* Add support for cascade switch connections via port's 'link' property [dsa-properties]. + +References +========== + +[adr] ACPI Specifications, Version 6.4 - Paragraph 6.1.1 _ADR Address + https://uefi.org/specs/ACPI/6.4/06_Device_Configuration/Device_Configuration.html#adr-address + +[dsa] + Documentation/networking/dsa/dsa.rst + +[dsa-properties] + Documentation/devicetree/bindings/net/dsa/dsa-port.yaml + +[dsd-guide] DSD Guide. + https://github.com/UEFI/DSD-Guide/blob/main/dsd-guide.adoc, referenced + 2022-06-20. + +[dsd-properties-rules] + Documentation/firmware-guide/acpi/DSD-properties-rules.rst + +[ethernet-controller] + Documentation/devicetree/bindings/net/ethernet-controller.yaml + +[phy] Documentation/networking/phy.rst diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst index b6a42f4ffe03..a6ed5ba90cdd 100644 --- a/Documentation/firmware-guide/acpi/index.rst +++ b/Documentation/firmware-guide/acpi/index.rst @@ -10,6 +10,7 @@ ACPI Support namespace dsd/graph dsd/data-node-references + dsd/dsa dsd/leds dsd/phy enumeration
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 <mw@semihalf.com> --- 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