[PATCH 0/3] Fix BWMONv4 for <SDM845

[PATCH 0/3] Fix BWMONv4 for <SDM845

[Konrad Dybcio]

BWMONv4 (the one used for DDR scaling on all SoCs from msm8998 to sm8550)
features two register regions: "monitor" and "global" with the first one
containing registers specific to the throughput monitor itself and the
second one containing some sort of a head switch.

The register layout on all BWMON versions an implementations up to that
looked like this:

|..........[GLOBAL].........[MONITOR]........|

however with SDM845 somebody thought it would be a good idea to turn it
into this:

|................[GLOBAL]....................|
|....................[MONITOR]...............|

Sadly, the existing upstream driver was architected with SDM845 in mind,
which means it doesn't support the global registers being somewhere else
than near the beginning of the monitor space. This series tries to address
that in the hopefully least painful way. Tested on msm8998 (the count unit
seems to be wrong, should probably be 1MiB and not 64 KiB but the point is
that this series makes it work at all, as without it the headswitch is
never turned on) and SM6375 (with the "combined" layout introduced in
SDM845). Equally sadly, everybody uses the qcom,msm8998-bwmon compatible
(which frankly should have been just qcom,bwmon-v4) that never actually
worked on MSM8998 , which prevents us from handling it in a simpler way..

While at it, an unused struct member is removed.

One suboptimal feature of this patchset is that it introduces an "invalid
resource" print from within devres. This could be solved with an
introduction of devm_ioremap_resource_optional or by dropping devres
functions in place of manual handling, which also doesn't sound great..
I'll leave it up to the reviewers to decide.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
Konrad Dybcio (3):
      dt-bindings: interconnect: qcom,msm8998-bwmon: Add global registers
      soc: qcom: icc-bwmon: Handle global registers correctly
      soc: qcom: icc-bwmon: Remove unused struct member

 .../bindings/interconnect/qcom,msm8998-bwmon.yaml  |  28 ++++-
 drivers/soc/qcom/icc-bwmon.c                       | 137 ++++++++++++++++++---
 2 files changed, 142 insertions(+), 23 deletions(-)
---
base-commit: 1acf39ef8f1425cd105f630dc2c7c1d8fff27ed1
change-id: 20230304-topic-ddr_bwmon-609022cd5e35

Best regards,
-- 
Konrad Dybcio <konrad.dybcio@linaro.org>

[PATCH 1/3] dt-bindings: interconnect: qcom,msm8998-bwmon: Add global registers

[Konrad Dybcio]

The BWMON has two sets of registers: one for handling the monitor itself
and one called "global", which we didn't care about before, as on newer
SoCs it was made contiguous with (but not the same as) the monitor's
register range. Describe it.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../bindings/interconnect/qcom,msm8998-bwmon.yaml  | 28 ++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
index 12a0d3ecbabb..6dd0cb0a1f43 100644
--- a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
+++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
@@ -49,9 +49,13 @@ properties:
     type: object
 
   reg:
-    # BWMON v4 (currently described) and BWMON v5 use one register address
-    # space.  BWMON v2 uses two register spaces - not yet described.
-    maxItems: 1
+    # BWMON v5 uses one register address space, v1-v4 use one or two.
+    minItems: 1
+    maxItems: 2
+
+  reg-names:
+    minItems: 1
+    maxItems: 2
 
 required:
   - compatible
@@ -63,6 +67,21 @@ required:
 
 additionalProperties: false
 
+allOf:
+  - if:
+      properties:
+        compatible:
+          const: qcom,msm8998-bwmon
+    then:
+      properties:
+        reg:
+          minItems: 2
+
+        reg-names:
+          items:
+            - const: monitor
+            - const: global
+
 examples:
   - |
     #include <dt-bindings/interconnect/qcom,sdm845.h>
@@ -70,7 +89,8 @@ examples:
 
     pmu@1436400 {
         compatible = "qcom,sdm845-bwmon", "qcom,msm8998-bwmon";
-        reg = <0x01436400 0x600>;
+        reg = <0x01436400 0x600>, <0x01436300 0x200>;
+        reg-names = "monitor", "global";
         interrupts = <GIC_SPI 581 IRQ_TYPE_LEVEL_HIGH>;
         interconnects = <&gladiator_noc MASTER_APPSS_PROC 3 &mem_noc SLAVE_LLCC 3>;
 

-- 
2.39.2

[PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Konrad Dybcio]

The BWMON hardware has two sets of registers: one for the monitor itself
and one called "global". It has what seems to be some kind of a head
switch and an interrupt control register. It's usually 0x200 in size.

On fairly recent SoCs (with the starting point seemingly being moving
the OSM programming to the firmware) these two register sets are
contiguous and overlapping, like this (on sm8450):

/* notice how base.start == global_base.start+0x100 */
reg = <0x90b6400 0x300>, <0x90b6300 0x200>;
reg-names = "base", "global_base";

Which led to some confusion and the assumption that since the
"interesting" global registers begin right after global_base+0x100,
there's no need to map two separate regions and one can simply subtract
0x100 from the offsets.

This is however not the case for anything older than SDM845, as the
global region can appear in seemingly random spots on the register map.

Add support for it to let bwmon function on older SoCs like MSM8998 and
allow operation with just one set of registers for newer platforms.

Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/soc/qcom/icc-bwmon.c | 136 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 118 insertions(+), 18 deletions(-)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index d07be3700db6..9ef632d80ee3 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -34,14 +34,27 @@
 /* Internal sampling clock frequency */
 #define HW_TIMER_HZ				19200000
 
-#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x008
-#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x00c
+#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x108
+#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x10c
 /*
  * All values here and further are matching regmap fields, so without absolute
  * register offsets.
  */
 #define BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE	BIT(0)
 
+/*
+ * Starting with SDM845, the BWMON4 register space has changed a bit:
+ * the global registers were jammed into the beginning of the monitor region.
+ * To keep the proper offsets, one would have to map <GLOBAL_BASE 0x200> and
+ * <GLOBAL_BASE+0x100 0x300>, which is straight up wrong.
+ * To facilitate for that, while allowing the older, arguably more proper
+ * implementations to work, offset the global registers by -0x100 to avoid
+ * having to map half of the global registers twice.
+ */
+#define BWMON_V4_845_OFFSET			0x100
+#define BWMON_V4_GLOBAL_IRQ_CLEAR_845		(BWMON_V4_GLOBAL_IRQ_CLEAR - BWMON_V4_845_OFFSET)
+#define BWMON_V4_GLOBAL_IRQ_ENABLE_845		(BWMON_V4_GLOBAL_IRQ_ENABLE - BWMON_V4_845_OFFSET)
+
 #define BWMON_V4_IRQ_STATUS			0x100
 #define BWMON_V4_IRQ_CLEAR			0x108
 
@@ -118,8 +131,10 @@
 #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
 
 enum bwmon_fields {
-	F_GLOBAL_IRQ_CLEAR,
-	F_GLOBAL_IRQ_ENABLE,
+	/* Fields used only on >=SDM845 with BWMON_HAS_GLOBAL_IRQ */
+	F_GLB_IRQ_CLEAR,
+	F_GLB_IRQ_ENABLE,
+
 	F_IRQ_STATUS,
 	F_IRQ_CLEAR,
 	F_IRQ_ENABLE,
@@ -145,6 +160,13 @@ enum bwmon_fields {
 	F_NUM_FIELDS
 };
 
+enum bwmon_global_fields {
+	F_GLOBAL_IRQ_CLEAR,
+	F_GLOBAL_IRQ_ENABLE,
+
+	F_NUM_GLOBAL_FIELDS
+};
+
 struct icc_bwmon_data {
 	unsigned int sample_ms;
 	unsigned int count_unit_kb; /* kbytes */
@@ -157,6 +179,9 @@ struct icc_bwmon_data {
 
 	const struct regmap_config *regmap_cfg;
 	const struct reg_field *regmap_fields;
+
+	const struct regmap_config *global_regmap_cfg;
+	const struct reg_field *global_regmap_fields;
 };
 
 struct icc_bwmon {
@@ -166,6 +191,7 @@ struct icc_bwmon {
 
 	struct regmap *regmap;
 	struct regmap_field *regs[F_NUM_FIELDS];
+	struct regmap_field *global_regs[F_NUM_FIELDS];
 
 	unsigned int max_bw_kbps;
 	unsigned int min_bw_kbps;
@@ -175,8 +201,8 @@ struct icc_bwmon {
 
 /* BWMON v4 */
 static const struct reg_field msm8998_bwmon_reg_fields[] = {
-	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
-	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
+	[F_GLB_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0, 0),
+	[F_GLB_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE_845, 0, 0),
 	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V4_IRQ_STATUS, 4, 7),
 	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V4_IRQ_CLEAR, 4, 7),
 	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V4_IRQ_ENABLE, 4, 7),
@@ -202,7 +228,7 @@ static const struct reg_field msm8998_bwmon_reg_fields[] = {
 };
 
 static const struct regmap_range msm8998_bwmon_reg_noread_ranges[] = {
-	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
+	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR_845, BWMON_V4_GLOBAL_IRQ_CLEAR_845),
 	regmap_reg_range(BWMON_V4_IRQ_CLEAR, BWMON_V4_IRQ_CLEAR),
 	regmap_reg_range(BWMON_V4_CLEAR, BWMON_V4_CLEAR),
 };
@@ -222,16 +248,34 @@ static const struct regmap_access_table msm8998_bwmon_reg_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(msm8998_bwmon_reg_volatile_ranges),
 };
 
+static const struct reg_field msm8998_bwmon_global_reg_fields[] = {
+	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
+	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
+};
+
+static const struct regmap_range msm8998_bwmon_global_reg_noread_ranges[] = {
+	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
+};
+
+static const struct regmap_access_table msm8998_bwmon_global_reg_read_table = {
+	.no_ranges	= msm8998_bwmon_global_reg_noread_ranges,
+	.n_no_ranges	= ARRAY_SIZE(msm8998_bwmon_global_reg_noread_ranges),
+};
+
 /*
  * Fill the cache for non-readable registers only as rest does not really
  * matter and can be read from the device.
  */
 static const struct reg_default msm8998_bwmon_reg_defaults[] = {
-	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
+	{ BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0x0 },
 	{ BWMON_V4_IRQ_CLEAR, 0x0 },
 	{ BWMON_V4_CLEAR, 0x0 },
 };
 
+static const struct reg_default msm8998_bwmon_global_reg_defaults[] = {
+	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
+};
+
 static const struct regmap_config msm8998_bwmon_regmap_cfg = {
 	.reg_bits		= 32,
 	.reg_stride		= 4,
@@ -252,10 +296,27 @@ static const struct regmap_config msm8998_bwmon_regmap_cfg = {
 	.cache_type		= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config msm8998_bwmon_global_regmap_cfg = {
+	.reg_bits		= 32,
+	.reg_stride		= 4,
+	.val_bits		= 32,
+	/*
+	 * No concurrent access expected - driver has one interrupt handler,
+	 * regmap is not shared, no driver or user-space API.
+	 */
+	.disable_locking	= true,
+	.rd_table		= &msm8998_bwmon_global_reg_read_table,
+	.reg_defaults		= msm8998_bwmon_global_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(msm8998_bwmon_global_reg_defaults),
+	/*
+	 * Cache is necessary for using regmap fields with non-readable
+	 * registers.
+	 */
+	.cache_type		= REGCACHE_RBTREE,
+};
+
 /* BWMON v5 */
 static const struct reg_field sdm845_llcc_bwmon_reg_fields[] = {
-	[F_GLOBAL_IRQ_CLEAR]	= {},
-	[F_GLOBAL_IRQ_ENABLE]	= {},
 	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V5_IRQ_STATUS, 0, 3),
 	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V5_IRQ_CLEAR, 0, 3),
 	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V5_IRQ_ENABLE, 0, 3),
@@ -369,16 +430,21 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
 	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
 	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
 		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
-	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
+	if (bwmon->global_regs[0])
+		regmap_field_force_write(bwmon->global_regs[F_GLOBAL_IRQ_CLEAR],
+					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+	else
+		regmap_field_force_write(bwmon->regs[F_GLB_IRQ_CLEAR],
 					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
 }
 
 static void bwmon_disable(struct icc_bwmon *bwmon)
 {
 	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
-	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE], 0x0);
+	if (bwmon->global_regs[0])
+		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE], 0x0);
+	else
+		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE], 0x0);
 	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
 
 	/*
@@ -391,9 +457,13 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
 static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
 {
 	/* Enable interrupts */
-	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
-		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE],
-				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+	if (bwmon->global_regs[0])
+		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE],
+					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+	else
+		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE],
+					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
+
 	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
 
 	/* Enable bwmon */
@@ -556,7 +626,9 @@ static int bwmon_init_regmap(struct platform_device *pdev,
 	struct device *dev = &pdev->dev;
 	void __iomem *base;
 	struct regmap *map;
+	int ret;
 
+	/* Map the monitor base */
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return dev_err_probe(dev, PTR_ERR(base),
@@ -567,12 +639,38 @@ static int bwmon_init_regmap(struct platform_device *pdev,
 		return dev_err_probe(dev, PTR_ERR(map),
 				     "failed to initialize regmap\n");
 
+	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_global_reg_fields) != F_NUM_GLOBAL_FIELDS);
 	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_reg_fields) != F_NUM_FIELDS);
 	BUILD_BUG_ON(ARRAY_SIZE(sdm845_llcc_bwmon_reg_fields) != F_NUM_FIELDS);
 
-	return devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
+	ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
 					   bwmon->data->regmap_fields,
 					   F_NUM_FIELDS);
+	if (ret)
+		return ret;
+
+	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
+		/* Map the global base, if separate */
+		base = devm_platform_ioremap_resource(pdev, 1);
+
+		/* If it's not, bail out early and assume the 845 register scheme */
+		if (IS_ERR(base) && PTR_ERR(base) == -EINVAL)
+			goto exit;
+		else if (IS_ERR(base))
+			return dev_err_probe(dev, PTR_ERR(base),
+					     "failed to map bwmon global registers\n");
+
+		map = devm_regmap_init_mmio(dev, base, bwmon->data->global_regmap_cfg);
+		if (IS_ERR(map))
+			return dev_err_probe(dev, PTR_ERR(map),
+					     "failed to initialize global regmap\n");
+
+		ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->global_regs,
+						   bwmon->data->global_regmap_fields,
+						   F_NUM_GLOBAL_FIELDS);
+	}
+exit:
+	return ret;
 }
 
 static int bwmon_probe(struct platform_device *pdev)
@@ -645,6 +743,8 @@ static const struct icc_bwmon_data msm8998_bwmon_data = {
 	.quirks = BWMON_HAS_GLOBAL_IRQ,
 	.regmap_fields = msm8998_bwmon_reg_fields,
 	.regmap_cfg = &msm8998_bwmon_regmap_cfg,
+	.global_regmap_fields = msm8998_bwmon_global_reg_fields,
+	.global_regmap_cfg = &msm8998_bwmon_global_regmap_cfg,
 };
 
 static const struct icc_bwmon_data sdm845_llcc_bwmon_data = {

-- 
2.39.2

[PATCH 3/3] soc: qcom: icc-bwmon: Remove unused struct member

[Konrad Dybcio]

bwmon->regmap was never used, as the regmap for bwmon is registered
through devres and accessed through bwmon's regmap_field members.
Remove it

Fixes: ec63dcd3c863 ("soc: qcom: icc-bwmon: use regmap and prepare for BWMON v5")
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/soc/qcom/icc-bwmon.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
index 9ef632d80ee3..3f88ccedde60 100644
--- a/drivers/soc/qcom/icc-bwmon.c
+++ b/drivers/soc/qcom/icc-bwmon.c
@@ -189,7 +189,6 @@ struct icc_bwmon {
 	const struct icc_bwmon_data *data;
 	int irq;
 
-	struct regmap *regmap;
 	struct regmap_field *regs[F_NUM_FIELDS];
 	struct regmap_field *global_regs[F_NUM_FIELDS];
 

-- 
2.39.2

Re: [PATCH 1/3] dt-bindings: interconnect: qcom,msm8998-bwmon: Add global registers

[Krzysztof Kozlowski]

On 04/03/2023 16:39, Konrad Dybcio wrote:
> The BWMON has two sets of registers: one for handling the monitor itself
> and one called "global", which we didn't care about before, as on newer
> SoCs it was made contiguous with (but not the same as) the monitor's
> register range. Describe it.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  .../bindings/interconnect/qcom,msm8998-bwmon.yaml  | 28 ++++++++++++++++++----
>  1 file changed, 24 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
> index 12a0d3ecbabb..6dd0cb0a1f43 100644
> --- a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
> +++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
> @@ -49,9 +49,13 @@ properties:
>      type: object
>  
>    reg:
> -    # BWMON v4 (currently described) and BWMON v5 use one register address
> -    # space.  BWMON v2 uses two register spaces - not yet described.
> -    maxItems: 1
> +    # BWMON v5 uses one register address space, v1-v4 use one or two.
> +    minItems: 1
> +    maxItems: 2
> +
> +  reg-names:
> +    minItems: 1
> +    maxItems: 2
>  
>  required:
>    - compatible
> @@ -63,6 +67,21 @@ required:
>  
>  additionalProperties: false
>  
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          const: qcom,msm8998-bwmon
> +    then:
> +      properties:
> +        reg:
> +          minItems: 2
> +
> +        reg-names:
> +          items:
> +            - const: monitor
> +            - const: global

else:
  reg:
    maxItems: 1

and either disallow reg-names or move it to the top-level.

> +
>  examples:
>    - |
>      #include <dt-bindings/interconnect/qcom,sdm845.h>
> @@ -70,7 +89,8 @@ examples:
>  
>      pmu@1436400 {
>          compatible = "qcom,sdm845-bwmon", "qcom,msm8998-bwmon";
> -        reg = <0x01436400 0x600>;
> +        reg = <0x01436400 0x600>, <0x01436300 0x200>;

That's not correct for sdm845. It's only one address space for sdm845.


Best regards,
Krzysztof

Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Krzysztof Kozlowski]

On 04/03/2023 16:39, Konrad Dybcio wrote:
> The BWMON hardware has two sets of registers: one for the monitor itself
> and one called "global". It has what seems to be some kind of a head
> switch and an interrupt control register. It's usually 0x200 in size.
> 
> On fairly recent SoCs (with the starting point seemingly being moving
> the OSM programming to the firmware) these two register sets are
> contiguous and overlapping, like this (on sm8450):
> 
> /* notice how base.start == global_base.start+0x100 */
> reg = <0x90b6400 0x300>, <0x90b6300 0x200>;
> reg-names = "base", "global_base";
> 
> Which led to some confusion and the assumption that since the
> "interesting" global registers begin right after global_base+0x100,
> there's no need to map two separate regions and one can simply subtract
> 0x100 from the offsets.
> 
> This is however not the case for anything older than SDM845, as the
> global region can appear in seemingly random spots on the register map.
> 
> Add support for it to let bwmon function on older SoCs like MSM8998 and
> allow operation with just one set of registers for newer platforms.
> 
> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")

You did not describe any bug to be fixed. Adding support for different
devices with different memory layour is a feature, not bugfix. If this
is a bugfix, please share what exactly is broken on sdm845?

> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/soc/qcom/icc-bwmon.c | 136 +++++++++++++++++++++++++++++++++++++------
>  1 file changed, 118 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
> index d07be3700db6..9ef632d80ee3 100644
> --- a/drivers/soc/qcom/icc-bwmon.c
> +++ b/drivers/soc/qcom/icc-bwmon.c
> @@ -34,14 +34,27 @@
>  /* Internal sampling clock frequency */
>  #define HW_TIMER_HZ				19200000
>  
> -#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x008
> -#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x00c
> +#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x108
> +#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x10c
>  /*
>   * All values here and further are matching regmap fields, so without absolute
>   * register offsets.
>   */
>  #define BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE	BIT(0)
>  
> +/*
> + * Starting with SDM845, the BWMON4 register space has changed a bit:
> + * the global registers were jammed into the beginning of the monitor region.
> + * To keep the proper offsets, one would have to map <GLOBAL_BASE 0x200> and
> + * <GLOBAL_BASE+0x100 0x300>, which is straight up wrong.
> + * To facilitate for that, while allowing the older, arguably more proper
> + * implementations to work, offset the global registers by -0x100 to avoid
> + * having to map half of the global registers twice.
> + */
> +#define BWMON_V4_845_OFFSET			0x100

MSM8998? It's a bit confusing to keep calling it 845 while it is for
MSM8998 variant... or it's not anymore for MSM8998?


> +#define BWMON_V4_GLOBAL_IRQ_CLEAR_845		(BWMON_V4_GLOBAL_IRQ_CLEAR - BWMON_V4_845_OFFSET)
> +#define BWMON_V4_GLOBAL_IRQ_ENABLE_845		(BWMON_V4_GLOBAL_IRQ_ENABLE - BWMON_V4_845_OFFSET)
> +
>  #define BWMON_V4_IRQ_STATUS			0x100
>  #define BWMON_V4_IRQ_CLEAR			0x108
>  
> @@ -118,8 +131,10 @@
>  #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
>  
>  enum bwmon_fields {
> -	F_GLOBAL_IRQ_CLEAR,
> -	F_GLOBAL_IRQ_ENABLE,
> +	/* Fields used only on >=SDM845 with BWMON_HAS_GLOBAL_IRQ */
> +	F_GLB_IRQ_CLEAR,
> +	F_GLB_IRQ_ENABLE,

I am not sure what's the benefit of this rename.

> +
>  	F_IRQ_STATUS,
>  	F_IRQ_CLEAR,
>  	F_IRQ_ENABLE,
> @@ -145,6 +160,13 @@ enum bwmon_fields {
>  	F_NUM_FIELDS
>  };
>  
> +enum bwmon_global_fields {
> +	F_GLOBAL_IRQ_CLEAR,
> +	F_GLOBAL_IRQ_ENABLE,
> +
> +	F_NUM_GLOBAL_FIELDS
> +};
> +
>  struct icc_bwmon_data {
>  	unsigned int sample_ms;
>  	unsigned int count_unit_kb; /* kbytes */
> @@ -157,6 +179,9 @@ struct icc_bwmon_data {
>  
>  	const struct regmap_config *regmap_cfg;
>  	const struct reg_field *regmap_fields;
> +
> +	const struct regmap_config *global_regmap_cfg;
> +	const struct reg_field *global_regmap_fields;
>  };
>  
>  struct icc_bwmon {
> @@ -166,6 +191,7 @@ struct icc_bwmon {
>  
>  	struct regmap *regmap;
>  	struct regmap_field *regs[F_NUM_FIELDS];
> +	struct regmap_field *global_regs[F_NUM_FIELDS];
>  
>  	unsigned int max_bw_kbps;
>  	unsigned int min_bw_kbps;
> @@ -175,8 +201,8 @@ struct icc_bwmon {
>  
>  /* BWMON v4 */
>  static const struct reg_field msm8998_bwmon_reg_fields[] = {
> -	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
> -	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
> +	[F_GLB_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0, 0),
> +	[F_GLB_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE_845, 0, 0),
>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V4_IRQ_STATUS, 4, 7),
>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V4_IRQ_CLEAR, 4, 7),
>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V4_IRQ_ENABLE, 4, 7),
> @@ -202,7 +228,7 @@ static const struct reg_field msm8998_bwmon_reg_fields[] = {
>  };
>  
>  static const struct regmap_range msm8998_bwmon_reg_noread_ranges[] = {
> -	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR_845, BWMON_V4_GLOBAL_IRQ_CLEAR_845),
>  	regmap_reg_range(BWMON_V4_IRQ_CLEAR, BWMON_V4_IRQ_CLEAR),
>  	regmap_reg_range(BWMON_V4_CLEAR, BWMON_V4_CLEAR),
>  };
> @@ -222,16 +248,34 @@ static const struct regmap_access_table msm8998_bwmon_reg_volatile_table = {
>  	.n_yes_ranges	= ARRAY_SIZE(msm8998_bwmon_reg_volatile_ranges),
>  };
>  
> +static const struct reg_field msm8998_bwmon_global_reg_fields[] = {
> +	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
> +	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
> +};
> +
> +static const struct regmap_range msm8998_bwmon_global_reg_noread_ranges[] = {
> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
> +};
> +
> +static const struct regmap_access_table msm8998_bwmon_global_reg_read_table = {
> +	.no_ranges	= msm8998_bwmon_global_reg_noread_ranges,
> +	.n_no_ranges	= ARRAY_SIZE(msm8998_bwmon_global_reg_noread_ranges),
> +};
> +
>  /*
>   * Fill the cache for non-readable registers only as rest does not really
>   * matter and can be read from the device.
>   */
>  static const struct reg_default msm8998_bwmon_reg_defaults[] = {
> -	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0x0 },
>  	{ BWMON_V4_IRQ_CLEAR, 0x0 },
>  	{ BWMON_V4_CLEAR, 0x0 },
>  };
>  
> +static const struct reg_default msm8998_bwmon_global_reg_defaults[] = {
> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
> +};
> +
>  static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>  	.reg_bits		= 32,
>  	.reg_stride		= 4,
> @@ -252,10 +296,27 @@ static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>  	.cache_type		= REGCACHE_RBTREE,
>  };
>  
> +static const struct regmap_config msm8998_bwmon_global_regmap_cfg = {
> +	.reg_bits		= 32,
> +	.reg_stride		= 4,
> +	.val_bits		= 32,
> +	/*
> +	 * No concurrent access expected - driver has one interrupt handler,
> +	 * regmap is not shared, no driver or user-space API.
> +	 */
> +	.disable_locking	= true,
> +	.rd_table		= &msm8998_bwmon_global_reg_read_table,
> +	.reg_defaults		= msm8998_bwmon_global_reg_defaults,
> +	.num_reg_defaults	= ARRAY_SIZE(msm8998_bwmon_global_reg_defaults),
> +	/*
> +	 * Cache is necessary for using regmap fields with non-readable
> +	 * registers.
> +	 */
> +	.cache_type		= REGCACHE_RBTREE,
> +};
> +
>  /* BWMON v5 */
>  static const struct reg_field sdm845_llcc_bwmon_reg_fields[] = {
> -	[F_GLOBAL_IRQ_CLEAR]	= {},
> -	[F_GLOBAL_IRQ_ENABLE]	= {},
>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V5_IRQ_STATUS, 0, 3),
>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V5_IRQ_CLEAR, 0, 3),
>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V5_IRQ_ENABLE, 0, 3),
> @@ -369,16 +430,21 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
>  	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
>  	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
>  		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
> -		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
> +	if (bwmon->global_regs[0])
> +		regmap_field_force_write(bwmon->global_regs[F_GLOBAL_IRQ_CLEAR],
> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
> +	else
> +		regmap_field_force_write(bwmon->regs[F_GLB_IRQ_CLEAR],
>  					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>  }
>  
>  static void bwmon_disable(struct icc_bwmon *bwmon)
>  {
>  	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE], 0x0);
> +	if (bwmon->global_regs[0])
> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE], 0x0);
> +	else
> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE], 0x0);
>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
>  
>  	/*
> @@ -391,9 +457,13 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
>  static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
>  {
>  	/* Enable interrupts */
> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE],
> -				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
> +	if (bwmon->global_regs[0])
> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE],
> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
> +	else
> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE],
> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);

Probably this would be more readable if regmap_field_write() is called
only once and you parametrize the first argument (field) from the
'struct bwmon'.

> +
>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
>  
>  	/* Enable bwmon */
> @@ -556,7 +626,9 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>  	struct device *dev = &pdev->dev;
>  	void __iomem *base;
>  	struct regmap *map;
> +	int ret;
>  
> +	/* Map the monitor base */
>  	base = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(base))
>  		return dev_err_probe(dev, PTR_ERR(base),
> @@ -567,12 +639,38 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>  		return dev_err_probe(dev, PTR_ERR(map),
>  				     "failed to initialize regmap\n");
>  
> +	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_global_reg_fields) != F_NUM_GLOBAL_FIELDS);
>  	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_reg_fields) != F_NUM_FIELDS);
>  	BUILD_BUG_ON(ARRAY_SIZE(sdm845_llcc_bwmon_reg_fields) != F_NUM_FIELDS);
>  
> -	return devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
> +	ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>  					   bwmon->data->regmap_fields,
>  					   F_NUM_FIELDS);

What exactly happens now on msm8998 (or updated sdm845 from your binding
example) for the "global" fields in this region? The regmap references
non-existing fields for the "monitor" region, doesn't it?

> +	if (ret)
> +		return ret;
> +
> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
> +		/* Map the global base, if separate */
> +		base = devm_platform_ioremap_resource(pdev, 1);

Wouldn't this now print errors for sdm845, thus introduce dmesg regression?

> +
> +		/* If it's not, bail out early and assume the 845 register scheme */
> +		if (IS_ERR(base) && PTR_ERR(base) == -EINVAL)
> +			goto exit;
> +		else if (IS_ERR(base))
> +			return dev_err_probe(dev, PTR_ERR(base),
> +					     "failed to map bwmon global registers\n");
> +
> +		map = devm_regmap_init_mmio(dev, base, bwmon->data->global_regmap_cfg);
> +		if (IS_ERR(map))
> +			return dev_err_probe(dev, PTR_ERR(map),
> +					     "failed to initialize global regmap\n");
> +
> +		ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->global_regs,
> +						   bwmon->data->global_regmap_fields,
> +						
> 

Best regards,
Krzysztof

Re: [PATCH 3/3] soc: qcom: icc-bwmon: Remove unused struct member

[Krzysztof Kozlowski]

On 04/03/2023 16:39, Konrad Dybcio wrote:
> bwmon->regmap was never used, as the regmap for bwmon is registered
> through devres and accessed through bwmon's regmap_field members.
> Remove it
> 
> Fixes: ec63dcd3c863 ("soc: qcom: icc-bwmon: use regmap and prepare for BWMON v5")
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> ---
>  drivers/soc/qcom/icc-bwmon.c | 1 -
>  1 file changed, 1 deletion(-)


Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: interconnect: qcom,msm8998-bwmon: Add global registers

[Konrad Dybcio]

On 5.03.2023 15:52, Krzysztof Kozlowski wrote:
> On 04/03/2023 16:39, Konrad Dybcio wrote:
>> The BWMON has two sets of registers: one for handling the monitor itself
>> and one called "global", which we didn't care about before, as on newer
>> SoCs it was made contiguous with (but not the same as) the monitor's
>> register range. Describe it.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  .../bindings/interconnect/qcom,msm8998-bwmon.yaml  | 28 ++++++++++++++++++----
>>  1 file changed, 24 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
>> index 12a0d3ecbabb..6dd0cb0a1f43 100644
>> --- a/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
>> +++ b/Documentation/devicetree/bindings/interconnect/qcom,msm8998-bwmon.yaml
>> @@ -49,9 +49,13 @@ properties:
>>      type: object
>>  
>>    reg:
>> -    # BWMON v4 (currently described) and BWMON v5 use one register address
>> -    # space.  BWMON v2 uses two register spaces - not yet described.
>> -    maxItems: 1
>> +    # BWMON v5 uses one register address space, v1-v4 use one or two.
>> +    minItems: 1
>> +    maxItems: 2
>> +
>> +  reg-names:
>> +    minItems: 1
>> +    maxItems: 2
>>  
>>  required:
>>    - compatible
>> @@ -63,6 +67,21 @@ required:
>>  
>>  additionalProperties: false
>>  
>> +allOf:
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          const: qcom,msm8998-bwmon
>> +    then:
>> +      properties:
>> +        reg:
>> +          minItems: 2
>> +
>> +        reg-names:
>> +          items:
>> +            - const: monitor
>> +            - const: global
> 
> else:
>   reg:
>     maxItems: 1
> 
> and either disallow reg-names or move it to the top-level.
Disallowing makes more sense in this case imo, will do.

> 
>> +
>>  examples:
>>    - |
>>      #include <dt-bindings/interconnect/qcom,sdm845.h>
>> @@ -70,7 +89,8 @@ examples:
>>  
>>      pmu@1436400 {
>>          compatible = "qcom,sdm845-bwmon", "qcom,msm8998-bwmon";
>> -        reg = <0x01436400 0x600>;
>> +        reg = <0x01436400 0x600>, <0x01436300 0x200>;
> 
> That's not correct for sdm845. It's only one address space for sdm845.
Ack, leftover from an old version again..

Konrad
> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Konrad Dybcio]

On 5.03.2023 16:06, Krzysztof Kozlowski wrote:
> On 04/03/2023 16:39, Konrad Dybcio wrote:
>> The BWMON hardware has two sets of registers: one for the monitor itself
>> and one called "global". It has what seems to be some kind of a head
>> switch and an interrupt control register. It's usually 0x200 in size.
>>
>> On fairly recent SoCs (with the starting point seemingly being moving
>> the OSM programming to the firmware) these two register sets are
>> contiguous and overlapping, like this (on sm8450):
>>
>> /* notice how base.start == global_base.start+0x100 */
>> reg = <0x90b6400 0x300>, <0x90b6300 0x200>;
>> reg-names = "base", "global_base";
>>
>> Which led to some confusion and the assumption that since the
>> "interesting" global registers begin right after global_base+0x100,
>> there's no need to map two separate regions and one can simply subtract
>> 0x100 from the offsets.
>>
>> This is however not the case for anything older than SDM845, as the
>> global region can appear in seemingly random spots on the register map.
>>
>> Add support for it to let bwmon function on older SoCs like MSM8998 and
>> allow operation with just one set of registers for newer platforms.
>>
>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
> 
> You did not describe any bug to be fixed. Adding support for different
> devices with different memory layour is a feature, not bugfix. If this
> is a bugfix, please share what exactly is broken on sdm845?
The driver was completely broken for MSM8998, even though it
had a compatible specifically for that SoC.

> 
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> ---
>>  drivers/soc/qcom/icc-bwmon.c | 136 +++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 118 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>> index d07be3700db6..9ef632d80ee3 100644
>> --- a/drivers/soc/qcom/icc-bwmon.c
>> +++ b/drivers/soc/qcom/icc-bwmon.c
>> @@ -34,14 +34,27 @@
>>  /* Internal sampling clock frequency */
>>  #define HW_TIMER_HZ				19200000
>>  
>> -#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x008
>> -#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x00c
>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x108
>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x10c
>>  /*
>>   * All values here and further are matching regmap fields, so without absolute
>>   * register offsets.
>>   */
>>  #define BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE	BIT(0)
>>  
>> +/*
>> + * Starting with SDM845, the BWMON4 register space has changed a bit:
>> + * the global registers were jammed into the beginning of the monitor region.
>> + * To keep the proper offsets, one would have to map <GLOBAL_BASE 0x200> and
>> + * <GLOBAL_BASE+0x100 0x300>, which is straight up wrong.
>> + * To facilitate for that, while allowing the older, arguably more proper
>> + * implementations to work, offset the global registers by -0x100 to avoid
>> + * having to map half of the global registers twice.
>> + */
>> +#define BWMON_V4_845_OFFSET			0x100
> 
> MSM8998? It's a bit confusing to keep calling it 845 while it is for
> MSM8998 variant... or it's not anymore for MSM8998?
No, it's for 845 and up.

> 
> 
>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR_845		(BWMON_V4_GLOBAL_IRQ_CLEAR - BWMON_V4_845_OFFSET)
>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE_845		(BWMON_V4_GLOBAL_IRQ_ENABLE - BWMON_V4_845_OFFSET)
>> +
>>  #define BWMON_V4_IRQ_STATUS			0x100
>>  #define BWMON_V4_IRQ_CLEAR			0x108
>>  
>> @@ -118,8 +131,10 @@
>>  #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
>>  
>>  enum bwmon_fields {
>> -	F_GLOBAL_IRQ_CLEAR,
>> -	F_GLOBAL_IRQ_ENABLE,
>> +	/* Fields used only on >=SDM845 with BWMON_HAS_GLOBAL_IRQ */
>> +	F_GLB_IRQ_CLEAR,
>> +	F_GLB_IRQ_ENABLE,
> 
> I am not sure what's the benefit of this rename.
I need to somehow differentiate the members of enum bwmon_fields
and enum bwmon_global_fields, keeping the "_GLOBAL" in global_fields
seems to make the most sense, imo.

> 
>> +
>>  	F_IRQ_STATUS,
>>  	F_IRQ_CLEAR,
>>  	F_IRQ_ENABLE,
>> @@ -145,6 +160,13 @@ enum bwmon_fields {
>>  	F_NUM_FIELDS
>>  };
>>  
>> +enum bwmon_global_fields {
>> +	F_GLOBAL_IRQ_CLEAR,
>> +	F_GLOBAL_IRQ_ENABLE,
>> +
>> +	F_NUM_GLOBAL_FIELDS
>> +};
>> +
>>  struct icc_bwmon_data {
>>  	unsigned int sample_ms;
>>  	unsigned int count_unit_kb; /* kbytes */
>> @@ -157,6 +179,9 @@ struct icc_bwmon_data {
>>  
>>  	const struct regmap_config *regmap_cfg;
>>  	const struct reg_field *regmap_fields;
>> +
>> +	const struct regmap_config *global_regmap_cfg;
>> +	const struct reg_field *global_regmap_fields;
>>  };
>>  
>>  struct icc_bwmon {
>> @@ -166,6 +191,7 @@ struct icc_bwmon {
>>  
>>  	struct regmap *regmap;
>>  	struct regmap_field *regs[F_NUM_FIELDS];
>> +	struct regmap_field *global_regs[F_NUM_FIELDS];
>>  
>>  	unsigned int max_bw_kbps;
>>  	unsigned int min_bw_kbps;
>> @@ -175,8 +201,8 @@ struct icc_bwmon {
>>  
>>  /* BWMON v4 */
>>  static const struct reg_field msm8998_bwmon_reg_fields[] = {
>> -	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>> -	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>> +	[F_GLB_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0, 0),
>> +	[F_GLB_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE_845, 0, 0),
>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V4_IRQ_STATUS, 4, 7),
>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V4_IRQ_CLEAR, 4, 7),
>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V4_IRQ_ENABLE, 4, 7),
>> @@ -202,7 +228,7 @@ static const struct reg_field msm8998_bwmon_reg_fields[] = {
>>  };
>>  
>>  static const struct regmap_range msm8998_bwmon_reg_noread_ranges[] = {
>> -	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR_845, BWMON_V4_GLOBAL_IRQ_CLEAR_845),
>>  	regmap_reg_range(BWMON_V4_IRQ_CLEAR, BWMON_V4_IRQ_CLEAR),
>>  	regmap_reg_range(BWMON_V4_CLEAR, BWMON_V4_CLEAR),
>>  };
>> @@ -222,16 +248,34 @@ static const struct regmap_access_table msm8998_bwmon_reg_volatile_table = {
>>  	.n_yes_ranges	= ARRAY_SIZE(msm8998_bwmon_reg_volatile_ranges),
>>  };
>>  
>> +static const struct reg_field msm8998_bwmon_global_reg_fields[] = {
>> +	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>> +	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>> +};
>> +
>> +static const struct regmap_range msm8998_bwmon_global_reg_noread_ranges[] = {
>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>> +};
>> +
>> +static const struct regmap_access_table msm8998_bwmon_global_reg_read_table = {
>> +	.no_ranges	= msm8998_bwmon_global_reg_noread_ranges,
>> +	.n_no_ranges	= ARRAY_SIZE(msm8998_bwmon_global_reg_noread_ranges),
>> +};
>> +
>>  /*
>>   * Fill the cache for non-readable registers only as rest does not really
>>   * matter and can be read from the device.
>>   */
>>  static const struct reg_default msm8998_bwmon_reg_defaults[] = {
>> -	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0x0 },
>>  	{ BWMON_V4_IRQ_CLEAR, 0x0 },
>>  	{ BWMON_V4_CLEAR, 0x0 },
>>  };
>>  
>> +static const struct reg_default msm8998_bwmon_global_reg_defaults[] = {
>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>> +};
>> +
>>  static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>  	.reg_bits		= 32,
>>  	.reg_stride		= 4,
>> @@ -252,10 +296,27 @@ static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>  	.cache_type		= REGCACHE_RBTREE,
>>  };
>>  
>> +static const struct regmap_config msm8998_bwmon_global_regmap_cfg = {
>> +	.reg_bits		= 32,
>> +	.reg_stride		= 4,
>> +	.val_bits		= 32,
>> +	/*
>> +	 * No concurrent access expected - driver has one interrupt handler,
>> +	 * regmap is not shared, no driver or user-space API.
>> +	 */
>> +	.disable_locking	= true,
>> +	.rd_table		= &msm8998_bwmon_global_reg_read_table,
>> +	.reg_defaults		= msm8998_bwmon_global_reg_defaults,
>> +	.num_reg_defaults	= ARRAY_SIZE(msm8998_bwmon_global_reg_defaults),
>> +	/*
>> +	 * Cache is necessary for using regmap fields with non-readable
>> +	 * registers.
>> +	 */
>> +	.cache_type		= REGCACHE_RBTREE,
>> +};
>> +
>>  /* BWMON v5 */
>>  static const struct reg_field sdm845_llcc_bwmon_reg_fields[] = {
>> -	[F_GLOBAL_IRQ_CLEAR]	= {},
>> -	[F_GLOBAL_IRQ_ENABLE]	= {},
>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V5_IRQ_STATUS, 0, 3),
>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V5_IRQ_CLEAR, 0, 3),
>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V5_IRQ_ENABLE, 0, 3),
>> @@ -369,16 +430,21 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
>>  	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
>>  	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
>>  		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>> -		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
>> +	if (bwmon->global_regs[0])
>> +		regmap_field_force_write(bwmon->global_regs[F_GLOBAL_IRQ_CLEAR],
>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>> +	else
>> +		regmap_field_force_write(bwmon->regs[F_GLB_IRQ_CLEAR],
>>  					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>  }
>>  
>>  static void bwmon_disable(struct icc_bwmon *bwmon)
>>  {
>>  	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>> +	if (bwmon->global_regs[0])
>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>> +	else
>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE], 0x0);
>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
>>  
>>  	/*
>> @@ -391,9 +457,13 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
>>  static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
>>  {
>>  	/* Enable interrupts */
>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE],
>> -				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>> +	if (bwmon->global_regs[0])
>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE],
>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>> +	else
>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE],
>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
> 
> Probably this would be more readable if regmap_field_write() is called
> only once and you parametrize the first argument (field) from the
> 'struct bwmon'.
Sounds good

> 
>> +
>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
>>  
>>  	/* Enable bwmon */
>> @@ -556,7 +626,9 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>  	struct device *dev = &pdev->dev;
>>  	void __iomem *base;
>>  	struct regmap *map;
>> +	int ret;
>>  
>> +	/* Map the monitor base */
>>  	base = devm_platform_ioremap_resource(pdev, 0);
>>  	if (IS_ERR(base))
>>  		return dev_err_probe(dev, PTR_ERR(base),
>> @@ -567,12 +639,38 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>  		return dev_err_probe(dev, PTR_ERR(map),
>>  				     "failed to initialize regmap\n");
>>  
>> +	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_global_reg_fields) != F_NUM_GLOBAL_FIELDS);
>>  	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_reg_fields) != F_NUM_FIELDS);
>>  	BUILD_BUG_ON(ARRAY_SIZE(sdm845_llcc_bwmon_reg_fields) != F_NUM_FIELDS);
>>  
>> -	return devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>> +	ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>>  					   bwmon->data->regmap_fields,
>>  					   F_NUM_FIELDS);
> 
> What exactly happens now on msm8998 (or updated sdm845 from your binding
> example) for the "global" fields in this region? The regmap references
> non-existing fields for the "monitor" region, doesn't it?
That's correct, the regmap references 3 non-existstent (or at least
not-what-we-think-they-are registers (ending with _845) for
targets with two separate regions (such as MSM8998), but they're
unaccessed if the "global" region has been mapped sucessfully
[see `if (bwmon->global_regs[0])`].

> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>> +		/* Map the global base, if separate */
>> +		base = devm_platform_ioremap_resource(pdev, 1);
> 
> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
I explicitly stated this in the cover letter and asked for opinions.

Konrad
> 
>> +
>> +		/* If it's not, bail out early and assume the 845 register scheme */
>> +		if (IS_ERR(base) && PTR_ERR(base) == -EINVAL)
>> +			goto exit;
>> +		else if (IS_ERR(base))
>> +			return dev_err_probe(dev, PTR_ERR(base),
>> +					     "failed to map bwmon global registers\n");
>> +
>> +		map = devm_regmap_init_mmio(dev, base, bwmon->data->global_regmap_cfg);
>> +		if (IS_ERR(map))
>> +			return dev_err_probe(dev, PTR_ERR(map),
>> +					     "failed to initialize global regmap\n");
>> +
>> +		ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->global_regs,
>> +						   bwmon->data->global_regmap_fields,
>> +						
>>
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Krzysztof Kozlowski]

On 06/03/2023 11:06, Konrad Dybcio wrote:
> 
> 
> On 5.03.2023 16:06, Krzysztof Kozlowski wrote:
>> On 04/03/2023 16:39, Konrad Dybcio wrote:
>>> The BWMON hardware has two sets of registers: one for the monitor itself
>>> and one called "global". It has what seems to be some kind of a head
>>> switch and an interrupt control register. It's usually 0x200 in size.
>>>
>>> On fairly recent SoCs (with the starting point seemingly being moving
>>> the OSM programming to the firmware) these two register sets are
>>> contiguous and overlapping, like this (on sm8450):
>>>
>>> /* notice how base.start == global_base.start+0x100 */
>>> reg = <0x90b6400 0x300>, <0x90b6300 0x200>;
>>> reg-names = "base", "global_base";
>>>
>>> Which led to some confusion and the assumption that since the
>>> "interesting" global registers begin right after global_base+0x100,
>>> there's no need to map two separate regions and one can simply subtract
>>> 0x100 from the offsets.
>>>
>>> This is however not the case for anything older than SDM845, as the
>>> global region can appear in seemingly random spots on the register map.
>>>
>>> Add support for it to let bwmon function on older SoCs like MSM8998 and
>>> allow operation with just one set of registers for newer platforms.
>>>
>>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
>>
>> You did not describe any bug to be fixed. Adding support for different
>> devices with different memory layour is a feature, not bugfix. If this
>> is a bugfix, please share what exactly is broken on sdm845?
> The driver was completely broken for MSM8998, even though it
> had a compatible specifically for that SoC.

That's still nothing to fix - msm8998 was added here only for
compatibility reasons for bindings. It wasn't ever tested on MSM8998 and
also probably never written in a way that it should work for MSM8998. It
could work, but that was not the intention. The driver is supposed to
work on sdm845 and according to your description - there is nothing
wrong with that.

> 
>>
>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> ---
>>>  drivers/soc/qcom/icc-bwmon.c | 136 +++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 118 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>>> index d07be3700db6..9ef632d80ee3 100644
>>> --- a/drivers/soc/qcom/icc-bwmon.c
>>> +++ b/drivers/soc/qcom/icc-bwmon.c
>>> @@ -34,14 +34,27 @@
>>>  /* Internal sampling clock frequency */
>>>  #define HW_TIMER_HZ				19200000
>>>  
>>> -#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x008
>>> -#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x00c
>>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x108
>>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x10c
>>>  /*
>>>   * All values here and further are matching regmap fields, so without absolute
>>>   * register offsets.
>>>   */
>>>  #define BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE	BIT(0)
>>>  
>>> +/*
>>> + * Starting with SDM845, the BWMON4 register space has changed a bit:
>>> + * the global registers were jammed into the beginning of the monitor region.
>>> + * To keep the proper offsets, one would have to map <GLOBAL_BASE 0x200> and
>>> + * <GLOBAL_BASE+0x100 0x300>, which is straight up wrong.
>>> + * To facilitate for that, while allowing the older, arguably more proper
>>> + * implementations to work, offset the global registers by -0x100 to avoid
>>> + * having to map half of the global registers twice.
>>> + */
>>> +#define BWMON_V4_845_OFFSET			0x100
>>
>> MSM8998? It's a bit confusing to keep calling it 845 while it is for
>> MSM8998 variant... or it's not anymore for MSM8998?
> No, it's for 845 and up.

If it is for 845 and up, why then it is used for msm8998 in
msm8998_bwmon_reg_noread_ranges?

> 
>>
>>
>>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR_845		(BWMON_V4_GLOBAL_IRQ_CLEAR - BWMON_V4_845_OFFSET)
>>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE_845		(BWMON_V4_GLOBAL_IRQ_ENABLE - BWMON_V4_845_OFFSET)
>>> +
>>>  #define BWMON_V4_IRQ_STATUS			0x100
>>>  #define BWMON_V4_IRQ_CLEAR			0x108
>>>  
>>> @@ -118,8 +131,10 @@
>>>  #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
>>>  
>>>  enum bwmon_fields {
>>> -	F_GLOBAL_IRQ_CLEAR,
>>> -	F_GLOBAL_IRQ_ENABLE,
>>> +	/* Fields used only on >=SDM845 with BWMON_HAS_GLOBAL_IRQ */
>>> +	F_GLB_IRQ_CLEAR,
>>> +	F_GLB_IRQ_ENABLE,
>>
>> I am not sure what's the benefit of this rename.
> I need to somehow differentiate the members of enum bwmon_fields
> and enum bwmon_global_fields, keeping the "_GLOBAL" in global_fields
> seems to make the most sense, imo.

This complicates and makes code difficult to read. The entire driver was
designed for single address space to make everything a bit simpler and
easier to read, so any changes should not reduce this. Maybe just use
the same defines? Or name the other fields MSM8998?

> 
>>
>>> +
>>>  	F_IRQ_STATUS,
>>>  	F_IRQ_CLEAR,
>>>  	F_IRQ_ENABLE,
>>> @@ -145,6 +160,13 @@ enum bwmon_fields {
>>>  	F_NUM_FIELDS
>>>  };
>>>  
>>> +enum bwmon_global_fields {
>>> +	F_GLOBAL_IRQ_CLEAR,
>>> +	F_GLOBAL_IRQ_ENABLE,
>>> +
>>> +	F_NUM_GLOBAL_FIELDS
>>> +};
>>> +
>>>  struct icc_bwmon_data {
>>>  	unsigned int sample_ms;
>>>  	unsigned int count_unit_kb; /* kbytes */
>>> @@ -157,6 +179,9 @@ struct icc_bwmon_data {
>>>  
>>>  	const struct regmap_config *regmap_cfg;
>>>  	const struct reg_field *regmap_fields;
>>> +
>>> +	const struct regmap_config *global_regmap_cfg;
>>> +	const struct reg_field *global_regmap_fields;
>>>  };
>>>  
>>>  struct icc_bwmon {
>>> @@ -166,6 +191,7 @@ struct icc_bwmon {
>>>  
>>>  	struct regmap *regmap;
>>>  	struct regmap_field *regs[F_NUM_FIELDS];
>>> +	struct regmap_field *global_regs[F_NUM_FIELDS];
>>>  
>>>  	unsigned int max_bw_kbps;
>>>  	unsigned int min_bw_kbps;
>>> @@ -175,8 +201,8 @@ struct icc_bwmon {
>>>  
>>>  /* BWMON v4 */
>>>  static const struct reg_field msm8998_bwmon_reg_fields[] = {
>>> -	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>>> -	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>>> +	[F_GLB_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0, 0),
>>> +	[F_GLB_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE_845, 0, 0),
>>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V4_IRQ_STATUS, 4, 7),
>>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V4_IRQ_CLEAR, 4, 7),
>>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V4_IRQ_ENABLE, 4, 7),
>>> @@ -202,7 +228,7 @@ static const struct reg_field msm8998_bwmon_reg_fields[] = {
>>>  };
>>>  
>>>  static const struct regmap_range msm8998_bwmon_reg_noread_ranges[] = {
>>> -	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR_845, BWMON_V4_GLOBAL_IRQ_CLEAR_845),
>>>  	regmap_reg_range(BWMON_V4_IRQ_CLEAR, BWMON_V4_IRQ_CLEAR),
>>>  	regmap_reg_range(BWMON_V4_CLEAR, BWMON_V4_CLEAR),
>>>  };
>>> @@ -222,16 +248,34 @@ static const struct regmap_access_table msm8998_bwmon_reg_volatile_table = {
>>>  	.n_yes_ranges	= ARRAY_SIZE(msm8998_bwmon_reg_volatile_ranges),
>>>  };
>>>  
>>> +static const struct reg_field msm8998_bwmon_global_reg_fields[] = {
>>> +	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>>> +	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>>> +};
>>> +
>>> +static const struct regmap_range msm8998_bwmon_global_reg_noread_ranges[] = {
>>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>>> +};
>>> +
>>> +static const struct regmap_access_table msm8998_bwmon_global_reg_read_table = {
>>> +	.no_ranges	= msm8998_bwmon_global_reg_noread_ranges,
>>> +	.n_no_ranges	= ARRAY_SIZE(msm8998_bwmon_global_reg_noread_ranges),
>>> +};
>>> +
>>>  /*
>>>   * Fill the cache for non-readable registers only as rest does not really
>>>   * matter and can be read from the device.
>>>   */
>>>  static const struct reg_default msm8998_bwmon_reg_defaults[] = {
>>> -	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0x0 },
>>>  	{ BWMON_V4_IRQ_CLEAR, 0x0 },
>>>  	{ BWMON_V4_CLEAR, 0x0 },
>>>  };
>>>  
>>> +static const struct reg_default msm8998_bwmon_global_reg_defaults[] = {
>>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>>> +};
>>> +
>>>  static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>>  	.reg_bits		= 32,
>>>  	.reg_stride		= 4,
>>> @@ -252,10 +296,27 @@ static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>>  	.cache_type		= REGCACHE_RBTREE,
>>>  };
>>>  
>>> +static const struct regmap_config msm8998_bwmon_global_regmap_cfg = {
>>> +	.reg_bits		= 32,
>>> +	.reg_stride		= 4,
>>> +	.val_bits		= 32,
>>> +	/*
>>> +	 * No concurrent access expected - driver has one interrupt handler,
>>> +	 * regmap is not shared, no driver or user-space API.
>>> +	 */
>>> +	.disable_locking	= true,
>>> +	.rd_table		= &msm8998_bwmon_global_reg_read_table,
>>> +	.reg_defaults		= msm8998_bwmon_global_reg_defaults,
>>> +	.num_reg_defaults	= ARRAY_SIZE(msm8998_bwmon_global_reg_defaults),
>>> +	/*
>>> +	 * Cache is necessary for using regmap fields with non-readable
>>> +	 * registers.
>>> +	 */
>>> +	.cache_type		= REGCACHE_RBTREE,
>>> +};
>>> +
>>>  /* BWMON v5 */
>>>  static const struct reg_field sdm845_llcc_bwmon_reg_fields[] = {
>>> -	[F_GLOBAL_IRQ_CLEAR]	= {},
>>> -	[F_GLOBAL_IRQ_ENABLE]	= {},
>>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V5_IRQ_STATUS, 0, 3),
>>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V5_IRQ_CLEAR, 0, 3),
>>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V5_IRQ_ENABLE, 0, 3),
>>> @@ -369,16 +430,21 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
>>>  	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
>>>  	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
>>>  		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>> -		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
>>> +	if (bwmon->global_regs[0])
>>> +		regmap_field_force_write(bwmon->global_regs[F_GLOBAL_IRQ_CLEAR],
>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>> +	else
>>> +		regmap_field_force_write(bwmon->regs[F_GLB_IRQ_CLEAR],
>>>  					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>  }
>>>  
>>>  static void bwmon_disable(struct icc_bwmon *bwmon)
>>>  {
>>>  	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>>> +	if (bwmon->global_regs[0])
>>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>>> +	else
>>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE], 0x0);
>>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
>>>  
>>>  	/*
>>> @@ -391,9 +457,13 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
>>>  static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
>>>  {
>>>  	/* Enable interrupts */
>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE],
>>> -				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>> +	if (bwmon->global_regs[0])
>>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE],
>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>> +	else
>>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE],
>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>
>> Probably this would be more readable if regmap_field_write() is called
>> only once and you parametrize the first argument (field) from the
>> 'struct bwmon'.
> Sounds good
> 
>>
>>> +
>>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
>>>  
>>>  	/* Enable bwmon */
>>> @@ -556,7 +626,9 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>>  	struct device *dev = &pdev->dev;
>>>  	void __iomem *base;
>>>  	struct regmap *map;
>>> +	int ret;
>>>  
>>> +	/* Map the monitor base */
>>>  	base = devm_platform_ioremap_resource(pdev, 0);
>>>  	if (IS_ERR(base))
>>>  		return dev_err_probe(dev, PTR_ERR(base),
>>> @@ -567,12 +639,38 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>>  		return dev_err_probe(dev, PTR_ERR(map),
>>>  				     "failed to initialize regmap\n");
>>>  
>>> +	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_global_reg_fields) != F_NUM_GLOBAL_FIELDS);
>>>  	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_reg_fields) != F_NUM_FIELDS);
>>>  	BUILD_BUG_ON(ARRAY_SIZE(sdm845_llcc_bwmon_reg_fields) != F_NUM_FIELDS);
>>>  
>>> -	return devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>>> +	ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>>>  					   bwmon->data->regmap_fields,
>>>  					   F_NUM_FIELDS);
>>
>> What exactly happens now on msm8998 (or updated sdm845 from your binding
>> example) for the "global" fields in this region? The regmap references
>> non-existing fields for the "monitor" region, doesn't it?
> That's correct, the regmap references 3 non-existstent (or at least
> not-what-we-think-they-are registers (ending with _845) for
> targets with two separate regions (such as MSM8998), but they're
> unaccessed if the "global" region has been mapped sucessfully
> [see `if (bwmon->global_regs[0])`].
> 
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>>> +		/* Map the global base, if separate */
>>> +		base = devm_platform_ioremap_resource(pdev, 1);
>>
>> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
> I explicitly stated this in the cover letter and asked for opinions.
> 

Sorry, long time ago I stopped reading cover letters, maybe except it's
top few sentences. Just too many of them and too much of text usually
useless. Commits should describe everything as they go to the Git and
they should justify their own existence.

Anyway, above dmesg error regression is a no. The devices turn out not
to be compatible with each other so just adjust the bindings and match
each driver by proper compatible string.

Best regards,
Krzysztof

Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Konrad Dybcio]

