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

[Roger Lu]

Hi Nicolas,

On Mon, 2021-01-04 at 17:27 +0800, Nicolas Boichat wrote:
> 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? ,-)

Ok, thanks. :)

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

Ok, I'll add BUG_ON(!svsb) to give an evident heads-up. Thanks.

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