| Message ID | 20240426125707.585269-4-christophe.roullier@foss.st.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Series to deliver Ethernets for STM32MP13 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
On 4/26/24 2:56 PM, Christophe Roullier wrote: > Change glue to be more generic and manage easily next stm32 products. > The goal of this commit is to have one stm32mp1_set_mode function which > can manage different STM32 SOC. SOC can have different SYSCFG register > bitfields. so in pmcsetr we defined the bitfields corresponding to the SOC. > > Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com> > --- > .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------ > 1 file changed, 51 insertions(+), 25 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > index c92dfc4ecf57..68a02de25ac7 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c > @@ -23,10 +23,6 @@ > > #define SYSCFG_MCU_ETH_MASK BIT(23) > #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16) > -#define SYSCFG_PMCCLRR_OFFSET 0x40 > - > -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16) > -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17) > > /* CLOCK feed to PHY*/ > #define ETH_CK_F_25M 25000000 > @@ -46,9 +42,6 @@ > * RMII | 1 | 0 | 0 | n/a | > *------------------------------------------ > */ > -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20) > -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21) > -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23) > #define SYSCFG_PMCR_ETH_SEL_GMII 0 > #define SYSCFG_MCU_ETH_SEL_MII 0 > #define SYSCFG_MCU_ETH_SEL_RMII 1 > @@ -90,19 +83,33 @@ 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; > struct device *dev; > }; > > +struct stm32_syscfg_pmcsetr { > + u32 eth1_clk_sel; > + u32 eth1_ref_clk_sel; > + u32 eth1_selmii; > + u32 eth1_sel_rgmii; > + u32 eth1_sel_rmii; > + u32 eth2_clk_sel; > + u32 eth2_ref_clk_sel; > + u32 eth2_sel_rgmii; > + u32 eth2_sel_rmii; > +}; [...] > @@ -487,8 +502,19 @@ 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, > - .clk_rx_enable_in_suspend = true > + .clk_rx_enable_in_suspend = true, > + .syscfg_clr_off = 0x44, > + .pmcsetr = { > + .eth1_clk_sel = BIT(16), > + .eth1_ref_clk_sel = BIT(17), > + .eth1_selmii = BIT(20), > + .eth1_sel_rgmii = BIT(21), > + .eth1_sel_rmii = BIT(23), > + .eth2_clk_sel = 0, > + .eth2_ref_clk_sel = 0, > + .eth2_sel_rgmii = 0, > + .eth2_sel_rmii = 0 > + } > }; Is this structure really necessary ? It seems the MP15 single ethernet config bitfield is at offset 16. MP13 has two bitfields, one at offset 16, the other at offset 24 . All you need to do is figure out which of the two MACs you are configuring, and then shift the bitfield mask by 16 or 24, since the bits are at the same offset for both bitfields. See the matching upstream U-Boot commit for how this shift can be done: a440d19c6c91 ("net: dwc_eth_qos: Add DT parsing for STM32MP13xx platform")
Hi On 4/26/24 16:53, Marek Vasut wrote: > On 4/26/24 2:56 PM, Christophe Roullier wrote: >> Change glue to be more generic and manage easily next stm32 products. >> The goal of this commit is to have one stm32mp1_set_mode function which >> can manage different STM32 SOC. SOC can have different SYSCFG register >> bitfields. so in pmcsetr we defined the bitfields corresponding to >> the SOC. >> >> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com> >> --- >> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------ >> 1 file changed, 51 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >> index c92dfc4ecf57..68a02de25ac7 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >> @@ -23,10 +23,6 @@ >> #define SYSCFG_MCU_ETH_MASK BIT(23) >> #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16) >> -#define SYSCFG_PMCCLRR_OFFSET 0x40 >> - >> -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16) >> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17) >> /* CLOCK feed to PHY*/ >> #define ETH_CK_F_25M 25000000 >> @@ -46,9 +42,6 @@ >> * RMII | 1 | 0 | 0 | n/a | >> *------------------------------------------ >> */ >> -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20) >> -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21) >> -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23) >> #define SYSCFG_PMCR_ETH_SEL_GMII 0 >> #define SYSCFG_MCU_ETH_SEL_MII 0 >> #define SYSCFG_MCU_ETH_SEL_RMII 1 >> @@ -90,19 +83,33 @@ 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; >> struct device *dev; >> }; >> +struct stm32_syscfg_pmcsetr { >> + u32 eth1_clk_sel; >> + u32 eth1_ref_clk_sel; >> + u32 eth1_selmii; >> + u32 eth1_sel_rgmii; >> + u32 eth1_sel_rmii; >> + u32 eth2_clk_sel; >> + u32 eth2_ref_clk_sel; >> + u32 eth2_sel_rgmii; >> + u32 eth2_sel_rmii; >> +}; > > [...] > >> @@ -487,8 +502,19 @@ 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, >> - .clk_rx_enable_in_suspend = true >> + .clk_rx_enable_in_suspend = true, >> + .syscfg_clr_off = 0x44, >> + .pmcsetr = { >> + .eth1_clk_sel = BIT(16), >> + .eth1_ref_clk_sel = BIT(17), >> + .eth1_selmii = BIT(20), >> + .eth1_sel_rgmii = BIT(21), >> + .eth1_sel_rmii = BIT(23), >> + .eth2_clk_sel = 0, >> + .eth2_ref_clk_sel = 0, >> + .eth2_sel_rgmii = 0, >> + .eth2_sel_rmii = 0 >> + } >> }; > > Is this structure really necessary ? > I prefer to keep this implementation for the moment, as it is working fine. Maybe at a later stage, I will send some optimizations. > It seems the MP15 single ethernet config bitfield is at offset 16. > MP13 has two bitfields, one at offset 16, the other at offset 24 . > > All you need to do is figure out which of the two MACs you are > configuring, and then shift the bitfield mask by 16 or 24, since the > bits are at the same offset for both bitfields. > > See the matching upstream U-Boot commit for how this shift can be done: > a440d19c6c91 ("net: dwc_eth_qos: Add DT parsing for STM32MP13xx > platform") >
On 5/13/24 2:48 PM, Christophe ROULLIER wrote: > Hi > > On 4/26/24 16:53, Marek Vasut wrote: >> On 4/26/24 2:56 PM, Christophe Roullier wrote: >>> Change glue to be more generic and manage easily next stm32 products. >>> The goal of this commit is to have one stm32mp1_set_mode function which >>> can manage different STM32 SOC. SOC can have different SYSCFG register >>> bitfields. so in pmcsetr we defined the bitfields corresponding to >>> the SOC. >>> >>> Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com> >>> --- >>> .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------ >>> 1 file changed, 51 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >>> index c92dfc4ecf57..68a02de25ac7 100644 >>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c >>> @@ -23,10 +23,6 @@ >>> #define SYSCFG_MCU_ETH_MASK BIT(23) >>> #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16) >>> -#define SYSCFG_PMCCLRR_OFFSET 0x40 >>> - >>> -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16) >>> -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17) >>> /* CLOCK feed to PHY*/ >>> #define ETH_CK_F_25M 25000000 >>> @@ -46,9 +42,6 @@ >>> * RMII | 1 | 0 | 0 | n/a | >>> *------------------------------------------ >>> */ >>> -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20) >>> -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21) >>> -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23) >>> #define SYSCFG_PMCR_ETH_SEL_GMII 0 >>> #define SYSCFG_MCU_ETH_SEL_MII 0 >>> #define SYSCFG_MCU_ETH_SEL_RMII 1 >>> @@ -90,19 +83,33 @@ 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; >>> struct device *dev; >>> }; >>> +struct stm32_syscfg_pmcsetr { >>> + u32 eth1_clk_sel; >>> + u32 eth1_ref_clk_sel; >>> + u32 eth1_selmii; >>> + u32 eth1_sel_rgmii; >>> + u32 eth1_sel_rmii; >>> + u32 eth2_clk_sel; >>> + u32 eth2_ref_clk_sel; >>> + u32 eth2_sel_rgmii; >>> + u32 eth2_sel_rmii; >>> +}; >> >> [...] >> >>> @@ -487,8 +502,19 @@ 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, >>> - .clk_rx_enable_in_suspend = true >>> + .clk_rx_enable_in_suspend = true, >>> + .syscfg_clr_off = 0x44, >>> + .pmcsetr = { >>> + .eth1_clk_sel = BIT(16), >>> + .eth1_ref_clk_sel = BIT(17), >>> + .eth1_selmii = BIT(20), >>> + .eth1_sel_rgmii = BIT(21), >>> + .eth1_sel_rmii = BIT(23), >>> + .eth2_clk_sel = 0, >>> + .eth2_ref_clk_sel = 0, >>> + .eth2_sel_rgmii = 0, >>> + .eth2_sel_rmii = 0 >>> + } >>> }; >> >> Is this structure really necessary ? >> > I prefer to keep this implementation for the moment, as it is working > fine. Maybe at a later stage, I will send some optimizations. BIT() and left shift by 8 works all the same, without all this complexity and new structures, since all you really have on MP13 are two identical bitfields (one at offset 16, the other at offset 24) with the same bits in them, so why not make this simple ?
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c index c92dfc4ecf57..68a02de25ac7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-stm32.c @@ -23,10 +23,6 @@ #define SYSCFG_MCU_ETH_MASK BIT(23) #define SYSCFG_MP1_ETH_MASK GENMASK(23, 16) -#define SYSCFG_PMCCLRR_OFFSET 0x40 - -#define SYSCFG_PMCR_ETH_CLK_SEL BIT(16) -#define SYSCFG_PMCR_ETH_REF_CLK_SEL BIT(17) /* CLOCK feed to PHY*/ #define ETH_CK_F_25M 25000000 @@ -46,9 +42,6 @@ * RMII | 1 | 0 | 0 | n/a | *------------------------------------------ */ -#define SYSCFG_PMCR_ETH_SEL_MII BIT(20) -#define SYSCFG_PMCR_ETH_SEL_RGMII BIT(21) -#define SYSCFG_PMCR_ETH_SEL_RMII BIT(23) #define SYSCFG_PMCR_ETH_SEL_GMII 0 #define SYSCFG_MCU_ETH_SEL_MII 0 #define SYSCFG_MCU_ETH_SEL_RMII 1 @@ -90,19 +83,33 @@ 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; struct device *dev; }; +struct stm32_syscfg_pmcsetr { + u32 eth1_clk_sel; + u32 eth1_ref_clk_sel; + u32 eth1_selmii; + u32 eth1_sel_rgmii; + u32 eth1_sel_rmii; + u32 eth2_clk_sel; + u32 eth2_ref_clk_sel; + u32 eth2_sel_rgmii; + u32 eth2_sel_rmii; +}; + struct stm32_ops { int (*set_mode)(struct plat_stmmacenet_data *plat_dat); int (*suspend)(struct stm32_dwmac *dwmac); void (*resume)(struct stm32_dwmac *dwmac); int (*parse_data)(struct stm32_dwmac *dwmac, struct device *dev); - u32 syscfg_eth_mask; + u32 syscfg_clr_off; + struct stm32_syscfg_pmcsetr pmcsetr; bool clk_rx_enable_in_suspend; }; @@ -161,7 +168,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) { struct stm32_dwmac *dwmac = plat_dat->bsp_priv; u32 reg = dwmac->mode_reg, clk_rate; - int val; + int val = 0; clk_rate = clk_get_rate(dwmac->clk_eth_ck); dwmac->enable_eth_ck = false; @@ -169,7 +176,7 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) case PHY_INTERFACE_MODE_MII: if (clk_rate == ETH_CK_F_25M && dwmac->ext_phyclk) dwmac->enable_eth_ck = true; - val = SYSCFG_PMCR_ETH_SEL_MII; + val = dwmac->ops->pmcsetr.eth1_selmii; pr_debug("SYSCFG init : PHY_INTERFACE_MODE_MII\n"); break; case PHY_INTERFACE_MODE_GMII: @@ -177,16 +184,17 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) if (clk_rate == ETH_CK_F_25M && (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) { dwmac->enable_eth_ck = true; - val |= SYSCFG_PMCR_ETH_CLK_SEL; + val |= dwmac->ops->pmcsetr.eth1_clk_sel; } pr_debug("SYSCFG init : PHY_INTERFACE_MODE_GMII\n"); break; case PHY_INTERFACE_MODE_RMII: - val = SYSCFG_PMCR_ETH_SEL_RMII; + val = dwmac->ops->pmcsetr.eth1_sel_rmii | dwmac->ops->pmcsetr.eth2_sel_rmii; if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_50M) && (dwmac->eth_ref_clk_sel_reg || dwmac->ext_phyclk)) { dwmac->enable_eth_ck = true; - val |= SYSCFG_PMCR_ETH_REF_CLK_SEL; + val |= dwmac->ops->pmcsetr.eth1_ref_clk_sel; + val |= dwmac->ops->pmcsetr.eth2_ref_clk_sel; } pr_debug("SYSCFG init : PHY_INTERFACE_MODE_RMII\n"); break; @@ -194,11 +202,12 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) case PHY_INTERFACE_MODE_RGMII_ID: case PHY_INTERFACE_MODE_RGMII_RXID: case PHY_INTERFACE_MODE_RGMII_TXID: - val = SYSCFG_PMCR_ETH_SEL_RGMII; + val = dwmac->ops->pmcsetr.eth1_sel_rgmii | dwmac->ops->pmcsetr.eth2_sel_rgmii; if ((clk_rate == ETH_CK_F_25M || clk_rate == ETH_CK_F_125M) && (dwmac->eth_clk_sel_reg || dwmac->ext_phyclk)) { dwmac->enable_eth_ck = true; - val |= SYSCFG_PMCR_ETH_CLK_SEL; + val |= dwmac->ops->pmcsetr.eth1_clk_sel; + val |= dwmac->ops->pmcsetr.eth2_clk_sel; } pr_debug("SYSCFG init : PHY_INTERFACE_MODE_RGMII\n"); break; @@ -210,12 +219,12 @@ static int stm32mp1_set_mode(struct plat_stmmacenet_data *plat_dat) } /* Need to update PMCCLRR (clear register) */ - regmap_write(dwmac->regmap, reg + SYSCFG_PMCCLRR_OFFSET, - dwmac->ops->syscfg_eth_mask); + regmap_write(dwmac->regmap, reg + 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 stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat) @@ -241,7 +250,7 @@ static int stm32mcu_set_mode(struct plat_stmmacenet_data *plat_dat) } 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) @@ -286,10 +295,17 @@ 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) { + dev_err(dev, "Can't get sysconfig register 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_err(dev, "Can't get sysconfig mode offset (%d)\n", err); + pr_debug("Warning sysconfig register mask not set\n"); - return err; + return 0; } static int stm32mp1_parse_data(struct stm32_dwmac *dwmac, @@ -478,8 +494,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 = { @@ -487,8 +502,19 @@ 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, - .clk_rx_enable_in_suspend = true + .clk_rx_enable_in_suspend = true, + .syscfg_clr_off = 0x44, + .pmcsetr = { + .eth1_clk_sel = BIT(16), + .eth1_ref_clk_sel = BIT(17), + .eth1_selmii = BIT(20), + .eth1_sel_rgmii = BIT(21), + .eth1_sel_rmii = BIT(23), + .eth2_clk_sel = 0, + .eth2_ref_clk_sel = 0, + .eth2_sel_rgmii = 0, + .eth2_sel_rmii = 0 + } }; static const struct of_device_id stm32_dwmac_match[] = {
Change glue to be more generic and manage easily next stm32 products. The goal of this commit is to have one stm32mp1_set_mode function which can manage different STM32 SOC. SOC can have different SYSCFG register bitfields. so in pmcsetr we defined the bitfields corresponding to the SOC. Signed-off-by: Christophe Roullier <christophe.roullier@foss.st.com> --- .../net/ethernet/stmicro/stmmac/dwmac-stm32.c | 76 +++++++++++++------ 1 file changed, 51 insertions(+), 25 deletions(-)