| Message ID | 1513588684-15647-1-git-send-email-mw@semihalf.com |
|---|---|
| Headers | show |
| Series | Armada 7k/8k PP2 ACPI support | expand |
On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote: > Hi, > > This patchset introduces ACPI support in mvpp2 and mvmdio drivers. > First three patches introduce fwnode helpers for obtaining PHY > information from nodes and also MDIO fwnode API for registering > the bus with its PHY/devices. > > Following patches update code of the mvmdio and mvpp2 drivers > to support ACPI tables handling. The latter is done in 4 steps, > as can be seen in the commits. For the details, please > refer to the commit messages. > > Drivers operation was tested on top of the net-next branch > with both DT and ACPI. Although for compatibility reasons > with older platforms, the mvmdio driver keeps using > of_ MDIO registering, new fwnode_ one proved to fully work > with DT as well (tested on MacchiatoBin board). > > mvpp2/mvmdio driver can work with the ACPI representation, as exposed > on a public branch: > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip > It was compiled together with the most recent Tianocore EDK2 revision. > Please refer to the firmware build instruction on MacchiatoBin board: > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II > > Above support configures 1G to use its PHY normally. 10G can work now > only with the link interrupt mode. Somehow reading of the > string property in fwnode_mdiobus_child_is_phy works only with > DT and cannot cope with 10G PHY nodes as in: > https://pastebin.com/3JnYpU0A > > Above root cause will be further checked. In the meantime I will > appreciate any comments or remarks for the kernel patches. > Hi Marcin, I have added linux-acpi and Graeme to cc. I think it makes sense to discuss the way you describe the device topology before looking at the patches in more detail. In particular, I would like to request feedback on the use of [redundant] 'reg' properties and the use of _DSD + compatible to describe PHYs. Usually, we try to avoid this, given that it is essentially a ACPI encapsulated DT dialect that is difficult to support in drivers unless they are based on DT to begin with. Also, non-standard _DSD properties require a vendor prefix, it is not freeform. For reference, the ACPI description is here (starting at line 175) https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
On 12/18/2017 10:17 AM, Marcin Wojtas wrote: > Hi, > > This patchset introduces ACPI support in mvpp2 and mvmdio drivers. > First three patches introduce fwnode helpers for obtaining PHY > information from nodes and also MDIO fwnode API for registering > the bus with its PHY/devices. > > Following patches update code of the mvmdio and mvpp2 drivers > to support ACPI tables handling. The latter is done in 4 steps, > as can be seen in the commits. For the details, please > refer to the commit messages. > > Drivers operation was tested on top of the net-next branch > with both DT and ACPI. Although for compatibility reasons > with older platforms, the mvmdio driver keeps using > of_ MDIO registering, new fwnode_ one proved to fully work > with DT as well (tested on MacchiatoBin board). > > mvpp2/mvmdio driver can work with the ACPI representation, as exposed > on a public branch: > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip > It was compiled together with the most recent Tianocore EDK2 revision. > Please refer to the firmware build instruction on MacchiatoBin board: > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II > > Above support configures 1G to use its PHY normally. 10G can work now > only with the link interrupt mode. Somehow reading of the > string property in fwnode_mdiobus_child_is_phy works only with > DT and cannot cope with 10G PHY nodes as in: > https://pastebin.com/3JnYpU0A > > Above root cause will be further checked. In the meantime I will > appreciate any comments or remarks for the kernel patches. > > Best regards, > Marcin > > Marcin Wojtas (8): > device property: Introduce fwnode_get_mac_address() > device property: Introduce fwnode_get_phy_mode() > mdio_bus: Introduce fwnode MDIO helpers > net: mvmdio: add ACPI support > net: mvpp2: simplify maintaining enabled ports' list > net: mvpp2: use device_*/fwnode_* APIs instead of of_* > net: mvpp2: handle PHY with its fwnode > net: mvpp2: enable ACPI support in the driver > > drivers/base/property.c | 52 +++-- > drivers/net/ethernet/marvell/mvmdio.c | 42 +++- > drivers/net/ethernet/marvell/mvpp2.c | 246 ++++++++++++-------- > drivers/net/phy/mdio_bus.c | 218 +++++++++++++++++ > include/linux/mdio.h | 3 + > include/linux/property.h | 3 + > 6 files changed, 454 insertions(+), 110 deletions(-) > Please CC linux-acpi on all submissions of patches touching ACPI, property.c or property.h. Thanks!
Hi Ard 2017-12-18 10:40 GMT+01:00 Ard Biesheuvel <ard.biesheuvel@linaro.org>: > On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote: >> Hi, >> >> This patchset introduces ACPI support in mvpp2 and mvmdio drivers. >> First three patches introduce fwnode helpers for obtaining PHY >> information from nodes and also MDIO fwnode API for registering >> the bus with its PHY/devices. >> >> Following patches update code of the mvmdio and mvpp2 drivers >> to support ACPI tables handling. The latter is done in 4 steps, >> as can be seen in the commits. For the details, please >> refer to the commit messages. >> >> Drivers operation was tested on top of the net-next branch >> with both DT and ACPI. Although for compatibility reasons >> with older platforms, the mvmdio driver keeps using >> of_ MDIO registering, new fwnode_ one proved to fully work >> with DT as well (tested on MacchiatoBin board). >> >> mvpp2/mvmdio driver can work with the ACPI representation, as exposed >> on a public branch: >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip >> It was compiled together with the most recent Tianocore EDK2 revision. >> Please refer to the firmware build instruction on MacchiatoBin board: >> http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II >> >> Above support configures 1G to use its PHY normally. 10G can work now >> only with the link interrupt mode. Somehow reading of the >> string property in fwnode_mdiobus_child_is_phy works only with >> DT and cannot cope with 10G PHY nodes as in: >> https://pastebin.com/3JnYpU0A >> >> Above root cause will be further checked. In the meantime I will >> appreciate any comments or remarks for the kernel patches. >> > > Hi Marcin, > > I have added linux-acpi and Graeme to cc. I think it makes sense to > discuss the way you describe the device topology before looking at the > patches in more detail. > Thanks. Tomasz Nowicki immediately pointed this to me off the lists. > In particular, I would like to request feedback on the use of > [redundant] 'reg' properties and the use of _DSD + compatible to > describe PHYs. Usually, we try to avoid this, given that it is > essentially a ACPI encapsulated DT dialect that is difficult to > support in drivers unless they are based on DT to begin with. Also, > non-standard _DSD properties require a vendor prefix, it is not > freeform. > Already a lot of drivers use both DT + ACPI. Some have IMO very fanciful bindings in both, mostly handled within the drivers with custom functions. OF_ - only drivers can use of_mdio_ helper routines, that assume a certain hierarchy: MDIO device node with PHYs as children, which are referenced in the ports node. I believe such approach could fit ACPI description too. I'm aware however that my code is pretty much DT transposed into ACPI environment and I'm of course open to discussion, how to do it in the most proper way. My main goal is to provide an fwnode-based glue code, that can be used among the NIC/MDIO drivers (+ phylink) without multiple ifs differentiating between ACPI/OF. What I sent has single calls in mvpp2/mvmdio with a common bottom layers, but I don't see a problem, that, e.g. when iterating over PHY subnodes, in case of OF 'reg'/'compatible' are used, whereas with ACPI some specific _ADR/_CID objects. I'd appreaciate any feedback. Best regards, Marcin > For reference, the ACPI description is here (starting at line 175) > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl
From: Marcin Wojtas <mw@semihalf.com> Date: Mon, 18 Dec 2017 10:17:56 +0100 > Above support configures 1G to use its PHY normally. 10G can work now > only with the link interrupt mode. Somehow reading of the > string property in fwnode_mdiobus_child_is_phy works only with > DT and cannot cope with 10G PHY nodes as in: > https://pastebin.com/3JnYpU0A > > Above root cause will be further checked. In the meantime I will > appreciate any comments or remarks for the kernel patches. I would like you to figure this out before these changes go in. Thanks.
Hi David, 2017-12-19 19:48 GMT+01:00 David Miller <davem@davemloft.net>: > From: Marcin Wojtas <mw@semihalf.com> > Date: Mon, 18 Dec 2017 10:17:56 +0100 > >> Above support configures 1G to use its PHY normally. 10G can work now >> only with the link interrupt mode. Somehow reading of the >> string property in fwnode_mdiobus_child_is_phy works only with >> DT and cannot cope with 10G PHY nodes as in: >> https://pastebin.com/3JnYpU0A >> >> Above root cause will be further checked. In the meantime I will >> appreciate any comments or remarks for the kernel patches. > > I would like you to figure this out before these changes go in. > Of course! v2 will not have such problem, I've been waiting however for the feedback about the ACPI representation. Anyway, I'm strongly leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so maybe I'll just issue the v2 in order to push the discussion a bit forward. Thanks, Marcin
> Of course! v2 will not have such problem, I've been waiting however > for the feedback about the ACPI representation. Anyway, I'm strongly > leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so > maybe I'll just issue the v2 in order to push the discussion a bit > forward. Hi Marcin I know ~0 about ACPI. But what seems to be missing for me is documentation. You are defining a ABI here, which all future MDIO busses, PHYs, and to some extent Ethernet switches need to follow. So i would expect this to be documented somewhere. How does documentation work in the ACPI world? Andrew
Hi Andrew, 2017-12-19 21:46 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: >> Of course! v2 will not have such problem, I've been waiting however >> for the feedback about the ACPI representation. Anyway, I'm strongly >> leaning towards using _ADR/_CID objects in PHY's nodes for ACPI, so >> maybe I'll just issue the v2 in order to push the discussion a bit >> forward. > > Hi Marcin > > I know ~0 about ACPI. But what seems to be missing for me is > documentation. You are defining a ABI here, which all future MDIO > busses, PHYs, and to some extent Ethernet switches need to follow. So > i would expect this to be documented somewhere. As the code derives straight from of_mdio.c, there is absolutely no deviation from Documentation/devicetree/bindings/net/mdio.txt. > > How does documentation work in the ACPI world? > ACPI has its own specification, the newest one is 6.2 http://www.uefi.org/sites/default/files/resources/ACPI_6_2.pdf It is pretty likely, that if we establish a generic solution for the MDIO bus, a short section about it may probably require adding to above - for now I see none. Best regards, Marcin
Hi Marcin, On Mon, Dec 18, 2017 at 10:17:56AM +0100, Marcin Wojtas wrote: > > Marcin Wojtas (8): > device property: Introduce fwnode_get_mac_address() > device property: Introduce fwnode_get_phy_mode() > mdio_bus: Introduce fwnode MDIO helpers > net: mvmdio: add ACPI support > net: mvpp2: simplify maintaining enabled ports' list > net: mvpp2: use device_*/fwnode_* APIs instead of of_* > net: mvpp2: handle PHY with its fwnode > net: mvpp2: enable ACPI support in the driver I tested your series on a mcbin, using the dt way. It still worked. If it is relevant, you can add on the mvpp2 related patches: Tested-by: Antoine Tenart <antoine.tenart@free-electrons.com> Thanks! Antoine
On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote: > On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote: > > Hi, > > > > This patchset introduces ACPI support in mvpp2 and mvmdio drivers. > > First three patches introduce fwnode helpers for obtaining PHY > > information from nodes and also MDIO fwnode API for registering > > the bus with its PHY/devices. > > > > Following patches update code of the mvmdio and mvpp2 drivers > > to support ACPI tables handling. The latter is done in 4 steps, > > as can be seen in the commits. For the details, please > > refer to the commit messages. > > > > Drivers operation was tested on top of the net-next branch > > with both DT and ACPI. Although for compatibility reasons > > with older platforms, the mvmdio driver keeps using > > of_ MDIO registering, new fwnode_ one proved to fully work > > with DT as well (tested on MacchiatoBin board). > > > > mvpp2/mvmdio driver can work with the ACPI representation, as exposed > > on a public branch: > > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip > > It was compiled together with the most recent Tianocore EDK2 revision. > > Please refer to the firmware build instruction on MacchiatoBin board: > > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II > > > > Above support configures 1G to use its PHY normally. 10G can work now > > only with the link interrupt mode. Somehow reading of the > > string property in fwnode_mdiobus_child_is_phy works only with > > DT and cannot cope with 10G PHY nodes as in: > > https://pastebin.com/3JnYpU0A > > > > Above root cause will be further checked. In the meantime I will > > appreciate any comments or remarks for the kernel patches. > > > > Hi Marcin, > > I have added linux-acpi and Graeme to cc. I think it makes sense to > discuss the way you describe the device topology before looking at the > patches in more detail. > > In particular, I would like to request feedback on the use of > [redundant] 'reg' properties and the use of _DSD + compatible to > describe PHYs. Usually, we try to avoid this, given that it is > essentially a ACPI encapsulated DT dialect that is difficult to > support in drivers unless they are based on DT to begin with. Also, > non-standard _DSD properties require a vendor prefix, it is not > freeform. > > For reference, the ACPI description is here (starting at line 175) > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl > So the representation of PHY's with _DSD was kind of formalised here http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf This is already in use in the kernel, and that DSDT seems to be along the same lines. So seems ok in that manner. The "reg", 0 property seems a little odd, it would probably make more sense to check for the lack of ranges passed in in ACPI manner _CRS. Graeme
Hi Graeme, 2018-01-03 12:00 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>: > On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote: >> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote: >> > Hi, >> > >> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers. >> > First three patches introduce fwnode helpers for obtaining PHY >> > information from nodes and also MDIO fwnode API for registering >> > the bus with its PHY/devices. >> > >> > Following patches update code of the mvmdio and mvpp2 drivers >> > to support ACPI tables handling. The latter is done in 4 steps, >> > as can be seen in the commits. For the details, please >> > refer to the commit messages. >> > >> > Drivers operation was tested on top of the net-next branch >> > with both DT and ACPI. Although for compatibility reasons >> > with older platforms, the mvmdio driver keeps using >> > of_ MDIO registering, new fwnode_ one proved to fully work >> > with DT as well (tested on MacchiatoBin board). >> > >> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed >> > on a public branch: >> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip >> > It was compiled together with the most recent Tianocore EDK2 revision. >> > Please refer to the firmware build instruction on MacchiatoBin board: >> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II >> > >> > Above support configures 1G to use its PHY normally. 10G can work now >> > only with the link interrupt mode. Somehow reading of the >> > string property in fwnode_mdiobus_child_is_phy works only with >> > DT and cannot cope with 10G PHY nodes as in: >> > https://pastebin.com/3JnYpU0A >> > >> > Above root cause will be further checked. In the meantime I will >> > appreciate any comments or remarks for the kernel patches. >> > >> >> Hi Marcin, >> >> I have added linux-acpi and Graeme to cc. I think it makes sense to >> discuss the way you describe the device topology before looking at the >> patches in more detail. >> >> In particular, I would like to request feedback on the use of >> [redundant] 'reg' properties and the use of _DSD + compatible to >> describe PHYs. Usually, we try to avoid this, given that it is >> essentially a ACPI encapsulated DT dialect that is difficult to >> support in drivers unless they are based on DT to begin with. Also, >> non-standard _DSD properties require a vendor prefix, it is not >> freeform. >> >> For reference, the ACPI description is here (starting at line 175) >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl >> > So the representation of PHY's with _DSD was kind of formalised here > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > This is already in use in the kernel, and that DSDT seems to be along the same > lines. So seems ok in that manner. > > The "reg", 0 property seems a little odd, it would probably make more > sense to check for the lack of ranges passed in in ACPI manner _CRS. > I already agreed with 'reg' being awkward in the later emails. Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? Thanks, Marcin
> I already agreed with 'reg' being awkward in the later emails. > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? Also, how do you specify which MDIO bus the PHY is on. To fully specify a PHY, you need both bits of information. In DT, the phy-handle phandle can point to any PHY anywhere in the system. This is particularly important when a Ethernet device has two MDIO busses. Andrew
Hi Andrew, 2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: >> I already agreed with 'reg' being awkward in the later emails. >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > Also, how do you specify which MDIO bus the PHY is on. To fully > specify a PHY, you need both bits of information. > > In DT, the phy-handle phandle can point to any PHY anywhere in the > system. This is particularly important when a Ethernet device has two > MDIO busses. > For now, my local MDIO bus description is pretty DT-like, i.e. master bus with children PHYs: Device (MDIO) { Name (_HID, "MRVL0100") // _HID: Hardware ID Name (_UID, 0x00) // _UID: Unique ID Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xf212a200, // Address Base 0x00000010, // Address Length ) }) Device (GPHY) { Name (_ADR, 0x0) } } Device (XSMI) { Name (_HID, "MRVL0101") // _HID: Hardware ID Name (_UID, 0x00) // _UID: Unique ID Name (_CRS, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xf212a600, // Address Base 0x00000010, // Address Length ) }) Device (PHY0) { Name (_ADR, 0x0) Name (_CID, "ethernet-phy-ieee802.3-c45") } Device (PHY8) { Name (_ADR, 0x8) Name (_CID, "ethernet-phy-ieee802.3-c45") } } Which is referenced in the port's node: Package () { "phy", Package (){\_SB.XSMI.PHY0}}, I'm studying an alternatives with graphs, as suggested by Tomasz Nowicki, but to me above is pretty natural and not complicated. Best regards, Marcin
On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote: > Hi Andrew, > > 2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > >> I already agreed with 'reg' being awkward in the later emails. > >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > > > Also, how do you specify which MDIO bus the PHY is on. To fully > > specify a PHY, you need both bits of information. > > > > In DT, the phy-handle phandle can point to any PHY anywhere in the > > system. This is particularly important when a Ethernet device has two > > MDIO busses. > > > > For now, my local MDIO bus description is pretty DT-like, i.e. master > bus with children PHYs: > Device (MDIO) > { > Name (_HID, "MRVL0100") // > _HID: Hardware ID > Name (_UID, 0x00) // > _UID: Unique ID > Name (_CRS, ResourceTemplate () > { > Memory32Fixed (ReadWrite, > 0xf212a200, // Address Base > 0x00000010, // > Address Length > ) > }) > Device (GPHY) > { > Name (_ADR, 0x0) > } > } > > Device (XSMI) > { > Name (_HID, "MRVL0101") // > _HID: Hardware ID > Name (_UID, 0x00) // > _UID: Unique ID > Name (_CRS, ResourceTemplate () > { > Memory32Fixed (ReadWrite, > 0xf212a600, // Address Base > 0x00000010, // > Address Length > ) > }) > Device (PHY0) > { > Name (_ADR, 0x0) > Name (_CID, "ethernet-phy-ieee802.3-c45") > } > Device (PHY8) > { > Name (_ADR, 0x8) > Name (_CID, "ethernet-phy-ieee802.3-c45") > } > } > > Which is referenced in the port's node: > > Package () { "phy", Package (){\_SB.XSMI.PHY0}}, Hi Marcin This reference looks good, giving both the bus and the PHY on the bus. I assume you can use references like this within the Device (PHY8) node? You need to be able to reference a GPIO used for resetting the PHY. And you also need to reference a GPIO at the Device (MDIO) level for resetting all the PHYs on the MDIO bus. Andrew
2018-01-03 14:33 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > On Wed, Jan 03, 2018 at 02:13:09PM +0100, Marcin Wojtas wrote: >> Hi Andrew, >> >> 2018-01-03 13:47 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: >> >> I already agreed with 'reg' being awkward in the later emails. >> >> Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? >> > >> > Also, how do you specify which MDIO bus the PHY is on. To fully >> > specify a PHY, you need both bits of information. >> > >> > In DT, the phy-handle phandle can point to any PHY anywhere in the >> > system. This is particularly important when a Ethernet device has two >> > MDIO busses. >> > >> >> For now, my local MDIO bus description is pretty DT-like, i.e. master >> bus with children PHYs: >> Device (MDIO) >> { >> Name (_HID, "MRVL0100") // >> _HID: Hardware ID >> Name (_UID, 0x00) // >> _UID: Unique ID >> Name (_CRS, ResourceTemplate () >> { >> Memory32Fixed (ReadWrite, >> 0xf212a200, // Address Base >> 0x00000010, // >> Address Length >> ) >> }) >> Device (GPHY) >> { >> Name (_ADR, 0x0) >> } >> } >> >> Device (XSMI) >> { >> Name (_HID, "MRVL0101") // >> _HID: Hardware ID >> Name (_UID, 0x00) // >> _UID: Unique ID >> Name (_CRS, ResourceTemplate () >> { >> Memory32Fixed (ReadWrite, >> 0xf212a600, // Address Base >> 0x00000010, // >> Address Length >> ) >> }) >> Device (PHY0) >> { >> Name (_ADR, 0x0) >> Name (_CID, "ethernet-phy-ieee802.3-c45") >> } >> Device (PHY8) >> { >> Name (_ADR, 0x8) >> Name (_CID, "ethernet-phy-ieee802.3-c45") >> } >> } >> >> Which is referenced in the port's node: >> >> Package () { "phy", Package (){\_SB.XSMI.PHY0}}, > > Hi Marcin > > This reference looks good, giving both the bus and the PHY on the bus. > > I assume you can use references like this within the Device (PHY8) > node? Yes. > You need to be able to reference a GPIO used for resetting the > PHY. And you also need to reference a GPIO at the Device (MDIO) level > for resetting all the PHYs on the MDIO bus. > Yes, for full support of PHYs the GPIO must be supported, as well as the PHY's IRQs. Best regards, Marcin
On Wed, Jan 03, 2018 at 12:12:06PM +0100, Marcin Wojtas wrote: > Hi Graeme, > > 2018-01-03 12:00 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>: > > On Mon, Dec 18, 2017 at 10:40:31AM +0100, Ard Biesheuvel wrote: > >> On 18 December 2017 at 10:17, Marcin Wojtas <mw@semihalf.com> wrote: > >> > Hi, > >> > > >> > This patchset introduces ACPI support in mvpp2 and mvmdio drivers. > >> > First three patches introduce fwnode helpers for obtaining PHY > >> > information from nodes and also MDIO fwnode API for registering > >> > the bus with its PHY/devices. > >> > > >> > Following patches update code of the mvmdio and mvpp2 drivers > >> > to support ACPI tables handling. The latter is done in 4 steps, > >> > as can be seen in the commits. For the details, please > >> > refer to the commit messages. > >> > > >> > Drivers operation was tested on top of the net-next branch > >> > with both DT and ACPI. Although for compatibility reasons > >> > with older platforms, the mvmdio driver keeps using > >> > of_ MDIO registering, new fwnode_ one proved to fully work > >> > with DT as well (tested on MacchiatoBin board). > >> > > >> > mvpp2/mvmdio driver can work with the ACPI representation, as exposed > >> > on a public branch: > >> > https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/commits/marvell-armada-wip > >> > It was compiled together with the most recent Tianocore EDK2 revision. > >> > Please refer to the firmware build instruction on MacchiatoBin board: > >> > http://wiki.macchiatobin.net/tiki-index.php?page=Build+from+source+-+UEFI+EDK+II > >> > > >> > Above support configures 1G to use its PHY normally. 10G can work now > >> > only with the link interrupt mode. Somehow reading of the > >> > string property in fwnode_mdiobus_child_is_phy works only with > >> > DT and cannot cope with 10G PHY nodes as in: > >> > https://pastebin.com/3JnYpU0A > >> > > >> > Above root cause will be further checked. In the meantime I will > >> > appreciate any comments or remarks for the kernel patches. > >> > > >> > >> Hi Marcin, > >> > >> I have added linux-acpi and Graeme to cc. I think it makes sense to > >> discuss the way you describe the device topology before looking at the > >> patches in more detail. > >> > >> In particular, I would like to request feedback on the use of > >> [redundant] 'reg' properties and the use of _DSD + compatible to > >> describe PHYs. Usually, we try to avoid this, given that it is > >> essentially a ACPI encapsulated DT dialect that is difficult to > >> support in drivers unless they are based on DT to begin with. Also, > >> non-standard _DSD properties require a vendor prefix, it is not > >> freeform. > >> > >> For reference, the ACPI description is here (starting at line 175) > >> https://github.com/MarvellEmbeddedProcessors/edk2-open-platform/blob/72d5ac23b20dd74d479daa5e40ba443264b31261/Platforms/Marvell/Armada/AcpiTables/Armada80x0McBin/Dsdt.asl > >> > > So the representation of PHY's with _DSD was kind of formalised here > > > > http://www.uefi.org/sites/default/files/resources/nic-request-v2.pdf > > > > This is already in use in the kernel, and that DSDT seems to be along the same > > lines. So seems ok in that manner. > > > > The "reg", 0 property seems a little odd, it would probably make more > > sense to check for the lack of ranges passed in in ACPI manner _CRS. > > > > I already agreed with 'reg' being awkward in the later emails. > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > Ah it is an actual address, then yes _ADR is probably more appropriate. Graeme
> > I already agreed with 'reg' being awkward in the later emails. > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > > Ah it is an actual address, then yes _ADR is probably more appropriate. Newbie ACPI question. What is the definition of an address? In this cause, we are talking about an address of a device on an MDIO bus. It takes a value between 0 and 31. How are IC2 device addresses represented in ACPI? MDIO devices and I2C devices are pretty similar. So it would make sense to use the same as what I2C uses. Andrew
On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote: > > > I already agreed with 'reg' being awkward in the later emails. > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > > > > Ah it is an actual address, then yes _ADR is probably more appropriate. > > Newbie ACPI question. What is the definition of an address? > > In this cause, we are talking about an address of a device on an MDIO > bus. It takes a value between 0 and 31. > > How are IC2 device addresses represented in ACPI? MDIO devices and I2C > devices are pretty similar. So it would make sense to use the same as > what I2C uses. > Too big (and has table) to sensibly quote, but defined in ACPI spec 6.1.1 _ADR (Address) Ive never though been quite sure if that is just an example list of address types or its supposed to be canonical (in which case some ECRs are needed to the spec). Thanks Graeme
On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote: > On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote: > > > > I already agreed with 'reg' being awkward in the later emails. > > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > > > > > > Ah it is an actual address, then yes _ADR is probably more appropriate. > > > > Newbie ACPI question. What is the definition of an address? > > > > In this cause, we are talking about an address of a device on an MDIO > > bus. It takes a value between 0 and 31. > > > > How are IC2 device addresses represented in ACPI? MDIO devices and I2C > > devices are pretty similar. So it would make sense to use the same as > > what I2C uses. > > > Too big (and has table) to sensibly quote, but defined in ACPI spec > > 6.1.1 _ADR (Address) > > Ive never though been quite sure if that is just an example list of > address types or its supposed to be canonical (in which case some ECRs > are needed to the spec). Hi Graeme I took a quick look at version 6.2, and noticed i2c devices use _ADR(). So using it for MDIO seems O.K. However, i2c, spi and uart devices all seem to be described using GenericSerialBus. Maybe the correct way to describe MDIO devices is to also use GenericSerialBus? Andrew
On Mon, Jan 08, 2018 at 03:53:12PM +0100, Andrew Lunn wrote: > On Mon, Jan 08, 2018 at 02:45:48PM +0000, Graeme Gregory wrote: > > On Thu, Jan 04, 2018 at 05:20:36PM +0100, Andrew Lunn wrote: > > > > > I already agreed with 'reg' being awkward in the later emails. > > > > > Wouldn't _ADR be more appropriate to specify PHY address on MDIO bus? > > > > > > > > > Ah it is an actual address, then yes _ADR is probably more appropriate. > > > > > > Newbie ACPI question. What is the definition of an address? > > > > > > In this cause, we are talking about an address of a device on an MDIO > > > bus. It takes a value between 0 and 31. > > > > > > How are IC2 device addresses represented in ACPI? MDIO devices and I2C > > > devices are pretty similar. So it would make sense to use the same as > > > what I2C uses. > > > > > Too big (and has table) to sensibly quote, but defined in ACPI spec > > > > 6.1.1 _ADR (Address) > > > > Ive never though been quite sure if that is just an example list of > > address types or its supposed to be canonical (in which case some ECRs > > are needed to the spec). > > Hi Graeme > > I took a quick look at version 6.2, and noticed i2c devices use > _ADR(). So using it for MDIO seems O.K. > > However, i2c, spi and uart devices all seem to be described using > GenericSerialBus. Maybe the correct way to describe MDIO devices is to > also use GenericSerialBus? > I am not familiar with MDIO, but if its similar or a specific implementation of a serial bus that does sound sane! Graeme
w> I am not familiar with MDIO, but if its similar or a specific
> implementation of a serial bus that does sound sane!
It is a two wire serial bus. A good overview can be found here:
https://www.totalphase.com/support/articles/200349206-MDIO-Background
Andrew
Hi Andrew, 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > w> I am not familiar with MDIO, but if its similar or a specific >> implementation of a serial bus that does sound sane! > Thanks for digging, I will check if and how we can use GenericSerialBus with MDIO. Best regards, Marcin > It is a two wire serial bus. A good overview can be found here: > > https://www.totalphase.com/support/articles/200349206-MDIO-Background >
On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote: > Hi Andrew, > > > > 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > > w> I am not familiar with MDIO, but if its similar or a specific > >> implementation of a serial bus that does sound sane! > > > > Thanks for digging, I will check if and how we can use > GenericSerialBus with MDIO. > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come across similar on other ARM boards. Graeme
2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>: > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote: >> Hi Andrew, >> >> >> >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: >> > w> I am not familiar with MDIO, but if its similar or a specific >> >> implementation of a serial bus that does sound sane! >> > >> >> Thanks for digging, I will check if and how we can use >> GenericSerialBus with MDIO. >> > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come > across similar on other ARM boards. > I'm looking forward to their feedback, however, what I've noticed, each driver handles mdio/phys on its own, not using any generic solution, which is what I need to actually avoid :) Best regards, Marcin
On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote: > 2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>: > > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote: > >> Hi Andrew, > >> > >> > >> > >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > >> > w> I am not familiar with MDIO, but if its similar or a specific > >> >> implementation of a serial bus that does sound sane! > >> > > >> > >> Thanks for digging, I will check if and how we can use > >> GenericSerialBus with MDIO. > >> > > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come > > across similar on other ARM boards. > > > > I'm looking forward to their feedback, however, what I've noticed, > each driver handles mdio/phys on its own, not using any generic > solution, which is what I need to actually avoid :) Agreed. Lets define it once for all drivers using phylib/phylink. Andrew
[+cc Mika] On Tue, Jan 09, 2018 at 02:00:12PM +0100, Andrew Lunn wrote: > On Tue, Jan 09, 2018 at 11:22:00AM +0100, Marcin Wojtas wrote: > > 2018-01-09 11:19 GMT+01:00 Graeme Gregory <graeme.gregory@linaro.org>: > > > On Mon, Jan 08, 2018 at 06:17:06PM +0100, Marcin Wojtas wrote: > > >> Hi Andrew, > > >> > > >> > > >> > > >> 2018-01-08 16:42 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > > >> > w> I am not familiar with MDIO, but if its similar or a specific > > >> >> implementation of a serial bus that does sound sane! > > >> > > > >> > > >> Thanks for digging, I will check if and how we can use > > >> GenericSerialBus with MDIO. > > >> > > > Maybe Lorenzo, Hanjun, Sudeep can comment here they might have come > > > across similar on other ARM boards. > > > > > > > I'm looking forward to their feedback, however, what I've noticed, > > each driver handles mdio/phys on its own, not using any generic > > solution, which is what I need to actually avoid :) > > Agreed. Lets define it once for all drivers using phylib/phylink. To start with, I am not entirely familiar with MDIO, apologies in advance. Building something on top of GenericSerialBus sounds correct but if I am not mistaken you would need a new bus type in the ACPI specs. I CC'ed Mika since he is more familiar with handling these bits of ACPI specs - I wonder whether this is a problem that cropped up on x86 systems too. I do not think there is one and only answer but there must be a single set of bindings and if the ACPI specs already cater for some of them we have to reuse them. Please take some time to ensure the solution you are pushing is widely deployable. Thanks, Lorenzo
> I CC'ed Mika since he is more familiar with handling these bits of ACPI > specs - I wonder whether this is a problem that cropped up on x86 > systems too. Hi Lorenzo There is nothing about MDIO, PHYs, Ethernet switches, etc in version 6.2 of the spec. If x86 has this, it must be after 6.2 was released. I would not be too surprised if x86 has none of this. If you look at the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They are all PCI devices, and hide all this. > I do not think there is one and only answer but there must be a single > set of bindings and if the ACPI specs already cater for some of them > we have to reuse them. Agreed. Due diligence so far suggests there is nothing already defined. But im a newbie to ACPI, so could be looking in the wrong place. I really hope there is somebody like Rob Herring, the DT maintainer, who keeps an eye on all ACPI talk and would tell us if we are heading off in the wrong direction. Andrew
Hi Mika, 2018-01-18 14:00 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: >> I CC'ed Mika since he is more familiar with handling these bits of ACPI >> specs - I wonder whether this is a problem that cropped up on x86 >> systems too. > > Hi Lorenzo > > There is nothing about MDIO, PHYs, Ethernet switches, etc in version > 6.2 of the spec. If x86 has this, it must be after 6.2 was released. > I would not be too surprised if x86 has none of this. If you look at > the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They > are all PCI devices, and hide all this. > >> I do not think there is one and only answer but there must be a single >> set of bindings and if the ACPI specs already cater for some of them >> we have to reuse them. > > Agreed. Due diligence so far suggests there is nothing already > defined. But im a newbie to ACPI, so could be looking in the wrong > place. I really hope there is somebody like Rob Herring, the DT > maintainer, who keeps an eye on all ACPI talk and would tell us if we > are heading off in the wrong direction. > My initial approach with MDIO bus with PHYs as child nodes was super easy to describe and handle in Linux - please refer to: - typical representation of mdio bus with the phys - https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8 - my patches in the initial series However I guess it would be more proper to use the GenericSerialBus-based description, as advised in ACPI Spec. The question is, whether we should define new type of a bus or not (MdioSerialBus, similar to e.g. I2cSerialBus). Since I have a code that can be tested and easily modified to use different ACPI approaches with real platform MDIO controller (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find some time to prepare a proof of concept based on GenericSerialBus. Please expect some RFC patches hopefully right after the coming merge window is closed. Of course, if I come up on some ACPI - specific implementation questions, I won't hesitate to ask in this thred. I will also appreciate any hints. For now my two main concerns are: - The PHY address on the mdio bus - should it be put into _CRS -> GenericSerialBus() field or separate _ADR? I'd lean towards first option. - The PHY type - in Linux it's resolved basing on two generic compatible strings (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8). I'd put it as a sort of ID into GenericSerialBus(). If you agree - any specific filed that you would try to use? Do I understand correctly that the MDIO controller node should comprise OperationRegion() definition of the GenericSerialBus? I'm looking forward to your feedback. Thanks, Marcin
> Since I have a code that can be tested and easily modified to use > different ACPI approaches with real platform MDIO controller > (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find > some time to prepare a proof of concept based on GenericSerialBus. > Please expect some RFC patches hopefully right after the coming merge > window is closed. It would also be interesting to know how the standardisation process works. I'm sure these is a FAQs, so maybe somebody could point us towards it. Do we need to submit a proposed extension to the ACPI standard at the same time as the patch? Should we not accept the code into Linux until the proposal has been accepted? Or can we accept the code provisionally, with the understanding that if the ACPI committee rejects the extension, or suggest alternations, we can take the code out of Linux without having to worry about backwards compatibility? Thanks Andrew
On Fri, Jan 19, 2018 at 07:07:29PM +0100, Marcin Wojtas wrote: > Hi Mika, Hi, > 2018-01-18 14:00 GMT+01:00 Andrew Lunn <andrew@lunn.ch>: > >> I CC'ed Mika since he is more familiar with handling these bits of ACPI > >> specs - I wonder whether this is a problem that cropped up on x86 > >> systems too. > > > > Hi Lorenzo > > > > There is nothing about MDIO, PHYs, Ethernet switches, etc in version > > 6.2 of the spec. If x86 has this, it must be after 6.2 was released. > > I would not be too surprised if x86 has none of this. If you look at > > the typical drives used on x86, i210, e1000e, ixgb, r8169, etc. They > > are all PCI devices, and hide all this. > > > >> I do not think there is one and only answer but there must be a single > >> set of bindings and if the ACPI specs already cater for some of them > >> we have to reuse them. > > > > Agreed. Due diligence so far suggests there is nothing already > > defined. But im a newbie to ACPI, so could be looking in the wrong > > place. I really hope there is somebody like Rob Herring, the DT > > maintainer, who keeps an eye on all ACPI talk and would tell us if we > > are heading off in the wrong direction. > > > > My initial approach with MDIO bus with PHYs as child nodes was super > easy to describe and handle in Linux - please refer to: > - typical representation of mdio bus with the phys - > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/mdio.txt?h=v4.15-rc8 > - my patches in the initial series > However I guess it would be more proper to use the > GenericSerialBus-based description, as advised in ACPI Spec. The > question is, whether we should define new type of a bus or not > (MdioSerialBus, similar to e.g. I2cSerialBus). I'm not familiar with MDIO bus but an alternative to GeneriSerialBus would be to follow what SDIO is doing, e.g have the PHY devices listed below the MDIO controller and use _ADR to describe their "address" on that bus. You can see how _ADR applies to SDIO bus from ACPI spec. Of course ACPI spec should then be updated accordingly to describe what _ADR means for devices on MDIO bus. > Since I have a code that can be tested and easily modified to use > different ACPI approaches with real platform MDIO controller > (mvmdio.c) and NIC (mvpp2.c), in coming weeks I may be able to find > some time to prepare a proof of concept based on GenericSerialBus. > Please expect some RFC patches hopefully right after the coming merge > window is closed. > > Of course, if I come up on some ACPI - specific implementation > questions, I won't hesitate to ask in this thred. I will also > appreciate any hints. For now my two main concerns are: > - The PHY address on the mdio bus - should it be put into _CRS -> > GenericSerialBus() field or separate _ADR? I'd lean towards first > option. > - The PHY type - in Linux it's resolved basing on two generic > compatible strings > (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/phy.txt?h=v4.15-rc8). > I'd put it as a sort of ID into GenericSerialBus(). If you agree - any > specific filed that you would try to use? If you go with the SDIO way then each PHY is described as normal ACPI device and you can use ACPI _HID/_CID to match the device to the corresponding driver. > Do I understand correctly that the MDIO controller node should > comprise OperationRegion() definition of the GenericSerialBus? I don't think OpRegions are useful in this case because they are mainly used to allow BIOS AML code to access the hardware through OS driver.
> I'm not familiar with MDIO bus but an alternative to GeneriSerialBus > would be to follow what SDIO is doing, e.g have the PHY devices listed > below the MDIO controller and use _ADR to describe their "address" on > that bus. You can see how _ADR applies to SDIO bus from ACPI spec. Hi Mika SDIO is not a serial bus, well it can be in its simplest form, but high speed implementations have 4 data lines. So i can understand them not using GenericSerialBus. MDIO is a serial bus, very similar to SPI, I2C, and UART. > If you go with the SDIO way then each PHY is described as normal ACPI > device and you can use ACPI _HID/_CID to match the device to the > corresponding driver. Just some background here. If you have a plain PHY as a device on an MDIO bus, you don't need to match it to a driver within ACPI. Registers 2 and 3 contain a vendor and product ID. That is what it used to match the device to the driver. What you might need to know is the protocol to talk on the bus. Most devices use clause 22 protocol. A few devices are clause 45. 22 is the default in Linux, and you need to indicate if 45 should be used. You can also indicate 22. It gets more complex when the device on the bus is not a PHY. It is a generic bus, you can connect anything to it. Ethernet switches can be on the bus. They generally cannot be identified using registers 2 and 3. So you do need to match the device to the driver. Most do have ID registers, so the driver can work out what specific device is on the bus. However, Marvell moved the ID registers on there newer generation of devices, so we need to give the driver a hint where to look. So in device tree, we have two different compatible string. Broadcom really do use it as a generic bus. They have their USB PHYs and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to match the device to the driver, as normal. We need to ensure what we define for ACPI has the same level of flexibility. Andrew
On Sun, Jan 21, 2018 at 02:08:40AM +0100, Andrew Lunn wrote: > > I'm not familiar with MDIO bus but an alternative to GeneriSerialBus > > would be to follow what SDIO is doing, e.g have the PHY devices listed > > below the MDIO controller and use _ADR to describe their "address" on > > that bus. You can see how _ADR applies to SDIO bus from ACPI spec. > > Hi Mika > > SDIO is not a serial bus, well it can be in its simplest form, but > high speed implementations have 4 data lines. So i can understand them > not using GenericSerialBus. Well, I think SDIO is more of a serial bus pretty much the same way than SPI but it can support more data lines as needed (in the same way than SPI). But I'm not an expert in SDIO. However, that's not the point here :-) SATA (which is definitely a serial bus) uses the same mechanism and not GenericSerialBus. > MDIO is a serial bus, very similar to SPI, I2C, and UART. > > > If you go with the SDIO way then each PHY is described as normal ACPI > > device and you can use ACPI _HID/_CID to match the device to the > > corresponding driver. > > Just some background here. If you have a plain PHY as a device on an > MDIO bus, you don't need to match it to a driver within ACPI. > Registers 2 and 3 contain a vendor and product ID. That is what it > used to match the device to the driver. > > What you might need to know is the protocol to talk on the bus. Most > devices use clause 22 protocol. A few devices are clause 45. 22 is the > default in Linux, and you need to indicate if 45 should be used. You > can also indicate 22. > > It gets more complex when the device on the bus is not a PHY. It is a > generic bus, you can connect anything to it. Ethernet switches can be > on the bus. They generally cannot be identified using registers 2 and > 3. So you do need to match the device to the driver. Most do have ID > registers, so the driver can work out what specific device is on the > bus. However, Marvell moved the ID registers on there newer generation > of devices, so we need to give the driver a hint where to look. So in > device tree, we have two different compatible string. > > Broadcom really do use it as a generic bus. They have their USB PHYs > and PCIE PHYs on an MDIO bus. In DT, they have compatible strings to > match the device to the driver, as normal. OK, thanks for the explanation. > We need to ensure what we define for ACPI has the same level of > flexibility. Right. So if you need to have some additional "parameters" with the connection, then I suppose you may want to go with the GenericSerialBus route. However, looking at the sample device tree description: davinci_mdio: ethernet@5c030000 { compatible = "ti,davinci_mdio"; reg = <0x5c030000 0x1000>; #address-cells = <1>; #size-cells = <0>; reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; reset-delay-us = <2>; ethphy0: ethernet-phy@1 { reg = <1>; }; ethphy1: ethernet-phy@3 { reg = <3>; }; }; would pretty much translate directly to this in ACPI if you don't need any additional attributes: Device (ETH0) { Name (_ADR, /* PCI address of the NIC */) Device (PHY0) { Name (_ADR, 1) ... } Device (PHY1) { Name (_ADR, 3) ... } } which looks pretty simple to me. You can also use _DSM and _DSD here to pass information (like the protocol number) for the PHY devices to Linux.
> Right. So if you need to have some additional "parameters" with the > connection, then I suppose you may want to go with the GenericSerialBus > route. However, looking at the sample device tree description: > > davinci_mdio: ethernet@5c030000 { > compatible = "ti,davinci_mdio"; > reg = <0x5c030000 0x1000>; > #address-cells = <1>; > #size-cells = <0>; > > reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; > reset-delay-us = <2>; > > ethphy0: ethernet-phy@1 { > reg = <1>; > }; > > ethphy1: ethernet-phy@3 { > reg = <3>; > }; > }; > > would pretty much translate directly to this in ACPI if you don't need > any additional attributes: > > Device (ETH0) { > Name (_ADR, /* PCI address of the NIC */) > > Device (PHY0) { > Name (_ADR, 1) > ... > } > > Device (PHY1) { > Name (_ADR, 3) > ... > } > } > > which looks pretty simple to me. You can also use _DSM and _DSD here to > pass information (like the protocol number) for the PHY devices to Linux. I'm not particularly worried about that simple case. Other than, i don't want people to think that is all that is required. For a more full example, take a look at vf610-zii-dev-rev-b.dts. The Freescale FEC Ethernet controller provides the base MDIO device, mdio1. On top of this is an MDIO mux, using a few GPIO lines to enable/disable 3 child MDIO busses. Each of these busses has an Ethernet Switch. The Ethernet switch exports up to two MDIO busses, and on these busses are Ethernet PHYs which are embedded inside the switch. The Ethernet switches are also interrupt controllers, with the PHYs having interrupt properties which point back to the interrupt controller in the switch. So i'm interested in an ACPI proposal which supports this board. Andrew
On 21 January 2018 at 16:13, Andrew Lunn <andrew@lunn.ch> wrote: >> Right. So if you need to have some additional "parameters" with the >> connection, then I suppose you may want to go with the GenericSerialBus >> route. However, looking at the sample device tree description: >> >> davinci_mdio: ethernet@5c030000 { >> compatible = "ti,davinci_mdio"; >> reg = <0x5c030000 0x1000>; >> #address-cells = <1>; >> #size-cells = <0>; >> >> reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>; >> reset-delay-us = <2>; >> >> ethphy0: ethernet-phy@1 { >> reg = <1>; >> }; >> >> ethphy1: ethernet-phy@3 { >> reg = <3>; >> }; >> }; >> >> would pretty much translate directly to this in ACPI if you don't need >> any additional attributes: >> >> Device (ETH0) { >> Name (_ADR, /* PCI address of the NIC */) >> >> Device (PHY0) { >> Name (_ADR, 1) >> ... >> } >> >> Device (PHY1) { >> Name (_ADR, 3) >> ... >> } >> } >> >> which looks pretty simple to me. You can also use _DSM and _DSD here to >> pass information (like the protocol number) for the PHY devices to Linux. > > I'm not particularly worried about that simple case. Other than, i > don't want people to think that is all that is required. > > For a more full example, take a look at vf610-zii-dev-rev-b.dts. The > Freescale FEC Ethernet controller provides the base MDIO device, > mdio1. On top of this is an MDIO mux, using a few GPIO lines to > enable/disable 3 child MDIO busses. Each of these busses has an > Ethernet Switch. The Ethernet switch exports up to two MDIO busses, > and on these busses are Ethernet PHYs which are embedded inside the > switch. The Ethernet switches are also interrupt controllers, with the > PHYs having interrupt properties which point back to the interrupt > controller in the switch. > > So i'm interested in an ACPI proposal which supports this board. > However interesting as an example, I'm not convinced this is what we should aim for. ACPI is not a replacement for DT, and it is unlikely that people would be interested in running ACPI-only distros such as RHEL on their Ethernet switch. DT is excellent at describing this, and there is no need to replace it. ACPI is about firmware abstractions: you don't need to describe every stacked interrupt controller in minute detail to the OS if the firmware configures it sufficiently. That way, the OS does not need to know all these details, and vendors can update their hardware without having to update the software as well. (Or that is the idea at least, how that works out in practice on arm64 systems remains to be seen) Taking the Marvell 8040 as an example: the ACPI description does not expose the ICU interrupt controllers to the OS, but the firmware configures them and describes their configured state as ordinary GIC interrupts. Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI forum, and only members (who are all under a contract regarding reasonable and non-discriminatory (RAND) licensing terms to IP they own that is covered by the spec) can contribute. Individuals can become members for free AFAIR, but that doesn't mean individuals are typically interested in getting involved in a process that is rather tedious and bureaucratic. So my suggestion would be to find out to what extent the latest version of the spec can describe non-trivial MDIO topologies, and hopefully that includes the mvpp2 on the Marvell 8040.
> However interesting as an example, I'm not convinced this is what we > should aim for. > > ACPI is not a replacement for DT, and it is unlikely that people would > be interested in running ACPI-only distros such as RHEL on their > Ethernet switch. DT is excellent at describing this, and there is no > need to replace it. I however do know of somebody who is building an industrial wireless access point, with a Marvell Ethernet switch. They have chosen to use an Intel CPU, and want to run CentOS on it, because that is what they know. So they will be using ACPI whatever, for the basic board support. They then have a choice of either ACPI for the switch, or having device tree as well as ACPI for the switch. And it will be interesting to see how that works. Can DT reference GPIOs and I2C busses in ACPI? > Also, please bear in mind that the ACPI spec is owned by the UEFI/ACPI > forum, and only members (who are all under a contract regarding > reasonable and non-discriminatory (RAND) licensing terms to IP they > own that is covered by the spec) can contribute. Individuals can > become members for free AFAIR, but that doesn't mean individuals are > typically interested in getting involved in a process that is rather > tedious and bureaucratic. And this is a bit what i'm worried about. How you represent MDIO busses, PHYs, Ethernet switches is implemented once in the core Linux code. So i would be interested in knowing what happens if the Linux community defines and implements something, boards start using it, but a year later the tedious and bureaucratic process rejects it, re-writes it, in an incompatible way. Andrew