[PATCH v2 0/2] MediaTek Helio X10 MT6795 - SMI Support

[PATCH v2 0/2] MediaTek Helio X10 MT6795 - SMI Support

[AngeloGioacchino Del Regno]

In an effort to give some love to the apparently forgotten MT6795 SoC,
I am upstreaming more components that are necessary to support platforms
powered by this one apart from a simple boot to serial console.

This series introduces support for the SMI common and LARBs, found in
this SoC.

Tested on a MT6795 Sony Xperia M5 (codename "Holly") smartphone.

Changes in v2:
 - Added forgotten new definitions

AngeloGioacchino Del Regno (2):
  dt-bindings: memory: mtk-smi: Add MT6795 Helio X10 bindings
  memory: mtk-smi: Add support for MT6795 Helio X10

 .../memory-controllers/mediatek,smi-common.yaml |  1 +
 .../memory-controllers/mediatek,smi-larb.yaml   |  1 +
 drivers/memory/mtk-smi.c                        | 17 +++++++++++++++++
 3 files changed, 19 insertions(+)

-- 
2.35.1

[PATCH v2 1/2] dt-bindings: memory: mtk-smi: Add MT6795 Helio X10 bindings

[AngeloGioacchino Del Regno]

Add SMI bindings for the MediaTek Helio X10 (MT6795) SoC

Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
---
 .../bindings/memory-controllers/mediatek,smi-common.yaml         | 1 +
 .../bindings/memory-controllers/mediatek,smi-larb.yaml           | 1 +
 2 files changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
index a98b359bf909..71bc5cefb49c 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
@@ -32,6 +32,7 @@ properties:
           - mediatek,mt2701-smi-common
           - mediatek,mt2712-smi-common
           - mediatek,mt6779-smi-common
+          - mediatek,mt6795-smi-common
           - mediatek,mt8167-smi-common
           - mediatek,mt8173-smi-common
           - mediatek,mt8183-smi-common
diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
index c886681f62a7..59dcd163668f 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
+++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
@@ -20,6 +20,7 @@ properties:
           - mediatek,mt2701-smi-larb
           - mediatek,mt2712-smi-larb
           - mediatek,mt6779-smi-larb
+          - mediatek,mt6795-smi-larb
           - mediatek,mt8167-smi-larb
           - mediatek,mt8173-smi-larb
           - mediatek,mt8183-smi-larb
-- 
2.35.1

Re: [PATCH v2 1/2] dt-bindings: memory: mtk-smi: Add MT6795 Helio X10 bindings

[Matthias Brugger]

On 13/05/2022 17:06, AngeloGioacchino Del Regno wrote:
> Add SMI bindings for the MediaTek Helio X10 (MT6795) SoC
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   .../bindings/memory-controllers/mediatek,smi-common.yaml         | 1 +
>   .../bindings/memory-controllers/mediatek,smi-larb.yaml           | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> index a98b359bf909..71bc5cefb49c 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -32,6 +32,7 @@ properties:
>             - mediatek,mt2701-smi-common
>             - mediatek,mt2712-smi-common
>             - mediatek,mt6779-smi-common
> +          - mediatek,mt6795-smi-common
>             - mediatek,mt8167-smi-common
>             - mediatek,mt8173-smi-common
>             - mediatek,mt8183-smi-common
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> index c886681f62a7..59dcd163668f 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> +++ b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
> @@ -20,6 +20,7 @@ properties:
>             - mediatek,mt2701-smi-larb
>             - mediatek,mt2712-smi-larb
>             - mediatek,mt6779-smi-larb
> +          - mediatek,mt6795-smi-larb
>             - mediatek,mt8167-smi-larb
>             - mediatek,mt8173-smi-larb
>             - mediatek,mt8183-smi-larb

Re: [PATCH v2 1/2] dt-bindings: memory: mtk-smi: Add MT6795 Helio X10 bindings

[Rob Herring]

On Fri, 13 May 2022 17:06:32 +0200, AngeloGioacchino Del Regno wrote:
> Add SMI bindings for the MediaTek Helio X10 (MT6795) SoC
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
>  .../bindings/memory-controllers/mediatek,smi-common.yaml         | 1 +
>  .../bindings/memory-controllers/mediatek,smi-larb.yaml           | 1 +
>  2 files changed, 2 insertions(+)
> 

Acked-by: Rob Herring <robh@kernel.org>

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

[AngeloGioacchino Del Regno]

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@collabora.com>
---
 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
 #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),
