[PATCH net-next V2 0/5] Add support to SFP for PCI11x1x chips

[PATCH net-next V2 0/5] Add support to SFP for PCI11x1x chips

[Raju Lakkaraju]

This is the follow-up patch series of
https://lkml.iu.edu/hypermail/linux/kernel/2310.2/02078.html

Divide the PHYLINK adaptation and SFP modifications into two separate patch
series.

This patch series focuses on sfp support for PCI11x1x chips.

Tested on PCI11010 Rev-1 Evaluation board with following SFP modules:
 1. FS's 2.5G SFP (SFP-2.5G-T) with 2.5 Gbps speed (interface 2500Base-X)
 2. Axcen's 1G SFP (AXGT-R1T4-05I1) at 1G/100M/10M bps speed (interface SGMII) 

Change Log:
===========
V1 -> V2:
  - Split the patches to "PHYLINK" and "SFP" parts
  - Change variable name from "chip_rev" to "fpga_rev"
  - SFP GPIO definitions and other macros move from lan743x_main.c to
    lan743x_main.h file
  - Change from "PCI11X1X_" to "PCI11X1X_EVB_PCI11010_" strings for GPIO macros
  - Add platform_device_unregister( ) when sfp register fail
  - Add two new patches to this patch series
V0 -> V1:
  - Integrate with Synopsys DesignWare XPCS drivers
  - Based on external review comments,
  - Changes made to SGMII interface support only 1G/100M/10M bps speed
  - Changes made to 2500Base-X interface support only 2.5Gbps speed
  - Add check for not is_sgmii_en with is_sfp_support_en support
  - Change the "pci11x1x_strap_get_status" function return type from void to
    int
- Add ethtool phylink wol, eee, pause get/set functions

Raju Lakkaraju (5):
  net: lan743x: Add SFP support check flag
  net: lan743x: Add support to software-nodes for sfp
  net: lan743x: Register the platform device for sfp pluggable module
  net: lan743x: Implement phylink pcs
  net: lan743x: Add Support for 2.5G SFP with 2500Base-X Interface

 drivers/net/ethernet/microchip/Kconfig        |   4 +
 drivers/net/ethernet/microchip/lan743x_main.c | 358 +++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  88 +++++
 3 files changed, 434 insertions(+), 16 deletions(-)

-- 
2.34.1

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

[Raju Lakkaraju]

Support for SFP in the PCI11x1x devices is indicated by the "is_sfp_support_en"
flag in the STRAP register. This register is loaded at power up from the
PCI11x1x EEPROM contents (which specify the board configuration).

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V1 -> V2:
  - Change variable name from "chip_rev" to "fpga_rev" 
V0 -> V1:
  - No changes

 drivers/net/ethernet/microchip/lan743x_main.c | 34 +++++++++++++++----
 drivers/net/ethernet/microchip/lan743x_main.h |  3 ++
 2 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 4dc5adcda6a3..20a42a2c7b0e 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -28,9 +28,9 @@
 
 #define RFE_RD_FIFO_TH_3_DWORDS	0x3
 
-static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
+static int pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 {
-	u32 chip_rev;
+	u32 fpga_rev;
 	u32 cfg_load;
 	u32 hw_cfg;
 	u32 strap;
@@ -41,7 +41,7 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 	if (ret < 0) {
 		netif_err(adapter, drv, adapter->netdev,
 			  "Sys Lock acquire failed ret:%d\n", ret);
-		return;
+		return ret;
 	}
 
 	cfg_load = lan743x_csr_read(adapter, ETH_SYS_CONFIG_LOAD_STARTED_REG);
@@ -55,10 +55,15 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 			adapter->is_sgmii_en = true;
 		else
 			adapter->is_sgmii_en = false;
+
+		if ((strap & STRAP_SFP_USE_EN_) && (strap & STRAP_SFP_EN_))
+			adapter->is_sfp_support_en = true;
+		else
+			adapter->is_sfp_support_en = false;
 	} else {
-		chip_rev = lan743x_csr_read(adapter, FPGA_REV);
-		if (chip_rev) {
-			if (chip_rev & FPGA_SGMII_OP)
+		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
+		if (fpga_rev) {
+			if (fpga_rev & FPGA_SGMII_OP)
 				adapter->is_sgmii_en = true;
 			else
 				adapter->is_sgmii_en = false;
@@ -66,8 +71,21 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
 			adapter->is_sgmii_en = false;
 		}
 	}
+
+	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;
+	}
+
 	netif_dbg(adapter, drv, adapter->netdev,
 		  "SGMII I/F %sable\n", adapter->is_sgmii_en ? "En" : "Dis");
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "SFP support %sable\n", adapter->is_sfp_support_en ?
+		  "En" : "Dis");
+
+	return 0;
 }
 
 static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
@@ -3470,7 +3488,9 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 		adapter->max_tx_channels = PCI11X1X_MAX_TX_CHANNELS;
 		adapter->used_tx_channels = PCI11X1X_USED_TX_CHANNELS;
 		adapter->max_vector_count = PCI11X1X_MAX_VECTOR_COUNT;
-		pci11x1x_strap_get_status(adapter);
+		ret = pci11x1x_strap_get_status(adapter);
+		if (ret < 0)
+			return ret;
 		spin_lock_init(&adapter->eth_syslock_spinlock);
 		mutex_init(&adapter->sgmii_rw_lock);
 		pci11x1x_set_rfe_rd_fifo_threshold(adapter);
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index 8ef897c114d3..f7e96496600b 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -36,6 +36,8 @@
 
 #define STRAP_READ			(0x0C)
 #define STRAP_READ_USE_SGMII_EN_	BIT(22)
+#define STRAP_SFP_USE_EN_		BIT(31)
+#define STRAP_SFP_EN_			BIT(15)
 #define STRAP_READ_SGMII_EN_		BIT(6)
 #define STRAP_READ_SGMII_REFCLK_	BIT(5)
 #define STRAP_READ_SGMII_2_5G_		BIT(4)
@@ -1079,6 +1081,7 @@ struct lan743x_adapter {
 	u8			max_tx_channels;
 	u8			used_tx_channels;
 	u8			max_vector_count;
+	bool			is_sfp_support_en;
 
 #define LAN743X_ADAPTER_FLAG_OTP		BIT(0)
 	u32			flags;
-- 
2.34.1

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

[Christophe JAILLET]

Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> Support for SFP in the PCI11x1x devices is indicated by the "is_sfp_support_en"
> flag in the STRAP register. This register is loaded at power up from the
> PCI11x1x EEPROM contents (which specify the board configuration).
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> ---
> Change List:
> ============
> V1 -> V2:
>    - Change variable name from "chip_rev" to "fpga_rev"
> V0 -> V1:
>    - No changes
> 
>   drivers/net/ethernet/microchip/lan743x_main.c | 34 +++++++++++++++----
>   drivers/net/ethernet/microchip/lan743x_main.h |  3 ++
>   2 files changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
> index 4dc5adcda6a3..20a42a2c7b0e 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.c
> +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> @@ -28,9 +28,9 @@
>   
>   #define RFE_RD_FIFO_TH_3_DWORDS	0x3
>   
> -static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
> +static int pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>   {
> -	u32 chip_rev;
> +	u32 fpga_rev;
>   	u32 cfg_load;
>   	u32 hw_cfg;
>   	u32 strap;
> @@ -41,7 +41,7 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>   	if (ret < 0) {
>   		netif_err(adapter, drv, adapter->netdev,
>   			  "Sys Lock acquire failed ret:%d\n", ret);
> -		return;
> +		return ret;
>   	}
>   
>   	cfg_load = lan743x_csr_read(adapter, ETH_SYS_CONFIG_LOAD_STARTED_REG);
> @@ -55,10 +55,15 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>   			adapter->is_sgmii_en = true;
>   		else
>   			adapter->is_sgmii_en = false;
> +
> +		if ((strap & STRAP_SFP_USE_EN_) && (strap & STRAP_SFP_EN_))
> +			adapter->is_sfp_support_en = true;
> +		else
> +			adapter->is_sfp_support_en = false;
>   	} else {
> -		chip_rev = lan743x_csr_read(adapter, FPGA_REV);
> -		if (chip_rev) {
> -			if (chip_rev & FPGA_SGMII_OP)
> +		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
> +		if (fpga_rev) {
> +			if (fpga_rev & FPGA_SGMII_OP)
>   				adapter->is_sgmii_en = true;
>   			else
>   				adapter->is_sgmii_en = false;
> @@ -66,8 +71,21 @@ static void pci11x1x_strap_get_status(struct lan743x_adapter *adapter)
>   			adapter->is_sgmii_en = false;
>   		}
>   	}
> +
> +	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;
> +	}
> +
>   	netif_dbg(adapter, drv, adapter->netdev,
>   		  "SGMII I/F %sable\n", adapter->is_sgmii_en ? "En" : "Dis");
> +	netif_dbg(adapter, drv, adapter->netdev,
> +		  "SFP support %sable\n", adapter->is_sfp_support_en ?
> +		  "En" : "Dis");

Hi,

Maybe using str_enable_disable() or str_enabled_disabled()?

CJ

> +
> +	return 0;
>   }
>   
>   static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
> @@ -3470,7 +3488,9 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
>   		adapter->max_tx_channels = PCI11X1X_MAX_TX_CHANNELS;
>   		adapter->used_tx_channels = PCI11X1X_USED_TX_CHANNELS;
>   		adapter->max_vector_count = PCI11X1X_MAX_VECTOR_COUNT;
> -		pci11x1x_strap_get_status(adapter);
> +		ret = pci11x1x_strap_get_status(adapter);
> +		if (ret < 0)
> +			return ret;
>   		spin_lock_init(&adapter->eth_syslock_spinlock);
>   		mutex_init(&adapter->sgmii_rw_lock);
>   		pci11x1x_set_rfe_rd_fifo_threshold(adapter);
> diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
> index 8ef897c114d3..f7e96496600b 100644
> --- a/drivers/net/ethernet/microchip/lan743x_main.h
> +++ b/drivers/net/ethernet/microchip/lan743x_main.h
> @@ -36,6 +36,8 @@
>   
>   #define STRAP_READ			(0x0C)
>   #define STRAP_READ_USE_SGMII_EN_	BIT(22)
> +#define STRAP_SFP_USE_EN_		BIT(31)
> +#define STRAP_SFP_EN_			BIT(15)
>   #define STRAP_READ_SGMII_EN_		BIT(6)
>   #define STRAP_READ_SGMII_REFCLK_	BIT(5)
>   #define STRAP_READ_SGMII_2_5G_		BIT(4)
> @@ -1079,6 +1081,7 @@ struct lan743x_adapter {
>   	u8			max_tx_channels;
>   	u8			used_tx_channels;
>   	u8			max_vector_count;
> +	bool			is_sfp_support_en;
>   
>   #define LAN743X_ADAPTER_FLAG_OTP		BIT(0)
>   	u32			flags;

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

[Raju Lakkaraju]

Hi Christophe,

Thank you for review the patches.

The 09/11/2024 18:44, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> > Support for SFP in the PCI11x1x devices is indicated by the "is_sfp_support_en"
> > flag in the STRAP register. This register is loaded at power up from the
> > PCI11x1x EEPROM contents (which specify the board configuration).
> > 
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> > ---
> > Change List:
> > ============
> > V1 -> V2:
> >    - Change variable name from "chip_rev" to "fpga_rev"
> > V0 -> V1:
> >    - No changes
> > 
> >   drivers/net/ethernet/microchip/lan743x_main.c | 34 +++++++++++++++----
> >   drivers/net/ethernet/microchip/lan743x_main.h |  3 ++
> >   2 files changed, 30 insertions(+), 7 deletions(-)
> > 
> >       netif_dbg(adapter, drv, adapter->netdev,
> >                 "SGMII I/F %sable\n", adapter->is_sgmii_en ? "En" : "Dis");
> > +     netif_dbg(adapter, drv, adapter->netdev,
> > +               "SFP support %sable\n", adapter->is_sfp_support_en ?
> > +               "En" : "Dis");
> 
> Hi,
> 
> Maybe using str_enable_disable() or str_enabled_disabled()?
> 

Accepted. I will use str_enabled_disabled( ).

> CJ
> 
> > +
> > +     return 0;
> >   }
> > 
> 

