by Niklas Cassel

[permalink] [raw]

Subject: Re: [PATCH v6 06/18] PCI: designware-ep: Add generic function for raising MSI irq

On Thu, Dec 28, 2017 at 08:09:50PM +0530, Kishon Vijay Abraham I wrote:
> Hi Niklas,
>
> On Thursday 28 December 2017 01:36 PM, Kishon Vijay Abraham I wrote:
> > Hi Niklas,
> >
> > On Thursday 28 December 2017 03:59 AM, Niklas Cassel wrote:
> >> On Tue, Dec 26, 2017 at 06:20:54PM +0530, Kishon Vijay Abraham I wrote:
> >>> Hi Niklas,
> >>
> >> Hello Kishon
> >>
> >>>
> >>> On Wednesday 20 December 2017 04:59 AM, Niklas Cassel wrote:
> >>>> Add a generic function for raising MSI irqs that can be used by all
> >>>> DWC based controllers.
> >>>>
> >>>> Note that certain controllers, like DRA7xx, have a special convenience
> >>>> register for raising MSI irqs that doesn't require you to explicitly map
> >>>> the MSI address. Therefore, it is likely that certain drivers will
> >>>> not use this generic function, even if they can.
> >>>>
> >>>> Signed-off-by: Niklas Cassel <[email protected]>
> >>>> ---
> >>>> drivers/pci/dwc/pcie-designware-ep.c | 35 +++++++++++++++++++++++++++++++++++
> >>>> drivers/pci/dwc/pcie-designware.h | 9 +++++++++
> >>>> 2 files changed, 44 insertions(+)
> >>>>
> >>>> diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
> >>>> index 700ed2f4becf..c5aa1cac5041 100644
> >>>> --- a/drivers/pci/dwc/pcie-designware-ep.c
> >>>> +++ b/drivers/pci/dwc/pcie-designware-ep.c
> >>>> @@ -282,6 +282,41 @@ static const struct pci_epc_ops epc_ops = {
> >>>> .stop = dw_pcie_ep_stop,
> >>>> };
> >>>>
> >>>> +int dw_pcie_ep_raise_msi_irq(struct dw_pcie_ep *ep,
> >>>> + u8 interrupt_num)
> >>>> +{
> >>>> + struct dw_pcie *pci = to_dw_pcie_from_ep(ep);
> >>>> + struct pci_epc *epc = ep->epc;
> >>>> + u16 msg_ctrl, msg_data;
> >>>> + u32 msg_addr_lower, msg_addr_upper;
> >>>> + u64 msg_addr;
> >>>> + bool has_upper;
> >>>> + int ret;
> >>>> +
> >>>> + /* Raise MSI per the PCI Local Bus Specification Revision 3.0, 6.8.1. */
> >>>> + msg_ctrl = dw_pcie_readw_dbi(pci, MSI_MESSAGE_CONTROL);
> >>>> + has_upper = !!(msg_ctrl & PCI_MSI_FLAGS_64BIT);
> >>>> + msg_addr_lower = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_L32);
> >>>> + if (has_upper) {
> >>>> + msg_addr_upper = dw_pcie_readl_dbi(pci, MSI_MESSAGE_ADDR_U32);
> >>>> + msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_64);
> >>>> + } else {
> >>>> + msg_addr_upper = 0;
> >>>> + msg_data = dw_pcie_readw_dbi(pci, MSI_MESSAGE_DATA_32);
> >>>> + }
> >>>> + msg_addr = ((u64) msg_addr_upper) << 32 | msg_addr_lower;
> >>>> + ret = dw_pcie_ep_map_addr(epc, ep->msi_mem_phys, msg_addr,
> >>>> + epc->mem->page_size);
> >>>> + if (ret)
> >>>> + return ret;
> >>>> +
> >>>> + writel(msg_data | (interrupt_num - 1), ep->msi_mem);
> >>>
> >>> Shouldn't this be msg_data + (interrupt_num - 1)?
> >>
> >> I'm not quite sure about this, but if there is a pending irq,
> >> not yet processed by the RC, the msg_data we read out in this
> >> function should have a bit set, matching the pending irq.
> >
> > IIUC, the msg_data that we read here should not depend on the pending irq on
> > the RC side. msg_data should have the starting MSI vector number assigned by RC
> > for that EP device. (msg.data = pos; in dw_msi_setup_msg() also seem to suggest
> > the same).
> >>
> >> If that irq is the same as the irq we are trying to raise,
> >> doing an addition will produce a bogus vector number,
> >> but a bitwise or should work.
> >
> > if msg_data has the starting MSI vector, doing an addition should get to the
> > correct MSI vector.
> >>
> >> For that reason, I think that doing bitwise or seems safer.
> >> However, other than this case, I don't see why it should
> >> matter if we do an addition or a bitwise or.
> >>
> >> Are you having some problem with the code?
> >> It seems to be working fine on ARTPEC-6:
> >>
> >> # ./pcitest -m 1
> >> MSI1: OKAY
> >> # ./pcitest -m 2
> >> MSI2: OKAY
> >> # ./pcitest -m 3
> >> MSI3: OKAY
> >> # ./pcitest -m 4
> >> MSI4: OKAY
> >> # ./pcitest -m 5
> >> MSI5: OKAY
> >> # ./pcitest -m 6
> >> MSI6: OKAY
> >> # ./pcitest -m 7
> >> MSI7: OKAY
> >> # ./pcitest -m 8
> >> MSI8: OKAY
> >> # ./pcitest -m 9
> >> MSI9: OKAY
> >> # cat /proc/interrupts | grep -i msi
> >> 82: 9 0 GIC-0 180 Level artpec6-pcie-msi
> >> 188: 1 0 PCI-MSI 16 Edge pci-endpoint-test
> >> 189: 1 0 PCI-MSI 17 Edge pci-endpoint-test
> >> 190: 1 0 PCI-MSI 18 Edge pci-endpoint-test
> >> 191: 1 0 PCI-MSI 19 Edge pci-endpoint-test
> >> 192: 1 0 PCI-MSI 20 Edge pci-endpoint-test
> >> 193: 1 0 PCI-MSI 21 Edge pci-endpoint-test
> >> 194: 1 0 PCI-MSI 22 Edge pci-endpoint-test
> >> 195: 1 0 PCI-MSI 23 Edge pci-endpoint-test
> >> 196: 1 0 PCI-MSI 24 Edge pci-endpoint-test
> >> 197: 0 0 PCI-MSI 25 Edge pci-endpoint-test
> >> 198: 0 0 PCI-MSI 26 Edge pci-endpoint-test
> >> 199: 0 0 PCI-MSI 27 Edge pci-endpoint-test
> >> 200: 0 0 PCI-MSI 28 Edge pci-endpoint-test
> >> 201: 0 0 PCI-MSI 29 Edge pci-endpoint-test
> >> 202: 0 0 PCI-MSI 30 Edge pci-endpoint-test
> >> 203: 0 0 PCI-MSI 31 Edge pci-endpoint-test
> >>
> >> From EP:
> >> irq: 1 read msg_data: 0x10 writing: 0x10
> >> irq: 2 read msg_data: 0x10 writing: 0x11
> >> irq: 3 read msg_data: 0x10 writing: 0x12
> >> irq: 4 read msg_data: 0x10 writing: 0x13
> >> irq: 5 read msg_data: 0x10 writing: 0x14
> >> irq: 6 read msg_data: 0x10 writing: 0x15
> >> irq: 7 read msg_data: 0x10 writing: 0x16
> >> irq: 8 read msg_data: 0x10 writing: 0x17
> >> irq: 9 read msg_data: 0x10 writing: 0x18
> >
> > since your msg_data is 0x10, you are not facing the issue. What if it's 0x1? In
> > my case If I have Gustavo's patch series applied, msg_data has a value of 0x1
> > and I don't get certain MSI interrupts.
>
> I think Gustavo's series doesn't align the MSI vector properly. That's why I
> get 0x1. So your patch is fine as such.

