On Wed, 27 Mar 2024 11:58:12 +0000
Sudeep Holla <sudeep.holla@xxxxxxx> wrote:
Hi Sudeep,
> On Wed, Mar 27, 2024 at 11:46:08AM +0000, Andre Przywara wrote:
> > On Tue, 26 Mar 2024 22:46:27 -0500
> > Samuel Holland <samuel@xxxxxxxxxxxx> wrote:
> >
> > Hi Samuel,
> >
> > > On 3/26/24 06:47, Andre Przywara wrote:
> > > > From: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> > > >
> > > > The Allwinner H616/H618 SoCs have different OPP tables per SoC version
> > > > and die revision. The SoC version is stored in NVMEM, as before, though
> > > > encoded differently. The die revision is in a different register, in the
> > > > SRAM controller. Firmware already exports that value in a standardised
> > > > way, through the SMCCC SoCID mechanism. We need both values, as some chips
> > > > have the same SoC version, but they don't support the same frequencies and
> > > > they get differentiated by the die revision.
> > > >
> > > > Add the new compatible string and tie the new translation function to
> > > > it. This mechanism not only covers the original H616 SoC, but also its
> > > > very close sibling SoCs H618 and H700, so add them to the list as well.
> > > >
> > > > Signed-off-by: Martin Botka <martin.botka@xxxxxxxxxxxxxx>
> > > > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> > > > ---
> > > > drivers/cpufreq/sun50i-cpufreq-nvmem.c | 61 ++++++++++++++++++++++++++
> > > > 1 file changed, 61 insertions(+)
> > > >
> > > > diff --git a/drivers/cpufreq/sun50i-cpufreq-nvmem.c b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > > index bd170611c7906..f9e9fc340f848 100644
> > > > --- a/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > > +++ b/drivers/cpufreq/sun50i-cpufreq-nvmem.c
> > > > @@ -10,6 +10,7 @@
> > > >
> > > > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > >
> > > > +#include <linux/arm-smccc.h>
> > > > #include <linux/cpu.h>
> > > > #include <linux/module.h>
> > > > #include <linux/nvmem-consumer.h>
> > > > @@ -46,14 +47,71 @@ static u32 sun50i_h6_efuse_xlate(u32 speedbin)
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * Judging by the OPP tables in the vendor BSP, the quality order of the
> > > > + * returned speedbin index is 4 -> 0/2 -> 3 -> 1, from worst to best.
> > > > + * 0 and 2 seem identical from the OPP tables' point of view.
> > > > + */
> > > > +static u32 sun50i_h616_efuse_xlate(u32 speedbin)
> > > > +{
> > > > + int ver_bits = arm_smccc_get_soc_id_revision();
> > >
> > > This needs a Kconfig dependency on ARM_SMCCC_SOC_ID.
> >
> > That was my first impulse as well, but it's actually not true:
> > ARM_SMCCC_SOC_ID just protects the sysfs export code, not this function
> > here. That does just depend on HAVE_ARM_SMCCC_DISCOVERY, which gets
> > selected by ARM_GIC_V3, which gets selected by CONFIG_ARM64. So the
> > arm64 kernel is safe.
>
> It is safe to add the dependency explicitly so that if GICV3 decides to drop
> it, this won't be affected. Thoughts ?
That would be one possibility, but since there are patches on the
list[1] that use this driver for the Allwinner D1 with RISC-V cores,
this would need to be revisited then anyway.
> > Now apart from ARM(32) (where the situation seems a bit more complex) I
> > just realise that this would torpedo Brandon's D1 efforts, so I need to
> > add this change I played with to provide an alternative:
> >
> > static int get_soc_id_revision(void)
> > {
> > #ifdef CONFIG_HAVE_ARM_SMCCC_DISCOVERY
> > return arm_smccc_get_soc_id_revision();
> > #else
> > /* Return the value for the worse die revision, to be safe. */
> > return 2;
> > #endif
> > }
> >
> > Does that look acceptable, despite the #ifdef?
> >
>
> if(IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)) instead ?
Well, but this would rely on at least the prototype to be visible,
right? And then on the toolchain to optimise away the call, so that the
symbol doesn't even appear in the object file?
So would this work for the RISC-V case?
Cheers,
Andre
[1]
https://lore.kernel.org/linux-sunxi/20231218110543.64044-1-fusibrandon13@xxxxxxxxx/T/#t
