[v4,2/5] dt-bindings: soc: qcom: eud: Add SM6115 / SM4250 support

  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(+)
  

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.