On 28.02.2023 09:19, AngeloGioacchino Del Regno wrote: > Il 27/02/23 14:20, Dmitry Baryshkov ha scritto: >> On Mon, 27 Feb 2023 at 15:06, AngeloGioacchino Del Regno >> <angelogioacchino.delregno@xxxxxxxxxxxxx> wrote: >>> >>> Il 27/02/23 13:01, Dmitry Baryshkov ha scritto: >>>> >>>> I took a glance at the 'cpufreq: qcom-hw: Implement CPRh aware OSM programming' >>>> patch, it doesn't seem to use the header (maybe I checked the older version of the >>>> patch). As for me, this is another signal that cpr_ext_data should come together >>>> with the LUT programming rather than with the CPRh itself. >>>> >>>>> Konrad, perhaps you can send the cpufreq-hw commits in a separate series, in >>>>> which cover letter you mention a dependency on this one? >>>>> That would *clearly* show the full picture to reviewers. If by "the cpufreq-hw commits" you mean the OSM enablement, that's the plan! I just don't think sending it parallel to this series makes a whole lot of sense logistically (even though it does logically), as they both are quite gigantic.. For reference, here's the last revision that made it to lkml, I think: https://lore.kernel.org/phone-devel/20210701105730.322718-7-angelogioacchino.delregno@xxxxxxxxxxxxxx/ >>>> >>>> Yes, that would be great. A small note regarding those patches. I see that you >>>> patched the qcom-cpufreq-hw.c. This way first the driver programs the LUT, then it >>>> reads it back to setup the OPPs. Would it be easier to split OSM-not-programmed >>>> driver? >>>> >>> >>> When I engineered that solution, I kept the cpufreq-hw reading *again* the values >>> from OSM to keep the driver *fully* compatible with the bootloader-programmed OSM >>> flow, which makes one thing (in my opinion) perfectly clear: that programming >>> sequence is exactly the same as what happens "under the hood" on SDM845 (and later) >>> but performed here-instead-of-there (linux instead of bootloader), with the actual >>> scaling driver being 100% the same between the two flows in the end. >>> >>> Having two drivers as you suggested would indeed achieve the same, but wouldn't be >>> any easier... if you do that, you'd have to *somehow* make sure that the >>> programming driver does its job before the cpufreq driver tries to read the OSM >>> status, adding one more link to an already long chain. >>> >>> Besides, I remember that this question got asked a while ago on the mailing lists >>> and there was a short discussion about it: >>> >>> https://www.mail-archive.com/linux-kernel@xxxxxxxxxxxxxxx/msg2555580.html >> >> Ack, I see. Maybe splitting LUT programming to a separate source file >> would emphasise the fact that it is only required for some (older) > > Maybe. I'm not sure it's worth adding a new helper file, but I don't really have > any strong arguments against... > > Konrad, your call. qcom-cpufreq-hw.c is currently a driver for a small subset of the functionality that's available from the hardware behind it. Hence I reckon it'd only be correct to also keep all the "proper" setup there, as it concerns the same hardware, just that previously one did not need to utilize it fully, as the boot firmware ever so graciously performed that exact same setup with elevated privileges and before jumping to HLOS. Konrad > > Cheers! > Angelo > >> SoCs. Other than that, I have no additional comments for that series. >>
