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