Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode

[Yong Wu]

On Thu, 2019-07-11 at 13:31 +0100, Will Deacon wrote:
> On Thu, Jul 11, 2019 at 07:53:56PM +0800, Yong Wu wrote:
> > On Wed, 2019-07-10 at 15:36 +0100, Will Deacon wrote:
> > > On Sat, Jun 29, 2019 at 10:09:13AM +0800, Yong Wu wrote:
> > > > MediaTek extend the arm v7s descriptor to support the dram over 4GB.
> > > > 
> > > > In the mt2712 and mt8173, it's called "4GB mode", the physical address
> > > > is from 0x4000_0000 to 0x1_3fff_ffff, but from EMI point of view, it
> > > > is remapped to high address from 0x1_0000_0000 to 0x1_ffff_ffff, the
> > > > bit32 is always enabled. thus, in the M4U, we always enable the bit9
> > > > for all PTEs which means to enable bit32 of physical address.
> > > > 
> > > > but in mt8183, M4U support the dram from 0x4000_0000 to 0x3_ffff_ffff
> > > > which isn't remaped. We extend the PTEs: the bit9 represent bit32 of
> > > > PA and the bit4 represent bit33 of PA. Meanwhile the iova still is
> > > > 32bits.
> > > 
> > > What happens if bit4 is set in the pte for mt2712 or mt8173? Perhaps the
> > 
> > bit4 is ignored in mt2712 and mt8173(No effect).
> > 
> > > io-pgtable backend should be allowing oas > 32 when
> > > IO_PGTABLE_QUIRK_ARM_MTK_4GB is set, and then enforcing that itself.
> > 
> > About oas, It looks the oas doesn't work in current the v7s. 
> > 
> > How about I add a new simple preparing patch like this(copy from
> > io-pgtable-arm.c)?
> 
> This looks like the right sort of idea. Basically, I was thinking that you
> can use the oas in conjunction with the quirk to specify whether or not
> your two magic bits should be set. You could also then cap the oas using
> the size of phys_addr_t to deal with my other comment.
> 
> Finally, I was hoping you could drop the |= BIT_ULL(32) and the &=
> ~BIT_ULL(32) bits of the mtk driver if the pgtable code now accepts higher
> addresses. Did that not work out?


After the current patch, the pgtable has accepted the higher address.
the " |= BIT_ULL(32)" and "& = ~ BIT_ULL(32)" is for a special case(we
call it 4GB mode).

Now MediaTek IOMMU support 2 kind memory:
1) normal case: PA is 0x4000_0000 - 0x3_ffff_ffff. the PA won't be
remapped. mt8183 and the non-4GB mode of mt8173/mt2712 use this mode.

2) 4GB Mode: PA is 0x4000_0000 - 0x1_3fff_ffff. But the PA will remapped
to 0x1_0000_0000 to 0x1_ffff_ffff. This is for the 4GB mode of
mt8173/mt2712. This case is so special that we should change the PA
manually(add bit32).
(mt2712 and mt8173 have both mode: 4GB and non-4GB.)

If we try to use oas and our quirk to cover this two case. Then I can
use "oas == 33" only for this 4GB mode. and "oas == 34" for the normal
case even though the PA mayn't reach 34bit. Also I should add some
"workaround" for the 4GB mode(oas==33).

I copy the new patch in the mail below(have dropped the "|= BIT_ULL(32)"
and the "&= ~BIT_ULL(32)) in mtk iommu". please help have a look if it
is ok.
(another thing: Current the PA can support over 4GB. So the quirk name
"MTK_4GB" looks not suitable, I used a new patch rename to "MTK_EXT").

> > 
> > ==========================================
> > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > @@ -495,7 +495,8 @@ static int arm_v7s_map(struct io_pgtable_ops *ops,
> > unsigned long iova,
> >         if (!(prot & (IOMMU_READ | IOMMU_WRITE)))
> >                 return 0;
> > 
> > -       if (WARN_ON(upper_32_bits(iova) || upper_32_bits(paddr)))
> > +       if (WARN_ON(iova >= (1ULL << data->iop.cfg.ias) ||
> > +                   paddr >= (1ULL << data->iop.cfg.oas)))
> >                 return -ERANGE;
> > 
> > ===============================================
> > 
> > Then, change the oas in MTK 4GB mode, like this:
> > 
> > ================================================
> > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > @@ -721,7 +721,9 @@ static struct io_pgtable
> > *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> >  {
> >         struct arm_v7s_io_pgtable *data;
> > 
> > -       if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas >
> > ARM_V7S_ADDR_BITS)
> > +       if (cfg->ias > ARM_V7S_ADDR_BITS ||
> > +           (cfg->oas > ARM_V7S_ADDR_BITS &&
> > +            !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_4GB)))
> 
> This should probably still be capped at 34 bits.

Don't get here. This is only a simple checking (if (oas > 32) return
NULL). I should avoid this checking for our case.

> 
> > > > +	paddr = pte & mask;
> > > > +	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_4GB) {
> > > > +		if (pte & ARM_V7S_ATTR_MTK_PA_BIT32)
> > > > +			paddr |= BIT_ULL(32);
> > > > +		if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> > > > +			paddr |= BIT_ULL(33);
> > > > +	}
> > > > +	return paddr;
> > > 
> > > I think this relies on CONFIG_PHYS_ADDR_T_64BIT, which isn't always set on
> > > 32-bit ARM.
> > 
> > This was discussed at [1]. Robin commented that this is not needed and
> > build won't complain about this.
> 
> It's not so much the build I was worried about, but more that we'd silently
> be doing the wrong thing and I think we can fix that as I mentioned above.

OK. see below.

> 
> Will
> 

