Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands

From: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
To: Roger Lu <roger.lu@mediatek.com>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Enric Balletbo Serra <eballetbo@gmail.com>,
	Kevin Hilman <khilman@kernel.org>,
	Rob Herring <robh+dt@kernel.org>,
	Nicolas Boichat <drinkcat@google.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Philipp Zabel <p.zabel@pengutronix.de>
Cc: Fan Chen <fan.chen@mediatek.com>,
	HenryC Chen <HenryC.Chen@mediatek.com>,
	 Xiaoqing Liu <Xiaoqing.Liu@mediatek.com>,
	Charles Yang <Charles.Yang@mediatek.com>,
	Angus Lin <Angus.Lin@mediatek.com>,
	Mark Rutland <mark.rutland@arm.com>, Nishanth Menon <nm@ti.com>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
	linux-pm@vger.kernel.org,
	Project_Global_Chrome_Upstream_Group@mediatek.com,
	Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands
Date: Mon, 31 Jan 2022 12:11:51 +0100	[thread overview]
Message-ID: <0846872b-03da-ee5d-6a9d-e6c9fa754191@collabora.com> (raw)
In-Reply-To: <20220127033956.24585-6-roger.lu@mediatek.com>

Il 27/01/22 04:39, 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>

Hello Roger,
I was thinking about what this patch is adding... and I have a few considerations.

It's nice to have a debugging mechanism to read the status and dump registers, as
that's very helpful when doing heavy debugging of the IP... but adding the
possibility to write a voltage offset may be very dangerous: think about the case
in which, either for misconfiguration, or for any other reason, the debugfs entry
that allows writing voffset becomes user-writable, or a user writes an impossibly
high voffset.
In case a very low (negative) voffset is entered, the platform would crash (denial
of service); if a very high voffset is entered, hardware damage may occur.

For this reason, there are two proposals:
1. If you want to keep the debugfs voffset write, please constrain the permissible
    voffset to an acceptable range that at least makes it unlikely to damage the HW;
    Moreover, since voffset write is a feature that would be used in very limited
    debugging cases, I think that this should be implemented over a build-time
    configuration barrier... something like CONFIG_MTK_SVS_DEBUG_ALLOW_WRITE, or
    similar;
2. Since it's very unlikely for someone to really play that much with a voltage
    offset during runtime, and since this looks like something very machine specific
    (perhaps addressing board-specific quirks?), I would suggest to add this as a
    device-tree parameter instead, such as "mediatek,svs-voffset", as it is indeed
    possible to specify both positive or negative values in DT.

I would prefer proposal 2, as it looks generally cleaner and way less risky.

Regards,
Angelo

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek