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

[Mukesh Ojha]

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@xxxxxxxxxxx/

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@xxxxxxxxxxx>
> > > +Cc Mani,
> > Thanks, Krzysztof!
> >
> > > This will conflict with:
> > > https://lore.kernel.org/all/20230314080443.64635-3-manivannan.sadhasivam@xxxxxxxxxx/
> > >
> > > 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