Re: [PATCH net-next 25/28] [RFC] net: dpaa: Convert to phylink

From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Sean Anderson <sean.anderson@seco.com>
Cc: "David S . Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Madalin Bucur <madalin.bucur@nxp.com>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>
Subject: Re: [PATCH net-next 25/28] [RFC] net: dpaa: Convert to phylink
Date: Fri, 17 Jun 2022 23:01:36 +0100	[thread overview]
Message-ID: <Yqz5wHy9zAQL1ddg@shell.armlinux.org.uk> (raw)
In-Reply-To: <20220617203312.3799646-26-sean.anderson@seco.com>

Hi,

On Fri, Jun 17, 2022 at 04:33:09PM -0400, Sean Anderson wrote:
> This converts DPAA to phylink. For the moment, only MEMAC is converted.
> This should work with no device tree modifications (including those made in
> this series), except for QSGMII (as noted previously).
> 
> One area where I wasn't sure how to do things was regarding when to call
> phy_init and phy_power_on. Should that happen when selecting the PCS?

Is this a common serdes PHY that is shared amongst the various PCS? I
think from what I understand having read the other patches, it is. In
which case, initialising the PHY prior to calling phylink_start() and
powering down the PHY after phylink_stop() should be sufficient.

> Similarly, I wasn't sure where to reconfigure the thresholds in
> dpaa_eth_cgr_init. Should happen in link_up? If so, I think we will need
> some kind of callback.

Bear in mind that with 1000BASE-X, SGMII, etc, we need the link working
in order for the link to come up, so if the serdes PHY hasn't been
properly configured for the interface mode, then the link may not come
up.

How granular are these threshold configurations? Do they depend on
speed? (Note that SGMII operates at a constant speed irrespective of
the data rate due to symbol replication, so there shouldn't be a speed
component beyond that described by the interface mode, aka
phy_interface_t.)

> This has been tested on an LS1046ARDB. Without the serdes enabled,
> everything works. With the serdes enabled, everything works but eth3 (aka
> MAC6). On that interface, SGMII never completes AN for whatever reason. I
> haven't tested the counterfactual (serdes enabled but no phylink). With
> managed=phy (e.g. unspecified), I was unable to get the interfaces to come
> up at all.

I'm not sure of the level of accurate detail in the above statement,
so the following is just to cover all bases...

It's worth enabling debug in phylink so you can see what's going on -
for example, whether the "MAC" (actually PCS today) is reporting that
the link came up (via its pcs_get_state() callback.) Also whether
phylib is reporting that the PHY is saying that the link is up. That
should allow you to identify which part of the system is not

Having looked through your phylink implementation, nothing obviously
wrong stands out horribly in terms of how you're using it.

The only issue I've noticed is in dpaa_ioctl(), where you only forward
one ioctl command to phylink, whereas there are actually three ioctls
for PHY access - SIOCGMIIPHY, SIOCGMIIREG and SIOCSMIIREG. Note that
phylink (and phylib) return -EOPNOTSUPP if the ioctl is not appropriate
for them to handle. However, note that phylib will handle
SIOCSHWTSTAMP.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel