[PATCH 0/3] Add support for VSC8531_02 PHY and DT RGMII tuning

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

Harini Katakam (3):
  include: dt-bindings: Add mscc-vsc8531 RGMII clock delay definitions
  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         |  9 +++-
 drivers/net/phy/mscc/mscc.h                   |  3 ++
 drivers/net/phy/mscc/mscc_main.c              | 41 ++++++++++++++++++-
 include/dt-bindings/net/mscc-phy-vsc8531.h    |  9 ++++
 4 files changed, 59 insertions(+), 3 deletions(-)

-- 
2.17.1

[PATCH 1/3] include: dt-bindings: Add mscc-vsc8531 RGMII clock delay definitions

[Harini Katakam]

Add RGMII clock delay definitions in mscc-phy-vsc8531 header.

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 include/dt-bindings/net/mscc-phy-vsc8531.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/dt-bindings/net/mscc-phy-vsc8531.h b/include/dt-bindings/net/mscc-phy-vsc8531.h
index 9eb2ec2b2ea9..242eb4c7ede9 100644
--- a/include/dt-bindings/net/mscc-phy-vsc8531.h
+++ b/include/dt-bindings/net/mscc-phy-vsc8531.h
@@ -28,4 +28,13 @@
 #define VSC8531_FORCE_LED_OFF           14
 #define VSC8531_FORCE_LED_ON            15
 
+#define VSC8531_RGMII_CLK_DELAY_0_2_NS	0
+#define VSC8531_RGMII_CLK_DELAY_0_8_NS	1
+#define VSC8531_RGMII_CLK_DELAY_1_1_NS	2
+#define VSC8531_RGMII_CLK_DELAY_1_7_NS	3
+#define VSC8531_RGMII_CLK_DELAY_2_0_NS	4
+#define VSC8531_RGMII_CLK_DELAY_2_3_NS	5
+#define VSC8531_RGMII_CLK_DELAY_2_6_NS	6
+#define VSC8531_RGMII_CLK_DELAY_3_4_NS	7
+
 #endif
-- 
2.17.1

Re: [PATCH 1/3] include: dt-bindings: Add mscc-vsc8531 RGMII clock delay definitions

[Andrew Lunn]

On Tue, Jun 29, 2021 at 03:10:36PM +0530, Harini Katakam wrote:
> Add RGMII clock delay definitions in mscc-phy-vsc8531 header.
> 
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  include/dt-bindings/net/mscc-phy-vsc8531.h | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/include/dt-bindings/net/mscc-phy-vsc8531.h b/include/dt-bindings/net/mscc-phy-vsc8531.h
> index 9eb2ec2b2ea9..242eb4c7ede9 100644
> --- a/include/dt-bindings/net/mscc-phy-vsc8531.h
> +++ b/include/dt-bindings/net/mscc-phy-vsc8531.h
> @@ -28,4 +28,13 @@
>  #define VSC8531_FORCE_LED_OFF           14
>  #define VSC8531_FORCE_LED_ON            15
>  
> +#define VSC8531_RGMII_CLK_DELAY_0_2_NS	0
> +#define VSC8531_RGMII_CLK_DELAY_0_8_NS	1
> +#define VSC8531_RGMII_CLK_DELAY_1_1_NS	2
> +#define VSC8531_RGMII_CLK_DELAY_1_7_NS	3
> +#define VSC8531_RGMII_CLK_DELAY_2_0_NS	4
> +#define VSC8531_RGMII_CLK_DELAY_2_3_NS	5
> +#define VSC8531_RGMII_CLK_DELAY_2_6_NS	6
> +#define VSC8531_RGMII_CLK_DELAY_3_4_NS	7

Using defines like this is no longer used. Please specify the delay in
pS and convert to a register value within the driver.

   Andrew

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

[Harini Katakam]

Add optional properties to tune RGMII RX and TX delay.

Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 .../devicetree/bindings/net/mscc-phy-vsc8531.txt         | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
index 87a27d775d48..e201d24d8e27 100644
--- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
+++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
@@ -33,7 +33,14 @@ Optional properties:
 			  VSC8531_DUPLEX_COLLISION (8).
 - load-save-gpios	: GPIO used for the load/save operation of the PTP
 			  hardware clock (PHC).
