lkml.org 
[lkml]   [2021]   [Dec]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v16 3/7] soc: mediatek: SVS: introduce MTK SVS engine
    From
    Date
    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:
    > > 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 <roger.lu@mediatek.com>
    > > ---
    > > 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]

    > > +/* 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

    /**
    * .........

    >
    >
    > > + * 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 condition
    >
    > Please 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.

    >
    > > + *
    > > + * 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]

    > > +/*
    >
    > Same here, use /**

    Sure, I'll improve it in the next version. Thanks.

    >
    > > + * 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.

    >
    > > + * 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]

    > > +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.

    > > + 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]

    > > +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;
    >
    > > +
    > > + 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.

    > > + 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.

    > > + break;
    > > + }
    > > +}

    [snip]

    > > +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.

    > > + 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.

    > > + 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]

    > > +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.

    > > + 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
    >
    >

    \
     
     \ /
      Last update: 2021-12-24 10:28    [W:2.822 / U:0.116 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site