| Message ID | 20240625-icc_bw_voting_from_ethqos-v2-2-eaa7cf9060f0@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add interconnect support for stmmac driver. | expand |
On 26/06/2024 01:49, Sagar Cheluvegowda wrote: > Add interconnect support to vote for bus bandwidth based > on the current speed of the driver.This change adds support Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 Also, space after full stop. > for two different paths - one from ethernet to DDR and the > other from Apps 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)); > +} > + > 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..e46c94b643a3 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, "axi"); > + if (IS_ERR(plat->axi_icc_path)) { > + ret = (void *)plat->axi_icc_path; > + goto error_hw_init; This sounds like an ABI break. Considering the interconnects are not required by the binding, are you sure this behaves correctly without interconnects in DTS? Best regards, Krzysztof
> + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); > + if (IS_ERR(plat->axi_icc_path)) { > + ret = (void *)plat->axi_icc_path; Casting to a void * seems odd. ERR_PTR()? Andrew
On Tue, Jun 25, 2024 at 04:49:29PM GMT, Sagar Cheluvegowda wrote: > Add interconnect support to vote for bus bandwidth based > on the current speed of the driver.This change adds support > for two different paths - one from ethernet to DDR and the > other from Apps to ethernet. "APPS" is a qualcomm term, since you're trying to go the generic route here maybe just say 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)); > +} > + > 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..e46c94b643a3 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, "axi"); > + if (IS_ERR(plat->axi_icc_path)) { > + ret = (void *)plat->axi_icc_path; > + goto error_hw_init; > + } > + > + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "ahb"); > + if (IS_ERR(plat->ahb_icc_path)) { > + ret = (void *)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; > > -- > 2.34.1 >
On 6/26/2024 6:07 AM, Andrew Lunn wrote: >> + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); >> + if (IS_ERR(plat->axi_icc_path)) { >> + ret = (void *)plat->axi_icc_path; > > Casting to a void * seems odd. ERR_PTR()? > > Andrew The output of devm_of_icc_get is a pointer of type icc_path, i am getting below warning when i try to ERR_PTR instead of Void* as ERR_PTR will try to convert a long integer to a Void*. "warning: passing argument 1 of ‘ERR_PTR’ makes integer from pointer without a cast"
On 6/26/2024 7:54 AM, Andrew Halaney wrote: > On Tue, Jun 25, 2024 at 04:49:29PM GMT, Sagar Cheluvegowda wrote: >> Add interconnect support to vote for bus bandwidth based >> on the current speed of the driver.This change adds support >> for two different paths - one from ethernet to DDR and the >> other from Apps to ethernet. > > "APPS" is a qualcomm term, since you're trying to go the generic route > here maybe just say CPU to ethernet? > I can update this in my next patch. Sagar >> 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)); >> +} >> + >> 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..e46c94b643a3 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, "axi"); >> + if (IS_ERR(plat->axi_icc_path)) { >> + ret = (void *)plat->axi_icc_path; >> + goto error_hw_init; >> + } >> + >> + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "ahb"); >> + if (IS_ERR(plat->ahb_icc_path)) { >> + ret = (void *)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; >> >> -- >> 2.34.1 >> >
On Wed, Jun 26, 2024 at 04:36:06PM -0700, Sagar Cheluvegowda wrote: > > > On 6/26/2024 6:07 AM, Andrew Lunn wrote: > >> + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); > >> + if (IS_ERR(plat->axi_icc_path)) { > >> + ret = (void *)plat->axi_icc_path; > > > > Casting to a void * seems odd. ERR_PTR()? > > > > Andrew > > The output of devm_of_icc_get is a pointer of type icc_path, > i am getting below warning when i try to ERR_PTR instead of Void* > as ERR_PTR will try to convert a long integer to a Void*. > > "warning: passing argument 1 of ‘ERR_PTR’ makes integer from pointer without a cast" > https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/crypto/qce/core.c#L224 https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/msm/adreno/a3xx_gpu.c#L591 https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/msm/adreno/a3xx_gpu.c#L597 https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/spi/spi-qup.c#L1052 Sorry, PTR_ERR(). In general, a cast to a void * is a red flag and will get looked at. It is generally wrong. So you might want to fixup where ever you copied this from. Andrew
On Wed, Jun 26, 2024 at 04:38:34PM -0700, Sagar Cheluvegowda wrote: > > > On 6/26/2024 7:54 AM, Andrew Halaney wrote: > > On Tue, Jun 25, 2024 at 04:49:29PM GMT, Sagar Cheluvegowda wrote: > >> Add interconnect support to vote for bus bandwidth based > >> on the current speed of the driver.This change adds support > >> for two different paths - one from ethernet to DDR and the > >> other from Apps to ethernet. > > > > "APPS" is a qualcomm term, since you're trying to go the generic route > > here maybe just say CPU to ethernet? > > > I can update this in my next patch. > > Sagar Please trim emails when replying to just the needed context. Also, i asked what Apps meant in response to an earlier version of this patch. I think you ignored me.... Andrew
On 6/26/2024 12:40 AM, Krzysztof Kozlowski wrote: > On 26/06/2024 01:49, Sagar Cheluvegowda wrote: >> Add interconnect support to vote for bus bandwidth based >> on the current speed of the driver.This change adds support > > Please do not use "This commit/patch/change", but imperative mood. See > longer explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > Also, space after full stop. > Agreed, i will fix this in my next patch. >> for two different paths - one from ethernet to DDR and the >> other from Apps 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)); >> +} >> + >> 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..e46c94b643a3 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, "axi"); >> + if (IS_ERR(plat->axi_icc_path)) { >> + ret = (void *)plat->axi_icc_path; >> + goto error_hw_init; > > This sounds like an ABI break. Considering the interconnects are not > required by the binding, are you sure this behaves correctly without > interconnects in DTS? > > Best regards, > Krzysztof > Yes, i did check without the interconnect entries in the dtsi and things are working fine, devm_of_icc_get is properly clearing out all the references in the case when "interconnects" are not present in the dtsi. In the stmmac driver we are only handling the error case and if the entries are not present in the dtsi we are simply continuing with the regular code flow. Regards, Sagar
On 6/26/2024 5:14 PM, Andrew Lunn wrote: > On Wed, Jun 26, 2024 at 04:38:34PM -0700, Sagar Cheluvegowda wrote: >> >> >> On 6/26/2024 7:54 AM, Andrew Halaney wrote: >>> On Tue, Jun 25, 2024 at 04:49:29PM GMT, Sagar Cheluvegowda wrote: >>>> Add interconnect support to vote for bus bandwidth based >>>> on the current speed of the driver.This change adds support >>>> for two different paths - one from ethernet to DDR and the >>>> other from Apps to ethernet. >>> >>> "APPS" is a qualcomm term, since you're trying to go the generic route >>> here maybe just say CPU to ethernet? >>> >> I can update this in my next patch. >> >> Sagar > > Please trim emails when replying to just the needed context. > > Also, i asked what Apps meant in response to an earlier version of > this patch. I think you ignored me.... > > Andrew Thanks Andrew, i will take a note of it when replying next time. Regarding the Apps part, i had replied to your email on 06/21.
> >> @@ -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, "axi"); > >> + if (IS_ERR(plat->axi_icc_path)) { > >> + ret = (void *)plat->axi_icc_path; > >> + goto error_hw_init; > > > > This sounds like an ABI break. Considering the interconnects are not > > required by the binding, are you sure this behaves correctly without > > interconnects in DTS? > > > > Best regards, > > Krzysztof > > > Yes, i did check without the interconnect entries in the dtsi and > things are working fine, devm_of_icc_get is properly clearing out > all the references in the case when "interconnects" are not present > in the dtsi. So the relevant code is: https://elixir.bootlin.com/linux/latest/source/drivers/interconnect/core.c#L566 /* * When the consumer DT node do not have "interconnects" property * return a NULL path to skip setting constraints. */ if (!of_property_present(np, "interconnects")) return NULL; The naming of of_icc_get() and devm_of_icc_get() is not great. Typically this behaviour of not giving an error if it is missing would mean the functions would be of_icc_get_optional() and devm_of_icc_get_optional(), e.g. we have clk_get_optional(), gpiod_get_optional(), regulator_get_optional(), etc. Andrew
On 6/26/2024 5:12 PM, Andrew Lunn wrote: > On Wed, Jun 26, 2024 at 04:36:06PM -0700, Sagar Cheluvegowda wrote: >> >> >> On 6/26/2024 6:07 AM, Andrew Lunn wrote: >>>> + plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); >>>> + if (IS_ERR(plat->axi_icc_path)) { >>>> + ret = (void *)plat->axi_icc_path; >>> >>> Casting to a void * seems odd. ERR_PTR()? >>> >>> Andrew >> >> The output of devm_of_icc_get is a pointer of type icc_path, >> i am getting below warning when i try to ERR_PTR instead of Void* >> as ERR_PTR will try to convert a long integer to a Void*. >> >> "warning: passing argument 1 of ‘ERR_PTR’ makes integer from pointer without a cast" >> > > https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/crypto/qce/core.c#L224 > https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/msm/adreno/a3xx_gpu.c#L591 > https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/gpu/drm/msm/adreno/a3xx_gpu.c#L597 > https://elixir.bootlin.com/linux/v6.10-rc5/source/drivers/spi/spi-qup.c#L1052 > > Sorry, PTR_ERR(). > > In general, a cast to a void * is a red flag and will get looked > at. It is generally wrong. So you might want to fixup where ever you > copied this from. > > Andrew the return type of stmmac_probe_config_dt is a pointer of type plat_stmmacenet_data, as PTR_ERR would give long integer value i don't think it would be ideal to return an integer value here, if casting plat->axi_icc_path to a void * doesn't look good, let me if the below solution is better or not? plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); if (IS_ERR(plat->axi_icc_path)) { rc = PTR_ERR(plat->axi_icc_path); ret = ERR_PTR(rc); goto error_hw_init; } plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "ahb"); if (IS_ERR(plat->ahb_icc_path)) { rc = PTR_ERR(plat->ahb_icc_path); ret = ERR_PTR(rc); goto error_hw_init; } plat->stmmac_rst = devm_reset_control_get_optional(&pdev->dev, STMMAC_RESOURCE_NAME); if (IS_ERR(plat->stmmac_rst)) { ret = plat->stmmac_rst; goto error_hw_init; } plat->stmmac_ahb_rst = devm_reset_control_get_optional_shared( &pdev->dev, "ahb"); if (IS_ERR(plat->stmmac_ahb_rst)) { ret = plat->stmmac_ahb_rst; goto error_hw_init; } return plat; error_hw_init: clk_disable_unprepare(plat->pclk); error_pclk_get: clk_disable_unprepare(plat->stmmac_clk); return ret; } Regards, Sagar
> > Sorry, PTR_ERR(). > > > > In general, a cast to a void * is a red flag and will get looked > > at. It is generally wrong. So you might want to fixup where ever you > > copied this from. > > > > Andrew > the return type of stmmac_probe_config_dt is a pointer of type plat_stmmacenet_data, > as PTR_ERR would give long integer value i don't think it would be ideal to > return an integer value here, if casting plat->axi_icc_path to a void * doesn't look > good, let me if the below solution is better or not? > plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); > if (IS_ERR(plat->axi_icc_path)) { > rc = PTR_ERR(plat->axi_icc_path); > ret = ERR_PTR(rc); Don't you think this looks ugly? If it looks ugly, it is probably wrong. You cannot be the first person to find the return type of an error is wrong. So a quick bit of searching found ERR_CAST(). Andrew
On 6/28/2024 3:23 PM, Andrew Lunn wrote: >>> Sorry, PTR_ERR(). >>> >>> In general, a cast to a void * is a red flag and will get looked >>> at. It is generally wrong. So you might want to fixup where ever you >>> copied this from. >>> >>> Andrew > >> the return type of stmmac_probe_config_dt is a pointer of type plat_stmmacenet_data, >> as PTR_ERR would give long integer value i don't think it would be ideal to >> return an integer value here, if casting plat->axi_icc_path to a void * doesn't look >> good, let me if the below solution is better or not? > >> plat->axi_icc_path = devm_of_icc_get(&pdev->dev, "axi"); >> if (IS_ERR(plat->axi_icc_path)) { >> rc = PTR_ERR(plat->axi_icc_path); >> ret = ERR_PTR(rc); > > Don't you think this looks ugly? > > If it looks ugly, it is probably wrong. You cannot be the first person > to find the return type of an error is wrong. So a quick bit of > searching found ERR_CAST(). > > Andrew Thanks Andrew, using ERR_CAST would be ideal here.
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..e46c94b643a3 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, "axi"); + if (IS_ERR(plat->axi_icc_path)) { + ret = (void *)plat->axi_icc_path; + goto error_hw_init; + } + + plat->ahb_icc_path = devm_of_icc_get(&pdev->dev, "ahb"); + if (IS_ERR(plat->ahb_icc_path)) { + ret = (void *)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.This change adds support for two different paths - one from ethernet to DDR and the other from Apps 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(+)