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