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

["Xueqi Zhang (张雪琦)" <Xueqi.Zhang@mediatek.com>]

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

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 RPM operations for
these SMI commons.

What do you think about this approach?

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;
> >   }
> 
>