Re: [PATCH v21 5/8] soc: mediatek: SVS: add debug commands

[Roger Lu <roger.lu@mediatek.com>]

Hi AngeloGioacchino,

Sorry for the late reply and thanks for the advice.

On Fri, 2022-01-07 at 15:34 +0100, AngeloGioacchino Del Regno wrote:
> Il 07/01/22 10:51, Roger Lu ha scritto:
> > The purpose of SVS is to help find the suitable voltages
> > for DVFS. Therefore, if SVS bank voltages are concerned
> > to be wrong, we can adjust SVS bank voltages by this patch.
> > 
> > Signed-off-by: Roger Lu <roger.lu@mediatek.com>
> > ---
> >   drivers/soc/mediatek/mtk-svs.c | 321 ++++++++++++++++++++++++++++++++-
> >   1 file changed, 318 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/soc/mediatek/mtk-svs.c b/drivers/soc/mediatek/mtk-svs.c
> > index 042c6e8e9069..93cdaecadd6d 100644
> > --- a/drivers/soc/mediatek/mtk-svs.c
> > +++ b/drivers/soc/mediatek/mtk-svs.c
> 
> ..snip..
> 
> > @@ -605,6 +896,16 @@ static void svs_set_bank_phase(struct svs_platform
> > *svsp,
> >   	}
> >   }
> >   
> > +static inline void svs_save_bank_register_data(struct svs_platform *svsp,
> > +					       enum svsb_phase phase)
> > +{
> > +	struct svs_bank *svsb = svsp->pbank;
> > +	enum svs_reg_index rg_i;
> > +
> 
> I think that it'd be a good idea to add an `enable` parameter, so that we
> don't always do a register dump; after all, this is a debugging feature and
> it's going to be completely irrelevant to the user, so keeping this disabled
> by default would ensure to get no performance degradation (even if small)
> unless really wanted.
> 
> So, in this case, here we'd have
> 
> 	if (!svsp->debug_enabled)
> 		return;

Thanks for pointing out the concern. Excuse us, we really need this to be
enabled by default. If we add a enable flag here, we'll face below problems and
make debug more difficult.

1. If we enable it afterward by cmd, init0[1~2] registers' data cannot be
recorded expectedly because the init flow has been finished already and won't be
run anymore. So, it doesn't work by using cmd to enable the flag.
2. If we add a enable flag here, it means we have to re-build the kernel load in
order to enable this flag. However, we cannot re-build the kernel load and needs
to debug directly sometimes. It's a sad situation... :(

> 
> > +	for (rg_i = DESCHAR; rg_i < SVS_REG_MAX; rg_i++)
> > +		svsb->reg_data[phase][rg_i] = svs_readl_relaxed(svsp, rg_i);
> > +}
> > +
> 
> Of course, this implies adding a new debugfs entry to enable/disable the
> debugging.
> Everything else looks good :)

Oh, excuse us, we have to keep the old design for better instant support and
thanks for the understanding.

> >   static inline void svs_error_isr_handler(struct svs_platform *svsp)
> >   {
> >   	struct svs_bank *svsb = svsp->pbank;
> > @@ -619,6 +920,8 @@ static inline void svs_error_isr_handler(struct
> > svs_platform *svsp)
> >   		svs_readl_relaxed(svsp, SMSTATE1));
> >   	dev_err(svsb->dev, "TEMP = 0x%08x\n", svs_readl_relaxed(svsp, TEMP));
> >   
> > +	svs_save_bank_register_data(svsp, SVSB_PHASE_ERROR);
> > +
> >   	svsb->mode_support = SVSB_MODE_ALL_DISABLE;
> >   	svsb->phase = SVSB_PHASE_ERROR;

[snip]