diff mbox series

[net] net: ethernet: renesas: rswitch Fix PHY station management clock setting

Message ID 20230925003416.3863560-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [net] net: ethernet: renesas: rswitch Fix PHY station management clock setting | expand

Commit Message

Yoshihiro Shimoda Sept. 25, 2023, 12:34 a.m. UTC 
From: Tam Nguyen <tam.nguyen.xa@renesas.com>

Fix the MPIC.PSMCS value following the programming example in the
section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
S4 Hardware User Manual Rev.1.00.

The value is calculated by
    MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
Kit board.

Fixes: 3590918b5d07 ("net: ethernet: renesas: Add support for "Ethernet Switch"")
Signed-off-by: Tam Nguyen <tam.nguyen.xa@renesas.com>
Signed-off-by: Hai Pham <hai.pham.ud@renesas.com>
[shimoda: Revise subject/commit description and add Fixes tag]
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Tested-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/net/ethernet/renesas/rswitch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andrew Lunn Sept. 25, 2023, 2:17 p.m. UTC | #1 
On Mon, Sep 25, 2023 at 09:34:16AM +0900, Yoshihiro Shimoda wrote:
> From: Tam Nguyen <tam.nguyen.xa@renesas.com>
> 
> Fix the MPIC.PSMCS value following the programming example in the
> section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> S4 Hardware User Manual Rev.1.00.
> 
> The value is calculated by
>     MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
> with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
> Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
> Kit board.

If you run this calculation backwards, what frequency does
MPIC_PSMCS(0x3f) map to?

Is 320MHz really fixed? For all silicon variants? Is it possible to do
a clk_get_rate() on a clock to get the actual clock rate?

	Andrew
Geert Uytterhoeven Sept. 26, 2023, 7:18 a.m. UTC | #2 
On Tue, Sep 26, 2023 at 8:45 AM Andrew Lunn <andrew@lunn.ch> wrote:
> On Mon, Sep 25, 2023 at 09:34:16AM +0900, Yoshihiro Shimoda wrote:
> > From: Tam Nguyen <tam.nguyen.xa@renesas.com>
> >
> > Fix the MPIC.PSMCS value following the programming example in the
> > section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> > S4 Hardware User Manual Rev.1.00.
> >
> > The value is calculated by
> >     MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
> > with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
> > Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
> > Kit board.
>
> If you run this calculation backwards, what frequency does
> MPIC_PSMCS(0x3f) map to?
>
> Is 320MHz really fixed? For all silicon variants? Is it possible to do
> a clk_get_rate() on a clock to get the actual clock rate?

With debugfs enabled, one can just look at /sys/kernel/debug/clk/clk_summary.

Gr{oetje,eeting}s,

                        Geert
Yoshihiro Shimoda Sept. 26, 2023, 7:21 a.m. UTC | #3 
Hello Andrew,

> From: Andrew Lunn, Sent: Monday, September 25, 2023 11:18 PM
> 
> On Mon, Sep 25, 2023 at 09:34:16AM +0900, Yoshihiro Shimoda wrote:
> > From: Tam Nguyen <tam.nguyen.xa@renesas.com>
> >
> > Fix the MPIC.PSMCS value following the programming example in the
> > section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> > S4 Hardware User Manual Rev.1.00.
> >
> > The value is calculated by
> >     MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
> > with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
> > Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
> > Kit board.
> 
> If you run this calculation backwards, what frequency does
> MPIC_PSMCS(0x3f) map to?

Thank you for your review! I completely misunderstood the formula. In
other words, the formula cannot calculate backwards. The correct
formula is:

MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1

> Is 320MHz really fixed? For all silicon variants? Is it possible to do
> a clk_get_rate() on a clock to get the actual clock rate?

320MHz is really fixed on the current existing all silicon variants.
Yes, it is possible to do a clk_get_rate() on a clock to get the actual
clock rate. So, I'll use clk_get_rate() on v2.

Best regards,
Yoshihiro Shimoda