-- 
Thanks,                                                                         
Raju

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

[Andrew Lunn]

> +	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,
phylink will tell you the mode to put the PCS into.

	Andrew

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

[Raju Lakkaraju]

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.

To differentiate between these interfaces, we need to enable or disable
the SGMII/1000Base-X/2500Base-X interface in the EEPROM.
This configuration is reflected in the STRAP_READ register (0x0C),
specifically bit 6.

According to the datasheet,
the "Strap Register (STRAP)" bit 6 is described as "SGMII_EN_STRAP"
Therefore, the flag is named "is_sgmii_en".

> phylink will tell you the mode to put the PCS into.

Yes. You are correct.
Based on PCS information, it will swich between SGMII or 1000Base-X or
2500Base-X I/F.

> 
>         Andrew

-- 
Thanks,                                                                         
Raju

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

[Andrew Lunn]

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

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@lunn.ch>
> Sent: Thursday, September 12, 2024 10:52 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; maxime.chevallier@bootlin.com;
> rdunlap@infradead.org; Steen Hegelund - M31857 <Steen.Hegelund@microchip.com>; Daniel Machon -
> M70577 <Daniel.Machon@microchip.com>; linux-kernel@vger.kernel.org
> 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

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

[Andrew Lunn]

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

> 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");

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.

     Andrew

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

[Ronnie.Kunin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, September 12, 2024 11:58 AM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>; netdev@vger.kernel.org;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Bryan
> Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; maxime.chevallier@bootlin.com;
> rdunlap@infradead.org; Steen Hegelund - M31857 <Steen.Hegelund@microchip.com>; Daniel Machon -
> M70577 <Daniel.Machon@microchip.com>; linux-kernel@vger.kernel.org
> 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

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

[Russell King (Oracle)]

On Thu, Sep 12, 2024 at 05:58:14PM +0200, Andrew Lunn wrote:
> > > > > > +     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.

While I can understand the desire to name stuff as documentation names
it, just because documentation calls an apple a banana does not mean
that everyone who doesn't have the documentation will understand that
is_a_banana will be true for an apple.

This is the problem here. SGMII has two meanings (and thanks to the
network industry for creating this in the first place).

First, there is Cisco SGMII, an adaptation of IEEE 802.3 1000base-X.
Secondly, there is its use as "Serial gigabit media independent
interface" which various manufacturers seem to use to refer to their
serial network interface supporting both Cisco SGMII, 1000base-X and
2500base-X.

_That_ is exactly where the problem is. "SGMII" is ambiguous. One can
not even use much in the way of context to separate out which it's
referring to, and naming a variable "is_sgmii_en" just doesn't have
the context. This ambiguous nature adds to the kernel maintenance
burden for those of us who look after subsystems.

It is unfortunate that people continue not to recognise this as the
problem that it is, but everyone loves acronyms... and acronyms are
a way of talking in code that excludes people from discussions or
understanding.

So, consider this a formal request _not_ to name your variable
"is_sgmii_en" but something else.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

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

[Russell King (Oracle)]

On Wed, Sep 11, 2024 at 09:40:50PM +0530, Raju Lakkaraju wrote:
>  {
> -	u32 chip_rev;
> +	u32 fpga_rev;
>  	u32 cfg_load;
>  	u32 hw_cfg;
>  	u32 strap;

...

>  	} else {
> -		chip_rev = lan743x_csr_read(adapter, FPGA_REV);
> -		if (chip_rev) {
> -			if (chip_rev & FPGA_SGMII_OP)
> +		fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
> +		if (fpga_rev) {
> +			if (fpga_rev & FPGA_SGMII_OP)

This looks like an unrelated change.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Raju Lakkaraju]

Register software nodes and define the device properties.
software-node contains following properties.
  - gpio pin details
  - i2c bus information
  - sfp signals
  - phylink mode

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V1 -> V2:
  - SFP GPIO definitions and other macros move from lan743x_main.c to
    lan743x_main.h file
  - Change from "PCI11X1X_" to "PCI11X1X_EVB_PCI11010_" strings for GPIO macros
V0 -> V1:                                                                       
  - No changes

 drivers/net/ethernet/microchip/Kconfig        |   2 +
 drivers/net/ethernet/microchip/lan743x_main.c | 198 +++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  82 ++++++++
 3 files changed, 278 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 2e3eb37a45cd..9c08a4af257a 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -50,6 +50,8 @@ config LAN743X
 	select CRC16
 	select CRC32
 	select PHYLINK
+	select I2C_PCI1XXXX
+	select GP_PCI1XXXX
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index 20a42a2c7b0e..dc571020ae1b 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -100,8 +100,98 @@ static bool is_pci11x1x_chip(struct lan743x_adapter *adapter)
 	return false;
 }
 
+static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
+					u16 perif_id)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct pci_bus *perif_bus;
+	struct pci_dev *perif_dev;
+	struct pci_dev *br_dev;
+	struct pci_bus *br_bus;
+	struct pci_dev *dev;
+
+	/* PCI11x1x devices' PCIe topology consists of a top level pcie
+	 * switch with up to four downstream ports, some of which have
+	 * integrated endpoints connected to them. One of the downstream ports
+	 * has an embedded single function pcie ethernet controller which is
+	 * handled by this driver. Another downstream port has an
+	 * embedded multifunction pcie endpoint, with four pcie functions
+	 * (the "peripheral controllers": I2C controller, GPIO controller,
+	 * UART controllers, SPIcontrollers)
+	 * The code below navigates the PCI11x1x topology
+	 * to find (by matching its PCI device ID) the peripheral controller
+	 * that should be paired to the embedded ethernet controller.
+	 */
+	br_dev = pci_upstream_bridge(pdev);
+	if (!br_dev) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "upstream bridge not found\n");
+		return br_dev;
+	}
+
+	br_bus = br_dev->bus;
+	list_for_each_entry(dev, &br_bus->devices, bus_list) {
+		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
+		    (dev->device & ~PCI1XXXX_DEV_MASK) ==
+		     PCI1XXXX_BR_PERIF_ID) {
+			perif_bus = dev->subordinate;
+			list_for_each_entry(perif_dev, &perif_bus->devices,
+					    bus_list) {
+				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
+				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
+				     perif_id)
+					return pci_get_drvdata(perif_dev);
+			}
+		}
+	}
+
+	netif_err(adapter, drv, adapter->netdev,
+		  "pci1xxxx peripheral (0x%X) device not found\n", perif_id);
+
+	return NULL;
+}
+
+static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
+{
+	struct pci1xxxx_i2c *i2c_drvdata;
+
+	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
+	if (!i2c_drvdata)
+		return -EPROBE_DEFER;
+
+	adapter->i2c_adap = &i2c_drvdata->adap;
+	snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
+		 adapter->i2c_adap->name);
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->i2c_adap->name);
+
+	return 0;
+}
+
+static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
+{
+	struct aux_bus_device *aux_bus;
+	struct device *gpio_dev;
+
+	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
+	if (!aux_bus)
+		return -EPROBE_DEFER;
+
+	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
+	snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
+		 dev_name(gpio_dev));
+	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
+		  adapter->nodes->gpio_name);
+	return 0;
+}
+
 static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->nodes) {
+		software_node_unregister_node_group(adapter->nodes->group);
+		kfree(adapter->nodes);
+	}
+
 	pci_release_selected_regions(adapter->pdev,
 				     pci_select_bars(adapter->pdev,
 						     IORESOURCE_MEM));
@@ -2888,6 +2978,90 @@ static int lan743x_rx_open(struct lan743x_rx *rx)
 	return ret;
 }
 
+static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct lan743x_sw_nodes *nodes;
+	struct software_node *swnodes;
+	int ret;
+	u32 id;
+
+	nodes = kzalloc(sizeof(*nodes), GFP_KERNEL);
+	if (!nodes)
+		return -ENOMEM;
+
+	adapter->nodes = nodes;
+
+	ret = pci1xxxx_gpio_dev_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	ret = pci1xxxx_i2c_adapter_get(adapter);
+	if (ret < 0)
+		return ret;
+
+	id = (pdev->bus->number << 8) | pdev->devfn;
+	snprintf(nodes->sfp_name, sizeof(nodes->sfp_name),
+		 "sfp-%d", id);
+	snprintf(nodes->phylink_name, sizeof(nodes->phylink_name),
+		 "mchp-pci1xxxx-phylink-%d", id);
+
+	swnodes = nodes->swnodes;
+
+	nodes->gpio_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names",
+						     "default");
+	swnodes[SWNODE_GPIO] = NODE_PROP(nodes->gpio_name, nodes->gpio_props);
+
+	nodes->tx_fault_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_EVB_PCI11010_TX_FAULT_GPIO,
+							 GPIO_ACTIVE_HIGH);
+	nodes->tx_disable_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							   PCI11X1X_EVB_PCI11010_TX_DIS_GPIO,
+							   GPIO_ACTIVE_HIGH);
+	nodes->mod_def0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							 PCI11X1X_EVB_PCI11010_MOD_DEF0_GPIO,
+							 GPIO_ACTIVE_LOW);
+	nodes->los_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+						    PCI11X1X_EVB_PCI11010_LOS_GPIO,
+						    GPIO_ACTIVE_HIGH);
+	nodes->rate_sel0_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_GPIO],
+							  PCI11X1X_EVB_PCI11010_RATE_SEL0_GPIO,
+							  GPIO_ACTIVE_HIGH);
+
+	nodes->i2c_props[0] = PROPERTY_ENTRY_STRING("pinctrl-names", "default");
+	swnodes[SWNODE_I2C] = NODE_PROP(nodes->i2c_name, nodes->i2c_props);
+	nodes->i2c_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_I2C]);
+
+	nodes->sfp_props[0] = PROPERTY_ENTRY_STRING("compatible", "sff,sfp");
+	nodes->sfp_props[1] = PROPERTY_ENTRY_REF_ARRAY("i2c-bus",
+						       nodes->i2c_ref);
+	nodes->sfp_props[2] = PROPERTY_ENTRY_REF_ARRAY("tx-fault-gpios",
+						       nodes->tx_fault_ref);
+	nodes->sfp_props[3] = PROPERTY_ENTRY_REF_ARRAY("tx-disable-gpios",
+						       nodes->tx_disable_ref);
+	nodes->sfp_props[4] = PROPERTY_ENTRY_REF_ARRAY("mod-def0-gpios",
+						       nodes->mod_def0_ref);
+	nodes->sfp_props[5] = PROPERTY_ENTRY_REF_ARRAY("los-gpios",
+						       nodes->los_ref);
+	nodes->sfp_props[6] = PROPERTY_ENTRY_REF_ARRAY("rate-select0-gpios",
+						       nodes->rate_sel0_ref);
+	swnodes[SWNODE_SFP] = NODE_PROP(nodes->sfp_name, nodes->sfp_props);
+	nodes->sfp_ref[0] = SOFTWARE_NODE_REFERENCE(&swnodes[SWNODE_SFP]);
+	nodes->phylink_props[0] = PROPERTY_ENTRY_STRING("managed",
+							"in-band-status");
+	nodes->phylink_props[1] = PROPERTY_ENTRY_REF_ARRAY("sfp",
+							   nodes->sfp_ref);
+	swnodes[SWNODE_PHYLINK] = NODE_PROP(nodes->phylink_name,
+					    nodes->phylink_props);
+
+	nodes->group[SWNODE_GPIO] = &swnodes[SWNODE_GPIO];
+	nodes->group[SWNODE_I2C] = &swnodes[SWNODE_I2C];
+	nodes->group[SWNODE_SFP] = &swnodes[SWNODE_SFP];
+	nodes->group[SWNODE_PHYLINK] = &swnodes[SWNODE_PHYLINK];
+
+	return software_node_register_node_group(nodes->group);
+}
+
 static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
 {
 	u32 sgmii_ctl;
@@ -3105,6 +3279,7 @@ static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
 static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 {
 	struct net_device *netdev = adapter->netdev;
+	struct fwnode_handle *fwnode = NULL;
 	struct phylink *pl;
 
 	adapter->phylink_config.dev = &netdev->dev;
@@ -3138,7 +3313,13 @@ static int lan743x_phylink_create(struct lan743x_adapter *adapter)
 		phy_interface_set_rgmii(adapter->phylink_config.supported_interfaces);
 	}
 
-	pl = phylink_create(&adapter->phylink_config, NULL,
+	if (adapter->nodes) {
+		fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_PHYLINK]);
+		if (!fwnode)
+			return -ENODEV;
+	}
+
+	pl = phylink_create(&adapter->phylink_config, fwnode,
 			    adapter->phy_interface, &lan743x_phylink_mac_ops);
 
 	if (IS_ERR(pl)) {
@@ -3511,9 +3692,18 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 	if (ret)
 		return ret;
 
-	ret = lan743x_phy_init(adapter);
-	if (ret)
-		return ret;
+	if (adapter->is_sfp_support_en && !adapter->nodes) {
+		ret = lan743x_swnodes_register(adapter);
+		if (ret) {
+			netdev_err(adapter->netdev,
+				   "failed to register software nodes\n");
+			return ret;
+		}
+	} else {
+		ret = lan743x_phy_init(adapter);
+		if (ret)
+			return ret;
+	}
 
 	ret = lan743x_ptp_init(adapter);
 	if (ret)
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index f7e96496600b..bf0d0f285e39 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -6,6 +6,10 @@
 
 #include <linux/phy.h>
 #include <linux/phylink.h>
+#include <linux/property.h>
+#include <linux/i2c.h>
+#include <linux/gpio/machine.h>
+#include <linux/auxiliary_bus.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
@@ -1047,6 +1051,40 @@ enum lan743x_sgmii_lsd {
 
 #define MAC_SUPPORTED_WAKES  (WAKE_BCAST | WAKE_UCAST | WAKE_MCAST | \
 			      WAKE_MAGIC | WAKE_ARP)
+
+enum lan743x_swnodes {
+	SWNODE_GPIO = 0,
+	SWNODE_I2C,
+	SWNODE_SFP,
+	SWNODE_PHYLINK,
+	SWNODE_MAX
+};
+
+#define I2C_DRV_NAME		48
+#define GPIO_DRV_NAME		32
+#define SFP_NODE_NAME		32
+#define PHYLINK_NODE_NAME	32
+
+struct lan743x_sw_nodes {
+	char gpio_name[GPIO_DRV_NAME];
+	char i2c_name[I2C_DRV_NAME];
+	char sfp_name[SFP_NODE_NAME];
+	char phylink_name[PHYLINK_NODE_NAME];
+	struct property_entry gpio_props[1];
+	struct property_entry i2c_props[1];
+	struct property_entry sfp_props[8];
+	struct property_entry phylink_props[2];
+	struct software_node_ref_args i2c_ref[1];
+	struct software_node_ref_args tx_fault_ref[1];
+	struct software_node_ref_args tx_disable_ref[1];
+	struct software_node_ref_args mod_def0_ref[1];
+	struct software_node_ref_args los_ref[1];
+	struct software_node_ref_args rate_sel0_ref[1];
+	struct software_node_ref_args sfp_ref[1];
+	struct software_node swnodes[SWNODE_MAX];
+	const struct software_node *group[SWNODE_MAX + 1];
+};
+
 struct lan743x_adapter {
 	struct net_device       *netdev;
 	struct mii_bus		*mdiobus;
@@ -1089,6 +1127,8 @@ struct lan743x_adapter {
 	phy_interface_t		phy_interface;
 	struct phylink		*phylink;
 	struct phylink_config	phylink_config;
+	struct lan743x_sw_nodes	*nodes;
+	struct i2c_adapter	*i2c_adap;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
@@ -1202,6 +1242,48 @@ struct lan743x_rx_buffer_info {
 #define RX_PROCESS_RESULT_NOTHING_TO_DO     (0)
 #define RX_PROCESS_RESULT_BUFFER_RECEIVED   (1)
 
+#define PCI1XXXX_VENDOR_ID      0x1055
+#define PCI1XXXX_BR_PERIF_ID    0xA00C
+#define PCI1XXXX_PERIF_I2C_ID   0xA003
+#define PCI1XXXX_PERIF_GPIO_ID  0xA005
+#define PCI1XXXX_DEV_MASK       GENMASK(7, 4)
+
+#define PCI11X1X_EVB_PCI11010_TX_FAULT_GPIO  46
+#define PCI11X1X_EVB_PCI11010_TX_DIS_GPIO    47
+#define PCI11X1X_EVB_PCI11010_RATE_SEL0_GPIO 48
+#define PCI11X1X_EVB_PCI11010_LOS_GPIO       49
+#define PCI11X1X_EVB_PCI11010_MOD_DEF0_GPIO  51
+
+#define NODE_PROP(_NAME, _PROP)         \
+	((const struct software_node) { \
+		.name = _NAME,          \
+		.properties = _PROP,    \
+	})
+
+struct pci1xxxx_i2c {
+	struct completion i2c_xfer_done;
+	bool i2c_xfer_in_progress;
+	struct i2c_adapter adap;
+	void __iomem *i2c_base;
+	u32 freq;
+	u32 flags;
+};
+
+struct gp_aux_data_type {
+	int irq_num;
+	resource_size_t region_start;
+	resource_size_t region_length;
+};
+
+struct auxiliary_device_wrapper {
+	struct auxiliary_device aux_dev;
+	struct gp_aux_data_type gp_aux_data;
+};
+
+struct aux_bus_device {
+	struct auxiliary_device_wrapper *aux_device_wrapper[2];
+};
+
 u32 lan743x_csr_read(struct lan743x_adapter *adapter, int offset);
 void lan743x_csr_write(struct lan743x_adapter *adapter, int offset, u32 data);
 int lan743x_hs_syslock_acquire(struct lan743x_adapter *adapter, u16 timeout);
-- 
2.34.1

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Christophe JAILLET]

Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> Register software nodes and define the device properties.
> software-node contains following properties.
>    - gpio pin details
>    - i2c bus information
>    - sfp signals
>    - phylink mode
> 
> Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>

Hi,

...

> +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> +{
> +	struct pci1xxxx_i2c *i2c_drvdata;
> +
> +	i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> +	if (!i2c_drvdata)
> +		return -EPROBE_DEFER;
> +
> +	adapter->i2c_adap = &i2c_drvdata->adap;
> +	snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> +		 adapter->i2c_adap->name);

strscpy() ?

> +	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> +		  adapter->i2c_adap->name);
> +
> +	return 0;
> +}
> +
> +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> +{
> +	struct aux_bus_device *aux_bus;
> +	struct device *gpio_dev;
> +
> +	aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> +	if (!aux_bus)
> +		return -EPROBE_DEFER;
> +
> +	gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> +	snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> +		 dev_name(gpio_dev));

strscpy() ?

> +	netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> +		  adapter->nodes->gpio_name);
> +	return 0;
> +}
> +

...

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Raju Lakkaraju]

Hi Christophe,

The 09/11/2024 18:54, Christophe JAILLET wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Le 11/09/2024 à 18:10, Raju Lakkaraju a écrit :
> > Register software nodes and define the device properties.
> > software-node contains following properties.
> >    - gpio pin details
> >    - i2c bus information
> >    - sfp signals
> >    - phylink mode
> > 
> > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
> 
> Hi,
> 
> ...
> 
> > +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct pci1xxxx_i2c *i2c_drvdata;
> > +
> > +     i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> > +     if (!i2c_drvdata)
> > +             return -EPROBE_DEFER;
> > +
> > +     adapter->i2c_adap = &i2c_drvdata->adap;
> > +     snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> > +              adapter->i2c_adap->name);
> 
> strscpy() ?
> 

Accepted. I will fix.
Here snprintf( ) does not take any format string, we can use strscpy( ).
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?
> 

Accepted. I will fix.
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?

Accepted. I will fix.
 
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->i2c_adap->name);
> > +
> > +     return 0;
> > +}
> > +
> > +static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
> > +{
> > +     struct aux_bus_device *aux_bus;
> > +     struct device *gpio_dev;
> > +
> > +     aux_bus = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_GPIO_ID);
> > +     if (!aux_bus)
> > +             return -EPROBE_DEFER;
> > +
> > +     gpio_dev = &aux_bus->aux_device_wrapper[1]->aux_dev.dev;
> > +     snprintf(adapter->nodes->gpio_name, sizeof(adapter->nodes->gpio_name),
> > +              dev_name(gpio_dev));
> 
> strscpy() ?
> > +     netif_dbg(adapter, drv, adapter->netdev, "Found %s\n",
> > +               adapter->nodes->gpio_name);
> > +     return 0;
> > +}
> > +
> 
> ...

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Russell King (Oracle)]

