| Message ID | 20221006115816.66853-1-angelogioacchino.delregno@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,v3] soc: mediatek: Introduce mediatek-regulator-coupler driver | expand |
Reviewed-by: Alyssa Rosenzweig <alyssa@collabora.com>
Il 06/10/22 13:58, AngeloGioacchino Del Regno ha scritto: > This driver currently deals with GPU-SRAM regulator coupling, ensuring > that the SRAM voltage is always between a specific range of distance to > the GPU voltage, depending on the SoC, necessary in order to achieve > system stability across the full range of supported GPU frequencies. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > > This driver was successfully tested for more than 3 months. > GPU DVFS works correctly with no stability issues. > Hey Matthias, gentle ping for this one... Cheers, Angelo
On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: > This driver currently deals with GPU-SRAM regulator coupling, ensuring > that the SRAM voltage is always between a specific range of distance to > the GPU voltage, depending on the SoC, necessary in order to achieve > system stability across the full range of supported GPU frequencies. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> > --- > > This driver was successfully tested for more than 3 months. > GPU DVFS works correctly with no stability issues. > > Changes in RESEND,v3: > Rebased over next-20221005 > > Changes in v3: > - Added braces to else-if branch > > Changes in v2: > - Added check for n_coupled > - Added check for vgpu to enforce attaching to vgpu<->sram coupling only > > Context: > This driver is the last of the pieces of a bigger puzzle, aiming to finally > enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek > SoCs on the fully open source graphics stack (Panfrost driver). > > No devicetree bindings are provided because this does not require any > driver-specific binding at all. > > Last but not least: it was chosen to have this driver enabled for > ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration > choice because, once the DVFS mechanism will be fully working, using one > of the listed MediaTek SoCs *without* this coupling mechanism *will* lead > to unstabilities and system crashes. > For COMPILE_TEST, choice is given for obvious reasons. > > drivers/soc/mediatek/Kconfig | 5 + > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ > 3 files changed, 165 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index 40d0cc600cae..30b5afc0e51d 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP > on different MediaTek SoCs. The PMIC wrapper is a proprietary > hardware to connect the PMIC. > > +config MTK_REGULATOR_COUPLER > + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST > + default ARCH_MEDIATEK > + depends on REGULATOR > + > config MTK_SCPSYS > bool "MediaTek SCPSYS Support" > default ARCH_MEDIATEK > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index 0e9e703c931a..8c0ddacbcde8 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o > obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o > diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c > new file mode 100644 > index 000000000000..ad2ed42aa697 > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c > @@ -0,0 +1,159 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Voltage regulators coupler for MediaTek SoCs > + * > + * Copyright (C) 2022 Collabora, Ltd. > + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/of.h> > +#include <linux/regulator/coupler.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > +#include <linux/suspend.h> > + > +#define to_mediatek_coupler(x) container_of(x, struct mediatek_regulator_coupler, coupler) > + > +struct mediatek_regulator_coupler { > + struct regulator_coupler coupler; > + struct regulator_dev *vsram_rdev; > +}; > + > +/* > + * We currently support only couples of not more than two vregs and > + * modify the vsram voltage only when changing voltage of vgpu. > + * > + * This function is limited to the GPU<->SRAM voltages relationships. > + */ > +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, > + struct regulator_dev *rdev, > + suspend_state_t state) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + int max_spread = rdev->constraints->max_spread[0]; > + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; > + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; > + int vsram_target_min_uV, vsram_target_max_uV; > + int min_uV = 0; > + int max_uV = INT_MAX; > + int ret; > + > + /* > + * If the target device is on, setting the SRAM voltage directly > + * is not supported as it scales through its coupled supply voltage. > + * > + * An exception is made in case the use_count is zero: this means > + * that this is the first time we power up the SRAM regulator, which > + * implies that the target device has yet to perform initialization > + * and setting a voltage at that time is harmless. > + */ > + if (rdev == mrc->vsram_rdev) { > + if (rdev->use_count == 0) > + return regulator_do_balance_voltage(rdev, state, true); > + > + return -EPERM; > + } > + > + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); > + if (ret < 0) > + return ret; > + > + if (min_uV == 0) { > + ret = regulator_get_voltage_rdev(rdev); > + if (ret < 0) > + return ret; > + min_uV = ret; > + } > + > + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); > + if (ret < 0) > + return ret; > + > + /* > + * If we're asked to set a voltage less than VSRAM min_uV, set > + * the minimum allowed voltage on VSRAM, as in this case it is > + * safe to ignore the max_spread parameter. > + */ > + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); > + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); > + > + /* Make sure we're not out of range */ > + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); > + > + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", > + vsram_target_min_uV, vsram_target_max_uV, > + rdev_get_name(mrc->vsram_rdev), min_uV); > + > + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, > + vsram_target_max_uV, state); > + if (ret) > + return ret; > + > + /* The sram voltage is now balanced: update the target vreg voltage */ > + return regulator_do_balance_voltage(rdev, state, true); > +} > + > +static int mediatek_regulator_attach(struct regulator_coupler *coupler, > + struct regulator_dev *rdev) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + const char *rdev_name = rdev_get_name(rdev); > + > + /* > + * If we're getting a coupling of more than two regulators here and > + * this means that this is surely not a GPU<->SRAM couple: in that > + * case, we may want to use another coupler implementation, if any, > + * or the generic one: the regulator core will keep walking through > + * the list of couplers when any .attach_regulator() cb returns 1. > + */ > + if (rdev->coupling_desc.n_coupled > 2) > + return 1; > + > + if (strstr(rdev_name, "sram")) { My understanding is, that we have to have either a DT node with regulator-name = "sram" property to pollute rdev->constraints->name or some regulator_desc->name populated in the drivers/regulator/mt*.c I wasn't able to find either of this, so I wonder how this is supposed to work. Please provide pointers to a working and complete implementation of this, so that I'm able to judge what is going on and if the approach is the correct one. I tried to figure out using mt8195-tracking-master-rolling Regards, Matthias > + if (mrc->vsram_rdev) > + return -EINVAL; > + mrc->vsram_rdev = rdev; > + } else if (!strstr(rdev_name, "vgpu") && !strstr(rdev_name, "Vgpu")) { > + return 1; > + } > + > + return 0; > +} > + > +static int mediatek_regulator_detach(struct regulator_coupler *coupler, > + struct regulator_dev *rdev) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + > + if (rdev == mrc->vsram_rdev) > + mrc->vsram_rdev = NULL; > + > + return 0; > +} > + > +static struct mediatek_regulator_coupler mediatek_coupler = { > + .coupler = { > + .attach_regulator = mediatek_regulator_attach, > + .detach_regulator = mediatek_regulator_detach, > + .balance_voltage = mediatek_regulator_balance_voltage, > + }, > +}; > + > +static int mediatek_regulator_coupler_init(void) > +{ > + if (!of_machine_is_compatible("mediatek,mt8183") && > + !of_machine_is_compatible("mediatek,mt8186") && > + !of_machine_is_compatible("mediatek,mt8192")) > + return 0; > + > + return regulator_coupler_register(&mediatek_coupler.coupler); > +} > +arch_initcall(mediatek_regulator_coupler_init); > + > +MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>"); > +MODULE_DESCRIPTION("MediaTek Regulator Coupler driver"); > +MODULE_LICENSE("GPL");
Il 21/11/22 12:44, Matthias Brugger ha scritto: > > > On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: >> This driver currently deals with GPU-SRAM regulator coupling, ensuring >> that the SRAM voltage is always between a specific range of distance to >> the GPU voltage, depending on the SoC, necessary in order to achieve >> system stability across the full range of supported GPU frequencies. >> >> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >> --- >> >> This driver was successfully tested for more than 3 months. >> GPU DVFS works correctly with no stability issues. >> >> Changes in RESEND,v3: >> Rebased over next-20221005 >> >> Changes in v3: >> - Added braces to else-if branch >> >> Changes in v2: >> - Added check for n_coupled >> - Added check for vgpu to enforce attaching to vgpu<->sram coupling only >> >> Context: >> This driver is the last of the pieces of a bigger puzzle, aiming to finally >> enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek >> SoCs on the fully open source graphics stack (Panfrost driver). >> >> No devicetree bindings are provided because this does not require any >> driver-specific binding at all. >> >> Last but not least: it was chosen to have this driver enabled for >> ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration >> choice because, once the DVFS mechanism will be fully working, using one >> of the listed MediaTek SoCs *without* this coupling mechanism *will* lead >> to unstabilities and system crashes. >> For COMPILE_TEST, choice is given for obvious reasons. >> >> drivers/soc/mediatek/Kconfig | 5 + >> drivers/soc/mediatek/Makefile | 1 + >> drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ >> 3 files changed, 165 insertions(+) >> create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c >> >> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >> index 40d0cc600cae..30b5afc0e51d 100644 >> --- a/drivers/soc/mediatek/Kconfig >> +++ b/drivers/soc/mediatek/Kconfig >> @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP >> on different MediaTek SoCs. The PMIC wrapper is a proprietary >> hardware to connect the PMIC. >> +config MTK_REGULATOR_COUPLER >> + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST >> + default ARCH_MEDIATEK >> + depends on REGULATOR >> + >> config MTK_SCPSYS >> bool "MediaTek SCPSYS Support" >> default ARCH_MEDIATEK >> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >> index 0e9e703c931a..8c0ddacbcde8 100644 >> --- a/drivers/soc/mediatek/Makefile >> +++ b/drivers/soc/mediatek/Makefile >> @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o >> obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o >> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >> +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o >> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o >> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o >> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c >> b/drivers/soc/mediatek/mtk-regulator-coupler.c >> new file mode 100644 >> index 000000000000..ad2ed42aa697 >> --- /dev/null >> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c >> @@ -0,0 +1,159 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Voltage regulators coupler for MediaTek SoCs >> + * >> + * Copyright (C) 2022 Collabora, Ltd. >> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >> + */ >> + >> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >> + >> +#include <linux/init.h> >> +#include <linux/kernel.h> >> +#include <linux/of.h> >> +#include <linux/regulator/coupler.h> >> +#include <linux/regulator/driver.h> >> +#include <linux/regulator/machine.h> >> +#include <linux/suspend.h> >> + >> +#define to_mediatek_coupler(x) container_of(x, struct >> mediatek_regulator_coupler, coupler) >> + >> +struct mediatek_regulator_coupler { >> + struct regulator_coupler coupler; >> + struct regulator_dev *vsram_rdev; >> +}; >> + >> +/* >> + * We currently support only couples of not more than two vregs and >> + * modify the vsram voltage only when changing voltage of vgpu. >> + * >> + * This function is limited to the GPU<->SRAM voltages relationships. >> + */ >> +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, >> + struct regulator_dev *rdev, >> + suspend_state_t state) >> +{ >> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >> + int max_spread = rdev->constraints->max_spread[0]; >> + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; >> + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; >> + int vsram_target_min_uV, vsram_target_max_uV; >> + int min_uV = 0; >> + int max_uV = INT_MAX; >> + int ret; >> + >> + /* >> + * If the target device is on, setting the SRAM voltage directly >> + * is not supported as it scales through its coupled supply voltage. >> + * >> + * An exception is made in case the use_count is zero: this means >> + * that this is the first time we power up the SRAM regulator, which >> + * implies that the target device has yet to perform initialization >> + * and setting a voltage at that time is harmless. >> + */ >> + if (rdev == mrc->vsram_rdev) { >> + if (rdev->use_count == 0) >> + return regulator_do_balance_voltage(rdev, state, true); >> + >> + return -EPERM; >> + } >> + >> + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); >> + if (ret < 0) >> + return ret; >> + >> + if (min_uV == 0) { >> + ret = regulator_get_voltage_rdev(rdev); >> + if (ret < 0) >> + return ret; >> + min_uV = ret; >> + } >> + >> + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); >> + if (ret < 0) >> + return ret; >> + >> + /* >> + * If we're asked to set a voltage less than VSRAM min_uV, set >> + * the minimum allowed voltage on VSRAM, as in this case it is >> + * safe to ignore the max_spread parameter. >> + */ >> + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); >> + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); >> + >> + /* Make sure we're not out of range */ >> + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); >> + >> + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", >> + vsram_target_min_uV, vsram_target_max_uV, >> + rdev_get_name(mrc->vsram_rdev), min_uV); >> + >> + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, >> + vsram_target_max_uV, state); >> + if (ret) >> + return ret; >> + >> + /* The sram voltage is now balanced: update the target vreg voltage */ >> + return regulator_do_balance_voltage(rdev, state, true); >> +} >> + >> +static int mediatek_regulator_attach(struct regulator_coupler *coupler, >> + struct regulator_dev *rdev) >> +{ >> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >> + const char *rdev_name = rdev_get_name(rdev); >> + >> + /* >> + * If we're getting a coupling of more than two regulators here and >> + * this means that this is surely not a GPU<->SRAM couple: in that >> + * case, we may want to use another coupler implementation, if any, >> + * or the generic one: the regulator core will keep walking through >> + * the list of couplers when any .attach_regulator() cb returns 1. >> + */ >> + if (rdev->coupling_desc.n_coupled > 2) >> + return 1; >> + >> + if (strstr(rdev_name, "sram")) { > > My understanding is, that we have to have either a DT node with regulator-name = > "sram" property to pollute rdev->constraints->name or some regulator_desc->name > populated in the drivers/regulator/mt*.c > No, it's not trying to find a regulator named "sram", but any regulator that *contains* the "sram" string in its name, but checks only regulators that are coupled to others. This is the same for the "Vgpu" / "vgpu". Example: Regulator A, coupled to Regulator B. Regulator A name = "Vgpu" or "vgpu", or *vgpu*, or *Vgpu* (name must contain either Vgpu or vgpu) Regulator B name = "vsram" or "sram_gpu" or *sram* (name must contain "sram"). mrc->vsram_rdev = rdev rdev == our Regulator B. We hence return 0 to the coupling API: this will produce the effect of making it use this driver's .balance_voltage() callback instead of the generic one on vgpu<->vsram. > I wasn't able to find either of this, so I wonder how this is supposed to work. > Please provide pointers to a working and complete implementation of this, so that > I'm able to judge what is going on and if the approach is the correct one. > > I tried to figure out using mt8195-tracking-master-rolling That's the right branch. Let's take MT8192 Asurada as an example of how this works.... `mt6359_vsram_others_ldo_reg´ is the SRAM regulator for the GPU: https://gitlab.collabora.com/google/chromeos-kernel/-/blob/mt8195-tracking-master-rolling/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi#L551 `mt6359_vsram_others_ldo_reg` regulator-name = "vsram_others"; ^^^^ Contains "sram", and this regulator is also regulator-coupled-with = <&mt6315_7_vbuck1>; `mt6315_7_vbuck1` regulator-name = "Vgpu"; ^^^^ Contains "Vgpu", and this regulator is also regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>; That's how the coupling works in this case. Now, looking at case exclusions: In MT8192 Asurada (or, actually, mt6359.dtsi) we have other regulators that do actually contain "sram" in their name, like "vsram_proc1" and "vsram_others_sshub". These regulators will be ignored, as they are *not* coupled with Vgpu. What this driver currently does in this regard is: 1. Regulator attach is called only on *coupled* regulators, not on the others 2. If the regulator contains name "vgpu" or "Vgpu" or "sram" we're good, otherwise we don't attach the balance_voltage logic of this driver to the unmatched regulators. Does this explanation clarify your doubts? Regards, Angelo
Il 21/11/22 13:01, AngeloGioacchino Del Regno ha scritto: > Il 21/11/22 12:44, Matthias Brugger ha scritto: >> >> >> On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: >>> This driver currently deals with GPU-SRAM regulator coupling, ensuring >>> that the SRAM voltage is always between a specific range of distance to >>> the GPU voltage, depending on the SoC, necessary in order to achieve >>> system stability across the full range of supported GPU frequencies. >>> >>> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>> --- >>> >>> This driver was successfully tested for more than 3 months. >>> GPU DVFS works correctly with no stability issues. >>> >>> Changes in RESEND,v3: >>> Rebased over next-20221005 >>> >>> Changes in v3: >>> - Added braces to else-if branch >>> >>> Changes in v2: >>> - Added check for n_coupled >>> - Added check for vgpu to enforce attaching to vgpu<->sram coupling only >>> >>> Context: >>> This driver is the last of the pieces of a bigger puzzle, aiming to finally >>> enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek >>> SoCs on the fully open source graphics stack (Panfrost driver). >>> >>> No devicetree bindings are provided because this does not require any >>> driver-specific binding at all. >>> >>> Last but not least: it was chosen to have this driver enabled for >>> ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration >>> choice because, once the DVFS mechanism will be fully working, using one >>> of the listed MediaTek SoCs *without* this coupling mechanism *will* lead >>> to unstabilities and system crashes. >>> For COMPILE_TEST, choice is given for obvious reasons. >>> >>> drivers/soc/mediatek/Kconfig | 5 + >>> drivers/soc/mediatek/Makefile | 1 + >>> drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ >>> 3 files changed, 165 insertions(+) >>> create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c >>> >>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >>> index 40d0cc600cae..30b5afc0e51d 100644 >>> --- a/drivers/soc/mediatek/Kconfig >>> +++ b/drivers/soc/mediatek/Kconfig >>> @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP >>> on different MediaTek SoCs. The PMIC wrapper is a proprietary >>> hardware to connect the PMIC. >>> +config MTK_REGULATOR_COUPLER >>> + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST >>> + default ARCH_MEDIATEK >>> + depends on REGULATOR >>> + >>> config MTK_SCPSYS >>> bool "MediaTek SCPSYS Support" >>> default ARCH_MEDIATEK >>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >>> index 0e9e703c931a..8c0ddacbcde8 100644 >>> --- a/drivers/soc/mediatek/Makefile >>> +++ b/drivers/soc/mediatek/Makefile >>> @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o >>> obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o >>> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >>> +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o >>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >>> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o >>> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o >>> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c >>> b/drivers/soc/mediatek/mtk-regulator-coupler.c >>> new file mode 100644 >>> index 000000000000..ad2ed42aa697 >>> --- /dev/null >>> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c >>> @@ -0,0 +1,159 @@ >>> +// SPDX-License-Identifier: GPL-2.0-only >>> +/* >>> + * Voltage regulators coupler for MediaTek SoCs >>> + * >>> + * Copyright (C) 2022 Collabora, Ltd. >>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>> + */ >>> + >>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>> + >>> +#include <linux/init.h> >>> +#include <linux/kernel.h> >>> +#include <linux/of.h> >>> +#include <linux/regulator/coupler.h> >>> +#include <linux/regulator/driver.h> >>> +#include <linux/regulator/machine.h> >>> +#include <linux/suspend.h> >>> + >>> +#define to_mediatek_coupler(x) container_of(x, struct >>> mediatek_regulator_coupler, coupler) >>> + >>> +struct mediatek_regulator_coupler { >>> + struct regulator_coupler coupler; >>> + struct regulator_dev *vsram_rdev; >>> +}; >>> + >>> +/* >>> + * We currently support only couples of not more than two vregs and >>> + * modify the vsram voltage only when changing voltage of vgpu. >>> + * >>> + * This function is limited to the GPU<->SRAM voltages relationships. >>> + */ >>> +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, >>> + struct regulator_dev *rdev, >>> + suspend_state_t state) >>> +{ >>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>> + int max_spread = rdev->constraints->max_spread[0]; >>> + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; >>> + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; >>> + int vsram_target_min_uV, vsram_target_max_uV; >>> + int min_uV = 0; >>> + int max_uV = INT_MAX; >>> + int ret; >>> + >>> + /* >>> + * If the target device is on, setting the SRAM voltage directly >>> + * is not supported as it scales through its coupled supply voltage. >>> + * >>> + * An exception is made in case the use_count is zero: this means >>> + * that this is the first time we power up the SRAM regulator, which >>> + * implies that the target device has yet to perform initialization >>> + * and setting a voltage at that time is harmless. >>> + */ >>> + if (rdev == mrc->vsram_rdev) { >>> + if (rdev->use_count == 0) >>> + return regulator_do_balance_voltage(rdev, state, true); >>> + >>> + return -EPERM; >>> + } >>> + >>> + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); >>> + if (ret < 0) >>> + return ret; >>> + >>> + if (min_uV == 0) { >>> + ret = regulator_get_voltage_rdev(rdev); >>> + if (ret < 0) >>> + return ret; >>> + min_uV = ret; >>> + } >>> + >>> + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); >>> + if (ret < 0) >>> + return ret; >>> + >>> + /* >>> + * If we're asked to set a voltage less than VSRAM min_uV, set >>> + * the minimum allowed voltage on VSRAM, as in this case it is >>> + * safe to ignore the max_spread parameter. >>> + */ >>> + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); >>> + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); >>> + >>> + /* Make sure we're not out of range */ >>> + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); >>> + >>> + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", >>> + vsram_target_min_uV, vsram_target_max_uV, >>> + rdev_get_name(mrc->vsram_rdev), min_uV); >>> + >>> + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, >>> + vsram_target_max_uV, state); >>> + if (ret) >>> + return ret; >>> + >>> + /* The sram voltage is now balanced: update the target vreg voltage */ >>> + return regulator_do_balance_voltage(rdev, state, true); >>> +} >>> + >>> +static int mediatek_regulator_attach(struct regulator_coupler *coupler, >>> + struct regulator_dev *rdev) >>> +{ >>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>> + const char *rdev_name = rdev_get_name(rdev); >>> + >>> + /* >>> + * If we're getting a coupling of more than two regulators here and >>> + * this means that this is surely not a GPU<->SRAM couple: in that >>> + * case, we may want to use another coupler implementation, if any, >>> + * or the generic one: the regulator core will keep walking through >>> + * the list of couplers when any .attach_regulator() cb returns 1. >>> + */ >>> + if (rdev->coupling_desc.n_coupled > 2) >>> + return 1; >>> + >>> + if (strstr(rdev_name, "sram")) { >> >> My understanding is, that we have to have either a DT node with regulator-name = >> "sram" property to pollute rdev->constraints->name or some regulator_desc->name >> populated in the drivers/regulator/mt*.c >> > > No, it's not trying to find a regulator named "sram", but any regulator that > *contains* the "sram" string in its name, but checks only regulators that are > coupled to others. This is the same for the "Vgpu" / "vgpu". > > Example: > > Regulator A, coupled to Regulator B. > > Regulator A name = "Vgpu" or "vgpu", or *vgpu*, or *Vgpu* > (name must contain either Vgpu or vgpu) > > Regulator B name = "vsram" or "sram_gpu" or *sram* > (name must contain "sram"). > > mrc->vsram_rdev = rdev > > rdev == our Regulator B. > > We hence return 0 to the coupling API: this will produce the effect of making > it use this driver's .balance_voltage() callback instead of the generic one > on vgpu<->vsram. > > >> I wasn't able to find either of this, so I wonder how this is supposed to work. >> Please provide pointers to a working and complete implementation of this, so that >> I'm able to judge what is going on and if the approach is the correct one. >> >> I tried to figure out using mt8195-tracking-master-rolling > > That's the right branch. > > Let's take MT8192 Asurada as an example of how this works.... > > `mt6359_vsram_others_ldo_reg´ is the SRAM regulator for the GPU: > https://gitlab.collabora.com/google/chromeos-kernel/-/blob/mt8195-tracking-master-rolling/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi#L551 > > `mt6359_vsram_others_ldo_reg` regulator-name = "vsram_others"; > ^^^^ > Contains "sram", and this regulator is also > > regulator-coupled-with = <&mt6315_7_vbuck1>; > > `mt6315_7_vbuck1` regulator-name = "Vgpu"; > ^^^^ > Contains "Vgpu", and this regulator is also > > regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>; > > That's how the coupling works in this case. > > > Now, looking at case exclusions: > In MT8192 Asurada (or, actually, mt6359.dtsi) we have other regulators that do > actually contain "sram" in their name, like "vsram_proc1" and "vsram_others_sshub". > > These regulators will be ignored, as they are *not* coupled with Vgpu. > > > What this driver currently does in this regard is: > 1. Regulator attach is called only on *coupled* regulators, not on the others > 2. If the regulator contains name "vgpu" or "Vgpu" or "sram" we're good, > otherwise we don't attach the balance_voltage logic of this driver to > the unmatched regulators. > > > Does this explanation clarify your doubts? > > Regards, > Angelo > Matthias, please don't forget about this one :-) Thanks! Angelo
Hi Angelo, On 16/12/2022 14:15, AngeloGioacchino Del Regno wrote: > Il 21/11/22 13:01, AngeloGioacchino Del Regno ha scritto: >> Il 21/11/22 12:44, Matthias Brugger ha scritto: >>> >>> >>> On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: >>>> This driver currently deals with GPU-SRAM regulator coupling, ensuring >>>> that the SRAM voltage is always between a specific range of distance to >>>> the GPU voltage, depending on the SoC, necessary in order to achieve >>>> system stability across the full range of supported GPU frequencies. >>>> >>>> Signed-off-by: AngeloGioacchino Del Regno >>>> <angelogioacchino.delregno@collabora.com> >>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>> --- >>>> >>>> This driver was successfully tested for more than 3 months. >>>> GPU DVFS works correctly with no stability issues. >>>> >>>> Changes in RESEND,v3: >>>> Rebased over next-20221005 >>>> >>>> Changes in v3: >>>> - Added braces to else-if branch >>>> >>>> Changes in v2: >>>> - Added check for n_coupled >>>> - Added check for vgpu to enforce attaching to vgpu<->sram coupling only >>>> >>>> Context: >>>> This driver is the last of the pieces of a bigger puzzle, aiming to finally >>>> enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek >>>> SoCs on the fully open source graphics stack (Panfrost driver). >>>> >>>> No devicetree bindings are provided because this does not require any >>>> driver-specific binding at all. >>>> >>>> Last but not least: it was chosen to have this driver enabled for >>>> ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration >>>> choice because, once the DVFS mechanism will be fully working, using one >>>> of the listed MediaTek SoCs *without* this coupling mechanism *will* lead >>>> to unstabilities and system crashes. >>>> For COMPILE_TEST, choice is given for obvious reasons. >>>> >>>> drivers/soc/mediatek/Kconfig | 5 + >>>> drivers/soc/mediatek/Makefile | 1 + >>>> drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ >>>> 3 files changed, 165 insertions(+) >>>> create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c >>>> >>>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >>>> index 40d0cc600cae..30b5afc0e51d 100644 >>>> --- a/drivers/soc/mediatek/Kconfig >>>> +++ b/drivers/soc/mediatek/Kconfig >>>> @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP >>>> on different MediaTek SoCs. The PMIC wrapper is a proprietary >>>> hardware to connect the PMIC. >>>> +config MTK_REGULATOR_COUPLER >>>> + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST >>>> + default ARCH_MEDIATEK >>>> + depends on REGULATOR >>>> + >>>> config MTK_SCPSYS >>>> bool "MediaTek SCPSYS Support" >>>> default ARCH_MEDIATEK >>>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >>>> index 0e9e703c931a..8c0ddacbcde8 100644 >>>> --- a/drivers/soc/mediatek/Makefile >>>> +++ b/drivers/soc/mediatek/Makefile >>>> @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o >>>> obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o >>>> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >>>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >>>> +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o >>>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >>>> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o >>>> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o >>>> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c >>>> b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>> new file mode 100644 >>>> index 000000000000..ad2ed42aa697 >>>> --- /dev/null >>>> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>> @@ -0,0 +1,159 @@ >>>> +// SPDX-License-Identifier: GPL-2.0-only >>>> +/* >>>> + * Voltage regulators coupler for MediaTek SoCs >>>> + * >>>> + * Copyright (C) 2022 Collabora, Ltd. >>>> + * Author: AngeloGioacchino Del Regno >>>> <angelogioacchino.delregno@collabora.com> >>>> + */ >>>> + >>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>> + >>>> +#include <linux/init.h> >>>> +#include <linux/kernel.h> >>>> +#include <linux/of.h> >>>> +#include <linux/regulator/coupler.h> >>>> +#include <linux/regulator/driver.h> >>>> +#include <linux/regulator/machine.h> >>>> +#include <linux/suspend.h> >>>> + >>>> +#define to_mediatek_coupler(x) container_of(x, struct >>>> mediatek_regulator_coupler, coupler) >>>> + >>>> +struct mediatek_regulator_coupler { >>>> + struct regulator_coupler coupler; >>>> + struct regulator_dev *vsram_rdev; >>>> +}; >>>> + >>>> +/* >>>> + * We currently support only couples of not more than two vregs and >>>> + * modify the vsram voltage only when changing voltage of vgpu. >>>> + * >>>> + * This function is limited to the GPU<->SRAM voltages relationships. >>>> + */ >>>> +static int mediatek_regulator_balance_voltage(struct regulator_coupler >>>> *coupler, >>>> + struct regulator_dev *rdev, >>>> + suspend_state_t state) >>>> +{ >>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>> + int max_spread = rdev->constraints->max_spread[0]; >>>> + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; >>>> + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; >>>> + int vsram_target_min_uV, vsram_target_max_uV; >>>> + int min_uV = 0; >>>> + int max_uV = INT_MAX; >>>> + int ret; >>>> + >>>> + /* >>>> + * If the target device is on, setting the SRAM voltage directly >>>> + * is not supported as it scales through its coupled supply voltage. >>>> + * >>>> + * An exception is made in case the use_count is zero: this means >>>> + * that this is the first time we power up the SRAM regulator, which >>>> + * implies that the target device has yet to perform initialization >>>> + * and setting a voltage at that time is harmless. >>>> + */ >>>> + if (rdev == mrc->vsram_rdev) { >>>> + if (rdev->use_count == 0) >>>> + return regulator_do_balance_voltage(rdev, state, true); >>>> + >>>> + return -EPERM; >>>> + } >>>> + >>>> + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + if (min_uV == 0) { >>>> + ret = regulator_get_voltage_rdev(rdev); >>>> + if (ret < 0) >>>> + return ret; >>>> + min_uV = ret; >>>> + } >>>> + >>>> + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> + /* >>>> + * If we're asked to set a voltage less than VSRAM min_uV, set >>>> + * the minimum allowed voltage on VSRAM, as in this case it is >>>> + * safe to ignore the max_spread parameter. >>>> + */ >>>> + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); >>>> + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); >>>> + >>>> + /* Make sure we're not out of range */ >>>> + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); >>>> + >>>> + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", >>>> + vsram_target_min_uV, vsram_target_max_uV, >>>> + rdev_get_name(mrc->vsram_rdev), min_uV); >>>> + >>>> + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, >>>> + vsram_target_max_uV, state); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* The sram voltage is now balanced: update the target vreg voltage */ >>>> + return regulator_do_balance_voltage(rdev, state, true); >>>> +} >>>> + >>>> +static int mediatek_regulator_attach(struct regulator_coupler *coupler, >>>> + struct regulator_dev *rdev) >>>> +{ >>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>> + const char *rdev_name = rdev_get_name(rdev); >>>> + >>>> + /* >>>> + * If we're getting a coupling of more than two regulators here and >>>> + * this means that this is surely not a GPU<->SRAM couple: in that >>>> + * case, we may want to use another coupler implementation, if any, >>>> + * or the generic one: the regulator core will keep walking through >>>> + * the list of couplers when any .attach_regulator() cb returns 1. >>>> + */ >>>> + if (rdev->coupling_desc.n_coupled > 2) >>>> + return 1; >>>> + >>>> + if (strstr(rdev_name, "sram")) { >>> >>> My understanding is, that we have to have either a DT node with >>> regulator-name = "sram" property to pollute rdev->constraints->name or some >>> regulator_desc->name populated in the drivers/regulator/mt*.c >>> >> >> No, it's not trying to find a regulator named "sram", but any regulator that >> *contains* the "sram" string in its name, but checks only regulators that are >> coupled to others. This is the same for the "Vgpu" / "vgpu". >> >> Example: >> >> Regulator A, coupled to Regulator B. >> >> Regulator A name = "Vgpu" or "vgpu", or *vgpu*, or *Vgpu* >> (name must contain either Vgpu or vgpu) >> >> Regulator B name = "vsram" or "sram_gpu" or *sram* >> (name must contain "sram"). >> >> mrc->vsram_rdev = rdev >> >> rdev == our Regulator B. >> >> We hence return 0 to the coupling API: this will produce the effect of making >> it use this driver's .balance_voltage() callback instead of the generic one >> on vgpu<->vsram. >> >> >>> I wasn't able to find either of this, so I wonder how this is supposed to >>> work. Please provide pointers to a working and complete implementation of >>> this, so that I'm able to judge what is going on and if the approach is the >>> correct one. >>> >>> I tried to figure out using mt8195-tracking-master-rolling >> >> That's the right branch. >> >> Let's take MT8192 Asurada as an example of how this works.... >> >> `mt6359_vsram_others_ldo_reg´ is the SRAM regulator for the GPU: >> https://gitlab.collabora.com/google/chromeos-kernel/-/blob/mt8195-tracking-master-rolling/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi#L551 >> >> `mt6359_vsram_others_ldo_reg` regulator-name = "vsram_others"; >> ^^^^ >> Contains "sram", and this regulator is also >> >> regulator-coupled-with = <&mt6315_7_vbuck1>; >> >> `mt6315_7_vbuck1` regulator-name = "Vgpu"; >> ^^^^ >> Contains "Vgpu", and this regulator is also >> >> regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>; >> >> That's how the coupling works in this case. >> >> >> Now, looking at case exclusions: >> In MT8192 Asurada (or, actually, mt6359.dtsi) we have other regulators that do >> actually contain "sram" in their name, like "vsram_proc1" and >> "vsram_others_sshub". >> >> These regulators will be ignored, as they are *not* coupled with Vgpu. >> >> >> What this driver currently does in this regard is: >> 1. Regulator attach is called only on *coupled* regulators, not on the others >> 2. If the regulator contains name "vgpu" or "Vgpu" or "sram" we're good, >> otherwise we don't attach the balance_voltage logic of this driver to >> the unmatched regulators. >> >> >> Does this explanation clarify your doubts? >> >> Regards, >> Angelo >> > > Matthias, please don't forget about this one :-) > Unfortunately I have to leave now and didn't had time to look more into it. So I'll write just my suspicion. So please feel free to correct me if I'm totally wrong (I know you will do anyway ;) 1) When the regulator-coupler driver was introduced the driver was bound by a compatible, but we use arch_initcall(...) for it. 2) I really don't like the hardcoded search for a the regulator name in the mediatek_regulator_attach function. I haven't seen that in the Nvidia driver, also I had no time to understand how they decide which regulators need to be coupled. My instinct would tell me that this should be described in device-tree. Have a nice weekend! Matthias
Il 16/12/22 16:26, Matthias Brugger ha scritto: > Hi Angelo, > > On 16/12/2022 14:15, AngeloGioacchino Del Regno wrote: >> Il 21/11/22 13:01, AngeloGioacchino Del Regno ha scritto: >>> Il 21/11/22 12:44, Matthias Brugger ha scritto: >>>> >>>> >>>> On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: >>>>> This driver currently deals with GPU-SRAM regulator coupling, ensuring >>>>> that the SRAM voltage is always between a specific range of distance to >>>>> the GPU voltage, depending on the SoC, necessary in order to achieve >>>>> system stability across the full range of supported GPU frequencies. >>>>> >>>>> Signed-off-by: AngeloGioacchino Del Regno >>>>> <angelogioacchino.delregno@collabora.com> >>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>>> --- >>>>> >>>>> This driver was successfully tested for more than 3 months. >>>>> GPU DVFS works correctly with no stability issues. >>>>> >>>>> Changes in RESEND,v3: >>>>> Rebased over next-20221005 >>>>> >>>>> Changes in v3: >>>>> - Added braces to else-if branch >>>>> >>>>> Changes in v2: >>>>> - Added check for n_coupled >>>>> - Added check for vgpu to enforce attaching to vgpu<->sram coupling only >>>>> >>>>> Context: >>>>> This driver is the last of the pieces of a bigger puzzle, aiming to finally >>>>> enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek >>>>> SoCs on the fully open source graphics stack (Panfrost driver). >>>>> >>>>> No devicetree bindings are provided because this does not require any >>>>> driver-specific binding at all. >>>>> >>>>> Last but not least: it was chosen to have this driver enabled for >>>>> ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration >>>>> choice because, once the DVFS mechanism will be fully working, using one >>>>> of the listed MediaTek SoCs *without* this coupling mechanism *will* lead >>>>> to unstabilities and system crashes. >>>>> For COMPILE_TEST, choice is given for obvious reasons. >>>>> >>>>> drivers/soc/mediatek/Kconfig | 5 + >>>>> drivers/soc/mediatek/Makefile | 1 + >>>>> drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ >>>>> 3 files changed, 165 insertions(+) >>>>> create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c >>>>> >>>>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >>>>> index 40d0cc600cae..30b5afc0e51d 100644 >>>>> --- a/drivers/soc/mediatek/Kconfig >>>>> +++ b/drivers/soc/mediatek/Kconfig >>>>> @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP >>>>> on different MediaTek SoCs. The PMIC wrapper is a proprietary >>>>> hardware to connect the PMIC. >>>>> +config MTK_REGULATOR_COUPLER >>>>> + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST >>>>> + default ARCH_MEDIATEK >>>>> + depends on REGULATOR >>>>> + >>>>> config MTK_SCPSYS >>>>> bool "MediaTek SCPSYS Support" >>>>> default ARCH_MEDIATEK >>>>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >>>>> index 0e9e703c931a..8c0ddacbcde8 100644 >>>>> --- a/drivers/soc/mediatek/Makefile >>>>> +++ b/drivers/soc/mediatek/Makefile >>>>> @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o >>>>> obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o >>>>> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >>>>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >>>>> +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o >>>>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >>>>> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o >>>>> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o >>>>> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>> b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>> new file mode 100644 >>>>> index 000000000000..ad2ed42aa697 >>>>> --- /dev/null >>>>> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>> @@ -0,0 +1,159 @@ >>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>> +/* >>>>> + * Voltage regulators coupler for MediaTek SoCs >>>>> + * >>>>> + * Copyright (C) 2022 Collabora, Ltd. >>>>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>>>> + */ >>>>> + >>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>>> + >>>>> +#include <linux/init.h> >>>>> +#include <linux/kernel.h> >>>>> +#include <linux/of.h> >>>>> +#include <linux/regulator/coupler.h> >>>>> +#include <linux/regulator/driver.h> >>>>> +#include <linux/regulator/machine.h> >>>>> +#include <linux/suspend.h> >>>>> + >>>>> +#define to_mediatek_coupler(x) container_of(x, struct >>>>> mediatek_regulator_coupler, coupler) >>>>> + >>>>> +struct mediatek_regulator_coupler { >>>>> + struct regulator_coupler coupler; >>>>> + struct regulator_dev *vsram_rdev; >>>>> +}; >>>>> + >>>>> +/* >>>>> + * We currently support only couples of not more than two vregs and >>>>> + * modify the vsram voltage only when changing voltage of vgpu. >>>>> + * >>>>> + * This function is limited to the GPU<->SRAM voltages relationships. >>>>> + */ >>>>> +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, >>>>> + struct regulator_dev *rdev, >>>>> + suspend_state_t state) >>>>> +{ >>>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>>> + int max_spread = rdev->constraints->max_spread[0]; >>>>> + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; >>>>> + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; >>>>> + int vsram_target_min_uV, vsram_target_max_uV; >>>>> + int min_uV = 0; >>>>> + int max_uV = INT_MAX; >>>>> + int ret; >>>>> + >>>>> + /* >>>>> + * If the target device is on, setting the SRAM voltage directly >>>>> + * is not supported as it scales through its coupled supply voltage. >>>>> + * >>>>> + * An exception is made in case the use_count is zero: this means >>>>> + * that this is the first time we power up the SRAM regulator, which >>>>> + * implies that the target device has yet to perform initialization >>>>> + * and setting a voltage at that time is harmless. >>>>> + */ >>>>> + if (rdev == mrc->vsram_rdev) { >>>>> + if (rdev->use_count == 0) >>>>> + return regulator_do_balance_voltage(rdev, state, true); >>>>> + >>>>> + return -EPERM; >>>>> + } >>>>> + >>>>> + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + if (min_uV == 0) { >>>>> + ret = regulator_get_voltage_rdev(rdev); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + min_uV = ret; >>>>> + } >>>>> + >>>>> + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); >>>>> + if (ret < 0) >>>>> + return ret; >>>>> + >>>>> + /* >>>>> + * If we're asked to set a voltage less than VSRAM min_uV, set >>>>> + * the minimum allowed voltage on VSRAM, as in this case it is >>>>> + * safe to ignore the max_spread parameter. >>>>> + */ >>>>> + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); >>>>> + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); >>>>> + >>>>> + /* Make sure we're not out of range */ >>>>> + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); >>>>> + >>>>> + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", >>>>> + vsram_target_min_uV, vsram_target_max_uV, >>>>> + rdev_get_name(mrc->vsram_rdev), min_uV); >>>>> + >>>>> + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, >>>>> + vsram_target_max_uV, state); >>>>> + if (ret) >>>>> + return ret; >>>>> + >>>>> + /* The sram voltage is now balanced: update the target vreg voltage */ >>>>> + return regulator_do_balance_voltage(rdev, state, true); >>>>> +} >>>>> + >>>>> +static int mediatek_regulator_attach(struct regulator_coupler *coupler, >>>>> + struct regulator_dev *rdev) >>>>> +{ >>>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>>> + const char *rdev_name = rdev_get_name(rdev); >>>>> + >>>>> + /* >>>>> + * If we're getting a coupling of more than two regulators here and >>>>> + * this means that this is surely not a GPU<->SRAM couple: in that >>>>> + * case, we may want to use another coupler implementation, if any, >>>>> + * or the generic one: the regulator core will keep walking through >>>>> + * the list of couplers when any .attach_regulator() cb returns 1. >>>>> + */ >>>>> + if (rdev->coupling_desc.n_coupled > 2) >>>>> + return 1; >>>>> + >>>>> + if (strstr(rdev_name, "sram")) { >>>> >>>> My understanding is, that we have to have either a DT node with regulator-name >>>> = "sram" property to pollute rdev->constraints->name or some >>>> regulator_desc->name populated in the drivers/regulator/mt*.c >>>> >>> >>> No, it's not trying to find a regulator named "sram", but any regulator that >>> *contains* the "sram" string in its name, but checks only regulators that are >>> coupled to others. This is the same for the "Vgpu" / "vgpu". >>> >>> Example: >>> >>> Regulator A, coupled to Regulator B. >>> >>> Regulator A name = "Vgpu" or "vgpu", or *vgpu*, or *Vgpu* >>> (name must contain either Vgpu or vgpu) >>> >>> Regulator B name = "vsram" or "sram_gpu" or *sram* >>> (name must contain "sram"). >>> >>> mrc->vsram_rdev = rdev >>> >>> rdev == our Regulator B. >>> >>> We hence return 0 to the coupling API: this will produce the effect of making >>> it use this driver's .balance_voltage() callback instead of the generic one >>> on vgpu<->vsram. >>> >>> >>>> I wasn't able to find either of this, so I wonder how this is supposed to work. >>>> Please provide pointers to a working and complete implementation of this, so >>>> that I'm able to judge what is going on and if the approach is the correct one. >>>> >>>> I tried to figure out using mt8195-tracking-master-rolling >>> >>> That's the right branch. >>> >>> Let's take MT8192 Asurada as an example of how this works.... >>> >>> `mt6359_vsram_others_ldo_reg´ is the SRAM regulator for the GPU: >>> https://gitlab.collabora.com/google/chromeos-kernel/-/blob/mt8195-tracking-master-rolling/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi#L551 >>> >>> `mt6359_vsram_others_ldo_reg` regulator-name = "vsram_others"; >>> ^^^^ >>> Contains "sram", and this regulator is also >>> >>> regulator-coupled-with = <&mt6315_7_vbuck1>; >>> >>> `mt6315_7_vbuck1` regulator-name = "Vgpu"; >>> ^^^^ >>> Contains "Vgpu", and this regulator is also >>> >>> regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>; >>> >>> That's how the coupling works in this case. >>> >>> >>> Now, looking at case exclusions: >>> In MT8192 Asurada (or, actually, mt6359.dtsi) we have other regulators that do >>> actually contain "sram" in their name, like "vsram_proc1" and "vsram_others_sshub". >>> >>> These regulators will be ignored, as they are *not* coupled with Vgpu. >>> >>> >>> What this driver currently does in this regard is: >>> 1. Regulator attach is called only on *coupled* regulators, not on the others >>> 2. If the regulator contains name "vgpu" or "Vgpu" or "sram" we're good, >>> otherwise we don't attach the balance_voltage logic of this driver to >>> the unmatched regulators. >>> >>> >>> Does this explanation clarify your doubts? >>> >>> Regards, >>> Angelo >>> >> >> Matthias, please don't forget about this one :-) >> > > Unfortunately I have to leave now and didn't had time to look more into it. So I'll > write just my suspicion. So please feel free to correct me if I'm totally wrong (I > know you will do anyway ;) > > 1) When the regulator-coupler driver was introduced the driver was bound by a > compatible, but we use arch_initcall(...) for it. > 2) I really don't like the hardcoded search for a the regulator name in the > mediatek_regulator_attach function. I haven't seen that in the Nvidia driver, also > I had no time to understand how they decide which regulators need to be coupled. My > instinct would tell me that this should be described in device-tree. > I didn't want to use custom devicetree properties to couple our regulators... Tegra wants to couple three regulators: CORE, RTC and CPU and they're getting that with three custom properties: * nvidia,tegra-core-regulator * nvidia,tegra-rtc-regulator * nvidia,tegra-gpu-regulator Hardcoding the regulator name *sub*string in the mediatek-regulator-coupler allows us to use *only* generic devicetree properties instead of inventing our own: after all, even in the case in which a board uses a regulator that doesn't have the VGPU and/or VSRAM_GPU "kind of name" in it, it's still possible to change the name via devicetree (which would be correct, because the regulator is used for that, and would be the right name). As of now, there's no board using a regulator that has a not matching name, and this is because MediaTek SoCs are always using a MediaTek PMIC in combination, which always specifies this kind of name. I remind you that this driver does *nothing* unless coupling is activated through devicetree propert*ies*, as this driver has to be seen as a hook to the regulator coupling API, and not standalone code - hence, in my opinion, the cleanest implementation of that is to use only properties provided by the regulator coupling API, without any assistance from custom properties, unless *really* needed. Summarizing: * Deciding which regulators need to be coupled *already needs to* be described in devicetree, otherwise this driver will do nothing; * The hardcoded strings are there only to restrict this driver to GPU related regulators (vgpu as gpu core supply - vsram-gpu as gpu sram supply) Cheers! Angelo
Il 19/12/22 09:49, AngeloGioacchino Del Regno ha scritto: > Il 16/12/22 16:26, Matthias Brugger ha scritto: >> Hi Angelo, >> >> On 16/12/2022 14:15, AngeloGioacchino Del Regno wrote: >>> Il 21/11/22 13:01, AngeloGioacchino Del Regno ha scritto: >>>> Il 21/11/22 12:44, Matthias Brugger ha scritto: >>>>> >>>>> >>>>> On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: >>>>>> This driver currently deals with GPU-SRAM regulator coupling, ensuring >>>>>> that the SRAM voltage is always between a specific range of distance to >>>>>> the GPU voltage, depending on the SoC, necessary in order to achieve >>>>>> system stability across the full range of supported GPU frequencies. >>>>>> >>>>>> Signed-off-by: AngeloGioacchino Del Regno >>>>>> <angelogioacchino.delregno@collabora.com> >>>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> >>>>>> --- >>>>>> >>>>>> This driver was successfully tested for more than 3 months. >>>>>> GPU DVFS works correctly with no stability issues. >>>>>> >>>>>> Changes in RESEND,v3: >>>>>> Rebased over next-20221005 >>>>>> >>>>>> Changes in v3: >>>>>> - Added braces to else-if branch >>>>>> >>>>>> Changes in v2: >>>>>> - Added check for n_coupled >>>>>> - Added check for vgpu to enforce attaching to vgpu<->sram coupling only >>>>>> >>>>>> Context: >>>>>> This driver is the last of the pieces of a bigger puzzle, aiming to finally >>>>>> enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek >>>>>> SoCs on the fully open source graphics stack (Panfrost driver). >>>>>> >>>>>> No devicetree bindings are provided because this does not require any >>>>>> driver-specific binding at all. >>>>>> >>>>>> Last but not least: it was chosen to have this driver enabled for >>>>>> ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration >>>>>> choice because, once the DVFS mechanism will be fully working, using one >>>>>> of the listed MediaTek SoCs *without* this coupling mechanism *will* lead >>>>>> to unstabilities and system crashes. >>>>>> For COMPILE_TEST, choice is given for obvious reasons. >>>>>> >>>>>> drivers/soc/mediatek/Kconfig | 5 + >>>>>> drivers/soc/mediatek/Makefile | 1 + >>>>>> drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ >>>>>> 3 files changed, 165 insertions(+) >>>>>> create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c >>>>>> >>>>>> diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig >>>>>> index 40d0cc600cae..30b5afc0e51d 100644 >>>>>> --- a/drivers/soc/mediatek/Kconfig >>>>>> +++ b/drivers/soc/mediatek/Kconfig >>>>>> @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP >>>>>> on different MediaTek SoCs. The PMIC wrapper is a proprietary >>>>>> hardware to connect the PMIC. >>>>>> +config MTK_REGULATOR_COUPLER >>>>>> + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST >>>>>> + default ARCH_MEDIATEK >>>>>> + depends on REGULATOR >>>>>> + >>>>>> config MTK_SCPSYS >>>>>> bool "MediaTek SCPSYS Support" >>>>>> default ARCH_MEDIATEK >>>>>> diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile >>>>>> index 0e9e703c931a..8c0ddacbcde8 100644 >>>>>> --- a/drivers/soc/mediatek/Makefile >>>>>> +++ b/drivers/soc/mediatek/Makefile >>>>>> @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o >>>>>> obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o >>>>>> obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o >>>>>> obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o >>>>>> +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o >>>>>> obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o >>>>>> obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o >>>>>> obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o >>>>>> diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>>> b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>>> new file mode 100644 >>>>>> index 000000000000..ad2ed42aa697 >>>>>> --- /dev/null >>>>>> +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c >>>>>> @@ -0,0 +1,159 @@ >>>>>> +// SPDX-License-Identifier: GPL-2.0-only >>>>>> +/* >>>>>> + * Voltage regulators coupler for MediaTek SoCs >>>>>> + * >>>>>> + * Copyright (C) 2022 Collabora, Ltd. >>>>>> + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> >>>>>> + */ >>>>>> + >>>>>> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt >>>>>> + >>>>>> +#include <linux/init.h> >>>>>> +#include <linux/kernel.h> >>>>>> +#include <linux/of.h> >>>>>> +#include <linux/regulator/coupler.h> >>>>>> +#include <linux/regulator/driver.h> >>>>>> +#include <linux/regulator/machine.h> >>>>>> +#include <linux/suspend.h> >>>>>> + >>>>>> +#define to_mediatek_coupler(x) container_of(x, struct >>>>>> mediatek_regulator_coupler, coupler) >>>>>> + >>>>>> +struct mediatek_regulator_coupler { >>>>>> + struct regulator_coupler coupler; >>>>>> + struct regulator_dev *vsram_rdev; >>>>>> +}; >>>>>> + >>>>>> +/* >>>>>> + * We currently support only couples of not more than two vregs and >>>>>> + * modify the vsram voltage only when changing voltage of vgpu. >>>>>> + * >>>>>> + * This function is limited to the GPU<->SRAM voltages relationships. >>>>>> + */ >>>>>> +static int mediatek_regulator_balance_voltage(struct regulator_coupler >>>>>> *coupler, >>>>>> + struct regulator_dev *rdev, >>>>>> + suspend_state_t state) >>>>>> +{ >>>>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>>>> + int max_spread = rdev->constraints->max_spread[0]; >>>>>> + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; >>>>>> + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; >>>>>> + int vsram_target_min_uV, vsram_target_max_uV; >>>>>> + int min_uV = 0; >>>>>> + int max_uV = INT_MAX; >>>>>> + int ret; >>>>>> + >>>>>> + /* >>>>>> + * If the target device is on, setting the SRAM voltage directly >>>>>> + * is not supported as it scales through its coupled supply voltage. >>>>>> + * >>>>>> + * An exception is made in case the use_count is zero: this means >>>>>> + * that this is the first time we power up the SRAM regulator, which >>>>>> + * implies that the target device has yet to perform initialization >>>>>> + * and setting a voltage at that time is harmless. >>>>>> + */ >>>>>> + if (rdev == mrc->vsram_rdev) { >>>>>> + if (rdev->use_count == 0) >>>>>> + return regulator_do_balance_voltage(rdev, state, true); >>>>>> + >>>>>> + return -EPERM; >>>>>> + } >>>>>> + >>>>>> + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + if (min_uV == 0) { >>>>>> + ret = regulator_get_voltage_rdev(rdev); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + min_uV = ret; >>>>>> + } >>>>>> + >>>>>> + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); >>>>>> + if (ret < 0) >>>>>> + return ret; >>>>>> + >>>>>> + /* >>>>>> + * If we're asked to set a voltage less than VSRAM min_uV, set >>>>>> + * the minimum allowed voltage on VSRAM, as in this case it is >>>>>> + * safe to ignore the max_spread parameter. >>>>>> + */ >>>>>> + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); >>>>>> + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); >>>>>> + >>>>>> + /* Make sure we're not out of range */ >>>>>> + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); >>>>>> + >>>>>> + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", >>>>>> + vsram_target_min_uV, vsram_target_max_uV, >>>>>> + rdev_get_name(mrc->vsram_rdev), min_uV); >>>>>> + >>>>>> + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, >>>>>> + vsram_target_max_uV, state); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + >>>>>> + /* The sram voltage is now balanced: update the target vreg voltage */ >>>>>> + return regulator_do_balance_voltage(rdev, state, true); >>>>>> +} >>>>>> + >>>>>> +static int mediatek_regulator_attach(struct regulator_coupler *coupler, >>>>>> + struct regulator_dev *rdev) >>>>>> +{ >>>>>> + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); >>>>>> + const char *rdev_name = rdev_get_name(rdev); >>>>>> + >>>>>> + /* >>>>>> + * If we're getting a coupling of more than two regulators here and >>>>>> + * this means that this is surely not a GPU<->SRAM couple: in that >>>>>> + * case, we may want to use another coupler implementation, if any, >>>>>> + * or the generic one: the regulator core will keep walking through >>>>>> + * the list of couplers when any .attach_regulator() cb returns 1. >>>>>> + */ >>>>>> + if (rdev->coupling_desc.n_coupled > 2) >>>>>> + return 1; >>>>>> + >>>>>> + if (strstr(rdev_name, "sram")) { >>>>> >>>>> My understanding is, that we have to have either a DT node with regulator-name >>>>> = "sram" property to pollute rdev->constraints->name or some >>>>> regulator_desc->name populated in the drivers/regulator/mt*.c >>>>> >>>> >>>> No, it's not trying to find a regulator named "sram", but any regulator that >>>> *contains* the "sram" string in its name, but checks only regulators that are >>>> coupled to others. This is the same for the "Vgpu" / "vgpu". >>>> >>>> Example: >>>> >>>> Regulator A, coupled to Regulator B. >>>> >>>> Regulator A name = "Vgpu" or "vgpu", or *vgpu*, or *Vgpu* >>>> (name must contain either Vgpu or vgpu) >>>> >>>> Regulator B name = "vsram" or "sram_gpu" or *sram* >>>> (name must contain "sram"). >>>> >>>> mrc->vsram_rdev = rdev >>>> >>>> rdev == our Regulator B. >>>> >>>> We hence return 0 to the coupling API: this will produce the effect of making >>>> it use this driver's .balance_voltage() callback instead of the generic one >>>> on vgpu<->vsram. >>>> >>>> >>>>> I wasn't able to find either of this, so I wonder how this is supposed to >>>>> work. Please provide pointers to a working and complete implementation of >>>>> this, so that I'm able to judge what is going on and if the approach is the >>>>> correct one. >>>>> >>>>> I tried to figure out using mt8195-tracking-master-rolling >>>> >>>> That's the right branch. >>>> >>>> Let's take MT8192 Asurada as an example of how this works.... >>>> >>>> `mt6359_vsram_others_ldo_reg´ is the SRAM regulator for the GPU: >>>> https://gitlab.collabora.com/google/chromeos-kernel/-/blob/mt8195-tracking-master-rolling/arch/arm64/boot/dts/mediatek/mt8192-asurada.dtsi#L551 >>>> >>>> `mt6359_vsram_others_ldo_reg` regulator-name = "vsram_others"; >>>> ^^^^ >>>> Contains "sram", and this regulator is also >>>> >>>> regulator-coupled-with = <&mt6315_7_vbuck1>; >>>> >>>> `mt6315_7_vbuck1` regulator-name = "Vgpu"; >>>> ^^^^ >>>> Contains "Vgpu", and this regulator is also >>>> >>>> regulator-coupled-with = <&mt6359_vsram_others_ldo_reg>; >>>> >>>> That's how the coupling works in this case. >>>> >>>> >>>> Now, looking at case exclusions: >>>> In MT8192 Asurada (or, actually, mt6359.dtsi) we have other regulators that do >>>> actually contain "sram" in their name, like "vsram_proc1" and >>>> "vsram_others_sshub". >>>> >>>> These regulators will be ignored, as they are *not* coupled with Vgpu. >>>> >>>> >>>> What this driver currently does in this regard is: >>>> 1. Regulator attach is called only on *coupled* regulators, not on the others >>>> 2. If the regulator contains name "vgpu" or "Vgpu" or "sram" we're good, >>>> otherwise we don't attach the balance_voltage logic of this driver to >>>> the unmatched regulators. >>>> >>>> >>>> Does this explanation clarify your doubts? >>>> >>>> Regards, >>>> Angelo >>>> >>> >>> Matthias, please don't forget about this one :-) >>> >> >> Unfortunately I have to leave now and didn't had time to look more into it. So >> I'll write just my suspicion. So please feel free to correct me if I'm totally >> wrong (I know you will do anyway ;) >> >> 1) When the regulator-coupler driver was introduced the driver was bound by a >> compatible, but we use arch_initcall(...) for it. >> 2) I really don't like the hardcoded search for a the regulator name in the >> mediatek_regulator_attach function. I haven't seen that in the Nvidia driver, >> also I had no time to understand how they decide which regulators need to be >> coupled. My instinct would tell me that this should be described in device-tree. >> > > I didn't want to use custom devicetree properties to couple our regulators... > > Tegra wants to couple three regulators: CORE, RTC and CPU and they're getting that > with three custom properties: > * nvidia,tegra-core-regulator > * nvidia,tegra-rtc-regulator > * nvidia,tegra-gpu-regulator > > Hardcoding the regulator name *sub*string in the mediatek-regulator-coupler allows > us to use *only* generic devicetree properties instead of inventing our own: after > all, even in the case in which a board uses a regulator that doesn't have the VGPU > and/or VSRAM_GPU "kind of name" in it, it's still possible to change the name via > devicetree (which would be correct, because the regulator is used for that, and > would be the right name). > > As of now, there's no board using a regulator that has a not matching name, and > this is because MediaTek SoCs are always using a MediaTek PMIC in combination, > which always specifies this kind of name. > > I remind you that this driver does *nothing* unless coupling is activated through > devicetree propert*ies*, as this driver has to be seen as a hook to the regulator > coupling API, and not standalone code - hence, in my opinion, the cleanest > implementation of that is to use only properties provided by the regulator coupling > API, without any assistance from custom properties, unless *really* needed. > > Summarizing: > * Deciding which regulators need to be coupled *already needs to* be described > in devicetree, otherwise this driver will do nothing; > * The hardcoded strings are there only to restrict this driver to GPU related > regulators (vgpu as gpu core supply - vsram-gpu as gpu sram supply) > > Cheers! > Angelo > Hello Matthias, do-not-forget ping :-) Cheers, Angelo
On 06/10/2022 13:58, AngeloGioacchino Del Regno wrote: > This driver currently deals with GPU-SRAM regulator coupling, ensuring > that the SRAM voltage is always between a specific range of distance to > the GPU voltage, depending on the SoC, necessary in order to achieve > system stability across the full range of supported GPU frequencies. > > Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > Reviewed-by: Dmitry Osipenko <dmitry.osipenko@collabora.com> Applied, thanks! Matthias > --- > > This driver was successfully tested for more than 3 months. > GPU DVFS works correctly with no stability issues. > > Changes in RESEND,v3: > Rebased over next-20221005 > > Changes in v3: > - Added braces to else-if branch > > Changes in v2: > - Added check for n_coupled > - Added check for vgpu to enforce attaching to vgpu<->sram coupling only > > Context: > This driver is the last of the pieces of a bigger puzzle, aiming to finally > enable Dynamic Voltage/Frequency Scaling for Mali GPUs found on MediaTek > SoCs on the fully open source graphics stack (Panfrost driver). > > No devicetree bindings are provided because this does not require any > driver-specific binding at all. > > Last but not least: it was chosen to have this driver enabled for > ( ARCH_MEDIATEK && REGULATOR ) without really giving a free configuration > choice because, once the DVFS mechanism will be fully working, using one > of the listed MediaTek SoCs *without* this coupling mechanism *will* lead > to unstabilities and system crashes. > For COMPILE_TEST, choice is given for obvious reasons. > > drivers/soc/mediatek/Kconfig | 5 + > drivers/soc/mediatek/Makefile | 1 + > drivers/soc/mediatek/mtk-regulator-coupler.c | 159 +++++++++++++++++++ > 3 files changed, 165 insertions(+) > create mode 100644 drivers/soc/mediatek/mtk-regulator-coupler.c > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig > index 40d0cc600cae..30b5afc0e51d 100644 > --- a/drivers/soc/mediatek/Kconfig > +++ b/drivers/soc/mediatek/Kconfig > @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP > on different MediaTek SoCs. The PMIC wrapper is a proprietary > hardware to connect the PMIC. > > +config MTK_REGULATOR_COUPLER > + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST > + default ARCH_MEDIATEK > + depends on REGULATOR > + > config MTK_SCPSYS > bool "MediaTek SCPSYS Support" > default ARCH_MEDIATEK > diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile > index 0e9e703c931a..8c0ddacbcde8 100644 > --- a/drivers/soc/mediatek/Makefile > +++ b/drivers/soc/mediatek/Makefile > @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o > obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o > obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o > obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o > +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o > obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o > obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o > obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o > diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c > new file mode 100644 > index 000000000000..ad2ed42aa697 > --- /dev/null > +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c > @@ -0,0 +1,159 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Voltage regulators coupler for MediaTek SoCs > + * > + * Copyright (C) 2022 Collabora, Ltd. > + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include <linux/init.h> > +#include <linux/kernel.h> > +#include <linux/of.h> > +#include <linux/regulator/coupler.h> > +#include <linux/regulator/driver.h> > +#include <linux/regulator/machine.h> > +#include <linux/suspend.h> > + > +#define to_mediatek_coupler(x) container_of(x, struct mediatek_regulator_coupler, coupler) > + > +struct mediatek_regulator_coupler { > + struct regulator_coupler coupler; > + struct regulator_dev *vsram_rdev; > +}; > + > +/* > + * We currently support only couples of not more than two vregs and > + * modify the vsram voltage only when changing voltage of vgpu. > + * > + * This function is limited to the GPU<->SRAM voltages relationships. > + */ > +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, > + struct regulator_dev *rdev, > + suspend_state_t state) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + int max_spread = rdev->constraints->max_spread[0]; > + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; > + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; > + int vsram_target_min_uV, vsram_target_max_uV; > + int min_uV = 0; > + int max_uV = INT_MAX; > + int ret; > + > + /* > + * If the target device is on, setting the SRAM voltage directly > + * is not supported as it scales through its coupled supply voltage. > + * > + * An exception is made in case the use_count is zero: this means > + * that this is the first time we power up the SRAM regulator, which > + * implies that the target device has yet to perform initialization > + * and setting a voltage at that time is harmless. > + */ > + if (rdev == mrc->vsram_rdev) { > + if (rdev->use_count == 0) > + return regulator_do_balance_voltage(rdev, state, true); > + > + return -EPERM; > + } > + > + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); > + if (ret < 0) > + return ret; > + > + if (min_uV == 0) { > + ret = regulator_get_voltage_rdev(rdev); > + if (ret < 0) > + return ret; > + min_uV = ret; > + } > + > + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); > + if (ret < 0) > + return ret; > + > + /* > + * If we're asked to set a voltage less than VSRAM min_uV, set > + * the minimum allowed voltage on VSRAM, as in this case it is > + * safe to ignore the max_spread parameter. > + */ > + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); > + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); > + > + /* Make sure we're not out of range */ > + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); > + > + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", > + vsram_target_min_uV, vsram_target_max_uV, > + rdev_get_name(mrc->vsram_rdev), min_uV); > + > + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, > + vsram_target_max_uV, state); > + if (ret) > + return ret; > + > + /* The sram voltage is now balanced: update the target vreg voltage */ > + return regulator_do_balance_voltage(rdev, state, true); > +} > + > +static int mediatek_regulator_attach(struct regulator_coupler *coupler, > + struct regulator_dev *rdev) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + const char *rdev_name = rdev_get_name(rdev); > + > + /* > + * If we're getting a coupling of more than two regulators here and > + * this means that this is surely not a GPU<->SRAM couple: in that > + * case, we may want to use another coupler implementation, if any, > + * or the generic one: the regulator core will keep walking through > + * the list of couplers when any .attach_regulator() cb returns 1. > + */ > + if (rdev->coupling_desc.n_coupled > 2) > + return 1; > + > + if (strstr(rdev_name, "sram")) { > + if (mrc->vsram_rdev) > + return -EINVAL; > + mrc->vsram_rdev = rdev; > + } else if (!strstr(rdev_name, "vgpu") && !strstr(rdev_name, "Vgpu")) { > + return 1; > + } > + > + return 0; > +} > + > +static int mediatek_regulator_detach(struct regulator_coupler *coupler, > + struct regulator_dev *rdev) > +{ > + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); > + > + if (rdev == mrc->vsram_rdev) > + mrc->vsram_rdev = NULL; > + > + return 0; > +} > + > +static struct mediatek_regulator_coupler mediatek_coupler = { > + .coupler = { > + .attach_regulator = mediatek_regulator_attach, > + .detach_regulator = mediatek_regulator_detach, > + .balance_voltage = mediatek_regulator_balance_voltage, > + }, > +}; > + > +static int mediatek_regulator_coupler_init(void) > +{ > + if (!of_machine_is_compatible("mediatek,mt8183") && > + !of_machine_is_compatible("mediatek,mt8186") && > + !of_machine_is_compatible("mediatek,mt8192")) > + return 0; > + > + return regulator_coupler_register(&mediatek_coupler.coupler); > +} > +arch_initcall(mediatek_regulator_coupler_init); > + > +MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>"); > +MODULE_DESCRIPTION("MediaTek Regulator Coupler driver"); > +MODULE_LICENSE("GPL");
diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig index 40d0cc600cae..30b5afc0e51d 100644 --- a/drivers/soc/mediatek/Kconfig +++ b/drivers/soc/mediatek/Kconfig @@ -44,6 +44,11 @@ config MTK_PMIC_WRAP on different MediaTek SoCs. The PMIC wrapper is a proprietary hardware to connect the PMIC. +config MTK_REGULATOR_COUPLER + bool "MediaTek SoC Regulator Coupler" if COMPILE_TEST + default ARCH_MEDIATEK + depends on REGULATOR + config MTK_SCPSYS bool "MediaTek SCPSYS Support" default ARCH_MEDIATEK diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile index 0e9e703c931a..8c0ddacbcde8 100644 --- a/drivers/soc/mediatek/Makefile +++ b/drivers/soc/mediatek/Makefile @@ -3,6 +3,7 @@ obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o +obj-$(CONFIG_MTK_REGULATOR_COUPLER) += mtk-regulator-coupler.o obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o obj-$(CONFIG_MTK_SCPSYS_PM_DOMAINS) += mtk-pm-domains.o obj-$(CONFIG_MTK_MMSYS) += mtk-mmsys.o diff --git a/drivers/soc/mediatek/mtk-regulator-coupler.c b/drivers/soc/mediatek/mtk-regulator-coupler.c new file mode 100644 index 000000000000..ad2ed42aa697 --- /dev/null +++ b/drivers/soc/mediatek/mtk-regulator-coupler.c @@ -0,0 +1,159 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Voltage regulators coupler for MediaTek SoCs + * + * Copyright (C) 2022 Collabora, Ltd. + * Author: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/of.h> +#include <linux/regulator/coupler.h> +#include <linux/regulator/driver.h> +#include <linux/regulator/machine.h> +#include <linux/suspend.h> + +#define to_mediatek_coupler(x) container_of(x, struct mediatek_regulator_coupler, coupler) + +struct mediatek_regulator_coupler { + struct regulator_coupler coupler; + struct regulator_dev *vsram_rdev; +}; + +/* + * We currently support only couples of not more than two vregs and + * modify the vsram voltage only when changing voltage of vgpu. + * + * This function is limited to the GPU<->SRAM voltages relationships. + */ +static int mediatek_regulator_balance_voltage(struct regulator_coupler *coupler, + struct regulator_dev *rdev, + suspend_state_t state) +{ + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); + int max_spread = rdev->constraints->max_spread[0]; + int vsram_min_uV = mrc->vsram_rdev->constraints->min_uV; + int vsram_max_uV = mrc->vsram_rdev->constraints->max_uV; + int vsram_target_min_uV, vsram_target_max_uV; + int min_uV = 0; + int max_uV = INT_MAX; + int ret; + + /* + * If the target device is on, setting the SRAM voltage directly + * is not supported as it scales through its coupled supply voltage. + * + * An exception is made in case the use_count is zero: this means + * that this is the first time we power up the SRAM regulator, which + * implies that the target device has yet to perform initialization + * and setting a voltage at that time is harmless. + */ + if (rdev == mrc->vsram_rdev) { + if (rdev->use_count == 0) + return regulator_do_balance_voltage(rdev, state, true); + + return -EPERM; + } + + ret = regulator_check_consumers(rdev, &min_uV, &max_uV, state); + if (ret < 0) + return ret; + + if (min_uV == 0) { + ret = regulator_get_voltage_rdev(rdev); + if (ret < 0) + return ret; + min_uV = ret; + } + + ret = regulator_check_voltage(rdev, &min_uV, &max_uV); + if (ret < 0) + return ret; + + /* + * If we're asked to set a voltage less than VSRAM min_uV, set + * the minimum allowed voltage on VSRAM, as in this case it is + * safe to ignore the max_spread parameter. + */ + vsram_target_min_uV = max(vsram_min_uV, min_uV + max_spread); + vsram_target_max_uV = min(vsram_max_uV, vsram_target_min_uV + max_spread); + + /* Make sure we're not out of range */ + vsram_target_min_uV = min(vsram_target_min_uV, vsram_max_uV); + + pr_debug("Setting voltage %d-%duV on %s (minuV %d)\n", + vsram_target_min_uV, vsram_target_max_uV, + rdev_get_name(mrc->vsram_rdev), min_uV); + + ret = regulator_set_voltage_rdev(mrc->vsram_rdev, vsram_target_min_uV, + vsram_target_max_uV, state); + if (ret) + return ret; + + /* The sram voltage is now balanced: update the target vreg voltage */ + return regulator_do_balance_voltage(rdev, state, true); +} + +static int mediatek_regulator_attach(struct regulator_coupler *coupler, + struct regulator_dev *rdev) +{ + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); + const char *rdev_name = rdev_get_name(rdev); + + /* + * If we're getting a coupling of more than two regulators here and + * this means that this is surely not a GPU<->SRAM couple: in that + * case, we may want to use another coupler implementation, if any, + * or the generic one: the regulator core will keep walking through + * the list of couplers when any .attach_regulator() cb returns 1. + */ + if (rdev->coupling_desc.n_coupled > 2) + return 1; + + if (strstr(rdev_name, "sram")) { + if (mrc->vsram_rdev) + return -EINVAL; + mrc->vsram_rdev = rdev; + } else if (!strstr(rdev_name, "vgpu") && !strstr(rdev_name, "Vgpu")) { + return 1; + } + + return 0; +} + +static int mediatek_regulator_detach(struct regulator_coupler *coupler, + struct regulator_dev *rdev) +{ + struct mediatek_regulator_coupler *mrc = to_mediatek_coupler(coupler); + + if (rdev == mrc->vsram_rdev) + mrc->vsram_rdev = NULL; + + return 0; +} + +static struct mediatek_regulator_coupler mediatek_coupler = { + .coupler = { + .attach_regulator = mediatek_regulator_attach, + .detach_regulator = mediatek_regulator_detach, + .balance_voltage = mediatek_regulator_balance_voltage, + }, +}; + +static int mediatek_regulator_coupler_init(void) +{ + if (!of_machine_is_compatible("mediatek,mt8183") && + !of_machine_is_compatible("mediatek,mt8186") && + !of_machine_is_compatible("mediatek,mt8192")) + return 0; + + return regulator_coupler_register(&mediatek_coupler.coupler); +} +arch_initcall(mediatek_regulator_coupler_init); + +MODULE_AUTHOR("AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>"); +MODULE_DESCRIPTION("MediaTek Regulator Coupler driver"); +MODULE_LICENSE("GPL");