| Message ID | 58f3a6ca-a051-a7dc-805f-ee3eb0f3ac3b@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Mark, Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: > Joao, > > On 05/05/17 15:11, Joao Pinto wrote: >> Hello, >> I am currently adding the support for both MSI and MSI-x in pcie-designware and >> I am now facing a dificulty. >> >> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >> the changes introduced by this patch we are able to enable MSI and MSI-X >> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >> >> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >> bogus (524288) when it should be 1, and so I am not receiving any interrupts >> from the endpoint. > > It is not bogus at all. It is computed from the PCI requester ID in > pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain > view of that interrupt, which is completely virtual. > > The real thing happens in your own irqdomain, where the hwirq for IRQ46 > is probably 1 (only you can know that). As for why it doesn't work, see > below: > snip (...) > static struct irq_chip dw_msi_irq_chip = { > .name = "PCI-MSI", > - .irq_enable = pci_msi_unmask_irq, > - .irq_disable = pci_msi_mask_irq, > - .irq_mask = pci_msi_mask_irq, > - .irq_unmask = pci_msi_unmask_irq, > + .irq_mask = dw_msi_mask_irq, > + .irq_unmask = dw_msi_unmask_irq, > }; > > I haven't dug any further, but this should be fixed first. > > Thanks, > > M. > Thanks for the help! I will check it out! Joao
Mark, Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: > Joao, > > On 05/05/17 15:11, Joao Pinto wrote: snip (...) > It looks to me like the first mistake is just above. You don't seem to > propagate the mask/unmask operations into the parent domain, so your > interrupts are never enabled... You'd need something like this: > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > index 28ed32ba4f1b..d42b8b12f168 100644 > --- a/drivers/pci/dwc/pcie-designware-host.c > +++ b/drivers/pci/dwc/pcie-designware-host.c > @@ -45,12 +45,22 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, > return dw_pcie_write(pci->dbi_base + where, size, val); > } > > +static void dw_msi_mask_irq(struct irq_data *d) > +{ > + pci_msi_mask_irq(d); > + irq_chip_mask_parent(d); > +} > + > +static void dw_msi_unmask_irq(struct irq_data *d) > +{ > + pci_msi_unmask_irq(d); > + irq_chip_unmask_parent(d); > +} > + > static struct irq_chip dw_msi_irq_chip = { > .name = "PCI-MSI", > - .irq_enable = pci_msi_unmask_irq, > - .irq_disable = pci_msi_mask_irq, > - .irq_mask = pci_msi_mask_irq, > - .irq_unmask = pci_msi_unmask_irq, > + .irq_mask = dw_msi_mask_irq, > + .irq_unmask = dw_msi_unmask_irq, > }; > > I haven't dug any further, but this should be fixed first. > > Thanks, > > M. > I added the changes that you suggested and firstly it crashed, and then I found out that in the domain parent structure (dw_pci_msi_bottom_irq_chip) did not have irq_mask and irq_unmask functions. I put it like this, just for testing: static struct irq_chip dw_pci_msi_bottom_irq_chip = { .name = "DW MSI", .irq_compose_msi_msg = dw_pci_compose_msi_msg, .irq_set_affinity = dw_pci_msi_set_affinity, + .irq_mask = pci_msi_mask_irq, + .irq_unmask = pci_msi_unmask_irq, }; Maybe irq_mask and irq_unmask should not be pci_msi_mask_irq / pci_msi_unmask_irq, but I did not find any existing case that needed it. Do we have any documentation about IRQ domains that I can check? Interrupts 45: 0 PCI-MSI 0 aerdrv 46: 0 PCI-MSI 524288 xhci_hcd The MSI seems to be well configured in the Bridge and in the Endpoint, so some glue is missing in the driver. 00:00.0 PCI bridge: Synopsys, Inc. Device eddc (rev 01) (prog-if 00 [Normal decode]) Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+ Address: 000000009a238000 Data: 0000 01:00.0 USB controller: ASMedia Technology Inc. ASM1142 USB 3.1 Host Controller (prog-if 30 [XHCI]) (...) Capabilities: [50] MSI: Enable+ Count=1/8 Maskable- 64bit+ Address: 000000009a238000 Data: 0001 Thanks for the support. Joao
On Fri, 5 May 2017 16:54:30 +0100 Joao Pinto <Joao.Pinto@synopsys.com> wrote: > Mark, > > Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: > > Joao, > > > > On 05/05/17 15:11, Joao Pinto wrote: > > snip (...) > > > It looks to me like the first mistake is just above. You don't seem to > > propagate the mask/unmask operations into the parent domain, so your > > interrupts are never enabled... You'd need something like this: > > > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > > index 28ed32ba4f1b..d42b8b12f168 100644 > > --- a/drivers/pci/dwc/pcie-designware-host.c > > +++ b/drivers/pci/dwc/pcie-designware-host.c > > @@ -45,12 +45,22 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, > > return dw_pcie_write(pci->dbi_base + where, size, val); > > } > > > > +static void dw_msi_mask_irq(struct irq_data *d) > > +{ > > + pci_msi_mask_irq(d); > > + irq_chip_mask_parent(d); > > +} > > + > > +static void dw_msi_unmask_irq(struct irq_data *d) > > +{ > > + pci_msi_unmask_irq(d); > > + irq_chip_unmask_parent(d); > > +} > > + > > static struct irq_chip dw_msi_irq_chip = { > > .name = "PCI-MSI", > > - .irq_enable = pci_msi_unmask_irq, > > - .irq_disable = pci_msi_mask_irq, > > - .irq_mask = pci_msi_mask_irq, > > - .irq_unmask = pci_msi_unmask_irq, > > + .irq_mask = dw_msi_mask_irq, > > + .irq_unmask = dw_msi_unmask_irq, > > }; > > > > I haven't dug any further, but this should be fixed first. > > > > Thanks, > > > > M. > > > > I added the changes that you suggested and firstly it crashed, and then I found > out that in the domain parent structure (dw_pci_msi_bottom_irq_chip) did not > have irq_mask and irq_unmask functions. I put it like this, just for testing: > > static struct irq_chip dw_pci_msi_bottom_irq_chip = { > .name = "DW MSI", > .irq_compose_msi_msg = dw_pci_compose_msi_msg, > .irq_set_affinity = dw_pci_msi_set_affinity, > + .irq_mask = pci_msi_mask_irq, > + .irq_unmask = pci_msi_unmask_irq, > }; > > Maybe irq_mask and irq_unmask should not be pci_msi_mask_irq / > pci_msi_unmask_irq, but I did not find any existing case that needed it. Do we > have any documentation about IRQ domains that I can check? We have some documentation in Documentation/IRQ-domain.txt, though it is old an incomplete. But there is enough there to show you your mistake. You have two domains: Top level domain: the PCI-MSI domain, in charge of the PCI-specific stuff, and particularly the masking/unmasking of MSIs. That's the purpose of the patch above. Bottom domain: your DW-MSI domain, which is only concerned with the details of your piece of HW. Obviously, calling pci_msi*irq is wrong there, since it is already called by the top level domain, and is hardly HW specific. So what you need there is the code that masks/unmasks an MSI for your HW, and nothing else. > > Interrupts > 45: 0 PCI-MSI 0 aerdrv > 46: 0 PCI-MSI 524288 xhci_hcd > > The MSI seems to be well configured in the Bridge and in the Endpoint, so some > glue is missing in the driver. Yup, see above. Thanks, M.
Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: > Joao, > > On 05/05/17 15:11, Joao Pinto wrote: >> Hello, >> I am currently adding the support for both MSI and MSI-x in pcie-designware and >> I am now facing a dificulty. >> >> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >> the changes introduced by this patch we are able to enable MSI and MSI-X >> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >> >> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >> bogus (524288) when it should be 1, and so I am not receiving any interrupts >> from the endpoint. > > It is not bogus at all. It is computed from the PCI requester ID in > pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain > view of that interrupt, which is completely virtual. > > The real thing happens in your own irqdomain, where the hwirq for IRQ46 > is probably 1 (only you can know that). As for why it doesn't work, see > below: > >> >> I would apreciate that more experienced people in this interrupt subject could >> give me an hint. >> >> I send you the "lspci -v" results and /proc/interrupts. >> >> Thank you so much for your help. >> >> # cat /proc/interrupts >> CPU0 >> 3: 755 ARC In-core Intc 3 Timer0 (per-cpu-tick) >> 4: 0 dw-apb-ictl 4 eth0 >> 8: 1 dw-apb-ictl 8 ehci_hcd:usb1, ohci_hcd:usb2 >> 9: 37 dw-apb-ictl 7 dw-mci >> 14: 0 dw-apb-ictl 14 e001d000.i2c >> 16: 0 dw-apb-ictl 16 e001f000.i2c >> 19: 145 dw-apb-ictl 19 serial >> 45: 0 PCI-MSI 0 aerdrv >> 46: 0 PCI-MSI 524288 xhci_hcd >> >> # lspci -v >> 00:00.0 PCI bridge: Synopsys, Inc. Device eddc (rev 01) (prog-if 00 [Normal decode]) >> Flags: bus master, fast devsel, latency 0, IRQ 45 >> Bus: primary=00, secondary=01, subordinate=01, sec-latency=0 >> Memory behind bridge: d0400000-d04fffff >> [virtual] Expansion ROM at d0500000 [disabled] [size=64K] >> Capabilities: [40] Power Management version 3 >> Capabilities: [50] MSI: Enable+ Count=1/1 Maskable- 64bit+ >> Capabilities: [70] Express Root Port (Slot-), MSI 00 >> Capabilities: [100] Advanced Error Reporting >> Capabilities: [148] #19 >> Capabilities: [158] Vendor Specific Information: ID=0002 Rev=4 Len=100 <?> >> Kernel driver in use: pcieport >> >> 01:00.0 USB controller: ASMedia Technology Inc. ASM1142 USB 3.1 Host Controller (prog-if 30 [XHCI]) >> Subsystem: ASMedia Technology Inc. ASM1142 USB 3.1 Host Controller >> Flags: bus master, fast devsel, latency 0, IRQ 46 >> Memory at d0400000 (64-bit, non-prefetchable) [size=32K] >> Capabilities: [50] MSI: Enable+ Count=1/8 Maskable- 64bit+ >> Capabilities: [68] MSI-X: Enable- Count=8 Masked- >> Capabilities: [78] Power Management version 3 >> Capabilities: [80] Express Endpoint, MSI 00 >> Capabilities: [100] Virtual Channel >> Capabilities: [200] Advanced Error Reporting >> Capabilities: [280] #19 >> Capabilities: [300] Latency Tolerance Reporting >> Kernel driver in use: xhci_hcd >> >> Signed-off-by: Joao Pinto <jpinto@synopsys.com> >> --- >> .../devicetree/bindings/pci/designware-pcie.txt | 2 + >> drivers/pci/dwc/pcie-designware-host.c | 292 ++++++++------------- >> drivers/pci/dwc/pcie-designware-plat.c | 35 +-- >> drivers/pci/dwc/pcie-designware.h | 7 +- >> 4 files changed, 143 insertions(+), 193 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/pci/designware-pcie.txt b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> index b2480dd..41803ea 100644 >> --- a/Documentation/devicetree/bindings/pci/designware-pcie.txt >> +++ b/Documentation/devicetree/bindings/pci/designware-pcie.txt >> @@ -34,6 +34,8 @@ Optional properties: >> RC mode: >> - num-viewport: number of view ports configured in >> hardware. If a platform does not specify it, the driver assumes 2. >> +- num-vectors: number of available interrupt vectors. If a platform does not >> + specify it, the driver assumes 1. >> - bus-range: PCI bus numbers covered (it is recommended >> for new devicetrees to specify this property, to keep backwards >> compatibility a range of 0x00-0xff is assumed if not present) >> diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c >> index 28ed32b..96a5fb3 100644 >> --- a/drivers/pci/dwc/pcie-designware-host.c >> +++ b/drivers/pci/dwc/pcie-designware-host.c >> @@ -11,6 +11,9 @@ >> * published by the Free Software Foundation. >> */ >> >> +#include <linux/interrupt.h> >> +#include <linux/irqchip/chained_irq.h> >> +#include <linux/msi.h> >> #include <linux/irqdomain.h> >> #include <linux/of_address.h> >> #include <linux/of_pci.h> >> @@ -53,32 +56,43 @@ static struct irq_chip dw_msi_irq_chip = { >> .irq_unmask = pci_msi_unmask_irq, >> }; > > It looks to me like the first mistake is just above. You don't seem to > propagate the mask/unmask operations into the parent domain, so your > interrupts are never enabled... You'd need something like this: > > diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c > index 28ed32ba4f1b..d42b8b12f168 100644 > --- a/drivers/pci/dwc/pcie-designware-host.c > +++ b/drivers/pci/dwc/pcie-designware-host.c > @@ -45,12 +45,22 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, > return dw_pcie_write(pci->dbi_base + where, size, val); > } > > +static void dw_msi_mask_irq(struct irq_data *d) > +{ > + pci_msi_mask_irq(d); > + irq_chip_mask_parent(d); > +} > + > +static void dw_msi_unmask_irq(struct irq_data *d) > +{ > + pci_msi_unmask_irq(d); > + irq_chip_unmask_parent(d); > +} > + > static struct irq_chip dw_msi_irq_chip = { > .name = "PCI-MSI", > - .irq_enable = pci_msi_unmask_irq, > - .irq_disable = pci_msi_mask_irq, > - .irq_mask = pci_msi_mask_irq, > - .irq_unmask = pci_msi_unmask_irq, > + .irq_mask = dw_msi_mask_irq, > + .irq_unmask = dw_msi_unmask_irq, > }; > > I haven't dug any further, but this should be fixed first. > > Thanks, > > M. > Yep makes perfectly sense and thanks for clearing the Domains :), I understood them now. Best regards, Joao
On 08/05/17 10:59, Joao Pinto wrote: > Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >> Joao, >> >> On 05/05/17 15:11, Joao Pinto wrote: >>> Hello, >>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>> I am now facing a dificulty. >>> >>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>> the changes introduced by this patch we are able to enable MSI and MSI-X >>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>> >>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>> from the endpoint. >> >> It is not bogus at all. It is computed from the PCI requester ID in >> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >> view of that interrupt, which is completely virtual. >> >> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >> is probably 1 (only you can know that). As for why it doesn't work, see >> below: >> >>> >>> I would apreciate that more experienced people in this interrupt subject could >>> give me an hint. [...] > Yep makes perfectly sense and thanks for clearing the Domains :), I understood > them now. I guess that you have your MSI controller working with MSI-X now? Thanks, M.
Às 11:22 AM de 5/8/2017, Marc Zyngier escreveu: > On 08/05/17 10:59, Joao Pinto wrote: >> Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >>> Joao, >>> >>> On 05/05/17 15:11, Joao Pinto wrote: >>>> Hello, >>>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>>> I am now facing a dificulty. >>>> >>>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>>> the changes introduced by this patch we are able to enable MSI and MSI-X >>>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>>> >>>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>>> from the endpoint. >>> >>> It is not bogus at all. It is computed from the PCI requester ID in >>> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >>> view of that interrupt, which is completely virtual. >>> >>> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >>> is probably 1 (only you can know that). As for why it doesn't work, see >>> below: >>> >>>> >>>> I would apreciate that more experienced people in this interrupt subject could >>>> give me an hint. > > [...] > >> Yep makes perfectly sense and thanks for clearing the Domains :), I understood >> them now. > > I guess that you have your MSI controller working with MSI-X now? I started working one hour ago and still putting some subjects in order, I am going to return to PCI in a bit. I will keep you updated. I thought you were in US Pacific timezone. Thanks, Joao > > Thanks, > > M. >
On 08/05/17 11:24, Joao Pinto wrote: > Às 11:22 AM de 5/8/2017, Marc Zyngier escreveu: >> On 08/05/17 10:59, Joao Pinto wrote: >>> Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >>>> Joao, >>>> >>>> On 05/05/17 15:11, Joao Pinto wrote: >>>>> Hello, >>>>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>>>> I am now facing a dificulty. >>>>> >>>>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>>>> the changes introduced by this patch we are able to enable MSI and MSI-X >>>>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>>>> >>>>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>>>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>>>> from the endpoint. >>>> >>>> It is not bogus at all. It is computed from the PCI requester ID in >>>> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >>>> view of that interrupt, which is completely virtual. >>>> >>>> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >>>> is probably 1 (only you can know that). As for why it doesn't work, see >>>> below: >>>> >>>>> >>>>> I would apreciate that more experienced people in this interrupt subject could >>>>> give me an hint. >> >> [...] >> >>> Yep makes perfectly sense and thanks for clearing the Domains :), I understood >>> them now. >> >> I guess that you have your MSI controller working with MSI-X now? > > I started working one hour ago and still putting some subjects in order, I am > going to return to PCI in a bit. I will keep you updated. I thought you were in > US Pacific timezone. I'm firmly rooted in BST! ;-) M.
Às 11:59 AM de 5/8/2017, Marc Zyngier escreveu: > On 08/05/17 11:24, Joao Pinto wrote: >> Às 11:22 AM de 5/8/2017, Marc Zyngier escreveu: >>> On 08/05/17 10:59, Joao Pinto wrote: >>>> Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >>>>> Joao, >>>>> >>>>> On 05/05/17 15:11, Joao Pinto wrote: >>>>>> Hello, >>>>>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>>>>> I am now facing a dificulty. >>>>>> >>>>>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>>>>> the changes introduced by this patch we are able to enable MSI and MSI-X >>>>>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>>>>> >>>>>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>>>>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>>>>> from the endpoint. >>>>> >>>>> It is not bogus at all. It is computed from the PCI requester ID in >>>>> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >>>>> view of that interrupt, which is completely virtual. >>>>> >>>>> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >>>>> is probably 1 (only you can know that). As for why it doesn't work, see >>>>> below: >>>>> >>>>>> >>>>>> I would apreciate that more experienced people in this interrupt subject could >>>>>> give me an hint. >>> >>> [...] >>> >>>> Yep makes perfectly sense and thanks for clearing the Domains :), I understood >>>> them now. >>> >>> I guess that you have your MSI controller working with MSI-X now? >> >> I started working one hour ago and still putting some subjects in order, I am >> going to return to PCI in a bit. I will keep you updated. I thought you were in >> US Pacific timezone. > > I'm firmly rooted in BST! ;-) > > M. > You were right, it was missing the bottom part related to the PCie DW IP: static void dw_pci_bottom_mask(struct irq_data *data) { struct dw_pcie *pci = irq_data_get_irq_chip_data(data); struct pcie_port *pp = &pci->pp; unsigned int res, bit, val; res = (data->hwirq / 32) * 12; bit = data->hwirq % 32; dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val); val &= ~(1 << bit); dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val); } static void dw_pci_bottom_unmask(struct irq_data *data) { struct dw_pcie *pci = irq_data_get_irq_chip_data(data); struct pcie_port *pp = &pci->pp; unsigned int res, bit, val; res = (data->hwirq / 32) * 12; bit = data->hwirq % 32; dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val); val |= 1 << bit; dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val); } static struct irq_chip dw_pci_msi_bottom_irq_chip = { .name = "DW MSI", .irq_compose_msi_msg = dw_pci_compose_msi_msg, .irq_set_affinity = dw_pci_msi_set_affinity, .irq_mask = dw_pci_bottom_mask, .irq_unmask = dw_pci_bottom_unmask, }; With these 2 functions I was able to perform test with MSI and with MSI-X Endpoints. I am goingo to clean this up and check the compatibility with other SoC drivers and send a patch soon. Thanks for the help Marc! Regards, Joao
On 08/05/17 15:40, Joao Pinto wrote: > Às 11:59 AM de 5/8/2017, Marc Zyngier escreveu: >> On 08/05/17 11:24, Joao Pinto wrote: >>> Às 11:22 AM de 5/8/2017, Marc Zyngier escreveu: >>>> On 08/05/17 10:59, Joao Pinto wrote: >>>>> Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >>>>>> Joao, >>>>>> >>>>>> On 05/05/17 15:11, Joao Pinto wrote: >>>>>>> Hello, >>>>>>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>>>>>> I am now facing a dificulty. >>>>>>> >>>>>>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>>>>>> the changes introduced by this patch we are able to enable MSI and MSI-X >>>>>>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>>>>>> >>>>>>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>>>>>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>>>>>> from the endpoint. >>>>>> >>>>>> It is not bogus at all. It is computed from the PCI requester ID in >>>>>> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >>>>>> view of that interrupt, which is completely virtual. >>>>>> >>>>>> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >>>>>> is probably 1 (only you can know that). As for why it doesn't work, see >>>>>> below: >>>>>> >>>>>>> >>>>>>> I would apreciate that more experienced people in this interrupt subject could >>>>>>> give me an hint. >>>> >>>> [...] >>>> >>>>> Yep makes perfectly sense and thanks for clearing the Domains :), I understood >>>>> them now. >>>> >>>> I guess that you have your MSI controller working with MSI-X now? >>> >>> I started working one hour ago and still putting some subjects in order, I am >>> going to return to PCI in a bit. I will keep you updated. I thought you were in >>> US Pacific timezone. >> >> I'm firmly rooted in BST! ;-) >> >> M. >> > > You were right, it was missing the bottom part related to the PCie DW IP: > > static void dw_pci_bottom_mask(struct irq_data *data) > { > struct dw_pcie *pci = irq_data_get_irq_chip_data(data); > struct pcie_port *pp = &pci->pp; > unsigned int res, bit, val; > > res = (data->hwirq / 32) * 12; > bit = data->hwirq % 32; > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val); > val &= ~(1 << bit); > dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val); be careful: you have a horrible race here, where another CPU try to mask another MSI at the same time. Any form of RMW should be guarded by a spinlock. > } > > static void dw_pci_bottom_unmask(struct irq_data *data) > { > struct dw_pcie *pci = irq_data_get_irq_chip_data(data); > struct pcie_port *pp = &pci->pp; > unsigned int res, bit, val; > > res = (data->hwirq / 32) * 12; > bit = data->hwirq % 32; > > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val); > val |= 1 << bit; > dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val); Same here. Also, you could perfectly replace the read with a variable that caches the most recent value (as there should be no other code changing this register). > } > > static struct irq_chip dw_pci_msi_bottom_irq_chip = { > .name = "DW MSI", > .irq_compose_msi_msg = dw_pci_compose_msi_msg, > .irq_set_affinity = dw_pci_msi_set_affinity, > .irq_mask = dw_pci_bottom_mask, > .irq_unmask = dw_pci_bottom_unmask, > }; > > > With these 2 functions I was able to perform test with MSI and with MSI-X > Endpoints. I am goingo to clean this up and check the compatibility with other > SoC drivers and send a patch soon. > > Thanks for the help Marc! No worries. Cheers, M.
Às 3:49 PM de 5/8/2017, Marc Zyngier escreveu: > On 08/05/17 15:40, Joao Pinto wrote: >> Às 11:59 AM de 5/8/2017, Marc Zyngier escreveu: >>> On 08/05/17 11:24, Joao Pinto wrote: >>>> Às 11:22 AM de 5/8/2017, Marc Zyngier escreveu: >>>>> On 08/05/17 10:59, Joao Pinto wrote: >>>>>> Às 3:57 PM de 5/5/2017, Marc Zyngier escreveu: >>>>>>> Joao, >>>>>>> >>>>>>> On 05/05/17 15:11, Joao Pinto wrote: >>>>>>>> Hello, >>>>>>>> I am currently adding the support for both MSI and MSI-x in pcie-designware and >>>>>>>> I am now facing a dificulty. >>>>>>>> >>>>>>>> My test endpoint is a USB 3.1 MSI / MSI-X capable and I tested that with >>>>>>>> the changes introduced by this patch we are able to enable MSI and MSI-X >>>>>>>> in this endpoint (depends on the usage of the MSI_FLAG_PCI_MSIX flag). >>>>>>>> >>>>>>>> The problem I am facing now is that Intc for the USB 3.1 Endpoint is completely >>>>>>>> bogus (524288) when it should be 1, and so I am not receiving any interrupts >>>>>>>> from the endpoint. >>>>>>> >>>>>>> It is not bogus at all. It is computed from the PCI requester ID in >>>>>>> pci_msi_domain_calc_hwirq. What you're seeing is the PCI/MDI domain >>>>>>> view of that interrupt, which is completely virtual. >>>>>>> >>>>>>> The real thing happens in your own irqdomain, where the hwirq for IRQ46 >>>>>>> is probably 1 (only you can know that). As for why it doesn't work, see >>>>>>> below: >>>>>>> >>>>>>>> >>>>>>>> I would apreciate that more experienced people in this interrupt subject could >>>>>>>> give me an hint. >>>>> >>>>> [...] >>>>> >>>>>> Yep makes perfectly sense and thanks for clearing the Domains :), I understood >>>>>> them now. >>>>> >>>>> I guess that you have your MSI controller working with MSI-X now? >>>> >>>> I started working one hour ago and still putting some subjects in order, I am >>>> going to return to PCI in a bit. I will keep you updated. I thought you were in >>>> US Pacific timezone. >>> >>> I'm firmly rooted in BST! ;-) >>> >>> M. >>> >> >> You were right, it was missing the bottom part related to the PCie DW IP: >> >> static void dw_pci_bottom_mask(struct irq_data *data) >> { >> struct dw_pcie *pci = irq_data_get_irq_chip_data(data); >> struct pcie_port *pp = &pci->pp; >> unsigned int res, bit, val; >> >> res = (data->hwirq / 32) * 12; >> bit = data->hwirq % 32; >> >> dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, &val); >> val &= ~(1 << bit); >> dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE + res, 4, val); > > be careful: you have a horrible race here, where another CPU try to mask > another MSI at the same time. Any form of RMW should be guarded by a > spinlock. Yes, I am going to do that. Altera's have locks. Thanks!
diff --git a/drivers/pci/dwc/pcie-designware-host.c b/drivers/pci/dwc/pcie-designware-host.c index 28ed32ba4f1b..d42b8b12f168 100644 --- a/drivers/pci/dwc/pcie-designware-host.c +++ b/drivers/pci/dwc/pcie-designware-host.c @@ -45,12 +45,22 @@ static int dw_pcie_wr_own_conf(struct pcie_port *pp, int where, int size, return dw_pcie_write(pci->dbi_base + where, size, val); } +static void dw_msi_mask_irq(struct irq_data *d) +{ + pci_msi_mask_irq(d); + irq_chip_mask_parent(d); +} + +static void dw_msi_unmask_irq(struct irq_data *d) +{ + pci_msi_unmask_irq(d); + irq_chip_unmask_parent(d); +} + static struct irq_chip dw_msi_irq_chip = { .name = "PCI-MSI", - .irq_enable = pci_msi_unmask_irq, - .irq_disable = pci_msi_mask_irq, - .irq_mask = pci_msi_mask_irq, - .irq_unmask = pci_msi_unmask_irq, + .irq_mask = dw_msi_mask_irq, + .irq_unmask = dw_msi_unmask_irq, }; I haven't dug any further, but this should be fixed first.