From mboxrd@z Thu Jan 1 00:00:00 1970 From: Will Deacon Subject: Re: [PATCH v8 07/21] iommu/io-pgtable-arm-v7s: Extend MediaTek 4GB Mode Date: Mon, 15 Jul 2019 10:51:57 +0100 Message-ID: <20190715095156.xczfkbm6zpjueq32@willie-the-truck> References: <1561774167-24141-1-git-send-email-yong.wu@mediatek.com> <1561774167-24141-8-git-send-email-yong.wu@mediatek.com> <20190710143649.w5dplhzdpi3bxp7e@willie-the-truck> <1562846036.31342.10.camel@mhfsdcap03> <20190711123129.da4rg35b54u4svfw@willie-the-truck> <1563079280.31342.22.camel@mhfsdcap03> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1563079280.31342.22.camel@mhfsdcap03> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Yong Wu Cc: youlin.pei-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Nicolas Boichat , cui.zhang-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, srv_heupstream-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Tomasz Figa , Will Deacon , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Evan Green , chao.hao-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, Rob Herring , linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, Matthias Brugger , yingjoe.chen-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, anan.sun-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org, Robin Murphy , Matthias Kaehlcke , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On Sun, Jul 14, 2019 at 12:41:20PM +0800, Yong Wu wrote: > On Thu, 2019-07-11 at 13:31 +0100, Will Deacon wrote: > > 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"). Makes sense, thanks. One comment below. > @@ -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; I think you can rework this to do something like: if (cfg->ias > ARM_V7S_ADDR_BITS) return NULL; if (cfg->quirks & IO_PGTABLE_QUIRK_ARM_MTK_EXT) { if (!IS_ENABLED(CONFIG_PHYS_ADDR_T_64BIT)) cfg->oas = min(cfg->oas, ARM_V7S_ADDR_BITS); else if (cfg->oas > 34) return NULL; } else if (cfg->oas > ARM_V7S_ADDR_BITS) { return NULL; } so that we clamp the oas when phys_addr_t is 32-bit for you. That should allow you to remove lots of the checking from iopte_to_paddr() too if you check against oas in the map() function. Does that make sense? Will