Hi Vladimir,
On Tue, 12 Jan 2021 13:11:39 +0200
Vladimir Oltean <olte...@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.
