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

Hi Vladimir,

On Tue, 12 Jan 2021 13:11:39 +0200
Vladimir Oltean [off-list ref] wrote:

On Mon, Jan 11, 2021 at 02:21:55AM +0100, Marek Behún wrote:

quoted

  via which serveral more registers can be accessed indirectly  

              ~~~~~~~~
              several

Thanks.

quoted

+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?

quoted

 	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.

quoted

+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.

quoted

+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.