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?
But, I found all other pci host controller drivers use this api.
>
>> +
>> + pcie_write(pp, reg, PCIE_CLIENT_INT_STATUS);
>> + return IRQ_HANDLED;
>> +}
>> +
[...]
>> +static struct platform_driver rockchip_pcie_driver = {
>> + .driver = {
>> + .name = "rockchip-pcie",
>> + .of_match_table = rockchip_pcie_of_match,
>> + .suppress_bind_attrs = true,
>> + },
>> + .probe = rockchip_pcie_probe,
>> + .remove = rockchip_pcie_remove,
>> +};
>> +module_platform_driver(rockchip_pcie_driver);
>> +
>> +MODULE_AUTHOR("Rockchip Inc");
>> +MODULE_DESCRIPTION("Rockchip AXI PCIe driver");
>> +MODULE_LICENSE("GPL v2");
>>
>
> This definitely requires some rework for both the interrupt and MSI
> parts. I'll leave Lorenzo to comment on the PCI side of things.
>
> Thanks,
>
> M.
>
Best Regards,
Li
