Re: [PATCH 5/5] soc: qcom: llcc: Add regmap for Broadcast_AND region
From: Satya Durga Srinivasu Prabhala
Date: Sun Feb 04 2024 - 20:01:39 EST
On 1/30/2024 10:57 AM, Dmitry Baryshkov wrote:
On Tue, 30 Jan 2024 at 19:52, Unnathi Chalicheemala
<quic_uchalich@xxxxxxxxxxx> wrote:
On 1/29/2024 2:03 PM, Dmitry Baryshkov wrote:
On Mon, 29 Jan 2024 at 20:17, Unnathi Chalicheemala
<quic_uchalich@xxxxxxxxxxx> wrote:
On 1/26/2024 12:29 PM, Dmitry Baryshkov wrote:
On Fri, 26 Jan 2024 at 21:48, Unnathi Chalicheemala
<quic_uchalich@xxxxxxxxxxx> 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@xxxxxxxxxxx>
---
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