Re: [PATCH 1/1] Add ST STPDDC60 pmbus driver

[Guenter Roeck]

On 2/1/21 8:48 AM, Erik Rosen wrote:
> Hi Guenter
> 
> 
> On Fri, Jan 29, 2021 at 4:50 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>>
>> Hi Erik,
>>
>> On 1/29/21 5:07 AM, Erik Rosen wrote:
>> [ ... ]
>>>>
>>>>> +             break;
>>>>> +     case PMBUS_VOUT_OV_FAULT_LIMIT:
>>>>> +     case PMBUS_VOUT_UV_FAULT_LIMIT:
>>>>> +             ret = pmbus_read_word_data(client, page, phase, reg);
>>>>> +             if (ret < 0)
>>>>> +                     return ret;
>>>>> +             ret &= 0x07ff;
>>>>
>>>> This needs explanation. The BMR481 datasheet does not suggest that only
>>>> 11 bits are valid.
>>>
>>> Ok, I will add a clarification. These registers use LINEAR11 whereas
>>> LINEAR16 is expected for vout-related registers.
>>>
>> Is that the correct fix, then ? LINEAR11 means that the exponent is flexible,
>> while it is fixed for LINEAR16. Just assuming that the exponents always match
>> seems risky. Also, bit 11 in LINEAR11 is the sign bit, meaning negative limits
>> (which seem at least theoretically be possible) would be reported as large
>> positive values.
> 
> The chip actually uses fixed exponents for representing all linear values and
> the specification explicitly states the values of the LSB for all registers.
> It also states that the limits can be handled as linear format when
> writing  _if_
> the exponent is the fixed value used for that limit. This means I'll have to
> convert all limit writes to use the expected exponent.

I understand the datasheet a bit differently. You are correct, the exponent for
VOUT limits is always the same (LSB=0.00390625V, exponent -8), as it should be.
But it also seems to me that the data format is LINEAR16, not LINEAR11.
The datasheet says that the vout limit values are in bit 0..15.

For other sensors, the datasheet is a bit ambiguous. I can read it as always
using a fixed exponent when reading, or that it expects a fixed exponent when
writing. It might make sense to check this with an actual chip instead of
guessing. In this context, it is a bit odd that the datasheet keeps talking
about a "System Register IOUT_EXP" without actually specifying it.

In this context, you might want to watch out for register MFR_SVID_SLOW_SR_SELECTOR.
Its value seems to impact the exponents used for IOUT and POUT.

Also, I am not sure about the scale of other registers. It almost seems as if
limit registers are LINEAR11, but actual READ_xxx registers are LINEAR16.
For example, READ_IOUT is supposed to be bit 15:0 with the LSB determined
by IOUT_EXP, but IOUT_OC_WARN_LIMIT is in bit 9:0. This would be a problem
if READ_IOUT does not include the exponent, since it is interpreted as
LINEAR11 and expects the exponent in bit 11..15. With an expected exponent
of -1, the reported value would always be wrong. The same might apply
to pretty much all READ_xxx registers (what a mess).

Thanks,
Guenter

> Given this, I think it's safe to assume that the exponents are
> constant, but I'll
> add a check to handle potential negative values.
> 
>>
>>>>
>>>>> +             break;
>>>>> +     default:
>>>>> +             ret = -ENODATA;
>>>>> +             break;
>>>>> +     }
>>>>> +
>>>>> +     return ret;
>>>>> +}
>>>>> +
>>>>> +/*
>>>>> + * The vout under- and over-voltage limits are readonly for this chip.
>>>>> + */
>>>>
>>>> Not really. The BMR481 datasheet suggests that the value can be changed
>>>> by writing to MFR_OV_LIMIT_OFFSET and MFR_UV_LIMIT_OFFSET.
>>>> I am not saying that you should implement that if you don't want to,
>>>> but I would suggest a better (more accurate) explanation.
>>>
>>> I have looked into this a bit more and it's a bit more to it than I expected.
>>> The limits are actually dynamic values that will follow both commanded
>>> output voltage via the PMBus or SVID/AVSBus as well as current
>>> load (droop). To account for this I propose a mechanism to set the
>>
>> Yes, I noticed.
>>
>>> 'update' flag on selected sensors after auto-detection of limit attributes.
>>>
>>> Maybe add a function like this to pmbus_core that can be called
>>> after the probing is done?
>>>
>>> int pmbus_set_update(struct i2c_client *client, u8 reg)
>>> {
>>>         struct pmbus_data *data = i2c_get_clientdata(client);
>>>         struct pmbus_sensor *sensor;
>>>
>>>         for (sensor = data->sensors; sensor; sensor = sensor->next) {
>>>                 if (sensor->reg == reg) {
>>>                         sensor->update = true;
>>>                         return 0;
>>>                 }
>>>         }
>>>         return -ENXIO;
>>> }
>>>
>>
>> Add a boolean 'update' parameter (to be able to disable updates),
>> and make the function void. Also, remember that 'reg' may be repeated
>> on a chip with multiple pages, so you can't return after the first match.
>> Otherwise looks ok.
> 
> Ok
> 
>>
>>> I did also implemented writes to the limit registers via the offset
>>> registers but it might be
>>> a bit confusing to the user since the limits are set in relation to
>>> the current commanded
>>> output voltage and will change together with this. In the worst case,
>>> the voltage might
>>> change at the same time as the limit is written to, which will give
>>> unexpected results.
>>
>> Agreed, that can be tricky.
>>
>>> The alternative would be to just set these limits read-only. What is
>>> your opinion?
>>>
>>
>> Your call. Just add a note explaining why it is made read-only to explain
>> the reasoning for future readers.
> 
> Ok, I'll keep the functionality and add a note in the docs.
> 
> 
>>
>>> Also, I found a problem in how pmbus_set_sensor() works together with
>>> pmbus_clear_cache()
>>> as used in the zl6100 and lm25066 drivers. The new value is written to
>>> the sensor struct _after_
>>> the _pmbus_write_word_data() has returned and thus after
>>> pmbus_clear_cache() is called.
>>> The effect is that all values are cleared, except the one just written
>>> to, which I believe defeats
>>> the purpose of clearing the cache in the first place.> One solution would be to write the new value to sensor->data before
>>> the _pmbus_write_word_data
>>> is called and to restore the old value in case of error.
>>> I can make a separate patch for this if it seems like a reasonable solution?
>>>
>>
>> Good catch.
>>
>> An alternate and more generic solution might be to set sensor->data to
>> -ENODATA after updates. After all, it seems risky to assume that the chip
>> returns the value that was written (I have seen it often enough in other
>> drivers that this is not necessarily the case). That would also mean that
>> it would no longer be necessary to call pmbus_clear_cache() in the zl6100
>> and lm25066 drivers. Impact would be that a limit read after a write would
>> always be sent to the chip for all drivers, but that seems minimal compared
>> to the gain (and presumably limit registers are not frequently updated).
> 
> I agree that this is a more robust solution. I'll send a separate patch for this
> update. The zl6100 in fact still needs to clear the cache since a write to the
> fault limit also changes the value of the warning limit and vice versa. As far
> as I can see the usage in lm25066 can be removed however.
> 
>>
>> Thanks,
>> Guenter