| Message ID | 20210708120656.663851-1-thara.gopinath@linaro.org |
|---|---|
| Headers | show |
| Series | Introduce LMh driver for Qualcomm SoCs | expand |
On 08-07-21, 08:06, Thara Gopinath wrote: > static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > { > struct platform_device *pdev = cpufreq_get_driver_data(); > @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); > } > > + ret = qcom_cpufreq_hw_lmh_init(policy, index); You missed unregistering EM here (which is also missing from exit, which you need to fix first in a separate patch). > + if (ret) > + goto error; > + > return 0; > error: > kfree(data); > @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) > > dev_pm_opp_remove_all_dynamic(cpu_dev); > dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); > + if (data->lmh_dcvs_irq > 0) { > + devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data); Why using devm variants here and while requesting the irq ? > + cancel_delayed_work_sync(&data->lmh_dcvs_poll_work); > + } Please move this to qcom_cpufreq_hw_lmh_exit() or something. Now with sequence of disabling interrupt, etc, I see a potential problem. CPU0 CPU1 qcom_cpufreq_hw_cpu_exit() -> devm_free_irq(); qcom_lmh_dcvs_poll() -> qcom_lmh_dcvs_notify() -> enable_irq() -> cancel_delayed_work_sync(); What will happen if enable_irq() gets called after freeing the irq ? Not sure, but it looks like you will hit this then from manage.c: WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before setup/request_irq: irq %u\n", irq)) ? You got a chicken n egg problem :)
On 7/9/21 2:46 AM, Viresh Kumar wrote: > On 08-07-21, 08:06, Thara Gopinath wrote: >> static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> { >> struct platform_device *pdev = cpufreq_get_driver_data(); >> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); >> } >> >> + ret = qcom_cpufreq_hw_lmh_init(policy, index); > > You missed unregistering EM here (which is also missing from exit, > which you need to fix first in a separate patch). Hi! So how exactly do you do this? I checked other users of the api and I do not see any free. I would say if needed, it should be a separate patch and outside of this series. > >> + if (ret) >> + goto error; >> + >> return 0; >> error: >> kfree(data); >> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) >> >> dev_pm_opp_remove_all_dynamic(cpu_dev); >> dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); >> + if (data->lmh_dcvs_irq > 0) { >> + devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data); > > Why using devm variants here and while requesting the irq ? > >> + cancel_delayed_work_sync(&data->lmh_dcvs_poll_work); >> + } > > Please move this to qcom_cpufreq_hw_lmh_exit() or something. Ok. > > Now with sequence of disabling interrupt, etc, I see a potential > problem. > > CPU0 CPU1 > > qcom_cpufreq_hw_cpu_exit() > -> devm_free_irq(); > qcom_lmh_dcvs_poll() > -> qcom_lmh_dcvs_notify() > -> enable_irq() > > -> cancel_delayed_work_sync(); > > > What will happen if enable_irq() gets called after freeing the irq ? > Not sure, but it looks like you will hit this then from manage.c: > > WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before > setup/request_irq: irq %u\n", irq)) > > ? > > You got a chicken n egg problem :) Yes indeed! But also it is a very rare chicken and egg problem. The scenario here is that the cpus are busy and running load causing a thermal overrun and lmh is engaged. At the same time for this issue to be hit the cpu is trying to exit/disable cpufreq. Calling cancel_delayed_work_sync first could solve this issue, right ? cancel_delayed_work_sync guarantees the work not to be pending even if it requeues itself on return. So once the delayed work is cancelled, the interrupts can be safely disabled. Thoughts ? > -- Warm Regards Thara (She/Her/Hers)
On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote: > Introduce SCM calls to access/configure limits management hardware(LMH). > > Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > > v2->v3: > Added freeing of payload_buf after the scm call in qcom_scm_lmh_dcvsh as per > Matthias review comments. > > v1->v2: > Changed the input parameters in qcom_scm_lmh_dcvsh from payload_buf and > payload_size to payload_fn, payload_reg, payload_val as per Bjorn's review > comments. > > drivers/firmware/qcom_scm.c | 58 +++++++++++++++++++++++++++++++++++++ > drivers/firmware/qcom_scm.h | 4 +++ > include/linux/qcom_scm.h | 14 +++++++++ > 3 files changed, 76 insertions(+) > > diff --git a/drivers/firmware/qcom_scm.c b/drivers/firmware/qcom_scm.c > index ee9cb545e73b..a8d236603e90 100644 > --- a/drivers/firmware/qcom_scm.c > +++ b/drivers/firmware/qcom_scm.c > @@ -1147,6 +1147,64 @@ int qcom_scm_qsmmu500_wait_safe_toggle(bool en) > } > EXPORT_SYMBOL(qcom_scm_qsmmu500_wait_safe_toggle); > > +bool qcom_scm_lmh_dcvsh_available(void) > +{ > + return __qcom_scm_is_call_available(__scm->dev, QCOM_SCM_SVC_LMH, QCOM_SCM_LMH_LIMIT_DCVSH); > +} > +EXPORT_SYMBOL(qcom_scm_lmh_dcvsh_available); > + > +int qcom_scm_lmh_profile_change(u32 profile_id) > +{ > + struct qcom_scm_desc desc = { > + .svc = QCOM_SCM_SVC_LMH, > + .cmd = QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE, > + .arginfo = QCOM_SCM_ARGS(1, QCOM_SCM_VAL), > + .args[0] = profile_id, > + .owner = ARM_SMCCC_OWNER_SIP, > + }; > + > + return qcom_scm_call(__scm->dev, &desc, NULL); > +} > +EXPORT_SYMBOL(qcom_scm_lmh_profile_change); > + > +int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > + u64 limit_node, u32 node_id, u64 version) > +{ > + dma_addr_t payload_phys; > + u32 *payload_buf; > + int ret, payload_size = 5 * sizeof(u32); > + > + struct qcom_scm_desc desc = { > + .svc = QCOM_SCM_SVC_LMH, > + .cmd = QCOM_SCM_LMH_LIMIT_DCVSH, > + .arginfo = QCOM_SCM_ARGS(5, QCOM_SCM_RO, QCOM_SCM_VAL, QCOM_SCM_VAL, > + QCOM_SCM_VAL, QCOM_SCM_VAL), > + .args[1] = payload_size, > + .args[2] = limit_node, > + .args[3] = node_id, > + .args[4] = version, > + .owner = ARM_SMCCC_OWNER_SIP, > + }; > + > + payload_buf = dma_alloc_coherent(__scm->dev, payload_size, &payload_phys, GFP_KERNEL); > + if (!payload_buf) > + return -ENOMEM; > + > + payload_buf[0] = payload_fn; > + payload_buf[1] = 0; > + payload_buf[2] = payload_reg; > + payload_buf[3] = 1; > + payload_buf[4] = payload_val; > + > + desc.args[0] = payload_phys; > + > + ret = qcom_scm_call(__scm->dev, &desc, NULL); > + > + dma_free_coherent(__scm->dev, payload_size, payload_buf, payload_phys); > + return ret; > +} > +EXPORT_SYMBOL(qcom_scm_lmh_dcvsh); > + > static int qcom_scm_find_dload_address(struct device *dev, u64 *addr) > { > struct device_node *tcsr; > diff --git a/drivers/firmware/qcom_scm.h b/drivers/firmware/qcom_scm.h > index 632fe3142462..d92156ceb3ac 100644 > --- a/drivers/firmware/qcom_scm.h > +++ b/drivers/firmware/qcom_scm.h > @@ -114,6 +114,10 @@ extern int scm_legacy_call(struct device *dev, const struct qcom_scm_desc *desc, > #define QCOM_SCM_SVC_HDCP 0x11 > #define QCOM_SCM_HDCP_INVOKE 0x01 > > +#define QCOM_SCM_SVC_LMH 0x13 > +#define QCOM_SCM_LMH_LIMIT_PROFILE_CHANGE 0x01 > +#define QCOM_SCM_LMH_LIMIT_DCVSH 0x10 > + > #define QCOM_SCM_SVC_SMMU_PROGRAM 0x15 > #define QCOM_SCM_SMMU_CONFIG_ERRATA1 0x03 > #define QCOM_SCM_SMMU_CONFIG_ERRATA1_CLIENT_ALL 0x02 > diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h > index 0165824c5128..c0475d1c9885 100644 > --- a/include/linux/qcom_scm.h > +++ b/include/linux/qcom_scm.h > @@ -109,6 +109,12 @@ extern int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, > u32 *resp); > > extern int qcom_scm_qsmmu500_wait_safe_toggle(bool en); > + > +extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > + u64 limit_node, u32 node_id, u64 version); > +extern int qcom_scm_lmh_profile_change(u32 profile_id); > +extern bool qcom_scm_lmh_dcvsh_available(void); > + > #else > > #include <linux/errno.h> > @@ -170,5 +176,13 @@ static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt, > > static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en) > { return -ENODEV; } > + > +static inline int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val, > + u64 limit_node, u32 node_id, u64 version) > + { return -ENODEV; } > + > +static inline int qcom_scm_lmh_profile_change(u32 profile_id) { return -ENODEV; } > + > +static inline bool qcom_scm_lmh_dcvsh_available(void) { return -ENODEV; } > #endif > #endif > -- > 2.25.1 >
+Lukasz, On 09-07-21, 11:37, Thara Gopinath wrote: > On 7/9/21 2:46 AM, Viresh Kumar wrote: > > On 08-07-21, 08:06, Thara Gopinath wrote: > > > static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > > > { > > > struct platform_device *pdev = cpufreq_get_driver_data(); > > > @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) > > > dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); > > > } > > > + ret = qcom_cpufreq_hw_lmh_init(policy, index); > > > > You missed unregistering EM here (which is also missing from exit, > > which you need to fix first in a separate patch). > > Hi! > > So how exactly do you do this? I checked other users of the api and I do not > see any free. Lukasz, I don't see the cpufreq drivers ever calling dev_pm_opp_of_unregister_em(), and even if they called, it would translate to em_dev_unregister_perf_domain(), which has this: if (_is_cpu_device(dev)) return; I am not sure what's going on here, can you help ?
On 09-07-21, 11:37, Thara Gopinath wrote: > On 7/9/21 2:46 AM, Viresh Kumar wrote: > > > @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) > > > dev_pm_opp_remove_all_dynamic(cpu_dev); > > > dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); > > > + if (data->lmh_dcvs_irq > 0) { > > > + devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data); > > > > Why using devm variants here and while requesting the irq ? Missed this one ? > > > > > + cancel_delayed_work_sync(&data->lmh_dcvs_poll_work); > > > + } > > > > Please move this to qcom_cpufreq_hw_lmh_exit() or something. > > Ok. > > > > > Now with sequence of disabling interrupt, etc, I see a potential > > problem. > > > > CPU0 CPU1 > > > > qcom_cpufreq_hw_cpu_exit() > > -> devm_free_irq(); > > qcom_lmh_dcvs_poll() > > -> qcom_lmh_dcvs_notify() > > -> enable_irq() > > > > -> cancel_delayed_work_sync(); > > > > > > What will happen if enable_irq() gets called after freeing the irq ? > > Not sure, but it looks like you will hit this then from manage.c: > > > > WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before > > setup/request_irq: irq %u\n", irq)) > > > > ? > > > > You got a chicken n egg problem :) > > Yes indeed! But also it is a very rare chicken and egg problem. > The scenario here is that the cpus are busy and running load causing a > thermal overrun and lmh is engaged. At the same time for this issue to be > hit the cpu is trying to exit/disable cpufreq. Yes, it is a very specific case but it needs to be resolved anyway. You don't want to get this ever :) > Calling > cancel_delayed_work_sync first could solve this issue, right ? > cancel_delayed_work_sync guarantees the work not to be pending even if > it requeues itself on return. So once the delayed work is cancelled, the > interrupts can be safely disabled. Thoughts ? I don't think even that would provide such guarantees to you here, as there is a chance the work gets queued again because of an interrupt that triggers right after you cancel the work. The basic way of solving such issues is that once you cancel something, you need to guarantee that it doesn't get triggered again, no matter what. The problem here I see is with your design itself, both delayed work and irq can enable each other, so no matter which one you disable first, won't be sufficient. You need to fix that design somehow. -- viresh
Hi Bjorn, Thanks for the review On 7/10/21 12:15 AM, Bjorn Andersson wrote: > On Thu 08 Jul 07:06 CDT 2021, Thara Gopinath wrote: > >> Driver enabling various pieces of Limits Management Hardware(LMh) for cpu >> cluster0 and cpu cluster1 namely kick starting monitoring of temperature, >> current, battery current violations, enabling reliability algorithm and >> setting up various temperature limits. >> >> The following has been explained in the cover letter. I am including this >> here so that this remains in the commit message as well. >> >> LMh is a hardware infrastructure on some Qualcomm SoCs that can enforce >> temperature and current limits as programmed by software for certain IPs >> like CPU. On many newer LMh is configured by firmware/TZ and no programming >> is needed from the kernel side. But on certain SoCs like sdm845 the >> firmware does not do a complete programming of the h/w. On such soc's >> kernel software has to explicitly set up the temperature limits and turn on >> various monitoring and enforcing algorithms on the hardware. >> >> Signed-off-by: Thara Gopinath <thara.gopinath@linaro.org> >> --- >> >> v2->v3: >> - Rearranged enabling of various LMh subfunction and removed returning >> on error in enabling any one subfunction as the different pieces can >> operate and thus be enabled independently. >> - Other minor cosmetic fixes. >> >> v1->v2: >> - Cosmetic and spelling fixes from review comments from Randy Dunlap >> - Added irq_disable to lmh_irq_ops and removed disabling of irq from >> lmh_handle_irq. Now cpufreq explicitly disables irq prior to >> handling it as per Bjorn's suggestion. >> - Rebased to new version of qcom_scm_lmh_dcvsh as changed in patch 1. >> - Removed generic dt compatible string and introduced platform specific one >> as per Bjorn's suggestion. >> - Take arm, low and high temp thresholds for LMh from dt properties instead of >> #defines in the driver as per Daniel's suggestion. >> - Other minor fixes. >> drivers/thermal/qcom/Kconfig | 10 ++ >> drivers/thermal/qcom/Makefile | 1 + >> drivers/thermal/qcom/lmh.c | 239 ++++++++++++++++++++++++++++++++++ >> 3 files changed, 250 insertions(+) >> create mode 100644 drivers/thermal/qcom/lmh.c >> >> diff --git a/drivers/thermal/qcom/Kconfig b/drivers/thermal/qcom/Kconfig >> index 8d5ac2df26dc..7d942f71e532 100644 >> --- a/drivers/thermal/qcom/Kconfig >> +++ b/drivers/thermal/qcom/Kconfig >> @@ -31,3 +31,13 @@ config QCOM_SPMI_TEMP_ALARM >> trip points. The temperature reported by the thermal sensor reflects the >> real time die temperature if an ADC is present or an estimate of the >> temperature based upon the over temperature stage value. >> + >> +config QCOM_LMH >> + tristate "Qualcomm Limits Management Hardware" >> + depends on ARCH_QCOM >> + help >> + This enables initialization of Qualcomm limits management >> + hardware(LMh). LMh allows for hardware-enforced mitigation for cpus based on >> + input from temperature and current sensors. On many newer Qualcomm SoCs >> + LMh is configured in the firmware and this feature need not be enabled. >> + However, on certain SoCs like sdm845 LMh has to be configured from kernel. >> diff --git a/drivers/thermal/qcom/Makefile b/drivers/thermal/qcom/Makefile >> index 252ea7d9da0b..0fa2512042e7 100644 >> --- a/drivers/thermal/qcom/Makefile >> +++ b/drivers/thermal/qcom/Makefile >> @@ -5,3 +5,4 @@ qcom_tsens-y += tsens.o tsens-v2.o tsens-v1.o tsens-v0_1.o \ >> tsens-8960.o >> obj-$(CONFIG_QCOM_SPMI_ADC_TM5) += qcom-spmi-adc-tm5.o >> obj-$(CONFIG_QCOM_SPMI_TEMP_ALARM) += qcom-spmi-temp-alarm.o >> +obj-$(CONFIG_QCOM_LMH) += lmh.o >> diff --git a/drivers/thermal/qcom/lmh.c b/drivers/thermal/qcom/lmh.c >> new file mode 100644 >> index 000000000000..a7b1eb308642 >> --- /dev/null >> +++ b/drivers/thermal/qcom/lmh.c >> @@ -0,0 +1,239 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> + >> +/* >> + * Copyright (C) 2021, Linaro Limited. All rights reserved. >> + */ >> +#include <linux/module.h> >> +#include <linux/interrupt.h> >> +#include <linux/irqdomain.h> >> +#include <linux/err.h> >> +#include <linux/platform_device.h> >> +#include <linux/of_platform.h> >> +#include <linux/slab.h> >> +#include <linux/qcom_scm.h> >> + >> +#define LMH_NODE_DCVS 0x44435653 >> +#define LMH_CLUSTER0_NODE_ID 0x6370302D >> +#define LMH_CLUSTER1_NODE_ID 0x6370312D >> + >> +#define LMH_SUB_FN_THERMAL 0x54484D4C >> +#define LMH_SUB_FN_CRNT 0x43524E54 >> +#define LMH_SUB_FN_REL 0x52454C00 >> +#define LMH_SUB_FN_BCL 0x42434C00 >> + >> +#define LMH_ALGO_MODE_ENABLE 0x454E424C >> +#define LMH_TH_HI_THRESHOLD 0x48494748 >> +#define LMH_TH_LOW_THRESHOLD 0x4C4F5700 >> +#define LMH_TH_ARM_THRESHOLD 0x41524D00 >> + >> +#define LMH_REG_DCVS_INTR_CLR 0x8 >> + >> +struct lmh_hw_data { >> + void __iomem *base; >> + struct irq_domain *domain; >> + int irq; >> + u32 cpu_id; > > cpu_id seems to only be used in lmh_probe(), how about making it a local > variable? yes, it makes sense. I will make it local. > >> +}; >> + >> +static irqreturn_t lmh_handle_irq(int hw_irq, void *data) >> +{ >> + struct lmh_hw_data *lmh_data = data; >> + int irq = irq_find_mapping(lmh_data->domain, 0); >> + >> + /* Call the cpufreq driver to handle the interrupt */ >> + if (irq) >> + generic_handle_irq(irq); >> + >> + return 0; >> +} >> + >> +static void lmh_enable_interrupt(struct irq_data *d) >> +{ >> + struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d); >> + >> + /* Clear the existing interrupt */ >> + writel(0xff, lmh_data->base + LMH_REG_DCVS_INTR_CLR); >> + enable_irq(lmh_data->irq); >> +} >> + >> +static void lmh_disable_interrupt(struct irq_data *d) >> +{ >> + struct lmh_hw_data *lmh_data = irq_data_get_irq_chip_data(d); >> + >> + disable_irq_nosync(lmh_data->irq); >> +} >> + >> +static struct irq_chip lmh_irq_chip = { >> + .name = "lmh", >> + .irq_enable = lmh_enable_interrupt, >> + .irq_disable = lmh_disable_interrupt >> +}; >> + >> +static int lmh_irq_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw) >> +{ >> + struct lmh_hw_data *lmh_data = d->host_data; >> + >> + irq_set_chip_and_handler(irq, &lmh_irq_chip, handle_simple_irq); >> + irq_set_chip_data(irq, lmh_data); >> + >> + return 0; >> +} >> + >> +static const struct irq_domain_ops lmh_irq_ops = { >> + .map = lmh_irq_map, >> + .xlate = irq_domain_xlate_onecell, >> +}; >> + >> +static int lmh_probe(struct platform_device *pdev) >> +{ >> + struct device *dev; >> + struct device_node *np; >> + struct lmh_hw_data *lmh_data; >> + u32 node_id; >> + int temp_low, temp_high, temp_arm, ret; >> + >> + dev = &pdev->dev; >> + np = dev->of_node; > > How about initialize these as you declare you variables? ok. > >> + if (!np) > > There's no reasonable way to probe this driver with !dev->of_node, so > you can skip this check. ok. > >> + return -EINVAL; >> + >> + lmh_data = devm_kzalloc(dev, sizeof(*lmh_data), GFP_KERNEL); >> + if (!lmh_data) >> + return -ENOMEM; >> + >> + lmh_data->base = devm_platform_ioremap_resource(pdev, 0); >> + if (IS_ERR(lmh_data->base)) >> + return PTR_ERR(lmh_data->base); >> + >> + ret = of_property_read_u32(np, "qcom,lmh-cpu-id", &lmh_data->cpu_id); >> + if (ret) { >> + dev_err(dev, "missing qcom,lmh-cpu-id property\n"); >> + return ret; >> + } >> + >> + ret = of_property_read_u32(np, "qcom,lmh-temperature-high", &temp_high); >> + if (ret) { >> + dev_err(dev, "missing qcom,lmh-temperature-high property\n"); >> + return ret; >> + } >> + >> + ret = of_property_read_u32(np, "qcom,lmh-temperature-low", &temp_low); >> + if (ret) { >> + dev_err(dev, "missing qcom,lmh-temperature-low property\n"); >> + return ret; >> + } >> + >> + ret = of_property_read_u32(np, "qcom,lmh-temperature-arm", &temp_arm); >> + if (ret) { >> + dev_err(dev, "missing qcom,lmh-temperature-arm property\n"); >> + return ret; >> + } >> + >> + /* >> + * Only sdm845 has lmh hardware currently enabled from hlos. If this is needed >> + * for other platforms, revisit this to check if the <cpu-id, node-id> should be part >> + * of a dt match table. >> + */ >> + if (lmh_data->cpu_id == 0) { >> + node_id = LMH_CLUSTER0_NODE_ID; >> + } else if (lmh_data->cpu_id == 4) { >> + node_id = LMH_CLUSTER1_NODE_ID; >> + } else { >> + dev_err(dev, "Wrong CPU id associated with LMh node\n"); >> + return -EINVAL; >> + } >> + >> + platform_set_drvdata(pdev, lmh_data); > > I don't see any get_drvdat(), so you can probably skip this? Yes. I will remove it. I think it is stray remaining from one of the earlier revisions. > >> + >> + if (!qcom_scm_lmh_dcvsh_available()) >> + return -EINVAL; >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_CRNT, LMH_ALGO_MODE_ENABLE, 1, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) >> + dev_err(dev, "Error %d enabling current subfunction\n", ret); >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_REL, LMH_ALGO_MODE_ENABLE, 1, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) >> + dev_err(dev, "Error %d enabling reliability subfunction\n", ret); >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_BCL, LMH_ALGO_MODE_ENABLE, 1, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) >> + dev_err(dev, "Error %d enabling BCL subfunction\n", ret); >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_ALGO_MODE_ENABLE, 1, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) { >> + dev_err(dev, "Error %d enabling thermal subfunction\n", ret); >> + return ret; >> + } >> + >> + ret = qcom_scm_lmh_profile_change(0x1); >> + if (ret) { >> + dev_err(dev, "Error %d changing profile\n", ret); >> + return ret; >> + } >> + >> + /* Set default thermal trips */ >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_ARM_THRESHOLD, temp_arm, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) { >> + dev_err(dev, "Error setting thermal ARM threshold%d\n", ret); >> + return ret; >> + } >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_HI_THRESHOLD, temp_high, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) { >> + dev_err(dev, "Error setting thermal HI threshold%d\n", ret); >> + return ret; >> + } >> + >> + ret = qcom_scm_lmh_dcvsh(LMH_SUB_FN_THERMAL, LMH_TH_LOW_THRESHOLD, temp_low, >> + LMH_NODE_DCVS, node_id, 0); >> + if (ret) { >> + dev_err(dev, "Error setting thermal ARM threshold%d\n", ret); >> + return ret; >> + } >> + >> + lmh_data->irq = platform_get_irq(pdev, 0); >> + lmh_data->domain = irq_domain_add_linear(np, 1, &lmh_irq_ops, lmh_data); >> + if (!lmh_data->domain) { >> + dev_err(dev, "Error adding irq_domain\n"); >> + return -EINVAL; >> + } >> + > > As written now, you might get interrupts before you get to disable_irq() > below. Instead of the disable_irq() you can add this before request_irq: > > irq_set_status_flags(lmh_dat->irq, IRQ_NOAUTOEN); > >> + ret = devm_request_irq(dev, lmh_data->irq, lmh_handle_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT | IRQF_NO_SUSPEND, > > Skip IRQF_TRIGGER_HIGH, as the flags will be merged with the properties > from DT. > >> + "lmh-irq", lmh_data); >> + if (ret) { >> + dev_err(dev, "Error %d registering irq %x\n", ret, lmh_data->irq); >> + irq_domain_remove(lmh_data->domain); >> + return ret; >> + } >> + >> + /* Disable the irq and let cpufreq enable it when ready to handle the interrupt */ >> + disable_irq(lmh_data->irq); >> + >> + return 0; >> +} >> + >> +static const struct of_device_id lmh_table[] = { >> + { .compatible = "qcom,sdm845-lmh", }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(of, lmh_table); >> + >> +static struct platform_driver lmh_driver = { >> + .probe = lmh_probe, > > I think you at least need to irq_domain_remove() during .remove, but > unless we have a clear understanding about how to stop the algorithm > (without causing harmful side effects) it might be better to add > .suppress_bind_attrs = true in .driver... sounds good. Like you said, I am not sure what is the right way to disable the algorithm. So I will add suppress_bind_attrs = true to prevent user space from doing something silly.
On 7/10/21 12:57 AM, Bjorn Andersson wrote: > On Thu 08 Jul 07:06 CDT 2021, 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> >> --- >> >> v2->v3: >> - Cosmetic fixes from review comments on the list. >> - Moved all LMh initializations to qcom_cpufreq_hw_lmh_init. >> - Added freeing of LMh interrupt and cancelling the polling worker to >> qcom_cpufreq_hw_cpu_exit as per Viresh's suggestion. >> - LMh interrupts are now tied to cpu dev and not cpufreq dev. This will be >> useful for further generation of SoCs where the same interrupt signals >> multiple cpu clusters. >> >> v1->v2: >> - Introduced qcom_cpufreq_hw_lmh_init to consolidate LMh related initializations >> as per Viresh's review comment. >> - Moved the piece of code restarting polling/re-enabling LMh interrupt to >> qcom_lmh_dcvs_notify therby simplifying isr and timer callback as per Viresh's >> suggestion. >> - Droped cpus from qcom_cpufreq_data and instead using cpus from cpufreq_policy in >> qcom_lmh_dcvs_notify as per Viresh's review comment. >> - Dropped dt property qcom,support-lmh as per Bjorn's suggestion. >> - Other minor/cosmetic fixes >> >> drivers/cpufreq/qcom-cpufreq-hw.c | 118 ++++++++++++++++++++++++++++++ >> 1 file changed, 118 insertions(+) >> >> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c >> index f86859bf76f1..bb5fc700d913 100644 >> --- a/drivers/cpufreq/qcom-cpufreq-hw.c >> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c >> @@ -7,6 +7,7 @@ >> #include <linux/cpufreq.h> >> #include <linux/init.h> >> #include <linux/interconnect.h> >> +#include <linux/interrupt.h> >> #include <linux/kernel.h> >> #include <linux/module.h> >> #include <linux/of_address.h> >> @@ -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,10 @@ struct qcom_cpufreq_soc_data { >> struct qcom_cpufreq_data { >> void __iomem *base; >> struct resource *res; >> + struct delayed_work lmh_dcvs_poll_work; > > How about dropping "lmh" from this variable name? > > Perhaps "throttle_work" or something like that? > >> const struct qcom_cpufreq_soc_data *soc_data; >> + struct cpufreq_policy *policy; >> + int lmh_dcvs_irq; > > throttle_irq ? sounds good! > >> }; >> >> static unsigned long cpu_hw_rate, xo_rate; >> @@ -251,10 +258,84 @@ 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 = data->policy; >> + struct dev_pm_opp *opp; >> + struct device *dev; >> + unsigned long max_capacity, capacity, freq_hz, throttled_freq; >> + unsigned int val, freq; >> + >> + /* >> + * Get the h/w throttled frequency, normalize it using the >> + * registered opp table and use it to calculate thermal pressure. >> + */ >> + val = readl_relaxed(data->base + data->soc_data->reg_current_vote); > > I would find it cleaner to move the readl() into the helper function, as > you don't care about the register value, only the resulting frequency. Ok.. > >> + freq = qcom_lmh_vote_to_freq(val); >> + freq_hz = freq * HZ_PER_KHZ; >> + >> + dev = get_cpu_device(cpumask_first(policy->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); >> + >> + throttled_freq = freq_hz / HZ_PER_KHZ; >> + >> + /* Update thermal pressure */ >> + >> + max_capacity = arch_scale_cpu_capacity(cpumask_first(policy->cpus)); >> + capacity = throttled_freq * max_capacity; >> + capacity /= policy->cpuinfo.max_freq; > > Perhaps, to avoid overflows if this is ever used on a 32-bit platform > use: > > mult_frac(max_capacity, throttled_freq, policy->cpuinfo.max_freq) yep. sounds good. > >> + >> + /* Don't pass boost capacity to scheduler */ >> + if (capacity > max_capacity) >> + capacity = max_capacity; >> + >> + arch_set_thermal_pressure(policy->cpus, max_capacity - capacity); >> + >> + /* >> + * If h/w throttled frequency is higher than what cpufreq has requested for, stop >> + * polling and switch back to interrupt mechanism >> + */ >> + >> + if (throttled_freq >= qcom_cpufreq_hw_get(cpumask_first(policy->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 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); >> +} >> + >> +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); >> + >> + 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, >> }; >> @@ -274,6 +355,35 @@ static const struct of_device_id qcom_cpufreq_hw_match[] = { >> }; >> MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match); >> >> +static int qcom_cpufreq_hw_lmh_init(struct cpufreq_policy *policy, int index) >> +{ >> + struct qcom_cpufreq_data *data = policy->driver_data; >> + struct platform_device *pdev = cpufreq_get_driver_data(); >> + struct device *cpu_dev = get_cpu_device(policy->cpu); >> + char irq_name[15]; >> + int ret; >> + >> + /* >> + * Look for LMh interrupt. If no interrupt line is specified / >> + * if there is an error, allow cpufreq to be enabled as usual. >> + */ >> + data->lmh_dcvs_irq = platform_get_irq(pdev, index); >> + if (data->lmh_dcvs_irq <= 0) >> + return data->lmh_dcvs_irq == -EPROBE_DEFER ? -EPROBE_DEFER : 0; >> + >> + snprintf(irq_name, sizeof(irq_name), "dcvsh-irq-%u", policy->cpu); >> + ret = devm_request_irq(cpu_dev, data->lmh_dcvs_irq, qcom_lmh_dcvs_handle_irq, >> + 0, irq_name, data); >> + if (ret) { >> + dev_err(&pdev->dev, "Error %d registering irq %x\n", ret, data->lmh_dcvs_irq); > > The irq number here won't have any meaning, and %x wouldn't be suitable. > > How about ..."Error registering %s: %d\n", irq_name, ret); ? ok. > >> + return 0; > > This sounds like a problem, wouldn't it be suitable to treat it as a > problem? I thought a lot about this. My point is even if LMh does not get enabled due to some reason, cpufreq should be enabled. If I return an error back from here, cpufreq will be disabled. > >> + } >> + data->policy = policy; > > Afaict, no one is going to access data->policy unless devm_request_irq() > succeeds and if it does and the interrupt fires immediately it would be > too late to set it here. So better move it earlier. > >> + INIT_DEFERRABLE_WORK(&data->lmh_dcvs_poll_work, qcom_lmh_dcvs_poll); > > What if the interrupt fires before you initialize the work? Better move > this higher up. I will move this and the data->policy = policy above before requesting the interrupt. > >> + >> + return 0; >> +} >> + >> static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> { >> struct platform_device *pdev = cpufreq_get_driver_data(); >> @@ -370,6 +480,10 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy) >> dev_warn(cpu_dev, "failed to enable boost: %d\n", ret); >> } >> >> + ret = qcom_cpufreq_hw_lmh_init(policy, index); >> + if (ret) >> + goto error; >> + >> return 0; >> error: >> kfree(data); >> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) >> >> dev_pm_opp_remove_all_dynamic(cpu_dev); >> dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); >> + if (data->lmh_dcvs_irq > 0) { >> + devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data); > > As init/exit are called multiple times you should avoid the devm > variants. Yes. I think Viresh was also mentioning this. I will move to non devm version. > > Regards, > Bjorn > >> + cancel_delayed_work_sync(&data->lmh_dcvs_poll_work); >> + } >> kfree(policy->freq_table); >> kfree(data); >> iounmap(base); >> -- >> 2.25.1 >> -- Warm Regards Thara (She/Her/Hers)
On 7/12/21 12:41 AM, Viresh Kumar wrote: > On 09-07-21, 11:37, Thara Gopinath wrote: >> On 7/9/21 2:46 AM, Viresh Kumar wrote: >>>> @@ -389,6 +503,10 @@ static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy) >>>> dev_pm_opp_remove_all_dynamic(cpu_dev); >>>> dev_pm_opp_of_cpumask_remove_table(policy->related_cpus); >>>> + if (data->lmh_dcvs_irq > 0) { >>>> + devm_free_irq(cpu_dev, data->lmh_dcvs_irq, data); >>> >>> Why using devm variants here and while requesting the irq ? > > Missed this one ? Yep. I just replied to Bjorn's email on this. I will move to non devm version. > >>> >>>> + cancel_delayed_work_sync(&data->lmh_dcvs_poll_work); >>>> + } >>> >>> Please move this to qcom_cpufreq_hw_lmh_exit() or something. >> >> Ok. >> >>> >>> Now with sequence of disabling interrupt, etc, I see a potential >>> problem. >>> >>> CPU0 CPU1 >>> >>> qcom_cpufreq_hw_cpu_exit() >>> -> devm_free_irq(); >>> qcom_lmh_dcvs_poll() >>> -> qcom_lmh_dcvs_notify() >>> -> enable_irq() >>> >>> -> cancel_delayed_work_sync(); >>> >>> >>> What will happen if enable_irq() gets called after freeing the irq ? >>> Not sure, but it looks like you will hit this then from manage.c: >>> >>> WARN(!desc->irq_data.chip, KERN_ERR "enable_irq before >>> setup/request_irq: irq %u\n", irq)) >>> >>> ? >>> >>> You got a chicken n egg problem :) >> >> Yes indeed! But also it is a very rare chicken and egg problem. >> The scenario here is that the cpus are busy and running load causing a >> thermal overrun and lmh is engaged. At the same time for this issue to be >> hit the cpu is trying to exit/disable cpufreq. > > Yes, it is a very specific case but it needs to be resolved anyway. You don't > want to get this ever :) > >> Calling >> cancel_delayed_work_sync first could solve this issue, right ? >> cancel_delayed_work_sync guarantees the work not to be pending even if >> it requeues itself on return. So once the delayed work is cancelled, the >> interrupts can be safely disabled. Thoughts ? > > I don't think even that would provide such guarantees to you here, as there is > a chance the work gets queued again because of an interrupt that triggers right > after you cancel the work. > > The basic way of solving such issues is that once you cancel something, you need > to guarantee that it doesn't get triggered again, no matter what. > > The problem here I see is with your design itself, both delayed work and irq can > enable each other, so no matter which one you disable first, won't be > sufficient. You need to fix that design somehow. So I really need the interrupt to fire and then the timer to kick in and take up the monitoring. I can think of introducing a variable is_disabled which is updated and read under a spinlock. qcom_cpufreq_hw_cpu_exit can hold the spinlock and set is_disabled to true prior to cancelling the work queue or disabling the interrupt. Before re-enabling the interrupt or re-queuing the work in qcom_lmh_dcvs_notify, is_disabled can be read and checked. But does this problem not exist in target_index , fast_switch etc also ? One cpu can be disabling and the other one can be updating the target right? >
On 12-07-21, 21:18, Thara Gopinath wrote: > So I really need the interrupt to fire and then the timer to kick in and > take up the monitoring. I can think of introducing a variable is_disabled > which is updated and read under a spinlock. qcom_cpufreq_hw_cpu_exit can > hold the spinlock and set is_disabled to true prior to cancelling the work > queue or disabling the interrupt. Before re-enabling the interrupt or > re-queuing the work in qcom_lmh_dcvs_notify, is_disabled can be read and > checked. Or you can make the lmh_dcvs_poll_work item a pointer and mark it NULL in exit, with proper locking etc. > But does this problem not exist in target_index , fast_switch etc also ? One > cpu can be disabling and the other one can be updating the target right? The race doesn't happen there as cpufreq_unregister_driver() takes care of stopping everything before removing the policy. To be more precise, governor's ->stop() function is responsible for making sure that frequency won't be updated any further. -- viresh
On 7/12/21 11:18 PM, Viresh Kumar wrote: > On 12-07-21, 21:18, Thara Gopinath wrote: >> So I really need the interrupt to fire and then the timer to kick in and >> take up the monitoring. I can think of introducing a variable is_disabled >> which is updated and read under a spinlock. qcom_cpufreq_hw_cpu_exit can >> hold the spinlock and set is_disabled to true prior to cancelling the work >> queue or disabling the interrupt. Before re-enabling the interrupt or >> re-queuing the work in qcom_lmh_dcvs_notify, is_disabled can be read and >> checked. > > Or you can make the lmh_dcvs_poll_work item a pointer and mark it NULL in exit, > with proper locking etc. Yes it could work. I will spin the next version with either this or introducing a new variable with locking. > >> But does this problem not exist in target_index , fast_switch etc also ? One >> cpu can be disabling and the other one can be updating the target right? > > The race doesn't happen there as cpufreq_unregister_driver() takes care of > stopping everything before removing the policy. To be more precise, governor's > ->stop() function is responsible for making sure that frequency won't be updated > any further. >
On 7/21/21 11:14 PM, Steev Klimaszewski wrote: > Hi Thara! > > On 7/8/21 7:06 AM, Thara Gopinath wrote: >> Limits Management Hardware(LMh) is a hardware infrastructure on some >> Qualcomm SoCs that can enforce temperature and current limits as programmed >> by software for certain IPs like CPU. On many newer SoCs LMh is configured >> by firmware/TZ and no programming is needed from the kernel side. But on >> certain SoCs like sdm845 the firmware does not do a complete programming of >> the h/w block. On such SoCs kernel software has to explicitly set up the >> temperature limits and turn on various monitoring and enforcing algorithms >> on the hardware. >> >> Introduce support for enabling and programming various limit settings and >> monitoring capabilities of Limits Management Hardware(LMh) associated with >> cpu clusters. Also introduce support in cpufreq hardware driver to monitor >> the interrupt associated with cpu frequency throttling so that this >> information can be conveyed to the schdeuler via thermal pressure >> interface. >> >> With this patch series following cpu performance improvement(30-70%) is >> observed on sdm845. The reasoning here is that without LMh being programmed >> properly from the kernel, the default settings were enabling thermal >> mitigation for CPUs at too low a temperature (around 70-75 degree C). This >> in turn meant that many a time CPUs were never actually allowed to hit the >> maximum possible/required frequencies. >> >> UnixBench whets and dhry (./Run whets dhry) >> System Benchmarks Index Score >> >> Without LMh Support With LMh Support >> 1 copy test 1353.7 1773.2 >> >> 8 copy tests 4473.6 7402.3 >> >> Sysbench cpu >> sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run >> >> Without LMh Support With LMh Support >> Events per >> second 355 614 >> >> Avg Latency(ms) 21.84 13.02 >> >> v2->v3: >> - Included patch adding dt binding documentation for LMh nodes. >> - Rebased to v5.13 >> >> Thara Gopinath (6): >> firmware: qcom_scm: Introduce SCM calls to access LMh >> thermal: qcom: Add support for LMh driver >> cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support >> arm64: boot: dts: qcom: sdm45: Add support for LMh node >> arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU >> thermal zones >> dt-bindings: thermal: Add dt binding for QCOM LMh >> >> .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++ >> arch/arm64/boot/dts/qcom/sdm845.dtsi | 162 ++---------- >> drivers/cpufreq/qcom-cpufreq-hw.c | 118 +++++++++ >> drivers/firmware/qcom_scm.c | 58 +++++ >> drivers/firmware/qcom_scm.h | 4 + >> drivers/thermal/qcom/Kconfig | 10 + >> drivers/thermal/qcom/Makefile | 1 + >> drivers/thermal/qcom/lmh.c | 239 ++++++++++++++++++ >> include/linux/qcom_scm.h | 14 + >> 9 files changed, 570 insertions(+), 136 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml >> create mode 100644 drivers/thermal/qcom/lmh.c >> > I've been using these patches on a 5.13 kernel > (https://github.com/steev/linux/tree/linux-5.13.y - while trying to > track down a different issue, while playing a video on youtube, as well > as compressing a 9.2GB file with xz, I got the following Hi Steev, Thanks for testing this. I was unable to reproduce this. I have posted v4 moving the interrupt handling in qcom-cpufreq-hw to threaded interrupt handler and hopefully this should fix the issue. It will be great if you can test and let me know.
On 7/27/21 10:29 AM, Thara Gopinath wrote: > > > On 7/21/21 11:14 PM, Steev Klimaszewski wrote: >> Hi Thara! >> >> On 7/8/21 7:06 AM, Thara Gopinath wrote: >>> Limits Management Hardware(LMh) is a hardware infrastructure on some >>> Qualcomm SoCs that can enforce temperature and current limits as >>> programmed >>> by software for certain IPs like CPU. On many newer SoCs LMh is >>> configured >>> by firmware/TZ and no programming is needed from the kernel side. >>> But on >>> certain SoCs like sdm845 the firmware does not do a complete >>> programming of >>> the h/w block. On such SoCs kernel software has to explicitly set up >>> the >>> temperature limits and turn on various monitoring and enforcing >>> algorithms >>> on the hardware. >>> >>> Introduce support for enabling and programming various limit >>> settings and >>> monitoring capabilities of Limits Management Hardware(LMh) >>> associated with >>> cpu clusters. Also introduce support in cpufreq hardware driver to >>> monitor >>> the interrupt associated with cpu frequency throttling so that this >>> information can be conveyed to the schdeuler via thermal pressure >>> interface. >>> >>> With this patch series following cpu performance improvement(30-70%) is >>> observed on sdm845. The reasoning here is that without LMh being >>> programmed >>> properly from the kernel, the default settings were enabling thermal >>> mitigation for CPUs at too low a temperature (around 70-75 degree >>> C). This >>> in turn meant that many a time CPUs were never actually allowed to >>> hit the >>> maximum possible/required frequencies. >>> >>> UnixBench whets and dhry (./Run whets dhry) >>> System Benchmarks Index Score >>> >>> Without LMh Support With LMh Support >>> 1 copy test 1353.7 1773.2 >>> >>> 8 copy tests 4473.6 7402.3 >>> >>> Sysbench cpu >>> sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run >>> >>> Without LMh Support With LMh Support >>> Events per >>> second 355 614 >>> >>> Avg Latency(ms) 21.84 13.02 >>> >>> v2->v3: >>> - Included patch adding dt binding documentation for LMh nodes. >>> - Rebased to v5.13 >>> >>> Thara Gopinath (6): >>> firmware: qcom_scm: Introduce SCM calls to access LMh >>> thermal: qcom: Add support for LMh driver >>> cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support >>> arm64: boot: dts: qcom: sdm45: Add support for LMh node >>> arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices >>> for CPU >>> thermal zones >>> dt-bindings: thermal: Add dt binding for QCOM LMh >>> >>> .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++ >>> arch/arm64/boot/dts/qcom/sdm845.dtsi | 162 ++---------- >>> drivers/cpufreq/qcom-cpufreq-hw.c | 118 +++++++++ >>> drivers/firmware/qcom_scm.c | 58 +++++ >>> drivers/firmware/qcom_scm.h | 4 + >>> drivers/thermal/qcom/Kconfig | 10 + >>> drivers/thermal/qcom/Makefile | 1 + >>> drivers/thermal/qcom/lmh.c | 239 >>> ++++++++++++++++++ >>> include/linux/qcom_scm.h | 14 + >>> 9 files changed, 570 insertions(+), 136 deletions(-) >>> create mode 100644 >>> Documentation/devicetree/bindings/thermal/qcom-lmh.yaml >>> create mode 100644 drivers/thermal/qcom/lmh.c >>> >> I've been using these patches on a 5.13 kernel >> (https://github.com/steev/linux/tree/linux-5.13.y - while trying to >> track down a different issue, while playing a video on youtube, as well >> as compressing a 9.2GB file with xz, I got the following > > Hi Steev, > > Thanks for testing this. I was unable to reproduce this. I have posted > v4 moving the interrupt handling in qcom-cpufreq-hw to threaded > interrupt handler and hopefully this should fix the issue. It will be > great if you can test and let me know. > Hi Thara, I've been testing v4 for a little bit here, and so far I can't seem to get it to reproduce anymore. I will keep trying but fingers crossed that that did the trick. For setup, I'm using https://github.com/steev/linux/tree/linux-5.13.y with the "distro_defconfig" configuration here on my c630s. I'm also running https://github.com/steev/scheduler as a systemd service. So far I've been able to sleep/suspend without issue while running "make -j$(nproc) deb-pkg" in those kernel sources as well as `xz --memlimit-compress=50 -T 4 imagefile.img" on a 9.2GB file at the same time. One system is running the Budgie desktop on top of Xorg, and the other is running Gnome 3.38 on top of Wayland. -- steev
Limits Management Hardware(LMh) is a hardware infrastructure on some Qualcomm SoCs that can enforce temperature and current limits as programmed by software for certain IPs like CPU. On many newer SoCs LMh is configured by firmware/TZ and no programming is needed from the kernel side. But on certain SoCs like sdm845 the firmware does not do a complete programming of the h/w block. On such SoCs kernel software has to explicitly set up the temperature limits and turn on various monitoring and enforcing algorithms on the hardware. Introduce support for enabling and programming various limit settings and monitoring capabilities of Limits Management Hardware(LMh) associated with cpu clusters. Also introduce support in cpufreq hardware driver to monitor the interrupt associated with cpu frequency throttling so that this information can be conveyed to the schdeuler via thermal pressure interface. With this patch series following cpu performance improvement(30-70%) is observed on sdm845. The reasoning here is that without LMh being programmed properly from the kernel, the default settings were enabling thermal mitigation for CPUs at too low a temperature (around 70-75 degree C). This in turn meant that many a time CPUs were never actually allowed to hit the maximum possible/required frequencies. UnixBench whets and dhry (./Run whets dhry) System Benchmarks Index Score Without LMh Support With LMh Support 1 copy test 1353.7 1773.2 8 copy tests 4473.6 7402.3 Sysbench cpu sysbench cpu --threads=8 --time=60 --cpu-max-prime=100000 run Without LMh Support With LMh Support Events per second 355 614 Avg Latency(ms) 21.84 13.02 v2->v3: - Included patch adding dt binding documentation for LMh nodes. - Rebased to v5.13 Thara Gopinath (6): firmware: qcom_scm: Introduce SCM calls to access LMh thermal: qcom: Add support for LMh driver cpufreq: qcom-cpufreq-hw: Add dcvs interrupt support arm64: boot: dts: qcom: sdm45: Add support for LMh node arm64: boot: dts: qcom: sdm845: Remove cpufreq cooling devices for CPU thermal zones dt-bindings: thermal: Add dt binding for QCOM LMh .../devicetree/bindings/thermal/qcom-lmh.yaml | 100 ++++++++ arch/arm64/boot/dts/qcom/sdm845.dtsi | 162 ++---------- drivers/cpufreq/qcom-cpufreq-hw.c | 118 +++++++++ drivers/firmware/qcom_scm.c | 58 +++++ drivers/firmware/qcom_scm.h | 4 + drivers/thermal/qcom/Kconfig | 10 + drivers/thermal/qcom/Makefile | 1 + drivers/thermal/qcom/lmh.c | 239 ++++++++++++++++++ include/linux/qcom_scm.h | 14 + 9 files changed, 570 insertions(+), 136 deletions(-) create mode 100644 Documentation/devicetree/bindings/thermal/qcom-lmh.yaml create mode 100644 drivers/thermal/qcom/lmh.c