[PATCH v2 net-next 0/3] net: phy: aquantia: enable support for aqr115c

[PATCH v2 net-next 0/3] net: phy: aquantia: enable support for aqr115c

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This is a smaller chunk of the bigger series that enables support for
aqr115c and fixes a firmware boot issue preventing it from working on
sa8775p-ride.

Changes since v1:
- split out the PHY patches into their own series
- don't introduce new mode (OCSGMII) but use existing 2500BASEX instead
- split the wait-for-FW patch into two: one renaming and exporting the
  relevant function and the second using it before checking the FW ID
Link to v1: https://lore.kernel.org/linux-arm-kernel/20240619184550.34524-1-brgl@bgdev.pl/T/

Bartosz Golaszewski (3):
  net: phy: aquantia: rename and export aqr107_wait_reset_complete()
  net: phy: aquantia: wait for FW reset before checking the vendor ID
  net: phy: aquantia: add support for aqr115c

 drivers/net/phy/aquantia/aquantia.h          |  1 +
 drivers/net/phy/aquantia/aquantia_firmware.c |  4 ++
 drivers/net/phy/aquantia/aquantia_main.c     | 45 ++++++++++++++++++--
 3 files changed, 46 insertions(+), 4 deletions(-)

-- 
2.43.0

[PATCH v2 net-next 1/3] net: phy: aquantia: rename and export aqr107_wait_reset_complete()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

This function is quite generic in this driver and not limited to aqr107.
We will use it outside its current compilation unit soon so rename it
and declare it in the header.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/net/phy/aquantia/aquantia.h      | 1 +
 drivers/net/phy/aquantia/aquantia_main.c | 6 +++---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/aquantia/aquantia.h b/drivers/net/phy/aquantia/aquantia.h
index b8502793962e..2465345081f8 100644
--- a/drivers/net/phy/aquantia/aquantia.h
+++ b/drivers/net/phy/aquantia/aquantia.h
@@ -201,5 +201,6 @@ int aqr_phy_led_hw_control_set(struct phy_device *phydev, u8 index,
 int aqr_phy_led_active_low_set(struct phy_device *phydev, int index, bool enable);
 int aqr_phy_led_polarity_set(struct phy_device *phydev, int index,
 			     unsigned long modes);
+int aqr_wait_reset_complete(struct phy_device *phydev);
 
 #endif /* AQUANTIA_H */
diff --git a/drivers/net/phy/aquantia/aquantia_main.c b/drivers/net/phy/aquantia/aquantia_main.c
index 6c14355744b7..974795bd0860 100644
--- a/drivers/net/phy/aquantia/aquantia_main.c
+++ b/drivers/net/phy/aquantia/aquantia_main.c
@@ -441,7 +441,7 @@ static int aqr107_set_tunable(struct phy_device *phydev,
  * The chip also provides a "reset completed" bit, but it's cleared after
  * read. Therefore function would time out if called again.
  */
-static int aqr107_wait_reset_complete(struct phy_device *phydev)
+int aqr_wait_reset_complete(struct phy_device *phydev)
 {
 	int val;
 
@@ -494,7 +494,7 @@ static int aqr107_config_init(struct phy_device *phydev)
 	WARN(phydev->interface == PHY_INTERFACE_MODE_XGMII,
 	     "Your devicetree is out of date, please update it. The AQR107 family doesn't support XGMII, maybe you mean USXGMII.\n");
 
-	ret = aqr107_wait_reset_complete(phydev);
+	ret = aqr_wait_reset_complete(phydev);
 	if (!ret)
 		aqr107_chip_info(phydev);
 
@@ -522,7 +522,7 @@ static int aqcs109_config_init(struct phy_device *phydev)
 	    phydev->interface != PHY_INTERFACE_MODE_2500BASEX)
 		return -ENODEV;
 
-	ret = aqr107_wait_reset_complete(phydev);
+	ret = aqr_wait_reset_complete(phydev);
 	if (!ret)
 		aqr107_chip_info(phydev);
 
-- 
2.43.0

Re: [PATCH v2 net-next 1/3] net: phy: aquantia: rename and export aqr107_wait_reset_complete()

[Andrew Lunn]

On Thu, Jun 27, 2024 at 01:30:15PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> This function is quite generic in this driver and not limited to aqr107.
> We will use it outside its current compilation unit soon so rename it
> and declare it in the header.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 net-next 2/3] net: phy: aquantia: wait for FW reset before checking the vendor ID

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Checking the firmware register before it complete the boot process makes
no sense, it will report 0 even if FW is available from internal memory.
Always wait for FW to boot before continuing or we'll unnecessarily try
to load it from nvmem/filesystem and fail.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/net/phy/aquantia/aquantia_firmware.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/aquantia/aquantia_firmware.c b/drivers/net/phy/aquantia/aquantia_firmware.c
index 0c9640ef153b..524627a36c6f 100644
--- a/drivers/net/phy/aquantia/aquantia_firmware.c
+++ b/drivers/net/phy/aquantia/aquantia_firmware.c
@@ -353,6 +353,10 @@ int aqr_firmware_load(struct phy_device *phydev)
 {
 	int ret;
 
+	ret = aqr_wait_reset_complete(phydev);
+	if (ret)
+		return ret;
+
 	/* Check if the firmware is not already loaded by pooling
 	 * the current version returned by the PHY. If 0 is returned,
 	 * no firmware is loaded.
-- 
2.43.0

Re: [PATCH v2 net-next 2/3] net: phy: aquantia: wait for FW reset before checking the vendor ID

[Andrew Lunn]

On Thu, Jun 27, 2024 at 01:30:16PM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Checking the firmware register before it complete the boot process makes
> no sense, it will report 0 even if FW is available from internal memory.
> Always wait for FW to boot before continuing or we'll unnecessarily try
> to load it from nvmem/filesystem and fail.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

[PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Bartosz Golaszewski]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Russell King (Oracle)]

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.

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Bartosz Golaszewski]

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!

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Andrew Lunn]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Bartosz Golaszewski]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Daniel Golle]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Andrew Lunn]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Daniel Golle]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Bartosz Golaszewski]

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

Re: [PATCH v2 net-next 3/3] net: phy: aquantia: add support for aqr115c

[Andrew Lunn]

> 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