Re: [PATCH v4 07/11] net: ethernet: stmmac: add management of stm32mp13 for stm32

[Christophe ROULLIER]

On 6/4/24 19:05, Marek Vasut wrote:
> On 6/4/24 4:34 PM, Christophe Roullier wrote:
> > Add Ethernet support for STM32MP13.
> > STM32MP13 is STM32 SOC with 2 GMACs instances.
> > GMAC IP version is SNPS 4.20.
> > GMAC IP configure with 1 RX and 1 TX queue.
> > DMA HW capability register supported
> > RX Checksum Offload Engine supported
> > TX Checksum insertion supported
> > Wake-Up On Lan supported
> > TSO supported
> >
> > Signed-off-by: Christophe Roullier <christophe.roullier@xxxxxxxxxxx>
> > ---
> >   .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 50 +++++++++++++++----
> >   1 file changed, 40 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c 
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > index bed2be129b2d2..e59f8a845e01e 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c
> > @@ -84,12 +84,14 @@ struct stm32_dwmac {
> >       struct clk *clk_eth_ck;
> >       struct clk *clk_ethstp;
> >       struct clk *syscfg_clk;
> > +    bool is_mp13;
> >       int ext_phyclk;
> >       int enable_eth_ck;
> >       int eth_clk_sel_reg;
> >       int eth_ref_clk_sel_reg;
> >       int irq_pwr_wakeup;
> >       u32 mode_reg;         /* MAC glue-logic mode register */
> > +    u32 mode_mask;
> >       struct regmap *regmap;
> >       u32 speed;
> >       const struct stm32_ops *ops;
> > @@ -102,8 +104,8 @@ struct stm32_ops {
> >       void (*resume)(struct stm32_dwmac *dwmac);
> >       int (*parse_data)(struct stm32_dwmac *dwmac,
> >                 struct device *dev);
> > -    u32 syscfg_eth_mask;
> >       bool clk_rx_enable_in_suspend;
> > +    u32 syscfg_clr_off;
> >   };
> >     static int stm32_dwmac_clk_enable(struct stm32_dwmac *dwmac, bool 
> > resume)
> > @@ -227,7 +229,14 @@ static int stm32mp1_configure_pmcr(struct 
> > plat_stmmacenet_data *plat_dat)
> >         switch (plat_dat->mac_interface) {
> >       case PHY_INTERFACE_MODE_MII:
> > -        val = SYSCFG_PMCR_ETH_SEL_MII;
> > +        /*
> > +         * STM32MP15xx supports both MII and GMII, STM32MP13xx MII 
> > only.
> > +         * SYSCFG_PMCSETR ETH_SELMII is present only on STM32MP15xx and
> > +         * acts as a selector between 0:GMII and 1:MII. As STM32MP13xx
> > +         * supports only MII, ETH_SELMII is not present.
> > +         */
> > +        if (!dwmac->is_mp13)    /* Select MII mode on STM32MP15xx */
> > +            val |= SYSCFG_PMCR_ETH_SEL_MII;
> >           break;
> >       case PHY_INTERFACE_MODE_GMII:
> >           val = SYSCFG_PMCR_ETH_SEL_GMII;
> > @@ -256,13 +265,16 @@ static int stm32mp1_configure_pmcr(struct 
> > plat_stmmacenet_data *plat_dat)
> >         dev_dbg(dwmac->dev, "Mode %s", 
> > phy_modes(plat_dat->mac_interface));
> >   +    /* Shift value at correct ethernet MAC offset in 
> > SYSCFG_PMCSETR */
> > +    val <<= ffs(dwmac->mode_mask) - ffs(SYSCFG_MP1_ETH_MASK);
> > +
> >       /* Need to update PMCCLRR (clear register) */
> > -    regmap_write(dwmac->regmap, reg + SYSCFG_PMCCLRR_OFFSET,
> > -             dwmac->ops->syscfg_eth_mask);
> > +    regmap_write(dwmac->regmap, dwmac->ops->syscfg_clr_off,
> > +             dwmac->mode_mask);
> >         /* Update PMCSETR (set register) */
> >       return regmap_update_bits(dwmac->regmap, reg,
> > -                 dwmac->ops->syscfg_eth_mask, val);
> > +                 dwmac->mode_mask, val);
> >   }
> >     static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat)
> > @@ -303,7 +315,7 @@ static int stm32mcu_set_mode(struct 
> > plat_stmmacenet_data *plat_dat)
> >       dev_dbg(dwmac->dev, "Mode %s", 
> > phy_modes(plat_dat->mac_interface));
> >         return regmap_update_bits(dwmac->regmap, reg,
> > -                 dwmac->ops->syscfg_eth_mask, val << 23);
> > +                 SYSCFG_MCU_ETH_MASK, val << 23);
> >   }
> >     static void stm32_dwmac_clk_disable(struct stm32_dwmac *dwmac, 
> > bool suspend)
> > @@ -348,8 +360,15 @@ static int stm32_dwmac_parse_data(struct 
> > stm32_dwmac *dwmac,
> >           return PTR_ERR(dwmac->regmap);
> >         err = of_property_read_u32_index(np, "st,syscon", 1, 
> > &dwmac->mode_reg);
> > -    if (err)
> > +    if (err) {
> >           dev_err(dev, "Can't get sysconfig mode offset (%d)\n", err);
> > +        return err;
> > +    }
> > +
> > +    dwmac->mode_mask = SYSCFG_MP1_ETH_MASK;
> > +    err = of_property_read_u32_index(np, "st,syscon", 2, 
> > &dwmac->mode_mask);
> > +    if (err)
> > +        pr_debug("Warning sysconfig register mask not set\n");
>
> I _think_ you need to left-shift the mode mask by 8 for STM32MP13xx 
> second GMAC somewhere in here, right ?
The shift is performed in function stm32mp1_configure_pmcr:

    /* Shift value at correct ethernet MAC offset in SYSCFG_PMCSETR */
    val <<= ffs(dwmac->mode_mask) - ffs(SYSCFG_MP1_ETH_MASK);

In case of MP13 Ethernet1 or MP15, shift equal 0

In case of MP13 Ethernet2 , shift equal 8  ;-)

> >       return err;
> >   }
> > @@ -361,6 +380,8 @@ static int stm32mp1_parse_data(struct stm32_dwmac 
> > *dwmac,
> >       struct device_node *np = dev->of_node;
> >       int err = 0;
> >   +    dwmac->is_mp13 = of_device_is_compatible(np, 
> > "st,stm32mp13-dwmac");
>
> You could make is_mp13 part of struct stm32_ops {} just like 
> syscfg_clr_off is part of struct stm32_ops {} .
ok
> >       /* Ethernet PHY have no crystal */
> >       dwmac->ext_phyclk = of_property_read_bool(np, "st,ext-phyclk");
> >   @@ -540,8 +561,7 @@ static SIMPLE_DEV_PM_OPS(stm32_dwmac_pm_ops,
> >       stm32_dwmac_suspend, stm32_dwmac_resume);
> >     static struct stm32_ops stm32mcu_dwmac_data = {
> > -    .set_mode = stm32mcu_set_mode,
> > -    .syscfg_eth_mask = SYSCFG_MCU_ETH_MASK
> > +    .set_mode = stm32mcu_set_mode
>
> It is not necessary to remove the trailing comma ','
ok
> >   };
> >     static struct stm32_ops stm32mp1_dwmac_data = {
> > @@ -549,13 +569,23 @@ static struct stm32_ops stm32mp1_dwmac_data = {
> >       .suspend = stm32mp1_suspend,
> >       .resume = stm32mp1_resume,
> >       .parse_data = stm32mp1_parse_data,
> > -    .syscfg_eth_mask = SYSCFG_MP1_ETH_MASK,
> > +    .syscfg_clr_off = 0x44,
> > +    .clk_rx_enable_in_suspend = true
> > +};
> > +
> > +static struct stm32_ops stm32mp13_dwmac_data = {
> > +    .set_mode = stm32mp1_set_mode,
> > +    .suspend = stm32mp1_suspend,
> > +    .resume = stm32mp1_resume,
> > +    .parse_data = stm32mp1_parse_data,
> > +    .syscfg_clr_off = 0x08,
> >       .clk_rx_enable_in_suspend = true
> >   };
> >     static const struct of_device_id stm32_dwmac_match[] = {
> >       { .compatible = "st,stm32-dwmac", .data = &stm32mcu_dwmac_data},
> >       { .compatible = "st,stm32mp1-dwmac", .data = 
> > &stm32mp1_dwmac_data},
> > +    { .compatible = "st,stm32mp13-dwmac", .data = 
> > &stm32mp13_dwmac_data},
> >       { }
> >   };
> >   MODULE_DEVICE_TABLE(of, stm32_dwmac_match);
>
> This patch definitely looks MUCH better than what this series started 
> with, it is much easier to grasp the MP13 specific changes.
>
> You could possibly improve this further and split the 
> dwmac->ops->syscfg_eth_mask to dwmac->mode_mask conversion into 
> separate preparatory patch (as a 6.5/11 in context of this series), 
> and then add the few MP13 changes on top (as 7/11 patch).
ok