netdev - RE: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add phy_power

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 30 Jan 2023 05:52:15 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Russell King <linux@...linux.org.uk>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>
Subject: RE: [PATCH net-next v4 4/4] net: ethernet: renesas: rswitch: Add
 phy_power_{on,off}() calling

Hi Russell,

> From: Russell King, Sent: Saturday, January 28, 2023 12:18 AM
> 
> On Fri, Jan 27, 2023 at 11:26:21PM +0900, Yoshihiro Shimoda wrote:
> > Some Ethernet PHYs (like marvell10g) will decide the host interface
> > mode by the media-side speed. So, the rswitch driver needs to
> > initialize one of the Ethernet SERDES (r8a779f0-eth-serdes) ports
> > after linked the Ethernet PHY up. The r8a779f0-eth-serdes driver has
> > .init() for initializing all ports and .power_on() for initializing
> > each port. So, add phy_power_{on,off} calling for it.
> 
> So how does this work?

This hardware has MDIO interfaces, and the MDIO can communicate the Ethernet
PHY without the Ethernet SERDES initialization. And, the Ethernet PHY can be
initialized, and media-side of the PHY works. So, this works.

> 88x3310 can change it's MAC facing interface according to the speed
> negotiated on the media side, or it can use rate adaption mode, but
> if it's not a MACSEC device, the MAC must pace its transmission
> rate to that of the media side link.

My platform has 88x2110 so that it's not a MACSEC device.

> The former requires one to reconfigure the interface mode in
> mac_config(), which I don't see happening in this patch set.

You're correct. This patch set doesn't have such reconfiguration
because this driver doesn't support such a feature (for now).

However, this driver has configured the interface mode when
driver is probing.

> The latter requires some kind of configuration in mac_link_up()
> which I also don't see happening in this patch set.

You're correct. This patch set doesn't have such configuration
in mac_link_up() because this hardware cannot change speed at
runtime (for now).

However, this driver has configured the speed when driver is
probing.

> So, I doubt this works properly.
> 
> Also, I can't see any sign of any working DT configuration for this
> switch to even be able to review a use case - all there is in net-next
> is the basic description of the rswitch in a .dtsi and no users. It
> may be helpful if there was some visibility of its use, and why
> phylink is being used in this driver - because right now with phylink's
> MAC methods stubbed out in the way they are, and even after this patch
> set, I see little point to this driver using phylink.

In the latest net-next, r8a779f0-spider.dts is a user.

In r8a779f0-spider-ether.dtsi:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider-ethernet.dtsi#n41

In r8a779f0-spider.dts:
https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tree/arch/arm64/boot/dts/renesas/r8a779f0-spider.dts#n10

> Moreover, looking at the binding document, you don't even support SFPs
> or fixed link, which are really the two reasons to use phylink over
> phylib.

You're correct. This hardware doesn't support SFPs or fixed link.

I sent a patch at the first, I had used phylib and had added a new function
for setting the phy_dev->host_interfaces [1]. And then, Marek suggested
that I should use phylink instead of phylib. That's why this driver
is using phylink even if this doesn't support SFPs and fixed link.

[1]
https://lore.kernel.org/netdev/20221019124100.41c9bbaf@dellmb/

> Also, phylink only really makes sense if the methods in its _ops
> structures actually do something useful, because without that there
> can be no dynamic configuration of the system to suit what is
> connected.

I think so. This rswitch doesn't need dynamic configuration,
but Marvell 88x2110 on my platform needs dynamic configuration.
That's why this driver uses phylink.

Best regards,
Yoshihiro Shimoda