| Message ID | 20240911161054.4494-2-Raju.Lakkaraju@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support to SFP for PCI11x1x chips | expand |
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;
> + 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
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; > > } > > >
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
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
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
> > > > > + 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
> -----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
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.
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.
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;
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(-)