netdev - Re: [PATCH] net: ethernet: ti: am65-cpsw: improve suspend/resume support for J7200

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] [day] [month] [year] [list]

Message-ID: <87bkbcg0l4.fsf@BL-laptop>
Date: Wed, 29 Nov 2023 14:17:59 +0100
From: Gregory CLEMENT <gregory.clement@...tlin.com>
To: Roger Quadros <rogerq@...nel.org>, Thomas Richard
 <thomas.richard@...tlin.com>, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
 s-vadapalli@...com, grygorii.strashko@...com, dan.carpenter@...aro.org,
 thomas.petazzoni@...tlin.com, u-kumar1@...com
Subject: Re: [PATCH] net: ethernet: ti: am65-cpsw: improve suspend/resume
 support for J7200

Hi Roger,

> Hi,
>
> On 28/11/2023 15:19, Thomas Richard wrote:
>> From: Gregory CLEMENT <gregory.clement@...tlin.com>
>
> Subject is vague. Please be explicit about you are trying to do.

Yes we can do better.

>
>> 
>> On J7200 the SoC is off during suspend, so the clocks have to be
>
> What do you mean by SoC is off? I suppose you are referring to a certain
> low power state of the SoC?

As during suspend to ram all the power rails of the SoC are shutdown, I
guess we can say that the SoC is off.

>
> By "clocks have to be completely powered down" you mean they have to
> be gated in addition to be disabled? What happens if they are left ungated?
> Does it prevent SoC form entering the target low power state?

That prevent to resume properly. The SoC being off, if you don't
explicitly shut down the clocks, then the state of the clock does not
math the expectation of the kernel.

>
>> completely power down, and phy_set_mode_ext must be called again.
>
> Why must phy_set_mode_ext() be called again?

If I remember well, it was because this information is lost during
suspend so we have to set it again.

>
>> 
>
> Not all SoCs behave like J7200 so can we please restrict this change
> to J7200? Thanks.

Did you test it on a Sitara SoC ?

Does it cause a regression ?

Gregory

>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@...tlin.com>
>> Signed-off-by: Thomas Richard <thomas.richard@...tlin.com>
>> ---
>>  drivers/net/ethernet/ti/am65-cpsw-nuss.c | 25 ++++++++++++++++++++++++
>>  drivers/net/ethernet/ti/am65-cpts.c      | 11 +++++++++--
>>  2 files changed, 34 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/net/ethernet/ti/am65-cpsw-nuss.c b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> index ece9f8df98ae..e95ef30bd67f 100644
>> --- a/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> +++ b/drivers/net/ethernet/ti/am65-cpsw-nuss.c
>> @@ -2115,6 +2115,27 @@ static int am65_cpsw_nuss_init_slave_ports(struct am65_cpsw_common *common)
>>  	return ret;
>>  }
>>  
>> +static int am65_cpsw_nuss_resume_slave_ports(struct am65_cpsw_common *common)
>> +{
>> +	struct device *dev = common->dev;
>> +	int i;
>> +
>> +	for (i = 1; i <= common->port_num; i++) {
>> +		struct am65_cpsw_port *port;
>> +		int ret;
>> +
>> +		port = am65_common_get_port(common, i);
>> +
>> +		ret = phy_set_mode_ext(port->slave.ifphy, PHY_MODE_ETHERNET, port->slave.phy_if);
>> +		if (ret) {
>> +			dev_err(dev, "port %d error setting phy mode %d\n", i, ret);
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>  static void am65_cpsw_pcpu_stats_free(void *data)
>>  {
>>  	struct am65_cpsw_ndev_stats __percpu *stats = data;
>> @@ -3087,6 +3108,10 @@ static int am65_cpsw_nuss_resume(struct device *dev)
>>  	if (common->rx_irq_disabled)
>>  		disable_irq(common->rx_chns.irq);
>>  
>> +	ret = am65_cpsw_nuss_resume_slave_ports(common);
>> +	if (ret)
>> +		dev_err(dev, "failed to resume slave ports: %d", ret);
>> +
>>  	am65_cpts_resume(common->cpts);
>>  
>>  	for (i = 0; i < common->port_num; i++) {
>> diff --git a/drivers/net/ethernet/ti/am65-cpts.c b/drivers/net/ethernet/ti/am65-cpts.c
>> index c66618d91c28..e6db5b61409a 100644
>> --- a/drivers/net/ethernet/ti/am65-cpts.c
>> +++ b/drivers/net/ethernet/ti/am65-cpts.c
>> @@ -1189,7 +1189,11 @@ void am65_cpts_suspend(struct am65_cpts *cpts)
>>  	cpts->sr_cpts_ns = am65_cpts_gettime(cpts, NULL);
>>  	cpts->sr_ktime_ns = ktime_to_ns(ktime_get_real());
>>  	am65_cpts_disable(cpts);
>> -	clk_disable(cpts->refclk);
>> +
>> +	/* During suspend the SoC can be power off, so let's not only
>> +	 * disable but also unprepare the clock
>> +	 */
>> +	clk_disable_unprepare(cpts->refclk);
>>  
>>  	/* Save GENF state */
>>  	memcpy_fromio(&cpts->sr_genf, &cpts->reg->genf, sizeof(cpts->sr_genf));
>> @@ -1204,8 +1208,11 @@ void am65_cpts_resume(struct am65_cpts *cpts)
>>  	int i;
>>  	s64 ktime_ns;
>>  
>> +	/* During suspend the SoC can be power off, so let's not only
>> +	 * enable but also prepare the clock
>> +	 */
>> +	clk_prepare_enable(cpts->refclk);
>>  	/* restore state and enable CPTS */
>> -	clk_enable(cpts->refclk);
>>  	am65_cpts_write32(cpts, cpts->sr_rftclk_sel, rftclk_sel);
>>  	am65_cpts_set_add_val(cpts);
>>  	am65_cpts_write32(cpts, cpts->sr_control, control);
>
> -- 
> cheers,
> -roger

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com