[V4,2/2] PCI: rcar: Recalculate inbound range alignment for each controller entry

Commit Message

Marek Vasut Oct. 26, 2019, 6:26 p.m. UTC

From: Marek Vasut <[e-mail]>

Due to hardware constraints, the size of each inbound range entry
populated into the controller cannot be larger than the alignment
of the entry's start address. Currently, the alignment for each
"dma-ranges" inbound range is calculated only once for each range
and the increment for programming the controller is also derived
from it only once. Thus, a "dma-ranges" entry describing a memory
at 0x48000000 and size 0x38000000 would lead to multiple controller
entries, each 0x08000000 long.

This is inefficient, especially considering that by adding the size
to the start address, the alignment increases. This patch moves the
alignment calculation into the loop populating the controller entries,
thus updating the alignment for each controller entry.

Signed-off-by: Marek Vasut <[e-mail]>
Reviewed-by: Andrew Murray <[e-mail]>
Reviewed-by: Simon Horman <[e-mail]>
Cc: Geert Uytterhoeven <[e-mail]>
Cc: Lorenzo Pieralisi <[e-mail]>
Cc: Wolfram Sang <[e-mail]>
Cc: [e-mail]
To: [e-mail]
---
V2: Update on top of 1/3
V3: No change
V4: Rebase on next/master and dropped 2/3 patch
---
 drivers/pci/controller/pcie-rcar.c | 37 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 18 deletions(-)

Comments

Yoshihiro Shimoda Oct. 28, 2019, 8:35 a.m. UTC | #1

Hi Marek-san.

> From: Marek Vasut, Sent: Sunday, October 27, 2019 3:27 AM
> 
> Due to hardware constraints, the size of each inbound range entry
> populated into the controller cannot be larger than the alignment
> of the entry's start address. Currently, the alignment for each
> "dma-ranges" inbound range is calculated only once for each range
> and the increment for programming the controller is also derived
> from it only once. Thus, a "dma-ranges" entry describing a memory
> at 0x48000000 and size 0x38000000 would lead to multiple controller
> entries, each 0x08000000 long.

I added a debug code [1] and I confirmed that each entry is not 0x08000000 long [2].

After fixed the commit log above,

Reviewed-by: Yoshihiro Shimoda <[e-mail]>

And I tested on r8a7795-salvator-xs with my debug code. So,

Tested-by: Yoshihiro Shimoda <[e-mail]>

Best regards,
Yoshihiro Shimoda

---
[1] Based on next-20191025 with this patch series:
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index fde6ec1..9bdd39e 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -2684,7 +2684,7 @@
 				0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
 				0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
 			/* Map all possible DDR as inbound ranges */
-			dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000>;
+			dma-ranges = <0x42000000 0 0x48000000 0 0x48000000 0 0x38000000>;
 			interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
 				<GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
 				<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index 0dadccb..54ad977 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -11,6 +11,8 @@
  * Author: Phil Edworthy <[e-mail]>
  */
 
+#define DEBUG
+
 #include <linux/bitops.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
@@ -1054,6 +1056,8 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
 		mask = roundup_pow_of_two(size) - 1;
 		mask &= ~0xf;
 
+		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
+			idx, cpu_addr, size, pci_addr);
 		/*
 		 * Set up 64-bit inbound regions as the range parser doesn't
 		 * distinguish between 32 and 64-bit types.
---
[2]
[    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000
[    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000
[    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000
---

Lorenzo Pieralisi Oct. 28, 2019, 10:20 a.m. UTC | #2

On Mon, Oct 28, 2019 at 08:35:32AM +0000, Yoshihiro Shimoda wrote:
> Hi Marek-san.
> 
> > From: Marek Vasut, Sent: Sunday, October 27, 2019 3:27 AM
> > 
> > Due to hardware constraints, the size of each inbound range entry
> > populated into the controller cannot be larger than the alignment
> > of the entry's start address. Currently, the alignment for each
> > "dma-ranges" inbound range is calculated only once for each range
> > and the increment for programming the controller is also derived
> > from it only once. Thus, a "dma-ranges" entry describing a memory
> > at 0x48000000 and size 0x38000000 would lead to multiple controller
> > entries, each 0x08000000 long.
> 
> I added a debug code [1] and I confirmed that each entry is not 0x08000000 long [2].
> 
> After fixed the commit log above,

So what does this mean in practice ? Does it mean that the commit log is
wrong or that the issue is not present as described, in the mainline
code ?

Please clarify and send a v5 accordingly.

Thanks,
Lorenzo

> Reviewed-by: Yoshihiro Shimoda <[e-mail]>
> 
> And I tested on r8a7795-salvator-xs with my debug code. So,
> 
> Tested-by: Yoshihiro Shimoda <[e-mail]>
> 
> Best regards,
> Yoshihiro Shimoda
> 
> ---
> [1] Based on next-20191025 with this patch series:
> diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> index fde6ec1..9bdd39e 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> @@ -2684,7 +2684,7 @@
>  				0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
>  				0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
>  			/* Map all possible DDR as inbound ranges */
> -			dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000>;
> +			dma-ranges = <0x42000000 0 0x48000000 0 0x48000000 0 0x38000000>;
>  			interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
>  				<GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
>  				<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
> diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> index 0dadccb..54ad977 100644
> --- a/drivers/pci/controller/pcie-rcar.c
> +++ b/drivers/pci/controller/pcie-rcar.c
> @@ -11,6 +11,8 @@
>   * Author: Phil Edworthy <[e-mail]>
>   */
>  
> +#define DEBUG
> +
>  #include <linux/bitops.h>
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> @@ -1054,6 +1056,8 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
>  		mask = roundup_pow_of_two(size) - 1;
>  		mask &= ~0xf;
>  
> +		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
> +			idx, cpu_addr, size, pci_addr);
>  		/*
>  		 * Set up 64-bit inbound regions as the range parser doesn't
>  		 * distinguish between 32 and 64-bit types.
> ---
> [2]
> [    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000
> [    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000
> [    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000
> ---
>

Yoshihiro Shimoda Oct. 29, 2019, 1:18 a.m. UTC | #3

Hi Lorenzo,

> From: Lorenzo Pieralisi, Sent: Monday, October 28, 2019 7:21 PM
> 
> On Mon, Oct 28, 2019 at 08:35:32AM +0000, Yoshihiro Shimoda wrote:
> > Hi Marek-san.
> >
> > > From: Marek Vasut, Sent: Sunday, October 27, 2019 3:27 AM
> > >
> > > Due to hardware constraints, the size of each inbound range entry
> > > populated into the controller cannot be larger than the alignment
> > > of the entry's start address. Currently, the alignment for each
> > > "dma-ranges" inbound range is calculated only once for each range
> > > and the increment for programming the controller is also derived
> > > from it only once. Thus, a "dma-ranges" entry describing a memory
> > > at 0x48000000 and size 0x38000000 would lead to multiple controller
> > > entries, each 0x08000000 long.
> >
> > I added a debug code [1] and I confirmed that each entry is not 0x08000000 long [2].
> >
> > After fixed the commit log above,
> 
> So what does this mean in practice ? Does it mean that the commit log is
> wrong or that the issue is not present as described, in the mainline
> code ?

I meant the commit log is wrong. In such the case, the multiple controller
entries has 3 kind of size like below.

> > +		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
> > +			idx, cpu_addr, size, pci_addr);
<snip>
> > [    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000

The first entry's size is 0x08000000.

> > [    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000

The second one's size is 0x10000000.

> > [    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000

The third one's size is 0x20000000.

> Please clarify and send a v5 accordingly.

Marek-san, would you send a v5 patch series?

Best regards,
Yoshihiro Shimoda

> Thanks,
> Lorenzo
> 
> > Reviewed-by: Yoshihiro Shimoda <[e-mail]>
> >
> > And I tested on r8a7795-salvator-xs with my debug code. So,
> >
> > Tested-by: Yoshihiro Shimoda <[e-mail]>
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> > ---
> > [1] Based on next-20191025 with this patch series:
> > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > index fde6ec1..9bdd39e 100644
> > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > @@ -2684,7 +2684,7 @@
> >  				0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
> >  				0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
> >  			/* Map all possible DDR as inbound ranges */
> > -			dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000>;
> > +			dma-ranges = <0x42000000 0 0x48000000 0 0x48000000 0 0x38000000>;
> >  			interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
> >  				<GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
> >  				<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
> > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > index 0dadccb..54ad977 100644
> > --- a/drivers/pci/controller/pcie-rcar.c
> > +++ b/drivers/pci/controller/pcie-rcar.c
> > @@ -11,6 +11,8 @@
> >   * Author: Phil Edworthy <[e-mail]>
> >   */
> >
> > +#define DEBUG
> > +
> >  #include <linux/bitops.h>
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > @@ -1054,6 +1056,8 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
> >  		mask = roundup_pow_of_two(size) - 1;
> >  		mask &= ~0xf;
> >
> > +		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
> > +			idx, cpu_addr, size, pci_addr);
> >  		/*
> >  		 * Set up 64-bit inbound regions as the range parser doesn't
> >  		 * distinguish between 32 and 64-bit types.
> > ---
> > [2]
> > [    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000
> > [    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000
> > [    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000
> > ---
> >

Lorenzo Pieralisi Oct. 29, 2019, 10:34 a.m. UTC | #4

On Tue, Oct 29, 2019 at 01:18:04AM +0000, Yoshihiro Shimoda wrote:
> Hi Lorenzo,
> 
> > From: Lorenzo Pieralisi, Sent: Monday, October 28, 2019 7:21 PM
> > 
> > On Mon, Oct 28, 2019 at 08:35:32AM +0000, Yoshihiro Shimoda wrote:
> > > Hi Marek-san.
> > >
> > > > From: Marek Vasut, Sent: Sunday, October 27, 2019 3:27 AM
> > > >
> > > > Due to hardware constraints, the size of each inbound range entry
> > > > populated into the controller cannot be larger than the alignment
> > > > of the entry's start address. Currently, the alignment for each
> > > > "dma-ranges" inbound range is calculated only once for each range
> > > > and the increment for programming the controller is also derived
> > > > from it only once. Thus, a "dma-ranges" entry describing a memory
> > > > at 0x48000000 and size 0x38000000 would lead to multiple controller
> > > > entries, each 0x08000000 long.
> > >
> > > I added a debug code [1] and I confirmed that each entry is not 0x08000000 long [2].
> > >
> > > After fixed the commit log above,
> > 
> > So what does this mean in practice ? Does it mean that the commit log is
> > wrong or that the issue is not present as described, in the mainline
> > code ?
> 
> I meant the commit log is wrong. In such the case, the multiple controller
> entries has 3 kind of size like below.

OK, that's confusing. The commit log is describing the issue it is
fixing and you are reporting that's not what happens in practice, so in
short my question is, is it possible to describe the issue you
are fixing with an example representative of what's happening and
explaining why we need to apply this patch please ?

Thanks,
Lorenzo

> > > +		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
> > > +			idx, cpu_addr, size, pci_addr);
> <snip>
> > > [    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000
> 
> The first entry's size is 0x08000000.
> 
> > > [    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000
> 
> The second one's size is 0x10000000.
> 
> > > [    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000
> 
> The third one's size is 0x20000000.
> 
> > Please clarify and send a v5 accordingly.
> 
> Marek-san, would you send a v5 patch series?
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > Thanks,
> > Lorenzo
> > 
> > > Reviewed-by: Yoshihiro Shimoda <[e-mail]>
> > >
> > > And I tested on r8a7795-salvator-xs with my debug code. So,
> > >
> > > Tested-by: Yoshihiro Shimoda <[e-mail]>
> > >
> > > Best regards,
> > > Yoshihiro Shimoda
> > >
> > > ---
> > > [1] Based on next-20191025 with this patch series:
> > > diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > > index fde6ec1..9bdd39e 100644
> > > --- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > > +++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
> > > @@ -2684,7 +2684,7 @@
> > >  				0x02000000 0 0x30000000 0 0x30000000 0 0x08000000
> > >  				0x42000000 0 0x38000000 0 0x38000000 0 0x08000000>;
> > >  			/* Map all possible DDR as inbound ranges */
> > > -			dma-ranges = <0x42000000 0 0x40000000 0 0x40000000 0 0x40000000>;
> > > +			dma-ranges = <0x42000000 0 0x48000000 0 0x48000000 0 0x38000000>;
> > >  			interrupts = <GIC_SPI 116 IRQ_TYPE_LEVEL_HIGH>,
> > >  				<GIC_SPI 117 IRQ_TYPE_LEVEL_HIGH>,
> > >  				<GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
> > > diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
> > > index 0dadccb..54ad977 100644
> > > --- a/drivers/pci/controller/pcie-rcar.c
> > > +++ b/drivers/pci/controller/pcie-rcar.c
> > > @@ -11,6 +11,8 @@
> > >   * Author: Phil Edworthy <[e-mail]>
> > >   */
> > >
> > > +#define DEBUG
> > > +
> > >  #include <linux/bitops.h>
> > >  #include <linux/clk.h>
> > >  #include <linux/delay.h>
> > > @@ -1054,6 +1056,8 @@ static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
> > >  		mask = roundup_pow_of_two(size) - 1;
> > >  		mask &= ~0xf;
> > >
> > > +		dev_dbg(pcie->dev, "idx%d: 0x%016llx..0x%016llx -> 0x%016llx\n",
> > > +			idx, cpu_addr, size, pci_addr);
> > >  		/*
> > >  		 * Set up 64-bit inbound regions as the range parser doesn't
> > >  		 * distinguish between 32 and 64-bit types.
> > > ---
> > > [2]
> > > [    0.374771] rcar-pcie fe000000.pcie: idx0: 0x0000000048000000..0x0000000008000000 -> 0x0000000048000000
> > > [    0.374777] rcar-pcie fe000000.pcie: idx2: 0x0000000050000000..0x0000000010000000 -> 0x0000000050000000
> > > [    0.374782] rcar-pcie fe000000.pcie: idx4: 0x0000000060000000..0x0000000020000000 -> 0x0000000060000000
> > > ---
> > >

Yoshihiro Shimoda Oct. 29, 2019, 11:12 a.m. UTC | #5

Hi Lorenzo,

> From: Lorenzo Pieralisi, Sent: Tuesday, October 29, 2019 7:35 PM
> 
> On Tue, Oct 29, 2019 at 01:18:04AM +0000, Yoshihiro Shimoda wrote:
> > Hi Lorenzo,
> >
> > > From: Lorenzo Pieralisi, Sent: Monday, October 28, 2019 7:21 PM
> > >
> > > On Mon, Oct 28, 2019 at 08:35:32AM +0000, Yoshihiro Shimoda wrote:
> > > > Hi Marek-san.
> > > >
> > > > > From: Marek Vasut, Sent: Sunday, October 27, 2019 3:27 AM
> > > > >
> > > > > Due to hardware constraints, the size of each inbound range entry
> > > > > populated into the controller cannot be larger than the alignment
> > > > > of the entry's start address. Currently, the alignment for each
> > > > > "dma-ranges" inbound range is calculated only once for each range
> > > > > and the increment for programming the controller is also derived
> > > > > from it only once. Thus, a "dma-ranges" entry describing a memory
> > > > > at 0x48000000 and size 0x38000000 would lead to multiple controller
> > > > > entries, each 0x08000000 long.
> > > >
> > > > I added a debug code [1] and I confirmed that each entry is not 0x08000000 long [2].
> > > >
> > > > After fixed the commit log above,
> > >
> > > So what does this mean in practice ? Does it mean that the commit log is
> > > wrong or that the issue is not present as described, in the mainline
> > > code ?
> >
> > I meant the commit log is wrong. In such the case, the multiple controller
> > entries has 3 kind of size like below.
> 
> OK, that's confusing. The commit log is describing the issue it is
> fixing and you are reporting that's not what happens in practice, so in
> short my question is, is it possible to describe the issue you
> are fixing with an example representative of what's happening and
> explaining why we need to apply this patch please ?

I'm very sorry, I completely misunderstood the original commit description.
I misunderstood "each 0x08000000 log" was a behavior of after we applied
this patch...

So, the description is no problem. In other words, we don't need to fix
any description on this patch.

Best regards,
Yoshihiro Shimoda

Patch

diff --git a/drivers/pci/controller/pcie-rcar.c b/drivers/pci/controller/pcie-rcar.c
index b2a5c3e94245..0dadccb61051 100644
--- a/drivers/pci/controller/pcie-rcar.c
+++ b/drivers/pci/controller/pcie-rcar.c
@@ -1030,29 +1030,30 @@  static int rcar_pcie_inbound_ranges(struct rcar_pcie *pcie,
 	if (restype & IORESOURCE_PREFETCH)
 		flags |= LAM_PREFETCH;
 
-	/*
-	 * If the size of the range is larger than the alignment of the start
-	 * address, we have to use multiple entries to perform the mapping.
-	 */
-	if (cpu_addr > 0) {
-		unsigned long nr_zeros = __ffs64(cpu_addr);
-		u64 alignment = 1ULL << nr_zeros;
-
-		size = min(range->size, alignment);
-	} else {
-		size = range->size;
-	}
-	/* Hardware supports max 4GiB inbound region */
-	size = min(size, 1ULL << 32);
-
-	mask = roundup_pow_of_two(size) - 1;
-	mask &= ~0xf;
-
 	while (cpu_addr < cpu_end) {
 		if (idx >= MAX_NR_INBOUND_MAPS - 1) {
 			dev_err(pcie->dev, "Failed to map inbound regions!\n");
 			return -EINVAL;
 		}
+		/*
+		 * If the size of the range is larger than the alignment of
+		 * the start address, we have to use multiple entries to
+		 * perform the mapping.
+		 */
+		if (cpu_addr > 0) {
+			unsigned long nr_zeros = __ffs64(cpu_addr);
+			u64 alignment = 1ULL << nr_zeros;
+
+			size = min(range->size, alignment);
+		} else {
+			size = range->size;
+		}
+		/* Hardware supports max 4GiB inbound region */
+		size = min(size, 1ULL << 32);
+
+		mask = roundup_pow_of_two(size) - 1;
+		mask &= ~0xf;
+
 		/*
 		 * Set up 64-bit inbound regions as the range parser doesn't
 		 * distinguish between 32 and 64-bit types.