Re: [PATCH 3/5] net: ethernet: renesas: Add Ethernet Switch driver

[Andrew Lunn <andrew@lunn.ch>]

> > > +static void rswitch_adjust_link(struct net_device *ndev)
> > > +{
> > > +	struct rswitch_device *rdev = netdev_priv(ndev);
> > > +	struct phy_device *phydev = ndev->phydev;
> > > +
> > > +	if (phydev->link != rdev->etha->link) {
> > > +		phy_print_status(phydev);
> > > +		rdev->etha->link = phydev->link;
> > > +	}
> > 
> > Given that the SERDES supports 100 and 1G, it seems odd you don't need
> > to do anything here.
> 
> Indeed. However, unfortunately, the current hardware cannot change the speed at runtime...
> So, I'll add such comments here.

Then you need to tell phylib about this. MAC drivers with limitations
often call phy_set_max_speed() to remove higher speeds which the PHY
can support, but the MAC cannot. You need to go further and remove
lower speeds as well. The autoneg in the PHY should then only work for
the speeds you actually support.

> > > +static int rswitch_serdes_common_setting(void __iomem *addr0,
> > > +					 enum rswitch_serdes_mode mode)
> > > +{
> > > +	switch (mode) {
> > > +	case SGMII:
> > > +		rswitch_serdes_write32(addr0, 0x0244, 0x180, 0x97);
> > > +		rswitch_serdes_write32(addr0, 0x01d0, 0x180, 0x60);
> > > +		rswitch_serdes_write32(addr0, 0x01d8, 0x180, 0x2200);
> > > +		rswitch_serdes_write32(addr0, 0x01d4, 0x180, 0);
> > > +		rswitch_serdes_write32(addr0, 0x01e0, 0x180, 0x3d);
> > 
> > Please add #defines for all these magic numbers.
> 
> I should have added comments before though, the datasheet also describes
> such magic numbers like below...
> Step S.4.1	bank 0x180	address = 0x0244		data = 0x00000097
> Step S.4.2	bank 0x180	address = 0x01d0		data = 0x00000060
> ...
> 
> So, perhaps we can define like the followings:
> #define	SERDES_BANK_180		0x180
> 
> #define	SERDES_STEP_S_4_1_ADDR	0x0244
> #define	SERDES_STEP_S_4_1_DATA	0x00000097

Not really any better. Better to comment that you have no idea what
any of this does, it is all black magic.

    Andrew