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

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

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 <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > And I tested on r8a7795-salvator-xs with my debug code. So,
> > >
> > > Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > >
> > > 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 <phil.edworthy@renesas.com>
> > >   */
> > >
> > > +#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
> > > ---
> > >