Sakari and Stanislaw,
On 11/1/24 3:46 PM, Sakari Ailus wrote:
> Hi Bingbu, Stanislaw,
>
> On Fri, Nov 01, 2024 at 12:26:52PM +0800, Bingbu Cao wrote:
>>
>> On 11/1/24 12:22 PM, Bingbu Cao wrote:
>>> Stanislaw,
>>>
>>> Thanks for the patch, this is what I intended to do.
>>>
>>> On 10/31/24 9:06 PM, Stanislaw Gruszka wrote:
>>>> The buttress ipc ish structure is not effectively used on IPU6 - data
>>>> is nullified on init. Remove it to cleanup the code a bit.
>>>>
>>>> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@xxxxxxxxxxxxxxx>
>>>> ---
>>>> v2: fix formatting: use media: prefix in topic and white space alignment
>>>> to match open parenthesis
>>>>
>>>> drivers/media/pci/intel/ipu6/ipu6-buttress.c | 27 ++++++--------------
>>>> drivers/media/pci/intel/ipu6/ipu6-buttress.h | 6 -----
>>>> 2 files changed, 8 insertions(+), 25 deletions(-)
>>>>
>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.c b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> index edaa285283a1..6644fd4c3d91 100644
>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.c
>>>> @@ -214,20 +214,17 @@ static void ipu6_buttress_ipc_recv(struct ipu6_device *isp,
>>>> }
>>>>
>>>> static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>>>> - enum ipu6_buttress_ipc_domain ipc_domain,
>>>> struct ipu6_ipc_buttress_bulk_msg *msgs,
>>>> u32 size)
>>>> {
>>>> unsigned long tx_timeout_jiffies, rx_timeout_jiffies;
>>>> unsigned int i, retry = BUTTRESS_IPC_CMD_SEND_RETRY;
>>>> struct ipu6_buttress *b = &isp->buttress;
>>>> - struct ipu6_buttress_ipc *ipc;
>>>> + struct ipu6_buttress_ipc *ipc = &b->cse;
>>>> u32 val;
>>>> int ret;
>>>> int tout;
>>>>
>>>> - ipc = ipc_domain == IPU6_BUTTRESS_IPC_CSE ? &b->cse : &b->ish;
>>>> -
>>>> mutex_lock(&b->ipc_mutex);
>>>>
>>>> ret = ipu6_buttress_ipc_validity_open(isp, ipc);
>>>> @@ -305,7 +302,6 @@ static int ipu6_buttress_ipc_send_bulk(struct ipu6_device *isp,
>>>>
>>>> static int
>>>> ipu6_buttress_ipc_send(struct ipu6_device *isp,
>>>> - enum ipu6_buttress_ipc_domain ipc_domain,
>>>> u32 ipc_msg, u32 size, bool require_resp,
>>>> u32 expected_resp)
>>>> {
>>>> @@ -316,7 +312,7 @@ ipu6_buttress_ipc_send(struct ipu6_device *isp,
>>>> .expected_resp = expected_resp,
>>>> };
>>>>
>>>> - return ipu6_buttress_ipc_send_bulk(isp, ipc_domain, &msg, 1);
>>>> + return ipu6_buttress_ipc_send_bulk(isp, &msg, 1);
>>>> }
>>>>
>>>> static irqreturn_t ipu6_buttress_call_isr(struct ipu6_bus_device *adev)
>>>> @@ -386,10 +382,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING) {
>>>> - dev_dbg(&isp->pdev->dev,
>>>> - "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>> - ipu6_buttress_ipc_recv(isp, &b->ish, &b->ish.recv_data);
>>>> - complete(&b->ish.recv_complete);
>>>> + dev_warn(&isp->pdev->dev,
>>>> + "BUTTRESS_ISR_IPC_FROM_ISH_IS_WAITING\n");
>>>
>>> I think this is a unrelated change, right?
>>
>> I mean the change from dev_dbg() to dev_warn().
>
> We're not handling these interrupts anymore in any way.
>
> I wonder if the ipu6_buttress_ipc_recv() call should still remain in place,
> even if we really do nothing with these. It looks like some kind of an
> acknowledgement mechanism.
I just confirm that IPC_FROM_ISH_IS_WAITING and IPC_EXEC_DONE_BY_ISH are
not valid anymore from IPU6, I think the handling here and below could be
removed.
>
>>>
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE) {
>>>> @@ -399,9 +393,8 @@ irqreturn_t ipu6_buttress_isr(int irq, void *isp_ptr)
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_IPC_EXEC_DONE_BY_ISH) {
>>>> - dev_dbg(&isp->pdev->dev,
>>>> - "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>>>> - complete(&b->ish.send_complete);
>>>> + dev_warn(&isp->pdev->dev,
>>>> + "BUTTRESS_ISR_IPC_EXEC_DONE_BY_CSE\n");
>>>
>>> Ditto.
>>>
>>>> }
>>>>
>>>> if (irq_status & BUTTRESS_ISR_SAI_VIOLATION &&
>>>> @@ -655,7 +648,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>>>> */
>>>> dev_info(&isp->pdev->dev, "Sending BOOT_LOAD to CSE\n");
>>>>
>>>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>>>> + ret = ipu6_buttress_ipc_send(isp,
>>>> BUTTRESS_IU2CSEDATA0_IPC_BOOT_LOAD,
>>>> 1, true,
>>>> BUTTRESS_CSE2IUDATA0_IPC_BOOT_LOAD_DONE);
>>>> @@ -697,7 +690,7 @@ int ipu6_buttress_authenticate(struct ipu6_device *isp)
>>>> * IU2CSEDB.IU2CSECMD and set IU2CSEDB.IU2CSEBUSY as
>>>> */
>>>> dev_info(&isp->pdev->dev, "Sending AUTHENTICATE_RUN to CSE\n");
>>>> - ret = ipu6_buttress_ipc_send(isp, IPU6_BUTTRESS_IPC_CSE,
>>>> + ret = ipu6_buttress_ipc_send(isp,
>>>> BUTTRESS_IU2CSEDATA0_IPC_AUTH_RUN,
>>>> 1, true,
>>>> BUTTRESS_CSE2IUDATA0_IPC_AUTH_RUN_DONE);
>>>> @@ -838,9 +831,7 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>>>> mutex_init(&b->auth_mutex);
>>>> mutex_init(&b->cons_mutex);
>>>> mutex_init(&b->ipc_mutex);
>>>> - init_completion(&b->ish.send_complete);
>>>> init_completion(&b->cse.send_complete);
>>>> - init_completion(&b->ish.recv_complete);
>>>> init_completion(&b->cse.recv_complete);
>>>>
>>>> b->cse.nack = BUTTRESS_CSE2IUDATA0_IPC_NACK;
>>>> @@ -852,8 +843,6 @@ int ipu6_buttress_init(struct ipu6_device *isp)
>>>> b->cse.data0_in = BUTTRESS_REG_CSE2IUDATA0;
>>>> b->cse.data0_out = BUTTRESS_REG_IU2CSEDATA0;
>>>>
>>>> - /* no ISH on IPU6 */
>>>> - memset(&b->ish, 0, sizeof(b->ish));
>>>> INIT_LIST_HEAD(&b->constraints);
>>>>
>>>> isp->secure_mode = ipu6_buttress_get_secure_mode(isp);
>>>> diff --git a/drivers/media/pci/intel/ipu6/ipu6-buttress.h b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> index 9b6f56958be7..482978c2a09d 100644
>>>> --- a/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> +++ b/drivers/media/pci/intel/ipu6/ipu6-buttress.h
>>>> @@ -46,18 +46,12 @@ struct ipu6_buttress_ipc {
>>>> struct ipu6_buttress {
>>>> struct mutex power_mutex, auth_mutex, cons_mutex, ipc_mutex;
>>>> struct ipu6_buttress_ipc cse;
>>>> - struct ipu6_buttress_ipc ish;
>>>> struct list_head constraints;
>>>> u32 wdt_cached_value;
>>>> bool force_suspend;
>>>> u32 ref_clk;
>>>> };
>>>>
>>>> -enum ipu6_buttress_ipc_domain {
>>>> - IPU6_BUTTRESS_IPC_CSE,
>>>> - IPU6_BUTTRESS_IPC_ISH,
>>>> -};
>>>> -
>>>> struct ipu6_ipc_buttress_bulk_msg {
>>>> u32 cmd;
>>>> u32 expected_resp;
>>>>
>>>
>>> Besides minor comment:
>>>
>>> Reviewed-by: Bingbu Cao <bingbu.cao@xxxxxxxxx>
>>>
>>
>
--
Best regards,
Bingbu Cao
