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

[Ronnie.Kunin]

Hi Andrew, thanks very much for your comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, September 12, 2024 10:52 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@xxxxxxxxxxxxx>
> Cc: 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
> 
> On Thu, Sep 12, 2024 at 11:59:04AM +0530, Raju Lakkaraju wrote:
> > Hi Andrew,
> >
> > Thank you for review the patches.
> >
> > The 09/11/2024 19:06, Andrew Lunn wrote:
> > > 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.

That being said, if it makes it more we can certainly call this flag "is_sgmii_basex_en". How's that sound ?

Ronnie