LLCC: Support for Broadcast_AND region

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 <[email protected]>

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 <[email protected]>

---
 drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +11 -1

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -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;
 

include/linux/soc/qcom/llcc-qcom.h +3 -2

index 1a886666bbb6..9e9f528b1370 100644
@@ -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

Quoted 7 lines · 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.

Quoted 6 lines

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?

Quoted 2 lines

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

Separate patch, Fixes tag.

Quoted 70 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
 drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +11 -1

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -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;
 

include/linux/soc/qcom/llcc-qcom.h +3 -2

index 1a886666bbb6..9e9f528b1370 100644
@@ -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

Quoted 16 lines · Dmitry Baryshkov

Quoted 13 lines · 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.

Quoted 2 lines

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.

Quoted 6 lines

Quoted 3 lines

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!

Quoted 72 lines

Quoted 70 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
 drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +11 -1

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -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;
 

include/linux/soc/qcom/llcc-qcom.h +3 -2

index 1a886666bbb6..9e9f528b1370 100644
@@ -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

Quoted 23 lines · Unnathi Chalicheemala

Quoted 18 lines · Dmitry Baryshkov

Quoted 7 lines · 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.

Quoted 6 lines

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?

Quoted 3 lines

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.

Quoted 24 lines

Quoted 6 lines

Quoted 3 lines

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!

Quoted 14 lines

Quoted 14 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
 drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +0 -0

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -849,7 +849,7 @@ static int llcc_update_act_ctrl(u32 sid,
                return ret;
 

Quoted 5 lines

Quoted 5 lines

drivers/soc/qcom/llcc-qcom.c static int llcc_update_act_ctrl(u32 sid, · line 854

Quoted 5 lines

drivers/soc/qcom/llcc-qcom.c +1 -1

        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);

Quoted 14 lines

Quoted 14 lines

Quoted 14 lines

drivers/soc/qcom/llcc-qcom.c +2 -0

                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;
 

Quoted 7 lines

Quoted 7 lines

drivers/soc/qcom/llcc-qcom.c static int qcom_llcc_probe(struct platform_device *pdev) · line 1293

Quoted 7 lines

drivers/soc/qcom/llcc-qcom.c +7 -0

+       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;
+               }
+       }

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

drivers/soc/qcom/llcc-qcom.c +1 -0

+
        llcc_cfg = cfg->sct_data;
        sz = cfg->size;
 

include/linux/soc/qcom/llcc-qcom.h +3 -1

index 1a886666bbb6..9e9f528b1370 100644
@@ -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;

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

include/linux/soc/qcom/llcc-qcom.h +0 -1

        const struct llcc_edac_reg_offset *edac_reg_offset;
        struct mutex lock;
--

2.25.1

Quoted 25 lines · Dmitry Baryshkov

Quoted 23 lines · Unnathi Chalicheemala

Quoted 18 lines · Dmitry Baryshkov

Quoted 7 lines · 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.

Quoted 6 lines

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?

Quoted 9 lines

Quoted 3 lines

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.

Quoted 24 lines

Quoted 24 lines

Quoted 6 lines

Quoted 3 lines

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!

Quoted 14 lines

Quoted 14 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
 drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  4 +++-
 2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +0 -0

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -849,7 +849,7 @@ static int llcc_update_act_ctrl(u32 sid,
                return ret;
 

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/soc/qcom/llcc-qcom.c static int llcc_update_act_ctrl(u32 sid, · line 854

Quoted 5 lines

drivers/soc/qcom/llcc-qcom.c +1 -1

        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.

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

Quoted 15 lines

drivers/soc/qcom/llcc-qcom.c +2 -0

                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;
 

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/soc/qcom/llcc-qcom.c static int qcom_llcc_probe(struct platform_device *pdev) · line 1293

Quoted 7 lines

drivers/soc/qcom/llcc-qcom.c +7 -0

+       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?

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

Quoted 24 lines

drivers/soc/qcom/llcc-qcom.c +1 -0

+
        llcc_cfg = cfg->sct_data;
        sz = cfg->size;
 

include/linux/soc/qcom/llcc-qcom.h +3 -1

index 1a886666bbb6..9e9f528b1370 100644
@@ -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;

Quoted 11 lines

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

include/linux/soc/qcom/llcc-qcom.h +0 -1

        const struct llcc_edac_reg_offset *edac_reg_offset;
        struct mutex lock;
--

2.25.1

--
Thanks & Warm Regards,
Unnathi

Quoted 55 lines · Unnathi Chalicheemala

Quoted 22 lines · Dmitry Baryshkov

Quoted 20 lines · Unnathi Chalicheemala

Quoted 17 lines · Dmitry Baryshkov

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 9 lines

Quoted 4 lines

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.

Quoted 21 lines

Quoted 21 lines

Quoted 4 lines

Quoted 2 lines

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!

Quoted 14 lines

Quoted 14 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
  2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +0 -0

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -849,7 +849,7 @@ static int llcc_update_act_ctrl(u32 sid,
                 return ret;
 

Quoted 13 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/soc/qcom/llcc-qcom.c +1 -1

         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.

Quoted 68 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

Quoted 18 lines

if (ret)

drivers/soc/qcom/llcc-qcom.c +2 -0

@@ -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,

drivers/soc/qcom/llcc-qcom.c +0 -0

@@ -1282,6 +1284,14 @@ static int qcom_llcc_probe(struct
 platform_device *pdev)
 
         drv_data->version = version;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/soc/qcom/llcc-qcom.c static int qcom_llcc_probe(struct platform_device *pdev) · line 1293

Quoted 8 lines

drivers/soc/qcom/llcc-qcom.c +7 -0

+       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?

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

drivers/soc/qcom/llcc-qcom.c +1 -0

+
         llcc_cfg = cfg->sct_data;

         sz = cfg->size;

include/linux/soc/qcom/llcc-qcom.h +2 -1

index 1a886666bbb6..9e9f528b1370 100644
@@ -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

include/linux/soc/qcom/llcc-qcom.h +1 -0

@@ -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;

Quoted 10 lines

Quoted 8 lines

Quoted 8 lines

Quoted 7 lines

include/linux/soc/qcom/llcc-qcom.h +0 -1

         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

Quoted 70 lines · Dmitry Baryshkov

Quoted 68 lines · Unnathi Chalicheemala

Quoted 22 lines · Dmitry Baryshkov

Quoted 20 lines · Unnathi Chalicheemala

Quoted 17 lines · Dmitry Baryshkov

Quoted 6 lines · 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.

Quoted 6 lines

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?

Quoted 9 lines

Quoted 4 lines

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.

Quoted 21 lines

Quoted 21 lines

Quoted 4 lines

Quoted 2 lines

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!

Quoted 14 lines

Quoted 14 lines

Signed-off-by Unnathi Chalicheemala <[email protected]>

---
  drivers/soc/qcom/llcc-qcom.c       | 12 +++++++++++-
  include/linux/soc/qcom/llcc-qcom.h |  4 +++-
  2 files changed, 14 insertions(+), 2 deletions(-)

drivers/soc/qcom/llcc-qcom.c +0 -0

index 4ca88eaebf06..5a2dac2d4772 100644
@@ -849,7 +849,7 @@ static int llcc_update_act_ctrl(u32 sid,
                 return ret;
 

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/soc/qcom/llcc-qcom.c +1 -1

         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.

Quoted 2 lines

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?