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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0666cba0-a5bb-44bf-845a-6a1c689cb485@quicinc.com>
Date: Fri, 28 Jun 2024 12:43:04 -0700
From: Sagar Cheluvegowda <quic_scheluve@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, Vinod Koul <vkoul@...nel.org>,
        Alexandre Torgue <alexandre.torgue@...s.st.com>,
        Jose Abreu
	<joabreu@...opsys.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni
	<pabeni@...hat.com>,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        "Russell
 King" <linux@...linux.org.uk>, Rob Herring <robh@...nel.org>,
        "Krzysztof
 Kozlowski" <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        "Bhupesh
 Sharma" <bhupesh.sharma@...aro.org>
CC: <kernel@...cinc.com>, Andrew Halaney <ahalaney@...hat.com>,
        Andrew Lunn
	<andrew@...n.ch>, <linux-arm-msm@...r.kernel.org>,
        <netdev@...r.kernel.org>, <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
        <devicetree@...r.kernel.org>
Subject: Re: [PATCH v2 2/3] net: stmmac: Add interconnect support



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@...hat.com>
>> Signed-off-by: Sagar Cheluvegowda <quic_scheluve@...cinc.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