Messages in this thread | | | Subject | Re: [PATCH v4 1/6] staging: media: wave5: Add vpuapi layer | From | Dafna Hirschfeld <> | Date | Tue, 14 Dec 2021 09:48:06 +0200 |
| |
On 14.12.21 07:33, Dafna Hirschfeld wrote: > > > On 04.12.21 15:43, Daniel Palmer wrote: >> Hi Dafna, >> >> Sorry for the piecemeal emails.. >> >> On Thu, 2 Dec 2021 at 02:56, Dafna Hirschfeld >> <dafna.hirschfeld@collabora.com> wrote: >>> diff --git a/drivers/staging/media/wave5/wave5-hw.c b/drivers/staging/media/wave5/wave5-hw.c >> >> ... snip ... >> >>> +static int wave5_wait_bus_busy(struct vpu_device *vpu_dev, int timeout, unsigned int addr) >>> +{ >>> + u32 gdi_status_check_value = 0x3f; >>> + u32 data; >>> + >>> + if (vpu_dev->product_code == WAVE521C_CODE || >>> + vpu_dev->product_code == WAVE521_CODE || >>> + vpu_dev->product_code == WAVE521E1_CODE) >>> + gdi_status_check_value = 0x00ff1f3f; >>> + >>> + return read_poll_timeout(wave5_vdi_read_register, data, data == gdi_status_check_value, >>> + 0, timeout * 1000, false, vpu_dev, addr); >>> +} >>> + >> >> This looks like it should be s/wave5_vdi_read_register/wave5_read_register/. >> For wave511 addr passed in here is 0x8e14 so well outside of what is >> directly accessible. > > Hi, I didn't understand this explanation. I see that > wave5_read_register eventually calls 'wave5_vdi_read_register'. > Could you please explain in more detail why you think > calling wave5_vdi_read_register is wrong?
hi, I see know that those backbone address are indeed not read and written directly but the address should be first written to a regsiter W5_VPU_FIO_CTRL_ADDR, and then the content is returned from W5_VPU_FIO_DATA.
I'll fix it on v5
Thanks, Dafna
> > Actually the name 'wave5_read_register' is a bad name for that > func since it eventually return the value of the W5_VPU_FIO_DATA > register upon success and not the address sent to it. > > >> >> Also it seems that this can either return 0 or -ETIMEDOUT... >> >> ... snip ... >> >>> +int wave5_vpu_reset(struct device *dev, enum sw_reset_mode reset_mode) >>> +{ >>> + u32 val = 0; >>> + int ret = 0; >>> + struct vpu_device *vpu_dev = dev_get_drvdata(dev); >>> + struct vpu_attr *p_attr = &vpu_dev->attr; >>> + // VPU doesn't send response. force to set BUSY flag to 0. >>> + vpu_write_reg(vpu_dev, W5_VPU_BUSY_STATUS, 0); >>> + >>> + if (reset_mode == SW_RESET_SAFETY) { >>> + ret = wave5_vpu_sleep_wake(dev, true, NULL, 0); >>> + if (ret) >>> + return ret; >>> + } >>> + >>> + val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG0); >>> + if ((val >> 16) & 0x1) >>> + p_attr->support_backbone = true; >>> + if ((val >> 22) & 0x1) >>> + p_attr->support_vcore_backbone = true; >>> + if ((val >> 28) & 0x1) >>> + p_attr->support_vcpu_backbone = true; >>> + >>> + val = vpu_read_reg(vpu_dev, W5_VPU_RET_VPU_CONFIG1); >>> + if ((val >> 26) & 0x1) >>> + p_attr->support_dual_core = true; >>> + >>> + // waiting for completion of bus transaction >>> + if (p_attr->support_backbone) { >>> + if (p_attr->support_dual_core) { >>> + // check CORE0 >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE0, 0x7); >>> + >>> + ret = wave5_wait_bus_busy(vpu_dev, VPU_BUSY_CHECK_TIMEOUT, >>> + W5_BACKBONE_BUS_STATUS_VCORE0); >>> + if (ret) { >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE0, 0x00); >>> + return ret; >>> + } >>> + >>> + // check CORE1 >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE1, 0x7); >>> + >>> + ret = wave5_wait_bus_busy(vpu_dev, VPU_BUSY_CHECK_TIMEOUT, >>> + W5_BACKBONE_BUS_STATUS_VCORE1); >>> + if (ret) { >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE1, 0x00); >>> + return ret; >>> + } >>> + >>> + } else if (p_attr->support_vcore_backbone) { >>> + if (p_attr->support_vcpu_backbone) { >>> + // step1 : disable request >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCPU, >>> + 0xFF); >>> + >>> + // step2 : waiting for completion of bus transaction >>> + ret = wave5_wait_vcpu_bus_busy(vpu_dev, VPU_BUSY_CHECK_TIMEOUT, >>> + W5_BACKBONE_BUS_STATUS_VCPU); >>> + if (ret) { >>> + wave5_write_register(vpu_dev, >>> + W5_BACKBONE_BUS_CTRL_VCPU, 0x00); >>> + return ret; >>> + } >>> + } >>> + // step1 : disable request >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE0, 0x7); >>> + >>> + // step2 : waiting for completion of bus transaction >>> + if (wave5_wait_bus_busy(vpu_dev, VPU_BUSY_CHECK_TIMEOUT, >>> + W5_BACKBONE_BUS_STATUS_VCORE0) == -1) { >>> + wave5_write_register(vpu_dev, W5_BACKBONE_BUS_CTRL_VCORE0, 0x00); >>> + return -EBUSY; >>> + } >> >> but this is looking for -1 on failure. > > right, thanks for finding this, I see that wave5_read_register return -1 on failure so maybe > this is the source of the confusion. > > Thanks, > Dafna > >> >>> + } else { >>> + // step1 : disable request >>> + wave5_write_register(vpu_dev, W5_COMBINED_BACKBONE_BUS_CTRL, 0x7); >>> + >>> + // step2 : waiting for completion of bus transaction >>> + if (wave5_wait_bus_busy(vpu_dev, VPU_BUSY_CHECK_TIMEOUT, >>> + W5_COMBINED_BACKBONE_BUS_STATUS) == -1) { >>> + wave5_write_register(vpu_dev, W5_COMBINED_BACKBONE_BUS_CTRL, 0x00); >>> + return -EBUSY; >>> + } >>> + } >> >> Here too. >> >> Cheers, >> >> Daniel >> >
| |