Re: [PATCH v3 3/9] PCI: keystone: Use hwirq to get the legacy IRQ number offset

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

[CC'ed MarcZ]

On Thu, Feb 14, 2019 at 10:27:19AM +0530, Kishon Vijay Abraham I wrote:
> Hi Lorenzo,
> 
> On 13/02/19 10:27 PM, Lorenzo Pieralisi wrote:
> > On Wed, Feb 13, 2019 at 06:56:23PM +0530, Kishon Vijay Abraham I wrote:
> >> ks_pcie_legacy_irq_handler() uses 'virq' to get the IRQ number offset.
> >> This offset is used to get the correct IRQ_STATUS register
> >> corresponding to the IRQ line that raised the interrupt.
> >> There is no guarantee that 'virq' assigned for consecutive hardware
> >> IRQ will be contiguous. And this might get us an incorrect IRQ number
> >> offset.
> >>
> >> Fix it here by using 'hwirq' to get the IRQ number offset.
> >>
> >> Link: https://lkml.kernel.org/r/bb081d21-7c03-0357-4294-7e92d95d838c@arm.com
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/controller/dwc/pci-keystone.c | 17 +++++++++++++----
> >>  1 file changed, 13 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> >> index e8b1d8eca78e..d35ac712a9f8 100644
> >> --- a/drivers/pci/controller/dwc/pci-keystone.c
> >> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> >> @@ -87,7 +87,7 @@ struct keystone_pcie {
> >>  	struct dw_pcie		*pci;
> >>  	/* PCI Device ID */
> >>  	u32			device_id;
> >> -	int			legacy_host_irqs[PCI_NUM_INTX];
> >> +	int			legacy_host_irq;
> >>  	struct			device_node *legacy_intc_np;
> >>  
> >>  	int			msi_host_irqs[MAX_MSI_HOST_IRQS];
> >> @@ -582,11 +582,11 @@ static void ks_pcie_msi_irq_handler(struct irq_desc *desc)
> >>   */
> >>  static void ks_pcie_legacy_irq_handler(struct irq_desc *desc)
> >>  {
> >> -	unsigned int irq = irq_desc_get_irq(desc);
> >> +	unsigned int irq = desc->irq_data.hwirq;
> >>  	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];
> >> +	u32 irq_offset = irq - ks_pcie->legacy_host_irq;
> > 
> > I think you should use the plain hwirq number (that if I understand
> > correctly range in [0,3]) and drop legacy_host_irq.
> 
> The hwirq is [80, 83] for Keystone. We store legacy_host_irq (for Keystone it
> is 80) to get the correct offset in the range [0, 3].

IIUC the _parent_ hw_irq number should be what you need here, Marc (if
he has time) can correct me if I am wrong (or what I am suggesting is
a violation of IRQ domain bstraction usage).

> > 
> > See below.
> > 
> >>  	struct irq_chip *chip = irq_desc_get_chip(desc);
> >>  
> >>  	dev_dbg(dev, ": Handling legacy irq %d\n", irq);
> >> @@ -657,6 +657,7 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >>  	struct device_node *np = ks_pcie->np;
> >>  	struct device_node *intc_np;
> >>  	int irq_count, irq, ret, i;
> >> +	struct irq_data *irq_data;
> >>  
> >>  	intc_np = of_get_child_by_name(np, "legacy-interrupt-controller");
> >>  	if (!intc_np) {
> >> @@ -677,7 +678,15 @@ static int ks_pcie_config_legacy_irq(struct keystone_pcie *ks_pcie)
> >>  			ret = -EINVAL;
> >>  			goto err;
> >>  		}
> >> -		ks_pcie->legacy_host_irqs[i] = irq;
> >> +
> >> +		if (!ks_pcie->legacy_host_irq) {
> >> +			irq_data = irq_get_irq_data(irq);
> >> +			if (!irq_data) {
> >> +				ret = -EINVAL;
> >> +				goto err;
> >> +			}
> >> +			ks_pcie->legacy_host_irq = irq_data->hwirq;
> > 
> > This depends on DT property ordering and may well be wrong. On top
> > of that as I said above I do not think it is even needed.
> 
> right, it depends on the ordering of the entries in the "interrupts" property.
> Right now we have something like below
> 
>                                interrupts = <GIC_SPI 48 IRQ_TYPE_EDGE_RISING>,
>                                         <GIC_SPI 49 IRQ_TYPE_EDGE_RISING>,
>                                         <GIC_SPI 50 IRQ_TYPE_EDGE_RISING>,
>                                         <GIC_SPI 51 IRQ_TYPE_EDGE_RISING>;
> 
> And changing the order will make legacy_host_irq store the wrong hwirq. With
> this patch we solve the problem of assuming virq to be contiguous for
> contiguous hwirq however we do expect the hwirqs to be populated in the DT in
> the correct order.
> 
> Ordering entries within a property is also followed in other
> properties like "reg" where the drivers use platform_get_resource with
> index as argument. I'm not sure if there's a way to get rid of the
> ordering requirement in DT for the "interrupts" property.

I am not sure there is an ordering requirement in place but hopefully we
should not need it, see above.

The 1:1 relationship between GIC-IRQ<->PCI-host-bridge-controller-IRQ is
what the kernel IRQ domain abstraction is there to implement, so a
reverse look-up should be feasible without adding anything else, lest
we duplicate IRQ domain data structures.

Thanks,
Lorenzo