| Message ID | 20210608222926.2707768-4-thara.gopinath@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Introduce LMh driver for Qualcomm SoCs | expand |
On 6/14/21 6:31 AM, Viresh Kumar wrote: > On 08-06-21, 18:29, Thara Gopinath wrote: >> Add interrupt support to notify the kernel of h/w initiated frequency >> throttling by LMh. Convey this to scheduler via thermal presssure >> interface. >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> drivers/cpufreq/qcom-cpufreq-hw.c | 100 ++++++++++++++++++++++++++++++ >> 1 file changed, 100 insertions(+) >> >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c >> index f86859bf76f1..95e17330aa9d 100644 >> --- a/drivers/cpufreq/qcom-cpufreq-hw.c >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c >> @@ -13,6 +13,7 @@ >> #include <linux/of_platform.h> >> #include <linux/pm_opp.h> >> #include <linux/slab.h> >> +#include <linux/interrupt.h> >> >> #define LUT_MAX_ENTRIES 40U >> #define LUT_SRC GENMASK(31, 30) >> @@ -22,10 +23,13 @@ >> #define CLK_HW_DIV 2 >> #define LUT_TURBO_IND 1 >> >> +#define HZ_PER_KHZ 1000 >> + >> struct qcom_cpufreq_soc_data { >> u32 reg_enable; >> u32 reg_freq_lut; >> u32 reg_volt_lut; >> + u32 reg_current_vote; >> u32 reg_perf_state; >> u8 lut_row_size; >> }; >> @@ -33,7 +37,11 @@ struct qcom_cpufreq_soc_data { >> struct qcom_cpufreq_data { >> void __iomem *base; >> struct resource *res; >> + struct delayed_work lmh_dcvs_poll_work; >> const struct qcom_cpufreq_soc_data *soc_data; >> + cpumask_var_t cpus; >> + unsigned long throttled_freq; >> + int lmh_dcvs_irq; >> }; >> >> static unsigned long cpu_hw_rate, xo_rate; >> @@ -251,10 +259,79 @@ static void qcom_get_related_cpus(int index, struct cpumask *m) >> } >> } >> >> +static inline unsigned long qcom_lmh_vote_to_freq(u32 val) >> +{ >> + return (val & 0x3FF) * 19200; >> +} >> + >> +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) >> +{ >> + struct cpufreq_policy policy; >> + struct dev_pm_opp *opp; >> + struct device *dev; >> + unsigned long max_capacity, capacity, freq_hz; >> + unsigned int val, freq; >> + >> + val = readl_relaxed(data->base + data->soc_data->reg_current_vote); >> + freq = qcom_lmh_vote_to_freq(val); >> + freq_hz = freq * HZ_PER_KHZ; >> + >> + /* Do I need to calculate ceil and floor ? */ > > You don't know ? stray comment! Will remove it. > >> + dev = get_cpu_device(cpumask_first(data->cpus)); >> + opp = dev_pm_opp_find_freq_floor(dev, &freq_hz); >> + if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE) >> + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz); >> + >> + data->throttled_freq = freq_hz / HZ_PER_KHZ; >> + > > What exactly are we trying to do here ? A comment would be good as > well. You want me to put a comment saying converting frequency in hz to khz ? > >> + cpufreq_get_policy(&policy, cpumask_first(data->cpus)); >> + >> + /* Update thermal pressure */ >> + max_capacity = arch_scale_cpu_capacity(cpumask_first(data->cpus)); > > Set capacity of a single CPU from a policy ? Get maximum capacity of a cpu. > >> + capacity = data->throttled_freq * max_capacity; >> + capacity /= policy.cpuinfo.max_freq; >> + /* Don't pass boost capacity to scheduler */ >> + if (capacity > max_capacity) >> + capacity = max_capacity; >> + arch_set_thermal_pressure(data->cpus, max_capacity - capacity); > > You should really be using policy->cpus instead of allocating > data->cpus.. Yes I should be. But I still need data->cpus to get the policy. > >> +} >> + >> +static void qcom_lmh_dcvs_poll(struct work_struct *work) >> +{ >> + struct qcom_cpufreq_data *data; >> + >> + data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work); >> + >> + qcom_lmh_dcvs_notify(data); > > You should really move the below stuff the disable_irq_nosync(), it > will make your life easier. I don't understand your comment here. I want to disable irq. call notify. Start polling. And in polling I want to call notify and if the thermal event has passed stop polling else continue polling. > >> + /** >> + * If h/w throttled frequency is higher than what cpufreq has requested for, stop >> + * polling and switch back to interrupt mechanism >> + */ >> + if (data->throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(data->cpus))) >> + /* Clear the existing interrupts and enable it back */ >> + enable_irq(data->lmh_dcvs_irq); >> + else >> + mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work, >> + msecs_to_jiffies(10)); >> +} >> + >> +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data) >> +{ >> + struct qcom_cpufreq_data *c_data = data; >> + >> + /* Disable interrupt and enable polling */ >> + disable_irq_nosync(c_data->lmh_dcvs_irq); >> + qcom_lmh_dcvs_notify(c_data); >> + mod_delayed_work(system_highpri_wq, &c_data->lmh_dcvs_poll_work, msecs_to_jiffies(10)); >> + >> + return 0; >> +} >> + >> static const struct qcom_cpufreq_soc_data qcom_soc_data = { >> .reg_enable = 0x0, >> .reg_freq_lut = 0x110, >> .reg_volt_lut = 0x114, >> + .reg_current_vote = 0x704, > > Should this be a different patch ? Why ? This is the register to read the throttled frequency. > >> .reg_perf_state = 0x920, >> .lut_row_size = 32, >> }; >> @@ -285,6 +362,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> void __iomem *base; >> struct qcom_cpufreq_data *data; >> int ret, index; >> + bool lmh_mitigation_enabled = false; > > You just overwrite it below, no need to initialize it. Sure. > >> >> cpu_dev = get_cpu_device(policy->cpu); >> if (!cpu_dev) { >> @@ -305,6 +383,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> >> index = args.args[0]; >> >> + lmh_mitigation_enabled = of_property_read_bool(pdev->dev.of_node, "qcom,support-lmh"); >> + >> res = platform_get_resource(pdev, IORESOURCE_MEM, index); >> if (!res) { >> dev_err(dev, "failed to get mem resource %d\n", index); >> @@ -329,6 +409,11 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> goto unmap_base; >> } >> >> + if (!alloc_cpumask_var(&data->cpus, GFP_KERNEL)) { >> + ret = -ENOMEM; >> + goto unmap_base; >> + } >> + >> data->soc_data = of_device_get_match_data(&pdev->dev); >> data->base = base; >> data->res = res; >> @@ -347,6 +432,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> goto error; >> } >> >> + cpumask_copy(data->cpus, policy->cpus); >> policy->driver_data = data; >> >> ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy); >> @@ -370,6 +456,20 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); >> } >> >> + if (lmh_mitigation_enabled) { > > Shouldn't you move the allocation and setting of data->cpus here ? I > suggest creating a separate routine for all initialization around this > stuff. I should considering nothing else is using data->cpus. Yes I will create a separate init function. > >> + data->lmh_dcvs_irq = platform_get_irq(pdev, index); >> + if (data->lmh_dcvs_irq < 0) { >> + ret = data->lmh_dcvs_irq; >> + goto error; >> + } >> + ret = devm_request_irq(dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq, >> + 0, "dcvsh-irq", data); > > I would rather pass policy as data here. So policy for a cpu can change runtime, right ? > >> + if (ret) { >> + dev_err(dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq); >> + goto error; >> + } >> + INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll); >> + } >> return 0; >> error: >> kfree(data); >
On 14-06-21, 21:58, Thara Gopinath wrote: > On 6/14/21 6:31 AM, Viresh Kumar wrote: > > On 08-06-21, 18:29, Thara Gopinath wrote: > > > +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) > > > +{ > > > + struct cpufreq_policy policy; > > > + struct dev_pm_opp *opp; > > > + struct device *dev; > > > + unsigned long max_capacity, capacity, freq_hz; > > > + unsigned int val, freq; > > > + > > > + val = readl_relaxed(data->base + data->soc_data->reg_current_vote); > > > + freq = qcom_lmh_vote_to_freq(val); > > > + freq_hz = freq * HZ_PER_KHZ; > > > + > > > + /* Do I need to calculate ceil and floor ? */ > > > > You don't know ? > > stray comment! Will remove it. > > > > > > + dev = get_cpu_device(cpumask_first(data->cpus)); > > > + opp = dev_pm_opp_find_freq_floor(dev, &freq_hz); > > > + if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE) > > > + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz); > > > + > > > + data->throttled_freq = freq_hz / HZ_PER_KHZ; > > > + > > > > What exactly are we trying to do here ? A comment would be good as > > well. > > You want me to put a comment saying converting frequency in hz to khz ? Not that, but for the entire routine. What exactly are we looking to do here and why? > > > > > + cpufreq_get_policy(&policy, cpumask_first(data->cpus)); > > > + > > > + /* Update thermal pressure */ > > > + max_capacity = arch_scale_cpu_capacity(cpumask_first(data->cpus)); > > > > Set capacity of a single CPU from a policy ? > > Get maximum capacity of a cpu. Ahh, I thought you are setting it :( > > > > > + capacity = data->throttled_freq * max_capacity; > > > + capacity /= policy.cpuinfo.max_freq; > > > + /* Don't pass boost capacity to scheduler */ > > > + if (capacity > max_capacity) > > > + capacity = max_capacity; > > > + arch_set_thermal_pressure(data->cpus, max_capacity - capacity); > > > > You should really be using policy->cpus instead of allocating > > data->cpus.. > > Yes I should be. But I still need data->cpus to get the policy. From the comment which comes later on, you shall get the policy here anyway. > > > +} > > > + > > > +static void qcom_lmh_dcvs_poll(struct work_struct *work) > > > +{ > > > + struct qcom_cpufreq_data *data; > > > + > > > + data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work); > > > + > > > + qcom_lmh_dcvs_notify(data); > > > > You should really move the below stuff the disable_irq_nosync(), it > > will make your life easier. Damn, s/disable_irq_nosync/qcom_lmh_dcvs_notify/ > I don't understand your comment here. I want to disable irq. call notify. > Start polling. And in polling I want to call notify and if the thermal event > has passed stop polling else continue polling. Yeah, I messed up in the comment. I was asking to move the enable-irq and mod_delayed_work to qcom_lmh_dcvs_notify() itself. > > > + /** > > > + * If h/w throttled frequency is higher than what cpufreq has requested for, stop > > > + * polling and switch back to interrupt mechanism > > > + */ > > > + if (data->throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(data->cpus))) > > > + /* Clear the existing interrupts and enable it back */ > > > + enable_irq(data->lmh_dcvs_irq); > > > + else > > > + mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work, > > > + msecs_to_jiffies(10)); > > > +} > > > + > > > +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data) > > > +{ > > > + struct qcom_cpufreq_data *c_data = data; > > > + > > > + /* Disable interrupt and enable polling */ > > > + disable_irq_nosync(c_data->lmh_dcvs_irq); > > > + qcom_lmh_dcvs_notify(c_data); > > > + mod_delayed_work(system_highpri_wq, &c_data->lmh_dcvs_poll_work, msecs_to_jiffies(10)); > > > + > > > + return 0; > > > +} > > > + > > > static const struct qcom_cpufreq_soc_data qcom_soc_data = { > > > .reg_enable = 0x0, > > > .reg_freq_lut = 0x110, > > > .reg_volt_lut = 0x114, > > > + .reg_current_vote = 0x704, > > > > Should this be a different patch ? > > Why ? This is the register to read the throttled frequency. Okay, it looked this is separate. Leave it. > > > + ret = devm_request_irq(dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq, > > > + 0, "dcvsh-irq", data); > > > > I would rather pass policy as data here. > > So policy for a cpu can change runtime, right ? No, it is allocated just once. -- viresh
On Tue 08 Jun 17:29 CDT 2021, Thara Gopinath wrote: > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c [..] > @@ -305,6 +383,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > > index = args.args[0]; > > + lmh_mitigation_enabled = of_property_read_bool(pdev->dev.of_node, "qcom,support-lmh"); Rather than adding a new interrupt _and_ a flag to tell the driver that this new interrupt should be used, wouldn't it be sufficient to just see if the interrupt is specified? > + > res = platform_get_resource(pdev, IORESOURCE_MEM, index); > if (!res) { > dev_err(dev, "failed to get mem resource %d\n", index); > @@ -329,6 +409,11 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > goto unmap_base; > } > > + if (!alloc_cpumask_var(&data->cpus, GFP_KERNEL)) { > + ret = -ENOMEM; > + goto unmap_base; > + } > + > data->soc_data = of_device_get_match_data(&pdev->dev); > data->base = base; > data->res = res; > @@ -347,6 +432,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > goto error; > } > > + cpumask_copy(data->cpus, policy->cpus); > policy->driver_data = data; > > ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy); > @@ -370,6 +456,20 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); > } > > + if (lmh_mitigation_enabled) { > + data->lmh_dcvs_irq = platform_get_irq(pdev, index); > + if (data->lmh_dcvs_irq < 0) { This will be -ENXIO if the interrupt isn't specified and <0 for other errors, so you should be able to distinguish the two failure cases. Regards, Bjorn > + ret = data->lmh_dcvs_irq; > + goto error; > + } > + ret = devm_request_irq(dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq, > + 0, "dcvsh-irq", data); > + if (ret) { > + dev_err(dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq); > + goto error; > + } > + INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll); > + } > return 0; > error: > kfree(data); > -- > 2.25.1 >
On 6/18/21 2:16 PM, Bjorn Andersson wrote: > On Tue 08 Jun 17:29 CDT 2021, Thara Gopinath wrote: >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c > [..] >> @@ -305,6 +383,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> >> index = args.args[0]; >> >> + lmh_mitigation_enabled = of_property_read_bool(pdev->dev.of_node, "qcom,support-lmh"); > > Rather than adding a new interrupt _and_ a flag to tell the driver that > this new interrupt should be used, wouldn't it be sufficient to just see > if the interrupt is specified? Yes. you are right. It should be. Though when I wrote it there was some reason which I forget now. I will remove it.
diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c index f86859bf76f1..95e17330aa9d 100644 --- a/drivers/cpufreq/qcom-cpufreq-hw.c +++ b/drivers/cpufreq/qcom-cpufreq-hw.c @@ -13,6 +13,7 @@ #include <linux/of_platform.h> #include <linux/pm_opp.h> #include <linux/slab.h> +#include <linux/interrupt.h> #define LUT_MAX_ENTRIES 40U #define LUT_SRC GENMASK(31, 30) @@ -22,10 +23,13 @@ #define CLK_HW_DIV 2 #define LUT_TURBO_IND 1 +#define HZ_PER_KHZ 1000 + struct qcom_cpufreq_soc_data { u32 reg_enable; u32 reg_freq_lut; u32 reg_volt_lut; + u32 reg_current_vote; u32 reg_perf_state; u8 lut_row_size; }; @@ -33,7 +37,11 @@ struct qcom_cpufreq_soc_data { struct qcom_cpufreq_data { void __iomem *base; struct resource *res; + struct delayed_work lmh_dcvs_poll_work; const struct qcom_cpufreq_soc_data *soc_data; + cpumask_var_t cpus; + unsigned long throttled_freq; + int lmh_dcvs_irq; }; static unsigned long cpu_hw_rate, xo_rate; @@ -251,10 +259,79 @@ static void qcom_get_related_cpus(int index, struct cpumask *m) } } +static inline unsigned long qcom_lmh_vote_to_freq(u32 val) +{ + return (val & 0x3FF) * 19200; +} + +static void qcom_lmh_dcvs_notify(struct qcom_cpufreq_data *data) +{ + struct cpufreq_policy policy; + struct dev_pm_opp *opp; + struct device *dev; + unsigned long max_capacity, capacity, freq_hz; + unsigned int val, freq; + + val = readl_relaxed(data->base + data->soc_data->reg_current_vote); + freq = qcom_lmh_vote_to_freq(val); + freq_hz = freq * HZ_PER_KHZ; + + /* Do I need to calculate ceil and floor ? */ + dev = get_cpu_device(cpumask_first(data->cpus)); + opp = dev_pm_opp_find_freq_floor(dev, &freq_hz); + if (IS_ERR(opp) && PTR_ERR(opp) == -ERANGE) + opp = dev_pm_opp_find_freq_ceil(dev, &freq_hz); + + data->throttled_freq = freq_hz / HZ_PER_KHZ; + + cpufreq_get_policy(&policy, cpumask_first(data->cpus)); + + /* Update thermal pressure */ + max_capacity = arch_scale_cpu_capacity(cpumask_first(data->cpus)); + capacity = data->throttled_freq * max_capacity; + capacity /= policy.cpuinfo.max_freq; + /* Don't pass boost capacity to scheduler */ + if (capacity > max_capacity) + capacity = max_capacity; + arch_set_thermal_pressure(data->cpus, max_capacity - capacity); +} + +static void qcom_lmh_dcvs_poll(struct work_struct *work) +{ + struct qcom_cpufreq_data *data; + + data = container_of(work, struct qcom_cpufreq_data, lmh_dcvs_poll_work.work); + + qcom_lmh_dcvs_notify(data); + /** + * If h/w throttled frequency is higher than what cpufreq has requested for, stop + * polling and switch back to interrupt mechanism + */ + if (data->throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(data->cpus))) + /* Clear the existing interrupts and enable it back */ + enable_irq(data->lmh_dcvs_irq); + else + mod_delayed_work(system_highpri_wq, &data->lmh_dcvs_poll_work, + msecs_to_jiffies(10)); +} + +static irqreturn_t qcom_lmh_dcvs_handle_irq(int irq, void *data) +{ + struct qcom_cpufreq_data *c_data = data; + + /* Disable interrupt and enable polling */ + disable_irq_nosync(c_data->lmh_dcvs_irq); + qcom_lmh_dcvs_notify(c_data); + mod_delayed_work(system_highpri_wq, &c_data->lmh_dcvs_poll_work, msecs_to_jiffies(10)); + + return 0; +} + static const struct qcom_cpufreq_soc_data qcom_soc_data = { .reg_enable = 0x0, .reg_freq_lut = 0x110, .reg_volt_lut = 0x114, + .reg_current_vote = 0x704, .reg_perf_state = 0x920, .lut_row_size = 32, }; @@ -285,6 +362,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) void __iomem *base; struct qcom_cpufreq_data *data; int ret, index; + bool lmh_mitigation_enabled = false; cpu_dev = get_cpu_device(policy->cpu); if (!cpu_dev) { @@ -305,6 +383,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) index = args.args[0]; + lmh_mitigation_enabled = of_property_read_bool(pdev->dev.of_node, "qcom,support-lmh"); + res = platform_get_resource(pdev, IORESOURCE_MEM, index); if (!res) { dev_err(dev, "failed to get mem resource %d\n", index); @@ -329,6 +409,11 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) goto unmap_base; } + if (!alloc_cpumask_var(&data->cpus, GFP_KERNEL)) { + ret = -ENOMEM; + goto unmap_base; + } + data->soc_data = of_device_get_match_data(&pdev->dev); data->base = base; data->res = res; @@ -347,6 +432,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) goto error; } + cpumask_copy(data->cpus, policy->cpus); policy->driver_data = data; ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy); @@ -370,6 +456,20 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); } + if (lmh_mitigation_enabled) { + data->lmh_dcvs_irq = platform_get_irq(pdev, index); + if (data->lmh_dcvs_irq < 0) { + ret = data->lmh_dcvs_irq; + goto error; + } + ret = devm_request_irq(dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq, + 0, "dcvsh-irq", data); + if (ret) { + dev_err(dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq); + goto error; + } + INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll); + } return 0; error: kfree(data);
Add interrupt support to notify the kernel of h/w initiated frequency throttling by LMh. Convey this to scheduler via thermal presssure interface. Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> --- drivers/cpufreq/qcom-cpufreq-hw.c | 100 ++++++++++++++++++++++++++++++ 1 file changed, 100 insertions(+) -- 2.25.1