[PATCH 0/6] Add support for J721e CPSW9G and SGMII mode

[PATCH 0/6] Add support for J721e CPSW9G and SGMII mode

[Siddharth Vadapalli]

Add compatible for J721e CPSW9G.

Add support for SGMII mode in phy-gmii-sel driver for CPSW5G of J7200 and
CPSW9G of J721e.

Add SGMII support in phy-j721e-wiz driver for J721E_WIZ_16G.

Add support for PCIe + SGMII multilink configuration in phy-cadence-sierra
driver.

Siddharth Vadapalli (5):
  dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e
  phy: ti: gmii-sel: Add support for configuring CPSW5G ports in SGMII
    mode
  phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e
  phy: ti: gmii-sel: Enable SGMII mode configuration for J721E
  phy: ti: phy-j721e-wiz: Add SGMII support in wiz driver for J721E

Swapnil Jakhade (1):
  phy: cadence: Sierra: Add PCIe + SGMII PHY multilink configuration

 .../bindings/phy/ti,phy-gmii-sel.yaml         |  52 ++++++-
 drivers/phy/cadence/phy-cadence-sierra.c      | 141 +++++++++++++++++-
 drivers/phy/ti/phy-gmii-sel.c                 |  57 +++++--
 drivers/phy/ti/phy-j721e-wiz.c                |   1 +
 4 files changed, 232 insertions(+), 19 deletions(-)

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
that are not supported on earlier SoCs. Add a compatible for it.

Extend ti,qsgmii-main-ports property to support selection of upto
two main ports at once across the two QSGMII interfaces.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 .../bindings/phy/ti,phy-gmii-sel.yaml         | 52 ++++++++++++++++---
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
index da7cac537e15..1e19efab018b 100644
--- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
+++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
@@ -54,6 +54,7 @@ properties:
       - ti,dm814-phy-gmii-sel
       - ti,am654-phy-gmii-sel
       - ti,j7200-cpsw5g-phy-gmii-sel
+      - ti,j721e-cpsw9g-phy-gmii-sel
 
   reg:
     maxItems: 1
@@ -65,12 +66,19 @@ properties:
     description: |
       Required only for QSGMII mode. Array to select the port for
       QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
-      ports automatically. Any one of the 4 CPSW5G ports can act as the
-      main port with the rest of them being the QSGMII_SUB ports.
-    maxItems: 1
-    items:
-      minimum: 1
-      maximum: 4
+      ports automatically. For J7200 CPSW5G with the compatible:
+      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
+      array of only one element, which is the port number ranging from
+      1 to 4. For J721e CPSW9G with the compatible:
+      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
+      of two elements, which corresponds to two potential QSGMII main
+      ports. The first element and second element of the array can both
+      range from 1 to 8 each, corresponding to two QSGMII main ports.
+      For J721e CPSW9G, to configure port 2 as the first QSGMII main
+      port and port 7 as the second QSGMII main port, we specify:
+      ti,qsgmii-main-ports = <2>, <7>;
+      If only one QSGMII main port is desired, mention the same main
+      port twice.
 
 allOf:
   - if:
@@ -81,12 +89,43 @@ allOf:
               - ti,dra7xx-phy-gmii-sel
               - ti,dm814-phy-gmii-sel
               - ti,am654-phy-gmii-sel
+              - ti,j7200-cpsw5g-phy-gmii-sel
+              - ti,j721e-cpsw9g-phy-gmii-sel
     then:
       properties:
         '#phy-cells':
           const: 1
           description: CPSW port number (starting from 1)
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ti,j7200-cpsw5g-phy-gmii-sel
+    then:
+      properties:
+        ti,qsgmii-main-ports:
+          maxItems: 1
+          items:
+            minimum: 1
+            maximum: 4
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - ti,j721e-cpsw9g-phy-gmii-sel
+    then:
+      properties:
+        ti,qsgmii-main-ports:
+          minItems: 2
+          maxItems: 2
+          items:
+            minimum: 1
+            maximum: 8
+
   - if:
       not:
         properties:
@@ -94,6 +133,7 @@ allOf:
             contains:
               enum:
                 - ti,j7200-cpsw5g-phy-gmii-sel
+                - ti,j721e-cpsw9g-phy-gmii-sel
     then:
       properties:
         ti,qsgmii-main-ports: false
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] phy: ti: gmii-sel: Add support for configuring CPSW5G ports in SGMII mode

[Siddharth Vadapalli]

CPSW5G ports on J7200 support SGMII mode. Add support to the phy-gmii-sel
driver to configure the CPSW5G ports in SGMII mode.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index 0bcfd6d96b4d..f0b2ba7a9c96 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -23,6 +23,7 @@
 #define AM33XX_GMII_SEL_MODE_RGMII	2
 
 /* J72xx SoC specific definitions for the CONTROL port */
+#define J72XX_GMII_SEL_MODE_SGMII	3
 #define J72XX_GMII_SEL_MODE_QSGMII	4
 #define J72XX_GMII_SEL_MODE_QSGMII_SUB	6
 
@@ -105,6 +106,13 @@ static int phy_gmii_sel_mode(struct phy *phy, enum phy_mode mode, int submode)
 			gmii_sel_mode = J72XX_GMII_SEL_MODE_QSGMII_SUB;
 		break;
 
