| Message ID | 20200908075716.30357-6-manivannan.sadhasivam@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | Add CPUFreq support for SM8250 SoC | expand |
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. > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 8 deletions(-) > > 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; > -- > 2.17.1
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. Moreover it handles the proper locking for us in the core (spinlock vs mutex). I've seen many subsystem maintainers prefer regmap over plain readl/writel calls. I'll add the reason in commit log. Thanks, Mani > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++----- > > 1 file changed, 47 insertions(+), 8 deletions(-) > > > > 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; > > -- > > 2.17.1 > > -- > viresh
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 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. > > > 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 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. We don't strictly need it for just different register offsets. Regards, Amit
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 Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote: > Use regmap for accessing cpufreq registers in hardware. > The content of the patch looks good, but in itself I don't see the reason for migrating to regmap. If you have subsequent patches, that would benefit from describing the hardware differences using reg_fields then it might be a good idea, but I would suggest that you postpone this patch until there's an actual beneficiary. Regards, Bjorn > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++----- > 1 file changed, 47 insertions(+), 8 deletions(-) > > 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; > -- > 2.17.1 >
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(-)