[v2,2/5] dt-bindings: arm: msm: Add bindings for multi channel DDR in LLCC

  Add description for additional nodes needed to support
mulitple channel DDR configurations in LLCC.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)
  

On Mon, 13 Mar 2023 18:10:37 +0530, Komal Bajaj wrote:
> Add description for additional nodes needed to support
> mulitple channel DDR configurations in LLCC.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.example.dtb: system-cache-controller@1100000: reg: [[17825792, 2097152], [19922944, 327680]] is too short
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.example.dtb: system-cache-controller@1100000: reg-names: ['llcc_base', 'llcc_broadcast_base'] is too short
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20230313124040.9463-3-quic_kbajaj@quicinc.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.
  

On 13/03/2023 13:40, Komal Bajaj wrote:
> Add description for additional nodes needed to support
> mulitple channel DDR configurations in LLCC.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>

+Cc Mani,

This will conflict with:
https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/

Please rebase on top of Mani's patches (assuming they are not
conflicting in principle)

> ---
>  Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> index 38efcad56dbd..9a4a76caf490 100644
> --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> @@ -37,15 +37,24 @@ properties:
>      items:

minItems: 2

>        - description: LLCC base register region
>        - description: LLCC broadcast base register region
> +      - description: Feature register to decide which LLCC configuration
> +                     to use, this is optional
>  
>    reg-names:

minItems: 2

>      items:
>        - const: llcc_base
>        - const: llcc_broadcast_base
> +      - const: multi_channel_register
>  
>    interrupts:
>      maxItems: 1
>  
> +  multi-ch-bit-off:
> +    items:
> +      - description: Specifies the offset in bits into the multi_channel_register
> +                     and the number of bits used to decide which LLCC configuration
> +                     to use

There are here few issues.
First, I don't fully understand the property. What is an LLCC
configuration? Like some fused values?

Second, don't make it a register specific, it will not scale easily to
any new version of this interface. Although how this should look like
depends on what is it.

Third, you need vendor prefix and type (unless this is a generic
property, but does not look like). Then "items" is probably wrong. Line
break after "description: "

Best regards,
Krzysztof
  

On Wed, Mar 15, 2023 at 08:41:21AM +0100, Krzysztof Kozlowski wrote:
> On 13/03/2023 13:40, Komal Bajaj wrote:
> > Add description for additional nodes needed to support
> > mulitple channel DDR configurations in LLCC.
> > 
> > Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> 
> +Cc Mani,
> 

Thanks, Krzysztof!

> This will conflict with:
> https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/
> 
> Please rebase on top of Mani's patches (assuming they are not
> conflicting in principle)
> 
> > ---
> >  Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > index 38efcad56dbd..9a4a76caf490 100644
> > --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > @@ -37,15 +37,24 @@ properties:
> >      items:
> 
> minItems: 2
> 
> >        - description: LLCC base register region
> >        - description: LLCC broadcast base register region
> > +      - description: Feature register to decide which LLCC configuration
> > +                     to use, this is optional
> >  
> >    reg-names:
> 
> minItems: 2
> 
> >      items:
> >        - const: llcc_base
> >        - const: llcc_broadcast_base
> > +      - const: multi_channel_register

Is this the actual register region or a specific register offset? We generally
try to pass the base address of the region along with the size and use the
offset inside the driver to access any specific registers.

Thanks,
Mani

> >  
> >    interrupts:
> >      maxItems: 1
> >  
> > +  multi-ch-bit-off:
> > +    items:
> > +      - description: Specifies the offset in bits into the multi_channel_register
> > +                     and the number of bits used to decide which LLCC configuration
> > +                     to use
> 
> There are here few issues.
> First, I don't fully understand the property. What is an LLCC
> configuration? Like some fused values?
> 
> Second, don't make it a register specific, it will not scale easily to
> any new version of this interface. Although how this should look like
> depends on what is it.
> 
> Third, you need vendor prefix and type (unless this is a generic
> property, but does not look like). Then "items" is probably wrong. Line
> break after "description: "
> 
> Best regards,
> Krzysztof
>
  

On 06/04/2023 11:19, Komal Bajaj wrote:
> 
>>>>   
>>>>     interrupts:
>>>>       maxItems: 1
>>>>   
>>>> +  multi-ch-bit-off:
>>>> +    items:
>>>> +      - description: Specifies the offset in bits into the multi_channel_register
>>>> +                     and the number of bits used to decide which LLCC configuration
>>>> +                     to use
>>> There are here few issues.
>>> First, I don't fully understand the property. What is an LLCC
>>> configuration? Like some fused values?
> 
> There are different configuration for LLCC based on the number of
> DDR channel it uses. Here, we are basically trying to get information
> about the same.
> 
>>>
>>> Second, don't make it a register specific, it will not scale easily to
>>> any new version of this interface. Although how this should look like
>>> depends on what is it.
> 
> LLCC driver can only get DDR channel information from the register.

And why would that exactly matter to DT? How does it solve my concern
that your approach does not scale?

Best regards,
Krzysztof
  

On 4/6/2023 2:49 PM, Komal Bajaj wrote:
> Didn't see my reply on the list, so sending it again.
> And also I see that the dt patch is already applied.

The reason why you are not seeing your replies at

https://lore.kernel.org/lkml/20230313124040.9463-1-quic_kbajaj@quicinc.com/

is because your reply cc-list contain some invalid domain 
(codeaurora.org) email id's and any list/email mentioned
after that would not be getting your emails.

-- Mukesh

> 
> Thanks Krzysztof and Manivannan for reviewing the patch.
> 
> 
> On 3/15/2023 7:18 PM, Manivannan Sadhasivam wrote:
>> On Wed, Mar 15, 2023 at 08:41:21AM +0100, Krzysztof Kozlowski wrote:
>>> On 13/03/2023 13:40, Komal Bajaj wrote:
>>>> Add description for additional nodes needed to support
>>>> mulitple channel DDR configurations in LLCC.
>>>>
>>>> Signed-off-by: Komal Bajaj<quic_kbajaj@quicinc.com>
>>> +Cc Mani,
>>>
>> Thanks, Krzysztof!
>>
>>> This will conflict with:
>>> https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@linaro.org/
>>>
>>> Please rebase on top of Mani's patches (assuming they are not
>>> conflicting in principle)
>>>
>>>> ---
>>>>   Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>>> index 38efcad56dbd..9a4a76caf490 100644
>>>> --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>>> +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
>>>> @@ -37,15 +37,24 @@ properties:
>>>>       items:
>>> minItems: 2
>>>
>>>>         - description: LLCC base register region
>>>>         - description: LLCC broadcast base register region
>>>> +      - description: Feature register to decide which LLCC configuration
>>>> +                     to use, this is optional
>>>>   
>>>>     reg-names:
>>> minItems: 2
>>>
>>>>       items:
>>>>         - const: llcc_base
>>>>         - const: llcc_broadcast_base
>>>> +      - const: multi_channel_register
>> Is this the actual register region or a specific register offset? We generally
>> try to pass the base address of the region along with the size and use the
>> offset inside the driver to access any specific registers.
>>
>> Thanks,
>> Mani
> 
> This is a specific register offset outside the LLCC register region which has the
> information of number of DDR channel.
> 
>>>>   
>>>>     interrupts:
>>>>       maxItems: 1
>>>>   
>>>> +  multi-ch-bit-off:
>>>> +    items:
>>>> +      - description: Specifies the offset in bits into the multi_channel_register
>>>> +                     and the number of bits used to decide which LLCC configuration
>>>> +                     to use
>>> There are here few issues.
>>> First, I don't fully understand the property. What is an LLCC
>>> configuration? Like some fused values?
> 
> There are different configuration for LLCC based on the number of
> DDR channel it uses. Here, we are basically trying to get information
> about the same.
> 
>>> Second, don't make it a register specific, it will not scale easily to
>>> any new version of this interface. Although how this should look like
>>> depends on what is it.
> 
> LLCC driver can only get DDR channel information from the register.
> 
>>> Third, you need vendor prefix and type (unless this is a generic
>>> property, but does not look like). Then "items" is probably wrong. Line
>>> break after "description: "
> 
> Noted, will take care of this in the next patchset.
> 
> Thanks
> Komal
> 
>>> Best regards,
>>> Krzysztof
>>>
>
  

On 4/6/2023 3:04 PM, Krzysztof Kozlowski wrote:
> On 06/04/2023 11:19, Komal Bajaj wrote:
>>
>>>>>    
>>>>>      interrupts:
>>>>>        maxItems: 1
>>>>>    
>>>>> +  multi-ch-bit-off:
>>>>> +    items:
>>>>> +      - description: Specifies the offset in bits into the multi_channel_register
>>>>> +                     and the number of bits used to decide which LLCC configuration
>>>>> +                     to use
>>>> There are here few issues.
>>>> First, I don't fully understand the property. What is an LLCC
>>>> configuration? Like some fused values?
>>
>> There are different configuration for LLCC based on the number of
>> DDR channel it uses. Here, we are basically trying to get information
>> about the same.
>>
>>>>
>>>> Second, don't make it a register specific, it will not scale easily to
>>>> any new version of this interface. Although how this should look like
>>>> depends on what is it.
>>
>> LLCC driver can only get DDR channel information from the register.

Actually, any one can read this property there is no boundation to that.
However, some of the bits information of this register only matters to 
LLCC to make decision.

> And why would that exactly matter to DT? How does it solve my concern
> that your approach does not scale?

I understand your concern about not making it register specific, however 
this register is a secure fuse register where the interested bits are
known to us and should only be used by llcc to select right slice 
configuration table to apply.

How about something like this,

Add another property like qcom,tcsr_feature_config under qcom,scm
and read this property under qcom_scm driver and expose
read exported symbol for LLCC driver. LLCC driver can use API and
apply bit offset/bit size to get right value inside the target driver 
data(e.g .data = &XXXX_cfg }) or maintain it inside same 
tcsr_feature_config DT (this can be discussed)
Also, we can have a yaml file for tcsr_feature_config.

e.g..


scm: scm {
         compatible = "qcom,scm-sm8450", "qcom,scm";
         qcom,dload-mode = <&tcsr 0x13000>;
         ...
	qcom,tcsr_feature_config = <&tcsr_feature_config>
};

tcsr_feature_config: syscon@fd484000 {
	compatible = "qcom, XXXX", qcom,tcsr-featureconfig";
	reg = <0xXXXXXXXX 0xYYYY>;
};


-- Mukesh

> 
> Best regards,
> Krzysztof
>