[PATCH net 0/3] net: phy: mscc: support VSC8501

[PATCH net 0/3] net: phy: mscc: support VSC8501

[David Epping]

Hello,

this series of patches adds support for the VSC8501 Ethernet PHY and
fixes support for the VSC8502 PHY in RGMII mode (see below for
discussion).

The first patch simply adds the VSC8502 to the MODULE_DEVICE_TABLE,
where I guess it was unintentionally missing. I have no hardware to
test my change.

The second patch adds the VSC8501 PHY with exactly the same driver
implementation as the existing VSC8502. Note that for at least RGMII
mode this patch is not sufficient to operate the PHY, but likely the
existing code was not sufficient for VSC8502, either.

The third patch fixes RGMII mode operation for the VSC8501 (I have
tested this on hardware) and very likely also the VSC8502, which share
the same description of relevant registers in the datasheet.
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/VSC8501-03_Datasheet_60001741A.PDF
https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/DataSheets/VSC8502-03_Datasheet_60001742B.pdf
Table 4-42 "RGMII CONTROL, ADDRESS 20E2 (0X14)" Bit 11 for each of
them.

By default the RX_CLK is disabled in both PHYs. This results in no
received packets being handed to the MAC. The patch enables this
clock.
Since I can only test RGMII mode, and the register is called RGMII,
my patch is limited to the RGMII mode. However, according to
Microchip support (case number 01268776) this applies to all modes
using the RX_CLK (which is all modes?).
Since the VSC8502 shares the same description, this would however mean
the existing code for VSC8502 could have never worked.
Is that possible? Has someone used VSC8502 successfully?

Other PHYs sharing the same basic code, like VSC8530/31/40/41 don't
have the clock disabled and the bit 11 is reserved for them.
Hence the check for PHY ID.

Should the uncertainty about GMII and MII modes be a source code
comment? Or in the commit message? Or not mentioned at all?

Thanks for your feedback,
David

David Epping (3):
  net: phy: mscc: add VSC8502 to MODULE_DEVICE_TABLE
  net: phy: mscc: add support for VSC8501
  net: phy: mscc: enable VSC8501/2 RGMII RX clock

 drivers/net/phy/mscc/mscc.h      |  2 ++
 drivers/net/phy/mscc/mscc_main.c | 50 ++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

-- 
2.17.1

[PATCH net 1/3] net: phy: mscc: add VSC8502 to MODULE_DEVICE_TABLE

[David Epping]

