Re: [PATCH net v2 1/2] net: stmmac: dwmac-spacemit: Fix wrong phy interface definition
Maxime Chevallier <[email protected]>
| Newsgroups | dev.linux.lists.spacemit,org.infradead.lists.linux-arm-kernel,org.infradead.lists.linux-riscv,org.kernel.vger.linux-kernel,org.kernel.vger.netdev |
|---|---|
| Message-ID | <[email protected]> |
Hello,
On 6/23/26 09:46, Inochi Amaoto wrote:
> The current MII interface register definition from the vendor is wrong,
> use the right number for the macro. Also, correct the interface mask
> in spacemit_set_phy_intf_sel() so it can update the register with the
> right number
>
> Fixes: 30f0ba420ed3 ("net: stmmac: Add glue layer for Spacemit K3 SoC")
> Signed-off-by: Inochi Amaoto <[email protected]>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac-spacemit.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-spacemit.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-spacemit.c
> index 223754cc5c79..3bfb6d49be6c 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-spacemit.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-spacemit.c
> @@ -18,8 +18,10 @@
> #include "stmmac_platform.h"
>
> /* ctrl register bits */
> -#define CTRL_PHY_INTF_RGMII BIT(3)
> -#define CTRL_PHY_INTF_MII BIT(4)
> +#define CTRL_PHY_INTF_MODE GENMASK(4, 3)
> +#define CTRL_PHY_INTF_RMII FIELD_PREP(CTRL_PHY_INTF_MODE, 0)
> +#define CTRL_PHY_INTF_RGMII FIELD_PREP(CTRL_PHY_INTF_MODE, 1)
> +#define CTRL_PHY_INTF_MII FIELD_PREP(CTRL_PHY_INTF_MODE, 3)
> #define CTRL_WAKE_IRQ_EN BIT(9)
> #define CTRL_PHY_IRQ_EN BIT(12)
>
> @@ -118,7 +120,7 @@ static void spacemit_get_interfaces(struct stmmac_priv *priv, void *bsp_priv,
>
> static int spacemit_set_phy_intf_sel(void *bsp_priv, u8 phy_intf_sel)
> {
> - unsigned int mask = CTRL_PHY_INTF_MII | CTRL_PHY_INTF_RGMII;
> + unsigned int mask = CTRL_PHY_INTF_MODE;
> struct spacmit_dwmac *dwmac = bsp_priv;
> unsigned int val = 0;
>
> @@ -128,6 +130,7 @@ static int spacemit_set_phy_intf_sel(void *bsp_priv, u8 phy_intf_sel)
> break;
>
> case PHY_INTF_SEL_RMII:
> + val = CTRL_PHY_INTF_RMII;
This isn't strictly-speaking necessary as this is 0 and val is already 0, maybe
compilers can figure it out and this leaves us with more self-documenting code ?
So I'm ok with that personally,
Reviewed-by: Maxime Chevallier <[email protected]>
Maxime
> break;
>
> case PHY_INTF_SEL_RGMII: