| Message ID | 20200715090400.4733-2-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [net-next,v7,1/6] Documentation: ACPI: DSD: Document MDIO PHY | expand |
On 7/15/2020 2:03 AM, Calvin Johnson wrote: > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > provide them to be connected to MAC. > > An ACPI node property "mdio-handle" is introduced to reference the > MDIO bus on which PHYs are registered with autoprobing method used > by mdiobus_register(). > > Describe properties "phy-channel" and "phy-mode" > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> You would probably want to submit an update to the PHY LIBRARY section of the MAINTAINERS file to add this PHY DSD documentation, can be done when this series gets merged.
On Wed, Jul 15, 2020 at 08:04:36PM -0700, Florian Fainelli wrote: > > > On 7/15/2020 2:03 AM, Calvin Johnson wrote: > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > provide them to be connected to MAC. > > > > An ACPI node property "mdio-handle" is introduced to reference the > > MDIO bus on which PHYs are registered with autoprobing method used > > by mdiobus_register(). > > > > Describe properties "phy-channel" and "phy-mode" > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> I really would like to see an ACPI maintainer ACK this before it gets merged. I'm not sure the current reviewers have deep enough knowledge of ACPI to know if this is going against parts of the standard, or philosophy of ACPI. And we are setting an ABI here, so we need to be particularly careful. Andrew
Hi, On 7/15/20 4:03 AM, Calvin Johnson wrote: > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > provide them to be connected to MAC. > > An ACPI node property "mdio-handle" is introduced to reference the > MDIO bus on which PHYs are registered with autoprobing method used > by mdiobus_register(). > > Describe properties "phy-channel" and "phy-mode" > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > Changes in v7: None > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: > - cleanup based on v2 comments > - Added description for more properties > - Added MDIO node DSDT entry > > Changes in v2: None > > Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst > > diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst > new file mode 100644 > index 000000000000..0132fee10b45 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,90 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an mdiobus are probed and registered using mdiobus_register(). > +Later, for connecting these PHYs to MAC, the PHYs registered on the > +mdiobus have to be referenced. First, this is all perfectly compatible with my literal interpretation and understanding of the ACPI spec. The use of _DSD is there to provide a way to "extend" if you will the specification for device specific edge cases that aren't directly covered by the spec. OTOH, it may be my lack of knowledge here, but IMHO this is a bit of a difficult pill for all arm/sbsa systems though. Primary because I don't see how one is expected to use the generic ACPI power states on the parent device here. I also have some questions about how one might import such a device into a VM. Further AFAIK arm's current recommendations for SBSA/ACPI systems point in the direction of RCiEP's. IMHO what should be clarified in this document is something to the effect that the "mdio-handle" is used for systems which have multiple nic/mac's sharing a single MDIO bus. Otherwise the MDIO bus and its phy should be a child of the nic/mac using it, with standardized behaviors/etc left up to the OSPM when it comes to MDIO bus enumeration/etc. Thanks, > + > +mdio-handle > +----------- > +For each MAC node, a property "mdio-handle" is used to reference the > +MDIO bus on which the PHYs are registered. On getting hold of the MDIO > +bus, use find_phy_device() to get the PHY connected to the MAC. > + > +phy-channel > +----------- > +Property "phy-channel" defines the address of the PHY on the mdiobus. > + > +phy-mode > +-------- > +Property "phy-mode" defines the type of PHY interface. > + > +An example of this is shown below:: > + > +DSDT entry for MAC where MDIO node is referenced > +------------------------------------------------ > + Scope(\_SB.MCE0.PR17) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"phy-channel", 1}, > + Package () {"phy-mode", "rgmii-id"}, > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"phy-channel", 2}, > + Package () {"phy-mode", "rgmii-id"}, > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > + } > + }) > + } > + > +DSDT entry for MDIO node > +------------------------ > +a) Silicon Component > +-------------------- > + Scope(_SB) > + { > + Device(MDI0) { > + Name(_HID, "NXP0006") > + Name(_CCA, 1) > + Name(_UID, 0) > + Name(_CRS, ResourceTemplate() { > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > + { > + MDI0_IT > + } > + }) // end of _CRS for MDI0 > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"little-endian", 1}, > + } > + }) > + } // end of MDI0 > + } > + > +b) Platform Component > +--------------------- > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } >
> Otherwise the MDIO bus and its phy should be a > child of the nic/mac using it, with standardized behaviors/etc left up to > the OSPM when it comes to MDIO bus enumeration/etc. Hi Jeremy Could you be a bit more specific here please. DT allows macb0: ethernet@fffc4000 { compatible = "cdns,at32ap7000-macb"; reg = <0xfffc4000 0x4000>; interrupts = <21>; phy-mode = "rmii"; local-mac-address = [3a 0e 03 04 05 06]; clock-names = "pclk", "hclk", "tx_clk"; clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; ethernet-phy@1 { reg = <0x1>; reset-gpios = <&pioE 6 1>; }; }; So the PHY is a direct child of the MAC. The MDIO bus is not modelled at all. Although this is allowed, it is deprecated, because it results in problems with advanced systems which have multiple different children, and the need to differentiate them. So drivers are slowly migrating to always modelling the MDIO bus. In that case, the phy-handle is always used to point to the PHY: eth0: ethernet@522d0000 { compatible = "socionext,synquacer-netsec"; reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0 0x10000>; interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>; clocks = <&clk_netsec>; clock-names = "phy_ref_clk"; phy-mode = "rgmii"; max-speed = <1000>; max-frame-size = <9000>; phy-handle = <&phy1>; mdio { #address-cells = <1>; #size-cells = <0>; phy1: ethernet-phy@1 { compatible = "ethernet-phy-ieee802.3-c22"; reg = <1>; }; }; "mdio-handle" is just half of phy-handle. What you seem to be say is that although we have defined a generic solution for ACPI which should work in all cases, it is suggested to not use it? What exactly are you suggesting in its place? Andrew
Hi, On 7/24/20 8:39 AM, Andrew Lunn wrote: >> Otherwise the MDIO bus and its phy should be a >> child of the nic/mac using it, with standardized behaviors/etc left up to >> the OSPM when it comes to MDIO bus enumeration/etc. > > Hi Jeremy > > Could you be a bit more specific here please. > > DT allows > > macb0: ethernet@fffc4000 { > compatible = "cdns,at32ap7000-macb"; > reg = <0xfffc4000 0x4000>; > interrupts = <21>; > phy-mode = "rmii"; > local-mac-address = [3a 0e 03 04 05 06]; > clock-names = "pclk", "hclk", "tx_clk"; > clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; > ethernet-phy@1 { > reg = <0x1>; > reset-gpios = <&pioE 6 1>; > }; > }; > > So the PHY is a direct child of the MAC. The MDIO bus is not modelled > at all. Although this is allowed, it is deprecated, because it results > in problems with advanced systems which have multiple different > children, and the need to differentiate them. So drivers are slowly I don't think i'm suggesting that, because AFAIK in ACPI you would have to specify the DEVICE() for mdio, in order to nest a further set of phy's via _ADR(). I think in general what I was describing would look more like what you have below. But.. > migrating to always modelling the MDIO bus. In that case, the > phy-handle is always used to point to the PHY: > > eth0: ethernet@522d0000 { > compatible = "socionext,synquacer-netsec"; > reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0 0x10000>; > interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>; > clocks = <&clk_netsec>; > clock-names = "phy_ref_clk"; > phy-mode = "rgmii"; > max-speed = <1000>; > max-frame-size = <9000>; > phy-handle = <&phy1>; > > mdio { > #address-cells = <1>; > #size-cells = <0>; > phy1: ethernet-phy@1 { > compatible = "ethernet-phy-ieee802.3-c22"; > reg = <1>; > }; > }; > > "mdio-handle" is just half of phy-handle. > > What you seem to be say is that although we have defined a generic > solution for ACPI which should work in all cases, it is suggested to > not use it? What exactly are you suggesting in its place? When you put it that way, what i'm saying sounds crazy. In this case what are are doing isn't as clean as what you have described above, its more like: mdio: { phy1: {} phy2: {} } ... // somewhere else dmac1: { phy-handle = <&phy1>; } ... //somewhere else eth0: { //another device talking to the mgmt controller } Which is special in a couple ways. Lets rewind for a moment and say for ARM/ACPI, what we are talking about are "edge/server class" devices (with RAS statements/etc) where the expectation is that they will be running virtualized workloads using LTS distros, or non linux OSes. DT/etc remains an option for networking devices which are more "embedded", aren't SBSA, etc. So an Arm based/ACPI machine should be more regular and share more in the way of system architecture with other SBSA/SBBR/ACPI/etc machines than has been the case for DT machines. A concern is then how we punch networking devices into an arbitrary VM in a standardized way using libvirt/whatever. If the networking device doesn't look like a simple self contained memory mapped resource with an IOMMU domain, I think everything becomes more complicated and you have to start going down the path of special caseing the VM firmware beyond how its done for self contained PCIe/SRIOV devices. The latter manage to pull this all off with a PCIe id, and a couple BARs fed into the VM. So, I would hope an ACPI nic representation is closer to just a minimal resource list like: eth0: { compatible = "cdns,at32ap7000-macb"; reg = <0xfffc4000 0x4000>; interrupts = <21>; } or in ACPI speak: Device (ETH0) { Name (_HID, "CDNSXXX") Method (_CRS, 0x0, Serialized) { Name (RBUF, ResourceTemplate () { Memory32Fixed (ReadWrite, 0xfffc4000, 0x4000, ) Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) { 21 } }) Return (RBUF) } } (Plus methods for pwr mgmt/etc as needed, the iommu info comes from another table). Returning to the NXP part. They avoid the entirety of the above discussion because all this MDIO/PHY mgmt is just feeding the data into the mgmt controller, and the bits that are punched into the VM are fairly standalone. Anyway, I think this set is generally fine, I would like to see this part working well with ACPI given its what we have available today. For the future, we also need to continue pushing everyone towards common hardware standards. One of the ways of doing this is having hardware which can be automatically enumerated/configured. Suggesting that the kernel has a recommended way of doing this which aids fragmentation isn't what we are trying to achieve with ACPI. Hence my previous comment that we should consider this an escape hatch rather than the last word in how to describe networking on ACPI/SBSA platforms. Thanks,
On 7/24/20 10:26 AM, Jeremy Linton wrote: > Hi, > > On 7/24/20 8:39 AM, Andrew Lunn wrote: >>> Otherwise the MDIO bus and its phy should be a >>> child of the nic/mac using it, with standardized behaviors/etc left >>> up to >>> the OSPM when it comes to MDIO bus enumeration/etc. >> >> Hi Jeremy >> >> Could you be a bit more specific here please. >> >> DT allows >> >> macb0: ethernet@fffc4000 { >> compatible = "cdns,at32ap7000-macb"; >> reg = <0xfffc4000 0x4000>; >> interrupts = <21>; >> phy-mode = "rmii"; >> local-mac-address = [3a 0e 03 04 05 06]; >> clock-names = "pclk", "hclk", "tx_clk"; >> clocks = <&clkc 30>, <&clkc 30>, <&clkc 13>; >> ethernet-phy@1 { >> reg = <0x1>; >> reset-gpios = <&pioE 6 1>; >> }; >> }; >> >> So the PHY is a direct child of the MAC. The MDIO bus is not modelled >> at all. Although this is allowed, it is deprecated, because it results >> > in problems with advanced systems which have multiple different >> children, and the need to differentiate them. So drivers are slowly > > I don't think i'm suggesting that, because AFAIK in ACPI you would have > to specify the DEVICE() for mdio, in order to nest a further set of > phy's via _ADR(). I think in general what I was describing would look > more like what you have below. But.. > >> migrating to always modelling the MDIO bus. In that case, the >> phy-handle is always used to point to the PHY: >> >> eth0: ethernet@522d0000 { >> compatible = "socionext,synquacer-netsec"; >> reg = <0 0x522d0000 0x0 0x10000>, <0 0x10000000 0x0 >> 0x10000>; >> interrupts = <GIC_SPI 176 IRQ_TYPE_LEVEL_HIGH>; >> clocks = <&clk_netsec>; >> clock-names = "phy_ref_clk"; >> phy-mode = "rgmii"; >> max-speed = <1000>; >> max-frame-size = <9000>; >> phy-handle = <&phy1>; >> >> mdio { >> #address-cells = <1>; >> #size-cells = <0>; >> phy1: ethernet-phy@1 { >> compatible = >> "ethernet-phy-ieee802.3-c22"; >> reg = <1>; >> }; >> }; >> >> "mdio-handle" is just half of phy-handle. >> >> What you seem to be say is that although we have defined a generic >> solution for ACPI which should work in all cases, it is suggested to >> not use it? What exactly are you suggesting in its place? > > When you put it that way, what i'm saying sounds crazy. > > In this case what are are doing isn't as clean as what you have > described above, its more like: > > mdio: { > phy1: {} > phy2: {} > } > ... > // somewhere else > dmac1: { > phy-handle = <&phy1>; > } > > ... //somewhere else > eth0: { > //another device talking to the mgmt controller > } > > > Which is special in a couple ways. > > Lets rewind for a moment and say for ARM/ACPI, what we are talking about > are "edge/server class" devices (with RAS statements/etc) where the > expectation is that they will be running virtualized workloads using LTS > distros, or non linux OSes. DT/etc remains an option for networking > devices which are more "embedded", aren't SBSA, etc. So an Arm > based/ACPI machine should be more regular and share more in the way of > system architecture with other SBSA/SBBR/ACPI/etc machines than has been > the case for DT machines. > > A concern is then how we punch networking devices into an arbitrary VM > in a standardized way using libvirt/whatever. If the networking device > doesn't look like a simple self contained memory mapped resource with an > IOMMU domain, I think everything becomes more complicated and you have > to start going down the path of special caseing the VM firmware beyond > how its done for self contained PCIe/SRIOV devices. The latter manage to > pull this all off with a PCIe id, and a couple BARs fed into the VM. > > So, I would hope an ACPI nic representation is closer to just a minimal > resource list like: > > eth0: { > compatible = "cdns,at32ap7000-macb"; > reg = <0xfffc4000 0x4000>; > interrupts = <21>; > } > or in ACPI speak: > Device (ETH0) > { > Name (_HID, "CDNSXXX") > Method (_CRS, 0x0, Serialized) > { > Name (RBUF, ResourceTemplate () > { > Memory32Fixed (ReadWrite, 0xfffc4000, 0x4000, ) > Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive) > { > 21 > } > }) > Return (RBUF) > } > } > > (Plus methods for pwr mgmt/etc as needed, the iommu info comes from > another table). > > Returning to the NXP part. They avoid the entirety of the above > discussion because all this MDIO/PHY mgmt is just feeding the data into > the mgmt controller, and the bits that are punched into the VM are > fairly standalone. > > Anyway, I think this set is generally fine, I would like to see this > part working well with ACPI given its what we have available today. For > the future, we also need to continue pushing everyone towards common > hardware standards. One of the ways of doing this is having hardware > which can be automatically enumerated/configured. Suggesting that the > kernel has a recommended way of doing this which aids fragmentation > isn't what we are trying to achieve with ACPI. Hence my previous comment > that we should consider this an escape hatch rather than the last word > in how to describe networking on ACPI/SBSA platforms. We are at v7 of this patch series, and no authoritative ACPI Linux maintainer appears to have reviewed this, so there is no clear sign of this converging anywhere. This is looking a lot like busy work for nothing. Given that the representation appears to be wildly misunderstood and no one seems to come up with something that reaches community agreement, what exactly is the plan here? I am going to suggest something highly unpopular here: how about you just load Device Tree overlays based on matching a particular board and ship those overlays somewhere in the kernel that take care of registering your network devices with the desired network topology?
> Hence my previous comment that we should consider this an escape > hatch rather than the last word in how to describe networking on > ACPI/SBSA platforms. One problem i have is that this patch set suggests ACPI can be used to describe complex network hardware. It is opening the door for others to follow and add more ACPI support in networking. How long before it is not considered an escape hatch, but the front door? For an example, see https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/ It is hard to see what the big picture is here. The [0/2] patch is not particularly good. But it makes it clear that people are wanting to add fixed-link PHYs into ACPI. These are pseudo devices, used to make the MAC think it is connected to a PHY when it is not. The MAC still gets informed of link speed, etc via the standard PHYLIB API. They are mostly used for when the Ethernet MAC is directly connected to an Ethernet Switch, at a MAC to MAC level. Now i could be wrong, but are Ethernet switches something you expect to see on ACPI/SBSA platforms? Or is this a legitimate use of the escape hatch? Andrew
> We are at v7 of this patch series, and no authoritative ACPI Linux > maintainer appears to have reviewed this, so there is no clear sign of > this converging anywhere. This is looking a lot like busy work for > nothing. Given that the representation appears to be wildly > misunderstood and no one seems to come up with something that reaches > community agreement, what exactly is the plan here? I think we need to NACK all attempts to add ACPI support to phylib and phylink until an authoritative ACPI Linux maintainer makes an appearance and actively steers the work. And not just this patchset, but all patchsets in the networking domain which have an ACPI component. Andrew
On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > I think we need to NACK all attempts to add ACPI support to phylib and > phylink until an authoritative ACPI Linux maintainer makes an > appearance and actively steers the work. And not just this patchset, > but all patchsets in the networking domain which have an ACPI > component. It's funny, since I see ACPI mailing list and none of the maintainers in the Cc here... I'm not sure they pay attention to some (noise-like?) activity which (from their perspective) happens on unrelated lists.
On 7/24/20 1:12 PM, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > >> I think we need to NACK all attempts to add ACPI support to phylib and >> phylink until an authoritative ACPI Linux maintainer makes an >> appearance and actively steers the work. And not just this patchset, >> but all patchsets in the networking domain which have an ACPI >> component. > > It's funny, since I see ACPI mailing list and none of the maintainers > in the Cc here... > I'm not sure they pay attention to some (noise-like?) activity which > (from their perspective) happens on unrelated lists. If you what you describe here is their perception of what is going on here, that is very encouraging, we are definitively going to make progress.
On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 7/24/20 1:12 PM, Andy Shevchenko wrote: > > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > >> I think we need to NACK all attempts to add ACPI support to phylib and > >> phylink until an authoritative ACPI Linux maintainer makes an > >> appearance and actively steers the work. And not just this patchset, > >> but all patchsets in the networking domain which have an ACPI > >> component. > > > > It's funny, since I see ACPI mailing list and none of the maintainers > > in the Cc here... > > I'm not sure they pay attention to some (noise-like?) activity which > > (from their perspective) happens on unrelated lists. > > If you what you describe here is their perception of what is going on > here, that is very encouraging, we are definitively going to make progress. I can't speak for them. As a maintainer in other areas I expect that people Cc explicitly maintainer(s) if they want more attention. Otherwise I look at the mails to the mailing list just from time to time. But this is my expectation, don't take me wrong.
On Fri, Jul 24, 2020 at 11:12:15PM +0300, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > I think we need to NACK all attempts to add ACPI support to phylib and > > phylink until an authoritative ACPI Linux maintainer makes an > > appearance and actively steers the work. And not just this patchset, > > but all patchsets in the networking domain which have an ACPI > > component. > > It's funny, since I see ACPI mailing list and none of the maintainers > in the Cc here... > I'm not sure they pay attention to some (noise-like?) activity which > (from their perspective) happens on unrelated lists. That is really disappointing that these patch sets are not being copied to the appropriate people (ACPI). I seem to remember I've already stated on at least a couple of occasions that these patch sets which add ACPI support to phylib need to be copied to ACPI people. I guess if ACPI people have been omitted, there will be a few more patch series iterations. Then there's that all the discussion that has already happened is not known to ACPI people, so we're probably doomed to repeating at least some of that.
On Fri, Jul 24, 2020 at 11:20:04PM +0300, Andy Shevchenko wrote: > On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 7/24/20 1:12 PM, Andy Shevchenko wrote: > > > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > > > >> I think we need to NACK all attempts to add ACPI support to phylib and > > >> phylink until an authoritative ACPI Linux maintainer makes an > > >> appearance and actively steers the work. And not just this patchset, > > >> but all patchsets in the networking domain which have an ACPI > > >> component. > > > > > > It's funny, since I see ACPI mailing list and none of the maintainers > > > in the Cc here... > > > I'm not sure they pay attention to some (noise-like?) activity which > > > (from their perspective) happens on unrelated lists. > > > > If you what you describe here is their perception of what is going on > > here, that is very encouraging, we are definitively going to make progress. > > I can't speak for them. As a maintainer in other areas I expect that > people Cc explicitly maintainer(s) if they want more attention. > Otherwise I look at the mails to the mailing list just from time to > time. But this is my expectation, don't take me wrong. Sorry about this miss. In some past patch-set, I had added Rafael but somehow missed him this time. From the "MAINTAINERS" file, I got two maintainers. I don't know who else can help with this discussion. I'll add others whom I know from ACPI list. M: "Rafael J. Wysocki" <rjw@rjwysocki.net> M: Len Brown <lenb@kernel.org> If you know others who can help, please add. Hi ACPI experts, Would you please help review this patchset and guide us. Please see the discussion on this patchset here: https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t Thanks Calvin
On Sat, Jul 25, 2020 at 10:37 AM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > On Fri, Jul 24, 2020 at 11:20:04PM +0300, Andy Shevchenko wrote: > > On Fri, Jul 24, 2020 at 11:13 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > > > On 7/24/20 1:12 PM, Andy Shevchenko wrote: > > > > On Fri, Jul 24, 2020 at 10:20 PM Andrew Lunn <andrew@lunn.ch> wrote: ... > > > >> I think we need to NACK all attempts to add ACPI support to phylib and > > > >> phylink until an authoritative ACPI Linux maintainer makes an > > > >> appearance and actively steers the work. And not just this patchset, > > > >> but all patchsets in the networking domain which have an ACPI > > > >> component. > > > > > > > > It's funny, since I see ACPI mailing list and none of the maintainers > > > > in the Cc here... > > > > I'm not sure they pay attention to some (noise-like?) activity which > > > > (from their perspective) happens on unrelated lists. > > > > > > If you what you describe here is their perception of what is going on > > > here, that is very encouraging, we are definitively going to make progress. > > > > I can't speak for them. As a maintainer in other areas I expect that > > people Cc explicitly maintainer(s) if they want more attention. > > Otherwise I look at the mails to the mailing list just from time to > > time. But this is my expectation, don't take me wrong. > > Sorry about this miss. > In some past patch-set, I had added Rafael but somehow missed him this time. > > From the "MAINTAINERS" file, I got two maintainers. I don't know who else > can help with this discussion. I'll add others whom I know from ACPI list. > M: "Rafael J. Wysocki" <rjw@rjwysocki.net> > M: Len Brown <lenb@kernel.org> > > If you know others who can help, please add. > > Hi ACPI experts, > Would you please help review this patchset and guide us. > > Please see the discussion on this patchset here: > https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t I would recommend resending the entire series with an appropriate Cc list. See below as well. ACPI FOR ARM64 (ACPI/arm64) M: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> M: Hanjun Guo <guohanjun@huawei.com> M: Sudeep Holla <sudeep.holla@arm.com>
On Fri, Jul 24, 2020 at 09:20:08PM +0200, Andrew Lunn wrote: > > We are at v7 of this patch series, and no authoritative ACPI Linux > > maintainer appears to have reviewed this, so there is no clear sign of > > this converging anywhere. This is looking a lot like busy work for > > nothing. Given that the representation appears to be wildly > > misunderstood and no one seems to come up with something that reaches > > community agreement, what exactly is the plan here? > > I think we need to NACK all attempts to add ACPI support to phylib and > phylink until an authoritative ACPI Linux maintainer makes an > appearance and actively steers the work. And not just this patchset, > but all patchsets in the networking domain which have an ACPI > component. > Unfortunately, this is one such problem that can never be solved easily TBH. We, in Linux kernel community had lots of discussion around _DSD and how it can be misused if not moderated after the introduction of ACPI support on Arm. It is useful property used by the kernel today both on x86 and Arm. Even other OS vendors do use, but the standard body recently deprecated the process we introduced few years back[1] as it really never kicked off. All OS vendors have introduced the properties as they need and have supported without a formal registry and this is the argument made to deprecate that process. As a general rule, we say no to any new property added unless there is no existing solution for the same. It might just expand exponential if not controlled. So if networking folks agree that there is a need for it and there exists no alternative solution, then we may need to add the support for the same. I don't have strong objection as I have least knowledge in network domain. But I agree, there exists a possibility of duplication of properties amongst different OS vendors and could be argument on the other side. -- Regards, Sudeep [1] http://www.uefi.org/sites/default/files/resources/web-page-v2.pdf
On Fri, Jul 24, 2020 at 09:14:36PM +0200, Andrew Lunn wrote: > > Hence my previous comment that we should consider this an escape > > hatch rather than the last word in how to describe networking on > > ACPI/SBSA platforms. > > One problem i have is that this patch set suggests ACPI can be used to > describe complex network hardware. It is opening the door for others > to follow and add more ACPI support in networking. How long before it > is not considered an escape hatch, but the front door? > I understand your concerns here. But as I mentioned in other email in the same thread, it is very tricky problem to solve as no one is ready to take up and maintain these. > For an example, see > > https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/ > > It is hard to see what the big picture is here. The [0/2] patch is not > particularly good. But it makes it clear that people are wanting to > add fixed-link PHYs into ACPI. These are pseudo devices, used to make > the MAC think it is connected to a PHY when it is not. The MAC still > gets informed of link speed, etc via the standard PHYLIB API. They are > mostly used for when the Ethernet MAC is directly connected to an > Ethernet Switch, at a MAC to MAC level. > > Now i could be wrong, but are Ethernet switches something you expect > to see on ACPI/SBSA platforms? Or is this a legitimate use of the > escape hatch? > My guess is that similar products running on other architectures(namely x86) might be running ACPI and hence the push to have ACPI on such ARM systems. It may weak argument for that and I agree with it. I want to think it as legitimate use here but I am well aware and afraid that this may become front door instead of escape hatch. Sorry, I am not helpful at all, but I am just sharing my personal opinion on this matter. -- Regards, Sudeep
On Fri, Jul 24, 2020 at 9:14 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > Hence my previous comment that we should consider this an escape > > hatch rather than the last word in how to describe networking on > > ACPI/SBSA platforms. > > One problem i have is that this patch set suggests ACPI can be used to > describe complex network hardware. It is opening the door for others > to follow and add more ACPI support in networking. How long before it > is not considered an escape hatch, but the front door? > > For an example, see > > https://patchwork.ozlabs.org/project/netdev/patch/1595417547-18957-3-git-send-email-vikas.singh@puresoftware.com/ > > It is hard to see what the big picture is here. The [0/2] patch is not > particularly good. But it makes it clear that people are wanting to > add fixed-link PHYs into ACPI. These are pseudo devices, used to make > the MAC think it is connected to a PHY when it is not. The MAC still > gets informed of link speed, etc via the standard PHYLIB API. They are > mostly used for when the Ethernet MAC is directly connected to an > Ethernet Switch, at a MAC to MAC level. > > Now i could be wrong, but are Ethernet switches something you expect > to see on ACPI/SBSA platforms? Or is this a legitimate use of the > escape hatch? I think with the rise in adoption of Smart-NICs in datacenters there will definitely be a lot more crossover between ACPI/SBSA and network appliance oriented hardware. -Jon
Hi Everybody
So i think it is time to try to bring this discussion to some sort of
conclusion.
No ACPI maintainer is willing to ACK any of these patches. Nor are
they willing to NACK them. ACPI maintainers simply don't want to get
involved in making use of ACPI in networking.
I personally don't have the knowledge to do ACPI correctly, review
patches, point people in the right direction. I suspect the same can
be said for the other PHY maintainers.
Having said that, there is clearly a wish from vendors to make use of
ACPI in the networking subsystem to describe hardware.
How do we go forward?
For the moment, we will need to NACK all patches adding ACPI support
to the PHY subsystem.
Vendors who really do want to use ACPI, not device tree, probably
need to get involved in standardisation. Vendors need to submit a
proposal to UEFI and get it accepted.
Developers should try to engage with the ACPI maintainers and see
if they can get them involved in networking. Patches with an
Acked-by from an ACPI maintainer will be accepted, assuming they
fulfil all the other usual requirements. But please don't submit
patches until you do have an ACPI maintainer on board. We don't
want to spamming the lists with NACKs all the time.
Andrew
On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote: > Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: > > Now i could be wrong, but are Ethernet switches something you expect > > to see on ACPI/SBSA platforms? Or is this a legitimate use of the > > escape hatch? > > As an extra data point: right now I am working on an x86 embedded > appliance (ACPI not Device Tree) with 3x integrated Marvell switches. > I have been watching this patch series with great interest, because > right now there is no way for me to configure a complex switch topology > in DSA without Device Tree. > > For the device I am working on, we will have units shipping before these > questions about how to represent Ethernet switches in ACPI can be > resolved. So realistically, we will have to actually configure the > switches using software_node structures supplied by an out-of-tree > platform driver, or some hackery like that, rather than configuring them > through ACPI. Hi Dan I also have an x86 platform, but with a single switch. For that, i have a platform driver, which instantiates a bit banging MDIO bus, and sets up the switch using platform data. This works, but it is limited to internal Copper only PHYs. > An approach I have been toying with is to port all of DSA to use the > fwnode_handle abstraction instead of Device Tree nodes, but that is > obviously a large task, and frankly I was not sure whether such a patch > series would be welcomed. I would actually suggest you look at using DT. We are struggling to get ACPI maintainers involved with really simple things, like the ACPI equivalent of a phandle from the MAC to the PHY. A full DSA binding for Marvell switches is pretty complex, especially if you need SFP support. I expect the ACPI maintainers will actively run away screaming when you make your proposal. DT can be used on x86, and i suspect it is a much easier path of least resistance. Andrew
On 7/28/2020 1:45 PM, Andrew Lunn wrote: > On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote: >> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >>> Now i could be wrong, but are Ethernet switches something you expect >>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >>> escape hatch? >> >> As an extra data point: right now I am working on an x86 embedded >> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. >> I have been watching this patch series with great interest, because >> right now there is no way for me to configure a complex switch topology >> in DSA without Device Tree. >> >> For the device I am working on, we will have units shipping before these >> questions about how to represent Ethernet switches in ACPI can be >> resolved. So realistically, we will have to actually configure the >> switches using software_node structures supplied by an out-of-tree >> platform driver, or some hackery like that, rather than configuring them >> through ACPI. > > Hi Dan > > I also have an x86 platform, but with a single switch. For that, i > have a platform driver, which instantiates a bit banging MDIO bus, and > sets up the switch using platform data. This works, but it is limited > to internal Copper only PHYs. At some point I had a dsa2_platform_data implementation which was intended to describe more complex switch set-ups and trees, the old code is still there for your entertainment: https://github.com/ffainelli/linux/commits/dsa-pdata > >> An approach I have been toying with is to port all of DSA to use the >> fwnode_handle abstraction instead of Device Tree nodes, but that is >> obviously a large task, and frankly I was not sure whether such a patch >> series would be welcomed. > > I would actually suggest you look at using DT. We are struggling to > get ACPI maintainers involved with really simple things, like the ACPI > equivalent of a phandle from the MAC to the PHY. A full DSA binding > for Marvell switches is pretty complex, especially if you need SFP > support. I expect the ACPI maintainers will actively run away > screaming when you make your proposal. > > DT can be used on x86, and i suspect it is a much easier path of least > resistance. And you can easily overlay Device Tree to an existing system by using either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and creating nodes on the fly.
On Tue, Jul 28, 2020 at 10:34:37PM +0200, Andrew Lunn wrote: > Hi Everybody > > So i think it is time to try to bring this discussion to some sort of > conclusion. > > No ACPI maintainer is willing to ACK any of these patches. Nor are > they willing to NACK them. ACPI maintainers simply don't want to get > involved in making use of ACPI in networking. > > I personally don't have the knowledge to do ACPI correctly, review > patches, point people in the right direction. I suspect the same can > be said for the other PHY maintainers. > > Having said that, there is clearly a wish from vendors to make use of > ACPI in the networking subsystem to describe hardware. > > How do we go forward? > > For the moment, we will need to NACK all patches adding ACPI support > to the PHY subsystem. > > Vendors who really do want to use ACPI, not device tree, probably > need to get involved in standardisation. Vendors need to submit a > proposal to UEFI and get it accepted. > > Developers should try to engage with the ACPI maintainers and see > if they can get them involved in networking. Patches with an > Acked-by from an ACPI maintainer will be accepted, assuming they > fulfil all the other usual requirements. But please don't submit > patches until you do have an ACPI maintainer on board. We don't > want to spamming the lists with NACKs all the time. For the record, this statement reflects my position as well (as one of the named phylib maintainers). Thanks Andrew.
On Tue, Jul 28, 2020 at 11:59 PM Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote: > > On Tue, Jul 28, 2020 at 10:34:37PM +0200, Andrew Lunn wrote: > > Hi Everybody > > > > So i think it is time to try to bring this discussion to some sort of > > conclusion. > > > > No ACPI maintainer is willing to ACK any of these patches. Nor are > > they willing to NACK them. ACPI maintainers simply don't want to get > > involved in making use of ACPI in networking. > > > > I personally don't have the knowledge to do ACPI correctly, review > > patches, point people in the right direction. I suspect the same can > > be said for the other PHY maintainers. > > > > Having said that, there is clearly a wish from vendors to make use of > > ACPI in the networking subsystem to describe hardware. > > > > How do we go forward? > > > > For the moment, we will need to NACK all patches adding ACPI support > > to the PHY subsystem. > > > > Vendors who really do want to use ACPI, not device tree, probably > > need to get involved in standardisation. Vendors need to submit a > > proposal to UEFI and get it accepted. > > > > Developers should try to engage with the ACPI maintainers and see > > if they can get them involved in networking. Patches with an > > Acked-by from an ACPI maintainer will be accepted, assuming they > > fulfil all the other usual requirements. But please don't submit > > patches until you do have an ACPI maintainer on board. We don't > > want to spamming the lists with NACKs all the time. > > For the record, this statement reflects my position as well (as one > of the named phylib maintainers). Thanks Andrew. Again, folks, you are discussing something without direct Cc'ing to them (I see a subset? of the maintainers we discussed in another mail). I believe that many maintainers are using some type of scoring for their emails and Cc'ing directly increases chances to get a reply. Also you have at least two or three people in ACPI/arm64. What do they think?
On Tue, Jul 28, 2020 at 11:56 PM Florian Fainelli <f.fainelli@gmail.com> wrote: > On 7/28/2020 1:45 PM, Andrew Lunn wrote: > > On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote: > >> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: > >>> Now i could be wrong, but are Ethernet switches something you expect > >>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the > >>> escape hatch? > >> > >> As an extra data point: right now I am working on an x86 embedded > >> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. > >> I have been watching this patch series with great interest, because > >> right now there is no way for me to configure a complex switch topology > >> in DSA without Device Tree. > >> > >> For the device I am working on, we will have units shipping before these > >> questions about how to represent Ethernet switches in ACPI can be > >> resolved. So realistically, we will have to actually configure the > >> switches using software_node structures supplied by an out-of-tree > >> platform driver, or some hackery like that, rather than configuring them > >> through ACPI. > > > > Hi Dan > > > > I also have an x86 platform, but with a single switch. For that, i > > have a platform driver, which instantiates a bit banging MDIO bus, and > > sets up the switch using platform data. This works, but it is limited > > to internal Copper only PHYs. > > At some point I had a dsa2_platform_data implementation which was > intended to describe more complex switch set-ups and trees, the old code > is still there for your entertainment: > > https://github.com/ffainelli/linux/commits/dsa-pdata Platform data in the modern kernel is definitely the wrong approach. Software nodes of firmware nodes can be much more appreciated. > >> An approach I have been toying with is to port all of DSA to use the > >> fwnode_handle abstraction instead of Device Tree nodes, but that is > >> obviously a large task, and frankly I was not sure whether such a patch > >> series would be welcomed. > > > > I would actually suggest you look at using DT. We are struggling to > > get ACPI maintainers involved with really simple things, like the ACPI > > equivalent of a phandle from the MAC to the PHY. A full DSA binding > > for Marvell switches is pretty complex, especially if you need SFP > > support. I expect the ACPI maintainers will actively run away > > screaming when you make your proposal. > > > > DT can be used on x86, and i suspect it is a much easier path of least > > resistance. > > And you can easily overlay Device Tree to an existing system by using > either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and > creating nodes on the fly. Why do you need DT on a system that runs without it and Linux has all means to extend to cover a lot of stuff DT provides for other types of firmware nodes?
On 7/28/2020 2:28 PM, Andy Shevchenko wrote: > On Tue, Jul 28, 2020 at 11:56 PM Florian Fainelli <f.fainelli@gmail.com> wrote: >> On 7/28/2020 1:45 PM, Andrew Lunn wrote: >>> On Tue, Jul 28, 2020 at 06:06:26PM +1000, Dan Callaghan wrote: >>>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >>>>> Now i could be wrong, but are Ethernet switches something you expect >>>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >>>>> escape hatch? >>>> >>>> As an extra data point: right now I am working on an x86 embedded >>>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. >>>> I have been watching this patch series with great interest, because >>>> right now there is no way for me to configure a complex switch topology >>>> in DSA without Device Tree. >>>> >>>> For the device I am working on, we will have units shipping before these >>>> questions about how to represent Ethernet switches in ACPI can be >>>> resolved. So realistically, we will have to actually configure the >>>> switches using software_node structures supplied by an out-of-tree >>>> platform driver, or some hackery like that, rather than configuring them >>>> through ACPI. >>> >>> Hi Dan >>> >>> I also have an x86 platform, but with a single switch. For that, i >>> have a platform driver, which instantiates a bit banging MDIO bus, and >>> sets up the switch using platform data. This works, but it is limited >>> to internal Copper only PHYs. >> >> At some point I had a dsa2_platform_data implementation which was >> intended to describe more complex switch set-ups and trees, the old code >> is still there for your entertainment: >> >> https://github.com/ffainelli/linux/commits/dsa-pdata > > Platform data in the modern kernel is definitely the wrong approach. > Software nodes of firmware nodes can be much more appreciated. Yes, yes, thank you. As you can see this was back from 2016 and it was never submitted. The only viable alternative that I can think of, unless the ACPI community at large finally decided to get its act together and invest some serious efforts and time into understanding modern and complex network topologies is to overlay a Device Tree onto a live system. > >>>> An approach I have been toying with is to port all of DSA to use the >>>> fwnode_handle abstraction instead of Device Tree nodes, but that is >>>> obviously a large task, and frankly I was not sure whether such a patch >>>> series would be welcomed. >>> >>> I would actually suggest you look at using DT. We are struggling to >>> get ACPI maintainers involved with really simple things, like the ACPI >>> equivalent of a phandle from the MAC to the PHY. A full DSA binding >>> for Marvell switches is pretty complex, especially if you need SFP >>> support. I expect the ACPI maintainers will actively run away >>> screaming when you make your proposal. >>> >>> DT can be used on x86, and i suspect it is a much easier path of least >>> resistance. >> >> And you can easily overlay Device Tree to an existing system by using >> either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and >> creating nodes on the fly. > > Why do you need DT on a system that runs without it and Linux has all > means to extend to cover a lot of stuff DT provides for other types of > firmware nodes? Because ACPI is beyond useless at providing nearly the same level of description as what DT can do today? I am not trying to wage a war of DT is better than ACPI, but when it is not even capable of describing a simple 1 to 1 mapping between an Ethernet MAC and a PHY device sitting on an integrated or separate MDIO bus, describing a full Ethernet switch fabric with 1 to 40 ports, each with a variety of connectivity options, and you have an pressing need to get your platform out to customers, then the choice is obvious.
Hi, On 7/28/20 3:06 AM, Dan Callaghan wrote: > Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >> Now i could be wrong, but are Ethernet switches something you expect >> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >> escape hatch? > > As an extra data point: right now I am working on an x86 embedded > appliance (ACPI not Device Tree) with 3x integrated Marvell switches. > I have been watching this patch series with great interest, because > right now there is no way for me to configure a complex switch topology > in DSA without Device Tree. DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring whether that NIC/MAC is actually hug off PCIe for the moment). It just has a bunch of phy devices strung out on that single MAC/MDIO. So in ACPI land it would still have a relationship similar to the one Andrew pointed out with his DT example where the eth0->mdio->phy are all contained in their physical parent. The phy in that case associated with the parent adapter would be the first direct decedent of the mdio, the switch itself could then be represented with another device, with a further string of device/phys representing the devices. (I dislike drawing acsii art, but if this isn't clear I will, its also worthwhile to look at the dpaa2 docs for how the mac/phys work on this device for contrast.). If so, then its probably possible to represent with a fairly regular looking set of ACPI objects and avoids part of the core discussion here about whether we need a standardized way to pick phy's out of arbitrary parts of the hierarchy using a part of the spec intended for one off problems. Am I missing something? > > For the device I am working on, we will have units shipping before these > questions about how to represent Ethernet switches in ACPI can be > resolved. So realistically, we will have to actually configure the > switches using software_node structures supplied by an out-of-tree > platform driver, or some hackery like that, rather than configuring them > through ACPI. > > An approach I have been toying with is to port all of DSA to use the > fwnode_handle abstraction instead of Device Tree nodes, but that is > obviously a large task, and frankly I was not sure whether such a patch > series would be welcomed. >
On 7/28/2020 3:30 PM, Jeremy Linton wrote: > Hi, > > On 7/28/20 3:06 AM, Dan Callaghan wrote: >> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >>> Now i could be wrong, but are Ethernet switches something you expect >>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >>> escape hatch? >> >> As an extra data point: right now I am working on an x86 embedded >> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. >> I have been watching this patch series with great interest, because >> right now there is no way for me to configure a complex switch topology >> in DSA without Device Tree. > > DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring > whether that NIC/MAC is actually hug off PCIe for the moment). There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches all supported within the driver framework right now. > > It just has a bunch of phy devices strung out on that single MAC/MDIO. It has a number of built-in PHYs that typically appear on a MDIO bus, whether that bus is the switch's internal MDIO bus, or another MDIO bus (which could be provided with just two GPIOs) depends on how the switch is connected to its management host. When the switch is interfaced via MDIO the switch also typically has a MDIO interface called the pseudo-PHY which is how you can actually tap into the control interface of the switch, as opposed to reading its internal PHYs from the MDIO bus. > So in ACPI land it would still have a relationship similar to the one > Andrew pointed out with his DT example where the eth0->mdio->phy are all > contained in their physical parent. The phy in that case associated with > the parent adapter would be the first direct decedent of the mdio, the > switch itself could then be represented with another device, with a > further string of device/phys representing the devices. (I dislike > drawing acsii art, but if this isn't clear I will, its also worthwhile > to look at the dpaa2 docs for how the mac/phys work on this device for > contrast.). The eth0->mdio->phy relationship you describe is the simple case that you are well aware of which is say what we have on the Raspberry Pi 4 with GENET and the external Broadcom PHY. For an Ethernet switch connected to an Ethernet MAC, we have 4 different types of objects: - the Ethernet MAC which sits on its specific bus - the Ethernet switch which also sits on its specific bus - the built-in PHYs of the Ethernet switch which sit on whatever bus/interface the switch provides to make them accessible - the specific bus controller that provides access to the Ethernet switch and this is a simplification that does not take into account Physical Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules. > > If so, then its probably possible to represent with a fairly regular > looking set of ACPI objects and avoids part of the core discussion here > about whether we need a standardized way to pick phy's out of arbitrary > parts of the hierarchy using a part of the spec intended for one off > problems. Using regular ACPI objects would work, however I do not see how it can alleviate having this discussion. It has been repeated again and again that we do not want to see snowflake ACPI representation that each and every driver writer is going to draw inspiration from. Upon further reading of the ACPI specification, I do not think we are going to see much definition or a driving force show up about how the Ethernet objects (MAC, PHY, switches, SFPs, etc.) relate to one another. The ACPI specification seems to be more about defining the ACPI objects and their methods, which is on a different scope than how to tie them together. What Calvin has done thus far is the closest to what I believe we can achieve given how nebulous and arcane ACPI is for the PHY library maintainers within this context.
Hi, On 7/28/20 7:39 PM, Florian Fainelli wrote: > On 7/28/2020 3:30 PM, Jeremy Linton wrote: >> Hi, >> >> On 7/28/20 3:06 AM, Dan Callaghan wrote: >>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >>>> Now i could be wrong, but are Ethernet switches something you expect >>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >>>> escape hatch? >>> >>> As an extra data point: right now I am working on an x86 embedded >>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. >>> I have been watching this patch series with great interest, because >>> right now there is no way for me to configure a complex switch topology >>> in DSA without Device Tree. >> >> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring >> whether that NIC/MAC is actually hug off PCIe for the moment). > > There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches > all supported within the driver framework right now. > >> >> It just has a bunch of phy devices strung out on that single MAC/MDIO. > > It has a number of built-in PHYs that typically appear on a MDIO bus, > whether that bus is the switch's internal MDIO bus, or another MDIO bus > (which could be provided with just two GPIOs) depends on how the switch > is connected to its management host. > > When the switch is interfaced via MDIO the switch also typically has a > MDIO interface called the pseudo-PHY which is how you can actually tap > into the control interface of the switch, as opposed to reading its > internal PHYs from the MDIO bus. > >> So in ACPI land it would still have a relationship similar to the one >> Andrew pointed out with his DT example where the eth0->mdio->phy are all >> contained in their physical parent. The phy in that case associated with >> the parent adapter would be the first direct decedent of the mdio, the >> switch itself could then be represented with another device, with a >> further string of device/phys representing the devices. (I dislike >> drawing acsii art, but if this isn't clear I will, its also worthwhile >> to look at the dpaa2 docs for how the mac/phys work on this device for >> contrast.). > > The eth0->mdio->phy relationship you describe is the simple case that > you are well aware of which is say what we have on the Raspberry Pi 4 > with GENET and the external Broadcom PHY. > > For an Ethernet switch connected to an Ethernet MAC, we have 4 different > types of objects: > > - the Ethernet MAC which sits on its specific bus > > - the Ethernet switch which also sits on its specific bus > > - the built-in PHYs of the Ethernet switch which sit on whatever > bus/interface the switch provides to make them accessible > > - the specific bus controller that provides access to the Ethernet switch > > and this is a simplification that does not take into account Physical > Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet > land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules. Which is why I've stayed away from much of the switch discussion. There are a lot of edge cases to fall into, because for whatever reason networking seems to be unique in all this non-enumerable customization vs many other areas of the system. Storage, being an example i'm more familiar with which has very similar problems yet, somehow has managed to avoid much of this, despite having run on fabrics significantly more complex than basic ethernet (including running on a wide range of hot pluggable GBIC/SFP/QSFP/etc media layers). ACPI's "problem" here is that its strongly influenced by the "Plug and Play" revolution of the 1990's where the industry went from having humans describing hardware using machine readable languages, to hardware which was enumerable using standard protocols. ACPI's device descriptions are there as a crutch for the remaining non plug an play hardware in the system. So at a basic level, if your describing hardware in ACPI rather than abstracting it, that is a problem. Thanks,
On 7/28/2020 7:53 PM, Jeremy Linton wrote: > Hi, > > On 7/28/20 7:39 PM, Florian Fainelli wrote: >> On 7/28/2020 3:30 PM, Jeremy Linton wrote: >>> Hi, >>> >>> On 7/28/20 3:06 AM, Dan Callaghan wrote: >>>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: >>>>> Now i could be wrong, but are Ethernet switches something you expect >>>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the >>>>> escape hatch? >>>> >>>> As an extra data point: right now I am working on an x86 embedded >>>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. >>>> I have been watching this patch series with great interest, because >>>> right now there is no way for me to configure a complex switch topology >>>> in DSA without Device Tree. >>> >>> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring >>> whether that NIC/MAC is actually hug off PCIe for the moment). >> >> There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches >> all supported within the driver framework right now. >> >>> >>> It just has a bunch of phy devices strung out on that single MAC/MDIO. >> >> It has a number of built-in PHYs that typically appear on a MDIO bus, >> whether that bus is the switch's internal MDIO bus, or another MDIO bus >> (which could be provided with just two GPIOs) depends on how the switch >> is connected to its management host. >> >> When the switch is interfaced via MDIO the switch also typically has a >> MDIO interface called the pseudo-PHY which is how you can actually tap >> into the control interface of the switch, as opposed to reading its >> internal PHYs from the MDIO bus. >> >>> So in ACPI land it would still have a relationship similar to the one >>> Andrew pointed out with his DT example where the eth0->mdio->phy are all >>> contained in their physical parent. The phy in that case associated with >>> the parent adapter would be the first direct decedent of the mdio, the >>> switch itself could then be represented with another device, with a >>> further string of device/phys representing the devices. (I dislike >>> drawing acsii art, but if this isn't clear I will, its also worthwhile >>> to look at the dpaa2 docs for how the mac/phys work on this device for >>> contrast.). >> >> The eth0->mdio->phy relationship you describe is the simple case that >> you are well aware of which is say what we have on the Raspberry Pi 4 >> with GENET and the external Broadcom PHY. >> >> For an Ethernet switch connected to an Ethernet MAC, we have 4 different >> types of objects: >> >> - the Ethernet MAC which sits on its specific bus >> >> - the Ethernet switch which also sits on its specific bus >> >> - the built-in PHYs of the Ethernet switch which sit on whatever >> bus/interface the switch provides to make them accessible >> >> - the specific bus controller that provides access to the Ethernet switch >> >> and this is a simplification that does not take into account Physical >> Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet >> land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable >> modules. > > Which is why I've stayed away from much of the switch discussion. There > are a lot of edge cases to fall into, because for whatever reason > networking seems to be unique in all this non-enumerable customization > vs many other areas of the system. Storage, being an example i'm more > familiar with which has very similar problems yet, somehow has managed > to avoid much of this, despite having run on fabrics significantly more > complex than basic ethernet (including running on a wide range of hot > pluggable GBIC/SFP/QSFP/etc media layers). > > ACPI's "problem" here is that its strongly influenced by the "Plug and > Play" revolution of the 1990's where the industry went from having > humans describing hardware using machine readable languages, to hardware > which was enumerable using standard protocols. ACPI's device > descriptions are there as a crutch for the remaining non plug an play > hardware in the system. > > So at a basic level, if your describing hardware in ACPI rather than > abstracting it, that is a problem. I suppose that is a good summary, my impression from this patch series is that we want the description part, not the abstraction, whether it is on purpose or because there is a misunderstanding of what ACPI is intended for, or higher powers have decided this must be done otherwise nothing gets sold, who knows?
On Wed, Jul 29, 2020 at 4:53 AM Jeremy Linton <jeremy.linton@arm.com> wrote: > > Hi, > > On 7/28/20 7:39 PM, Florian Fainelli wrote: > > On 7/28/2020 3:30 PM, Jeremy Linton wrote: > >> Hi, > >> > >> On 7/28/20 3:06 AM, Dan Callaghan wrote: > >>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: > >>>> Now i could be wrong, but are Ethernet switches something you expect > >>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the > >>>> escape hatch? > >>> > >>> As an extra data point: right now I am working on an x86 embedded > >>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. > >>> I have been watching this patch series with great interest, because > >>> right now there is no way for me to configure a complex switch topology > >>> in DSA without Device Tree. > >> > >> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring > >> whether that NIC/MAC is actually hug off PCIe for the moment). > > > > There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches > > all supported within the driver framework right now. > > > >> > >> It just has a bunch of phy devices strung out on that single MAC/MDIO. > > > > It has a number of built-in PHYs that typically appear on a MDIO bus, > > whether that bus is the switch's internal MDIO bus, or another MDIO bus > > (which could be provided with just two GPIOs) depends on how the switch > > is connected to its management host. > > > > When the switch is interfaced via MDIO the switch also typically has a > > MDIO interface called the pseudo-PHY which is how you can actually tap > > into the control interface of the switch, as opposed to reading its > > internal PHYs from the MDIO bus. > > > >> So in ACPI land it would still have a relationship similar to the one > >> Andrew pointed out with his DT example where the eth0->mdio->phy are all > >> contained in their physical parent. The phy in that case associated with > >> the parent adapter would be the first direct decedent of the mdio, the > >> switch itself could then be represented with another device, with a > >> further string of device/phys representing the devices. (I dislike > >> drawing acsii art, but if this isn't clear I will, its also worthwhile > >> to look at the dpaa2 docs for how the mac/phys work on this device for > >> contrast.). > > > > The eth0->mdio->phy relationship you describe is the simple case that > > you are well aware of which is say what we have on the Raspberry Pi 4 > > with GENET and the external Broadcom PHY. > > > > For an Ethernet switch connected to an Ethernet MAC, we have 4 different > > types of objects: > > > > - the Ethernet MAC which sits on its specific bus > > > > - the Ethernet switch which also sits on its specific bus > > > > - the built-in PHYs of the Ethernet switch which sit on whatever > > bus/interface the switch provides to make them accessible > > > > - the specific bus controller that provides access to the Ethernet switch > > > > and this is a simplification that does not take into account Physical > > Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet > > land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules. > > Which is why I've stayed away from much of the switch discussion. There > are a lot of edge cases to fall into, because for whatever reason > networking seems to be unique in all this non-enumerable customization > vs many other areas of the system. Storage, being an example i'm more > familiar with which has very similar problems yet, somehow has managed > to avoid much of this, despite having run on fabrics significantly more > complex than basic ethernet (including running on a wide range of hot > pluggable GBIC/SFP/QSFP/etc media layers). > > ACPI's "problem" here is that its strongly influenced by the "Plug and > Play" revolution of the 1990's where the industry went from having > humans describing hardware using machine readable languages, to hardware > which was enumerable using standard protocols. ACPI's device > descriptions are there as a crutch for the remaining non plug an play > hardware in the system. > > So at a basic level, if your describing hardware in ACPI rather than > abstracting it, that is a problem. > This is also my first run with ACPI and this discussion needs to be brought back to the powers that be regarding sorting this out. This is where I see it from an Armada and Layerscape perspective. This isn't a silver bullet fix but the small things I think that need to be done to move this forward. From Microsoft's documentation. Device dependencies Typically, there are hardware dependencies between devices on a particular platform. Windows requires that all such dependencies be described so that it can ensure that all devices function correctly as things change dynamically in the system (device power is removed, drivers are stopped and started, and so on). In ACPI, dependencies between devices are described in the following ways: 1) Namespace hierarchy. Any device that is a child device (listed as a device within the namespace of another device) is dependent on the parent device. For example, a USB HSIC device is dependent on the port (parent) and controller (grandparent) it is connected to. Similarly, a GPU device listed within the namespace of a system memory-management unit (MMU) device is dependent on the MMU device. 2) Resource connections. Devices connected to GPIO or SPB controllers are dependent on those controllers. This type of dependency is described by the inclusion of Connection Resources in the device's _CRS. 3) OpRegion dependencies. For ASL control methods that use OpRegions to perform I/O, dependencies are not implicitly known by the operating system because they are only determined during control method evaluation. This issue is particularly applicable to GeneralPurposeIO and GenericSerialBus OpRegions in which Plug and Play drivers provide access to the region. To mitigate this issue, ACPI defines the OpRegion Dependency (_DEP) object. _DEP should be used in any device namespace in which an OpRegion (HW resource) is referenced by a control method, and neither 1 nor 2 above already applies for the referenced OpRegion's connection resource. For more information, see section 6.5.8, "_DEP (Operation Region Dependencies)", of the ACPI 5.0 specification. We can forget about 3 because even though _DEP would solve many of our problems, and Intel has kind of used it for some of their architectures, according to the ACPI spec it should not be used this way. 1) can be achievable on some platforms like the LX2160a. We have the mcbin firmware which is the bus (the ACPI spec does allow you to define a platform defined bus), which has MACs as the children, which then can have phy's or SFP modules as their children. This works okay for enumeration and parenting but how do they talk? This is where 2) comes into play. The big problem is that MDIO isn't designated as a SPB (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/simple-peripheral-bus--spb-) We have GPIO, I2C, SPI, UART, MIPI and a couple of others. While not a silver bullet I think getting MDIO added to the spec would be the next step forward to being able to implement this in Linux with phylink / phylib in a sane manner. Currently SFP definitions are using the SPB to designate the various GPIO and I2C interfaces that are needed to probe devices and handle interrupts. The other alternatives is the ACPI maintainers agree on the _DSD method (would be quickest and should be easy to migrate to SBP if MDIO were adopter), or nothing is done at all (which I know seems to be a popular opinion). -Jon
On Wed, Jul 29, 2020 at 10:43:34AM +0200, Jon Nettleton wrote: > On Wed, Jul 29, 2020 at 4:53 AM Jeremy Linton <jeremy.linton@arm.com> wrote: > > > > Hi, > > > > On 7/28/20 7:39 PM, Florian Fainelli wrote: > > > On 7/28/2020 3:30 PM, Jeremy Linton wrote: > > >> Hi, > > >> > > >> On 7/28/20 3:06 AM, Dan Callaghan wrote: > > >>> Excerpts from Andrew Lunn's message of 2020-07-24 21:14:36 +02:00: > > >>>> Now i could be wrong, but are Ethernet switches something you expect > > >>>> to see on ACPI/SBSA platforms? Or is this a legitimate use of the > > >>>> escape hatch? > > >>> > > >>> As an extra data point: right now I am working on an x86 embedded > > >>> appliance (ACPI not Device Tree) with 3x integrated Marvell switches. > > >>> I have been watching this patch series with great interest, because > > >>> right now there is no way for me to configure a complex switch topology > > >>> in DSA without Device Tree. > > >> > > >> DSA though, the switch is hung off a normal MAC/MDIO, right? (ignoring > > >> whether that NIC/MAC is actually hug off PCIe for the moment). > > > > > > There is no specific bus, we have memory mapped, MDIO, SPI, I2C swiches > > > all supported within the driver framework right now. > > > > > >> > > >> It just has a bunch of phy devices strung out on that single MAC/MDIO. > > > > > > It has a number of built-in PHYs that typically appear on a MDIO bus, > > > whether that bus is the switch's internal MDIO bus, or another MDIO bus > > > (which could be provided with just two GPIOs) depends on how the switch > > > is connected to its management host. > > > > > > When the switch is interfaced via MDIO the switch also typically has a > > > MDIO interface called the pseudo-PHY which is how you can actually tap > > > into the control interface of the switch, as opposed to reading its > > > internal PHYs from the MDIO bus. > > > > > >> So in ACPI land it would still have a relationship similar to the one > > >> Andrew pointed out with his DT example where the eth0->mdio->phy are all > > >> contained in their physical parent. The phy in that case associated with > > >> the parent adapter would be the first direct decedent of the mdio, the > > >> switch itself could then be represented with another device, with a > > >> further string of device/phys representing the devices. (I dislike > > >> drawing acsii art, but if this isn't clear I will, its also worthwhile > > >> to look at the dpaa2 docs for how the mac/phys work on this device for > > >> contrast.). > > > > > > The eth0->mdio->phy relationship you describe is the simple case that > > > you are well aware of which is say what we have on the Raspberry Pi 4 > > > with GENET and the external Broadcom PHY. > > > > > > For an Ethernet switch connected to an Ethernet MAC, we have 4 different > > > types of objects: > > > > > > - the Ethernet MAC which sits on its specific bus > > > > > > - the Ethernet switch which also sits on its specific bus > > > > > > - the built-in PHYs of the Ethernet switch which sit on whatever > > > bus/interface the switch provides to make them accessible > > > > > > - the specific bus controller that provides access to the Ethernet switch > > > > > > and this is a simplification that does not take into account Physical > > > Coding Sublayer devices, pure MDIO devices (with no foot in the Ethernet > > > land such as PCIe, USB3 or SATA PHYs), SFP, SFF and other pluggable modules. > > > > Which is why I've stayed away from much of the switch discussion. There > > are a lot of edge cases to fall into, because for whatever reason > > networking seems to be unique in all this non-enumerable customization > > vs many other areas of the system. Storage, being an example i'm more > > familiar with which has very similar problems yet, somehow has managed > > to avoid much of this, despite having run on fabrics significantly more > > complex than basic ethernet (including running on a wide range of hot > > pluggable GBIC/SFP/QSFP/etc media layers). > > > > ACPI's "problem" here is that its strongly influenced by the "Plug and > > Play" revolution of the 1990's where the industry went from having > > humans describing hardware using machine readable languages, to hardware > > which was enumerable using standard protocols. ACPI's device > > descriptions are there as a crutch for the remaining non plug an play > > hardware in the system. > > > > So at a basic level, if your describing hardware in ACPI rather than > > abstracting it, that is a problem. > > > This is also my first run with ACPI and this discussion needs to be > brought back to the powers that be regarding sorting this out. This > is where I see it from an Armada and Layerscape perspective. This > isn't a silver bullet fix but the small things I think that need to be > done to move this forward. > > From Microsoft's documentation. > > Device dependencies > > Typically, there are hardware dependencies between devices on a > particular platform. Windows requires that all such dependencies be > described so that it can ensure that all devices function correctly as > things change dynamically in the system (device power is removed, > drivers are stopped and started, and so on). In ACPI, dependencies > between devices are described in the following ways: > > 1) Namespace hierarchy. Any device that is a child device (listed as a > device within the namespace of another device) is dependent on the > parent device. For example, a USB HSIC device is dependent on the port > (parent) and controller (grandparent) it is connected to. Similarly, a > GPU device listed within the namespace of a system memory-management > unit (MMU) device is dependent on the MMU device. > > 2) Resource connections. Devices connected to GPIO or SPB controllers > are dependent on those controllers. This type of dependency is > described by the inclusion of Connection Resources in the device's > _CRS. > > 3) OpRegion dependencies. For ASL control methods that use OpRegions > to perform I/O, dependencies are not implicitly known by the operating > system because they are only determined during control method > evaluation. This issue is particularly applicable to GeneralPurposeIO > and GenericSerialBus OpRegions in which Plug and Play drivers provide > access to the region. To mitigate this issue, ACPI defines the > OpRegion Dependency (_DEP) object. _DEP should be used in any device > namespace in which an OpRegion (HW resource) is referenced by a > control method, and neither 1 nor 2 above already applies for the > referenced OpRegion's connection resource. For more information, see > section 6.5.8, "_DEP (Operation Region Dependencies)", of the ACPI 5.0 > specification. > > We can forget about 3 because even though _DEP would solve many of our > problems, and Intel has kind of used it for some of their > architectures, according to the ACPI spec it should not be used this > way. > > 1) can be achievable on some platforms like the LX2160a. We have the > mcbin firmware which is the bus (the ACPI spec does allow you to > define a platform defined bus), which has MACs as the children, which > then can have phy's or SFP modules as their children. This works okay > for enumeration and parenting but how do they talk? > > This is where 2) comes into play. The big problem is that MDIO isn't > designated as a SPB > (https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/simple-peripheral-bus--spb-) > We have GPIO, I2C, SPI, UART, MIPI and a couple of others. While not > a silver bullet I think getting MDIO added to the spec would be the > next step forward to being able to implement this in Linux with > phylink / phylib in a sane manner. Currently SFP definitions are > using the SPB to designate the various GPIO and I2C interfaces that > are needed to probe devices and handle interrupts. > > The other alternatives is the ACPI maintainers agree on the _DSD > method (would be quickest and should be easy to migrate to SBP if MDIO > were adopter), or nothing is done at all (which I know seems to be a > popular opinion). > Before other ACPI experts miss any further discussion let me add them to the loop. Hi ACPI maintainers, please have a look at the discussion and some conclusions in this thread: https://lore.kernel.org/linux-acpi/20200715090400.4733-1-calvin.johnson@oss.nxp.com/T/#t Discussion is around adding ACPI support into the PHY subsystem. Thanks Calvin
Hi Andrew, On Tue, Jul 28, 2020 at 10:34 PM Andrew Lunn <andrew@lunn.ch> wrote: > > Hi Everybody > > So i think it is time to try to bring this discussion to some sort of > conclusion. > > No ACPI maintainer is willing to ACK any of these patches. Nor are > they willing to NACK them. Let's first clarify one thing: ACPI maintainers take care of the generic code implementing the interactions between the OS and the platform firmware in accordance with ACPI (which is an interface specification to be precise). They do not set rules on what ACPI-related things device drivers can do. Those rules are set in the ACPI specification and other standard definitions (like PCI, USB, etc.) and they just need to be followed. So ACPI maintainers cannot "bless" any new rule to be followed going forward. An ACPI maintainer can tell you whether or not some driver code follows the rules set by the ACPI specification (or conventions related to using the ACPI support code in the kernel) and that's about it. In this particular case, a bit of ACPI-related code is there in the last two patches and it doesn't look broken. It uses the ACPI side of the device properties API correctly AFAICS. Also, from a slightly broader perspective, this patch series adds an ability to look up certain device properties in the ACPI namespace. That appears to be done in accordance with all of the rules set so far, so there is nothing wrong with it in principle. However, if those properties are never going to be supplied via ACPI on any production systems, the code added in order to be able to process them will turn out to be useless and I don't think anyone wants useless code in the kernel. So the real question is whether or not there will be production systems in which those properties will be supplied via ACPI and I cannot answer that question. Therefore I cannot ACK the patches (because I don't know whether or not the code added by them is going to be useful), but I cannot NACK them either, because the code added by them looks correct to me. > ACPI maintainers simply don't want to get > involved in making use of ACPI in networking. That's not about making use of ACPI in networking in general (which already happens in many ways), but about a specific use of ACPI for a specific purpose related to networking. > I personally don't have the knowledge to do ACPI correctly, review > patches, point people in the right direction. I suspect the same can > be said for the other PHY maintainers. > > Having said that, there is clearly a wish from vendors to make use of > ACPI in the networking subsystem to describe hardware. > > How do we go forward? Basically, the interested vendors need to agree on how exactly they want ACPI to be used and come up with a specification setting the rules to be followed by the platform firmware on the one side and by the code in the kernel on the other side. > For the moment, we will need to NACK all patches adding ACPI support > to the PHY subsystem. > > Vendors who really do want to use ACPI, not device tree, probably > need to get involved in standardisation. Vendors need to submit a > proposal to UEFI and get it accepted. The UEFI Forum maintains the ACPI specification itself (so changes to the specification need to be accepted by it), but it is not uncommon for a group of vendors (or even one vendor in some cases) to come up with additional rules and specify them separately. Of course, involving the UEFI Forum may help to simplify things from the legal and spec hosting perspective, but I don't think it is mandatory. In the particular case at hand the additional rules may just be based on the analogous DT bindings, but they need to be officially defined. > Developers should try to engage with the ACPI maintainers and see > if they can get them involved in networking. Patches with an > Acked-by from an ACPI maintainer will be accepted, assuming they > fulfil all the other usual requirements. But please don't submit > patches until you do have an ACPI maintainer on board. We don't > want to spamming the lists with NACKs all the time. Well, do you ask for a PCI maintainer ACK on a patch adding a PCI driver for a NIC as a rule? If not, I don't see a reason for making ACPI an exception. Besides, you really should be asking for a spec the work is based on, IMO, instead of asking for an ACPI maintainer ACK which is not going to be sufficient if the former is missing anyway. Thanks!
> However, if those properties are never going to be supplied via ACPI > on any production systems, the code added in order to be able to > process them will turn out to be useless and I don't think anyone > wants useless code in the kernel. > > So the real question is whether or not there will be production > systems in which those properties will be supplied via ACPI and I > cannot answer that question. Hi Rafael I suspect we are going to have a lot of newbie ACPI questions over the next few weeks/months as vendors and the PHY maintainers get up to speed on all this. In the device tree world, we would expect a patch as part of the patchset to a device tree file somewhere under arch/*/boot/dts to make use of any new property added. We then know it is used. How does this work in the ACPI world? How does somebody show they are supplying the property in a production system? > Basically, the interested vendors need to agree on how exactly they > want ACPI to be used and come up with a specification setting the > rules to be followed by the platform firmware on the one side and by > the code in the kernel on the other side. ... > Besides, you really should be asking for a spec the work is based on, > IMO, instead of asking for an ACPI maintainer ACK which is not going > to be sufficient if the former is missing anyway. Could you point us towards real world example specs? Giving us a good best practice example would likely help us to do this work. And reduce the amount of work you need to do keeping the process going in the correct direction. Thanks Andrew
> > > DT can be used on x86, and i suspect it is a much easier path of least > > > resistance. > > > > And you can easily overlay Device Tree to an existing system by using > > either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and > > creating nodes on the fly. > > Why do you need DT on a system that runs without it and Linux has all > means to extend to cover a lot of stuff DT provides for other types of > firmware nodes? As i said, path of least resistance. It is here today, heavily used, well understood by lots of network developers, has a very active maintainer in the form of Rob Herring, and avoids 'showflakes' as Florian likes to call it, so we are all sharing the same code, providing a lot of testing and maintenance. Andrew
On 31/07/2020 16:14, Andrew Lunn wrote: >>>> DT can be used on x86, and i suspect it is a much easier path of least >>>> resistance. >>> >>> And you can easily overlay Device Tree to an existing system by using >>> either a full Device Tree overlay (dtbo) or using CONFIG_OF_DYNAMIC and >>> creating nodes on the fly. >> >> Why do you need DT on a system that runs without it and Linux has all >> means to extend to cover a lot of stuff DT provides for other types of >> firmware nodes? > > As i said, path of least resistance. It is here today, heavily used, > well understood by lots of network developers, has a very active > maintainer in the form of Rob Herring, and avoids 'showflakes' as > Florian likes to call it, so we are all sharing the same code, > providing a lot of testing and maintenance. > > Andrew Hi Andrew, I'm just coming into this thread now. With my alumni DT-maintainer had on I think that trying to use ACPI & DT on the same system is the worst of both worlds. Trying to do so makes the solution far more complicated than either an ACPI-only or DT-only approach. There is no good way for references between DT & ACPI nodes. I have serious doubts about the reliability of the dynamic DT code in the kernel. Perhaps most problematic is it excludes platform specific data from the ACPI description provided by firmware, which means platform-specific data needs to be shipped with the OS. Rather defeats the whole point of firmware providing the platform description. An ACPI solution is absolutely needed. Regarding this specific series, I think it is approximately the right approach. I have some specific concerns that I've talked with Calvin about and I'm going to post as replies to the individual patches. My most significant concern is the reference from the ACPI MAC node to the MDIO node, which makes little sense. The MAC should have a reference to the PHY node. There have been other issues raised in this thread. I'm going to go back and respond to a few of those points in separate emails, but as a larger issue I think there is a fair bit of misunderstanding on what ACPI does and does not do, and how much is expected to be standardized in ACPI specs. In the ACPI world the typical model is the firmware/platform vendor decides what data to put into the ACPI nodes that works for them, and then the OS just has to deal with it. Linux typically never gets a choice about what goes into ACPI nodes. Already, threads like this one are setting the bar *far* higher on ACPI schema than has ever been done before. I do think it is right to be asking for a common data model for describing PHY connections. Lining the model up with the DT PHY model is also valuable because we can use common code. I also think as first through the door, what gets accepted (after review) for the layerscape platforms here should become the defacto standard that other vendors are expected to adopt, and I have very high confidence that it will be acceptable because it follow the pattern already used in devicetree. Cheers, g.
On 15/07/2020 10:03, Calvin Johnson wrote: > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > provide them to be connected to MAC. > > An ACPI node property "mdio-handle" is introduced to reference the > MDIO bus on which PHYs are registered with autoprobing method used > by mdiobus_register(). > > Describe properties "phy-channel" and "phy-mode" > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > Changes in v7: None > Changes in v6: None > Changes in v5: None > Changes in v4: None > Changes in v3: > - cleanup based on v2 comments > - Added description for more properties > - Added MDIO node DSDT entry > > Changes in v2: None > > Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++ > 1 file changed, 90 insertions(+) > create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst > > diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst > new file mode 100644 > index 000000000000..0132fee10b45 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,90 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an mdiobus are probed and registered using mdiobus_register(). > +Later, for connecting these PHYs to MAC, the PHYs registered on the > +mdiobus have to be referenced. > + > +mdio-handle > +----------- > +For each MAC node, a property "mdio-handle" is used to reference the > +MDIO bus on which the PHYs are registered. On getting hold of the MDIO > +bus, use find_phy_device() to get the PHY connected to the MAC. > + > +phy-channel > +----------- > +Property "phy-channel" defines the address of the PHY on the mdiobus. Hi Calvin, As we discussed offline, using 'mdio-handle'+'phy-channel' doesn't make a lot of sense. The MAC should be referencing the PHY it is attached to, not the MDIO bus. Referencing the PHY makes assumptions about how the PHY is wired into the system, which may not be via a traditional MDIO bus. These two properties should be dropped, and replaced with a single property reference to the PHY node. e.g., Package () {"phy-handle", Package (){\_SB.MDI0.PHY1}} This is also future proof against any changes to how MDIO busses may get modeled in the future. They can be modeled as normal devices now, but if a future version of the ACPI spec adds an MDIO bus type, then the reference to the PHY from the MAC doesn't need to change. > + > +phy-mode > +-------- > +Property "phy-mode" defines the type of PHY interface. > + > +An example of this is shown below:: > + > +DSDT entry for MAC where MDIO node is referenced > +------------------------------------------------ > + Scope(\_SB.MCE0.PR17) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"phy-channel", 1}, > + Package () {"phy-mode", "rgmii-id"}, > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"phy-channel", 2}, > + Package () {"phy-mode", "rgmii-id"}, > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > + } > + }) > + } > + > +DSDT entry for MDIO node > +------------------------ > +a) Silicon Component > +-------------------- > + Scope(_SB) > + { > + Device(MDI0) { > + Name(_HID, "NXP0006") > + Name(_CCA, 1) > + Name(_UID, 0) > + Name(_CRS, ResourceTemplate() { > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > + { > + MDI0_IT > + } > + }) // end of _CRS for MDI0 > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package () {"little-endian", 1}, > + } Adopting the 'little-endian' property here makes little sense. This looks like legacy from old PowerPC DT platforms that doesn't belong here. I would drop this bit. > + }) > + } // end of MDI0 > + } > + > +b) Platform Component > +--------------------- > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } >
On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > On 15/07/2020 10:03, Calvin Johnson wrote: > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > provide them to be connected to MAC. > > > > An ACPI node property "mdio-handle" is introduced to reference the > > MDIO bus on which PHYs are registered with autoprobing method used > > by mdiobus_register(). > > > > Describe properties "phy-channel" and "phy-mode" > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > --- > > > > Changes in v7: None > > Changes in v6: None > > Changes in v5: None > > Changes in v4: None > > Changes in v3: > > - cleanup based on v2 comments > > - Added description for more properties > > - Added MDIO node DSDT entry > > > > Changes in v2: None > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++ > > 1 file changed, 90 insertions(+) > > create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst > > > > diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst > > new file mode 100644 > > index 000000000000..0132fee10b45 > > --- /dev/null > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > @@ -0,0 +1,90 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +========================= > > +MDIO bus and PHYs in ACPI > > +========================= > > + > > +The PHYs on an mdiobus are probed and registered using mdiobus_register(). > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > +mdiobus have to be referenced. > > + > > +mdio-handle > > +----------- > > +For each MAC node, a property "mdio-handle" is used to reference the > > +MDIO bus on which the PHYs are registered. On getting hold of the MDIO > > +bus, use find_phy_device() to get the PHY connected to the MAC. > > + > > +phy-channel > > +----------- > > +Property "phy-channel" defines the address of the PHY on the mdiobus. > > Hi Calvin, > > As we discussed offline, using 'mdio-handle'+'phy-channel' doesn't make a > lot of sense. The MAC should be referencing the PHY it is attached to, not > the MDIO bus. Referencing the PHY makes assumptions about how the PHY is > wired into the system, which may not be via a traditional MDIO bus. These > two properties should be dropped, and replaced with a single property > reference to the PHY node. > > e.g., > Package () {"phy-handle", Package (){\_SB.MDI0.PHY1}} > > This is also future proof against any changes to how MDIO busses may get > modeled in the future. They can be modeled as normal devices now, but if a > future version of the ACPI spec adds an MDIO bus type, then the reference to > the PHY from the MAC doesn't need to change. > Hi Grant, Understood. I'll make the changes. > > + > > +phy-mode > > +-------- > > +Property "phy-mode" defines the type of PHY interface. > > + > > +An example of this is shown below:: > > + > > +DSDT entry for MAC where MDIO node is referenced > > +------------------------------------------------ > > + Scope(\_SB.MCE0.PR17) // 1G > > + { > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package () {"phy-channel", 1}, > > + Package () {"phy-mode", "rgmii-id"}, > > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > > + } > > + }) > > + } > > + > > + Scope(\_SB.MCE0.PR18) // 1G > > + { > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package () {"phy-channel", 2}, > > + Package () {"phy-mode", "rgmii-id"}, > > + Package () {"mdio-handle", Package (){\_SB.MDI0}} > > + } > > + }) > > + } > > + > > +DSDT entry for MDIO node > > +------------------------ > > +a) Silicon Component > > +-------------------- > > + Scope(_SB) > > + { > > + Device(MDI0) { > > + Name(_HID, "NXP0006") > > + Name(_CCA, 1) > > + Name(_UID, 0) > > + Name(_CRS, ResourceTemplate() { > > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > + { > > + MDI0_IT > > + } > > + }) // end of _CRS for MDI0 > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package () {"little-endian", 1}, > > + } > > Adopting the 'little-endian' property here makes little sense. This looks > like legacy from old PowerPC DT platforms that doesn't belong here. I would > drop this bit. Ok. Will drop it. Thanks Calvin
Hi Grant, On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > +DSDT entry for MDIO node > > +------------------------ > > +a) Silicon Component > > +-------------------- > > + Scope(_SB) > > + { > > + Device(MDI0) { > > + Name(_HID, "NXP0006") > > + Name(_CCA, 1) > > + Name(_UID, 0) > > + Name(_CRS, ResourceTemplate() { > > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > + { > > + MDI0_IT > > + } > > + }) // end of _CRS for MDI0 > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package () {"little-endian", 1}, > > + } > > Adopting the 'little-endian' property here makes little sense. This looks > like legacy from old PowerPC DT platforms that doesn't belong here. I would > drop this bit. I'm unable to drop this as the xgmac_mdio driver relies on this variable to change the io access to little-endian. Default is big-endian. Please see: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55 Thanks Calvin
On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > Hi Grant, > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > > +DSDT entry for MDIO node > > > +------------------------ > > > +a) Silicon Component > > > +-------------------- > > > + Scope(_SB) > > > + { > > > + Device(MDI0) { > > > + Name(_HID, "NXP0006") > > > + Name(_CCA, 1) > > > + Name(_UID, 0) > > > + Name(_CRS, ResourceTemplate() { > > > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > > > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > > + { > > > + MDI0_IT > > > + } > > > + }) // end of _CRS for MDI0 > > > + Name (_DSD, Package () { > > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > + Package () { > > > + Package () {"little-endian", 1}, > > > + } > > > > Adopting the 'little-endian' property here makes little sense. This looks > > like legacy from old PowerPC DT platforms that doesn't belong here. I would > > drop this bit. > > I'm unable to drop this as the xgmac_mdio driver relies on this variable to > change the io access to little-endian. Default is big-endian. > Please see: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55 Hi Calvin Are we talking about the bus controller endiannes, or the CPU endianness? If we are talking about the CPU endiannes, are you plan on supporting any big endian platforms using ACPI? If not, just hard code it. Newbie ACPI question: Does ACPI even support big endian CPUs, given its x86 origins? If this is the bus controller endianness, are all the SoCs you plan to support via ACPI the same endianness? If they are all the same, you can hard code it. To some extent, this should be a moot point, assuming sane hardware. Generally, the bus endian is fixed. It is either native, or like PCI, little endian. The CPU endian is what can change. But in general, once you figure out what you have, there is an IO macro which will do the right thing without any configuration. Andrew
On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: ... > Newbie ACPI question: Does ACPI even support big endian CPUs, given > its x86 origins? I understand the newbie part, but can you elaborate what did you mean under 'support'? To me it sounds like 'network stack was developed for BE CPUs, does it support LE ones?'
On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote: > On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > ... > > > Newbie ACPI question: Does ACPI even support big endian CPUs, given > > its x86 origins? > > I understand the newbie part, but can you elaborate what did you mean > under 'support'? > To me it sounds like 'network stack was developed for BE CPUs, does it > support LE ones?' Does ACPI define the endianness of its tables? Is it written in the standard that they should be little endian? Does Tianocore, or any other implementations, have the needed le32_to_cpu() calls so that they can boot on a big endian CPU? Does it have a standardized way of saying a device is big endian, swap words around if appropriate when doing IO? Is it feasible to boot an ARM system big endian? Can i boot the same system little endian? The CPU should be able to do it, but are the needed things in the ACPI specification and implementation to allow it? Andrew
On Tue, Sep 29, 2020 at 3:44 PM Andrew Lunn <andrew@lunn.ch> wrote: > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > > > +DSDT entry for MDIO node > > > > +------------------------ > > > > +a) Silicon Component > > > > +-------------------- > > > > + Scope(_SB) > > > > + { > > > > + Device(MDI0) { > > > > + Name(_HID, "NXP0006") > > > > + Name(_CCA, 1) > > > > + Name(_UID, 0) > > > > + Name(_CRS, ResourceTemplate() { > > > > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > > > > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > > > + { > > > > + MDI0_IT > > > > + } > > > > + }) // end of _CRS for MDI0 > > > > + Name (_DSD, Package () { > > > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > + Package () { > > > > + Package () {"little-endian", 1}, > > > > + } > > > > > > Adopting the 'little-endian' property here makes little sense. This looks > > > like legacy from old PowerPC DT platforms that doesn't belong here. I would > > > drop this bit. > > > > I'm unable to drop this as the xgmac_mdio driver relies on this variable to > > change the io access to little-endian. Default is big-endian. > > Please see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55 > > Hi Calvin > > Are we talking about the bus controller endiannes, or the CPU > endianness? > > If we are talking about the CPU endiannes, are you plan on supporting > any big endian platforms using ACPI? If not, just hard code it. > Newbie ACPI question: Does ACPI even support big endian CPUs, given > its x86 origins? IIRC both UEFI and ACPI define only little-endian data structures. The code does not attempt to convert these into CPU endianness at the moment. In theory it could be changed to support either, but this seems non-practical for the UEFI runtime services that require calling into firmware code in little-endian mode. > If this is the bus controller endianness, are all the SoCs you plan to > support via ACPI the same endianness? If they are all the same, you > can hard code it. NXP has a bunch of SoCs that reuse the same on-chip devices but change the endianness between them based on what the chip designers guessed the OS would want, which is why the drivers usually support both register layouts and switch at runtime. Worse, depending on which SoC was the first to get a DT binding for a particular NXP on-chip device, the default endianness is different, and there is either a "big-endian" or "little-endian" override in the binding. I would guess that for modern NXP chips that you might boot with ACPI the endianness is always wired the same way, but I understand the caution when they have been burned by this problem before. Arnd
On Tue, Sep 29, 2020 at 5:32 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote: > > On Tue, Sep 29, 2020 at 4:43 PM Andrew Lunn <andrew@lunn.ch> wrote: > > > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > > > ... > > > > > Newbie ACPI question: Does ACPI even support big endian CPUs, given > > > its x86 origins? > > > > I understand the newbie part, but can you elaborate what did you mean > > under 'support'? > > To me it sounds like 'network stack was developed for BE CPUs, does it > > support LE ones?' > > Does ACPI define the endianness of its tables? Is it written in the > standard that they should be little endian? 5.2: "All numeric values in ACPI-defined tables, blocks, and structures are always encoded in little endian format. Signature values are stored as fixed-length strings." > Does Tianocore, or any > other implementations, have the needed le32_to_cpu() calls so that > they can boot on a big endian CPU? Not of my knowledge. > Does it have a standardized way of > saying a device is big endian, swap words around if appropriate when > doing IO? I guess this is not applicable to ACPI. Does Linux have a standardized way? So, what did you mean under doing I/O? I mean in which context? > Is it feasible to boot an ARM system big endian? Not an ARM guy. > Can i boot the same > system little endian? The CPU should be able to do it, but are the > needed things in the ACPI specification and implementation to allow > it?
> IIRC both UEFI and ACPI define only little-endian data structures. > The code does not attempt to convert these into CPU endianness > at the moment. In theory it could be changed to support either, but > this seems non-practical for the UEFI runtime services that require > calling into firmware code in little-endian mode. Hi Arnd Thanks for the info. So we can assume the CPU is little endian. That helps narrow down the problem. > > If this is the bus controller endianness, are all the SoCs you plan to > > support via ACPI the same endianness? If they are all the same, you > > can hard code it. > > NXP has a bunch of SoCs that reuse the same on-chip devices but > change the endianness between them based on what the chip > designers guessed the OS would want, which is why the drivers > usually support both register layouts and switch at runtime. > Worse, depending on which SoC was the first to get a DT binding > for a particular NXP on-chip device, the default endianness is > different, and there is either a "big-endian" or "little-endian" > override in the binding. > > I would guess that for modern NXP chips that you might boot with > ACPI the endianness is always wired the same way, but I > understand the caution when they have been burned by this > problem before. So it might depend on if NXP is worried it might flip the endianness of the synthesis of the MDIO controller at some point for devices it wants to support using ACPI? Does ACPI have a standard way of declaring the endianness of a device? We don't really want to put the DT parameter in ACPI, we want to use the ACPI way of doing it. Thanks Andrew
> > Does it have a standardized way of > > saying a device is big endian, swap words around if appropriate when > > doing IO? > > I guess this is not applicable to ACPI. Does Linux have a standardized way? DT does. You add the property 'little-endian' to indicate you should do IO to the device using little endian. > So, what did you mean under doing I/O? I mean in which context? NXP can synthesise the MDIO controller either big endian, or little endian. So on some SoCs we need to tell the driver to talk to the hardware using big endian accesses. On other SoCs we need to tell the driver to talk to the hardware using little endian. Is there a standard way in ACPI to describe this? Andrew
On Tue, Sep 29, 2020 at 4:49 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Tue, Sep 29, 2020 at 5:32 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Tue, Sep 29, 2020 at 04:55:40PM +0300, Andy Shevchenko wrote: > > Does Tianocore, or any > > other implementations, have the needed le32_to_cpu() calls so that > > they can boot on a big endian CPU? > > Not of my knowledge. > > Is it feasible to boot an ARM system big endian? > > Not an ARM guy. Most CPUs these days support both big-endian and little-endian, and either allow code to switch between the two modes at runtime or are stateless in the way that you have two sets of load/store instructions, making endianness purely a compiler construct (see also: Intel's icc compiler has a big-endian mode using the MOVBE instruction). For Arm kernels, we assume that the firmware is little-endian, but you can build a big-endian kernel that switches into big-endian mode before doing anything else. As I said, I don't think that will ever be used with UEFI (and ACPI, by extension), since it would be a ton of work and few users care about big-endian kernels. Arnd
On 29/09/2020 14:43, Andrew Lunn wrote: > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: >> Hi Grant, >> >> On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: >>>> +DSDT entry for MDIO node >>>> +------------------------ >>>> +a) Silicon Component >>>> +-------------------- >>>> + Scope(_SB) >>>> + { >>>> + Device(MDI0) { >>>> + Name(_HID, "NXP0006") >>>> + Name(_CCA, 1) >>>> + Name(_UID, 0) >>>> + Name(_CRS, ResourceTemplate() { >>>> + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) >>>> + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) >>>> + { >>>> + MDI0_IT >>>> + } >>>> + }) // end of _CRS for MDI0 >>>> + Name (_DSD, Package () { >>>> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >>>> + Package () { >>>> + Package () {"little-endian", 1}, >>>> + } >>> >>> Adopting the 'little-endian' property here makes little sense. This looks >>> like legacy from old PowerPC DT platforms that doesn't belong here. I would >>> drop this bit. >> >> I'm unable to drop this as the xgmac_mdio driver relies on this variable to >> change the io access to little-endian. Default is big-endian. >> Please see: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55 > > Hi Calvin > > Are we talking about the bus controller endiannes, or the CPU > endianness? This is orthogonal to the MDIO bus issue. This is a legacy of the xgmac IP block originating in PowerPC platforms with a big-endian bus wiring. The flag here tells the driver to use little endian when accessing MMIO registers. > If we are talking about the CPU endiannes, are you plan on supporting > any big endian platforms using ACPI? If not, just hard code it. > Newbie ACPI question: Does ACPI even support big endian CPUs, given > its x86 origins? > > If this is the bus controller endianness, are all the SoCs you plan to > support via ACPI the same endianness? If they are all the same, you > can hard code it. I would agree. The ACPI and DT probe paths are different. It would be easy to automatically set the little-endian flag by default when xgmac is described via ACPI. g.
On 29/09/2020 15:59, Andrew Lunn wrote: >> IIRC both UEFI and ACPI define only little-endian data structures. >> The code does not attempt to convert these into CPU endianness >> at the moment. In theory it could be changed to support either, but >> this seems non-practical for the UEFI runtime services that require >> calling into firmware code in little-endian mode. > > Hi Arnd > > Thanks for the info. So we can assume the CPU is little endian. That > helps narrow down the problem. > >>> If this is the bus controller endianness, are all the SoCs you plan to >>> support via ACPI the same endianness? If they are all the same, you >>> can hard code it. >> >> NXP has a bunch of SoCs that reuse the same on-chip devices but >> change the endianness between them based on what the chip >> designers guessed the OS would want, which is why the drivers >> usually support both register layouts and switch at runtime. >> Worse, depending on which SoC was the first to get a DT binding >> for a particular NXP on-chip device, the default endianness is >> different, and there is either a "big-endian" or "little-endian" >> override in the binding. >> >> I would guess that for modern NXP chips that you might boot with >> ACPI the endianness is always wired the same way, but I >> understand the caution when they have been burned by this >> problem before. > > So it might depend on if NXP is worried it might flip the endianness > of the synthesis of the MDIO controller at some point for devices it > wants to support using ACPI? > > Does ACPI have a standard way of declaring the endianness of a device? > We don't really want to put the DT parameter in ACPI, we want to use > the ACPI way of doing it. No, and it doesn't need one. If a device is wired up big-endian, then it is between the device driver and the device. The OS, and the ACPI framework doesn't come into play other than providing a generic way of encoding data useful to the device driver. Encoding endian hasn't been a common problem, and the tools are already there to deal with it when it is. g.
On Tue, Sep 29, 2020 at 04:53:47PM +0100, Grant Likely wrote: > > > On 29/09/2020 14:43, Andrew Lunn wrote: > > On Tue, Sep 29, 2020 at 10:47:03AM +0530, Calvin Johnson wrote: > > > Hi Grant, > > > > > > On Fri, Sep 25, 2020 at 02:34:21PM +0100, Grant Likely wrote: > > > > > +DSDT entry for MDIO node > > > > > +------------------------ > > > > > +a) Silicon Component > > > > > +-------------------- > > > > > + Scope(_SB) > > > > > + { > > > > > + Device(MDI0) { > > > > > + Name(_HID, "NXP0006") > > > > > + Name(_CCA, 1) > > > > > + Name(_UID, 0) > > > > > + Name(_CRS, ResourceTemplate() { > > > > > + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) > > > > > + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > > > > + { > > > > > + MDI0_IT > > > > > + } > > > > > + }) // end of _CRS for MDI0 > > > > > + Name (_DSD, Package () { > > > > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > > + Package () { > > > > > + Package () {"little-endian", 1}, > > > > > + } > > > > > > > > Adopting the 'little-endian' property here makes little sense. This looks > > > > like legacy from old PowerPC DT platforms that doesn't belong here. I would > > > > drop this bit. > > > > > > I'm unable to drop this as the xgmac_mdio driver relies on this variable to > > > change the io access to little-endian. Default is big-endian. > > > Please see: > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/ethernet/freescale/xgmac_mdio.c?h=v5.9-rc7#n55 > > > > Hi Calvin > > > > Are we talking about the bus controller endiannes, or the CPU > > endianness? > > This is orthogonal to the MDIO bus issue. This is a legacy of the xgmac IP > block originating in PowerPC platforms with a big-endian bus wiring. The > flag here tells the driver to use little endian when accessing MMIO > registers. > > > If we are talking about the CPU endiannes, are you plan on supporting > > any big endian platforms using ACPI? If not, just hard code it. > > Newbie ACPI question: Does ACPI even support big endian CPUs, given > > its x86 origins? > > > If this is the bus controller endianness, are all the SoCs you plan to > > support via ACPI the same endianness? If they are all the same, you > > can hard code it. > > I would agree. The ACPI and DT probe paths are different. It would be easy > to automatically set the little-endian flag by default when xgmac is > described via ACPI. Thanks Andrew and Grant for this suggestion. Yes, this is an easy way to solve this problem. Will do that. Regards Calvin
diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst new file mode 100644 index 000000000000..0132fee10b45 --- /dev/null +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst @@ -0,0 +1,90 @@ +.. SPDX-License-Identifier: GPL-2.0 + +========================= +MDIO bus and PHYs in ACPI +========================= + +The PHYs on an mdiobus are probed and registered using mdiobus_register(). +Later, for connecting these PHYs to MAC, the PHYs registered on the +mdiobus have to be referenced. + +mdio-handle +----------- +For each MAC node, a property "mdio-handle" is used to reference the +MDIO bus on which the PHYs are registered. On getting hold of the MDIO +bus, use find_phy_device() to get the PHY connected to the MAC. + +phy-channel +----------- +Property "phy-channel" defines the address of the PHY on the mdiobus. + +phy-mode +-------- +Property "phy-mode" defines the type of PHY interface. + +An example of this is shown below:: + +DSDT entry for MAC where MDIO node is referenced +------------------------------------------------ + Scope(\_SB.MCE0.PR17) // 1G + { + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () {"phy-channel", 1}, + Package () {"phy-mode", "rgmii-id"}, + Package () {"mdio-handle", Package (){\_SB.MDI0}} + } + }) + } + + Scope(\_SB.MCE0.PR18) // 1G + { + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () {"phy-channel", 2}, + Package () {"phy-mode", "rgmii-id"}, + Package () {"mdio-handle", Package (){\_SB.MDI0}} + } + }) + } + +DSDT entry for MDIO node +------------------------ +a) Silicon Component +-------------------- + Scope(_SB) + { + Device(MDI0) { + Name(_HID, "NXP0006") + Name(_CCA, 1) + Name(_UID, 0) + Name(_CRS, ResourceTemplate() { + Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN) + Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) + { + MDI0_IT + } + }) // end of _CRS for MDI0 + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package () {"little-endian", 1}, + } + }) + } // end of MDI0 + } + +b) Platform Component +--------------------- + Scope(\_SB.MDI0) + { + Device(PHY1) { + Name (_ADR, 0x1) + } // end of PHY1 + + Device(PHY2) { + Name (_ADR, 0x2) + } // end of PHY2 + }
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and provide them to be connected to MAC. An ACPI node property "mdio-handle" is introduced to reference the MDIO bus on which PHYs are registered with autoprobing method used by mdiobus_register(). Describe properties "phy-channel" and "phy-mode" Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v7: None Changes in v6: None Changes in v5: None Changes in v4: None Changes in v3: - cleanup based on v2 comments - Added description for more properties - Added MDIO node DSDT entry Changes in v2: None Documentation/firmware-guide/acpi/dsd/phy.rst | 90 +++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst