[v5,15/18] PCI: dwc: Make cpu_addr_fixup take struct dw_pcie as argument

There is no need to hard code the cpu to bus address fixup mask.
The PCIe controller has a global address on the AXI bus, however,
from the perspective of the PCIe controller, its base starts at 0x0,
so the local address is 0x0. To get the bus address, simply subtract
the global address from the cpu address. The global address is taken
from device tree.

Also for ARTPEC-7, hard coding the cpu to bus address fixup mask is
not possible, since it uses a High Address Bits Look Up Table, which
means that it can, at runtime, map the PCIe window to an arbitrary
address in the 32-bit address space.

This also fixes a bug for ARTPEC-6, where the cpu to bus address
fixup mask previously was off by one (GENMASK(27, 0), rather than
GENMASK(28, 0)). This is another reason to calculate the mask by
using values from device tree.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pci-dra7xx.c      |  2 +-
 drivers/pci/dwc/pcie-artpec6.c    | 18 ++++++++++++++----
 drivers/pci/dwc/pcie-designware.c |  2 +-
 drivers/pci/dwc/pcie-designware.h |  2 +-
 4 files changed, 17 insertions(+), 7 deletions(-)

On Mon, Nov 20, 2017 at 02:32:18PM +0100, Niklas Cassel wrote:
> There is no need to hard code the cpu to bus address fixup mask.

There is no need or hardcoding it is broken ? There is a difference
between those two.

> The PCIe controller has a global address on the AXI bus, however,
> from the perspective of the PCIe controller, its base starts at 0x0,
> so the local address is 0x0. To get the bus address, simply subtract
> the global address from the cpu address. The global address is taken
> from device tree.

I do not understand this paragraph, I would kindly ask you and Kishon to
explain please this patch and

commit a660083eb06c ("PCI: dwc: designware: Add new *ops* for CPU addr
fixup")

What the cpu_addr_fixup() hook is supposed to do ? And what does the
"addr_space" property represent ?

> Also for ARTPEC-7, hard coding the cpu to bus address fixup mask is
> not possible, since it uses a High Address Bits Look Up Table, which
> means that it can, at runtime, map the PCIe window to an arbitrary
> address in the 32-bit address space.

But you are not changing the ARTPEC-7 mechanism, are you ? I do not
understand this paragraph - I see no change for ARTPEC-7 in this patch.

> This also fixes a bug for ARTPEC-6, where the cpu to bus address
> fixup mask previously was off by one (GENMASK(27, 0), rather than
> GENMASK(28, 0)). This is another reason to calculate the mask by
> using values from device tree.

What this patch does (and how the cpu_addr_fixup() mechanism works)
is not clear to me, please explain, we can rewrite the commit log
with a clear explanation then.

Thanks,
Lorenzo

> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
> ---
>  drivers/pci/dwc/pci-dra7xx.c      |  2 +-
>  drivers/pci/dwc/pcie-artpec6.c    | 18 ++++++++++++++----
>  drivers/pci/dwc/pcie-designware.c |  2 +-
>  drivers/pci/dwc/pcie-designware.h |  2 +-
>  4 files changed, 17 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/dwc/pci-dra7xx.c b/drivers/pci/dwc/pci-dra7xx.c
> index 224ff8affdce..89d87844abb3 100644
> --- a/drivers/pci/dwc/pci-dra7xx.c
> +++ b/drivers/pci/dwc/pci-dra7xx.c
> @@ -110,7 +110,7 @@ static inline void dra7xx_pcie_writel(struct dra7xx_pcie *pcie, u32 offset,
>  	writel(value, pcie->base + offset);
>  }
>  
> -static u64 dra7xx_pcie_cpu_addr_fixup(u64 pci_addr)
> +static u64 dra7xx_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
>  {
>  	return pci_addr & DRA7XX_CPU_TO_BUS_ADDR;
>  }
> diff --git a/drivers/pci/dwc/pcie-artpec6.c b/drivers/pci/dwc/pcie-artpec6.c
> index e7de4e4649eb..318a2bd0d97e 100644
> --- a/drivers/pci/dwc/pcie-artpec6.c
> +++ b/drivers/pci/dwc/pcie-artpec6.c
> @@ -67,8 +67,6 @@ static const struct of_device_id artpec6_pcie_of_match[];
>  #define PHY_STATUS			0x118
>  #define  PHY_COSPLLLOCK			BIT(0)
>  
> -#define ARTPEC6_CPU_TO_BUS_ADDR		GENMASK(27, 0)
> -
>  static u32 artpec6_pcie_readl(struct artpec6_pcie *artpec6_pcie, u32 offset)
>  {
>  	u32 val;
> @@ -82,9 +80,21 @@ static void artpec6_pcie_writel(struct artpec6_pcie *artpec6_pcie, u32 offset, u
>  	regmap_write(artpec6_pcie->regmap, offset, val);
>  }
>  
> -static u64 artpec6_pcie_cpu_addr_fixup(u64 pci_addr)
> +static u64 artpec6_pcie_cpu_addr_fixup(struct dw_pcie *pci, u64 pci_addr)
>  {
> -	return pci_addr & ARTPEC6_CPU_TO_BUS_ADDR;
> +	struct artpec6_pcie *artpec6_pcie = to_artpec6_pcie(pci);
> +	struct pcie_port *pp = &pci->pp;
> +	struct dw_pcie_ep *ep = &pci->ep;
> +
> +	switch (artpec6_pcie->mode) {
> +	case DW_PCIE_RC_TYPE:
> +		return pci_addr - pp->cfg0_base;
> +	case DW_PCIE_EP_TYPE:
> +		return pci_addr - ep->phys_base;
> +	default:
> +		dev_err(pci->dev, "UNKNOWN device type\n");
> +	}
> +	return pci_addr;
>  }
>  
>  static int artpec6_pcie_establish_link(struct dw_pcie *pci)
> diff --git a/drivers/pci/dwc/pcie-designware.c b/drivers/pci/dwc/pcie-designware.c
> index 88abdddee2ad..800be7a4f087 100644
> --- a/drivers/pci/dwc/pcie-designware.c
> +++ b/drivers/pci/dwc/pcie-designware.c
> @@ -149,7 +149,7 @@ void dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
>  	u32 retries, val;
>  
>  	if (pci->ops->cpu_addr_fixup)
> -		cpu_addr = pci->ops->cpu_addr_fixup(cpu_addr);
> +		cpu_addr = pci->ops->cpu_addr_fixup(pci, cpu_addr);
>  
>  	if (pci->iatu_unroll_enabled) {
>  		dw_pcie_prog_outbound_atu_unroll(pci, index, type, cpu_addr,
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index 24edac035160..cca5a81c1c74 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -205,7 +205,7 @@ struct dw_pcie_ep {
>  };
>  
>  struct dw_pcie_ops {
> -	u64	(*cpu_addr_fixup)(u64 cpu_addr);
> +	u64	(*cpu_addr_fixup)(struct dw_pcie *pcie, u64 cpu_addr);
>  	u32	(*read_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
>  			    size_t size);
>  	void	(*write_dbi)(struct dw_pcie *pcie, void __iomem *base, u32 reg,
> -- 
> 2.14.2
>

On Mon, Dec 18, 2017 at 06:10:58PM +0000, Lorenzo Pieralisi wrote:
> On Mon, Nov 20, 2017 at 02:32:18PM +0100, Niklas Cassel wrote:
> > There is no need to hard code the cpu to bus address fixup mask.
> 
> There is no need or hardcoding it is broken ? There is a difference
> between those two.

Well, both :P

The current hardcoding for ARTPEC-6: GENMASK(27, 0), is wrong.
Hardcoding the correct mask GENMASK(28, 0) would be sufficient
for ARTPEC-6.

However, the ARTPEC-7 is a 32-bit CPU (without LPAE), but has
a 64-bit bus. So the ARTPEC-7 can have more devices than fits
in the 32-bit range, therefore a lookup table is placed between
the bus and the CPU, so you can choose what devices to map.
This mapping, which decides what devices to map, is currently
done at boot via devicetree.
So, on ARTPEC-7, it is not possible to hardcode a CPU_TO_BUS
mask, since the PCIe controller's address is only known via DT.

Having a hardcoded value is arguably wrong. It should probably
have been a DT property called something like "cpu-bus-fixup-mask".
However, this property is not needed since we already have
pp->cfg0_base and ep->phys_base, which is derived from already
existing DT properties.

By using pp->cfg0_base and ep->phys_base, we avoid hardcoding a mask
in the driver. This should work for ARTPEC-6, DRA7xx and ARTPEC-7.
I have not changed the code in DRA7xx though, since their existing
code works, but if they want, they could use the same logic as
artpec6_pcie_cpu_addr_fixup, and thus remove their hardcoded mask.

> 
> > The PCIe controller has a global address on the AXI bus, however,
> > from the perspective of the PCIe controller, its base starts at 0x0,
> > so the local address is 0x0. To get the bus address, simply subtract
> > the global address from the cpu address. The global address is taken
> > from device tree.
> 
> I do not understand this paragraph, I would kindly ask you and Kishon to
> explain please this patch and
> 
> commit a660083eb06c ("PCI: dwc: designware: Add new *ops* for CPU addr
> fixup")
> 
> What the cpu_addr_fixup() hook is supposed to do ? And what does the
> "addr_space" property represent ?

The ARTPEC-6 and DRA7xx SoCs both has an interconnect configured is a way
where the local address of the PCIe controller is 0x0.

For ARTPEC-6 the global address of the PCIe controller is 0xf8050000,
so if the CPU writes to address 0xf8050008, from the perspective of the
PCIe controller, it will see a write to address 0x8.

This is normally not a problem, but when reading/writing from an endpoint,
we go via the outbound iATU. The outbound iATU has to be programmed with
a base address (reads/writes in the range [base address + limit] will be
subject to translation).

However, since the address the PCIe controller sees, in reality is
"0xf8050000 + the address the PCIe controller sees", we need to subtract
the global address before programming the base address in the outbound
iATU.

Kishon explains the same thing in commit f4c55c5a3f7f68c0.

For this patch, I tried make the commit message understandable,
without going into too much detail, but there is probably still
room for improvement.

> 
> > Also for ARTPEC-7, hard coding the cpu to bus address fixup mask is
> > not possible, since it uses a High Address Bits Look Up Table, which
> > means that it can, at runtime, map the PCIe window to an arbitrary
> > address in the 32-bit address space.
> 
> But you are not changing the ARTPEC-7 mechanism, are you ? I do not
> understand this paragraph - I see no change for ARTPEC-7 in this patch.

Hopefully this is clarified by the comments above.

> 
> > This also fixes a bug for ARTPEC-6, where the cpu to bus address
> > fixup mask previously was off by one (GENMASK(27, 0), rather than
> > GENMASK(28, 0)). This is another reason to calculate the mask by
> > using values from device tree.
> 
> What this patch does (and how the cpu_addr_fixup() mechanism works)
> is not clear to me, please explain, we can rewrite the commit log
> with a clear explanation then.

Hopefully this is clarified by the comments above.


Regards,
Niklas

On Mon, Dec 18, 2017 at 10:15:43PM +0100, Niklas Cassel wrote:
> On Mon, Dec 18, 2017 at 06:10:58PM +0000, Lorenzo Pieralisi wrote:
> > On Mon, Nov 20, 2017 at 02:32:18PM +0100, Niklas Cassel wrote:
> > > There is no need to hard code the cpu to bus address fixup mask.
> > 
> > There is no need or hardcoding it is broken ? There is a difference
> > between those two.
> 
> Well, both :P
> 
> The current hardcoding for ARTPEC-6: GENMASK(27, 0), is wrong.
> Hardcoding the correct mask GENMASK(28, 0) would be sufficient
> for ARTPEC-6.
> 
> However, the ARTPEC-7 is a 32-bit CPU (without LPAE), but has
> a 64-bit bus. So the ARTPEC-7 can have more devices than fits
> in the 32-bit range, therefore a lookup table is placed between
> the bus and the CPU, so you can choose what devices to map.
> This mapping, which decides what devices to map, is currently
> done at boot via devicetree.
> So, on ARTPEC-7, it is not possible to hardcode a CPU_TO_BUS
> mask, since the PCIe controller's address is only known via DT.
> 
> Having a hardcoded value is arguably wrong. It should probably
> have been a DT property called something like "cpu-bus-fixup-mask".
> However, this property is not needed since we already have
> pp->cfg0_base and ep->phys_base, which is derived from already
> existing DT properties.
> 
> By using pp->cfg0_base and ep->phys_base, we avoid hardcoding a mask
> in the driver. This should work for ARTPEC-6, DRA7xx and ARTPEC-7.
> I have not changed the code in DRA7xx though, since their existing
> code works, but if they want, they could use the same logic as
> artpec6_pcie_cpu_addr_fixup, and thus remove their hardcoded mask.
> 
> > 
> > > The PCIe controller has a global address on the AXI bus, however,
> > > from the perspective of the PCIe controller, its base starts at 0x0,
> > > so the local address is 0x0. To get the bus address, simply subtract
> > > the global address from the cpu address. The global address is taken
> > > from device tree.
> > 
> > I do not understand this paragraph, I would kindly ask you and Kishon to
> > explain please this patch and
> > 
> > commit a660083eb06c ("PCI: dwc: designware: Add new *ops* for CPU addr
> > fixup")
> > 
> > What the cpu_addr_fixup() hook is supposed to do ? And what does the
> > "addr_space" property represent ?
> 
> The ARTPEC-6 and DRA7xx SoCs both has an interconnect configured is a way
> where the local address of the PCIe controller is 0x0.
> 
> For ARTPEC-6 the global address of the PCIe controller is 0xf8050000,
> so if the CPU writes to address 0xf8050008, from the perspective of the
> PCIe controller, it will see a write to address 0x8.
> 
> This is normally not a problem, but when reading/writing from an endpoint,
> we go via the outbound iATU. The outbound iATU has to be programmed with
> a base address (reads/writes in the range [base address + limit] will be
> subject to translation).
> 
> However, since the address the PCIe controller sees, in reality is
> "0xf8050000 + the address the PCIe controller sees", we need to subtract
> the global address before programming the base address in the outbound
> iATU.
> 
> Kishon explains the same thing in commit f4c55c5a3f7f68c0.
> 
> For this patch, I tried make the commit message understandable,
> without going into too much detail, but there is probably still
> room for improvement.
> 
> > 
> > > Also for ARTPEC-7, hard coding the cpu to bus address fixup mask is
> > > not possible, since it uses a High Address Bits Look Up Table, which
> > > means that it can, at runtime, map the PCIe window to an arbitrary
> > > address in the 32-bit address space.
> > 
> > But you are not changing the ARTPEC-7 mechanism, are you ? I do not
> > understand this paragraph - I see no change for ARTPEC-7 in this patch.
> 
> Hopefully this is clarified by the comments above.
> 
> > 
> > > This also fixes a bug for ARTPEC-6, where the cpu to bus address
> > > fixup mask previously was off by one (GENMASK(27, 0), rather than
> > > GENMASK(28, 0)). This is another reason to calculate the mask by
> > > using values from device tree.
> > 
> > What this patch does (and how the cpu_addr_fixup() mechanism works)
> > is not clear to me, please explain, we can rewrite the commit log
> > with a clear explanation then.
> 
> Hopefully this is clarified by the comments above.

It is, thank you - it would be good to convert this thread into commit
log format, you can send it as an update to this patch.

Thanks,
Lorenzo