diff mbox series

[net-next,v2,09/21] net: ravb: Split GTI computation and set operations

Message ID 20231214114600.2451162-10-claudiu.beznea.uj@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series net: ravb: Add suspend to RAM and runtime PM support for RZ/G3S | expand

Commit Message

Claudiu Beznea Dec. 14, 2023, 11:45 a.m. UTC 
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

ravb_set_gti() was computing the value of GTI based on the reference clock
rate and then applied it to register. This was done on the driver's probe
function. In order to implement runtime PM for all IP variants (as some IP
variants switches to reset operation mode (and thus the register's content
is lost) when module standby is configured through clock APIs) the GTI was
split in 2 parts: one computing the value of the GTI register (done in the
driver's probe function) and one applying the computed value to register
(done in the driver's ndo_open API).

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---

Changes in v2:
- none; this patch is new

 drivers/net/ethernet/renesas/ravb.h      |   2 +
 drivers/net/ethernet/renesas/ravb_main.c | 110 ++++++++++++-----------
 2 files changed, 58 insertions(+), 54 deletions(-)

Comments

Sergey Shtylyov Dec. 16, 2023, 4:38 p.m. UTC | #1 
On 12/14/23 2:45 PM, Claudiu wrote:

> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> ravb_set_gti() was computing the value of GTI based on the reference clock
> rate and then applied it to register. This was done on the driver's probe
> function. In order to implement runtime PM for all IP variants (as some IP
> variants switches to reset operation mode (and thus the register's content

   Again, operating mode...

> is lost) when module standby is configured through clock APIs) the GTI was

   The GTI what? Setup?

> split in 2 parts: one computing the value of the GTI register (done in the
> driver's probe function) and one applying the computed value to register
> (done in the driver's ndo_open API).
> 
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
[...]

> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index e0f8276cffed..76202395b68d 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1106,6 +1106,8 @@ struct ravb_private {
>  
>  	const struct ravb_hw_info *info;
>  	struct reset_control *rstc;
> +
> +	uint64_t gti_tiv;

   Please use the kernel type, u64; uint64_t is for userland, IIRC.

[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index d7f6e8ea8e79..5e01e03e1b43 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>  	return error;
>  }
>  
> +static int ravb_set_gti(struct net_device *ndev)
> +{
[...]
> +	/* Request GTI loading */
> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
> +
> +	/* Check completion status. */
> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);

   Is this really necessary?

[...]
> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>  		goto out_napi_off;
>  	ravb_emac_init(ndev);
>  
> +	error = ravb_set_gti(ndev);
> +	if (error)
> +		goto out_dma_stop;
> +

   Hm... belongs in ravb_dmac_init() now, as it seems... 

[...]

MBR, Sergey
Claudiu Beznea Dec. 17, 2023, 12:40 p.m. UTC | #2 
On 16.12.2023 18:38, Sergey Shtylyov wrote:
> On 12/14/23 2:45 PM, Claudiu wrote:
> 
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> ravb_set_gti() was computing the value of GTI based on the reference clock
>> rate and then applied it to register. This was done on the driver's probe
>> function. In order to implement runtime PM for all IP variants (as some IP
>> variants switches to reset operation mode (and thus the register's content
> 
>    Again, operating mode...
> 
>> is lost) when module standby is configured through clock APIs) the GTI was
> 
>    The GTI what? Setup?
> 
>> split in 2 parts: one computing the value of the GTI register (done in the
>> driver's probe function) and one applying the computed value to register
>> (done in the driver's ndo_open API).
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> [...]
> 
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index e0f8276cffed..76202395b68d 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1106,6 +1106,8 @@ struct ravb_private {
>>  
>>  	const struct ravb_hw_info *info;
>>  	struct reset_control *rstc;
>> +
>> +	uint64_t gti_tiv;
> 
>    Please use the kernel type, u64; uint64_t is for userland, IIRC.

I just kept the initial type here. Anyway, uint64_t should translate to u64
AFAICT.

Looking at it again the field here is enough to be 32 bit as the register
field is no longer than that. It is needed on 64 bits when checking the
ranges in compute function.

> 
> [...]
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index d7f6e8ea8e79..5e01e03e1b43 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>>  	return error;
>>  }
>>  
>> +static int ravb_set_gti(struct net_device *ndev)
>> +{
> [...]
>> +	/* Request GTI loading */
>> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>> +
>> +	/* Check completion status. */
>> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
> 
>    Is this really necessary?

I've just updated it to respect the manual specifications. Please let me
know if you want me to drop it. For this series it should be harmless
keeping it as it was previously (I will double check it).

> 
> [...]
>> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>>  		goto out_napi_off;
>>  	ravb_emac_init(ndev);
>>  
>> +	error = ravb_set_gti(ndev);
>> +	if (error)
>> +		goto out_dma_stop;
>> +
> 
>    Hm... belongs in ravb_dmac_init() now, as it seems... 

Isn't it PTP specific?

> 
> [...]
> 
> MBR, Sergey
Sergey Shtylyov Dec. 19, 2023, 6:20 p.m. UTC | #3 
On 12/17/23 3:40 PM, claudiu beznea wrote:

[...]
>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>> ravb_set_gti() was computing the value of GTI based on the reference clock
>>> rate and then applied it to register. This was done on the driver's probe
>>> function. In order to implement runtime PM for all IP variants (as some IP
>>> variants switches to reset operation mode (and thus the register's content
>>
>>    Again, operating mode...
>>
>>> is lost) when module standby is configured through clock APIs) the GTI was
>>
>>    The GTI what? Setup?
>>
>>> split in 2 parts: one computing the value of the GTI register (done in the
>>> driver's probe function) and one applying the computed value to register
>>> (done in the driver's ndo_open API).
>>>
>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>> [...]
>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>>> index e0f8276cffed..76202395b68d 100644
>>> --- a/drivers/net/ethernet/renesas/ravb.h
>>> +++ b/drivers/net/ethernet/renesas/ravb.h
>>> @@ -1106,6 +1106,8 @@ struct ravb_private {
>>>  
>>>  	const struct ravb_hw_info *info;
>>>  	struct reset_control *rstc;
>>> +
>>> +	uint64_t gti_tiv;
>>
>>    Please use the kernel type, u64; uint64_t is for userland, IIRC.
> 
> I just kept the initial type here.

   Oops, that type slipped in while I wasn't yet a reviewer. :-/

> Anyway, uint64_t should translate to u64 AFAICT.

   Yes.

> Looking at it again the field here is enough to be 32 bit as the register
> field is no longer than that. It is needed on 64 bits when checking the
> ranges in compute function.

   Indeed. The actual GTI.TIV field is even 28-bit wide only...

[...]
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index d7f6e8ea8e79..5e01e03e1b43 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -1750,6 +1750,51 @@ static int ravb_set_reset_mode(struct net_device *ndev)
>>>  	return error;
>>>  }
>>>  
>>> +static int ravb_set_gti(struct net_device *ndev)
>>> +{
>> [...]
>>> +	/* Request GTI loading */
>>> +	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
>>> +
>>> +	/* Check completion status. */
>>> +	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
>>
>>    Is this really necessary?
> 
> I've just updated it to respect the manual specifications. Please let me
> know if you want me to drop it. For this series it should be harmless
> keeping it as it was previously (I will double check it).

   Looks like you'll have to frop the fix patch #2, so this ravb_wait()
call shouldn't be placed here as well...

>> [...]
>>> @@ -1767,6 +1812,10 @@ static int ravb_open(struct net_device *ndev)
>>>  		goto out_napi_off;
>>>  	ravb_emac_init(ndev);
>>>  
>>> +	error = ravb_set_gti(ndev);
>>> +	if (error)
>>> +		goto out_dma_stop;
>>> +
>>
>>    Hm... belongs in ravb_dmac_init() now, as it seems... 
> 
> Isn't it PTP specific?

   I just had an impression it belonged to the AVB-DMAC register range
but perhaps I'm wrong...

[...]

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index e0f8276cffed..76202395b68d 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1106,6 +1106,8 @@  struct ravb_private {
 
 	const struct ravb_hw_info *info;
 	struct reset_control *rstc;
+
+	uint64_t gti_tiv;
 };
 
 static inline u32 ravb_read(struct net_device *ndev, enum ravb_reg reg)
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index d7f6e8ea8e79..5e01e03e1b43 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -1750,6 +1750,51 @@  static int ravb_set_reset_mode(struct net_device *ndev)
 	return error;
 }
 
+static int ravb_set_gti(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+
+	if (!(info->gptp || info->ccc_gac))
+		return 0;
+
+	ravb_write(ndev, priv->gti_tiv, GTI);
+
+	/* Request GTI loading */
+	ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
+
+	/* Check completion status. */
+	return ravb_wait(ndev, GCCR, GCCR_LTI, 0);
+}
+
+static int ravb_compute_gti(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	const struct ravb_hw_info *info = priv->info;
+	struct device *dev = ndev->dev.parent;
+	unsigned long rate;
+
+	if (!(info->gptp || info->ccc_gac))
+		return 0;
+
+	if (info->gptp_ref_clk)
+		rate = clk_get_rate(priv->gptp_clk);
+	else
+		rate = clk_get_rate(priv->clk);
+	if (!rate)
+		return -EINVAL;
+
+	priv->gti_tiv = div64_ul(1000000000ULL << 20, rate);
+
+	if (priv->gti_tiv < GTI_TIV_MIN || priv->gti_tiv > GTI_TIV_MAX) {
+		dev_err(dev, "gti.tiv increment 0x%llx is outside the range 0x%x - 0x%x\n",
+			priv->gti_tiv, GTI_TIV_MIN, GTI_TIV_MAX);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /* Network device open function for Ethernet AVB */
 static int ravb_open(struct net_device *ndev)
 {
@@ -1767,6 +1812,10 @@  static int ravb_open(struct net_device *ndev)
 		goto out_napi_off;
 	ravb_emac_init(ndev);
 
+	error = ravb_set_gti(ndev);
+	if (error)
+		goto out_dma_stop;
+
 	/* Initialise PTP Clock driver */
 	if (info->gptp)
 		ravb_ptp_init(ndev, priv->pdev);
@@ -1784,6 +1833,7 @@  static int ravb_open(struct net_device *ndev)
 	/* Stop PTP Clock driver */
 	if (info->gptp)
 		ravb_ptp_stop(ndev);
+out_dma_stop:
 	ravb_stop_dma(ndev);
 out_napi_off:
 	if (info->nc_queues)
@@ -2449,34 +2499,6 @@  static const struct of_device_id ravb_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, ravb_match_table);
 
-static int ravb_set_gti(struct net_device *ndev)
-{
-	struct ravb_private *priv = netdev_priv(ndev);
-	const struct ravb_hw_info *info = priv->info;
-	struct device *dev = ndev->dev.parent;
-	unsigned long rate;
-	uint64_t inc;
-
-	if (info->gptp_ref_clk)
-		rate = clk_get_rate(priv->gptp_clk);
-	else
-		rate = clk_get_rate(priv->clk);
-	if (!rate)
-		return -EINVAL;
-
-	inc = div64_ul(1000000000ULL << 20, rate);
-
-	if (inc < GTI_TIV_MIN || inc > GTI_TIV_MAX) {
-		dev_err(dev, "gti.tiv increment 0x%llx is outside the range 0x%x - 0x%x\n",
-			inc, GTI_TIV_MIN, GTI_TIV_MAX);
-		return -EINVAL;
-	}
-
-	ravb_write(ndev, inc, GTI);
-
-	return 0;
-}
-
 static int ravb_set_config_mode(struct net_device *ndev)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
@@ -2792,19 +2814,9 @@  static int ravb_probe(struct platform_device *pdev)
 	if (error)
 		goto out_rpm_put;
 
-	if (info->gptp || info->ccc_gac) {
-		/* Set GTI value */
-		error = ravb_set_gti(ndev);
-		if (error)
-			goto out_rpm_put;
-
-		/* Request GTI loading */
-		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-		/* Check completion status. */
-		error = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
-		if (error)
-			goto out_rpm_put;
-	}
+	error = ravb_compute_gti(ndev);
+	if (error)
+		goto out_rpm_put;
 
 	if (info->internal_delay) {
 		ravb_parse_delay_mode(np, ndev);
@@ -3020,19 +3032,9 @@  static int ravb_resume(struct device *dev)
 	if (ret)
 		return ret;
 
-	if (info->gptp || info->ccc_gac) {
-		/* Set GTI value */
-		ret = ravb_set_gti(ndev);
-		if (ret)
-			return ret;
-
-		/* Request GTI loading */
-		ravb_modify(ndev, GCCR, GCCR_LTI, GCCR_LTI);
-		/* Check completion status. */
-		ret = ravb_wait(ndev, GCCR, GCCR_LTI, 0);
-		if (ret)
-			return ret;
-	}
+	ret = ravb_set_gti(ndev);
+	if (ret)
+		return ret;
 
 	if (info->internal_delay)
 		ravb_set_delay_mode(ndev);