+	case PHY_INTERFACE_MODE_SGMII:
+		if (!(soc_data->extra_modes & BIT(PHY_INTERFACE_MODE_SGMII)))
+			goto unsupported;
+		else
+			gmii_sel_mode = J72XX_GMII_SEL_MODE_SGMII;
+		break;
+
 	default:
 		goto unsupported;
 	}
@@ -212,7 +220,7 @@ static const
 struct phy_gmii_sel_soc_data phy_gmii_sel_cpsw5g_soc_j7200 = {
 	.use_of_data = true,
 	.regfields = phy_gmii_sel_fields_am654,
-	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII),
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
 };
 
 static const struct of_device_id phy_gmii_sel_id_table[] = {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

[Siddharth Vadapalli]

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) {
+		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)) {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] phy: ti: gmii-sel: Enable SGMII mode configuration for J721E

[Siddharth Vadapalli]

CPSW9G ports on J721E support SGMII mode. Add SGMII mode to the
extra_modes member of struct "phy_gmii_sel_cpsw9g_soc_j721e".

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-gmii-sel.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/ti/phy-gmii-sel.c b/drivers/phy/ti/phy-gmii-sel.c
index fdb1a7db123d..13877d0d15e4 100644
--- a/drivers/phy/ti/phy-gmii-sel.c
+++ b/drivers/phy/ti/phy-gmii-sel.c
@@ -227,7 +227,7 @@ 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),
+	.extra_modes = BIT(PHY_INTERFACE_MODE_QSGMII) | BIT(PHY_INTERFACE_MODE_SGMII),
 };
 
 static const struct of_device_id phy_gmii_sel_id_table[] = {
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] phy: ti: phy-j721e-wiz: Add SGMII support in wiz driver for J721E

[Siddharth Vadapalli]

Enable full rate divider configuration support for J721E_WIZ_16G for
SGMII.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/ti/phy-j721e-wiz.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/phy/ti/phy-j721e-wiz.c b/drivers/phy/ti/phy-j721e-wiz.c
index 20af142580ad..9a33aebdcbe5 100644
--- a/drivers/phy/ti/phy-j721e-wiz.c
+++ b/drivers/phy/ti/phy-j721e-wiz.c
@@ -1191,6 +1191,7 @@ static int wiz_phy_fullrt_div(struct wiz *wiz, int lane)
 			return regmap_field_write(wiz->p0_fullrt_div[lane], 0x1);
 		break;
 	case J721E_WIZ_10G:
+	case J721E_WIZ_16G:
 	case J7200_WIZ_10G:
 		if (wiz->lane_phy_type[lane] == PHY_TYPE_SGMII)
 			return regmap_field_write(wiz->p0_fullrt_div[lane], 0x2);
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] phy: cadence: Sierra: Add PCIe + SGMII PHY multilink configuration

[Siddharth Vadapalli]

From: Swapnil Jakhade <sjakhade@cadence.com>

Add register sequences for PCIe + SGMII PHY multilink configuration.

Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
---
 drivers/phy/cadence/phy-cadence-sierra.c | 141 ++++++++++++++++++++++-
 1 file changed, 139 insertions(+), 2 deletions(-)

diff --git a/drivers/phy/cadence/phy-cadence-sierra.c b/drivers/phy/cadence/phy-cadence-sierra.c
index 6e86a6517f37..7c0daf3e8880 100644
--- a/drivers/phy/cadence/phy-cadence-sierra.c
+++ b/drivers/phy/cadence/phy-cadence-sierra.c
@@ -24,7 +24,7 @@
 #include <dt-bindings/phy/phy-cadence.h>
 
 #define NUM_SSC_MODE		3
-#define NUM_PHY_TYPE		4
+#define NUM_PHY_TYPE		5
 
 /* PHY register offsets */
 #define SIERRA_COMMON_CDB_OFFSET			0x0
@@ -46,7 +46,9 @@
 #define SIERRA_CMN_REFRCV_PREG				0x98
 #define SIERRA_CMN_REFRCV1_PREG				0xB8
 #define SIERRA_CMN_PLLLC1_GEN_PREG			0xC2
+#define SIERRA_CMN_PLLLC1_FBDIV_INT_PREG		0xC3
 #define SIERRA_CMN_PLLLC1_LF_COEFF_MODE0_PREG		0xCA
+#define SIERRA_CMN_PLLLC1_CLK0_PREG			0xCE
 #define SIERRA_CMN_PLLLC1_BWCAL_MODE0_PREG		0xD0
 #define SIERRA_CMN_PLLLC1_SS_TIME_STEPSIZE_MODE_PREG	0xE2
 
@@ -74,6 +76,7 @@
 #define SIERRA_PSC_RX_A1_PREG				0x031
 #define SIERRA_PSC_RX_A2_PREG				0x032
 #define SIERRA_PSC_RX_A3_PREG				0x033
+#define SIERRA_PLLCTRL_FBDIV_MODE01_PREG		0x039
 #define SIERRA_PLLCTRL_SUBRATE_PREG			0x03A
 #define SIERRA_PLLCTRL_GEN_A_PREG			0x03B
 #define SIERRA_PLLCTRL_GEN_D_PREG			0x03E
@@ -298,6 +301,7 @@ enum cdns_sierra_phy_type {
 	TYPE_NONE,
 	TYPE_PCIE,
 	TYPE_USB,
+	TYPE_SGMII,
 	TYPE_QSGMII
 };
 
