From: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
To: Daniel Palmer <daniel@0x0f.com>
Cc: "open list:MEDIA INPUT INFRASTRUCTURE (V4L/DVB)"
<linux-media@vger.kernel.org>,
Robert Beckett <bob.beckett@collabora.com>,
Mauro Carvalho Chehab <mchehab@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
"open list:STAGING SUBSYSTEM" <linux-staging@lists.linux.dev>,
open list <linux-kernel@vger.kernel.org>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com,
kiril.bicevski@collabora.com,
Nas Chung <nas.chung@chipsnmedia.com>,
lafley.kim@chipsnmedia.com, scott.woo@chipsnmedia.com,
olivier.crete@collabora.com, dan.carpenter@oracle.com,
Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH v4 1/6] staging: media: wave5: Add vpuapi layer
Date: Tue, 14 Dec 2021 09:48:06 +0200 [thread overview]
Message-ID: <e9905774-a994-6311-7b53-b40588d4f6ec@collabora.com> (raw)
In-Reply-To: <cdd9b485-364f-c6bd-776f-a0ca2d260762@collabora.com>
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
>>
>
next prev parent reply other threads:[~2021-12-14 7:48 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-01 17:56 [PATCH v4 0/6] staging: media: wave5: add wave5 codec driver Dafna Hirschfeld
2021-12-01 17:56 ` [PATCH v4 1/6] staging: media: wave5: Add vpuapi layer Dafna Hirschfeld
2021-12-04 13:43 ` Daniel Palmer
2021-12-14 5:33 ` Dafna Hirschfeld
2021-12-14 7:48 ` Dafna Hirschfeld [this message]
2021-12-14 10:20 ` Daniel Palmer
2022-01-21 9:22 ` Dafna Hirschfeld
2022-01-21 9:33 ` Daniel Palmer
[not found] ` <b06402c9-e27e-f6e0-abd5-688106a36ba6@collabora.com>
[not found] ` <CAFr9PXnn5EnApGk7vYcFEF0+J6kojhNTMspNnM8asN512fkTew@mail.gmail.com>
2022-01-27 19:49 ` Dafna Hirschfeld
2022-01-30 7:53 ` Daniel Palmer
2021-12-01 17:56 ` [PATCH v4 2/6] staging: media: wave5: Add the vdi layer Dafna Hirschfeld
2021-12-01 17:56 ` [PATCH v4 3/6] staging: media: wave5: Add the v4l2 layer Dafna Hirschfeld
2021-12-01 17:56 ` [PATCH v4 4/6] staging: media: wave5: Add TODO file Dafna Hirschfeld
2021-12-01 17:56 ` [PATCH v4 5/6] dt-bindings: media: staging: wave5: add yaml devicetree bindings Dafna Hirschfeld
2021-12-01 17:56 ` [PATCH v4 6/6] media: wave5: Add wave5 driver to maintainers file Dafna Hirschfeld
2021-12-02 10:11 ` [PATCH v4 0/6] staging: media: wave5: add wave5 codec driver Daniel Palmer
2021-12-13 20:27 ` Dafna Hirschfeld
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e9905774-a994-6311-7b53-b40588d4f6ec@collabora.com \
--to=dafna.hirschfeld@collabora.com \
--cc=bob.beckett@collabora.com \
--cc=dafna3@gmail.com \
--cc=dan.carpenter@oracle.com \
--cc=daniel@0x0f.com \
--cc=gregkh@linuxfoundation.org \
--cc=hverkuil@xs4all.nl \
--cc=kernel@collabora.com \
--cc=kiril.bicevski@collabora.com \
--cc=lafley.kim@chipsnmedia.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-staging@lists.linux.dev \
--cc=mchehab@kernel.org \
--cc=nas.chung@chipsnmedia.com \
--cc=olivier.crete@collabora.com \
--cc=rdunlap@infradead.org \
--cc=scott.woo@chipsnmedia.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).