On Fri, Dec 16, 2022 at 02:01:20PM +0000, Robin Murphy wrote: > On 2022-12-16 10:18, Serge Semin wrote: > > On Fri, Dec 16, 2022 at 01:49:38AM -0800, Christoph Hellwig wrote: > > > On Fri, Dec 16, 2022 at 12:34:23PM +0300, Serge Semin wrote: > > > > What about instead of save/restore pattern I'll just change the > > > > dma_set_mask_and_coherent() method with the dma_set_coherent_mask() > > > > function call? It seems cleaner. Like this: > > > > > > > Thus the platform-specific streaming DMA mask would be preserved. > > > > Since it's PCIe then having the streaming DMA-mask less than 32-bits > > > > wide is very much improbable. Moreover DW PCIe AXI-interface can be > > > > synthesize only with one out of two address bus widths: 32 and 64. > > > > > > > > Where platform-specific means the dwc subdriver? > > > > Right. I meant the streaming DMA-mask set by the low-level DWC PCIe drivers > > (like pcie-qcom(-ep)?.c, pcie-bt1.c, etc). It's very much important to > > have the real DMA-mask (at least the streaming one) set for the eDMA-capable > > controllers so the DMA-engine clients would work with the best performance. > > > > > Yes, that seems to work. > > > > Ok. I'll just use the direct dma_set_coherent_mask() method here then. > > > > > Alternatively have a flag that says which streaming mask > > > to set. > > > > I'd prefer to have more flexibility here relying on the low-level > > drivers to set the mask(s) instead of adding the new flag, just in case > > if there is vendor-specific IP-core/platform changes in the address > > bus width. > > Presumably the low-level glue drivers could pass a bus size or mask value in > struct dw_pcie_rp/dw_pcie, so the actual dma_set_mask() call itself could be > centralised? I guess there's also an argument that only glue drivers which > care about eDMA need to care about setting a mask at all, so I don't have a > string preference either way. There is another peculiarity here due to which the centralized approach turns to be less suitable. The dw_pcie_msi_host_init() method, which currently updates the mask, isn't always executed. It's run only if pci_msi_enabled() returns true and there is no platform-specific dw_pci_rp.msi_host_init callback specified. Thus if we got to implement the centralized DMA-mask setting up procedure then it should have been done in the always executed place. That code would have also implied the 32-bit coherent DMA-mask if the generic iMSI-RX config is required. Thus the iMSI-RX-specific setups (data allocation and the mask settings) would have been split up into different places which would be less maintainable. Moreover as you correctly noted the real DMA-mask setting up is only needed for the eDMA-capable controllers. Meanwhile in the most of the cases there is no eDMA embedded in the PCIe controller, but due to the centralized approach we would need to set some mask anyway. Since I don't know the real address bus width of all the already available DW PCIe platforms we'll have to fallback to using some default mask value, which might incorrectly describe the actual device capability (and possible cause some side-effects/regressions). To sum up weighing up pros and cons the centralized approach seems to me more complex, less maintainable and less flexible. In my opinion relying on the glue-drivers to set the mask if it's needed to be set (that is there is the embedded eDMA) is the best and the simplest choice. > If you'd rather stick with that approach then > it might be worth a brief comment at each site to clarify why the other mask > is being set from an entirely different place, just in case anyone comes > along and tries to "fix" it. Exactly what I was going to do. I'll add a brief comment why the coherent DMA-mask is updated in the dw_pcie_msi_host_init() method. -Serge(y) > > Cheers, > Robin.
