Skip to contentSkip to conversation

iommu/tegra: smmu: Support variable MMIO ranges/blocks

PATCH In review Hiroshi Doyu 3 participants 7 messages Jan 31, 2013 – Feb 5, 2013 6 messages not in the archive

Subsystems (3)
Tegra architecture support IOMMU Tegra IOMMU drivers
lore
Share & tools
lore

Hiroshi Doyu

author
#0 lore raw

PATCH v3 1/1 iommu/tegra: smmu: Support variable MMIO ranges/blocks

Presently SMMU registers are located in discontiguous 3 blocks. They are interleaved by MC registers. Ideally SMMU register blocks should be in an independent one block, but it is too late to change this H/W design. In the future Tegra chips over some generations, it is expected that some of register block "size" can be extended towards the end and also more new register blocks will be added at most a few blocks. The starting address of each existing block won't change. This patch allocates multiple number of register blocks dynamically based on the info passed from DT. Those ranges are verified in the accessors{read,write}. This may sacrifice some performance because a new accessors prevents compiler optimization of a fixed size register offset calculation. Since SMMU register accesses are not so frequent, this would be acceptable. This patch is necessary to unify "tegra-smmu.ko" over some Tegra SoC generations.

Signed-off-by Hiroshi Doyu <[email protected]>
Reviewed-by Stephen Warren <[email protected]>
---
 drivers/iommu/tegra-smmu.c |   61 ++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)
drivers/iommu/tegra-smmu.c +36 -25
index 25c1210..024a7e1 100644
@@ -293,7 +293,11 @@ struct smmu_debugfs_info {
  * Per SMMU device - IOMMU device
  */
 struct smmu_device {
-	void __iomem	*regs[NUM_SMMU_REG_BANKS];
+	void __iomem	*regbase;	/* register offset base */
+	void __iomem	**regs;		/* register block start address array */
+	void __iomem	**rege;		/* register block end address array */
+	int		nregs;		/* number of register blocks */
+
 	unsigned long	iovmm_base;	/* remappable base address */
 	unsigned long	page_count;	/* total remappable size */
 	spinlock_t	lock;

@@ -325,35 +329,33 @@ static struct smmu_device *smmu_handle; /* unique for a system */
  */
 static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
 {
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c)
-		return readl(smmu->regs[0] + offs - 0x10);
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200)
-		return readl(smmu->regs[1] + offs - 0x1f0);
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284)
-		return readl(smmu->regs[2] + offs - 0x228);
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i])
+			return readl(addr);
+	}
+
 	BUG();
 }
 
 static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
 {
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c) {
-		writel(val, smmu->regs[0] + offs - 0x10);
-		return;
-	}
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200) {
-		writel(val, smmu->regs[1] + offs - 0x1f0);
-		return;
-	}
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284) {
-		writel(val, smmu->regs[2] + offs - 0x228);
-		return;
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i]) {
+			writel(val, addr);
+			return;
+		}
 	}
+
 	BUG();
 }
 

@@ -1170,7 +1172,13 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(smmu->regs); i++) {
+	smmu->nregs = pdev->num_resources;
+	smmu->regs = devm_kzalloc(dev, 2 * smmu->nregs * sizeof(*smmu->regs),
+				  GFP_KERNEL);
+	smmu->rege = smmu->regs + smmu->nregs;
+	if (!smmu->regs)
+		return -ENOMEM;
+	for (i = 0; i < smmu->nregs; i++) {
 		struct resource *res;
 
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);

@@ -1179,7 +1187,10 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		smmu->regs[i] = devm_request_and_ioremap(&pdev->dev, res);
 		if (!smmu->regs[i])
 			return -EBUSY;
+		smmu->rege[i] = smmu->regs[i] + resource_size(res);
 	}
+	/* Same as "mc" 1st regiter block start address */
+	smmu->regbase = (void __iomem *)((u32)smmu->regs[0] & ~PAGE_MASK);
 
 	err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, &size);
 	if (err)
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): 1359620050-28727-1-git-send-email-hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

#2 lore raw
Quoted 2 lines · Hiroshi Doyu
 drivers/iommu/tegra-smmu.c |   61 ++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)

Okay, applied this patch to arm/tegra, but

Quoted 10 lines

static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) {

(quoted diff) +0 -8
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c)
-		return readl(smmu->regs[0] + offs - 0x10);
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200)
-		return readl(smmu->regs[1] + offs - 0x1f0);
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284)

Review

Quoted 10 lines
(quoted diff) +9 -1
-		return readl(smmu->regs[2] + offs - 0x228);
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i])
+			return readl(addr);
+	}

This loop is purely for checking offset to be valid. And this loop is repeated in the smmu_write() function. I queued a patch on-top to make this more clear. Please double-check:

Quoted 1 line

From 08c4ae0b8955bd818bdfd438a684bd5f8db7fb67 Mon Sep 17 00:00:00 2001

From: Joerg Roedel <[email protected]>
Date: Mon, 4 Feb 2013 20:40:58 +0100
Subject: [PATCH 1/1] iommu/tegra: smmu: Use helper function to check for
valid register offset

Do not repeat the checking loop in the read and write functions. Use a single helper function for that check and call it in both accessors.

Signed-off-by Joerg Roedel <[email protected]>
---
 drivers/iommu/tegra-smmu.c |   35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)
drivers/iommu/tegra-smmu.c +18 -17
index 3e07e8b..5ea8d66 100644
@@ -327,36 +327,37 @@ static struct smmu_device *smmu_handle; /* unique for a system */
 /*
  *	SMMU register accessors
  */
-static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
+static bool inline smmu_valid_reg(struct smmu_device *smmu,
+				  void __iomem *addr)
 {
 	int i;
 
 	for (i = 0; i < smmu->nregs; i++) {
-		void __iomem *addr = smmu->regbase + offs;
-
-		BUG_ON(addr < smmu->regs[i]);
+		if (addr < smmu->regs[i])
+			break;
 		if (addr <= smmu->rege[i])
-			return readl(addr);
+			return true;
 	}
 
-	BUG();
+	return false;
 }
 
-static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
+static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
 {
-	int i;
+	void __iomem *addr = smmu->regbase + offs;
 
-	for (i = 0; i < smmu->nregs; i++) {
-		void __iomem *addr = smmu->regbase + offs;
+	BUG_ON(!smmu_valid_reg(smmu, addr));
 
-		BUG_ON(addr < smmu->regs[i]);
-		if (addr <= smmu->rege[i]) {
-			writel(val, addr);
-			return;
-		}
-	}
+	return readl(addr);
+}
+
+static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
+{
+	void __iomem *addr = smmu->regbase + offs;
+
+	BUG_ON(!smmu_valid_reg(smmu, addr));
 
-	BUG();
+	writel(val, addr);
 }
 
 #define VA_PAGE_TO_PA(va, page)	\
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw

Hi Joerg,

Joerg Roedel <[email protected]> wrote @ Mon, 4 Feb 2013 20:53:32 +0100:

Quoted 16 lines
Quoted 16 lines

static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) {

(quoted diff) +5 -9
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c)
-		return readl(smmu->regs[0] + offs - 0x10);
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200)
-		return readl(smmu->regs[1] + offs - 0x1f0);
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284)
-		return readl(smmu->regs[2] + offs - 0x228);
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
Quoted 8 lines

drivers/iommu/tegra-smmu.c static struct smmu_device *smmu_handle; /* unique for a system */ · line 352

Quoted 4 lines
drivers/iommu/tegra-smmu.c +4 -0
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i])
+			return readl(addr);
+	}

This loop is purely for checking offset to be valid. And this loop is repeated in the smmu_write() function. I queued a patch on-top to make this more clear. Please double-check:

Actually I did the similar thing in the first version of this patch(*1)

+static inline void smmu_check_reg_range(size_t offs) +{
+ int i;
+
+ for (i = 0; i < smmu->nregs; i++) {
+ BUG_ON(offs < smmu->regs[i] - smmu->regbase);
+ if (offs <= smmu->rege[i] - smmu->regbase)
+ break;
+ }
+}

*1:
http://lists.linuxfoundation.org/pipermail/iommu/2013-January/005072.html

Then, Stehpen pointed out about this check function(*2).

"... here, you'd be doing the loop every access anyway, so you may as well not calculate regbase at all, move the body of smmu_check_reg_range() into smmu_read()/smmu_write(), and do the access inside the if statement inside the loop, with the per-range mapping."

*2:
http://lists.linuxfoundation.org/pipermail/iommu/2013-January/005074.html

I might not get Stehpen's point in the latest patch(?), though....

A message that is not in the archive (sent off-list or not yet received): 20130204.223114.121259250064618894.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

#6 lore raw
Quoted 45 lines · Hiroshi Doyu

Hi Joerg,

Joerg Roedel <[email protected]> wrote @ Mon, 4 Feb 2013 20:53:32 +0100:

Quoted 16 lines
Quoted 16 lines

static inline u32 smmu_read(struct smmu_device *smmu, size_t offs) {

(quoted diff) +5 -9
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c)
-		return readl(smmu->regs[0] + offs - 0x10);
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200)
-		return readl(smmu->regs[1] + offs - 0x1f0);
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284)
-		return readl(smmu->regs[2] + offs - 0x228);
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
Quoted 8 lines

drivers/iommu/tegra-smmu.c static struct smmu_device *smmu_handle; /* unique for a system */ · line 352

Quoted 4 lines
drivers/iommu/tegra-smmu.c +4 -0
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i])
+			return readl(addr);
+	}

This loop is purely for checking offset to be valid. And this loop is repeated in the smmu_write() function. I queued a patch on-top to make this more clear. Please double-check:

Actually I did the similar thing in the first version of this patch(*1)

(quoted diff) +10 -0
+static inline void smmu_check_reg_range(size_t offs)
+{
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+	    BUG_ON(offs < smmu->regs[i] - smmu->regbase);
+	    		if (offs <= smmu->rege[i] - smmu->regbase)
+			   	 break;
+	 }
+}

*1:
http://lists.linuxfoundation.org/pipermail/iommu/2013-January/005072.html

Quoted 6 lines

Then, Stehpen pointed out about this check function(*2).

"... here, you'd be doing the loop every access anyway, so you may as well not calculate regbase at all, move the body of smmu_check_reg_range() into smmu_read()/smmu_write(), and do the access inside the if statement inside the loop, with the per-range mapping."

Upon reflection, that comment probably isn't correct, since the only way to know where each register range begins, relative to the register numbers that the driver uses, is to calculate reg_base. So, I think you do need reg_base, and Joerg's latest patch seems reasonable.

Quoted 4 lines

*2:
http://lists.linuxfoundation.org/pipermail/iommu/2013-January/005074.html

I might not get Stehpen's point in the latest patch(?), though....

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#8 lore raw

Stephen Warren <[email protected]> wrote @ Mon, 4 Feb 2013 21:35:45 +0100:

Quoted 22 lines
Quoted 22 lines
Quoted 3 lines

This loop is purely for checking offset to be valid. And this loop is repeated in the smmu_write() function. I queued a patch on-top to make this more clear. Please double-check:

Actually I did the similar thing in the first version of this patch(*1)

(quoted diff) +10 -0
+static inline void smmu_check_reg_range(size_t offs)
+{
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+	    BUG_ON(offs < smmu->regs[i] - smmu->regbase);
+	    		if (offs <= smmu->rege[i] - smmu->regbase)
+			   	 break;
+	 }
+}

*1:
http://lists.linuxfoundation.org/pipermail/iommu/2013-January/005072.html

Then, Stehpen pointed out about this check function(*2).

Quoted 9 lines
Quoted 4 lines

"... here, you'd be doing the loop every access anyway, so you may as well not calculate regbase at all, move the body of smmu_check_reg_range() into smmu_read()/smmu_write(), and do the access inside the if statement inside the loop, with the per-range mapping."

Upon reflection, that comment probably isn't correct, since the only way to know where each register range begins, relative to the register numbers that the driver uses, is to calculate reg_base. So, I think you do need reg_base, and Joerg's latest patch seems reasonable.

Ok, now it's clear. Joerg, please get yours merged on the top of this.

A message that is not in the archive (sent off-list or not yet received): 20130204.223921.2367725583637314.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

author Reviewed-by Stephen Warren
#10 lore raw

Hiroshi Doyu <[email protected]> wrote @ Mon, 04 Feb 2013 22:39:21 +0200 (EET):

Quoted 6 lines
Quoted 4 lines

Upon reflection, that comment probably isn't correct, since the only way to know where each register range begins, relative to the register numbers that the driver uses, is to calculate reg_base. So, I think you do need reg_base, and Joerg's latest patch seems reasonable.

Ok, now it's clear. Joerg, please get yours merged on the top of this.

Joerg, could you please replace v3 with the attached v4?

From 2f71337453483da291c29b744ca165857c8b84e4 Mon Sep 17 00:00:00 2001
From: Hiroshi Doyu <[email protected]>
Date: Wed, 9 Jan 2013 12:38:47 +0200
Subject: [v4 1/1] iommu/tegra: smmu: Support variable MMIO ranges/blocks

Presently SMMU registers are located in discontiguous 3 blocks. They are interleaved by MC registers. Ideally SMMU register blocks should be in an independent one block, but it is too late to change this H/W design. In the future Tegra chips over some generations, it is expected that some of register block "size" can be extended towards the end and also more new register blocks will be added at most a few blocks. The starting address of each existing block won't change. This patch allocates multiple number of register blocks dynamically based on the info passed from DT. Those ranges are verified in the accessors{read,write}. This may sacrifice some performance because a new accessors prevents compiler optimization of a fixed size register offset calculation. Since SMMU register accesses are not so frequent, this would be acceptable. This patch is necessary to unify "tegra-smmu.ko" over some Tegra SoC generations.

