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

[PATCH v2 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.

Merging strategy
----------------

All patches should be merged due to LLCC DeviceTree/driver dependency.

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 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 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
  soc: qcom: llcc: Add regmap for Broadcast_AND region

 .../devicetree/bindings/cache/qcom,llcc.yaml  | 41 +++++++++++++++----
 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                  | 22 ++++++++--
 include/linux/soc/qcom/llcc-qcom.h            |  4 +-
 6 files changed, 66 insertions(+), 18 deletions(-)

-- 
2.25.1

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

[Unnathi Chalicheemala]

The LLCC driver 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>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../devicetree/bindings/cache/qcom,llcc.yaml  | 41 +++++++++++++++----
 1 file changed, 33 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 07ccbda4a0ab..b12ac4308284 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
 
@@ -170,13 +195,13 @@ examples:
         #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>;
+        system-cache-controller@25000000 {
+            compatible = "qcom,sm8550-llcc";
+            reg = <0 0x25200000 0 0x200000>, <0 0x25400000 0 0x200000>,
+                <0 0x25600000 0 0x200000>, <0 0x25800000 0 0x200000>,
+                <0 0x25a00000 0 0x200000>;
             reg-names = "llcc0_base", "llcc1_base", "llcc2_base",
-                "llcc3_base", "llcc_broadcast_base";
-            interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
+                "llcc3_base", "llcc_broadcast_base", "llcc_broadcast_and_base";
+            interrupts = <GIC_SPI 266 IRQ_TYPE_LEVEL_HIGH>;
         };
     };
-- 
2.25.1

[PATCH v2 2/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

[PATCH v2 3/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

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

[Unnathi Chalicheemala]

Mapping Broadcast_AND region for LLCC in SM8650.

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

diff --git a/arch/arm64/boot/dts/qcom/sm8650.dtsi b/arch/arm64/boot/dts/qcom/sm8650.dtsi
index 2df77123a8c7..daa5738b955e 100644
--- a/arch/arm64/boot/dts/qcom/sm8650.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8650.dtsi
@@ -4735,12 +4735,14 @@ system-cache-controller@25000000 {
 			      <0 0x25400000 0 0x200000>,
 			      <0 0x25200000 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

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

[Unnathi Chalicheemala]

Define new regmap structure for Broadcast_AND region and initialize
regmap for Broadcast_AND region when HW block version
is greater than 4.1 for backwards compatibility.

Switch from broadcast_OR to broadcast_AND region 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       | 22 +++++++++++++++++++---
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 4ca88eaebf06..fbd2542cd4c5 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -849,9 +849,14 @@ 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,
-				      slice_status, (slice_status & ACT_COMPLETE),
-				      0, LLCC_STATUS_READ_DELAY);
+		if (!drv_data->bcast_and_regmap)
+			ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
+					slice_status, (slice_status & ACT_COMPLETE),
+					0, LLCC_STATUS_READ_DELAY);
+		else
+			ret = regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
+					slice_status, (slice_status & ACT_COMPLETE),
+					0, LLCC_STATUS_READ_DELAY);
 		if (ret)
 			return ret;
 	}
@@ -1282,6 +1287,17 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
+	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
+		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 v2 0/5] LLCC: Support for Broadcast_AND region

[Krzysztof Kozlowski]

On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
> 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.
> 
> Merging strategy
> ----------------
> 
> All patches should be merged due to LLCC DeviceTree/driver dependency.

Dependency? Sorry, there cannot be a dependency between DTS and driver.
Please fix your patchset.



Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
> Mapping Broadcast_AND region for LLCC in SM8450.

Why?

And why your DTS is in the middle of driver changes? Driver cannot
depend on DTS - you are now breaking all existing boards and users.


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
> Define new regmap structure for Broadcast_AND region and initialize
> regmap for Broadcast_AND region when HW block version
> is greater than 4.1 for backwards compatibility.
> 
> Switch from broadcast_OR to broadcast_AND region for checking
> status bit 1 as Broadcast_OR region checks only for bit 0.
> 

Driver changes cannot be after DTS, because there should be no
dependency between hardware description and drivers. Hardware did not
change, did it? Your patchset might have wrong ordering or might break
bisectability.


Best regards,
Krzysztof

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

[Rob Herring]

On Mon, 05 Feb 2024 23:15:52 -0800, Unnathi Chalicheemala wrote:
> The LLCC driver 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>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../devicetree/bindings/cache/qcom,llcc.yaml  | 41 +++++++++++++++----
>  1 file changed, 33 insertions(+), 8 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/cache/qcom,llcc.example.dtb: system-cache-controller@25000000: reg: [[0, 622854144, 0, 2097152], [0, 624951296, 0, 2097152], [0, 627048448, 0, 2097152], [0, 629145600, 0, 2097152], [0, 631242752, 0, 2097152]] is too short
	from schema $id: http://devicetree.org/schemas/cache/qcom,llcc.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/53ba029fb1e3f66505b9f6487030e5d981e1621f.1707202761.git.quic_uchalich@quicinc.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

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

[Konrad Dybcio]

On 6.02.2024 08:15, Unnathi Chalicheemala wrote:
> Define new regmap structure for Broadcast_AND region and initialize
> regmap for Broadcast_AND region when HW block version
> is greater than 4.1 for backwards compatibility.

Are they actually separate regions and not a single contiguous one?

> 
> Switch from broadcast_OR to broadcast_AND region 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       | 22 +++++++++++++++++++---
>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 4ca88eaebf06..fbd2542cd4c5 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -849,9 +849,14 @@ 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,
> -				      slice_status, (slice_status & ACT_COMPLETE),
> -				      0, LLCC_STATUS_READ_DELAY);
> +		if (!drv_data->bcast_and_regmap)
> +			ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
> +					slice_status, (slice_status & ACT_COMPLETE),
> +					0, LLCC_STATUS_READ_DELAY);
> +		else
> +			ret = regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
> +					slice_status, (slice_status & ACT_COMPLETE),
> +					0, LLCC_STATUS_READ_DELAY);

struct regmap *regmap = drv_data->bcast_and_regmap ?: bcast_regmap;

?

> +	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {

This check is rather redundant.. If there's no such region in hardware,
it won't be described, and as such the _get()s will return some sort
of an error.

Might as well make it a comment that it's intended for >=v4.1 and
definitely leave a comment for the next guy that there's a backwards
compatibility quirk involved..

Konrad

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

[Unnathi Chalicheemala]

On 2/6/2024 12:34 AM, Krzysztof Kozlowski wrote:
> On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
>> 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.
>>
>> Merging strategy
>> ----------------
>>
>> All patches should be merged due to LLCC DeviceTree/driver dependency.
> 
> Dependency? Sorry, there cannot be a dependency between DTS and driver.
> Please fix your patchset.
> 

This is the suggested merge strategy. But I have updated the driver in v2
to ensure that applying the driver patch alone will work with existing DTs.
Sorry for being unclear; "should" does make it sound like there is a hard
dependency. 

> 
> 
> Best regards,
> Krzysztof
> 

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

[Krzysztof Kozlowski]

On 06/02/2024 20:41, Unnathi Chalicheemala wrote:
> On 2/6/2024 12:34 AM, Krzysztof Kozlowski wrote:
>> On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
>>> 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.
>>>
>>> Merging strategy
>>> ----------------
>>>
>>> All patches should be merged due to LLCC DeviceTree/driver dependency.
>>
>> Dependency? Sorry, there cannot be a dependency between DTS and driver.
>> Please fix your patchset.
>>
> 
> This is the suggested merge strategy. But I have updated the driver in v2
> to ensure that applying the driver patch alone will work with existing DTs.
> Sorry for being unclear; "should" does make it sound like there is a hard
> dependency. 

OK, anyway, the patch was not tested.

Please drop my Reviewed-by and test the patch before sending.

Best regards,
Krzysztof

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

[Unnathi Chalicheemala]

On 2/6/2024 12:35 AM, Krzysztof Kozlowski wrote:
> On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
>> Mapping Broadcast_AND region for LLCC in SM8450.
> 
> Why?
> 
> And why your DTS is in the middle of driver changes? Driver cannot
> depend on DTS - you are now breaking all existing boards and users.
> 

I was following a similar patch which has DT and driver changes in the
same patchset:
https://lore.kernel.org/all/20230314080443.64635-1-manivannan.sadhasivam@linaro.org/

The AND region was added in the IP block in SM8450, but was not added to the DT or
driver. That is why I included both in the same patchset - if you think the DT
changes should be separate I can correct it in the next version.

Thanks a lot for taking the time to review Krzysztof.

> 
> Best regards,
> Krzysztof
> 

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

[Unnathi Chalicheemala]

On 2/6/2024 10:42 AM, Konrad Dybcio wrote:
> On 6.02.2024 08:15, Unnathi Chalicheemala wrote:
>> Define new regmap structure for Broadcast_AND region and initialize
>> regmap for Broadcast_AND region when HW block version
>> is greater than 4.1 for backwards compatibility.
> 
> Are they actually separate regions and not a single contiguous one?
> 

Yes, they are separate regions.

>>
>> Switch from broadcast_OR to broadcast_AND region 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       | 22 +++++++++++++++++++---
>>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>>  2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 4ca88eaebf06..fbd2542cd4c5 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -849,9 +849,14 @@ 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,
>> -				      slice_status, (slice_status & ACT_COMPLETE),
>> -				      0, LLCC_STATUS_READ_DELAY);
>> +		if (!drv_data->bcast_and_regmap)
>> +			ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
>> +					slice_status, (slice_status & ACT_COMPLETE),
>> +					0, LLCC_STATUS_READ_DELAY);
>> +		else
>> +			ret = regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
>> +					slice_status, (slice_status & ACT_COMPLETE),
>> +					0, LLCC_STATUS_READ_DELAY);
> 
> struct regmap *regmap = drv_data->bcast_and_regmap ?: bcast_regmap;
> 
> ?

Ack. Will minimize the redundancy.

> 
>> +	if (drv_data->version >= LLCC_VERSION_4_1_0_0) {
> 
> This check is rather redundant.. If there's no such region in hardware,
> it won't be described, and as such the _get()s will return some sort
> of an error.
> 

I see what you're saying.

> Might as well make it a comment that it's intended for >=v4.1 and
> definitely leave a comment for the next guy that there's a backwards
> compatibility quirk involved..

Ack.
Thanks for the review Konrad!

> 
> Konrad
> 

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

[Dmitry Baryshkov]

On Tue, 6 Feb 2024 at 22:07, Unnathi Chalicheemala
<quic_uchalich@quicinc.com> wrote:
>
> On 2/6/2024 12:35 AM, Krzysztof Kozlowski wrote:
> > On 06/02/2024 08:15, Unnathi Chalicheemala wrote:
> >> Mapping Broadcast_AND region for LLCC in SM8450.
> >
> > Why?
> >
> > And why your DTS is in the middle of driver changes? Driver cannot
> > depend on DTS - you are now breaking all existing boards and users.
> >
>
> I was following a similar patch which has DT and driver changes in the
> same patchset:
> https://lore.kernel.org/all/20230314080443.64635-1-manivannan.sadhasivam@linaro.org/
>
> The AND region was added in the IP block in SM8450, but was not added to the DT or
> driver. That is why I included both in the same patchset - if you think the DT
> changes should be separate I can correct it in the next version.

Just move the DT changes to be the last patches in the patchset.

>
> Thanks a lot for taking the time to review Krzysztof.
>
> >
> > Best regards,
> > Krzysztof
> >
>


-- 
With best wishes
Dmitry