Hi Matthias,
Thanks for the feedback.
On Fri, 2020-05-22 at 17:38 +0200, Matthias Brugger wrote:
>
> On 22/05/2020 11:40, Roger Lu wrote:
> >
> > Hi Enric,
> >
> > On Tue, 2020-05-19 at 17:30 +0200, Enric Balletbo Serra wrote:
> >> Hi Roger,
> >>
> >> Thank you for your patch. I have the feeling that this driver is
> >> complex and difficult to follow and I am wondering if it wouldn't be
> >> better if you can send a version that simply adds basic functionality
> >> for now. Some comments below.
> >
> > Thanks for the advices. I'll submit SVS v9 with basic functionality
> > patch + step by step functionalities' patches.
> >
> >>
> >> Missatge de Roger Lu <roger.lu@xxxxxxxxxxxx> del dia dl., 18 de maig
> >> 2020 a les 11:25:
> >>>
> >>> The SVS (Smart Voltage Scaling) engine is a piece
> >>> of hardware which is used to calculate optimized
> >>> voltage values of several power domains,
> >>> e.g. CPU/GPU/CCI, according to chip process corner,
> >>> temperatures, and other factors. Then DVFS driver
> >>> could apply those optimized voltage values to reduce
> >>> power consumption.
> >>>
> >>> Signed-off-by: Roger Lu <roger.lu@xxxxxxxxxxxx>
> >>> ---
> >>> drivers/power/avs/Kconfig | 10 +
> >>> drivers/power/avs/Makefile | 1 +
> >>> drivers/power/avs/mtk_svs.c | 2119 +++++++++++++++++++++++++++++++++
> >>> include/linux/power/mtk_svs.h | 23 +
> >>> 4 files changed, 2153 insertions(+)
> >>> create mode 100644 drivers/power/avs/mtk_svs.c
> >>> create mode 100644 include/linux/power/mtk_svs.h
> >>>
> >>> diff --git a/drivers/power/avs/Kconfig b/drivers/power/avs/Kconfig
> >>> index cdb4237bfd02..67089ac6040e 100644
> >>> --- a/drivers/power/avs/Kconfig
> >>> +++ b/drivers/power/avs/Kconfig
> >>> @@ -35,3 +35,13 @@ config ROCKCHIP_IODOMAIN
> >>> Say y here to enable support io domains on Rockchip SoCs. It is
> >>> necessary for the io domain setting of the SoC to match the
> >>> voltage supplied by the regulators.
> >>> +
> >>> +config MTK_SVS
> >>> + bool "MediaTek Smart Voltage Scaling(SVS)"
> >>
> >> Can't be this a module? Why? In such case, you should use tristate option
> >
> > Generally, MTK_SVS is needed in MTK SoC(mt81xx) products. So, we don't provide
> > module option in config. If, somehow, SVS isn't needed, we suggest
> > CONFIG_MTK_SVS=n to be set.
> >
>
> The question here is if it needs to be probed before we probe the modules. If
> not, we should add a Kconfig option for MT81xx SoCs to select MTK_SVS.
Excuse me to make you confuse. MT81xx SoCs is the subset MTK ICs that
will use CONFIG_MTK_SVS. In other words, CONFIG_MTK_SVS will be used
with other MTK ICs as well. So, MTK_SVS is the general naming for MTK IC
to enable SVS power feature. Anyway, back to Enric's question, I'll make
MTK_SVS become a tristate feature in the next patch. Thanks.
>
> >>
> >>> + depends on POWER_AVS && MTK_EFUSE && NVMEM
> >>> + help
> >>> + The SVS engine is a piece of hardware which is used to calculate
> >>> + optimized voltage values of several power domains, e.g.
> >>> + CPU clusters/GPU/CCI, according to chip process corner, temperatures,
> >>> + and other factors. Then DVFS driver could apply those optimized voltage
> >>> + values to reduce power consumption.
> >>> diff --git a/drivers/power/avs/Makefile b/drivers/power/avs/Makefile
> >>> index 9007d05853e2..231adf078582 100644
> >>> --- a/drivers/power/avs/Makefile
> >>> +++ b/drivers/power/avs/Makefile
> >>> @@ -2,3 +2,4 @@
> >>> obj-$(CONFIG_POWER_AVS_OMAP) += smartreflex.o
> >>> obj-$(CONFIG_QCOM_CPR) += qcom-cpr.o
> >>> obj-$(CONFIG_ROCKCHIP_IODOMAIN) += rockchip-io-domain.o
> >>> +obj-$(CONFIG_MTK_SVS) += mtk_svs.o
> >>
> >> Will this driver be SoC specific or the idea is to support different
> >> SoCs? If the answer to the first question is yes, please name the file
> >> with the SoC prefix (i.e mt8183_svs). However, If the answer to the
> >> second question is yes, make sure you prefix common
> >> functions/structs/defines with a generic prefix mtk_svs but use the
> >> SoC prefix for the ones you expect will be different between SoC, i.e
> >> mt8183_svs_. This helps the readability of the driver. Also, try to
> >> avoid too generic names.
> >
> > MTK_SVS is designed for supporting different MTK SoCs.Therefore, the answer is second
> > question and thanks for the heads-up.
> >
> >>
> >>> diff --git a/drivers/power/avs/mtk_svs.c b/drivers/power/avs/mtk_svs.c
> >>> new file mode 100644
> >>> index 000000000000..a4083b3ef175
> >>> --- /dev/null
> >>> +++ b/drivers/power/avs/mtk_svs.c
> >>> @@ -0,0 +1,2119 @@
> >>> +// SPDX-License-Identifier: GPL-2.0
> >>
> >> I suspect you want this only GPLv2 compliant. Use GPL-2.0-only
> >
> > OK. I'll use GPL-2.0-only.Thanks.
> >
> >>
> >>> +/*
> >>> + * Copyright (C) 2020 MediaTek Inc.
> >>> + */
> >>> +
> >>> +#define pr_fmt(fmt) "[mtk_svs] " fmt
> >>
> >> I don't see any reason to use pr_fmt in this driver. Use dev_*
> >> functions instead and remove the above.
> >
> > Ok. I will remove it. Thanks.
> >
> >>
> >>> +
> >>> +#include <linux/bits.h>
> >>> +#include <linux/clk.h>
> >>> +#include <linux/completion.h>
> >>> +#include <linux/init.h>
> >>> +#include <linux/interrupt.h>
> >>> +#include <linux/kernel.h>
> >>> +#include <linux/kthread.h>
> >>> +#include <linux/module.h>
> >>> +#include <linux/mutex.h>
> >>> +#include <linux/nvmem-consumer.h>
> >>> +#include <linux/of_address.h>
> >>> +#include <linux/of_irq.h>
> >>> +#include <linux/of_platform.h>
> >>> +#include <linux/platform_device.h>
> >>> +#include <linux/pm_domain.h>
> >>> +#include <linux/pm_opp.h>
> >>> +#include <linux/pm_qos.h>
> >>> +#include <linux/pm_runtime.h>
> >>> +#include <linux/power/mtk_svs.h>
> >>> +#include <linux/proc_fs.h>
> >>> +#include <linux/regulator/consumer.h>
> >>> +#include <linux/reset.h>
> >>> +#include <linux/seq_file.h>
> >>> +#include <linux/slab.h>
> >>> +#include <linux/spinlock.h>
> >>> +#include <linux/thermal.h>
> >>> +#include <linux/uaccess.h>
> >>> +
> >>> +/* svs 1-line sw id */
> >>> +#define SVS_CPU_LITTLE BIT(0)
> >>> +#define SVS_CPU_BIG BIT(1)
> >>> +#define SVS_CCI BIT(2)
> >>> +#define SVS_GPU BIT(3)
> >>> +
> >>> +/* svs bank mode support */
> >>> +#define SVSB_MODE_ALL_DISABLE (0)
> >>
> >> nit: SVS_BMODE_?
> >
> > Oh. If we add bank wording like SVS_Bxxx, it might cause some confusion when B combines
> > with other words. So, I'll keep SVSB for SVS Bank representation.
> > E.g: SVS_BDC_SIGNED_BIT might lead to be explained differently ("SVS bank + DC_SIGNED_BIT" or "SVS + BDC_SIGNED_BIT")
> > - "SVS bank + DC_SIGNED_BIT" is what we want for naming SVS_BDC_SIGNED_BIT but it might be misunderstood.
> >
> >>
> >>> +#define SVSB_MODE_INIT01 BIT(1)
> >>> +#define SVSB_MODE_INIT02 BIT(2)
> >>> +#define SVSB_MODE_MON BIT(3)
> >>> +
> >>> +/* svs bank init01 condition */
> >>> +#define SVSB_INIT01_VOLT_IGNORE BIT(1)
> >>> +#define SVSB_INIT01_VOLT_INC_ONLY BIT(2)
> >>> +
> >>> +/* svs bank common setting */
> >>> +#define HIGH_TEMP_MAX (U32_MAX)
> >>
> >> nit: SVS_*
> >
> > ok. I will add SVS or SVSB when it refers to SVS BANK.
> >
> >>
> >>> +#define RUNCONFIG_DEFAULT (0x80000000)
> >>
> >> Btw, there is any public datasheet where I can see those addresses and
> >> registers and bit fields?
> >
> > Excuse us, there is no public datasheet. We can reply it on patchwork. Thanks.
> >
> >>
> >>> +#define DC_SIGNED_BIT (0x8000)
> >>> +#define INTEN_INIT0x (0x00005f01)
> >>> +#define INTEN_MONVOPEN (0x00ff0000)
> >>> +#define SVSEN_OFF (0x0)
> >>> +#define SVSEN_MASK (0x7)
> >>> +#define SVSEN_INIT01 (0x1)
> >>> +#define SVSEN_INIT02 (0x5)
> >>> +#define SVSEN_MON (0x2)
> >>> +#define INTSTS_MONVOP (0x00ff0000)
> >>> +#define INTSTS_COMPLETE (0x1)
> >>> +#define INTSTS_CLEAN (0x00ffffff)
> >>> +
> >>> +#define proc_fops_rw(name) \
> >>> + static int name ## _proc_open(struct inode *inode, \
> >>> + struct file *file) \
> >>> + { \
> >>> + return single_open(file, name ## _proc_show, \
> >>> + PDE_DATA(inode)); \
> >>> + } \
> >>> + static const struct proc_ops name ## _proc_fops = { \
> >>> + .proc_open = name ## _proc_open, \
> >>> + .proc_read = seq_read, \
> >>> + .proc_lseek = seq_lseek, \
> >>> + .proc_release = single_release, \
> >>> + .proc_write = name ## _proc_write, \
> >>> + }
> >>> +
> >>> +#define proc_fops_ro(name) \
> >>> + static int name ## _proc_open(struct inode *inode, \
> >>> + struct file *file) \
> >>> + { \
> >>> + return single_open(file, name ## _proc_show, \
> >>> + PDE_DATA(inode)); \
> >>> + } \
> >>> + static const struct proc_ops name ## _proc_fops = { \
> >>> + .proc_open = name ## _proc_open, \
> >>> + .proc_read = seq_read, \
> >>> + .proc_lseek = seq_lseek, \
> >>> + .proc_release = single_release, \
> >>> + }
> >>> +
> >>> +#define proc_entry(name) {__stringify(name), &name ## _proc_fops}
> >>> +
> >>
> >> /proc is usually the old way of exporting files to userspace, so
> >> unless you have a really good reason use sysfs instead, or even
> >> better, if it is only for debug purposes use debugfs. Also, you should
> >> document the entries in Documentation.
> >
> > Ok. I'll change it to debugfs and could you give us an example about entries in documentation?
> > We can follow them. Thanks.
> >
> >>
> >>> +static DEFINE_SPINLOCK(mtk_svs_lock);
> >>> +struct mtk_svs;
> >>> +
> >>> +enum svsb_phase {
> >>
> >> nit: mtk_svs_bphase?
> >
> > ditto
> >
> >>
> >>> + SVSB_PHASE_INIT01 = 0,
> >>
> >> nit: SVS_BPHASE_?
> >
> > ditto
> >
> >>
> >>> + SVSB_PHASE_INIT02,
> >>> + SVSB_PHASE_MON,
> >>> + SVSB_PHASE_ERROR,
> >>> +};
> >>> +
> >>> +enum reg_index {
> >>
> >> nit: svs_reg_index?
> >
> > OK. Thanks.
> >
> >>
> >>> + TEMPMONCTL0 = 0,
> >>> + TEMPMONCTL1,
> >>> + TEMPMONCTL2,
> >>> + TEMPMONINT,
> >>> + TEMPMONINTSTS,
> >>> + TEMPMONIDET0,
> >>> + TEMPMONIDET1,
> >>> + TEMPMONIDET2,
> >>> + TEMPH2NTHRE,
> >>> + TEMPHTHRE,
> >>> + TEMPCTHRE,
> >>> + TEMPOFFSETH,
> >>> + TEMPOFFSETL,
> >>> + TEMPMSRCTL0,
> >>> + TEMPMSRCTL1,
> >>> + TEMPAHBPOLL,
> >>> + TEMPAHBTO,
> >>> + TEMPADCPNP0,
> >>> + TEMPADCPNP1,
> >>> + TEMPADCPNP2,
> >>> + TEMPADCMUX,
> >>> + TEMPADCEXT,
> >>> + TEMPADCEXT1,
> >>> + TEMPADCEN,
> >>> + TEMPPNPMUXADDR,
> >>> + TEMPADCMUXADDR,
> >>> + TEMPADCEXTADDR,
> >>> + TEMPADCEXT1ADDR,
> >>> + TEMPADCENADDR,
> >>> + TEMPADCVALIDADDR,
> >>> + TEMPADCVOLTADDR,
> >>> + TEMPRDCTRL,
> >>> + TEMPADCVALIDMASK,
> >>> + TEMPADCVOLTAGESHIFT,
> >>> + TEMPADCWRITECTRL,
> >>> + TEMPMSR0,
> >>> + TEMPMSR1,
> >>> + TEMPMSR2,
> >>> + TEMPADCHADDR,
> >>> + TEMPIMMD0,
> >>> + TEMPIMMD1,
> >>> + TEMPIMMD2,
> >>> + TEMPMONIDET3,
> >>> + TEMPADCPNP3,
> >>> + TEMPMSR3,
> >>> + TEMPIMMD3,
> >>> + TEMPPROTCTL,
> >>> + TEMPPROTTA,
> >>> + TEMPPROTTB,
> >>> + TEMPPROTTC,
> >>> + TEMPSPARE0,
> >>> + TEMPSPARE1,
> >>> + TEMPSPARE2,
> >>> + TEMPSPARE3,
> >>> + TEMPMSR0_1,
> >>> + TEMPMSR1_1,
> >>> + TEMPMSR2_1,
> >>> + TEMPMSR3_1,
> >>> + DESCHAR,
> >>> + TEMPCHAR,
> >>> + DETCHAR,
> >>> + AGECHAR,
> >>> + DCCONFIG,
> >>> + AGECONFIG,
> >>> + FREQPCT30,
> >>> + FREQPCT74,
> >>> + LIMITVALS,
> >>> + VBOOT,
> >>> + DETWINDOW,
> >>> + CONFIG,
> >>> + TSCALCS,
> >>> + RUNCONFIG,
> >>> + SVSEN,
> >>> + INIT2VALS,
> >>> + DCVALUES,
> >>> + AGEVALUES,
> >>> + VOP30,
> >>> + VOP74,
> >>> + TEMP,
> >>> + INTSTS,
> >>> + INTSTSRAW,
> >>> + INTEN,
> >>> + CHKINT,
> >>> + CHKSHIFT,
> >>> + STATUS,
> >>> + VDESIGN30,
> >>> + VDESIGN74,
> >>> + DVT30,
> >>> + DVT74,
> >>> + AGECOUNT,
> >>> + SMSTATE0,
> >>> + SMSTATE1,
> >>> + CTL0,
> >>> + DESDETSEC,
> >>> + TEMPAGESEC,
> >>> + CTRLSPARE0,
> >>> + CTRLSPARE1,
> >>> + CTRLSPARE2,
> >>> + CTRLSPARE3,
> >>> + CORESEL,
> >>> + THERMINTST,
> >>> + INTST,
> >>> + THSTAGE0ST,
> >>> + THSTAGE1ST,
> >>> + THSTAGE2ST,
> >>> + THAHBST0,
> >>> + THAHBST1,
> >>> + SPARE0,
> >>> + SPARE1,
> >>> + SPARE2,
> >>> + SPARE3,
> >>> + THSLPEVEB,
> >>> + reg_num,
> >>> +};
> >>> +
> >>> +static const u32 svs_regs_v2[] = {
> >>
> >> Is this SoC specific or shared between SoCs?
> >
> > Shared between SoCs. Some SVS in MTK SoCs use v2 register map.
> >
>
> And which silicon uses v1 then? Is v2 a MediaTek internal naming you want to keep?
1. MT8173 IC uses v1 register map.
2. Yes, I'll keep v2 postfix.
>
> >>
> >>> + [TEMPMONCTL0] = 0x000,
> >>> + [TEMPMONCTL1] = 0x004,
> >>> + [TEMPMONCTL2] = 0x008,
> >>> + [TEMPMONINT] = 0x00c,
> >>> + [TEMPMONINTSTS] = 0x010,
> >>> + [TEMPMONIDET0] = 0x014,
> >>> + [TEMPMONIDET1] = 0x018,
> >>> + [TEMPMONIDET2] = 0x01c,
> >>> + [TEMPH2NTHRE] = 0x024,
> >>> + [TEMPHTHRE] = 0x028,
> >>> + [TEMPCTHRE] = 0x02c,
> >>> + [TEMPOFFSETH] = 0x030,
> >>> + [TEMPOFFSETL] = 0x034,
> >>> + [TEMPMSRCTL0] = 0x038,
> >>> + [TEMPMSRCTL1] = 0x03c,
> >>> + [TEMPAHBPOLL] = 0x040,
> >>> + [TEMPAHBTO] = 0x044,
> >>> + [TEMPADCPNP0] = 0x048,
> >>> + [TEMPADCPNP1] = 0x04c,
> >>> + [TEMPADCPNP2] = 0x050,
> >>> + [TEMPADCMUX] = 0x054,
> >>> + [TEMPADCEXT] = 0x058,
> >>> + [TEMPADCEXT1] = 0x05c,
> >>> + [TEMPADCEN] = 0x060,
> >>> + [TEMPPNPMUXADDR] = 0x064,
> >>> + [TEMPADCMUXADDR] = 0x068,
> >>> + [TEMPADCEXTADDR] = 0x06c,
> >>> + [TEMPADCEXT1ADDR] = 0x070,
> >>> + [TEMPADCENADDR] = 0x074,
> >>> + [TEMPADCVALIDADDR] = 0x078,
> >>> + [TEMPADCVOLTADDR] = 0x07c,
> >>> + [TEMPRDCTRL] = 0x080,
> >>> + [TEMPADCVALIDMASK] = 0x084,
> >>> + [TEMPADCVOLTAGESHIFT] = 0x088,
> >>> + [TEMPADCWRITECTRL] = 0x08c,
> >>> + [TEMPMSR0] = 0x090,
> >>> + [TEMPMSR1] = 0x094,
> >>> + [TEMPMSR2] = 0x098,
> >>> + [TEMPADCHADDR] = 0x09c,
> >>> + [TEMPIMMD0] = 0x0a0,
> >>> + [TEMPIMMD1] = 0x0a4,
> >>> + [TEMPIMMD2] = 0x0a8,
> >>> + [TEMPMONIDET3] = 0x0b0,
> >>> + [TEMPADCPNP3] = 0x0b4,
> >>> + [TEMPMSR3] = 0x0b8,
> >>> + [TEMPIMMD3] = 0x0bc,
> >>> + [TEMPPROTCTL] = 0x0c0,
> >>> + [TEMPPROTTA] = 0x0c4,
> >>> + [TEMPPROTTB] = 0x0c8,
> >>> + [TEMPPROTTC] = 0x0cc,
> >>> + [TEMPSPARE0] = 0x0f0,
> >>> + [TEMPSPARE1] = 0x0f4,
> >>> + [TEMPSPARE2] = 0x0f8,
> >>> + [TEMPSPARE3] = 0x0fc,
> >>> + [TEMPMSR0_1] = 0x190,
> >>> + [TEMPMSR1_1] = 0x194,
> >>> + [TEMPMSR2_1] = 0x198,
> >>> + [TEMPMSR3_1] = 0x1b8,
> >>> + [DESCHAR] = 0xc00,
> >>> + [TEMPCHAR] = 0xc04,
> >>> + [DETCHAR] = 0xc08,
> >>> + [AGECHAR] = 0xc0c,
> >>> + [DCCONFIG] = 0xc10,
> >>> + [AGECONFIG] = 0xc14,
> >>> + [FREQPCT30] = 0xc18,
> >>> + [FREQPCT74] = 0xc1c,
> >>> + [LIMITVALS] = 0xc20,
> >>> + [VBOOT] = 0xc24,
> >>> + [DETWINDOW] = 0xc28,
> >>> + [CONFIG] = 0xc2c,
> >>> + [TSCALCS] = 0xc30,
> >>> + [RUNCONFIG] = 0xc34,
> >>> + [SVSEN] = 0xc38,
> >>> + [INIT2VALS] = 0xc3c,
> >>> + [DCVALUES] = 0xc40,
> >>> + [AGEVALUES] = 0xc44,
> >>> + [VOP30] = 0xc48,
> >>> + [VOP74] = 0xc4c,
> >>> + [TEMP] = 0xc50,
> >>> + [INTSTS] = 0xc54,
> >>> + [INTSTSRAW] = 0xc58,
> >>> + [INTEN] = 0xc5c,
> >>> + [CHKINT] = 0xc60,
> >>> + [CHKSHIFT] = 0xc64,
> >>> + [STATUS] = 0xc68,
> >>> + [VDESIGN30] = 0xc6c,
> >>> + [VDESIGN74] = 0xc70,
> >>> + [DVT30] = 0xc74,
> >>> + [DVT74] = 0xc78,
> >>> + [AGECOUNT] = 0xc7c,
> >>> + [SMSTATE0] = 0xc80,
> >>> + [SMSTATE1] = 0xc84,
> >>> + [CTL0] = 0xc88,
> >>> + [DESDETSEC] = 0xce0,
> >>> + [TEMPAGESEC] = 0xce4,
> >>> + [CTRLSPARE0] = 0xcf0,
> >>> + [CTRLSPARE1] = 0xcf4,
> >>> + [CTRLSPARE2] = 0xcf8,
> >>> + [CTRLSPARE3] = 0xcfc,
> >>> + [CORESEL] = 0xf00,
> >>> + [THERMINTST] = 0xf04,
> >>> + [INTST] = 0xf08,
> >>> + [THSTAGE0ST] = 0xf0c,
> >>> + [THSTAGE1ST] = 0xf10,
> >>> + [THSTAGE2ST] = 0xf14,
> >>> + [THAHBST0] = 0xf18,
> >>> + [THAHBST1] = 0xf1c,
> >>> + [SPARE0] = 0xf20,
> >>> + [SPARE1] = 0xf24,
> >>> + [SPARE2] = 0xf28,
> >>> + [SPARE3] = 0xf2c,
> >>> + [THSLPEVEB] = 0xf30,
> >>> +};
> >>> +
> >>> +struct thermal_parameter {
> >>
> >> In general, not only in this struct, would be good have some
> >> documentation to have a better undestanding of the fields. That makes
> >> the job of the reviewer a bit easier.
> >
> > Ok. Could you share a documentation example to us? We'll share the
> > information as much as we can. Thanks a lot.
> >
>
> you should find that in all drivers, eg:
> https://elixir.bootlin.com/linux/latest/source/drivers/soc/mediatek/mtk-scpsys.c#L111
No problem Sir. Thanks for showing a direction to me. I'll take a look
at it.
>
> Regards,
> Matthias
