Pass resource start as the input argument to iomap() instead of
atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
be the same here, it actually represents the parent bus address before ATU,
which may not always align with the CPU address. Using resource start
ensures correctness and clarity.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
change from v9 to v10
- new patch
---
 drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index ffaded8f2df7b..ae3fd2a5dbf85 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
 	if (ret)
 		return ret;
 
-	mem = ioremap(atu.cpu_addr, pci->region_align);
+	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
 	if (!mem)
 		return -ENOMEM;
 

-- 
2.34.1

On Tue, Jan 28, 2025 at 05:07:34PM -0500, Frank Li wrote:
> Pass resource start as the input argument to iomap() instead of
> atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
> be the same here, it actually represents the parent bus address before ATU,
> which may not always align with the CPU address. Using resource start
> ensures correctness and clarity.

1) "atu.cpu_address happens to be the same here" is currently true
   but only because this function is buggy and assumes the ATU input
   address is the same as a CPU physical address.

2) We're trying to make a correctness fix, not a clarity fix.  This
   patch by itself isn't a functional change because of the cpu_addr
   bug, but without this patch, fixing the cpu_addr bug would break
   the iomap.

3) "Pass resource start as the input to iomap()" just restates the
   patch.  We should say *why* this is important.  Something like
   this:

     The msg_res region translates writes into PCIe Message TLPs.
     Previously we mapped this region using atu.cpu_addr, the input
     address programmed into the ATU.

     "cpu_addr" is a misnomer because when a bus fabric translates
     addresses between the CPU and the ATU, the ATU input address is
     different from the CPU address.  A future patch will rename
     "cpu_addr" and correct the value to be the ATU input address
     instead of the CPU physical address.

     Map the msg_res region before writing to it using the msg_res
     resource start, a CPU physical address.

> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> change from v9 to v10
> - new patch
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ffaded8f2df7b..ae3fd2a5dbf85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  	if (ret)
>  		return ret;
>  
> -	mem = ioremap(atu.cpu_addr, pci->region_align);
> +	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
>  	if (!mem)
>  		return -ENOMEM;
>  
> 
> -- 
> 2.34.1
>

On Wed, Jan 29, 2025 at 05:19:37PM -0600, Bjorn Helgaas wrote:
> On Tue, Jan 28, 2025 at 05:07:34PM -0500, Frank Li wrote:
> > Pass resource start as the input argument to iomap() instead of
> > atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
> > be the same here, it actually represents the parent bus address before ATU,
> > which may not always align with the CPU address. Using resource start
> > ensures correctness and clarity.
>
> 1) "atu.cpu_address happens to be the same here" is currently true
>    but only because this function is buggy and assumes the ATU input
>    address is the same as a CPU physical address.
>
> 2) We're trying to make a correctness fix, not a clarity fix.  This
>    patch by itself isn't a functional change because of the cpu_addr
>    bug, but without this patch, fixing the cpu_addr bug would break
>    the iomap.
>
> 3) "Pass resource start as the input to iomap()" just restates the
>    patch.  We should say *why* this is important.  Something like
>    this:
>
>      The msg_res region translates writes into PCIe Message TLPs.
>      Previously we mapped this region using atu.cpu_addr, the input
>      address programmed into the ATU.
>
>      "cpu_addr" is a misnomer because when a bus fabric translates
>      addresses between the CPU and the ATU, the ATU input address is
>      different from the CPU address.  A future patch will rename
>      "cpu_addr" and correct the value to be the ATU input address
>      instead of the CPU physical address.
>
>      Map the msg_res region before writing to it using the msg_res
>      resource start, a CPU physical address.

Thanks, will update commit message. The key change is patch 3

PCI: Add parent_bus_offset to resource_entry

Frank

>
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> > change from v9 to v10
> > - new patch
> > ---
> >  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index ffaded8f2df7b..ae3fd2a5dbf85 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
> >  	if (ret)
> >  		return ret;
> >
> > -	mem = ioremap(atu.cpu_addr, pci->region_align);
> > +	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
> >  	if (!mem)
> >  		return -ENOMEM;
> >
> >
> > --
> > 2.34.1
> >