[PATCH v2 0/2] PCI: mediatek: fix warning and enlarge the PCIe2AHB window size

[PATCH v2 0/2] PCI: mediatek: fix warning and enlarge the PCIe2AHB window size

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

Two patches:
patch 1 fix the complain of scripts/coccinelle/api/resource_size.cocci
patch 2 enlarge the PCIe2AHB window size to support fully access of 4GB DRAM from EP DMA.

v2:
 - Fix the checkpatch complains for patch 1.
 - update the commit message and change title of patch 1 for changelog conventions.
 - Add patch 2.

Honghui Zhang (2):
  PCI: mediatek: Use resource_size function on resource object
  PCI: mediatek: Enlarge PCIe2AHB window size to support 4GB DRAM

 drivers/pci/controller/pcie-mediatek.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

-- 
2.6.4

[PATCH v2 1/2] PCI: mediatek: Use resource_size function on resource object

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

scripts/coccinelle/api/resource_size.cocci complain about the
following warning:

pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem

Use resource_size(mem) instead of mem->end - mem->start to eliminate the
complain. Since the MMIO window size for both MT2712 and MT7622 are all
0x1000_0000, this change also fix the AHB2PCIe window size smaller than
HW MMIO window size issue by change the values of fls(size) from
fls(0xfff_ffff) to fls(0x1000_0000).

Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 55e471c..01126b8 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	struct resource *mem = &pcie->mem;
 	const struct mtk_pcie_soc *soc = port->pcie->soc;
 	u32 val;
-	size_t size;
 	int err;
 
 	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
@@ -706,8 +705,8 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 		mtk_pcie_enable_msi(port);
 
 	/* Set AHB to PCIe translation windows */
-	size = mem->end - mem->start;
-	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
+	val = lower_32_bits(mem->start)
+	      | AHB2PCIE_SIZE(fls(resource_size(mem)));
 	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
 
 	val = upper_32_bits(mem->start);
-- 
2.6.4

[PATCH v2 2/2] PCI: mediatek: Enlarge PCIe2AHB window size to support 4GB DRAM

[honghui.zhang]

From: Honghui Zhang <honghui.zhang@mediatek.com>

The PCIE_AXI_WINDOW0 defines the translate window size for the request
from EP side. Request outside of this window will be treated as
unsupported request.

Enlarge this window size from fls(0xffffffff) to 2^33 to support 8GB
translate address range then EP DMA is capable of fully access 4GB
DRAM range(physical DRAM is start from 0x40000000).

Reported-by: Bjorn Helgaas <bhelgaas@google.com>
Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
---
 drivers/pci/controller/pcie-mediatek.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
index 01126b8..60326c4 100644
--- a/drivers/pci/controller/pcie-mediatek.c
+++ b/drivers/pci/controller/pcie-mediatek.c
@@ -90,6 +90,12 @@
 #define AHB2PCIE_SIZE(x)	((x) & GENMASK(4, 0))
 #define PCIE_AXI_WINDOW0	0x448
 #define WIN_ENABLE		BIT(7)
+/*
+ * Define PCIe to AHB window size as 2^33 to support max 8GB address space
+ * translate, support least 4GB DRAM size access from EP DMA(physical DRAM
+ * start from 0x40000000).
+ */
+#define PCIE2AHB_SIZE	0x21
 
 /* PCIe V2 configuration transaction header */
 #define PCIE_CFG_HEADER0	0x460
@@ -713,7 +719,7 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
 	writel(val, port->base + PCIE_AHB_TRANS_BASE0_H);
 
 	/* Set PCIe to AXI translation memory space.*/
-	val = fls(0xffffffff) | WIN_ENABLE;
+	val = PCIE2AHB_SIZE | WIN_ENABLE;
 	writel(val, port->base + PCIE_AXI_WINDOW0);
 
 	return 0;
-- 
2.6.4

Re: [PATCH v2 1/2] PCI: mediatek: Use resource_size function on resource object

[Bjorn Helgaas]

On Thu, Jan 31, 2019 at 04:01:52PM +0800, honghui.zhang@mediatek.com wrote:
> From: Honghui Zhang <honghui.zhang@mediatek.com>
> 
> scripts/coccinelle/api/resource_size.cocci complain about the
> following warning:
> 
> pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> 
> Use resource_size(mem) instead of mem->end - mem->start to eliminate the
> complain. Since the MMIO window size for both MT2712 and MT7622 are all
> 0x1000_0000, this change also fix the AHB2PCIe window size smaller than
> HW MMIO window size issue by change the values of fls(size) from
> fls(0xfff_ffff) to fls(0x1000_0000).

Good, I'm glad this actually fixes a bug.  The warning was actually
useful!

Since that's the case, the *bug* is the important thing (not the
warning), and the subject line should be about the bug fix.  The fact
that it also happens to remove a warning is really just incidental.

You say "the AHB2PCIe window size smaller than HW MMIO window size
issue" as though it should be familiar to us.  But it's not :)

So the changelog needs to start by explaining what the AHB2PCIe window
size issue is, mention what user-visible problem that causes, then
explain how you're fixing it by using resource_size().

Then you can mention that this also incidentally removes a coccinelle
warning.

Bjorn

> Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> ---
>  drivers/pci/controller/pcie-mediatek.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> index 55e471c..01126b8 100644
> --- a/drivers/pci/controller/pcie-mediatek.c
> +++ b/drivers/pci/controller/pcie-mediatek.c
> @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  	struct resource *mem = &pcie->mem;
>  	const struct mtk_pcie_soc *soc = port->pcie->soc;
>  	u32 val;
> -	size_t size;
>  	int err;
>  
>  	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> @@ -706,8 +705,8 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
>  		mtk_pcie_enable_msi(port);
>  
>  	/* Set AHB to PCIe translation windows */
> -	size = mem->end - mem->start;
> -	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> +	val = lower_32_bits(mem->start)
> +	      | AHB2PCIE_SIZE(fls(resource_size(mem)));

Nit: I think it's more typical to put the "|" on the first line.

>  	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
>  
>  	val = upper_32_bits(mem->start);
> -- 
> 2.6.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/2] PCI: mediatek: Use resource_size function on resource object

[Honghui Zhang]

On Thu, 2019-01-31 at 15:36 -0600, Bjorn Helgaas wrote:
> On Thu, Jan 31, 2019 at 04:01:52PM +0800, honghui.zhang@mediatek.com wrote:
> > From: Honghui Zhang <honghui.zhang@mediatek.com>
> > 
> > scripts/coccinelle/api/resource_size.cocci complain about the
> > following warning:
> > 
> > pcie-mediatek.c:720:13-16: WARNING: Suspicious code. resource_size is maybe missing with mem
> > 
> > Use resource_size(mem) instead of mem->end - mem->start to eliminate the
> > complain. Since the MMIO window size for both MT2712 and MT7622 are all
> > 0x1000_0000, this change also fix the AHB2PCIe window size smaller than
> > HW MMIO window size issue by change the values of fls(size) from
> > fls(0xfff_ffff) to fls(0x1000_0000).
> 
> Good, I'm glad this actually fixes a bug.  The warning was actually
> useful!
> 
> Since that's the case, the *bug* is the important thing (not the
> warning), and the subject line should be about the bug fix.  The fact
> that it also happens to remove a warning is really just incidental.
> 
> You say "the AHB2PCIe window size smaller than HW MMIO window size
> issue" as though it should be familiar to us.  But it's not :)
> 

Oh, My bad.

The HW design assigned a bus address range(typically start from
0x2000_0000 to 0x2fff_ffff for both mt2712 and mt7622) for PCIe usage.
This means, when CPU or other HW access address in this range, PCIe RC
HW should response to this access. Normally it will translate those
access request to TLPs and send to EP side. Tt's like the total memory
resource which could be allocated by EP's BAR and RC's itself BAR.

Although those address range is available for allocated, but it should
be enabled by this PCIE_AHB_TRANS_BASE register, what size should be
enabled is determined by AHB2PCIE_SIZE bits in this register.

In previous code we did not enable the full size of HW assigned address
range, if the EP's BAR requested size is bigger than the size we enabled
and smaller than the HW available size. The access request from CPU
which address is bigger than the address we enabled but smaller that HW
available address, then RC will block those request and those request
will never get to EP side by TLPs.

Previous code never run into a system error in production because even
half of those range(128MB) is bigger enough for typical EP device's BAR
request(4MB).

But all those HW assigned bus range should be enabled. And it's Okay to
do that. RC will never forward a request to EP when this request is not
suitable for EP's BAR range.


> So the changelog needs to start by explaining what the AHB2PCIe window
> size issue is, mention what user-visible problem that causes, then
> explain how you're fixing it by using resource_size().
> 
> Then you can mention that this also incidentally removes a coccinelle
> warning.
> 

Okay, thanks for your advise.

> Bjorn
> 
> > Signed-off-by: Honghui Zhang <honghui.zhang@mediatek.com>
> > ---
> >  drivers/pci/controller/pcie-mediatek.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pcie-mediatek.c b/drivers/pci/controller/pcie-mediatek.c
> > index 55e471c..01126b8 100644
> > --- a/drivers/pci/controller/pcie-mediatek.c
> > +++ b/drivers/pci/controller/pcie-mediatek.c
> > @@ -654,7 +654,6 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  	struct resource *mem = &pcie->mem;
> >  	const struct mtk_pcie_soc *soc = port->pcie->soc;
> >  	u32 val;
> > -	size_t size;
> >  	int err;
> >  
> >  	/* MT7622 platforms need to enable LTSSM and ASPM from PCIe subsys */
> > @@ -706,8 +705,8 @@ static int mtk_pcie_startup_port_v2(struct mtk_pcie_port *port)
> >  		mtk_pcie_enable_msi(port);
> >  
> >  	/* Set AHB to PCIe translation windows */
> > -	size = mem->end - mem->start;
> > -	val = lower_32_bits(mem->start) | AHB2PCIE_SIZE(fls(size));
> > +	val = lower_32_bits(mem->start)
> > +	      | AHB2PCIE_SIZE(fls(resource_size(mem)));
> 
> Nit: I think it's more typical to put the "|" on the first line.

Okay, will update this in next version.

Thanks for your comments.

> 
> >  	writel(val, port->base + PCIE_AHB_TRANS_BASE0_L);
> >  
> >  	val = upper_32_bits(mem->start);
> > -- 
> > 2.6.4
> > 
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel