| Message ID | 20210122154300.7628-1-calvin.johnson@oss.nxp.com |
|---|---|
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
On Fri, Jan 22, 2021 at 09:12:54PM +0530, Calvin Johnson wrote: > Using fwnode_get_id(), get the reg property value for DT node > or get the _ADR object value for ACPI node. ... > +/** > + * fwnode_get_id - Get the id of a fwnode. > + * @fwnode: firmware node > + * @id: id of the fwnode > + * > + * This function provides the id of a fwnode which can be either > + * DT or ACPI node. For ACPI, "reg" property value, if present will > + * be provided or else _ADR value will be provided. > + * Returns 0 on success or a negative errno. > + */ > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > +{ > +#ifdef CONFIG_ACPI > + unsigned long long adr; > + acpi_status status; > +#endif Instead you may do... > + int ret; > + > + ret = fwnode_property_read_u32(fwnode, "reg", id); > + if (ret) { > +#ifdef CONFIG_ACPI ...it here like 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 -EINVAL; > + *id = (u32)adr; > +#else > + return ret; > +#endif > + } > + return 0; > +}
On Fri, Jan 22, 2021 at 4:46 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Using fwnode_get_id(), get the reg property value for DT node > or get the _ADR object value for ACPI node. So I'm not really sure if this is going to be generically useful. First of all, the meaning of the _ADR return value is specific to a given bus type (e.g. the PCI encoding of it is different from the I2C encoding of it) and it just happens to be matching the definition of the "reg" property for this particular binding. IOW, not everyone may expect the "reg" property and the _ADR return value to have the same encoding and belong to the same set of values, so maybe put this function somewhere closer to the code that's going to use it, because it seems to be kind of specific to this particular use case? > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > Changes in v4: > - Improve code structure to handle all cases > > Changes in v3: > - Modified to retrieve reg property value for ACPI as well > - Resolved compilation issue with CONFIG_ACPI = n > - Added more info into documentation > > Changes in v2: None > > drivers/base/property.c | 34 ++++++++++++++++++++++++++++++++++ > include/linux/property.h | 1 + > 2 files changed, 35 insertions(+) > > diff --git a/drivers/base/property.c b/drivers/base/property.c > index 35b95c6ac0c6..f0581bbf7a4b 100644 > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -580,6 +580,40 @@ const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode) > return fwnode_call_ptr_op(fwnode, get_name_prefix); > } > > +/** > + * fwnode_get_id - Get the id of a fwnode. > + * @fwnode: firmware node > + * @id: id of the fwnode > + * > + * This function provides the id of a fwnode which can be either > + * DT or ACPI node. For ACPI, "reg" property value, if present will > + * be provided or else _ADR value will be provided. > + * Returns 0 on success or a negative errno. > + */ > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id) > +{ > +#ifdef CONFIG_ACPI > + unsigned long long adr; > + acpi_status status; > +#endif > + int ret; > + > + ret = fwnode_property_read_u32(fwnode, "reg", id); > + if (ret) { > +#ifdef CONFIG_ACPI > + status = acpi_evaluate_integer(ACPI_HANDLE_FWNODE(fwnode), > + METHOD_NAME__ADR, NULL, &adr); > + if (ACPI_FAILURE(status)) > + return -EINVAL; > + *id = (u32)adr; > +#else > + return ret; > +#endif > + } > + return 0; > +} > +EXPORT_SYMBOL_GPL(fwnode_get_id); > + > /** > * fwnode_get_parent - Return parent firwmare node > * @fwnode: Firmware whose parent is retrieved > diff --git a/include/linux/property.h b/include/linux/property.h > index 0a9001fe7aea..3f41475f010b 100644 > --- a/include/linux/property.h > +++ b/include/linux/property.h > @@ -82,6 +82,7 @@ struct fwnode_handle *fwnode_find_reference(const struct fwnode_handle *fwnode, > > const char *fwnode_get_name(const struct fwnode_handle *fwnode); > const char *fwnode_get_name_prefix(const struct fwnode_handle *fwnode); > +int fwnode_get_id(struct fwnode_handle *fwnode, u32 *id); > struct fwnode_handle *fwnode_get_parent(const struct fwnode_handle *fwnode); > struct fwnode_handle *fwnode_get_next_parent( > struct fwnode_handle *fwnode); > -- > 2.17.1 >
On Fri, Jan 22, 2021 at 7:11 PM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Fri, Jan 22, 2021 at 6:12 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > > > On Fri, Jan 22, 2021 at 05:40:41PM +0100, Rafael J. Wysocki wrote: > > > On Fri, Jan 22, 2021 at 4:46 PM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: > > > > > > > > Using fwnode_get_id(), get the reg property value for DT node > > > > or get the _ADR object value for ACPI node. > > > > > > So I'm not really sure if this is going to be generically useful. > > > > > > First of all, the meaning of the _ADR return value is specific to a > > > given bus type (e.g. the PCI encoding of it is different from the I2C > > > encoding of it) and it just happens to be matching the definition of > > > the "reg" property for this particular binding. > > > > > IOW, not everyone may expect the "reg" property and the _ADR return > > > value to have the same encoding and belong to the same set of values, > > > > I have counted three or even four attempts to open code exact this scenario > > in the past couple of years. And I have no idea where to put a common base for > > them so they will not duplicate this in each case. > > In that case it makes sense to have it in the core, but calling the > _ADR return value an "id" generically is a stretch to put it lightly. > > It may be better to call the function something like > fwnode_get_local_bus_id() Or fwnode_get_local_address() for that matter.
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 >
On Fri, 22 Jan 2021 21:12:55 +0530 Calvin Johnson wrote: > Define acpi_mdiobus_register() to Register mii_bus and create PHYs for > each ACPI child node. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> ERROR: modpost: missing MODULE_LICENSE() in drivers/net/mdio/acpi_mdio.o make[2]: *** [Module.symvers] Error 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.
On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote: > Introduce fwnode_mdiobus_register_phy() to register PHYs on the > mdiobus. From the compatible string, identify whether the PHY is > c45 and based on this create a PHY device instance which is > registered on the mdiobus. > > Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com> > --- > > Changes in v4: None > Changes in v3: None > Changes in v2: None > > drivers/net/mdio/of_mdio.c | 3 +- > drivers/net/phy/mdio_bus.c | 67 ++++++++++++++++++++++++++++++++++++++ > include/linux/mdio.h | 2 ++ > include/linux/of_mdio.h | 6 +++- > 4 files changed, 76 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c > index d4cc293358f7..cd7da38ae763 100644 > --- a/drivers/net/mdio/of_mdio.c > +++ b/drivers/net/mdio/of_mdio.c > @@ -32,7 +32,7 @@ static int of_get_phy_id(struct device_node *device, u32 *phy_id) > return fwnode_get_phy_id(of_fwnode_handle(device), phy_id); > } > > -static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node) > +struct mii_timestamper *of_find_mii_timestamper(struct device_node *node) > { > struct of_phandle_args arg; > int err; > @@ -49,6 +49,7 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node) > > return register_mii_timestamper(arg.np, arg.args[0]); > } > +EXPORT_SYMBOL(of_find_mii_timestamper); > > int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy, > struct device_node *child, u32 addr) > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 040509b81f02..44ddfb0ba99f 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -8,6 +8,7 @@ > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > > +#include <linux/acpi.h> > #include <linux/delay.h> > #include <linux/device.h> > #include <linux/errno.h> > @@ -106,6 +107,72 @@ 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 mii_timestamper *mii_ts; > + struct phy_device *phy; > + bool is_c45 = false; > + u32 phy_id; > + int rc; > + > + if (is_of_node(child)) { > + mii_ts = of_find_mii_timestamper(to_of_node(child)); > + if (IS_ERR(mii_ts)) > + return PTR_ERR(mii_ts); > + } > + > + rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45"); With ACPI, I'm facing some problem with fwnode_property_match_string(). It is unable to detect the compatible string and returns -EPROTO. ACPI node for PHY4 is as below: Device(PHY4) { Name (_ADR, 0x4) Name(_CRS, ResourceTemplate() { Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) { AQR_PHY4_IT } }) // end of _CRS for PHY4 Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { Package () {"compatible", "ethernet-phy-ieee802.3-c45"} } }) } // end of PHY4 What is see is that in acpi_data_get_property(), propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE). Any help please? fwnode_property_match_string() works fine for DT. Thanks Calvin > + if (rc >= 0) > + is_c45 = true; > + > + if (is_c45 || fwnode_get_phy_id(child, &phy_id)) > + phy = get_phy_device(bus, addr, is_c45); > + else > + phy = phy_device_create(bus, addr, phy_id, 0, NULL); > + if (IS_ERR(phy)) { > + if (mii_ts && is_of_node(child)) > + unregister_mii_timestamper(mii_ts); > + return PTR_ERR(phy); > + } > + > + if (is_acpi_node(child)) { > + phy->irq = bus->irq[addr]; > + > + /* Associate the fwnode with the device structure so it > + * can be looked up later. > + */ > + phy->mdio.dev.fwnode = child; > + > + /* All data is now stored in the phy struct, so register it */ > + rc = phy_device_register(phy); > + if (rc) { > + phy_device_free(phy); > + fwnode_handle_put(phy->mdio.dev.fwnode); > + return rc; > + } > + > + dev_dbg(&bus->dev, "registered phy at address %i\n", addr); > + } else if (is_of_node(child)) { > + rc = of_mdiobus_phy_device_register(bus, phy, to_of_node(child), addr); > + if (rc) { > + if (mii_ts) > + unregister_mii_timestamper(mii_ts); > + phy_device_free(phy); > + return rc; > + } > + > + /* phy->mii_ts may already be defined by the PHY driver. A > + * mii_timestamper probed via the device tree will still have > + * precedence. > + */ > + if (mii_ts) > + phy->mii_ts = mii_ts; > + } > + return 0; > +} > +EXPORT_SYMBOL(fwnode_mdiobus_register_phy); > + > struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr) > { > struct mdio_device *mdiodev = bus->mdio_map[addr]; > diff --git a/include/linux/mdio.h b/include/linux/mdio.h > index ffb787d5ebde..7f4215c069fe 100644 > --- a/include/linux/mdio.h > +++ b/include/linux/mdio.h > @@ -381,6 +381,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev); > int mdiobus_unregister_device(struct mdio_device *mdiodev); > bool mdiobus_is_registered_device(struct mii_bus *bus, int addr); > struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr); > +int fwnode_mdiobus_register_phy(struct mii_bus *bus, > + struct fwnode_handle *child, u32 addr); > > /** > * mdio_module_driver() - Helper macro for registering mdio drivers > diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h > index cfe8c607a628..3b66016f18aa 100644 > --- a/include/linux/of_mdio.h > +++ b/include/linux/of_mdio.h > @@ -34,6 +34,7 @@ struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np); > int of_phy_register_fixed_link(struct device_node *np); > void of_phy_deregister_fixed_link(struct device_node *np); > bool of_phy_is_fixed_link(struct device_node *np); > +struct mii_timestamper *of_find_mii_timestamper(struct device_node *np); > int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy, > struct device_node *child, u32 addr); > > @@ -128,7 +129,10 @@ static inline bool of_phy_is_fixed_link(struct device_node *np) > { > return false; > } > - > +static inline struct mii_timestamper *of_find_mii_timestamper(struct device_node *np) > +{ > + return NULL; > +} > static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio, > struct phy_device *phy, > struct device_node *child, u32 addr) > -- > 2.17.1 >
On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote: ... > > + rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45"); > With ACPI, I'm facing some problem with fwnode_property_match_string(). It is > unable to detect the compatible string and returns -EPROTO. > > ACPI node for PHY4 is as below: > > Device(PHY4) { > Name (_ADR, 0x4) > Name(_CRS, ResourceTemplate() { > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > { > AQR_PHY4_IT > } > }) // end of _CRS for PHY4 > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > Package () {"compatible", "ethernet-phy-ieee802.3-c45"} > } > }) > } // end of PHY4 > > What is see is that in acpi_data_get_property(), > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE). > > Any help please? > > fwnode_property_match_string() works fine for DT. Can you show the DT node which works and also input for the )match_string() (i.o.w what exactly you are trying to match with)? -- With Best Regards, Andy Shevchenko
On Fri, Feb 5, 2021 at 8:25 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote: > > ... > > > > + rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45"); > > With ACPI, I'm facing some problem with fwnode_property_match_string(). It is > > unable to detect the compatible string and returns -EPROTO. > > > > ACPI node for PHY4 is as below: > > > > Device(PHY4) { > > Name (_ADR, 0x4) > > Name(_CRS, ResourceTemplate() { > > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > { > > AQR_PHY4_IT > > } > > }) // end of _CRS for PHY4 > > Name (_DSD, Package () { > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > Package () { > > Package () {"compatible", "ethernet-phy-ieee802.3-c45"} I guess converting this to Package () {"compatible", Package() {"ethernet-phy-ieee802.3-c45"}} will solve it. > > } > > }) > > } // end of PHY4 > > > > What is see is that in acpi_data_get_property(), > > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE). > > > > Any help please? > > > > fwnode_property_match_string() works fine for DT. > > Can you show the DT node which works and also input for the > )match_string() (i.o.w what exactly you are trying to match with)? > > -- > With Best Regards, > Andy Shevchenko
On Fri, Feb 5, 2021 at 8:41 PM Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Feb 5, 2021 at 8:25 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: > > > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote: > > > > ... > > > > > > + rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45"); > > > With ACPI, I'm facing some problem with fwnode_property_match_string(). It is > > > unable to detect the compatible string and returns -EPROTO. > > > > > > ACPI node for PHY4 is as below: > > > > > > Device(PHY4) { > > > Name (_ADR, 0x4) > > > Name(_CRS, ResourceTemplate() { > > > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > > { > > > AQR_PHY4_IT > > > } > > > }) // end of _CRS for PHY4 > > > Name (_DSD, Package () { > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > Package () { > > > > Package () {"compatible", "ethernet-phy-ieee802.3-c45"} > > I guess converting this to > Package () {"compatible", Package() {"ethernet-phy-ieee802.3-c45"}} > will solve it. For the record, it doesn't mean there is no bug in the code. DT treats a single string as an array, but ACPI doesn't. And this is specific to _match_string() because it has two passes. And the first one fails. While reading a single string as an array of 1 element will work I believe. > > > } > > > > }) > > > } // end of PHY4 > > > > > > What is see is that in acpi_data_get_property(), > > > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE). > > > > > > Any help please? > > > > > > fwnode_property_match_string() works fine for DT. > > > > Can you show the DT node which works and also input for the > > )match_string() (i.o.w what exactly you are trying to match with)? -- With Best Regards, Andy Shevchenko
On Fri, Feb 05, 2021 at 08:58:06PM +0200, Andy Shevchenko wrote: > On Fri, Feb 5, 2021 at 8:41 PM Andy Shevchenko > <andy.shevchenko@gmail.com> wrote: > > On Fri, Feb 5, 2021 at 8:25 PM Andy Shevchenko > > <andy.shevchenko@gmail.com> wrote: > > > On Fri, Feb 5, 2021 at 7:25 PM Calvin Johnson > > > <calvin.johnson@oss.nxp.com> wrote: > > > > On Fri, Jan 22, 2021 at 09:12:52PM +0530, Calvin Johnson wrote: > > > > > > ... > > > > > > > > + rc = fwnode_property_match_string(child, "compatible", "ethernet-phy-ieee802.3-c45"); > > > > With ACPI, I'm facing some problem with fwnode_property_match_string(). It is > > > > unable to detect the compatible string and returns -EPROTO. > > > > > > > > ACPI node for PHY4 is as below: > > > > > > > > Device(PHY4) { > > > > Name (_ADR, 0x4) > > > > Name(_CRS, ResourceTemplate() { > > > > Interrupt(ResourceConsumer, Level, ActiveHigh, Shared) > > > > { > > > > AQR_PHY4_IT > > > > } > > > > }) // end of _CRS for PHY4 > > > > Name (_DSD, Package () { > > > > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > > > > Package () { > > > > > > Package () {"compatible", "ethernet-phy-ieee802.3-c45"} > > > > I guess converting this to > > Package () {"compatible", Package() {"ethernet-phy-ieee802.3-c45"}} > > will solve it. Thanks a lot Andy! This helped. But is this the correct way to define compatible string value, i.e as a sub package. > > For the record, it doesn't mean there is no bug in the code. DT treats > a single string as an array, but ACPI doesn't. > And this is specific to _match_string() because it has two passes. And > the first one fails. > While reading a single string as an array of 1 element will work I believe. > > > > > } > > > > > > }) > > > > } // end of PHY4 > > > > > > > > What is see is that in acpi_data_get_property(), > > > > propvalue->type = 0x2(ACPI_TYPE_STRING) and type = 0x4(ACPI_TYPE_PACKAGE). > > > > > > > > Any help please? > > > > > > > > fwnode_property_match_string() works fine for DT. > > > > > > Can you show the DT node which works and also input for the > > > )match_string() (i.o.w what exactly you are trying to match with)? > > > -- > With Best Regards, > Andy Shevchenko
This patch set provides ACPI support to DPAA2 network drivers. It also introduces new fwnode based APIs to support phylink and phy layers Following functions are defined: phylink_fwnode_phy_connect() fwnode_mdiobus_register_phy() fwnode_mdiobus_register() fwnode_get_phy_id() fwnode_phy_find_device() device_phy_find_device() fwnode_get_phy_node() fwnode_mdio_find_device() fwnode_get_id() First one helps in connecting phy to phylink instance. Next three helps in getting phy_id and registering phy to mdiobus Next two help in finding a phy on a mdiobus. Next one helps in getting phy_node from a fwnode. Last one is used to get fwnode ID. Corresponding OF functions are refactored. Tested-on: LS2088ARDB and LX2160ARDB Changes in v4: - More cleanup - Improve code structure to handle all cases - Remove redundant else from fwnode_mdiobus_register() - Cleanup xgmac_mdio_probe() - call phy_device_free() before returning Changes in v3: - Add more info on legacy DT properties "phy" and "phy-device" - Redefine fwnode_phy_find_device() to follow of_phy_find_device() - Use traditional comparison pattern - Use GENMASK - Modified to retrieve reg property value for ACPI as well - Resolved compilation issue with CONFIG_ACPI = n - Added more info into documentation - Use acpi_mdiobus_register() - Avoid unnecessary line removal - Remove unused inclusion of acpi.h Changes in v2: - Updated with more description in document - use reverse christmas tree ordering for local variables - Refactor OF functions to use fwnode functions Calvin Johnson (15): Documentation: ACPI: DSD: Document MDIO PHY net: phy: Introduce fwnode_mdio_find_device() net: phy: Introduce phy related fwnode functions of: mdio: Refactor of_phy_find_device() net: phy: Introduce fwnode_get_phy_id() of: mdio: Refactor of_get_phy_id() net: mdiobus: Introduce fwnode_mdiobus_register_phy() of: mdio: Refactor of_mdiobus_register_phy() device property: Introduce fwnode_get_id() net: mdio: Add ACPI support code for mdio net: mdiobus: Introduce fwnode_mdiobus_register() net/fsl: Use fwnode_mdiobus_register() phylink: introduce phylink_fwnode_phy_connect() net: phylink: Refactor phylink_of_phy_connect() net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ MAINTAINERS | 1 + drivers/base/property.c | 34 +++++ .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 87 +++++++----- drivers/net/ethernet/freescale/xgmac_mdio.c | 11 +- drivers/net/mdio/Kconfig | 7 + drivers/net/mdio/Makefile | 1 + drivers/net/mdio/acpi_mdio.c | 49 +++++++ drivers/net/mdio/of_mdio.c | 79 +---------- drivers/net/phy/mdio_bus.c | 88 ++++++++++++ drivers/net/phy/phy_device.c | 106 ++++++++++++++ drivers/net/phy/phylink.c | 53 ++++--- include/linux/acpi_mdio.h | 27 ++++ include/linux/mdio.h | 2 + include/linux/of_mdio.h | 6 +- include/linux/phy.h | 32 +++++ include/linux/phylink.h | 3 + include/linux/property.h | 1 + 18 files changed, 584 insertions(+), 132 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst create mode 100644 drivers/net/mdio/acpi_mdio.c create mode 100644 include/linux/acpi_mdio.h