| Message ID | 20240326114743.712167-7-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | viresh kumar |
| Headers | show |
| Series | cpufreq: sun50i: Add Allwinner H616 support | expand |
Hi Andre, On 3/26/24 06:47, Andre Przywara wrote: > From: Martin Botka <martin.botka@somainline.org> > > 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@somainline.org> > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > 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. Regards, Samuel > + u32 value = 0; > + > + switch (speedbin & 0xffff) { > + case 0x2000: > + value = 0; > + break; > + case 0x2400: > + case 0x7400: > + case 0x2c00: > + case 0x7c00: > + if (ver_bits != SMCCC_RET_NOT_SUPPORTED && ver_bits <= 1) { > + /* ic version A/B */ > + value = 1; > + } else { > + /* ic version C and later version */ > + value = 2; > + } > + break; > + case 0x5000: > + case 0x5400: > + case 0x6000: > + value = 3; > + break; > + case 0x5c00: > + value = 4; > + break; > + case 0x5d00: > + value = 0; > + break; > + case 0x6c00: > + value = 5; > + break; > + default: > + pr_warn("sun50i-cpufreq-nvmem: unknown speed bin 0x%x, using default bin 0\n", > + speedbin & 0xffff); > + value = 0; > + break; > + } > + > + return value; > +} > + > static struct sunxi_cpufreq_data sun50i_h6_cpufreq_data = { > .efuse_xlate = sun50i_h6_efuse_xlate, > }; > > +static struct sunxi_cpufreq_data sun50i_h616_cpufreq_data = { > + .efuse_xlate = sun50i_h616_efuse_xlate, > +}; > + > static const struct of_device_id cpu_opp_match_list[] = { > { .compatible = "allwinner,sun50i-h6-operating-points", > .data = &sun50i_h6_cpufreq_data, > }, > + { .compatible = "allwinner,sun50i-h616-operating-points", > + .data = &sun50i_h616_cpufreq_data, > + }, > {} > }; > > @@ -230,6 +288,9 @@ static struct platform_driver sun50i_cpufreq_driver = { > > static const struct of_device_id sun50i_cpufreq_match_list[] = { > { .compatible = "allwinner,sun50i-h6" }, > + { .compatible = "allwinner,sun50i-h616" }, > + { .compatible = "allwinner,sun50i-h618" }, > + { .compatible = "allwinner,sun50i-h700" }, > {} > }; > MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);
On Tue, 26 Mar 2024 22:46:27 -0500 Samuel Holland <samuel@sholland.org> wrote: Hi Samuel, > On 3/26/24 06:47, Andre Przywara wrote: > > From: Martin Botka <martin.botka@somainline.org> > > > > 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@somainline.org> > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > --- > > 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. 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? Cheers, Andre > > Regards, > Samuel > > > + u32 value = 0; > > + > > + switch (speedbin & 0xffff) { > > + case 0x2000: > > + value = 0; > > + break; > > + case 0x2400: > > + case 0x7400: > > + case 0x2c00: > > + case 0x7c00: > > + if (ver_bits != SMCCC_RET_NOT_SUPPORTED && > > ver_bits <= 1) { > > + /* ic version A/B */ > > + value = 1; > > + } else { > > + /* ic version C and later version */ > > + value = 2; > > + } > > + break; > > + case 0x5000: > > + case 0x5400: > > + case 0x6000: > > + value = 3; > > + break; > > + case 0x5c00: > > + value = 4; > > + break; > > + case 0x5d00: > > + value = 0; > > + break; > > + case 0x6c00: > > + value = 5; > > + break; > > + default: > > + pr_warn("sun50i-cpufreq-nvmem: unknown speed bin > > 0x%x, using default bin 0\n", > > + speedbin & 0xffff); > > + value = 0; > > + break; > > + } > > + > > + return value; > > +} > > + > > static struct sunxi_cpufreq_data sun50i_h6_cpufreq_data = { > > .efuse_xlate = sun50i_h6_efuse_xlate, > > }; > > > > +static struct sunxi_cpufreq_data sun50i_h616_cpufreq_data = { > > + .efuse_xlate = sun50i_h616_efuse_xlate, > > +}; > > + > > static const struct of_device_id cpu_opp_match_list[] = { > > { .compatible = "allwinner,sun50i-h6-operating-points", > > .data = &sun50i_h6_cpufreq_data, > > }, > > + { .compatible = "allwinner,sun50i-h616-operating-points", > > + .data = &sun50i_h616_cpufreq_data, > > + }, > > {} > > }; > > > > @@ -230,6 +288,9 @@ static struct platform_driver > > sun50i_cpufreq_driver = { > > static const struct of_device_id sun50i_cpufreq_match_list[] = { > > { .compatible = "allwinner,sun50i-h6" }, > > + { .compatible = "allwinner,sun50i-h616" }, > > + { .compatible = "allwinner,sun50i-h618" }, > > + { .compatible = "allwinner,sun50i-h700" }, > > {} > > }; > > MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list); > >
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@sholland.org> wrote: > > Hi Samuel, > > > On 3/26/24 06:47, Andre Przywara wrote: > > > From: Martin Botka <martin.botka@somainline.org> > > > > > > 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@somainline.org> > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > --- > > > 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 ? > 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 ?
On Wed, Mar 27, 2024 at 11:58:12AM +0000, Sudeep Holla wrote: > 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@sholland.org> wrote: > > > > Hi Samuel, > > > > > On 3/26/24 06:47, Andre Przywara wrote: > > > > From: Martin Botka <martin.botka@somainline.org> > > > > > > > > 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@somainline.org> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > 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 ? Ignore this as this will block the ARM(32) build of the driver which I suppose is needed as well. -- Regards, Sudeep
On Wed, 27 Mar 2024 11:58:12 +0000 Sudeep Holla <sudeep.holla@arm.com> 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@sholland.org> wrote: > > > > Hi Samuel, > > > > > On 3/26/24 06:47, Andre Przywara wrote: > > > > From: Martin Botka <martin.botka@somainline.org> > > > > > > > > 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@somainline.org> > > > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > > > > --- > > > > 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@gmail.com/T/#t
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(); + u32 value = 0; + + switch (speedbin & 0xffff) { + case 0x2000: + value = 0; + break; + case 0x2400: + case 0x7400: + case 0x2c00: + case 0x7c00: + if (ver_bits != SMCCC_RET_NOT_SUPPORTED && ver_bits <= 1) { + /* ic version A/B */ + value = 1; + } else { + /* ic version C and later version */ + value = 2; + } + break; + case 0x5000: + case 0x5400: + case 0x6000: + value = 3; + break; + case 0x5c00: + value = 4; + break; + case 0x5d00: + value = 0; + break; + case 0x6c00: + value = 5; + break; + default: + pr_warn("sun50i-cpufreq-nvmem: unknown speed bin 0x%x, using default bin 0\n", + speedbin & 0xffff); + value = 0; + break; + } + + return value; +} + static struct sunxi_cpufreq_data sun50i_h6_cpufreq_data = { .efuse_xlate = sun50i_h6_efuse_xlate, }; +static struct sunxi_cpufreq_data sun50i_h616_cpufreq_data = { + .efuse_xlate = sun50i_h616_efuse_xlate, +}; + static const struct of_device_id cpu_opp_match_list[] = { { .compatible = "allwinner,sun50i-h6-operating-points", .data = &sun50i_h6_cpufreq_data, }, + { .compatible = "allwinner,sun50i-h616-operating-points", + .data = &sun50i_h616_cpufreq_data, + }, {} }; @@ -230,6 +288,9 @@ static struct platform_driver sun50i_cpufreq_driver = { static const struct of_device_id sun50i_cpufreq_match_list[] = { { .compatible = "allwinner,sun50i-h6" }, + { .compatible = "allwinner,sun50i-h616" }, + { .compatible = "allwinner,sun50i-h618" }, + { .compatible = "allwinner,sun50i-h700" }, {} }; MODULE_DEVICE_TABLE(of, sun50i_cpufreq_match_list);