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

[Andre Przywara <andre.przywara@arm.com>]

On Thu, 21 Mar 2024 08:39:23 +0530
Viresh Kumar <viresh.kumar@linaro.org> 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@kernel.org> wrote:  
> > > On Mon, Mar 18, 2024 at 01:12:23AM +0000, Andre Przywara wrote:  
> > > > From: Martin Botka <martin.botka@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.

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