Re: [PATCH] hwmon: (pmbus) Clear pmbus fault/warning bits before read

[Vikash Chandola <vikash.chandola@linux.intel.com>]

On 2/11/2022 1:27 AM, Guenter Roeck wrote:
> On 2/10/22 11:55, Guenter Roeck wrote:
>> On 2/10/22 10:29, Vikash Chandola wrote:
>>>
>>>
>>> On 2/10/2022 10:55 PM, Guenter Roeck wrote:
>>>> On 2/10/22 07:57, Vikash Chandola wrote:
>>>>>
>>>>>
>>>>> On 2/10/2022 8:14 PM, Guenter Roeck wrote:
>>>>>> On 2/10/22 04:41, Vikash Chandola wrote:
>>>>>>> pmbus fault and warning bits are not cleared by itself once 
>>>>>>> fault/warning
>>>>>>> condition is not valid anymore. As per pmbus datasheet faults 
>>>>>>> must be
>>>>>>> cleared by user.
>>>>>>> Modify hwmon behavior to clear latched status bytes if any bit in 
>>>>>>> status
>>>>>>> register is high prior to returning fresh data to userspace. If
>>>>>>> fault/warning conditions are still applicable fault/warning bits 
>>>>>>> will be
>>>>>>> set and we will get updated data in second read.
>>>>>>>
>>>>>>> Hwmon behavior is changed here. Now sysfs reads will reflect latest
>>>>>>> values from pmbus slave, not latched values.
>>>>>>> In case a transient warning/fault has happened in the past, it 
>>>>>>> will no
>>>>>>> longer be reported to userspace.
>>>>>>>
>>>>>>
>>>>>>
>>>>>> NACK.
>>>>>>
>>>>>> Reporting that information is exactly the point of the current code.
>>>>>> We _do_ want to report at least once that a problem occurred in 
>>>>>> the past,
>>>>>> and only clear the warning flag(s) afterwards.
>>>>>>
>>>>>> Guenter
>>>>>>
>>>>> But as per current implementation we will continue to report fault 
>>>>> even after fault condition is cleared. I could not find sysfs entry 
>>>>> or any other means by which user/kernel can clear the 
>>>>> faults/warnings after it is reported. Please point if I am missing 
>>>>> anything.
>>>>>
>>>>
>>>> Normally a chip should clear the status after the fault condition 
>>>> cleared
>>>> and the register was read. At least that was my experience so far.
>>>> What chip (or chips) don't do that ?
>>>>
>>>> Guenter
>>>
>>> I see that pmbus spec says that once fault/warning bits are set
>>> they won't be cleared by itself. Section 10.2.3 of
>>> "PMBus Specification Rev. 1.3.1 Part II 2015-03-13" from
>>> https://pmbus.org/specification-archives/   says
>>>
>>> "
>>> Almost all of the warning or fault bits set in the status registers 
>>> remain set, even if the fault or warning condition is removed
>>> or corrected, until one of the following occur:
>>> * The bit is individually cleared (Section 10.2.4),
>>> * The device receives a CLEAR_FAULTS command (Section 15.1),
>>> * A RESET signal (if one exists) is asserted,
>>> * The output is commanded through the CONTROL pin, the OPERATION 
>>> command, or the combined action of the CONTROL pin and OPERATION
>>> command, to turn off and then to turn back on, or
>>> * Bias power is removed from the PMBus device...
>>> "
>>>
>>>  From this I concluded that slave(PSU) following pmbus spec will not
>>> clear the fault/warning in status registers.
>>> I don't have exact chip details handy but I can get it by tomorrow.
>>> However this looks to be issue not related to specific chip ?
>>>
>>
>> Good point. Interesting that I have not seen this before.
>>
>>> If this is a problem, how should I approach this ? Shall I create a new
>>> sysfs entry that user space application can invoke and clear all faults
>>> or provide sysfs entry to clear individual fault/warning bits or
>>> something else ?
>>>
>>
>> No. What we need to do is to add code to pmbus_get_boolean() and 
>> selectively
>> write the reported status bit back into the status register. Something 
>> like
>>      ...
>>      regval = status & mask;
>>      if (regval) {
>>          ret = pmbus_write_status(client, page, reg, regval);
>>                        ^^^^^^^^^^^^^^^^^^ new function
>>          if (ret)
>>              goto unlock;
>>      }
>>
>> and that will need to be tested on a variety of real hardware.
Hi Guenter,
I see that there is already a method pmbus_write_byte_data that does
same thing. I will post patch using that method to update one bit.
>>
> 
> Plus, of course, we need to confirm that there is at least one chip which
> does follow the specification and does not auto-clear alarms.
We observed above behavior SOLUM G36234-015 PSU.
Apologies for delay. It took some time to get information from our PSU
experts that PSU firmware itself is not broken here.
> 
> Guenter

-- 
Vikash