On Wed, Jun 29, 2022 at 11:34 AM Peter Delevoryas <
pdel@fb.com> wrote:
> 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.
Peter, feel free to reach out to me, or Titus, and we can sync up. I used to work with Patrick who's now at Fb on OpenBMC stuff. We sent a bunch of the Nuvoton patches up for review recently, and we're actively adding more devices, etc.
We also have quite a few patches downstream we're looking to upstream, including PECI, and sensors, etc, etc that we've seen on BMC servers.
Patrick
Thanks!
Peter
>
>
> Titus