+	.init     = mtk_smi_common_mt6795_init,
+};
+
 static const struct mtk_smi_common_plat mtk_smi_common_mt8183 = {
 	.type     = MTK_SMI_GEN2,
 	.has_gals = true,
@@ -609,6 +625,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = {
 	{.compatible = "mediatek,mt2701-smi-common", .data = &mtk_smi_common_gen1},
 	{.compatible = "mediatek,mt2712-smi-common", .data = &mtk_smi_common_gen2},
 	{.compatible = "mediatek,mt6779-smi-common", .data = &mtk_smi_common_mt6779},
+	{.compatible = "mediatek,mt6795-smi-common", .data = &mtk_smi_common_mt6795},
 	{.compatible = "mediatek,mt8167-smi-common", .data = &mtk_smi_common_gen2},
 	{.compatible = "mediatek,mt8173-smi-common", .data = &mtk_smi_common_gen2},
 	{.compatible = "mediatek,mt8183-smi-common", .data = &mtk_smi_common_mt8183},
-- 
2.35.1

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

[Matthias Brugger]

On 13/05/2022 17:06, 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@collabora.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>   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
>   #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),
> +	.init     = mtk_smi_common_mt6795_init,
> +};
> +
>   static const struct mtk_smi_common_plat mtk_smi_common_mt8183 = {
>   	.type     = MTK_SMI_GEN2,
>   	.has_gals = true,
> @@ -609,6 +625,7 @@ static const struct of_device_id mtk_smi_common_of_ids[] = {
>   	{.compatible = "mediatek,mt2701-smi-common", .data = &mtk_smi_common_gen1},
>   	{.compatible = "mediatek,mt2712-smi-common", .data = &mtk_smi_common_gen2},
>   	{.compatible = "mediatek,mt6779-smi-common", .data = &mtk_smi_common_mt6779},
> +	{.compatible = "mediatek,mt6795-smi-common", .data = &mtk_smi_common_mt6795},
>   	{.compatible = "mediatek,mt8167-smi-common", .data = &mtk_smi_common_gen2},
>   	{.compatible = "mediatek,mt8173-smi-common", .data = &mtk_smi_common_gen2},
>   	{.compatible = "mediatek,mt8183-smi-common", .data = &mtk_smi_common_mt8183},

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

[Yong Wu]

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@collabora.com>
> ---
>  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.

>  #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?


After the two changes,

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

Thanks.

> +	.init     = mtk_smi_common_mt6795_init,
> +};
> +
>  static const struct mtk_smi_common_plat mtk_smi_common_mt8183 = {
>  	.type     = MTK_SMI_GEN2,
>  	.has_gals = true,
> @@ -609,6 +625,7 @@ static const struct of_device_id
> mtk_smi_common_of_ids[] = {
>  	{.compatible = "mediatek,mt2701-smi-common", .data =
> &mtk_smi_common_gen1},
>  	{.compatible = "mediatek,mt2712-smi-common", .data =
> &mtk_smi_common_gen2},
>  	{.compatible = "mediatek,mt6779-smi-common", .data =
> &mtk_smi_common_mt6779},
> +	{.compatible = "mediatek,mt6795-smi-common", .data =
> &mtk_smi_common_mt6795},
>  	{.compatible = "mediatek,mt8167-smi-common", .data =
> &mtk_smi_common_gen2},
>  	{.compatible = "mediatek,mt8173-smi-common", .data =
> &mtk_smi_common_gen2},
>  	{.compatible = "mediatek,mt8183-smi-common", .data =
> &mtk_smi_common_mt8183},

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

[AngeloGioacchino Del Regno]

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@collabora.com>
>> ---
>>   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?
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 indeed
be F_MMU1_LARB(x), you'll get a big(bigger) thank you from me :-)

Meanwhile...

Thanks!
Angelo

> 
> After the two changes,
> 
> Reviewed-by: Yong Wu <yong.wu@mediatek.com>
> 
> Thanks.

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

[Yong Wu]

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@collabora.com>
> > > ---
> > >   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:)

> 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.

> indeed
> be F_MMU1_LARB(x), you'll get a big(bigger) thank you from me :-)
> 
> Meanwhile...
> 
> Thanks!
> Angelo
> 
> > 
> > After the two changes,
> > 
> > Reviewed-by: Yong Wu <yong.wu@mediatek.com>
> > 
> > Thanks.

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@collabora.com>
>>>> ---
>>>>    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