Re: [PATCH net-next V2 5/5] net: lan743x: Add Support for 2.5G SFP with 2500Base-X Interface

[Maxime Chevallier]

Hi Raju,

On Thu, 12 Sep 2024 12:31:33 +0530
Raju Lakkaraju <Raju.Lakkaraju@xxxxxxxxxxxxx> wrote:

> The 09/11/2024 22:01, Maxime Chevallier wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > On Wed, 11 Sep 2024 19:31:01 +0200
> > Andrew Lunn <andrew@xxxxxxx> wrote:
> >   
> > > > @@ -3359,6 +3362,7 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
> > > >     lan743x_phy_interface_select(adapter);
> > > >
> > > >     switch (adapter->phy_interface) {
> > > > +   case PHY_INTERFACE_MODE_2500BASEX:
> > > >     case PHY_INTERFACE_MODE_SGMII:
> > > >             __set_bit(PHY_INTERFACE_MODE_SGMII,
> > > >                       adapter->phylink_config.supported_interfaces);  
> > >
> > > I _think_ you also need to set the PHY_INTERFACE_MODE_2500BASEX bit in
> > > phylink_config.supported_interfaces if you actually support it.  
> > 
> > It's actually being set a bit below. However that raises the
> > question of why.
> > 
> > On the variant that don't have this newly-introduced SFP support but do
> > have sgmii support (!is_sfp_support_en && is_sgmii_en), can this chip
> > actually support 2500BaseX ?  
> 
> Yes. 
> PCI11010/PCI11414 chip's PCS support SGMII/2500Baxe-X I/F at 2.5Gpbs
> We need to over clocking at a bit rate of 3.125 Gbps for 2.5Gbps event SGMII
> I/F
> 
> From data sheet:
> "The SGMII interface also supports over clocking at a bit rate of 3.125 Gbps for an effective 2.5 Gbps data rate. 10 and
> 100 Mbps modes are also scaled up by 2.5x but are most likely not useful."
> 
> > 
> > If so, is there a point in getting a different default interface
> > returned from lan743x_phy_interface_select() depending on wether or not
> > there's SFP support ?  
> 
> Yes.
> 
> This LAN743x driver support following chips
>  1. LAN7430 - GMII I/F
>  2. LAN7431 - MII I/F
>  3. PCI11010/PCI11414 - RGMII or SGMII/1000Base-X/2500Base-X

In your patch there's the following change :

@@ -1495,7 +1495,10 @@ static void lan743x_phy_interface_select(struct lan743x_adapter *adapter)
 	data = lan743x_csr_read(adapter, MAC_CR);
 	id_rev = adapter->csr.id_rev & ID_REV_ID_MASK_;
 
-	if (adapter->is_pci11x1x && adapter->is_sgmii_en)
+	if (adapter->is_pci11x1x && adapter->is_sgmii_en &&
+	    adapter->is_sfp_support_en)
+		adapter->phy_interface = PHY_INTERFACE_MODE_2500BASEX;
+	else if (adapter->is_pci11x1x && adapter->is_sgmii_en)
 		adapter->phy_interface = PHY_INTERFACE_MODE_SGMII;
 	else if (id_rev == ID_REV_ID_LAN7430_)
 		adapter->phy_interface = PHY_INTERFACE_MODE_GMII;

From what I get, if the chip is a pci11x1x and has sgmii_en, it doesn't
really matter wether or not the "is_sfp_support" it set, as you support
the same sets of interface modes.

The phy_interface will be re-configured the moment the SFP module is
plugged, so it shouldn't matter wether you set the default interface to
SGMII or 2500BaseX.

So, the change quoted above doesn't really bring anything, am I correct
?

Thanks,

Maxime