[PATCH v2 0/3] Add interconnect support for stmmac driver.

[PATCH v2 0/3] Add interconnect support for stmmac driver.

[Sagar Cheluvegowda]

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,
-- 
Sagar Cheluvegowda <quic_scheluve@quicinc.com>

[PATCH v2 1/3] dt-bindings: net: qcom: ethernet: Add interconnect properties

[Sagar Cheluvegowda]

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:
 
   dma-coherent: true
 
+  interconnects:
+    maxItems: 2
+
+  interconnect-names:
+    items:
+      - const: axi
+      - const: ahb
+
   phys: true
 
   phy-names:

-- 
2.34.1

Re: [PATCH v2 1/3] dt-bindings: net: qcom: ethernet: Add interconnect properties

[Krzysztof Kozlowski]

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

Re: [PATCH v2 1/3] dt-bindings: net: qcom: ethernet: Add interconnect properties

[Andrew Halaney]

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.

Re: [PATCH v2 1/3] dt-bindings: net: qcom: ethernet: Add interconnect properties

[Sagar Cheluvegowda]

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.

[PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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(+)

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Krzysztof Kozlowski]

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Lunn]

> >> @@ -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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Lunn]

> +	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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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"

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Lunn]

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Lunn]

> > 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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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.

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Halaney]

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
> 

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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
>>
> 

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Andrew Lunn]

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

Re: [PATCH v2 2/3] net: stmmac: Add interconnect support

[Sagar Cheluvegowda]

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.

[PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Sagar Cheluvegowda]

When mac link goes down we don't need to mainitain the clocks to operate
at higher frequencies, as an optimized solution to save power when
the link goes down we are trying to bring down the clocks to the
frequencies corresponding to the lowest speed possible.

Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index ec7c61ee44d4..f0166f0bc25f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 {
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 
+	if (priv->plat->fix_mac_speed)
+		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
+
 	stmmac_mac_set(priv, priv->ioaddr, false);
 	priv->eee_active = false;
 	priv->tx_lpi_enabled = false;
@@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
 
 	if (priv->dma_cap.fpesel)
 		stmmac_fpe_link_state_handle(priv, false);
+
+	stmmac_set_icc_bw(priv, SPEED_10);
+
+	if (priv->plat->fix_mac_speed)
+		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
 }
 
 static void stmmac_mac_link_up(struct phylink_config *config,

-- 
2.34.1

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Andrew Halaney]

On Tue, Jun 25, 2024 at 04:49:30PM GMT, Sagar Cheluvegowda wrote:
> When mac link goes down we don't need to mainitain the clocks to operate
> at higher frequencies, as an optimized solution to save power when
> the link goes down we are trying to bring down the clocks to the
> frequencies corresponding to the lowest speed possible.
> 
> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec7c61ee44d4..f0166f0bc25f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>  {
>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>  
> +	if (priv->plat->fix_mac_speed)
> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
> +
>  	stmmac_mac_set(priv, priv->ioaddr, false);
>  	priv->eee_active = false;
>  	priv->tx_lpi_enabled = false;
> @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>  
>  	if (priv->dma_cap.fpesel)
>  		stmmac_fpe_link_state_handle(priv, false);
> +
> +	stmmac_set_icc_bw(priv, SPEED_10);
> +
> +	if (priv->plat->fix_mac_speed)
> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);


I think you're doing this at the beginning and end of
stmmac_mac_link_down(), is that intentional?

I'm still curious if any of the netdev folks have any opinion on scaling
things down like this on link down.

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Andrew Lunn]

> I'm still curious if any of the netdev folks have any opinion on scaling
> things down like this on link down.

It does make sense, in that there are no frames to process, so the
clock can be reduced. But i also think it is a bit of a workaround for
poor hardware design. Often you can tell the MAC the link is down, and
it can shut down a lot more, and even turn all the clocks off.

I also wounder if there are going to be any side effects of this. Some
Ethernet MACs export a clock to the PHY. Is that clock going to
change? I don't think it will, because we are changing to a valid MAC
speed, not 0. So the PHY has to work at this speed clock.

But to make it easier to find issues like this, open() should probably
set the clocks to a low speed until the link is up. That way, if there
are going to be problems, the link should never come up, as opposed to
the link never comes up after being lost the first time...

     Andrew

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Sagar Cheluvegowda]

On 6/26/2024 7:58 AM, Andrew Halaney wrote:
> On Tue, Jun 25, 2024 at 04:49:30PM GMT, Sagar Cheluvegowda wrote:
>> When mac link goes down we don't need to mainitain the clocks to operate
>> at higher frequencies, as an optimized solution to save power when
>> the link goes down we are trying to bring down the clocks to the
>> frequencies corresponding to the lowest speed possible.
>>
>> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index ec7c61ee44d4..f0166f0bc25f 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>>  
>> +	if (priv->plat->fix_mac_speed)
>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
>> +
>>  	stmmac_mac_set(priv, priv->ioaddr, false);
>>  	priv->eee_active = false;
>>  	priv->tx_lpi_enabled = false;
>> @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>  
>>  	if (priv->dma_cap.fpesel)
>>  		stmmac_fpe_link_state_handle(priv, false);
>> +
>> +	stmmac_set_icc_bw(priv, SPEED_10);
>> +
>> +	if (priv->plat->fix_mac_speed)
>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
> 
> 
> I think you're doing this at the beginning and end of
> stmmac_mac_link_down(), is that intentional?
> 
>

I realised that bringing down the clock to 10Mbps should be the last operation
of the link down process, the reason being if we bring down the clocks first it will
deprive essential internal clocks to DMA/MTL modules which are required for
Cleanup operations this might cause excessive delays in stopping DMA
or flusing MTL queues.
 
> 

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Sagar Cheluvegowda]

On 6/28/2024 2:50 PM, Sagar Cheluvegowda wrote:
> 
> 
> On 6/26/2024 7:58 AM, Andrew Halaney wrote:
>> On Tue, Jun 25, 2024 at 04:49:30PM GMT, Sagar Cheluvegowda wrote:
>>> When mac link goes down we don't need to mainitain the clocks to operate
>>> at higher frequencies, as an optimized solution to save power when
>>> the link goes down we are trying to bring down the clocks to the
>>> frequencies corresponding to the lowest speed possible.
>>>
>>> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
>>> ---
>>>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> index ec7c61ee44d4..f0166f0bc25f 100644
>>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>>> @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>>  {
>>>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>>>  
>>> +	if (priv->plat->fix_mac_speed)
>>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
>>> +
The above fix_mac_speed needs to be removed, i lately realized this mistake.
>>>  	stmmac_mac_set(priv, priv->ioaddr, false);
>>>  	priv->eee_active = false;
>>>  	priv->tx_lpi_enabled = false;
>>> @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>>  
>>>  	if (priv->dma_cap.fpesel)
>>>  		stmmac_fpe_link_state_handle(priv, false);
>>> +
>>> +	stmmac_set_icc_bw(priv, SPEED_10);
>>> +
>>> +	if (priv->plat->fix_mac_speed)
>>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
>>
>>
>> I think you're doing this at the beginning and end of
>> stmmac_mac_link_down(), is that intentional?
>>
>>
> 
> I realised that bringing down the clock to 10Mbps should be the last operation
> of the link down process, the reason being if we bring down the clocks first it will
> deprive essential internal clocks to DMA/MTL modules which are required for
> Cleanup operations this might cause excessive delays in stopping DMA
> or flusing MTL queues.
>  
>>

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Russell King (Oracle)]

On Tue, Jun 25, 2024 at 04:49:30PM -0700, Sagar Cheluvegowda wrote:
> When mac link goes down we don't need to mainitain the clocks to operate
> at higher frequencies, as an optimized solution to save power when
> the link goes down we are trying to bring down the clocks to the
> frequencies corresponding to the lowest speed possible.

I thought I had already commented on a similar patch, but I can't find
anything in my mailboxes to suggest I had.

> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index ec7c61ee44d4..f0166f0bc25f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>  {
>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>  
> +	if (priv->plat->fix_mac_speed)
> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
> +
>  	stmmac_mac_set(priv, priv->ioaddr, false);
>  	priv->eee_active = false;
>  	priv->tx_lpi_enabled = false;
> @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>  
>  	if (priv->dma_cap.fpesel)
>  		stmmac_fpe_link_state_handle(priv, false);
> +
> +	stmmac_set_icc_bw(priv, SPEED_10);
> +
> +	if (priv->plat->fix_mac_speed)
> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);

Two things here:

1) Why do we need to call fix_mac_speed() at the start and end of this
   stmmac_mac_link_down()?

2) What if the MAC doesn't support 10M operation? For example, dwxgmac2
   and dwxlgmac2 do not support anything below 1G. It feels that this
   is storing up a problem for the future, where a platform that uses
   e.g. xlgmac2 also implements fix_mac_speed() and then gets a
   surprise when it's called with SPEED_10.

Personally, I don't like "fix_mac_speed", and I don't like it even more
with this change. I would prefer to see link_up/link_down style
operations so that platforms can do whatever they need to on those
events, rather than being told what to do by a single call that may
look identical irrespective of whether the link came up or went down.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH v2 3/3] net: stmmac: Bring down the clocks to lower frequencies when mac link goes down

[Sagar Cheluvegowda]

On 6/28/2024 3:16 PM, Russell King (Oracle) wrote:
> On Tue, Jun 25, 2024 at 04:49:30PM -0700, Sagar Cheluvegowda wrote:
>> When mac link goes down we don't need to mainitain the clocks to operate
>> at higher frequencies, as an optimized solution to save power when
>> the link goes down we are trying to bring down the clocks to the
>> frequencies corresponding to the lowest speed possible.
> 
> I thought I had already commented on a similar patch, but I can't find
> anything in my mailboxes to suggest I had.
> 
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> index ec7c61ee44d4..f0166f0bc25f 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
>> @@ -996,6 +996,9 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>  {
>>  	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
>>  
>> +	if (priv->plat->fix_mac_speed)
>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
>> +
>>  	stmmac_mac_set(priv, priv->ioaddr, false);
>>  	priv->eee_active = false;
>>  	priv->tx_lpi_enabled = false;
>> @@ -1004,6 +1007,11 @@ static void stmmac_mac_link_down(struct phylink_config *config,
>>  
>>  	if (priv->dma_cap.fpesel)
>>  		stmmac_fpe_link_state_handle(priv, false);
>> +
>> +	stmmac_set_icc_bw(priv, SPEED_10);
>> +
>> +	if (priv->plat->fix_mac_speed)
>> +		priv->plat->fix_mac_speed(priv->plat->bsp_priv, SPEED_10, mode);
> 
> Two things here:
> 
> 1) Why do we need to call fix_mac_speed() at the start and end of this
>    stmmac_mac_link_down()?
This was a typo, i will remove this.
> 
> 2) What if the MAC doesn't support 10M operation? For example, dwxgmac2
>    and dwxlgmac2 do not support anything below 1G. It feels that this
>    is storing up a problem for the future, where a platform that uses
>    e.g. xlgmac2 also implements fix_mac_speed() and then gets a
>    surprise when it's called with SPEED_10.
> 
> Personally, I don't like "fix_mac_speed", and I don't like it even more
> with this change. I would prefer to see link_up/link_down style
> operations so that platforms can do whatever they need to on those
> events, rather than being told what to do by a single call that may
> look identical irrespective of whether the link came up or went down.
> 
I will drop this patch[3/3] from this series now and i will do some analysis
on platform level link up and link down functions and post the changes as a
new series altogether.