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@xxxxxxxxxxxx>
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
