Messages in this thread | | | Date | Mon, 21 Nov 2022 13:01:02 +0100 | Subject | Re: [RESEND PATCH v3] soc: mediatek: Introduce mediatek-regulator-coupler driver | From | AngeloGioacchino Del Regno <> |
| |
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
| |