| Message ID | 20230918192204.32263-1-jason-jh.lin@mediatek.com |
|---|---|
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
On Tue, Sep 19, 2023 at 03:21:50AM +0800, Jason-JH.Lin wrote: > Add mboxes to define a GCE loopping thread as a secure irq handler. > Add mediatek,event to define a GCE software event siganl as a secure > irq. > > These 2 properties are required for CMDQ secure driver. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > .../mailbox/mediatek,gce-mailbox.yaml | 30 +++++++++++++++---- > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > index cef9d7601398..5c9aebe83d2d 100644 > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > @@ -49,6 +49,21 @@ properties: > items: > - const: gce > > + mboxes: > + description: > + A mailbox channel used as a secure irq handler in normal world. > + Using mailbox to communicate with GCE to setup looping thread, > + it should have this property and a phandle, mailbox specifiers. All cases of 'mboxes' have a phandle and specifiers. No need to repeat that here. > + $ref: /schemas/types.yaml#/definitions/phandle-array Already has a type definition too. You need to define how many entries and what each entry is if more than one. IOW, the same thing as clocks, resets, interrupts, etc. > + > + mediatek,gce-events: This is used all over. It really needs a single definition which is then referenced by the users. > + description: > + The event id which is mapping to a software event signal to gce. > + It is used as a secure irq for every secure gce threads. > + The event id is defined in the gce header > + include/dt-bindings/mailbox/mediatek,<chip>-gce.h of each chips. > + $ref: /schemas/types.yaml#/definitions/uint32-array > + > required: > - compatible > - "#mbox-cells" > @@ -71,20 +86,23 @@ additionalProperties: false > > examples: > - | > - #include <dt-bindings/clock/mt8173-clk.h> > + #include <dt-bindings/clock/mediatek,mt8188-clk.h> > #include <dt-bindings/interrupt-controller/arm-gic.h> > #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/mailbox/mediatek,mt8188-gce.h> > > soc { > #address-cells = <2>; > #size-cells = <2>; > > - gce: mailbox@10212000 { > - compatible = "mediatek,mt8173-gce"; > - reg = <0 0x10212000 0 0x1000>; > - interrupts = <GIC_SPI 135 IRQ_TYPE_LEVEL_LOW>; > + gce0: mailbox@10320000 { > + compatible = "mediatek,mt8188-gce"; Are these new properties only for mt8188? If so, then you need a schema saying that. If not, then this is an unnecessary change to the example. > + reg = <0 0x10320000 0 0x4000>; > + interrupts = <GIC_SPI 226 IRQ_TYPE_LEVEL_HIGH 0>; > #mbox-cells = <2>; > - clocks = <&infracfg CLK_INFRA_GCE>; > + clocks = <&infracfg_ao CLK_INFRA_AO_GCE>; > clock-names = "gce"; > + mboxes = <&gce0 15 CMDQ_THR_PRIO_1>; The provider is also a consumer? > + mediatek,gce-events = <CMDQ_SYNC_TOKEN_SECURE_THR_EOF>; > }; > }; > -- > 2.18.0 >
On 18/09/2023 21:21, Jason-JH.Lin wrote: > CMDQ_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify CMDQ > driver in the normal world that GCE secure thread has completed a task > in thee secure world. How can #define be added after its usage? Does it even make any sense of being separate patch? Do you folks in Mediatek have any internal review before posting? Best regards, Krzysztof
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
On 18/09/2023 21:22, Jason-JH.Lin wrote: > Add mt8195 support for CMDQ secure driver. How is it anyhow related to your patch content? > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > --- > drivers/mailbox/mtk-cmdq-mailbox.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mailbox/mtk-cmdq-mailbox.c b/drivers/mailbox/mtk-cmdq-mailbox.c > index 4e047dc916b9..d27d033c587d 100644 > --- a/drivers/mailbox/mtk-cmdq-mailbox.c > +++ b/drivers/mailbox/mtk-cmdq-mailbox.c > @@ -735,6 +735,7 @@ static const struct gce_plat gce_plat_v6 = { > .thread_nr = 24, > .shift = 3, > .control_by_sw = true, > + .has_sec = true, Really, how? 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 Mon, 2023-09-25 at 08:42 +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:05, Jason-JH Lin (林睿祥) wrote: > > Hi Krzysztof, > > > > Thanks for the reviews. > > > > On Sat, 2023-09-23 at 20:01 +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_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify > CMDQ > >>> driver in the normal world that GCE secure thread has completed a > >> task > >>> in thee secure world. > >> > >> How can #define be added after its usage? Does it even make any > sense > >> of > >> being separate patch? > >> > > > > This definition is used in the mt8195.dts at [PATCH 15/15] and the > CMDQ > > No, the define is used in previous patch, which means your patchset > is > not bisectable and not tested. > Do you mean this patch should add before patch 1? The example of dts in patch 1 is used the definition of mt8188, so I think I can add this patch to define the gce event id for mt8195 after patch 1. I will swap the patch 1 and the patch 2 in the next version, if that can make it more appropriate. Regards, Jason-JH.Lin > Best regards, > Krzysztof > >
On Mon, 2023-09-25 at 11:28 +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 11:11, Jason-JH Lin (林睿祥) wrote: > > On Mon, 2023-09-25 at 08:42 +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:05, Jason-JH Lin (林睿祥) wrote: > >>> Hi Krzysztof, > >>> > >>> Thanks for the reviews. > >>> > >>> On Sat, 2023-09-23 at 20:01 +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_SYNC_TOKEN_SECURE_THR_EOF is used as secure irq to notify > >> CMDQ > >>>>> driver in the normal world that GCE secure thread has completed > a > >>>> task > >>>>> in thee secure world. > >>>> > >>>> How can #define be added after its usage? Does it even make any > >> sense > >>>> of > >>>> being separate patch? > >>>> > >>> > >>> This definition is used in the mt8195.dts at [PATCH 15/15] and > the > >> CMDQ > >> > >> No, the define is used in previous patch, which means your > patchset > >> is > >> not bisectable and not tested. > >> > > > > Do you mean this patch should add before patch 1? > > > > > > The example of dts in patch 1 is used the definition of mt8188, so > I > > think I can add this patch to define the gce event id for mt8195 > after > > patch 1. > > > > I will swap the patch 1 and the patch 2 in the next version, if > that > > can make it more appropriate. > > Really, test your patches. Each of them individually. Appropriateness > is > one thing, but broken bisectability is an error. > > Anyway, the patch is logically part of binding adding this, so it > should > be squashed. Don't create some weird patch ordering where every > define > or every function is in its own patch. I commented about it multiple > times in this patchset. > OK, I'll re-order the definition before the patch using it or squash the definition to the patch using it. 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