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

[Sean Anderson]

On 3/24/23 09:07, Ioana Ciornei wrote:
> 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?

The existing checks let the above scenario happen.

> 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.

Ah, sorry. I was referring to the check for dpaa2_pcs_create.

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

Say we have DPAA2_MAC_FEATURE_PROTOCOL_CHANGE and DPMAC_LINK_TYPE_PHY.
Then this clause will be taken, even though we are not manging the phy.

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

The RGMII check for the PCS should be in place for both PHY and BACKPLANE.

--Sean