| Message ID | 20250312095411.1392379-3-suraj.gupta2@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add support for 2500Base-X only configuration | expand |
On 2025-03-12 10:54 AM, Suraj Gupta wrote: > AXI 1G/2.5G ethernet IP has following synthesis options: > 1) SGMII/1000base-X only. > 2) 2500base-X only. > 3) dynamically switching between (1) and (2). > Add support for 2500base-X only configuration. Hi, thanks for the patch. nit: a discrepancy between the commit description for and the comments in the code for 3) Maybe adding that information here in the commit description would make sense as well? Or giving a bit of a background that SGMII/1000base-X is already implemented in the driver and you are adding 2500base-X only support. > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > + * 1) SGMII/1000base-X only. > + * 2) 2500base-X only. > + * 3) Dynamically switching between (1) and (2), and is not > + * implemented in driver. > + */ For the rest of the patch, it looks good to me but I'd rather have someone more experienced provide the Reviewed-By tag if they find the patch appropriate. Best regards, Dawid
> + /* AXI 1G/2.5G ethernet IP has following synthesis options: > + * 1) SGMII/1000base-X only. > + * 2) 2500base-X only. > + * 3) Dynamically switching between (1) and (2), and is not > + * implemented in driver. > + */ > + > + if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_2_5G) How can we tell if the synthesis allows 3)? Don't we have a backwards compatibility issue here? Maybe there are systems which have been synthesised with 3), but are currently limited to 1) due to the driver. If you don't differentiate between 2 and 3, such systems are going to swap to 2) and regress. Andrew
On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > + * 1) SGMII/1000base-X only. > > + * 2) 2500base-X only. > > + * 3) Dynamically switching between (1) and (2), and is not > > + * implemented in driver. > > + */ > > + > > + if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_2_5G) > > How can we tell if the synthesis allows 3)? > > Don't we have a backwards compatibility issue here? Maybe there are > systems which have been synthesised with 3), but are currently limited > to 1) due to the driver. If you don't differentiate between 2 and 3, > such systems are going to swap to 2) and regress. We've discussed this before... but because the author doesn't post regularly enough, it's not suprising that context keeps getting lost. Here's the discussion from 20th February 2025 on a patch series that I commented on on 19th November 2024. https://lore.kernel.org/r/BL3PR12MB6571FE73FA8D5AAB9FB4BB3CC9C42@BL3PR12MB6571.namprd12.prod.outlook.com Suraj Gupta - you _must_ be more responsive so that reviewers can keep the context of previous discussions in their heads to avoid going over the same points time and time again. If you can't do that (and it's a good idea anyway) then you need to supplement the commit descriptions with the salient points from the previous patch series discussion to remind reviewers of the appropriate context.
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Russell King <linux@armlinux.org.uk> > Sent: Wednesday, March 12, 2025 7:44 PM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Gupta, Suraj <Suraj.Gupta2@amd.com>; Pandey, Radhey Shyam > <radhey.shyam.pandey@amd.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > Simek, Michal <michal.simek@amd.com>; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Katakam, Harini > <harini.katakam@amd.com> > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only > configuration. > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > > + * 1) SGMII/1000base-X only. > > > + * 2) 2500base-X only. > > > + * 3) Dynamically switching between (1) and (2), and is not > > > + * implemented in driver. > > > + */ > > > + > > > + if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_2_5G) > > > > How can we tell if the synthesis allows 3)? > > > > Don't we have a backwards compatibility issue here? Maybe there are > > systems which have been synthesised with 3), but are currently limited > > to 1) due to the driver. If you don't differentiate between 2 and 3, > > such systems are going to swap to 2) and regress. > > We've discussed this before... but because the author doesn't post regularly enough, > it's not suprising that context keeps getting lost. > > Here's the discussion from 20th February 2025 on a patch series that I commented > on on 19th November 2024. > > https://lore.kernel.org/r/BL3PR12MB6571FE73FA8D5AAB9FB4BB3CC9C42@BL3 > PR12MB6571.namprd12.prod.outlook.com > > Suraj Gupta - you _must_ be more responsive so that reviewers can keep the > context of previous discussions in their heads to avoid going over the same points > time and time again. If you can't do that (and it's a good idea anyway) then you need > to supplement the commit descriptions with the salient points from the previous > patch series discussion to remind reviewers of the appropriate context. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last! Thanks Russell, sure I'll add salient points from previous discussions if it becomes old (won't try to delay at first). Andrew - Keeping previous discussion short, identification of (3) depends on how user implements switching logic in FPGA (external GT or RTL logic). AXI 1G/2.5G IP provides only static speed selections and there is no standard register to communicate that to software.
> > On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > > > + * 1) SGMII/1000base-X only. > > > > + * 2) 2500base-X only. > > > > + * 3) Dynamically switching between (1) and (2), and is not > > > > + * implemented in driver. > > > > + */ > - Keeping previous discussion short, identification of (3) depends > on how user implements switching logic in FPGA (external GT or RTL > logic). AXI 1G/2.5G IP provides only static speed selections and > there is no standard register to communicate that to software. So if anybody has synthesised it as 3) this change will break their system? Andrew
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, March 12, 2025 8:29 PM > To: Gupta, Suraj <Suraj.Gupta2@amd.com> > Cc: Russell King <linux@armlinux.org.uk>; Pandey, Radhey Shyam > <radhey.shyam.pandey@amd.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > Simek, Michal <michal.simek@amd.com>; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Katakam, Harini > <harini.katakam@amd.com> > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only > configuration. > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > > > On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > > > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > > > > + * 1) SGMII/1000base-X only. > > > > > + * 2) 2500base-X only. > > > > > + * 3) Dynamically switching between (1) and (2), and is not > > > > > + * implemented in driver. > > > > > + */ > > > - Keeping previous discussion short, identification of (3) depends on > > how user implements switching logic in FPGA (external GT or RTL > > logic). AXI 1G/2.5G IP provides only static speed selections and there > > is no standard register to communicate that to software. > > So if anybody has synthesised it as 3) this change will break their system? > > Andrew It will just restrict their system to (2)
On Wed, Mar 12, 2025 at 03:06:32PM +0000, Gupta, Suraj wrote: > [AMD Official Use Only - AMD Internal Distribution Only] > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, March 12, 2025 8:29 PM > > To: Gupta, Suraj <Suraj.Gupta2@amd.com> > > Cc: Russell King <linux@armlinux.org.uk>; Pandey, Radhey Shyam > > <radhey.shyam.pandey@amd.com>; andrew+netdev@lunn.ch; > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > > Simek, Michal <michal.simek@amd.com>; netdev@vger.kernel.org; > > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Katakam, Harini > > <harini.katakam@amd.com> > > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only > > configuration. > > > > Caution: This message originated from an External Source. Use proper caution > > when opening attachments, clicking links, or responding. > > > > > > > > On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > > > > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > > > > > + * 1) SGMII/1000base-X only. > > > > > > + * 2) 2500base-X only. > > > > > > + * 3) Dynamically switching between (1) and (2), and is not > > > > > > + * implemented in driver. > > > > > > + */ > > > > > - Keeping previous discussion short, identification of (3) depends on > > > how user implements switching logic in FPGA (external GT or RTL > > > logic). AXI 1G/2.5G IP provides only static speed selections and there > > > is no standard register to communicate that to software. > > > > So if anybody has synthesised it as 3) this change will break their system? > > > > Andrew > > It will just restrict their system to (2) Where as before, it was doing SGMII/1000base-X only. So such systems break? Andrew
[AMD Official Use Only - AMD Internal Distribution Only] > -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Wednesday, March 12, 2025 9:03 PM > To: Gupta, Suraj <Suraj.Gupta2@amd.com> > Cc: Russell King <linux@armlinux.org.uk>; Pandey, Radhey Shyam > <radhey.shyam.pandey@amd.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > Simek, Michal <michal.simek@amd.com>; netdev@vger.kernel.org; > devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; linux-arm- > kernel@lists.infradead.org; git (AMD-Xilinx) <git@amd.com>; Katakam, Harini > <harini.katakam@amd.com> > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for 2500base-X only > configuration. > > Caution: This message originated from an External Source. Use proper caution > when opening attachments, clicking links, or responding. > > > On Wed, Mar 12, 2025 at 03:06:32PM +0000, Gupta, Suraj wrote: > > [AMD Official Use Only - AMD Internal Distribution Only] > > > > > -----Original Message----- > > > From: Andrew Lunn <andrew@lunn.ch> > > > Sent: Wednesday, March 12, 2025 8:29 PM > > > To: Gupta, Suraj <Suraj.Gupta2@amd.com> > > > Cc: Russell King <linux@armlinux.org.uk>; Pandey, Radhey Shyam > > > <radhey.shyam.pandey@amd.com>; andrew+netdev@lunn.ch; > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > > > pabeni@redhat.com; robh@kernel.org; krzk+dt@kernel.org; > > > conor+dt@kernel.org; Simek, Michal <michal.simek@amd.com>; > > > netdev@vger.kernel.org; devicetree@vger.kernel.org; > > > linux-kernel@vger.kernel.org; linux-arm- kernel@lists.infradead.org; > > > git (AMD-Xilinx) <git@amd.com>; Katakam, Harini > > > <harini.katakam@amd.com> > > > Subject: Re: [PATCH net-next V2 2/2] net: axienet: Add support for > > > 2500base-X only configuration. > > > > > > Caution: This message originated from an External Source. Use proper > > > caution when opening attachments, clicking links, or responding. > > > > > > > > > > > On Wed, Mar 12, 2025 at 02:25:27PM +0100, Andrew Lunn wrote: > > > > > > > + /* AXI 1G/2.5G ethernet IP has following synthesis options: > > > > > > > + * 1) SGMII/1000base-X only. > > > > > > > + * 2) 2500base-X only. > > > > > > > + * 3) Dynamically switching between (1) and (2), and is not > > > > > > > + * implemented in driver. > > > > > > > + */ > > > > > > > - Keeping previous discussion short, identification of (3) depends > > > > on how user implements switching logic in FPGA (external GT or RTL > > > > logic). AXI 1G/2.5G IP provides only static speed selections and > > > > there is no standard register to communicate that to software. > > > > > > So if anybody has synthesised it as 3) this change will break their system? > > > > > > Andrew > > > > It will just restrict their system to (2) > > Where as before, it was doing SGMII/1000base-X only. So such systems break? > > Andrew If the user wants (3), they need to add their custom FPGA logic which anyway will require additional driver changes. (3) was not completely supported by existing driver.
diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet.h b/drivers/net/ethernet/xilinx/xilinx_axienet.h index 5ff742103beb..ded3e32999d5 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet.h +++ b/drivers/net/ethernet/xilinx/xilinx_axienet.h @@ -274,7 +274,7 @@ #define XAE_EMMC_RX16BIT 0x01000000 /* 16 bit Rx client enable */ #define XAE_EMMC_LINKSPD_10 0x00000000 /* Link Speed mask for 10 Mbit */ #define XAE_EMMC_LINKSPD_100 0x40000000 /* Link Speed mask for 100 Mbit */ -#define XAE_EMMC_LINKSPD_1000 0x80000000 /* Link Speed mask for 1000 Mbit */ +#define XAE_EMMC_LINKSPD_1000_2500 0x80000000 /* Link Speed mask for 1000 or 2500 Mbit */ /* Bit masks for Axi Ethernet PHYC register */ #define XAE_PHYC_SGMIILINKSPEED_MASK 0xC0000000 /* SGMII link speed mask*/ diff --git a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c index 054abf283ab3..0885ce201b0a 100644 --- a/drivers/net/ethernet/xilinx/xilinx_axienet_main.c +++ b/drivers/net/ethernet/xilinx/xilinx_axienet_main.c @@ -2583,6 +2583,7 @@ static struct phylink_pcs *axienet_mac_select_pcs(struct phylink_config *config, struct axienet_local *lp = netdev_priv(ndev); if (interface == PHY_INTERFACE_MODE_1000BASEX || + interface == PHY_INTERFACE_MODE_2500BASEX || interface == PHY_INTERFACE_MODE_SGMII) return &lp->pcs; @@ -2616,8 +2617,9 @@ static void axienet_mac_link_up(struct phylink_config *config, emmc_reg &= ~XAE_EMMC_LINKSPEED_MASK; switch (speed) { + case SPEED_2500: case SPEED_1000: - emmc_reg |= XAE_EMMC_LINKSPD_1000; + emmc_reg |= XAE_EMMC_LINKSPD_1000_2500; break; case SPEED_100: emmc_reg |= XAE_EMMC_LINKSPD_100; @@ -2627,7 +2629,7 @@ static void axienet_mac_link_up(struct phylink_config *config, break; default: dev_err(&ndev->dev, - "Speed other than 10, 100 or 1Gbps is not supported\n"); + "Speed other than 10, 100, 1Gbps or 2.5Gbps is not supported\n"); break; } @@ -3055,7 +3057,8 @@ static int axienet_probe(struct platform_device *pdev) "error registering MDIO bus: %d\n", ret); if (lp->phy_mode == PHY_INTERFACE_MODE_SGMII || - lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX) { + lp->phy_mode == PHY_INTERFACE_MODE_1000BASEX || + lp->phy_mode == PHY_INTERFACE_MODE_2500BASEX) { np = of_parse_phandle(pdev->dev.of_node, "pcs-handle", 0); if (!np) { /* Deprecated: Always use "pcs-handle" for pcs_phy. @@ -3083,8 +3086,19 @@ static int axienet_probe(struct platform_device *pdev) lp->phylink_config.dev = &ndev->dev; lp->phylink_config.type = PHYLINK_NETDEV; lp->phylink_config.mac_managed_pm = true; - lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE | - MAC_10FD | MAC_100FD | MAC_1000FD; + lp->phylink_config.mac_capabilities = MAC_SYM_PAUSE | MAC_ASYM_PAUSE; + + /* AXI 1G/2.5G ethernet IP has following synthesis options: + * 1) SGMII/1000base-X only. + * 2) 2500base-X only. + * 3) Dynamically switching between (1) and (2), and is not + * implemented in driver. + */ + + if (axienet_ior(lp, XAE_ABILITY_OFFSET) & XAE_ABILITY_2_5G) + lp->phylink_config.mac_capabilities |= MAC_2500FD; + else + lp->phylink_config.mac_capabilities |= MAC_10FD | MAC_100FD | MAC_1000FD; __set_bit(lp->phy_mode, lp->phylink_config.supported_interfaces); if (lp->switch_x_sgmii) {