Message-ID: <b10e5f06-379b-7aaf-137b-60f4176ae325@collabora.com>
Date:   Tue, 10 Jan 2023 10:06:33 +0100
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Matthias Brugger <matthias.bgg@...il.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

Il 19/12/22 09:49, AngeloGioacchino Del Regno ha scritto:
> Il 16/12/22 16:26, Matthias Brugger ha scritto:
>> 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.
>>
> 
> I didn't want to use custom devicetree properties to couple our regulators...
> 
> Tegra wants to couple three regulators: CORE, RTC and CPU and they're getting that
> with three custom properties:
> * nvidia,tegra-core-regulator
> * nvidia,tegra-rtc-regulator
> * nvidia,tegra-gpu-regulator
> 
> Hardcoding the regulator name *sub*string in the mediatek-regulator-coupler allows
> us to use *only* generic devicetree properties instead of inventing our own: after
> all, even in the case in which a board uses a regulator that doesn't have the VGPU
> and/or VSRAM_GPU "kind of name" in it, it's still possible to change the name via
> devicetree (which would be correct, because the regulator is used for that, and
> would be the right name).
> 
> As of now, there's no board using a regulator that has a not matching name, and
> this is because MediaTek SoCs are always using a MediaTek PMIC in combination,
> which always specifies this kind of name.
> 
> I remind you that this driver does *nothing* unless coupling is activated through
> devicetree propert*ies*, as this driver has to be seen as a hook to the regulator
> coupling API, and not standalone code - hence, in my opinion, the cleanest
> implementation of that is to use only properties provided by the regulator coupling
> API, without any assistance from custom properties, unless *really* needed.
> 
> Summarizing:
> * Deciding which regulators need to be coupled *already needs to* be described
>    in devicetree, otherwise this driver will do nothing;
> * The hardcoded strings are there only to restrict this driver to GPU related
>    regulators (vgpu as gpu core supply - vsram-gpu as gpu sram supply)
> 
> Cheers!
> Angelo
> 

Hello Matthias,

do-not-forget ping :-)

Cheers,
Angelo

Powered by blists - more mailing lists