In previous discussion [1] and [2], we found that it is risky to
use max_pfn or totalram_pages to tell if 4GB mode is enabled.
Check 4GB mode by reading infracfg register, remove the usage
of the un-exported symbol max_pfn.
This is a step towards building mtk_iommu as a kernel module.
Change since v1:
1. remove the phandle usage, search for infracfg instead [3]
2. use infracfg instead of infracfg_regmap
3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
4. update enable_4GB only when has_4gb_mode
[1] https://lkml.org/lkml/2020/6/3/733
[2] https://lkml.org/lkml/2020/6/4/136
[3] https://lkml.org/lkml/2020/7/15/1147
Cc: Mike Rapoport <[email protected]>
Cc: David Hildenbrand <[email protected]>
Cc: Yong Wu <[email protected]>
Cc: Yingjoe Chen <[email protected]>
Cc: Christoph Hellwig <[email protected]>
Cc: Yong Wu <[email protected]>
Cc: Chao Hao <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Matthias Brugger <[email protected]>
Signed-off-by: Miles Chen <[email protected]>
---
drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
include/linux/soc/mediatek/infracfg.h | 3 +++
2 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 2be96f1cdbd2..16765f532853 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -3,7 +3,6 @@
* Copyright (c) 2015-2016 MediaTek Inc.
* Author: Yong Wu <[email protected]>
*/
-#include <linux/memblock.h>
#include <linux/bug.h>
#include <linux/clk.h>
#include <linux/component.h>
@@ -15,13 +14,16 @@
#include <linux/iommu.h>
#include <linux/iopoll.h>
#include <linux/list.h>
+#include <linux/mfd/syscon.h>
#include <linux/of_address.h>
#include <linux/of_iommu.h>
#include <linux/of_irq.h>
#include <linux/of_platform.h>
#include <linux/platform_device.h>
+#include <linux/regmap.h>
#include <linux/slab.h>
#include <linux/spinlock.h>
+#include <linux/soc/mediatek/infracfg.h>
#include <asm/barrier.h>
#include <soc/mediatek/smi.h>
@@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
struct resource *res;
resource_size_t ioaddr;
struct component_match *match = NULL;
+ struct regmap *infracfg;
void *protect;
int i, larb_nr, ret;
+ u32 val;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
return -ENOMEM;
data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
- /* Whether the current dram is over 4GB */
- data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
- if (!data->plat_data->has_4gb_mode)
- data->enable_4GB = false;
+ data->enable_4GB = false;
+ if (data->plat_data->has_4gb_mode) {
+ infracfg = syscon_regmap_lookup_by_compatible(
+ "mediatek,mt8173-infracfg");
+ if (IS_ERR(infracfg)) {
+ infracfg = syscon_regmap_lookup_by_compatible(
+ "mediatek,mt2712-infracfg");
+ if (IS_ERR(infracfg))
+ return PTR_ERR(infracfg);
+
+ }
+ ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
+ if (ret)
+ return ret;
+ data->enable_4GB = !!(val & F_DDR_4GB_SUPPORT_EN);
+ }
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
data->base = devm_ioremap_resource(dev, res);
diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
index fd25f0148566..233463d789c6 100644
--- a/include/linux/soc/mediatek/infracfg.h
+++ b/include/linux/soc/mediatek/infracfg.h
@@ -32,6 +32,9 @@
#define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
BIT(7) | BIT(8))
+#define REG_INFRA_MISC 0xf00
+#define F_DDR_4GB_SUPPORT_EN BIT(13)
+
int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
bool reg_update);
int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
--
2.18.0
On 21.07.20 04:16, Miles Chen wrote:
> In previous discussion [1] and [2], we found that it is risky to
> use max_pfn or totalram_pages to tell if 4GB mode is enabled.
>
> Check 4GB mode by reading infracfg register, remove the usage
> of the un-exported symbol max_pfn.
>
> This is a step towards building mtk_iommu as a kernel module.
>
> Change since v1:
> 1. remove the phandle usage, search for infracfg instead [3]
> 2. use infracfg instead of infracfg_regmap
> 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> 4. update enable_4GB only when has_4gb_mode
Nit: We tend to place such information below the "---" (adding a second
one) such that this information is discarded when the patch is picked up.
>
> [1] https://lkml.org/lkml/2020/6/3/733
> [2] https://lkml.org/lkml/2020/6/4/136
> [3] https://lkml.org/lkml/2020/7/15/1147
>
> Cc: Mike Rapoport <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Yong Wu <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Yong Wu <[email protected]>
> Cc: Chao Hao <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Signed-off-by: Miles Chen <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> include/linux/soc/mediatek/infracfg.h | 3 +++
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 2be96f1cdbd2..16765f532853 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -3,7 +3,6 @@
> * Copyright (c) 2015-2016 MediaTek Inc.
> * Author: Yong Wu <[email protected]>
> */
> -#include <linux/memblock.h>
> #include <linux/bug.h>
> #include <linux/clk.h>
> #include <linux/component.h>
> @@ -15,13 +14,16 @@
> #include <linux/iommu.h>
> #include <linux/iopoll.h>
> #include <linux/list.h>
> +#include <linux/mfd/syscon.h>
> #include <linux/of_address.h>
> #include <linux/of_iommu.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +#include <linux/soc/mediatek/infracfg.h>
> #include <asm/barrier.h>
> #include <soc/mediatek/smi.h>
>
> @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> struct resource *res;
> resource_size_t ioaddr;
> struct component_match *match = NULL;
> + struct regmap *infracfg;
> void *protect;
> int i, larb_nr, ret;
> + u32 val;
>
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> return -ENOMEM;
> data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
>
> - /* Whether the current dram is over 4GB */
> - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> - if (!data->plat_data->has_4gb_mode)
> - data->enable_4GB = false;
> + data->enable_4GB = false;
> + if (data->plat_data->has_4gb_mode) {
> + infracfg = syscon_regmap_lookup_by_compatible(
> + "mediatek,mt8173-infracfg");
> + if (IS_ERR(infracfg)) {
> + infracfg = syscon_regmap_lookup_by_compatible(
> + "mediatek,mt2712-infracfg");
> + if (IS_ERR(infracfg))
> + return PTR_ERR(infracfg);
> +
> + }
> + ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
> + if (ret)
> + return ret;
> + data->enable_4GB = !!(val & F_DDR_4GB_SUPPORT_EN);
(I am absolutely not familiar with syscon_regmap_lookup_by ..., I am
missing some context, so sorry for the stupid questions)
Who sets the regmap value and based on what? Who decides that 4GB mode
is supported or not? And who decides if 4GB mode is *required* or not?
--
Thanks,
David / dhildenb
On 21/07/2020 04:16, Miles Chen wrote:
> In previous discussion [1] and [2], we found that it is risky to
> use max_pfn or totalram_pages to tell if 4GB mode is enabled.
>
> Check 4GB mode by reading infracfg register, remove the usage
> of the un-exported symbol max_pfn.
>
> This is a step towards building mtk_iommu as a kernel module.
>
> Change since v1:
> 1. remove the phandle usage, search for infracfg instead [3]
> 2. use infracfg instead of infracfg_regmap
> 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> 4. update enable_4GB only when has_4gb_mode
>
> [1] https://lkml.org/lkml/2020/6/3/733
> [2] https://lkml.org/lkml/2020/6/4/136
> [3] https://lkml.org/lkml/2020/7/15/1147
>
> Cc: Mike Rapoport <[email protected]>
> Cc: David Hildenbrand <[email protected]>
> Cc: Yong Wu <[email protected]>
> Cc: Yingjoe Chen <[email protected]>
> Cc: Christoph Hellwig <[email protected]>
> Cc: Yong Wu <[email protected]>
> Cc: Chao Hao <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Matthias Brugger <[email protected]>
> Signed-off-by: Miles Chen <[email protected]>
> ---
> drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> include/linux/soc/mediatek/infracfg.h | 3 +++
> 2 files changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 2be96f1cdbd2..16765f532853 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -3,7 +3,6 @@
> * Copyright (c) 2015-2016 MediaTek Inc.
> * Author: Yong Wu <[email protected]>
> */
> -#include <linux/memblock.h>
> #include <linux/bug.h>
> #include <linux/clk.h>
> #include <linux/component.h>
> @@ -15,13 +14,16 @@
> #include <linux/iommu.h>
> #include <linux/iopoll.h>
> #include <linux/list.h>
> +#include <linux/mfd/syscon.h>
> #include <linux/of_address.h>
> #include <linux/of_iommu.h>
> #include <linux/of_irq.h>
> #include <linux/of_platform.h>
> #include <linux/platform_device.h>
> +#include <linux/regmap.h>
> #include <linux/slab.h>
> #include <linux/spinlock.h>
> +#include <linux/soc/mediatek/infracfg.h>
> #include <asm/barrier.h>
> #include <soc/mediatek/smi.h>
>
> @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> struct resource *res;
> resource_size_t ioaddr;
> struct component_match *match = NULL;
> + struct regmap *infracfg;
> void *protect;
> int i, larb_nr, ret;
> + u32 val;
>
> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> if (!data)
> @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> return -ENOMEM;
> data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
>
> - /* Whether the current dram is over 4GB */
> - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> - if (!data->plat_data->has_4gb_mode)
> - data->enable_4GB = false;
> + data->enable_4GB = false;
> + if (data->plat_data->has_4gb_mode) {
> + infracfg = syscon_regmap_lookup_by_compatible(
> + "mediatek,mt8173-infracfg");
> + if (IS_ERR(infracfg)) {
> + infracfg = syscon_regmap_lookup_by_compatible(
> + "mediatek,mt2712-infracfg");
> + if (IS_ERR(infracfg))
> + return PTR_ERR(infracfg);
I think we should check m4u_plat instead to decide which compatible we have to
look for.
Another option would be to add a general compatible something like
"mtk-infracfg" and search for that. That would need an update of all DTS having
a infracfg compatible right now. After thinking twice, this would break newer
kernel with older device tree, so maybe it's better to go with m4u_plat switch
statement.
Regards,
Matthias
> +
> + }
> + ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
> + if (ret)
> + return ret;
> + data->enable_4GB = !!(val & F_DDR_4GB_SUPPORT_EN);
> + }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> data->base = devm_ioremap_resource(dev, res);
> diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
> index fd25f0148566..233463d789c6 100644
> --- a/include/linux/soc/mediatek/infracfg.h
> +++ b/include/linux/soc/mediatek/infracfg.h
> @@ -32,6 +32,9 @@
> #define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> BIT(7) | BIT(8))
>
> +#define REG_INFRA_MISC 0xf00
> +#define F_DDR_4GB_SUPPORT_EN BIT(13)
> +
> int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> bool reg_update);
> int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
>
On Tue, 2020-07-21 at 11:40 +0200, Matthias Brugger wrote:
>
> On 21/07/2020 04:16, Miles Chen wrote:
> > In previous discussion [1] and [2], we found that it is risky to
> > use max_pfn or totalram_pages to tell if 4GB mode is enabled.
> >
> > Check 4GB mode by reading infracfg register, remove the usage
> > of the un-exported symbol max_pfn.
> >
> > This is a step towards building mtk_iommu as a kernel module.
> >
> > Change since v1:
> > 1. remove the phandle usage, search for infracfg instead [3]
> > 2. use infracfg instead of infracfg_regmap
> > 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> > 4. update enable_4GB only when has_4gb_mode
> >
> > [1] https://lkml.org/lkml/2020/6/3/733
> > [2] https://lkml.org/lkml/2020/6/4/136
> > [3] https://lkml.org/lkml/2020/7/15/1147
> >
> > Cc: Mike Rapoport <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Yong Wu <[email protected]>
> > Cc: Yingjoe Chen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Yong Wu <[email protected]>
> > Cc: Chao Hao <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Matthias Brugger <[email protected]>
> > Signed-off-by: Miles Chen <[email protected]>
> > ---
> > drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> > include/linux/soc/mediatek/infracfg.h | 3 +++
> > 2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 2be96f1cdbd2..16765f532853 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -3,7 +3,6 @@
> > * Copyright (c) 2015-2016 MediaTek Inc.
> > * Author: Yong Wu <[email protected]>
> > */
> > -#include <linux/memblock.h>
> > #include <linux/bug.h>
> > #include <linux/clk.h>
> > #include <linux/component.h>
> > @@ -15,13 +14,16 @@
> > #include <linux/iommu.h>
> > #include <linux/iopoll.h>
> > #include <linux/list.h>
> > +#include <linux/mfd/syscon.h>
> > #include <linux/of_address.h>
> > #include <linux/of_iommu.h>
> > #include <linux/of_irq.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > +#include <linux/soc/mediatek/infracfg.h>
> > #include <asm/barrier.h>
> > #include <soc/mediatek/smi.h>
> >
> > @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > struct resource *res;
> > resource_size_t ioaddr;
> > struct component_match *match = NULL;
> > + struct regmap *infracfg;
> > void *protect;
> > int i, larb_nr, ret;
> > + u32 val;
> >
> > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > return -ENOMEM;
> > data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
> >
> > - /* Whether the current dram is over 4GB */
> > - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> > - if (!data->plat_data->has_4gb_mode)
> > - data->enable_4GB = false;
> > + data->enable_4GB = false;
> > + if (data->plat_data->has_4gb_mode) {
> > + infracfg = syscon_regmap_lookup_by_compatible(
> > + "mediatek,mt8173-infracfg");
> > + if (IS_ERR(infracfg)) {
> > + infracfg = syscon_regmap_lookup_by_compatible(
> > + "mediatek,mt2712-infracfg");
> > + if (IS_ERR(infracfg))
> > + return PTR_ERR(infracfg);
>
> I think we should check m4u_plat instead to decide which compatible we have to
> look for.
> Another option would be to add a general compatible something like
> "mtk-infracfg" and search for that. That would need an update of all DTS having
> a infracfg compatible right now. After thinking twice, this would break newer
> kernel with older device tree, so maybe it's better to go with m4u_plat switch
> statement.
Add a "char *infracfg" in the plat_data, Use the mt2712, mt8173
corresponding string in it. If it is NULL, It means the "enable_4GB"
always is false. Then we also can remove the flag "has_4gb_mode".
is this OK?
>
> Regards,
> Matthias
>
> > +
> > + }
> > + ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
> > + if (ret)
> > + return ret;
> > + data->enable_4GB = !!(val & F_DDR_4GB_SUPPORT_EN);
> > + }
> >
> > res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > data->base = devm_ioremap_resource(dev, res);
> > diff --git a/include/linux/soc/mediatek/infracfg.h b/include/linux/soc/mediatek/infracfg.h
> > index fd25f0148566..233463d789c6 100644
> > --- a/include/linux/soc/mediatek/infracfg.h
> > +++ b/include/linux/soc/mediatek/infracfg.h
> > @@ -32,6 +32,9 @@
> > #define MT7622_TOP_AXI_PROT_EN_WB (BIT(2) | BIT(6) | \
> > BIT(7) | BIT(8))
> >
> > +#define REG_INFRA_MISC 0xf00
> > +#define F_DDR_4GB_SUPPORT_EN BIT(13)
> > +
> > int mtk_infracfg_set_bus_protection(struct regmap *infracfg, u32 mask,
> > bool reg_update);
> > int mtk_infracfg_clear_bus_protection(struct regmap *infracfg, u32 mask,
> >
On 21/07/2020 13:24, Yong Wu wrote:
> On Tue, 2020-07-21 at 11:40 +0200, Matthias Brugger wrote:
>>
>> On 21/07/2020 04:16, Miles Chen wrote:
>>> In previous discussion [1] and [2], we found that it is risky to
>>> use max_pfn or totalram_pages to tell if 4GB mode is enabled.
>>>
>>> Check 4GB mode by reading infracfg register, remove the usage
>>> of the un-exported symbol max_pfn.
>>>
>>> This is a step towards building mtk_iommu as a kernel module.
>>>
>>> Change since v1:
>>> 1. remove the phandle usage, search for infracfg instead [3]
>>> 2. use infracfg instead of infracfg_regmap
>>> 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
>>> 4. update enable_4GB only when has_4gb_mode
>>>
>>> [1] https://lkml.org/lkml/2020/6/3/733
>>> [2] https://lkml.org/lkml/2020/6/4/136
>>> [3] https://lkml.org/lkml/2020/7/15/1147
>>>
>>> Cc: Mike Rapoport <[email protected]>
>>> Cc: David Hildenbrand <[email protected]>
>>> Cc: Yong Wu <[email protected]>
>>> Cc: Yingjoe Chen <[email protected]>
>>> Cc: Christoph Hellwig <[email protected]>
>>> Cc: Yong Wu <[email protected]>
>>> Cc: Chao Hao <[email protected]>
>>> Cc: Rob Herring <[email protected]>
>>> Cc: Matthias Brugger <[email protected]>
>>> Signed-off-by: Miles Chen <[email protected]>
>>> ---
>>> drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
>>> include/linux/soc/mediatek/infracfg.h | 3 +++
>>> 2 files changed, 24 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 2be96f1cdbd2..16765f532853 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -3,7 +3,6 @@
>>> * Copyright (c) 2015-2016 MediaTek Inc.
>>> * Author: Yong Wu <[email protected]>
>>> */
>>> -#include <linux/memblock.h>
>>> #include <linux/bug.h>
>>> #include <linux/clk.h>
>>> #include <linux/component.h>
>>> @@ -15,13 +14,16 @@
>>> #include <linux/iommu.h>
>>> #include <linux/iopoll.h>
>>> #include <linux/list.h>
>>> +#include <linux/mfd/syscon.h>
>>> #include <linux/of_address.h>
>>> #include <linux/of_iommu.h>
>>> #include <linux/of_irq.h>
>>> #include <linux/of_platform.h>
>>> #include <linux/platform_device.h>
>>> +#include <linux/regmap.h>
>>> #include <linux/slab.h>
>>> #include <linux/spinlock.h>
>>> +#include <linux/soc/mediatek/infracfg.h>
>>> #include <asm/barrier.h>
>>> #include <soc/mediatek/smi.h>
>>>
>>> @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>>> struct resource *res;
>>> resource_size_t ioaddr;
>>> struct component_match *match = NULL;
>>> + struct regmap *infracfg;
>>> void *protect;
>>> int i, larb_nr, ret;
>>> + u32 val;
>>>
>>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>>> if (!data)
>>> @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>>> return -ENOMEM;
>>> data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
>>>
>>> - /* Whether the current dram is over 4GB */
>>> - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
>>> - if (!data->plat_data->has_4gb_mode)
>>> - data->enable_4GB = false;
>>> + data->enable_4GB = false;
>>> + if (data->plat_data->has_4gb_mode) {
>>> + infracfg = syscon_regmap_lookup_by_compatible(
>>> + "mediatek,mt8173-infracfg");
>>> + if (IS_ERR(infracfg)) {
>>> + infracfg = syscon_regmap_lookup_by_compatible(
>>> + "mediatek,mt2712-infracfg");
>>> + if (IS_ERR(infracfg))
>>> + return PTR_ERR(infracfg);
>>
>> I think we should check m4u_plat instead to decide which compatible we have to
>> look for.
>> Another option would be to add a general compatible something like
>> "mtk-infracfg" and search for that. That would need an update of all DTS having
>> a infracfg compatible right now. After thinking twice, this would break newer
>> kernel with older device tree, so maybe it's better to go with m4u_plat switch
>> statement.
>
> Add a "char *infracfg" in the plat_data, Use the mt2712, mt8173
> corresponding string in it. If it is NULL, It means the "enable_4GB"
> always is false. Then we also can remove the flag "has_4gb_mode".
>
> is this OK?
>
It's an option, but I personally find that a bit hacky.
Regards,
Matthias
On Tue, 2020-07-21 at 23:19 +0200, Matthias Brugger wrote:
>
> On 21/07/2020 13:24, Yong Wu wrote:
> > On Tue, 2020-07-21 at 11:40 +0200, Matthias Brugger wrote:
> >>
> >> On 21/07/2020 04:16, Miles Chen wrote:
> >>> In previous discussion [1] and [2], we found that it is risky to
> >>> use max_pfn or totalram_pages to tell if 4GB mode is enabled.
> >>>
> >>> Check 4GB mode by reading infracfg register, remove the usage
> >>> of the un-exported symbol max_pfn.
> >>>
> >>> This is a step towards building mtk_iommu as a kernel module.
> >>>
> >>> Change since v1:
> >>> 1. remove the phandle usage, search for infracfg instead [3]
> >>> 2. use infracfg instead of infracfg_regmap
> >>> 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> >>> 4. update enable_4GB only when has_4gb_mode
> >>>
> >>> [1] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/3/733__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2AaiFZejiQ$
> >>> [2] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/4/136__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2Aa9U2yQyg$
> >>> [3] https://urldefense.com/v3/__https://lkml.org/lkml/2020/7/15/1147__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2Aaxpk_Wjw$
> >>>
> >>> Cc: Mike Rapoport <[email protected]>
> >>> Cc: David Hildenbrand <[email protected]>
> >>> Cc: Yong Wu <[email protected]>
> >>> Cc: Yingjoe Chen <[email protected]>
> >>> Cc: Christoph Hellwig <[email protected]>
> >>> Cc: Yong Wu <[email protected]>
> >>> Cc: Chao Hao <[email protected]>
> >>> Cc: Rob Herring <[email protected]>
> >>> Cc: Matthias Brugger <[email protected]>
> >>> Signed-off-by: Miles Chen <[email protected]>
> >>> ---
> >>> drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> >>> include/linux/soc/mediatek/infracfg.h | 3 +++
> >>> 2 files changed, 24 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> >>> index 2be96f1cdbd2..16765f532853 100644
> >>> --- a/drivers/iommu/mtk_iommu.c
> >>> +++ b/drivers/iommu/mtk_iommu.c
> >>> @@ -3,7 +3,6 @@
> >>> * Copyright (c) 2015-2016 MediaTek Inc.
> >>> * Author: Yong Wu <[email protected]>
> >>> */
> >>> -#include <linux/memblock.h>
> >>> #include <linux/bug.h>
> >>> #include <linux/clk.h>
> >>> #include <linux/component.h>
> >>> @@ -15,13 +14,16 @@
> >>> #include <linux/iommu.h>
> >>> #include <linux/iopoll.h>
> >>> #include <linux/list.h>
> >>> +#include <linux/mfd/syscon.h>
> >>> #include <linux/of_address.h>
> >>> #include <linux/of_iommu.h>
> >>> #include <linux/of_irq.h>
> >>> #include <linux/of_platform.h>
> >>> #include <linux/platform_device.h>
> >>> +#include <linux/regmap.h>
> >>> #include <linux/slab.h>
> >>> #include <linux/spinlock.h>
> >>> +#include <linux/soc/mediatek/infracfg.h>
> >>> #include <asm/barrier.h>
> >>> #include <soc/mediatek/smi.h>
> >>>
> >>> @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >>> struct resource *res;
> >>> resource_size_t ioaddr;
> >>> struct component_match *match = NULL;
> >>> + struct regmap *infracfg;
> >>> void *protect;
> >>> int i, larb_nr, ret;
> >>> + u32 val;
> >>>
> >>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >>> if (!data)
> >>> @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >>> return -ENOMEM;
> >>> data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
> >>>
> >>> - /* Whether the current dram is over 4GB */
> >>> - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> >>> - if (!data->plat_data->has_4gb_mode)
> >>> - data->enable_4GB = false;
> >>> + data->enable_4GB = false;
> >>> + if (data->plat_data->has_4gb_mode) {
> >>> + infracfg = syscon_regmap_lookup_by_compatible(
> >>> + "mediatek,mt8173-infracfg");
> >>> + if (IS_ERR(infracfg)) {
> >>> + infracfg = syscon_regmap_lookup_by_compatible(
> >>> + "mediatek,mt2712-infracfg");
> >>> + if (IS_ERR(infracfg))
> >>> + return PTR_ERR(infracfg);
> >>
> >> I think we should check m4u_plat instead to decide which compatible we have to
> >> look for.
> >> Another option would be to add a general compatible something like
> >> "mtk-infracfg" and search for that. That would need an update of all DTS having
> >> a infracfg compatible right now. After thinking twice, this would break newer
> >> kernel with older device tree, so maybe it's better to go with m4u_plat switch
> >> statement.
> >
> > Add a "char *infracfg" in the plat_data, Use the mt2712, mt8173
> > corresponding string in it. If it is NULL, It means the "enable_4GB"
> > always is false. Then we also can remove the flag "has_4gb_mode".
> >
> > is this OK?
> >
>
> It's an option, but I personally find that a bit hacky.
Thanks Yong and Matthias for your comment.
I will try adding a char *infracfg in patch v3.
>
> Regards,
> Matthias
On Tue, 2020-07-21 at 11:10 +0200, David Hildenbrand wrote:
> On 21.07.20 04:16, Miles Chen wrote:
> > In previous discussion [1] and [2], we found that it is risky to
> > use max_pfn or totalram_pages to tell if 4GB mode is enabled.
> >
> > Check 4GB mode by reading infracfg register, remove the usage
> > of the un-exported symbol max_pfn.
> >
> > This is a step towards building mtk_iommu as a kernel module.
> >
> > Change since v1:
> > 1. remove the phandle usage, search for infracfg instead [3]
> > 2. use infracfg instead of infracfg_regmap
> > 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> > 4. update enable_4GB only when has_4gb_mode
>
> Nit: We tend to place such information below the "---" (adding a second
> one) such that this information is discarded when the patch is picked up.
Thanks, I'll add a "---" before the "Change since..." in patch v3.
>
> >
> > [1] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/3/733__;!!CTRNKA9wMg0ARbw!wrP2H0azuZkTHSnM_ETki4kT_FMF0Cl7abQX8tIHX0iTr65JkvMCe4jDt84d_YhBrA$
> > [2] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/4/136__;!!CTRNKA9wMg0ARbw!wrP2H0azuZkTHSnM_ETki4kT_FMF0Cl7abQX8tIHX0iTr65JkvMCe4jDt86IGhmouQ$
> > [3] https://urldefense.com/v3/__https://lkml.org/lkml/2020/7/15/1147__;!!CTRNKA9wMg0ARbw!wrP2H0azuZkTHSnM_ETki4kT_FMF0Cl7abQX8tIHX0iTr65JkvMCe4jDt855z4xdqw$
> >
> > Cc: Mike Rapoport <[email protected]>
> > Cc: David Hildenbrand <[email protected]>
> > Cc: Yong Wu <[email protected]>
> > Cc: Yingjoe Chen <[email protected]>
> > Cc: Christoph Hellwig <[email protected]>
> > Cc: Yong Wu <[email protected]>
> > Cc: Chao Hao <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Matthias Brugger <[email protected]>
> > Signed-off-by: Miles Chen <[email protected]>
> > ---
> > drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> > include/linux/soc/mediatek/infracfg.h | 3 +++
> > 2 files changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 2be96f1cdbd2..16765f532853 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -3,7 +3,6 @@
> > * Copyright (c) 2015-2016 MediaTek Inc.
> > * Author: Yong Wu <[email protected]>
> > */
> > -#include <linux/memblock.h>
> > #include <linux/bug.h>
> > #include <linux/clk.h>
> > #include <linux/component.h>
> > @@ -15,13 +14,16 @@
> > #include <linux/iommu.h>
> > #include <linux/iopoll.h>
> > #include <linux/list.h>
> > +#include <linux/mfd/syscon.h>
> > #include <linux/of_address.h>
> > #include <linux/of_iommu.h>
> > #include <linux/of_irq.h>
> > #include <linux/of_platform.h>
> > #include <linux/platform_device.h>
> > +#include <linux/regmap.h>
> > #include <linux/slab.h>
> > #include <linux/spinlock.h>
> > +#include <linux/soc/mediatek/infracfg.h>
> > #include <asm/barrier.h>
> > #include <soc/mediatek/smi.h>
> >
> > @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > struct resource *res;
> > resource_size_t ioaddr;
> > struct component_match *match = NULL;
> > + struct regmap *infracfg;
> > void *protect;
> > int i, larb_nr, ret;
> > + u32 val;
> >
> > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > if (!data)
> > @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > return -ENOMEM;
> > data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
> >
> > - /* Whether the current dram is over 4GB */
> > - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> > - if (!data->plat_data->has_4gb_mode)
> > - data->enable_4GB = false;
> > + data->enable_4GB = false;
> > + if (data->plat_data->has_4gb_mode) {
> > + infracfg = syscon_regmap_lookup_by_compatible(
> > + "mediatek,mt8173-infracfg");
> > + if (IS_ERR(infracfg)) {
> > + infracfg = syscon_regmap_lookup_by_compatible(
> > + "mediatek,mt2712-infracfg");
> > + if (IS_ERR(infracfg))
> > + return PTR_ERR(infracfg);
> > +
> > + }
> > + ret = regmap_read(infracfg, REG_INFRA_MISC, &val);
> > + if (ret)
> > + return ret;
> > + data->enable_4GB = !!(val & F_DDR_4GB_SUPPORT_EN);
>
> (I am absolutely not familiar with syscon_regmap_lookup_by ..., I am
> missing some context, so sorry for the stupid questions)
>
> Who sets the regmap value and based on what? Who decides that 4GB mode
> is supported or not? And who decides if 4GB mode is *required* or not?
>
On Wed, 2020-07-22 at 15:17 +0800, Miles Chen wrote:
> On Tue, 2020-07-21 at 23:19 +0200, Matthias Brugger wrote:
> >
> > On 21/07/2020 13:24, Yong Wu wrote:
> > > On Tue, 2020-07-21 at 11:40 +0200, Matthias Brugger wrote:
> > >>
> > >> On 21/07/2020 04:16, Miles Chen wrote:
> > >>> In previous discussion [1] and [2], we found that it is risky to
> > >>> use max_pfn or totalram_pages to tell if 4GB mode is enabled.
> > >>>
> > >>> Check 4GB mode by reading infracfg register, remove the usage
> > >>> of the un-exported symbol max_pfn.
> > >>>
> > >>> This is a step towards building mtk_iommu as a kernel module.
> > >>>
> > >>> Change since v1:
> > >>> 1. remove the phandle usage, search for infracfg instead [3]
> > >>> 2. use infracfg instead of infracfg_regmap
> > >>> 3. move infracfg definitaions to linux/soc/mediatek/infracfg.h
> > >>> 4. update enable_4GB only when has_4gb_mode
> > >>>
> > >>> [1] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/3/733__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2AaiFZejiQ$
> > >>> [2] https://urldefense.com/v3/__https://lkml.org/lkml/2020/6/4/136__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2Aa9U2yQyg$
> > >>> [3] https://urldefense.com/v3/__https://lkml.org/lkml/2020/7/15/1147__;!!CTRNKA9wMg0ARbw!w5YjY83YRL9_ijgXHwB1x2Dnb5BqiFUI8H5IAyAWWFMvUJKI9Qbj_zta2Aaxpk_Wjw$
> > >>>
> > >>> Cc: Mike Rapoport <[email protected]>
> > >>> Cc: David Hildenbrand <[email protected]>
> > >>> Cc: Yong Wu <[email protected]>
> > >>> Cc: Yingjoe Chen <[email protected]>
> > >>> Cc: Christoph Hellwig <[email protected]>
> > >>> Cc: Yong Wu <[email protected]>
> > >>> Cc: Chao Hao <[email protected]>
> > >>> Cc: Rob Herring <[email protected]>
> > >>> Cc: Matthias Brugger <[email protected]>
> > >>> Signed-off-by: Miles Chen <[email protected]>
> > >>> ---
> > >>> drivers/iommu/mtk_iommu.c | 26 +++++++++++++++++++++-----
> > >>> include/linux/soc/mediatek/infracfg.h | 3 +++
> > >>> 2 files changed, 24 insertions(+), 5 deletions(-)
> > >>>
> > >>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > >>> index 2be96f1cdbd2..16765f532853 100644
> > >>> --- a/drivers/iommu/mtk_iommu.c
> > >>> +++ b/drivers/iommu/mtk_iommu.c
> > >>> @@ -3,7 +3,6 @@
> > >>> * Copyright (c) 2015-2016 MediaTek Inc.
> > >>> * Author: Yong Wu <[email protected]>
> > >>> */
> > >>> -#include <linux/memblock.h>
> > >>> #include <linux/bug.h>
> > >>> #include <linux/clk.h>
> > >>> #include <linux/component.h>
> > >>> @@ -15,13 +14,16 @@
> > >>> #include <linux/iommu.h>
> > >>> #include <linux/iopoll.h>
> > >>> #include <linux/list.h>
> > >>> +#include <linux/mfd/syscon.h>
> > >>> #include <linux/of_address.h>
> > >>> #include <linux/of_iommu.h>
> > >>> #include <linux/of_irq.h>
> > >>> #include <linux/of_platform.h>
> > >>> #include <linux/platform_device.h>
> > >>> +#include <linux/regmap.h>
> > >>> #include <linux/slab.h>
> > >>> #include <linux/spinlock.h>
> > >>> +#include <linux/soc/mediatek/infracfg.h>
> > >>> #include <asm/barrier.h>
> > >>> #include <soc/mediatek/smi.h>
> > >>>
> > >>> @@ -599,8 +601,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > >>> struct resource *res;
> > >>> resource_size_t ioaddr;
> > >>> struct component_match *match = NULL;
> > >>> + struct regmap *infracfg;
> > >>> void *protect;
> > >>> int i, larb_nr, ret;
> > >>> + u32 val;
> > >>>
> > >>> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> > >>> if (!data)
> > >>> @@ -614,10 +618,22 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > >>> return -ENOMEM;
> > >>> data->protect_base = ALIGN(virt_to_phys(protect), MTK_PROTECT_PA_ALIGN);
> > >>>
> > >>> - /* Whether the current dram is over 4GB */
> > >>> - data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> > >>> - if (!data->plat_data->has_4gb_mode)
> > >>> - data->enable_4GB = false;
> > >>> + data->enable_4GB = false;
> > >>> + if (data->plat_data->has_4gb_mode) {
> > >>> + infracfg = syscon_regmap_lookup_by_compatible(
> > >>> + "mediatek,mt8173-infracfg");
> > >>> + if (IS_ERR(infracfg)) {
> > >>> + infracfg = syscon_regmap_lookup_by_compatible(
> > >>> + "mediatek,mt2712-infracfg");
> > >>> + if (IS_ERR(infracfg))
> > >>> + return PTR_ERR(infracfg);
> > >>
> > >> I think we should check m4u_plat instead to decide which compatible we have to
> > >> look for.
> > >> Another option would be to add a general compatible something like
> > >> "mtk-infracfg" and search for that. That would need an update of all DTS having
> > >> a infracfg compatible right now. After thinking twice, this would break newer
> > >> kernel with older device tree, so maybe it's better to go with m4u_plat switch
> > >> statement.
> > >
> > > Add a "char *infracfg" in the plat_data, Use the mt2712, mt8173
> > > corresponding string in it. If it is NULL, It means the "enable_4GB"
> > > always is false. Then we also can remove the flag "has_4gb_mode".
> > >
> > > is this OK?
> > >
> >
> > It's an option, but I personally find that a bit hacky.
>
> Thanks Yong and Matthias for your comment.
> I will try adding a char *infracfg in patch v3.
>
I misunderstood the comment.
I should check m4u_plat to decide which string to look for, not adding a
char *infracfg in patch v3.
Miles
>
> >
> > Regards,
> > Matthias
>