| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1578546169.32655.13.camel@mtksdaap41>
Date: Thu, 9 Jan 2020 13:02:49 +0800
From: Roger Lu <roger.lu@...iatek.com>
To: Pi-Hsun Shih <pihsun@...omium.org>
CC: Kevin Hilman <khilman@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Nicolas Boichat <drinkcat@...gle.com>,
Stephen Boyd <sboyd@...nel.org>,
"Fan Chen" <fan.chen@...iatek.com>,
HenryC Chen <HenryC.Chen@...iatek.com>,
"YT Lee" <yt.lee@...iatek.com>,
Xiaoqing Liu <Xiaoqing.Liu@...iatek.com>,
Charles Yang <Charles.Yang@...iatek.com>,
Angus Lin <Angus.Lin@...iatek.com>,
Mark Rutland <mark.rutland@....com>,
"Matthias Brugger" <matthias.bgg@...il.com>,
Nishanth Menon <nm@...com>,
"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS"
<devicetree@...r.kernel.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-arm-kernel@...ts.infradead.org>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>,
open list <linux-kernel@...r.kernel.org>,
"open list:THERMAL" <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v6 3/3] PM / AVS: SVS: Introduce SVS engine
Dear Pi-Hsun,
Thanks for the advices.
On Wed, 2020-01-08 at 19:12 +0800, Pi-Hsun Shih wrote:
> Hi Roger,
>
> On Tue, Jan 7, 2020 at 3:02 PM Roger Lu <roger.lu@...iatek.com> wrote:
> >
> > The SVS (Smart Voltage Scaling) engine is a piece
> > of hardware which is used to calculate optimized
> > voltage values of several power domains,
> > e.g. CPU/GPU/CCI, according to chip process corner,
> > temperatures, and other factors. Then DVFS driver
> > could apply those optimized voltage values to reduce
> > power consumption.
> >
> > Signed-off-by: Roger Lu <roger.lu@...iatek.com>
> > ---
> > drivers/power/avs/Kconfig | 10 +
> > drivers/power/avs/Makefile | 1 +
> > drivers/power/avs/mtk_svs.c | 2075 +++++++++++++++++++++++++++++++++
> > include/linux/power/mtk_svs.h | 23 +
> > 4 files changed, 2109 insertions(+)
> > create mode 100644 drivers/power/avs/mtk_svs.c
> > create mode 100644 include/linux/power/mtk_svs.h
> > [...]
> > new file mode 100644
> > index 000000000000..c46211a15fcd
> > --- /dev/null
> > +++ b/drivers/power/avs/mtk_svs.c
> > [...]
> > +
> > +static bool svs_mt8183_efuse_parsing(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct thermal_parameter tp;
> > + struct svs_bank *svsb;
> > + bool mon_mode_support = true;
> > + int format[6], x_roomt[6], tb_roomt = 0;
> > + u32 idx, i, ft_pgm, mts, temp0, temp1, temp2;
> > +
> > + if (svsp->fake_efuse) {
> > + pr_notice("fake efuse\n");
> > + svs->efuse[0] = 0x00310080;
> > + svs->efuse[1] = 0xabfbf757;
> > + svs->efuse[2] = 0x47c747c7;
> > + svs->efuse[3] = 0xabfbf757;
> > + svs->efuse[4] = 0xe7fca0ec;
> > + svs->efuse[5] = 0x47bf4b88;
> > + svs->efuse[6] = 0xabfb8fa5;
> > + svs->efuse[7] = 0xabfb217b;
> > + svs->efuse[8] = 0x4bf34be1;
> > + svs->efuse[9] = 0xabfb670d;
> > + svs->efuse[16] = 0xabfbc653;
> > + svs->efuse[17] = 0x47f347e1;
> > + svs->efuse[18] = 0xabfbd848;
> > +
> > + svs->thermal_efuse[0] = 0x02873f69;
> > + svs->thermal_efuse[1] = 0xa11d9142;
> > + svs->thermal_efuse[2] = 0xa2526900;
> > + }
> > +
> > + for (i = 0; i < svsp->efuse_num; i++) {
> > + if (svs->efuse[i])
> > + pr_notice("M_HW_RES%d: 0x%08x\n", i, svs->efuse[i]);
> > + }
> > +
> > + /* svs efuse parsing */
> > + ft_pgm = (svs->efuse[0] >> 4) & 0xf;
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + if (ft_pgm <= 1)
> > + svsb->init01_volt_flag = SVSB_INIT01_VOLT_IGNORE;
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + svsb->bdes = svs->efuse[16] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[16] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[16] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[16] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = (svs->efuse[17] >> 16) & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 10;
> > + else
> > + svsb->volt_offset += 2;
> > + break;
> > + case SVS_CPU_BIG:
> > + svsb->bdes = svs->efuse[18] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[18] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[18] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[18] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = svs->efuse[17] & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 15;
> > + else
> > + svsb->volt_offset += 12;
> > + break;
> > + case SVS_CCI:
> > + svsb->bdes = svs->efuse[4] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[4] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[4] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[4] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = (svs->efuse[5] >> 16) & GENMASK(7, 0);
> > +
> > + if (ft_pgm <= 3)
> > + svsb->volt_offset += 10;
> > + else
> > + svsb->volt_offset += 2;
> > + break;
> > + case SVS_GPU:
> > + svsb->bdes = svs->efuse[6] & GENMASK(7, 0);
> > + svsb->mdes = (svs->efuse[6] >> 8) & GENMASK(7, 0);
> > + svsb->dcbdet = (svs->efuse[6] >> 16) & GENMASK(7, 0);
> > + svsb->dcmdet = (svs->efuse[6] >> 24) & GENMASK(7, 0);
> > + svsb->mtdes = svs->efuse[5] & GENMASK(7, 0);
> > +
> > + if (ft_pgm >= 2) {
> > + svsb->freq_base = 800000000; /* 800MHz */
> > + svsb->dvt_fixed = 2;
> > + }
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > +
> > + /* Thermal efuse parsing */
> > + if (!svs->thermal_efuse)
> > + return true;
> > +
> > + tp.adc_ge_t = (svs->thermal_efuse[1] >> 22) & GENMASK(9, 0);
> > + tp.adc_oe_t = (svs->thermal_efuse[1] >> 12) & GENMASK(9, 0);
> > +
> > + tp.o_vtsmcu1 = (svs->thermal_efuse[0] >> 17) & GENMASK(8, 0);
> > + tp.o_vtsmcu2 = (svs->thermal_efuse[0] >> 8) & GENMASK(8, 0);
> > + tp.o_vtsmcu3 = svs->thermal_efuse[1] & GENMASK(8, 0);
> > + tp.o_vtsmcu4 = (svs->thermal_efuse[2] >> 23) & GENMASK(8, 0);
> > + tp.o_vtsmcu5 = (svs->thermal_efuse[2] >> 5) & GENMASK(8, 0);
> > + tp.o_vtsabb = (svs->thermal_efuse[2] >> 14) & GENMASK(8, 0);
> > +
> > + tp.degc_cali = (svs->thermal_efuse[0] >> 1) & GENMASK(5, 0);
> > + tp.adc_cali_en_t = svs->thermal_efuse[0] & BIT(0);
> > + tp.o_slope_sign = (svs->thermal_efuse[0] >> 7) & BIT(0);
> > +
> > + tp.ts_id = (svs->thermal_efuse[1] >> 9) & BIT(0);
> > + tp.o_slope = (svs->thermal_efuse[0] >> 26) & GENMASK(5, 0);
> > +
> > + if (tp.adc_cali_en_t == 1) {
> > + if (tp.ts_id == 0)
> > + tp.o_slope = 0;
> > +
> > + if ((tp.adc_ge_t < 265 || tp.adc_ge_t > 758) ||
> > + (tp.adc_oe_t < 265 || tp.adc_oe_t > 758) ||
> > + (tp.o_vtsmcu1 < -8 || tp.o_vtsmcu1 > 484) ||
> > + (tp.o_vtsmcu2 < -8 || tp.o_vtsmcu2 > 484) ||
> > + (tp.o_vtsmcu3 < -8 || tp.o_vtsmcu3 > 484) ||
> > + (tp.o_vtsmcu4 < -8 || tp.o_vtsmcu4 > 484) ||
> > + (tp.o_vtsmcu5 < -8 || tp.o_vtsmcu5 > 484) ||
> > + (tp.o_vtsabb < -8 || tp.o_vtsabb > 484) ||
> > + (tp.degc_cali < 1 || tp.degc_cali > 63)) {
> > + pr_err("bad thermal efuse data. disable mon mode\n");
> > + mon_mode_support = false;
> > + }
> > + } else {
> > + pr_err("no thermal efuse data. disable mon mode\n");
> > + mon_mode_support = false;
> > + }
> > +
> > + if (!mon_mode_support) {
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->mode_support &= ~SVSB_MODE_MON;
> > + }
> > +
> > + return true;
> > + }
> > +
> > + tp.ge = ((tp.adc_ge_t - 512) * 10000) / 4096;
> > + tp.oe = (tp.adc_oe_t - 512);
> > + tp.gain = (10000 + tp.ge);
> > +
> > + format[0] = (tp.o_vtsmcu1 + 3350 - tp.oe);
> > + format[1] = (tp.o_vtsmcu2 + 3350 - tp.oe);
> > + format[2] = (tp.o_vtsmcu3 + 3350 - tp.oe);
> > + format[3] = (tp.o_vtsmcu4 + 3350 - tp.oe);
> > + format[4] = (tp.o_vtsmcu5 + 3350 - tp.oe);
> > + format[5] = (tp.o_vtsabb + 3350 - tp.oe);
> > +
> > + for (i = 0; i < 6; i++)
> > + x_roomt[i] = (((format[i] * 10000) / 4096) * 10000) / tp.gain;
> > +
> > + temp0 = (10000 * 100000 / tp.gain) * 15 / 18;
> > +
> > + if (tp.o_slope_sign == 0)
> > + mts = (temp0 * 10) / (1534 + tp.o_slope * 10);
> > + else
> > + mts = (temp0 * 10) / (1534 - tp.o_slope * 10);
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > + svsb->mts = mts;
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + tb_roomt = x_roomt[3];
> > + break;
> > + case SVS_CPU_BIG:
> > + tb_roomt = x_roomt[4];
> > + break;
> > + case SVS_CCI:
> > + tb_roomt = x_roomt[3];
> > + break;
> > + case SVS_GPU:
> > + tb_roomt = x_roomt[1];
> > + break;
> > + default:
> > + break;
>
> There was a "return -EINVAL;" here in v5, should this be a "return
> false"? This function currently always return true.
This "return -EINVAL" will be detected in svs common flow
"svs_resource_setup()". Platform efuse parsing flow does efuse
parsing/checking job only.
>
> > + }
> > +
> > + temp0 = (tp.degc_cali * 10 / 2);
> > + temp1 = ((10000 * 100000 / 4096 / tp.gain) *
> > + tp.oe + tb_roomt * 10) * 15 / 18;
> > +
> > + if (tp.o_slope_sign == 0)
> > + temp2 = temp1 * 100 / (1534 + tp.o_slope * 10);
> > + else
> > + temp2 = temp1 * 100 / (1534 - tp.o_slope * 10);
> > +
> > + svsb->bts = (temp0 + temp2 - 250) * 4 / 10;
> > + }
> > +
> > + return true;
> > +}
> > [...]
> > +
> > +static int svs_resource_setup(struct mtk_svs *svs)
> > +{
> > + const struct svs_platform *svsp = svs->platform;
> > + struct svs_bank *svsb;
> > + struct platform_device *pdev;
> > + struct device_node *np = NULL;
> > + struct dev_pm_opp *opp;
> > + unsigned long freq;
> > + int count, ret;
> > + u32 idx, i;
> > +
> > + for (idx = 0; idx < svsp->bank_num; idx++) {
> > + svsb = &svsp->banks[idx];
> > +
> > + switch (svsb->sw_id) {
> > + case SVS_CPU_LITTLE:
> > + svsb->name = "SVS_CPU_LITTLE";
> > + break;
> > + case SVS_CPU_BIG:
> > + svsb->name = "SVS_CPU_BIG";
> > + break;
> > + case SVS_CCI:
> > + svsb->name = "SVS_CCI";
> > + break;
> > + case SVS_GPU:
> > + svsb->name = "SVS_GPU";
> > + break;
> > + default:
> > + WARN_ON(1);
> > + return -EINVAL;
> > + }
> > +
> > + /* Add svs bank device for opp-table/mtcmos/buck control */
> > + pdev = platform_device_alloc(svsb->name, 0);
> > + if (!pdev) {
> > + pr_err("%s: fail to alloc pdev for svs_bank\n",
> > + svsb->name);
> > + return -ENOMEM;
> > + }
> > +
> > + for_each_child_of_node(svs->dev->of_node, np) {
> > + if (of_device_is_compatible(np, svsb->of_compatible)) {
> > + pdev->dev.of_node = np;
> > + break;
> > + }
> > + }
> > +
> > + ret = platform_device_add(pdev);
> > + if (ret) {
> > + pr_err("%s: fail to add svs_bank device: %d\n",
> > + svsb->name, ret);
> > + return ret;
> > + }
> > +
> > + svsb->dev = &pdev->dev;
> > + dev_set_drvdata(svsb->dev, svs);
> > + ret = dev_pm_opp_of_add_table(svsb->dev);
> > + if (ret) {
> > + pr_err("%s: fail to add opp table: %d\n",
> > + svsb->name, ret);
> > + return ret;
> > + }
> > +
> > + mutex_init(&svsb->lock);
> > +
> > + svsb->buck = devm_regulator_get_optional(svsb->dev,
> > + svsb->buck_name);
> > + if (IS_ERR(svsb->buck)) {
> > + pr_err("%s: cannot get regulator \"%s-supply\"\n",
> > + svsb->name, svsb->buck_name);
> > + return PTR_ERR(svsb->buck);
> > + }
> > +
> > + count = dev_pm_opp_get_opp_count(svsb->dev);
> > + if (svsb->opp_count != count) {
> > + pr_err("%s: opp_count not \"%u\" but get \"%d\"?\n",
> > + svsb->name, svsb->opp_count, count);
> > + return count;
> > + }
> > +
> > + for (i = 0, freq = (u32)-1; i < svsb->opp_count; i++, freq--) {
>
> Can use U32_MAX instead of (u32)-1.
Oh Sure. Thanks for the reference. I'll update it in the next patch.
>
> > + opp = dev_pm_opp_find_freq_floor(svsb->dev, &freq);
> > + if (IS_ERR(opp)) {
> > + pr_err("%s: error opp entry!!, err = %ld\n",
> > + svsb->name, PTR_ERR(opp));
> > + return PTR_ERR(opp);
> > + }
> > +
> > + svsb->opp_freqs[i] = freq;
> > + svsb->opp_volts[i] = dev_pm_opp_get_voltage(opp);
> > + svsb->freqs_pct[i] = percent(svsb->opp_freqs[i],
> > + svsb->freq_base);
> > + dev_pm_opp_put(opp);
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > [...]
> > +
> > +static ssize_t svs_debug_proc_write(struct file *file,
> > + const char __user *buffer,
> > + size_t count, loff_t *pos)
> > +{
> > + struct svs_bank *svsb = (struct svs_bank *)PDE_DATA(file_inode(file));
> > + struct mtk_svs *svs = dev_get_drvdata(svsb->dev);
> > + char *buf = (char *)__get_free_page(GFP_USER);
> > + unsigned long flags;
> > + int enabled, ret;
> > +
> > + if (svsb->phase == SVSB_PHASE_ERROR)
> > + return count;
> > +
> > + if (!buf)
> > + return -ENOMEM;
> > +
> > + if (count >= PAGE_SIZE) {
> > + free_page((unsigned long)buf);
> > + return -EINVAL;
> > + }
> > +
> > + if (copy_from_user(buf, buffer, count)) {
> > + free_page((unsigned long)buf);
> > + return -EFAULT;
> > + }
> > +
> > + buf[count] = '\0';
>
> Can use memdup_user_nul to allocate the buf and copy from user buffer
> for the above operations (and for other _write() functions).
Cool. I'll use memdup_user_nul() instead in the next patch. Thanks.
>
> > +
> > + ret = kstrtoint(buf, 10, &enabled);
> > + if (ret)
> > + return ret;
> > +
> > + if (!enabled) {
> > + flags = claim_mtk_svs_lock();
> > + svs->bank = svsb;
> > + svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> > + svs_switch_bank(svs);
> > + svs_writel(svs, SVSEN_OFF, SVSEN);
> > + svs_writel(svs, INTSTS_CLEAN, INTSTS);
> > + release_mtk_svs_lock(flags);
> > + }
> > +
> > + svsb->phase = SVSB_PHASE_ERROR;
> > + svsb_set_volts(svsb, true);
>
> Missing free_page() (or kfree() if changing to memdup_user_nul) here
> (and in other _write() functions).
No problem. I'll add free method here in the next patch. Thanks.
>
> > +
> > + return count;
> > +}
> > +
> > +proc_fops_rw(svs_debug);
> > +
> > [...]
Powered by blists - more mailing lists