@@ -936,6 +940,9 @@ static int cdns_sierra_get_optional(struct cdns_sierra_inst *inst,
 	case PHY_TYPE_USB3:
 		inst->phy_type = TYPE_USB;
 		break;
+	case PHY_TYPE_SGMII:
+		inst->phy_type = TYPE_SGMII;
+		break;
 	case PHY_TYPE_QSGMII:
 		inst->phy_type = TYPE_QSGMII;
 		break;
@@ -1339,7 +1346,7 @@ static int cdns_sierra_phy_configure_multilink(struct cdns_sierra_phy *sp)
 			}
 		}
 
-		if (phy_t1 == TYPE_QSGMII)
+		if (phy_t1 == TYPE_SGMII || phy_t1 == TYPE_QSGMII)
 			reset_control_deassert(sp->phys[node].lnk_rst);
 	}
 
@@ -1537,6 +1544,71 @@ static int cdns_sierra_phy_remove(struct platform_device *pdev)
 	return 0;
 }
 
+/* SGMII PHY PMA lane configuration */
+static struct cdns_reg_pairs sgmii_phy_pma_ln_regs[] = {
+	{0x9010, SIERRA_PHY_PMA_XCVR_CTRL}
+};
+
+static struct cdns_sierra_vals sgmii_phy_pma_ln_vals = {
+	.reg_pairs = sgmii_phy_pma_ln_regs,
+	.num_regs = ARRAY_SIZE(sgmii_phy_pma_ln_regs),
+};
+
+/* SGMII refclk 100MHz, no ssc, opt3 and GE1 links using PLL LC1 */
+static const struct cdns_reg_pairs sgmii_100_no_ssc_plllc1_opt3_cmn_regs[] = {
+	{0x002D, SIERRA_CMN_PLLLC1_FBDIV_INT_PREG},
+	{0x2085, SIERRA_CMN_PLLLC1_LF_COEFF_MODE0_PREG},
+	{0x1005, SIERRA_CMN_PLLLC1_CLK0_PREG},
+	{0x0000, SIERRA_CMN_PLLLC1_BWCAL_MODE0_PREG},
+	{0x0800, SIERRA_CMN_PLLLC1_SS_TIME_STEPSIZE_MODE_PREG}
+};
+
+static const struct cdns_reg_pairs sgmii_100_no_ssc_plllc1_opt3_ln_regs[] = {
+	{0x688E, SIERRA_DET_STANDEC_D_PREG},
+	{0x0004, SIERRA_PSC_LN_IDLE_PREG},
+	{0x0FFE, SIERRA_PSC_RX_A0_PREG},
+	{0x0106, SIERRA_PLLCTRL_FBDIV_MODE01_PREG},
+	{0x0013, SIERRA_PLLCTRL_SUBRATE_PREG},
+	{0x0003, SIERRA_PLLCTRL_GEN_A_PREG},
+	{0x0106, SIERRA_PLLCTRL_GEN_D_PREG},
+	{0x5231, SIERRA_PLLCTRL_CPGAIN_MODE_PREG },
+	{0x0000, SIERRA_DRVCTRL_ATTEN_PREG},
+	{0x9702, SIERRA_DRVCTRL_BOOST_PREG},
+	{0x0051, SIERRA_RX_CREQ_FLTR_A_MODE0_PREG},
+	{0x3C0E, SIERRA_CREQ_CCLKDET_MODE01_PREG},
+	{0x3220, SIERRA_CREQ_FSMCLK_SEL_PREG},
+	{0x0000, SIERRA_CREQ_EQ_CTRL_PREG},
+	{0x0002, SIERRA_DEQ_PHALIGN_CTRL},
+	{0x0186, SIERRA_DEQ_GLUT0},
+	{0x0186, SIERRA_DEQ_GLUT1},
+	{0x0186, SIERRA_DEQ_GLUT2},
+	{0x0186, SIERRA_DEQ_GLUT3},
+	{0x0186, SIERRA_DEQ_GLUT4},
+	{0x0861, SIERRA_DEQ_ALUT0},
+	{0x07E0, SIERRA_DEQ_ALUT1},
+	{0x079E, SIERRA_DEQ_ALUT2},
+	{0x071D, SIERRA_DEQ_ALUT3},
+	{0x03F5, SIERRA_DEQ_DFETAP_CTRL_PREG},
+	{0x0C01, SIERRA_DEQ_TAU_CTRL1_FAST_MAINT_PREG},
+	{0x3C40, SIERRA_DEQ_TAU_CTRL1_SLOW_MAINT_PREG},
+	{0x1C04, SIERRA_DEQ_TAU_CTRL2_PREG},
+	{0x0033, SIERRA_DEQ_PICTRL_PREG},
+	{0x0000, SIERRA_CPI_OUTBUF_RATESEL_PREG},
+	{0x0B6D, SIERRA_CPI_RESBIAS_BIN_PREG},
+	{0x0102, SIERRA_RXBUFFER_CTLECTRL_PREG},
+	{0x0002, SIERRA_RXBUFFER_RCDFECTRL_PREG}
+};
+
+static struct cdns_sierra_vals sgmii_100_no_ssc_plllc1_opt3_cmn_vals = {
+	.reg_pairs = sgmii_100_no_ssc_plllc1_opt3_cmn_regs,
+	.num_regs = ARRAY_SIZE(sgmii_100_no_ssc_plllc1_opt3_cmn_regs),
+};
+
+static struct cdns_sierra_vals sgmii_100_no_ssc_plllc1_opt3_ln_vals = {
+	.reg_pairs = sgmii_100_no_ssc_plllc1_opt3_ln_regs,
+	.num_regs = ARRAY_SIZE(sgmii_100_no_ssc_plllc1_opt3_ln_regs),
+};
+
 /* QSGMII PHY PMA lane configuration */
 static struct cdns_reg_pairs qsgmii_phy_pma_ln_regs[] = {
 	{0x9010, SIERRA_PHY_PMA_XCVR_CTRL}
@@ -2363,6 +2435,11 @@ static const struct cdns_sierra_data cdns_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
 				[INTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &pcie_phy_pcs_cmn_vals,
+				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
+				[INTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &pcie_phy_pcs_cmn_vals,
 				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
@@ -2377,6 +2454,11 @@ static const struct cdns_sierra_data cdns_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_cmn_vals,
 				[INTERNAL_SSC] = &pcie_100_int_ssc_cmn_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &pcie_100_no_ssc_plllc_cmn_vals,
+				[EXTERNAL_SSC] = &pcie_100_ext_ssc_plllc_cmn_vals,
+				[INTERNAL_SSC] = &pcie_100_int_ssc_plllc_cmn_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &pcie_100_no_ssc_plllc_cmn_vals,
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_plllc_cmn_vals,
@@ -2388,6 +2470,13 @@ static const struct cdns_sierra_data cdns_map_sierra = {
 				[EXTERNAL_SSC] = &usb_100_ext_ssc_cmn_vals,
 			},
 		},
+		[TYPE_SGMII] = {
+			[TYPE_PCIE] = {
+				[NO_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+				[EXTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+				[INTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+			},
+		},
 		[TYPE_QSGMII] = {
 			[TYPE_PCIE] = {
 				[NO_SSC] = &qsgmii_100_no_ssc_plllc1_cmn_vals,
@@ -2403,6 +2492,11 @@ static const struct cdns_sierra_data cdns_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_ln_vals,
 				[INTERNAL_SSC] = &pcie_100_int_ssc_ln_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &ml_pcie_100_no_ssc_ln_vals,
+				[EXTERNAL_SSC] = &ml_pcie_100_ext_ssc_ln_vals,
+				[INTERNAL_SSC] = &ml_pcie_100_int_ssc_ln_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &ml_pcie_100_no_ssc_ln_vals,
 				[EXTERNAL_SSC] = &ml_pcie_100_ext_ssc_ln_vals,
@@ -2414,6 +2508,13 @@ static const struct cdns_sierra_data cdns_map_sierra = {
 				[EXTERNAL_SSC] = &usb_100_ext_ssc_ln_vals,
 			},
 		},
+		[TYPE_SGMII] = {
+			[TYPE_PCIE] = {
+				[NO_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+				[EXTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+				[INTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+			},
+		},
 		[TYPE_QSGMII] = {
 			[TYPE_PCIE] = {
 				[NO_SSC] = &qsgmii_100_no_ssc_plllc1_ln_vals,
@@ -2435,6 +2536,11 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
 				[INTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &pcie_phy_pcs_cmn_vals,
+				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
+				[INTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &pcie_phy_pcs_cmn_vals,
 				[EXTERNAL_SSC] = &pcie_phy_pcs_cmn_vals,
@@ -2443,6 +2549,13 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 		},
 	},
 	.phy_pma_ln_vals = {
+		[TYPE_SGMII] = {
+			[TYPE_PCIE] = {
+				[NO_SSC] = &sgmii_phy_pma_ln_vals,
+				[EXTERNAL_SSC] = &sgmii_phy_pma_ln_vals,
+				[INTERNAL_SSC] = &sgmii_phy_pma_ln_vals,
+			},
+		},
 		[TYPE_QSGMII] = {
 			[TYPE_PCIE] = {
 				[NO_SSC] = &qsgmii_phy_pma_ln_vals,
@@ -2458,6 +2571,11 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_cmn_vals,
 				[INTERNAL_SSC] = &pcie_100_int_ssc_cmn_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &pcie_100_no_ssc_plllc_cmn_vals,
+				[EXTERNAL_SSC] = &pcie_100_ext_ssc_plllc_cmn_vals,
+				[INTERNAL_SSC] = &pcie_100_int_ssc_plllc_cmn_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &pcie_100_no_ssc_plllc_cmn_vals,
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_plllc_cmn_vals,
@@ -2469,6 +2587,13 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 				[EXTERNAL_SSC] = &usb_100_ext_ssc_cmn_vals,
 			},
 		},
+		[TYPE_SGMII] = {
+			[TYPE_PCIE] = {
+				[NO_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+				[EXTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+				[INTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_cmn_vals,
+			},
+		},
 		[TYPE_QSGMII] = {
 			[TYPE_PCIE] = {
 				[NO_SSC] = &qsgmii_100_no_ssc_plllc1_cmn_vals,
@@ -2484,6 +2609,11 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 				[EXTERNAL_SSC] = &pcie_100_ext_ssc_ln_vals,
 				[INTERNAL_SSC] = &pcie_100_int_ssc_ln_vals,
 			},
+			[TYPE_SGMII] = {
+				[NO_SSC] = &ti_ml_pcie_100_no_ssc_ln_vals,
+				[EXTERNAL_SSC] = &ti_ml_pcie_100_ext_ssc_ln_vals,
+				[INTERNAL_SSC] = &ti_ml_pcie_100_int_ssc_ln_vals,
+			},
 			[TYPE_QSGMII] = {
 				[NO_SSC] = &ti_ml_pcie_100_no_ssc_ln_vals,
 				[EXTERNAL_SSC] = &ti_ml_pcie_100_ext_ssc_ln_vals,
@@ -2495,6 +2625,13 @@ static const struct cdns_sierra_data cdns_ti_map_sierra = {
 				[EXTERNAL_SSC] = &usb_100_ext_ssc_ln_vals,
 			},
 		},
+		[TYPE_SGMII] = {
+			[TYPE_PCIE] = {
+				[NO_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+				[EXTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+				[INTERNAL_SSC] = &sgmii_100_no_ssc_plllc1_opt3_ln_vals,
+			},
+		},
 		[TYPE_QSGMII] = {
 			[TYPE_PCIE] = {
 				[NO_SSC] = &qsgmii_100_no_ssc_plllc1_ln_vals,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

[Roger Quadros]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Rob Herring]

On Wed, Sep 14, 2022 at 03:09:06PM +0530, Siddharth Vadapalli wrote:
> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
> that are not supported on earlier SoCs. Add a compatible for it.
> 
> Extend ti,qsgmii-main-ports property to support selection of upto
> two main ports at once across the two QSGMII interfaces.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 52 ++++++++++++++++---
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> index da7cac537e15..1e19efab018b 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -54,6 +54,7 @@ properties:
>        - ti,dm814-phy-gmii-sel
>        - ti,am654-phy-gmii-sel
>        - ti,j7200-cpsw5g-phy-gmii-sel
> +      - ti,j721e-cpsw9g-phy-gmii-sel
>  
>    reg:
>      maxItems: 1
> @@ -65,12 +66,19 @@ properties:
>      description: |
>        Required only for QSGMII mode. Array to select the port for
>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
> -      main port with the rest of them being the QSGMII_SUB ports.
> -    maxItems: 1
> -    items:
> -      minimum: 1
> -      maximum: 4
> +      ports automatically. For J7200 CPSW5G with the compatible:
> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
> +      array of only one element, which is the port number ranging from
> +      1 to 4. For J721e CPSW9G with the compatible:
> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
> +      of two elements, which corresponds to two potential QSGMII main
> +      ports. The first element and second element of the array can both
> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
> +      port and port 7 as the second QSGMII main port, we specify:
> +      ti,qsgmii-main-ports = <2>, <7>;
> +      If only one QSGMII main port is desired, mention the same main
> +      port twice.

Two different forms for the same property name is not great. Just make a 
new property if you need something different.

Rob

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

Hello Rob,

On 14/09/22 21:45, Rob Herring wrote:
> On Wed, Sep 14, 2022 at 03:09:06PM +0530, Siddharth Vadapalli wrote:
>> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
>> that are not supported on earlier SoCs. Add a compatible for it.
>>
>> Extend ti,qsgmii-main-ports property to support selection of upto
>> two main ports at once across the two QSGMII interfaces.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 52 ++++++++++++++++---
>>  1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> index da7cac537e15..1e19efab018b 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -54,6 +54,7 @@ properties:
>>        - ti,dm814-phy-gmii-sel
>>        - ti,am654-phy-gmii-sel
>>        - ti,j7200-cpsw5g-phy-gmii-sel
>> +      - ti,j721e-cpsw9g-phy-gmii-sel
>>  
>>    reg:
>>      maxItems: 1
>> @@ -65,12 +66,19 @@ properties:
>>      description: |
>>        Required only for QSGMII mode. Array to select the port for
>>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
>> -      main port with the rest of them being the QSGMII_SUB ports.
>> -    maxItems: 1
>> -    items:
>> -      minimum: 1
>> -      maximum: 4
>> +      ports automatically. For J7200 CPSW5G with the compatible:
>> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>> +      array of only one element, which is the port number ranging from
>> +      1 to 4. For J721e CPSW9G with the compatible:
>> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>> +      of two elements, which corresponds to two potential QSGMII main
>> +      ports. The first element and second element of the array can both
>> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
>> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
>> +      port and port 7 as the second QSGMII main port, we specify:
>> +      ti,qsgmii-main-ports = <2>, <7>;
>> +      If only one QSGMII main port is desired, mention the same main
>> +      port twice.
> 
> Two different forms for the same property name is not great. Just make a 
> new property if you need something different.

Thank you for reviewing the patch. Based on the discussion for the
previous series at [1], I had planned to reuse the same property
"ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
that the property represents the same feature on both devices which is
that of the QSGMII main port. The only difference between the two of
them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
has 8 external ports. Thus, J7200 can have at most one QSGMII main port
while J721e can have up to two. Adding a new property which describes
the same feature appears to be redundant to me. Please let me know.

[1]-> https://lore.kernel.org/r/48080f45-ea9d-e8dc-7720-ef82fc84e3b3@linaro.org/

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] phy: ti: gmii-sel: Add support for CPSW9G GMII SEL in J721e

[Siddharth Vadapalli]

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.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Krzysztof Kozlowski]

On 14/09/2022 11:39, Siddharth Vadapalli wrote:
> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
> that are not supported on earlier SoCs. Add a compatible for it.
> 
> Extend ti,qsgmii-main-ports property to support selection of upto
> two main ports at once across the two QSGMII interfaces.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> ---
>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 52 ++++++++++++++++---
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> index da7cac537e15..1e19efab018b 100644
> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
> @@ -54,6 +54,7 @@ properties:
>        - ti,dm814-phy-gmii-sel
>        - ti,am654-phy-gmii-sel
>        - ti,j7200-cpsw5g-phy-gmii-sel
> +      - ti,j721e-cpsw9g-phy-gmii-sel
>  
>    reg:
>      maxItems: 1
> @@ -65,12 +66,19 @@ properties:
>      description: |
>        Required only for QSGMII mode. Array to select the port for
>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
> -      main port with the rest of them being the QSGMII_SUB ports.
> -    maxItems: 1
> -    items:
> -      minimum: 1
> -      maximum: 4

minItems: 1
maxItems: 2
  items:
    minimum: 1
    maximum: 8

> +      ports automatically. For J7200 CPSW5G with the compatible:
> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
> +      array of only one element, which is the port number ranging from
> +      1 to 4. For J721e CPSW9G with the compatible:
> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
> +      of two elements, which corresponds to two potential QSGMII main
> +      ports. The first element and second element of the array can both
> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
> +      port and port 7 as the second QSGMII main port, we specify:
> +      ti,qsgmii-main-ports = <2>, <7>;
> +      If only one QSGMII main port is desired, mention the same main
> +      port twice.
>  
>  allOf:
>    - if:
> @@ -81,12 +89,43 @@ allOf:
>                - ti,dra7xx-phy-gmii-sel
>                - ti,dm814-phy-gmii-sel
>                - ti,am654-phy-gmii-sel
> +              - ti,j7200-cpsw5g-phy-gmii-sel
> +              - ti,j721e-cpsw9g-phy-gmii-sel
>      then:
>        properties:
>          '#phy-cells':
>            const: 1
>            description: CPSW port number (starting from 1)
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,j7200-cpsw5g-phy-gmii-sel
> +    then:
> +      properties:
> +        ti,qsgmii-main-ports:
> +          maxItems: 1
> +          items:
> +            minimum: 1
> +            maximum: 4
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - ti,j721e-cpsw9g-phy-gmii-sel
> +    then:
> +      properties:
> +        ti,qsgmii-main-ports:
> +          minItems: 2
> +          maxItems: 2
> +          items:
> +            minimum: 1
> +            maximum: 8
> +
>    - if:
>        not:
>          properties:
> @@ -94,6 +133,7 @@ allOf:
>              contains:
>                enum:
>                  - ti,j7200-cpsw5g-phy-gmii-sel
> +                - ti,j721e-cpsw9g-phy-gmii-sel
>      then:
>        properties:
>          ti,qsgmii-main-ports: false

This is interesting here... Did you test the bindings with your DTS?


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Krzysztof Kozlowski]

On 15/09/2022 07:28, Siddharth Vadapalli wrote:
>>> @@ -65,12 +66,19 @@ properties:
>>>      description: |
>>>        Required only for QSGMII mode. Array to select the port for
>>>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
>>> -      main port with the rest of them being the QSGMII_SUB ports.
>>> -    maxItems: 1
>>> -    items:
>>> -      minimum: 1
>>> -      maximum: 4
>>> +      ports automatically. For J7200 CPSW5G with the compatible:
>>> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>>> +      array of only one element, which is the port number ranging from
>>> +      1 to 4. For J721e CPSW9G with the compatible:
>>> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>>> +      of two elements, which corresponds to two potential QSGMII main
>>> +      ports. The first element and second element of the array can both
>>> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
>>> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
>>> +      port and port 7 as the second QSGMII main port, we specify:
>>> +      ti,qsgmii-main-ports = <2>, <7>;
>>> +      If only one QSGMII main port is desired, mention the same main
>>> +      port twice.
>>
>> Two different forms for the same property name is not great. Just make a 
>> new property if you need something different.
> 
> Thank you for reviewing the patch. Based on the discussion for the
> previous series at [1], I had planned to reuse the same property
> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
> that the property represents the same feature on both devices which is
> that of the QSGMII main port. The only difference between the two of
> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
> while J721e can have up to two. Adding a new property which describes
> the same feature appears to be redundant to me. Please let me know.
> 

The trouble is that you wrote the description like it were two different
properties (for xx this is one element, for yy this is something else).
You need to describe the property in unified way.


Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

Hello Krzysztof,

On 19/09/22 15:45, Krzysztof Kozlowski wrote:
> On 14/09/2022 11:39, Siddharth Vadapalli wrote:
>> TI's J721e SoC supports additional PHY modes like QSGMII and SGMII
>> that are not supported on earlier SoCs. Add a compatible for it.
>>
>> Extend ti,qsgmii-main-ports property to support selection of upto
>> two main ports at once across the two QSGMII interfaces.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> ---
>>  .../bindings/phy/ti,phy-gmii-sel.yaml         | 52 ++++++++++++++++---
>>  1 file changed, 46 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> index da7cac537e15..1e19efab018b 100644
>> --- a/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> +++ b/Documentation/devicetree/bindings/phy/ti,phy-gmii-sel.yaml
>> @@ -54,6 +54,7 @@ properties:
>>        - ti,dm814-phy-gmii-sel
>>        - ti,am654-phy-gmii-sel
>>        - ti,j7200-cpsw5g-phy-gmii-sel
>> +      - ti,j721e-cpsw9g-phy-gmii-sel
>>  
>>    reg:
>>      maxItems: 1
>> @@ -65,12 +66,19 @@ properties:
>>      description: |
>>        Required only for QSGMII mode. Array to select the port for
>>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
>> -      main port with the rest of them being the QSGMII_SUB ports.
>> -    maxItems: 1
>> -    items:
>> -      minimum: 1
>> -      maximum: 4
> 
> minItems: 1
> maxItems: 2
>   items:
>     minimum: 1
>     maximum: 8

Thank you for reviewing the patch. I assume that you want me to mention
the values for "minItems", "maxItems", "minimum" and "maximum" here and
modify them later based on the compatible where applicable. I will do so
in the v2 series.

> 
>> +      ports automatically. For J7200 CPSW5G with the compatible:
>> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>> +      array of only one element, which is the port number ranging from
>> +      1 to 4. For J721e CPSW9G with the compatible:
>> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>> +      of two elements, which corresponds to two potential QSGMII main
>> +      ports. The first element and second element of the array can both
>> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
>> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
>> +      port and port 7 as the second QSGMII main port, we specify:
>> +      ti,qsgmii-main-ports = <2>, <7>;
>> +      If only one QSGMII main port is desired, mention the same main
>> +      port twice.
>>  
>>  allOf:
>>    - if:
>> @@ -81,12 +89,43 @@ allOf:
>>                - ti,dra7xx-phy-gmii-sel
>>                - ti,dm814-phy-gmii-sel
>>                - ti,am654-phy-gmii-sel
>> +              - ti,j7200-cpsw5g-phy-gmii-sel
>> +              - ti,j721e-cpsw9g-phy-gmii-sel
>>      then:
>>        properties:
>>          '#phy-cells':
>>            const: 1
>>            description: CPSW port number (starting from 1)
>>  
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,j7200-cpsw5g-phy-gmii-sel
>> +    then:
>> +      properties:
>> +        ti,qsgmii-main-ports:
>> +          maxItems: 1
>> +          items:
>> +            minimum: 1
>> +            maximum: 4
>> +
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - ti,j721e-cpsw9g-phy-gmii-sel
>> +    then:
>> +      properties:
>> +        ti,qsgmii-main-ports:
>> +          minItems: 2
>> +          maxItems: 2
>> +          items:
>> +            minimum: 1
>> +            maximum: 8
>> +
>>    - if:
>>        not:
>>          properties:
>> @@ -94,6 +133,7 @@ allOf:
>>              contains:
>>                enum:
>>                  - ti,j7200-cpsw5g-phy-gmii-sel
>> +                - ti,j721e-cpsw9g-phy-gmii-sel
>>      then:
>>        properties:
>>          ti,qsgmii-main-ports: false
> 
> This is interesting here... Did you test the bindings with your DTS?

Yes, I tried it out with different compatibles in the DTS file for the
node, making sure that the property "ti,qsgmii-main-ports" is allowed
only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
"ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
All of the rules within the "allOf", are enforced one after the other in
sequence, based on my testing. Please let me know in case of any
suggestions to implement it in a better way.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

Hello Krzysztof,

On 19/09/22 15:47, Krzysztof Kozlowski wrote:
> On 15/09/2022 07:28, Siddharth Vadapalli wrote:
>>>> @@ -65,12 +66,19 @@ properties:
>>>>      description: |
>>>>        Required only for QSGMII mode. Array to select the port for
>>>>        QSGMII main mode. Rest of the ports are selected as QSGMII_SUB
>>>> -      ports automatically. Any one of the 4 CPSW5G ports can act as the
>>>> -      main port with the rest of them being the QSGMII_SUB ports.
>>>> -    maxItems: 1
>>>> -    items:
>>>> -      minimum: 1
>>>> -      maximum: 4
>>>> +      ports automatically. For J7200 CPSW5G with the compatible:
>>>> +      ti,j7200-cpsw5g-phy-gmii-sel, ti,qsgmii-main-ports is an
>>>> +      array of only one element, which is the port number ranging from
>>>> +      1 to 4. For J721e CPSW9G with the compatible:
>>>> +      ti,j721e-cpsw9g-phy-gmii-sel, ti,qsgmii-main-ports is an array
>>>> +      of two elements, which corresponds to two potential QSGMII main
>>>> +      ports. The first element and second element of the array can both
>>>> +      range from 1 to 8 each, corresponding to two QSGMII main ports.
>>>> +      For J721e CPSW9G, to configure port 2 as the first QSGMII main
>>>> +      port and port 7 as the second QSGMII main port, we specify:
>>>> +      ti,qsgmii-main-ports = <2>, <7>;
>>>> +      If only one QSGMII main port is desired, mention the same main
>>>> +      port twice.
>>>
>>> Two different forms for the same property name is not great. Just make a 
>>> new property if you need something different.
>>
>> Thank you for reviewing the patch. Based on the discussion for the
>> previous series at [1], I had planned to reuse the same property
>> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
>> that the property represents the same feature on both devices which is
>> that of the QSGMII main port. The only difference between the two of
>> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
>> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
>> while J721e can have up to two. Adding a new property which describes
>> the same feature appears to be redundant to me. Please let me know.
>>
> 
> The trouble is that you wrote the description like it were two different
> properties (for xx this is one element, for yy this is something else).
> You need to describe the property in unified way.

Thank you for reviewing the patch. I plan to update the description to
the following:
"Required only for QSGMII mode. Array to select the port/s for QSGMII
main mode. The size of the array corresponds to the number of QSGMII
interfaces and thus, the number of distinct QSGMII main ports, supported
by the device. If the device supports two QSGMII interfaces but only one
QSGMII interface is desired, repeat the QSGMII main port value
corresponding to the QSGMII interface in the array."

I intend to describe the property in detail to help users understand the
property and its usage better. In the process, I might have
unintentionally made it appear as two different properties in the
previous description. I hope the new description shows that the property
describes the same feature across devices while making its usage clear
to the users at the same time. Please let me know if this is fine.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Krzysztof Kozlowski]

On 20/09/2022 06:56, Siddharth Vadapalli wrote:
>>> Thank you for reviewing the patch. Based on the discussion for the
>>> previous series at [1], I had planned to reuse the same property
>>> "ti,qsgmii-main-ports" for TI's J721e device too. The reason for this is
>>> that the property represents the same feature on both devices which is
>>> that of the QSGMII main port. The only difference between the two of
>>> them is that J7200's CPSW5G has 4 external ports while J721e's CPSW9G
>>> has 8 external ports. Thus, J7200 can have at most one QSGMII main port
>>> while J721e can have up to two. Adding a new property which describes
>>> the same feature appears to be redundant to me. Please let me know.
>>>
>>
>> The trouble is that you wrote the description like it were two different
>> properties (for xx this is one element, for yy this is something else).
>> You need to describe the property in unified way.
> 
> Thank you for reviewing the patch. I plan to update the description to
> the following:
> "Required only for QSGMII mode. Array to select the port/s for QSGMII
> main mode. The size of the array corresponds to the number of QSGMII
> interfaces and thus, the number of distinct QSGMII main ports, supported
> by the device. If the device supports two QSGMII interfaces but only one
> QSGMII interface is desired, repeat the QSGMII main port value
> corresponding to the QSGMII interface in the array."
> 
> I intend to describe the property in detail to help users understand the
> property and its usage better. In the process, I might have
> unintentionally made it appear as two different properties in the
> previous description. I hope the new description shows that the property
> describes the same feature across devices while making its usage clear
> to the users at the same time. Please let me know if this is fine.

Sounds good to me.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Krzysztof Kozlowski]

On 20/09/2022 06:27, Siddharth Vadapalli wrote:
> Hello Krzysztof,
>
>>> +    then:
>>> +      properties:
>>> +        ti,qsgmii-main-ports:
>>> +          minItems: 2
>>> +          maxItems: 2
>>> +          items:
>>> +            minimum: 1
>>> +            maximum: 8
>>> +
>>>    - if:
>>>        not:
>>>          properties:
>>> @@ -94,6 +133,7 @@ allOf:
>>>              contains:
>>>                enum:
>>>                  - ti,j7200-cpsw5g-phy-gmii-sel
>>> +                - ti,j721e-cpsw9g-phy-gmii-sel
>>>      then:
>>>        properties:
>>>          ti,qsgmii-main-ports: false
>>
>> This is interesting here... Did you test the bindings with your DTS?
> 
> Yes, I tried it out with different compatibles in the DTS file for the
> node, making sure that the property "ti,qsgmii-main-ports" is allowed
> only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
> "ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
> that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
> All of the rules within the "allOf", are enforced one after the other in
> sequence, based on my testing. Please let me know in case of any
> suggestions to implement it in a better way.

Great! I think I see now what I missed previously. The last hunk with
"ti,qsgmii-main-ports: false" is in a if: with negation ("not:")?

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: phy: ti: phy-gmii-sel: Add bindings for J721e

[Siddharth Vadapalli]

Hello Krzysztof,

On 21/09/22 12:09, Krzysztof Kozlowski wrote:
> On 20/09/2022 06:27, Siddharth Vadapalli wrote:
>> Hello Krzysztof,
>>
>>>> +    then:
>>>> +      properties:
>>>> +        ti,qsgmii-main-ports:
>>>> +          minItems: 2
>>>> +          maxItems: 2
>>>> +          items:
>>>> +            minimum: 1
>>>> +            maximum: 8
>>>> +
>>>>    - if:
>>>>        not:
>>>>          properties:
>>>> @@ -94,6 +133,7 @@ allOf:
>>>>              contains:
>>>>                enum:
>>>>                  - ti,j7200-cpsw5g-phy-gmii-sel
>>>> +                - ti,j721e-cpsw9g-phy-gmii-sel
>>>>      then:
>>>>        properties:
>>>>          ti,qsgmii-main-ports: false
>>>
>>> This is interesting here... Did you test the bindings with your DTS?
>>
>> Yes, I tried it out with different compatibles in the DTS file for the
>> node, making sure that the property "ti,qsgmii-main-ports" is allowed
>> only for the "ti,j7200-cpsw5g-phy-gmii-sel" and the
>> "ti,j721e-cpsw9g-phy-gmii-sel" compatibles. Additionally, I also tested
>> that the "minItems", "maxItems", "minimum" and "maximum" checks apply.
>> All of the rules within the "allOf", are enforced one after the other in
>> sequence, based on my testing. Please let me know in case of any
>> suggestions to implement it in a better way.
> 
> Great! I think I see now what I missed previously. The last hunk with
> "ti,qsgmii-main-ports: false" is in a if: with negation ("not:")?

Yes, the newly added compatible "ti,j721e-cpsw9g-phy-gmii-sel" is placed
within an "if:" followed by a "not:", along with the already existing
"ti,j7200-cpsw5g-phy-gmii-sel" compatible. With this,
"ti,qsgmii-main-ports" is allowed only for the aforementioned
compatibles and disallowed for the rest.

Regards,
Siddharth.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel