From: Chen-Yu Tsai <[email protected]>
In commit 9cbadf094d9d ("net: stmmac: support max-speed device tree
property"), when DT platforms don't set "max-speed", max_speed is set to
-1; for non-DT platforms, it stays the default 0.
Prior to commit eeef2f6b9f6e ("net: stmmac: Start adding phylink support"),
the check for a valid max_speed setting was to check if it was greater
than zero. This commit got it right, but subsequent patches just checked
for non-zero, which is incorrect for DT platforms.
In commit 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
the conversion switched completely to checking for non-zero value as a
valid value, which caused 1000base-T to stop getting advertised by
default.
Instead of trying to fix all the checks, simply leave max_speed alone if
DT property parsing fails.
Fixes: 9cbadf094d9d ("net: stmmac: support max-speed device tree property")
Fixes: 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
Signed-off-by: Chen-Yu Tsai <[email protected]>
Acked-by: Russell King (Oracle) <[email protected]>
---
Resend2: CC Srinivas at Linaro instead of ST. Collected Russell's ack.
Resend: added Srinivas (author of first fixed commit) to CC list.
This was first noticed on ROC-RK3399-PC, and also observed on ROC-RK3328-CC.
The fix was tested on ROC-RK3328-CC and Libre Computer ALL-H5-ALL-CC.
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 5d29f336315b..11e1055e8260 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -431,8 +431,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
plat->phylink_node = np;
/* Get max speed of operation from device tree */
- if (of_property_read_u32(np, "max-speed", &plat->max_speed))
- plat->max_speed = -1;
+ of_property_read_u32(np, "max-speed", &plat->max_speed);
plat->bus_id = of_alias_get_id(np, "ethernet");
if (plat->bus_id < 0)
--
2.34.1
> Resend2: CC Srinivas at Linaro instead of ST. Collected Russell's ack.
> Resend: added Srinivas (author of first fixed commit) to CC list.
Please don't resend very quickly in general, and specifically not to
just add a reviewed-by. Patchwork will keep track for reviewed-by:s so
if this patch was to be picked up, they would be automatically
added. You only need to add them if you need to resend for another
reason.
In general, wait for at least 24 hours before doing a resend, so
others who are interested can review the patch and comment.
Andrew
On 31/03/2022 19:48, Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> In commit 9cbadf094d9d ("net: stmmac: support max-speed device tree
> property"), when DT platforms don't set "max-speed", max_speed is set to
> -1; for non-DT platforms, it stays the default 0.
>
> Prior to commit eeef2f6b9f6e ("net: stmmac: Start adding phylink support"),
> the check for a valid max_speed setting was to check if it was greater
> than zero. This commit got it right, but subsequent patches just checked
> for non-zero, which is incorrect for DT platforms.
>
> In commit 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> the conversion switched completely to checking for non-zero value as a
> valid value, which caused 1000base-T to stop getting advertised by
> default.
>
> Instead of trying to fix all the checks, simply leave max_speed alone if
> DT property parsing fails.
>
> Fixes: 9cbadf094d9d ("net: stmmac: support max-speed device tree property")
> Fixes: 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> Acked-by: Russell King (Oracle) <[email protected]>
> ---
LGTM,
Reviewed-by: Srinivas Kandagatla <[email protected]>
--srini
>
> Resend2: CC Srinivas at Linaro instead of ST. Collected Russell's ack.
> Resend: added Srinivas (author of first fixed commit) to CC list.
>
> This was first noticed on ROC-RK3399-PC, and also observed on ROC-RK3328-CC.
> The fix was tested on ROC-RK3328-CC and Libre Computer ALL-H5-ALL-CC.
>
> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> index 5d29f336315b..11e1055e8260 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
> @@ -431,8 +431,7 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac)
> plat->phylink_node = np;
>
> /* Get max speed of operation from device tree */
> - if (of_property_read_u32(np, "max-speed", &plat->max_speed))
> - plat->max_speed = -1;
> + of_property_read_u32(np, "max-speed", &plat->max_speed);
>
> plat->bus_id = of_alias_get_id(np, "ethernet");
> if (plat->bus_id < 0)
On Sat, Apr 2, 2022 at 12:42 PM Jakub Kicinski <[email protected]> wrote:
>
> On Fri, 1 Apr 2022 02:48:32 +0800 Chen-Yu Tsai wrote:
> > From: Chen-Yu Tsai <[email protected]>
> >
> > In commit 9cbadf094d9d ("net: stmmac: support max-speed device tree
> > property"), when DT platforms don't set "max-speed", max_speed is set to
> > -1; for non-DT platforms, it stays the default 0.
> >
> > Prior to commit eeef2f6b9f6e ("net: stmmac: Start adding phylink support"),
> > the check for a valid max_speed setting was to check if it was greater
> > than zero. This commit got it right, but subsequent patches just checked
> > for non-zero, which is incorrect for DT platforms.
> >
> > In commit 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> > the conversion switched completely to checking for non-zero value as a
> > valid value, which caused 1000base-T to stop getting advertised by
> > default.
> >
> > Instead of trying to fix all the checks, simply leave max_speed alone if
> > DT property parsing fails.
> >
> > Fixes: 9cbadf094d9d ("net: stmmac: support max-speed device tree property")
> > Fixes: 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> > Signed-off-by: Chen-Yu Tsai <[email protected]>
> > Acked-by: Russell King (Oracle) <[email protected]>
> > ---
> >
> > Resend2: CC Srinivas at Linaro instead of ST. Collected Russell's ack.
> > Resend: added Srinivas (author of first fixed commit) to CC list.
> >
> > This was first noticed on ROC-RK3399-PC, and also observed on ROC-RK3328-CC.
> > The fix was tested on ROC-RK3328-CC and Libre Computer ALL-H5-ALL-CC.
>
> This patch got marked Changes Requested in pw, but I can't see why,
> so I went on a limb and applied it. LMK if that was a mistake,
> otherwise its commit c21cabb0fd0b ("net: stmmac: Fix unset max_speed
> difference between DT and non-DT platforms") in net.
I don't remember anyone asking for any changes.
Thanks
ChenYu
On Fri, 1 Apr 2022 02:48:32 +0800 Chen-Yu Tsai wrote:
> From: Chen-Yu Tsai <[email protected]>
>
> In commit 9cbadf094d9d ("net: stmmac: support max-speed device tree
> property"), when DT platforms don't set "max-speed", max_speed is set to
> -1; for non-DT platforms, it stays the default 0.
>
> Prior to commit eeef2f6b9f6e ("net: stmmac: Start adding phylink support"),
> the check for a valid max_speed setting was to check if it was greater
> than zero. This commit got it right, but subsequent patches just checked
> for non-zero, which is incorrect for DT platforms.
>
> In commit 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> the conversion switched completely to checking for non-zero value as a
> valid value, which caused 1000base-T to stop getting advertised by
> default.
>
> Instead of trying to fix all the checks, simply leave max_speed alone if
> DT property parsing fails.
>
> Fixes: 9cbadf094d9d ("net: stmmac: support max-speed device tree property")
> Fixes: 92c3807b9ac3 ("net: stmmac: convert to phylink_get_linkmodes()")
> Signed-off-by: Chen-Yu Tsai <[email protected]>
> Acked-by: Russell King (Oracle) <[email protected]>
> ---
>
> Resend2: CC Srinivas at Linaro instead of ST. Collected Russell's ack.
> Resend: added Srinivas (author of first fixed commit) to CC list.
>
> This was first noticed on ROC-RK3399-PC, and also observed on ROC-RK3328-CC.
> The fix was tested on ROC-RK3328-CC and Libre Computer ALL-H5-ALL-CC.
This patch got marked Changes Requested in pw, but I can't see why,
so I went on a limb and applied it. LMK if that was a mistake,
otherwise its commit c21cabb0fd0b ("net: stmmac: Fix unset max_speed
difference between DT and non-DT platforms") in net.