| Message ID | 20240708-icc_bw_voting_from_ethqos-v4-2-c6bc3db86071@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add interconnect support for stmmac driver. | expand |
On Mon, Jul 08, 2024 at 02:30:01PM -0700, Sagar Cheluvegowda wrote: > Add interconnect support to vote for bus bandwidth based > on the current speed of the driver. > Adds support for two different paths - one from ethernet to > DDR and the other from CPU to ethernet, Vote from each > interconnect client is aggregated and the on-chip interconnect > hardware is configured to the most appropriate bandwidth profile. > > Suggested-by: Andrew Halaney <ahalaney@redhat.com> > Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
Hi Sagar On Mon, Jul 08, 2024 at 02:30:01PM -0700, Sagar Cheluvegowda wrote: > Add interconnect support to vote for bus bandwidth based > on the current speed of the driver. > Adds support for two different paths - one from ethernet to > DDR and the other from CPU to ethernet, Vote from each > interconnect client is aggregated and the on-chip interconnect > hardware is configured to the most appropriate bandwidth profile. > > Suggested-by: Andrew Halaney <ahalaney@redhat.com> > Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ > drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++++ > include/linux/stmmac.h | 2 ++ > 4 files changed, 23 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > index b23b920eedb1..56a282d2b8cd 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h > @@ -21,6 +21,7 @@ > #include <linux/ptp_clock_kernel.h> > #include <linux/net_tstamp.h> > #include <linux/reset.h> > +#include <linux/interconnect.h> > #include <net/page_pool/types.h> > #include <net/xdp.h> > #include <uapi/linux/bpf.h> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index b3afc7cb7d72..ec7c61ee44d4 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -985,6 +985,12 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) > } > } > > +static void stmmac_set_icc_bw(struct stmmac_priv *priv, unsigned int speed) > +{ > + icc_set_bw(priv->plat->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); > + icc_set_bw(priv->plat->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); I've got two questions in this regard: 1. Don't we need to call icc_enable()/icc_disable() in someplace in the driver? For instance the CPU-MEM path must be enabled before even the stmmac_dvr_probe() is called, otherwise the CSR won't be accessible. Right? For the same reason the CPU-MEM bandwidth should be set in sync with that. 2. Why is the CPU-MAC speed is specified to match the Ethernet link speed? It doesn't seem reasonable. It's the CSR's access speed and should be done as fast as possible. Shouldn't it? > +} > + > static void stmmac_mac_link_down(struct phylink_config *config, > unsigned int mode, phy_interface_t interface) > { > @@ -1080,6 +1086,8 @@ static void stmmac_mac_link_up(struct phylink_config *config, > if (priv->plat->fix_mac_speed) > priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed, mode); > > + stmmac_set_icc_bw(priv, speed); > + > if (!duplex) > ctrl &= ~priv->hw->link.duplex; > else > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index 54797edc9b38..201f9dea6da9 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -642,6 +642,18 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate); > } > > + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "mac-mem"); > + if (IS_ERR(plat->axi_icc_path)) { > + ret = ERR_CAST(plat->axi_icc_path); > + goto error_hw_init; > + } > + > + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "cpu-mac"); > + if (IS_ERR(plat->ahb_icc_path)) { > + ret = ERR_CAST(plat->ahb_icc_path); > + goto error_hw_init; > + } > + > plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev, > STMMAC_RESOURCE_NAME); > if (IS_ERR(plat->stmmac_rst)) { > diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h > index f92c195c76ed..385f352a0c23 100644 > --- a/include/linux/stmmac.h > +++ b/include/linux/stmmac.h > @@ -283,6 +283,8 @@ struct plat_stmmacenet_data { > struct reset_control *stmmac_rst; > struct reset_control *stmmac_ahb_rst; > struct stmmac_axi *axi; > + struct icc_path *axi_icc_path; The MAC<->MEM interface isn't always AXI (it can be AHB or custom) and > + struct icc_path *ahb_icc_path; the CPU<->MAC isn't always AHB (it can also be APB, AXI, custom). So the more generic naming would be: axi_icc_path -> dma_icc_path and ahb_icc_path -> csr_icc_path -Serge(y) > int has_gmac4; > int rss_en; > int mac_port_sel_speed; > > -- > 2.34.1 > >
On 8/1/2024 11:32 AM, Serge Semin wrote: > Hi Sagar > > On Mon, Jul 08, 2024 at 02:30:01PM -0700, Sagar Cheluvegowda wrote: >> Add interconnect support to vote for bus bandwidth based >> on the current speed of the driver. >> Adds support for two different paths - one from ethernet to >> DDR and the other from CPU to ethernet, Vote from each >> interconnect client is aggregated and the on-chip interconnect >> hardware is configured to the most appropriate bandwidth profile. >> >> Suggested-by: Andrew Halaney <ahalaney@redhat.com> >> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> >> --- >> drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + >> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++++ >> include/linux/stmmac.h | 2 ++ >> 4 files changed, 23 insertions(+) >> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> index b23b920eedb1..56a282d2b8cd 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h >> @@ -21,6 +21,7 @@ >> #include <linux/ptp_clock_kernel.h> >> #include <linux/net_tstamp.h> >> #include <linux/reset.h> >> +#include <linux/interconnect.h> >> #include <net/page_pool/types.h> >> #include <net/xdp.h> >> #include <uapi/linux/bpf.h> >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> index b3afc7cb7d72..ec7c61ee44d4 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c >> @@ -985,6 +985,12 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) >> } >> } >> >> +static void stmmac_set_icc_bw(struct stmmac_priv *priv, unsigned int speed) >> +{ > >> + icc_set_bw(priv->plat->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); >> + icc_set_bw(priv->plat->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); > > I've got two questions in this regard: > > 1. Don't we need to call icc_enable()/icc_disable() in someplace in > the driver? For instance the CPU-MEM path must be enabled before even > the stmmac_dvr_probe() is called, otherwise the CSR won't be > accessible. Right? For the same reason the CPU-MEM bandwidth should be > set in sync with that. > > 2. Why is the CPU-MAC speed is specified to match the Ethernet link > speed? It doesn't seem reasonable. It's the CSR's access speed and > should be done as fast as possible. Shouldn't it? > >> +} I am having internal discussions with clocks team, I will revert back soon with answers. >> + >> static void stmmac_mac_link_down(struct phylink_config *config, >> unsigned int mode, phy_interface_t interface) >> { >> @@ -1080,6 +1086,8 @@ static void stmmac_mac_link_up(struct phylink_config *config, >> if (priv->plat->fix_mac_speed) >> priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed, mode); >> >> + stmmac_set_icc_bw(priv, speed); >> + >> if (!duplex) >> ctrl &= ~priv->hw->link.duplex; >> else >> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> index 54797edc9b38..201f9dea6da9 100644 >> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c >> @@ -642,6 +642,18 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) >> dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate); >> } >> >> + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "mac-mem"); >> + if (IS_ERR(plat->axi_icc_path)) { >> + ret = ERR_CAST(plat->axi_icc_path); >> + goto error_hw_init; >> + } >> + >> + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "cpu-mac"); >> + if (IS_ERR(plat->ahb_icc_path)) { >> + ret = ERR_CAST(plat->ahb_icc_path); >> + goto error_hw_init; >> + } >> + >> plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev, >> STMMAC_RESOURCE_NAME); >> if (IS_ERR(plat->stmmac_rst)) { >> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h >> index f92c195c76ed..385f352a0c23 100644 >> --- a/include/linux/stmmac.h >> +++ b/include/linux/stmmac.h >> @@ -283,6 +283,8 @@ struct plat_stmmacenet_data { >> struct reset_control *stmmac_rst; >> struct reset_control *stmmac_ahb_rst; >> struct stmmac_axi *axi; > >> + struct icc_path *axi_icc_path; > > The MAC<->MEM interface isn't always AXI (it can be AHB or custom) and > >> + struct icc_path *ahb_icc_path; > > the CPU<->MAC isn't always AHB (it can also be APB, AXI, custom). So > the more generic naming would be: > > axi_icc_path -> dma_icc_path > and > ahb_icc_path -> csr_icc_path > > -Serge(y) > >> int has_gmac4; >> int rss_en; >> int mac_port_sel_speed; >> >> -- >> 2.34.1 >> >>
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h index b23b920eedb1..56a282d2b8cd 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h @@ -21,6 +21,7 @@ #include <linux/ptp_clock_kernel.h> #include <linux/net_tstamp.h> #include <linux/reset.h> +#include <linux/interconnect.h> #include <net/page_pool/types.h> #include <net/xdp.h> #include <uapi/linux/bpf.h> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index b3afc7cb7d72..ec7c61ee44d4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -985,6 +985,12 @@ static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up) } } +static void stmmac_set_icc_bw(struct stmmac_priv *priv, unsigned int speed) +{ + icc_set_bw(priv->plat->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); + icc_set_bw(priv->plat->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed)); +} + static void stmmac_mac_link_down(struct phylink_config *config, unsigned int mode, phy_interface_t interface) { @@ -1080,6 +1086,8 @@ static void stmmac_mac_link_up(struct phylink_config *config, if (priv->plat->fix_mac_speed) priv->plat->fix_mac_speed(priv->plat->bsp_priv, speed, mode); + stmmac_set_icc_bw(priv, speed); + if (!duplex) ctrl &= ~priv->hw->link.duplex; else diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index 54797edc9b38..201f9dea6da9 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -642,6 +642,18 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) dev_dbg(&pdev->dev, "PTP rate %d\n", plat->clk_ptp_rate); } + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "mac-mem"); + if (IS_ERR(plat->axi_icc_path)) { + ret = ERR_CAST(plat->axi_icc_path); + goto error_hw_init; + } + + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "cpu-mac"); + if (IS_ERR(plat->ahb_icc_path)) { + ret = ERR_CAST(plat->ahb_icc_path); + goto error_hw_init; + } + plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev, STMMAC_RESOURCE_NAME); if (IS_ERR(plat->stmmac_rst)) { diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index f92c195c76ed..385f352a0c23 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -283,6 +283,8 @@ struct plat_stmmacenet_data { struct reset_control *stmmac_rst; struct reset_control *stmmac_ahb_rst; struct stmmac_axi *axi; + struct icc_path *axi_icc_path; + struct icc_path *ahb_icc_path; int has_gmac4; int rss_en; int mac_port_sel_speed;
Add interconnect support to vote for bus bandwidth based on the current speed of the driver. Adds support for two different paths - one from ethernet to DDR and the other from CPU to ethernet, Vote from each interconnect client is aggregated and the on-chip interconnect hardware is configured to the most appropriate bandwidth profile. Suggested-by: Andrew Halaney <ahalaney@redhat.com> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++++ include/linux/stmmac.h | 2 ++ 4 files changed, 23 insertions(+)