diff mbox series

[RESEND,v4,15/15] PCI: dwc: Introduce dma-ranges property support for RC-host

Message ID 20220624143947.8991-16-Sergey.Semin@baikalelectronics.ru (mailing list archive)
State Superseded
Delegated to: Lorenzo Pieralisi
Headers show
Series PCI: dwc: Add hw version and dma-ranges support | expand

Commit Message

Serge Semin June 24, 2022, 2:39 p.m. UTC
In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
property has the same format as the "ranges" property. The only difference
is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
property. Even though the DW PCIe controllers are normally equipped with
the internal Address Translation Unit which inbound and outbound tables
can be used to implement both properties semantics, it was surprising for
me to discover that the host-related part of the DW PCIe driver currently
supports the "ranges" property only while the "dma-ranges" windows are
just ignored. Having the "dma-ranges" supported in the driver would be
very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
mapping and require a customized PCIe memory layout. So let's fix that by
introducing the "dma-ranges" property support.

First of all we suggest to rename the dw_pcie_prog_inbound_atu() method to
dw_pcie_prog_ep_inbound_atu() and create a new version of the
dw_pcie_prog_inbound_atu() function. Thus we'll have two methods for the
RC and EP controllers respectively in the same way as it has been
developed for the outbound ATU setup methods.

Secondly aside with the memory window index and type the new
dw_pcie_prog_inbound_atu() function will accept CPU address, PCIe address
and size as its arguments. These parameters define the PCIe and CPU memory
ranges which will be used to setup the respective inbound ATU mapping. The
passed parameters need to be verified against the ATU ranges constraints
in the same way as it is done for the outbound ranges.

Finally the DMA-ranges detected for the PCIe controller need to be
converted to the inbound ATU entries during the host controller
initialization procedure. It will be done in the framework of the
dw_pcie_iatu_setup() method. Note before setting the inbound ranges up we
need to disable all the inbound ATU entries in order to prevent unexpected
PCIe TLPs translations defined by some third party software like
bootloaders.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Rob Herring <robh@kernel.org>

---

Changelog v3:
- Drop inbound iATU window size alignment constraint. (@Manivannan)
---
 .../pci/controller/dwc/pcie-designware-ep.c   |  4 +-
 .../pci/controller/dwc/pcie-designware-host.c | 32 ++++++++++-
 drivers/pci/controller/dwc/pcie-designware.c  | 56 ++++++++++++++++++-
 drivers/pci/controller/dwc/pcie-designware.h  |  6 +-
 4 files changed, 89 insertions(+), 9 deletions(-)

Comments

Bjorn Helgaas July 28, 2022, 10:11 p.m. UTC | #1
On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> property has the same format as the "ranges" property. The only difference
> is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> property. Even though the DW PCIe controllers are normally equipped with
> the internal Address Translation Unit which inbound and outbound tables
> can be used to implement both properties semantics, it was surprising for
> me to discover that the host-related part of the DW PCIe driver currently
> supports the "ranges" property only while the "dma-ranges" windows are
> just ignored. Having the "dma-ranges" supported in the driver would be
> very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> mapping and require a customized PCIe memory layout. So let's fix that by
> introducing the "dma-ranges" property support.

Do we have a platform that requires this yet?  Or does this fix a bug?

I see that dw_pcie_host_init() calls devm_pci_alloc_host_bridge(),
which eventually parses "dma-ranges", but I don't see any DWC DT
bindings that use it yet.

I'm not clear on what value this adds today.

Bjorn
Serge Semin July 29, 2022, 4:52 a.m. UTC | #2
On Thu, Jul 28, 2022 at 05:11:20PM -0500, Bjorn Helgaas wrote:
> On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> > In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> > property has the same format as the "ranges" property. The only difference
> > is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> > memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> > property. Even though the DW PCIe controllers are normally equipped with
> > the internal Address Translation Unit which inbound and outbound tables
> > can be used to implement both properties semantics, it was surprising for
> > me to discover that the host-related part of the DW PCIe driver currently
> > supports the "ranges" property only while the "dma-ranges" windows are
> > just ignored. Having the "dma-ranges" supported in the driver would be
> > very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> > mapping and require a customized PCIe memory layout. So let's fix that by
> > introducing the "dma-ranges" property support.
> 