> 	Andrew
Andrew Lunn Sept. 26, 2023, 12:47 p.m. UTC | #4 
On Tue, Sep 26, 2023 at 07:21:59AM +0000, Yoshihiro Shimoda wrote:
> Hello Andrew,
> 
> > From: Andrew Lunn, Sent: Monday, September 25, 2023 11:18 PM
> > 
> > On Mon, Sep 25, 2023 at 09:34:16AM +0900, Yoshihiro Shimoda wrote:
> > > From: Tam Nguyen <tam.nguyen.xa@renesas.com>
> > >
> > > Fix the MPIC.PSMCS value following the programming example in the
> > > section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> > > S4 Hardware User Manual Rev.1.00.
> > >
> > > The value is calculated by
> > >     MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
> > > with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
> > > Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
> > > Kit board.
> > 
> > If you run this calculation backwards, what frequency does
> > MPIC_PSMCS(0x3f) map to?
> 
> Thank you for your review! I completely misunderstood the formula. In
> other words, the formula cannot calculate backwards. The correct
> formula is:
> 
> MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1
> 
> > Is 320MHz really fixed? For all silicon variants? Is it possible to do
> > a clk_get_rate() on a clock to get the actual clock rate?
> 
> 320MHz is really fixed on the current existing all silicon variants.
> Yes, it is possible to do a clk_get_rate() on a clock to get the actual
> clock rate. So, I'll use clk_get_rate() on v2.

Was the original version tested?

I've run Marvell PHYs are 5Mhz, sometimes 6MHz. This is within spec as
given by the datasheet, even if IEEE 802.3 says 2.5Mhz is the max.

Now if MPIC_PSMCS(0x3f) maps to 20MHz or more, it could never of
worked, which makes me think the clock has changed. If it maps to
6Mhz, yes it could of worked with some PHY but not others, and the
clock might not of changed.

      Andrew
Yoshihiro Shimoda Sept. 27, 2023, 12:35 a.m. UTC | #5 
Hello Andrew,

> From: Andrew Lunn, Sent: Tuesday, September 26, 2023 9:48 PM
> 
> On Tue, Sep 26, 2023 at 07:21:59AM +0000, Yoshihiro Shimoda wrote:
> > Hello Andrew,
> >
> > > From: Andrew Lunn, Sent: Monday, September 25, 2023 11:18 PM
> > >
> > > On Mon, Sep 25, 2023 at 09:34:16AM +0900, Yoshihiro Shimoda wrote:
> > > > From: Tam Nguyen <tam.nguyen.xa@renesas.com>
> > > >
> > > > Fix the MPIC.PSMCS value following the programming example in the
> > > > section 6.4.2 Management Data Clock (MDC) Setting, Ethernet MAC IP,
> > > > S4 Hardware User Manual Rev.1.00.
> > > >
> > > > The value is calculated by
> > > >     MPIC.PSMCS = clk[MHz] / ((MDC frequency[MHz] + 1) * 2)
> > > > with the input clock frequency of 320MHz and MDC frequency of 2.5MHz.
> > > > Otherwise, this driver cannot communicate PHYs on the R-Car S4 Starter
> > > > Kit board.
> > >
> > > If you run this calculation backwards, what frequency does
> > > MPIC_PSMCS(0x3f) map to?
> >
> > Thank you for your review! I completely misunderstood the formula. In
> > other words, the formula cannot calculate backwards. The correct
> > formula is:
> >
> > MPIC.PSMCS = clk[MHz] / (MDC frequency[MHz] * 2) - 1
> >
> > > Is 320MHz really fixed? For all silicon variants? Is it possible to do
> > > a clk_get_rate() on a clock to get the actual clock rate?
> >
> > 320MHz is really fixed on the current existing all silicon variants.
> > Yes, it is possible to do a clk_get_rate() on a clock to get the actual
> > clock rate. So, I'll use clk_get_rate() on v2.
> 
> Was the original version tested?

Yes, the original version was tested on Spider board.
The original version's MDC frequency was 25MHz.
And the PHY (Marvell 88E2110) on Spider board can use such frequency,
IIUC because the MDC period is 35 ns (so 28.57143MHz).

However, I don't know why this setting cannot work on the Starter Kit board
because the board also has the same PHY. I guess that this is related to
board design, especially voltage of I/O (Spider = 1.8V, Starter Kit = 3.3V).

Anyway, changing the MDC frequency from 25MHz to 2.5MHz works correctly on
both Spider and Starter Kit. So, I would like to apply the v3 patch [1] for safe.

[1] https://lore.kernel.org/all/20230926123054.3976752-1-yoshihiro.shimoda.uh@renesas.com/

