On Mon, 2015-07-27 at 12:21 +0800, Yong Wu wrote:
> On Fri, 2015-07-24 at 17:53 +0100, Will Deacon wrote:
> > On Fri, Jul 24, 2015 at 06:24:26AM +0100, Yong Wu wrote:
> > > On Tue, 2015-07-21 at 18:11 +0100, Will Deacon wrote:
> > > > On Thu, Jul 16, 2015 at 10:04:32AM +0100, Yong Wu wrote:
> > > > > +/* level 2 pagetable */
> > > > > +#define ARM_SHORT_PTE_TYPE_LARGE BIT(0)
> > > > > +#define ARM_SHORT_PTE_SMALL_XN BIT(0)
> > > > > +#define ARM_SHORT_PTE_TYPE_SMALL BIT(1)
> > > > > +#define ARM_SHORT_PTE_B BIT(2)
> > > > > +#define ARM_SHORT_PTE_C BIT(3)
> > > > > +#define ARM_SHORT_PTE_SMALL_TEX0 BIT(6)
> > > > > +#define ARM_SHORT_PTE_IMPLE BIT(9)
> > > >
> > > > This is AP[2] for small pages.
> > >
> > > Sorry, In our pagetable bit9 in PGD and PTE is PA[32] that is for the
> > > dram size over 4G. I didn't care it is different in PTE of the standard
> > > spec.
> > > And I don't use the AP[2] currently, so I only delete this line in next
> > > time.
> >
> > Is this related to the "special bit". What would be good is a comment
> > next to the #define for the quirk describing *exactly* that differs in
> > your implementation. Without that, it's very difficult to know what is
> > intentional and what is actually broken.
>
> I will add the comment alongside the #define.
>
> >
> > > > > +static arm_short_iopte
> > > > > +__arm_short_pte_prot(struct arm_short_io_pgtable *data, int prot,
> > > > > bool large)
> > > > > +{
> > > > > + arm_short_iopte pteprot;
> > > > > +
> > > > > + pteprot = ARM_SHORT_PTE_S | ARM_SHORT_PTE_nG;
> > > > > + pteprot |= large ? ARM_SHORT_PTE_TYPE_LARGE :
> > > > > + ARM_SHORT_PTE_TYPE_SMALL;
> > > > > + if (prot & IOMMU_CACHE)
> > > > > + pteprot |= ARM_SHORT_PTE_B | ARM_SHORT_PTE_C;
> > > > > + if (prot & IOMMU_WRITE)
> > > > > + pteprot |= large ? ARM_SHORT_PTE_LARGE_TEX0 :
> > > > > + ARM_SHORT_PTE_SMALL_TEX0;
> > > >
> > > > This doesn't make any sense. TEX[2:0] is all about memory attributes,
> > > > not
> > > > permissions, so you're making the mapping write-back, write-allocate but
> > > > that's not what the IOMMU_* values are about.
> > >
> > > I will delete it.
> >
> > Well, can you not control mapping permissions with the AP bits? The idea
> > of the IOMMU flags are:
> >
> > IOMMU_CACHE : Install a normal, cacheable mapping (you've got this right)
> > IOMMU_READ : Allow read access for the device
> > IOMMU_WRITE : Allow write access for the device
> > IOMMU_NOEXEC : Disallow execute access for the device
> >
> > so the caller to iommu_map passes in a bitmap of these, which you need to
> > encode in the page-table entry.
>
> From the spec, AP[2] differentiate the read/write and readonly.
> How about this?:
> //===============
> #define ARM_SHORT_PGD_FULL_ACCESS (3 << 10)
> #define ARM_SHORT_PGD_RDONLY BIT(15)
>
> pgdprot |= ARM_SHORT_PGD_FULL_ACCESS;/* or other names? */
> if(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
> pgdprot |= ARM_SHORT_PGD_RDONLY;
> //===============
> pte is the same.
>
> Sorry, Our HW don't meet the standard spec fully. it don't implement the
> AP bits.
Hi Will,
About the AP bits, I may have to add a new quirk for it...
Current I add AP in pte like this:
#define ARM_SHORT_PTE_RD_WR (3 << 4)
#define ARM_SHORT_PTE_RDONLY BIT(9)
pteprot |= ARM_SHORT_PTE_RD_WR;
If(!(prot & IOMMU_WRITE) && (prot & IOMMU_READ))
pteprot |= ARM_SHORT_PTE_RDONLY;
The problem is that the BIT(9) in the level1 and level2 pagetable of our
HW has been used for PA[32] that is for the dram size over 4G.
so I had to add a quirk to disable bit9 while RDONLY case.
(If BIT9 isn't disabled, the HW treat it as the PA[32] case then it will
translation fault..)
like: IO_PGTABLE_QUIRK_SHORT_MTK ?
> >
> > Will
> _______________________________________________
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
