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(+)
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);
--
2.34.1On 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
>
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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!
© 2016 - 2026 Red Hat, Inc.