Signed-off-by Hiroshi Doyu <[email protected]>
Reviewed-by Stephen Warren <[email protected]>

---
Update: Set rege the last valid address.
---

---
 drivers/iommu/tegra-smmu.c |   61 ++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 25 deletions(-)
drivers/iommu/tegra-smmu.c +36 -25
index fc17889..0d403fe 100644
@@ -293,7 +293,11 @@ struct smmu_debugfs_info {
  * Per SMMU device - IOMMU device
  */
 struct smmu_device {
-	void __iomem	*regs[NUM_SMMU_REG_BANKS];
+	void __iomem	*regbase;	/* register offset base */
+	void __iomem	**regs;		/* register block start address array */
+	void __iomem	**rege;		/* register block end address array */
+	int		nregs;		/* number of register blocks */
+
 	unsigned long	iovmm_base;	/* remappable base address */
 	unsigned long	page_count;	/* total remappable size */
 	spinlock_t	lock;

@@ -325,35 +329,33 @@ static struct smmu_device *smmu_handle; /* unique for a system */
  */
 static inline u32 smmu_read(struct smmu_device *smmu, size_t offs)
 {
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c)
-		return readl(smmu->regs[0] + offs - 0x10);
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200)
-		return readl(smmu->regs[1] + offs - 0x1f0);
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284)
-		return readl(smmu->regs[2] + offs - 0x228);
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i])
+			return readl(addr);
+	}
+
 	BUG();
 }
 
 static inline void smmu_write(struct smmu_device *smmu, u32 val, size_t offs)
 {
-	BUG_ON(offs < 0x10);
-	if (offs < 0x3c) {
-		writel(val, smmu->regs[0] + offs - 0x10);
-		return;
-	}
-	BUG_ON(offs < 0x1f0);
-	if (offs < 0x200) {
-		writel(val, smmu->regs[1] + offs - 0x1f0);
-		return;
-	}
-	BUG_ON(offs < 0x228);
-	if (offs < 0x284) {
-		writel(val, smmu->regs[2] + offs - 0x228);
-		return;
+	int i;
+
+	for (i = 0; i < smmu->nregs; i++) {
+		void __iomem *addr = smmu->regbase + offs;
+
+		BUG_ON(addr < smmu->regs[i]);
+		if (addr <= smmu->rege[i]) {
+			writel(val, addr);
+			return;
+		}
 	}
+
 	BUG();
 }
 

@@ -1170,7 +1172,13 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
-	for (i = 0; i < ARRAY_SIZE(smmu->regs); i++) {
+	smmu->nregs = pdev->num_resources;
+	smmu->regs = devm_kzalloc(dev, 2 * smmu->nregs * sizeof(*smmu->regs),
+				  GFP_KERNEL);
+	smmu->rege = smmu->regs + smmu->nregs;
+	if (!smmu->regs)
+		return -ENOMEM;
+	for (i = 0; i < smmu->nregs; i++) {
 		struct resource *res;
 
 		res = platform_get_resource(pdev, IORESOURCE_MEM, i);

@@ -1179,7 +1187,10 @@ static int tegra_smmu_probe(struct platform_device *pdev)
 		smmu->regs[i] = devm_request_and_ioremap(&pdev->dev, res);
 		if (!smmu->regs[i])
 			return -EBUSY;
+		smmu->rege[i] = smmu->regs[i] + resource_size(res) - 1;
 	}
+	/* Same as "mc" 1st regiter block start address */
+	smmu->regbase = (void __iomem *)((u32)smmu->regs[0] & ~PAGE_MASK);
 
 	err = of_get_dma_window(dev->of_node, NULL, 0, NULL, &base, &size);
 	if (err)
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): 20130204.225407.2202093543593597795.hdoyu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org

#12 lore raw
Quoted 10 lines · Hiroshi Doyu

Hiroshi Doyu <[email protected]> wrote @ Mon, 04 Feb 2013 22:39:21 +0200 (EET):

Quoted 6 lines
Quoted 4 lines

Upon reflection, that comment probably isn't correct, since the only way to know where each register range begins, relative to the register numbers that the driver uses, is to calculate reg_base. So, I think you do need reg_base, and Joerg's latest patch seems reasonable.

Ok, now it's clear. Joerg, please get yours merged on the top of this.

Joerg, could you please replace v3 with the attached v4?

Done.