| Message ID | 20201215164315.3666-1-calvin.johnson@oss.nxp.com |
|---|---|
| Headers | show |
| Series | ACPI support for dpaa2 driver | expand |
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > Define fwnode_phy_find_device() to iterate an mdiobus and find the > phy device of the provided phy fwnode. Additionally define > device_phy_find_device() to find phy device of provided device. > > Define fwnode_get_phy_node() to get phy_node using named reference. ... > +#include <linux/acpi.h> Not sure we need this. See below. ... > +/** > + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided > + * phy_fwnode. Can we keep a summary on one line? > + * @phy_fwnode: Pointer to the phy's fwnode. > + * > + * If successful, returns a pointer to the phy_device with the embedded > + * struct device refcount incremented by one, or NULL on failure. > + */ > +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode) > +{ > + struct mdio_device *mdiodev; > + struct device *d; > + if (!phy_fwnode) > + return NULL; Why is this needed? Perhaps a comment to the function description explains a case when @phy_fwnode == NULL. > + d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode); > + if (d) { > + mdiodev = to_mdio_device(d); > + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) > + return to_phy_device(d); > + put_device(d); > + } > + > + return NULL; > +} ... > + * For ACPI, only "phy-handle" is supported. DT supports all the three > + * named references to the phy node. ... > + /* Only phy-handle is used for ACPI */ > + phy_node = fwnode_find_reference(fwnode, "phy-handle", 0); > + if (is_acpi_node(fwnode) || !IS_ERR(phy_node)) > + return phy_node; So, what is the problem with going through the rest on ACPI? Usually we describe the restrictions in the documentation.
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson <calvin.johnson@oss.nxp.com> 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. ... > +int fwnode_mdiobus_register_phy(struct mii_bus *bus, > + struct fwnode_handle *child, u32 addr) > +{ > + struct mii_timestamper *mii_ts; > + struct phy_device *phy; > + const char *cp; > + bool is_c45; > + 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); > + } Perhaps mii_ts = of_find_mii_timestamper(to_of_node(child)); > + > + rc = fwnode_property_read_string(child, "compatible", &cp); > + is_c45 = !(rc || strcmp(cp, "ethernet-phy-ieee802.3-c45")); > + > + 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); if (!IS_ERR_OR_NULL(mii_ts)) ... However it points to the question why unregister() doesn't handle the above cases. I would expect unconditional call to unregister() here. > + 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); Ditto. > + 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; How is that defined? Do you need to do something with an old pointer? > + } > + return 0; > +}
On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > fwnode_mdiobus_register() internally takes care of both DT > and ACPI cases to register mdiobus. Replace existing > of_mdiobus_register() with fwnode_mdiobus_register(). > > Note: For both ACPI and DT cases, endianness of MDIO controller > need to be specified using "little-endian" property. ... > @@ -2,6 +2,7 @@ > * QorIQ 10G MDIO Controller > * > * Copyright 2012 Freescale Semiconductor, Inc. > + * Copyright 2020 NXP > * > * Authors: Andy Fleming <afleming@freescale.com> > * Timur Tabi <timur@freescale.com> > @@ -11,6 +12,7 @@ I guess this... > priv->is_little_endian = device_property_read_bool(&pdev->dev, > "little-endian"); > - > priv->has_a011043 = device_property_read_bool(&pdev->dev, > "fsl,erratum-a011043"); ...this... > - ...and this changes can go to a separate patch.
On Tue, Dec 15, 2020 at 07:23:26PM +0200, Andy Shevchenko wrote: > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > Define fwnode_phy_find_device() to iterate an mdiobus and find the > > phy device of the provided phy fwnode. Additionally define > > device_phy_find_device() to find phy device of provided device. > > > > Define fwnode_get_phy_node() to get phy_node using named reference. > > ... > > > +#include <linux/acpi.h> > > Not sure we need this. See below. This is required to use is_acpi_node(). > > ... > > > +/** > > + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided > > + * phy_fwnode. > > Can we keep a summary on one line? Ok > > > + * @phy_fwnode: Pointer to the phy's fwnode. > > + * > > + * If successful, returns a pointer to the phy_device with the embedded > > + * struct device refcount incremented by one, or NULL on failure. > > + */ > > +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode) > > +{ > > + struct mdio_device *mdiodev; > > + struct device *d; > > > + if (!phy_fwnode) > > + return NULL; > > Why is this needed? > Perhaps a comment to the function description explains a case when > @phy_fwnode == NULL. I think this function should be modified to follow of_phy_find_device() which has NULL check. I'll add fwnode_mdio_find_device() also. Here is a case where of_phy_find_device() is called without checking phy_np. https://elixir.bootlin.com/linux/latest/source/drivers/net/ethernet/qualcomm/emac/emac-phy.c#L145 > > > + d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode); > > + if (d) { > > + mdiodev = to_mdio_device(d); > > + if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY) > > + return to_phy_device(d); > > + put_device(d); > > + } > > + > > + return NULL; > > +} > > ... > > > + * For ACPI, only "phy-handle" is supported. DT supports all the three > > + * named references to the phy node. > > ... > > > + /* Only phy-handle is used for ACPI */ > > + phy_node = fwnode_find_reference(fwnode, "phy-handle", 0); > > + if (is_acpi_node(fwnode) || !IS_ERR(phy_node)) > > + return phy_node; > > So, what is the problem with going through the rest on ACPI? > Usually we describe the restrictions in the documentation. Others are legacy DT properties which are not intended to be supported in ACPI. I can add this info in the document. Thanks for the review, Andy! Regards Calvin
On Tue, Dec 15, 2020 at 07:55:01PM +0200, Andy Shevchenko wrote: > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > <calvin.johnson@oss.nxp.com> wrote: > > > > fwnode_mdiobus_register() internally takes care of both DT > > and ACPI cases to register mdiobus. Replace existing > > of_mdiobus_register() with fwnode_mdiobus_register(). > > > > Note: For both ACPI and DT cases, endianness of MDIO controller > > need to be specified using "little-endian" property. > > ... > > > @@ -2,6 +2,7 @@ > > * QorIQ 10G MDIO Controller > > * > > * Copyright 2012 Freescale Semiconductor, Inc. > > + * Copyright 2020 NXP > > * > > * Authors: Andy Fleming <afleming@freescale.com> > > * Timur Tabi <timur@freescale.com> > > @@ -11,6 +12,7 @@ > > I guess this... > > > priv->is_little_endian = device_property_read_bool(&pdev->dev, > > "little-endian"); > > - > > priv->has_a011043 = device_property_read_bool(&pdev->dev, > > "fsl,erratum-a011043"); > > ...this... > > > - > > ...and this changes can go to a separate patch. I think I'll remove the unnecessary cleanup. Regards Calvin
On Fri, Dec 18, 2020 at 7:34 AM Calvin Johnson <calvin.johnson@oss.nxp.com> wrote: > > On Tue, Dec 15, 2020 at 07:33:40PM +0200, Andy Shevchenko wrote: > > On Tue, Dec 15, 2020 at 6:44 PM Calvin Johnson > > <calvin.johnson@oss.nxp.com> wrote: ... > > > + /* 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; > > > > How is that defined? Do you need to do something with an old pointer? > > As the comment says, I think PHY drivers which got invoked before calling > of_mdiobus_register_phy() may have defined phy->mii_ts. What I meant here is that the old pointer might need some care (freeing?). > > > + }
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. END 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 (14): Documentation: ACPI: DSD: Document MDIO PHY 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() net: mdiobus: Introduce fwnode_mdiobus_register() net/fsl: Use fwnode_mdiobus_register() device property: Introduce fwnode_get_id() phylink: introduce phylink_fwnode_phy_connect() net: phylink: Refactor phylink_of_phy_connect() net: phy: Introduce fwnode_mdio_find_device() net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver Documentation/firmware-guide/acpi/dsd/phy.rst | 129 ++++++++++++++++++ drivers/base/property.c | 26 ++++ .../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 86 +++++++----- drivers/net/ethernet/freescale/xgmac_mdio.c | 14 +- drivers/net/mdio/of_mdio.c | 79 +---------- drivers/net/phy/mdio_bus.c | 116 ++++++++++++++++ drivers/net/phy/phy_device.c | 108 +++++++++++++++ drivers/net/phy/phylink.c | 49 ++++--- 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 + 13 files changed, 519 insertions(+), 132 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst