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