On 6.03.2023 13:09, Krzysztof Kozlowski wrote:
> On 06/03/2023 11:06, Konrad Dybcio wrote:
>>
>>
>> On 5.03.2023 16:06, Krzysztof Kozlowski wrote:
>>> On 04/03/2023 16:39, Konrad Dybcio wrote:
>>>> The BWMON hardware has two sets of registers: one for the monitor itself
>>>> and one called "global". It has what seems to be some kind of a head
>>>> switch and an interrupt control register. It's usually 0x200 in size.
>>>>
>>>> On fairly recent SoCs (with the starting point seemingly being moving
>>>> the OSM programming to the firmware) these two register sets are
>>>> contiguous and overlapping, like this (on sm8450):
>>>>
>>>> /* notice how base.start == global_base.start+0x100 */
>>>> reg = <0x90b6400 0x300>, <0x90b6300 0x200>;
>>>> reg-names = "base", "global_base";
>>>>
>>>> Which led to some confusion and the assumption that since the
>>>> "interesting" global registers begin right after global_base+0x100,
>>>> there's no need to map two separate regions and one can simply subtract
>>>> 0x100 from the offsets.
>>>>
>>>> This is however not the case for anything older than SDM845, as the
>>>> global region can appear in seemingly random spots on the register map.
>>>>
>>>> Add support for it to let bwmon function on older SoCs like MSM8998 and
>>>> allow operation with just one set of registers for newer platforms.
>>>>
>>>> Fixes: b9c2ae6cac40 ("soc: qcom: icc-bwmon: Add bandwidth monitoring driver")
>>>
>>> You did not describe any bug to be fixed. Adding support for different
>>> devices with different memory layour is a feature, not bugfix. If this
>>> is a bugfix, please share what exactly is broken on sdm845?
>> The driver was completely broken for MSM8998, even though it
>> had a compatible specifically for that SoC.
> 
> That's still nothing to fix - msm8998 was added here only for
> compatibility reasons for bindings. It wasn't ever tested on MSM8998 and
> also probably never written in a way that it should work for MSM8998.
Don't you think making something like

compatible = "qcom,msm8998-bwmon"

not actually compatible with MSM8998 is a bit wrong?

 It
> could work, but that was not the intention. The driver is supposed to
> work on sdm845 and according to your description - there is nothing
> wrong with that.
> 
>>
>>>
>>>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>>> ---
>>>>  drivers/soc/qcom/icc-bwmon.c | 136 +++++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 118 insertions(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
>>>> index d07be3700db6..9ef632d80ee3 100644
>>>> --- a/drivers/soc/qcom/icc-bwmon.c
>>>> +++ b/drivers/soc/qcom/icc-bwmon.c
>>>> @@ -34,14 +34,27 @@
>>>>  /* Internal sampling clock frequency */
>>>>  #define HW_TIMER_HZ				19200000
>>>>  
>>>> -#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x008
>>>> -#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x00c
>>>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR		0x108
>>>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE		0x10c
>>>>  /*
>>>>   * All values here and further are matching regmap fields, so without absolute
>>>>   * register offsets.
>>>>   */
>>>>  #define BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE	BIT(0)
>>>>  
>>>> +/*
>>>> + * Starting with SDM845, the BWMON4 register space has changed a bit:
>>>> + * the global registers were jammed into the beginning of the monitor region.
>>>> + * To keep the proper offsets, one would have to map <GLOBAL_BASE 0x200> and
>>>> + * <GLOBAL_BASE+0x100 0x300>, which is straight up wrong.
>>>> + * To facilitate for that, while allowing the older, arguably more proper
>>>> + * implementations to work, offset the global registers by -0x100 to avoid
>>>> + * having to map half of the global registers twice.
>>>> + */
>>>> +#define BWMON_V4_845_OFFSET			0x100
>>>
>>> MSM8998? It's a bit confusing to keep calling it 845 while it is for
>>> MSM8998 variant... or it's not anymore for MSM8998?
>> No, it's for 845 and up.
> 
> If it is for 845 and up, why then it is used for msm8998 in
> msm8998_bwmon_reg_noread_ranges?
Ouch, it should have never gotten there. msm8998_bwmon_global_reg_fields
uses the correct one.

> 
>>
>>>
>>>
>>>> +#define BWMON_V4_GLOBAL_IRQ_CLEAR_845		(BWMON_V4_GLOBAL_IRQ_CLEAR - BWMON_V4_845_OFFSET)
>>>> +#define BWMON_V4_GLOBAL_IRQ_ENABLE_845		(BWMON_V4_GLOBAL_IRQ_ENABLE - BWMON_V4_845_OFFSET)
>>>> +
>>>>  #define BWMON_V4_IRQ_STATUS			0x100
>>>>  #define BWMON_V4_IRQ_CLEAR			0x108
>>>>  
>>>> @@ -118,8 +131,10 @@
>>>>  #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
>>>>  
>>>>  enum bwmon_fields {
>>>> -	F_GLOBAL_IRQ_CLEAR,
>>>> -	F_GLOBAL_IRQ_ENABLE,
>>>> +	/* Fields used only on >=SDM845 with BWMON_HAS_GLOBAL_IRQ */
>>>> +	F_GLB_IRQ_CLEAR,
>>>> +	F_GLB_IRQ_ENABLE,
>>>
>>> I am not sure what's the benefit of this rename.
>> I need to somehow differentiate the members of enum bwmon_fields
>> and enum bwmon_global_fields, keeping the "_GLOBAL" in global_fields
>> seems to make the most sense, imo.
> 
> This complicates and makes code difficult to read. The entire driver was
> designed for single address space to make everything a bit simpler and
> easier to read, so any changes should not reduce this. Maybe just use
> the same defines?
I suppose that could work with some shuffling..

Or name the other fields MSM8998?
> 
>>
>>>
>>>> +
>>>>  	F_IRQ_STATUS,
>>>>  	F_IRQ_CLEAR,
>>>>  	F_IRQ_ENABLE,
>>>> @@ -145,6 +160,13 @@ enum bwmon_fields {
>>>>  	F_NUM_FIELDS
>>>>  };
>>>>  
>>>> +enum bwmon_global_fields {
>>>> +	F_GLOBAL_IRQ_CLEAR,
>>>> +	F_GLOBAL_IRQ_ENABLE,
>>>> +
>>>> +	F_NUM_GLOBAL_FIELDS
>>>> +};
>>>> +
>>>>  struct icc_bwmon_data {
>>>>  	unsigned int sample_ms;
>>>>  	unsigned int count_unit_kb; /* kbytes */
>>>> @@ -157,6 +179,9 @@ struct icc_bwmon_data {
>>>>  
>>>>  	const struct regmap_config *regmap_cfg;
>>>>  	const struct reg_field *regmap_fields;
>>>> +
>>>> +	const struct regmap_config *global_regmap_cfg;
>>>> +	const struct reg_field *global_regmap_fields;
>>>>  };
>>>>  
>>>>  struct icc_bwmon {
>>>> @@ -166,6 +191,7 @@ struct icc_bwmon {
>>>>  
>>>>  	struct regmap *regmap;
>>>>  	struct regmap_field *regs[F_NUM_FIELDS];
>>>> +	struct regmap_field *global_regs[F_NUM_FIELDS];
>>>>  
>>>>  	unsigned int max_bw_kbps;
>>>>  	unsigned int min_bw_kbps;
>>>> @@ -175,8 +201,8 @@ struct icc_bwmon {
>>>>  
>>>>  /* BWMON v4 */
>>>>  static const struct reg_field msm8998_bwmon_reg_fields[] = {
>>>> -	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>>>> -	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>>>> +	[F_GLB_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0, 0),
>>>> +	[F_GLB_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE_845, 0, 0),
>>>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V4_IRQ_STATUS, 4, 7),
>>>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V4_IRQ_CLEAR, 4, 7),
>>>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V4_IRQ_ENABLE, 4, 7),
>>>> @@ -202,7 +228,7 @@ static const struct reg_field msm8998_bwmon_reg_fields[] = {
>>>>  };
>>>>  
>>>>  static const struct regmap_range msm8998_bwmon_reg_noread_ranges[] = {
>>>> -	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>>>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR_845, BWMON_V4_GLOBAL_IRQ_CLEAR_845),
>>>>  	regmap_reg_range(BWMON_V4_IRQ_CLEAR, BWMON_V4_IRQ_CLEAR),
>>>>  	regmap_reg_range(BWMON_V4_CLEAR, BWMON_V4_CLEAR),
>>>>  };
>>>> @@ -222,16 +248,34 @@ static const struct regmap_access_table msm8998_bwmon_reg_volatile_table = {
>>>>  	.n_yes_ranges	= ARRAY_SIZE(msm8998_bwmon_reg_volatile_ranges),
>>>>  };
>>>>  
>>>> +static const struct reg_field msm8998_bwmon_global_reg_fields[] = {
>>>> +	[F_GLOBAL_IRQ_CLEAR]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_CLEAR, 0, 0),
>>>> +	[F_GLOBAL_IRQ_ENABLE]	= REG_FIELD(BWMON_V4_GLOBAL_IRQ_ENABLE, 0, 0),
>>>> +};
>>>> +
>>>> +static const struct regmap_range msm8998_bwmon_global_reg_noread_ranges[] = {
>>>> +	regmap_reg_range(BWMON_V4_GLOBAL_IRQ_CLEAR, BWMON_V4_GLOBAL_IRQ_CLEAR),
>>>> +};
>>>> +
>>>> +static const struct regmap_access_table msm8998_bwmon_global_reg_read_table = {
>>>> +	.no_ranges	= msm8998_bwmon_global_reg_noread_ranges,
>>>> +	.n_no_ranges	= ARRAY_SIZE(msm8998_bwmon_global_reg_noread_ranges),
>>>> +};
>>>> +
>>>>  /*
>>>>   * Fill the cache for non-readable registers only as rest does not really
>>>>   * matter and can be read from the device.
>>>>   */
>>>>  static const struct reg_default msm8998_bwmon_reg_defaults[] = {
>>>> -	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>>>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR_845, 0x0 },
>>>>  	{ BWMON_V4_IRQ_CLEAR, 0x0 },
>>>>  	{ BWMON_V4_CLEAR, 0x0 },
>>>>  };
>>>>  
>>>> +static const struct reg_default msm8998_bwmon_global_reg_defaults[] = {
>>>> +	{ BWMON_V4_GLOBAL_IRQ_CLEAR, 0x0 },
>>>> +};
>>>> +
>>>>  static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>>>  	.reg_bits		= 32,
>>>>  	.reg_stride		= 4,
>>>> @@ -252,10 +296,27 @@ static const struct regmap_config msm8998_bwmon_regmap_cfg = {
>>>>  	.cache_type		= REGCACHE_RBTREE,
>>>>  };
>>>>  
>>>> +static const struct regmap_config msm8998_bwmon_global_regmap_cfg = {
>>>> +	.reg_bits		= 32,
>>>> +	.reg_stride		= 4,
>>>> +	.val_bits		= 32,
>>>> +	/*
>>>> +	 * No concurrent access expected - driver has one interrupt handler,
>>>> +	 * regmap is not shared, no driver or user-space API.
>>>> +	 */
>>>> +	.disable_locking	= true,
>>>> +	.rd_table		= &msm8998_bwmon_global_reg_read_table,
>>>> +	.reg_defaults		= msm8998_bwmon_global_reg_defaults,
>>>> +	.num_reg_defaults	= ARRAY_SIZE(msm8998_bwmon_global_reg_defaults),
>>>> +	/*
>>>> +	 * Cache is necessary for using regmap fields with non-readable
>>>> +	 * registers.
>>>> +	 */
>>>> +	.cache_type		= REGCACHE_RBTREE,
>>>> +};
>>>> +
>>>>  /* BWMON v5 */
>>>>  static const struct reg_field sdm845_llcc_bwmon_reg_fields[] = {
>>>> -	[F_GLOBAL_IRQ_CLEAR]	= {},
>>>> -	[F_GLOBAL_IRQ_ENABLE]	= {},
>>>>  	[F_IRQ_STATUS]		= REG_FIELD(BWMON_V5_IRQ_STATUS, 0, 3),
>>>>  	[F_IRQ_CLEAR]		= REG_FIELD(BWMON_V5_IRQ_CLEAR, 0, 3),
>>>>  	[F_IRQ_ENABLE]		= REG_FIELD(BWMON_V5_IRQ_ENABLE, 0, 3),
>>>> @@ -369,16 +430,21 @@ static void bwmon_clear_irq(struct icc_bwmon *bwmon)
>>>>  	regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], BWMON_IRQ_ENABLE_MASK);
>>>>  	if (bwmon->data->quirks & BWMON_NEEDS_FORCE_CLEAR)
>>>>  		regmap_field_force_write(bwmon->regs[F_IRQ_CLEAR], 0);
>>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>>> -		regmap_field_force_write(bwmon->regs[F_GLOBAL_IRQ_CLEAR],
>>>> +	if (bwmon->global_regs[0])
>>>> +		regmap_field_force_write(bwmon->global_regs[F_GLOBAL_IRQ_CLEAR],
>>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>> +	else
>>>> +		regmap_field_force_write(bwmon->regs[F_GLB_IRQ_CLEAR],
>>>>  					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>>  }
>>>>  
>>>>  static void bwmon_disable(struct icc_bwmon *bwmon)
>>>>  {
>>>>  	/* Disable interrupts. Strict ordering, see bwmon_clear_irq(). */
>>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>>>> +	if (bwmon->global_regs[0])
>>>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE], 0x0);
>>>> +	else
>>>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE], 0x0);
>>>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], 0x0);
>>>>  
>>>>  	/*
>>>> @@ -391,9 +457,13 @@ static void bwmon_disable(struct icc_bwmon *bwmon)
>>>>  static void bwmon_enable(struct icc_bwmon *bwmon, unsigned int irq_enable)
>>>>  {
>>>>  	/* Enable interrupts */
>>>> -	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ)
>>>> -		regmap_field_write(bwmon->regs[F_GLOBAL_IRQ_ENABLE],
>>>> -				   BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>> +	if (bwmon->global_regs[0])
>>>> +		regmap_field_write(bwmon->global_regs[F_GLOBAL_IRQ_ENABLE],
>>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>> +	else
>>>> +		regmap_field_write(bwmon->regs[F_GLB_IRQ_ENABLE],
>>>> +					 BWMON_V4_GLOBAL_IRQ_ENABLE_ENABLE);
>>>
>>> Probably this would be more readable if regmap_field_write() is called
>>> only once and you parametrize the first argument (field) from the
>>> 'struct bwmon'.
>> Sounds good
>>
>>>
>>>> +
>>>>  	regmap_field_write(bwmon->regs[F_IRQ_ENABLE], irq_enable);
>>>>  
>>>>  	/* Enable bwmon */
>>>> @@ -556,7 +626,9 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>>>  	struct device *dev = &pdev->dev;
>>>>  	void __iomem *base;
>>>>  	struct regmap *map;
>>>> +	int ret;
>>>>  
>>>> +	/* Map the monitor base */
>>>>  	base = devm_platform_ioremap_resource(pdev, 0);
>>>>  	if (IS_ERR(base))
>>>>  		return dev_err_probe(dev, PTR_ERR(base),
>>>> @@ -567,12 +639,38 @@ static int bwmon_init_regmap(struct platform_device *pdev,
>>>>  		return dev_err_probe(dev, PTR_ERR(map),
>>>>  				     "failed to initialize regmap\n");
>>>>  
>>>> +	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_global_reg_fields) != F_NUM_GLOBAL_FIELDS);
>>>>  	BUILD_BUG_ON(ARRAY_SIZE(msm8998_bwmon_reg_fields) != F_NUM_FIELDS);
>>>>  	BUILD_BUG_ON(ARRAY_SIZE(sdm845_llcc_bwmon_reg_fields) != F_NUM_FIELDS);
>>>>  
>>>> -	return devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>>>> +	ret = devm_regmap_field_bulk_alloc(dev, map, bwmon->regs,
>>>>  					   bwmon->data->regmap_fields,
>>>>  					   F_NUM_FIELDS);
>>>
>>> What exactly happens now on msm8998 (or updated sdm845 from your binding
>>> example) for the "global" fields in this region? The regmap references
>>> non-existing fields for the "monitor" region, doesn't it?
>> That's correct, the regmap references 3 non-existstent (or at least
>> not-what-we-think-they-are registers (ending with _845) for
>> targets with two separate regions (such as MSM8998), but they're
>> unaccessed if the "global" region has been mapped sucessfully
>> [see `if (bwmon->global_regs[0])`].
>>
>>>
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>>>> +		/* Map the global base, if separate */
>>>> +		base = devm_platform_ioremap_resource(pdev, 1);
>>>
>>> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
>> I explicitly stated this in the cover letter and asked for opinions.
>>
> 
> Sorry, long time ago I stopped reading cover letters, maybe except it's
> top few sentences. Just too many of them and too much of text usually
> useless. Commits should describe everything as they go to the Git and
> they should justify their own existence.
> 
> Anyway, above dmesg error regression is a no. The devices turn out not
> to be compatible with each other so just adjust the bindings and match
> each driver by proper compatible string.
So what am I supposed to do here? Add "qcom,msm8998-actual-msm8998-bwmon"?

Otherwise, changing msm8998 data would break 845-8550, unless I added all
of them to a separate match table. Or I can add some boilerplate C code
that would not throw a warning, or perhaps try and introduce
devm_platform_ioremap_resource_optional..

I guess the last option sounds the best.

Konrad
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 2/3] soc: qcom: icc-bwmon: Handle global registers correctly

[Krzysztof Kozlowski]

On 06/03/2023 13:17, Konrad Dybcio wrote:
>> That's still nothing to fix - msm8998 was added here only for
>> compatibility reasons for bindings. It wasn't ever tested on MSM8998 and
>> also probably never written in a way that it should work for MSM8998.
> Don't you think making something like
> 
> compatible = "qcom,msm8998-bwmon"
> 
> not actually compatible with MSM8998 is a bit wrong?

I would argue that's the binding problem, but I get your point. Driver
just incorrectly stated that it could work with msm8998.

> 
>  It
>> could work, but that was not the intention. The driver is supposed to
>> work on sdm845 and according to your description - there is nothing
>> wrong with that.
>>
>>>

(...)

>>>
>>>>
>>>>> +	if (ret)
>>>>> +		return ret;
>>>>> +
>>>>> +	if (bwmon->data->quirks & BWMON_HAS_GLOBAL_IRQ) {
>>>>> +		/* Map the global base, if separate */
>>>>> +		base = devm_platform_ioremap_resource(pdev, 1);
>>>>
>>>> Wouldn't this now print errors for sdm845, thus introduce dmesg regression?
>>> I explicitly stated this in the cover letter and asked for opinions.
>>>
>>
>> Sorry, long time ago I stopped reading cover letters, maybe except it's
>> top few sentences. Just too many of them and too much of text usually
>> useless. Commits should describe everything as they go to the Git and
>> they should justify their own existence.
>>
>> Anyway, above dmesg error regression is a no. The devices turn out not
>> to be compatible with each other so just adjust the bindings and match
>> each driver by proper compatible string.
> So what am I supposed to do here? Add "qcom,msm8998-actual-msm8998-bwmon"?

No, make msm8998 binding working for msm8998 and add entry for sdm845.


> Otherwise, changing msm8998 data would break 845-8550, unless I added all
> of them to a separate match table.

Exactly.

>  Or I can add some boilerplate C code
> that would not throw a warning, or perhaps try and introduce
> devm_platform_ioremap_resource_optional..
> 
> I guess the last option sounds the best.

Best regards,
Krzysztof