Messages in this thread | | | Subject | Re: [PATCH 4/5] PCI: iproc: Add iProc PCIe MSI support | From | Ray Jui <> | Date | Thu, 19 Nov 2015 11:22:30 -0800 |
| |
Hi Arnd,
On 11/18/2015 1:50 AM, Arnd Bergmann wrote: > On Wednesday 18 November 2015 08:48:45 Marc Zyngier wrote: >>> +static inline u32 iproc_msi_read_reg(struct iproc_msi *msi, >>> + enum iproc_msi_reg reg, >>> + unsigned int eq) >>> +{ >>> + struct iproc_pcie *pcie = msi->pcie; >>> + >>> + return readl(pcie->base + msi->reg_offsets[eq][reg]); >> >> Do you need the extra barrier implied by readl? readl_relaxed should be >> enough. > > I suspect this is the one place where it's needed for a lot of > drivers: when the PCI device sends DMA data followed by the MSI > message, the device driver can safely assume that the DMA data > has arrived in memory even without doing another readl() from > the device itself. > > It really depends on how the MSI implementation here interacts > with the memory controller, and we should probably have a comment > to explain this either way. > >>> +static inline void iproc_msi_write_reg(struct iproc_msi *msi, >>> + enum iproc_msi_reg reg, >>> + int eq, u32 val) >>> +{ >>> + struct iproc_pcie *pcie = msi->pcie; >>> + >>> + writel(val, pcie->base + msi->reg_offsets[eq][reg]); >> >> Same here for writel vs writel_relaxed. > > We probably want writel_relaxed() when calling this from > iproc_msi_handler(), but not when calling from > iproc_msi_enable(), which should default to a normal > writel(), so we can be sure it's actually configured right > at the time we return from iproc_msi_init(). You could > try to prove that using writel_relaxed is correct here, but > using writel makes it so much easier. > > Arnd >
I need to think through the logic in iproc_msi_handler to make sure the correct accesses are used at the right place. The iproc_msi_handler needs to be re-written to support multiple MSI vectors per wired interrupt.
Thanks,
Ray
| |