Re: [PATCH v10 5/6] soc: qcom: Add support for Core Power Reduction v3, v4 and Hardened

[AngeloGioacchino Del Regno]

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.
> > >
> > > 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.

Cheers!
Angelo

> SoCs. Other than that, I have no additional comments for that series.