| Message ID | 20220914093911.187764-4-s-vadapalli@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for J721e CPSW9G and SGMII mode | expand |
Hi Siddharth, On 14/09/2022 12:39, Siddharth Vadapalli wrote: > Each of the CPSW9G ports in J721e support additional modes like QSGMII. > Add a new compatible for J721e to support the additional modes. > > In TI's J721e, each of the CPSW9G ethernet interfaces can act as a > QSGMII main or QSGMII-SUB port. The QSGMII main interface is responsible > for performing auto-negotiation between the MAC and the PHY while the rest > of the interfaces are designated as QSGMII-SUB interfaces, indicating that > they will not be taking part in the auto-negotiation process. > > Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> > --- > drivers/phy/ti/phy-gmii-sel.c | 47 +++++++++++++++++++++++++++-------- > 1 file changed, 37 insertions(+), 10 deletions(-) > > diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c > index f0b2ba7a9c96..fdb1a7db123d 100644 > --- a/drivers/phy/ti/phy-gmii-sel.c > +++ b/drivers/phy/ti/phy-gmii-sel.c > @@ -223,6 +223,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = { > .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII), > }; > > +static const > +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = { > + .use_of_data = true, > + .regfields = phy_gmii_sel_fields_am654, > + .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII), > +}; > + > static const struct of_device_id phy_gmii_sel_id_table[] = { > { > .compatible = "ti,am3352-phy-gmii-sel", > @@ -248,6 +255,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = { > .compatible = "ti,j7200-cpsw5g-phy-gmii-sel", > .data = &phy_gmii_sel_cpsw5g_soc_j7200, > }, > + { > + .compatible = "ti,j721e-cpsw9g-phy-gmii-sel", > + .data = &phy_gmii_sel_cpsw9g_soc_j721e, > + }, > {} > }; > MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table); > @@ -389,7 +400,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) > struct device_node *node = dev->of_node; > const struct of_device_id *of_id; > struct phy_gmii_sel_priv *priv; > - u32 main_ports = 1; > + u32 main_ports[2] = {1, 1}; > int ret; > > of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node); > @@ -403,15 +414,31 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) > priv->dev = &pdev->dev; > priv->soc_data = of_id->data; > priv->num_ports = priv->soc_data->num_ports; > - of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports); > - /* > - * Ensure that main_ports is within bounds. If the property > - * ti,qsgmii-main-ports is not mentioned, or the value mentioned > - * is out of bounds, default to 1. > - */ > - if (main_ports < 1 || main_ports > 4) > - main_ports = 1; > - priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports); > + /* Differentiate between J7200 CPSW5G and J721e CPSW9G */ > + if (of_device_is_compatible(node, "ti,j7200-cpsw5g-phy-gmii-sel") > 0) { Why not just "if (of_device_is_compatible())" ? > + of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports[0]); > + /* > + * Ensure that main_ports is within bounds. If the property > + * ti,qsgmii-main-ports is not mentioned, or the value mentioned > + * is out of bounds, default to 1. > + */ > + if (main_ports[0] < 1 || main_ports[0] > 4) > + main_ports[0] = 1; how about printing this issue with dev_err()? > + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); > + } else if (of_device_is_compatible(node, "ti,j721e-cpsw9g-phy-gmii-sel") > 0) { > + of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports[0], 2); > + /* > + * Ensure that main_ports is within bounds. If the property > + * ti,qsgmii-main-ports is not mentioned, or the value mentioned > + * is out of bounds, default to 1. > + */ > + if (main_ports[0] < 1 || main_ports[0] > 8) > + main_ports[0] = 1; > + if (main_ports[1] < 1 || main_ports[1] > 8) > + main_ports[1] = 1; > + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); > + priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports[1]); > + } The whole if/else logic can be got rid of if you store num_qsgmii_main_ports in priv data structure after obtaining it from of_data. Then all the above reduces to for (i = 0; i < priv->num_qsgmii_main_ports; i++) { if (main_ports[i] ...) } It will also make it very easy to scale later on for future platforms. > > priv->regmap = syscon_node_to_regmap(node->parent); > if (IS_ERR(priv->regmap)) { cheers, -roger
Hello Roger, On 14/09/22 17:04, Roger Quadros wrote: > Hi Siddharth, > > On 14/09/2022 12:39, Siddharth Vadapalli wrote: >> Each of the CPSW9G ports in J721e support additional modes like QSGMII. >> Add a new compatible for J721e to support the additional modes. >> >> In TI's J721e, each of the CPSW9G ethernet interfaces can act as a >> QSGMII main or QSGMII-SUB port. The QSGMII main interface is responsible >> for performing auto-negotiation between the MAC and the PHY while the rest >> of the interfaces are designated as QSGMII-SUB interfaces, indicating that >> they will not be taking part in the auto-negotiation process. >> >> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> >> --- >> drivers/phy/ti/phy-gmii-sel.c | 47 +++++++++++++++++++++++++++-------- >> 1 file changed, 37 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c >> index f0b2ba7a9c96..fdb1a7db123d 100644 >> --- a/drivers/phy/ti/phy-gmii-sel.c >> +++ b/drivers/phy/ti/phy-gmii-sel.c >> @@ -223,6 +223,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = { >> .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII), >> }; >> >> +static const >> +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = { >> + .use_of_data = true, >> + .regfields = phy_gmii_sel_fields_am654, >> + .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII), >> +}; >> + >> static const struct of_device_id phy_gmii_sel_id_table[] = { >> { >> .compatible = "ti,am3352-phy-gmii-sel", >> @@ -248,6 +255,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = { >> .compatible = "ti,j7200-cpsw5g-phy-gmii-sel", >> .data = &phy_gmii_sel_cpsw5g_soc_j7200, >> }, >> + { >> + .compatible = "ti,j721e-cpsw9g-phy-gmii-sel", >> + .data = &phy_gmii_sel_cpsw9g_soc_j721e, >> + }, >> {} >> }; >> MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table); >> @@ -389,7 +400,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) >> struct device_node *node = dev->of_node; >> const struct of_device_id *of_id; >> struct phy_gmii_sel_priv *priv; >> - u32 main_ports = 1; >> + u32 main_ports[2] = {1, 1}; >> int ret; >> >> of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node); >> @@ -403,15 +414,31 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) >> priv->dev = &pdev->dev; >> priv->soc_data = of_id->data; >> priv->num_ports = priv->soc_data->num_ports; >> - of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports); >> - /* >> - * Ensure that main_ports is within bounds. If the property >> - * ti,qsgmii-main-ports is not mentioned, or the value mentioned >> - * is out of bounds, default to 1. >> - */ >> - if (main_ports < 1 || main_ports > 4) >> - main_ports = 1; >> - priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports); >> + /* Differentiate between J7200 CPSW5G and J721e CPSW9G */ >> + if (of_device_is_compatible(node, "ti,j7200-cpsw5g-phy-gmii-sel") > 0) { > > Why not just "if (of_device_is_compatible())" ? Thank you for reviewing the patch. I will fix this in the v2 series. > >> + of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports[0]); >> + /* >> + * Ensure that main_ports is within bounds. If the property >> + * ti,qsgmii-main-ports is not mentioned, or the value mentioned >> + * is out of bounds, default to 1. >> + */ >> + if (main_ports[0] < 1 || main_ports[0] > 4) >> + main_ports[0] = 1; > > how about printing this issue with dev_err()? I agree that using dev_err() instead of defaulting to a value is a better choice here. I had initially planned on defaulting to a value since this check is a part of the probe function and I had thought that the phy-mode is not yet known at this point. However, looking at it again, for the special case where the property "ti,qsgmii-main-ports" is mentioned in the devicetree node, it is possible to know with certainty that QSGMII mode is intended and a wrong value has been provided in the devicetree node. I will add dev_err() in the v2 series, instead of defaulting to 1 if the check fails. For the other scenario where "ti,qsgmii-main-ports" is not mentioned in the devicetree node, I think that defaulting to 1 would be the correct choice since the intended phy-mode is not yet known at this point. > >> + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); >> + } else if (of_device_is_compatible(node, "ti,j721e-cpsw9g-phy-gmii-sel") > 0) { >> + of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports[0], 2); >> + /* >> + * Ensure that main_ports is within bounds. If the property >> + * ti,qsgmii-main-ports is not mentioned, or the value mentioned >> + * is out of bounds, default to 1. >> + */ >> + if (main_ports[0] < 1 || main_ports[0] > 8) >> + main_ports[0] = 1; >> + if (main_ports[1] < 1 || main_ports[1] > 8) >> + main_ports[1] = 1; >> + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); >> + priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports[1]); >> + } > > The whole if/else logic can be got rid of if you store num_qsgmii_main_ports in priv data structure > after obtaining it from of_data. > > Then all the above reduces to > for (i = 0; i < priv->num_qsgmii_main_ports; i++) { > if (main_ports[i] ...) > } > > It will also make it very easy to scale later on for future platforms. Thank you for the suggestion. I will add the variable "u32 num_qsgmii_main_ports" in "struct phy_gmii_sel_soc_data" and set its value to 1 for the "phy_gmii_sel_cpsw5g_soc_j7200" compatible and to 2 for the "phy_gmii_sel_cpsw9g_soc_j721e" compatible. I will implement this in the v2 series. Regards, Siddharth.
diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c index f0b2ba7a9c96..fdb1a7db123d 100644 --- a/drivers/phy/ti/phy-gmii-sel.c +++ b/drivers/phy/ti/phy-gmii-sel.c @@ -223,6 +223,13 @@ struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = { .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII), }; +static const +struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw9g_soc_j721e = { + .use_of_data = true, + .regfields = phy_gmii_sel_fields_am654, + .extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII), +}; + static const struct of_device_id phy_gmii_sel_id_table[] = { { .compatible = "ti,am3352-phy-gmii-sel", @@ -248,6 +255,10 @@ static const struct of_device_id phy_gmii_sel_id_table[] = { .compatible = "ti,j7200-cpsw5g-phy-gmii-sel", .data = &phy_gmii_sel_cpsw5g_soc_j7200, }, + { + .compatible = "ti,j721e-cpsw9g-phy-gmii-sel", + .data = &phy_gmii_sel_cpsw9g_soc_j721e, + }, {} }; MODULE_DEVICE_TABLE(of, phy_gmii_sel_id_table); @@ -389,7 +400,7 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) struct device_node *node = dev->of_node; const struct of_device_id *of_id; struct phy_gmii_sel_priv *priv; - u32 main_ports = 1; + u32 main_ports[2] = {1, 1}; int ret; of_id = of_match_node(phy_gmii_sel_id_table, pdev->dev.of_node); @@ -403,15 +414,31 @@ static int phy_gmii_sel_probe(struct platform_device *pdev) priv->dev = &pdev->dev; priv->soc_data = of_id->data; priv->num_ports = priv->soc_data->num_ports; - of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports); - /* - * Ensure that main_ports is within bounds. If the property - * ti,qsgmii-main-ports is not mentioned, or the value mentioned - * is out of bounds, default to 1. - */ - if (main_ports < 1 || main_ports > 4) - main_ports = 1; - priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports); + /* Differentiate between J7200 CPSW5G and J721e CPSW9G */ + if (of_device_is_compatible(node, "ti,j7200-cpsw5g-phy-gmii-sel") > 0) { + of_property_read_u32(node, "ti,qsgmii-main-ports", &main_ports[0]); + /* + * Ensure that main_ports is within bounds. If the property + * ti,qsgmii-main-ports is not mentioned, or the value mentioned + * is out of bounds, default to 1. + */ + if (main_ports[0] < 1 || main_ports[0] > 4) + main_ports[0] = 1; + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); + } else if (of_device_is_compatible(node, "ti,j721e-cpsw9g-phy-gmii-sel") > 0) { + of_property_read_u32_array(node, "ti,qsgmii-main-ports", &main_ports[0], 2); + /* + * Ensure that main_ports is within bounds. If the property + * ti,qsgmii-main-ports is not mentioned, or the value mentioned + * is out of bounds, default to 1. + */ + if (main_ports[0] < 1 || main_ports[0] > 8) + main_ports[0] = 1; + if (main_ports[1] < 1 || main_ports[1] > 8) + main_ports[1] = 1; + priv->qsgmii_main_ports = PHY_GMII_PORT(main_ports[0]); + priv->qsgmii_main_ports |= PHY_GMII_PORT(main_ports[1]); + } priv->regmap = syscon_node_to_regmap(node->parent); if (IS_ERR(priv->regmap)) {
Each of the CPSW9G ports in J721e support additional modes like QSGMII. Add a new compatible for J721e to support the additional modes. In TI's J721e, each of the CPSW9G ethernet interfaces can act as a QSGMII main or QSGMII-SUB port. The QSGMII main interface is responsible for performing auto-negotiation between the MAC and the PHY while the rest of the interfaces are designated as QSGMII-SUB interfaces, indicating that they will not be taking part in the auto-negotiation process. Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com> --- drivers/phy/ti/phy-gmii-sel.c | 47 +++++++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 10 deletions(-)