| Message ID | 20240911161054.4494-5-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 |
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
> +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
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
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
> > 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
> -----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
> 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
> -----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
> > 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
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. >
> 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
> > 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
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
> > - 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.
> 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
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))
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(-)