Messages in this thread | | | Date | Mon, 21 Nov 2022 12:44:41 +0100 | From | Matthias Brugger <> | Subject | Re: [RESEND PATCH v3] soc: mediatek: Introduce mediatek-regulator-coupler driver |
| |
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");
| |