[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

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 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

[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

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 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

[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 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 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

[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

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

[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

[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/

...