The mscc driver implements support for VSC8502, so its ID should be in
the MODULE_DEVICE_TABLE for automatic loading.

Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
---
 drivers/net/phy/mscc/mscc_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 62bf99e45af1..bd81a4b041e5 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2656,6 +2656,7 @@ static struct phy_driver vsc85xx_driver[] = {
 module_phy_driver(vsc85xx_driver);
 
 static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
+	{ PHY_ID_VSC8502, 0xfffffff0, },
 	{ PHY_ID_VSC8504, 0xfffffff0, },
 	{ PHY_ID_VSC8514, 0xfffffff0, },
 	{ PHY_ID_VSC8530, 0xfffffff0, },
-- 
2.17.1

[PATCH net 2/3] net: phy: mscc: add support for VSC8501

[David Epping]

The VSC8501 PHY can use the same driver implementation as the VSC8502.
Adding the PHY ID and copying the handler functions of VSC8502 is
sufficient to operate it.

Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
---
 drivers/net/phy/mscc/mscc.h      |  1 +
 drivers/net/phy/mscc/mscc_main.c | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index a50235fdf7d9..79cbb2418664 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -276,6 +276,7 @@ enum rgmii_clock_delay {
 /* Microsemi PHY ID's
  *   Code assumes lowest nibble is 0
  */
+#define PHY_ID_VSC8501			  0x00070530
 #define PHY_ID_VSC8502			  0x00070630
 #define PHY_ID_VSC8504			  0x000704c0
 #define PHY_ID_VSC8514			  0x00070670
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index bd81a4b041e5..29fc27a16805 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -2316,6 +2316,30 @@ static int vsc85xx_probe(struct phy_device *phydev)
 
 /* Microsemi VSC85xx PHYs */
 static struct phy_driver vsc85xx_driver[] = {
+{
+	.phy_id		= PHY_ID_VSC8501,
+	.name		= "Microsemi GE VSC8501 SyncE",
+	.phy_id_mask	= 0xfffffff0,
+	/* PHY_BASIC_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_VSC8502,
 	.name		= "Microsemi GE VSC8502 SyncE",
@@ -2656,6 +2680,7 @@ static struct phy_driver vsc85xx_driver[] = {
 module_phy_driver(vsc85xx_driver);
 
 static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
+	{ PHY_ID_VSC8501, 0xfffffff0, },
 	{ PHY_ID_VSC8502, 0xfffffff0, },
 	{ PHY_ID_VSC8504, 0xfffffff0, },
 	{ PHY_ID_VSC8514, 0xfffffff0, },
-- 
2.17.1

[PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

By default the VSC8501 and VSC8502 RGMII RX clock output is disabled.
To allow packet forwarding towards the MAC it needs to be enabled.
The same may be necessary for GMII and MII modes, but that's currently
unclear.

For VSC853x and VSC854x the respective disable bit is reserved and the
clock output is enabled by default.

Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
---
 drivers/net/phy/mscc/mscc.h      |  1 +
 drivers/net/phy/mscc/mscc_main.c | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
index 79cbb2418664..defe5cc6d4fc 100644
--- a/drivers/net/phy/mscc/mscc.h
+++ b/drivers/net/phy/mscc/mscc.h
@@ -179,6 +179,7 @@ enum rgmii_clock_delay {
 #define VSC8502_RGMII_CNTL		  20
 #define VSC8502_RGMII_RX_DELAY_MASK	  0x0070
 #define VSC8502_RGMII_TX_DELAY_MASK	  0x0007
+#define VSC8502_RGMII_RX_CLK_DISABLE	  0x0800
 
 #define MSCC_PHY_WOL_LOWER_MAC_ADDR	  21
 #define MSCC_PHY_WOL_MID_MAC_ADDR	  22
diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
index 29fc27a16805..c7a8f5561c66 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -547,6 +547,26 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
 	return rc;
 }
 
+/* For VSC8501 and VSC8502 the RGMII RX clock output is disabled by default. */
+static int vsc85xx_rgmii_enable_rx_clk(struct phy_device *phydev,
+				       u32 rgmii_cntl)
+{
+	int rc, phy_id;
+
+	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
+	if (PHY_ID_VSC8501 != phy_id && PHY_ID_VSC8502 != phy_id)
+		return 0;
+
+	mutex_lock(&phydev->lock);
+
+	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, rgmii_cntl,
+			      VSC8502_RGMII_RX_CLK_DISABLE, 0);
+
+	mutex_unlock(&phydev->lock);
+
+	return rc;
+}
+
 static int vsc85xx_default_config(struct phy_device *phydev)
 {
 	int rc;
@@ -559,6 +579,10 @@ static int vsc85xx_default_config(struct phy_device *phydev)
 					     VSC8502_RGMII_TX_DELAY_MASK);
 		if (rc)
 			return rc;
+
+		rc = vsc85xx_rgmii_enable_rx_clk(phydev, VSC8502_RGMII_CNTL);
+		if (rc)
+			return rc;
 	}
 
 	return 0;
-- 
2.17.1

Re: [PATCH net 1/3] net: phy: mscc: add VSC8502 to MODULE_DEVICE_TABLE

[Vladimir Oltean]

On Sat, May 20, 2023 at 06:06:01PM +0200, David Epping wrote:
> The mscc driver implements support for VSC8502, so its ID should be in
> the MODULE_DEVICE_TABLE for automatic loading.
> 
> Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
> ---

Fixes: d3169863310d ("net: phy: mscc: add support for VSC8502")
Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Please carry these tags with future revisions of the patches.

Re: [PATCH net 2/3] net: phy: mscc: add support for VSC8501

[Vladimir Oltean]

On Sat, May 20, 2023 at 06:06:02PM +0200, David Epping wrote:
> The VSC8501 PHY can use the same driver implementation as the VSC8502.
> Adding the PHY ID and copying the handler functions of VSC8502 is
> sufficient to operate it.
> 
> Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sat, May 20, 2023 at 06:06:03PM +0200, David Epping wrote:
> By default the VSC8501 and VSC8502 RGMII RX clock output is disabled.
> To allow packet forwarding towards the MAC it needs to be enabled.
> The same may be necessary for GMII and MII modes, but that's currently
> unclear.
> 
> For VSC853x and VSC854x the respective disable bit is reserved and the
> clock output is enabled by default.
> 
> Signed-off-by: David Epping <david.epping@missinglinkelectronics.com>
> ---
>  drivers/net/phy/mscc/mscc.h      |  1 +
>  drivers/net/phy/mscc/mscc_main.c | 24 ++++++++++++++++++++++++
>  2 files changed, 25 insertions(+)
> 
> diff --git a/drivers/net/phy/mscc/mscc.h b/drivers/net/phy/mscc/mscc.h
> index 79cbb2418664..defe5cc6d4fc 100644
> --- a/drivers/net/phy/mscc/mscc.h
> +++ b/drivers/net/phy/mscc/mscc.h
> @@ -179,6 +179,7 @@ enum rgmii_clock_delay {
>  #define VSC8502_RGMII_CNTL		  20
>  #define VSC8502_RGMII_RX_DELAY_MASK	  0x0070
>  #define VSC8502_RGMII_TX_DELAY_MASK	  0x0007
> +#define VSC8502_RGMII_RX_CLK_DISABLE	  0x0800
>  
>  #define MSCC_PHY_WOL_LOWER_MAC_ADDR	  21
>  #define MSCC_PHY_WOL_MID_MAC_ADDR	  22
> diff --git a/drivers/net/phy/mscc/mscc_main.c b/drivers/net/phy/mscc/mscc_main.c
> index 29fc27a16805..c7a8f5561c66 100644
> --- a/drivers/net/phy/mscc/mscc_main.c
> +++ b/drivers/net/phy/mscc/mscc_main.c
> @@ -547,6 +547,26 @@ static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
>  	return rc;
>  }
>  
> +/* For VSC8501 and VSC8502 the RGMII RX clock output is disabled by default. */

This statement is not exactly true, proven by my board where I've just
printed these values:

[    6.454638] Microsemi GE VSC8502 SyncE 0000:00:00.3:03: vsc85xx_rgmii_enable_rx_clk: RGMII_CNTL 0x44, RX_CLK_DISABLE 0x0
[    6.544652] sja1105 spi2.2 sw2p0 (uninitialized): PHY [0000:00:00.3:03] driver [Microsemi GE VSC8502 SyncE] (irq=POLL)
[    6.630864] Microsemi GE VSC8502 SyncE 0000:00:00.3:02: vsc85xx_rgmii_enable_rx_clk: RGMII_CNTL 0x44, RX_CLK_DISABLE 0x0
[    6.720218] sja1105 spi2.2 sw2p1 (uninitialized): PHY [0000:00:00.3:02] driver [Microsemi GE VSC8502 SyncE] (irq=POLL)
[    6.806876] Microsemi GE VSC8502 SyncE 0000:00:00.3:11: vsc85xx_rgmii_enable_rx_clk: RGMII_CNTL 0x44, RX_CLK_DISABLE 0x0
[    6.896185] sja1105 spi2.2 sw2p2 (uninitialized): PHY [0000:00:00.3:11] driver [Microsemi GE VSC8502 SyncE] (irq=POLL)
[    6.982775] Microsemi GE VSC8502 SyncE 0000:00:00.3:10: vsc85xx_rgmii_enable_rx_clk: RGMII_CNTL 0x44, RX_CLK_DISABLE 0x0
[    7.071988] sja1105 spi2.2 sw2p3 (uninitialized): PHY [0000:00:00.3:10] driver [Microsemi GE VSC8502 SyncE] (irq=POLL)

Let's resolve that difference before the patches are merged, and write
some correct comments.

I agree that the datasheet is not clear, but I think that the RX_CLK
output is enabled or not based on the strapping of the RCVRDCLK1 and
RCVRDCLK2 pins. Coincidentally, these are also muxed with PHYADD1 and
PHYADD2, so the default value of RX_CLK_DISABLE might depend on the
PHY address (?!).

What is your PHY address? Mine are 0x10 and 0x11 for the VSC8502 on my
board.

Not saying that the patch is wrong or that the resolution should be any
different than it is. Just that it's clear we can't both be right, and
my PHYs clearly work (re-tested just now).

--
pw-bot: changes-requested

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sun, May 21, 2023 at 03:35:12PM +0300, Vladimir Oltean wrote:
> Let's resolve that difference before the patches are merged, and write
> some correct comments.
> 
> I agree that the datasheet is not clear, but I think that the RX_CLK
> output is enabled or not based on the strapping of the RCVRDCLK1 and
> RCVRDCLK2 pins. Coincidentally, these are also muxed with PHYADD1 and
> PHYADD2, so the default value of RX_CLK_DISABLE might depend on the
> PHY address (?!).
> 
> What is your PHY address? Mine are 0x10 and 0x11 for the VSC8502 on my
> board.
> 
> Not saying that the patch is wrong or that the resolution should be any
> different than it is. Just that it's clear we can't both be right, and
> my PHYs clearly work (re-tested just now).
> 
> --
> pw-bot: changes-requested

Ah, no, I think the explanation is much simpler. I see the datasheet
mentions that "RX_CLK output disable" is a sticky bit, which means it
preserves its value across a reset.

In my case, it is the U-Boot driver which clears that setting, as part
of configuring RGMII delays.
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mscc.c#L1553

Re: [PATCH net 0/3] net: phy: mscc: support VSC8501

[Vladimir Oltean]

On Sat, May 20, 2023 at 06:06:00PM +0200, David Epping wrote:
> Since I can only test RGMII mode, and the register is called RGMII,
> my patch is limited to the RGMII mode. However, according to
> Microchip support (case number 01268776) this applies to all modes
> using the RX_CLK (which is all modes?).

I logged into my Microchip support account, but it looks like I can only
view cases which are mine. If they say that bit 11 applies to all PHY
modes where the PHY drives RX_CLK, that would mean MII, GMII and RGMII.

> Since the VSC8502 shares the same description, this would however mean
> the existing code for VSC8502 could have never worked.
> Is that possible? Has someone used VSC8502 successfully?

Yup (with U-Boot pre-initialization though). Thanks for the investigation.

> Other PHYs sharing the same basic code, like VSC8530/31/40/41 don't
> have the clock disabled and the bit 11 is reserved for them.
> Hence the check for PHY ID.
> 
> Should the uncertainty about GMII and MII modes be a source code
> comment? Or in the commit message? Or not mentioned at all?

I think we'd be better off moving the vsc85xx_rgmii_enable_rx_clk() call
outside the "if phy_interface_mode_is_rgmii(phydev->interface)" block,
if that's what Microchip support seems to suggest.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sat, May 20, 2023 at 06:06:03PM +0200, David Epping wrote:
> +/* For VSC8501 and VSC8502 the RGMII RX clock output is disabled by default. */
> +static int vsc85xx_rgmii_enable_rx_clk(struct phy_device *phydev,
> +				       u32 rgmii_cntl)
> +{
> +	int rc, phy_id;
> +
> +	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> +	if (PHY_ID_VSC8501 != phy_id && PHY_ID_VSC8502 != phy_id)
> +		return 0;

Not only bit 11 is reserved for VSC8530, but it's also read-only, so it
should not matter what is written there.

Since vsc85xx_rgmii_enable_rx_clk() and vsc85xx_rgmii_set_skews() write
to the same register, would it not make sense to combine the two into a
single phy_modify_paged() call, and to zeroize bit 11 as part of that?

The other caller of vsc85xx_rgmii_set_skews(), VSC8572, unfortunately
does not document bit 11 at all - it doesn't say if it's read-only or not.
We could conditionally include the VSC8502_RGMII_RX_CLK_DISABLE bit in the
"mask" argument of phy_modify_paged() based on rgmii_cntl == VSC8502_RGMII_CNTL,
such as to exclude VSC8572.

What do you think?

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

On Sun, May 21, 2023 at 04:12:26PM +0300, Vladimir Oltean wrote:
> Ah, no, I think the explanation is much simpler. I see the datasheet
> mentions that "RX_CLK output disable" is a sticky bit, which means it
> preserves its value across a reset.
> 
> In my case, it is the U-Boot driver which clears that setting, as part
> of configuring RGMII delays.
> https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/net/phy/mscc.c#L1553

Thanks for investigating and checking on your hardware.
Yes, my U-Boot does not support VSC850x yet, so Linux is the first
touching the registers.
For completeness: My PHY address is 0.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

On Sun, May 21, 2023 at 04:43:56PM +0300, Vladimir Oltean wrote:
> Not only bit 11 is reserved for VSC8530, but it's also read-only, so it
> should not matter what is written there.

I agree and am ok with removing the PHY ID condition.

> Since vsc85xx_rgmii_enable_rx_clk() and vsc85xx_rgmii_set_skews() write
> to the same register, would it not make sense to combine the two into a
> single phy_modify_paged() call, and to zeroize bit 11 as part of that?

Since we found an explanation why the current driver works in some
setups (U-Boot), I would go with the Microchip support statement, that
writing bit 11 to 0 is required in all modes.
It would thus stay a separate function, called without a phy mode
condition, and not be combined with the RGMII skew setting function.

> The other caller of vsc85xx_rgmii_set_skews(), VSC8572, unfortunately
> does not document bit 11 at all - it doesn't say if it's read-only or not.
> We could conditionally include the VSC8502_RGMII_RX_CLK_DISABLE bit in the
> "mask" argument of phy_modify_paged() based on rgmii_cntl == VSC8502_RGMII_CNTL,
> such as to exclude VSC8572.

Because of the above, I would still call from vsc85xx_default_config(),
so not for the PHYs where bit 11 is not documented.

> What do you think?

If you agree to the above, should the function be named
vsc85xx_enable_rx_clk() or rather vsc8502_enable_rx_clk()?
It is called for more than just VSC8502, but not for all of the PHYs
the driver supports.
The same is true for the existing vsc85xx_default_config(), however.
I don't have a real preference.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Russell King (Oracle)]

On Sat, May 20, 2023 at 06:06:03PM +0200, David Epping wrote:
> +static int vsc85xx_rgmii_enable_rx_clk(struct phy_device *phydev,
> +				       u32 rgmii_cntl)
> +{
> +	int rc, phy_id;
> +
> +	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> +	if (PHY_ID_VSC8501 != phy_id && PHY_ID_VSC8502 != phy_id)
> +		return 0;

As you are accessing the phy_id in the phy_driver struct, isn't it
already true that this will be initialised to constants such as
PHY_ID_VSC8501 or PHY_ID_VSC8502? In which case, why would you need
to mask it with drv->phy_id_mask ?

> +
> +	mutex_lock(&phydev->lock);
> +
> +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, rgmii_cntl,
> +			      VSC8502_RGMII_RX_CLK_DISABLE, 0);
> +
> +	mutex_unlock(&phydev->lock);

What is the purpose of taking this lock? phy_modify_paged() will do its
read-modify-write access and page accesses under the MDIO bus lock,
which should be all that's required to guarantee an atomic update.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sun, May 21, 2023 at 06:16:50PM +0200, David Epping wrote:
> On Sun, May 21, 2023 at 04:43:56PM +0300, Vladimir Oltean wrote:
> > Not only bit 11 is reserved for VSC8530, but it's also read-only, so it
> > should not matter what is written there.
> 
> I agree and am ok with removing the PHY ID condition.
> 
> > Since vsc85xx_rgmii_enable_rx_clk() and vsc85xx_rgmii_set_skews() write
> > to the same register, would it not make sense to combine the two into a
> > single phy_modify_paged() call, and to zeroize bit 11 as part of that?
> 
> Since we found an explanation why the current driver works in some
> setups (U-Boot), I would go with the Microchip support statement, that
> writing bit 11 to 0 is required in all modes.
> It would thus stay a separate function, called without a phy mode
> condition, and not be combined with the RGMII skew setting function.
> 
> > The other caller of vsc85xx_rgmii_set_skews(), VSC8572, unfortunately
> > does not document bit 11 at all - it doesn't say if it's read-only or not.
> > We could conditionally include the VSC8502_RGMII_RX_CLK_DISABLE bit in the
> > "mask" argument of phy_modify_paged() based on rgmii_cntl == VSC8502_RGMII_CNTL,
> > such as to exclude VSC8572.
> 
> Because of the above, I would still call from vsc85xx_default_config(),
> so not for the PHYs where bit 11 is not documented.
> 
> > What do you think?
> 
> If you agree to the above, should the function be named
> vsc85xx_enable_rx_clk() or rather vsc8502_enable_rx_clk()?
> It is called for more than just VSC8502, but not for all of the PHYs
> the driver supports.
> The same is true for the existing vsc85xx_default_config(), however.
> I don't have a real preference.

Well, to be clear, I was suggesting:

/* Set the RGMII RX and TX clock skews individually, according to the PHY
 * interface type, to:
 *  * 0.2 ns (their default, and lowest, hardware value) if delays should
 *    not be enabled
 *  * 2.0 ns (which causes the data to be sampled at exactly half way between
 *    clock transitions at 1000 Mbps) if delays should be enabled
 */
static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
				   u16 rgmii_rx_delay_mask,
				   u16 rgmii_tx_delay_mask)
{
	u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
	u16 mask = rgmii_rx_delay_mask | rgmii_tx_delay_mask;
	u16 reg_val = 0;
	int rc;

	/* For traffic to pass, the VSC8502 family needs the RX_CLK disable bit
	 * to be unset for all PHY modes, so do that as part of the paged
	 * register modification.
	 */
	if (rgmii_cntl == VSC8502_RGMII_CNTL)
		mask |= VSC8502_RGMII_RX_CLK_DISABLE;

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

	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
			      rgmii_cntl, mask, reg_val);

	mutex_unlock(&phydev->lock);

	return rc;
}

static int vsc85xx_default_config(struct phy_device *phydev)
{
	int rc;

	phydev->mdix_ctrl = ETH_TP_MDI_AUTO;

	return vsc85xx_rgmii_set_skews(phydev, VSC8502_RGMII_CNTL,
				       VSC8502_RGMII_RX_DELAY_MASK,
				       VSC8502_RGMII_TX_DELAY_MASK);
}

// no changes to the vsc85xx_rgmii_set_skews() call from vsc8584_config_init()

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Mon, May 22, 2023 at 12:49:44PM +0300, Vladimir Oltean wrote:
> Well, to be clear, I was suggesting:
> 
> /* Set the RGMII RX and TX clock skews individually, according to the PHY
>  * interface type, to:
>  *  * 0.2 ns (their default, and lowest, hardware value) if delays should
>  *    not be enabled
>  *  * 2.0 ns (which causes the data to be sampled at exactly half way between
>  *    clock transitions at 1000 Mbps) if delays should be enabled
>  */
> static int vsc85xx_rgmii_set_skews(struct phy_device *phydev, u32 rgmii_cntl,
> 				   u16 rgmii_rx_delay_mask,
> 				   u16 rgmii_tx_delay_mask)
> {
> 	u16 rgmii_rx_delay_pos = ffs(rgmii_rx_delay_mask) - 1;
> 	u16 rgmii_tx_delay_pos = ffs(rgmii_tx_delay_mask) - 1;
> 	u16 mask = rgmii_rx_delay_mask | rgmii_tx_delay_mask;
> 	u16 reg_val = 0;
> 	int rc;

Or rather:

	u16 mask = 0;

	if (phy_interface_is_rgmii(phydev))
		mask |= rgmii_rx_delay_mask | rgmii_tx_delay_mask;

	if (rgmii_cntl == VSC8502_RGMII_CNTL)
		mask |= VSC8502_RGMII_RX_CLK_DISABLE;

> 
> 	/* For traffic to pass, the VSC8502 family needs the RX_CLK disable bit
> 	 * to be unset for all PHY modes, so do that as part of the paged
> 	 * register modification.
> 	 */
> 	if (rgmii_cntl == VSC8502_RGMII_CNTL)
> 		mask |= VSC8502_RGMII_RX_CLK_DISABLE;
> 
> 	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;
> 	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;
> 
> 	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2,
> 			      rgmii_cntl, mask, reg_val);
> 
> 	mutex_unlock(&phydev->lock);
> 
> 	return rc;
> }

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Sun, May 21, 2023 at 06:16:50PM +0200, David Epping wrote:
> Since we found an explanation why the current driver works in some
> setups (U-Boot), I would go with the Microchip support statement, that
> writing bit 11 to 0 is required in all modes.
> It would thus stay a separate function, called without a phy mode
> condition, and not be combined with the RGMII skew setting function.

If you still prefer to write twice in a row to the same paged register
instead of combining the changes, then fine by me, it's not a huge deal.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

On Sun, May 21, 2023 at 06:59:53PM +0100, Russell King (Oracle) wrote:
> On Sat, May 20, 2023 at 06:06:03PM +0200, David Epping wrote:
> > +static int vsc85xx_rgmii_enable_rx_clk(struct phy_device *phydev,
> > +				       u32 rgmii_cntl)
> > +{
> > +	int rc, phy_id;
> > +
> > +	phy_id = phydev->drv->phy_id & phydev->drv->phy_id_mask;
> > +	if (PHY_ID_VSC8501 != phy_id && PHY_ID_VSC8502 != phy_id)
> > +		return 0;
> 
> As you are accessing the phy_id in the phy_driver struct, isn't it
> already true that this will be initialised to constants such as
> PHY_ID_VSC8501 or PHY_ID_VSC8502? In which case, why would you need
> to mask it with drv->phy_id_mask ?

Yes you are right. I copied the code from the vsc85xx_config_init()
function in the same driver, but the extra masking is not necessary.
It seems to be the phy_id in the struct phy_device which is read from
the MDIO bus and thus needs masking. phy_id in struct phy_driver seems
compile time defined and already masked.
I'll adjust my patch.

> > +
> > +	mutex_lock(&phydev->lock);
> > +
> > +	rc = phy_modify_paged(phydev, MSCC_PHY_PAGE_EXTENDED_2, rgmii_cntl,
> > +			      VSC8502_RGMII_RX_CLK_DISABLE, 0);
> > +
> > +	mutex_unlock(&phydev->lock);
> 
> What is the purpose of taking this lock? phy_modify_paged() will do its
> read-modify-write access and page accesses under the MDIO bus lock,
> which should be all that's required to guarantee an atomic update.

Yes, I copied this from the vsc85xx_rgmii_set_skews() function in the
same driver accessing the same register in the same context.
But maybe it is used there because of repeated phydev->interface
accesses, which may otherwise change during function execution?
I'll remove the locks from my patch.

Thanks for your feedback.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

On Mon, May 22, 2023 at 12:58:33PM +0300, Vladimir Oltean wrote:
> If you still prefer to write twice in a row to the same paged register
> instead of combining the changes, then fine by me, it's not a huge deal.

Since the clock enablement now happens in all modes the existing rgmii
function name seems misleading to me. Also we don't want to enable for
all PHY types, and the differentiation is already available at the
caller. I would thus opt for a separate function and fewer conditional
statements.

Its my first patch re-submission, so sorry for the noob question:
Should I include your "pw-bot: changes-requested" tag with the third
patch? Probably not.
Of course I'll include your tags for patches 1 and 2.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Andrew Lunn]

> Its my first patch re-submission, so sorry for the noob question:
> Should I include your "pw-bot: changes-requested" tag with the third
> patch? Probably not.

No.

There is a robot listening to emails, and when it sees pw-bot: It uses
the label to change the state of the patch in patchworks:

https://patchwork.kernel.org/project/netdevbpf/list/

The robot does have some basic authentication, so it should actually
ignore such a line from you, since you are not a Maintainer. But even
so, you don't want to make your own new patches as needing changes.

    Andrew

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Mon, May 22, 2023 at 04:00:57PM +0200, David Epping wrote:
> On Mon, May 22, 2023 at 12:58:33PM +0300, Vladimir Oltean wrote:
> > If you still prefer to write twice in a row to the same paged register
> > instead of combining the changes, then fine by me, it's not a huge deal.
> 
> Since the clock enablement now happens in all modes the existing rgmii
> function name seems misleading to me.

To be fair, it's only as misleading as the datasheet name for the register
that holds this field, "RGMII CONTROL". Anyway, the function could be
renamed as necessary to be less confusing: vsc85xx_update_rgmii_ctrl()
or something along those lines.

MDIO reads and writes are not exactly the quickest I/O in the world, and
having 2 read-modify-write consecutive accesses to the same paged
register (which in turn implies indirect access) just because readability
seems like the type of thing that can play its part in deteriorating
boot time latency. Maybe we can deal with the readability some other way.

> Also we don't want to enable for
> all PHY types, and the differentiation is already available at the
> caller. I would thus opt for a separate function and fewer conditional
> statements.

I don't understand this. We don't? For what PHY types don't we want to
enable the RX_CLK?

> Its my first patch re-submission, so sorry for the noob question:
> Should I include your "pw-bot: changes-requested" tag with the third
> patch? Probably not.

Nope.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[David Epping]

On Mon, May 22, 2023 at 06:11:04PM +0300, Vladimir Oltean wrote:
> On Mon, May 22, 2023 at 04:00:57PM +0200, David Epping wrote:
> > Since the clock enablement now happens in all modes the existing rgmii
> > function name seems misleading to me.
> 
> To be fair, it's only as misleading as the datasheet name for the register
> that holds this field, "RGMII CONTROL". Anyway, the function could be
> renamed as necessary to be less confusing: vsc85xx_update_rgmii_ctrl()
> or something along those lines.
> 
> MDIO reads and writes are not exactly the quickest I/O in the world, and
> having 2 read-modify-write consecutive accesses to the same paged
> register (which in turn implies indirect access) just because readability
> seems like the type of thing that can play its part in deteriorating
> boot time latency. Maybe we can deal with the readability some other way.

You are right. It's an easy job for the CPU and saves time for
hardware access. I'll prepare and send a new patch set.

> > Also we don't want to enable for
> > all PHY types, and the differentiation is already available at the
> > caller. I would thus opt for a separate function and fewer conditional
> > statements.
> 
> I don't understand this. We don't? For what PHY types don't we want to
> enable the RX_CLK?

I meant all PHYs using vsc85xx_rgmii_set_skews() via vsc8584_config_init().
As you pointed out they don't have a clear definition of what bit 11 means
for them.
But we can easily differentiate using the condition you suggested.
I'll do that for the new patch version.

Re: [PATCH net 3/3] net: phy: mscc: enable VSC8501/2 RGMII RX clock

[Vladimir Oltean]

On Mon, May 22, 2023 at 05:22:21PM +0200, David Epping wrote:
> > > Also we don't want to enable for
> > > all PHY types, and the differentiation is already available at the
> > > caller. I would thus opt for a separate function and fewer conditional
> > > statements.
> > 
> > I don't understand this. We don't? For what PHY types don't we want to
> > enable the RX_CLK?
> 
> I meant all PHYs using vsc85xx_rgmii_set_skews() via vsc8584_config_init().

Aha. By "PHY types" I thought you mean "PHY interface types" like RGMII/GMII/MII,
not PHY device models.

> As you pointed out they don't have a clear definition of what bit 11 means
> for them.

And I wasn't suggesting to make bit 11 part of the modified "mask" for them.

> But we can easily differentiate using the condition you suggested.
> I'll do that for the new patch version.

Sounds good, thanks.