> Do we have a platform that requires this yet?  Or does this fix a bug?
> 
> I see that dw_pcie_host_init() calls devm_pci_alloc_host_bridge(),
> which eventually parses "dma-ranges", but I don't see any DWC DT
> bindings that use it yet.
> 
> I'm not clear on what value this adds today.

There are several points of having this supported.
First of all, generic PCIe DT-bindings permit having the dma-ranges
specified for the PCIe RCs. If so having it unsupported by the driver
just breaks the bindings or at least makes it incomplete.
Second, the main point of this patchset is to add the dma-ranges
support.) Especially seeing some other PCIe RC drivers do have it
supported too.
Finally. It is required for our platform (and for all the platforms
with similar issues). The problem is that the outbound source window
base address (on CPU-side) is size-unaligned. It resides at the 128MB
base address (size is somewhat about ~335MB). In case of the
one-on-one CPU->PCI mapping the peripherals with relatively big BARs
(at least of 256MB) and which need the BARs having size-aligned memory
won't be supported. So we had to remap the PCIe space to the
size-aligned base address. But in its turn that caused the PCIe-CPU
memory overlap. So PCIe DMA stopped working for the overlapped memory
due to the implicit P2P transactions. In order to fix that we had to
add the dma-ranges support to the DW PCIe driver and use it to remap
the overlapped memory. So please add this patch to the repo. We really
need it.

-Sergey

> 
> Bjorn
Bjorn Helgaas July 29, 2022, 11:33 a.m. UTC | #3
On Fri, Jul 29, 2022 at 07:52:20AM +0300, Serge Semin wrote:
> On Thu, Jul 28, 2022 at 05:11:20PM -0500, Bjorn Helgaas wrote:
> > On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> > > In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> > > property has the same format as the "ranges" property. The only difference
> > > is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> > > memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> > > property. Even though the DW PCIe controllers are normally equipped with
> > > the internal Address Translation Unit which inbound and outbound tables
> > > can be used to implement both properties semantics, it was surprising for
> > > me to discover that the host-related part of the DW PCIe driver currently
> > > supports the "ranges" property only while the "dma-ranges" windows are
> > > just ignored. Having the "dma-ranges" supported in the driver would be
> > > very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> > > mapping and require a customized PCIe memory layout. So let's fix that by
> > > introducing the "dma-ranges" property support.
> 
> > Do we have a platform that requires this yet?  Or does this fix a bug?
> > 
> > I see that dw_pcie_host_init() calls devm_pci_alloc_host_bridge(),
> > which eventually parses "dma-ranges", but I don't see any DWC DT
> > bindings that use it yet.
> > 
> > I'm not clear on what value this adds today.
> 
> There are several points of having this supported.
> First of all, generic PCIe DT-bindings permit having the dma-ranges
> specified for the PCIe RCs. If so having it unsupported by the driver
> just breaks the bindings or at least makes it incomplete.

Are there bindings in the tree that are broken and will be fixed by
this?

> Second, the main point of this patchset is to add the dma-ranges
> support.) Especially seeing some other PCIe RC drivers do have it
> supported too.
> Finally. It is required for our platform (and for all the platforms
> with similar issues). The problem is that the outbound source window
> base address (on CPU-side) is size-unaligned. It resides at the 128MB
> base address (size is somewhat about ~335MB). In case of the
> one-on-one CPU->PCI mapping the peripherals with relatively big BARs
> (at least of 256MB) and which need the BARs having size-aligned memory
> won't be supported. So we had to remap the PCIe space to the
> size-aligned base address. But in its turn that caused the PCIe-CPU
> memory overlap. So PCIe DMA stopped working for the overlapped memory
> due to the implicit P2P transactions. In order to fix that we had to
> add the dma-ranges support to the DW PCIe driver and use it to remap
> the overlapped memory. So please add this patch to the repo. We really
> need it.

Does the above apply to the pending Baikal-T1 driver?  If so, let's
just include this patch in that series.  Then we'll have a user of
this functionality and we'll be able to exercise and test this code.

Bjorn
Serge Semin July 29, 2022, 2:38 p.m. UTC | #4
On Fri, Jul 29, 2022 at 06:33:45AM -0500, Bjorn Helgaas wrote:
> On Fri, Jul 29, 2022 at 07:52:20AM +0300, Serge Semin wrote:
> > On Thu, Jul 28, 2022 at 05:11:20PM -0500, Bjorn Helgaas wrote:
> > > On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> > > > In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> > > > property has the same format as the "ranges" property. The only difference
> > > > is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> > > > memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> > > > property. Even though the DW PCIe controllers are normally equipped with
> > > > the internal Address Translation Unit which inbound and outbound tables
> > > > can be used to implement both properties semantics, it was surprising for
> > > > me to discover that the host-related part of the DW PCIe driver currently
> > > > supports the "ranges" property only while the "dma-ranges" windows are
> > > > just ignored. Having the "dma-ranges" supported in the driver would be
> > > > very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> > > > mapping and require a customized PCIe memory layout. So let's fix that by
> > > > introducing the "dma-ranges" property support.
> > 
> > > Do we have a platform that requires this yet?  Or does this fix a bug?
> > > 
> > > I see that dw_pcie_host_init() calls devm_pci_alloc_host_bridge(),
> > > which eventually parses "dma-ranges", but I don't see any DWC DT
> > > bindings that use it yet.
> > > 
> > > I'm not clear on what value this adds today.
> > 
> > There are several points of having this supported.
> > First of all, generic PCIe DT-bindings permit having the dma-ranges
> > specified for the PCIe RCs. If so having it unsupported by the driver
> > just breaks the bindings or at least makes it incomplete.
> 

> Are there bindings in the tree that are broken and will be fixed by
> this?

Shortly speaking: explicitly none of them are broken, but implicitly most
of them are broken.)

As I said the generic DT-bindings permit having the dma-ranges
property specified for the PCIe RCs:
Link: https://github.com/robherring/dt-schema/blob/master/schemas/pci/pci-bus.yaml#L47

So all the PCIe RC bindings can be equipped with the dma-ranges
property. At least dtschema check tool won't print any error if the
property is specified for any of the currently available PCIe RC
device including DW PCIe RC. But if the dma-ranges aren't supported by
the driver having it specified won't give any expected effect. In case
of the DW PCIe RC device setting the dma-ranges property up won't
change anything because without this patch the dma-ranges won't be
accordingly translated to the inbound iATU settings.

> 
> > Second, the main point of this patchset is to add the dma-ranges
> > support.) Especially seeing some other PCIe RC drivers do have it
> > supported too.
> > Finally. It is required for our platform (and for all the platforms
> > with similar issues). The problem is that the outbound source window
> > base address (on CPU-side) is size-unaligned. It resides at the 128MB
> > base address (size is somewhat about ~335MB). In case of the
> > one-on-one CPU->PCI mapping the peripherals with relatively big BARs
> > (at least of 256MB) and which need the BARs having size-aligned memory
> > won't be supported. So we had to remap the PCIe space to the
> > size-aligned base address. But in its turn that caused the PCIe-CPU
> > memory overlap. So PCIe DMA stopped working for the overlapped memory
> > due to the implicit P2P transactions. In order to fix that we had to
> > add the dma-ranges support to the DW PCIe driver and use it to remap
> > the overlapped memory. So please add this patch to the repo. We really
> > need it.
> 

> Does the above apply to the pending Baikal-T1 driver? 

The problem described above is indeed specific to the Baikal-T1
platform.

> If so, let's
> just include this patch in that series.  Then we'll have a user of
> this functionality and we'll be able to exercise and test this code.

Basically it can be tested out on any platform, not only on ours. You
don't even need to have the problem described above. All you need is a
DW PCIe RC-device with the inbound iATU windows support, this patch
and a dts file with the dma-ranges specified for the corresponding DT
device node.

Secondly even if you do move this patch to that series it likely won't
be tested by anyone but me, which I and our customer have already done
many times here. By postponing the patch merge in you'll just postpone
the feature being utilized by the rest of the kernel users and nothing
else. I am sure it's working at least in our case. Let's add it to the
kernel on this cycle so in case of unexpected problems they could be
fixed in the next rc-stages.

-Sergey

> 
> Bjorn
Manivannan Sadhasivam Aug. 1, 2022, 2 p.m. UTC | #5
On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> property has the same format as the "ranges" property. The only difference
> is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> property. Even though the DW PCIe controllers are normally equipped with
> the internal Address Translation Unit which inbound and outbound tables
> can be used to implement both properties semantics, it was surprising for
> me to discover that the host-related part of the DW PCIe driver currently
> supports the "ranges" property only while the "dma-ranges" windows are
> just ignored. Having the "dma-ranges" supported in the driver would be
> very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> mapping and require a customized PCIe memory layout. So let's fix that by
> introducing the "dma-ranges" property support.
> 
> First of all we suggest to rename the dw_pcie_prog_inbound_atu() method to
> dw_pcie_prog_ep_inbound_atu() and create a new version of the
> dw_pcie_prog_inbound_atu() function. Thus we'll have two methods for the
> RC and EP controllers respectively in the same way as it has been
> developed for the outbound ATU setup methods.
> 
> Secondly aside with the memory window index and type the new
> dw_pcie_prog_inbound_atu() function will accept CPU address, PCIe address
> and size as its arguments. These parameters define the PCIe and CPU memory
> ranges which will be used to setup the respective inbound ATU mapping. The
> passed parameters need to be verified against the ATU ranges constraints
> in the same way as it is done for the outbound ranges.
> 
> Finally the DMA-ranges detected for the PCIe controller need to be
> converted to the inbound ATU entries during the host controller
> initialization procedure. It will be done in the framework of the
> dw_pcie_iatu_setup() method. Note before setting the inbound ranges up we
> need to disable all the inbound ATU entries in order to prevent unexpected
> PCIe TLPs translations defined by some third party software like
> bootloaders.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

Small nitpick below,

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> Reviewed-by: Rob Herring <robh@kernel.org>
> 
> ---
> 
> Changelog v3:
> - Drop inbound iATU window size alignment constraint. (@Manivannan)
> ---
>  .../pci/controller/dwc/pcie-designware-ep.c   |  4 +-
>  .../pci/controller/dwc/pcie-designware-host.c | 32 ++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.c  | 56 ++++++++++++++++++-
>  drivers/pci/controller/dwc/pcie-designware.h  |  6 +-
>  4 files changed, 89 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> index 627c4b69878c..441feff1917a 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> @@ -167,8 +167,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
>  		return -EINVAL;
>  	}
>  
> -	ret = dw_pcie_prog_inbound_atu(pci, func_no, free_win, type,
> -				       cpu_addr, bar);
> +	ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
> +					  cpu_addr, bar);
>  	if (ret < 0) {
>  		dev_err(pci->dev, "Failed to program IB window\n");
>  		return ret;
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index 6993ce9e856d..2fbe9dc11634 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -581,12 +581,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  	}
>  
>  	/*
> -	 * Ensure all outbound windows are disabled before proceeding with
> -	 * the MEM/IO ranges setups.
> +	 * Ensure all out/inbound windows are disabled before proceeding with
> +	 * the MEM/IO (dma-)ranges setups.
>  	 */
>  	for (i = 0; i < pci->num_ob_windows; i++)
>  		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
>  
> +	for (i = 0; i < pci->num_ib_windows; i++)
> +		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, i);
> +
>  	i = 0;
>  	resource_list_for_each_entry(entry, &pp->bridge->windows) {
>  		if (resource_type(entry->res) != IORESOURCE_MEM)
> @@ -623,9 +626,32 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
>  	}
>  
>  	if (pci->num_ob_windows <= i)
> -		dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
> +		dev_warn(pci->dev, "Ranges exceed outbound iATU size (%d)\n",
>  			 pci->num_ob_windows);
>  

I think a comment here explaining what's going on below would be helpful in the
future.

> +	i = 0;
> +	resource_list_for_each_entry(entry, &pp->bridge->dma_ranges) {
> +		if (resource_type(entry->res) != IORESOURCE_MEM)
> +			continue;
> +
> +		if (pci->num_ib_windows <= i)
> +			break;
> +
> +		ret = dw_pcie_prog_inbound_atu(pci, i++, PCIE_ATU_TYPE_MEM,
> +					       entry->res->start,
> +					       entry->res->start - entry->offset,
> +					       resource_size(entry->res));
> +		if (ret) {
> +			dev_err(pci->dev, "Failed to set DMA range %pr\n",
> +				entry->res);
> +			return ret;
> +		}
> +	}
> +
> +	if (pci->num_ib_windows <= i)
> +		dev_warn(pci->dev, "Dma-ranges exceed inbound iATU size (%u)\n",

"dma-ranges"

> +			 pci->num_ib_windows);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> index 9c622b635fdd..7a5be3c4f8e0 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.c
> +++ b/drivers/pci/controller/dwc/pcie-designware.c
> @@ -396,8 +396,60 @@ static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg
>  	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg, val);
>  }
>  
> -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -			     int type, u64 cpu_addr, u8 bar)
> +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> +			     u64 cpu_addr, u64 pci_addr, u64 size)
> +{
> +	u64 limit_addr = pci_addr + size - 1;
> +	u32 retries, val;
> +
> +	if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) ||
> +	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> +	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> +		return -EINVAL;
> +	}
> +
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_BASE,
> +			      lower_32_bits(pci_addr));
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_BASE,
> +			      upper_32_bits(pci_addr));
> +
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LIMIT,
> +			      lower_32_bits(limit_addr));
> +	if (dw_pcie_ver_is_ge(pci, 460A))
> +		dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_LIMIT,
> +				      upper_32_bits(limit_addr));
> +
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
> +			      lower_32_bits(cpu_addr));
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
> +			      upper_32_bits(cpu_addr));
> +
> +	val = type;
> +	if (upper_32_bits(limit_addr) > upper_32_bits(pci_addr) &&
> +	    dw_pcie_ver_is_ge(pci, 460A))
> +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, val);
> +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> +
> +	/*
> +	 * Make sure ATU enable takes effect before any subsequent config
> +	 * and I/O accesses.
> +	 */
> +	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> +		val = dw_pcie_readl_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2);
> +		if (val & PCIE_ATU_ENABLE)
> +			return 0;
> +
> +		mdelay(LINK_WAIT_IATU);
> +	}
> +
> +	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
> +
> +	return -ETIMEDOUT;
> +}
> +
> +int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> +				int type, u64 cpu_addr, u8 bar)
>  {
>  	u32 retries, val;
>  
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index c3e73ed9aff5..5954e8cf9eec 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -308,8 +308,10 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
>  			      u64 cpu_addr, u64 pci_addr, u64 size);
>  int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
>  				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> -			     int type, u64 cpu_addr, u8 bar);
> +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> +			     u64 cpu_addr, u64 pci_addr, u64 size);
> +int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> +				int type, u64 cpu_addr, u8 bar);
>  void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
>  void dw_pcie_setup(struct dw_pcie *pci);
>  void dw_pcie_iatu_detect(struct dw_pcie *pci);
> -- 
> 2.35.1
>
Serge Semin Aug. 9, 2022, 8:07 p.m. UTC | #6
On Mon, Aug 01, 2022 at 07:30:47PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Jun 24, 2022 at 05:39:47PM +0300, Serge Semin wrote:
> > In accordance with the generic PCIe Root Port DT-bindings the "dma-ranges"
> > property has the same format as the "ranges" property. The only difference
> > is in their semantics. The "dma-ranges" property describes the PCIe-to-CPU
> > memory mapping in opposite to the CPU-to-PCIe mapping of the "ranges"
> > property. Even though the DW PCIe controllers are normally equipped with
> > the internal Address Translation Unit which inbound and outbound tables
> > can be used to implement both properties semantics, it was surprising for
> > me to discover that the host-related part of the DW PCIe driver currently
> > supports the "ranges" property only while the "dma-ranges" windows are
> > just ignored. Having the "dma-ranges" supported in the driver would be
> > very handy for the platforms, that don't tolerate the 1:1 CPU-PCIe memory
> > mapping and require a customized PCIe memory layout. So let's fix that by
> > introducing the "dma-ranges" property support.
> > 
> > First of all we suggest to rename the dw_pcie_prog_inbound_atu() method to
> > dw_pcie_prog_ep_inbound_atu() and create a new version of the
> > dw_pcie_prog_inbound_atu() function. Thus we'll have two methods for the
> > RC and EP controllers respectively in the same way as it has been
> > developed for the outbound ATU setup methods.
> > 
> > Secondly aside with the memory window index and type the new
> > dw_pcie_prog_inbound_atu() function will accept CPU address, PCIe address
> > and size as its arguments. These parameters define the PCIe and CPU memory
> > ranges which will be used to setup the respective inbound ATU mapping. The
> > passed parameters need to be verified against the ATU ranges constraints
> > in the same way as it is done for the outbound ranges.
> > 
> > Finally the DMA-ranges detected for the PCIe controller need to be
> > converted to the inbound ATU entries during the host controller
> > initialization procedure. It will be done in the framework of the
> > dw_pcie_iatu_setup() method. Note before setting the inbound ranges up we
> > need to disable all the inbound ATU entries in order to prevent unexpected
> > PCIe TLPs translations defined by some third party software like
> > bootloaders.
> > 
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> Small nitpick below,
> 
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Thanks,
> Mani
> 
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > 
> > ---
> > 
> > Changelog v3:
> > - Drop inbound iATU window size alignment constraint. (@Manivannan)
> > ---
> >  .../pci/controller/dwc/pcie-designware-ep.c   |  4 +-
> >  .../pci/controller/dwc/pcie-designware-host.c | 32 ++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.c  | 56 ++++++++++++++++++-
> >  drivers/pci/controller/dwc/pcie-designware.h  |  6 +-
> >  4 files changed, 89 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > index 627c4b69878c..441feff1917a 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-ep.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
> > @@ -167,8 +167,8 @@ static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
> >  		return -EINVAL;
> >  	}
> >  
> > -	ret = dw_pcie_prog_inbound_atu(pci, func_no, free_win, type,
> > -				       cpu_addr, bar);
> > +	ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
> > +					  cpu_addr, bar);
> >  	if (ret < 0) {
> >  		dev_err(pci->dev, "Failed to program IB window\n");
> >  		return ret;
> > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> > index 6993ce9e856d..2fbe9dc11634 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> > @@ -581,12 +581,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  	}
> >  
> >  	/*
> > -	 * Ensure all outbound windows are disabled before proceeding with
> > -	 * the MEM/IO ranges setups.
> > +	 * Ensure all out/inbound windows are disabled before proceeding with
> > +	 * the MEM/IO (dma-)ranges setups.
> >  	 */
> >  	for (i = 0; i < pci->num_ob_windows; i++)
> >  		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
> >  
> > +	for (i = 0; i < pci->num_ib_windows; i++)
> > +		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, i);
> > +
> >  	i = 0;
> >  	resource_list_for_each_entry(entry, &pp->bridge->windows) {
> >  		if (resource_type(entry->res) != IORESOURCE_MEM)
> > @@ -623,9 +626,32 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  	}
> >  
> >  	if (pci->num_ob_windows <= i)
> > -		dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
> > +		dev_warn(pci->dev, "Ranges exceed outbound iATU size (%d)\n",
> >  			 pci->num_ob_windows);
> >  
> 

> I think a comment here explaining what's going on below would be helpful in the
> future.

Both ranges and dma-ranges are initialized in the same way. Moreover
the method is pretty much coherent with self-explained name. So adding
an additional comment here doesn't seem much required.

> 
> > +	i = 0;
> > +	resource_list_for_each_entry(entry, &pp->bridge->dma_ranges) {
> > +		if (resource_type(entry->res) != IORESOURCE_MEM)
> > +			continue;
> > +
> > +		if (pci->num_ib_windows <= i)
> > +			break;
> > +
> > +		ret = dw_pcie_prog_inbound_atu(pci, i++, PCIE_ATU_TYPE_MEM,
> > +					       entry->res->start,
> > +					       entry->res->start - entry->offset,
> > +					       resource_size(entry->res));
> > +		if (ret) {
> > +			dev_err(pci->dev, "Failed to set DMA range %pr\n",
> > +				entry->res);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	if (pci->num_ib_windows <= i)
> > +		dev_warn(pci->dev, "Dma-ranges exceed inbound iATU size (%u)\n",
> 

> "dma-ranges"

I was referring to the similar message printed in case of the number
of specified ranges goes out of bounds.

-Sergey

> 
> > +			 pci->num_ib_windows);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
> > index 9c622b635fdd..7a5be3c4f8e0 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.c
> > +++ b/drivers/pci/controller/dwc/pcie-designware.c
> > @@ -396,8 +396,60 @@ static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg
> >  	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg, val);
> >  }
> >  
> > -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -			     int type, u64 cpu_addr, u8 bar)
> > +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > +			     u64 cpu_addr, u64 pci_addr, u64 size)
> > +{
> > +	u64 limit_addr = pci_addr + size - 1;
> > +	u32 retries, val;
> > +
> > +	if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) ||
> > +	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
> > +	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
> > +		return -EINVAL;
> > +	}
> > +
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_BASE,
> > +			      lower_32_bits(pci_addr));
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_BASE,
> > +			      upper_32_bits(pci_addr));
> > +
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LIMIT,
> > +			      lower_32_bits(limit_addr));
> > +	if (dw_pcie_ver_is_ge(pci, 460A))
> > +		dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_LIMIT,
> > +				      upper_32_bits(limit_addr));
> > +
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
> > +			      lower_32_bits(cpu_addr));
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
> > +			      upper_32_bits(cpu_addr));
> > +
> > +	val = type;
> > +	if (upper_32_bits(limit_addr) > upper_32_bits(pci_addr) &&
> > +	    dw_pcie_ver_is_ge(pci, 460A))
> > +		val |= PCIE_ATU_INCREASE_REGION_SIZE;
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, val);
> > +	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
> > +
> > +	/*
> > +	 * Make sure ATU enable takes effect before any subsequent config
> > +	 * and I/O accesses.
> > +	 */
> > +	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
> > +		val = dw_pcie_readl_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2);
> > +		if (val & PCIE_ATU_ENABLE)
> > +			return 0;
> > +
> > +		mdelay(LINK_WAIT_IATU);
> > +	}
> > +
> > +	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
> > +
> > +	return -ETIMEDOUT;
> > +}
> > +
> > +int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > +				int type, u64 cpu_addr, u8 bar)
> >  {
> >  	u32 retries, val;
> >  
> > diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> > index c3e73ed9aff5..5954e8cf9eec 100644
> > --- a/drivers/pci/controller/dwc/pcie-designware.h
> > +++ b/drivers/pci/controller/dwc/pcie-designware.h
> > @@ -308,8 +308,10 @@ int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
> >  			      u64 cpu_addr, u64 pci_addr, u64 size);
> >  int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> >  				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
> > -int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > -			     int type, u64 cpu_addr, u8 bar);
> > +int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
> > +			     u64 cpu_addr, u64 pci_addr, u64 size);
> > +int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
> > +				int type, u64 cpu_addr, u8 bar);
> >  void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
> >  void dw_pcie_setup(struct dw_pcie *pci);
> >  void dw_pcie_iatu_detect(struct dw_pcie *pci);
> > -- 
> > 2.35.1
> > 
> 
> -- 
> மணிவண்ணன் சதாசிவம்
diff mbox series

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-ep.c b/drivers/pci/controller/dwc/pcie-designware-ep.c
index 627c4b69878c..441feff1917a 100644
--- a/drivers/pci/controller/dwc/pcie-designware-ep.c
+++ b/drivers/pci/controller/dwc/pcie-designware-ep.c
@@ -167,8 +167,8 @@  static int dw_pcie_ep_inbound_atu(struct dw_pcie_ep *ep, u8 func_no, int type,
 		return -EINVAL;
 	}
 
-	ret = dw_pcie_prog_inbound_atu(pci, func_no, free_win, type,
-				       cpu_addr, bar);
+	ret = dw_pcie_prog_ep_inbound_atu(pci, func_no, free_win, type,
+					  cpu_addr, bar);
 	if (ret < 0) {
 		dev_err(pci->dev, "Failed to program IB window\n");
 		return ret;
diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 6993ce9e856d..2fbe9dc11634 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -581,12 +581,15 @@  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	}
 
 	/*
-	 * Ensure all outbound windows are disabled before proceeding with
-	 * the MEM/IO ranges setups.
+	 * Ensure all out/inbound windows are disabled before proceeding with
+	 * the MEM/IO (dma-)ranges setups.
 	 */
 	for (i = 0; i < pci->num_ob_windows; i++)
 		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_OB, i);
 
+	for (i = 0; i < pci->num_ib_windows; i++)
+		dw_pcie_disable_atu(pci, PCIE_ATU_REGION_DIR_IB, i);
+
 	i = 0;
 	resource_list_for_each_entry(entry, &pp->bridge->windows) {
 		if (resource_type(entry->res) != IORESOURCE_MEM)
@@ -623,9 +626,32 @@  static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
 	}
 
 	if (pci->num_ob_windows <= i)
-		dev_warn(pci->dev, "Resources exceed number of ATU entries (%d)\n",
+		dev_warn(pci->dev, "Ranges exceed outbound iATU size (%d)\n",
 			 pci->num_ob_windows);
 
+	i = 0;
+	resource_list_for_each_entry(entry, &pp->bridge->dma_ranges) {
+		if (resource_type(entry->res) != IORESOURCE_MEM)
+			continue;
+
+		if (pci->num_ib_windows <= i)
+			break;
+
+		ret = dw_pcie_prog_inbound_atu(pci, i++, PCIE_ATU_TYPE_MEM,
+					       entry->res->start,
+					       entry->res->start - entry->offset,
+					       resource_size(entry->res));
+		if (ret) {
+			dev_err(pci->dev, "Failed to set DMA range %pr\n",
+				entry->res);
+			return ret;
+		}
+	}
+
+	if (pci->num_ib_windows <= i)
+		dev_warn(pci->dev, "Dma-ranges exceed inbound iATU size (%u)\n",
+			 pci->num_ib_windows);
+
 	return 0;
 }
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
index 9c622b635fdd..7a5be3c4f8e0 100644
--- a/drivers/pci/controller/dwc/pcie-designware.c
+++ b/drivers/pci/controller/dwc/pcie-designware.c
@@ -396,8 +396,60 @@  static inline void dw_pcie_writel_atu_ib(struct dw_pcie *pci, u32 index, u32 reg
 	dw_pcie_writel_atu(pci, PCIE_ATU_REGION_DIR_IB, index, reg, val);
 }
 
-int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-			     int type, u64 cpu_addr, u8 bar)
+int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
+			     u64 cpu_addr, u64 pci_addr, u64 size)
+{
+	u64 limit_addr = pci_addr + size - 1;
+	u32 retries, val;
+
+	if ((limit_addr & ~pci->region_limit) != (pci_addr & ~pci->region_limit) ||
+	    !IS_ALIGNED(cpu_addr, pci->region_align) ||
+	    !IS_ALIGNED(pci_addr, pci->region_align) || !size) {
+		return -EINVAL;
+	}
+
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_BASE,
+			      lower_32_bits(pci_addr));
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_BASE,
+			      upper_32_bits(pci_addr));
+
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LIMIT,
+			      lower_32_bits(limit_addr));
+	if (dw_pcie_ver_is_ge(pci, 460A))
+		dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_LIMIT,
+				      upper_32_bits(limit_addr));
+
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_LOWER_TARGET,
+			      lower_32_bits(cpu_addr));
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_UPPER_TARGET,
+			      upper_32_bits(cpu_addr));
+
+	val = type;
+	if (upper_32_bits(limit_addr) > upper_32_bits(pci_addr) &&
+	    dw_pcie_ver_is_ge(pci, 460A))
+		val |= PCIE_ATU_INCREASE_REGION_SIZE;
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL1, val);
+	dw_pcie_writel_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2, PCIE_ATU_ENABLE);
+
+	/*
+	 * Make sure ATU enable takes effect before any subsequent config
+	 * and I/O accesses.
+	 */
+	for (retries = 0; retries < LINK_WAIT_MAX_IATU_RETRIES; retries++) {
+		val = dw_pcie_readl_atu_ib(pci, index, PCIE_ATU_REGION_CTRL2);
+		if (val & PCIE_ATU_ENABLE)
+			return 0;
+
+		mdelay(LINK_WAIT_IATU);
+	}
+
+	dev_err(pci->dev, "Inbound iATU is not being enabled\n");
+
+	return -ETIMEDOUT;
+}
+
+int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
+				int type, u64 cpu_addr, u8 bar)
 {
 	u32 retries, val;
 
diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
index c3e73ed9aff5..5954e8cf9eec 100644
--- a/drivers/pci/controller/dwc/pcie-designware.h
+++ b/drivers/pci/controller/dwc/pcie-designware.h
@@ -308,8 +308,10 @@  int dw_pcie_prog_outbound_atu(struct dw_pcie *pci, int index, int type,
 			      u64 cpu_addr, u64 pci_addr, u64 size);
 int dw_pcie_prog_ep_outbound_atu(struct dw_pcie *pci, u8 func_no, int index,
 				 int type, u64 cpu_addr, u64 pci_addr, u64 size);
-int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
-			     int type, u64 cpu_addr, u8 bar);
+int dw_pcie_prog_inbound_atu(struct dw_pcie *pci, int index, int type,
+			     u64 cpu_addr, u64 pci_addr, u64 size);
+int dw_pcie_prog_ep_inbound_atu(struct dw_pcie *pci, u8 func_no, int index,
+				int type, u64 cpu_addr, u8 bar);
 void dw_pcie_disable_atu(struct dw_pcie *pci, u32 dir, int index);
 void dw_pcie_setup(struct dw_pcie *pci);
 void dw_pcie_iatu_detect(struct dw_pcie *pci);