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

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

[Sagar Cheluvegowda]

Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
---
Sagar Cheluvegowda (3):
      net: stmmac: Add interconnect support in qcom-ethqos driver
      net: stmmac: Bring down the clocks to lower frequencies when mac link goes down
      dt-bindings: net: qcom: ethernet: Add interconnect properties

 .../devicetree/bindings/net/qcom,ethqos.yaml          |  8 ++++++++
 .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c   | 19 +++++++++++++++++++
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c     |  3 +++
 3 files changed, 30 insertions(+)
---
base-commit: 8a92980606e3585d72d510a03b59906e96755b8a
change-id: 20240610-icc_bw_voting_from_ethqos-12f5c6ed46c2

Best regards,
-- 
Sagar Cheluvegowda <quic_scheluve@quicinc.com>

[PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Sagar Cheluvegowda]

Add interconnect support in qcom-ethqos driver 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.

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

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
index e254b21fdb59..682e68f37dbd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
@@ -7,6 +7,7 @@
 #include <linux/platform_device.h>
 #include <linux/phy.h>
 #include <linux/phy/phy.h>
+#include <linux/interconnect.h>
 
 #include "stmmac.h"
 #include "stmmac_platform.h"
@@ -113,6 +114,9 @@ struct qcom_ethqos {
 	unsigned int num_por;
 	bool rgmii_config_loopback_en;
 	bool has_emac_ge_3;
+
+	struct icc_path *axi_icc_path;
+	struct icc_path *ahb_icc_path;
 };
 
 static int rgmii_readl(struct qcom_ethqos *ethqos, unsigned int offset)
@@ -668,12 +672,19 @@ static int ethqos_configure(struct qcom_ethqos *ethqos)
 	return ethqos->configure_func(ethqos);
 }
 
+static void ethqos_set_icc_bw(struct qcom_ethqos *ethqos, unsigned int speed)
+{
+	icc_set_bw(ethqos->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
+	icc_set_bw(ethqos->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
+}
+
 static void ethqos_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
 {
 	struct qcom_ethqos *ethqos = priv;
 
 	ethqos->speed = speed;
 	ethqos_update_link_clk(ethqos, speed);
+	ethqos_set_icc_bw(ethqos, speed);
 	ethqos_configure(ethqos);
 }
 
@@ -813,6 +824,14 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
 				     "Failed to get link_clk\n");
 
+	ethqos->axi_icc_path = devm_of_icc_get(dev, "axi_icc_path");
+	if (IS_ERR(ethqos->axi_icc_path))
+		return PTR_ERR(ethqos->axi_icc_path);
+
+	ethqos->ahb_icc_path = devm_of_icc_get(dev, "ahb_icc_path");
+	if (IS_ERR(ethqos->axi_icc_path))
+		return PTR_ERR(ethqos->axi_icc_path);
+
 	ret = ethqos_clks_config(ethqos, true);
 	if (ret)
 		return ret;

-- 
2.34.1

Re: [PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Andrew Lunn]

On Wed, Jun 19, 2024 at 03:41:29PM -0700, Sagar Cheluvegowda wrote:
> Add interconnect support in qcom-ethqos driver 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.

What do you mean by Apps?

> Vote from each interconnect client is aggregated and the on-chip
> interconnect hardware is configured to the most appropriate
> bandwidth profile.
> 
> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c   | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> index e254b21fdb59..682e68f37dbd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> @@ -7,6 +7,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/phy.h>
>  #include <linux/phy/phy.h>
> +#include <linux/interconnect.h>

If you look at these includes, you should notice they are
alphabetical.

> +static void ethqos_set_icc_bw(struct qcom_ethqos *ethqos, unsigned int speed)
> +{
> +	icc_set_bw(ethqos->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
> +	icc_set_bw(ethqos->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
> +}
> +
>  static void ethqos_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>  {
>  	struct qcom_ethqos *ethqos = priv;
>  
>  	ethqos->speed = speed;
>  	ethqos_update_link_clk(ethqos, speed);
> +	ethqos_set_icc_bw(ethqos, speed);
>  	ethqos_configure(ethqos);
>  }
>  
> @@ -813,6 +824,14 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
>  				     "Failed to get link_clk\n");
>  
> +	ethqos->axi_icc_path = devm_of_icc_get(dev, "axi_icc_path");
> +	if (IS_ERR(ethqos->axi_icc_path))
> +		return PTR_ERR(ethqos->axi_icc_path);
> +
> +	ethqos->ahb_icc_path = devm_of_icc_get(dev, "ahb_icc_path");
> +	if (IS_ERR(ethqos->axi_icc_path))
> +		return PTR_ERR(ethqos->axi_icc_path);
> +

This all looks pretty generic. Any reason why this is just in the
Qualcomm device, and not at a higher level so it could be used for all
stmmac devices if the needed properties are found in DT?

       Andrew

Re: [PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Sagar Cheluvegowda]

On 6/19/2024 4:13 PM, Andrew Lunn wrote:
> On Wed, Jun 19, 2024 at 03:41:29PM -0700, Sagar Cheluvegowda wrote:
>> Add interconnect support in qcom-ethqos driver 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.
> 
> What do you mean by Apps?
> Apps means application processor.

>> Vote from each interconnect client is aggregated and the on-chip
>> interconnect hardware is configured to the most appropriate
>> bandwidth profile.
>>
>> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
>> ---
>>  .../net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c   | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> index e254b21fdb59..682e68f37dbd 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
>> @@ -7,6 +7,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/phy.h>
>>  #include <linux/phy/phy.h>
>> +#include <linux/interconnect.h>
> 
> If you look at these includes, you should notice they are
> alphabetical.
> Agreed, let me update it in v2 of this series.
>> +static void ethqos_set_icc_bw(struct qcom_ethqos *ethqos, unsigned int speed)
>> +{
>> +	icc_set_bw(ethqos->axi_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
>> +	icc_set_bw(ethqos->ahb_icc_path, Mbps_to_icc(speed), Mbps_to_icc(speed));
>> +}
>> +
>>  static void ethqos_fix_mac_speed(void *priv, unsigned int speed, unsigned int mode)
>>  {
>>  	struct qcom_ethqos *ethqos = priv;
>>  
>>  	ethqos->speed = speed;
>>  	ethqos_update_link_clk(ethqos, speed);
>> +	ethqos_set_icc_bw(ethqos, speed);
>>  	ethqos_configure(ethqos);
>>  }
>>  
>> @@ -813,6 +824,14 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
>>  				     "Failed to get link_clk\n");
>>  
>> +	ethqos->axi_icc_path = devm_of_icc_get(dev, "axi_icc_path");
>> +	if (IS_ERR(ethqos->axi_icc_path))
>> +		return PTR_ERR(ethqos->axi_icc_path);
>> +
>> +	ethqos->ahb_icc_path = devm_of_icc_get(dev, "ahb_icc_path");
>> +	if (IS_ERR(ethqos->axi_icc_path))
>> +		return PTR_ERR(ethqos->axi_icc_path);
>> +
> 
> This all looks pretty generic. Any reason why this is just in the
> Qualcomm device, and not at a higher level so it could be used for all
> stmmac devices if the needed properties are found in DT?
> 
>        Andrew
ICC is a software framework to access the NOC bus topology of the
system, all though "axi" and "ahb" buses seem generic but the 
topologies of these NOC's are specific to the vendors of synopsys chipset hence
this framework might not be applicable to all the vendors of stmmac driver.

	Sagar

Re: [PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Andrew Lunn]

> > This all looks pretty generic. Any reason why this is just in the
> > Qualcomm device, and not at a higher level so it could be used for all
> > stmmac devices if the needed properties are found in DT?
> > 
> >        Andrew
> ICC is a software framework to access the NOC bus topology of the
> system, all though "axi" and "ahb" buses seem generic but the 
> topologies of these NOC's are specific to the vendors of synopsys chipset hence
> this framework might not be applicable to all the vendors of stmmac driver.

There are however a number of SoCs using synopsys IP. Am i right in
says they could all make use of this? Do we really want them to one by
one copy/paste what you have here to other vendor specific parts of
stmmac?

This code looks in DT. If there are no properties in DT, it does
nothing. So in general it should be safe, right?

	 Andrew

Re: [PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Andrew Halaney]

On Fri, Jun 21, 2024 at 10:01:39PM GMT, Andrew Lunn wrote:
> > > This all looks pretty generic. Any reason why this is just in the
> > > Qualcomm device, and not at a higher level so it could be used for all
> > > stmmac devices if the needed properties are found in DT?
> > > 
> > >        Andrew
> > ICC is a software framework to access the NOC bus topology of the
> > system, all though "axi" and "ahb" buses seem generic but the 
> > topologies of these NOC's are specific to the vendors of synopsys chipset hence
> > this framework might not be applicable to all the vendors of stmmac driver.
> 
> There are however a number of SoCs using synopsys IP. Am i right in
> says they could all make use of this? Do we really want them to one by
> one copy/paste what you have here to other vendor specific parts of
> stmmac?
> 
> This code looks in DT. If there are no properties in DT, it does
> nothing. So in general it should be safe, right?

That logic makes sense to me, and thinking about it more you request a
"path" between two "endpoints" in the network, and that's pretty
generic. Sort of like the clocks, etc, and then let the provider figure
out the gory SoC specific details.

i.e., for example I see the UFS driver uses the paths "ufs-ddr" and
"cpu-ufs", and thinking about it generically for this IP that's probably
the same thing going on here (and lends weight to Krzysztof's request to
use names similar to other interconnect users).

That being said, grepping around I don't see users outside of platform
driver bits (i.e. I was hoping to see drivers/pci/controller/dwc/ doing
some shared usage, but that's not the case). Given what you said I'm
of the opinion now this should be done in stmmac_platform.c
and described for all stmmac users since it feels like a property of the
IP itself similar to the clocks required, etc. The interconnect framework handles
when they're not described in the dts gracefully so it shouldn't break any
other SoCs that don't describe interconnects currently.

Thanks,
Andrew

Re: [PATCH 1/3] net: stmmac: Add interconnect support in qcom-ethqos driver

[Krzysztof Kozlowski]

On 20/06/2024 00:41, Sagar Cheluvegowda wrote:
>  
> @@ -813,6 +824,14 @@ static int qcom_ethqos_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(ethqos->link_clk),
>  				     "Failed to get link_clk\n");
>  
> +	ethqos->axi_icc_path = devm_of_icc_get(dev, "axi_icc_path");

Order your patches correctly. Bindings always go before users.

Best regards,
Krzysztof

[PATCH 2/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 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index b3afc7cb7d72..bfb201a4b885 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -990,6 +990,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;

-- 
2.34.1

[PATCH 3/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.

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..bf2a197342a5 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_icc_path
+      - const: ahb_icc_path
+
   phys: true
 
   phy-names:

-- 
2.34.1

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

[Krzysztof Kozlowski]

On 20/06/2024 00:41, Sagar Cheluvegowda wrote:
> Add documentation for the interconnect and interconnect-names
> properties required when voting for AHB and AXI buses.
> 
> 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..bf2a197342a5 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_icc_path
> +      - const: ahb_icc_path

Drop "_icc_path", redundant and instead describe it like other bindings.
Also, names use hyphens. Just open any Qualcomm DTS.


Best regards,
Krzysztof

Re: [PATCH 0/3] Add interconnect support for qcom_ethqos driver.

[Andrew Lunn]

On Wed, Jun 19, 2024 at 03:41:28PM -0700, Sagar Cheluvegowda wrote:
> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@quicinc.com>
> ---

You are supposed to put some text here which explains the big picture
of this patchset. The text will also be used in the text of the git
merge when the patchset is merged into net-next/main.

    Andrew

---
pw-bot: cr