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 0x230Does 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_pairmtk_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 shouldI 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
