Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

[Andre Przywara]

On Thu, 21 Dec 2023 18:11:07 +0100
Brandon Cheo Fusi <fusibrandon13@xxxxxxxxx> wrote:

Hi Brandon,

> On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@xxxxxxx> wrote:
> >
> > On Thu, 21 Dec 2023 11:10:12 +0100
> > Brandon Cheo Fusi <fusibrandon13@xxxxxxxxx> wrote:
> >
> > Hi Brandon,
> >
> > thanks for the quick turnaround, and for splitting this code up, that
> > makes reasoning about this much easier!
> >  
> > > Adds support for decoding the efuse value read from D1 efuse speed
> > > bins, and factors out equivalent code for sun50i.
> > >
> > > The algorithm is gotten from
> > >
> > > https://github.com/Tina-Linux/linux-5.4/blob/master/drivers/cpufreq/sun50i-cpufreq-nvmem.c#L293-L338
> > >
> > > and maps an efuse value to either 0 or 1, with 1 meaning stable at
> > > a lower supply voltage for the same clock frequency.
> > >
> > > Signed-off-by: Brandon Cheo Fusi <fusibrandon13@xxxxxxxxx>
> > > ---
> > >  drivers/cpufreq/sun50i-cpufreq-nvmem.c | 34 ++++++++++++++++++++++++++
> > >  1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > index fc509fc49..b1cb95308 100644
> > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > @@ -29,6 +29,33 @@ struct sunxi_cpufreq_data {
> > >       u32 (*efuse_xlate)(u32 *speedbin, size_t len);
> > >  };
> > >
> > > +static u32 sun20i_efuse_xlate(u32 *speedbin, size_t len)  
> >
> > I feel like this prototype can be shortened to:
> >
> > static u32 sun20i_efuse_xlate(u32 speedbin)
> >
> > See below.
> >  
> > > +{
> > > +     u32 ret, efuse_value = 0;
> > > +     int i;
> > > +
> > > +     for (i = 0; i < len; i++)
> > > +             efuse_value |= ((u32)speedbin[i] << (i * 8));  
> >
> > The cast is not needed. Looking deeper into the original code you linked
> > to, cell_value[] there is an array of u8, so they assemble a little endian
> > 32-bit integer from *up to* four 8-bit values read from the nvmem.
> >
> > So I think this code here is wrong, len is the size of the nvmem cells
> > holding the bin identifier, in *bytes*, so the idea here is to just read
> > the (lowest) 16 bits (in the D1 case, cf. "reg = <0x00 0x2>;" in the next
> > patch) from this nvmem cell. Here you are combining two 32-bit words into  
> 
> This is true. Not sure though what the 'in the D1 case...' bit means.

In the next patch you introduce the nvmem DT property, and set the length
part to "0x2". So for the D1 we will always read two bytes.

> > efuse_value.
> >
> > So I think this whole part above is actually not necessary: we are
> > expecting maximum 32 bits, and nvmem_cell_read() should take care of
> > masking off unrequested bits, so we get the correct value back already. So
> > can you try to remove the loop above, and use ...
> >  
> > > +
> > > +     switch (efuse_value) {  
> >
> >         switch (*speedbin & 0xffff) {
> >  
> 
> Shouldn't the bytes in *speedbin be reversed? 

I believe they are stored as a little endian 16-bit integer in the fuses.
I haven't tried a BE kernel, but I think the NVMEM framework takes care of
that.
If you dump the values as returned by nvmem_cell_read(), we would know for
sure.

> > here instead? Or drop the pointer at all, and just use one u32 value, see
> > the above prototype.
> >  
> 
> I was uncomfortable dropping the len parameter, because then each
> platform's efuse_xlate would ignore the number of valid bytes actually
> read.

Well, I am not sure either, but neither the H6, nor the H616 or the D1
apparently really need that: they all use either 4 or 2 bytes to encode
the speed bin. And since the routines are SoC specific anyway, and the
first 32-bit word of the buffer filled by nvmem_cell_read() should always
be valid (and be it 0), I think there is little need to check that.
I ported the H616 code over, and it looks somewhat similar to the D1 (with
different numbers, though): it's (ab)using some die revision code (the
first two bytes in the SID) to derive the speed bin. The H6 had a
dedicated bin fuse.

So iff we are going to see a SoC needing to check the length, we can always
introduce that later: it's just an internal function.
But for now I'd like to keep it simple.

Cheers,
Andre

> 
> > Cheers,
> > Andre
> >
> > P.S. This is just a "peephole review" of this patch, I haven't got around
> > to look at this whole scheme in whole yet, to see if we actually need this
> > or can simplify this or clean it up.
> >
> >  
> > > +     case 0x5e00:
> > > +             /* QFN package */
> > > +             ret = 0;
> > > +             break;
> > > +     case 0x5c00:
> > > +     case 0x7400:
> > > +             /* QFN package */
> > > +             ret = 1;
> > > +             break;
> > > +     case 0x5000:
> > > +     default:
> > > +             /* BGA package */
> > > +             ret = 0;
> > > +     }
> > > +
> > > +     return ret;
> > > +}
> > > +
> > >  static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >  {
> > >       u32 efuse_value = 0;
> > > @@ -46,6 +73,10 @@ static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
> > >               return 0;
> > >  }
> > >
> > > +struct sunxi_cpufreq_data sun20i_cpufreq_data = {
> > > +     .efuse_xlate = sun20i_efuse_xlate,
> > > +};
> > > +
> > >  struct sunxi_cpufreq_data sun50i_cpufreq_data = {
> > >       .efuse_xlate = sun50i_efuse_xlate,
> > >  };
> > > @@ -54,6 +85,9 @@ static const struct of_device_id cpu_opp_match_list[] = {
> > >       { .compatible = "allwinner,sun50i-h6-operating-points",
> > >         .data = &sun50i_cpufreq_data,
> > >       },
> > > +     { .compatible = "allwinner,sun20i-d1-operating-points",
> > > +       .data = &sun20i_cpufreq_data,
> > > +     },
> > >       {}
> > >  };
> > >  
> >  
> 
> Thank you for reviewing.
> Brandon.
>