| Message ID | 20240610071459.287500-8-christophe.roullier@foss.st.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Series to deliver Ethernet for STM32MP13 | expand |
On 6/10/24 9:14 AM, Christophe Roullier wrote: [...] > static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) > @@ -303,7 +307,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 +352,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) > + dev_dbg(dev, "Warning sysconfig register mask not set\n"); Isn't this an error , so dev_err() ? Include the err variable in the error message, see the dev_err() above for an example. That way the log already contains useful information (the error code) that can be used to narrow down the problem.
On 6/10/24 12:39, Marek Vasut wrote: > On 6/10/24 9:14 AM, Christophe Roullier wrote: > > [...] > >> static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) >> @@ -303,7 +307,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 +352,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) >> + dev_dbg(dev, "Warning sysconfig register mask not set\n"); > > Isn't this an error , so dev_err() ? No, it is only "warning" information, for MP1 the mask is not needed (and for backward compatibility is not planned to put mask parameter mandatory) > > Include the err variable in the error message, see the dev_err() above > for an example. That way the log already contains useful information > (the error code) that can be used to narrow down the problem.
On 6/10/24 1:45 PM, Christophe ROULLIER wrote: > > On 6/10/24 12:39, Marek Vasut wrote: >> On 6/10/24 9:14 AM, Christophe Roullier wrote: >> >> [...] >> >>> static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) >>> @@ -303,7 +307,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 +352,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) >>> + dev_dbg(dev, "Warning sysconfig register mask not set\n"); >> >> Isn't this an error , so dev_err() ? > No, it is only "warning" information, for MP1 the mask is not needed > (and for backward compatibility is not planned to put mask parameter > mandatory) Should this be an error for anything newer than MP15 then ?
On 6/10/24 15:43, Marek Vasut wrote: > On 6/10/24 1:45 PM, Christophe ROULLIER wrote: >> >> On 6/10/24 12:39, Marek Vasut wrote: >>> On 6/10/24 9:14 AM, Christophe Roullier wrote: >>> >>> [...] >>> >>>> static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) >>>> @@ -303,7 +307,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 +352,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) >>>> + dev_dbg(dev, "Warning sysconfig register mask not set\n"); >>> >>> Isn't this an error , so dev_err() ? >> No, it is only "warning" information, for MP1 the mask is not needed >> (and for backward compatibility is not planned to put mask parameter >> mandatory) > > Should this be an error for anything newer than MP15 then ? For MP25, no need of mask, so for moment it is specific to MP13.
On 6/10/24 3:49 PM, Christophe ROULLIER wrote: > > On 6/10/24 15:43, Marek Vasut wrote: >> On 6/10/24 1:45 PM, Christophe ROULLIER wrote: >>> >>> On 6/10/24 12:39, Marek Vasut wrote: >>>> On 6/10/24 9:14 AM, Christophe Roullier wrote: >>>> >>>> [...] >>>> >>>>> static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) >>>>> @@ -303,7 +307,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 +352,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) >>>>> + dev_dbg(dev, "Warning sysconfig register mask not set\n"); >>>> >>>> Isn't this an error , so dev_err() ? >>> No, it is only "warning" information, for MP1 the mask is not needed >>> (and for backward compatibility is not planned to put mask parameter >>> mandatory) >> >> Should this be an error for anything newer than MP15 then ? > For MP25, no need of mask, so for moment it is specific to MP13. Make this a warning for MP15, error for MP13, do not check st,syscon presence for MP2 at all. Would that work ?
On 6/10/24 19:29, Marek Vasut wrote: > On 6/10/24 3:49 PM, Christophe ROULLIER wrote: >> >> On 6/10/24 15:43, Marek Vasut wrote: >>> On 6/10/24 1:45 PM, Christophe ROULLIER wrote: >>>> >>>> On 6/10/24 12:39, Marek Vasut wrote: >>>>> On 6/10/24 9:14 AM, Christophe Roullier wrote: >>>>> >>>>> [...] >>>>> >>>>>> static int stm32mp1_set_mode(struct plat_stmmacenet_data >>>>>> *plat_dat) >>>>>> @@ -303,7 +307,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 +352,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) >>>>>> + dev_dbg(dev, "Warning sysconfig register mask not set\n"); >>>>> >>>>> Isn't this an error , so dev_err() ? >>>> No, it is only "warning" information, for MP1 the mask is not >>>> needed (and for backward compatibility is not planned to put mask >>>> parameter mandatory) >>> >>> Should this be an error for anything newer than MP15 then ? >> For MP25, no need of mask, so for moment it is specific to MP13. > > Make this a warning for MP15, error for MP13, do not check st,syscon > presence for MP2 at all. Would that work ? Ok I will make a warning for MP15 and MP25 and error for MP13.
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c index bed2be129b2d2..09ff0be0bdcdc 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c @@ -90,6 +90,7 @@ struct stm32_dwmac { 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 +103,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) @@ -256,13 +257,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 +307,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 +352,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) + dev_dbg(dev, "Warning sysconfig register mask not set\n"); return err; } @@ -540,8 +551,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 }; static struct stm32_ops stm32mp1_dwmac_data = { @@ -549,7 +559,7 @@ 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 };
Add possibility to have second argument in syscon property to manage mask. This mask will be used to address right BITFIELDS of PMCR register. Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com> --- .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-)