Re: [PATCH 5/9] hw/i2c: pmbus: Page #255 is valid page for read requests.

[Jae Hyun Yoo]

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