| Message ID | 20200908075716.30357-6-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible | expand |
On 08-09-20, 16:41, Manivannan Sadhasivam wrote: > On 0908, Viresh Kumar wrote: > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote: > > > Use regmap for accessing cpufreq registers in hardware. > > > > Why ? Please mention why a change is required in the log. > > > > Only because it is recommended to use regmap for abstracting the hw access. Yes it can be very useful in abstracting the hw access in case of busses like SPI/I2C, others, but in this case there is only one way of doing it with the exact same registers. I am not sure it is worth it here. FWIW, I have never played with regmaps personally, and so every chance I can be wrong here. > Moreover it handles the proper locking for us in the core (spinlock vs mutex). What locking do you need here ? > I've seen many subsystem maintainers prefer regmap over plain readl/writel > calls. I'll add the reason in commit log. I am not sure if it is worth it here.
On 0908, Viresh Kumar wrote: > On 08-09-20, 16:41, Manivannan Sadhasivam wrote: > > On 0908, Viresh Kumar wrote: > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote: > > > > Use regmap for accessing cpufreq registers in hardware. > > > > > > Why ? Please mention why a change is required in the log. > > > > > > > Only because it is recommended to use regmap for abstracting the hw access. > > Yes it can be very useful in abstracting the hw access in case of > busses like SPI/I2C, others, but in this case there is only one way of > doing it with the exact same registers. I am not sure it is worth it > here. FWIW, I have never played with regmaps personally, and so every > chance I can be wrong here. > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex). > > What locking do you need here ? > I was just referring the case where if we need the locking in future, regmap handles it nicely in the core. > > I've seen many subsystem maintainers prefer regmap over plain readl/writel > > calls. I'll add the reason in commit log. > > I am not sure if it is worth it here. > Hmm, I thought it is recommended to use regmap for MMIO access as well. I can drop the patch if you want but let's wait for Bjorn/Amit to get their views. Thanks, Mani > -- > viresh
On Tue, Sep 08, 2020 at 05:18:35PM +0530, Amit Kucheria wrote: > On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 08-09-20, 16:41, Manivannan Sadhasivam wrote: > > > On 0908, Viresh Kumar wrote: > > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote: > > > > > Use regmap for accessing cpufreq registers in hardware. > > > > > > > > Why ? Please mention why a change is required in the log. > > > > > > > > > > Only because it is recommended to use regmap for abstracting the hw access. > > > > Yes it can be very useful in abstracting the hw access in case of > > busses like SPI/I2C, others, but in this case there is only one way of > > doing it with the exact same registers. I am not sure it is worth it > > here. FWIW, I have never played with regmaps personally, and so every > > chance I can be wrong here. > > One could handle the reg offsets through a struct initialisation, but > then you end up with lots of #defines for bitmasks and bits for each > version of the IP. And the core code becomes a bit convoluted IMO, > trying to handle the differences. > > regmap hides the differences of the bit positions and register offsets > between several IP versions. > > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex). > > > > What locking do you need here ? > > Right, locking isn't the main reason here. If that is the case, IMO it is better to set disable_lock or something in config especially as this regmap_write is used in qcom_cpufreq_hw_fast_switch
On 0908, Amit Kucheria wrote: > On Tue, Sep 8, 2020 at 5:18 PM Amit Kucheria <amitk@kernel.org> wrote: > > > > On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 08-09-20, 16:41, Manivannan Sadhasivam wrote: > > > > On 0908, Viresh Kumar wrote: > > > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote: > > > > > > Use regmap for accessing cpufreq registers in hardware. > > > > > > > > > > Why ? Please mention why a change is required in the log. > > > > > > > > > > > > > Only because it is recommended to use regmap for abstracting the hw access. > > > > > > Yes it can be very useful in abstracting the hw access in case of > > > busses like SPI/I2C, others, but in this case there is only one way of > > > doing it with the exact same registers. I am not sure it is worth it > > > here. FWIW, I have never played with regmaps personally, and so every > > > chance I can be wrong here. > > > > One could handle the reg offsets through a struct initialisation, but > > then you end up with lots of #defines for bitmasks and bits for each > > version of the IP. And the core code becomes a bit convoluted IMO, > > trying to handle the differences. > > > > regmap hides the differences of the bit positions and register offsets > > between several IP versions. > > > > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex). > > > > > > What locking do you need here ? > > > > Right, locking isn't the main reason here. > > Having said this, perhaps this patch can be held back for now, since > we're not yet using some of the features of regmap to abstract away > bit fields and such. > Okay. Dropping this patch for now (in v2)! Thanks, Mani > We don't strictly need it for just different register offsets. > > Regards, > Amit
On 08-09-20, 17:38, Amit Kucheria wrote: > On Tue, Sep 8, 2020 at 5:18 PM Amit Kucheria <amitk@kernel.org> wrote: > > > > On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 08-09-20, 16:41, Manivannan Sadhasivam wrote: > > > > On 0908, Viresh Kumar wrote: > > > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote: > > > > > > Use regmap for accessing cpufreq registers in hardware. > > > > > > > > > > Why ? Please mention why a change is required in the log. > > > > > > > > > > > > > Only because it is recommended to use regmap for abstracting the hw access. > > > > > > Yes it can be very useful in abstracting the hw access in case of > > > busses like SPI/I2C, others, but in this case there is only one way of > > > doing it with the exact same registers. I am not sure it is worth it > > > here. FWIW, I have never played with regmaps personally, and so every > > > chance I can be wrong here. > > > > One could handle the reg offsets through a struct initialisation, but > > then you end up with lots of #defines for bitmasks and bits for each > > version of the IP. And the core code becomes a bit convoluted IMO, > > trying to handle the differences. > > > > regmap hides the differences of the bit positions and register offsets > > between several IP versions. Right and I agree that is another useful aspect of it which I missed mentioning. > > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex). > > > > > > What locking do you need here ? > > > > Right, locking isn't the main reason here. > > Having said this, perhaps this patch can be held back for now, since > we're not yet using some of the features of regmap to abstract away > bit fields and such. > > We don't strictly need it for just different register offsets. Right, I just didn't understood why it was required currently as it wasn't all that complex at all.
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index 41853db7c9b8..de816bcafd33 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -12,6 +12,7 @@ #include <linux/of_address.h> #include <linux/of_platform.h> #include <linux/pm_opp.h> +#include <linux/regmap.h> #include <linux/slab.h> #define LUT_MAX_ENTRIES 40U @@ -32,6 +33,7 @@ struct qcom_cpufreq_soc_data { struct qcom_cpufreq_data { void __iomem *base; + struct regmap *regmap; const struct qcom_cpufreq_soc_data *soc_data; }; @@ -85,8 +87,11 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy, struct qcom_cpufreq_data *data = policy->driver_data; const struct qcom_cpufreq_soc_data *soc_data = data->soc_data; unsigned long freq = policy->freq_table[index].frequency; + int ret; - writel_relaxed(index, data->base + soc_data->reg_perf_state); + ret = regmap_write(data->regmap, soc_data->reg_perf_state, index); + if (ret) + return ret; if (icc_scaling_enabled) qcom_cpufreq_set_bw(policy, freq); @@ -102,6 +107,7 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) const struct qcom_cpufreq_soc_data *soc_data; struct cpufreq_policy *policy; unsigned int index; + int ret; policy = cpufreq_cpu_get_raw(cpu); if (!policy) @@ -110,7 +116,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu) data = policy->driver_data; soc_data = data->soc_data; - index = readl_relaxed(data->base + soc_data->reg_perf_state); + ret = regmap_read(data->regmap, soc_data->reg_perf_state, &index); + if (ret) + return 0; + index = min(index, LUT_MAX_ENTRIES - 1); return policy->freq_table[index].frequency; @@ -123,9 +132,12 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy, const struct qcom_cpufreq_soc_data *soc_data = data->soc_data; unsigned int index; unsigned long freq; + int ret; index = policy->cached_resolved_idx; - writel_relaxed(index, data->base + soc_data->reg_perf_state); + ret = regmap_write(data->regmap, soc_data->reg_perf_state, index); + if (ret) + return 0; freq = policy->freq_table[index].frequency; arch_set_freq_scale(policy->related_cpus, freq, @@ -171,14 +183,24 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev, } for (i = 0; i < LUT_MAX_ENTRIES; i++) { - data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut + - i * soc_data->lut_row_size); + ret = regmap_read(drv_data->regmap, soc_data->reg_freq_lut + + i * soc_data->lut_row_size, &data); + if (ret) { + kfree(table); + return ret; + } + src = FIELD_GET(LUT_SRC, data); lval = FIELD_GET(LUT_L_VAL, data); core_count = FIELD_GET(LUT_CORE_COUNT, data); - data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut + - i * soc_data->lut_row_size); + ret = regmap_read(drv_data->regmap, soc_data->reg_volt_lut + + i * soc_data->lut_row_size, &data); + if (ret) { + kfree(table); + return ret; + } + volt = FIELD_GET(LUT_VOLT, data) * 1000; if (src) @@ -248,6 +270,13 @@ static void qcom_get_related_cpus(int index, struct cpumask *m) } } +static struct regmap_config qcom_cpufreq_regmap = { + .reg_bits = 32, + .reg_stride = 4, + .val_bits = 32, + .fast_io = true, +}; + static const struct qcom_cpufreq_soc_data qcom_soc_data = { .reg_enable = 0x0, .reg_freq_lut = 0x110, @@ -274,6 +303,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) struct qcom_cpufreq_data *data; const struct of_device_id *match; int ret, index; + u32 val; cpu_dev = get_cpu_device(policy->cpu); if (!cpu_dev) { @@ -316,9 +346,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) data->soc_data = match->data; data->base = base; + data->regmap = devm_regmap_init_mmio(dev, base, &qcom_cpufreq_regmap); + if (IS_ERR(data->regmap)) { + ret = PTR_ERR(data->regmap); + goto error; + } /* HW should be in enabled state to proceed */ - if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) { + ret = regmap_read(data->regmap, data->soc_data->reg_enable, &val); + if (ret) + goto error; + + if (!(val & 0x1)) { dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index); ret = -ENODEV; goto error;
Use regmap for accessing cpufreq registers in hardware. Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> --- drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++----- 1 file changed, 47 insertions(+), 8 deletions(-)