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

[Mukesh Ojha <quic_mojha@quicinc.com>]

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
>>>
>