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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231221171107.85991-1-fusibrandon13@gmail.com>
Date: Thu, 21 Dec 2023 18:11:07 +0100
From: Brandon Cheo Fusi <fusibrandon13@...il.com>
To: andre.przywara@....com
Cc: aou@...s.berkeley.edu,
	conor+dt@...nel.org,
	devicetree@...r.kernel.org,
	fusibrandon13@...il.com,
	jernej.skrabec@...il.com,
	krzysztof.kozlowski+dt@...aro.org,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org,
	linux-pm@...r.kernel.org,
	linux-riscv@...ts.infradead.org,
	linux-sunxi@...ts.linux.dev,
	palmer@...belt.com,
	paul.walmsley@...ive.com,
	rafael@...nel.org,
	robh+dt@...nel.org,
	samuel@...lland.org,
	sfr@...b.auug.org.au,
	tiny.windzz@...il.com,
	viresh.kumar@...aro.org,
	wens@...e.org
Subject: Re: [RFC PATCH v2 2/3] cpufreq: sun50i: Add support for D1's speed bin decoding

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