| Message ID | 20240627113018.25083-4-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: aquantia: enable support for aqr115c | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next-1 |
On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > +static int aqr115c_config_init(struct phy_device *phydev) > +{ > + /* Check that the PHY interface type is compatible */ > + if (phydev->interface != PHY_INTERFACE_MODE_SGMII && > + phydev->interface != PHY_INTERFACE_MODE_2500BASEX) > + return -ENODEV; > + > + phy_set_max_speed(phydev, SPEED_2500); Please can you explain why this is necessary? Does the PHY report that it incorrectly supports faster speeds than 2500base-X ? If phylib is incorrectly detecting the PHYs features, then this should be corrected via the .get_features method, not in the .config_init method. (The same should be true of the other Aquantia PHYs.) Note that phy_set_max_speed() is documented as: * The PHY might be more capable than the MAC. For example a Fast Ethernet * is connected to a 1G PHY. This function allows the MAC to indicate its * maximum speed, and so limit what the PHY will advertise. Aquantia seems to be the only PHY driver that calls this function. Thanks.
On Thu, Jun 27, 2024 at 2:09 PM Russell King (Oracle) <linux@armlinux.org.uk> wrote: > > On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > > +static int aqr115c_config_init(struct phy_device *phydev) > > +{ > > + /* Check that the PHY interface type is compatible */ > > + if (phydev->interface != PHY_INTERFACE_MODE_SGMII && > > + phydev->interface != PHY_INTERFACE_MODE_2500BASEX) > > + return -ENODEV; > > + > > + phy_set_max_speed(phydev, SPEED_2500); > > Please can you explain why this is necessary? Does the PHY report that > it incorrectly supports faster speeds than 2500base-X ? > > If phylib is incorrectly detecting the PHYs features, then this should > be corrected via the .get_features method, not in the .config_init > method. > > (The same should be true of the other Aquantia PHYs.) > > Note that phy_set_max_speed() is documented as: > > * The PHY might be more capable than the MAC. For example a Fast Ethernet > * is connected to a 1G PHY. This function allows the MAC to indicate its > * maximum speed, and so limit what the PHY will advertise. > Well I should have RTFM. You're right, I'll drop it. Bart > Aquantia seems to be the only PHY driver that calls this function. > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add support for a new model to the Aquantia driver. This PHY supports > Overlocked SGMII mode with 2.5G speeds. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/net/phy/aquantia/aquantia_main.c | 39 +++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > index 974795bd0860..98ccefd355d5 100644 > --- a/drivers/net/phy/aquantia/aquantia_main.c > +++ b/drivers/net/phy/aquantia/aquantia_main.c > @@ -29,6 +29,7 @@ > #define PHY_ID_AQR113 0x31c31c40 > #define PHY_ID_AQR113C 0x31c31c12 > #define PHY_ID_AQR114C 0x31c31c22 > +#define PHY_ID_AQR115C 0x31c31c33 > #define PHY_ID_AQR813 0x31c31cb2 > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > @@ -111,7 +112,6 @@ static u64 aqr107_get_stat(struct phy_device *phydev, int index) > int len_h = stat->size - len_l; > u64 ret; > int val; > - > val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg); > if (val < 0) > return U64_MAX; White space change. And that blank line is actually wanted to separate the variables from the code. Andrew --- pw-bot: cr
On Thu, Jun 27, 2024 at 6:22 PM Andrew Lunn <andrew@lunn.ch> wrote: > > On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add support for a new model to the Aquantia driver. This PHY supports > > Overlocked SGMII mode with 2.5G speeds. > > > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > --- > > drivers/net/phy/aquantia/aquantia_main.c | 39 +++++++++++++++++++++++- > > 1 file changed, 38 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > > index 974795bd0860..98ccefd355d5 100644 > > --- a/drivers/net/phy/aquantia/aquantia_main.c > > +++ b/drivers/net/phy/aquantia/aquantia_main.c > > @@ -29,6 +29,7 @@ > > #define PHY_ID_AQR113 0x31c31c40 > > #define PHY_ID_AQR113C 0x31c31c12 > > #define PHY_ID_AQR114C 0x31c31c22 > > +#define PHY_ID_AQR115C 0x31c31c33 > > #define PHY_ID_AQR813 0x31c31cb2 > > > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > > @@ -111,7 +112,6 @@ static u64 aqr107_get_stat(struct phy_device *phydev, int index) > > int len_h = stat->size - len_l; > > u64 ret; > > int val; > > - > > val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg); > > if (val < 0) > > return U64_MAX; > > White space change. And that blank line is actually wanted to separate > the variables from the code. > Ah, this is accidental, thanks for catching it. Bart > Andrew > > --- > pw-bot: cr
Hi Bartosz, On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > Add support for a new model to the Aquantia driver. This PHY supports > Overlocked SGMII mode with 2.5G speeds. I don't think that there is such a thing as "Overclocked SGMII mode with 2.5G speed". Lets take a short look at Cisco SGMII, which is defined as a serialzed version of the Gigabit Media-Independent Interface. As such, it supports 10M, 100M and 1000M speed. There is negotiation for speed, duplex, flow-control and link status (up/down). The data signals always operate at 1.25 Gbaud and the clocks operate at 625 MHz (a DDR interface), and there is a 10:8 FEC coding applied, resulting in 1 Gbit/s usable bandwidth. For lower speeds lower than 1 Gbit/s each symbol is repeated 10x for 100M and 100x for 10M. Now, assuming SGMII running at 2.5x the clock speed of actual Cisco SGMII would exist, how would that look like for lower speeds like 1000M, 100M or 10M? Obviously you cannot repeat a symbol 2.5 times, which would make it impossible to support 1000M links with the same strategy used for lower speeds in regular SGMII. Hence I assume that what you meant to say here is that the PHY uses 2500Base-X as interface mode and performs rate-adaptation for speeds less than 2500M (or half-duplex) using pause frames. This is also what e.g. AQR112 is doing, which I would assume is fairly similar to the newer AQR115. > > Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > --- > drivers/net/phy/aquantia/aquantia_main.c | 39 +++++++++++++++++++++++- > 1 file changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c > index 974795bd0860..98ccefd355d5 100644 > --- a/drivers/net/phy/aquantia/aquantia_main.c > +++ b/drivers/net/phy/aquantia/aquantia_main.c > @@ -29,6 +29,7 @@ > #define PHY_ID_AQR113 0x31c31c40 > #define PHY_ID_AQR113C 0x31c31c12 > #define PHY_ID_AQR114C 0x31c31c22 > +#define PHY_ID_AQR115C 0x31c31c33 > #define PHY_ID_AQR813 0x31c31cb2 > > #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 > @@ -111,7 +112,6 @@ static u64 aqr107_get_stat(struct phy_device *phydev, int index) > int len_h = stat->size - len_l; > u64 ret; > int val; > - > val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg); > if (val < 0) > return U64_MAX; > @@ -721,6 +721,18 @@ static int aqr113c_config_init(struct phy_device *phydev) > return aqr107_fill_interface_modes(phydev); > } > > +static int aqr115c_config_init(struct phy_device *phydev) > +{ > + /* Check that the PHY interface type is compatible */ > + if (phydev->interface != PHY_INTERFACE_MODE_SGMII && > + phydev->interface != PHY_INTERFACE_MODE_2500BASEX) > + return -ENODEV; > + > + phy_set_max_speed(phydev, SPEED_2500); > + > + return 0; > +} > + > static int aqr107_probe(struct phy_device *phydev) > { > int ret; > @@ -999,6 +1011,30 @@ static struct phy_driver aqr_driver[] = { > .led_hw_control_get = aqr_phy_led_hw_control_get, > .led_polarity_set = aqr_phy_led_polarity_set, > }, > +{ > + PHY_ID_MATCH_MODEL(PHY_ID_AQR115C), > + .name = "Aquantia AQR115C", > + .probe = aqr107_probe, > + .get_rate_matching = aqr107_get_rate_matching, > + .config_init = aqr115c_config_init, > + .config_aneg = aqr_config_aneg, > + .config_intr = aqr_config_intr, > + .handle_interrupt = aqr_handle_interrupt, > + .read_status = aqr107_read_status, > + .get_tunable = aqr107_get_tunable, > + .set_tunable = aqr107_set_tunable, > + .suspend = aqr107_suspend, > + .resume = aqr107_resume, > + .get_sset_count = aqr107_get_sset_count, > + .get_strings = aqr107_get_strings, > + .get_stats = aqr107_get_stats, > + .link_change_notify = aqr107_link_change_notify, > + .led_brightness_set = aqr_phy_led_brightness_set, > + .led_hw_is_supported = aqr_phy_led_hw_is_supported, > + .led_hw_control_set = aqr_phy_led_hw_control_set, > + .led_hw_control_get = aqr_phy_led_hw_control_get, > + .led_polarity_set = aqr_phy_led_polarity_set, > +}, > { > PHY_ID_MATCH_MODEL(PHY_ID_AQR813), > .name = "Aquantia AQR813", > @@ -1042,6 +1078,7 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = { > { PHY_ID_MATCH_MODEL(PHY_ID_AQR113) }, > { PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) }, > { PHY_ID_MATCH_MODEL(PHY_ID_AQR114C) }, > + { PHY_ID_MATCH_MODEL(PHY_ID_AQR115C) }, > { PHY_ID_MATCH_MODEL(PHY_ID_AQR813) }, > { } > }; > -- > 2.43.0 > >
On Thu, Jun 27, 2024 at 11:42:45PM +0100, Daniel Golle wrote: > Hi Bartosz, > > On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Add support for a new model to the Aquantia driver. This PHY supports > > Overlocked SGMII mode with 2.5G speeds. > > I don't think that there is such a thing as "Overclocked SGMII mode with > 2.5G speed". Unfortunately, there is. A number of vendors say they do this, without saying quite what they actually do. As you point out, symbol replication does not work, and in-band signalling also makes no sense. So they throw all that away. Leaving just the higher clock rate, single speed, and no in-band signalling. In the end, that looks very similar to 2500BaseX with broken inband signalling. > Hence I assume that what you meant to say here is that the PHY uses > 2500Base-X as interface mode and performs rate-adaptation for speeds > less than 2500M (or half-duplex) using pause frames. Not all systems assume rate adaptation. Some are known to use SGMII for 10/100/1G with inband signalling, and then swap to 2500BaseX without inband-signalling for 2.5G operation! 2.5G is a mess. Andrew
On Fri, Jun 28, 2024 at 02:18:45AM +0200, Andrew Lunn wrote: > On Thu, Jun 27, 2024 at 11:42:45PM +0100, Daniel Golle wrote: > > Hi Bartosz, > > > > On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > Add support for a new model to the Aquantia driver. This PHY supports > > > Overlocked SGMII mode with 2.5G speeds. > > > > I don't think that there is such a thing as "Overclocked SGMII mode with > > 2.5G speed". > > Unfortunately, there is. A number of vendors say they do this, without > saying quite what they actually do. As you point out, symbol > replication does not work, and in-band signalling also makes no > sense. So they throw all that away. Leaving just the higher clock > rate, single speed, and no in-band signalling. > > In the end, that looks very similar to 2500BaseX with broken inband > signalling. Let's call it that then: "2500Base-X with broken in-band signalling". MaxLinear describes that quite clearly in their (open!) datasheets[1], and gives some insight into the (mis-)use of the term "SGMII" in the industry as synonymous to just any type of serialized Ethernet MII: " 3.4 SGMII Interface The GPY211 implements a serial data interface, called SGMII or SerDes, to connect to another chip implementing the MAC layer (MAC SoC). " (page 32) Later on they mention that " 3.4.7 Auto-negotiation Modes Supported by SGMII Two modes are supported for the SGMII auto-negotiation protocol: * Cisco* Serial-GMII Specification 1.8 [4] * 1000BX IEEE 802.3 following IEEE Clause 37 [2] " (page 37) Aquantia's datasheets are only available under NDA, so I cannot quote them directly, but I can tell you that their definition of "SGMII" is pretty similar to that of MaxLinear. > > > Hence I assume that what you meant to say here is that the PHY uses > > 2500Base-X as interface mode and performs rate-adaptation for speeds > > less than 2500M (or half-duplex) using pause frames. > > Not all systems assume rate adaptation. Some are known to use SGMII > for 10/100/1G with inband signalling, and then swap to 2500BaseX > without inband-signalling for 2.5G operation! Yes, most 2.5G PHYs out there (MaxLinear, RealTek) actually support both, with interface-mode switching being the better option compared to often rather problematic rate-adaptation... When it comes to Aquantia we are using 2500Base-X with rate adaptation for the older 2.5G PHYs, so I assume the newer ones would not differ in that regard. Or rather: If we were to introduce interface-mode-switching also for the Aquantia 2.5G PHYs then we should try doing it for all of them at least. > > 2.5G is a mess. +1 [1]: https://assets.maxlinear.com/web/documents/617810_gpy211b1vc_gpy211c0vc_ds_rev1.4.pdf
On Fri, Jun 28, 2024 at 3:11 AM Daniel Golle <daniel@makrotopia.org> wrote: > > On Fri, Jun 28, 2024 at 02:18:45AM +0200, Andrew Lunn wrote: > > On Thu, Jun 27, 2024 at 11:42:45PM +0100, Daniel Golle wrote: > > > Hi Bartosz, > > > > > > On Thu, Jun 27, 2024 at 01:30:17PM +0200, Bartosz Golaszewski wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > > > > > Add support for a new model to the Aquantia driver. This PHY supports > > > > Overlocked SGMII mode with 2.5G speeds. > > > > > > I don't think that there is such a thing as "Overclocked SGMII mode with > > > 2.5G speed". > > > > Unfortunately, there is. A number of vendors say they do this, without > > saying quite what they actually do. As you point out, symbol > > replication does not work, and in-band signalling also makes no > > sense. So they throw all that away. Leaving just the higher clock > > rate, single speed, and no in-band signalling. > > > > In the end, that looks very similar to 2500BaseX with broken inband > > signalling. > > Let's call it that then: "2500Base-X with broken in-band signalling". > > MaxLinear describes that quite clearly in their (open!) datasheets[1], > and gives some insight into the (mis-)use of the term "SGMII" in the > industry as synonymous to just any type of serialized Ethernet MII: > > " > 3.4 SGMII Interface > > The GPY211 implements a serial data interface, called SGMII or SerDes, > to connect to another chip implementing the MAC layer (MAC SoC). > " > (page 32) > > Later on they mention that > " > 3.4.7 Auto-negotiation Modes Supported by SGMII > > Two modes are supported for the SGMII auto-negotiation protocol: > * Cisco* Serial-GMII Specification 1.8 [4] > * 1000BX IEEE 802.3 following IEEE Clause 37 [2] > " > (page 37) > > Aquantia's datasheets are only available under NDA, so I cannot quote > them directly, but I can tell you that their definition of "SGMII" is > pretty similar to that of MaxLinear. > Well, hopefully without breaching the NDA I can tell you that there's no definition at all. At least not in the ~700 pages doc I have access to anyway. > > > > > Hence I assume that what you meant to say here is that the PHY uses > > > 2500Base-X as interface mode and performs rate-adaptation for speeds > > > less than 2500M (or half-duplex) using pause frames. > > > > Not all systems assume rate adaptation. Some are known to use SGMII > > for 10/100/1G with inband signalling, and then swap to 2500BaseX > > without inband-signalling for 2.5G operation! > > Yes, most 2.5G PHYs out there (MaxLinear, RealTek) actually support both, > with interface-mode switching being the better option compared to often > rather problematic rate-adaptation... > > When it comes to Aquantia we are using 2500Base-X with rate adaptation > for the older 2.5G PHYs, so I assume the newer ones would not differ in > that regard. Or rather: If we were to introduce interface-mode-switching > also for the Aquantia 2.5G PHYs then we should try doing it for all of > them at least. > > > > > 2.5G is a mess. > > +1 > Not sure what to do, should I still be adding a new mode here or is it fine to just explain in the commit message that this really is "2500Base-X-sans-in-band-signalling" and keep the code as is? Or maybe some quirk disallowing `managed = "in-band-status"`? Bartosz
> Not sure what to do, should I still be adding a new mode here or is it > fine to just explain in the commit message that this really is > "2500Base-X-sans-in-band-signalling" and keep the code as is? Or maybe > some quirk disallowing `managed = "in-band-status"`? Yes, please do make it clear in the commit message. It is good to have a full commit message explaining all the messy details to help the next person who needs to modify this code, or has the same issue with another vendor specific glue driver for stmmac, etc. Andrew
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c index 974795bd0860..98ccefd355d5 100644 --- a/drivers/net/phy/aquantia/aquantia_main.c +++ b/drivers/net/phy/aquantia/aquantia_main.c @@ -29,6 +29,7 @@ #define PHY_ID_AQR113 0x31c31c40 #define PHY_ID_AQR113C 0x31c31c12 #define PHY_ID_AQR114C 0x31c31c22 +#define PHY_ID_AQR115C 0x31c31c33 #define PHY_ID_AQR813 0x31c31cb2 #define MDIO_PHYXS_VEND_IF_STATUS 0xe812 @@ -111,7 +112,6 @@ static u64 aqr107_get_stat(struct phy_device *phydev, int index) int len_h = stat->size - len_l; u64 ret; int val; - val = phy_read_mmd(phydev, MDIO_MMD_C22EXT, stat->reg); if (val < 0) return U64_MAX; @@ -721,6 +721,18 @@ static int aqr113c_config_init(struct phy_device *phydev) return aqr107_fill_interface_modes(phydev); } +static int aqr115c_config_init(struct phy_device *phydev) +{ + /* Check that the PHY interface type is compatible */ + if (phydev->interface != PHY_INTERFACE_MODE_SGMII && + phydev->interface != PHY_INTERFACE_MODE_2500BASEX) + return -ENODEV; + + phy_set_max_speed(phydev, SPEED_2500); + + return 0; +} + static int aqr107_probe(struct phy_device *phydev) { int ret; @@ -999,6 +1011,30 @@ static struct phy_driver aqr_driver[] = { .led_hw_control_get = aqr_phy_led_hw_control_get, .led_polarity_set = aqr_phy_led_polarity_set, }, +{ + PHY_ID_MATCH_MODEL(PHY_ID_AQR115C), + .name = "Aquantia AQR115C", + .probe = aqr107_probe, + .get_rate_matching = aqr107_get_rate_matching, + .config_init = aqr115c_config_init, + .config_aneg = aqr_config_aneg, + .config_intr = aqr_config_intr, + .handle_interrupt = aqr_handle_interrupt, + .read_status = aqr107_read_status, + .get_tunable = aqr107_get_tunable, + .set_tunable = aqr107_set_tunable, + .suspend = aqr107_suspend, + .resume = aqr107_resume, + .get_sset_count = aqr107_get_sset_count, + .get_strings = aqr107_get_strings, + .get_stats = aqr107_get_stats, + .link_change_notify = aqr107_link_change_notify, + .led_brightness_set = aqr_phy_led_brightness_set, + .led_hw_is_supported = aqr_phy_led_hw_is_supported, + .led_hw_control_set = aqr_phy_led_hw_control_set, + .led_hw_control_get = aqr_phy_led_hw_control_get, + .led_polarity_set = aqr_phy_led_polarity_set, +}, { PHY_ID_MATCH_MODEL(PHY_ID_AQR813), .name = "Aquantia AQR813", @@ -1042,6 +1078,7 @@ static struct mdio_device_id __maybe_unused aqr_tbl[] = { { PHY_ID_MATCH_MODEL(PHY_ID_AQR113) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR113C) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR114C) }, + { PHY_ID_MATCH_MODEL(PHY_ID_AQR115C) }, { PHY_ID_MATCH_MODEL(PHY_ID_AQR813) }, { } };