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

[Krzysztof Kozlowski]

On 13/03/2023 12:41, 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.
> 
> Handle the case where the global registers are mapped separately to allow
> proper functioning of BWMONv4 on MSM8998 and older. Add specific
> compatibles for 845, 8280xp, 7280 and 8550 (all of which use the single
> reg space scheme) to keep backwards compatibility with old DTs.
> 
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
>  drivers/soc/qcom/icc-bwmon.c | 228 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 208 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/soc/qcom/icc-bwmon.c b/drivers/soc/qcom/icc-bwmon.c
> index d07be3700db6..2fe67a3cd2d7 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,12 @@
>  #define BWMON_NEEDS_FORCE_CLEAR			BIT(1)
>  
>  enum bwmon_fields {
> +	/* Global region fields, keep them at the top */
>  	F_GLOBAL_IRQ_CLEAR,
>  	F_GLOBAL_IRQ_ENABLE,
> +	F_NUM_GLOBAL_FIELDS,
> +
> +	/* Monitor region fields */
>  	F_IRQ_STATUS,

F_IRQ_STATUS = F_NUM_GLOBAL_FIELDS
or = 2, so you won't waste one space in the array.

>  	F_IRQ_CLEAR,
>  	F_IRQ_ENABLE,
> @@ -157,6 +174,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 +186,7 @@ struct icc_bwmon {
>  
>  	struct regmap *regmap;
>  	struct regmap_field *regs[F_NUM_FIELDS];
> +	struct regmap_field *global_regs[F_NUM_FIELDS];

F_NUM_GLOBAL_FIELDS?

>  

(...)

>  }
>  
>  static int bwmon_probe(struct platform_device *pdev)
> @@ -645,6 +816,21 @@ 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_ddr_bwmon_data = {

The name "ddr" is here (and other places) confusing. This is not the DDR
bwmon.

> +	.sample_ms = 4,
> +	.count_unit_kb = 64,
> +	.default_highbw_kbps = 4800 * 1024, /* 4.8 GBps */
> +	.default_medbw_kbps = 512 * 1024, /* 512 MBps */
> +	.default_lowbw_kbps = 0,
> +	.zone1_thres_count = 16,
> +	.zone3_thres_count = 1,
> +	.quirks = BWMON_HAS_GLOBAL_IRQ,
> +	.regmap_fields = sdm845_ddr_bwmon_reg_fields,
> +	.regmap_cfg = &sdm845_ddr_bwmon_regmap_cfg,
>  };
>  
>  static const struct icc_bwmon_data sdm845_llcc_bwmon_data = {
> @@ -673,16 +859,18 @@ static const struct icc_bwmon_data sc7280_llcc_bwmon_data = {
>  };
>  
>  static const struct of_device_id bwmon_of_match[] = {
> -	{
> -		.compatible = "qcom,msm8998-bwmon",
> -		.data = &msm8998_bwmon_data
> -	}, {
> -		.compatible = "qcom,sdm845-llcc-bwmon",
> -		.data = &sdm845_llcc_bwmon_data
> -	}, {
> -		.compatible = "qcom,sc7280-llcc-bwmon",
> -		.data = &sc7280_llcc_bwmon_data
> -	},
> +	/* BWMONv4, separate monitor and global register spaces */
> +	{ .compatible = "qcom,msm8998-bwmon", .data = &msm8998_bwmon_data },
> +	/* BWMONv4, unified register space */
> +	{ .compatible = "qcom,sdm845-bwmon", .data = &sdm845_ddr_bwmon_data },
> +	/* BWMONv5 */
> +	{ .compatible = "qcom,sdm845-llcc-bwmon", .data = &sdm845_llcc_bwmon_data },
> +	{ .compatible = "qcom,sc7280-llcc-bwmon", .data = &sc7280_llcc_bwmon_data },
> +
> +	/* Compatibles kept for legacy reasons */
> +	{ .compatible = "qcom,sc7280-cpu-bwmon", .data = &sdm845_ddr_bwmon_data },
> +	{ .compatible = "qcom,sc8280xp-cpu-bwmon", .data = &sdm845_ddr_bwmon_data },
> +	{ .compatible = "qcom,sm8550-cpu-bwmon", .data = &sdm845_ddr_bwmon_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, bwmon_of_match);
> 

Best regards,
Krzysztof