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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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;

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

>   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;
>   
> @@ -635,6 +938,8 @@ static inline void svs_init01_isr_handler(struct svs_platform *svsp)
>   		 svs_readl_relaxed(svsp, VDESIGN30),
>   		 svs_readl_relaxed(svsp, DCVALUES));
>   
> +	svs_save_bank_register_data(svsp, SVSB_PHASE_INIT01);
> +
>   	svsb->phase = SVSB_PHASE_INIT01;
>   	svsb->dc_voffset_in = ~(svs_readl_relaxed(svsp, DCVALUES) &
>   				GENMASK(15, 0)) + 1;
> @@ -662,6 +967,8 @@ static inline void svs_init02_isr_handler(struct svs_platform *svsp)
>   		 svs_readl_relaxed(svsp, VOP30),
>   		 svs_readl_relaxed(svsp, DCVALUES));
>   
> +	svs_save_bank_register_data(svsp, SVSB_PHASE_INIT02);
> +
>   	svsb->phase = SVSB_PHASE_INIT02;
>   	svsb->get_volts(svsp);
>   
> @@ -673,6 +980,8 @@ static inline void svs_mon_mode_isr_handler(struct svs_platform *svsp)
>   {
>   	struct svs_bank *svsb = svsp->pbank;
>   
> +	svs_save_bank_register_data(svsp, SVSB_PHASE_MON);
> +
>   	svsb->phase = SVSB_PHASE_MON;
>   	svsb->temp = svs_readl_relaxed(svsp, TEMP) & GENMASK(7, 0);
>   	svsb->get_volts(svsp);
> @@ -1658,6 +1967,12 @@ static int svs_probe(struct platform_device *pdev)
>   		goto svs_probe_iounmap;
>   	}
>   
> +	ret = svs_create_debug_cmds(svsp);
> +	if (ret) {
> +		dev_err(svsp->dev, "svs create debug cmds fail: %d\n", ret);
> +		goto svs_probe_iounmap;
> +	}
> +
>   	return 0;
>   
>   svs_probe_iounmap:
>