[1/6] thermal: brcmstb_thermal: Do not use DT coefficients
diff mbox series

Message ID 20191030182132.25763-2-f.fainelli@gmail.com
State New
Delegated to: Eduardo Valentin
Headers show
Series
  • brcmstb_thermal updates for new processes
Related show

Commit Message

Florian Fainelli Oct. 30, 2019, 6:21 p.m. UTC
At the time the brcmstb_thermal driver and its binding were merged, the
DT binding did not make the coefficients properties a mandatory one,
therefore all users of the brcmstb_thermal driver out there have a non
functional implementation with zero coefficients. Even if these
properties were provided, the formula used for computation is incorrect.

The coefficients are entirely process specific (right now, only 28nm is
supported) and not board or SoC specific, it is therefore appropriate to
hard code them in the driver given the compatibility string we are
probed with which has to be updated whenever a new process is
introduced.

We remove the existing coefficients definition since subsequent patches
are going to add support for a new process and will introduce new
coefficients as well.

Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
 1 file changed, 6 insertions(+), 31 deletions(-)

Comments

Amit Kucheria Dec. 4, 2019, 8:41 a.m. UTC | #1
On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> At the time the brcmstb_thermal driver and its binding were merged, the
> DT binding did not make the coefficients properties a mandatory one,
> therefore all users of the brcmstb_thermal driver out there have a non
> functional implementation with zero coefficients. Even if these
> properties were provided, the formula used for computation is incorrect.
>
> The coefficients are entirely process specific (right now, only 28nm is
> supported) and not board or SoC specific, it is therefore appropriate to
> hard code them in the driver given the compatibility string we are
> probed with which has to be updated whenever a new process is
> introduced.
>
> We remove the existing coefficients definition since subsequent patches
> are going to add support for a new process and will introduce new
> coefficients as well.
>
> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")

I think you should fix the computation formula as the first patch and
then merge the rest of this patch into your second patch.

I don't think the intermediate state of converting named constants to
magic numbers is needed just to convert it over to another set of
parameters.

Regards,
Amit

> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
>  1 file changed, 6 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> index 5825ac581f56..42482af0422e 100644
> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> @@ -48,15 +48,6 @@
>  #define AVS_TMON_TEMP_INT_CODE         0x1c
>  #define AVS_TMON_TP_TEST_ENABLE                0x20
>
> -/* Default coefficients */
> -#define AVS_TMON_TEMP_SLOPE            -487
> -#define AVS_TMON_TEMP_OFFSET           410040
> -
> -/* HW related temperature constants */
> -#define AVS_TMON_TEMP_MAX              0x3ff
> -#define AVS_TMON_TEMP_MIN              -88161
> -#define AVS_TMON_TEMP_MASK             AVS_TMON_TEMP_MAX
> -
>  enum avs_tmon_trip_type {
>         TMON_TRIP_TYPE_LOW = 0,
>         TMON_TRIP_TYPE_HIGH,
> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
>         struct thermal_zone_device *thermal;
>  };
>
> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
> -                               int *offset)
> -{
> -       *slope = thermal_zone_get_slope(tz);
> -       *offset = thermal_zone_get_offset(tz);
> -}
> -
>  /* Convert a HW code to a temperature reading (millidegree celsius) */
>  static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>                                         u32 code)
>  {
> -       const int val = code & AVS_TMON_TEMP_MASK;
> -       int slope, offset;
> -
> -       avs_tmon_get_coeffs(tz, &slope, &offset);
> -
> -       return slope * val + offset;
> +       return (410040 - (int)((code & 0x3FF) * 487));
>  }
>
>  /*
> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>  static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
>                                         int temp, bool low)
>  {
> -       int slope, offset;
> -
> -       if (temp < AVS_TMON_TEMP_MIN)
> -               return AVS_TMON_TEMP_MAX; /* Maximum code value */
> -
> -       avs_tmon_get_coeffs(tz, &slope, &offset);
> +       if (temp < -88161)
> +               return 0x3FF;   /* Maximum code value */
>
> -       if (temp >= offset)
> +       if (temp >= 410040)
>                 return 0;       /* Minimum code value */
>
>         if (low)
> -               return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
> +               return (u32)(DIV_ROUND_UP(410040 - temp, 487));
>         else
> -               return (u32)((offset - temp) / abs(slope));
> +               return (u32)((410040 - temp) / 487);
>  }
>
>  static int brcmstb_get_temp(void *data, int *temp)
> --
> 2.17.1
>
Florian Fainelli Dec. 4, 2019, 5:41 p.m. UTC | #2
On 12/4/19 12:41 AM, Amit Kucheria wrote:
> On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> At the time the brcmstb_thermal driver and its binding were merged, the
>> DT binding did not make the coefficients properties a mandatory one,
>> therefore all users of the brcmstb_thermal driver out there have a non
>> functional implementation with zero coefficients. Even if these
>> properties were provided, the formula used for computation is incorrect.
>>
>> The coefficients are entirely process specific (right now, only 28nm is
>> supported) and not board or SoC specific, it is therefore appropriate to
>> hard code them in the driver given the compatibility string we are
>> probed with which has to be updated whenever a new process is
>> introduced.
>>
>> We remove the existing coefficients definition since subsequent patches
>> are going to add support for a new process and will introduce new
>> coefficients as well.
>>
>> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
> 
> I think you should fix the computation formula as the first patch and
> then merge the rest of this patch into your second patch.
> 
> I don't think the intermediate state of converting named constants to
> magic numbers is needed just to convert it over to another set of
> parameters.

ok, so you would rather see this patch using the defined constants while
fixing the formula, and in a subsequent patch getting rid of the
constant names since they are going to be added to a per-process table,
does that sound reasonable?

> 
> Regards,
> Amit
> 
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
>>  1 file changed, 6 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
>> index 5825ac581f56..42482af0422e 100644
>> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
>> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
>> @@ -48,15 +48,6 @@
>>  #define AVS_TMON_TEMP_INT_CODE         0x1c
>>  #define AVS_TMON_TP_TEST_ENABLE                0x20
>>
>> -/* Default coefficients */
>> -#define AVS_TMON_TEMP_SLOPE            -487
>> -#define AVS_TMON_TEMP_OFFSET           410040
>> -
>> -/* HW related temperature constants */
>> -#define AVS_TMON_TEMP_MAX              0x3ff
>> -#define AVS_TMON_TEMP_MIN              -88161
>> -#define AVS_TMON_TEMP_MASK             AVS_TMON_TEMP_MAX
>> -
>>  enum avs_tmon_trip_type {
>>         TMON_TRIP_TYPE_LOW = 0,
>>         TMON_TRIP_TYPE_HIGH,
>> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
>>         struct thermal_zone_device *thermal;
>>  };
>>
>> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
>> -                               int *offset)
>> -{
>> -       *slope = thermal_zone_get_slope(tz);
>> -       *offset = thermal_zone_get_offset(tz);
>> -}
>> -
>>  /* Convert a HW code to a temperature reading (millidegree celsius) */
>>  static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>>                                         u32 code)
>>  {
>> -       const int val = code & AVS_TMON_TEMP_MASK;
>> -       int slope, offset;
>> -
>> -       avs_tmon_get_coeffs(tz, &slope, &offset);
>> -
>> -       return slope * val + offset;
>> +       return (410040 - (int)((code & 0x3FF) * 487));
>>  }
>>
>>  /*
>> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
>>  static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
>>                                         int temp, bool low)
>>  {
>> -       int slope, offset;
>> -
>> -       if (temp < AVS_TMON_TEMP_MIN)
>> -               return AVS_TMON_TEMP_MAX; /* Maximum code value */
>> -
>> -       avs_tmon_get_coeffs(tz, &slope, &offset);
>> +       if (temp < -88161)
>> +               return 0x3FF;   /* Maximum code value */
>>
>> -       if (temp >= offset)
>> +       if (temp >= 410040)
>>                 return 0;       /* Minimum code value */
>>
>>         if (low)
>> -               return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
>> +               return (u32)(DIV_ROUND_UP(410040 - temp, 487));
>>         else
>> -               return (u32)((offset - temp) / abs(slope));
>> +               return (u32)((410040 - temp) / 487);
>>  }
>>
>>  static int brcmstb_get_temp(void *data, int *temp)
>> --
>> 2.17.1
>>
Amit Kucheria Dec. 4, 2019, 7:01 p.m. UTC | #3
On Wed, Dec 4, 2019 at 11:12 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> On 12/4/19 12:41 AM, Amit Kucheria wrote:
> > On Wed, Oct 30, 2019 at 11:51 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> >>
> >> At the time the brcmstb_thermal driver and its binding were merged, the
> >> DT binding did not make the coefficients properties a mandatory one,
> >> therefore all users of the brcmstb_thermal driver out there have a non
> >> functional implementation with zero coefficients. Even if these
> >> properties were provided, the formula used for computation is incorrect.
> >>
> >> The coefficients are entirely process specific (right now, only 28nm is
> >> supported) and not board or SoC specific, it is therefore appropriate to
> >> hard code them in the driver given the compatibility string we are
> >> probed with which has to be updated whenever a new process is
> >> introduced.
> >>
> >> We remove the existing coefficients definition since subsequent patches
> >> are going to add support for a new process and will introduce new
> >> coefficients as well.
> >>
> >> Fixes: 9e03cf1b2dd5 ("thermal: add brcmstb AVS TMON driver")
> >
> > I think you should fix the computation formula as the first patch and
> > then merge the rest of this patch into your second patch.
> >
> > I don't think the intermediate state of converting named constants to
> > magic numbers is needed just to convert it over to another set of
> > parameters.
>
> ok, so you would rather see this patch using the defined constants while
> fixing the formula, and in a subsequent patch getting rid of the
> constant names since they are going to be added to a per-process table,
> does that sound reasonable?

