| Message ID | 20230505064039.1630025-3-bhupesh.sharma@linaro.org |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Add Qualcomm SM6115 / SM4250 EUD dt-bindings & driver support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 05/05/2023 08:40, Bhupesh Sharma wrote: > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. > > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' > needs to be set first to 'enable' the eud module. > > So, update the dt-bindings to accommodate the third register > property (TCSR Base) required by the driver on these SoCs. > > Also for these SoCs, introduce a new bool property > 'qcom,secure-mode-enable', which indicates that the mode manager > needs to be accessed only via the secure world. > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > --- > .../devicetree/bindings/soc/qcom/qcom,eud.yaml | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > index f2c5ec7e6437..3b92cdf4e306 100644 > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > @@ -18,17 +18,33 @@ properties: > items: > - enum: > - qcom,sc7280-eud > + - qcom,sm6115-eud > - const: qcom,eud > > reg: > + minItems: 2 > items: > - description: EUD Base Register Region > - description: EUD Mode Manager Register > + - description: TCSR Base Register Region > + > + reg-names: > + minItems: 2 > + items: > + - const: eud-base > + - const: eud-mode-mgr > + - const: tcsr-base > > interrupts: > description: EUD interrupt > maxItems: 1 > > + qcom,secure-mode-enable: > + type: boolean > + description: > + Indicates that the mode manager needs to be accessed only via the secure > + world (through 'scm' calls). I understood tcsr-base aplies only to SM6115, so this should be further constrained in allOf:if:then:. Best regards, Krzysztof
On 05/05/2023 08:40, Bhupesh Sharma wrote: > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. > > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' > needs to be set first to 'enable' the eud module. > > So, update the dt-bindings to accommodate the third register > property (TCSR Base) required by the driver on these SoCs. > > Also for these SoCs, introduce a new bool property > 'qcom,secure-mode-enable', which indicates that the mode manager > needs to be accessed only via the secure world. Cannot it be implied by compatible? Best regards, Krzysztof
Hi Krzysztof, On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/05/2023 08:40, Bhupesh Sharma wrote: > > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. > > > > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' > > needs to be set first to 'enable' the eud module. > > > > So, update the dt-bindings to accommodate the third register > > property (TCSR Base) required by the driver on these SoCs. > > > > Also for these SoCs, introduce a new bool property > > 'qcom,secure-mode-enable', which indicates that the mode manager > > needs to be accessed only via the secure world. > > Cannot it be implied by compatible? I can see this will be used by future SoCs as well from the available EUD documentation. So let's keep a dedicated dt property as suggested by Bjorn in earlier reviews, as otherwise the compatible checks would start getting bigger / messier in the driver code, in my opinion, when we add EUD support for other SoCs + boards. Regards, Bhupesh
HI Krzysztof, On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/05/2023 08:40, Bhupesh Sharma wrote: > > Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. > > > > On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' > > needs to be set first to 'enable' the eud module. > > > > So, update the dt-bindings to accommodate the third register > > property (TCSR Base) required by the driver on these SoCs. > > > > Also for these SoCs, introduce a new bool property > > 'qcom,secure-mode-enable', which indicates that the mode manager > > needs to be accessed only via the secure world. > > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > > --- > > .../devicetree/bindings/soc/qcom/qcom,eud.yaml | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > > index f2c5ec7e6437..3b92cdf4e306 100644 > > --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > > +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > > @@ -18,17 +18,33 @@ properties: > > items: > > - enum: > > - qcom,sc7280-eud > > + - qcom,sm6115-eud > > - const: qcom,eud > > > > reg: > > + minItems: 2 > > items: > > - description: EUD Base Register Region > > - description: EUD Mode Manager Register > > + - description: TCSR Base Register Region > > + > > + reg-names: > > + minItems: 2 > > + items: > > + - const: eud-base > > + - const: eud-mode-mgr > > + - const: tcsr-base > > > > interrupts: > > description: EUD interrupt > > maxItems: 1 > > > > + qcom,secure-mode-enable: > > + type: boolean > > + description: > > + Indicates that the mode manager needs to be accessed only via the secure > > + world (through 'scm' calls). > > I understood tcsr-base aplies only to SM6115, so this should be further > constrained in allOf:if:then:. Please refer to my reply to your query in another review. I can see that secure access to mode_mgr register via TCSR will be exposed by other Qualcomm SoCs as well (from the available EUD documentation). So, maybe keeping it generic instead of limiting it to sm6115 only would be more useful, for future changes. Thanks, Bhupesh
On 05/05/2023 18:27, Bhupesh Sharma wrote: > Hi Krzysztof, > > On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 05/05/2023 08:40, Bhupesh Sharma wrote: >>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. >>> >>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' >>> needs to be set first to 'enable' the eud module. >>> >>> So, update the dt-bindings to accommodate the third register >>> property (TCSR Base) required by the driver on these SoCs. >>> >>> Also for these SoCs, introduce a new bool property >>> 'qcom,secure-mode-enable', which indicates that the mode manager >>> needs to be accessed only via the secure world. >> >> Cannot it be implied by compatible? > > I can see this will be used by future SoCs as well from the available > EUD documentation. > > So let's keep a dedicated dt property as suggested by Bjorn in earlier > reviews, as otherwise the compatible checks would start getting bigger > / messier in the driver code, in my opinion, when we add EUD support > for other SoCs + boards. I don't understand the last part about compatible checks would grow. Why would you have any compatible check in the driver? This looks standard as we do with all SoC properties, so via driver data. Best regards, Krzysztof
On 05/05/2023 18:31, Bhupesh Sharma wrote: > HI Krzysztof, > > On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> wrote: >> >> On 05/05/2023 08:40, Bhupesh Sharma wrote: >>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. >>> >>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' >>> needs to be set first to 'enable' the eud module. >>> >>> So, update the dt-bindings to accommodate the third register >>> property (TCSR Base) required by the driver on these SoCs. >>> >>> Also for these SoCs, introduce a new bool property >>> 'qcom,secure-mode-enable', which indicates that the mode manager >>> needs to be accessed only via the secure world. >>> >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> >>> --- >>> .../devicetree/bindings/soc/qcom/qcom,eud.yaml | 16 ++++++++++++++++ >>> 1 file changed, 16 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml >>> index f2c5ec7e6437..3b92cdf4e306 100644 >>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml >>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml >>> @@ -18,17 +18,33 @@ properties: >>> items: >>> - enum: >>> - qcom,sc7280-eud >>> + - qcom,sm6115-eud >>> - const: qcom,eud >>> >>> reg: >>> + minItems: 2 >>> items: >>> - description: EUD Base Register Region >>> - description: EUD Mode Manager Register >>> + - description: TCSR Base Register Region >>> + >>> + reg-names: >>> + minItems: 2 >>> + items: >>> + - const: eud-base >>> + - const: eud-mode-mgr >>> + - const: tcsr-base >>> >>> interrupts: >>> description: EUD interrupt >>> maxItems: 1 >>> >>> + qcom,secure-mode-enable: >>> + type: boolean >>> + description: >>> + Indicates that the mode manager needs to be accessed only via the secure >>> + world (through 'scm' calls). >> >> I understood tcsr-base aplies only to SM6115, so this should be further >> constrained in allOf:if:then:. > > Please refer to my reply to your query in another review. > I can see that secure access to mode_mgr register via TCSR will be > exposed by other Qualcomm SoCs as well (from the available EUD > documentation). So, maybe keeping it generic instead of limiting it to > sm6115 only would be more useful, for future changes. Your reply does not say that qcom,sc7280-eud uses it, so I don't understand. This is not valid to qcom,sc7280-eud, so allowing it there is not correct. Best regards, Krzysztof
Hi Krzysztof, On Fri, 5 May 2023 at 23:27, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > > On 05/05/2023 18:31, Bhupesh Sharma wrote: > > HI Krzysztof, > > > > On Fri, 5 May 2023 at 21:54, Krzysztof Kozlowski > > <krzysztof.kozlowski@linaro.org> wrote: > >> > >> On 05/05/2023 08:40, Bhupesh Sharma wrote: > >>> Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. > >>> > >>> On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' > >>> needs to be set first to 'enable' the eud module. > >>> > >>> So, update the dt-bindings to accommodate the third register > >>> property (TCSR Base) required by the driver on these SoCs. > >>> > >>> Also for these SoCs, introduce a new bool property > >>> 'qcom,secure-mode-enable', which indicates that the mode manager > >>> needs to be accessed only via the secure world. > >>> > >>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> > >>> --- > >>> .../devicetree/bindings/soc/qcom/qcom,eud.yaml | 16 ++++++++++++++++ > >>> 1 file changed, 16 insertions(+) > >>> > >>> diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > >>> index f2c5ec7e6437..3b92cdf4e306 100644 > >>> --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > >>> +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml > >>> @@ -18,17 +18,33 @@ properties: > >>> items: > >>> - enum: > >>> - qcom,sc7280-eud > >>> + - qcom,sm6115-eud > >>> - const: qcom,eud > >>> > >>> reg: > >>> + minItems: 2 > >>> items: > >>> - description: EUD Base Register Region > >>> - description: EUD Mode Manager Register > >>> + - description: TCSR Base Register Region > >>> + > >>> + reg-names: > >>> + minItems: 2 > >>> + items: > >>> + - const: eud-base > >>> + - const: eud-mode-mgr > >>> + - const: tcsr-base > >>> > >>> interrupts: > >>> description: EUD interrupt > >>> maxItems: 1 > >>> > >>> + qcom,secure-mode-enable: > >>> + type: boolean > >>> + description: > >>> + Indicates that the mode manager needs to be accessed only via the secure > >>> + world (through 'scm' calls). > >> > >> I understood tcsr-base aplies only to SM6115, so this should be further > >> constrained in allOf:if:then:. > > > > Please refer to my reply to your query in another review. > > I can see that secure access to mode_mgr register via TCSR will be > > exposed by other Qualcomm SoCs as well (from the available EUD > > documentation). So, maybe keeping it generic instead of limiting it to > > sm6115 only would be more useful, for future changes. > > Your reply does not say that qcom,sc7280-eud uses it, so I don't > understand. This is not valid to qcom,sc7280-eud, so allowing it there > is not correct. Ok, I got your concern now. I will send a v5 with the change shortly. Thanks.
diff --git a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml index f2c5ec7e6437..3b92cdf4e306 100644 --- a/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml +++ b/Documentation/devicetree/bindings/soc/qcom/qcom,eud.yaml @@ -18,17 +18,33 @@ properties: items: - enum: - qcom,sc7280-eud + - qcom,sm6115-eud - const: qcom,eud reg: + minItems: 2 items: - description: EUD Base Register Region - description: EUD Mode Manager Register + - description: TCSR Base Register Region + + reg-names: + minItems: 2 + items: + - const: eud-base + - const: eud-mode-mgr + - const: tcsr-base interrupts: description: EUD interrupt maxItems: 1 + qcom,secure-mode-enable: + type: boolean + description: + Indicates that the mode manager needs to be accessed only via the secure + world (through 'scm' calls). + ports: $ref: /schemas/graph.yaml#/properties/ports description:
Add dt-bindings for EUD found on Qualcomm SM6115 / SM4250 SoC. On this SoC (and derivatives) the enable bit inside 'tcsr_check_reg' needs to be set first to 'enable' the eud module. So, update the dt-bindings to accommodate the third register property (TCSR Base) required by the driver on these SoCs. Also for these SoCs, introduce a new bool property 'qcom,secure-mode-enable', which indicates that the mode manager needs to be accessed only via the secure world. Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org> --- .../devicetree/bindings/soc/qcom/qcom,eud.yaml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+)