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

Message ID 20190213132629.24790-4-kishon@ti.com
State New
Headers show
Series
  • PCI: DWC/Keystone: MSI configuration cleanup
Related show

Commit Message

Kishon Vijay Abraham I Feb. 13, 2019, 1:26 p.m.
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(-)

-- 
2.17.1

Comments

Lorenzo Pieralisi Feb. 13, 2019, 4:57 p.m. | #1
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.

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.

Thanks,
Lorenzo

> +		}

>  

>  		irq_set_chained_handler_and_data(irq,

>  						 ks_pcie_legacy_irq_handler,

> -- 

> 2.17.1

>
Kishon Vijay Abraham I Feb. 14, 2019, 4:57 a.m. | #2
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].
> 

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

Thanks
Kishon
Lorenzo Pieralisi Feb. 14, 2019, 10:31 a.m. | #3
[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
Marc Zyngier Feb. 14, 2019, 12:46 p.m. | #4
Hi Lirenzo,

On Thu, 14 Feb 2019 10:31:38 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 

> [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).


It isn't pretty, but it is something we already do in some cases (see
irq-gic-v3-mbi.c::mbi_compose_msi_msg as an infamous example).

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.
Kishon Vijay Abraham I Feb. 21, 2019, 9:18 a.m. | #5
Hi Lorenzo,

On 14/02/19 4:01 PM, Lorenzo Pieralisi wrote:
> [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.


I got legacy interrupt working using hierarchy domain. With that we don't need
to have chained irq handler here. I'll post v4 soon.

Thanks
Kishon

Patch

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;
 	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;
+		}
 
 		irq_set_chained_handler_and_data(irq,
 						 ks_pcie_legacy_irq_handler,