| Message ID | 20210122154300.7628-2-calvin.johnson@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > provide them to be connected to MAC. > > Describe properties "phy-handle" and "phy-mode". > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > Changes in v4: > - More cleanup This looks much better that the previous versions IMV, some nits below. > Changes in v3: None > Changes in v2: > - Updated with more description in document > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > 1 file changed, 129 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..76fca994bc99 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,129 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an MDIO bus [1] are probed and registered using > +fwnode_mdiobus_register_phy(). Empty line here, please. > +Later, for connecting these PHYs to MAC, the PHYs registered on the > +MDIO bus have to be referenced. > + > +The UUID given below should be used as mentioned in the "Device Properties > +UUID For _DSD" [2] document. > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 I would drop the above paragraph. > + > +This document introduces two _DSD properties that are to be used > +for PHYs on the MDIO bus.[3] I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." above and add the following here: "These properties are defined in accordance with the "Device Properties UUID For _DSD" [2] document and the daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device Data Descriptors containing them." > + > +phy-handle > +---------- > +For each MAC node, a device property "phy-handle" is used to reference > +the PHY that is registered on an MDIO bus. This is mandatory for > +network interfaces that have PHYs connected to MAC via MDIO bus. > + > +During the MDIO bus driver initialization, PHYs on this bus are probed > +using the _ADR object as shown below and are registered on the MDIO bus. Do you want to mention the "reg" property here? I think it would be useful to do that. > + > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > +Later, during the MAC driver initialization, the registered PHY devices > +have to be retrieved from the MDIO bus. For this, MAC driver needs "the MAC driver" I suppose? > +reference to the previously registered PHYs which are provided s/reference/references/ (plural) > +using reference to the device as {\_SB.MDI0.PHY1}. "as device object references (e.g. \_SB.MDI0.PHY1}." > + > +phy-mode > +-------- > +The "phy-mode" _DSD property is used to describe the connection to > +the PHY. The valid values for "phy-mode" are defined in [4]. > + One empty line should be sufficient. > + > +An ASL example of this is shown below. "The following ASL example illustrates the usage of these properties." > + > +DSDT entry for MDIO node > +------------------------ Empty line here, please. > +The MDIO bus has an SoC component(MDIO controller) and a platform Missing space after "component". > +component (PHYs on the MDIO bus). > + > +a) Silicon Component > +This node describes the MDIO controller, MDI0 > +--------------------------------------------- > +:: > + 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 > + } // end of MDI0 > + } > + > +b) Platform Component > +This node defines the PHYs that are connected to the MDIO bus, MDI0 "The PHY1 and PHY2 nodes represent the PHYs connected to MDIO bus MDI0." > +------------------------------------------------------------------- > +:: > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > + > + "DSDT entries representing MAC nodes -----------------------------------" Plus an empty line. > +Below are the MAC nodes where PHY nodes are referenced. > +phy-mode and phy-handle are used as explained earlier. > +------------------------------------------------------ > +:: > + Scope(\_SB.MCE0.PR17) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY1} > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", \_SB.MDI0.PHY2}} > + } > + }) > + } > + > +References > +========== > + > +[1] Documentation/networking/phy.rst > + > +[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf > + > +[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst > + > +[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml > -- > 2.17.1 >
Hi Rafael, Thanks for the review. I'll work on all the comments. On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > provide them to be connected to MAC. > > > > Describe properties "phy-handle" and "phy-mode". > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > --- > > > > Changes in v4: > > - More cleanup > > This looks much better that the previous versions IMV, some nits below. > > > Changes in v3: None > > Changes in v2: > > - Updated with more description in document > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > 1 file changed, 129 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..76fca994bc99 > > --- /dev/null > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > @@ -0,0 +1,129 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +========================= > > +MDIO bus and PHYs in ACPI > > +========================= > > + > > +The PHYs on an MDIO bus [1] are probed and registered using > > +fwnode_mdiobus_register_phy(). > > Empty line here, please. > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > +MDIO bus have to be referenced. > > + > > +The UUID given below should be used as mentioned in the "Device Properties > > +UUID For _DSD" [2] document. > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > I would drop the above paragraph. > > > + > > +This document introduces two _DSD properties that are to be used > > +for PHYs on the MDIO bus.[3] > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > above and add the following here: > > "These properties are defined in accordance with the "Device > Properties UUID For _DSD" [2] document and the > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > Data Descriptors containing them." > > > + > > +phy-handle > > +---------- > > +For each MAC node, a device property "phy-handle" is used to reference > > +the PHY that is registered on an MDIO bus. This is mandatory for > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > + > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > +using the _ADR object as shown below and are registered on the MDIO bus. > > Do you want to mention the "reg" property here? I think it would be > useful to do that. No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI may be useful for other use cases that Andy is aware of. Regards Calvin
On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Hi Rafael, > > Thanks for the review. I'll work on all the comments. > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > provide them to be connected to MAC. > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > --- > > > > > > Changes in v4: > > > - More cleanup > > > > This looks much better that the previous versions IMV, some nits below. > > > > > Changes in v3: None > > > Changes in v2: > > > - Updated with more description in document > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > 1 file changed, 129 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..76fca994bc99 > > > --- /dev/null > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > @@ -0,0 +1,129 @@ > > > +.. SPDX-License-Identifier: GPL-2.0 > > > + > > > +========================= > > > +MDIO bus and PHYs in ACPI > > > +========================= > > > + > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > +fwnode_mdiobus_register_phy(). > > > > Empty line here, please. > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > +MDIO bus have to be referenced. > > > + > > > +The UUID given below should be used as mentioned in the "Device Properties > > > +UUID For _DSD" [2] document. > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > I would drop the above paragraph. > > > > > + > > > +This document introduces two _DSD properties that are to be used > > > +for PHYs on the MDIO bus.[3] > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > above and add the following here: > > > > "These properties are defined in accordance with the "Device > > Properties UUID For _DSD" [2] document and the > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > Data Descriptors containing them." > > > > > + > > > +phy-handle > > > +---------- > > > +For each MAC node, a device property "phy-handle" is used to reference > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > + > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > Do you want to mention the "reg" property here? I think it would be > > useful to do that. > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > may be useful for other use cases that Andy is aware of. The code should reflect this, then. I mean it sounds like you want to check the "reg" property only if this is a non-ACPI node.
On Thu, Jan 28, 2021 at 01:00:40PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > Hi Rafael, > > > > Thanks for the review. I'll work on all the comments. > > > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > > provide them to be connected to MAC. > > > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > --- > > > > > > > > Changes in v4: > > > > - More cleanup > > > > > > This looks much better that the previous versions IMV, some nits below. > > > > > > > Changes in v3: None > > > > Changes in v2: > > > > - Updated with more description in document > > > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > > 1 file changed, 129 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..76fca994bc99 > > > > --- /dev/null > > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > > @@ -0,0 +1,129 @@ > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +========================= > > > > +MDIO bus and PHYs in ACPI > > > > +========================= > > > > + > > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > > +fwnode_mdiobus_register_phy(). > > > > > > Empty line here, please. > > > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > > +MDIO bus have to be referenced. > > > > + > > > > +The UUID given below should be used as mentioned in the "Device Properties > > > > +UUID For _DSD" [2] document. > > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > > > I would drop the above paragraph. > > > > > > > + > > > > +This document introduces two _DSD properties that are to be used > > > > +for PHYs on the MDIO bus.[3] > > > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > > above and add the following here: > > > > > > "These properties are defined in accordance with the "Device > > > Properties UUID For _DSD" [2] document and the > > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > > Data Descriptors containing them." > > > > > > > + > > > > +phy-handle > > > > +---------- > > > > +For each MAC node, a device property "phy-handle" is used to reference > > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > > + > > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > > > Do you want to mention the "reg" property here? I think it would be > > > useful to do that. > > > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > > may be useful for other use cases that Andy is aware of. > > The code should reflect this, then. I mean it sounds like you want to > check the "reg" property only if this is a non-ACPI node. Right. For MDIO case, that is what is required. "reg" for DT and "_ADR" for ACPI. However, Andy pointed out [1] that ACPI nodes can also hold reg property and therefore, fwnode_get_id() need to be capable to handling that situation as well. Andy, any suggestion? [1] https://lore.kernel.org/netdev/CAHp75Vef7Ln2hwx8BYao3SFxB8U2QTsfxPpxA_jxmujAMFpboA@mail.gmail.com/ Thanks Calvin
On Thu, Jan 28, 2021 at 2:12 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > On Thu, Jan 28, 2021 at 01:00:40PM +0100, Rafael J. Wysocki wrote: > > On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > Hi Rafael, > > > > > > Thanks for the review. I'll work on all the comments. > > > > > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > > > provide them to be connected to MAC. > > > > > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > > --- > > > > > > > > > > Changes in v4: > > > > > - More cleanup > > > > > > > > This looks much better that the previous versions IMV, some nits below. > > > > > > > > > Changes in v3: None > > > > > Changes in v2: > > > > > - Updated with more description in document > > > > > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > > > 1 file changed, 129 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..76fca994bc99 > > > > > --- /dev/null > > > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > > > @@ -0,0 +1,129 @@ > > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > > + > > > > > +========================= > > > > > +MDIO bus and PHYs in ACPI > > > > > +========================= > > > > > + > > > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > > > +fwnode_mdiobus_register_phy(). > > > > > > > > Empty line here, please. > > > > > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > > > +MDIO bus have to be referenced. > > > > > + > > > > > +The UUID given below should be used as mentioned in the "Device Properties > > > > > +UUID For _DSD" [2] document. > > > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > > > > > I would drop the above paragraph. > > > > > > > > > + > > > > > +This document introduces two _DSD properties that are to be used > > > > > +for PHYs on the MDIO bus.[3] > > > > > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > > > above and add the following here: > > > > > > > > "These properties are defined in accordance with the "Device > > > > Properties UUID For _DSD" [2] document and the > > > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > > > Data Descriptors containing them." > > > > > > > > > + > > > > > +phy-handle > > > > > +---------- > > > > > +For each MAC node, a device property "phy-handle" is used to reference > > > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > > > + > > > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > > > > > Do you want to mention the "reg" property here? I think it would be > > > > useful to do that. > > > > > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > > > may be useful for other use cases that Andy is aware of. > > > > The code should reflect this, then. I mean it sounds like you want to > > check the "reg" property only if this is a non-ACPI node. > > Right. For MDIO case, that is what is required. > "reg" for DT and "_ADR" for ACPI. > > However, Andy pointed out [1] that ACPI nodes can also hold reg property and > therefore, fwnode_get_id() need to be capable to handling that situation as > well. No, please don't confuse those two things. Yes, ACPI nodes can also hold a "reg" property, but the meaning of it depends on the binding which is exactly my point: _ADR is not a fallback replacement for "reg" in general and it is not so for MDIO too. The new function as proposed doesn't match the MDIO requirements and so it should not be used for MDIO. For MDIO, the exact flow mentioned above needs to be implemented (and if someone wants to use it for their use case too, fine). Otherwise the code wouldn't match the documentation.
On Thu, Jan 28, 2021 at 02:27:00PM +0100, Rafael J. Wysocki wrote: > On Thu, Jan 28, 2021 at 2:12 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > On Thu, Jan 28, 2021 at 01:00:40PM +0100, Rafael J. Wysocki wrote: > > > On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > Hi Rafael, > > > > > > > > Thanks for the review. I'll work on all the comments. > > > > > > > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > > > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > > > > provide them to be connected to MAC. > > > > > > > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > > > --- > > > > > > > > > > > > Changes in v4: > > > > > > - More cleanup > > > > > > > > > > This looks much better that the previous versions IMV, some nits below. > > > > > > > > > > > Changes in v3: None > > > > > > Changes in v2: > > > > > > - Updated with more description in document > > > > > > > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > > > > 1 file changed, 129 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..76fca994bc99 > > > > > > --- /dev/null > > > > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > > > > @@ -0,0 +1,129 @@ > > > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > > > + > > > > > > +========================= > > > > > > +MDIO bus and PHYs in ACPI > > > > > > +========================= > > > > > > + > > > > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > > > > +fwnode_mdiobus_register_phy(). > > > > > > > > > > Empty line here, please. > > > > > > > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > > > > +MDIO bus have to be referenced. > > > > > > + > > > > > > +The UUID given below should be used as mentioned in the "Device Properties > > > > > > +UUID For _DSD" [2] document. > > > > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > > > > > > > I would drop the above paragraph. > > > > > > > > > > > + > > > > > > +This document introduces two _DSD properties that are to be used > > > > > > +for PHYs on the MDIO bus.[3] > > > > > > > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > > > > above and add the following here: > > > > > > > > > > "These properties are defined in accordance with the "Device > > > > > Properties UUID For _DSD" [2] document and the > > > > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > > > > Data Descriptors containing them." > > > > > > > > > > > + > > > > > > +phy-handle > > > > > > +---------- > > > > > > +For each MAC node, a device property "phy-handle" is used to reference > > > > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > > > > + > > > > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > > > > > > > Do you want to mention the "reg" property here? I think it would be > > > > > useful to do that. > > > > > > > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > > > > may be useful for other use cases that Andy is aware of. > > > > > > The code should reflect this, then. I mean it sounds like you want to > > > check the "reg" property only if this is a non-ACPI node. > > > > Right. For MDIO case, that is what is required. > > "reg" for DT and "_ADR" for ACPI. > > > > However, Andy pointed out [1] that ACPI nodes can also hold reg property and > > therefore, fwnode_get_id() need to be capable to handling that situation as > > well. > > No, please don't confuse those two things. > > Yes, ACPI nodes can also hold a "reg" property, but the meaning of it > depends on the binding which is exactly my point: _ADR is not a > fallback replacement for "reg" in general and it is not so for MDIO > too. The new function as proposed doesn't match the MDIO requirements > and so it should not be used for MDIO. > > For MDIO, the exact flow mentioned above needs to be implemented (and > if someone wants to use it for their use case too, fine). > > Otherwise the code wouldn't match the documentation. In that case, is this good? /** * fwnode_get_local_addr - Get the local address of fwnode. * @fwnode: firmware node * @addr: addr value contained in the fwnode * * For DT, retrieve the value of the "reg" property for @fwnode. * * In the ACPI case, evaluate the _ADR object located under the * given node, if present, and provide its return value to the * caller. * * Return 0 on success or a negative error code. */ int fwnode_get_local_addr(struct fwnode_handle *fwnode, u32 *addr) { int ret; if (is_of_node(fwnode)) return of_property_read_u32(to_of_node(fwnode), "reg", addr); #ifdef CONFIG_ACPI if (is_acpi_node(fwnode)) { unsigned long long adr; acpi_status status; status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), METHOD_NAME__ADR, NULL, &adr); if (ACPI_FAILURE(status)) return -ENODATA; *addr = (u32)adr; return 0; } #endif return -EINVAL; }
On Fri, Jan 29, 2021 at 7:48 AM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > On Thu, Jan 28, 2021 at 02:27:00PM +0100, Rafael J. Wysocki wrote: > > On Thu, Jan 28, 2021 at 2:12 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > On Thu, Jan 28, 2021 at 01:00:40PM +0100, Rafael J. Wysocki wrote: > > > > On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > Hi Rafael, > > > > > > > > > > Thanks for the review. I'll work on all the comments. > > > > > > > > > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > > > > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > > > > > provide them to be connected to MAC. > > > > > > > > > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > > > > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > > > > --- > > > > > > > > > > > > > > Changes in v4: > > > > > > > - More cleanup > > > > > > > > > > > > This looks much better that the previous versions IMV, some nits below. > > > > > > > > > > > > > Changes in v3: None > > > > > > > Changes in v2: > > > > > > > - Updated with more description in document > > > > > > > > > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > > > > > 1 file changed, 129 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..76fca994bc99 > > > > > > > --- /dev/null > > > > > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > > > > > @@ -0,0 +1,129 @@ > > > > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > > > > + > > > > > > > +========================= > > > > > > > +MDIO bus and PHYs in ACPI > > > > > > > +========================= > > > > > > > + > > > > > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > > > > > +fwnode_mdiobus_register_phy(). > > > > > > > > > > > > Empty line here, please. > > > > > > > > > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > > > > > +MDIO bus have to be referenced. > > > > > > > + > > > > > > > +The UUID given below should be used as mentioned in the "Device Properties > > > > > > > +UUID For _DSD" [2] document. > > > > > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > > > > > > > > > I would drop the above paragraph. > > > > > > > > > > > > > + > > > > > > > +This document introduces two _DSD properties that are to be used > > > > > > > +for PHYs on the MDIO bus.[3] > > > > > > > > > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > > > > > above and add the following here: > > > > > > > > > > > > "These properties are defined in accordance with the "Device > > > > > > Properties UUID For _DSD" [2] document and the > > > > > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > > > > > Data Descriptors containing them." > > > > > > > > > > > > > + > > > > > > > +phy-handle > > > > > > > +---------- > > > > > > > +For each MAC node, a device property "phy-handle" is used to reference > > > > > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > > > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > > > > > + > > > > > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > > > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > > > > > > > > > Do you want to mention the "reg" property here? I think it would be > > > > > > useful to do that. > > > > > > > > > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > > > > > may be useful for other use cases that Andy is aware of. > > > > > > > > The code should reflect this, then. I mean it sounds like you want to > > > > check the "reg" property only if this is a non-ACPI node. > > > > > > Right. For MDIO case, that is what is required. > > > "reg" for DT and "_ADR" for ACPI. > > > > > > However, Andy pointed out [1] that ACPI nodes can also hold reg property and > > > therefore, fwnode_get_id() need to be capable to handling that situation as > > > well. > > > > No, please don't confuse those two things. > > > > Yes, ACPI nodes can also hold a "reg" property, but the meaning of it > > depends on the binding which is exactly my point: _ADR is not a > > fallback replacement for "reg" in general and it is not so for MDIO > > too. The new function as proposed doesn't match the MDIO requirements > > and so it should not be used for MDIO. > > > > For MDIO, the exact flow mentioned above needs to be implemented (and > > if someone wants to use it for their use case too, fine). > > > > Otherwise the code wouldn't match the documentation. > > In that case, is this good? It would work, but I would introduce a wrapper around the _ADR evaluation, something like: int acpi_get_local_address(acpi_handle handle, u32 *addr) { unsigned long long adr; acpi_status status; status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); if (ACPI_FAILURE(status)) return -ENODATA; *addr = (u32)adr; return 0; } in drivers/acpi/utils.c and add a static inline stub always returning -ENODEV for it for !CONFIG_ACPI. > /** > * fwnode_get_local_addr - Get the local address of fwnode. > * @fwnode: firmware node > * @addr: addr value contained in the fwnode > * > * For DT, retrieve the value of the "reg" property for @fwnode. > * > * In the ACPI case, evaluate the _ADR object located under the > * given node, if present, and provide its return value to the > * caller. > * > * Return 0 on success or a negative error code. > */ > int fwnode_get_local_addr(struct fwnode_handle *fwnode, u32 *addr) > { > int ret; > > if (is_of_node(fwnode)) > return of_property_read_u32(to_of_node(fwnode), "reg", addr); So you can write the below as if (is_acpi_device_node(fwnode)) return acpi_get_local_address(ACPI_HANDLE_FWNODE(fwnode), addr); return -EINVAL; and this should compile just fine if CONFIG_ACPI is unset, so you can avoid the whole #ifdeffery in this function. > > #ifdef CONFIG_ACPI > if (is_acpi_node(fwnode)) { > unsigned long long adr; > acpi_status status; > > status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > METHOD_NAME__ADR, NULL, &adr); > if (ACPI_FAILURE(status)) > return -ENODATA; > *addr = (u32)adr; > return 0; > } > #endif > return -EINVAL; > }
On Fri, Jan 29, 2021 at 5:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Jan 29, 2021 at 7:48 AM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > On Thu, Jan 28, 2021 at 02:27:00PM +0100, Rafael J. Wysocki wrote: > > > On Thu, Jan 28, 2021 at 2:12 PM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > On Thu, Jan 28, 2021 at 01:00:40PM +0100, Rafael J. Wysocki wrote: > > > > > On Thu, Jan 28, 2021 at 12:27 PM Calvin Johnson > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > > > Hi Rafael, > > > > > > > > > > > > Thanks for the review. I'll work on all the comments. > > > > > > > > > > > > On Fri, Jan 22, 2021 at 08:22:21PM +0100, Rafael J. Wysocki wrote: > > > > > > > On Fri, Jan 22, 2021 at 4:43 PM Calvin Johnson > > > > > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > > > > > > > > > Introduce ACPI mechanism to get PHYs registered on a MDIO bus and > > > > > > > > provide them to be connected to MAC. > > > > > > > > > > > > > > > > Describe properties "phy-handle" and "phy-mode". > > > > > > > > > > > > > > > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > > > > > > > > --- > > > > > > > > > > > > > > > > Changes in v4: > > > > > > > > - More cleanup > > > > > > > > > > > > > > This looks much better that the previous versions IMV, some nits below. > > > > > > > > > > > > > > > Changes in v3: None > > > > > > > > Changes in v2: > > > > > > > > - Updated with more description in document > > > > > > > > > > > > > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ > > > > > > > > 1 file changed, 129 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..76fca994bc99 > > > > > > > > --- /dev/null > > > > > > > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > > > > > > > @@ -0,0 +1,129 @@ > > > > > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > > > > > + > > > > > > > > +========================= > > > > > > > > +MDIO bus and PHYs in ACPI > > > > > > > > +========================= > > > > > > > > + > > > > > > > > +The PHYs on an MDIO bus [1] are probed and registered using > > > > > > > > +fwnode_mdiobus_register_phy(). > > > > > > > > > > > > > > Empty line here, please. > > > > > > > > > > > > > > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > > > > > > > +MDIO bus have to be referenced. > > > > > > > > + > > > > > > > > +The UUID given below should be used as mentioned in the "Device Properties > > > > > > > > +UUID For _DSD" [2] document. > > > > > > > > + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 > > > > > > > > > > > > > > I would drop the above paragraph. > > > > > > > > > > > > > > > + > > > > > > > > +This document introduces two _DSD properties that are to be used > > > > > > > > +for PHYs on the MDIO bus.[3] > > > > > > > > > > > > > > I'd say "for connecting PHYs on the MDIO bus [3] to the MAC layer." > > > > > > > above and add the following here: > > > > > > > > > > > > > > "These properties are defined in accordance with the "Device > > > > > > > Properties UUID For _DSD" [2] document and the > > > > > > > daffd814-6eba-4d8c-8a91-bc9bbf4aa301 UUID must be used in the Device > > > > > > > Data Descriptors containing them." > > > > > > > > > > > > > > > + > > > > > > > > +phy-handle > > > > > > > > +---------- > > > > > > > > +For each MAC node, a device property "phy-handle" is used to reference > > > > > > > > +the PHY that is registered on an MDIO bus. This is mandatory for > > > > > > > > +network interfaces that have PHYs connected to MAC via MDIO bus. > > > > > > > > + > > > > > > > > +During the MDIO bus driver initialization, PHYs on this bus are probed > > > > > > > > +using the _ADR object as shown below and are registered on the MDIO bus. > > > > > > > > > > > > > > Do you want to mention the "reg" property here? I think it would be > > > > > > > useful to do that. > > > > > > > > > > > > No. I think we should adhere to _ADR in MDIO case. The "reg" property for ACPI > > > > > > may be useful for other use cases that Andy is aware of. > > > > > > > > > > The code should reflect this, then. I mean it sounds like you want to > > > > > check the "reg" property only if this is a non-ACPI node. > > > > > > > > Right. For MDIO case, that is what is required. > > > > "reg" for DT and "_ADR" for ACPI. > > > > > > > > However, Andy pointed out [1] that ACPI nodes can also hold reg property and > > > > therefore, fwnode_get_id() need to be capable to handling that situation as > > > > well. > > > > > > No, please don't confuse those two things. > > > > > > Yes, ACPI nodes can also hold a "reg" property, but the meaning of it > > > depends on the binding which is exactly my point: _ADR is not a > > > fallback replacement for "reg" in general and it is not so for MDIO > > > too. The new function as proposed doesn't match the MDIO requirements > > > and so it should not be used for MDIO. > > > > > > For MDIO, the exact flow mentioned above needs to be implemented (and > > > if someone wants to use it for their use case too, fine). > > > > > > Otherwise the code wouldn't match the documentation. > > > > In that case, is this good? > > It would work, but I would introduce a wrapper around the _ADR > evaluation, something like: > > int acpi_get_local_address(acpi_handle handle, u32 *addr) > { > unsigned long long adr; > acpi_status status; > > status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); > if (ACPI_FAILURE(status)) > return -ENODATA; > > *addr = (u32)adr; > return 0; > } > > in drivers/acpi/utils.c and add a static inline stub always returning > -ENODEV for it for !CONFIG_ACPI. > > > /** > > * fwnode_get_local_addr - Get the local address of fwnode. > > * @fwnode: firmware node > > * @addr: addr value contained in the fwnode > > * > > * For DT, retrieve the value of the "reg" property for @fwnode. > > * > > * In the ACPI case, evaluate the _ADR object located under the > > * given node, if present, and provide its return value to the > > * caller. > > * > > * Return 0 on success or a negative error code. > > */ > > int fwnode_get_local_addr(struct fwnode_handle *fwnode, u32 *addr) > > { > > int ret; > > > > if (is_of_node(fwnode)) > > return of_property_read_u32(to_of_node(fwnode), "reg", addr); > > So you can write the below as > > if (is_acpi_device_node(fwnode)) > return acpi_get_local_address(ACPI_HANDLE_FWNODE(fwnode), addr); > > return -EINVAL; > > and this should compile just fine if CONFIG_ACPI is unset, so you can > avoid the whole #ifdeffery in this function. BTW, you may not need the fwnode_get_local_addr() at all then, just evaluate either the "reg" property for OF or acpi_get_local_address() for ACPI in the "caller" code directly. A common helper doing this can be added later. > > > > #ifdef CONFIG_ACPI > > if (is_acpi_node(fwnode)) { > > unsigned long long adr; > > acpi_status status; > > > > status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > > METHOD_NAME__ADR, NULL, &adr); > > if (ACPI_FAILURE(status)) > > return -ENODATA; > > *addr = (u32)adr; > > return 0; > > } > > #endif > > return -EINVAL; > > }
On Fri, Jan 29, 2021 at 6:44 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > On Fri, Jan 29, 2021 at 5:37 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Jan 29, 2021 at 7:48 AM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: ... > > It would work, but I would introduce a wrapper around the _ADR > > evaluation, something like: > > > > int acpi_get_local_address(acpi_handle handle, u32 *addr) > > { > > unsigned long long adr; > > acpi_status status; > > > > status = acpi_evaluate_integer(handle, METHOD_NAME__ADR, NULL, &adr); > > if (ACPI_FAILURE(status)) > > return -ENODATA; > > > > *addr = (u32)adr; > > return 0; > > } > > > > in drivers/acpi/utils.c and add a static inline stub always returning > > -ENODEV for it for !CONFIG_ACPI. ... > BTW, you may not need the fwnode_get_local_addr() at all then, just > evaluate either the "reg" property for OF or acpi_get_local_address() > for ACPI in the "caller" code directly. A common helper doing this can > be added later. Sounds good to me and it will address your concern about different semantics of reg/_ADR on per driver/subsystem basis.
diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst b/Documentation/firmware-guide/acpi/dsd/phy.rst new file mode 100644 index 000000000000..76fca994bc99 --- /dev/null +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst @@ -0,0 +1,129 @@ +.. SPDX-License-Identifier: GPL-2.0 + +========================= +MDIO bus and PHYs in ACPI +========================= + +The PHYs on an MDIO bus [1] are probed and registered using +fwnode_mdiobus_register_phy(). +Later, for connecting these PHYs to MAC, the PHYs registered on the +MDIO bus have to be referenced. + +The UUID given below should be used as mentioned in the "Device Properties +UUID For _DSD" [2] document. + - UUID: daffd814-6eba-4d8c-8a91-bc9bbf4aa301 + +This document introduces two _DSD properties that are to be used +for PHYs on the MDIO bus.[3] + +phy-handle +---------- +For each MAC node, a device property "phy-handle" is used to reference +the PHY that is registered on an MDIO bus. This is mandatory for +network interfaces that have PHYs connected to MAC via MDIO bus. + +During the MDIO bus driver initialization, PHYs on this bus are probed +using the _ADR object as shown below and are registered on the MDIO bus. + +:: + Scope(\_SB.MDI0) + { + Device(PHY1) { + Name (_ADR, 0x1) + } // end of PHY1 + + Device(PHY2) { + Name (_ADR, 0x2) + } // end of PHY2 + } + +Later, during the MAC driver initialization, the registered PHY devices +have to be retrieved from the MDIO bus. For this, MAC driver needs +reference to the previously registered PHYs which are provided +using reference to the device as {\_SB.MDI0.PHY1}. + +phy-mode +-------- +The "phy-mode" _DSD property is used to describe the connection to +the PHY. The valid values for "phy-mode" are defined in [4]. + + +An ASL example of this is shown below. + +DSDT entry for MDIO node +------------------------ +The MDIO bus has an SoC component(MDIO controller) and a platform +component (PHYs on the MDIO bus). + +a) Silicon Component +This node describes the MDIO controller, MDI0 +--------------------------------------------- +:: + 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 + } // end of MDI0 + } + +b) Platform Component +This node defines the PHYs that are connected to the MDIO bus, MDI0 +------------------------------------------------------------------- +:: + Scope(\_SB.MDI0) + { + Device(PHY1) { + Name (_ADR, 0x1) + } // end of PHY1 + + Device(PHY2) { + Name (_ADR, 0x2) + } // end of PHY2 + } + + +Below are the MAC nodes where PHY nodes are referenced. +phy-mode and phy-handle are used as explained earlier. +------------------------------------------------------ +:: + Scope(\_SB.MCE0.PR17) + { + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package (2) {"phy-mode", "rgmii-id"}, + Package (2) {"phy-handle", \_SB.MDI0.PHY1} + } + }) + } + + Scope(\_SB.MCE0.PR18) + { + Name (_DSD, Package () { + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), + Package () { + Package (2) {"phy-mode", "rgmii-id"}, + Package (2) {"phy-handle", \_SB.MDI0.PHY2}} + } + }) + } + +References +========== + +[1] Documentation/networking/phy.rst + +[2] https://www.uefi.org/sites/default/files/resources/_DSD-device-properties-UUID.pdf + +[3] Documentation/firmware-guide/acpi/DSD-properties-rules.rst + +[4] Documentation/devicetree/bindings/net/ethernet-controller.yaml
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and provide them to be connected to MAC. Describe properties "phy-handle" and "phy-mode". Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> --- Changes in v4: - More cleanup Changes in v3: None Changes in v2: - Updated with more description in document Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst