[PATCH v3 0/5] LLCC: Support for Broadcast_AND region

[PATCH v3 0/5] LLCC: Support for Broadcast_AND region

[Unnathi Chalicheemala]

This series adds:
1. Device tree register mapping for Broadcast_AND region in SM8450,
SM8550, SM8650.
2. LLCC driver updates to reflect addition of Broadcast_AND regmap.

To support CSR programming, a broadcast interface is used to program all
channels in a single command. Until SM8450 there was only one broadcast
region (Broadcast_OR) used to broadcast write and check for status bit
0. From SM8450 onwards another broadcast region (Broadcast_AND) has been
added which checks for status bit 1.

This series updates the device trees from SM8450 onwards to have a
mapping to this Broadcast_AND region. It also updates the llcc_drv_data
structure with a regmap for Broadcast_AND region and corrects the
broadcast region used to check for status bit 1.

Changes in v3:
- Removed new example in dt-bindings patch and ran 'make
DT_CHECKER_FLAGS=-m dt_binding_check'
- Use of ternary operator in llcc_update_act_ctrl()
- Add comment before initialization of Broadcast_AND regmap in probe
function
- Move DeviceTree patches to the end

Changes in v2:
- Added an additional check in the case old DT files are used for
above mentioned chipsets for backwards compatibility
- Moved addition of if check in llcc_update_act_ctrl() to a separate
"Fixes" patch; not part of this series

Link to v2: https://lore.kernel.org/all/cover.1707202761.git.quic_uchalich@quicinc.com/
Link to v1: https://lore.kernel.org/all/cover.1706296015.git.quic_uchalich@quicinc.com/

Unnathi Chalicheemala (5):
  dt-bindings: arm: msm: Add llcc Broadcast_AND register
  soc: qcom: llcc: Add regmap for Broadcast_AND region
  arm64: dts: qcom: sm8450: Add mapping to llcc Broadcast_AND region
  arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region
  arm64: dts: qcom: sm8650: Add mapping to llcc Broadcast_AND region

 .../devicetree/bindings/cache/qcom,llcc.yaml  | 27 ++++++++++++++++++-
 arch/arm64/boot/dts/qcom/sm8450.dtsi          |  5 ++--
 arch/arm64/boot/dts/qcom/sm8550.dtsi          |  6 +++--
 arch/arm64/boot/dts/qcom/sm8650.dtsi          |  6 +++--
 drivers/soc/qcom/llcc-qcom.c                  | 15 ++++++++++-
 include/linux/soc/qcom/llcc-qcom.h            |  4 ++-
 6 files changed, 54 insertions(+), 9 deletions(-)

-- 
2.25.1

[PATCH v3 1/5] dt-bindings: arm: msm: Add llcc Broadcast_AND register

[Unnathi Chalicheemala]

The LLCC block in SM8450, SM8550 and SM8650 have a new register
space for Broadcast_AND region. This is used to check that all
channels have bit set to "1", mainly in SCID activation/deactivation.

Previously we were mapping only the Broadcast_OR region assuming
there was only one broadcast register region. Now we also map
Broadcast_AND region.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 .../devicetree/bindings/cache/qcom,llcc.yaml  | 27 ++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 07ccbda4a0ab..a6237028957f 100644
--- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -141,8 +141,31 @@ allOf:
               - qcom,sm8150-llcc
               - qcom,sm8250-llcc
               - qcom,sm8350-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
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
               - qcom,sm8450-llcc
               - qcom,sm8550-llcc
+              - qcom,sm8650-llcc
     then:
       properties:
         reg:
@@ -151,7 +174,8 @@ allOf:
             - description: LLCC1 base register region
             - description: LLCC2 base register region
             - description: LLCC3 base register region
-            - description: LLCC broadcast base register region
+            - description: LLCC broadcast OR register region
+            - description: LLCC broadcast AND register region
         reg-names:
           items:
             - const: llcc0_base
@@ -159,6 +183,7 @@ allOf:
             - const: llcc2_base
             - const: llcc3_base
             - const: llcc_broadcast_base
+            - const: llcc_broadcast_and_base
 
 additionalProperties: false
 
-- 
2.25.1

Re: [PATCH v3 1/5] dt-bindings: arm: msm: Add llcc Broadcast_AND register

[Krzysztof Kozlowski]

On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
> The LLCC block in SM8450, SM8550 and SM8650 have a new register
> space for Broadcast_AND region. This is used to check that all
> channels have bit set to "1", mainly in SCID activation/deactivation.
> 
> Previously we were mapping only the Broadcast_OR region assuming
> there was only one broadcast register region. Now we also map
> Broadcast_AND region.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>

Why did you ignore the tags? Qualcomm upstreaming guide covers this
case, so please read it before sending patches.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] dt-bindings: arm: msm: Add llcc Broadcast_AND register

[Unnathi Chalicheemala]

On 2/27/2024 7:47 AM, Krzysztof Kozlowski wrote:
> On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
>> The LLCC block in SM8450, SM8550 and SM8650 have a new register
>> space for Broadcast_AND region. This is used to check that all
>> channels have bit set to "1", mainly in SCID activation/deactivation.
>>
>> Previously we were mapping only the Broadcast_OR region assuming
>> there was only one broadcast register region. Now we also map
>> Broadcast_AND region.
>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> 
> Why did you ignore the tags? Qualcomm upstreaming guide covers this
> case, so please read it before sending patches.
> 

I was asked to remove the Reviewed-by tag as the previous version of the
patch gave errors on running 'make DT_CHECKER_FLAGS=-m dt_binding_check',
that's why I dropped it.
It was sometime ago, https://lore.kernel.org/all/d78b4bce-7bcd-4ab0-a76b-135cee3ec9b0@linaro.org/

> Best regards,
> Krzysztof
> 

Re: [PATCH v3 1/5] dt-bindings: arm: msm: Add llcc Broadcast_AND register

[Krzysztof Kozlowski]

On 28/02/2024 02:04, Unnathi Chalicheemala wrote:
> On 2/27/2024 7:47 AM, Krzysztof Kozlowski wrote:
>> On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
>>> The LLCC block in SM8450, SM8550 and SM8650 have a new register
>>> space for Broadcast_AND region. This is used to check that all
>>> channels have bit set to "1", mainly in SCID activation/deactivation.
>>>
>>> Previously we were mapping only the Broadcast_OR region assuming
>>> there was only one broadcast register region. Now we also map
>>> Broadcast_AND region.
>>>
>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>>
>> Why did you ignore the tags? Qualcomm upstreaming guide covers this
>> case, so please read it before sending patches.
>>
> 
> I was asked to remove the Reviewed-by tag as the previous version of the
> patch gave errors on running 'make DT_CHECKER_FLAGS=-m dt_binding_check',
> that's why I dropped it.
> It was sometime ago, https://lore.kernel.org/all/d78b4bce-7bcd-4ab0-a76b-135cee3ec9b0@linaro.org/

And where is it explained/mentioned? Every tag drop should be documented.

Best regards,
Krzysztof

Re: [PATCH v3 1/5] dt-bindings: arm: msm: Add llcc Broadcast_AND register

[Unnathi Chalicheemala]

On 2/27/2024 11:14 PM, Krzysztof Kozlowski wrote:
> On 28/02/2024 02:04, Unnathi Chalicheemala wrote:
>> On 2/27/2024 7:47 AM, Krzysztof Kozlowski wrote:
>>> On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
>>>> The LLCC block in SM8450, SM8550 and SM8650 have a new register
>>>> space for Broadcast_AND region. This is used to check that all
>>>> channels have bit set to "1", mainly in SCID activation/deactivation.
>>>>
>>>> Previously we were mapping only the Broadcast_OR region assuming
>>>> there was only one broadcast register region. Now we also map
>>>> Broadcast_AND region.
>>>>
>>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>>>
>>> Why did you ignore the tags? Qualcomm upstreaming guide covers this
>>> case, so please read it before sending patches.
>>>
>>
>> I was asked to remove the Reviewed-by tag as the previous version of the
>> patch gave errors on running 'make DT_CHECKER_FLAGS=-m dt_binding_check',
>> that's why I dropped it.
>> It was sometime ago, https://lore.kernel.org/all/d78b4bce-7bcd-4ab0-a76b-135cee3ec9b0@linaro.org/
> 
> And where is it explained/mentioned? Every tag drop should be documented.
> 

I understand, thanks for pointing out. I will record such tag drops in the change log.

> Best regards,
> Krzysztof
> 

[PATCH v3 2/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

[Unnathi Chalicheemala]

Define new regmap structure for Broadcast_AND region and initialize
this regmap when HW block version is greater than 4.1, otherwise
initialize as a NULL pointer for backwards compatibility.

Switch from broadcast_OR to broadcast_AND region (when defined in DT)
for checking status bit 1 as Broadcast_OR region checks only for bit 0.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 drivers/soc/qcom/llcc-qcom.c       | 15 ++++++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 4ca88eaebf06..cfdc7d9d7773 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -826,6 +826,7 @@ static int llcc_update_act_ctrl(u32 sid,
 	u32 status_reg;
 	u32 slice_status;
 	int ret;
+	struct regmap *regmap;
 
 	if (IS_ERR(drv_data))
 		return PTR_ERR(drv_data);
@@ -849,7 +850,9 @@ static int llcc_update_act_ctrl(u32 sid,
 		return ret;
 
 	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
-		ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
+		regmap = (!drv_data->bcast_and_regmap) ? drv_data->bcast_regmap
+			: drv_data->bcast_and_regmap;
+		ret = regmap_read_poll_timeout(regmap, status_reg,
 				      slice_status, (slice_status & ACT_COMPLETE),
 				      0, LLCC_STATUS_READ_DELAY);
 		if (ret)
@@ -1282,6 +1285,16 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
+	/* Applicable only when drv_data->version >= 4.1 */
+	drv_data->bcast_and_regmap = qcom_llcc_init_mmio(pdev, i + 1, "llcc_broadcast_and_base");
+	if (IS_ERR(drv_data->bcast_and_regmap)) {
+		ret = PTR_ERR(drv_data->bcast_and_regmap);
+		if (ret == -EINVAL)
+			drv_data->bcast_and_regmap = NULL;
+		else
+			goto err;
+	}
+
 	llcc_cfg = cfg->sct_data;
 	sz = cfg->size;
 
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
index 1a886666bbb6..9e9f528b1370 100644
--- a/include/linux/soc/qcom/llcc-qcom.h
+++ b/include/linux/soc/qcom/llcc-qcom.h
@@ -115,7 +115,8 @@ struct llcc_edac_reg_offset {
 /**
  * struct llcc_drv_data - Data associated with the llcc driver
  * @regmaps: regmaps associated with the llcc device
- * @bcast_regmap: regmap associated with llcc broadcast offset
+ * @bcast_regmap: regmap associated with llcc broadcast OR offset
+ * @bcast_and_regmap: regmap associated with llcc broadcast AND offset
  * @cfg: pointer to the data structure for slice configuration
  * @edac_reg_offset: Offset of the LLCC EDAC registers
  * @lock: mutex associated with each slice
@@ -129,6 +130,7 @@ struct llcc_edac_reg_offset {
 struct llcc_drv_data {
 	struct regmap **regmaps;
 	struct regmap *bcast_regmap;
+	struct regmap *bcast_and_regmap;
 	const struct llcc_slice_config *cfg;
 	const struct llcc_edac_reg_offset *edac_reg_offset;
 	struct mutex lock;
-- 
2.25.1

Re: [PATCH v3 2/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

[Konrad Dybcio]

On 2/23/24 00:07, Unnathi Chalicheemala wrote:
> Define new regmap structure for Broadcast_AND region and initialize
> this regmap when HW block version is greater than 4.1, otherwise
> initialize as a NULL pointer for backwards compatibility.
> 
> Switch from broadcast_OR to broadcast_AND region (when defined in DT)
> for checking status bit 1 as Broadcast_OR region checks only for bit 0.
> 
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>   drivers/soc/qcom/llcc-qcom.c       | 15 ++++++++++++++-
>   include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>   2 files changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 4ca88eaebf06..cfdc7d9d7773 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -826,6 +826,7 @@ static int llcc_update_act_ctrl(u32 sid,
>   	u32 status_reg;
>   	u32 slice_status;
>   	int ret;
> +	struct regmap *regmap;
Reverse-Christmas-tree, please

>   
>   	if (IS_ERR(drv_data))
>   		return PTR_ERR(drv_data);
> @@ -849,7 +850,9 @@ static int llcc_update_act_ctrl(u32 sid,
>   		return ret;
>   
>   	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
> -		ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
> +		regmap = (!drv_data->bcast_and_regmap) ? drv_data->bcast_regmap
> +			: drv_data->bcast_and_regmap;

<raised eyebrow emoji>

regmap = drv_data->bcast_and_regmap ?: drv_data->bcast_regmap

Konrad

[PATCH v3 3/5] arm64: dts: qcom: sm8450: Add mapping to llcc Broadcast_AND region

[Unnathi Chalicheemala]

Mapping Broadcast_AND region for LLCC in SM8450.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 01e4dfc4babd..d2d6fae55e2a 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -4306,9 +4306,10 @@ system-cache-controller@19200000 {
 			compatible = "qcom,sm8450-llcc";
 			reg = <0 0x19200000 0 0x80000>, <0 0x19600000 0 0x80000>,
 			      <0 0x19300000 0 0x80000>, <0 0x19700000 0 0x80000>,
-			      <0 0x19a00000 0 0x80000>;
+			      <0 0x19a00000 0 0x80000>, <0 0x19c00000 0 0x80000>;
 			reg-names = "llcc0_base", "llcc1_base", "llcc2_base",
-				    "llcc3_base", "llcc_broadcast_base";
+				    "llcc3_base", "llcc_broadcast_base",
+				    "llcc_broadcast_and_base";
 			interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v3 3/5] arm64: dts: qcom: sm8450: Add mapping to llcc Broadcast_AND region

[Krzysztof Kozlowski]

On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
> Mapping Broadcast_AND region for LLCC in SM8450.
> 

Nothing improved here.

Best regards,
Krzysztof

[PATCH v3 4/5] arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region

[Unnathi Chalicheemala]

Mapping Broadcast_AND region for LLCC in SM8550.

Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sm8550.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8550.dtsi b/arch/arm64/boot/dts/qcom/sm8550.dtsi
index ee1ba5a8c8fc..1a52e30330c3 100644
--- a/arch/arm64/boot/dts/qcom/sm8550.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8550.dtsi
@@ -4193,12 +4193,14 @@ system-cache-controller@25000000 {
 			      <0 0x25200000 0 0x200000>,
 			      <0 0x25400000 0 0x200000>,
 			      <0 0x25600000 0 0x200000>,
-			      <0 0x25800000 0 0x200000>;
+			      <0 0x25800000 0 0x200000>,
+			      <0 0x25a00000 0 0x200000>;
 			reg-names = "llcc0_base",
 				    "llcc1_base",
 				    "llcc2_base",
 				    "llcc3_base",
-				    "llcc_broadcast_base";
+				    "llcc_broadcast_base",
+				    "llcc_broadcast_and_base";
 			interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
-- 
2.25.1

Re: [PATCH v3 4/5] arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region

[Krzysztof Kozlowski]

On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
> Mapping Broadcast_AND region for LLCC in SM8550.

I don't understand this sentence and I still do not know why.

Best regards,
Krzysztof

Re: [PATCH v3 4/5] arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region

[Unnathi Chalicheemala]

On 2/27/2024 7:49 AM, Krzysztof Kozlowski wrote:
> On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
>> Mapping Broadcast_AND region for LLCC in SM8550.
> 
> I don't understand this sentence and I still do not know why.
> 

The check of whether status bit is 1 in the driver is being done
with the wrong register all along (sm8450 onwards). So I am adding
the base address of the right register region in the DeviceTree files.

I can add this explanation to the commit message of these
patches if you think that would help.

> Best regards,
> Krzysztof
> 

Re: [PATCH v3 4/5] arm64: dts: qcom: sm8550: Add mapping to llcc Broadcast_AND region

[Konrad Dybcio]

On 28.02.2024 02:17, Unnathi Chalicheemala wrote:
> On 2/27/2024 7:49 AM, Krzysztof Kozlowski wrote:
>> On 23/02/2024 00:07, Unnathi Chalicheemala wrote:
>>> Mapping Broadcast_AND region for LLCC in SM8550.

"Map" would be grammatically connect here

>>
>> I don't understand this sentence and I still do not know why.
>>
> 
> The check of whether status bit is 1 in the driver is being done
> with the wrong register all along (sm8450 onwards). So I am adding
> the base address of the right register region in the DeviceTree files.
> 
> I can add this explanation to the commit message of these
> patches if you think that would help.

Yes, the commit message should definitely state the problem, and if
not obvious, the reason for the solution.

Paraphrasing Greg KH (I think?), the maintainers are going to assume
your patch is unnecessary and your job is to convince them that it's
not the case. You do it through good code and meaningful commit titles&
messages.

Please refer to [1].

Konrad

[1] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#describe-your-changes