On Thu, Sep 12, 2024 at 12:02:57PM +0530, Raju Lakkaraju wrote:
> Hi Christophe,
> 
> The 09/11/2024 18:54, Christophe JAILLET wrote:
> > > +static int pci1xxxx_i2c_adapter_get(struct lan743x_adapter *adapter)
> > > +{
> > > +     struct pci1xxxx_i2c *i2c_drvdata;
> > > +
> > > +     i2c_drvdata = pci1xxxx_perif_drvdata_get(adapter, PCI1XXXX_PERIF_I2C_ID);
> > > +     if (!i2c_drvdata)
> > > +             return -EPROBE_DEFER;
> > > +
> > > +     adapter->i2c_adap = &i2c_drvdata->adap;
> > > +     snprintf(adapter->nodes->i2c_name, sizeof(adapter->nodes->i2c_name),
> > > +              adapter->i2c_adap->name);
> > 
> > strscpy() ?
> > 
> 
> Accepted. I will fix.
> Here snprintf( ) does not take any format string, we can use strscpy( ).

As a general tip for safe programming... never use snprintf() as a
"short cut" for copying strings. It may do stuff that you don't
expect!

For example, taking the above case, if "adapter->i2c_adap->name"
contains any % characters, then, as you are passing it as the
_format_ _string_, sprintf() will try to interpret those as printf
escape sequences, and thus _can_ attempt to dereference arguments
that were never passed to snprintf().

If you really want to do this kind of thing, at least write it in
a safe way...

	snprintf(..., "%s", string);

rather than:

	snprintf(..., string);

so that "string" doesn't attempt to be escape-expanded.

Of course, using proper string copying functions that do what you
want in a cheap way is always more preferable to the printf related
functions!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Andrew Lunn]

> If you really want to do this kind of thing, at least write it in
> a safe way...
> 
> 	snprintf(..., "%s", string);
> 
> rather than:
> 
> 	snprintf(..., string);
> 
> so that "string" doesn't attempt to be escape-expanded.

One of the static analysers is complaining about this danger, or it
might be GCC itself if you up the warning level. The kernel hardening
people are replacing all such bad cases, one by one. So we definitely
don't want to add more.

	Andrew

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Andrew Lunn]

> diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> index 2e3eb37a45cd..9c08a4af257a 100644
> --- a/drivers/net/ethernet/microchip/Kconfig
> +++ b/drivers/net/ethernet/microchip/Kconfig
> @@ -50,6 +50,8 @@ config LAN743X
>  	select CRC16
>  	select CRC32
>  	select PHYLINK
> +	select I2C_PCI1XXXX
> +	select GP_PCI1XXXX

GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
is not in 6.11-rc7. Is it in gpio-next?

> +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> +					u16 perif_id)
> +{
> +	struct pci_dev *pdev = adapter->pdev;
> +	struct pci_bus *perif_bus;
> +	struct pci_dev *perif_dev;
> +	struct pci_dev *br_dev;
> +	struct pci_bus *br_bus;
> +	struct pci_dev *dev;
> +
> +	/* PCI11x1x devices' PCIe topology consists of a top level pcie
> +	 * switch with up to four downstream ports, some of which have
> +	 * integrated endpoints connected to them. One of the downstream ports
> +	 * has an embedded single function pcie ethernet controller which is
> +	 * handled by this driver. Another downstream port has an
> +	 * embedded multifunction pcie endpoint, with four pcie functions
> +	 * (the "peripheral controllers": I2C controller, GPIO controller,
> +	 * UART controllers, SPIcontrollers)
> +	 * The code below navigates the PCI11x1x topology
> +	 * to find (by matching its PCI device ID) the peripheral controller
> +	 * that should be paired to the embedded ethernet controller.
> +	 */
> +	br_dev = pci_upstream_bridge(pdev);
> +	if (!br_dev) {
> +		netif_err(adapter, drv, adapter->netdev,
> +			  "upstream bridge not found\n");
> +		return br_dev;
> +	}
> +
> +	br_bus = br_dev->bus;
> +	list_for_each_entry(dev, &br_bus->devices, bus_list) {
> +		if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> +		    (dev->device & ~PCI1XXXX_DEV_MASK) ==
> +		     PCI1XXXX_BR_PERIF_ID) {
> +			perif_bus = dev->subordinate;
> +			list_for_each_entry(perif_dev, &perif_bus->devices,
> +					    bus_list) {
> +				if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> +				    (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> +				     perif_id)
> +					return pci_get_drvdata(perif_dev);
> +			}
> +		}
> +	}

It would be good to have the PCI Maintainers review of this. Maybe
pull this out into a patch of its own and Cc: Bjorn Helgaas
<bhelgaas@google.com>

	Andrew

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Raju Lakkaraju]

Hi Andrew,

The 09/11/2024 19:17, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > index 2e3eb37a45cd..9c08a4af257a 100644
> > --- a/drivers/net/ethernet/microchip/Kconfig
> > +++ b/drivers/net/ethernet/microchip/Kconfig
> > @@ -50,6 +50,8 @@ config LAN743X
> >       select CRC16
> >       select CRC32
> >       select PHYLINK
> > +     select I2C_PCI1XXXX
> > +     select GP_PCI1XXXX
> 
> GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> is not in 6.11-rc7. Is it in gpio-next?
> 

Yes. But GPIO driver developer use this.
I have to use the same here.

It's exist in 6.11-rc6
drivers/misc/mchp_pci1xxxx/Kconfig
This was defined along back.

>Is it in gpio-next? 
No. available in net-next

> > +static void *pci1xxxx_perif_drvdata_get(struct lan743x_adapter *adapter,
> > +                                     u16 perif_id)
> > +{
> > +     struct pci_dev *pdev = adapter->pdev;
> > +     struct pci_bus *perif_bus;
> > +     struct pci_dev *perif_dev;
> > +     struct pci_dev *br_dev;
> > +     struct pci_bus *br_bus;
> > +     struct pci_dev *dev;
> > +
> > +     /* PCI11x1x devices' PCIe topology consists of a top level pcie
> > +      * switch with up to four downstream ports, some of which have
> > +      * integrated endpoints connected to them. One of the downstream ports
> > +      * has an embedded single function pcie ethernet controller which is
> > +      * handled by this driver. Another downstream port has an
> > +      * embedded multifunction pcie endpoint, with four pcie functions
> > +      * (the "peripheral controllers": I2C controller, GPIO controller,
> > +      * UART controllers, SPIcontrollers)
> > +      * The code below navigates the PCI11x1x topology
> > +      * to find (by matching its PCI device ID) the peripheral controller
> > +      * that should be paired to the embedded ethernet controller.
> > +      */
> > +     br_dev = pci_upstream_bridge(pdev);
> > +     if (!br_dev) {
> > +             netif_err(adapter, drv, adapter->netdev,
> > +                       "upstream bridge not found\n");
> > +             return br_dev;
> > +     }
> > +
> > +     br_bus = br_dev->bus;
> > +     list_for_each_entry(dev, &br_bus->devices, bus_list) {
> > +             if (dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                 (dev->device & ~PCI1XXXX_DEV_MASK) ==
> > +                  PCI1XXXX_BR_PERIF_ID) {
> > +                     perif_bus = dev->subordinate;
> > +                     list_for_each_entry(perif_dev, &perif_bus->devices,
> > +                                         bus_list) {
> > +                             if (perif_dev->vendor == PCI1XXXX_VENDOR_ID &&
> > +                                 (perif_dev->device & ~PCI1XXXX_DEV_MASK) ==
> > +                                  perif_id)
> > +                                     return pci_get_drvdata(perif_dev);
> > +                     }
> > +             }
> > +     }
> 
> It would be good to have the PCI Maintainers review of this. Maybe
> pull this out into a patch of its own and Cc: Bjorn Helgaas
> <bhelgaas@google.com>

Sure. I will add Cc: Bjorn Helgaas.

> 
>         Andrew

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Andrew Lunn]

On Thu, Sep 12, 2024 at 12:08:40PM +0530, Raju Lakkaraju wrote:
> Hi Andrew,
> 
> The 09/11/2024 19:17, Andrew Lunn wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > > index 2e3eb37a45cd..9c08a4af257a 100644
> > > --- a/drivers/net/ethernet/microchip/Kconfig
> > > +++ b/drivers/net/ethernet/microchip/Kconfig
> > > @@ -50,6 +50,8 @@ config LAN743X
> > >       select CRC16
> > >       select CRC32
> > >       select PHYLINK
> > > +     select I2C_PCI1XXXX
> > > +     select GP_PCI1XXXX
> > 
> > GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> > is not in 6.11-rc7. Is it in gpio-next?
> > 
> 
> Yes. But GPIO driver developer use this.

And the GPIO Maintainer accepted this, despite it not being the same
as every other GPIO driver?

Ah, there is no Acked-by: from anybody i recognise as a GPIO
maintainer. Was it even reviewed by GPIO people? And why is it hiding
in driver/misc? I don't see any reason it cannot be in drivers/gpio,
which is where i looked for it. There are other auxiliary_driver in
drivers/gpio.

> I have to use the same here.

Unfortunately, i have to agree, for the moment.

But it would be good to clean it up. I _think_ mchp_pci1xxxx_gpio.c
can be moved into driver/gpio, given the expected Kconfig symbol
GPIO_PCI1XXXX and depends on GP_PCI1XXXX. It would then also get
reviewed by the GPIO Maintainers, which you probably want.

	Andrew

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[Russell King (Oracle)]

On Thu, Sep 12, 2024 at 05:19:37PM +0200, Andrew Lunn wrote:
> On Thu, Sep 12, 2024 at 12:08:40PM +0530, Raju Lakkaraju wrote:
> > Hi Andrew,
> > 
> > The 09/11/2024 19:17, Andrew Lunn wrote:
> > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > > 
> > > > diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
> > > > index 2e3eb37a45cd..9c08a4af257a 100644
> > > > --- a/drivers/net/ethernet/microchip/Kconfig
> > > > +++ b/drivers/net/ethernet/microchip/Kconfig
> > > > @@ -50,6 +50,8 @@ config LAN743X
> > > >       select CRC16
> > > >       select CRC32
> > > >       select PHYLINK
> > > > +     select I2C_PCI1XXXX
> > > > +     select GP_PCI1XXXX
> > > 
> > > GP_ is odd. GPIO drivers usually use GPIO_. Saying that, GPIO_PCI1XXXX
> > > is not in 6.11-rc7. Is it in gpio-next?
> > > 
> > 
> > Yes. But GPIO driver developer use this.
> 
> And the GPIO Maintainer accepted this, despite it not being the same
> as every other GPIO driver?
> 
> Ah, there is no Acked-by: from anybody i recognise as a GPIO
> maintainer. Was it even reviewed by GPIO people? And why is it hiding
> in driver/misc? I don't see any reason it cannot be in drivers/gpio,
> which is where i looked for it. There are other auxiliary_driver in
> drivers/gpio.


