Re: [PATCH 02/12] dt-bindings: arm: msm: Fix register regions used for LLCC banks

From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Bjorn Andersson <andersson@kernel.org>
Cc: robh+dt@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	bp@alien8.de, tony.luck@intel.com, quic_saipraka@quicinc.com,
	konrad.dybcio@linaro.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org, james.morse@arm.com,
	mchehab@kernel.org, rric@kernel.org, linux-edac@vger.kernel.org,
	quic_ppareek@quicinc.com, stable@vger.kernel.org
Subject: Re: [PATCH 02/12] dt-bindings: arm: msm: Fix register regions used for LLCC banks
Date: Wed, 7 Dec 2022 22:23:39 +0530	[thread overview]
Message-ID: <20221207165339.GB315152@thinkpad> (raw)
In-Reply-To: <20221207160825.nafywxehxjx42kww@builder.lan>

On Wed, Dec 07, 2022 at 10:08:25AM -0600, Bjorn Andersson wrote:
> On Wed, Dec 07, 2022 at 07:29:11PM +0530, Manivannan Sadhasivam wrote:
> > Register regions of the LLCC banks are located at separate addresses.
> > Currently, the binding just lists the LLCC0 base address and specifies
> > the size to cover all banks. This is not the correct approach since,
> > there are holes and other registers located in between.
> > 
> > So let's specify the base address of each LLCC bank. It should be noted
> > that the bank count differs for each SoC, so that also needs to be taken
> > into account in the binding.
> > 
> > Cc: <stable@vger.kernel.org> # 4.19
> > Fixes: 7e5700ae64f6 ("dt-bindings: Documentation for qcom, llcc")
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  .../bindings/arm/msm/qcom,llcc.yaml           | 125 ++++++++++++++++--
> >  1 file changed, 114 insertions(+), 11 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > index d1df49ffcc1b..7f694baa017c 100644
> > --- a/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > +++ b/Documentation/devicetree/bindings/arm/msm/qcom,llcc.yaml
> > @@ -33,14 +33,12 @@ properties:
> >        - qcom,sm8550-llcc
> >  
> >    reg:
> > -    items:
> > -      - description: LLCC base register region
> > -      - description: LLCC broadcast base register region
> > +    minItems: 2
> > +    maxItems: 9
> >  
> >    reg-names:
> > -    items:
> > -      - const: llcc_base
> > -      - const: llcc_broadcast_base
> > +    minItems: 2
> > +    maxItems: 9
> 
> Afaict changing llcc_base to llcc0_base would not allow the
> implementation to find the llcc_base region with an older DTB.
> 
> But if you instead modify the driver to pick up N-1 regions for base and
> the last for broadcast, by index. This should continue to work for the
> platforms where it actually worked.
> 

TBH, only platform that is supposed to work is SDM845. But on that also, some
of the registers are secured. So when the EDAC driver accesses them, it results
in reboot into EDL mode.

That's one of the reason why I didn't consider backwards compatibility here.

I may have to add a patch that removes the interrupts property from LLCC node
for this platform, as without that property EDAC driver will not be probed.

But using indexes also makes sense, so I will adapt it in next version.

> Based on that I suggest that you just remove reg-names from the binding.
> It will clean up the binding and we won't have reg-names containing
> "llcc" or "base" :)
> 

Sure.

Thanks,
Mani

> Regards,
> Bjorn
> 
> >  
> >    interrupts:
> >      maxItems: 1
> > @@ -50,15 +48,120 @@ required:
> >    - reg
> >    - reg-names
> >  
> > +allOf:
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sc7180-llcc
> > +              - qcom,sm6350-llcc
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: LLCC0 base register region
> > +            - description: LLCC broadcast base register region
> > +        reg-names:
> > +          items:
> > +            - const: llcc0_base
> > +            - const: llcc_broadcast_base
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sc7280-llcc
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: LLCC0 base register region
> > +            - description: LLCC1 base register region
> > +            - description: LLCC broadcast base register region
> > +        reg-names:
> > +          items:
> > +            - const: llcc0_base
> > +            - const: llcc1_base
> > +            - const: llcc_broadcast_base
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sc8180x-llcc
> > +              - qcom,sc8280xp-llcc
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: LLCC0 base register region
> > +            - description: LLCC1 base register region
> > +            - description: LLCC2 base register region
> > +            - description: LLCC3 base register region
> > +            - description: LLCC4 base register region
> > +            - description: LLCC5 base register region
> > +            - description: LLCC6 base register region
> > +            - description: LLCC7 base register region
> > +            - description: LLCC broadcast base register region
> > +        reg-names:
> > +          items:
> > +            - const: llcc0_base
> > +            - const: llcc1_base
> > +            - const: llcc2_base
> > +            - const: llcc3_base
> > +            - const: llcc4_base
> > +            - const: llcc5_base
> > +            - const: llcc6_base
> > +            - const: llcc7_base
> > +            - const: llcc_broadcast_base
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - qcom,sdm845-llcc
> > +              - qcom,sm8150-llcc
> > +              - qcom,sm8250-llcc
> > +              - qcom,sm8350-llcc
> > +              - qcom,sm8450-llcc
> > +    then:
> > +      properties:
> > +        reg:
> > +          items:
> > +            - description: LLCC0 base register region
> > +            - description: LLCC1 base register region
> > +            - description: LLCC2 base register region
> > +            - description: LLCC3 base register region
> > +            - description: LLCC broadcast base register region
> > +        reg-names:
> > +          items:
> > +            - const: llcc0_base
> > +            - const: llcc1_base
> > +            - const: llcc2_base
> > +            - const: llcc3_base
> > +            - const: llcc_broadcast_base
> > +
> >  additionalProperties: false
> >  
> >  examples:
> >    - |
> >      #include <dt-bindings/interrupt-controller/arm-gic.h>
> >  
> > -    system-cache-controller@1100000 {
> > -      compatible = "qcom,sdm845-llcc";
> > -      reg = <0x1100000 0x200000>, <0x1300000 0x50000> ;
> > -      reg-names = "llcc_base", "llcc_broadcast_base";
> > -      interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        system-cache-controller@1100000 {
> > +          compatible = "qcom,sdm845-llcc";
> > +          reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
> > +                <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
> > +                <0 0x01300000 0 0x50000>;
> > +          reg-names = "llcc0_base", "llcc1_base", "llcc2_base",
> > +                "llcc3_base", "llcc_broadcast_base";
> > +          interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> > +        };
> >      };
> > -- 
> > 2.25.1
> > 

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