Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fe236ad5-c4e0-1649-6521-ab7c14a1ff15@collabora.com>
Date:   Mon, 21 Nov 2022 13:01:02 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.5.0
Subject: Re: [RESEND PATCH v3] soc: mediatek: Introduce
 mediatek-regulator-coupler driver
Content-Language: en-US
To:     Matthias Brugger <matthias.bgg@gmail.com>
Cc:     linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, wenst@chromium.org,
        alyssa.rosenzweig@collabora.com, nfraprado@collabora.com,
        dmitry.osipenko@collabora.com
References: <20221006115816.66853-1-angelogioacchino.delregno@collabora.com>
 <e04d15ee-9570-c919-5218-91109a88c0cb@gmail.com>
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>
In-Reply-To: <e04d15ee-9570-c919-5218-91109a88c0cb@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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