Hi AngeloGioacchino,
Thanks very much for reviewing whole the patchset.
On Tue, 2022-01-04 at 16:53 +0100, AngeloGioacchino Del Regno wrote:
> Il 23/09/21 13:58, Yong Wu ha scritto:
> > Prepare for supporting multi-banks for the IOMMU HW, No functional
> > change.
> >
> > Add a new structure(mtk_iommu_bank_data) for each a bank. Each a
> > bank have
> > the independent HW base/IRQ/tlb-range ops, and each a bank has its
> > special
> > iommu-domain(independent pgtable), thus, also move the domain
> > information
> > into it.
> >
> > In previous SoC, we have only one bank which could be treated as
> > bank0(
> > bankid always is 0 for the previous SoC).
> >
> > After adding this structure, the tlb operations and irq could use
> > bank_data as parameter.
> >
> > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> > ---
[...]
> > -struct mtk_iommu_data {
> > +struct mtk_iommu_bank_data {
> > void __iomem *base;
> > int irq;
> > + unsigned int id;
> > + struct device *pdev;
>
> `pdev` may be a bit misleading, as it's conventionally read as
> "platform device"
> and not as the intended "parent device"... perhaps calling it
> parent_dev would be
> more appropriate.
will rename it. Thanks.
>
> > + struct mtk_iommu_data *pdata; /* parent data */
>
> Same here, pdata -> parent_data
Will fix.
>
> > + spinlock_t tlb_lock; /* lock for tlb range
> > flush */
> > + struct mtk_iommu_domain *m4u_dom; /* Each bank has
> > a domain */
> > +};
> > +
> > +struct mtk_iommu_data {
> > + union {
> > + struct { /* only for gen1 */
> > + void __iomem *base;
> > + int irq;
> > + struct mtk_iommu_domain *m4u_dom;
> > + };
> > +
> > + /* only for gen2 that support multi-banks */
> > + struct mtk_iommu_bank_data bank[MTK_IOMMU_BANK_MAX];
> > + };
>
> Sorry, but I really don't like this union... please, update
> mtk_iommu_v1 to always
> use bank[0] or, more appropriately, dynamically allocate the bank
> array with a
> devm_kzalloc call (as to not waste memory on platforms with less
> available banks).
>
> In that case, you would have...
>
> > struct device *dev;
> > struct clk *bclk;
> > phys_addr_t protect_base; /* protect memory
> > base */
> > struct mtk_iommu_suspend_reg reg;
> > - struct mtk_iommu_domain *m4u_dom;
> > struct iommu_group *m4u_group[MTK_IOMMU_GROUP_MAX];
> > bool enable_4GB;
> > - spinlock_t tlb_lock; /* lock for tlb range
> > flush */
>
> struct mtk_iommu_bank_data *banks;
> u8 num_banks;
>
> ... where `num_banks` gets copied from the same in
> mtk_iommu_plat_data, defined
> for each SoC, and where `banks` is dynamically allocated in
> mtk_iommu.c and
> mtk_iommu_v1.c's probe() callback.
Thanks for this idea. I will try this to see if the code will be too
complicate after changing this. If it is, I will use bank[0] always in
mtk_iommu_v1, this looks simpler.
>
> >
> > struct iommu_device iommu;
> > const struct mtk_iommu_plat_data *plat_data;
> >
>
>
