Date:   Fri, 16 Dec 2022 16:26:23 +0100
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org, wenst@...omium.org,
        alyssa.rosenzweig@...labora.com, nfraprado@...labora.com,
        dmitry.osipenko@...labora.com
Subject: Re: [RESEND PATCH v3] soc: mediatek: Introduce
 mediatek-regulator-coupler driver

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@...labora.com>
>>>> Reviewed-by: Dmitry Osipenko <dmitry.osipenko@...labora.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@...labora.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

Powered by blists - more mailing lists