Re: [PATCH net-next v14 5/6] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell

["Marek Behún" <kabel@kernel.org>]

Hi Vladimir,

On Tue, 12 Jan 2021 13:11:39 +0200
Vladimir Oltean <olteanv@gmail.com> wrote:

> On Mon, Jan 11, 2021 at 02:21:55AM +0100, Marek Behún wrote:
> >   via which serveral more registers can be accessed indirectly  
>               ~~~~~~~~
>               several

Thanks.

> > +static void mv88e6393x_phylink_validate(struct mv88e6xxx_chip *chip, int port,
> > +					unsigned long *mask,
> > +					struct phylink_link_state *state)
> > +{
> > +	if (port == 0 || port == 9 || port == 10) {
> > +		phylink_set(mask, 10000baseT_Full);
> > +		phylink_set(mask, 10000baseKR_Full);  
> 
> I think I understand the reason for declaring 10GBase-KR support in
> phylink_validate, in case the PHY supports that link mode on the media
> side, but...

Hmm, yes, maybe KR shouldn't be here, but then why is it in
mv88e6390x_phylink_validate?

> >  	case PHY_INTERFACE_MODE_2500BASEX:
> >  		cmode = MV88E6XXX_PORT_STS_CMODE_2500BASEX;
> >  		break;
> > +	case PHY_INTERFACE_MODE_5GBASER:
> > +		cmode = MV88E6393X_PORT_STS_CMODE_5GBASER;
> > +		break;
> >  	case PHY_INTERFACE_MODE_XGMII:
> >  	case PHY_INTERFACE_MODE_XAUI:
> >  		cmode = MV88E6XXX_PORT_STS_CMODE_XAUI;
> > @@ -457,6 +569,10 @@ static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
> >  	case PHY_INTERFACE_MODE_RXAUI:
> >  		cmode = MV88E6XXX_PORT_STS_CMODE_RXAUI;
> >  		break;
> > +	case PHY_INTERFACE_MODE_10GBASER:
> > +	case PHY_INTERFACE_MODE_10GKR:  
> 
> Does the SERDES actually support 10GBase-KR (aka 10GBase-R for copper
> backplanes)? It is different than plain 10GBase-R (abusingly called XFI)
> by the need of a link training procedure to negotiate SERDES eye
> parameters. There have been discussion in the past where it turned out
> that drivers which didn't really support 10GBase-KR incorrectly reported
> that they did.

Yes, PHY_INTERFACE_MODE_10GKR should probably not be here. I think some
other drivers still support it because of old device trees, but this
driver does not need to.

> > +static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, int port,
> > +					u16 pointer, u8 data)
> > +{
> > +	u16 reg;
> > +
> > +	reg = MV88E6393X_PORT_POLICY_MGMT_CTL_UPDATE | pointer | data;  
> 
> I think the assignment fits on the same line as the declaration?
>

And I think it read better this way. But even if it was not, this
structure of code was copied from mv88e6390_g1_monitor_write in
global1.c, where it is written this way. I prefer this patch to do a
new thing in the same style. If you want, you can then send a patch
that changes it in all places at once.

> > +static int mv88e6393x_serdes_port_config(struct mv88e6xxx_chip *chip, int lane,
> > +					 bool on)
> > +{
> > +	u8 cmode = chip->ports[lane].cmode;
> > +	u16 reg, pcs;
> > +	int err;
> > +
> > +	if (on) {  
> 
> And if "on" is false? Nothing? Why even pass it as an argument then? Why
> even call mv88e6393x_serdes_port_config?

You are right, I will change it.

Thanks, Vladimir.