Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
> 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. So a simple PECI model is now merged. Sensors are always welcome, it's nice to have properties to change values. On my wish-list are PCIe and a working USB gadget network device. Thanks, C.
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
On Wed, Jun 29, 2022 at 11:34 AM Peter Delevoryas wrote:
>
>
> > On Jun 29, 2022, at 11:04 AM, Titus Rwantare wrote:
> >
> > On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> > wrote:
> >>
> >> Signed-off-by: Peter Delevoryas
> >> ---
> >
> >> --- 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
>
>
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
> On Jun 29, 2022, at 11:04 AM, Titus Rwantare wrote:
>
> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
> wrote:
>>
>> Signed-off-by: Peter Delevoryas
>> ---
>
>> --- 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
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas
wrote:
>
> Signed-off-by: Peter Delevoryas
> ---
> --- 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.
> 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()
> 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.
Thanks for the new device. It helps me see where to expand on PMBus.
Titus
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
> On Jun 29, 2022, at 1:40 AM, Cédric Le Goater wrote:
>
> On 6/29/22 05:36, Peter Delevoryas wrote:
>> Signed-off-by: Peter Delevoryas
>
> This is also adding a "Renesas ISL69259 Digital Multiphase Voltage
> Regulator" device. There should be 2 patches.
H yes definitely, I’ll fix this. One patch to add IC_DEVICE_ID
to pmbus, one to add ISL69259 to isl_pmbus_vr.c
>
> Thanks,
>
> C.
>
>
>
>> ---
>> hw/i2c/pmbus_device.c| 5 +
>> hw/sensor/isl_pmbus_vr.c | 31 +++
>> include/hw/i2c/pmbus_device.h| 1 +
>> include/hw/sensor/isl_pmbus_vr.h | 1 +
>> 4 files changed, 38 insertions(+)
>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
>> index efddc36fd9..82131fff85 100644
>> --- 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;
>> +
>> 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));
>> +}
>> +}
>> +
>> +static void isl69259_class_init(ObjectClass *klass, void *data)
>> +{
>> +ResettableClass *rc = RESETTABLE_CLASS(klass);
>> +DeviceClass *dc = DEVICE_CLASS(klass);
>> +dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator";
>> +rc->phases.exit = isl_pmbus_vr_exit_reset;
>> +isl_pmbus_vr_class_init(klass, data, 2);
>> +}
>> +
>> static void isl69260_class_init(ObjectClass *klass, void *data)
>> {
>> ResettableClass *rc = RESETTABLE_CLASS(klass);
>> @@ -245,6 +267,14 @@ static void raa229004_class_init(ObjectClass *klass,
>> void *data)
>> isl_pmbus_vr_class_init(klass, data, 2);
>> }
>> +static const TypeInfo isl69259_info = {
>> +.name = TYPE_ISL69259,
>> +.parent = TYPE_PMBUS_DEVICE,
>> +.instance_size = sizeof(ISLState),
>> +.instance_init = isl69259_init,
>> +.class_init = isl69259_class_init,
>> +};
>> +
>> static const TypeInfo isl69260_info = {
>> .name = TYPE_ISL69260,
>> .parent = TYPE_PMBUS_DEVICE,
>> @@ -271,6 +301,7 @@ static const TypeInfo raa228000_info = {
>>static void isl_pmbus_vr_register_types(void)
>> {
>> +type_register_static(_info);
>> type_register_static(_info);
>> type_register_static(_info);
>> type_register_static(_info);
>> 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 */
>> } PMBusPage;
>>/* State */
>> diff --git a/include/hw/sensor/isl_pmbus_vr.h
>> b/include/hw/sensor/isl_pmbus_vr.h
>> index 3e47ff7e48..d501b3bc82 100644
>> --- a/include/hw/sensor/isl_pmbus_vr.h
>> +++ b/include/hw/sensor/isl_pmbus_vr.h
>> @@ -12,6 +12,7 @@
>> #include "hw/i2c/pmbus_device.h"
>> #include "qom/object.h"
>> +#define TYPE_ISL69259 "isl69259"
>> #define TYPE_ISL69260 "isl69260"
>> #define TYPE_RAA228000 "raa228000"
>> #define TYPE_RAA229004 "raa229004"
>
Re: [PATCH v2 09/13] hw/i2c/pmbus: Add read-only IC_DEVICE_ID support
On 6/29/22 05:36, Peter Delevoryas wrote:
Signed-off-by: Peter Delevoryas
This is also adding a "Renesas ISL69259 Digital Multiphase Voltage
Regulator" device. There should be 2 patches.
Thanks,
C.
---
hw/i2c/pmbus_device.c| 5 +
hw/sensor/isl_pmbus_vr.c | 31 +++
include/hw/i2c/pmbus_device.h| 1 +
include/hw/sensor/isl_pmbus_vr.h | 1 +
4 files changed, 38 insertions(+)
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c
index efddc36fd9..82131fff85 100644
--- 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;
+
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));
+}
+}
+
+static void isl69259_class_init(ObjectClass *klass, void *data)
+{
+ResettableClass *rc = RESETTABLE_CLASS(klass);
+DeviceClass *dc = DEVICE_CLASS(klass);
+dc->desc = "Renesas ISL69259 Digital Multiphase Voltage Regulator";
+rc->phases.exit = isl_pmbus_vr_exit_reset;
+isl_pmbus_vr_class_init(klass, data, 2);
+}
+
static void isl69260_class_init(ObjectClass *klass, void *data)
{
ResettableClass *rc = RESETTABLE_CLASS(klass);
@@ -245,6 +267,14 @@ static void raa229004_class_init(ObjectClass *klass, void
*data)
isl_pmbus_vr_class_init(klass, data, 2);
}
+static const TypeInfo isl69259_info = {
+.name = TYPE_ISL69259,
+.parent = TYPE_PMBUS_DEVICE,
+.instance_size = sizeof(ISLState),
+.instance_init = isl69259_init,
+.class_init = isl69259_class_init,
+};
+
static const TypeInfo isl69260_info = {
.name = TYPE_ISL69260,
.parent = TYPE_PMBUS_DEVICE,
@@ -271,6 +301,7 @@ static const TypeInfo raa228000_info = {
static void isl_pmbus_vr_register_types(void)
{
+type_register_static(_info);
type_register_static(_info);
type_register_static(_info);
type_register_static(_info);
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 */
} PMBusPage;
/* State */
diff --git a/include/hw/sensor/isl_pmbus_vr.h b/include/hw/sensor/isl_pmbus_vr.h
index 3e47ff7e48..d501b3bc82 100644
--- a/include/hw/sensor/isl_pmbus_vr.h
+++ b/include/hw/sensor/isl_pmbus_vr.h
@@ -12,6 +12,7 @@
#include "hw/i2c/pmbus_device.h"
#include "qom/object.h"
+#define TYPE_ISL69259 "isl69259"
#define TYPE_ISL69260 "isl69260"
#define TYPE_RAA228000 "raa228000"
#define TYPE_RAA229004 "raa229004"