===================================
-#define ARM_V7S_ATTR_MTK_4GB  BIT(9) /* MTK extend it for 4GB mode */
+/* MediaTek extend the two bits for PA 32bit/33bit */
+#define ARM_V7S_ATTR_MTK_PA_BIT32  BIT(9)
+#define ARM_V7S_ATTR_MTK_PA_BIT33  BIT(4)
 
 /* *well, except for TEX on level 2 large pages, of course :( */
 #define ARM_V7S_CONT_PAGE_TEX_SHIFT 6
@@ -190,13 +192,22 @@ static dma_addr_t __arm_v7s_dma_addr(void *pages)
 static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl,
 				    struct io_pgtable_cfg *cfg)
 {
-	return paddr & ARM_V7S_LVL_MASK(lvl);
+	arm_v7s_iopte pte = paddr & ARM_V7S_LVL_MASK(lvl);
+
+	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) {
+		if ((paddr & BIT_ULL(32)) || cfg->oas == 33 /* 4GB mode */)
+			pte |= ARM_V7S_ATTR_MTK_PA_BIT32;
+		if (paddr & BIT_ULL(33))
+			pte |= ARM_V7S_ATTR_MTK_PA_BIT33;
+	}
+	return pte;
 }
 
 static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
 				  struct io_pgtable_cfg *cfg)
 {
 	arm_v7s_iopte mask;
+	phys_addr_t paddr;
 
 	if (ARM_V7S_PTE_IS_TABLE(pte, lvl))
 		mask = ARM_V7S_TABLE_MASK;
@@ -205,7 +216,20 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte
pte, int lvl,
 	else
 		mask = ARM_V7S_LVL_MASK(lvl);
 
-	return pte & mask;
+	paddr = pte & mask;
+	if (IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT) &&
+	    (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)) {
+		/*
+		 * Workaround for MTK 4GB Mode:
+		 * Add BIT32 only when PA < 0x4000_0000.
+		 */
+		if ((cfg->oas == 33 && paddr < 0x40000000UL) ||
+		    (cfg->oas > 33 && (pte & ARM_V7S_ATTR_MTK_PA_BIT32)))
+			paddr |= BIT_ULL(32);
+		if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
+			paddr |= BIT_ULL(33);
+	}
+	return paddr;
 }
 
 static arm_v7s_iopte *iopte_deref(arm_v7s_iopte pte, int lvl,
@@ -326,9 +350,6 @@ static arm_v7s_iopte arm_v7s_prot_to_pte(int prot,
int lvl,
 	if (lvl == 1 && (cfg->quirks & IO_PGTABLE_QUIRK_ARM_NS))
 		pte |= ARM_V7S_ATTR_NS_SECTION;
 
-	if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)
-		pte |= ARM_V7S_ATTR_MTK_4GB;
-
 	return pte;
 }
 
@@ -742,7 +763,9 @@ static struct io_pgtable
*arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
 {
 	struct arm_v7s_io_pgtable *data;
 
-	if (cfg->ias > ARM_V7S_ADDR_BITS || cfg->oas > ARM_V7S_ADDR_BITS)
+	if (cfg->ias > ARM_V7S_ADDR_BITS ||
+	    (cfg->oas > ARM_V7S_ADDR_BITS &&
+	     !(cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT)))
 		return NULL;
 
 	if (cfg->quirks & ~(IO_PGTABLE_QUIRK_ARM_NS |
diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 85e71fb..1a141ea 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -271,16 +271,18 @@ static int mtk_iommu_domain_finalise(struct
mtk_iommu_domain *dom)
 	dom->cfg = (struct io_pgtable_cfg) {
 		.quirks = IO_PGTABLE_QUIRK_ARM_NS |
 			IO_PGTABLE_QUIRK_NO_PERMS |
-			IO_PGTABLE_QUIRK_TLBI_ON_MAP,
+			IO_PGTABLE_QUIRK_TLBI_ON_MAP |
+			IO_PGTABLE_QUIRK_ARM_MTK_EXT,
 		.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
 		.ias = 32,
-		.oas = 32,
 		.tlb = &mtk_iommu_gather_ops,
 		.iommu_dev = data->dev,
 	};
 
 	if (data->enable_4GB)
-		dom->cfg.quirks |= IO_PGTABLE_QUIRK_ARM_MTK_EXT;
+		dom->cfg.oas = 33; /* Only for 4GB mode */
+	else
+		dom->cfg.oas = 34;
 
 	dom->iop = alloc_io_pgtable_ops(ARM_V7S, &dom->cfg, data);
 	if (!dom->iop) {
@@ -371,8 +373,7 @@ static int mtk_iommu_map(struct iommu_domain
*domain, unsigned long iova,
 	int ret;
 
 	spin_lock_irqsave(&dom->pgtlock, flags);
-	ret = dom->iop->map(dom->iop, iova, paddr & DMA_BIT_MASK(32),
-			    size, prot);
+	ret = dom->iop->map(dom->iop, iova, paddr, size, prot);
 	spin_unlock_irqrestore(&dom->pgtlock, flags);
 
 	return ret;
@@ -401,7 +402,6 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct
iommu_domain *domain,
 					  dma_addr_t iova)
 {
 	struct mtk_iommu_domain *dom = to_mtk_domain(domain);
-	struct mtk_iommu_data *data = mtk_iommu_get_m4u_data();
 	unsigned long flags;
 	phys_addr_t pa;
 
@@ -409,9 +409,6 @@ static phys_addr_t mtk_iommu_iova_to_phys(struct
iommu_domain *domain,
 	pa = dom->iop->iova_to_phys(dom->iop, iova);
 	spin_unlock_irqrestore(&dom->pgtlock, flags);
 
-	if (data->enable_4GB)
-		pa |= BIT_ULL(32);
-
 	return pa;
 } 
===================================
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-mediatek