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
