| 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 | expand |
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.
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
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
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 >
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
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;
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(+)