diff mbox series

[v2,03/13] arm64: dts: qcom: sdm845: Fix the base addresses of LLCC banks

Message ID 20221212123311.146261-4-manivannan.sadhasivam@linaro.org
State New
Headers
Series Qcom: LLCC/EDAC: Fix base address used for LLCC banks |

Commit Message

Manivannan Sadhasivam Dec. 12, 2022, 12:33 p.m. UTC 
  The LLCC block has several banks each with a different base address
and holes in between. So it is not a correct approach to cover these
banks with a single offset/size. Instead, the individual bank's base
address needs to be specified in devicetree with the exact size.

Also, let's get rid of reg-names property as it is not needed anymore.
The driver is expected to parse the reg field based on index to get the
addresses of each LLCC banks.

Cc: <stable@vger.kernel.org> # 5.4
Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Comments

Sai Prakash Ranjan Dec. 13, 2022, 5:04 a.m. UTC | #1 
On 12/12/2022 6:03 PM, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.4
> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>   arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 65032b94b46d..683b861e060d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>   
>   		llcc: system-cache-controller@1100000 {
>   			compatible = "qcom,sdm845-llcc";
> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";
> +			reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
> +			      <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
> +			      <0 0x01300000 0 0x50000>;
>   			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>   		};
>   

Reviewed-by: Sai Prakash Ranjan <quic_saipraka@quicinc.com>
Krzysztof Kozlowski Dec. 13, 2022, 4:27 p.m. UTC | #2 
On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> The LLCC block has several banks each with a different base address
> and holes in between. So it is not a correct approach to cover these
> banks with a single offset/size. Instead, the individual bank's base
> address needs to be specified in devicetree with the exact size.
> 
> Also, let's get rid of reg-names property as it is not needed anymore.
> The driver is expected to parse the reg field based on index to get the
> addresses of each LLCC banks.
> 
> Cc: <stable@vger.kernel.org> # 5.4

No, you cannot backport it. You will break users.

> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 65032b94b46d..683b861e060d 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>  
>  		llcc: system-cache-controller@1100000 {
>  			compatible = "qcom,sdm845-llcc";
> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> -			reg-names = "llcc_base", "llcc_broadcast_base";

Once property was made required, you cannot remove it. What if other
bindings user depends on it?

Please instead keep/update the reg-names and/or mark it as deprecated.
It must stay in DTS for some time.

Best regards,
Krzysztof
Manivannan Sadhasivam Dec. 13, 2022, 5:13 p.m. UTC | #3 
On Tue, Dec 13, 2022 at 05:27:45PM +0100, Krzysztof Kozlowski wrote:
> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
> > The LLCC block has several banks each with a different base address
> > and holes in between. So it is not a correct approach to cover these
> > banks with a single offset/size. Instead, the individual bank's base
> > address needs to be specified in devicetree with the exact size.
> > 
> > Also, let's get rid of reg-names property as it is not needed anymore.
> > The driver is expected to parse the reg field based on index to get the
> > addresses of each LLCC banks.
> > 
> > Cc: <stable@vger.kernel.org> # 5.4
> 
> No, you cannot backport it. You will break users.
> 

If the driver change gets backported, it will break users, isn't it?

> > Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
> > Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index 65032b94b46d..683b861e060d 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
> >  
> >  		llcc: system-cache-controller@1100000 {
> >  			compatible = "qcom,sdm845-llcc";
> > -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
> > -			reg-names = "llcc_base", "llcc_broadcast_base";
> 
> Once property was made required, you cannot remove it. What if other
> bindings user depends on it?
> 
> Please instead keep/update the reg-names and/or mark it as deprecated.
> It must stay in DTS for some time.
> 

Fair enough. I will mark it as deprecated in binding and will keep it in dts.

Thanks,
Mani

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Dec. 13, 2022, 6:37 p.m. UTC | #4 
On 13/12/2022 18:13, Manivannan Sadhasivam wrote:
> On Tue, Dec 13, 2022 at 05:27:45PM +0100, Krzysztof Kozlowski wrote:
>> On 12/12/2022 13:33, Manivannan Sadhasivam wrote:
>>> The LLCC block has several banks each with a different base address
>>> and holes in between. So it is not a correct approach to cover these
>>> banks with a single offset/size. Instead, the individual bank's base
>>> address needs to be specified in devicetree with the exact size.
>>>
>>> Also, let's get rid of reg-names property as it is not needed anymore.
>>> The driver is expected to parse the reg field based on index to get the
>>> addresses of each LLCC banks.
>>>
>>> Cc: <stable@vger.kernel.org> # 5.4
>>
>> No, you cannot backport it. You will break users.
>>
> 
> If the driver change gets backported, it will break users, isn't it?

Whether driver change gets backported or not - all out of tree kernel
users, other systems, firmwares/bootloaders are broken and backporting
driver piece will not fix it.

By this backport you mean that the change can go alone to v5.4 kernel
(you did not write here dependency on other backport) and I wonder if
v5.4 kernel works with this patch...

> 
>>> Fixes: ba0411ddd133 ("arm64: dts: sdm845: Add device node for Last level cache controller")
>>> Reported-by: Parikshit Pareek <quic_ppareek@quicinc.com>
>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> index 65032b94b46d..683b861e060d 100644
>>> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
>>> @@ -2132,8 +2132,9 @@ uart15: serial@a9c000 {
>>>  
>>>  		llcc: system-cache-controller@1100000 {
>>>  			compatible = "qcom,sdm845-llcc";
>>> -			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
>>> -			reg-names = "llcc_base", "llcc_broadcast_base";
>>
>> Once property was made required, you cannot remove it. What if other
>> bindings user depends on it?
>>
>> Please instead keep/update the reg-names and/or mark it as deprecated.
>> It must stay in DTS for some time.
>>
> 
> Fair enough. I will mark it as deprecated in binding and will keep it in dts.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index 65032b94b46d..683b861e060d 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -2132,8 +2132,9 @@  uart15: serial@a9c000 {
 
 		llcc: system-cache-controller@1100000 {
 			compatible = "qcom,sdm845-llcc";
-			reg = <0 0x01100000 0 0x31000>, <0 0x01300000 0 0x50000>;
-			reg-names = "llcc_base", "llcc_broadcast_base";
+			reg = <0 0x01100000 0 0x50000>, <0 0x01180000 0 0x50000>,
+			      <0 0x01200000 0 0x50000>, <0 0x01280000 0 0x50000>,
+			      <0 0x01300000 0 0x50000>;
 			interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
 		};