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

Message-ID: <e37e8d74-d741-44fb-9e28-2b9203331637@kernel.org>
Date: Wed, 29 Nov 2023 14:38:49 +0200
From: Roger Quadros <rogerq@...nel.org>
To: 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, gregory.clement@...tlin.com, u-kumar1@...com
Subject: Re: [PATCH] net: ethernet: ti: am65-cpsw: improve suspend/resume
 support for J7200

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.

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

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?

> completely power down, and phy_set_mode_ext must be called again.

Why must phy_set_mode_ext() be called again?

> 

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

> 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