RE: [PATCH v2] net: tsec: add option to set device max-speed via dts

RE: [PATCH v2] net: tsec: add option to set device max-speed via dts

[Aleksandar Gerasimovski]

Current tsec adapter sets adapter gigabit capabilities by default, and in
reality this must not always be the case.
It is possible that tsec adapter is used for 100Mbit connection, and in
this case setting 1000Mbit capabilities can lead to some side effects such
longer autoneg process.

In our ls102x designs this problem leads to long autoneg times (> 4 sec)
in case board rgmii link is 100Mbs capble only.
Limiting the rgmii link capabilities provides smother and faster
link establishment.

Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
Changes for v2:
  - keep the variable sort order in a decreasing order of line length
  - fix comment typo
---
 drivers/net/tsec.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
index c68e4b7..ee820aa 100644
--- a/drivers/net/tsec.c
+++ b/drivers/net/tsec.c
@@ -828,6 +828,7 @@ int tsec_probe(struct udevice *dev)
 	const char *phy_mode;
 	ofnode parent, child;
 	fdt_addr_t reg;
+	u32 max_speed;
 	int ret;
 
 	data = (struct tsec_data *)dev_get_driver_data(dev);
@@ -893,8 +894,12 @@ int tsec_probe(struct udevice *dev)
 	}
 	priv->interface = pdata->phy_interface;
 
+	/* Check for speed limit, default is 1000Mbps */
+	max_speed = dev_read_u32_default(dev, "max-speed", 1000);
+
 	/* Initialize flags */
-	priv->flags = TSEC_GIGABIT;
+	if (max_speed == 1000)
+		priv->flags = TSEC_GIGABIT;
 	if (priv->interface == PHY_INTERFACE_MODE_SGMII)
 		priv->flags |= TSEC_SGMII;
 
-- 
1.8.3.1

-----Original Message-----
From: Vladimir Oltean <vladimir.oltean@nxp.com> 
Sent: Freitag, 4. Juni 2021 13:29
To: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
Cc: u-boot@lists.denx.de; Priyanka Jain (OSS) <priyanka.jain@oss.nxp.com>; Rainer Boschung <rainer.boschung@hitachi-powergrids.com>; Ramon Fried <rfried.dev@gmail.com>
Subject: Re: [PATCH] net: tsec: add option to set device max-speed via dts

CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Fri, Jun 04, 2021 at 11:13:36AM +0000, Aleksandar Gerasimovski wrote:
> Current tsec adapter sets adapter gigabit capabilities by default, and 
> in reality this must not always be the case.
> It is possible that tsec adapter is used for 100Mbit connection, and 
> in this case setting 1000Mbit capabilities can lead to some side 
> effects such longer autoneg process.
>
> In our ls102x designs this problem leads to long autoneg times (> 4 
> sec) in case board rgmii link is 100Mbs capble only.
> Limiting the rgmii link capabilities provides smother and faster link 
> establishment.
>
> Signed-off-by: Aleksandar Gerasimovski 
> <aleksandar.gerasimovski@hitachi-powergrids.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Just one comment below:

>  drivers/net/tsec.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c index 
> c68e4b7..501390f 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -824,6 +824,7 @@ int tsec_probe(struct udevice *dev)
>       struct tsec_private *priv = dev_get_priv(dev);
>       struct ofnode_phandle_args phandle_args;
>       u32 tbiaddr = CONFIG_SYS_TBIPA_VALUE;
> +     u32 max_speed;

It would be nice if you could keep the variable sort order, which is in decreasing order of line length (so-called "reverse Christmas tree notation").

>       struct tsec_data *data;
>       const char *phy_mode;
>       ofnode parent, child;
> @@ -893,8 +894,12 @@ int tsec_probe(struct udevice *dev)
>       }
>       priv->interface = pdata->phy_interface;
>
> +     /* Check for speed limis, default is 1000Mbps */
> +     max_speed = dev_read_u32_default(dev, "max-speed", 1000);
> +
>       /* Initialize flags */
> -     priv->flags = TSEC_GIGABIT;
> +     if (max_speed == 1000)
> +             priv->flags = TSEC_GIGABIT;
>       if (priv->interface == PHY_INTERFACE_MODE_SGMII)
>               priv->flags |= TSEC_SGMII;
>
> --
> 1.8.3.1

Thanks.

Re: [PATCH v2] net: tsec: add option to set device max-speed via dts

[Bin Meng]

On Fri, Jun 4, 2021 at 8:05 PM Aleksandar Gerasimovski
<aleksandar.gerasimovski@hitachi-powergrids.com> wrote:
>
> Current tsec adapter sets adapter gigabit capabilities by default, and in
> reality this must not always be the case.
> It is possible that tsec adapter is used for 100Mbit connection, and in
> this case setting 1000Mbit capabilities can lead to some side effects such
> longer autoneg process.
>
> In our ls102x designs this problem leads to long autoneg times (> 4 sec)
> in case board rgmii link is 100Mbs capble only.

typo: capable?

> Limiting the rgmii link capabilities provides smother and faster

typo: smoother?

> link establishment.
>
> Signed-off-by: Aleksandar Gerasimovski <aleksandar.gerasimovski@hitachi-powergrids.com>
> Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> Changes for v2:
>   - keep the variable sort order in a decreasing order of line length
>   - fix comment typo
> ---
>  drivers/net/tsec.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/tsec.c b/drivers/net/tsec.c
> index c68e4b7..ee820aa 100644
> --- a/drivers/net/tsec.c
> +++ b/drivers/net/tsec.c
> @@ -828,6 +828,7 @@ int tsec_probe(struct udevice *dev)
>         const char *phy_mode;
>         ofnode parent, child;
>         fdt_addr_t reg;
> +       u32 max_speed;
>         int ret;
>
>         data = (struct tsec_data *)dev_get_driver_data(dev);
> @@ -893,8 +894,12 @@ int tsec_probe(struct udevice *dev)
>         }
>         priv->interface = pdata->phy_interface;
>
> +       /* Check for speed limit, default is 1000Mbps */
> +       max_speed = dev_read_u32_default(dev, "max-speed", 1000);
> +
>         /* Initialize flags */
> -       priv->flags = TSEC_GIGABIT;
> +       if (max_speed == 1000)
> +               priv->flags = TSEC_GIGABIT;
>         if (priv->interface == PHY_INTERFACE_MODE_SGMII)
>                 priv->flags |= TSEC_SGMII;
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Could you please send the new version as a separate thread, instead of
replying the old version thread?

Regards,
Bin