Re: [PATCH v10 3/7] [v10, 3/7]: soc: mediatek: SVS: introduce MTK SVS engine

[Nicolas Boichat]

On Mon, Jan 4, 2021 at 4:51 PM Roger Lu <roger.lu@xxxxxxxxxxxx> wrote:
>
>
> Hi Nicolas,
>
> Thanks for all the advices.
>
> On Thu, 2020-12-31 at 10:10 +0800, Nicolas Boichat wrote:
> > On Sun, Dec 27, 2020 at 6:55 PM Roger Lu <roger.lu@xxxxxxxxxxxx> wrote:
[snip]
> > > +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.

>
> >
> > > +       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? ,-)

>
> >
[snip]
> > > +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.
>
> >
> > > +       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.
>
> >
> > > +                               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]
> > > +
> > > +       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 *).
>
> >
> > 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]