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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 15 Mar 2023 11:13:44 +0100
From:   Konrad Dybcio <konrad.dybcio@...aro.org>
To:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Georgi Djakov <djakov@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Thara Gopinath <thara.gopinath@...il.com>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
Cc:     linux-arm-msm@...r.kernel.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/7] soc: qcom: icc-bwmon: Handle global registers
 correctly



On 15.03.2023 08:27, Krzysztof Kozlowski wrote:
> 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@...aro.org>
>> ---
>>  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.
Good find!

> 
>>  	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?
Totally.

> 
>>  
> 
> (...)
> 
>>  }
>>  
>>  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.
I suppose cpu_bwmon could make more sense?

Konrad
> 
>> +	.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
>