Re: [PATCH v10 3/7] [v10, 3/7]: soc: mediatek: SVS: introduce MTK SVS engine
From: Nicolas Boichat <hidden>
Date: 2021-01-04 09:28:43
On Mon, Jan 4, 2021 at 4:51 PM Roger Lu [off-list ref] wrote:
Hi Nicolas, Thanks for all the advices. On Thu, 2020-12-31 at 10:10 +0800, Nicolas Boichat wrote:quoted
On Sun, Dec 27, 2020 at 6:55 PM Roger Lu [off-list ref] wrote:
[snip]
quoted
quoted
+static int svs_adjust_pm_opp_volts(struct svs_bank *svsb, bool force_update) +{ + int tzone_temp, ret = -EPERM;No need to initialize ret.Oh, excuse me, some coding check tool warn that this `ret` might return without being uninitialized. Therefore, I'll keep the initialization.
Oh, you're right, there is a possible path where ret is not set. sgtm then.
quoted
quoted
+ 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)?\n", + svsb->tzone_name, ret); + dev_err(svsb->dev, "set signed-off voltage\n");Please merge the error message in one line (I'm not sure what "set signed-off voltage" means here).1. Ok, I'll merge them. Thanks. 2. signed-off voltages means CPU DVFS default voltages
So just write "default voltages" then? ,-)
quoted
[snip]
quoted
quoted
+static irqreturn_t svs_isr(int irq, void *data) +{ + struct svs_platform *svsp = (struct svs_platform *)data;cast not needed.Ok, I'll remove it. Thanks.quoted
quoted
+ struct svs_bank *svsb = NULL; + unsigned long flags; + u32 idx, int_sts, svs_en; + + for (idx = 0; idx < svsp->bank_num; idx++) { + svsb = &svsp->banks[idx]; + + spin_lock_irqsave(&mtk_svs_lock, flags); + svsp->pbank = svsb; + + /* Find out which svs bank fires interrupt */ + if (svsb->int_st & svs_readl(svsp, INTST)) { + spin_unlock_irqrestore(&mtk_svs_lock, flags); + continue; + } + + if (!svsb->suspended) { + svs_switch_bank(svsp); + int_sts = svs_readl(svsp, INTSTS); + svs_en = svs_readl(svsp, SVSEN); + + if (int_sts == SVSB_INTSTS_COMPLETE && + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT01)) + svs_init01_isr_handler(svsp); + else if ((int_sts == SVSB_INTSTS_COMPLETE) && + ((svs_en & SVSB_EN_MASK) == SVSB_EN_INIT02)) + svs_init02_isr_handler(svsp); + else if (!!(int_sts & SVSB_INTSTS_MONVOP))!! is not required.Ok, I'll remove it. Thanks.quoted
quoted
+ svs_mon_mode_isr_handler(svsp); + else + svs_error_isr_handler(svsp); + } + + spin_unlock_irqrestore(&mtk_svs_lock, flags); + break; + }This will panic if svsb is NULL, is that ok or do you want to catch that?Oh, it is fine. Thanks for the heads-up.
I should have been stronger in my statement, I think you want to add a BUG_ON(!svsb) to crash in a slightly more predictable manner. [snip]
quoted
quoted
+ + svsp->tefuse = (u32 *)nvmem_cell_read(cell, &svsp->tefuse_num);Cast not needed.Ok, I'll remove it if build/test ok. Because nvmem_cell_read returns (void *).quoted
Also, this need to be freed somewhere in remove code (kfree(svsp->tefuse)). And it seems like svsp->tefuse is only used in this function, can you just allocate it here?Oh, svsp->tefuse will be used in SVS debug patch for debug purpose. So, I need to save it as struct member.
Oh I missed that, sgtm then. Thanks.
[snip] _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel