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@xxxxxxxxxxxxx> 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?
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
