Re: [PATCH v5 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver
On 09-09-20, 17:51, Hector Yuan wrote:
> From: "Hector.Yuan"
>
> Add MT6779 cpufreq HW support.
>
> Signed-off-by: Hector.Yuan
> ---
> drivers/cpufreq/Kconfig.arm | 12 ++
> drivers/cpufreq/Makefile |1 +
> drivers/cpufreq/mediatek-cpufreq-hw.c | 289
> +
> 3 files changed, 302 insertions(+)
> create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index c6cbfc8..8e58c12 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -121,6 +121,18 @@ config ARM_MEDIATEK_CPUFREQ
> help
> This adds the CPUFreq driver support for MediaTek SoCs.
>
> +config ARM_MEDIATEK_CPUFREQ_HW
> + tristate "MediaTek CPUFreq HW driver"
> + depends on ARCH_MEDIATEK || COMPILE_TEST
> + default m
> + help
> + Support for the CPUFreq HW driver.
> + Some MediaTek chipsets have a HW engine to offload the steps
> + necessary for changing the frequency of the CPUs. Firmware loaded
> + in this engine exposes a programming interface to the OS.
> + The driver implements the cpufreq interface for this HW engine.
> + Say Y if you want to support CPUFreq HW.
> +
> config ARM_OMAP2PLUS_CPUFREQ
> bool "TI OMAP2+"
> depends on ARCH_OMAP2PLUS
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index f6670c4..dc1f371 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) +=
> imx6q-cpufreq.o
> obj-$(CONFIG_ARM_IMX_CPUFREQ_DT) += imx-cpufreq-dt.o
> obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o
> obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ) += mediatek-cpufreq.o
> +obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ_HW)+= mediatek-cpufreq-hw.o
> obj-$(CONFIG_MACH_MVEBU_V7) += mvebu-cpufreq.o
> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c
> b/drivers/cpufreq/mediatek-cpufreq-hw.c
> new file mode 100644
> index 000..ae4b38b
> --- /dev/null
> +++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
> @@ -0,0 +1,289 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define LUT_MAX_ENTRIES 32U
> +#define LUT_FREQ GENMASK(11, 0)
> +#define LUT_ROW_SIZE 0x4
> +
> +enum {
> + REG_LUT_TABLE,
> + REG_ENABLE,
> + REG_PERF_STATE,
> +
> + REG_ARRAY_SIZE,
> +};
> +
> +struct cpufreq_mtk {
> + struct cpufreq_frequency_table *table;
> + void __iomem *reg_bases[REG_ARRAY_SIZE];
> + cpumask_t related_cpus;
> +};
> +
> +static const u16 cpufreq_mtk_offsets[REG_ARRAY_SIZE] = {
> + [REG_LUT_TABLE] = 0x0,
> + [REG_ENABLE]= 0x84,
> + [REG_PERF_STATE]= 0x88,
> +};
> +
> +static struct cpufreq_mtk *mtk_freq_domain_map[NR_CPUS];
> +
> +static int mtk_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +unsigned int index)
> +{
> + struct cpufreq_mtk *c = policy->driver_data;
> +
> + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> + arch_set_freq_scale(policy->related_cpus,
> + policy->freq_table[index].frequency,
> + policy->cpuinfo.max_freq);
Please drop the arch_set_freq_scale() stuff. This is getting moved to
cpufreq core in another series and will be done by core code for
everyone. Sorry I forgot to mention that earlier.
> +
> + return 0;
> +}
> +
> +static unsigned int mtk_cpufreq_hw_get(unsigned int cpu)
> +{
> + struct cpufreq_mtk *c;
> + struct cpufreq_policy *policy;
> + unsigned int index;
> +
> + policy = cpufreq_cpu_get_raw(cpu);
> + if (!policy)
> + return 0;
> +
> + c = policy->driver_data;
What about doing this instead ?
c = mtk_freq_domain_map[cpu];
You won't require to get policy here, right ?
> +
> + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> + index = min(index, LUT_MAX_ENTRIES - 1);
> +
> + return policy->freq_table[index].frequency;
> +}
> +
> +static int mtk_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> + struct cpufreq_mtk *c;
> + struct device *cpu_dev;
> +
> + cpu_dev = get_cpu_device(policy->cpu);
> + if (!cpu_dev) {
Why do you need this here ? You don't use it.
> + pr_err("%s: failed to get cpu%d device\n", __func__,
> +policy->cpu);
> + return -ENODEV;
> + }
> +
> + c = mtk_freq_domain_map[policy->cpu];
> + if (!c) {
> + pr_err("No scaling support for CPU%d\n", policy->cpu);
> + return -ENODEV;
> + }
Re: [PATCH v5 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver
On Wed, 2020-09-09 at 16:59 +0530, Viresh Kumar wrote:
> On 09-09-20, 17:51, Hector Yuan wrote:
> > From: "Hector.Yuan"
> >
> > Add MT6779 cpufreq HW support.
> >
> > Signed-off-by: Hector.Yuan
> > ---
> > drivers/cpufreq/Kconfig.arm | 12 ++
> > drivers/cpufreq/Makefile |1 +
> > drivers/cpufreq/mediatek-cpufreq-hw.c | 289
> > +
> > 3 files changed, 302 insertions(+)
> > create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index c6cbfc8..8e58c12 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -121,6 +121,18 @@ config ARM_MEDIATEK_CPUFREQ
> > help
> > This adds the CPUFreq driver support for MediaTek SoCs.
> >
> > +config ARM_MEDIATEK_CPUFREQ_HW
> > + tristate "MediaTek CPUFreq HW driver"
> > + depends on ARCH_MEDIATEK || COMPILE_TEST
> > + default m
> > + help
> > + Support for the CPUFreq HW driver.
> > + Some MediaTek chipsets have a HW engine to offload the steps
> > + necessary for changing the frequency of the CPUs. Firmware loaded
> > + in this engine exposes a programming interface to the OS.
> > + The driver implements the cpufreq interface for this HW engine.
> > + Say Y if you want to support CPUFreq HW.
> > +
> > config ARM_OMAP2PLUS_CPUFREQ
> > bool "TI OMAP2+"
> > depends on ARCH_OMAP2PLUS
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index f6670c4..dc1f371 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) +=
> > imx6q-cpufreq.o
> > obj-$(CONFIG_ARM_IMX_CPUFREQ_DT) += imx-cpufreq-dt.o
> > obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o
> > obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ) += mediatek-cpufreq.o
> > +obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ_HW) += mediatek-cpufreq-hw.o
> > obj-$(CONFIG_MACH_MVEBU_V7)+= mvebu-cpufreq.o
> > obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)+= omap-cpufreq.o
> > obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> > diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c
> > b/drivers/cpufreq/mediatek-cpufreq-hw.c
> > new file mode 100644
> > index 000..ae4b38b
> > --- /dev/null
> > +++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
> > @@ -0,0 +1,289 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 MediaTek Inc.
> > + */
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define LUT_MAX_ENTRIES32U
> > +#define LUT_FREQ GENMASK(11, 0)
> > +#define LUT_ROW_SIZE 0x4
> > +
> > +enum {
> > + REG_LUT_TABLE,
> > + REG_ENABLE,
> > + REG_PERF_STATE,
> > +
> > + REG_ARRAY_SIZE,
> > +};
> > +
> > +struct cpufreq_mtk {
> > + struct cpufreq_frequency_table *table;
> > + void __iomem *reg_bases[REG_ARRAY_SIZE];
> > + cpumask_t related_cpus;
> > +};
> > +
> > +static const u16 cpufreq_mtk_offsets[REG_ARRAY_SIZE] = {
> > + [REG_LUT_TABLE] = 0x0,
> > + [REG_ENABLE]= 0x84,
> > + [REG_PERF_STATE]= 0x88,
> > +};
> > +
> > +static struct cpufreq_mtk *mtk_freq_domain_map[NR_CPUS];
> > +
> > +static int mtk_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> > + unsigned int index)
> > +{
> > + struct cpufreq_mtk *c = policy->driver_data;
> > +
> > + writel_relaxed(index, c->reg_bases[REG_PERF_STATE]);
> > + arch_set_freq_scale(policy->related_cpus,
> > + policy->freq_table[index].frequency,
> > + policy->cpuinfo.max_freq);
>
> Please drop the arch_set_freq_scale() stuff. This is getting moved to
> cpufreq core in another series and will be done by core code for
> everyone. Sorry I forgot to mention that earlier.
>
OK, will remove it, Thank you.
> > +
> > + return 0;
> > +}
> > +
> > +static unsigned int mtk_cpufreq_hw_get(unsigned int cpu)
> > +{
> > + struct cpufreq_mtk *c;
> > + struct cpufreq_policy *policy;
> > + unsigned int index;
> > +
> > + policy = cpufreq_cpu_get_raw(cpu);
> > + if (!policy)
> > + return 0;
> > +
> > + c = policy->driver_data;
>
> What about doing this instead ?
>
> c = mtk_freq_domain_map[cpu];
>
> You won't require to get policy here, right ?
>
Yes, will get c from platform data.
> > +
> > + index = readl_relaxed(c->reg_bases[REG_PERF_STATE]);
> > + index = min(index, LUT_MAX_ENTRIES - 1);
> > +
> > + return policy->freq_table[index].frequency;
> > +}
> > +
> > +static int mtk_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > +{
> > + struct cpufreq_mtk *c;
> > + struct device *cpu_dev;
> > +
> > + cpu_dev = get_cpu_device(policy->cpu);
> > + if (!cpu_dev) {
>
> Why do you need this here ? You don't use it
