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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47bcbffc-42f6-335e-dfab-990e0ab5f103@collabora.com>
Date:   Fri, 7 Jan 2022 15:34:56 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Roger Lu <roger.lu@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Enric Balletbo Serra <eballetbo@...il.com>,
        Kevin Hilman <khilman@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Nicolas Boichat <drinkcat@...gle.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>
Cc:     Fan Chen <fan.chen@...iatek.com>,
        HenryC Chen <HenryC.Chen@...iatek.com>,
        YT Lee <yt.lee@...iatek.com>,
        Xiaoqing Liu <Xiaoqing.Liu@...iatek.com>,
        Charles Yang <Charles.Yang@...iatek.com>,
        Angus Lin <Angus.Lin@...iatek.com>,
        Mark Rutland <mark.rutland@....com>,
        Nishanth Menon <nm@...com>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pm@...r.kernel.org,
        Project_Global_Chrome_Upstream_Group@...iatek.com,
        Guenter Roeck <linux@...ck-us.net>
Subject: Re: [PATCH v21 5/8] soc: mediatek: SVS: add debug commands

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@...iatek.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:
>