[PATCH v2 3/8] dt-bindings: opp: Describe H616 OPPs and opp-supported-hw

[Viresh Kumar]

On 20-03-24, 15:37, Andre Przywara wrote:
> On Wed, 20 Mar 2024 10:02:28 -0500
> Rob Herring <robh at kernel.org> wrote:
> > On Mon, Mar 18, 2024 at 01:12:23AM +0000, Andre Przywara wrote:
> > > From: Martin Botka <martin.botka at somainline.org>
> > > -        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.

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

-- 
viresh