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

[Andre Przywara <andre.przywara@arm.com>]

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
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,
> +	},
>  	{}
>  };
>