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:

quoted

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@mediatek.com>
---

[...]
  

quoted

-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.

quoted

+	struct mtk_iommu_data		*pdata;   /* parent data */

Same here, pdata -> parent_data

Will fix.

quoted

+	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...

quoted

  	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.

quoted

  
  	struct iommu_device		iommu;
  	const struct mtk_iommu_plat_data *plat_data;

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel