Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support

[Peter Delevoryas]

> On Jun 29, 2022, at 11:04 AM, Titus Rwantare <titusr@google.com> wrote:
> 
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> <peterdelevoryas@gmail.com> wrote:
>> 
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
> 
>> --- a/hw/i2c/pmbus_device.c
>> +++ b/hw/i2c/pmbus_device.c
>> @@ -984,6 +984,11 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>>         }
>>         break;
>> 
>> +    case PMBUS_IC_DEVICE_ID:
>> +        pmbus_send(pmdev, pmdev->pages[index].ic_device_id,
>> +                   sizeof(pmdev->pages[index].ic_device_id));
>> +        break;
>> +
> 
> I don't think it's a good idea to add this here because this sends 16
> bytes for all PMBus devices. I have at least one device that formats
> IC_DEVICE_ID differently that I've not got permission to upstream.
> The spec leaves the size and format up to the manufacturer, so this is
> best done in isl_pmbus_vr.c in isl_pmbus_vr_read_byte().
> Look at the adm1272_read_byte() which is more interesting than
> isl_pmbus_vr one as an example.

Argh, yes, you’re absolutely right. I didn’t read the spec in very
much detail, I see now that the length is device-specific. For the
ISL69259 I see that it’s 4 bytes, which makes sense now. This
is not the exact datasheet for the ISL69259, but I think the IC_DEVICE_ID
part is the same.

        https://www.renesas.com/us/en/document/dst/isl68229-isl68239-datasheet

Putting the logic in isl_pmbus_vr_read_byte() is a good idea, I hadn’t
seen the implementation in adm1272_read_byte(), that looks great,
I’ll just add a switch on the command code and move the error message
to the default case.

> 
> 
>>     case PMBUS_CLEAR_FAULTS:              /* Send Byte */
>>     case PMBUS_PAGE_PLUS_WRITE:           /* Block Write-only */
>>     case PMBUS_STORE_DEFAULT_ALL:         /* Send Byte */
>> diff --git a/hw/sensor/isl_pmbus_vr.c b/hw/sensor/isl_pmbus_vr.c
>> index e11e028884..b12c46ab6d 100644
>> --- a/hw/sensor/isl_pmbus_vr.c
>> +++ b/hw/sensor/isl_pmbus_vr.c
>> @@ -218,6 +218,28 @@ static void isl_pmbus_vr_class_init(ObjectClass *klass, 
>> void *data,
>>     k->device_num_pages = pages;
>> }
>> 
>> +static void isl69259_init(Object *obj)
>> +{
>> +    static const uint8_t ic_device_id[] = {0x04, 0x00, 0x81, 0xD2, 0x49};
>> +    PMBusDevice *pmdev = PMBUS_DEVICE(obj);
>> +    int i;
>> +
>> +    raa22xx_init(obj);
>> +    for (i = 0; i < pmdev->num_pages; i++) {
>> +        memcpy(pmdev->pages[i].ic_device_id, ic_device_id,
>> +               sizeof(ic_device_id));
>> +    }
>> +}
>> +
> 
> We tend to set default register values in exit_reset() calls. You can
> do something like in raa228000_exit_reset()

Oh got it. If I can move the logic to isl_pmbus_vr_read_byte perhaps
I can avoid this whole function though.

> 
> 
>> diff --git a/include/hw/i2c/pmbus_device.h b/include/hw/i2c/pmbus_device.h
>> index 0f4d6b3fad..aed7809841 100644
>> --- a/include/hw/i2c/pmbus_device.h
>> +++ b/include/hw/i2c/pmbus_device.h
>> @@ -407,6 +407,7 @@ typedef struct PMBusPage {
>>     uint16_t mfr_max_temp_1;           /* R/W word */
>>     uint16_t mfr_max_temp_2;           /* R/W word */
>>     uint16_t mfr_max_temp_3;           /* R/W word */
>> +    uint8_t ic_device_id[16];          /* Read-Only block-read */
> 
> You wouldn't be able to do this here either, since the size could be
> anything for other devices.

Right, yeah I see what you mean.

> 
> Thanks for the new device. It helps me see where to expand on PMBus.

Thanks for adding the whole pmbus infrastructure! It’s really useful.
And thanks for the review.

Off-topic, but I’ve been meaning to reach out to you guys (Google
engineers working on QEMU for OpenBMC) about your Nuvoton NPCM845R
series, my team is interested in using it. I was just curious about
the status of it and if there’s any features missing and what plans
you have for the future, maybe we can collaborate.

Thanks!
Peter

> 
> 
> Titus