RE: [PATCH net-next V2 1/5] net: lan743x: Add SFP support check flag

[Ronnie.Kunin]

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, September 12, 2024 11:58 AM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@xxxxxxxxxxxxx>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx; Bryan
> Whitehead - C21958 <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; linux@xxxxxxxxxxxxxxx; maxime.chevallier@xxxxxxxxxxx;
> rdunlap@xxxxxxxxxxxxx; Steen Hegelund - M31857 <Steen.Hegelund@xxxxxxxxxxxxx>; Daniel Machon -
> M70577 <Daniel.Machon@xxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next V2 1/5] net: lan743x: Add SFP support check flag
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > > > > > +     if (adapter->is_pci11x1x && !adapter->is_sgmii_en &&
> > > > > > +         adapter->is_sfp_support_en) {
> > > > > > +             netif_err(adapter, drv, adapter->netdev,
> > > > > > +                       "Invalid eeprom cfg: sfp enabled with sgmii disabled");
> > > > > > +             return -EINVAL;
> > > > >
> > > > > is_sgmii_en actually means PCS? An SFP might need 1000BaseX or
> > > > > SGMII,
> > > >
> > > > No, not really.
> > > > The PCI11010/PCI1414 chip can support either an RGMII interface or
> > > > an SGMII/1000Base-X/2500Base-X interface.
> > >
> > > A generic name for SGMII/1000Base-X/2500Base-X would be PCS, or
> > > maybe SERDES. To me, is_sgmii_en means SGMII is enabled, but in fact
> > > it actually means SGMII/1000Base-X/2500Base-X is enabled. I just think this is badly named. It would
> be more understandable if it was is_pcs_en.
> > >
> > > > According to the datasheet,
> > > > the "Strap Register (STRAP)" bit 6 is described as "SGMII_EN_STRAP"
> > > > Therefore, the flag is named "is_sgmii_en".
> > >
> > > Just because the datasheet uses a bad name does not mean the driver has to also use it.
> > >
> > >         Andrew
> >
> > The hardware architect, who is a very bright guy (it's not me :-), just called the strap SGMII_EN in order
> not to make the name too long and to contrast it with the opposite polarity of the bit which means the
> interface is set to RGMII; but in the description of the strap he clearly stated what it is:
> >       SGMII_EN_STRAP
> >       0 = RGMII
> >       1 = SGMII / 1000/2500BASE-X
> >
> > I don't think PCS or Serdes (both of which get used in other technologies - some of which are also
> included in this chip and are therefore bound to create even more confusion if used) are good choices
> either.
> 
> SERDES i understand, PCI itself is a SERDES. But what are the other uses of PCS? At least in the context of
> networking, PCS is reasonably well understood.

Here's one example: the LAN743x device, which this driver also services, does not support either SGMII or BASEX. It only supports RGMII, but it does have an internal PHY and its MMDs at address 3 controls the Ethernet PCS. 

> 
> > That being said, if it makes it more we can certainly call this flag "is_sgmii_basex_en". How's that
> sound ?
> 
> Better. But i still think PCS is better.
> 
> But you need to look at the wider context:
> 
> > > > > > +                       "Invalid eeprom cfg: sfp enabled with
> > > > > > + sgmii disabled");
> 
> SGMII is wrong here as well. You could flip it around:
> 
> > > > > > +                       "Invalid eeprom cfg: sfp enabled with
> > > > > > + RGMII");

Agreed. There are some other debug/err messages that were not clear that I also suggested Raju to change and he will submit in the next version of this the patch series.

> 
> In terms of reviewing this code, i have to ask myself the question, does it really mean SGMII when it says
> SGMII? When you are talking about Base-T, i don't know of any 1000BaseX PHYs, so you can be sloppy
> with the term SGMII. But as soon as SFPs come into the mix, SGMII vs 1000BaseX becomes important, so
> you want the code to really mean SGMII when it says SGMII.

That's fine. But the particular strap (and that is what that flag tracks) you are talking about needs to be set for either SGMII or BASEX. Whatever else may need to be handled differently is taken care (I guess within the Linux framework)  when the phylink interface (PHY_INTERFACE_MODE_*) ends up flipping 

> 
>      Andrew