| Message ID | 20230116173420.1278704-3-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA: switch to fwnode_/device_ | expand |
On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > fixed-link PHYs API is used by DSA and a number of drivers > and was depending on of_. Switch to fwnode_ so to make it > hardware description agnostic and allow to be used in ACPI > world as well. Would it be better to let the fixed-link PHY die, and have everyone use the more flexible fixed link implementation in phylink?
On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote: > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > fixed-link PHYs API is used by DSA and a number of drivers > > and was depending on of_. Switch to fwnode_ so to make it > > hardware description agnostic and allow to be used in ACPI > > world as well. > > Would it be better to let the fixed-link PHY die, and have everyone use > the more flexible fixed link implementation in phylink? Would it be even better if DSA had some driver-level prerequisites to impose for ACPI support - like phylink support rather than adjust_link - and we would simply branch off to a dsa_shared_port_link_register_acpi() function, leaving the current dsa_shared_port_link_register_of() alone, with all its workarounds and hacks? I don't believe that carrying all that logic over to a common fwnode based API is the proper way forward.
On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote: > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote: > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > fixed-link PHYs API is used by DSA and a number of drivers > > > and was depending on of_. Switch to fwnode_ so to make it > > > hardware description agnostic and allow to be used in ACPI > > > world as well. > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > the more flexible fixed link implementation in phylink? > > Would it be even better if DSA had some driver-level prerequisites to > impose for ACPI support - like phylink support rather than adjust_link - > and we would simply branch off to a dsa_shared_port_link_register_acpi() > function, leaving the current dsa_shared_port_link_register_of() alone, > with all its workarounds and hacks? I don't believe that carrying all > that logic over to a common fwnode based API is the proper way forward. I agree with you there, here is little attempt to make a clean ACPI binding. Most of the attempts to add ACPI support seem to try to take the short cut for just search/replace of_ with fwnode_. And we then have to push back and say no, and generally it then goes quiet. Marcin, please approach this from the other end. Please document in Documentation/firmware-guide/acpi/dsd what a clean binding should look like, and then try to implement it. Andrew
> +int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode) > +{ > + struct fixed_phy_status status = {}; > + struct fwnode_handle *fixed_link_node; > + u32 fixed_link_prop[5]; > + const char *managed; > + int rc; > + > + if (fwnode_property_read_string(fwnode, "managed", &managed) == 0 && > + strcmp(managed, "in-band-status") == 0) { > + /* status is zeroed, namely its .link member */ > + goto register_phy; > + } > + > + /* New binding */ > + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link"); > + if (fixed_link_node) { > + status.link = 1; > + status.duplex = fwnode_property_present(fixed_link_node, > + "full-duplex"); > + rc = fwnode_property_read_u32(fixed_link_node, "speed", > + &status.speed); > + if (rc) { > + fwnode_handle_put(fixed_link_node); > + return rc; > + } > + status.pause = fwnode_property_present(fixed_link_node, "pause"); > + status.asym_pause = fwnode_property_present(fixed_link_node, > + "asym-pause"); > + fwnode_handle_put(fixed_link_node); > + > + goto register_phy; > + } > + > + /* Old binding */ > + rc = fwnode_property_read_u32_array(fwnode, "fixed-link", fixed_link_prop, > + ARRAY_SIZE(fixed_link_prop)); > + if (rc) > + return rc; > + > + status.link = 1; > + status.duplex = fixed_link_prop[1]; > + status.speed = fixed_link_prop[2]; > + status.pause = fixed_link_prop[3]; > + status.asym_pause = fixed_link_prop[4]; This is one example of the issue i just pointed out. The "Old binding" has been deprecated for years. Maybe a decade? There is no reason it should be used in ACPI. Andrew
Hi Marcin, On Mon, 2023-01-16 at 18:34 +0100, Marcin Wojtas wrote: > fixed-link PHYs API is used by DSA and a number of drivers > and was depending on of_. Switch to fwnode_ so to make it > hardware description agnostic and allow to be used in ACPI > world as well. > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > include/linux/fwnode_mdio.h | 19 ++++ > drivers/net/mdio/fwnode_mdio.c | 96 ++++++++++++++++++++ > drivers/net/mdio/of_mdio.c | 79 +--------------- > 3 files changed, 118 insertions(+), 76 deletions(-) > > #endif /* __LINUX_FWNODE_MDIO_H */ > diff --git a/drivers/net/mdio/fwnode_mdio.c > b/drivers/net/mdio/fwnode_mdio.c > index b782c35c4ac1..56f57381ae69 100644 > --- a/drivers/net/mdio/fwnode_mdio.c > +++ b/drivers/net/mdio/fwnode_mdio.c > @@ -10,6 +10,7 @@ > #include <linux/fwnode_mdio.h> > #include <linux/of.h> > #include <linux/phy.h> > +#include <linux/phy_fixed.h> > #include <linux/pse-pd/pse.h> > > MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); > @@ -185,3 +186,98 @@ int fwnode_mdiobus_register_phy(struct mii_bus > *bus, > return rc; > } > EXPORT_SYMBOL(fwnode_mdiobus_register_phy); > + > +/* > + * fwnode_phy_is_fixed_link() and fwnode_phy_register_fixed_link() > must > + * support two bindings: > + * - the old binding, where 'fixed-link' was a property with 5 > + * cells encoding various information about the fixed PHY > + * - the new binding, where 'fixed-link' is a sub-node of the > + * Ethernet device. > + */ networking comment style > +bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode) > +{ > + struct fwnode_handle *fixed_link_node; > + const char *managed; > + > + /* New binding */ > + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed- > link"); > + if (fixed_link_node) { > + fwnode_handle_put(fixed_link_node); > + return true; > + } > + > + if (fwnode_property_read_string(fwnode, "managed", &managed) == > 0 && > + strcmp(managed, "auto") != 0) > + return true; > + > + /* Old binding */ > + return fwnode_property_count_u32(fwnode, "fixed-link") == 5; > +} > +EXPORT_SYMBOL(fwnode_phy_is_fixed_link); > + > +int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode) > +{ > + struct fixed_phy_status status = {}; > + struct fwnode_handle *fixed_link_node; Reverse christmas tree > + u32 fixed_link_prop[5]; > + const char *managed; > + int rc; > + > + if (fwnode_property_read_string(fwnode, "managed", &managed) == > 0 && > + strcmp(managed, "in-band-status") == 0) { > + /* status is zeroed, namely its .link member */ > + goto register_phy; > + } > + > + /* New binding */ > + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed- > link"); > + if (fixed_link_node) { > + status.link = 1; > + status.duplex = > fwnode_property_present(fixed_link_node, > + "full-duplex"); > + rc = fwnode_property_read_u32(fixed_link_node, "speed", > + &status.speed); > + if (rc) { > + fwnode_handle_put(fixed_link_node); > + return rc; > + } > + status.pause = fwnode_property_present(fixed_link_node, > "pause"); > + status.asym_pause = > fwnode_property_present(fixed_link_node, > + "asym- > pause"); > + fwnode_handle_put(fixed_link_node); > + > + goto register_phy; > + } > + > + /* Old binding */ > + rc = fwnode_property_read_u32_array(fwnode, "fixed-link", > fixed_link_prop, > + ARRAY_SIZE(fixed_link_prop) > ); > + if (rc) > + return rc; > + > + status.link = 1; > + status.duplex = fixed_link_prop[1]; > + status.speed = fixed_link_prop[2]; > + status.pause = fixed_link_prop[3]; > + status.asym_pause = fixed_link_prop[4]; > + > +register_phy: > + return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, > fwnode)); > +} > +EXPORT_SYMBOL(fwnode_phy_register_fixed_link); > + > >
Hi Andrew and Vladimir, pon., 16 sty 2023 o 23:04 Andrew Lunn <andrew@lunn.ch> napisał(a): > > On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote: > > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote: > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > > fixed-link PHYs API is used by DSA and a number of drivers > > > > and was depending on of_. Switch to fwnode_ so to make it > > > > hardware description agnostic and allow to be used in ACPI > > > > world as well. > > > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > > the more flexible fixed link implementation in phylink? > > > > Would it be even better if DSA had some driver-level prerequisites to > > impose for ACPI support - like phylink support rather than adjust_link - > > and we would simply branch off to a dsa_shared_port_link_register_acpi() > > function, leaving the current dsa_shared_port_link_register_of() alone, > > with all its workarounds and hacks? I don't believe that carrying all > > that logic over to a common fwnode based API is the proper way forward. In the past couple of years, a number of subsystems have migrated to a more generic HW description abstraction (e.g. a big chunk of network, pinctrl, gpio). ACPI aside, with this patchset one can even try to describe the switch topology with the swnode (I haven't tried that though). I fully agree that there should be no 0-day baggage in the DSA ACPI binding (FYI the more fwnode- version of the dsa_shared_port_validate_of() cought one issue in the WIP ACPI description in my setup). On the other hand, I find fwnode_/device_ APIs really helpful for most of the cases - ACPI/OF/swnode differences can be hidden to a generic layer and the need of maintaining separate code paths related to the hardware description on the driver/subsystem level is minimized. An example could be found in v1 of this series, the last 4 patches in [1] show that it can be done in a simple / seamless way, especially given the ACPI (fwnode) PHY description in phylink is already settled and widely used. I am aware at the end of the day, after final review all this can be more complex. I expect that the actual DSA ACPI support acceptance will require a lot of discussions and decisions, on whether certain solutions are worth migrating from OF world or require spec modification. For now my goal was to migrate to a more generic HW description API, and so to allow possible follow-up ACPI-related modifications, and additions to be extracted and better tracked. > > I agree with you there, here is little attempt to make a clean ACPI > binding. Most of the attempts to add ACPI support seem to try to take > the short cut for just search/replace of_ with fwnode_. And we then > have to push back and say no, and generally it then goes quiet. In most cases, the devices' description is pretty straightforward: * a node (single or with some children), resources (mem, irqs), mmio address space, optionally address on a bus and a couple of properties The DSDT/SSDT tables are very well suited for this. In case of separate, contained drivers that is also really easy to maintain. However, I fully understand your concerns and caution before blessing any change related to subsystem/generic code. Therefore ACPI support addition was split after v1 (refer to discussion in [1]) and will require ACPI maintainers' input and guidelines. > > Marcin, please approach this from the other end. Please document in > Documentation/firmware-guide/acpi/dsd what a clean binding should look > like, and then try to implement it. > This is how I initially approached this (original submission: [2]; a bit updated version, working on top of the current patchset: [3]). We then agreed that in order to remove a bit hacky mitigation of the double ACPI scan problem, an MDIOSerialBus _CRS method should be defined in the ACPI spec, similar to the I2CSerialBus/SPISerialBus/UARTSerialBus. I am going to submit the first version for review in the coming days. The DSA purely ACPI-related changes would be updated and submitted, once the method is accepted. Best regards, Marcin [1] https://www.spinics.net/lists/netdev/msg827337.html [2] https://www.spinics.net/lists/netdev/msg827345.html [3] https://github.com/semihalf-wojtas-marcin/Linux-Kernel/commit/e017e69c0eda18747029bfe0c335df204670ba59
On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote: > In the past couple of years, a number of subsystems have migrated to a > more generic HW description abstraction (e.g. a big chunk of network, > pinctrl, gpio). ACPI aside, with this patchset one can even try to > describe the switch topology with the swnode (I haven't tried that > though). I fully agree that there should be no 0-day baggage in the > DSA ACPI binding (FYI the more fwnode- version of the > dsa_shared_port_validate_of() cought one issue in the WIP ACPI > description in my setup). On the other hand, I find fwnode_/device_ > APIs really helpful for most of the cases - ACPI/OF/swnode differences > can be hidden to a generic layer and the need of maintaining separate > code paths related to the hardware description on the driver/subsystem > level is minimized. An example could be found in v1 of this series, > the last 4 patches in [1] show that it can be done in a simple / > seamless way, especially given the ACPI (fwnode) PHY description in > phylink is already settled and widely used. I am aware at the end of > the day, after final review all this can be more complex. > > I expect that the actual DSA ACPI support acceptance will require a > lot of discussions and decisions, on whether certain solutions are > worth migrating from OF world or require spec modification. For now my > goal was to migrate to a more generic HW description API, and so to > allow possible follow-up ACPI-related modifications, and additions to > be extracted and better tracked. I have a simple question. If you expect that the DSA ACPI bindings will require a lot of discussions, then how do you know that what you convert to fwnode now will be needed later, and why do you insist to mechanically convert everything to fwnode without having that discussion first? You see the lack of a need to maintain separate code paths between OF and ACPI as useful. Yet the DSA maintainers don't, and in some cases this is specifically what they want to avoid. So a mechanical conversion will end up making absolutely no progress.
Hi Marcin, On Tue, Jan 17, 2023 at 05:20:01PM +0100, Marcin Wojtas wrote: > Hi Russell, > > > pon., 16 sty 2023 o 18:50 Russell King (Oracle) <linux@armlinux.org.uk> > napisał(a): > > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > fixed-link PHYs API is used by DSA and a number of drivers > > > and was depending on of_. Switch to fwnode_ so to make it > > > hardware description agnostic and allow to be used in ACPI > > > world as well. > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > the more flexible fixed link implementation in phylink? > > > , > This patchset did not intend to introduce any functional change, simply > switch to a more generic HW description abstraction. Killing > of/fwnode_phy_(de)register_fixed_link entirely seems to be a challenge, as > there are a lot of users beyond the DSA. Otoh I see a value in having > of_/fwnode_phy_is_fixed_link check, afaik there is no equivalent in > phylink... Phylink provides a much improved implementation of fixed-link that is way more flexible than the phylib approach - it can implement speeds in excess of 1G. DSA already supports phylink with modern updated drivers that do not use the "adjust_link" implementation. What I'm proposing is that we don't bring the baggage of the phylib based fixed link forwards into fwnode, and leave this to be DT-only. I think this is what Andrew and Vladimir have also said. Thanks.
Hi, wt., 17 sty 2023 o 17:34 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote: > > In the past couple of years, a number of subsystems have migrated to a > > more generic HW description abstraction (e.g. a big chunk of network, > > pinctrl, gpio). ACPI aside, with this patchset one can even try to > > describe the switch topology with the swnode (I haven't tried that > > though). I fully agree that there should be no 0-day baggage in the > > DSA ACPI binding (FYI the more fwnode- version of the > > dsa_shared_port_validate_of() cought one issue in the WIP ACPI > > description in my setup). On the other hand, I find fwnode_/device_ > > APIs really helpful for most of the cases - ACPI/OF/swnode differences > > can be hidden to a generic layer and the need of maintaining separate > > code paths related to the hardware description on the driver/subsystem > > level is minimized. An example could be found in v1 of this series, > > the last 4 patches in [1] show that it can be done in a simple / > > seamless way, especially given the ACPI (fwnode) PHY description in > > phylink is already settled and widely used. I am aware at the end of > > the day, after final review all this can be more complex. > > > > I expect that the actual DSA ACPI support acceptance will require a > > lot of discussions and decisions, on whether certain solutions are > > worth migrating from OF world or require spec modification. For now my > > goal was to migrate to a more generic HW description API, and so to > > allow possible follow-up ACPI-related modifications, and additions to > > be extracted and better tracked. > > I have a simple question. > > If you expect that the DSA ACPI bindings will require a lot of > discussions, then how do you know that what you convert to fwnode now > will be needed later, and why do you insist to mechanically convert > everything to fwnode without having that discussion first? > Ok, let me clarify. From the technical standpoint, I think it is fairly easy and to a very big extent, we should be able to reuse, what is already existing - I made it work with a really minimal set of changes, using a standard nodes' hierarchy and generic methods in the ACPI tables. As more difficult, I consider getting this solution accepted by the ACPI and the network subsystem maintainers, also given the OF quirks/legacy stuff, that apparently needs to be ruled out in such circumstances. However, I perceive a preparation step, with migrating to the more generic HW description API in the generic net/dsa, as a sort of improvement, but I get your point and I will wait with resubmitting these changes again. > You see the lack of a need to maintain separate code paths between OF > and ACPI as useful. Yet the DSA maintainers don't, and in some cases > this is specifically what they want to avoid. So a mechanical conversion > will end up making absolutely no progress. Fair enough. I'll keep it on hold until MDIOSerialBus gets accepted and repost a bigger, combined patchset with all changes like in the v1. Best regards, Marcin
wt., 17 sty 2023 o 18:58 Russell King (Oracle) <linux@armlinux.org.uk> napisał(a): > > Hi Marcin, > > On Tue, Jan 17, 2023 at 05:20:01PM +0100, Marcin Wojtas wrote: > > Hi Russell, > > > > > > pon., 16 sty 2023 o 18:50 Russell King (Oracle) <linux@armlinux.org.uk> > > napisał(a): > > > > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > > fixed-link PHYs API is used by DSA and a number of drivers > > > > and was depending on of_. Switch to fwnode_ so to make it > > > > hardware description agnostic and allow to be used in ACPI > > > > world as well. > > > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > > the more flexible fixed link implementation in phylink? > > > > > , > > This patchset did not intend to introduce any functional change, simply > > switch to a more generic HW description abstraction. Killing > > of/fwnode_phy_(de)register_fixed_link entirely seems to be a challenge, as > > there are a lot of users beyond the DSA. Otoh I see a value in having > > of_/fwnode_phy_is_fixed_link check, afaik there is no equivalent in > > phylink... > > Phylink provides a much improved implementation of fixed-link that is > way more flexible than the phylib approach - it can implement speeds > in excess of 1G. DSA already supports phylink with modern updated > drivers that do not use the "adjust_link" implementation. > > What I'm proposing is that we don't bring the baggage of the phylib > based fixed link forwards into fwnode, and leave this to be DT-only. > I think this is what Andrew and Vladimir have also said. > Ok, thanks for clarifying. Best regards, Marcin
On Tue, Jan 17, 2023 at 05:05:53PM +0100, Marcin Wojtas wrote: > Hi Andrew and Vladimir, > > pon., 16 sty 2023 o 23:04 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > On Mon, Jan 16, 2023 at 08:16:18PM +0200, Vladimir Oltean wrote: > > > On Mon, Jan 16, 2023 at 05:50:13PM +0000, Russell King (Oracle) wrote: > > > > On Mon, Jan 16, 2023 at 06:34:14PM +0100, Marcin Wojtas wrote: > > > > > fixed-link PHYs API is used by DSA and a number of drivers > > > > > and was depending on of_. Switch to fwnode_ so to make it > > > > > hardware description agnostic and allow to be used in ACPI > > > > > world as well. > > > > > > > > Would it be better to let the fixed-link PHY die, and have everyone use > > > > the more flexible fixed link implementation in phylink? > > > > > > Would it be even better if DSA had some driver-level prerequisites to > > > impose for ACPI support - like phylink support rather than adjust_link - > > > and we would simply branch off to a dsa_shared_port_link_register_acpi() > > > function, leaving the current dsa_shared_port_link_register_of() alone, > > > with all its workarounds and hacks? I don't believe that carrying all > > > that logic over to a common fwnode based API is the proper way forward. > > In the past couple of years, a number of subsystems have migrated to a > more generic HW description abstraction (e.g. a big chunk of network, > pinctrl, gpio). ACPI aside, with this patchset one can even try to > describe the switch topology with the swnode (I haven't tried that > though). I fully agree that there should be no 0-day baggage in the > DSA ACPI binding (FYI the more fwnode- version of the > dsa_shared_port_validate_of() cought one issue in the WIP ACPI > description in my setup). On the other hand, I find fwnode_/device_ > APIs really helpful for most of the cases - ACPI/OF/swnode differences > can be hidden to a generic layer and the need of maintaining separate > code paths related to the hardware description on the driver/subsystem > level is minimized. It looks like we are heading towards three different descriptions. OF, ACPI and swnode. Each is likely to be different. OF has a lot of history in it, deprecated things etc, which should not appear in the others. So i see a big ugly block of code for the OF binding, and hopefully clean and tidy code for ACPI binding and a clean and tidy bit of code for swmode., It would be nice if the results of that parsing could be presented to the drivers in a uniform way, so the driver itself does not need to care where the information came from. But to me it is clear that this uniform layer has no direct access to the databases, since the database as are different. Andrew
diff --git a/include/linux/fwnode_mdio.h b/include/linux/fwnode_mdio.h index faf603c48c86..98755b8c6c8a 100644 --- a/include/linux/fwnode_mdio.h +++ b/include/linux/fwnode_mdio.h @@ -16,6 +16,11 @@ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, int fwnode_mdiobus_register_phy(struct mii_bus *bus, struct fwnode_handle *child, u32 addr); +int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode); + +void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode); + +bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode); #else /* CONFIG_FWNODE_MDIO */ int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy, @@ -30,6 +35,20 @@ static inline int fwnode_mdiobus_register_phy(struct mii_bus *bus, { return -EINVAL; } + +static inline int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode) +{ + return -ENODEV; +} + +static inline void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode) +{ +} + +static inline bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode) +{ + return false; +} #endif #endif /* __LINUX_FWNODE_MDIO_H */ diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c index b782c35c4ac1..56f57381ae69 100644 --- a/drivers/net/mdio/fwnode_mdio.c +++ b/drivers/net/mdio/fwnode_mdio.c @@ -10,6 +10,7 @@ #include <linux/fwnode_mdio.h> #include <linux/of.h> #include <linux/phy.h> +#include <linux/phy_fixed.h> #include <linux/pse-pd/pse.h> MODULE_AUTHOR("Calvin Johnson <calvin.johnson@oss.nxp.com>"); @@ -185,3 +186,98 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus, return rc; } EXPORT_SYMBOL(fwnode_mdiobus_register_phy); + +/* + * fwnode_phy_is_fixed_link() and fwnode_phy_register_fixed_link() must + * support two bindings: + * - the old binding, where 'fixed-link' was a property with 5 + * cells encoding various information about the fixed PHY + * - the new binding, where 'fixed-link' is a sub-node of the + * Ethernet device. + */ +bool fwnode_phy_is_fixed_link(struct fwnode_handle *fwnode) +{ + struct fwnode_handle *fixed_link_node; + const char *managed; + + /* New binding */ + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link"); + if (fixed_link_node) { + fwnode_handle_put(fixed_link_node); + return true; + } + + if (fwnode_property_read_string(fwnode, "managed", &managed) == 0 && + strcmp(managed, "auto") != 0) + return true; + + /* Old binding */ + return fwnode_property_count_u32(fwnode, "fixed-link") == 5; +} +EXPORT_SYMBOL(fwnode_phy_is_fixed_link); + +int fwnode_phy_register_fixed_link(struct fwnode_handle *fwnode) +{ + struct fixed_phy_status status = {}; + struct fwnode_handle *fixed_link_node; + u32 fixed_link_prop[5]; + const char *managed; + int rc; + + if (fwnode_property_read_string(fwnode, "managed", &managed) == 0 && + strcmp(managed, "in-band-status") == 0) { + /* status is zeroed, namely its .link member */ + goto register_phy; + } + + /* New binding */ + fixed_link_node = fwnode_get_named_child_node(fwnode, "fixed-link"); + if (fixed_link_node) { + status.link = 1; + status.duplex = fwnode_property_present(fixed_link_node, + "full-duplex"); + rc = fwnode_property_read_u32(fixed_link_node, "speed", + &status.speed); + if (rc) { + fwnode_handle_put(fixed_link_node); + return rc; + } + status.pause = fwnode_property_present(fixed_link_node, "pause"); + status.asym_pause = fwnode_property_present(fixed_link_node, + "asym-pause"); + fwnode_handle_put(fixed_link_node); + + goto register_phy; + } + + /* Old binding */ + rc = fwnode_property_read_u32_array(fwnode, "fixed-link", fixed_link_prop, + ARRAY_SIZE(fixed_link_prop)); + if (rc) + return rc; + + status.link = 1; + status.duplex = fixed_link_prop[1]; + status.speed = fixed_link_prop[2]; + status.pause = fixed_link_prop[3]; + status.asym_pause = fixed_link_prop[4]; + +register_phy: + return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, fwnode)); +} +EXPORT_SYMBOL(fwnode_phy_register_fixed_link); + +void fwnode_phy_deregister_fixed_link(struct fwnode_handle *fwnode) +{ + struct phy_device *phydev; + + phydev = fwnode_phy_find_device(fwnode); + if (!phydev) + return; + + fixed_phy_unregister(phydev); + + put_device(&phydev->mdio.dev); /* fwnode_phy_find_device() */ + phy_device_free(phydev); /* fixed_phy_register() */ +} +EXPORT_SYMBOL(fwnode_phy_deregister_fixed_link); diff --git a/drivers/net/mdio/of_mdio.c b/drivers/net/mdio/of_mdio.c index ba22b7110cdc..e6b3a4e251a1 100644 --- a/drivers/net/mdio/of_mdio.c +++ b/drivers/net/mdio/of_mdio.c @@ -353,91 +353,18 @@ EXPORT_SYMBOL(of_phy_get_and_connect); */ bool of_phy_is_fixed_link(struct device_node *np) { - struct device_node *dn; - int len, err; - const char *managed; - - /* New binding */ - dn = of_get_child_by_name(np, "fixed-link"); - if (dn) { - of_node_put(dn); - return true; - } - - err = of_property_read_string(np, "managed", &managed); - if (err == 0 && strcmp(managed, "auto") != 0) - return true; - - /* Old binding */ - if (of_get_property(np, "fixed-link", &len) && - len == (5 * sizeof(__be32))) - return true; - - return false; + return fwnode_phy_is_fixed_link(of_fwnode_handle(np)); } EXPORT_SYMBOL(of_phy_is_fixed_link); int of_phy_register_fixed_link(struct device_node *np) { - struct fixed_phy_status status = {}; - struct device_node *fixed_link_node; - u32 fixed_link_prop[5]; - const char *managed; - - if (of_property_read_string(np, "managed", &managed) == 0 && - strcmp(managed, "in-band-status") == 0) { - /* status is zeroed, namely its .link member */ - goto register_phy; - } - - /* New binding */ - fixed_link_node = of_get_child_by_name(np, "fixed-link"); - if (fixed_link_node) { - status.link = 1; - status.duplex = of_property_read_bool(fixed_link_node, - "full-duplex"); - if (of_property_read_u32(fixed_link_node, "speed", - &status.speed)) { - of_node_put(fixed_link_node); - return -EINVAL; - } - status.pause = of_property_read_bool(fixed_link_node, "pause"); - status.asym_pause = of_property_read_bool(fixed_link_node, - "asym-pause"); - of_node_put(fixed_link_node); - - goto register_phy; - } - - /* Old binding */ - if (of_property_read_u32_array(np, "fixed-link", fixed_link_prop, - ARRAY_SIZE(fixed_link_prop)) == 0) { - status.link = 1; - status.duplex = fixed_link_prop[1]; - status.speed = fixed_link_prop[2]; - status.pause = fixed_link_prop[3]; - status.asym_pause = fixed_link_prop[4]; - goto register_phy; - } - - return -ENODEV; - -register_phy: - return PTR_ERR_OR_ZERO(fixed_phy_register(PHY_POLL, &status, of_fwnode_handle(np))); + return fwnode_phy_register_fixed_link(of_fwnode_handle(np)); } EXPORT_SYMBOL(of_phy_register_fixed_link); void of_phy_deregister_fixed_link(struct device_node *np) { - struct phy_device *phydev; - - phydev = of_phy_find_device(np); - if (!phydev) - return; - - fixed_phy_unregister(phydev); - - put_device(&phydev->mdio.dev); /* of_phy_find_device() */ - phy_device_free(phydev); /* fixed_phy_register() */ + fwnode_phy_deregister_fixed_link(of_fwnode_handle(np)); } EXPORT_SYMBOL(of_phy_deregister_fixed_link);
fixed-link PHYs API is used by DSA and a number of drivers and was depending on of_. Switch to fwnode_ so to make it hardware description agnostic and allow to be used in ACPI world as well. Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- include/linux/fwnode_mdio.h | 19 ++++ drivers/net/mdio/fwnode_mdio.c | 96 ++++++++++++++++++++ drivers/net/mdio/of_mdio.c | 79 +--------------- 3 files changed, 118 insertions(+), 76 deletions(-)