| Message ID | 20200930160430.7908-1-calvin.johnson@oss.nxp.com |
|---|---|
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
Hi Calvin > priv->has_a011043 = device_property_read_bool(&pdev->dev, > "fsl,erratum-a011043"); > - > - ret = of_mdiobus_register(bus, np); > - if (ret) { > - dev_err(&pdev->dev, "cannot register MDIO bus\n"); > + if (is_of_node(pdev->dev.fwnode)) { > + ret = of_mdiobus_register(bus, np); > + if (ret) { > + dev_err(&pdev->dev, "cannot register MDIO bus\n"); > + goto err_registration; > + } > + } else if (is_acpi_node(pdev->dev.fwnode)) { > + priv->is_little_endian = true; > + /* Mask out all PHYs from auto probing. */ > + bus->phy_mask = ~0; > + ret = mdiobus_register(bus); > + if (ret) { > + dev_err(&pdev->dev, "mdiobus register err(%d)\n", ret); > + return ret; > + } > + > + fwnode = pdev->dev.fwnode; > + /* Loop over the child nodes and register a phy_device for each PHY */ > + fwnode_for_each_child_node(fwnode, child) { > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(child), > + "_ADR", NULL, &addr); > + if (ACPI_FAILURE(status)) { > + pr_debug("_ADR returned %d\n", status); > + continue; > + } > + > + if (addr < 0 || addr >= PHY_MAX_ADDR) > + continue; > + > + ret = fwnode_mdiobus_register_phy(bus, child, addr); > + if (ret == -ENODEV) > + dev_err(&bus->dev, > + "MDIO device at address %lld is missing.\n", > + addr); > + } Hi Calvin This looping over the properties should be in the core, in the same way of_mdiobus_register() loops over the OF properties in the core. We don't want MDIO drivers doing this in their own way, with their own bugs. Andrew
On Wed, Sep 30, 2020 at 6:05 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> > --- > > Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++ > 1 file changed, 78 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..f10feb24ec1c > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > @@ -0,0 +1,78 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +========================= > +MDIO bus and PHYs in ACPI > +========================= > + > +The PHYs on an mdiobus are probed and registered using > +fwnode_mdiobus_register_phy(). > +Later, for connecting these PHYs to MAC, the PHYs registered on the > +mdiobus have to be referenced. > + > +phy-handle > +----------- > +For each MAC node, a property "phy-handle" is used to reference the > +PHY that is registered on an MDIO bus. It is not clear what "a property" means in this context. This should refer to the documents introducing the _DSD-based generic device properties rules, including the GUID used below. You need to say whether or not the property is mandatory and if it isn't mandatory, you need to say what the lack of it means. > + > +phy-mode > +-------- > +Property "phy-mode" defines the type of PHY interface. This needs to be more detailed too, IMO. At the very least, please list all of the possible values of it and document their meaning. > + > +An example of this is shown below:: > + > +DSDT entry for MACs where PHY nodes are referenced > +-------------------------------------------------- > + Scope(\_SB.MCE0.PR17) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}} What is "phy-handle"? You haven't introduced it above. > + } > + }) > + } > + > + Scope(\_SB.MCE0.PR18) // 1G > + { > + Name (_DSD, Package () { > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > + Package () { > + Package (2) {"phy-mode", "rgmii-id"}, > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}} > + } > + }) > + } > + > +DSDT entry for MDIO node > +------------------------ > +a) Silicon Component What is this device, exactly? > +-------------------- > + 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 > +--------------------- > + Scope(\_SB.MDI0) > + { > + Device(PHY1) { > + Name (_ADR, 0x1) > + } // end of PHY1 > + > + Device(PHY2) { > + Name (_ADR, 0x2) > + } // end of PHY2 > + } > -- What is the connection between the last two pieces of ASL and the _DSD definitions above?
From: Calvin Johnson <calvin.johnson@oss.nxp.com> Date: Wed, 30 Sep 2020 21:34:27 +0530 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -106,6 +106,46 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev) > } > EXPORT_SYMBOL(mdiobus_unregister_device); > > +int fwnode_mdiobus_register_phy(struct mii_bus *bus, > + struct fwnode_handle *child, u32 addr) > +{ > + struct phy_device *phy; > + bool is_c45; > + const char *cp; > + u32 phy_id; > + int rc; Reverse christmas tree here please.
Hi Rafael, On Wed, Sep 30, 2020 at 06:37:09PM +0200, Rafael J. Wysocki wrote: > On Wed, Sep 30, 2020 at 6:05 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> > > --- > > > > Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++ > > 1 file changed, 78 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..f10feb24ec1c > > --- /dev/null > > +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > > @@ -0,0 +1,78 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +========================= > > +MDIO bus and PHYs in ACPI > > +========================= > > + > > +The PHYs on an mdiobus are probed and registered using > > +fwnode_mdiobus_register_phy(). > > +Later, for connecting these PHYs to MAC, the PHYs registered on the > > +mdiobus have to be referenced. > > + > > +phy-handle > > +----------- > > +For each MAC node, a property "phy-handle" is used to reference the > > +PHY that is registered on an MDIO bus. > > It is not clear what "a property" means in this context. > In rev-2, I'll add more info on this. 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}. > This should refer to the documents introducing the _DSD-based generic > device properties rules, including the GUID used below. > Sure. I'll refer "Documentation/firmware-guide/acpi/DSD-properties-rules.rst" > You need to say whether or not the property is mandatory and if it > isn't mandatory, you need to say what the lack of it means. > I'll do that. > > + > > +phy-mode > > +-------- > > +Property "phy-mode" defines the type of PHY interface. > > This needs to be more detailed too, IMO. At the very least, please > list all of the possible values of it and document their meaning. > > > + > > +An example of this is shown below:: > > + > > +DSDT entry for MACs where PHY nodes are referenced > > +-------------------------------------------------- > > + Scope(\_SB.MCE0.PR17) // 1G > > + { > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package (2) {"phy-mode", "rgmii-id"}, > > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}} > > What is "phy-handle"? > > You haven't introduced it above. I thought I introduced it earlier in this document as a property. Ofcourse, more info needs to be added as you mentioned. Other than that am I missing something? I've a correction here. Based on referring some more documents, I'll be using Package (2) {"phy-handle",\_SB.MDI0.PHY1} instead of Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}} . > > + } > > + }) > > + } > > + > > + Scope(\_SB.MCE0.PR18) // 1G > > + { > > + Name (_DSD, Package () { > > + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > + Package () { > > + Package (2) {"phy-mode", "rgmii-id"}, > > + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}} > > + } > > + }) > > + } > > + > > +DSDT entry for MDIO node > > +------------------------ > > +a) Silicon Component > > What is this device, exactly? I'll explain it more clearly. > > > +-------------------- > > + 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 > > +--------------------- > > + Scope(\_SB.MDI0) > > + { > > + Device(PHY1) { > > + Name (_ADR, 0x1) > > + } // end of PHY1 > > + > > + Device(PHY2) { > > + Name (_ADR, 0x2) > > + } // end of PHY2 > > + } > > -- > > What is the connection between the last two pieces of ASL and the _DSD > definitions above? In rev-2, I'll explain the relation between these pieces. What I tried to show is that the MDIO bus has an SoC component(mdio controller) and a platform component(PHYs on the mdiobus). In the MAC nodes, the PHYs are referenced and that is done using the "phy-handle" property. Thanks Calvin
On 30/09/2020 17:37, Rafael J. Wysocki wrote: > On Wed, Sep 30, 2020 at 6:05 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> >> --- >> >> Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++ >> 1 file changed, 78 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..f10feb24ec1c >> --- /dev/null >> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst >> @@ -0,0 +1,78 @@ >> +.. SPDX-License-Identifier: GPL-2.0 >> + >> +========================= >> +MDIO bus and PHYs in ACPI >> +========================= >> + >> +The PHYs on an mdiobus are probed and registered using >> +fwnode_mdiobus_register_phy(). >> +Later, for connecting these PHYs to MAC, the PHYs registered on the >> +mdiobus have to be referenced. >> + >> +phy-handle >> +----------- >> +For each MAC node, a property "phy-handle" is used to reference the >> +PHY that is registered on an MDIO bus. > > It is not clear what "a property" means in this context. > > This should refer to the documents introducing the _DSD-based generic > device properties rules, including the GUID used below. > > You need to say whether or not the property is mandatory and if it > isn't mandatory, you need to say what the lack of it means. > >> + >> +phy-mode >> +-------- >> +Property "phy-mode" defines the type of PHY interface. > > This needs to be more detailed too, IMO. At the very least, please > list all of the possible values of it and document their meaning. If the goal is to align with DT, it would be appropriate to point to where those properties are defined for DT rather than to have a separate description here. I suggest something along the lines of: The "phy-mode" _DSD property is used to describe the connection to the PHY. The valid values for "phy-mode" are defined in Documentation/devicetree/bindings/ethernet-controller.yaml > >> + >> +An example of this is shown below:: >> + >> +DSDT entry for MACs where PHY nodes are referenced >> +-------------------------------------------------- >> + Scope(\_SB.MCE0.PR17) // 1G >> + { >> + Name (_DSD, Package () { >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> + Package () { >> + Package (2) {"phy-mode", "rgmii-id"}, >> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}} > > What is "phy-handle"? > > You haven't introduced it above. Can you elaborate? "phy-handle" has a section to itself in this document. Agree that it needs to be defined more, but it does read to me as having been defined. > >> + } >> + }) >> + } >> + >> + Scope(\_SB.MCE0.PR18) // 1G >> + { >> + Name (_DSD, Package () { >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), >> + Package () { >> + Package (2) {"phy-mode", "rgmii-id"}, >> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}} >> + } >> + }) >> + } >> + >> +DSDT entry for MDIO node >> +------------------------ >> +a) Silicon Component > > What is this device, exactly? > >> +-------------------- >> + 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 >> +--------------------- >> + Scope(\_SB.MDI0) >> + { >> + Device(PHY1) { >> + Name (_ADR, 0x1) >> + } // end of PHY1 >> + >> + Device(PHY2) { >> + Name (_ADR, 0x2) >> + } // end of PHY2 >> + } >> -- > > What is the connection between the last two pieces of ASL and the _DSD > definitions above? >
On Fri, Oct 2, 2020 at 1:09 PM Grant Likely <grant.likely@arm.com> wrote: > > > > On 30/09/2020 17:37, Rafael J. Wysocki wrote: > > On Wed, Sep 30, 2020 at 6:05 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> > >> --- > >> > >> Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++++++++++++++++++ > >> 1 file changed, 78 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..f10feb24ec1c > >> --- /dev/null > >> +++ b/Documentation/firmware-guide/acpi/dsd/phy.rst > >> @@ -0,0 +1,78 @@ > >> +.. SPDX-License-Identifier: GPL-2.0 > >> + > >> +========================= > >> +MDIO bus and PHYs in ACPI > >> +========================= > >> + > >> +The PHYs on an mdiobus are probed and registered using > >> +fwnode_mdiobus_register_phy(). > >> +Later, for connecting these PHYs to MAC, the PHYs registered on the > >> +mdiobus have to be referenced. > >> + > >> +phy-handle > >> +----------- > >> +For each MAC node, a property "phy-handle" is used to reference the > >> +PHY that is registered on an MDIO bus. > > > > It is not clear what "a property" means in this context. > > > > This should refer to the documents introducing the _DSD-based generic > > device properties rules, including the GUID used below. > > > > You need to say whether or not the property is mandatory and if it > > isn't mandatory, you need to say what the lack of it means. > > > >> + > >> +phy-mode > >> +-------- > >> +Property "phy-mode" defines the type of PHY interface. > > > > This needs to be more detailed too, IMO. At the very least, please > > list all of the possible values of it and document their meaning. > > If the goal is to align with DT, it would be appropriate to point to > where those properties are defined for DT rather than to have a separate > description here. I suggest something along the lines of: > > The "phy-mode" _DSD property is used to describe the connection to > the PHY. The valid values for "phy-mode" are defined in > Documentation/devicetree/bindings/ethernet-controller.yaml > > > > >> + > >> +An example of this is shown below:: > >> + > >> +DSDT entry for MACs where PHY nodes are referenced > >> +-------------------------------------------------- > >> + Scope(\_SB.MCE0.PR17) // 1G > >> + { > >> + Name (_DSD, Package () { > >> + ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > >> + Package () { > >> + Package (2) {"phy-mode", "rgmii-id"}, > >> + Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}} > > > > What is "phy-handle"? > > > > You haven't introduced it above. > > Can you elaborate? "phy-handle" has a section to itself in this > document. Yes, it does. I overlooked it, sorry. > Agree that it needs to be defined more, but it does read to me > as having been defined. Yup. Cheers!