Date:   Tue, 31 Jan 2023 04:42:00 +0000
From:   Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
To:     Russell King <linux@...linux.org.uk>
CC:     Marek Behún <kabel@...nel.org>,
        "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: Monday, January 30, 2023 9:16 PM
> 
> On Mon, Jan 30, 2023 at 05:52:15AM +0000, Yoshihiro Shimoda wrote:
> > 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.
> 
> Ethernet PHYs can generally be communicated with irrespective of the
> serdes state, so that isn't the concern.
> 
> What I'm trying to grasp is your decision making behind putting the
> serdes phy power control in the link_up/link_down functions, when
> doing so is fundamentally problematical if in-band mode is ever
> supported - and in-band mode has to be supported for things like
> fibre connections to work.

I understood it.

> > > 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.
> 
> ... which supports USXGMII, 10GBaseR, 5GBaseR, 2500BaseX and SGMII,
> possibly with rate adaption, and if it's not a MACSEC device, that
> rate adaption will likely require the MAC side to pace its
> transmission to the media speed.

I understood it.

> > > 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).
> 
> Is this planned? When are we likely to see this code?

I'm afraid, but this is not planned.

> > > 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).
> 
> the hardware can't even change between the various SGMII speeds?

Unfortunately, it's true. But, I'm sorry for my unclear explanation.
This is related to a hardware restriction which cannot changed
an internal mode if it enters "operation" mode once...
But, I heard that this restriction will be fixed on a new SoC revision.

> What
> kind of utterly crippled hardware implementation is this? You make it
> sound like the hardware designers don't have a clue what they're doing.
> 
> > > 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:
> >
> <snip the URL>
> >
> > In r8a779f0-spider.dts:
> >
<snip the URL>
> 
> So these configure the ports with PHYs on to use SGMII mode. No mention
> of any speed, yet you say that's configured at probe time? Do you just
> set them to 1G, and hope that the media side link negotiates to 1G
> speeds?

You're correct.

> That doesn't sound like a good idea to me.

I agreed. So, I will fix it somehow...

> > > 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]
> >
<snip the URL>
> 
> [Adding Marek to the Cc]
> 
> I'm afraid I don't agree with Marek given the state of this driver.
> His assertion is "there's an API for doing this" which is demonstrably
> false. If his assertion were true, then you wouldn't need to add the
> code to phylink to set phydev->host_interfaces for on-board PHYs.
> 
> I'm not particularly happy about adding that to phylink, and now that
> I read your current rather poor implementation of phylink, I'm even
> less happy about it.

I understood it.

> > > 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.
> 
> Given that you use the 88x2110, and you've set the phy-mode to
> SGMII, it should support 10M, 100M and 1G speeds on the media
> side. Please test - and if not, I think the code which supports
> that should at the very least be part of this patch set - so we
> begin to see a proper implementation in the mac_* ops.

I got it.

> The reason for this is I utterly detest shoddy users of phylink, and
> I will ask people not to use phylink if they aren't prepared to
> implement it properly - because shoddy phylink users add greatly to
> my maintenance workload.

I understood it. I don't want to add your maintenance workload by my patch.

Best regards,
Yoshihiro Shimoda

Powered by blists - more mailing lists