? 2016/6/1 16:34, Marc Zyngier ??:
> On 01/06/16 03:56, Wenrui Li wrote:
>> Hi:
>>
>> On 2016/5/27 20:25, Marc Zyngier Wrote:
>>> [+Lorenzo]
>>>
>>> On 20/05/16 11:29, Shawn Lin wrote:
>>>> RK3399 has a PCIe controller which can be used as Root Complex.
>>>> This driver supports a PCIe controller as Root Complex mode.
>>>>
>>
>> [....]
>>
>>>> +static int rockchip_pcie_init_irq_domain(struct rockchip_pcie_port *pp)
>>>> +{
>>>> + struct device *dev = pp->dev;
>>>> + struct device_node *node = dev->of_node;
>>>> + struct device_node *pcie_intc_node = of_get_next_child(node, NULL);
>>>
>>> That's really ugly, as it depends on the layout of your DT.
>>>
>>>> +
>>>> + if (!pcie_intc_node) {
>>>> + dev_err(dev, "No PCIe Intc node found\n");
>>>> + return PTR_ERR(pcie_intc_node);
>>>> + }
>>>> + pp->irq_domain = irq_domain_add_linear(pcie_intc_node, 4,
>>>> + &intx_domain_ops, pp);
>>>
>>> Why can't you just register your host controller as the interrupt
>>> controller? You don't need an intermediate node for that.
>>
>> OK, the child node is really no need here, we will use the host
>> controller as interrupt controller next patch. Thanks!
>>
>>>
>>>> + if (!pp->irq_domain) {
>>>> + dev_err(dev, "Failed to get a INTx IRQ domain\n");
>>>> + return PTR_ERR(pp->irq_domain);
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static irqreturn_t rockchip_pcie_subsys_irq_handler(int irq, void *arg)
>>>> +{
>>>> + struct rockchip_pcie_port *pp = arg;
>>>> + u32 reg;
>>>> + u32 sub_reg;
>>>> +
>>>> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>>>> + if (reg & LOC_INT) {
>>>> + dev_dbg(pp->dev, "local interrupt recived\n");
>>>> + sub_reg = pcie_read(pp, PCIE_CORE_INT_STATUS);
>>>> + if (sub_reg & PRFPE)
>>>> + dev_dbg(pp->dev, "Parity error detected while reading from the PNP Receive FIFO RAM\n");
>>>> +
>>>> + if (sub_reg & CRFPE)
>>>> + dev_dbg(pp->dev, "Parity error detected while reading from the Completion Receive FIFO RAM\n");
>>>> +
>>>> + if (sub_reg & RRPE)
>>>> + dev_dbg(pp->dev, "Parity error detected while reading from Replay Buffer RAM\n");
>>>> +
>>>> + if (sub_reg & PRFO)
>>>> + dev_dbg(pp->dev, "Overflow occurred in the PNP Receive FIFO\n");
>>>> +
>>>> + if (sub_reg & CRFO)
>>>> + dev_dbg(pp->dev, "Overflow occurred in the Completion Receive FIFO\n");
>>>> +
>>>> + if (sub_reg & RT)
>>>> + dev_dbg(pp->dev, "Replay timer timed out\n");
>>>> +
>>>> + if (sub_reg & RTR)
>>>> + dev_dbg(pp->dev, "Replay timer rolled over after 4 transmissions of the same TLP\n");
>>>> +
>>>> + if (sub_reg & PE)
>>>> + dev_dbg(pp->dev, "Phy error detected on receive side\n");
>>>> +
>>>> + if (sub_reg & MTR)
>>>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>>>> +
>>>> + if (sub_reg & UCR)
>>>> + dev_dbg(pp->dev, "Malformed TLP received from the link\n");
>>>> +
>>>> + if (sub_reg & FCE)
>>>> + dev_dbg(pp->dev, "An error was observed in the flow control advertisements from the other side\n");
>>>> +
>>>> + if (sub_reg & CT)
>>>> + dev_dbg(pp->dev, "A request timed out waiting for completion\n");
>>>> +
>>>> + if (sub_reg & UTC)
>>>> + dev_dbg(pp->dev, "Unmapped TC error\n");
>>>> +
>>>> + if (sub_reg & MMVC)
>>>> + dev_dbg(pp->dev, "MSI mask register changes\n");
>>>> +
>>>> + pcie_write(pp, sub_reg, PCIE_CORE_INT_STATUS);
>>>> + }
>>>> +
>>>> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>>>> +
>>>> + return IRQ_HANDLED;
>>>> +}
>>
>> [....]
>>
>>>> +static irqreturn_t rockchip_pcie_legacy_int_handler(int irq, void *arg)
>>>> +{
>>>> + struct rockchip_pcie_port *pp = arg;
>>>> + u32 reg;
>>>> +
>>>> + reg = pcie_read(pp, PCIE_CLIENT_INT_STATUS);
>>>> + reg = (reg & ROCKCHIP_PCIE_RPIFR1_INTR_MASK) >>
>>>> + ROCKCHIP_PCIE_RPIFR1_INTR_SHIFT;
>>>> + generic_handle_irq(irq_find_mapping(pp->irq_domain, ffs(reg)));
>>>
>>> NAK. What you have here is a chained interrupt controller, please
>>> implement it as such.
>>
>> Your mean is use handle_nested_irq instead of generic_handle_irq here?
>
> No, handle_nested_irq() is for threaded interrupts. What I mean is that
> you need to configure the interrupt as a cascaded interrupt, and use the
> chained_irq_enter/exit helpers. As a rule of thumb, if you're calling
> generic_handle_irq() in an interrupt handler, you're doing something wrong.
>
> If you don't do that, you may end up with interrupts that are not EOIed
> properly, RCU violations, and double accounting of interrupts.
>
>> But, I found all other pci host controller drivers use this api.
>
> I'm afraid that doesn't make it any better. Buggy code is everywhere,
> and it does spread faster than properly written code. Have a look at
> drivers/pci/host/pcie-altera.c as an example of what it should look like.
>
I get you mean, we will fix it, thanks!
> Thanks,
>
> M.
>
