Re: [PATCH 2/3] memory: mtk-smi: Add a flag skip_rpm

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

On 5/18/26 09:16, Xueqi Zhang (张雪琦) wrote:
> Hi Angelo,
> 
> First of all, please accept my apologies for the delayed response. I
> have been deeply occupied with MT8196 Aluminium pKVM SMMU and SMI
> related tasks recently.
> 
> Regarding your question, my previous description in the patch was not
> accurate enough and may have caused some confusion. In fact, not
> all SMI commons have their backup/restore handled by the RTFF
> hardware. The SMI commons are distributed across various subsystems
> (e.g., mminfra, venc, display, cam, etc.). Currently, only the SMI
> common under the mminfra subsystem is backed up and restored by
> the RTFF hardware.
> 
> Therefore, I believe adding a specific 'skip_rpm' flag is more
> appropriate here. If we were to differentiate this based on a new
> MTK_SMI_GEN3 type, it would imply that all SMI common modules of
> that generation would skip the RPM operations, which is not the
> intended behavior.
> 
> To make this clearer, I plan to update the commit message in the
> next version as follows:
> 
> Subject: memory: mtk-smi: Add skip_rpm flag for certain MT8196 SMI
> commons

memory: mtk-smi: Allow no clocks for RTFF managed SMI commons

> 
> Body:
> On MT8196, certain SMI commons are backed up and restored by the RTFF
> hardware rather than by software.
> 
> For these specific SMI commons, software-controlled register backup
> and restore in the runtime callback is no longer necessary. Therefore,
> introduce a 'skip_rpm' flag to bypass these redundant RPMoperations
> for these SMI commons.
> 
> What do you think about this approach?
> 

That would be kind-of ok, but keep in mind: pm_runtime doesn't only manage clocks.

I think that the best option here would be to allow having no clocks instead,
and to still call pm_runtime_{en,dis}able() - as that would get a bit more
future-proof, should any other (newer, older, etc) SoC need to declare any power
domain but still no clocks.

So at this point, I think that just doing something like:

	if (common->plat->has_gals) {
		if (common->plat->rtff_managed) <--- not "skip_rpm"
			clk_required = 0;
		else if (common->plat->type == MTK_SMI_GEN2)
			clk_required = MTK_SMI_COM_GALS_REQ_CLK_NR;
		else if (common->plat->type == MTK_SMI_GEN2_SUB_COMM)
			clk_required = MTK_SMI_SUB_COM_GALS_REQ_CLK_NR;
	}

should be sufficient (and/or check zero required clocks in smi_dts_clk_init).

Cheers,
Angelo

> Thanks,
> Xueqi
> 
> On Thu, 2025-03-20 at 13:11 +0100, AngeloGioacchino Del Regno wrote:
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>>
>>
>> Il 20/03/25 08:36, Xueqi Zhang ha scritto:
>>> MT8196 SMI commons is backed up/restored by RTFF HW.
>>> It doesn't need SW control the register backup/store
>>> in the runtime callback.Therefore, add a flag skip_rpm
>>> to help skip RPM operations for SMI commons.
>>>
>>> Signed-off-by: Xueqi Zhang <xueqi.zhang@mediatek.com>
>>
>> So the MT8196 SMI common doesn't require any clocks?
>>
>> That's fine for me, but this looks bloody similar to MT6989's SMI
>> common, which
>> is SMI GEN3 and not GEN2....
>>
>> ....so, are you sure that you need a `skip_rpm` flag and not new
>> MTK_SMI_GEN3 and
>> MTK_SMI_GEN3_SUB_COMM types? :-)
>>
>> Regards,
>> Angelo
>>
>>> ---
>>>    drivers/memory/mtk-smi.c | 11 ++++++++---
>>>    1 file changed, 8 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
>>> index a8f5467d6b31..b9affa3c3185 100644
>>> --- a/drivers/memory/mtk-smi.c
>>> +++ b/drivers/memory/mtk-smi.c
>>> @@ -123,6 +123,7 @@ static const char * const mtk_smi_common_clks[]
>>> = {"apb", "smi", "gals0", "gals1
>>>    struct mtk_smi_common_plat {
>>>        enum mtk_smi_type       type;
>>>        bool                    has_gals;
>>> +     bool                    skip_rpm;
>>>        u32                     bus_sel; /* Balance some larbs to
>>> enter mmu0 or mmu1 */
>>>
>>>        const struct mtk_smi_reg_pair   *init;
>>> @@ -547,6 +548,9 @@ static int mtk_smi_dts_clk_init(struct device
>>> *dev, struct mtk_smi *smi,
>>>    {
>>>        int i, ret;
>>>
>>> +     if (smi->plat->skip_rpm)
>>> +             return 0;
>>> +
>>>        for (i = 0; i < clk_nr_required; i++)
>>>                smi->clks[i].id = clks[i];
>>>        ret = devm_clk_bulk_get(dev, clk_nr_required, smi->clks);
>>> @@ -783,7 +787,7 @@ static int mtk_smi_common_probe(struct
>>> platform_device *pdev)
>>>        common->dev = dev;
>>>        common->plat = of_device_get_match_data(dev);
>>>
>>> -     if (common->plat->has_gals) {
>>> +     if (!common->plat->skip_rpm && common->plat->has_gals) {
>>>                if (common->plat->type == MTK_SMI_GEN2)
>>>                        clk_required = MTK_SMI_COM_GALS_REQ_CLK_NR;
>>>                else if (common->plat->type == MTK_SMI_GEN2_SUB_COMM)
>>> @@ -814,13 +818,14 @@ static int mtk_smi_common_probe(struct
>>> platform_device *pdev)
>>>        }
>>>
>>>        /* link its smi-common if this is smi-sub-common */
>>> -     if (common->plat->type == MTK_SMI_GEN2_SUB_COMM) {
>>> +     if (common->plat->type == MTK_SMI_GEN2_SUB_COMM && !common-
>>>> plat->skip_rpm) {
>>>                ret = mtk_smi_device_link_common(dev, &common-
>>>> smi_common_dev);
>>>                if (ret < 0)
>>>                        return ret;
>>>        }
>>>
>>> -     pm_runtime_enable(dev);
>>> +     if (!common->plat->skip_rpm)
>>> +             pm_runtime_enable(dev);
>>>        platform_set_drvdata(pdev, common);
>>>        return 0;
>>>    }
>>
>>