On Wed, Jun 29, 2022 at 11:34 AM Peter Delevoryas <p...@fb.com> wrote:
> > > > 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. > 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 > >
