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

[Brandon Cheo Fusi <fusibrandon13@gmail.com>]

On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <andre.przywara@arm.com> wrote:
>
> On Thu, 21 Dec 2023 11:10:12 +0100
> Brandon Cheo Fusi <fusibrandon13@gmail.com> 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@gmail.com>
> > ---
> >  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.

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

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

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