| Message ID | 20190523011958.14944-9-ioana.ciornei@nxp.com |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Decoupling PHYLINK from struct net_device | expand |
On 5/22/2019 6:20 PM, Ioana Ciornei wrote: > This completely removes the usage of PHYLIB from DSA, namely for the > aforementioned switch ports which used to drive a software PHY manually > using genphy operations. > > For these ports, the newly introduced phylink_create_raw API must be > used, and the callbacks are received through a notifier block registered > per dsa_port, but otherwise the implementation is fairly > straightforward, and the handling of the regular vs raw PHYLINK > instances is common from the perspective of the driver. > > What changes for drivers: > > The .adjust_link callback is no longer called for the fixed-link CPU/DSA > ports and drivers must migrate to standard PHYLINK operations (e.g. > .phylink_mac_config). The reason why we can't do anything for them is > because PHYLINK does not wrap the fixed link state behind a phydev > object, so we cannot wrap .phylink_mac_config into .adjust_link unless > we fabricate a phy_device structure. Can't we offer a slightly nicer transition period for DSA switch drivers: - if adjust_link and phylink_mac_ops are both supported, prefer phylink_mac_ops - if phylink_mac_ops is defined alone use it, we're good - if adjust_link alone is defined, keep using it with the existing code but print a warning inviting to migrate? The changes look fine but the transition path needs to be a little more gentle IMHO.
On Thu, 23 May 2019 at 05:17, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 5/22/2019 6:20 PM, Ioana Ciornei wrote: > > This completely removes the usage of PHYLIB from DSA, namely for the > > aforementioned switch ports which used to drive a software PHY manually > > using genphy operations. > > > > For these ports, the newly introduced phylink_create_raw API must be > > used, and the callbacks are received through a notifier block registered > > per dsa_port, but otherwise the implementation is fairly > > straightforward, and the handling of the regular vs raw PHYLINK > > instances is common from the perspective of the driver. > > > > What changes for drivers: > > > > The .adjust_link callback is no longer called for the fixed-link CPU/DSA > > ports and drivers must migrate to standard PHYLINK operations (e.g. > > .phylink_mac_config). The reason why we can't do anything for them is > > because PHYLINK does not wrap the fixed link state behind a phydev > > object, so we cannot wrap .phylink_mac_config into .adjust_link unless > > we fabricate a phy_device structure. > > Can't we offer a slightly nicer transition period for DSA switch drivers: > > - if adjust_link and phylink_mac_ops are both supported, prefer > phylink_mac_ops > - if phylink_mac_ops is defined alone use it, we're good > - if adjust_link alone is defined, keep using it with the existing code > but print a warning inviting to migrate? > > The changes look fine but the transition path needs to be a little more > gentle IMHO. > -- > Florian Hi Florian, Yes we could, but since most of the adjust_link -> phylink_mac_ops changes appear trivial, and we have the knowledge behind b53 right here, can't we just migrate everything in the next patchset and remove adjust_link altogether from DSA? ...So why does b53 configure the SGMII SerDes in phylink_mac_config, and RGMII delays for the CPU port in adjust_link? Why are pause frames only configured for the CPU port? A migration from my side would be rather mechanical, so could you please share some suggestions? -Vladimir
> Hi Florian, > > Yes we could, but since most of the adjust_link -> phylink_mac_ops > changes appear trivial, and we have the knowledge behind b53 right > here, can't we just migrate everything in the next patchset and remove > adjust_link altogether from DSA? I agree with Florian, we either need to support both, or their needs to be another patchset which comes first and converts all DSA drivers to PHYLINK. And it is this conversion patchset which is likely to break things, so it would be good to sit in net-next for a week or two to allow testing, before the second patchset is applied. Andrew
On Fri, 24 May 2019 at 16:19, Andrew Lunn <andrew@lunn.ch> wrote: > > > Hi Florian, > > > > Yes we could, but since most of the adjust_link -> phylink_mac_ops > > changes appear trivial, and we have the knowledge behind b53 right > > here, can't we just migrate everything in the next patchset and remove > > adjust_link altogether from DSA? > > I agree with Florian, we either need to support both, or their needs > to be another patchset which comes first and converts all DSA drivers > to PHYLINK. And it is this conversion patchset which is likely to > break things, so it would be good to sit in net-next for a week or two > to allow testing, before the second patchset is applied. > > Andrew Hi Andrew, I think that converting drivers to PHYLINK in a separate patchset is going to introduce useless work, since the complete migration to PHYLINK is necessarily going to take 2 steps. As of now, the CPU and DSA ports still use the PHYLIB adjust_link callback exclusively. So let's see what the drivers need to do in adjust_link now and how to map that over phylink_mac_config, and in v2 we can just remove the adjust_link wrappers completely from DSA. -Vladimir
diff --git a/include/net/dsa.h b/include/net/dsa.h index 685294817712..87616ff00919 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -212,6 +212,9 @@ struct dsa_port { * Original copy of the master netdev net_device_ops */ const struct net_device_ops *orig_ndo_ops; + + /* Listener for phylink events on ports with no netdev */ + struct notifier_block nb; }; struct dsa_switch { diff --git a/net/dsa/port.c b/net/dsa/port.c index d0f955e8b731..31bd07dd42db 100644 --- a/net/dsa/port.c +++ b/net/dsa/port.c @@ -13,6 +13,7 @@ #include <linux/if_bridge.h> #include <linux/notifier.h> #include <linux/of_mdio.h> +#include <linux/phylink.h> #include <linux/of_net.h> #include "dsa_priv.h" @@ -511,104 +512,88 @@ void dsa_port_phylink_fixed_state(struct dsa_port *dp, { struct dsa_switch *ds = dp->ds; - /* No need to check that this operation is valid, the callback would - * not be called if it was not. + /* We need to check that the callback exists because phylink raw will + * send PHYLINK_GET_FIXED_STATE events without an explicit register. */ - ds->ops->phylink_fixed_state(ds, dp->index, state); + if (ds->ops->phylink_fixed_state) + ds->ops->phylink_fixed_state(ds, dp->index, state); } EXPORT_SYMBOL(dsa_port_phylink_fixed_state); -static int dsa_port_setup_phy_of(struct dsa_port *dp, bool enable) -{ - struct dsa_switch *ds = dp->ds; - struct phy_device *phydev; - int port = dp->index; - int err = 0; - - phydev = dsa_port_get_phy_device(dp); - if (!phydev) - return 0; - - if (IS_ERR(phydev)) - return PTR_ERR(phydev); - - if (enable) { - err = genphy_config_init(phydev); - if (err < 0) - goto err_put_dev; - - err = genphy_resume(phydev); - if (err < 0) - goto err_put_dev; - - err = genphy_read_status(phydev); - if (err < 0) - goto err_put_dev; - } else { - err = genphy_suspend(phydev); - if (err < 0) - goto err_put_dev; +static int dsa_cpu_port_event(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct phylink_notifier_info *info = ptr; + struct dsa_port *dp = container_of(nb, struct dsa_port, nb); + + switch (event) { + case PHYLINK_VALIDATE: + dsa_port_phylink_validate(dp, info->supported, info->state); + break; + case PHYLINK_MAC_AN_RESTART: + dsa_port_phylink_mac_an_restart(dp); + break; + case PHYLINK_MAC_CONFIG: + dsa_port_phylink_mac_config(dp, info->link_an_mode, + info->state); + break; + case PHYLINK_MAC_LINK_DOWN: + dsa_port_phylink_mac_link_down(dp, info->link_an_mode, + info->interface, info->phydev); + break; + case PHYLINK_MAC_LINK_UP: + dsa_port_phylink_mac_link_up(dp, info->link_an_mode, + info->interface, info->phydev); + break; + case PHYLINK_GET_FIXED_STATE: + dsa_port_phylink_fixed_state(dp, info->state); + break; + default: + return NOTIFY_OK; } - if (ds->ops->adjust_link) - ds->ops->adjust_link(ds, port, phydev); - - dev_dbg(ds->dev, "enabled port's phy: %s", phydev_name(phydev)); - -err_put_dev: - put_device(&phydev->mdio.dev); - return err; + return NOTIFY_DONE; } -static int dsa_port_fixed_link_register_of(struct dsa_port *dp) +int dsa_port_link_register_of(struct dsa_port *dp) { - struct device_node *dn = dp->dn; - struct dsa_switch *ds = dp->ds; - struct phy_device *phydev; - int port = dp->index; - int mode; - int err; + struct device_node *port_dn = dp->dn; + int mode, err; - err = of_phy_register_fixed_link(dn); - if (err) { - dev_err(ds->dev, - "failed to register the fixed PHY of port %d\n", - port); - return err; - } - - phydev = of_phy_find_device(dn); - - mode = of_get_phy_mode(dn); + mode = of_get_phy_mode(port_dn); if (mode < 0) mode = PHY_INTERFACE_MODE_NA; - phydev->interface = mode; - genphy_config_init(phydev); - genphy_read_status(phydev); + dp->nb.notifier_call = dsa_cpu_port_event; + dp->pl = phylink_create_raw(&dp->nb, of_fwnode_handle(port_dn), mode); + if (IS_ERR(dp->pl)) { + pr_err("error creating PHYLINK: %ld\n", PTR_ERR(dp->pl)); + return PTR_ERR(dp->pl); + } - if (ds->ops->adjust_link) - ds->ops->adjust_link(ds, port, phydev); + err = phylink_of_phy_connect(dp->pl, port_dn, 0); + if (err) { + pr_err("could not attach to PHY: %d\n", err); + goto err_phy_connect; + } - put_device(&phydev->mdio.dev); + rtnl_lock(); + phylink_start(dp->pl); + rtnl_unlock(); return 0; -} -int dsa_port_link_register_of(struct dsa_port *dp) -{ - if (of_phy_is_fixed_link(dp->dn)) - return dsa_port_fixed_link_register_of(dp); - else - return dsa_port_setup_phy_of(dp, true); +err_phy_connect: + phylink_destroy(dp->pl); + return err; } void dsa_port_link_unregister_of(struct dsa_port *dp) { - if (of_phy_is_fixed_link(dp->dn)) - of_phy_deregister_fixed_link(dp->dn); - else - dsa_port_setup_phy_of(dp, false); + rtnl_lock(); + phylink_disconnect_phy(dp->pl); + rtnl_unlock(); + phylink_destroy(dp->pl); } int dsa_port_get_phy_strings(struct dsa_port *dp, uint8_t *data)
This completely removes the usage of PHYLIB from DSA, namely for the aforementioned switch ports which used to drive a software PHY manually using genphy operations. For these ports, the newly introduced phylink_create_raw API must be used, and the callbacks are received through a notifier block registered per dsa_port, but otherwise the implementation is fairly straightforward, and the handling of the regular vs raw PHYLINK instances is common from the perspective of the driver. What changes for drivers: The .adjust_link callback is no longer called for the fixed-link CPU/DSA ports and drivers must migrate to standard PHYLINK operations (e.g. .phylink_mac_config). The reason why we can't do anything for them is because PHYLINK does not wrap the fixed link state behind a phydev object, so we cannot wrap .phylink_mac_config into .adjust_link unless we fabricate a phy_device structure. Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com> --- include/net/dsa.h | 3 + net/dsa/port.c | 137 +++++++++++++++++++++------------------------- 2 files changed, 64 insertions(+), 76 deletions(-)