Messages in this thread | | | Date | Mon, 31 Jan 2022 12:11:51 +0100 | Subject | Re: [PATCH v22 5/7] soc: mediatek: SVS: add debug commands | From | AngeloGioacchino Del Regno <> |
| |
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
| |