Re: [PATCH net-next v6 5/5] net: phy: add driver for built-in 2.5G ethernet PHY on MT7988

[Russell King (Oracle)]

On Mon, Jun 03, 2024 at 06:00:49PM +0100, Daniel Golle wrote:
> On Mon, Jun 03, 2024 at 04:47:28PM +0100, Russell King (Oracle) wrote:
> > On Mon, Jun 03, 2024 at 03:52:19PM +0100, Daniel Golle wrote:
> > > On Mon, Jun 03, 2024 at 02:41:44PM +0100, Russell King (Oracle) wrote:
> > > > On Mon, Jun 03, 2024 at 02:31:46PM +0100, Daniel Golle wrote:
> > > > > On Mon, Jun 03, 2024 at 02:25:01PM +0100, Russell King (Oracle) wrote:
> > > > > > On Mon, Jun 03, 2024 at 08:18:34PM +0800, Sky Huang wrote:
> > > > > > > Add support for internal 2.5Gphy on MT7988. This driver will load
> > > > > > > necessary firmware, add appropriate time delay and figure out LED.
> > > > > > > Also, certain control registers will be set to fix link-up issues.
> > > > > > 
> > > > > > Based on our previous discussion, it may be worth checking in the
> > > > > > .config_init() method whether phydev->interface is one of the
> > > > > > PHY interface modes that this PHY supports. As I understand from one
> > > > > > of your previous emails, the possibilities are XGMII, USXGMII or
> > > > > > INTERNAL. Thus:
> > > > > > 
> > > > > > > +static int mt798x_2p5ge_phy_config_init(struct phy_device *phydev)
> > > > > > > +{
> > > > > > > +	struct pinctrl *pinctrl;
> > > > > > > +	int ret;
> > > > > > 
> > > > > > 	/* Check that the PHY interface type is compatible */
> > > > > > 	if (phydev->interface != PHY_INTERFACE_MODE_INTERNAL &&
> > > > > > 	    phydev->interface != PHY_INTERFACE_MODE_XGMII &&
> > > > > > 	    phydev->interface != PHY_INTERFACE_MODE_USXGMII)
> > > > > > 		return -ENODEV;
> > > > > 
> > > > > The PHY is built-into the SoC, and as such the connection type should
> > > > > always be "internal". The PHY does not exist as dedicated IC, only
> > > > > as built-in part of the MT7988 SoC.
> > > > 
> > > > That's not how it was described to me by Sky.
> > > > 
> > > > If what you say is correct, then the implementation of
> > > > mt798x_2p5ge_phy_get_rate_matching() which checks for interface modes
> > > > other than INTERNAL is not correct. Also it means that config_init()
> > > > should not permit anything but INTERNAL.
> > > 
> > > The way the PHY is connected to the MAC *inside the chip* is XGMII
> > > according the MediaTek. So call it "internal" or "xgmii", however, up to
> > > my knowledge it's a fact that there is **only one way** this PHY is
> > > connected and used, and that is being an internal part of the MT7988 SoC.
> > > 
> > > Imho, as there are no actual XGMII signals exposed anywhere I'd use
> > > "internal" to describe the link between MAC and PHY (which are both
> > > inside the same chip package).
> > 
> > I don't care what gets decided about what's acceptable for the PHY to
> > accept, just that it checks for the acceptable modes in .config_init()
> > and the .get_rate_matching() method is not checking for interface
> > modes that are not permitted.
> 
> What I meant to express is that there is no need for such a check, also
> not in config_init. There is only one way and one MAC-side interface mode
> to operate that PHY, so the value will anyway not be considered anywhere
> in the driver.

No, it matters. With drivers using phylink, the PHY interface mode is
used in certain circumstances to constrain what the net device can do.
So, it makes sense for new PHY drivers to ensure that the PHY interface
mode is one that they can support, rather than just accepting whatever
is passed to them (which then can lead to maintainability issues for
subsystems.)

So, excuse me for disagreeing with you, but I do want to see such a
check in new PHY drivers.

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