-
+- vsc8531,rx-delay	: RGMII RX delay. Allowed values are defined in
+			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
+			  Default value, set by the driver is
+			  VSC8531_RGMII_CLK_DELAY_2_0_NS.
+- vsc8531,tx-delay	: RGMII TX delay. Allowed values are defined in
+			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
+			  Default value, set by the driver is
+			  VSC8531_RGMII_CLK_DELAY_2_0_NS.
 
 Table: 1 - Edge rate change
 ----------------------------------------------------------------|
-- 
2.17.1

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

[Andrew Lunn]

On Tue, Jun 29, 2021 at 03:10:37PM +0530, Harini Katakam wrote:
> Add optional properties to tune RGMII RX and TX delay.
> 
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  .../devicetree/bindings/net/mscc-phy-vsc8531.txt         | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 87a27d775d48..e201d24d8e27 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -33,7 +33,14 @@ Optional properties:
>  			  VSC8531_DUPLEX_COLLISION (8).
>  - load-save-gpios	: GPIO used for the load/save operation of the PTP
>  			  hardware clock (PHC).
> -
> +- vsc8531,rx-delay	: RGMII RX delay. Allowed values are defined in
> +			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
> +			  Default value, set by the driver is
> +			  VSC8531_RGMII_CLK_DELAY_2_0_NS.
> +- vsc8531,tx-delay	: RGMII TX delay. Allowed values are defined in
> +			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
> +			  Default value, set by the driver is
> +			  VSC8531_RGMII_CLK_DELAY_2_0_NS.

The default values need better explanation. So you are saying they are
only used when 'rgmii' is not used. And they replace the default 2ns
delay, they don't add to the default 2ns delay.

       Andrew

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

[Harini Katakam]

Hi Andrew,

On Tue, Jun 29, 2021 at 7:37 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Tue, Jun 29, 2021 at 03:10:37PM +0530, Harini Katakam wrote:
> > Add optional properties to tune RGMII RX and TX delay.
> >
> > Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >  .../devicetree/bindings/net/mscc-phy-vsc8531.txt         | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > index 87a27d775d48..e201d24d8e27 100644
> > --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> > @@ -33,7 +33,14 @@ Optional properties:
> >                         VSC8531_DUPLEX_COLLISION (8).
> >  - load-save-gpios    : GPIO used for the load/save operation of the PTP
> >                         hardware clock (PHC).
> > -
> > +- vsc8531,rx-delay   : RGMII RX delay. Allowed values are defined in
> > +                       "include/dt-bindings/net/mscc-phy-vsc8531.h".
> > +                       Default value, set by the driver is
> > +                       VSC8531_RGMII_CLK_DELAY_2_0_NS.
> > +- vsc8531,tx-delay   : RGMII TX delay. Allowed values are defined in
> > +                       "include/dt-bindings/net/mscc-phy-vsc8531.h".
> > +                       Default value, set by the driver is
> > +                       VSC8531_RGMII_CLK_DELAY_2_0_NS.
>
> The default values need better explanation. So you are saying they are
> only used when 'rgmii' is not used. And they replace the default 2ns
> delay, they don't add to the default 2ns delay.

Thanks for the review. Yes, I'm saying that they replace the default 2ns delay.
But they only come in when rgmii or rgmii-id is used. When that's not used,
the default 0.2ns in the driver is retained. I'll update the DT description here
to be clear.

Regards,
Harini

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

[Rob Herring]

On Tue, Jun 29, 2021 at 03:10:37PM +0530, Harini Katakam wrote:
> Add optional properties to tune RGMII RX and TX delay.
> 
> Signed-off-by: Harini Katakam <harini.katakam@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>

Your S-o-b should be last as the last one to touch this.

> ---
>  .../devicetree/bindings/net/mscc-phy-vsc8531.txt         | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> index 87a27d775d48..e201d24d8e27 100644
> --- a/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> +++ b/Documentation/devicetree/bindings/net/mscc-phy-vsc8531.txt
> @@ -33,7 +33,14 @@ Optional properties:
>  			  VSC8531_DUPLEX_COLLISION (8).
>  - load-save-gpios	: GPIO used for the load/save operation of the PTP
>  			  hardware clock (PHC).
> -
> +- vsc8531,rx-delay	: RGMII RX delay. Allowed values are defined in

'vsc8531' is not a vendor. The form is <vendor>,<propname>.

> +			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
> +			  Default value, set by the driver is
> +			  VSC8531_RGMII_CLK_DELAY_2_0_NS.
> +- vsc8531,tx-delay	: RGMII TX delay. Allowed values are defined in
> +			  "include/dt-bindings/net/mscc-phy-vsc8531.h".
> +			  Default value, set by the driver is
> +			  VSC8531_RGMII_CLK_DELAY_2_0_NS.
>  
>  Table: 1 - Edge rate change
>  ----------------------------------------------------------------|
> -- 
> 2.17.1
> 
> 

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

[Harini Katakam]

Hi Rob,

Thanks for the review and sorry I din't reply earlier.
<snip>
> > +- vsc8531,rx-delay   : RGMII RX delay. Allowed values are defined in
>
> 'vsc8531' is not a vendor. The form is <vendor>,<propname>.

I was just aligning it with existing optional property names in this document.
Could you please let me know if mscc,rx-delay is more appropriate?

Regards,
Harini

[PATCH 3/3] phy: mscc: Add support for VSC8531_02 with RGMII tuning

[Harini Katakam]

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@xilinx.com>
Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
 drivers/net/phy/mscc/mscc.h      |  3 +++
 drivers/net/phy/mscc/mscc_main.c | 41 ++++++++++++++++++++++++++++++--
 2 files changed, 42 insertions(+), 2 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 6e32da28e138..2b501824c802 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -535,15 +535,16 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
 	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
 	u16 reg_val = 0;
 	int rc;
+	struct vsc8531_private *vsc8531 = phydev->priv;
 
 	mutex_lock(&phydev->lock);
 
 	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,
@@ -1820,6 +1821,17 @@ 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;
+
+	rc = of_property_read_u32(of_node, "vsc8531,rx-delay",
+				  &vsc8531->rx_delay);
+	if (rc < 0)
+		vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
+
+	rc = of_property_read_u32(of_node, "vsc8531,tx-delay",
+				  &vsc8531->tx_delay);
+	if (rc < 0)
+		vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
 
 	rc = vsc85xx_default_config(phydev);
 	if (rc)
@@ -2444,6 +2456,30 @@ static struct phy_driver vsc85xx_driver[] = {
 	.get_strings    = &vsc85xx_get_strings,
 	.get_stats      = &vsc85xx_get_stats,
 },
+{
+	.phy_id		= PHY_ID_VSC8531_02,
+	.name		= "Microsemi VSC8531-02",
+	.phy_id_mask	= 0xfffffff0,
+	/* 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",
@@ -2668,6 +2704,7 @@ static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
 	{ PHY_ID_VSC8514, 0xfffffff0, },
 	{ PHY_ID_VSC8530, 0xfffffff0, },
 	{ PHY_ID_VSC8531, 0xfffffff0, },
+	{ PHY_ID_VSC8531_02, 0xfffffff0, },
 	{ PHY_ID_VSC8540, 0xfffffff0, },
 	{ PHY_ID_VSC8541, 0xfffffff0, },
 	{ PHY_ID_VSC8552, 0xfffffff0, },
-- 
2.17.1

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

[Heiner Kallweit]

On 29.06.2021 11:40, Harini Katakam wrote:
> 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@xilinx.com>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>  drivers/net/phy/mscc/mscc.h      |  3 +++
>  drivers/net/phy/mscc/mscc_main.c | 41 ++++++++++++++++++++++++++++++--
>  2 files changed, 42 insertions(+), 2 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 6e32da28e138..2b501824c802 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -535,15 +535,16 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>  	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
>  	u16 reg_val = 0;
>  	int rc;
> +	struct vsc8531_private *vsc8531 = phydev->priv;
>  
>  	mutex_lock(&phydev->lock);
>  
>  	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,
> @@ -1820,6 +1821,17 @@ 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;
> +
> +	rc = of_property_read_u32(of_node, "vsc8531,rx-delay",
> +				  &vsc8531->rx_delay);
> +	if (rc < 0)
> +		vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
> +
> +	rc = of_property_read_u32(of_node, "vsc8531,tx-delay",
> +				  &vsc8531->tx_delay);
> +	if (rc < 0)
> +		vsc8531->tx_delay = RGMII_CLK_DELAY_2_0_NS;
>  
>  	rc = vsc85xx_default_config(phydev);
>  	if (rc)
> @@ -2444,6 +2456,30 @@ static struct phy_driver vsc85xx_driver[] = {
>  	.get_strings    = &vsc85xx_get_strings,
>  	.get_stats      = &vsc85xx_get_stats,
>  },
> +{
> +	.phy_id		= PHY_ID_VSC8531_02,
> +	.name		= "Microsemi VSC8531-02",
> +	.phy_id_mask	= 0xfffffff0,

VSC8531 and VSC8531-02 share the same model number, therefore you
would have to switch both PHY drivers to exact match.
phy_id_mask = 0xffffffff, or better use macro PHY_ID_MATCH_EXACT().


> +	/* 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",
> @@ -2668,6 +2704,7 @@ static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
>  	{ PHY_ID_VSC8514, 0xfffffff0, },
>  	{ PHY_ID_VSC8530, 0xfffffff0, },
>  	{ PHY_ID_VSC8531, 0xfffffff0, },
> +	{ PHY_ID_VSC8531_02, 0xfffffff0, },

Effectively this is the same as the line before. Maybe it would make sense
to change this table in a follow-up patch to just one entry covering all
PHY ID's with the vendor part being Microsemi, e.g. using macro
PHY_ID_MATCH_VENDOR().

>  	{ PHY_ID_VSC8540, 0xfffffff0, },
>  	{ PHY_ID_VSC8541, 0xfffffff0, },
>  	{ PHY_ID_VSC8552, 0xfffffff0, },
> 

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

[Harini Katakam]

Hi Heiner,

On Tue, Jun 29, 2021 at 4:11 PM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 29.06.2021 11:40, Harini Katakam wrote:
> > 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@xilinx.com>
> > Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@xilinx.com>
> > Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> > ---
> >  drivers/net/phy/mscc/mscc.h      |  3 +++
> >  drivers/net/phy/mscc/mscc_main.c | 41 ++++++++++++++++++++++++++++++--
> >  2 files changed, 42 insertions(+), 2 deletions(-)
> >
<snip>
>  {
> >       .phy_id         = PHY_ID_VSC8540,
> >       .name           = "Microsemi FE VSC8540 SyncE",
> > @@ -2668,6 +2704,7 @@ static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
> >       { PHY_ID_VSC8514, 0xfffffff0, },
> >       { PHY_ID_VSC8530, 0xfffffff0, },
> >       { PHY_ID_VSC8531, 0xfffffff0, },
> > +     { PHY_ID_VSC8531_02, 0xfffffff0, },
>
> Effectively this is the same as the line before. Maybe it would make sense
> to change this table in a follow-up patch to just one entry covering all
> PHY ID's with the vendor part being Microsemi, e.g. using macro
> PHY_ID_MATCH_VENDOR().

Thanks for the review. Let me check and test - I have a couple of these parts.
Will try to include the patch in this series.

Regards,
Harini

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

[Andrew Lunn]

> @@ -535,15 +535,16 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>  	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
>  	u16 reg_val = 0;
>  	int rc;
> +	struct vsc8531_private *vsc8531 = phydev->priv;
  
reverse christmass tree.


>  	mutex_lock(&phydev->lock);
>  
>  	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,
> @@ -1820,6 +1821,17 @@ 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;
> +
> +	rc = of_property_read_u32(of_node, "vsc8531,rx-delay",
> +				  &vsc8531->rx_delay);
> +	if (rc < 0)
> +		vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;

of_property_read_u32() is guaranteed not to touch the result value, if
it is not in device tree. So you can simplify this to:

	vsc8531->rx_delay = RGMII_CLK_DELAY_2_0_NS;
	of_property_read_u32(of_node, "vsc8531,rx-delay", &vsc8531->rx_delay);