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: Netdev Maintainers
Headers show
Series [net] net: ethernet: renesas: rswitch Fix PHY station management clock setting | expand

Checks

Context Check Description
netdev/series_format success Single patches do not need cover letters
netdev/tree_selection success Clearly marked for net
netdev/fixes_present success Fixes tag present in non-next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1340 this patch: 1340
netdev/cc_maintainers fail 1 blamed authors not CCed: andrew@lunn.ch; 1 maintainers not CCed: andrew@lunn.ch
netdev/build_clang success Errors and warnings before: 1363 this patch: 1363
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 1363 this patch: 1363
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 8 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

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