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 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) { here instead? Or drop the pointer at all, and just use one u32 value, see the above prototype. 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, > + }, > {} > }; >