What's worse is that the Link: in the commit adding it doesn't
work!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp

[kernel test robot]

Hi Raju,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Lakkaraju/net-lan743x-Add-SFP-support-check-flag/20240912-002444
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240911161054.4494-3-Raju.Lakkaraju%40microchip.com
patch subject: [PATCH net-next V2 2/5] net: lan743x: Add support to software-nodes for sfp
config: x86_64-randconfig-003-20240914 (https://download.01.org/0day-ci/archive/20240915/202409150110.dSOZKgpK-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409150110.dSOZKgpK-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409150110.dSOZKgpK-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: devm_i2c_add_adapter
   >>> referenced by i2c-mchp-pci1xxxx.c:1182 (drivers/i2c/busses/i2c-mchp-pci1xxxx.c:1182)
   >>>               drivers/i2c/busses/i2c-mchp-pci1xxxx.o:(pci1xxxx_i2c_probe_pci) in archive vmlinux.a

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GP_PCI1XXXX
   Depends on [n]: PCI [=y] && GPIOLIB [=y] && NVMEM_SYSFS [=n]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]
   WARNING: unmet direct dependencies detected for I2C_PCI1XXXX
   Depends on [m]: I2C [=m] && HAS_IOMEM [=y] && PCI [=y]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next V2 3/5] net: lan743x: Register the platform device for sfp pluggable module

[Raju Lakkaraju]

Add support for sfp pluggable module as platform device handle the gpio
input and output signals and i2c bus access the sfp eeprom data.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V1 -> V2:
  - Add platform_device_unregister( ) when sfp register fail 
V0 -> V1:
  - No change

 drivers/net/ethernet/microchip/Kconfig        |  1 +
 drivers/net/ethernet/microchip/lan743x_main.c | 43 +++++++++++++++++++
 drivers/net/ethernet/microchip/lan743x_main.h |  1 +
 3 files changed, 45 insertions(+)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 9c08a4af257a..3dacf39b49b4 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -52,6 +52,7 @@ config LAN743X
 	select PHYLINK
 	select I2C_PCI1XXXX
 	select GP_PCI1XXXX
+	select SFP
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index dc571020ae1b..c1061e2972f9 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -16,6 +16,7 @@
 #include <linux/iopoll.h>
 #include <linux/crc16.h>
 #include <linux/phylink.h>
+#include <linux/platform_device.h>
 #include "lan743x_main.h"
 #include "lan743x_ethtool.h"
 
@@ -187,6 +188,11 @@ static int pci1xxxx_gpio_dev_get(struct lan743x_adapter *adapter)
 
 static void lan743x_pci_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->sfp_dev) {
+		platform_device_unregister(adapter->sfp_dev);
+		adapter->sfp_dev = NULL;
+	}
+
 	if (adapter->nodes) {
 		software_node_unregister_node_group(adapter->nodes->group);
 		kfree(adapter->nodes);
@@ -3062,6 +3068,31 @@ static int lan743x_swnodes_register(struct lan743x_adapter *adapter)
 	return software_node_register_node_group(nodes->group);
 }
 
+static int lan743x_sfp_register(struct lan743x_adapter *adapter)
+{
+	struct pci_dev *pdev = adapter->pdev;
+	struct platform_device_info sfp_info;
+	struct platform_device *sfp_dev;
+
+	memset(&sfp_info, 0, sizeof(sfp_info));
+	sfp_info.parent = &adapter->pdev->dev;
+	sfp_info.fwnode = software_node_fwnode(adapter->nodes->group[SWNODE_SFP]);
+	sfp_info.name = "sfp";
+	sfp_info.id = (pdev->bus->number << 8) | pdev->devfn;
+	sfp_dev = platform_device_register_full(&sfp_info);
+	if (IS_ERR(sfp_dev)) {
+		netif_err(adapter, drv, adapter->netdev,
+			  "Failed to register SFP device\n");
+		return PTR_ERR(sfp_dev);
+	}
+
+	adapter->sfp_dev = sfp_dev;
+	netif_dbg(adapter, drv, adapter->netdev,
+		  "SFP platform device registered");
+
+	return 0;
+}
+
 static int lan743x_phylink_sgmii_config(struct lan743x_adapter *adapter)
 {
 	u32 sgmii_ctl;
@@ -3851,6 +3882,18 @@ static int lan743x_pcidev_probe(struct pci_dev *pdev,
 	if (ret)
 		goto cleanup_pci;
 
+	if (adapter->is_sfp_support_en) {
+		adapter->i2c_adap->dev.fwnode =
+			software_node_fwnode(adapter->nodes->group[SWNODE_I2C]);
+
+		ret = lan743x_sfp_register(adapter);
+		if (ret < 0) {
+			netif_err(adapter, probe, netdev,
+				  "Failed to register sfp (%d)\n", ret);
+			goto cleanup_pci;
+		}
+	}
+
 	ret = lan743x_mdiobus_init(adapter);
 	if (ret)
 		goto cleanup_hardware;
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index bf0d0f285e39..c303a69c3bea 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -1129,6 +1129,7 @@ struct lan743x_adapter {
 	struct phylink_config	phylink_config;
 	struct lan743x_sw_nodes	*nodes;
 	struct i2c_adapter	*i2c_adap;
+	struct platform_device	*sfp_dev;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1

Re: [PATCH net-next V2 3/5] net: lan743x: Register the platform device for sfp pluggable module

[kernel test robot]

Hi Raju,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Raju-Lakkaraju/net-lan743x-Add-SFP-support-check-flag/20240912-002444
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240911161054.4494-4-Raju.Lakkaraju%40microchip.com
patch subject: [PATCH net-next V2 3/5] net: lan743x: Register the platform device for sfp pluggable module
config: x86_64-randconfig-003-20240914 (https://download.01.org/0day-ci/archive/20240915/202409151058.rOgbMAJJ-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240915/202409151058.rOgbMAJJ-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409151058.rOgbMAJJ-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: i2c_get_adapter_by_fwnode
   >>> referenced by sfp.c:2981 (drivers/net/phy/sfp.c:2981)
   >>>               drivers/net/phy/sfp.o:(sfp_probe) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: i2c_put_adapter
   >>> referenced by sfp.c:2989 (drivers/net/phy/sfp.c:2989)
   >>>               drivers/net/phy/sfp.o:(sfp_probe) in archive vmlinux.a
   >>> referenced by sfp.c:2965 (drivers/net/phy/sfp.c:2965)
   >>>               drivers/net/phy/sfp.o:(sfp_cleanup) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: hwmon_device_unregister
   >>> referenced by sfp.c:1640 (drivers/net/phy/sfp.c:1640)
   >>>               drivers/net/phy/sfp.o:(__sfp_sm_event) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: mdio_i2c_alloc
   >>> referenced by sfp.c:707 (drivers/net/phy/sfp.c:707)
   >>>               drivers/net/phy/sfp.o:(__sfp_sm_event) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: hwmon_sanitize_name
   >>> referenced by sfp.c:1611 (drivers/net/phy/sfp.c:1611)
   >>>               drivers/net/phy/sfp.o:(sfp_hwmon_probe) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: hwmon_device_register_with_info
   >>> referenced by sfp.c:1617 (drivers/net/phy/sfp.c:1617)
   >>>               drivers/net/phy/sfp.o:(sfp_hwmon_probe) in archive vmlinux.a
--
>> ld.lld: error: undefined symbol: i2c_transfer
   >>> referenced by sfp.c:648 (drivers/net/phy/sfp.c:648)
   >>>               drivers/net/phy/sfp.o:(sfp_i2c_read) in archive vmlinux.a
   >>> referenced by sfp.c:680 (drivers/net/phy/sfp.c:680)
   >>>               drivers/net/phy/sfp.o:(sfp_i2c_write) in archive vmlinux.a

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for GP_PCI1XXXX
   Depends on [n]: PCI [=y] && GPIOLIB [=y] && NVMEM_SYSFS [=n]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]
   WARNING: unmet direct dependencies detected for SFP
   Depends on [m]: NETDEVICES [=y] && PHYLIB [=y] && I2C [=m] && PHYLINK [=y] && (HWMON [=m] || HWMON [=m]=n [=n])
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]
   WARNING: unmet direct dependencies detected for I2C_PCI1XXXX
   Depends on [m]: I2C [=m] && HAS_IOMEM [=y] && PCI [=y]
   Selected by [y]:
   - LAN743X [=y] && NETDEVICES [=y] && ETHERNET [=y] && NET_VENDOR_MICROCHIP [=y] && PCI [=y] && PTP_1588_CLOCK_OPTIONAL [=y]

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Raju Lakkaraju]

Register MDIO bus for PCS layer to use Synopsys designware XPCS, support
SGMII/1000Base-X/2500Base-X interfaces.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============                                                                    
V2 : Include this new patch in the V2 series. 

 drivers/net/ethernet/microchip/Kconfig        |  1 +
 drivers/net/ethernet/microchip/lan743x_main.c | 72 ++++++++++++++++++-
 drivers/net/ethernet/microchip/lan743x_main.h |  2 +
 3 files changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/microchip/Kconfig b/drivers/net/ethernet/microchip/Kconfig
index 3dacf39b49b4..793a20ef51fc 100644
--- a/drivers/net/ethernet/microchip/Kconfig
+++ b/drivers/net/ethernet/microchip/Kconfig
@@ -53,6 +53,7 @@ config LAN743X
 	select I2C_PCI1XXXX
 	select GP_PCI1XXXX
 	select SFP
+	select PCS_XPCS
 	help
 	  Support for the Microchip LAN743x and PCI11x1x families of PCI
 	  Express Ethernet devices
diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index c1061e2972f9..ef76d0c1642f 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -1143,6 +1143,28 @@ static int lan743x_get_lsd(int speed, int duplex, u8 mss)
 	return lsd;
 }
 
+static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
+			     int regnum)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_read(adapter, devnum, regnum);
+}
+
+static int pci11x1x_pcs_write(struct mii_bus *bus, int addr, int devnum,
+			      int regnum, u16 val)
+{
+	struct lan743x_adapter *adapter = bus->priv;
+
+	if (addr)
+		return -EOPNOTSUPP;
+
+	return lan743x_sgmii_write(adapter, devnum, regnum, val);
+}
+
 static int lan743x_sgmii_mpll_set(struct lan743x_adapter *adapter,
 				  u16 baud)
 {
@@ -3201,6 +3223,19 @@ void lan743x_mac_eee_enable(struct lan743x_adapter *adapter, bool enable)
 	lan743x_csr_write(adapter, MAC_CR, mac_cr);
 }
 
