Re: [PATCH net v2] net: dpaa2-mac: Get serdes only for backplane links

[Ioana Ciornei]

On Thu, Mar 23, 2023 at 12:30:34PM -0400, Sean Anderson wrote:
> On 3/23/23 11:27, Ioana Ciornei wrote:
> > On Mon, Mar 06, 2023 at 11:13:17AM -0500, Sean Anderson wrote:
> >> On 3/6/23 03:09, Ioana Ciornei wrote:
> >> > On Fri, Mar 03, 2023 at 07:31:59PM -0500, Sean Anderson wrote:
> >> >> When commenting on what would become commit 085f1776fa03 ("net: dpaa2-mac:
> >> >> add backplane link mode support"), Ioana Ciornei said [1]:
> >> >> 
> >> >> > ...DPMACs in TYPE_BACKPLANE can have both their PCS and SerDes managed
> >> >> > by Linux (since the firmware is not touching these). That being said,
> >> >> > DPMACs in TYPE_PHY (the type that is already supported in dpaa2-mac) can
> >> >> > also have their PCS managed by Linux (no interraction from the
> >> >> > firmware's part with the PCS, just the SerDes).
> >> >> 
> >> >> This implies that Linux only manages the SerDes when the link type is
> >> >> backplane. Modify the condition in dpaa2_mac_connect to reflect this,
> >> >> moving the existing conditions to more appropriate places.
> >> > 
> >> > I am not sure I understand why are you moving the conditions to
> >> > different places. Could you please explain?
> >> 
> >> This is not (just) a movement of conditions, but a changing of what they
> >> apply to.
> >> 
> >> There are two things which this patch changes: whether we manage the phy
> >> and whether we say we support alternate interfaces. According to your
> >> comment above (and roughly in-line with my testing), Linux manages the
> >> phy *exactly* when the link type is BACKPLANE. In all other cases, the
> >> firmware manages the phy. Similarly, alternate interfaces are supported
> >> *exactly* when the firmware supports PROTOCOL_CHANGE. However, currently
> >> the conditions do not match this.
> >> 
> >> > Why not just append the existing condition from dpaa2_mac_connect() with
> >> > "mac->attr.link_type == DPMAC_LINK_TYPE_BACKPLANE"?
> >> > 
> >> > This way, the serdes_phy is populated only if all the conditions pass
> >> > and you don't have to scatter them all around the driver.
> >> 
> >> If we have link type BACKPLANE, Linux manages the phy, even if the
> >> firmware doesn't support changing the interface. Therefore, we need to
> >> grab the phy, but not fill in alternate interfaces.
> >> 
> >> This does not scatter the conditions, but instead moves them to exactly
> >> where they are needed. Currently, they are in the wrong places.
> > 
> > Sorry for not making my position clear from the first time which is:
> > there is no point in having a SerDes driver or a reference to the
> > SerDes PHY if the firmware does not actually support changing of
> > interfaces.
> > 
> > Why I think that is because the SerDes is configured at boot time
> > anyway for the interface type defined in the RCW (Reset Configuration
> > Word). If the firmware does not support any protocol change then why
> > trouble the dpaa2-eth driver with anything SerDes related?
> 
> It's actually the other way around. If the firmware is managing the phy,
> why try to probe it? Consider a situation where the firmware supports
> protocol change, but the link type is PHY. Then we will probe the
> serdes, but we may confuse the firmware (or vice versa).

And how is that conflicting with what I said?

Again, I agree that we don't want to manage the SerDes PHY in situations
in which the firmware also does it. And that means adding and extra
check in the driver so that the SerDes PHY is setup only in BACKPLANE
mode.

> 
> > This is why I am ok with only extending the condition from
> > dpaa2_mac_connect() with an additional check but not the exact patch
> > that you sent.
> 
> AIUI the condition there is correct because Linux controls the PCS in
> both PHY and BACKPLANE modes (although the RGMII check is a bit
> strange).
> 

I am not sure if we talk about the same check.
I was referring to this check which has nothing to do with the PCS
(which is why I don't understand why you mentioned it).

	if (mac->features & DPAA2_MAC_FEATURE_PROTOCOL_CHANGE &&
	    !phy_interface_mode_is_rgmii(mac->if_mode) &&
	    is_of_node(dpmac_node)) {
		serdes_phy = of_phy_get(to_of_node(dpmac_node), NULL);

Also, why is the RGMII check strange? RGMII does not use a SerDes PHY.

Ioana