| Message ID | 20230923134904.3627402-16-vladimir.oltean@nxp.com |
|---|---|
| State | RFC |
| Headers | show |
| Series | Add C72/C73 copper backplane support for LX2160 | expand |
On Sat, Sep 23, 2023 at 04:49:04PM +0300, Vladimir Oltean wrote: > +static int lynx_pcs_parse_fwnode(struct lynx_pcs *lynx) > +{ > + struct fwnode_handle *node = lynx->mdio->dev.fwnode; > + enum mtip_model model = MTIP_MODEL_AUTODETECT; > + struct device_node *np = to_of_node(node); > + struct mdio_device *mdio = lynx->mdio; > + struct device *dev = &mdio->dev; > + struct phy *phy; > + int i, err; > + > + if (!node) > + return 0; > + > + lynx->backplane_mode = fwnode_property_present(node, "fsl,backplane-mode"); > + if (!lynx->backplane_mode) > + return 0; > + > + if (fwnode_device_is_compatible(node, "fsl,lx2160a-lynx-pcs")) > + model = MTIP_MODEL_LX2160A; > + > + lynx->num_lanes = of_count_phandle_with_args(np, "phys", "#phy-cells"); > + if (lynx->num_lanes < 0) > + return lynx->num_lanes; Is it possible for ->num_lanes to be zero at this point? If that is possible, then ->anlt[PRIMARY_LANE] will be NULL but ->backplane_mode will be set, so won't that cause the mtip_* calls above to pass a NULL pointer into those functions? Is that safe? Should we trap that case here? If that's correct, then I don't see any point in storing ->backplane_mode, since we can then use ->num_lanes > PRIMARY_LANE or similar instead. > + > + if (WARN_ON(lynx->num_lanes > MAX_NUM_LANES)) > + return -EINVAL; Do we need to use WARN_ON() here, or would it be better to print a short error-level message? > + > + for (i = 0; i < lynx->num_lanes; i++) { > + phy = devm_of_phy_get_by_index(dev, np, i); > + if (IS_ERR(phy)) > + return dev_err_probe(dev, PTR_ERR(phy), > + "Failed to get SerDes PHY %d\n", i); > + > + lynx->anlt[i] = mtip_backplane_create(mdio, phy, model); > + if (IS_ERR(lynx->anlt[i])) { > + err = PTR_ERR(lynx->anlt[i]); > + > + while (i-- > 0) > + mtip_backplane_destroy(lynx->anlt[i]); > + > + return err; > + } > + } > + > + for (i = 1; i < lynx->num_lanes; i++) { > + err = mtip_backplane_add_subordinate(lynx->anlt[PRIMARY_LANE], > + lynx->anlt[i]); > + if (WARN_ON(err)) { Again, does this need to be a backtrace-producing WARN_ON()? > + /* Too many SerDes lanes in the device tree? */ > + for (i = 0; i < lynx->num_lanes; i++) > + mtip_backplane_destroy(lynx->anlt[i]); > + return err; > + } > + } > + > + return 0; > +} > + > static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) > { > struct lynx_pcs *lynx; > + int err; > > lynx = kzalloc(sizeof(*lynx), GFP_KERNEL); > if (!lynx) > @@ -327,6 +451,12 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) > lynx->pcs.neg_mode = true; > lynx->pcs.poll = true; > > + err = lynx_pcs_parse_fwnode(lynx); > + if (err) { > + kfree(lynx); > + return ERR_PTR(err); > + } > + > return lynx_to_phylink_pcs(lynx); > } > > @@ -392,6 +522,11 @@ EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); > void lynx_pcs_destroy(struct phylink_pcs *pcs) > { > struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); > + int i; > + > + if (lynx->backplane_mode) > + for (i = 0; i < lynx->num_lanes; i++) > + mtip_backplane_destroy(lynx->anlt[i]); Won't ->num_lanes only be non-zero when ->backplane_mode is set, so isn't the test for ->backplane_mode redundant here? > > mdio_device_put(lynx->mdio); > kfree(lynx); > -- > 2.34.1 > >
Hi Russell, On Tue, Oct 03, 2023 at 02:14:41PM +0100, Russell King (Oracle) wrote: > On Sat, Sep 23, 2023 at 04:49:04PM +0300, Vladimir Oltean wrote: > > +static int lynx_pcs_parse_fwnode(struct lynx_pcs *lynx) > > +{ > > + struct fwnode_handle *node = lynx->mdio->dev.fwnode; > > + enum mtip_model model = MTIP_MODEL_AUTODETECT; > > + struct device_node *np = to_of_node(node); > > + struct mdio_device *mdio = lynx->mdio; > > + struct device *dev = &mdio->dev; > > + struct phy *phy; > > + int i, err; > > + > > + if (!node) > > + return 0; > > + > > + lynx->backplane_mode = fwnode_property_present(node, "fsl,backplane-mode"); > > + if (!lynx->backplane_mode) > > + return 0; > > + > > + if (fwnode_device_is_compatible(node, "fsl,lx2160a-lynx-pcs")) > > + model = MTIP_MODEL_LX2160A; > > + > > + lynx->num_lanes = of_count_phandle_with_args(np, "phys", "#phy-cells"); > > + if (lynx->num_lanes < 0) > > + return lynx->num_lanes; > > Is it possible for ->num_lanes to be zero at this point? If that is > possible, then ->anlt[PRIMARY_LANE] will be NULL but ->backplane_mode > will be set, so won't that cause the mtip_* calls above to pass a > NULL pointer into those functions? Is that safe? Should we trap that > case here? Assuming the dt-bindings as proposed here, that case would be an invalid device tree ("fsl,backplane-mode" present but "phys" isn't present), which I indeed failed to catch. But in my reply to Krzysztof here: https://lore.kernel.org/netdev/20231002121958.xybzovgjzldfiae2@skbuf/ I said that for v3, I'm looking to move the "phys" property from the PCS to the MAC (it's already in the MAC for the non-backplane use cases). On LS1028A (ENETC, Felix), the lynx-pcs is not OF-based (we use lynx_pcs_create_mdiodev()), but it would be good to support the 1000Base-KX link mode there also. As I'm starting to look beyond LX2160A, I'm starting to see why adding extra dt-bindings to the lynx-pcs (both "phys" and "fsl,backplane-mode") will be problematic if there is no OF node to speak of. I will leave a separate comment with some new ideas. > If that's correct, then I don't see any point in storing > ->backplane_mode, since we can then use ->num_lanes > PRIMARY_LANE > or similar instead. Well, in v3, my plan is for the caller of lynx_pcs_create() (aka the MAC) to always pass an array of phys (the ones from its own OF node). In that case, we would indeed need the "fsl,backplane-mode" property in the PCS, because otherwise, with your proposal, the PCS would instantiate the AN/LT block even when it's not expected. > > + > > + if (WARN_ON(lynx->num_lanes > MAX_NUM_LANES)) > > + return -EINVAL; > > Do we need to use WARN_ON() here, or would it be better to print a short > error-level message? Admittedly I may not have the best intuition here, but I didn't want to over-complicate the code with error messages that can only be triggered with invalid device trees. > > + > > + for (i = 0; i < lynx->num_lanes; i++) { > > + phy = devm_of_phy_get_by_index(dev, np, i); > > + if (IS_ERR(phy)) > > + return dev_err_probe(dev, PTR_ERR(phy), > > + "Failed to get SerDes PHY %d\n", i); > > + > > + lynx->anlt[i] = mtip_backplane_create(mdio, phy, model); > > + if (IS_ERR(lynx->anlt[i])) { > > + err = PTR_ERR(lynx->anlt[i]); > > + > > + while (i-- > 0) > > + mtip_backplane_destroy(lynx->anlt[i]); > > + > > + return err; > > + } > > + } > > + > > + for (i = 1; i < lynx->num_lanes; i++) { > > + err = mtip_backplane_add_subordinate(lynx->anlt[PRIMARY_LANE], > > + lynx->anlt[i]); > > + if (WARN_ON(err)) { > > Again, does this need to be a backtrace-producing WARN_ON()? mtip_backplane_add_subordinate() will only return -ERANGE if called too many times (more than MTIP_MAX_NUM_SUBORDINATES times, aka more than "MAX_NUM_LANES - 1" times). Given the way that the code is constructed, it is technically impossible for that to happen, but only because MTIP_MAX_NUM_SUBORDINATES is hand-crafted to be 3 and MAX_NUM_LANES to be 4. I think that if I define MTIP_MAX_NUM_SUBORDINATES in terms of MAX_NUM_LANES - 1, I can simply make mtip_backplane_add_subordinate() return void. What I want to avoid is to add error handling for errors which cannot take place. Which is where the WARN_ON() came from. > > + /* Too many SerDes lanes in the device tree? */ > > + for (i = 0; i < lynx->num_lanes; i++) > > + mtip_backplane_destroy(lynx->anlt[i]); > > + return err; > > + } > > + } > > + > > + return 0; > > +} > > + > > static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) > > { > > struct lynx_pcs *lynx; > > + int err; > > > > lynx = kzalloc(sizeof(*lynx), GFP_KERNEL); > > if (!lynx) > > @@ -327,6 +451,12 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) > > lynx->pcs.neg_mode = true; > > lynx->pcs.poll = true; > > > > + err = lynx_pcs_parse_fwnode(lynx); > > + if (err) { > > + kfree(lynx); > > + return ERR_PTR(err); > > + } > > + > > return lynx_to_phylink_pcs(lynx); > > } > > > > @@ -392,6 +522,11 @@ EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); > > void lynx_pcs_destroy(struct phylink_pcs *pcs) > > { > > struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); > > + int i; > > + > > + if (lynx->backplane_mode) > > + for (i = 0; i < lynx->num_lanes; i++) > > + mtip_backplane_destroy(lynx->anlt[i]); > > Won't ->num_lanes only be non-zero when ->backplane_mode is set, so > isn't the test for ->backplane_mode redundant here? I think it won't be redundant anymore once the series reaches a less "WIP" state. > > > > mdio_device_put(lynx->mdio); > > kfree(lynx); > > -- > > 2.34.1 > > > > > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig index 24a033e93bdd..be561c465b4a 100644 --- a/drivers/net/pcs/Kconfig +++ b/drivers/net/pcs/Kconfig @@ -13,6 +13,7 @@ config MTIP_BACKPLANE_PHY SoCs. config PCS_XPCS + depends on MTIP_BACKPLANE_PHY || MTIP_BACKPLANE_PHY=n tristate select PHYLINK help diff --git a/drivers/net/pcs/pcs-lynx.c b/drivers/net/pcs/pcs-lynx.c index dc3962b2aa6b..1352f08edcf3 100644 --- a/drivers/net/pcs/pcs-lynx.c +++ b/drivers/net/pcs/pcs-lynx.c @@ -4,10 +4,14 @@ */ #include <linux/mdio.h> +#include <linux/of.h> +#include <linux/phy/phy.h> #include <linux/phylink.h> #include <linux/pcs-lynx.h> #include <linux/property.h> +#include "mtip_backplane.h" + #define SGMII_CLOCK_PERIOD_NS 8 /* PCS is clocked at 125 MHz */ #define LINK_TIMER_VAL(ns) ((u32)((ns) / SGMII_CLOCK_PERIOD_NS)) @@ -20,9 +24,15 @@ #define IF_MODE_SPEED_MSK GENMASK(3, 2) #define IF_MODE_HALF_DUPLEX BIT(4) +#define PRIMARY_LANE 0 +#define MAX_NUM_LANES 4 + struct lynx_pcs { struct phylink_pcs pcs; struct mdio_device *mdio; + struct mtip_backplane *anlt[MAX_NUM_LANES]; + int num_lanes; + bool backplane_mode; }; enum sgmii_speed { @@ -100,6 +110,9 @@ static void lynx_pcs_get_state(struct phylink_pcs *pcs, case PHY_INTERFACE_MODE_10GBASER: phylink_mii_c45_pcs_get_state(lynx->mdio, state); break; + case PHY_INTERFACE_MODE_INTERNAL: + mtip_backplane_get_state(lynx->anlt[PRIMARY_LANE], state); + break; default: break; } @@ -168,6 +181,17 @@ static int lynx_pcs_config_usxgmii(struct mdio_device *pcs, ADVERTISE_SGMII | ADVERTISE_LPACK); } +static int lynx_pcs_config_backplane(struct phylink_pcs *pcs, + unsigned int neg_mode, + const unsigned long *advertising) +{ + bool autoneg = neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED; + struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + + return mtip_backplane_config_aneg(lynx->anlt[PRIMARY_LANE], autoneg, + advertising); +} + static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, phy_interface_t ifmode, const unsigned long *advertising, bool permit) @@ -193,6 +217,8 @@ static int lynx_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode, case PHY_INTERFACE_MODE_10GBASER: /* Nothing to do here for 10GBASER */ break; + case PHY_INTERFACE_MODE_INTERNAL: + return lynx_pcs_config_backplane(pcs, neg_mode, advertising); default: return -EOPNOTSUPP; } @@ -204,6 +230,9 @@ static void lynx_pcs_an_restart(struct phylink_pcs *pcs) { struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + if (lynx->backplane_mode) + return mtip_backplane_an_restart(lynx->anlt[PRIMARY_LANE]); + phylink_mii_c22_pcs_an_restart(lynx->mdio); } @@ -306,16 +335,111 @@ static void lynx_pcs_link_up(struct phylink_pcs *pcs, unsigned int neg_mode, } } +static int lynx_pcs_validate(struct phylink_pcs *pcs, unsigned long *supported, + const struct phylink_link_state *state) +{ + struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + + if (state->interface != PHY_INTERFACE_MODE_INTERNAL) + return 0; + + return mtip_backplane_validate(lynx->anlt[PRIMARY_LANE], supported); +} + +static int lynx_pcs_enable(struct phylink_pcs *pcs) +{ + struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + int err; + + if (lynx->backplane_mode) { + err = mtip_backplane_resume(lynx->anlt[PRIMARY_LANE]); + if (err) + return err; + } + + return 0; +} + +static void lynx_pcs_disable(struct phylink_pcs *pcs) +{ + struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + + if (lynx->backplane_mode) + mtip_backplane_suspend(lynx->anlt[PRIMARY_LANE]); +} + static const struct phylink_pcs_ops lynx_pcs_phylink_ops = { .pcs_get_state = lynx_pcs_get_state, .pcs_config = lynx_pcs_config, .pcs_an_restart = lynx_pcs_an_restart, .pcs_link_up = lynx_pcs_link_up, + .pcs_validate = lynx_pcs_validate, + .pcs_enable = lynx_pcs_enable, + .pcs_disable = lynx_pcs_disable, }; +static int lynx_pcs_parse_fwnode(struct lynx_pcs *lynx) +{ + struct fwnode_handle *node = lynx->mdio->dev.fwnode; + enum mtip_model model = MTIP_MODEL_AUTODETECT; + struct device_node *np = to_of_node(node); + struct mdio_device *mdio = lynx->mdio; + struct device *dev = &mdio->dev; + struct phy *phy; + int i, err; + + if (!node) + return 0; + + lynx->backplane_mode = fwnode_property_present(node, "fsl,backplane-mode"); + if (!lynx->backplane_mode) + return 0; + + if (fwnode_device_is_compatible(node, "fsl,lx2160a-lynx-pcs")) + model = MTIP_MODEL_LX2160A; + + lynx->num_lanes = of_count_phandle_with_args(np, "phys", "#phy-cells"); + if (lynx->num_lanes < 0) + return lynx->num_lanes; + + if (WARN_ON(lynx->num_lanes > MAX_NUM_LANES)) + return -EINVAL; + + for (i = 0; i < lynx->num_lanes; i++) { + phy = devm_of_phy_get_by_index(dev, np, i); + if (IS_ERR(phy)) + return dev_err_probe(dev, PTR_ERR(phy), + "Failed to get SerDes PHY %d\n", i); + + lynx->anlt[i] = mtip_backplane_create(mdio, phy, model); + if (IS_ERR(lynx->anlt[i])) { + err = PTR_ERR(lynx->anlt[i]); + + while (i-- > 0) + mtip_backplane_destroy(lynx->anlt[i]); + + return err; + } + } + + for (i = 1; i < lynx->num_lanes; i++) { + err = mtip_backplane_add_subordinate(lynx->anlt[PRIMARY_LANE], + lynx->anlt[i]); + if (WARN_ON(err)) { + /* Too many SerDes lanes in the device tree? */ + for (i = 0; i < lynx->num_lanes; i++) + mtip_backplane_destroy(lynx->anlt[i]); + return err; + } + } + + return 0; +} + static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) { struct lynx_pcs *lynx; + int err; lynx = kzalloc(sizeof(*lynx), GFP_KERNEL); if (!lynx) @@ -327,6 +451,12 @@ static struct phylink_pcs *lynx_pcs_create(struct mdio_device *mdio) lynx->pcs.neg_mode = true; lynx->pcs.poll = true; + err = lynx_pcs_parse_fwnode(lynx); + if (err) { + kfree(lynx); + return ERR_PTR(err); + } + return lynx_to_phylink_pcs(lynx); } @@ -392,6 +522,11 @@ EXPORT_SYMBOL_GPL(lynx_pcs_create_fwnode); void lynx_pcs_destroy(struct phylink_pcs *pcs) { struct lynx_pcs *lynx = phylink_pcs_to_lynx(pcs); + int i; + + if (lynx->backplane_mode) + for (i = 0; i < lynx->num_lanes; i++) + mtip_backplane_destroy(lynx->anlt[i]); mdio_device_put(lynx->mdio); kfree(lynx);
If the fsl,backplane-mode device tree property is present, then the Lynx PCS makes use of the AN/LT block to advertise the supported backplane link modes using clause 73 autoneg. Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- v1->v2: code is new drivers/net/pcs/Kconfig | 1 + drivers/net/pcs/pcs-lynx.c | 135 +++++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+)