Re: [PATCH 2/2] pci: Add PCIe driver for Rockchip Soc

[Marc Zyngier <marc.zyngier@arm.com>]

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.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...