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

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Cc: Komal Bajaj <quic_kbajaj@quicinc.com>,
	Rob Herring <robh+dt@kernel.org>,
	Bjorn Andersson <andersson@kernel.org>,
	Konrad Dybcio <konrad.dybcio@linaro.org>,
	Abel Vesa <abel.vesa@linaro.org>,
	Rishabh Bhatnagar <rishabhb@codeaurora.org>,
	Prakash Ranjan <saiprakash.ranjan@codeaurora.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Andy Gross <agross@kernel.org>,
	devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org
Subject: Re: [PATCH v2 2/5] dt-bindings: arm: msm: Add bindings for multi channel DDR in LLCC
Date: Wed, 15 Mar 2023 19:18:14 +0530	[thread overview]
Message-ID: <20230315134814.GA98488@thinkpad> (raw)
In-Reply-To: <2b3e39b9-ea70-db9b-89f7-09054df363c3@linaro.org>

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
> 

-- 
மணிவண்ணன் சதாசிவம்