| Message ID | 20240526144443.14345-3-jason-jh.lin@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add CMDQ secure driver for SVP | expand |
On Sun, 26 May 2024 22:44:37 +0800, Jason-JH.Lin wrote: > 1. Add mboxes property to define a GCE loopping thread as a secure IRQ > handler. > The CMDQ secure driver requests a mbox channel and sends a looping > command to the GCE thread. The looping command will wait for a secure > packet done event signal from secure world and then jump back to the > first instuction. Each time it waits for an event, it notifies the > CMDQ driver to perform the same action as the IRQ handler. > > 2. Add gce-events property from gce-props.yaml to define a > secure packet done signal in secure world. > There are 1024 events IDs for GCE to use to execute instructions in > the specific event happened. These events could be signaled by HW or SW > and their value would be different in different SoC because of HW event > IDs distribution range from 0 to 1023. > If we set a static event ID: 855 for mt8188, it might be conflict the > event ID original set in mt8195. > So we define an event ID that will be set when GCE runs to the end of > secure cmdq packet in the secure world. > > This can reduce the latency of software communication between normal > world and secure world. In addition, we can also remove the complex > logic after the secure packet done in the secure world. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > .../devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > My bot found errors running 'make dt_binding_check' on your patch: yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.example.dtb: mailbox@10212000: False schema does not allow {'compatible': ['mediatek,mt8173-gce'], 'reg': [[0, 270606336, 0, 4096]], 'interrupts': [[0, 135, 8]], '#mbox-cells': [[2]], 'clocks': [[4294967295, 4]], 'clock-names': ['gce'], '$nodename': ['mailbox@10212000']} from schema $id: http://devicetree.org/schemas/mailbox/mediatek,gce-mailbox.yaml# doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240526144443.14345-3-jason-jh.lin@mediatek.com The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Hi Rob, [snip] > My bot found errors running 'make dt_binding_check' on your patch: > > yamllint warnings/errors: > > dtschema/dtc warnings/errors: > /builds/robherring/dt-review- > ci/linux/Documentation/devicetree/bindings/mailbox/mediatek,gce- > mailbox.example.dtb: mailbox@10212000: False schema does not allow > {'compatible': ['mediatek,mt8173-gce'], 'reg': [[0, 270606336, 0, > 4096]], 'interrupts': [[0, 135, 8]], '#mbox-cells': [[2]], 'clocks': > [[4294967295, 4]], 'clock-names': ['gce'], '$nodename': [ > 'mailbox@10212000']} > from schema $id: > http://devicetree.org/schemas/mailbox/mediatek,gce-mailbox.yaml# > > doc reference errors (make refcheckdocs): > I have run 'make dt_binding_check' with this series [1] "Add mediatek,gce-props.yaml for other bindings reference" - https://patchwork.kernel.org/project/linux-mediatek/list/?series=819298 in my environment and I didn't see this error. > See > https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20240526144443.14345-3-jason-jh.lin@mediatek.com > > The base for the series is generally the latest rc1. A different > dependency > should be noted in *this* patch. > Event though I send the patch based on the series[1], I think the robot still won't know what patches need to be based. Should I need to add "some note" and re-submit patch to make the robot won't get this error? Regards, Jason-JH.Lin
On Sun, May 26, 2024 at 10:44:37PM +0800, Jason-JH.Lin wrote: > 1. Add mboxes property to define a GCE loopping thread as a secure IRQ > handler. > The CMDQ secure driver requests a mbox channel and sends a looping > command to the GCE thread. The looping command will wait for a secure > packet done event signal from secure world and then jump back to the > first instuction. Each time it waits for an event, it notifies the > CMDQ driver to perform the same action as the IRQ handler. > > 2. Add gce-events property from gce-props.yaml to define a > secure packet done signal in secure world. > There are 1024 events IDs for GCE to use to execute instructions in > the specific event happened. These events could be signaled by HW or SW > and their value would be different in different SoC because of HW event > IDs distribution range from 0 to 1023. > If we set a static event ID: 855 for mt8188, it might be conflict the > event ID original set in mt8195. Two different SoCs, two different compatibles, no problem. I'm almost certain you previously told me that the firmware changing could result in a different event ID, but I see no mention of that here. The commit messages makes it seem like this can be determined by the compatible, so either give me a commit message that explains why the compatible is not sufficient or drop the patch. > So we define an event ID that will be set when GCE runs to the end of > secure cmdq packet in the secure world. > > This can reduce the latency of software communication between normal > world and secure world. In addition, we can also remove the complex > logic after the secure packet done in the secure world. > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > --- > .../devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > index cef9d7601398..6e5e848d61d9 100644 > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml > @@ -49,6 +49,10 @@ properties: > items: > - const: gce > > + mboxes: > + items: > + - description: GCE looping thread as a secure IRQ handler Why is this needed? It's going to be a reference to itself, right? Why can't you just reserve a channel in the driver? Thanks, Conor. > + > required: > - compatible > - "#mbox-cells" > @@ -57,6 +61,8 @@ required: > - clocks > > allOf: > + - $ref: /schemas/mailbox/mediatek,gce-props.yaml# > + > - if: > not: > properties: > @@ -67,7 +73,7 @@ allOf: > required: > - clock-names > > -additionalProperties: false > +unevaluatedProperties: false > > examples: > - | > -- > 2.18.0 >
Hi Conor, On Mon, 2024-05-27 at 18:36 +0100, Conor Dooley wrote: > On Sun, May 26, 2024 at 10:44:37PM +0800, Jason-JH.Lin wrote: > > 1. Add mboxes property to define a GCE loopping thread as a secure > > IRQ > > handler. > > The CMDQ secure driver requests a mbox channel and sends a looping > > command to the GCE thread. The looping command will wait for a > > secure > > packet done event signal from secure world and then jump back to > > the > > first instuction. Each time it waits for an event, it notifies the > > CMDQ driver to perform the same action as the IRQ handler. > > > > 2. Add gce-events property from gce-props.yaml to define a > > secure packet done signal in secure world. > > There are 1024 events IDs for GCE to use to execute instructions in > > the specific event happened. These events could be signaled by HW > > or SW > > and their value would be different in different SoC because of HW > > event > > IDs distribution range from 0 to 1023. > > If we set a static event ID: 855 for mt8188, it might be conflict > > the > > event ID original set in mt8195. > > Two different SoCs, two different compatibles, no problem. > I'm almost certain you previously told me that the firmware changing > could result in a different event ID, but I see no mention of that > here. Yes, it could be, but we don't use it that way. > The commit messages makes it seem like this can be determined by the > compatible, so either give me a commit message that explains why the > compatible is not sufficient or drop the patch. > Yes, this can be determined by compatible in CMDQ mailbox driver, so I think it's possible to put this in the CMDQ mailbox driver data and configure by different SoC. The problem is these events are defined in include/dt- bindings/mailbox/mediatek,mt8188-gce.h and include/dt- bindings/gce/mt8195-gce.h. I can only use them in their mt8188.dts or mt8195.dts. If I want to use the same event define in 2 different headers in the same CMDQ mailbox driver, I think I just can parse their dts to get the corresponding one. Do you know how to generally deal with this problem? Or I can just use the number of event ID in driver data without the event define in dt-bindings header. > > So we define an event ID that will be set when GCE runs to the end > > of > > secure cmdq packet in the secure world. > > > > This can reduce the latency of software communication between > > normal > > world and secure world. In addition, we can also remove the complex > > logic after the secure packet done in the secure world. > > > > Signed-off-by: Jason-JH.Lin <jason-jh.lin@mediatek.com> > > Signed-off-by: Hsiao Chien Sung <shawn.sung@mediatek.com> > > --- > > .../devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml | 8 > > +++++++- > > 1 file changed, 7 insertions(+), 1 deletion(-) > > > > diff --git > > a/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > mailbox.yaml > > b/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > mailbox.yaml > > index cef9d7601398..6e5e848d61d9 100644 > > --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > mailbox.yaml > > +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce- > > mailbox.yaml > > @@ -49,6 +49,10 @@ properties: > > items: > > - const: gce > > > > + mboxes: > > + items: > > + - description: GCE looping thread as a secure IRQ handler > > Why is this needed? It's going to be a reference to itself, right? > Why can't you just reserve a channel in the driver? > I think you're right. CMDQ mailbox driver can reserved a channel itself and it will know which channel has occupied. CMDQ users will request fail if they want to use the reserved channel. I'll drop this and move it into CMDQ mailbox driver data for mt8188 and mt8195. Regards, Jason-JH.Lin > Thanks, > Conor. > > > + > > required: > > - compatible > > - "#mbox-cells" > > @@ -57,6 +61,8 @@ required: > > - clocks > > > > allOf: > > + - $ref: /schemas/mailbox/mediatek,gce-props.yaml# > > + > > - if: > > not: > > properties: > > @@ -67,7 +73,7 @@ allOf: > > required: > > - clock-names > > > > -additionalProperties: false > > +unevaluatedProperties: false > > > > examples: > > - | > > -- > > 2.18.0 > >
On Tue, May 28, 2024 at 02:38:46PM +0000, Jason-JH Lin (林睿祥) wrote: > Hi Conor, > > On Mon, 2024-05-27 at 18:36 +0100, Conor Dooley wrote: > > On Sun, May 26, 2024 at 10:44:37PM +0800, Jason-JH.Lin wrote: > > > 1. Add mboxes property to define a GCE loopping thread as a secure > > > IRQ > > > handler. > > > The CMDQ secure driver requests a mbox channel and sends a looping > > > command to the GCE thread. The looping command will wait for a > > > secure > > > packet done event signal from secure world and then jump back to > > > the > > > first instuction. Each time it waits for an event, it notifies the > > > CMDQ driver to perform the same action as the IRQ handler. > > > > > > 2. Add gce-events property from gce-props.yaml to define a > > > secure packet done signal in secure world. > > > There are 1024 events IDs for GCE to use to execute instructions in > > > the specific event happened. These events could be signaled by HW > > > or SW > > > and their value would be different in different SoC because of HW > > > event > > > IDs distribution range from 0 to 1023. > > > If we set a static event ID: 855 for mt8188, it might be conflict > > > the > > > event ID original set in mt8195. > > > > Two different SoCs, two different compatibles, no problem. > > I'm almost certain you previously told me that the firmware changing > > could result in a different event ID, but I see no mention of that > > here. > > Yes, it could be, but we don't use it that way. > > > The commit messages makes it seem like this can be determined by the > > compatible, so either give me a commit message that explains why the > > compatible is not sufficient or drop the patch. > > > > Yes, this can be determined by compatible in CMDQ mailbox driver, > so I think it's possible to put this in the CMDQ mailbox driver data > and configure by different SoC. > > The problem is these events are defined in include/dt- > bindings/mailbox/mediatek,mt8188-gce.h and include/dt- > bindings/gce/mt8195-gce.h. > I can only use them in their mt8188.dts or mt8195.dts. > > If I want to use the same event define in 2 different headers in the > same CMDQ mailbox driver, I think I just can parse their dts to get the > corresponding one. > Do you know how to generally deal with this problem? > Or I can just use the number of event ID in driver data without the > event define in dt-bindings header. I don't think I really understand the problem. You get the channelid/event data from the match data, right? Is the problem that both files define the same "word" to mean different numbers? In that case, just define the numbers locally in the driver, you don't need to include a binding header when there's no data sharing between dts and kernel.
On Tue, 2024-05-28 at 15:59 +0100, Conor Dooley wrote: > On Tue, May 28, 2024 at 02:38:46PM +0000, Jason-JH Lin (林睿祥) wrote: > > Hi Conor, > > > > On Mon, 2024-05-27 at 18:36 +0100, Conor Dooley wrote: > > > On Sun, May 26, 2024 at 10:44:37PM +0800, Jason-JH.Lin wrote: > > > > 1. Add mboxes property to define a GCE loopping thread as a > > > > secure > > > > IRQ > > > > handler. > > > > The CMDQ secure driver requests a mbox channel and sends a > > > > looping > > > > command to the GCE thread. The looping command will wait for a > > > > secure > > > > packet done event signal from secure world and then jump back > > > > to > > > > the > > > > first instuction. Each time it waits for an event, it notifies > > > > the > > > > CMDQ driver to perform the same action as the IRQ handler. > > > > > > > > 2. Add gce-events property from gce-props.yaml to define a > > > > secure packet done signal in secure world. > > > > There are 1024 events IDs for GCE to use to execute > > > > instructions in > > > > the specific event happened. These events could be signaled by > > > > HW > > > > or SW > > > > and their value would be different in different SoC because of > > > > HW > > > > event > > > > IDs distribution range from 0 to 1023. > > > > If we set a static event ID: 855 for mt8188, it might be > > > > conflict > > > > the > > > > event ID original set in mt8195. > > > > > > Two different SoCs, two different compatibles, no problem. > > > I'm almost certain you previously told me that the firmware > > > changing > > > could result in a different event ID, but I see no mention of > > > that > > > here. > > > > Yes, it could be, but we don't use it that way. > > > > > The commit messages makes it seem like this can be determined by > > > the > > > compatible, so either give me a commit message that explains why > > > the > > > compatible is not sufficient or drop the patch. > > > > > > > Yes, this can be determined by compatible in CMDQ mailbox driver, > > so I think it's possible to put this in the CMDQ mailbox driver > > data > > and configure by different SoC. > > > > The problem is these events are defined in include/dt- > > bindings/mailbox/mediatek,mt8188-gce.h and include/dt- > > bindings/gce/mt8195-gce.h. > > I can only use them in their mt8188.dts or mt8195.dts. > > > > If I want to use the same event define in 2 different headers in > > the > > same CMDQ mailbox driver, I think I just can parse their dts to get > > the > > corresponding one. > > Do you know how to generally deal with this problem? > > Or I can just use the number of event ID in driver data without the > > event define in dt-bindings header. > > I don't think I really understand the problem. You get the > channelid/event data from the match data, right? Is the problem that > both files define the same "word" to mean different numbers? Yes, I mean the same "event define" with different event ID numbers in different SoC headers. > In that > case, just define the numbers locally in the driver, you don't need > to > include a binding header when there's no data sharing between dts and > kernel. OK, I'll do that and drop this patch. Thanks~ Regards, Jason-JH.Lin
diff --git a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml index cef9d7601398..6e5e848d61d9 100644 --- a/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml +++ b/Documentation/devicetree/bindings/mailbox/mediatek,gce-mailbox.yaml @@ -49,6 +49,10 @@ properties: items: - const: gce + mboxes: + items: + - description: GCE looping thread as a secure IRQ handler + required: - compatible - "#mbox-cells" @@ -57,6 +61,8 @@ required: - clocks allOf: + - $ref: /schemas/mailbox/mediatek,gce-props.yaml# + - if: not: properties: @@ -67,7 +73,7 @@ allOf: required: - clock-names -additionalProperties: false +unevaluatedProperties: false examples: - |