| Message ID | 20220622172830.101210-6-quic_jaehyoo@quicinc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Qualcomm BMC machines | expand |
On Wed, 22 Jun 2022 at 10:29, Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote: > > From: Maheswara Kurapati <quic_mkurapat@quicinc.com> > > Current implementation of the pmbus core driver treats the read request > for page 255 as invalid request and sets the invalid command bit (bit 7) in the > STATUS_CML register. As per the PMBus specification it is a valid request. > > Refer to the PMBus specification, revision 1.3.1, section 11.10 PAGE, on the page 58: > "Setting the PAGE to FFh means that all subsequent comands are to be applied to > all outputs. > > Some commands, such as READ_TEMPERATURE, may use a common sensor but be > available on all pages of a device. Such implementations are the decision of > each device manufacturer or are specified in a PMBus Application Profile. Consult > the manufacturer's socuments or the Applicatin Profile Specification as needed." > Thanks for this, the copy of the spec I used was older. > For e.g., > The VOUT_MODE is a valid command for page 255 for maxim 31785 device. > refer to Table 1. PMBus Command Codes on page 14 in the datasheet. > https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf > > Fixes: 38870253f1d1 ("hw/i2c: pmbus: fix error returns and guard against out of range accesses") > > Signed-off-by: Maheswara Kurapati <quic_mkurapat@quicinc.com> > --- > hw/i2c/pmbus_device.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c > index 62885fa6a15e..7db3343a83b6 100644 > --- a/hw/i2c/pmbus_device.c > +++ b/hw/i2c/pmbus_device.c > @@ -291,7 +291,6 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd) > qemu_log_mask(LOG_GUEST_ERROR, > "%s: tried to read from all pages\n", > __func__); > - pmbus_cml_error(pmdev); > } else if (pmdev->page > pmdev->num_pages - 1) { > qemu_log_mask(LOG_GUEST_ERROR, > "%s: page %d is out of range\n", > -- > 2.25.1 > Please also update the stale comment just above, since this is now specified behaviour. Reviewed-by: Titus Rwantare <titusr@google.com>
Hello Titus, On 6/22/2022 1:49 PM, Titus Rwantare wrote: > On Wed, 22 Jun 2022 at 10:29, Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote: >> >> From: Maheswara Kurapati <quic_mkurapat@quicinc.com> >> >> Current implementation of the pmbus core driver treats the read request >> for page 255 as invalid request and sets the invalid command bit (bit 7) in the >> STATUS_CML register. As per the PMBus specification it is a valid request. >> >> Refer to the PMBus specification, revision 1.3.1, section 11.10 PAGE, on the page 58: >> "Setting the PAGE to FFh means that all subsequent comands are to be applied to >> all outputs. >> >> Some commands, such as READ_TEMPERATURE, may use a common sensor but be >> available on all pages of a device. Such implementations are the decision of >> each device manufacturer or are specified in a PMBus Application Profile. Consult >> the manufacturer's socuments or the Applicatin Profile Specification as needed." >> > Thanks for this, the copy of the spec I used was older. > > >> For e.g., >> The VOUT_MODE is a valid command for page 255 for maxim 31785 device. >> refer to Table 1. PMBus Command Codes on page 14 in the datasheet. >> https://datasheets.maximintegrated.com/en/ds/MAX31785.pdf >> >> Fixes: 38870253f1d1 ("hw/i2c: pmbus: fix error returns and guard against out of range accesses") >> >> Signed-off-by: Maheswara Kurapati <quic_mkurapat@quicinc.com> >> --- >> hw/i2c/pmbus_device.c | 1 - >> 1 file changed, 1 deletion(-) >> >> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c >> index 62885fa6a15e..7db3343a83b6 100644 >> --- a/hw/i2c/pmbus_device.c >> +++ b/hw/i2c/pmbus_device.c >> @@ -291,7 +291,6 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd) >> qemu_log_mask(LOG_GUEST_ERROR, >> "%s: tried to read from all pages\n", >> __func__); >> - pmbus_cml_error(pmdev); >> } else if (pmdev->page > pmdev->num_pages - 1) { >> qemu_log_mask(LOG_GUEST_ERROR, >> "%s: page %d is out of range\n", >> -- >> 2.25.1 >> > > Please also update the stale comment just above, since this is now > specified behaviour. Right. The error log printing also needs to be removed so I'll revise this fix like below in v2. diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c index 62885fa6a15e..749a33af827b 100644 --- a/hw/i2c/pmbus_device.c +++ b/hw/i2c/pmbus_device.c @@ -284,14 +284,10 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd) /* * Reading from all pages will return the value from page 0, - * this is unspecified behaviour in general. + * means that all subsequent commands are to be applied to all output. */ if (pmdev->page == PB_ALL_PAGES) { index = 0; - qemu_log_mask(LOG_GUEST_ERROR, - "%s: tried to read from all pages\n", - __func__); - pmbus_cml_error(pmdev); } else if (pmdev->page > pmdev->num_pages - 1) { qemu_log_mask(LOG_GUEST_ERROR, "%s: page %d is out of range\n", > Reviewed-by: Titus Rwantare <titusr@google.com> Thank you so much for your review. Regards, Jae
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c index 62885fa6a15e..7db3343a83b6 100644 --- a/hw/i2c/pmbus_device.c +++ b/hw/i2c/pmbus_device.c @@ -291,7 +291,6 @@ static uint8_t pmbus_receive_byte(SMBusDevice *smd) qemu_log_mask(LOG_GUEST_ERROR, "%s: tried to read from all pages\n", __func__); - pmbus_cml_error(pmdev); } else if (pmdev->page > pmdev->num_pages - 1) { qemu_log_mask(LOG_GUEST_ERROR, "%s: page %d is out of range\n",