| Message ID | 20240625-icc_bw_voting_from_ethqos-v2-0-eaa7cf9060f0@quicinc.com |
|---|---|
| Headers | show |
| Series | Add interconnect support for stmmac driver. | expand |
On 26/06/2024 01:49, Sagar Cheluvegowda wrote: > Add documentation for the interconnect and interconnect-names > properties required when voting for AHB and AXI buses. > > Suggested-by: Andrew Halaney <ahalaney@redhat.com> > Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> > --- Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 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:28PM GMT, Sagar Cheluvegowda wrote: > Add documentation for the interconnect and interconnect-names > properties required when voting for AHB and AXI buses. > > Suggested-by: Andrew Halaney <ahalaney@redhat.com> > Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> > --- > Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml > index 6672327358bc..b7e2644bfb18 100644 > --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml > +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml > @@ -63,6 +63,14 @@ properties: > Does it make sense to make these changes in snps,dwmac.yaml since you're trying to do this generically for stmmac? I don't poke bindings super often so might be a silly question, the inheritance of snps,dwmac.yaml into the various platform specific bindings (qcom,ethqos.yaml) would then let you define it once in the snps,dwmac.yaml right? > dma-coherent: true > > + interconnects: > + maxItems: 2 > + > + interconnect-names: > + items: > + - const: axi > + - const: ahb Sorry to bikeshed, and with Krzysztof's review on this already its probably unnecessary, but would names like cpu-mac and mac-mem be more generic / appropriate? I see that sort of convention a lot in the other bindings, and to me those read really well and are understandable.
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 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.
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.
On 6/26/2024 7:53 AM, Andrew Halaney wrote: > On Tue, Jun 25, 2024 at 04:49:28PM GMT, Sagar Cheluvegowda wrote: >> Add documentation for the interconnect and interconnect-names >> properties required when voting for AHB and AXI buses. >> >> Suggested-by: Andrew Halaney <ahalaney@redhat.com> >> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> >> --- >> Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml >> index 6672327358bc..b7e2644bfb18 100644 >> --- a/Documentation/devicetree/bindings/net/qcom,ethqos.yaml >> +++ b/Documentation/devicetree/bindings/net/qcom,ethqos.yaml >> @@ -63,6 +63,14 @@ properties: >> > > Does it make sense to make these changes in snps,dwmac.yaml since you're > trying to do this generically for stmmac? I don't poke bindings super > often so might be a silly question, the inheritance of snps,dwmac.yaml > into the various platform specific bindings (qcom,ethqos.yaml) would > then let you define it once in the snps,dwmac.yaml right? > >> dma-coherent: true >> >> + interconnects: >> + maxItems: 2 >> + >> + interconnect-names: >> + items: >> + - const: axi >> + - const: ahb > > Sorry to bikeshed, and with Krzysztof's review on this already its > probably unnecessary, but would names like cpu-mac and mac-mem be > more generic / appropriate? I see that sort of convention a lot in the > other bindings, and to me those read really well and are understandable. I agree with changing the names to "cpu-mac" and "mac-mem" in that way the properties are more understandable. @Krzysztof Kozlowski let me know your opinion on the same.
Interconnect is a software framework to access NOC bus topology of the system, this framework is designed to provide a standard kernel interface to control the settings of the interconnects on an SoC. The interconnect support is now being added to the stmmac driver so that any vendors who wants to use this feature can just define corresponging dtsi properties according to their NOC bus topologies. Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com> --- Changes in v2: - Edit the cover letter to give a big picture of this change. - Move the interconnect changes from ethqos driver to stmmac driver. - Reorder the the patches to place bindings patch on the top. - Remove "_icc_path" redundant string from the "interconnect-names" property. - Link to v1: https://lore.kernel.org/r/20240619-icc_bw_voting_from_ethqos-v1-0-6112948b825e@quicinc.com --- Sagar Cheluvegowda (3): dt-bindings: net: qcom: ethernet: Add interconnect properties net: stmmac: Add interconnect support net: stmmac: Bring down the clocks to lower frequencies when mac link goes down Documentation/devicetree/bindings/net/qcom,ethqos.yaml | 8 ++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac.h | 1 + drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 16 ++++++++++++++++ drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 12 ++++++++++++ include/linux/stmmac.h | 2 ++ 5 files changed, 39 insertions(+) --- base-commit: 8a92980606e3585d72d510a03b59906e96755b8a change-id: 20240610-icc_bw_voting_from_ethqos-12f5c6ed46c2 Best regards,