> I've run Marvell PHYs are 5Mhz, sometimes 6MHz. This is within spec as
> given by the datasheet, even if IEEE 802.3 says 2.5Mhz is the max.
> 
> Now if MPIC_PSMCS(0x3f) maps to 20MHz or more, it could never of
> worked, which makes me think the clock has changed. If it maps to
> 6Mhz, yes it could of worked with some PHY but not others, and the
> clock might not of changed.

I'm sorry for lacking information. MPIC_PSMCS(0x3f) maps to 2.5MHz.

Best regards,
Yoshihiro Shimoda

>       Andrew
Andrew Lunn Sept. 27, 2023, 12:11 p.m. UTC | #6 
> Yes, the original version was tested on Spider board.
> The original version's MDC frequency was 25MHz.
> And the PHY (Marvell 88E2110) on Spider board can use such frequency,
> IIUC because the MDC period is 35 ns (so 28.57143MHz).

25MHz is way above anything i've tested.

> However, I don't know why this setting cannot work on the Starter Kit board
> because the board also has the same PHY. I guess that this is related to
> board design, especially voltage of I/O (Spider = 1.8V, Starter Kit = 3.3V).

I've had boards which will not work at 2.5Mhz until the pull up
resistor was changed to make the waveform compliant. So it probably is
related to the board.

> Anyway, changing the MDC frequency from 25MHz to 2.5MHz works correctly on
> both Spider and Starter Kit. So, I would like to apply the v3 patch [1] for safe.

O.K. That makes sense. If you want to support the higher speed, you
could implement the device tree property:


  clock-frequency:
    description:
      Desired MDIO bus clock frequency in Hz. Values greater than IEEE 802.3
      defined 2.5MHz should only be used when all devices on the bus support
      the given clock speed.

	Andrew
Yoshihiro Shimoda Sept. 27, 2023, 11:58 p.m. UTC | #7 
Hello Andrew,

> From: Andrew Lunn, Sent: Wednesday, September 27, 2023 9:11 PM
> 
> > Yes, the original version was tested on Spider board.
> > The original version's MDC frequency was 25MHz.
> > And the PHY (Marvell 88E2110) on Spider board can use such frequency,
> > IIUC because the MDC period is 35 ns (so 28.57143MHz).
> 
> 25MHz is way above anything i've tested.
> 
> > However, I don't know why this setting cannot work on the Starter Kit board
> > because the board also has the same PHY. I guess that this is related to
> > board design, especially voltage of I/O (Spider = 1.8V, Starter Kit = 3.3V).
> 
> I've had boards which will not work at 2.5Mhz until the pull up
> resistor was changed to make the waveform compliant. So it probably is
> related to the board.

I got it.

> > Anyway, changing the MDC frequency from 25MHz to 2.5MHz works correctly on
> > both Spider and Starter Kit. So, I would like to apply the v3 patch [1] for safe.
> 
> O.K. That makes sense. If you want to support the higher speed, you
> could implement the device tree property:
> 
> 
>   clock-frequency:
>     description:
>       Desired MDIO bus clock frequency in Hz. Values greater than IEEE 802.3
>       defined 2.5MHz should only be used when all devices on the bus support
>       the given clock speed.

Thank you for the information. If I need to support the higher speed,
I'll implement such a code with this device tree property.

Best regards,
Yoshihiro Shimoda

> 	Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/rswitch.c b/drivers/net/ethernet/renesas/rswitch.c
index ea9186178091..8b5e2380f114 100644
--- a/drivers/net/ethernet/renesas/rswitch.c
+++ b/drivers/net/ethernet/renesas/rswitch.c
@@ -1049,7 +1049,7 @@  static void rswitch_rmac_setting(struct rswitch_etha *etha, const u8 *mac)
 static void rswitch_etha_enable_mii(struct rswitch_etha *etha)
 {
 	rswitch_modify(etha->addr, MPIC, MPIC_PSMCS_MASK | MPIC_PSMHT_MASK,
-		       MPIC_PSMCS(0x05) | MPIC_PSMHT(0x06));
+		       MPIC_PSMCS(0x3f) | MPIC_PSMHT(0x06));
 	rswitch_modify(etha->addr, MPSM, 0, MPSM_MFF_C45);
 }