sensors: occ: Skip counter type of sensors

Message ID 1508396903-19476-1-git-send-email-shilpa.bhat@linux.vnet.ibm.com
State Accepted
Headers show
Series
  • sensors: occ: Skip counter type of sensors
Related show

Commit Message

Shilpasri G Bhat Oct. 19, 2017, 7:08 a.m.
Don't add counter type of sensors to device-tree as they don't
fit into hwmon sensor interface.

Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
---
 hw/occ-sensor.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Stewart Smith Oct. 23, 2017, 1:39 a.m. | #1
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> Don't add counter type of sensors to device-tree as they don't
> fit into hwmon sensor interface.

Shouldn't we expose these in the device tree somehow anyway? even if we
don't currently have a good way to read it from kernel?

I'd prefer we add something in skiboot now so we can get kernel patches
out later rather than be stuck with not being able to read these sensors
at all for the first rounds of machines.
Oliver Nov. 24, 2017, 4:43 a.m. | #2
On Mon, Oct 23, 2017 at 12:39 PM, Stewart Smith
<stewart@linux.vnet.ibm.com> wrote:
> Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
>> Don't add counter type of sensors to device-tree as they don't
>> fit into hwmon sensor interface.
>
> Shouldn't we expose these in the device tree somehow anyway? even if we
> don't currently have a good way to read it from kernel?

How useful to the end user is this information though? If you're
getting voltage droops at the quad or core level it points to either a
system level power distribution issue or the OCC doing a poor job of
managing frequency scaling. This is useful information for developers
rather than system administrators  and there are already interfaces
for developers to extract this information. We already filter out OCC
sensors that don't fit into the standard voltage/current/thermal/power
sensor types that hwmon supports so I don't see why we should keep
this.

> I'd prefer we add something in skiboot now so we can get kernel patches
> out later rather than be stuck with not being able to read these sensors
> at all for the first rounds of machines.

Why not just wait until the kernel has a better interface for this
type of sensor? It's not critical and odds are the device-tree
bindings will be different anyway. Trying to shoe-horn it into the
voltage sensor interface just seems like a mistake.


Not just randomly necroposting. I dug up this patch since it was
referenced in BZ159862 and I think you should merge it ;)

> --
> Stewart Smith
> OPAL Architect, IBM.
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Oliver Nov. 24, 2017, 5:12 a.m. | #3
On Thu, Oct 19, 2017 at 6:08 PM, Shilpasri G Bhat
<shilpa.bhat@linux.vnet.ibm.com> wrote:
> Don't add counter type of sensors to device-tree as they don't
> fit into hwmon sensor interface.
>
> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
> ---
>  hw/occ-sensor.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
> index 1042c11..3de91a0 100644
> --- a/hw/occ-sensor.c
> +++ b/hw/occ-sensor.c
> @@ -591,6 +591,9 @@ void occ_sensors_init(void)
>                         struct cpu_thread *c = NULL;
>                         u32 handler;
>
> +                       if (md[i].structure_type != OCC_SENSOR_READING_FULL)
> +                               continue;
> +
>                         if (!(md[i].type & HWMON_SENSORS_MASK))
>                                 continue;

You might be better off filtering these based on the unit in the name
block rather than the type.

>
> --
> 1.8.3.1
>
> _______________________________________________
> Skiboot mailing list
> Skiboot@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
Shilpasri G Bhat Nov. 24, 2017, 6:58 a.m. | #4
Hi,

On 11/24/2017 10:42 AM, Oliver wrote:
> On Thu, Oct 19, 2017 at 6:08 PM, Shilpasri G Bhat
> <shilpa.bhat@linux.vnet.ibm.com> wrote:
>> Don't add counter type of sensors to device-tree as they don't
>> fit into hwmon sensor interface.
>>
>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>> ---
>>  hw/occ-sensor.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
>> index 1042c11..3de91a0 100644
>> --- a/hw/occ-sensor.c
>> +++ b/hw/occ-sensor.c
>> @@ -591,6 +591,9 @@ void occ_sensors_init(void)
>>                         struct cpu_thread *c = NULL;
>>                         u32 handler;
>>
>> +                       if (md[i].structure_type != OCC_SENSOR_READING_FULL)
>> +                               continue;
>> +
>>                         if (!(md[i].type & HWMON_SENSORS_MASK))
>>                                 continue;
> 
> You might be better off filtering these based on the unit in the name
> block rather than the type.


The unit of a sensor is of type string. OCC does not guarantee backward
compatibility with name, unit and number of sensors. The unit of counter sensors
is "#" today. So I am suggesting it would be better to use the type of the
sensor record itself to filter out counter type of sensors.

OCC_SENSOR_READING_FULL         = 0x01,
OCC_SENSOR_READING_COUNTER      = 0x02,

Thanks and Regards,
Shilpa

> 
>>
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Skiboot mailing list
>> Skiboot@lists.ozlabs.org
>> https://lists.ozlabs.org/listinfo/skiboot
>
Stewart Smith Dec. 14, 2017, 4:47 a.m. | #5
Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com> writes:
> On 11/24/2017 10:42 AM, Oliver wrote:
>> On Thu, Oct 19, 2017 at 6:08 PM, Shilpasri G Bhat
>> <shilpa.bhat@linux.vnet.ibm.com> wrote:
>>> Don't add counter type of sensors to device-tree as they don't
>>> fit into hwmon sensor interface.
>>>
>>> Signed-off-by: Shilpasri G Bhat <shilpa.bhat@linux.vnet.ibm.com>
>>> ---
>>>  hw/occ-sensor.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
>>> index 1042c11..3de91a0 100644
>>> --- a/hw/occ-sensor.c
>>> +++ b/hw/occ-sensor.c
>>> @@ -591,6 +591,9 @@ void occ_sensors_init(void)
>>>                         struct cpu_thread *c = NULL;
>>>                         u32 handler;
>>>
>>> +                       if (md[i].structure_type != OCC_SENSOR_READING_FULL)
>>> +                               continue;
>>> +
>>>                         if (!(md[i].type & HWMON_SENSORS_MASK))
>>>                                 continue;
>> 
>> You might be better off filtering these based on the unit in the name
>> block rather than the type.
>
>
> The unit of a sensor is of type string. OCC does not guarantee backward
> compatibility with name, unit and number of sensors. The unit of counter sensors
> is "#" today. So I am suggesting it would be better to use the type of the
> sensor record itself to filter out counter type of sensors.
>
> OCC_SENSOR_READING_FULL         = 0x01,
> OCC_SENSOR_READING_COUNTER      = 0x02,


Thanks for the discussion, makes sense now.

Merged to master as of 275efcc44fdad3572e6180f625d356c8b7cca68d
and 5.9.x as of 87043c358b7b53693c89113b483cd702616ee994

Patch

diff --git a/hw/occ-sensor.c b/hw/occ-sensor.c
index 1042c11..3de91a0 100644
--- a/hw/occ-sensor.c
+++ b/hw/occ-sensor.c
@@ -591,6 +591,9 @@  void occ_sensors_init(void)
 			struct cpu_thread *c = NULL;
 			u32 handler;
 
+			if (md[i].structure_type != OCC_SENSOR_READING_FULL)
+				continue;
+
 			if (!(md[i].type & HWMON_SENSORS_MASK))
 				continue;