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

[Wenrui Li]

在 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.
>