On 28/09/18 12:06, Lorenzo Pieralisi wrote:
[+Murali, Marc]
On Thu, Sep 27, 2018 at 04:44:26PM +0900, Kunihiko Hayashi wrote:
Hi Lorenzo, Gustavo,
On Wed, 26 Sep 2018 21:31:36 +0900 <hayashi.kunihiko@xxxxxxxxxxxxx> wrote:
Hi Lorenzo, Gustavo,
Thank you for reviewing.
On Tue, 25 Sep 2018 18:53:01 +0100
Gustavo Pimentel <gustavo.pimentel@xxxxxxxxxxxx> wrote:
On 25/09/2018 17:14, Lorenzo Pieralisi wrote:
[+Gustavo, please have a look at INTX/MSI management]
On Thu, Sep 06, 2018 at 06:40:32PM +0900, Kunihiko Hayashi wrote:
This introduces specific glue layer for UniPhier platform to support
PCIe host controller that is based on the DesignWare PCIe core, and
this driver supports Root Complex (host) mode.
Please read this thread and apply it to next versions:
https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Dpci-26m-3D150905742808166-26w-3D2&d=DwIBAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=bkWxpLoW-f-E3EdiDCCa0_h0PicsViasSlvIpzZvPxs&m=H8UNDDUGQnQnqfWr4CBios689dJcjxu4qeTTRGulLmU&s=CgcXc_2LThyOpW-4bCriJNo9H1lzROEdy_cG9p-Y5hU&e=
I also found this thread in previous linux-pci, and I think it's helpful for me.
I'll check it carefully.
[snip]
+ ret = devm_request_irq(dev, pp->irq, uniphier_pcie_irq_handler,
+ IRQF_SHARED, "pcie", priv);
This is wrong, you should set-up a chained IRQ for INTX.
I *think* that
ks_pcie_setup_interrupts()
is a good example to start with but I wonder whether it is worth
generalizing the INTX approach to designware as a whole as it was
done for MSIs.
Thoughts ?
From what I understood this is for legacy IRQ, right?
Yes. For legacy IRQ.
Like you (Lorenzo) said there is 2 drivers (pci-keystone-dw.c and pci-dra7xx.c)
that uses it and can be use as a template for handling this type of interrupts.
We can try to pass some kind of generic INTX function to the DesignWare host
library to handling this, but this will require some help from keystone and
dra7xx maintainers, since my setup doesn't have legacy IRQ HW support.
Now I think it's difficult to make a template for INTX function,
and at first, I'll try to re-write this part with reference to pci-keystone-dw.c.
I understand that there are 2 types of interrupt and the drivers.
One like pci-keystone-dw.c is:
- there are 4 interrupts for legacy,
- invoke handlers for each interrupt, and handle the interrupt,
- call irq_set_chained_handler_and_data() to make a chain of the interrupts
when initializing
The other like pci-dra7xx.c is:
- there is 1 IRQ for legacy as a parent,
- check an interrupt factor register, and handle the interrupt correspond
to the factor,
- call request_irq() for the parent IRQ and irq_domain_add_linear() for
the factor when initializing
The pcie-uniphier.c is the same type as the latter (like pci-dra7xx.c).
However, in pci-dra7xx.c, MSI and legacy IRQ share the same interrupt number,
so the same handler is called and the handler divides these IRQs.
(found in dra7xx_pcie_msi_irq_handler())
In pcie-uniphier.c, MSI and legacy IRQ are independent.
Therefore it's necessary to prepare the handler for the legacy IRQ.
I think that it's difficult to apply the way of pci-keystone-dw.c, and
uniphier_pcie_irq_handler() and calling devm_request_irq() are still
necessary to handle legacy IRQ.
I do not think it is difficult, the difference is that keystone has
1 GIC irq line allocated per legacy IRQ, your set-up has one for
all INTX.
*However*, I would like some clarifications from Murali on this code
in drivers/pci/controller/dwc/pci-keystone.c:
static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
{
unsigned int irq = irq_desc_get_irq(desc);
struct keystone_pcie *ks_pcie = irq_desc_get_handler_data(desc);
struct dw_pcie *pci = ks_pcie->pci;
struct device *dev = pci->dev;
u32 irq_offset = irq - ks_pcie->legacy_host_irqs[0];
Here the IRQ numbers are virtual IRQs, is it correct to consider
the virq numbers as sequential values ? The "offset" is used to
handle the PCI controller interrupt registers, so it must be a value
between 0-3 IIUC.
There is absolutely no reason why virtual interrupt numbers should be
contiguous. Shake the allocator hard enough, and you'll see gaps appearing.
In general, the only thing that makes sense is to compute this offset
based on the hwirq which is HW-specific.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
