| Message ID | 20230520160603.32458-4-david.epping@missinglinkelectronics.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: mscc: support VSC8501 | expand |
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
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
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?
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.
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.
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.
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()
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; > }
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.
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.
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.
> 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
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.
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.
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.
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;
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(+)