Hi everyone,
This series is an attempt to get feedback on decoding D1 efuse speed bins
in the Sun50i H6 cpufreq driver, and turning the result into a meaningful
value that selects voltage ranges in an OPP table.
I want to make sure I get this right before sending in a v3 of the D1
cpufreq support series here
https://lore.kernel.org/linux-sunxi/[email protected]/T/#t
which is currently stuck at
https://lore.kernel.org/linux-sunxi/[email protected]/
Changes in v2:
- Make speed bin decoding generic in one patch and add D1 support in a
separate patch
- Fix OPP voltage ranges to avoid stability issues
Brandon Cheo Fusi (3):
cpufreq: sun50i: Refactor speed bin decoding
cpufreq: sun50i: Add support for D1's speed bin decoding
riscv: dts: allwinner: Fill in OPPs
arch/riscv/boot/dts/allwinner/sun20i-d1s.dtsi | 19 +++-
drivers/cpufreq/sun50i-cpufreq-nvmem.c | 89 +++++++++++++++----
2 files changed, 89 insertions(+), 19 deletions(-)
--
2.30.2
Make converting the speed bin value into a speed grade generic
and determined by a platform specific callback.
Signed-off-by: Brandon Cheo Fusi <[email protected]>
---
drivers/cpufreq/sun50i-cpufreq-nvmem.c | 55 ++++++++++++++++++--------
1 file changed, 39 insertions(+), 16 deletions(-)
diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
index 32a9c88f8..fc509fc49 100644
--- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
+++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
@@ -25,6 +25,38 @@
static struct platform_device *cpufreq_dt_pdev, *sun50i_cpufreq_pdev;
+struct sunxi_cpufreq_data {
+ u32 (*efuse_xlate)(u32 *speedbin, size_t len);
+};
+
+static u32 sun50i_efuse_xlate(u32 *speedbin, size_t len)
+{
+ u32 efuse_value = 0;
+
+ efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
+
+ /*
+ * We treat unexpected efuse values as if the SoC was from
+ * the slowest bin. Expected efuse values are 1-3, slowest
+ * to fastest.
+ */
+ if (efuse_value >= 1 && efuse_value <= 3)
+ return efuse_value - 1;
+ else
+ return 0;
+}
+
+struct sunxi_cpufreq_data sun50i_cpufreq_data = {
+ .efuse_xlate = sun50i_efuse_xlate,
+};
+
+static const struct of_device_id cpu_opp_match_list[] = {
+ { .compatible = "allwinner,sun50i-h6-operating-points",
+ .data = &sun50i_cpufreq_data,
+ },
+ {}
+};
+
/**
* sun50i_cpufreq_get_efuse() - Determine speed grade from efuse value
* @versions: Set to the value parsed from efuse
@@ -36,9 +68,10 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
struct nvmem_cell *speedbin_nvmem;
struct device_node *np;
struct device *cpu_dev;
- u32 *speedbin, efuse_value;
+ const struct of_device_id *match;
+ const struct sunxi_cpufreq_data *opp_data;
+ u32 *speedbin;
size_t len;
- int ret;
cpu_dev = get_cpu_device(0);
if (!cpu_dev)
@@ -48,12 +81,12 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
if (!np)
return -ENOENT;
- ret = of_device_is_compatible(np,
- "allwinner,sun50i-h6-operating-points");
- if (!ret) {
+ match = of_match_node(cpu_opp_match_list, np);
+ if (!match) {
of_node_put(np);
return -ENOENT;
}
+ opp_data = match->data;
speedbin_nvmem = of_nvmem_cell_get(np, NULL);
of_node_put(np);
@@ -66,17 +99,7 @@ static int sun50i_cpufreq_get_efuse(u32 *versions)
if (IS_ERR(speedbin))
return PTR_ERR(speedbin);
- efuse_value = (*speedbin >> NVMEM_SHIFT) & NVMEM_MASK;
-
- /*
- * We treat unexpected efuse values as if the SoC was from
- * the slowest bin. Expected efuse values are 1-3, slowest
- * to fastest.
- */
- if (efuse_value >= 1 && efuse_value <= 3)
- *versions = efuse_value - 1;
- else
- *versions = 0;
+ *versions = opp_data->efuse_xlate(speedbin, len);
kfree(speedbin);
return 0;
--
2.30.2
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 <[email protected]>
---
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)
+{
+ u32 ret, efuse_value = 0;
+ int i;
+
+ for (i = 0; i < len; i++)
+ efuse_value |= ((u32)speedbin[i] << (i * 8));
+
+ switch (efuse_value) {
+ 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,
+ },
{}
};
--
2.30.2
On Thu, 21 Dec 2023 11:10:12 +0100
Brandon Cheo Fusi <[email protected]> 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 <[email protected]>
> ---
> 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,
> + },
> {}
> };
>
On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <[email protected]> wrote:
>
> On Thu, 21 Dec 2023 11:10:12 +0100
> Brandon Cheo Fusi <[email protected]> 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 <[email protected]>
> > ---
> > 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.
On Thu, 21 Dec 2023 18:11:07 +0100
Brandon Cheo Fusi <[email protected]> wrote:
Hi Brandon,
> On Thu, Dec 21, 2023 at 1:50 PM Andre Przywara <[email protected]> wrote:
> >
> > On Thu, 21 Dec 2023 11:10:12 +0100
> > Brandon Cheo Fusi <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
In the next patch you introduce the nvmem DT property, and set the length
part to "0x2". So for the D1 we will always read two bytes.
> > 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?
I believe they are stored as a little endian 16-bit integer in the fuses.
I haven't tried a BE kernel, but I think the NVMEM framework takes care of
that.
If you dump the values as returned by nvmem_cell_read(), we would know for
sure.
> > 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.
Well, I am not sure either, but neither the H6, nor the H616 or the D1
apparently really need that: they all use either 4 or 2 bytes to encode
the speed bin. And since the routines are SoC specific anyway, and the
first 32-bit word of the buffer filled by nvmem_cell_read() should always
be valid (and be it 0), I think there is little need to check that.
I ported the H616 code over, and it looks somewhat similar to the D1 (with
different numbers, though): it's (ab)using some die revision code (the
first two bytes in the SID) to derive the speed bin. The H6 had a
dedicated bin fuse.
So iff we are going to see a SoC needing to check the length, we can always
introduce that later: it's just an internal function.
But for now I'd like to keep it simple.
Cheers,
Andre
>
> > 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.
>