Re: [PATCH v2 2/2] memory: mtk-smi: Add support for MT6795 Helio X10

[AngeloGioacchino Del Regno]

Il 17/05/22 11:44, Yong Wu ha scritto:
> On Tue, 2022-05-17 at 10:27 +0200, AngeloGioacchino Del Regno wrote:
> > Il 17/05/22 08:37, Yong Wu ha scritto:
> > > On Fri, 2022-05-13 at 17:06 +0200, AngeloGioacchino Del Regno
> > > wrote:
> > > > The MediaTek Helio X10 (MT6795) SoC has 5 LARBs and one common
> > > > SMI
> > > > instance without any sub-common and without GALS.
> > > >
> > > > While the smi-common configuration is specific to this SoC, on
> > > > the
> > > > LARB side, this is similar to MT8173, in the sense that it
> > > > doesn't
> > > > need the port in LARB, and the register layout is also compatible
> > > > with that one, which makes us able to fully reuse the smi-larb
> > > > platform data struct that was introduced for MT8173.
> > > >
> > > > Signed-off-by: AngeloGioacchino Del Regno <
> > > > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > > ---
> > > >    drivers/memory/mtk-smi.c | 17 +++++++++++++++++
> > > >    1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > > > index 86a3d34f418e..7e7c3ede19e4 100644
> > > > --- a/drivers/memory/mtk-smi.c
> > > > +++ b/drivers/memory/mtk-smi.c
> > > > @@ -21,11 +21,13 @@
> > > >    /* SMI COMMON */
> > > >    #define SMI_L1LEN			0x100
> > > >    
> > > > +#define SMI_L1_ARB			0x200
> > > >    #define SMI_BUS_SEL			0x220
> > > >    #define SMI_BUS_LARB_SHIFT(larbid)	((larbid) << 1)
> > > >    /* All are MMU0 defaultly. Only specialize mmu1 here. */
> > > >    #define F_MMU1_LARB(larbid)		(0x1 <<
> > > > SMI_BUS_LARB_SHIFT(larbid))
> > > >    
> > > > +#define SMI_FIFO_TH0			0x230
> > >
> > > Does the name come from the coda you got?
> > > It is called SMI_READ_FIFO_TH in my coda.
> >
> > Documentation for this SoC is not public and I have no access to it,
> > so
> > everything that you see here comes from reading downstream kernel
> > code :-(
> >
> > I'll change the name to SMI_READ_FIFO_TH as suggested, thanks!
> >
> > > >    #define SMI_M4U_TH			0x234
> > > >    #define SMI_FIFO_TH1			0x238
> > > >    #define SMI_FIFO_TH2			0x23c
> > > > @@ -360,6 +362,7 @@ static const struct of_device_id
> > > > mtk_smi_larb_of_ids[] = {
> > > >    	{.compatible = "mediatek,mt2701-smi-larb", .data =
> > > > &mtk_smi_larb_mt2701},
> > > >    	{.compatible = "mediatek,mt2712-smi-larb", .data =
> > > > &mtk_smi_larb_mt2712},
> > > >    	{.compatible = "mediatek,mt6779-smi-larb", .data =
> > > > &mtk_smi_larb_mt6779},
> > > > +	{.compatible = "mediatek,mt6795-smi-larb", .data =
> > > > &mtk_smi_larb_mt8173},
> > > >    	{.compatible = "mediatek,mt8167-smi-larb", .data =
> > > > &mtk_smi_larb_mt8167},
> > > >    	{.compatible = "mediatek,mt8173-smi-larb", .data =
> > > > &mtk_smi_larb_mt8173},
> > > >    	{.compatible = "mediatek,mt8183-smi-larb", .data =
> > > > &mtk_smi_larb_mt8183},
> > > > @@ -541,6 +544,13 @@ static struct platform_driver
> > > > mtk_smi_larb_driver = {
> > > >    	}
> > > >    };
> > > >    
> > > > +static const struct mtk_smi_reg_pair
> > > > mtk_smi_common_mt6795_init[SMI_COMMON_INIT_REGS_NR] = {
> > > > +	{SMI_L1_ARB, 0x1b},
> > > > +	{SMI_M4U_TH, 0xce810c85},
> > > > +	{SMI_FIFO_TH1, 0x43214c8},
> > > > +	{SMI_FIFO_TH0, 0x191f},
> > > > +};
> > > > +
> > > >    static const struct mtk_smi_reg_pair
> > > > mtk_smi_common_mt8195_init[SMI_COMMON_INIT_REGS_NR] = {
> > > >    	{SMI_L1LEN, 0xb},
> > > >    	{SMI_M4U_TH, 0xe100e10},
> > > > @@ -565,6 +575,12 @@ static const struct mtk_smi_common_plat
> > > > mtk_smi_common_mt6779 = {
> > > >    		    F_MMU1_LARB(5) | F_MMU1_LARB(6) |
> > > > F_MMU1_LARB(7),
> > > >    };
> > > >    
> > > > +static const struct mtk_smi_common_plat mtk_smi_common_mt6795 =
> > > > {
> > > > +	.type	  = MTK_SMI_GEN2,
> > > > +	.bus_sel  = BIT(0),
> > >
> > > Like the other larbs, use F_MMU1_LARB(0) here?
> >
> > I agree that F_MMU1_LARB(0) == (1 << (0 << 1)) == BIT(0), but that
> > would
> > not be correct and induce other people to mistake, I think?
>
> F_MMU1_LARB(x) means larbx enter MMU1. this is correct for me.
>
> OK. Maybe the macro name is not good. About the macro background,
> please see:
> 567e58cf96dd (memory: mtk-smi: Add bus_sel for mt8183)
>
> If you have better name for this, please tell me:)

I checked that commit. It's not about the macro name... this confusion
would've been avoided if there was a better comment in the code that
explained what was actually going on with that bus selection mechanism.

No worries though, I'll take care of that and will try to write a good
and short explanation for that macro in the code, so that the next
developer trying to do the same will not be induced into such big
misunderstanding of what's going on here.

By the way - now that I know - that bus switching is pretty smart, I
wonder if there's any way to dynamically switch them to eventually save
power by entering some sort of power saving on MMU1 when unused, and/or
achieve better performance in heavy workloads... but I will leave that
improvement idea to you :-)

> > Downstream doesn't do MMU1 bits, but MMU0 in this case... but if you
> > can
> > check on internal documentation and confirm that the downstream
> > kernel's
> > logic is wrong on that - and that you've verified that this should
>
> I don't know the detailed downstream code, But I find a internal branch
> about this SoC. I see the bus_sel did set to 0x1 as you did here. thus
> I don't think the downstream kernel is wrong. 0x1 means larb0 enter
> MMU1 while the others still enter MMU0. we could use F_MMU1_LARB(0)
> here.

I promised a bigger thank you, so there you go: THANK YOU! :-)

At this point, I definitely agree about using F_MMU1_LARB(0) for MT6795.

Cheers,
Angelo