+static struct phylink_pcs *
+lan743x_phylink_mac_select_pcs(struct phylink_config *config,
+			       phy_interface_t interface)
+{
+	struct net_device *netdev = to_net_dev(config->dev);
+	struct lan743x_adapter *adapter = netdev_priv(netdev);
+
+	if (adapter->xpcs)
+		return &adapter->xpcs->pcs;
+
+	return NULL;
+}
+
 static void lan743x_phylink_mac_config(struct phylink_config *config,
 				       unsigned int link_an_mode,
 				       const struct phylink_link_state *state)
@@ -3302,6 +3337,7 @@ static void lan743x_phylink_mac_link_up(struct phylink_config *config,
 }
 
 static const struct phylink_mac_ops lan743x_phylink_mac_ops = {
+	.mac_select_pcs = lan743x_phylink_mac_select_pcs,
 	.mac_config = lan743x_phylink_mac_config,
 	.mac_link_down = lan743x_phylink_mac_link_down,
 	.mac_link_up = lan743x_phylink_mac_link_up,
@@ -3654,6 +3690,9 @@ static void lan743x_hardware_cleanup(struct lan743x_adapter *adapter)
 
 static void lan743x_mdiobus_cleanup(struct lan743x_adapter *adapter)
 {
+	if (adapter->xpcs)
+		xpcs_destroy(adapter->xpcs);
+
 	mdiobus_unregister(adapter->mdiobus);
 }
 
@@ -3763,6 +3802,7 @@ static int lan743x_hardware_init(struct lan743x_adapter *adapter,
 
 static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 {
+	struct dw_xpcs *xpcs;
 	u32 sgmii_ctl;
 	int ret;
 
@@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 				  "SGMII operation\n");
 			adapter->mdiobus->read = lan743x_mdiobus_read_c22;
 			adapter->mdiobus->write = lan743x_mdiobus_write_c22;
-			adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
-			adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
+			if (adapter->is_sfp_support_en) {
+				adapter->mdiobus->read_c45 =
+					pci11x1x_pcs_read;
+				adapter->mdiobus->write_c45 =
+					pci11x1x_pcs_write;
+			} else {
+				adapter->mdiobus->read_c45 =
+					 lan743x_mdiobus_read_c45;
+				adapter->mdiobus->write_c45 =
+					 lan743x_mdiobus_write_c45;
+			}
 			adapter->mdiobus->name = "lan743x-mdiobus-c45";
 			netif_dbg(adapter, drv, adapter->netdev,
 				  "lan743x-mdiobus-c45\n");
@@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
 	ret = mdiobus_register(adapter->mdiobus);
 	if (ret < 0)
 		goto return_error;
+
+	if (adapter->is_sfp_support_en) {
+		if (!adapter->phy_interface)
+			lan743x_phy_interface_select(adapter);
+
+		xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
+					   adapter->phy_interface);
+		if (IS_ERR(xpcs)) {
+			netdev_err(adapter->netdev, "failed to create xpcs\n");
+			ret = PTR_ERR(xpcs);
+			goto err_destroy_xpcs;
+		}
+		adapter->xpcs = xpcs;
+	}
+
 	return 0;
 
+err_destroy_xpcs:
+	xpcs_destroy(xpcs);
+
 return_error:
+	mdiobus_free(adapter->mdiobus);
 	return ret;
 }
 
diff --git a/drivers/net/ethernet/microchip/lan743x_main.h b/drivers/net/ethernet/microchip/lan743x_main.h
index c303a69c3bea..f7480a401a27 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.h
+++ b/drivers/net/ethernet/microchip/lan743x_main.h
@@ -10,6 +10,7 @@
 #include <linux/i2c.h>
 #include <linux/gpio/machine.h>
 #include <linux/auxiliary_bus.h>
+#include <linux/pcs/pcs-xpcs.h>
 #include "lan743x_ptp.h"
 
 #define DRIVER_AUTHOR   "Bryan Whitehead <Bryan.Whitehead@microchip.com>"
@@ -1130,6 +1131,7 @@ struct lan743x_adapter {
 	struct lan743x_sw_nodes	*nodes;
 	struct i2c_adapter	*i2c_adap;
 	struct platform_device	*sfp_dev;
+	struct dw_xpcs		*xpcs;
 };
 
 #define LAN743X_COMPONENT_FLAG_RX(channel)  BIT(20 + (channel))
-- 
2.34.1

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Maxime Chevallier]

Hello Raju,

On Wed, 11 Sep 2024 21:40:53 +0530
Raju Lakkaraju <Raju.Lakkaraju@microchip.com> wrote:

[...]

> @@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  	ret = mdiobus_register(adapter->mdiobus);
>  	if (ret < 0)
>  		goto return_error;
> +
> +	if (adapter->is_sfp_support_en) {
> +		if (!adapter->phy_interface)
> +			lan743x_phy_interface_select(adapter);
> +
> +		xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
> +					   adapter->phy_interface);
> +		if (IS_ERR(xpcs)) {
> +			netdev_err(adapter->netdev, "failed to create xpcs\n");
> +			ret = PTR_ERR(xpcs);
> +			goto err_destroy_xpcs;
> +		}
> +		adapter->xpcs = xpcs;
> +	}
> +
>  	return 0;
>  
> +err_destroy_xpcs:
> +	xpcs_destroy(xpcs);

It looks like here, you're destroying the xpcs only when the xpcs
couln't be created in the first place, so no need to destroy it :)

Best regards,

Maxime

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Raju Lakkaraju]

Hi Maxime,

Thank you for review the patches.

The 09/11/2024 19:24, Maxime Chevallier wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hello Raju,
> 
> On Wed, 11 Sep 2024 21:40:53 +0530
> Raju Lakkaraju <Raju.Lakkaraju@microchip.com> wrote:
> 
> [...]
> 
> > @@ -3820,9 +3869,28 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >       ret = mdiobus_register(adapter->mdiobus);
> >       if (ret < 0)
> >               goto return_error;
> > +
> > +     if (adapter->is_sfp_support_en) {
> > +             if (!adapter->phy_interface)
> > +                     lan743x_phy_interface_select(adapter);
> > +
> > +             xpcs = xpcs_create_mdiodev(adapter->mdiobus, 0,
> > +                                        adapter->phy_interface);
> > +             if (IS_ERR(xpcs)) {
> > +                     netdev_err(adapter->netdev, "failed to create xpcs\n");
> > +                     ret = PTR_ERR(xpcs);
> > +                     goto err_destroy_xpcs;
> > +             }
> > +             adapter->xpcs = xpcs;
> > +     }
> > +
> >       return 0;
> >
> > +err_destroy_xpcs:
> > +     xpcs_destroy(xpcs);
> 
> It looks like here, you're destroying the xpcs only when the xpcs
> couln't be created in the first place, so no need to destroy it :)

Ok. I will remove

But i was little bit confusion here.

In xpcs_create_mdiodev( ) function, inside the mdio_device_create( ) function 
allocate memory for mdio_device

Then, in xpcs_create( ) function to create data by calling xpcs_create_data( )
function, create dw_xpcs memory.

It's reason, for safe side, I updte xpcs destroy

> 
> Best regards,
> 
> Maxime

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> +static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
> +			     int regnum)
> +{
> +	struct lan743x_adapter *adapter = bus->priv;
> +
> +	if (addr)
> +		return -EOPNOTSUPP;
> +
> +	return lan743x_sgmii_read(adapter, devnum, regnum);
> +}

>  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  {
> +	struct dw_xpcs *xpcs;
>  	u32 sgmii_ctl;
>  	int ret;
>  
> @@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
>  				  "SGMII operation\n");
>  			adapter->mdiobus->read = lan743x_mdiobus_read_c22;
>  			adapter->mdiobus->write = lan743x_mdiobus_write_c22;
> -			adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
> -			adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
> +			if (adapter->is_sfp_support_en) {
> +				adapter->mdiobus->read_c45 =
> +					pci11x1x_pcs_read;
> +				adapter->mdiobus->write_c45 =
> +					pci11x1x_pcs_write;

As you can see, the naming convention is to put the bus transaction
type on the end. So please name these pci11x1x_pcs_read_c45...

Also, am i reading this correct. C22 transfers will go out a
completely different bus to C45 transfers when there is an SFP?

If there are two physical MDIO busses, please instantiate two Linux
MDIO busses.

    Andrew

---
pw-bot: cr

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Raju Lakkaraju]

Hi Andrew,

The 09/11/2024 19:26, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > +static int pci11x1x_pcs_read(struct mii_bus *bus, int addr, int devnum,
> > +                          int regnum)
> > +{
> > +     struct lan743x_adapter *adapter = bus->priv;
> > +
> > +     if (addr)
> > +             return -EOPNOTSUPP;
> > +
> > +     return lan743x_sgmii_read(adapter, devnum, regnum);
> > +}
> 
> >  static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >  {
> > +     struct dw_xpcs *xpcs;
> >       u32 sgmii_ctl;
> >       int ret;
> >
> > @@ -3783,8 +3823,17 @@ static int lan743x_mdiobus_init(struct lan743x_adapter *adapter)
> >                                 "SGMII operation\n");
> >                       adapter->mdiobus->read = lan743x_mdiobus_read_c22;
> >                       adapter->mdiobus->write = lan743x_mdiobus_write_c22;
> > -                     adapter->mdiobus->read_c45 = lan743x_mdiobus_read_c45;
> > -                     adapter->mdiobus->write_c45 = lan743x_mdiobus_write_c45;
> > +                     if (adapter->is_sfp_support_en) {
> > +                             adapter->mdiobus->read_c45 =
> > +                                     pci11x1x_pcs_read;
> > +                             adapter->mdiobus->write_c45 =
> > +                                     pci11x1x_pcs_write;
> 
> As you can see, the naming convention is to put the bus transaction
> type on the end. So please name these pci11x1x_pcs_read_c45...

Accpeted. I will fix

> 
> Also, am i reading this correct. C22 transfers will go out a
> completely different bus to C45 transfers when there is an SFP?

No. You are correct.
This LAN743x driver support following chips
1. LAN7430 - C22 only with GMII/RGMII I/F
2. LAN7431 - C22 only with MII I/F
3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
   If SFP enable, then XPCS's C45 PCS access
   If SGMII only enable, then SGMII (PCS) C45 access

> 
> If there are two physical MDIO busses, please instantiate two Linux
> MDIO busses.
> 

XPCS driver is doing.
Am i miss anything there?

>     Andrew
> 
> ---
> pw-bot: cr

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> > Also, am i reading this correct. C22 transfers will go out a
> > completely different bus to C45 transfers when there is an SFP?
> 
> No. You are correct.
> This LAN743x driver support following chips
> 1. LAN7430 - C22 only with GMII/RGMII I/F
> 2. LAN7431 - C22 only with MII I/F

Fine, simple, not a problem.

> 3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
>    If SFP enable, then XPCS's C45 PCS access
>    If SGMII only enable, then SGMII (PCS) C45 access

Physically, there are two MDIO busses? There is an external MDIO bus
with two pins along side the RGMII/SGMII pins? And internally, there
is an MDIO bus to the PCS block?

Some designs do have only one bus, the internal PCS uses address X on
the bus and you are simply not allowed to put an external device at
that address.

But from my reading of the code, you have two MDIO busses, so you need
two Linux MDIO busses.

	Andrew

RE: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Ronnie.Kunin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, September 12, 2024 11:28 AM
> To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; maxime.chevallier@bootlin.com;
> rdunlap@infradead.org; Steen Hegelund - M31857 <Steen.Hegelund@microchip.com>; Daniel Machon -
> M70577 <Daniel.Machon@microchip.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > > Also, am i reading this correct. C22 transfers will go out a
> > > completely different bus to C45 transfers when there is an SFP?
> >
> > No. You are correct.
> > This LAN743x driver support following chips 1. LAN7430 - C22 only with
> > GMII/RGMII I/F 2. LAN7431 - C22 only with MII I/F
> 
> Fine, simple, not a problem.
> 
> > 3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
> >    If SFP enable, then XPCS's C45 PCS access
> >    If SGMII only enable, then SGMII (PCS) C45 access
> 
> Physically, there are two MDIO busses? There is an external MDIO bus with two pins along side the
> RGMII/SGMII pins? And internally, there is an MDIO bus to the PCS block?
> 
> Some designs do have only one bus, the internal PCS uses address X on the bus and you are simply not
> allowed to put an external device at that address.
> 
> But from my reading of the code, you have two MDIO busses, so you need two Linux MDIO busses.
> 
>         Andrew

Our PCI11x1x hardware has a single MDIO controller that gets used regardless of whether the chip interface is set to RGMII or to SGMII/BASE-X.
When we are using an SFP, the MDIO lines from our controller are not used / connected at all to the SFP.

Raju can probably explain this way better than me since the how all this interaction in the linux mdio/sfp/xpcs frameworks work honestly goes over my head. From what he told me even when we are not using our mdio controller lines, since there is indirect access to the PHY (the one inside of the SFP) via the I2C controller (which btw does not share any hardware pins with those used by the MDIO controller), he had to change the PHY management functions for that indirect access to be used when SFP is selected.

Ronnie

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> Our PCI11x1x hardware has a single MDIO controller that gets used
> regardless of whether the chip interface is set to RGMII or to
> SGMII/BASE-X.

That would be the external MDIO bus. 

But where is the PCS connected?

> When we are using an SFP, the MDIO lines from our controller are not
> used / connected at all to the SFP.

Correct. The SFP cage does not have MDIO pins. There are two commonly
used protocols for MDIO over I2C, which phylink supports. The MAC
driver is not involved. All it needs to report to phylink is when the
PCS transitions up/down.

    Andrew

RE: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Ronnie.Kunin]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Thursday, September 12, 2024 12:13 PM
> To: Ronnie Kunin - C21729 <Ronnie.Kunin@microchip.com>
> Cc: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>; netdev@vger.kernel.org;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Bryan
> Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; maxime.chevallier@bootlin.com;
> rdunlap@infradead.org; Steen Hegelund - M31857 <Steen.Hegelund@microchip.com>; Daniel Machon -
> M70577 <Daniel.Machon@microchip.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > Our PCI11x1x hardware has a single MDIO controller that gets used
> > regardless of whether the chip interface is set to RGMII or to
> > SGMII/BASE-X.
> 
> That would be the external MDIO bus.
> 
> But where is the PCS connected?

For SGMII/BASE-X support the PCI11010 uses Synopsys IP which is all internal to the device. The registers of this Synopsys block are accessible indirectly using a couple of registers (called SGMII_ACCESS and SGMII_DATA) that are mapped into the PCI11010 PCIe BAR.

> 
> > When we are using an SFP, the MDIO lines from our controller are not
> > used / connected at all to the SFP.
> 
> Correct. The SFP cage does not have MDIO pins. There are two commonly used protocols for MDIO over
> I2C, which phylink supports. The MAC driver is not involved. All it needs to report to phylink is when the
> PCS transitions up/down.
> 
>     Andrew

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> > But where is the PCS connected?
> 
> For SGMII/BASE-X support the PCI11010 uses Synopsys IP which is all internal to the device. The registers of this Synopsys block are accessible indirectly using a couple of registers (called SGMII_ACCESS and SGMII_DATA) that are mapped into the PCI11010 PCIe BAR.

Right, so not actually on the MDIO bus. Hence it is not correct to
replace the true MDIO access functions with these that do MMIO.

I do understand wanting this to look like an MDIO bus, that is what
the Synopsys driver expects. So create an MDIO bus for it. A bus of
its own.

Just for my understanding. The configuration is purely EEPROM, not
fuses? The PCS exists independent of the 'RGMII/not RGMII' bit in the
EEPROM. So you could unconditionally create the PCS MDIO bus, and
instantiate the PCS driver. In the RGMII case it would just sit there
idle. In the 'not RGMII' mode, the PCS could be used to connect to an
external PHY using SGMII or 2500BaseX. Or it could be connected to an
SFP cage, using SGMII, 1000BaseX or 2500BaseX. In both cases, you tell
phylink about it, phylink will tell you how to configure it.

	  Andrew

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Raju Lakkaraju - I30499]

Hi Andrew / Ronnie,

The 09/12/2024 16:04, Ronnie Kunin - C21729 wrote:
> 
> 
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Thursday, September 12, 2024 11:28 AM
> > To: Raju Lakkaraju - I30499 <Raju.Lakkaraju@microchip.com>
> > Cc: netdev@vger.kernel.org; davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; Bryan Whitehead - C21958 <Bryan.Whitehead@microchip.com>; UNGLinuxDriver
> > <UNGLinuxDriver@microchip.com>; linux@armlinux.org.uk; maxime.chevallier@bootlin.com;
> > rdunlap@infradead.org; Steen Hegelund - M31857 <Steen.Hegelund@microchip.com>; Daniel Machon -
> > M70577 <Daniel.Machon@microchip.com>; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs
> > 
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > > > Also, am i reading this correct. C22 transfers will go out a
> > > > completely different bus to C45 transfers when there is an SFP?
> > >
> > > No. You are correct.
> > > This LAN743x driver support following chips 1. LAN7430 - C22 only with
> > > GMII/RGMII I/F 2. LAN7431 - C22 only with MII I/F
> > 
> > Fine, simple, not a problem.
> > 
> > > 3. PCI11010/PCI11414 - C45 with RGMII or SGMII/1000Base-X/2500Base-X
> > >    If SFP enable, then XPCS's C45 PCS access
> > >    If SGMII only enable, then SGMII (PCS) C45 access
> > 
> > Physically, there are two MDIO busses? There is an external MDIO bus with two pins along side the
> > RGMII/SGMII pins? And internally, there is an MDIO bus to the PCS block?
> > 
> > Some designs do have only one bus, the internal PCS uses address X on the bus and you are simply not
> > allowed to put an external device at that address.
> > 
> > But from my reading of the code, you have two MDIO busses, so you need two Linux MDIO busses.
> > 
> >         Andrew
> 
> Our PCI11x1x hardware has a single MDIO controller that gets used regardless of whether the chip interface is set to RGMII or to SGMII/BASE-X.
> When we are using an SFP, the MDIO lines from our controller are not used / connected at all to the SFP.
> 
> Raju can probably explain this way better than me since the how all this interaction in the linux mdio/sfp/xpcs frameworks work honestly goes over my head. From what he told me even when we are not using our mdio controller lines, since there is indirect access to the PHY (the one inside of the SFP) via the I2C controller (which btw does not share any hardware pins with those used by the MDIO controller), he had to change the PHY management functions for that indirect access to be used when SFP is selected.
> 
> Ronnie

It's my mistake. We don't need 2 MDIO buses. 
If SFP present, XPC's MDIO bus can use,
If not sfp, LAN743x MDIO bus can use. 

We will fix.

> 

-- 
Thanks,                                                                         
Raju

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> It's my mistake. We don't need 2 MDIO buses. 
> If SFP present, XPC's MDIO bus can use,
> If not sfp, LAN743x MDIO bus can use. 

I still think this is wrong. Don't focus on 'sfp present'.

Other MAC drivers don't even know there is an SFP cage connected vs a
PHY. They just tell phylink the list of link modes they support, and
phylink tells it which one to use when the media has link.

You have a set of hardware building blocks, A MAC, a PCS, an MDIO bus.
Use the given abstractions in Linux to export them to the core, and
then let Linux combine them as needed.

Back to my question about EEPROM vs fuses. I assume it is an EEPROM,
and the PCS always exists. So always instantiate an MDIO bus and
instantiate the PCS. The MDIO bus always exists, so instantiate an
MDIO bus.

The driver itself should not really need to take any notice of the
EEPROM contents. All the EEPROM is used for is to indicate what
swnodes to create. It is a replacement of DT. Look at other drivers,
they don't parse DT to see if there is an SFP and so instantiate
different blocks.

As a silicon vendor, you want to export all the capabilities of the
device, and then sit back and watch all the weird and wonderful ways
you never even imagined it could be used come to life.

One such use case: What you can express in the EEPROM is very
limited. I would not be too surprised if somebody comes along and adds
DT overlay support. Look at what is going on with MikrotekBus and the
RPI add on chip RP1. Even microchip itself is using DT overlays with
some of its switch chips.

	Andrew

RE: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Ronnie.Kunin]

> > It's my mistake. We don't need 2 MDIO buses.
> > If SFP present, XPC's MDIO bus can use, If not sfp, LAN743x MDIO bus
> > can use.
> 
> I still think this is wrong. Don't focus on 'sfp present'.
> 
> Other MAC drivers don't even know there is an SFP cage connected vs a PHY. They just tell phylink the list
> of link modes they support, and phylink tells it which one to use when the media has link.
> 
> You have a set of hardware building blocks, A MAC, a PCS, an MDIO bus.
> Use the given abstractions in Linux to export them to the core, and then let Linux combine them as
> needed.
> 
> Back to my question about EEPROM vs fuses. I assume it is an EEPROM, ...

How RGMII vs "SGMII-BASEX"  is controlled ?
The hardware default is RGMII. That can be overwritten by OTP (similar functionality to EFuse, inside the PCI11010), which also can be further overwritten by EEPROM (outside the PCI11010). That will setup the initial value the device will have by the time the software first sees it. But it is a live bit in a register, so it can be changed at runtime if it was desired.

> ... and the PCS always exists. So
> always instantiate an MDIO bus and instantiate the PCS. The MDIO bus always exists, so instantiate an
> MDIO bus.

No, you can't do that with this device because:
- There are shared pins in the chip between RGMII and SGMII/BASEX (and before you comment that it is a shame/bad hw design/etc: this chip has a lot of other functionality besides ethernet which results in physical constraints, so tradeoffs had to be done). The selection of which functionality is the active one on the pins is done by that SGMII_EN control we have been talking about. Most of our customer designs have one type interface only which is hardwired on the PCB design, and the setting in OTP/EEPROM informs our chip what it is (as I said if you wanted to flip it later either for something fairly static coming from elsewhere - i.e. BIOS settings, DT, etc -, or even runtime fully dynamic you also can, but with the restriction that only one of them can be used at a time).
- Furthermore, I need to check with the HW architect, but I suspect that the block that was not selected is shutdown to save power as well.

> 
> The driver itself should not really need to take any notice of the EEPROM contents. All the EEPROM is
> used for is to indicate what swnodes to create. It is a replacement of DT. Look at other drivers, they don't
> parse DT to see if there is an SFP and so instantiate different blocks.
> 
> As a silicon vendor, you want to export all the capabilities of the device, and then sit back and watch all
> the weird and wonderful ways you never even imagined it could be used come to life.
> 
> One such use case: What you can express in the EEPROM is very limited. I would not be too surprised if
> somebody comes along and adds DT overlay support. Look at what is going on with MikrotekBus and the
> RPI add on chip RP1. Even microchip itself is using DT overlays with some of its switch chips.
> 
>         Andrew

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

On Fri, Sep 13, 2024 at 02:23:03PM +0000, Ronnie.Kunin@microchip.com wrote:
> > > It's my mistake. We don't need 2 MDIO buses.
> > > If SFP present, XPC's MDIO bus can use, If not sfp, LAN743x MDIO bus
> > > can use.
> > 
> > I still think this is wrong. Don't focus on 'sfp present'.
> > 
> > Other MAC drivers don't even know there is an SFP cage connected vs a PHY. They just tell phylink the list
> > of link modes they support, and phylink tells it which one to use when the media has link.
> > 
> > You have a set of hardware building blocks, A MAC, a PCS, an MDIO bus.
> > Use the given abstractions in Linux to export them to the core, and then let Linux combine them as
> > needed.
> > 
> > Back to my question about EEPROM vs fuses. I assume it is an EEPROM, ...
> 
> How RGMII vs "SGMII-BASEX"  is controlled ?
> The hardware default is RGMII. That can be overwritten by OTP (similar functionality to EFuse, inside the PCI11010), which also can be further overwritten by EEPROM (outside the PCI11010). That will setup the initial value the device will have by the time the software first sees it. But it is a live bit in a register, so it can be changed at runtime if it was desired.

In a DT system phy-mode will tell you this. You don't have DT, at
least not at the moment, so your EEPROM makes sense for this, the
RGMII vs "SGMII-BASEX" bit.

> 
> > ... and the PCS always exists. So
> > always instantiate an MDIO bus and instantiate the PCS. The MDIO bus always exists, so instantiate an
> > MDIO bus.
> 
> No, you can't do that with this device because:
> - There are shared pins in the chip between RGMII and SGMII/BASEX

Which is typical in SoC. What you generally have is lots of IP blocks,
I2C, SPI, GPIO, PWM, NAND controllers etc. The total number of
inputs/outputs of these blocks is more than the legs on the chip. You
then have a pinmux, which connects the internal blocks to the pins.

All the devices exist, but only a subset is connected to the outside
world.

For RGMII vs SGMII/BASEX, it probably does not make sense to
instantiate the PCS in RGMII mode. However, in SGMII/BASEX it should
always exist, because it is connected to the outside world.

> - Furthermore, I need to check with the HW architect, but I suspect
>   that the block that was not selected is shutdown to save power as
>   well.

I would also expect that when the PCS device is probed, it is left in
a lower power state. For an external PHY, you don't need the PCS
running until the PHY has link, autoneg has completed, and phylink
will tell you to configure the PCS to SGMII or 2500BaseX. For an SFP,
you need to read out the contents of the SFP EEPROM, look for LOS to
indicate there is link, and then phylink will determine SGMII,
1000BaseX or 2500BaseX and tell you how to configure it. It is only at
that point do you need to take the PCS out of low power mode.

Independent of RGMII vs SGMII/BASEX, the MDIO bus always exists. Both
modes need it. And Linux just considers it an MDIO, not necessarily an
MDIO bus for this MAC. So i would expect to always see a fully
functioning MDIO bus.

One of the weird and wonderful use cases: There are lots of ComExpress
boards with Intel 10G Ethernet interfaces. There are developers who
create base boards for them with Ethernet switches. They connect the
10G interface to one port of the switch. But to manage the switch they
need MDIO. The Intel 10G drivers bury the MDIO in firmware, Linux
cannot access is. So they are forced to use three GPIOs and bitbang
MDIO. It is slow. Now imaging i put your device on the baseboard. I
use its MAC connected to a 1G/2.5G PHY, on the MDIO bus, which i uses
as the management interface for the box. Additional i connect the MDIO
bus to the switch, to manage the switch. Linux has no problems with
this, MDIO is just a bus with devices on it. But phylink will want
access to the PCS to switch it between SGMII and 2500BaseX depending
on what the PHY negotiates. Plus we need C45 to talk to the switch.

The proposed hijacking for C45 from the MDIO bus to talk to the PCS
when there is an SFP breaks this, and as far as i can see, for no real
reason other than being too lazy to put the PCS on its own Linux MDIO
bus.

      Andrew

RE: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Ronnie.Kunin]

> > - Furthermore, I need to check with the HW architect, but I suspect
> >   that the block that was not selected is shutdown to save power as
> >   well.
> 
> I would also expect that when the PCS device is probed, it is left in a lower power state. For an external
> PHY, you don't need the PCS running until the PHY has link, autoneg has completed, and phylink will tell
> you to configure the PCS to SGMII or 2500BaseX. For an SFP, you need to read out the contents of the SFP
> EEPROM, look for LOS to indicate there is link, and then phylink will determine SGMII, 1000BaseX or
> 2500BaseX and tell you how to configure it. It is only at that point do you need to take the PCS out of low
> power mode.
> 
> Independent of RGMII vs SGMII/BASEX, the MDIO bus always exists. Both modes need it. And Linux just
> considers it an MDIO, not necessarily an MDIO bus for this MAC. So i would expect to always see a fully
> functioning MDIO bus.
> 
> One of the weird and wonderful use cases: There are lots of ComExpress boards with Intel 10G Ethernet
> interfaces. There are developers who create base boards for them with Ethernet switches. They connect
> the 10G interface to one port of the switch. But to manage the switch they need MDIO. The Intel 10G
> drivers bury the MDIO in firmware, Linux cannot access is. So they are forced to use three GPIOs and
> bitbang MDIO. It is slow. Now imaging i put your device on the baseboard. I use its MAC connected to a
> 1G/2.5G PHY, on the MDIO bus, which i uses as the management interface for the box. Additional i
> connect the MDIO bus to the switch, to manage the switch. Linux has no problems with this, MDIO is just
> a bus with devices on it. But phylink will want access to the PCS to switch it between SGMII and
> 2500BaseX depending on what the PHY negotiates. Plus we need C45 to talk to the switch.
> 
> The proposed hijacking for C45 from the MDIO bus to talk to the PCS when there is an SFP breaks this, and
> as far as i can see, for no real reason other than being too lazy to put the PCS on its own Linux MDIO bus.

Ok, I see I probably misunderstood what had to "always exists" (assumed everything) as you mentioned in your previous email for Linux to make use of the frameworks. Interesting use cases, thanks.  

As I have mentioned before I am not (and will likely never be) a Linux expert. I have just been advising Raju from the perspective of how the chip hardware works, the drivers I have written for it for a different OS, and what our customers have been requiring from us. I wasn't advocating against instantiating a second MDIO bus or whatever else makes sense in the Linux frameworks/environment, specially if the overhead to implement it is low and allows for richer or alternative use cases. I was just pointing out potential problems based on my knowledge of our hardware and the information I was told (now obviously misunderstood and/or incomplete) about these newer Linux frameworks (phylink, xpcs, sfp) by Raju / was understanding from your email. Unfortunately today was Raju's last day with Microchip, so I guess this discussions will probably pause for a while until a new developer is allocated to continue/complete this patch set.

Re: [PATCH net-next V2 4/5] net: lan743x: Implement phylink pcs

[Andrew Lunn]

> As I have mentioned before I am not (and will likely never be) a
> Linux expert. I have just been advising Raju from the perspective of
> how the chip hardware works, the drivers I have written for it for a
> different OS, and what our customers have been requiring from us. I
> wasn't advocating against instantiating a second MDIO bus or
> whatever else makes sense in the Linux frameworks/environment,
> specially if the overhead to implement it is low and allows for
> richer or alternative use cases. I was just pointing out potential
> problems based on my knowledge of our hardware and the information I
> was told (now obviously misunderstood and/or incomplete) about these
> newer Linux frameworks (phylink, xpcs, sfp) by Raju / was
> understanding from your email.

Not a problem. That is partially why we do reviews, to make sure the
architecture is correct. And there is a tendency for developers and
drivers coming from other OSes to re-invent wheels, because some other
OSes do not provide those wheels. With Linux you have a bigger API to
understand, but less code to write.

	Andrew

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

[Raju Lakkaraju]

Support for 2.5G SFP modules utilizing the 2500Base-X interface.
The implementation includes integration with the Phylink subsystem to
manage the link state and configuration dynamically.

Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microchip.com>
---
Change List:
============
V2 : Include this new patch in the V2 series. 

 drivers/net/ethernet/microchip/lan743x_main.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/microchip/lan743x_main.c b/drivers/net/ethernet/microchip/lan743x_main.c
index ef76d0c1642f..7fe699e5a134 100644
--- a/drivers/net/ethernet/microchip/lan743x_main.c
+++ b/drivers/net/ethernet/microchip/lan743x_main.c
@@ -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;
@@ -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);
@@ -3412,12 +3416,13 @@ static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
 	struct device_node *dn = adapter->pdev->dev.of_node;
 	struct net_device *dev = adapter->netdev;
 	struct phy_device *phydev;
-	int ret;
+	int ret = 0;
 
 	if (dn)
 		ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
 
-	if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
+	if (!adapter->is_sfp_support_en &&
+	    (!dn || (ret && !lan743x_phy_handle_exists(dn)))) {
 		phydev = phy_find_first(adapter->mdiobus);
 		if (phydev) {
 			/* attach the mac to the phy */
-- 
2.34.1

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

[Andrew Lunn]

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

Have you tested an SFP module capable of 2500BASEX?

	Andrew

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

[Maxime Chevallier]

On Wed, 11 Sep 2024 19:31:01 +0200
Andrew Lunn <andrew@lunn.ch> 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 ?

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 ?

Maxime

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

[Raju Lakkaraju]

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@lunn.ch> 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

> 
> Maxime
> 

-- 
Thanks,                                                                         
Raju

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@microchip.com> 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@lunn.ch> 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

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

[Raju Lakkaraju]

Hi Andrew,

The 09/11/2024 19:31, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > @@ -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 already add support. Here it's showing only diff changes

> Have you tested an SFP module capable of 2500BASEX?
> 
Yes. I test SFP module (FS's make 2.5G Cu SFP (SFP-2.5G-T))
it's working as expected.

>         Andrew

-- 
Thanks,                                                                         
Raju