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

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

[Thomas Richard]

From: Gregory CLEMENT <gregory.clement@bootlin.com>

On J7200 the SoC is off during suspend, so the clocks have to be
completely power down, and phy_set_mode_ext must be called again.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.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);
-- 
2.39.2

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

[Roger Quadros]

Hi,

On 28/11/2023 15:19, Thomas Richard wrote:
> From: Gregory CLEMENT <gregory.clement@bootlin.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@bootlin.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.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

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

[Dan Carpenter]

On Wed, Nov 29, 2023 at 02:38:49PM +0200, Roger Quadros wrote:
> Hi,
> 
> On 28/11/2023 15:19, Thomas Richard wrote:
> > From: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Subject is vague. Please be explicit about you are trying to do.
> 

I'm glad someone else said this.  I wrote a similar email but never sent
it.

It's not clear from reading the commit message what this looks like to
the user.  Is the network slow or does it stop working altogether or
what?  Is there an error message printed in dmesg or something?

I feel like if I were more of a domain expert I would understand the
impact better perhaps?

regards,
dan carpenter

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

[Gregory CLEMENT]

Hi Roger,

> Hi,
>
> On 28/11/2023 15:19, Thomas Richard wrote:
>> From: Gregory CLEMENT <gregory.clement@bootlin.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@bootlin.com>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.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