[RESEND PATCH v3] soc: mediatek: Introduce mediatek-regulator-coupler driver
AngeloGioacchino Del Regno
angelogioacchino.delregno at collabora.com
Fri Dec 16 05:15:02 PST 2022
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 at collabora.com>
>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko at 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 at 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
More information about the Linux-mediatek
mailing list