If the EP requests 16 irqs via Multiple Message Capable field,
and the host grants it 16 irqs, by writing "100" to Multiple Message
Enable field, then the endpoint can read MSI message data, modify
bits 0-3, then write this modified value to the MSI message address.

All bits that the EP is allowed to modify has to be initialized to
0 by the host. E.g., in order to be able to raise 16 unique irqs,
bits 0-3 has to be initialized to 0 (since we need to be able to use
the whole range, 0x0-0xf).

i.e. initializing MSI message data with 0x1 seems wrong, if the EP
is requesting 16 irqs.

Note that the case where Multiple Message Enable is "000", is special,
since then the EP is not allowed to modify any bit, so then it is ok
if bit 0 in MSI message data is set, but the code should handle this
case already.

So I agree, this patch should be fine as is.

Regards,
Niklas

2020-09-23 23:20:29

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v6 01/18] PCI: dwc: Use the DMA-API to get the MSI address

On Wed, Dec 20, 2017 at 12:29:22AM +0100, Niklas Cassel wrote:
> Use the DMA-API to get the MSI address. This address will be written to
> our PCI config space and to the register which determines which AXI
> address the DWC IP will spoof for incoming MSI irqs.
>
> Since it is a PCIe endpoint device, rather than the CPU, that is supposed
> to write to the MSI address, the proper way to get the MSI address is by
> using the DMA API, not by using virt_to_phys().
>
> Using virt_to_phys() might work on some systems, but using the DMA API
> should work on all systems.
>
> This is essentially the same thing as allocating a buffer in a driver
> to which the endpoint will write to. To do this, we use the DMA API.
>
> Signed-off-by: Niklas Cassel <[email protected]>
> ---
> drivers/pci/dwc/pcie-designware-host.c | 15 ++++++++++++---
> drivers/pci/dwc/pcie-designware.h | 3 ++-
> 2 files changed, 14 insertions(+), 4 deletions(-)

Resurrecting an old thread...

Did this fix any actual problem for you? I'm asking because this
introduces a bug that an unmap is never done in the error path and
there's an issue with suspend/resume[1].

I'd like to simplify all this. The whole thing including the existing
alloc_page() seems pointless. AIUI, the DWC MSI controller just needs a
single address which is never forwarded out of the RC. So we could use
any address that an EP would never write to. Some drivers just take the
address of a field in their driver data. Perhaps even just the DBI base
would work or the address of PCIE_MSI_ADDR_LO.

Rob

[1] https://lore.kernel.org/r/[email protected]

>
> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c
> index 81e2157a7cfb..bf558df5b7b3 100644
> --- a/drivers/pci/dwc/pcie-designware-host.c
> +++ b/drivers/pci/dwc/pcie-designware-host.c
> @@ -83,10 +83,19 @@ irqreturn_t dw_handle_msi_irq(struct pcie_port *pp)
>
> void dw_pcie_msi_init(struct pcie_port *pp)
> {
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct device *dev = pci->dev;
> + struct page *page;
> u64 msi_target;
>
> - pp->msi_data = __get_free_pages(GFP_KERNEL, 0);
> - msi_target = virt_to_phys((void *)pp->msi_data);
> + page = alloc_page(GFP_KERNEL);
> + pp->msi_data = dma_map_page(dev, page, 0, PAGE_SIZE, DMA_FROM_DEVICE);
> + if (dma_mapping_error(dev, pp->msi_data)) {
> + dev_err(dev, "failed to map MSI data\n");
> + __free_page(page);
> + return;
> + }
> + msi_target = (u64)pp->msi_data;
>
> /* program the msi_data */
> dw_pcie_wr_own_conf(pp, PCIE_MSI_ADDR_LO, 4,
> @@ -187,7 +196,7 @@ static void dw_msi_setup_msg(struct pcie_port *pp, unsigned int irq, u32 pos)
> if (pp->ops->get_msi_addr)
> msi_target = pp->ops->get_msi_addr(pp);
> else
> - msi_target = virt_to_phys((void *)pp->msi_data);
> + msi_target = (u64)pp->msi_data;
>
> msg.address_lo = (u32)(msi_target & 0xffffffff);
> msg.address_hi = (u32)(msi_target >> 32 & 0xffffffff);
> diff --git a/drivers/pci/dwc/pcie-designware.h b/drivers/pci/dwc/pcie-designware.h
> index e5d9d77b778e..ecdede68522a 100644
> --- a/drivers/pci/dwc/pcie-designware.h
> +++ b/drivers/pci/dwc/pcie-designware.h
> @@ -14,6 +14,7 @@
> #ifndef _PCIE_DESIGNWARE_H
> #define _PCIE_DESIGNWARE_H
>
> +#include <linux/dma-mapping.h>
> #include <linux/irq.h>
> #include <linux/msi.h>
> #include <linux/pci.h>
> @@ -168,7 +169,7 @@ struct pcie_port {
> const struct dw_pcie_host_ops *ops;
> int msi_irq;
> struct irq_domain *irq_domain;
> - unsigned long msi_data;
> + dma_addr_t msi_data;
> DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> };
>
> --
> 2.14.2