diff mbox series

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

Message ID 169277f53affed98ef41e5a7cbf2401fe62716bd.1707202761.git.quic_uchalich@quicinc.com
State New
Headers
Series LLCC: Support for Broadcast_AND region |

Commit Message

Unnathi Chalicheemala Feb. 6, 2024, 7:15 a.m. UTC 
  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(-)

Comments

Krzysztof Kozlowski Feb. 6, 2024, 8:40 a.m. UTC | #1 
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
Konrad Dybcio Feb. 6, 2024, 6:42 p.m. UTC | #2 
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
Unnathi Chalicheemala Feb. 6, 2024, 9:45 p.m. UTC | #3 
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
>
diff mbox series

Patch

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;