Re: [PATCH v5 09/27] iommu/io-pgtable-arm-v7s: Extend PA34 for MediaTek

From: Yong Wu <yong.wu@mediatek.com>
To: Tomasz Figa <tfiga@chromium.org>
Cc: youlin.pei@mediatek.com, devicetree@vger.kernel.org,
	Nicolas Boichat <drinkcat@chromium.org>,
	srv_heupstream@mediatek.com, Tomasz Figa <tfiga@google.com>,
	Will Deacon <will@kernel.org>,
	linux-kernel@vger.kernel.org, Evan Green <evgreen@chromium.org>,
	chao.hao@mediatek.com, iommu@lists.linux-foundation.org,
	Rob Herring <robh+dt@kernel.org>,
	linux-mediatek@lists.infradead.org,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	anan.sun@mediatek.com, Robin Murphy <robin.murphy@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 09/27] iommu/io-pgtable-arm-v7s: Extend PA34 for MediaTek
Date: Tue, 29 Dec 2020 19:17:37 +0800	[thread overview]
Message-ID: <1609240657.26323.298.camel@mhfsdcap03> (raw)
In-Reply-To: <X+L96uZU2aZWfjSP@chromium.org>

On Wed, 2020-12-23 at 17:20 +0900, Tomasz Figa wrote:
> On Wed, Dec 09, 2020 at 04:00:44PM +0800, Yong Wu wrote:
> > MediaTek extend the bit5 in lvl1 and lvl2 descriptor as PA34.
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > Acked-by: Will Deacon <will@kernel.org>
> > Reviewed-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> >  drivers/iommu/io-pgtable-arm-v7s.c | 9 +++++++--
> >  drivers/iommu/mtk_iommu.c          | 2 +-
> >  include/linux/io-pgtable.h         | 4 ++--
> >  3 files changed, 10 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iommu/io-pgtable-arm-v7s.c b/drivers/iommu/io-pgtable-arm-v7s.c
> > index e880745ab1e8..4d0aa079470f 100644
> > --- a/drivers/iommu/io-pgtable-arm-v7s.c
> > +++ b/drivers/iommu/io-pgtable-arm-v7s.c
> > @@ -112,9 +112,10 @@
> >  #define ARM_V7S_TEX_MASK		0x7
> >  #define ARM_V7S_ATTR_TEX(val)		(((val) & ARM_V7S_TEX_MASK) << ARM_V7S_TEX_SHIFT)
> >  
> > -/* MediaTek extend the two bits for PA 32bit/33bit */
> > +/* MediaTek extend the bits below for PA 32bit/33bit/34bit */
> >  #define ARM_V7S_ATTR_MTK_PA_BIT32	BIT(9)
> >  #define ARM_V7S_ATTR_MTK_PA_BIT33	BIT(4)
> > +#define ARM_V7S_ATTR_MTK_PA_BIT34	BIT(5)
> >  
> >  /* *well, except for TEX on level 2 large pages, of course :( */
> >  #define ARM_V7S_CONT_PAGE_TEX_SHIFT	6
> > @@ -194,6 +195,8 @@ static arm_v7s_iopte paddr_to_iopte(phys_addr_t paddr, int lvl,
> >  		pte |= ARM_V7S_ATTR_MTK_PA_BIT32;
> >  	if (paddr & BIT_ULL(33))
> >  		pte |= ARM_V7S_ATTR_MTK_PA_BIT33;
> > +	if (paddr & BIT_ULL(34))
> > +		pte |= ARM_V7S_ATTR_MTK_PA_BIT34;
> >  	return pte;
> >  }
> >  
> > @@ -218,6 +221,8 @@ static phys_addr_t iopte_to_paddr(arm_v7s_iopte pte, int lvl,
> >  		paddr |= BIT_ULL(32);
> >  	if (pte & ARM_V7S_ATTR_MTK_PA_BIT33)
> >  		paddr |= BIT_ULL(33);
> > +	if (pte & ARM_V7S_ATTR_MTK_PA_BIT34)
> > +		paddr |= BIT_ULL(34);
> >  	return paddr;
> >  }
> >  
> > @@ -754,7 +759,7 @@ static struct io_pgtable *arm_v7s_alloc_pgtable(struct io_pgtable_cfg *cfg,
> >  	if (cfg->ias > ARM_V7S_ADDR_BITS)
> >  		return NULL;
> >  
> > -	if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 34 : ARM_V7S_ADDR_BITS))
> > +	if (cfg->oas > (arm_v7s_is_mtk_enabled(cfg) ? 35 : ARM_V7S_ADDR_BITS))
> >  		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 6451d83753e1..ec3c87d4b172 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -320,7 +320,7 @@ static int mtk_iommu_domain_finalise(struct mtk_iommu_domain *dom)
> >  			IO_PGTABLE_QUIRK_ARM_MTK_EXT,
> >  		.pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap,
> >  		.ias = 32,
> > -		.oas = 34,
> > +		.oas = 35,
> 
> Shouldn't this be set according to the real hardware capabilities,
> instead of always setting it to 35?

Here only make the code clean. 35 is ok for all the SoC.
But you are right from the HW point, the logic is like this:

if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
	dom->cfg.oas = data->enable_4GB ? 33 : 32;
else
	dom->cfg.oas = 35;

I will use this in next version.

> 
> Best regards,
> Tomasz
> 
> >  		.tlb = &mtk_iommu_flush_ops,
> >  		.iommu_dev = data->dev,
> >  	};
> > diff --git a/include/linux/io-pgtable.h b/include/linux/io-pgtable.h
> > index 4cde111e425b..1ae0757f4f94 100644
> > --- a/include/linux/io-pgtable.h
> > +++ b/include/linux/io-pgtable.h
> > @@ -77,8 +77,8 @@ struct io_pgtable_cfg {
> >  	 *	TLB maintenance when mapping as well as when unmapping.
> >  	 *
> >  	 * IO_PGTABLE_QUIRK_ARM_MTK_EXT: (ARM v7s format) MediaTek IOMMUs extend
> > -	 *	to support up to 34 bits PA where the bit32 and bit33 are
> > -	 *	encoded in the bit9 and bit4 of the PTE respectively.
> > +	 *	to support up to 35 bits PA where the bit32, bit33 and bit34 are
> > +	 *	encoded in the bit9, bit4 and bit5 of the PTE respectively.
> >  	 *
> >  	 * IO_PGTABLE_QUIRK_NON_STRICT: Skip issuing synchronous leaf TLBIs
> >  	 *	on unmap, for DMA domains using the flush queue mechanism for
> > -- 
> > 2.18.0
> > 
> > _______________________________________________
> > iommu mailing list
> > iommu@lists.linux-foundation.org
> > https://lists.linuxfoundation.org/mailman/listinfo/iommu

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu