Re: [PATCH v16 3/7] soc: mediatek: SVS: introduce MTK SVS engine
From: Roger Lu <hidden>
Date: 2021-12-24 09:27:30
Also in:
linux-devicetree, linux-mediatek, linux-pm, lkml
Hi AngeloGioacchino, Sorry for the late reply and thanks for all the advices. On Thu, 2021-10-21 at 10:46 +0200, AngeloGioacchino Del Regno wrote:
Il 28/04/21 08:54, Roger Lu ha scritto:quoted
The Smart Voltage Scaling(SVS) engine is a piece of hardware which calculates suitable SVS bank voltages to OPP voltage table. Then, DVFS driver could apply those SVS bank voltages to PMIC/Buck when receiving OPP_EVENT_ADJUST_VOLTAGE. Signed-off-by: Roger Lu <redacted> --- drivers/soc/mediatek/Kconfig | 10 + drivers/soc/mediatek/Makefile | 1 + drivers/soc/mediatek/mtk-svs.c | 1723 ++++++++++++++++++++++++++++++++ 3 files changed, 1734 insertions(+) create mode 100644 drivers/soc/mediatek/mtk-svs.c
[snip]
quoted
+/* svs bank common setting */ +#define SVSB_DET_CLK_EN BIT(31) +#define SVSB_TZONE_HIGH_TEMP_MAX U32_MAX +#define SVSB_RUNCONFIG_DEFAULT 0x80000000 +#define SVSB_DC_SIGNED_BIT 0x8000 +#define SVSB_INTEN_INIT0x 0x00005f01 +#define SVSB_INTEN_MONVOPEN 0x00ff0000 +#define SVSB_EN_OFF 0x0 +#define SVSB_EN_MASK 0x7 +#define SVSB_EN_INIT01 0x1 +#define SVSB_EN_INIT02 0x5 +#define SVSB_EN_MON 0x2 +#define SVSB_INTSTS_MONVOP 0x00ff0000 +#define SVSB_INTSTS_COMPLETE 0x1 +#define SVSB_INTSTS_CLEAN 0x00ffffff + +static DEFINE_SPINLOCK(mtk_svs_lock); + +/*Thanks for using kernel-doc!! However, to be proper, this has to be /** * ..........
Oh okay. Thanks. I will use /** to add multi-line comments. However, I checked kernel doc-guide and it uses indent as below. So, I'll follow it. If I'm doing it wrong, please correct me. Thanks a lot. https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html /** * .........
quoted
+ * enum svsb_phase - svs bank phase enumeration + * @SVSB_PHASE_INIT01: basic init for svs bank + * @SVSB_PHASE_INIT02: svs bank can provide voltages + * @SVSB_PHASE_MON: svs bank can provide voltages with thermal effect + * @SVSB_PHASE_ERROR: svs bank encounters unexpected conditionPlease move @SVSB_PHASE_ERROR before @SVSB_PHASE_INIT01: the order is important here, and has to be the same as the actual enumeration.
Sure, I'll improve it in the next version. Thanks.
quoted
+ * + * Each svs bank has its own independent phase. We enable each svs bank by + * running their phase orderly. However, When svs bank encounters unexpected + * condition, it will fire an irq (PHASE_ERROR) to inform svs software. + * + * svs bank general phase-enabled order: + * SVSB_PHASE_INIT01 -> SVSB_PHASE_INIT02 -> SVSB_PHASE_MON + */ +enum svsb_phase { + SVSB_PHASE_ERROR = 0, + SVSB_PHASE_INIT01, + SVSB_PHASE_INIT02, + SVSB_PHASE_MON, +};
[snip]
quoted
+/*Same here, use /**
Sure, I'll improve it in the next version. Thanks.
quoted
+ * struct svs_platform - svs platform data + * @dev: svs platform device + * @base: svs platform register address base + * @main_clk: main clock for svs bank + * @pbank: phandle of svs bank and needs to be protected by spin_lock + * @banks: phandle of the banks that support + * @efuse_parsing: phandle of efuse parsing function + * @irqflags: irq settings flags + * @rst: svs reset control + * @regs: phandle to the registers map + * @efuse_num: the total number of svs platform efuse + * @tefuse_num: the total number of thermal efuse + * @bank_num: the total number of banks + * @efuse_check: the svs efuse check index + * @efuse: svs platform efuse data received from NVMEM framework + * @tefuse: thermal efuse data received from NVMEM framework + * @name: svs platform name + */ +struct svs_platform { + struct device *dev; + void __iomem *base; + struct clk *main_clk; + struct svs_bank *pbank; + struct svs_bank *banks; + bool (*efuse_parsing)(struct svs_platform *svsp); + unsigned long irqflags; + struct reset_control *rst; + const u32 *regs; + char *name; + size_t efuse_num; + size_t tefuse_num; + u32 bank_num; + u32 efuse_check; + u32 *efuse; + u32 *tefuse; +}; + +/*ditto.
Sure, I'll improve it in the next version. Thanks.
quoted
+ * struct svs_bank - svs bank representation + * @dev: svs bank device + * @opp_dev: device for opp table/buck control + * @pd_dev: power domain device for SoC mtcmos control + * @init_completion: the timeout completion for bank init + * @buck: phandle of the regulator
[snip]
quoted
+static int svs_adjust_pm_opp_volts(struct svs_bank *svsb, bool force_update) +{ + int tzone_temp = 0, ret = -EPERM; + u32 i, svsb_volt, opp_volt, temp_offset = 0; + + mutex_lock(&svsb->lock); + + /* + * If svs bank is suspended, it means signed-off voltages are applied. + * Don't need to update opp voltage anymore. + */ + if (svsb->suspended && !force_update) { + dev_notice(svsb->dev, "bank is suspended\n"); + ret = -EPERM; + goto unlock_mutex; + } + + /* Get thermal effect */ + if (svsb->phase == SVSB_PHASE_MON) { + if (svsb->temp > svsb->temp_upper_bound && + svsb->temp < svsb->temp_lower_bound) { + dev_warn(svsb->dev, "svsb temp = 0x%x?\n", svsb->temp); + ret = -EINVAL; + goto unlock_mutex; + } + + ret = svs_get_bank_zone_temperature(svsb->tzone_name, + &tzone_temp); + if (ret) { + dev_err(svsb->dev, "no %s? (%d), run default volts\n", + svsb->tzone_name, ret); + svsb->phase = SVSB_PHASE_ERROR; + } + + if (tzone_temp >= svsb->tzone_high_temp) + temp_offset += svsb->tzone_high_temp_offset; + else if (tzone_temp <= svsb->tzone_low_temp) + temp_offset += svsb->tzone_low_temp_offset; + } + + /* vmin <= svsb_volt (opp_volt) <= signed-off (default) voltage */ + for (i = 0; i < svsb->opp_count; i++) {What about using switch here? switch (svsb->phase) { case SVSB_PHASE_MON: ...... break; case .......: ......... break; default: dev_err(......); ret = -EINVAL; goto unlock_mutex; }
Okay. I'll use switch here in the next version. Thanks for the advice.
quoted
+ if (svsb->phase == SVSB_PHASE_MON) { + svsb_volt = max(svsb->volts[i] + svsb->volt_offset + + temp_offset, svsb->vmin); + opp_volt = svs_bank_volt_to_opp_volt(svsb_volt, + svsb->volt_step, + svsb->volt_base); + } else if (svsb->phase == SVSB_PHASE_INIT02) { + svsb_volt = max(svsb->volts[i] + svsb->volt_offset, + svsb->vmin); + opp_volt = svs_bank_volt_to_opp_volt(svsb_volt, + svsb->volt_step, + svsb->volt_base); + } else if (svsb->phase == SVSB_PHASE_ERROR) { + opp_volt = svsb->opp_volts[i]; + } else { + dev_err(svsb->dev, "unknown phase: %u?\n", svsb->phase); + ret = -EINVAL; + goto unlock_mutex; + } + + opp_volt = min(opp_volt, svsb->opp_volts[i]); + ret = dev_pm_opp_adjust_voltage(svsb->opp_dev, + svsb->opp_freqs[i], + opp_volt, opp_volt, + svsb->opp_volts[i]); + if (ret) { + dev_err(svsb->dev, "set voltage fail: %d\n", ret); + goto unlock_mutex; + } + } + +unlock_mutex: + mutex_unlock(&svsb->lock); + + return ret; +}
[snip]
quoted
+static void svs_set_bank_phase(struct svs_platform *svsp, + enum svsb_phase target_phase) +{ + struct svs_bank *svsb = svsp->pbank; + u32 des_char, temp_char, det_char, limit_vals; + u32 init2vals, ts_calcs, val, filter, i; + + svs_switch_bank(svsp); + + des_char = (svsb->bdes << 8) | svsb->mdes; + svs_writel(svsp, des_char, DESCHAR); + + temp_char = (svsb->vco << 16) | (svsb->mtdes << 8) | svsb->dvt_fixed; + svs_writel(svsp, temp_char, TEMPCHAR); + + det_char = (svsb->dcbdet << 8) | svsb->dcmdet; + svs_writel(svsp, det_char, DETCHAR); + + svs_writel(svsp, svsb->dc_config, DCCONFIG); + svs_writel(svsp, svsb->age_config, AGECONFIG); + + if (!svsb->agem) { + svs_writel(svsp, SVSB_RUNCONFIG_DEFAULT, RUNCONFIG); + } else { + val = 0x0;val = 0;quoted
+ + for (i = 0; i < 24; i += 2) { + filter = 0x3 << i; + + if (!(svsb->age_config & filter)) + val |= (0x1 << i);val |= BIT(i);
I'll remove these if / else for "agem" in this patch as Matthias recommended in v20 review. Thanks for the advice.
quoted
+ else + val |= (svsb->age_config & filter); + } + svs_writel(svsp, val, RUNCONFIG); + } + + svsb->set_freqs_pct(svsp); + + limit_vals = (svsb->vmax << 24) | (svsb->vmin << 16) | + (svsb->dthi << 8) | svsb->dtlo; + svs_writel(svsp, limit_vals, LIMITVALS); + svs_writel(svsp, svsb->vboot, VBOOT); + svs_writel(svsp, svsb->det_window, DETWINDOW); + svs_writel(svsp, svsb->det_max, CONFIG); + + if (svsb->chk_shift) + svs_writel(svsp, svsb->chk_shift, CHKSHIFT); + + if (svsb->ctl0) + svs_writel(svsp, svsb->ctl0, CTL0); + + svs_writel(svsp, SVSB_INTSTS_CLEAN, INTSTS); + + switch (target_phase) { + case SVSB_PHASE_INIT01: + svs_writel(svsp, SVSB_INTEN_INIT0x, INTEN); + svs_writel(svsp, SVSB_EN_INIT01, SVSEN); + break; + case SVSB_PHASE_INIT02: + svs_writel(svsp, SVSB_INTEN_INIT0x, INTEN); + init2vals = (svsb->age_voffset_in << 16) | svsb->dc_voffset_in; + svs_writel(svsp, init2vals, INIT2VALS); + svs_writel(svsp, SVSB_EN_INIT02, SVSEN); + break; + case SVSB_PHASE_MON: + ts_calcs = (svsb->bts << 12) | svsb->mts; + svs_writel(svsp, ts_calcs, TSCALCS); + svs_writel(svsp, SVSB_INTEN_MONVOPEN, INTEN); + svs_writel(svsp, SVSB_EN_MON, SVSEN); + break; + default: + WARN_ON(1);I agree about printing a big warning in kmsg here, but you can do that in a slightly more descriptive way: WARN(1, "Requested unknown target phase %u", target_phase);
Okay. I'll add description in these kinds of warning kmsg. Thanks for the advice.
quoted
+ break; + } +}
[snip]
quoted
+static int svs_init02(struct svs_platform *svsp) +{ + struct svs_bank *svsb; + unsigned long flags, time_left; + u32 idx; + + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + if (!(svsb->mode_support & SVSB_MODE_INIT02)) + continue; + + reinit_completion(&svsb->init_completion); + spin_lock_irqsave(&mtk_svs_lock, flags); + svsp->pbank = svsb; + svs_set_bank_phase(svsp, SVSB_PHASE_INIT02); + spin_unlock_irqrestore(&mtk_svs_lock, flags); + + time_left = + wait_for_completion_timeout(&svsb->init_completion, + msecs_to_jiffies(5000));There's no need to break the line... that's going to be fine: time_left = wait_for_completion_timeout(&svsb->init_completion, msecs_to_jiffies(5000));
Okay. I'll keep it one line. Thanks.
quoted
+ if (!time_left) { + dev_err(svsb->dev, "init02 completion timeout\n"); + return -EBUSY; + } + } + + return 0; +} + +static int svs_init01(struct svs_platform *svsp) +{ + struct svs_bank *svsb; + struct pm_qos_request *qos_request; + unsigned long flags, time_left; + bool search_done; + int ret = 0; + u32 opp_freqs, opp_vboot, buck_volt, idx, i; + + qos_request = kzalloc(sizeof(*qos_request), GFP_KERNEL); + if (!qos_request) + return -ENOMEM; + + /* Let CPUs leave idle-off state for initializing svs_init01. */ + cpu_latency_qos_add_request(qos_request, 0); + + /* + * Sometimes two svs banks use the same buck. + * Therefore, we set each svs bank to vboot voltage first. + */ + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + if (!(svsb->mode_support & SVSB_MODE_INIT01)) + continue; + + search_done = false; + + if (svsb->pd_req) { + ret = regulator_enable(svsb->buck); + if (ret) { + dev_err(svsb->dev, "%s enable fail: %d\n", + svsb->buck_name, ret); + goto init01_finish; + } + + if (!pm_runtime_enabled(svsb->pd_dev)) { + pm_runtime_enable(svsb->pd_dev); + svsb->enable_pm_runtime_ever = true; + } + + ret = pm_runtime_get_sync(svsb->pd_dev); + if (ret < 0) { + dev_err(svsb->dev, "mtcmos on fail: %d\n", ret); + goto init01_finish; + } + } + + if (regulator_set_mode(svsb->buck, REGULATOR_MODE_FAST)) + dev_notice(svsb->dev, "set fast mode fail\n"); + + /* + * Find the fastest freq that can be run at vboot and + * fix to that freq until svs_init01 is done. + */ + opp_vboot = svs_bank_volt_to_opp_volt(svsb->vboot, + svsb->volt_step, + svsb->volt_base); + + for (i = 0; i < svsb->opp_count; i++) { + opp_freqs = svsb->opp_freqs[i]; + if (!search_done && svsb->opp_volts[i] <= opp_vboot) { + ret = dev_pm_opp_adjust_voltage(svsb->opp_dev, + opp_freqs, + opp_vboot, + opp_vboot, + opp_vboot); + if (ret) { + dev_err(svsb->dev, + "set voltage fail: %d\n", ret); + goto init01_finish; + } + + search_done = true; + } else { + dev_pm_opp_disable(svsb->opp_dev, + svsb->opp_freqs[i]); + } + } + } + + /* svs bank init01 begins */ + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + if (!(svsb->mode_support & SVSB_MODE_INIT01)) + continue; + + opp_vboot = svs_bank_volt_to_opp_volt(svsb->vboot, + svsb->volt_step, + svsb->volt_base); + + buck_volt = regulator_get_voltage(svsb->buck); + if (buck_volt != opp_vboot) { + dev_err(svsb->dev, + "buck voltage: %u, expected vboot: %u\n", + buck_volt, opp_vboot); + ret = -EPERM; + goto init01_finish; + } + + spin_lock_irqsave(&mtk_svs_lock, flags); + svsp->pbank = svsb; + svs_set_bank_phase(svsp, SVSB_PHASE_INIT01); + spin_unlock_irqrestore(&mtk_svs_lock, flags); + + time_left = + wait_for_completion_timeout(&svsb->init_completion,81 columns is ok to have.
Okay. Thanks. I'll keep it one line.
quoted
+ msecs_to_jiffies(5000)); + if (!time_left) { + dev_err(svsb->dev, "init01 completion timeout\n"); + ret = -EBUSY; + goto init01_finish; + } + } + +init01_finish: + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + if (!(svsb->mode_support & SVSB_MODE_INIT01)) + continue; + + for (i = 0; i < svsb->opp_count; i++) + dev_pm_opp_enable(svsb->opp_dev, svsb->opp_freqs[i]); + + if (regulator_set_mode(svsb->buck, REGULATOR_MODE_NORMAL)) + dev_notice(svsb->dev, "fail to set normal mode\n"); + + if (svsb->pd_req) { + if (pm_runtime_put_sync(svsb->pd_dev)) + dev_err(svsb->dev, "mtcmos off fail\n"); + + if (svsb->enable_pm_runtime_ever) { + pm_runtime_disable(svsb->pd_dev); + svsb->enable_pm_runtime_ever = false; + } + + if (regulator_disable(svsb->buck)) + dev_err(svsb->dev, "%s disable fail: %d\n", + svsb->buck_name, ret); + } + } + + cpu_latency_qos_remove_request(qos_request); + kfree(qos_request); + + return ret; +}
[snip]
quoted
+static bool svs_mt8183_efuse_parsing(struct svs_platform *svsp) +{ + struct thermal_parameter tp; + struct svs_bank *svsb; + bool mon_mode_support = true; + int format[6], x_roomt[6], tb_roomt = 0; + struct nvmem_cell *cell; + u32 idx, i, ft_pgm, mts, temp0, temp1, temp2; + + for (i = 0; i < svsp->efuse_num; i++) + if (svsp->efuse[i]) + dev_info(svsp->dev, "M_HW_RES%d: 0x%08x\n", + i, svsp->efuse[i]); + + /* Svs efuse parsing */ + ft_pgm = (svsp->efuse[0] >> 4) & GENMASK(3, 0); + + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + if (ft_pgm <= 1) + svsb->volt_flags |= SVSB_INIT01_VOLT_IGNORE; + + switch (svsb->sw_id) { + case SVSB_CPU_LITTLE: + svsb->bdes = svsp->efuse[16] & GENMASK(7, 0); + svsb->mdes = (svsp->efuse[16] >> 8) & GENMASK(7, 0); + svsb->dcbdet = (svsp->efuse[16] >> 16) & GENMASK(7, 0); + svsb->dcmdet = (svsp->efuse[16] >> 24) & GENMASK(7, 0); + svsb->mtdes = (svsp->efuse[17] >> 16) & GENMASK(7, 0); + + if (ft_pgm <= 3) + svsb->volt_offset += 10; + else + svsb->volt_offset += 2; + break; + case SVSB_CPU_BIG: + svsb->bdes = svsp->efuse[18] & GENMASK(7, 0); + svsb->mdes = (svsp->efuse[18] >> 8) & GENMASK(7, 0); + svsb->dcbdet = (svsp->efuse[18] >> 16) & GENMASK(7, 0); + svsb->dcmdet = (svsp->efuse[18] >> 24) & GENMASK(7, 0); + svsb->mtdes = svsp->efuse[17] & GENMASK(7, 0); + + if (ft_pgm <= 3) + svsb->volt_offset += 15; + else + svsb->volt_offset += 12; + break; + case SVSB_CCI: + svsb->bdes = svsp->efuse[4] & GENMASK(7, 0); + svsb->mdes = (svsp->efuse[4] >> 8) & GENMASK(7, 0); + svsb->dcbdet = (svsp->efuse[4] >> 16) & GENMASK(7, 0); + svsb->dcmdet = (svsp->efuse[4] >> 24) & GENMASK(7, 0); + svsb->mtdes = (svsp->efuse[5] >> 16) & GENMASK(7, 0); + + if (ft_pgm <= 3) + svsb->volt_offset += 10; + else + svsb->volt_offset += 2; + break; + case SVSB_GPU: + svsb->bdes = svsp->efuse[6] & GENMASK(7, 0); + svsb->mdes = (svsp->efuse[6] >> 8) & GENMASK(7, 0); + svsb->dcbdet = (svsp->efuse[6] >> 16) & GENMASK(7, 0); + svsb->dcmdet = (svsp->efuse[6] >> 24) & GENMASK(7, 0); + svsb->mtdes = svsp->efuse[5] & GENMASK(7, 0); + + if (ft_pgm >= 2) { + svsb->freq_base = 800000000; /* 800MHz */ + svsb->dvt_fixed = 2; + } + break; + default: + break; + } + } + + /* Get thermal efuse by nvmem */ + cell = nvmem_cell_get(svsp->dev, "t-calibration-data"); + if (IS_ERR_OR_NULL(cell)) { + dev_err(svsp->dev, "no thermal cell, no mon mode\n"); + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + svsb->mode_support &= ~SVSB_MODE_MON; + } + + return true; + } + + svsp->tefuse = nvmem_cell_read(cell, &svsp->tefuse_num);nvmem_cell_read may return an error pointer: you have to check that. if (IS_ERR(svsp->tefuse)) ......... Failing to perform this check will produce unpredictable behavior during the parsing stage.
Sure. I'll add the error handling when using nvmem_cell_read(). Thanks for the advice.
quoted
+ svsp->tefuse_num /= sizeof(u32); + nvmem_cell_put(cell); + + /* Thermal efuse parsing */ + tp.adc_ge_t = (svsp->tefuse[1] >> 22) & GENMASK(9, 0); + tp.adc_oe_t = (svsp->tefuse[1] >> 12) & GENMASK(9, 0); + + tp.o_vtsmcu1 = (svsp->tefuse[0] >> 17) & GENMASK(8, 0); + tp.o_vtsmcu2 = (svsp->tefuse[0] >> 8) & GENMASK(8, 0); + tp.o_vtsmcu3 = svsp->tefuse[1] & GENMASK(8, 0); + tp.o_vtsmcu4 = (svsp->tefuse[2] >> 23) & GENMASK(8, 0); + tp.o_vtsmcu5 = (svsp->tefuse[2] >> 5) & GENMASK(8, 0); + tp.o_vtsabb = (svsp->tefuse[2] >> 14) & GENMASK(8, 0); + + tp.degc_cali = (svsp->tefuse[0] >> 1) & GENMASK(5, 0); + tp.adc_cali_en_t = svsp->tefuse[0] & BIT(0); + tp.o_slope_sign = (svsp->tefuse[0] >> 7) & BIT(0); + + tp.ts_id = (svsp->tefuse[1] >> 9) & BIT(0); + tp.o_slope = (svsp->tefuse[0] >> 26) & GENMASK(5, 0); +Regards, - Angelo
_______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel