[v4,7/8] PCI: dwc: Introduce configurable DMA mask

Commit Message

Elad Nachman March 13, 2023, 12:40 p.m. UTC

From: Elad Nachman <[e-mail]>

Some devices, such as AC5 and AC5X have their physical DDR memory
start at address 0x2_0000_0000. In order to have the DMA coherent
allocation succeed later, a different DMA mask is required, as
defined in the DT file for such SOCs, using dma-ranges.

If not defined, fallback to 32-bit as previously done in the code.

Signed-off-by: Elad Nachman <[e-mail]>
---
v4:
   1) Fix commit message formatting.

   2) Fix removal / addition of blank lines.

 .../pci/controller/dwc/pcie-designware-host.c | 28 +++++++++++++++++--
 1 file changed, 25 insertions(+), 3 deletions(-)

Comments

Rob Herring March 17, 2023, 6:23 p.m. UTC | #1

+Robin

On Mon, Mar 13, 2023 at 02:40:15PM +0200, Elad Nachman wrote:
> From: Elad Nachman <[e-mail]>
> 
> Some devices, such as AC5 and AC5X have their physical DDR memory
> start at address 0x2_0000_0000. In order to have the DMA coherent
> allocation succeed later, a different DMA mask is required, as
> defined in the DT file for such SOCs, using dma-ranges.

I'm afraid this is not right. 'dma-ranges' in the PCI host bridge node 
applies to PCI devices (i.e. child node), not the host bridge itself. 
It's 'dma-ranges' in the parent node of the host bridge that applies 
here. The core code will set masks (ranges really now) based on bus 
restrictions. The mask for the device should only be based on the 
device's limits (i.e. the device is 32-bit only). 

I think you will need whatever solution comes out of this thread[1].

Rob

[1] https://lore.kernel.org/all/[e-mail]/

Robin Murphy March 27, 2023, 5:01 p.m. UTC | #2

On 2023-03-17 18:23, Rob Herring wrote:
> +Robin
> 
> On Mon, Mar 13, 2023 at 02:40:15PM +0200, Elad Nachman wrote:
>> From: Elad Nachman <[e-mail]>
>>
>> Some devices, such as AC5 and AC5X have their physical DDR memory
>> start at address 0x2_0000_0000. In order to have the DMA coherent
>> allocation succeed later, a different DMA mask is required, as
>> defined in the DT file for such SOCs, using dma-ranges.
> 
> I'm afraid this is not right. 'dma-ranges' in the PCI host bridge node
> applies to PCI devices (i.e. child node), not the host bridge itself.
> It's 'dma-ranges' in the parent node of the host bridge that applies
> here. The core code will set masks (ranges really now) based on bus
> restrictions. The mask for the device should only be based on the
> device's limits (i.e. the device is 32-bit only).
> 
> I think you will need whatever solution comes out of this thread[1].

Right, "make the allocation succeed later" is entirely missing the point 
of this code. The only reason we're doing that allocation at all is to 
reserve a 32-bit bus address. If it fails, it means we can't reliably 
support endpoints with only a 32-bit MSI capability. Using a bigger mask 
in order to successfully reserve a >32-bit bus address basically 
*guarantees* that you can't support endpoints with only a 32-bit MSI 
capability.

Thanks Rob for digging up that thread; the original idea there should 
still be fine, but the alternative suggestion from Serge at the end is 
potentially even better for this situation where it's down to the SoC's 
memory map rather than the kernel config. It just needs somebody with 
sufficient motivation and resources to write and test a patch :)

Robin.

> 
> Rob
> 
> [1] https://lore.kernel.org/all/[e-mail]/

Patch

diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
index 9952057c8819..74393e59e7a7 100644
--- a/drivers/pci/controller/dwc/pcie-designware-host.c
+++ b/drivers/pci/controller/dwc/pcie-designware-host.c
@@ -325,10 +325,14 @@  static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 {
 	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
 	struct device *dev = pci->dev;
+	struct device_node *np = dev->of_node;
 	struct platform_device *pdev = to_platform_device(dev);
 	u64 *msi_vaddr;
 	int ret;
 	u32 ctrl, num_ctrls;
+	u32 num_dma_maskbits = 32;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
 
 	for (ctrl = 0; ctrl < MAX_MSI_CTRLS; ctrl++)
 		pp->irq_mask[ctrl] = ~0;
@@ -367,18 +371,36 @@  static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)
 	}
 
 	/*
+	 * Some devices, such as AC5 and AC5X have their physical DDR memory
+	 * start at address 0x2_0000_0000 . In order to have the DMA
+	 * coherent allocation succeed later, a different DMA mask is
+	 * required, as defined in the DT file for such SOCs using dma-ranges.
+	 * If not defined, fallback to 32-bit as described below:
+	 *
 	 * Even though the iMSI-RX Module supports 64-bit addresses some
 	 * peripheral PCIe devices may lack 64-bit message support. In
 	 * order not to miss MSI TLPs from those devices the MSI target
 	 * address has to be within the lowest 4GB.
 	 *
-	 * Note until there is a better alternative found the reservation is
+	 * Note until there is a better alternative found, the reservation is
 	 * done by allocating from the artificially limited DMA-coherent
 	 * memory.
 	 */
-	ret = dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
+	ret = of_pci_dma_range_parser_init(&parser, np);
+	if (!ret) {
+		if (of_pci_range_parser_one(&parser, &range)) {
+			if (range.size > BIT_MASK(32) ) {
+				num_dma_maskbits = fls64(range.size);
+				dev_info(dev, "Overriding DMA mask to %u bits...\n", num_dma_maskbits);
+			}
+		}
+	}
+
+	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(num_dma_maskbits));
 	if (ret)
-		dev_warn(dev, "Failed to set DMA mask to 32-bit. Devices with only 32-bit MSI support may not work properly\n");
+		dev_warn(dev,
+			 "Failed to set DMA mask to %u-bit. Devices with only 32-bit MSI support may not work properly\n",
+			 num_dma_maskbits);
 
 	msi_vaddr = dmam_alloc_coherent(dev, sizeof(u64), &pp->msi_data,
 					GFP_KERNEL);