[PATCH net-next v2 0/3] Add support for VSC8531_02 PHY and DT RGMII tuning

[PATCH net-next v2 0/3] Add support for VSC8531_02 PHY and DT RGMII tuning

[Harini Katakam]

Add support for VSC8531_02 PHY ID.
Also provide an option to tune RGMII delay value via devicetree.
The default delays are retained in the driver.

v2 changes:
- Added patch to use a common vendor phy id match
- Removed dt include header patch because delays should be specied in
ps, not register values
- Updated DT binding description and commit for optional delay tuning to
be clearer on the precedence
- Updated dt property name to include vendor instead of phy device name
- Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
share the same model number
- Ensure RCT
- Improve optional property read

RFC link: https://lore.kernel.org/all/20210629094038.18610-1-harini.katakam@xilinx.com/

Harini Katakam (3):
  phy: mscc: Use PHY_ID_MATCH_VENDOR to minimize PHY ID table
  dt-bindings: mscc: Add RGMII RX and TX delay tuning
  phy: mscc: Add support for VSC8531_02 with RGMII tuning

 .../bindings/net/mscc-phy-vsc8531.txt         |  2 +
 drivers/net/phy/mscc/mscc.h                   |  3 ++
 drivers/net/phy/mscc/mscc_main.c              | 54 +++++++++++++------
 3 files changed, 42 insertions(+), 17 deletions(-)

-- 
2.17.1

[PATCH net-next v2 1/3] phy: mscc: Use PHY_ID_MATCH_VENDOR to minimize PHY ID table

[Harini Katakam]

All the PHY devices variants specified have the same mask and
hence can be simplified to one vendor look up for 0xfffffff0.
Any individual config can be identified by PHY_ID_MATCH_EXACT
in the respective structure.

Signed-off-by: Harini Katakam <harini.katakam@amd.com>
---
v2:
New patch

 drivers/net/phy/mscc/mscc_main.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 62bf99e45af1..75d9582e5784 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2656,19 +2656,7 @@ static struct phy_driver vsc85xx_driver[] = {
 module_phy_driver(vsc85xx_driver);
 
 static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
-	{ PHY_ID_VSC8504, 0xfffffff0, },
-	{ PHY_ID_VSC8514, 0xfffffff0, },
-	{ PHY_ID_VSC8530, 0xfffffff0, },
-	{ PHY_ID_VSC8531, 0xfffffff0, },
-	{ PHY_ID_VSC8540, 0xfffffff0, },
-	{ PHY_ID_VSC8541, 0xfffffff0, },
-	{ PHY_ID_VSC8552, 0xfffffff0, },
-	{ PHY_ID_VSC856X, 0xfffffff0, },
-	{ PHY_ID_VSC8572, 0xfffffff0, },
-	{ PHY_ID_VSC8574, 0xfffffff0, },
-	{ PHY_ID_VSC8575, 0xfffffff0, },
-	{ PHY_ID_VSC8582, 0xfffffff0, },
-	{ PHY_ID_VSC8584, 0xfffffff0, },
+	{ PHY_ID_MATCH_VENDOR(0xfffffff0) },
 	{ }
 };
 
-- 
2.17.1

[PATCH net-next v2 2/3] dt-bindings: mscc: Add RGMII RX and TX delay tuning

[Harini Katakam]

From: Harini Katakam <harini.katakam@xilinx.com>

Add optional properties to tune RGMII RX and TX delay. The current
default value in the Linux driver, when the phy-mode is rgmii-id,
is 2ns for both. These properties take priority if specified.

Signed-off-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
v2:
- Updated DT binding description and commit for optional delay tuning
to be clearer on the precedence
- Updated dt property name to include vendor instead of phy device name

 Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 0a3647fe331b..2b779bc3096b 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -33,6 +33,8 @@ Optional properties:
 			  VSC8531_DUPLEX_COLLISION (8).
 - load-save-gpios	: GPIO used for the load/save operation of the PTP
 			  hardware clock (PHC).
+- mscc,rx-delay		: RGMII RX delay. Allowed values are 0.2 - 3.4 ns.
+- mscc,tx-delay		: RGMII TX delay. Allowed values are 0.2 - 3.4 ns.
 
 
 Table: 1 - Edge rate change
-- 
2.17.1

[PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Harini Katakam]

From: Harini Katakam <harini.katakam@xilinx.com>

Add support for VSC8531_02 (Rev 2) device.
Add support for optional RGMII RX and TX delay tuning via devicetree.
The hierarchy is:
- Retain the defaul 0.2ns delay when RGMII tuning is not set.
- Retain the default 2ns delay when RGMII tuning is set and DT delay
property is NOT specified.
- Use the DT delay value when RGMII tuning is set and a DT delay
property is specified.

Signed-off-by: Harini Katakam <harini.katakam@amd.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
v2:
- Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
share the same model number
- Ensure RCT
- Improve optional property read

 drivers/net/phy/mscc/mscc.h      |  3 +++
 drivers/net/phy/mscc/mscc_main.c | 40 ++++++++++++++++++++++++++++----
 2 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..5a26eba0ace0 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -281,6 +281,7 @@ enum rgmii_clock_delay {
 #define PHY_ID_VSC8514			  0x00070670
 #define PHY_ID_VSC8530			  0x00070560
 #define PHY_ID_VSC8531			  0x00070570
+#define PHY_ID_VSC8531_02		  0x00070572
 #define PHY_ID_VSC8540			  0x00070760
 #define PHY_ID_VSC8541			  0x00070770
 #define PHY_ID_VSC8552			  0x000704e0
@@ -373,6 +374,8 @@ struct vsc8531_private {
 	 * package.
 	 */
 	unsigned int base_addr;
+	u32 rx_delay;
+	u32 tx_delay;
 
 #if IS_ENABLED(CONFIG_MACSEC)
 	/* MACsec fields:
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 75d9582e5784..80cc90a23d57 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
 {
 	u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
 	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
+	struct vsc8531_private *vsc8531 = phydev->priv;
 	u16 reg_val = 0;
 	int rc;
 
@@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
 
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
 	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
-		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos;
+		reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
 	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
 	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
-		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos;
+		reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
 
 	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
 			      rgmii_cntl,
@@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev)
 {
 	int rc, i, phy_id;
 	struct vsc8531_private *vsc8531 = phydev->priv;
+	struct device_node *of_node = phydev->mdio.dev.of_node;
+
+	vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
+	rc = of_property_read_u32(of_node, "mscc,rx-delay",
+				  &vsc8531->rx_delay);
+
+	vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
+	rc = of_property_read_u32(of_node, "mscc,tx-delay",
+				  &vsc8531->tx_delay);
 
 	rc = vsc85xx_default_config(phydev);
 	if (rc)
@@ -2413,9 +2423,8 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_stats      = &vsc85xx_get_stats,
 },
 {
-	.phy_id		= PHY_ID_VSC8531,
+	PHY_ID_MATCH_EXACT(PHY_ID_VSC8531),
 	.name		= "Microsemi VSC8531",
-	.phy_id_mask    = 0xfffffff0,
 	/* PHY_GBIT_FEATURES */
 	.soft_reset	= &genphy_soft_reset,
 	.config_init    = &vsc85xx_config_init,
@@ -2436,6 +2445,29 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_strings    = &vsc85xx_get_strings,
 	.get_stats      = &vsc85xx_get_stats,
 },
+{
+	PHY_ID_MATCH_EXACT(PHY_ID_VSC8531_02),
+	.name		= "Microsemi VSC8531-02",
+	/* PHY_GBIT_FEATURES */
+	.soft_reset	= &genphy_soft_reset,
+	.config_init	= &vsc85xx_config_init,
+	.config_aneg	= &vsc85xx_config_aneg,
+	.read_status	= &vsc85xx_read_status,
+	.handle_interrupt	= vsc85xx_handle_interrupt,
+	.config_intr	= &vsc85xx_config_intr,
+	.suspend	= &genphy_suspend,
+	.resume		= &genphy_resume,
+	.probe		= &vsc85xx_probe,
+	.set_wol	= &vsc85xx_wol_set,
+	.get_wol	= &vsc85xx_wol_get,
+	.get_tunable	= &vsc85xx_get_tunable,
+	.set_tunable	= &vsc85xx_set_tunable,
+	.read_page	= &vsc85xx_phy_read_page,
+	.write_page	= &vsc85xx_phy_write_page,
+	.get_sset_count = &vsc85xx_get_sset_count,
+	.get_strings	= &vsc85xx_get_strings,
+	.get_stats	= &vsc85xx_get_stats,
+},
 {
 	.phy_id		= PHY_ID_VSC8540,
 	.name		= "Microsemi FE VSC8540 SyncE",
-- 
2.17.1

Re: [PATCH net-next v2 2/3] dt-bindings: mscc: Add RGMII RX and TX delay tuning

[Vladimir Oltean]

On Wed, Apr 26, 2023 at 04:13:12PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> Add optional properties to tune RGMII RX and TX delay. The current
> default value in the Linux driver, when the phy-mode is rgmii-id,
> is 2ns for both. These properties take priority if specified.
> 
> Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> v2:
> - Updated DT binding description and commit for optional delay tuning
> to be clearer on the precedence
> - Updated dt property name to include vendor instead of phy device name
> 
>  Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 0a3647fe331b..2b779bc3096b 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -33,6 +33,8 @@ Optional properties:
>  			  VSC8531_DUPLEX_COLLISION (8).
>  - load-save-gpios	: GPIO used for the load/save operation of the PTP
>  			  hardware clock (PHC).
> +- mscc,rx-delay		: RGMII RX delay. Allowed values are 0.2 - 3.4 ns.
> +- mscc,tx-delay		: RGMII TX delay. Allowed values are 0.2 - 3.4 ns.

We have rx-internal-delay-ps and tx-internal-delay-ps in the common
ethernet-phy.yaml, there is no need for a custom property.

Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Vladimir Oltean]

On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> - Retain the default 2ns delay when RGMII tuning is set and DT delay
> property is NOT specified.
> - Use the DT delay value when RGMII tuning is set and a DT delay
> property is specified.
> 
> Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
> v2:
> - Switch both VSC8531 and VSC8531-02 to use exact phy id match as they
> share the same model number
> - Ensure RCT
> - Improve optional property read
> 
>  drivers/net/phy/mscc/mscc.h      |  3 +++
>  drivers/net/phy/mscc/mscc_main.c | 40 ++++++++++++++++++++++++++++----
>  2 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index a50235fdf7d9..5a26eba0ace0 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -281,6 +281,7 @@ enum rgmii_clock_delay {
>  #define PHY_ID_VSC8514			  0x00070670
>  #define PHY_ID_VSC8530			  0x00070560
>  #define PHY_ID_VSC8531			  0x00070570
> +#define PHY_ID_VSC8531_02		  0x00070572
>  #define PHY_ID_VSC8540			  0x00070760
>  #define PHY_ID_VSC8541			  0x00070770
>  #define PHY_ID_VSC8552			  0x000704e0
> @@ -373,6 +374,8 @@ struct vsc8531_private {
>  	 * package.
>  	 */
>  	unsigned int base_addr;
> +	u32 rx_delay;
> +	u32 tx_delay;
>  
>  #if IS_ENABLED(CONFIG_MACSEC)
>  	/* MACsec fields:
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 75d9582e5784..80cc90a23d57 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -525,6 +525,7 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>  {
>  	u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
>  	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
> +	struct vsc8531_private *vsc8531 = phydev->priv;
>  	u16 reg_val = 0;
>  	int rc;
>  
> @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>  
>  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
>  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> -		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_rx_delay_pos;
> +		reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
>  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
>  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> -		reg_val |= RGMII_CLK_DELAY_2_0_NS << rgmii_tx_delay_pos;
> +		reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
>  
>  	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
>  			      rgmii_cntl,
> @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device *phydev)
>  {
>  	int rc, i, phy_id;
>  	struct vsc8531_private *vsc8531 = phydev->priv;
> +	struct device_node *of_node = phydev->mdio.dev.of_node;
> +
> +	vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> +	rc = of_property_read_u32(of_node, "mscc,rx-delay",
> +				  &vsc8531->rx_delay);
> +
> +	vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
> +	rc = of_property_read_u32(of_node, "mscc,tx-delay",
> +				  &vsc8531->tx_delay);

Since the dt-bindings document says "If this property is present then
the PHY applies the RX|TX delay", then I guess the precedence as applied
by vsc85xx_rgmii_set_skews() should be different. The RX delays should
be applied based on rx-internal-delay-ps (if present) regardless of
phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid
or rgmii-id modes. Similar for tx.

Also, please split the VSC8531-02 addition into a separate patch, since
the configurable RGMII delays also affect existing PHYs (like VSC8502).

RE: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Katakam, Harini]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Wednesday, April 26, 2023 4:48 PM
> To: Katakam, Harini <harini.katakam@amd.com>
> Cc: robh+dt@kernel.org; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> edumazet@google.com; pabeni@redhat.com; wsa+renesas@sang-
> engineering.com; krzysztof.kozlowski+dt@linaro.org;
> simon.horman@corigine.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
> 
> On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> > From: Harini Katakam <harini.katakam@xilinx.com>
> >
> > Add support for VSC8531_02 (Rev 2) device.
> > Add support for optional RGMII RX and TX delay tuning via devicetree.
> > The hierarchy is:
> > - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> > - Retain the default 2ns delay when RGMII tuning is set and DT delay
> > property is NOT specified.
> > - Use the DT delay value when RGMII tuning is set and a DT delay
> > property is specified.
> >
> > Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> > Signed-off-by: Radhey Shyam Pandey
> <radhey.shyam.pandey@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
<snip>
> > @@ -532,10 +533,10 @@ static int vsc85xx_rgmii_set_skews(struct
> phy_device *phydev, u32 rgmii_cntl,
> >
> >  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID ||
> >  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > -		reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_rx_delay_pos;
> > +		reg_val |= vsc8531->rx_delay << rgmii_rx_delay_pos;
> >  	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID ||
> >  	    phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
> > -		reg_val |= RGMII_CLK_DELAY_2_0_NS <<
> rgmii_tx_delay_pos;
> > +		reg_val |= vsc8531->tx_delay << rgmii_tx_delay_pos;
> >
> >  	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> >  			      rgmii_cntl,
> > @@ -1812,6 +1813,15 @@ static int vsc85xx_config_init(struct phy_device
> *phydev)
> >  {
> >  	int rc, i, phy_id;
> >  	struct vsc8531_private *vsc8531 = phydev->priv;
> > +	struct device_node *of_node = phydev->mdio.dev.of_node;
> > +
> > +	vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> > +	rc = of_property_read_u32(of_node, "mscc,rx-delay",
> > +				  &vsc8531->rx_delay);
> > +
> > +	vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
> > +	rc = of_property_read_u32(of_node, "mscc,tx-delay",
> > +				  &vsc8531->tx_delay);
> 
> Since the dt-bindings document says "If this property is present then
> the PHY applies the RX|TX delay", then I guess the precedence as applied
> by vsc85xx_rgmii_set_skews() should be different. The RX delays should
> be applied based on rx-internal-delay-ps (if present) regardless of
> phy-mode, or set to RGMII_CLK_DELAY_2_0_NS if we are in the rgmii-rxid phy_get_internal_delay
> or rgmii-id modes. Similar for tx.

Thanks for the review.
The intention is to have the following precedence (I'll rephrase the commit if required)
-> If phy-mode is rgmii, current behavior persists for all devices
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices
(i.e. delay of RGMII_CLK_DELAY_2_0_NS)
-> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps
is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode)

I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode
in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like
the above is allowed. Please do let me know otherwise.

I will re-spin the series using phy_get_internal_delay.

Regards,
Harini

Re: [PATCH net-next v2 1/3] phy: mscc: Use PHY_ID_MATCH_VENDOR to minimize PHY ID table

[Andrew Lunn]

On Wed, Apr 26, 2023 at 04:13:11PM +0530, Harini Katakam wrote:
> All the PHY devices variants specified have the same mask and
> hence can be simplified to one vendor look up for 0xfffffff0.
> Any individual config can be identified by PHY_ID_MATCH_EXACT
> in the respective structure.
> 
> Signed-off-by: Harini Katakam <harini.katakam@amd.com>

net-next is closed at the moment, so you will need to report in two
weeks time.

> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 62bf99e45af1..75d9582e5784 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -2656,19 +2656,7 @@ static struct phy_driver vsc85xx_driver[] = {
>  module_phy_driver(vsc85xx_driver);
>  
>  static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
> -	{ PHY_ID_VSC8504, 0xfffffff0, },
> -	{ PHY_ID_VSC8514, 0xfffffff0, },
> -	{ PHY_ID_VSC8530, 0xfffffff0, },
> -	{ PHY_ID_VSC8531, 0xfffffff0, },
> -	{ PHY_ID_VSC8540, 0xfffffff0, },
> -	{ PHY_ID_VSC8541, 0xfffffff0, },
> -	{ PHY_ID_VSC8552, 0xfffffff0, },
> -	{ PHY_ID_VSC856X, 0xfffffff0, },
> -	{ PHY_ID_VSC8572, 0xfffffff0, },
> -	{ PHY_ID_VSC8574, 0xfffffff0, },
> -	{ PHY_ID_VSC8575, 0xfffffff0, },
> -	{ PHY_ID_VSC8582, 0xfffffff0, },
> -	{ PHY_ID_VSC8584, 0xfffffff0, },
> +	{ PHY_ID_MATCH_VENDOR(0xfffffff0) },

The vendor ID is 0xfffffff0 ???

    Andrew

Re: [PATCH net-next v2 2/3] dt-bindings: mscc: Add RGMII RX and TX delay tuning

[Andrew Lunn]

On Wed, Apr 26, 2023 at 04:13:12PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> Add optional properties to tune RGMII RX and TX delay. The current
> default value in the Linux driver, when the phy-mode is rgmii-id,
> is 2ns for both. These properties take priority if specified.
> 
> Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Your signed-off-by needs to come last, since you are submitting it.

But as Vladimir pointed out, you don't need to add anything.

You could however convert to yaml, if you want.

    Andrew

Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Andrew Lunn]

On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> - Retain the default 2ns delay when RGMII tuning is set and DT delay
> property is NOT specified.

tuning is probably the wrong word here. I normally consider tuning as
small changes from 0ns/2ns. The course setting of 0ns or 2ns is not
tuning. I normally use RGMII internal delays to refer to that.

However, i'm not sure there is consistency among drivers.

	 Andrew

Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Vladimir Oltean]

On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote:
> Thanks for the review.
> The intention is to have the following precedence (I'll rephrase the commit if required)
> -> If phy-mode is rgmii, current behavior persists for all devices
> -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior persists for all devices
> (i.e. delay of RGMII_CLK_DELAY_2_0_NS)
> -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND rx-internal-delay-ps/tx-internal-delay-ps
> is defined, then the value from DT is considered instead of 2ns. (NOT irrespective of phy-mode)
> 
> I'm checking the phy drivers that use phy_get_internal_delay and the description phy-mode
> in ethernet-controller.yaml and rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like
> the above is allowed. Please do let me know otherwise.

I understood what your intention was. What I meant was:

 phy-mode                       rgmii                          rgmii-rxid/rgmii-id
 --------------------------------------------------------------------------------------------
 rx-internal-delay-ps absent    0.2 ns                         2 ns
 rx-internal-delay-ps present   follow rx-internal-delay-ps    follow rx-internal-delay-ps

I agree with Andrew that probably there isn't consistency among PHY
drivers for this interpretation - see dp83822 vs intel-xway for example.
My interpretation was based on the wording from the dt-bindings document,
which seems to suggest that rx-internal-delay-ps takes precedence.

Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Simon Horman]

On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> From: Harini Katakam <harini.katakam@xilinx.com>
> 
> Add support for VSC8531_02 (Rev 2) device.
> Add support for optional RGMII RX and TX delay tuning via devicetree.
> The hierarchy is:
> - Retain the defaul 0.2ns delay when RGMII tuning is not set.

nit: s/defaul/default/

...

RE: [PATCH net-next v2 1/3] phy: mscc: Use PHY_ID_MATCH_VENDOR to minimize PHY ID table

[Katakam, Harini]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, April 26, 2023 7:10 PM
> To: Katakam, Harini <harini.katakam@amd.com>
> Cc: robh+dt@kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; edumazet@google.com;
> pabeni@redhat.com; vladimir.oltean@nxp.com; wsa+renesas@sang-
> engineering.com; krzysztof.kozlowski+dt@linaro.org;
> simon.horman@corigine.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Subject: Re: [PATCH net-next v2 1/3] phy: mscc: Use
> PHY_ID_MATCH_VENDOR to minimize PHY ID table
> 
> On Wed, Apr 26, 2023 at 04:13:11PM +0530, Harini Katakam wrote:
> > All the PHY devices variants specified have the same mask and hence
> > can be simplified to one vendor look up for 0xfffffff0.
> > Any individual config can be identified by PHY_ID_MATCH_EXACT in the
> > respective structure.
> >
> > Signed-off-by: Harini Katakam <harini.katakam@amd.com>
> 
<snip>
> > +	{ PHY_ID_MATCH_VENDOR(0xfffffff0) },
> 
> The vendor ID is 0xfffffff0 ???

Sorry this is supposed to be 0x00070400 (Microsemi OUI).
Will fix in the next version.

Regards,
Harini

RE: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Katakam, Harini]

Hi Andrew,

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Wednesday, April 26, 2023 7:18 PM
> To: Katakam, Harini <harini.katakam@amd.com>
> Cc: robh+dt@kernel.org; hkallweit1@gmail.com; linux@armlinux.org.uk;
> davem@davemloft.net; kuba@kernel.org; edumazet@google.com;
> pabeni@redhat.com; vladimir.oltean@nxp.com; wsa+renesas@sang-
> engineering.com; krzysztof.kozlowski+dt@linaro.org;
> simon.horman@corigine.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
> 
> On Wed, Apr 26, 2023 at 04:13:13PM +0530, Harini Katakam wrote:
> > From: Harini Katakam <harini.katakam@xilinx.com>
> >
> > Add support for VSC8531_02 (Rev 2) device.
> > Add support for optional RGMII RX and TX delay tuning via devicetree.
> > The hierarchy is:
> > - Retain the defaul 0.2ns delay when RGMII tuning is not set.
> > - Retain the default 2ns delay when RGMII tuning is set and DT delay
> > property is NOT specified.
> 
> tuning is probably the wrong word here. I normally consider tuning as small
> changes from 0ns/2ns. The course setting of 0ns or 2ns is not tuning. I
> normally use RGMII internal delays to refer to that.
> 
> However, i'm not sure there is consistency among drivers.

Thanks for the input, I understand. I'm planning on re-phrasing this
with phy-mode and property values to describe rather than say tuning.

Regards,
Harini

RE: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Katakam, Harini]

Hi Vladimir,

> -----Original Message-----
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Sent: Wednesday, April 26, 2023 7:49 PM
> To: Katakam, Harini <harini.katakam@amd.com>
> Cc: robh+dt@kernel.org; andrew@lunn.ch; hkallweit1@gmail.com;
> linux@armlinux.org.uk; davem@davemloft.net; kuba@kernel.org;
> edumazet@google.com; pabeni@redhat.com; wsa+renesas@sang-
> engineering.com; krzysztof.kozlowski+dt@linaro.org;
> simon.horman@corigine.com; netdev@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> harinikatakamlinux@gmail.com; Simek, Michal <michal.simek@amd.com>;
> Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Subject: Re: [PATCH net-next v2 3/3] phy: mscc: Add support for VSC8531_02
> with RGMII tuning
> 
> On Wed, Apr 26, 2023 at 12:21:47PM +0000, Katakam, Harini wrote:
> > Thanks for the review.
> > The intention is to have the following precedence (I'll rephrase the
> > commit if required)
> > -> If phy-mode is rgmii, current behavior persists for all devices If
> > -> phy-mode is rgmii-id/rgmii-rxid/rgmii-txid, current behavior
> > -> persists for all devices
> > (i.e. delay of RGMII_CLK_DELAY_2_0_NS)
> > -> If phy-mode is rgmii-id/rgmii-rxid/rgmii-txid AND
> > -> rx-internal-delay-ps/tx-internal-delay-ps
> > is defined, then the value from DT is considered instead of 2ns. (NOT
> > irrespective of phy-mode)
> >
> > I'm checking the phy drivers that use phy_get_internal_delay and the
> > description phy-mode in ethernet-controller.yaml and
> > rx/tx-internal-delay-ps in ethernet-phy.yaml. It does look like the above is
> allowed. Please do let me know otherwise.
> 
> I understood what your intention was. What I meant was:
> 
>  phy-mode                       rgmii                          rgmii-rxid/rgmii-id
>  --------------------------------------------------------------------------------------------
>  rx-internal-delay-ps absent    0.2 ns                         2 ns
>  rx-internal-delay-ps present   follow rx-internal-delay-ps    follow rx-internal-
> delay-ps
> 
> I agree with Andrew that probably there isn't consistency among PHY drivers
> for this interpretation - see dp83822 vs intel-xway for example.

Thanks, yes I noticed the difference here and also in older PHY drivers that
used custom properties (see dp83867 which is what I originally aligned it to).
But the table you mentioned above makes sense; I'll re-spin accordingly.

> My interpretation was based on the wording from the dt-bindings document,
> which seems to suggest that rx-internal-delay-ps takes precedence.

OK I understand.

Regards,
Harini