| Message ID | 20230918192204.32263-8-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
On 18/09/2023 21:21, Jason-JH.Lin wrote: > CMDQ client can use a loop flag for the CMDQ packet to make current > command buffer jumps to the beginning when GCE executes to the end > of commands buffer. > > GCE irq occurs when GCE executes to the end of command instruction. > If the CMDQ packet is a loopping command, GCE irq handler can not > delete the CMDQ task and disable the GCE thread. > > Add cmdq_mbox_stop to support thread disable How or where do you add it? I do not see it in this patch. Your patchset looks randomly organized. Best regards, Krzysztof
Hi Krzysztof, Thanks for the reviews. On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 18/09/2023 21:21, Jason-JH.Lin wrote: > > CMDQ client can use a loop flag for the CMDQ packet to make current > > command buffer jumps to the beginning when GCE executes to the end > > of commands buffer. > > > > GCE irq occurs when GCE executes to the end of command instruction. > > If the CMDQ packet is a loopping command, GCE irq handler can not > > delete the CMDQ task and disable the GCE thread. > > > > Add cmdq_mbox_stop to support thread disable > > How or where do you add it? I do not see it in this patch. Your > patchset > looks randomly organized. This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the same maintainer's tree, so I separate this to another patch from [PATCH 8/15]. Regards, Jason-JH.Lin > > Best regards, > Krzysztof > >
On 25/09/2023 07:21, Jason-JH Lin (林睿祥) wrote: > Hi Krzysztof, > > Thanks for the reviews. > > On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: >> >> External email : Please do not click links or open attachments until >> you have verified the sender or the content. >> On 18/09/2023 21:21, Jason-JH.Lin wrote: >>> CMDQ client can use a loop flag for the CMDQ packet to make current >>> command buffer jumps to the beginning when GCE executes to the end >>> of commands buffer. >>> >>> GCE irq occurs when GCE executes to the end of command instruction. >>> If the CMDQ packet is a loopping command, GCE irq handler can not >>> delete the CMDQ task and disable the GCE thread. >>> >>> Add cmdq_mbox_stop to support thread disable >> >> How or where do you add it? I do not see it in this patch. Your >> patchset >> looks randomly organized. > > This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. > > mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the > same maintainer's tree, so I separate this to another patch from [PATCH > 8/15]. Why? Anyway it has to go through same tree. You have dependencies. Such artificial split makes it only difficult to review and understand. Re-organize your patchset to be correctly split per each logical feature/change. Split per subsystems is not the same. Best regards, Krzysztof
On Mon, 2023-09-25 at 08:44 +0200, Krzysztof Kozlowski wrote: > > External email : Please do not click links or open attachments until > you have verified the sender or the content. > On 25/09/2023 07:21, Jason-JH Lin (林睿祥) wrote: > > Hi Krzysztof, > > > > Thanks for the reviews. > > > > On Sat, 2023-09-23 at 20:03 +0200, Krzysztof Kozlowski wrote: > >> > >> External email : Please do not click links or open attachments > until > >> you have verified the sender or the content. > >> On 18/09/2023 21:21, Jason-JH.Lin wrote: > >>> CMDQ client can use a loop flag for the CMDQ packet to make > current > >>> command buffer jumps to the beginning when GCE executes to the > end > >>> of commands buffer. > >>> > >>> GCE irq occurs when GCE executes to the end of command > instruction. > >>> If the CMDQ packet is a loopping command, GCE irq handler can not > >>> delete the CMDQ task and disable the GCE thread. > >>> > >>> Add cmdq_mbox_stop to support thread disable > >> > >> How or where do you add it? I do not see it in this patch. Your > >> patchset > >> looks randomly organized. > > > > This will be used in cmdq_pkt_finialize_loop() at [PATCH 8/15]. > > > > mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the > > same maintainer's tree, so I separate this to another patch from > [PATCH > > 8/15]. > > Why? Anyway it has to go through same tree. You have dependencies. > Such > artificial split makes it only difficult to review and understand. > Re-organize your patchset to be correctly split per each logical > feature/change. Split per subsystems is not the same. > Yes, these related files are in the different maintainer's tree. Refer to https://www.kernel.org/doc/linux/MAINTAINERS MAILBOX API M: Jassi Brar F: drivers/mailbox/ - drivers/mailbox/mtk-cmdq-mailbox.c - drivers/mailbox/mtk-cmdq-sec- mailbox.c ARM/Mediatek SoC support M: Matthias Brugger F: drivers/soc/mediatek/ K: mediatek - drivers/soc/mediatek/mtk-cmdq-helper.c - include/linux/soc/mediatek/mtk-cmdq.h I think we should add a new MAINTAINER label for mediatek CMDQ mailbox and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM IPCC MAILBOX DRIVER". But I don't know how to make a request for that. Anyway, I'll squash this logical feature to the same patch, no matter these files are not in the same tree. Regards, Jason-JH.Lin > Best regards, > Krzysztof >
On 26/09/2023 05:20, Jason-JH Lin (林睿祥) wrote: mdq_pkt_finialize_loop() at [PATCH 8/15]. >>> >>> mtk-cmdq-helper.c and mtk-cmdq-mailbox.c are not in the >>> same maintainer's tree, so I separate this to another patch from >> [PATCH >>> 8/15]. >> >> Why? Anyway it has to go through same tree. You have dependencies. >> Such >> artificial split makes it only difficult to review and understand. >> Re-organize your patchset to be correctly split per each logical >> feature/change. Split per subsystems is not the same. >> > > Yes, these related files are in the different maintainer's tree. > Refer to https://www.kernel.org/doc/linux/MAINTAINERS > > MAILBOX API > M: Jassi Brar > F: drivers/mailbox/ > - drivers/mailbox/mtk-cmdq-mailbox.c > - drivers/mailbox/mtk-cmdq-sec- > mailbox.c > > ARM/Mediatek SoC support > M: Matthias Brugger > F: drivers/soc/mediatek/ > K: mediatek > - drivers/soc/mediatek/mtk-cmdq-helper.c > - > include/linux/soc/mediatek/mtk-cmdq.h > > I think we should add a new MAINTAINER label for mediatek CMDQ mailbox > and put these files together, such as "MAILBOX ARM MHUv2" and "QUALCOM > IPCC MAILBOX DRIVER". Why? It's not related to the topic of splitting patchset into patches. There is no problem of patchsets touching multiple subsystems. We already solved this problem many years ago... > But I don't know how to make a request for that. Anyway, you would not be a maintainer taking patches, just a reviewer called "M:" here... > > Anyway, I'll squash this logical feature to the same patch, no matter > these files are not in the same tree. > Best regards, Krzysztof
diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c index 8bd39fecbf00..a3b831b6bab9 100644 --- a/drivers/mailbox/mtk-cmdq-mailbox.c +++ b/drivers/mailbox/mtk-cmdq-mailbox.c @@ -264,6 +264,17 @@ static void cmdq_thread_irq_handler(struct cmdq *cmdq, curr_pa = readl(thread->base + CMDQ_THR_CURR_ADDR) << cmdq->pdata->shift; + task = list_first_entry_or_null(&thread->task_busy_list, + struct cmdq_task, list_entry); + if (task && task->pkt->loop) { + struct cmdq_cb_data data; + + data.sta = err; + data.pkt = task->pkt; + mbox_chan_received_data(task->thread->chan, &data); + return; + } + list_for_each_entry_safe(task, tmp, &thread->task_busy_list, list_entry) { task_end_pa = task->pa_base + task->pkt->cmd_buf_size; diff --git a/include/linux/mailbox/mtk-cmdq-mailbox.h b/include/linux/mailbox/mtk-cmdq-mailbox.h index f3e577335acb..fc663b994b7a 100644 --- a/include/linux/mailbox/mtk-cmdq-mailbox.h +++ b/include/linux/mailbox/mtk-cmdq-mailbox.h @@ -76,6 +76,7 @@ struct cmdq_pkt { size_t cmd_buf_size; /* command occupied size */ size_t buf_size; /* real buffer size */ void *cl; + bool loop; }; u8 cmdq_get_shift_pa(struct mbox_chan *chan);
CMDQ client can use a loop flag for the CMDQ packet to make current command buffer jumps to the beginning when GCE executes to the end of commands buffer. GCE irq occurs when GCE executes to the end of command instruction. If the CMDQ packet is a loopping command, GCE irq handler can not delete the CMDQ task and disable the GCE thread. Add cmdq_mbox_stop to support thread disable Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> --- drivers/mailbox/mtk-cmdq-mailbox.c | 11 +++++++++++ include/linux/mailbox/mtk-cmdq-mailbox.h | 1 + 2 files changed, 12 insertions(+)