Yes, that way the first patch would fix a bug (and with a Fixes tag
potentially gets backported to stable), the second would convert over
to the per-process table.

Regards,
Amit

> >
> > Regards,
> > Amit
> >
> >> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> >> ---
> >>  drivers/thermal/broadcom/brcmstb_thermal.c | 37 ++++------------------
> >>  1 file changed, 6 insertions(+), 31 deletions(-)
> >>
> >> diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
> >> index 5825ac581f56..42482af0422e 100644
> >> --- a/drivers/thermal/broadcom/brcmstb_thermal.c
> >> +++ b/drivers/thermal/broadcom/brcmstb_thermal.c
> >> @@ -48,15 +48,6 @@
> >>  #define AVS_TMON_TEMP_INT_CODE         0x1c
> >>  #define AVS_TMON_TP_TEST_ENABLE                0x20
> >>
> >> -/* Default coefficients */
> >> -#define AVS_TMON_TEMP_SLOPE            -487
> >> -#define AVS_TMON_TEMP_OFFSET           410040
> >> -
> >> -/* HW related temperature constants */
> >> -#define AVS_TMON_TEMP_MAX              0x3ff
> >> -#define AVS_TMON_TEMP_MIN              -88161
> >> -#define AVS_TMON_TEMP_MASK             AVS_TMON_TEMP_MAX
> >> -
> >>  enum avs_tmon_trip_type {
> >>         TMON_TRIP_TYPE_LOW = 0,
> >>         TMON_TRIP_TYPE_HIGH,
> >> @@ -108,23 +99,11 @@ struct brcmstb_thermal_priv {
> >>         struct thermal_zone_device *thermal;
> >>  };
> >>
> >> -static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
> >> -                               int *offset)
> >> -{
> >> -       *slope = thermal_zone_get_slope(tz);
> >> -       *offset = thermal_zone_get_offset(tz);
> >> -}
> >> -
> >>  /* Convert a HW code to a temperature reading (millidegree celsius) */
> >>  static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> >>                                         u32 code)
> >>  {
> >> -       const int val = code & AVS_TMON_TEMP_MASK;
> >> -       int slope, offset;
> >> -
> >> -       avs_tmon_get_coeffs(tz, &slope, &offset);
> >> -
> >> -       return slope * val + offset;
> >> +       return (410040 - (int)((code & 0x3FF) * 487));
> >>  }
> >>l
> >>  /*
> >> @@ -136,20 +115,16 @@ static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
> >>  static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
> >>                                         int temp, bool low)
> >>  {
> >> -       int slope, offset;
> >> -
> >> -       if (temp < AVS_TMON_TEMP_MIN)
> >> -               return AVS_TMON_TEMP_MAX; /* Maximum code value */
> >> -
> >> -       avs_tmon_get_coeffs(tz, &slope, &offset);
> >> +       if (temp < -88161)
> >> +               return 0x3FF;   /* Maximum code value */
> >>
> >> -       if (temp >= offset)
> >> +       if (temp >= 410040)
> >>                 return 0;       /* Minimum code value */
> >>
> >>         if (low)
> >> -               return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
> >> +               return (u32)(DIV_ROUND_UP(410040 - temp, 487));
> >>         else
> >> -               return (u32)((offset - temp) / abs(slope));
> >> +               return (u32)((410040 - temp) / 487);
> >>  }
> >>
> >>  static int brcmstb_get_temp(void *data, int *temp)
> >> --
> >> 2.17.1
> >>
>
>
> --
> Florian

Patch
diff mbox series

diff --git a/drivers/thermal/broadcom/brcmstb_thermal.c b/drivers/thermal/broadcom/brcmstb_thermal.c
index 5825ac581f56..42482af0422e 100644
--- a/drivers/thermal/broadcom/brcmstb_thermal.c
+++ b/drivers/thermal/broadcom/brcmstb_thermal.c
@@ -48,15 +48,6 @@ 
 #define AVS_TMON_TEMP_INT_CODE		0x1c
 #define AVS_TMON_TP_TEST_ENABLE		0x20
 
-/* Default coefficients */
-#define AVS_TMON_TEMP_SLOPE		-487
-#define AVS_TMON_TEMP_OFFSET		410040
-
-/* HW related temperature constants */
-#define AVS_TMON_TEMP_MAX		0x3ff
-#define AVS_TMON_TEMP_MIN		-88161
-#define AVS_TMON_TEMP_MASK		AVS_TMON_TEMP_MAX
-
 enum avs_tmon_trip_type {
 	TMON_TRIP_TYPE_LOW = 0,
 	TMON_TRIP_TYPE_HIGH,
@@ -108,23 +99,11 @@  struct brcmstb_thermal_priv {
 	struct thermal_zone_device *thermal;
 };
 
-static void avs_tmon_get_coeffs(struct thermal_zone_device *tz, int *slope,
-				int *offset)
-{
-	*slope = thermal_zone_get_slope(tz);
-	*offset = thermal_zone_get_offset(tz);
-}
-
 /* Convert a HW code to a temperature reading (millidegree celsius) */
 static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
 					u32 code)
 {
-	const int val = code & AVS_TMON_TEMP_MASK;
-	int slope, offset;
-
-	avs_tmon_get_coeffs(tz, &slope, &offset);
-
-	return slope * val + offset;
+	return (410040 - (int)((code & 0x3FF) * 487));
 }
 
 /*
@@ -136,20 +115,16 @@  static inline int avs_tmon_code_to_temp(struct thermal_zone_device *tz,
 static inline u32 avs_tmon_temp_to_code(struct thermal_zone_device *tz,
 					int temp, bool low)
 {
-	int slope, offset;
-
-	if (temp < AVS_TMON_TEMP_MIN)
-		return AVS_TMON_TEMP_MAX; /* Maximum code value */
-
-	avs_tmon_get_coeffs(tz, &slope, &offset);
+	if (temp < -88161)
+		return 0x3FF;	/* Maximum code value */
 
-	if (temp >= offset)
+	if (temp >= 410040)
 		return 0;	/* Minimum code value */
 
 	if (low)
-		return (u32)(DIV_ROUND_UP(offset - temp, abs(slope)));
+		return (u32)(DIV_ROUND_UP(410040 - temp, 487));
 	else
-		return (u32)((offset - temp) / abs(slope));
+		return (u32)((410040 - temp) / 487);
 }
 
 static int brcmstb_get_temp(void *data, int *temp)