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

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

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  | 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                  | 12 ++++++++-
 include/linux/soc/qcom/llcc-qcom.h            |  4 ++-
 6 files changed, 51 insertions(+), 9 deletions(-)

-- 
2.25.1

Best Regards,
Unnathi

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

[Unnathi Chalicheemala]

The LLCC block in SM8450, SM8550 and SM8650 has 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

[PATCH 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 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 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 5/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

[Unnathi Chalicheemala]

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.

Update llcc_drv_data structure with new regmap 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.
While at it, also check return value after reading Broadcast_OR
region in llcc_update_act_ctrl().

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

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 4ca88eaebf06..5a2dac2d4772 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -849,7 +849,7 @@ 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,
+		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)
@@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
 	ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
 				      slice_status, !(slice_status & status),
 				      0, LLCC_STATUS_READ_DELAY);
+	if (ret)
+		return ret;
 
 	if (drv_data->version >= LLCC_VERSION_4_1_0_0)
 		ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
@@ -1282,6 +1284,14 @@ 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);
+			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 5/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

[Dmitry Baryshkov]

On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
<quic_uchalich@quicinc.com> wrote:
>
> 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.
>
> Update llcc_drv_data structure with new regmap 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.

This breaks backwards compatibility with the existing DT files, doesn't it?

> While at it, also check return value after reading Broadcast_OR
> region in llcc_update_act_ctrl().

Separate patch, Fixes tag.

>
> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 4ca88eaebf06..5a2dac2d4772 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -849,7 +849,7 @@ 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,
> +               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)
> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
>         ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
>                                       slice_status, !(slice_status & status),
>                                       0, LLCC_STATUS_READ_DELAY);
> +       if (ret)
> +               return ret;
>
>         if (drv_data->version >= LLCC_VERSION_4_1_0_0)
>                 ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
> @@ -1282,6 +1284,14 @@ 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);
> +                       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
>
>


-- 
With best wishes
Dmitry

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

[Krzysztof Kozlowski]

On 26/01/2024 20:47, Unnathi Chalicheemala wrote:
> The LLCC block in SM8450, SM8550 and SM8650 has 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>

Best regards,
Krzysztof

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

[Unnathi Chalicheemala]

On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
> <quic_uchalich@quicinc.com> wrote:
>>
>> 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.
>>
>> Update llcc_drv_data structure with new regmap 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.
> 
> This breaks backwards compatibility with the existing DT files, doesn't it?
> 

It shouldn't as checking for status bit 1 is happening only when the block
version is greater than 4.1, which is when Broadcast_AND region support
is added.

>> While at it, also check return value after reading Broadcast_OR
>> region in llcc_update_act_ctrl().
> 
> Separate patch, Fixes tag.
> 

Ack. Will remove this from existing patch.
Thanks for the review Dmitry!

>>
>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>> ---
>>  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
>>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 4ca88eaebf06..5a2dac2d4772 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -849,7 +849,7 @@ 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,
>> +               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)
>> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
>>         ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
>>                                       slice_status, !(slice_status & status),
>>                                       0, LLCC_STATUS_READ_DELAY);
>> +       if (ret)
>> +               return ret;
>>
>>         if (drv_data->version >= LLCC_VERSION_4_1_0_0)
>>                 ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
>> @@ -1282,6 +1284,14 @@ 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);
>> +                       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 5/5] soc: qcom: llcc: Add regmap for Broadcast_AND region

[Dmitry Baryshkov]

On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
<quic_uchalich@quicinc.com> wrote:
>
>
>
> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
> > On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
> > <quic_uchalich@quicinc.com> wrote:
> >>
> >> 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.
> >>
> >> Update llcc_drv_data structure with new regmap 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.
> >
> > This breaks backwards compatibility with the existing DT files, doesn't it?
> >
>
> It shouldn't as checking for status bit 1 is happening only when the block
> version is greater than 4.1, which is when Broadcast_AND region support
> is added.

Let me reiterate, please: with the existing DT files. You are patching
DT files in patches 2-4, but this is not enough. DT files are
considered to be ABI. As such old DT files must continue to work with
newer kernels.

>
> >> While at it, also check return value after reading Broadcast_OR
> >> region in llcc_update_act_ctrl().
> >
> > Separate patch, Fixes tag.
> >
>
> Ack. Will remove this from existing patch.
> Thanks for the review Dmitry!
>
> >>
> >> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> >> ---
> >>  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
> >>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
> >>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> >> index 4ca88eaebf06..5a2dac2d4772 100644
> >> --- a/drivers/soc/qcom/llcc-qcom.c
> >> +++ b/drivers/soc/qcom/llcc-qcom.c
> >> @@ -849,7 +849,7 @@ 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,
> >> +               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)
> >> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
> >>         ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
> >>                                       slice_status, !(slice_status & status),
> >>                                       0, LLCC_STATUS_READ_DELAY);
> >> +       if (ret)
> >> +               return ret;
> >>
> >>         if (drv_data->version >= LLCC_VERSION_4_1_0_0)
> >>                 ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
> >> @@ -1282,6 +1284,14 @@ 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);
> >> +                       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
> >>
> >>
> >
> >



-- 
With best wishes
Dmitry

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

[Unnathi Chalicheemala]

On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
> On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
> <quic_uchalich@quicinc.com> wrote:
>>
>>
>>
>> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
>>> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
>>> <quic_uchalich@quicinc.com> wrote:
>>>>
>>>> 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.
>>>>
>>>> Update llcc_drv_data structure with new regmap 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.
>>>
>>> This breaks backwards compatibility with the existing DT files, doesn't it?
>>>
>>
>> It shouldn't as checking for status bit 1 is happening only when the block
>> version is greater than 4.1, which is when Broadcast_AND region support
>> is added.
> 
> Let me reiterate, please: with the existing DT files. You are patching
> DT files in patches 2-4, but this is not enough. DT files are
> considered to be ABI. As such old DT files must continue to work with
> newer kernels.
>

I'm sorry, I think I'm not understanding this right.
 
>>
>>>> While at it, also check return value after reading Broadcast_OR
>>>> region in llcc_update_act_ctrl().
>>>
>>> Separate patch, Fixes tag.
>>>
>>
>> Ack. Will remove this from existing patch.
>> Thanks for the review Dmitry!
>>
>>>>
>>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>>>> ---
>>>>  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
>>>>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>>>>  2 files changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>>> index 4ca88eaebf06..5a2dac2d4772 100644
>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>> @@ -849,7 +849,7 @@ 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,
>>>> +               ret = regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
>>>>                                       slice_status, (slice_status & ACT_COMPLETE),
>>>>                                       0, LLCC_STATUS_READ_DELAY);

Above if condition will be true only for SM8450, 8550 and 8650 - whose DT files have been changed.
It would never check for other existing DT files - I guess I'm failing to understand why the code
would break with other DeviceTree files.

>>>>                 if (ret)
>>>> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
>>>>         ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
>>>>                                       slice_status, !(slice_status & status),
>>>>                                       0, LLCC_STATUS_READ_DELAY);
>>>> +       if (ret)
>>>> +               return ret;
>>>>
>>>>         if (drv_data->version >= LLCC_VERSION_4_1_0_0)
>>>>                 ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
>>>> @@ -1282,6 +1284,14 @@ 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);
>>>> +                       goto err;
>>>> +               }
>>>> +       }

I have added a similar check in the probe function above; are you saying this too will break with
existing DT files? 

>>>> +
>>>>         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
>>>>
>>>>
>>>
>>>
> 
> 
> 
--
Thanks & Warm Regards,
Unnathi

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

[Dmitry Baryshkov]

On Tue, 30 Jan 2024 at 19:52, Unnathi Chalicheemala
<quic_uchalich@quicinc.com> wrote:
>
> On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
> > On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
> > <quic_uchalich@quicinc.com> wrote:
> >>
> >>
> >>
> >> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
> >>> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
> >>> <quic_uchalich@quicinc.com> wrote:
> >>>>
> >>>> 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.
> >>>>
> >>>> Update llcc_drv_data structure with new regmap 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.
> >>>
> >>> This breaks backwards compatibility with the existing DT files, doesn't it?
> >>>
> >>
> >> It shouldn't as checking for status bit 1 is happening only when the block
> >> version is greater than 4.1, which is when Broadcast_AND region support
> >> is added.
> >
> > Let me reiterate, please: with the existing DT files. You are patching
> > DT files in patches 2-4, but this is not enough. DT files are
> > considered to be ABI. As such old DT files must continue to work with
> > newer kernels.
> >
>
> I'm sorry, I think I'm not understanding this right.
>
> >>
> >>>> While at it, also check return value after reading Broadcast_OR
> >>>> region in llcc_update_act_ctrl().
> >>>
> >>> Separate patch, Fixes tag.
> >>>
> >>
> >> Ack. Will remove this from existing patch.
> >> Thanks for the review Dmitry!
> >>
> >>>>
> >>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> >>>> ---
> >>>>  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
> >>>>  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
> >>>>  2 files changed, 14 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> >>>> index 4ca88eaebf06..5a2dac2d4772 100644
> >>>> --- a/drivers/soc/qcom/llcc-qcom.c
> >>>> +++ b/drivers/soc/qcom/llcc-qcom.c
> >>>> @@ -849,7 +849,7 @@ 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,
> >>>> +               ret = regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
> >>>>                                       slice_status, (slice_status & ACT_COMPLETE),
> >>>>                                       0, LLCC_STATUS_READ_DELAY);
>
> Above if condition will be true only for SM8450, 8550 and 8650 - whose DT files have been changed.
> It would never check for other existing DT files - I guess I'm failing to understand why the code
> would break with other DeviceTree files.

I'm saying that the driver must continue to work (well, at least not
to crash) even if somebody runs the kernel with older DT.

>
> >>>>                 if (ret)
> >>>> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
> >>>>         ret = regmap_read_poll_timeout(drv_data->bcast_regmap, status_reg,
> >>>>                                       slice_status, !(slice_status & status),
> >>>>                                       0, LLCC_STATUS_READ_DELAY);
> >>>> +       if (ret)
> >>>> +               return ret;
> >>>>
> >>>>         if (drv_data->version >= LLCC_VERSION_4_1_0_0)
> >>>>                 ret = regmap_write(drv_data->bcast_regmap, act_clear_reg,
> >>>> @@ -1282,6 +1284,14 @@ 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);
> >>>> +                       goto err;
> >>>> +               }
> >>>> +       }
>
> I have added a similar check in the probe function above; are you saying this too will break with
> existing DT files?
>
> >>>> +
> >>>>         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
> >>>>
> >>>>
> >>>
> >>>
> >
> >
> >
> --
> Thanks & Warm Regards,
> Unnathi



-- 
With best wishes
Dmitry

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

[Satya Durga Srinivasu Prabhala]

On 1/30/2024 10:57 AM, Dmitry Baryshkov wrote:
> On Tue, 30 Jan 2024 at 19:52, Unnathi Chalicheemala
> <quic_uchalich@quicinc.com> wrote:
>> On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
>>> On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
>>> <quic_uchalich@quicinc.com> wrote:
>>>>
>>>>
>>>> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
>>>>> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
>>>>> <quic_uchalich@quicinc.com> wrote:
>>>>>> 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.
>>>>>>
>>>>>> Update llcc_drv_data structure with new regmap 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.
>>>>> This breaks backwards compatibility with the existing DT files,
>>>>> doesn't it?
>>>>>
>>>> It shouldn't as checking for status bit 1 is happening only when the
>>>> block
>>>> version is greater than 4.1, which is when Broadcast_AND region support
>>>> is added.
>>> Let me reiterate, please: with the existing DT files. You are patching
>>> DT files in patches 2-4, but this is not enough. DT files are
>>> considered to be ABI. As such old DT files must continue to work with
>>> newer kernels.
>>>
>> I'm sorry, I think I'm not understanding this right.
>>
>>>>>> While at it, also check return value after reading Broadcast_OR
>>>>>> region in llcc_update_act_ctrl().
>>>>> Separate patch, Fixes tag.
>>>>>
>>>> Ack. Will remove this from existing patch.
>>>> Thanks for the review Dmitry!
>>>>
>>>>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
>>>>>> ---
>>>>>>   drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
>>>>>>   include/linux/soc/qcom/llcc-qcom.h |  4 +++-
>>>>>>   2 files changed, 14 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c
>>>>>> b/drivers/soc/qcom/llcc-qcom.c
>>>>>> index 4ca88eaebf06..5a2dac2d4772 100644
>>>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>>>> @@ -849,7 +849,7 @@ 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,
>>>>>> +               ret =
>>>>>> regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
>>>>>>                                        slice_status, (slice_status &
>>>>>> ACT_COMPLETE),
>>>>>>                                        0, LLCC_STATUS_READ_DELAY);
>> Above if condition will be true only for SM8450, 8550 and 8650 - whose DT
>> files have been changed.
>> It would never check for other existing DT files - I guess I'm failing to
>> understand why the code
>> would break with other DeviceTree files.
> I'm saying that the driver must continue to work (well, at least not
> to crash) even if somebody runs the kernel with older DT.
Thanks Dmitry. While I get the ask, wondering why someone would use old 
DT while DT
is also being updated in this series along with the driver change?

Unnathi, you add check and make sure to update only when bcast_and 
region is specified in DT,
otherwise, stick to what is being done today, that way backwards 
compatibility is maintained.
>>>>>>                  if (ret)
>>>>>> @@ -859,6 +859,8 @@ static int llcc_update_act_ctrl(u32 sid,
>>>>>>          ret = regmap_read_poll_timeout(drv_data->bcast_regmap,
>>>>>> status_reg,
>>>>>>                                        slice_status, !(slice_status &
>>>>>> status),
>>>>>>                                        0, LLCC_STATUS_READ_DELAY);
>>>>>> +       if (ret)
>>>>>> +               return ret;
>>>>>>
>>>>>>          if (drv_data->version >= LLCC_VERSION_4_1_0_0)
>>>>>>                  ret = regmap_write(drv_data->bcast_regmap,
>>>>>> act_clear_reg,
>>>>>> @@ -1282,6 +1284,14 @@ 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);
>>>>>> +                       goto err;
>>>>>> +               }
>>>>>> +       }
>> I have added a similar check in the probe function above; are you saying
>> this too will break with
>> existing DT files?
>>
>>>>>> +
>>>>>>          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
>>>>>>
>>>>>>
>>>>>
>>>
>>>
>> --
>> Thanks & Warm Regards,
>> Unnathi
>
>

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

[Dmitry Baryshkov]

On Mon, 5 Feb 2024 at 02:01, Satya Durga Srinivasu Prabhala
<quic_satyap@quicinc.com> wrote:
>
>
> On 1/30/2024 10:57 AM, Dmitry Baryshkov wrote:
> > On Tue, 30 Jan 2024 at 19:52, Unnathi Chalicheemala
> > <quic_uchalich@quicinc.com> wrote:
> >> On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
> >>> On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
> >>> <quic_uchalich@quicinc.com> wrote:
> >>>>
> >>>>
> >>>> On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
> >>>>> On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
> >>>>> <quic_uchalich@quicinc.com> wrote:
> >>>>>> 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.
> >>>>>>
> >>>>>> Update llcc_drv_data structure with new regmap 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.
> >>>>> This breaks backwards compatibility with the existing DT files,
> >>>>> doesn't it?
> >>>>>
> >>>> It shouldn't as checking for status bit 1 is happening only when the
> >>>> block
> >>>> version is greater than 4.1, which is when Broadcast_AND region support
> >>>> is added.
> >>> Let me reiterate, please: with the existing DT files. You are patching
> >>> DT files in patches 2-4, but this is not enough. DT files are
> >>> considered to be ABI. As such old DT files must continue to work with
> >>> newer kernels.
> >>>
> >> I'm sorry, I think I'm not understanding this right.
> >>
> >>>>>> While at it, also check return value after reading Broadcast_OR
> >>>>>> region in llcc_update_act_ctrl().
> >>>>> Separate patch, Fixes tag.
> >>>>>
> >>>> Ack. Will remove this from existing patch.
> >>>> Thanks for the review Dmitry!
> >>>>
> >>>>>> Signed-off-by: Unnathi Chalicheemala <quic_uchalich@quicinc.com>
> >>>>>> ---
> >>>>>>   drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
> >>>>>>   include/linux/soc/qcom/llcc-qcom.h |  4 +++-
> >>>>>>   2 files changed, 14 insertions(+), 2 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c
> >>>>>> b/drivers/soc/qcom/llcc-qcom.c
> >>>>>> index 4ca88eaebf06..5a2dac2d4772 100644
> >>>>>> --- a/drivers/soc/qcom/llcc-qcom.c
> >>>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
> >>>>>> @@ -849,7 +849,7 @@ 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,
> >>>>>> +               ret =
> >>>>>> regmap_read_poll_timeout(drv_data->bcast_and_regmap, status_reg,
> >>>>>>                                        slice_status, (slice_status &
> >>>>>> ACT_COMPLETE),
> >>>>>>                                        0, LLCC_STATUS_READ_DELAY);
> >> Above if condition will be true only for SM8450, 8550 and 8650 - whose DT
> >> files have been changed.
> >> It would never check for other existing DT files - I guess I'm failing to
> >> understand why the code
> >> would break with other DeviceTree files.
> > I'm saying that the driver must continue to work (well, at least not
> > to crash) even if somebody runs the kernel with older DT.
> Thanks Dmitry. While I get the ask, wondering why someone would use old
> DT while DT
> is also being updated in this series along with the driver change?

This is quite simple. The DT might be coming from the sources other
than just the kernel. It might be a part of the firmware. It might be
stored separately in the /boot partition, etc. The rule of thumb is
that you must never depend on the DT being updated together with the
kernel. This is one of the reasons why DT maintainers spend so much
time and effort to get DT bindings right in the first place. Once they
are merged, we have to support old DT nearly forever.


--
With best wishes
Dmitry