Re: [RESEND PATCH v3] soc: mediatek: Introduce mediatek-regulator-coupler driver
From: Matthias Brugger
Date: Fri Dec 16 2022 - 10:26:39 EST
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@xxxxxxxxxxxxx>
Reviewed-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxx>
+ */
+
+#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