diff mbox series

[repost] thermal: rcar_thermal: update calculation formula for E3

Message ID 20190418071514.13027-1-horms+renesas@verge.net.au (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series [repost] thermal: rcar_thermal: update calculation formula for E3 | expand

Commit Message

Simon Horman April 18, 2019, 7:15 a.m. UTC 
From: Yoshihiro Kaneko <ykaneko0929@gmail.com>

HW manual changes temperature calculation formula for E3:
- When CTEMP is less than 24
   T = CTEMP[5:0] * 5.5 - 72
- When CTEMP is equal to/greater than 24
   T = CTEMP[5:0] * 5 - 60

This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>

Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
Tested-by: Simon Horman <horms+renesas@verge.net.au>
Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/thermal/rcar_thermal.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Niklas Söderlund April 18, 2019, 8:12 a.m. UTC | #1 
Hi Kaneko-san,

On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> HW manual changes temperature calculation formula for E3:

Is this not also true for V3M and D3?

> - When CTEMP is less than 24
>    T = CTEMP[5:0] * 5.5 - 72
> - When CTEMP is equal to/greater than 24
>    T = CTEMP[5:0] * 5 - 60
> 
> This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> 
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> Tested-by: Simon Horman <horms+renesas@verge.net.au>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>  drivers/thermal/rcar_thermal.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 97462e9b40d8..11df0cc63bed 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
> @@ -52,6 +52,7 @@ struct rcar_thermal_chip {
>  	unsigned int irq_per_ch : 1;
>  	unsigned int needs_suspend_resume : 1;
>  	unsigned int nirqs;
> +	unsigned int ctemp_bands;

Would it be possible to rename this to something indicating that this is 
a gen3 thing? Maybe move it to the bit fields above and name it gen3 ?

>  };
>  
>  static const struct rcar_thermal_chip rcar_thermal = {
> @@ -60,6 +61,7 @@ static const struct rcar_thermal_chip rcar_thermal = {
>  	.irq_per_ch = 0,
>  	.needs_suspend_resume = 0,
>  	.nirqs = 1,
> +	.ctemp_bands = 1,
>  };
>  
>  static const struct rcar_thermal_chip rcar_gen2_thermal = {
> @@ -68,6 +70,7 @@ static const struct rcar_thermal_chip rcar_gen2_thermal = {
>  	.irq_per_ch = 0,
>  	.needs_suspend_resume = 0,
>  	.nirqs = 1,
> +	.ctemp_bands = 1,
>  };
>  
>  static const struct rcar_thermal_chip rcar_gen3_thermal = {
> @@ -80,6 +83,7 @@ static const struct rcar_thermal_chip rcar_gen3_thermal = {
>  	 * interrupts to detect a temperature change, rise or fall.
>  	 */
>  	.nirqs = 2,
> +	.ctemp_bands = 2,
>  };
>  
>  struct rcar_thermal_priv {
> @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
>  		return ret;
>  
>  	mutex_lock(&priv->lock);
> -	tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> +	if (priv->chip->ctemp_bands == 1)
> +		tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> +	else if (priv->ctemp < 24)
> +		tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> +	else
> +		tmp = MCELSIUS((priv->ctemp * 5) - 60);

I confirm that the calculations here are correct, but hard to read ;-) 
With the rename about how about.

    if (priv->chip->gen3) {
        if (priv->ctemp < 24)
                tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
        else
                tmp = MCELSIUS((priv->ctemp * 5) - 60);
    } else {
            tmp =  MCELSIUS((priv->ctemp * 5) - 65);
    }

>  	mutex_unlock(&priv->lock);
>  
>  	if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) {
> -- 
> 2.11.0
>
Sergei Shtylyov April 18, 2019, 8:17 a.m. UTC | #2 
On 18.04.2019 10:15, Simon Horman wrote:

> From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> 
> HW manual changes temperature calculation formula for E3:
> - When CTEMP is less than 24
>     T = CTEMP[5:0] * 5.5 - 72
> - When CTEMP is equal to/greater than 24
>     T = CTEMP[5:0] * 5 - 60
> 
> This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> 
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> Tested-by: Simon Horman <horms+renesas@verge.net.au>
> Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
>   drivers/thermal/rcar_thermal.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> index 97462e9b40d8..11df0cc63bed 100644
> --- a/drivers/thermal/rcar_thermal.c
> +++ b/drivers/thermal/rcar_thermal.c
[...]
> @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
>   		return ret;
>   
>   	mutex_lock(&priv->lock);
> -	tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> +	if (priv->chip->ctemp_bands == 1)
> +		tmp =  MCELSIUS((priv->ctemp * 5) - 65);
                      ^ stray space?

> +	else if (priv->ctemp < 24)
> +		tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> +	else
> +		tmp = MCELSIUS((priv->ctemp * 5) - 60);
>   	mutex_unlock(&priv->lock);
>   
>   	if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) {

MBR, Sergei
Geert Uytterhoeven April 24, 2019, 7:10 a.m. UTC | #3 
Hi Niklas,

On Thu, Apr 18, 2019 at 10:12 AM Niklas Söderlund
<niklas.soderlund@ragnatech.se> wrote:
> On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> > From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> >
> > HW manual changes temperature calculation formula for E3:
>
> Is this not also true for V3M and D3?
>
> > - When CTEMP is less than 24
> >    T = CTEMP[5:0] * 5.5 - 72
> > - When CTEMP is equal to/greater than 24
> >    T = CTEMP[5:0] * 5 - 60
> >
> > This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> >
> > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > Tested-by: Simon Horman <horms+renesas@verge.net.au>
> > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > ---
> >  drivers/thermal/rcar_thermal.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > index 97462e9b40d8..11df0cc63bed 100644
> > --- a/drivers/thermal/rcar_thermal.c
> > +++ b/drivers/thermal/rcar_thermal.c
> > @@ -52,6 +52,7 @@ struct rcar_thermal_chip {
> >       unsigned int irq_per_ch : 1;
> >       unsigned int needs_suspend_resume : 1;
> >       unsigned int nirqs;
> > +     unsigned int ctemp_bands;
>
> Would it be possible to rename this to something indicating that this is
> a gen3 thing? Maybe move it to the bit fields above and name it gen3 ?

Is that really a good thing to do? This structure describes features of
the thermal module, and we're already beyond the point where a simple
check  for gen2 or gen3 was sufficient.
Here the feature is having multiple temperature bands.
What if some other Gen3 SoC starts having 3 temperature bands?

> > @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
> >               return ret;
> >
> >       mutex_lock(&priv->lock);
> > -     tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > +     if (priv->chip->ctemp_bands == 1)
> > +             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > +     else if (priv->ctemp < 24)
> > +             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> > +     else
> > +             tmp = MCELSIUS((priv->ctemp * 5) - 60);
>
> I confirm that the calculations here are correct, but hard to read ;-)
> With the rename about how about.
>
>     if (priv->chip->gen3) {
>         if (priv->ctemp < 24)
>                 tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
>         else
>                 tmp = MCELSIUS((priv->ctemp * 5) - 60);
>     } else {
>             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
>     }

_Iff_ we decide on going for the rename, I'd still write it as:

    if (!priv->chip->gen3)
            tmp =  MCELSIUS((priv->ctemp * 5) - 65);
    else if (priv->ctemp < 24)
            tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
    else
            tmp = MCELSIUS((priv->ctemp * 5) - 60);

Always fold your if/else if/else constructs to minimize the need for indentation
and braces ;-)

Gr{oetje,eeting}s,

                        Geert
Simon Horman April 26, 2019, 9:55 a.m. UTC | #4 
On Wed, Apr 24, 2019 at 09:10:56AM +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Thu, Apr 18, 2019 at 10:12 AM Niklas Söderlund
> <niklas.soderlund@ragnatech.se> wrote:
> > On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> > > From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > >
> > > HW manual changes temperature calculation formula for E3:
> >
> > Is this not also true for V3M and D3?
> >
> > > - When CTEMP is less than 24
> > >    T = CTEMP[5:0] * 5.5 - 72
> > > - When CTEMP is equal to/greater than 24
> > >    T = CTEMP[5:0] * 5 - 60
> > >
> > > This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> > >
> > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > Tested-by: Simon Horman <horms+renesas@verge.net.au>
> > > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > > ---
> > >  drivers/thermal/rcar_thermal.c | 11 ++++++++++-
> > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > > index 97462e9b40d8..11df0cc63bed 100644
> > > --- a/drivers/thermal/rcar_thermal.c
> > > +++ b/drivers/thermal/rcar_thermal.c
> > > @@ -52,6 +52,7 @@ struct rcar_thermal_chip {
> > >       unsigned int irq_per_ch : 1;
> > >       unsigned int needs_suspend_resume : 1;
> > >       unsigned int nirqs;
> > > +     unsigned int ctemp_bands;
> >
> > Would it be possible to rename this to something indicating that this is
> > a gen3 thing? Maybe move it to the bit fields above and name it gen3 ?
> 
> Is that really a good thing to do? This structure describes features of
> the thermal module, and we're already beyond the point where a simple
> check  for gen2 or gen3 was sufficient.
> Here the feature is having multiple temperature bands.
> What if some other Gen3 SoC starts having 3 temperature bands?
> 
> > > @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
> > >               return ret;
> > >
> > >       mutex_lock(&priv->lock);
> > > -     tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > > +     if (priv->chip->ctemp_bands == 1)
> > > +             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > > +     else if (priv->ctemp < 24)
> > > +             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> > > +     else
> > > +             tmp = MCELSIUS((priv->ctemp * 5) - 60);
> >
> > I confirm that the calculations here are correct, but hard to read ;-)
> > With the rename about how about.
> >
> >     if (priv->chip->gen3) {
> >         if (priv->ctemp < 24)
> >                 tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> >         else
> >                 tmp = MCELSIUS((priv->ctemp * 5) - 60);
> >     } else {
> >             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> >     }
> 
> _Iff_ we decide on going for the rename, I'd still write it as:
> 
>     if (!priv->chip->gen3)
>             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
>     else if (priv->ctemp < 24)
>             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
>     else
>             tmp = MCELSIUS((priv->ctemp * 5) - 60);
> 
> Always fold your if/else if/else constructs to minimize the need for indentation
> and braces ;-)

From my PoV I think the patch is fine in its current form.
Niklas do you feel particularly strongly about changing it?
Niklas Söderlund May 7, 2019, 10:14 p.m. UTC | #5 
Hello,

On 2019-04-26 11:55:11 +0200, Simon Horman wrote:
> On Wed, Apr 24, 2019 at 09:10:56AM +0200, Geert Uytterhoeven wrote:
> > Hi Niklas,
> > 
> > On Thu, Apr 18, 2019 at 10:12 AM Niklas Söderlund
> > <niklas.soderlund@ragnatech.se> wrote:
> > > On 2019-04-18 09:15:14 +0200, Simon Horman wrote:
> > > > From: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > >
> > > > HW manual changes temperature calculation formula for E3:
> > >
> > > Is this not also true for V3M and D3?
> > >
> > > > - When CTEMP is less than 24
> > > >    T = CTEMP[5:0] * 5.5 - 72
> > > > - When CTEMP is equal to/greater than 24
> > > >    T = CTEMP[5:0] * 5 - 60
> > > >
> > > > This was inspired by a patch in the BSP by Van Do <van.do.xw@renesas.com>
> > > >
> > > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
> > > > Tested-by: Simon Horman <horms+renesas@verge.net.au>
> > > > Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> > > > Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> > > > ---
> > > >  drivers/thermal/rcar_thermal.c | 11 ++++++++++-
> > > >  1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
> > > > index 97462e9b40d8..11df0cc63bed 100644
> > > > --- a/drivers/thermal/rcar_thermal.c
> > > > +++ b/drivers/thermal/rcar_thermal.c
> > > > @@ -52,6 +52,7 @@ struct rcar_thermal_chip {
> > > >       unsigned int irq_per_ch : 1;
> > > >       unsigned int needs_suspend_resume : 1;
> > > >       unsigned int nirqs;
> > > > +     unsigned int ctemp_bands;
> > >
> > > Would it be possible to rename this to something indicating that this is
> > > a gen3 thing? Maybe move it to the bit fields above and name it gen3 ?
> > 
> > Is that really a good thing to do? This structure describes features of
> > the thermal module, and we're already beyond the point where a simple
> > check  for gen2 or gen3 was sufficient.
> > Here the feature is having multiple temperature bands.
> > What if some other Gen3 SoC starts having 3 temperature bands?

Good point, after reviewing your comment I now agree with the original 
patch.

> > 
> > > > @@ -263,7 +267,12 @@ static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
> > > >               return ret;
> > > >
> > > >       mutex_lock(&priv->lock);
> > > > -     tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > > > +     if (priv->chip->ctemp_bands == 1)
> > > > +             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > > > +     else if (priv->ctemp < 24)
> > > > +             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> > > > +     else
> > > > +             tmp = MCELSIUS((priv->ctemp * 5) - 60);
> > >
> > > I confirm that the calculations here are correct, but hard to read ;-)
> > > With the rename about how about.
> > >
> > >     if (priv->chip->gen3) {
> > >         if (priv->ctemp < 24)
> > >                 tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> > >         else
> > >                 tmp = MCELSIUS((priv->ctemp * 5) - 60);
> > >     } else {
> > >             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> > >     }
> > 
> > _Iff_ we decide on going for the rename, I'd still write it as:
> > 
> >     if (!priv->chip->gen3)
> >             tmp =  MCELSIUS((priv->ctemp * 5) - 65);
> >     else if (priv->ctemp < 24)
> >             tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
> >     else
> >             tmp = MCELSIUS((priv->ctemp * 5) - 60);
> > 
> > Always fold your if/else if/else constructs to minimize the need for indentation
> > and braces ;-)
> 
> From my PoV I think the patch is fine in its current form.
> Niklas do you feel particularly strongly about changing it?

No I do not feel strongly about this and would be fine with the patch in 
it's current form. With and update commit message to mention V3M and D3 
feel free to add

Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Simon Horman May 8, 2019, 11:09 a.m. UTC | #6 
On Wed, May 08, 2019 at 12:14:46AM +0200, Niklas Söderlund wrote:

...

> > From my PoV I think the patch is fine in its current form.
> > Niklas do you feel particularly strongly about changing it?
> 
> No I do not feel strongly about this and would be fine with the patch in 
> it's current form. With and update commit message to mention V3M and D3 
> feel free to add
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks Niklas,

I have gone ahead and posted
[PATCH v2] thermal: rcar_thermal: update calculation formula for R-Car Gen3 SoCs
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 97462e9b40d8..11df0cc63bed 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -52,6 +52,7 @@  struct rcar_thermal_chip {
 	unsigned int irq_per_ch : 1;
 	unsigned int needs_suspend_resume : 1;
 	unsigned int nirqs;
+	unsigned int ctemp_bands;
 };
 
 static const struct rcar_thermal_chip rcar_thermal = {
@@ -60,6 +61,7 @@  static const struct rcar_thermal_chip rcar_thermal = {
 	.irq_per_ch = 0,
 	.needs_suspend_resume = 0,
 	.nirqs = 1,
+	.ctemp_bands = 1,
 };
 
 static const struct rcar_thermal_chip rcar_gen2_thermal = {
@@ -68,6 +70,7 @@  static const struct rcar_thermal_chip rcar_gen2_thermal = {
 	.irq_per_ch = 0,
 	.needs_suspend_resume = 0,
 	.nirqs = 1,
+	.ctemp_bands = 1,
 };
 
 static const struct rcar_thermal_chip rcar_gen3_thermal = {
@@ -80,6 +83,7 @@  static const struct rcar_thermal_chip rcar_gen3_thermal = {
 	 * interrupts to detect a temperature change, rise or fall.
 	 */
 	.nirqs = 2,
+	.ctemp_bands = 2,
 };
 
 struct rcar_thermal_priv {
@@ -263,7 +267,12 @@  static int rcar_thermal_get_current_temp(struct rcar_thermal_priv *priv,
 		return ret;
 
 	mutex_lock(&priv->lock);
-	tmp =  MCELSIUS((priv->ctemp * 5) - 65);
+	if (priv->chip->ctemp_bands == 1)
+		tmp =  MCELSIUS((priv->ctemp * 5) - 65);
+	else if (priv->ctemp < 24)
+		tmp = MCELSIUS(((priv->ctemp * 55) - 720) / 10);
+	else
+		tmp = MCELSIUS((priv->ctemp * 5) - 60);
 	mutex_unlock(&priv->lock);
 
 	if ((tmp < MCELSIUS(-45)) || (tmp > MCELSIUS(125))) {