| Message ID | 20220629033634.3850922-9-pdel@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/i2c/aspeed: Add new-registers DMA slave mode RX support | expand |
On 6/29/22 05:36, Peter Delevoryas wrote: > When a pmbus device switches pages, it should clears its output buffer so > that the next transaction doesn't emit data from the previous page. > > Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”) > Signed-off-by: Peter Delevoryas <pdel@fb.com> > --- Reviewed-by: Cédric Le Goater <clg@kaod.org> Thanks, C. > hw/i2c/pmbus_device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c > index 62885fa6a1..efddc36fd9 100644 > --- a/hw/i2c/pmbus_device.c > +++ b/hw/i2c/pmbus_device.c > @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) > > if (pmdev->code == PMBUS_PAGE) { > pmdev->page = pmbus_receive8(pmdev); > + pmdev->out_buf_len = 0; > return 0; > } >
On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas <peterdelevoryas@gmail.com> wrote: > > When a pmbus device switches pages, it should clears its output buffer so > that the next transaction doesn't emit data from the previous page. > > Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”) > Signed-off-by: Peter Delevoryas <pdel@fb.com> > --- > hw/i2c/pmbus_device.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c > index 62885fa6a1..efddc36fd9 100644 > --- a/hw/i2c/pmbus_device.c > +++ b/hw/i2c/pmbus_device.c > @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) > > if (pmdev->code == PMBUS_PAGE) { > pmdev->page = pmbus_receive8(pmdev); > + pmdev->out_buf_len = 0; > return 0; > } > I suspect you were running into this because ic_device_id was putting too much data in the output buffer. Still, I wouldn't want the buffer cleared if the page hasn't changed. Some drivers write the same page before every read. Titus
> On Jun 29, 2022, at 11:05 AM, Titus Rwantare <titusr@google.com> wrote: > > On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas > <peterdelevoryas@gmail.com> wrote: >> >> When a pmbus device switches pages, it should clears its output buffer so >> that the next transaction doesn't emit data from the previous page. >> >> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”) >> Signed-off-by: Peter Delevoryas <pdel@fb.com> >> --- >> hw/i2c/pmbus_device.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c >> index 62885fa6a1..efddc36fd9 100644 >> --- a/hw/i2c/pmbus_device.c >> +++ b/hw/i2c/pmbus_device.c >> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) >> >> if (pmdev->code == PMBUS_PAGE) { >> pmdev->page = pmbus_receive8(pmdev); >> + pmdev->out_buf_len = 0; >> return 0; >> } >> > > I suspect you were running into this because ic_device_id was putting > too much data in the output buffer. Still, I wouldn't want the buffer > cleared if the page hasn't changed. Some drivers write the same page > before every read. Yes you’re correct: this is the code that was querying the ic_device_id [1]: memset(&msg, 0, sizeof(msg)); msg.bus = sensor_config[index].port; msg.target_addr = sensor_config[index].target_addr; msg.tx_len = 1; msg.rx_len = 7; msg.data[0] = PMBUS_IC_DEVICE_ID; if (i2c_master_read(&msg, retry)) { printf("Failed to read VR IC_DEVICE_ID: register(0x%x)\n", PMBUS_IC_DEVICE_ID); return; } By sending a buffer that was way larger than the rx buffer of 7, it was leaving stuff lying around for the next query. I’ll test it out and see what happens if I fix the IC_DEVICE_ID length transmitted by the ISL69259 to 4, maybe we don’t need this patch. But, at the very least, I’ll make sure to gate this on the page value changing, not just being set. Thanks for the review, this was really useful. I’m not very familiar with pmbus devices. [1] https://github.com/facebook/OpenBIC/blob/cda4c00b032b1d9c9b94c45faa2c0eca4886a0a3/meta-facebook/yv35-cl/src/platform/plat_sensor_table.c#L332-L355 > > Titus
> On Jun 29, 2022, at 11:28 AM, Peter Delevoryas <pdel@fb.com> wrote: > > > >> On Jun 29, 2022, at 11:05 AM, Titus Rwantare <titusr@google.com> wrote: >> >> On Tue, 28 Jun 2022 at 20:36, Peter Delevoryas >> <peterdelevoryas@gmail.com> wrote: >>> >>> When a pmbus device switches pages, it should clears its output buffer so >>> that the next transaction doesn't emit data from the previous page. >>> >>> Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”) >>> Signed-off-by: Peter Delevoryas <pdel@fb.com> >>> --- >>> hw/i2c/pmbus_device.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c >>> index 62885fa6a1..efddc36fd9 100644 >>> --- a/hw/i2c/pmbus_device.c >>> +++ b/hw/i2c/pmbus_device.c >>> @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) >>> >>> if (pmdev->code == PMBUS_PAGE) { >>> pmdev->page = pmbus_receive8(pmdev); >>> + pmdev->out_buf_len = 0; >>> return 0; >>> } >>> >> >> I suspect you were running into this because ic_device_id was putting >> too much data in the output buffer. Still, I wouldn't want the buffer >> cleared if the page hasn't changed. Some drivers write the same page >> before every read. > > Yes you’re correct: this is the code that was querying the ic_device_id [1]: > > memset(&msg, 0, sizeof(msg)); > msg.bus = sensor_config[index].port; > msg.target_addr = sensor_config[index].target_addr; > msg.tx_len = 1; > msg.rx_len = 7; > msg.data[0] = PMBUS_IC_DEVICE_ID; > if (i2c_master_read(&msg, retry)) { > printf("Failed to read VR IC_DEVICE_ID: register(0x%x)\n", PMBUS_IC_DEVICE_ID); > return; > } > > By sending a buffer that was way larger than the rx buffer of 7, it was > leaving stuff lying around for the next query. > > I’ll test it out and see what happens if I fix the IC_DEVICE_ID length > transmitted by the ISL69259 to 4, maybe we don’t need this patch. But, > at the very least, I’ll make sure to gate this on the page value changing, > not just being set. Hmmm, actually I’m not going to change this. It seems that our Zephyr code is actually querying one of our devices multiple times, setting the page to zero before each read, and expecting it to return the device ID without any wrapping. If it was only resetting the output buffer on page commands that change the value of the page, then the Zephyr code wouldn’t work. I also added some printing and tested it on some hardware: check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff] endpoint check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff] [00:00:00. check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff] 003,000] heck_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff] m<inf> usb_d check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff] <<<<< c_aspeed: se check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff] lect ep[0x3] check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff] as OUT endp check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff] oint [0 check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff] 0:00:00.059, check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff] <<<<< 000] <in check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff] f> kcs_aspee check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff] d: KCS3: add check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff] r=0xca2, idr check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff] =0x2c, odr=0 check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff] x38, str=0x4 check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff] 4 [00: check_vr_type: i2c4 76 page 01 [04 00 81 d2 49 56 ff] 00:00.059,00 check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff] 0] <e check_vr_type: i2c4 60 page 01 [04 00 81 d2 49 3c ff] Correct me if I’m wrong, but I think the VR at 0x62 is getting queried multiple times, and resetting the page is causing it to go back to the start. Also, here’s an example where I removed the pre-read page-set command and increased the output buffer size to 64: heck_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] 00:00:00.003,000] <inf> usb_dc_asp check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] eed: select ep[0x3] as OUT endpoint heck_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] m [00:00:00.059,000] <inf> kcs_asp check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] eed: KCS3: addr=0xca2, idr=0x2c, odr=0 check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] x38, str=0x44 [00:00:00.060,000] check_vr_type: i2c4 62 page 00 [04 00 81 d2 49 d4 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] <err> spi_nor_multi_dev: [1216 check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] ][spi1_cs0]SFDP magic 00000000 invalid check_vr_type: i2c4 76 page 00 [04 00 81 d2 49 56 ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] [00:00:00.060,000] <err> s check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] pi_nor_multi_dev: [1456]SFDP read faile check_vr_type: i2c4 60 page 00 [04 00 81 d2 49 3c ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff f f ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff] So, actually, it seems like the IC_DEVICE_ID is 6 bytes (I only had 5), and there’s no wrapping behavior. Perhaps I don’t need to add this, and instead I should remove the wrapping behavior for the ISL69259? I’m not sure what the behavior is for other kinds of PMBUS devices or voltage regulators. Thanks, Peter > > Thanks for the review, this was really useful. I’m not very familiar > with pmbus devices. > > [1] https://github.com/facebook/OpenBIC/blob/cda4c00b032b1d9c9b94c45faa2c0eca4886a0a3/meta-facebook/yv35-cl/src/platform/plat_sensor_table.c#L332-L355 > >> >> Titus
diff --git a/hw/i2c/pmbus_device.c b/hw/i2c/pmbus_device.c index 62885fa6a1..efddc36fd9 100644 --- a/hw/i2c/pmbus_device.c +++ b/hw/i2c/pmbus_device.c @@ -1088,6 +1088,7 @@ static int pmbus_write_data(SMBusDevice *smd, uint8_t *buf, uint8_t len) if (pmdev->code == PMBUS_PAGE) { pmdev->page = pmbus_receive8(pmdev); + pmdev->out_buf_len = 0; return 0; }
When a pmbus device switches pages, it should clears its output buffer so that the next transaction doesn't emit data from the previous page. Fixes: 3746d5c15e70570b ("hw/i2c: add support for PMBus”) Signed-off-by: Peter Delevoryas <pdel@fb.com> --- hw/i2c/pmbus_device.c | 1 + 1 file changed, 1 insertion(+)