On Thu, 21 Mar 2024 08:39:23 +0530
Viresh Kumar <viresh.kumar@xxxxxxxxxx> wrote:
Hi Viresh,
thanks for chiming in!
> On 20-03-24, 15:37, Andre Przywara wrote:
> > On Wed, 20 Mar 2024 10:02:28 -0500
> > Rob Herring <robh@xxxxxxxxxx> wrote:
> > > On Mon, Mar 18, 2024 at 01:12:23AM +0000, Andre Przywara wrote:
> > > > From: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> > > > - opp-1080000000 {
> > > > + opp-792000000-l {
> > > > clock-latency-ns = <244144>; /* 8 32k periods */
> > > > - opp-hz = /bits/ 64 <1080000000>;
> > > > + opp-hz = /bits/ 64 <792000000>;
> > > >
> > > > - opp-microvolt-speed0 = <1060000>;
> > > > - opp-microvolt-speed1 = <880000>;
> > > > - opp-microvolt-speed2 = <840000>;
> > > > + opp-microvolt = <900000>;
> > > > + opp-supported-hw = <0x02>;
> > > > };
> > > >
> > > > - opp-1320000000 {
> > > > + opp-792000000-h {
> > > > clock-latency-ns = <244144>; /* 8 32k periods */
> > > > - opp-hz = /bits/ 64 <1320000000>;
> > > > + opp-hz = /bits/ 64 <792000000>;
> > > >
> > > > - opp-microvolt-speed0 = <1160000>;
> > > > - opp-microvolt-speed1 = <940000>;
> > > > - opp-microvolt-speed2 = <900000>;
> > > > + opp-microvolt = <940000>;
> > > > + opp-supported-hw = <0x10>;
> > >
> > > So far, we've avoided multiple entries for a single frequency. I think
> > > it would be good to maintain that.
> >
> > Fair, I wasn't super happy with that either, but it still seemed better
> > than the alternatives.
> >
> > > Couldn't you just do:
> > >
> > > opp-supported-hw = <0>, <0x10>, <0x02>;
> > >
> > > Where the index corresponds to speed0, speed1, speed2.
> > >
> > > If not, then I don't understand how multiple entries of opp-supported-hw
> > > are supposed to work.
> >
> > If I got this correctly, multiple cells in opp-supported-hw are to
> > describe various levels of hierarchy for a chip version, so like silicon
> > mask, metal layer revision, bin, I guess? The binding doc speaks of "cuts,
> > substrate and process", not really sure what that means exactly.
>
> Right. That basically translates to hardware versions the OPP will be parsed
> for.
>
> > I think currently we cannot easily combine microvolt suffixes and
> > opp-supported-hw in one OPP node?
>
> It should be fine.
You are of course right, that works. I think I tried without
opp-supported-hw before, and then the code doesn't like missing voltage
lines.
>
> > I think it bails out if one
> > microvolt-speed<x> property is missing, but I have to double check.
> > But IIRC v1 of this series somehow pulled that off, so we can maybe bring
> > it back? To end up with:
> > opp-792 {
> > opp-hz = <792000000>;
> > opp-microvolt-speed1 = <900000>;
> > opp-microvolt-speed4 = <940000>;
> > opp-supported-hw = <0x12>;
> > };
>
> That's what I thought too while reading your email.. Just populate the OPP for
> both 0x10 and 0x02 versions and let the speedN thing get you the right voltage.
Yes, that works nicely. I adjusted the binding example and the actual OPP
table accordingly. Will send a v3 shortly.
Cheers,
Andre
