> On Jun 29, 2022, at 11:04 AM, Titus Rwantare <tit...@google.com> wrote:
>
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> <peterdelevor...@gmail.com> wrote:
>>
>> Signed-off-by: Peter Delevoryas <p...@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
