PCI: dwc: Add support for 64-bit MSI target addresses
v3 cover PCI: dwc: Add support for 64-bit MSI target addresses
Hi All,
I have updated the series based on the reviews to use managed DMA allocation API. This allows me to drop the msi_page parameter from struct dw_pcie_rp. Please take a look and let me know if you have any concerns.
Thanks again for the reviews!
Regards,
Will
Will McVicker (2):
PCI: dwc: drop dependency on ZONE_DMA32
PCI: dwc: add support for 64-bit MSI target address
v3:
* Switched to a managed DMA allocation.
* Simplified the DMA allocation cleanup.
* Dropped msi_page from struct dw_pcie_rp.
* Allocating a u64 instead of a full page.
v2:
* Fixed build error caught by kernel test robot
* Fixed error handling reported by Isaac Manjarres
.../pci/controller/dwc/pcie-designware-host.c | 42 +++++++++---------- drivers/pci/controller/dwc/pcie-designware.c | 9 ++++ drivers/pci/controller/dwc/pcie-designware.h | 2 +- 3 files changed, 29 insertions(+), 24 deletions(-)
base-commit: aeb6e6ac18c73ec287b3b1e2c913520699358c13
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
PATCH v3 1/2 PCI: dwc: drop dependency on ZONE_DMA32
Re-work the msi_msg DMA allocation logic to use dmam_alloc_coherent() which uses the coherent DMA mask to try and return an allocation within the DMA mask limits. With that, we can now drop the msi_page parameter in struct dw_pcie_rp. This allows kernel configurations that disable ZONE_DMA32 to continue supporting a 32-bit DMA mask. Without this patch, the PCIe host device will fail to probe when ZONE_DMA32 is disabled.
--- .../pci/controller/dwc/pcie-designware-host.c | 28 +++++-------------- drivers/pci/controller/dwc/pcie-designware.h | 1 - 2 files changed, 7 insertions(+), 22 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +7 -21
@@ -267,15 +267,6 @@ static void dw_pcie_free_msi(struct dw_pcie_rp *pp)irq_domain_remove (pp->msi_domain);irq_domain_remove (pp->irq_domain); - -if (pp->msi_data) { -struct dw_pcie *pci =to_dw_pcie_from_pp (pp); -struct device *dev = pci->dev; - -dma_unmap_page (dev, pp->msi_data,PAGE_SIZE ,DMA_FROM_DEVICE ); -if (pp->msi_page) -__free_page (pp->msi_page); - } }static void dw_pcie_msi_init (struct dw_pcie_rp *pp)
@@ -336,6 +327,7 @@ 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 platform_device *pdev =to_platform_device (dev); +u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls;
@@ -375,22 +367,16 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask (dev,DMA_BIT_MASK (32 )); + ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 ));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" ); - pp->msi_page =alloc_page (GFP_DMA32 ); - pp->msi_data =dma_map_page (dev, pp->msi_page,0 , -PAGE_SIZE ,DMA_FROM_DEVICE ); - ret =dma_mapping_error (dev, pp->msi_data); -if (ret) { -dev_err (pci->dev,"Failed to map MSI data\n" ); -__free_page (pp->msi_page); - pp->msi_page =NULL ; - pp->msi_data =0 ; + msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data, +GFP_KERNEL ); +if (!msi_vaddr) { +dev_err (dev,"Failed to alloc and map MSI data\n" );dw_pcie_free_msi (pp); - -return ret; +return -ENOMEM ; }return 0 ;
drivers/pci/controller/dwc/pcie-designware.h +0 -1
@@ -243,7 +243,6 @@ struct dw_pcie_rp {struct irq_domain *irq_domain;struct irq_domain *msi_domain;dma_addr_t msi_data; -struct page *msi_page;struct irq_chip *msi_irq_chip;u32 num_vectors;u32 irq_mask[MAX_MSI_CTRLS ];
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
PATCH v3 2/2 PCI: dwc: add support for 64-bit MSI target address
In reply to Will McVickerSince not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable ZONE_DMA32 and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate the msi_msg page due to a DMA address overflow (seen in [1]). With this patch, the PCIe driver can advertise 64-bit support via it's MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
[1] https://lore.kernel.org/all/[email protected]/
--- drivers/pci/controller/dwc/pcie-designware-host.c | 14 ++++++++++++-- drivers/pci/controller/dwc/pcie-designware.c | 9 +++++++++ drivers/pci/controller/dwc/pcie-designware.h | 1 + 3 files changed, 22 insertions(+), 2 deletions(-)
drivers/pci/controller/dwc/pcie-designware-host.c +12 -2
@@ -330,6 +330,8 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp)u64 *msi_vaddr;int ret;u32 ctrl, num_ctrls; +bool msi_64b = false; +u16 msi_capabilities;for (ctrl =0 ; ctrl <MAX_MSI_CTRLS ; ctrl++) pp->irq_mask[ctrl] = ~0 ;
@@ -367,9 +369,17 @@ static int dw_pcie_msi_host_init(struct dw_pcie_rp *pp) dw_chained_msi_isr, pp); } - ret =dma_set_mask_and_coherent (dev,DMA_BIT_MASK (32 )); + msi_capabilities =dw_pcie_msi_capabilities (pci); +if (msi_capabilities &PCI_MSI_FLAGS_ENABLE ) + msi_64b = msi_capabilities &PCI_MSI_FLAGS_64BIT ? true : false; + +dev_dbg (dev,"Setting MSI DMA mask to %s-bit.\n" , + msi_64b ?"64" :"32" ); + ret =dma_set_mask_and_coherent (dev, msi_64b ? +DMA_BIT_MASK (64 ) :DMA_BIT_MASK (32 ));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 %s-bit.\n" , + msi_64b ?"64" :"32" ); msi_vaddr =dmam_alloc_coherent (dev,sizeof (u64), &pp->msi_data,GFP_KERNEL );
drivers/pci/controller/dwc/pcie-designware.c +9 -0
@@ -82,6 +82,15 @@ u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) }EXPORT_SYMBOL_GPL (dw_pcie_find_capability); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +EXPORT_SYMBOL_GPL (dw_pcie_msi_capabilities); +static u16 dw_pcie_find_next_ext_capability (struct dw_pcie *pci,u16 start,u8 cap) {
drivers/pci/controller/dwc/pcie-designware.h +1 -0
@@ -332,6 +332,7 @@ void dw_pcie_version_detect(struct dw_pcie *pci);u8 dw_pcie_find_capability (struct dw_pcie *pci,u8 cap);u16 dw_pcie_find_ext_capability (struct dw_pcie *pci,u8 cap); +u16 dw_pcie_msi_capabilities (struct dw_pcie *pci);int dw_pcie_read (void __iomem *addr,int size,u32 *val);int dw_pcie_write (void __iomem *addr,int size,u32 val);
Signature · 1 line
2.37.1.559.g78731f0fdb-goog
Re: [PATCH v3 2/2] PCI: dwc: add support for 64-bit MSI target address
No need to rush the versions (I see v1 on 8/9, v2 and v3 on 8/10).
But if/when you update this, capitalize the subject lines ("PCI: dwc: Add support ...") to match previous history.
Quoted 11 lines · Will McVicker
Since not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable
ZONE_DMA32and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate themsi_msgpage due to a DMA address overflow (seen in [1]).With this patch, the PCIe driver can advertise 64-bit support via it's MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
s/via it's/via its/
I'm not quite sure what this sentence is saying. I think it's actually the *device* (not the PCIe driver) that advertises 64-bit support in its MSI capability.
drivers/pci/controller/dwc/pcie-designware.c u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) · line 92
Quoted 8 lines
drivers/pci/controller/dwc/pcie-designware.c +8
-0+u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +EXPORT_SYMBOL_GPL (dw_pcie_msi_capabilities);
Why does this need to be exported? CONFIG_PCIE_DW and CONFIG_PCIE_DW_HOST are both bool, so I don't see any callers from modules.
I see there are some other functions in pcie-designware.c that are exported, but I'm a little dubious about those, too. There are several DWC drivers that are tristate (PCI_DRA7XX, PCI_EXYNOS, etc), but they select PCIE_DW_HOST and PCIE_DW, which are bool. I guess this means the DWC core code gets built-in while the dra7xx, exynos, etc code is a module.
If we want these to be modules, it would make more sense to me to have the module include both the DWC core code and the specific driver. I.e., the DWC core code would not be statically included at all, and the dra7xx module would contain DWC core and dra7xx, the exynos module would contain DWC core and exynos, etc.
Maybe Kconfig isn't expressive enough for that? I don't know Kconfig well enough.
Bjorn
Re: [PATCH v3 2/2] PCI: dwc: add support for 64-bit MSI target address
Quoted 4 lines · Bjorn Helgaas
No need to rush the versions (I see v1 on 8/9, v2 and v3 on 8/10).
But if/when you update this, capitalize the subject lines ("PCI: dwc: Add support ...") to match previous history.
Sorry for the quick updates. I'll be sure to update the subject on the next iteration.
Quoted 10 lines · Will McVicker
Quoted 7 lines
Since not all devices require a 32-bit MSI address, add support to the PCIe host driver to allow setting the DMA mask to 64-bits. This allows kernels to disable
ZONE_DMA32and bounce buffering (swiotlb) without risking not being able to get a 32-bit address during DMA allocation. Basically, in the slim chance that there are no 32-bit allocations available, the current PCIe host driver will fail to allocate themsi_msgpage due to a DMA address overflow (seen in [1]).
Quoted 5 lines
Quoted 3 lines
With this patch, the PCIe driver can advertise 64-bit support via it's MSI capabilities to hint to the PCIe host driver to set the DMA mask to 64-bits.
s/via it's/via its/
Thanks.
Quoted 4 lines
I'm not quite sure what this sentence is saying. I think it's actually the *device* (not the PCIe driver) that advertises 64-bit support in its MSI capability.
You're right. The PCIe device is doing the advertising. I'll updae this wording
Quoted 21 lines
drivers/pci/controller/dwc/pcie-designware.c u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap) · line 92
Quoted 9 lines
drivers/pci/controller/dwc/pcie-designware.c +8
-0+u16 dw_pcie_msi_capabilities (struct dw_pcie *pci) +{ +u8 offset; + + offset =dw_pcie_find_capability (pci,PCI_CAP_ID_MSI ); +return dw_pcie_readw_dbi (pci, offset +PCI_MSI_FLAGS ); +} +EXPORT_SYMBOL_GPL (dw_pcie_msi_capabilities);Why does this need to be exported?
CONFIG_PCIE_DWandCONFIG_PCIE_DW_HOSTare both bool, so I don't see any callers from modules.I see there are some other functions in
pcie-designware.cthat are exported, but I'm a little dubious about those, too. There are several DWC drivers that are tristate (PCI_DRA7XX,PCI_EXYNOS, etc), but they selectPCIE_DW_HOSTandPCIE_DW, which are bool. I guess this means the DWC core code gets built-in while the dra7xx, exynos, etc code is a module.
Quoted 8 lines
If we want these to be modules, it would make more sense to me to have the module include both the DWC core code and the specific driver. I.e., the DWC core code would not be statically included at all, and the dra7xx module would contain DWC core and dra7xx, the exynos module would contain DWC core and exynos, etc.
Maybe
Kconfigisn't expressive enough for that? I don't knowKconfigwell enough.
So you usually want to separate out the core driver from the device driver and then add the core driver dependencies in the device driver Kconfig definition. This allows you to statically include the core drivers (commonly used code between multiple device drivers) into the kernel and then build the device drivers as kernel modules. And just to note, it's okay for PCIE_DW and PCIE_DW_HOST to be configured as a bool and selected from a tristate config. If you take PCI_EXYNOS=m as an example, that will result in:
CONFIG_PCIE_DW=y
CONFIG_PCIE_DW_HOST=y
CONFIG_PCI_EXYNOS=m
With regards to exporting dw_pcie_msi_capabilities(), the idea is that this function is a helper for the PCI DW device drivers. For those that want to enable the 64-bit MSI flag, they can use this function to get the MSI capabilities in order to update the MSI flags. I haven't added a use-case to an existing driver yet.
Quoted 6 lines
Bjorn
Signature · 1 line
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].