diff mbox

[V2,3/5] PCI: xilinx: Modifying AXI PCIe Host Bridge driver to work on both Zynq and Microblaze

Message ID 1452620173-4905-4-git-send-email-bharatku@xilinx.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bharat Kumar Gogada Jan. 12, 2016, 5:36 p.m. UTC
Modifying Xilinx AXI PCIe Host Bridge Soft IP driver to work on both
Zynq and Microblaze Architectures.
With these modifications drivers/pci/host/pcie-xilinx.c, will
work on both Zynq and Microblaze Architectures.

Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
---
Changes:
Changed Total number of MSI IRQ count logic according to both architectures.
Updated MSI assigning functions accordingly to new count.
Modified irq_domain_add_linear with new MSI IRQ count.
Added #ifdef to pci_fixup_irqs which is ARM specific API.
---
 drivers/pci/host/pcie-xilinx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

Comments

Arnd Bergmann Jan. 12, 2016, 10:27 p.m. UTC | #1
On Tuesday 12 January 2016 23:06:11 Bharat Kumar Gogada wrote:
> Modifying Xilinx AXI PCIe Host Bridge Soft IP driver to work on both
> Zynq and Microblaze Architectures.
> With these modifications drivers/pci/host/pcie-xilinx.c, will
> work on both Zynq and Microblaze Architectures.
> 
> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>

I think this patch should be split into three, as you are doing three
unrelated things here.

> ---
> Changes:
> Changed Total number of MSI IRQ count logic according to both architectures.
> Updated MSI assigning functions accordingly to new count.
> Modified irq_domain_add_linear with new MSI IRQ count.
> Added #ifdef to pci_fixup_irqs which is ARM specific API.
> ---
>  drivers/pci/host/pcie-xilinx.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> index 3e3757f..1981948 100644
> --- a/drivers/pci/host/pcie-xilinx.c
> +++ b/drivers/pci/host/pcie-xilinx.c
> @@ -92,7 +92,12 @@
>  #define ECAM_DEV_NUM_SHIFT		12
>  
>  /* Number of MSI IRQs */
> -#define XILINX_NUM_MSI_IRQS		128
> +#define XILINX_NUM_MSI_IRQS	128
> +#ifdef CONFIG_ARM
> +#define TOT_NR_IRQS		XILINX_NUM_MSI_IRQS
> +#else
> +#define TOT_NR_IRQS		(NR_IRQS + XILINX_NUM_MSI_IRQS)
> +#endif

Something looks wrong here in the microblaze variant. What does NR_IRQS
have to do with it?

> @@ -238,15 +243,20 @@ static void xilinx_pcie_destroy_msi(unsigned int irq)
>   */
>  static int xilinx_pcie_assign_msi(struct xilinx_pcie_port *port)
>  {
> +	int irq;
>  	int pos;
>  
>  	pos = find_first_zero_bit(msi_irq_in_use, XILINX_NUM_MSI_IRQS);
> -	if (pos < XILINX_NUM_MSI_IRQS)
> +	irq = pos;
> +#ifdef CONFIG_MICROBLAZE
> +	irq = XILINX_NUM_MSI_IRQS + pos;
> +#endif


	if (IS_ENABLED(CONFIG_MICROBLAZE))
		irq = XILINX_NUM_MSI_IRQS + pos;

> @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct platform_device *pdev)
>  #endif
>  	pci_scan_child_bus(bus);
>  	pci_assign_unassigned_bus_resources(bus);
> +#ifdef CONFIG_ARM
>  	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> +#endif
>  	pci_bus_add_devices(bus);
>  	platform_set_drvdata(pdev, port);

Here it looks like microblaze gets it right. I'm not sure why we still
need the pci_fixup_irqs() on ARM, but my feeling is that this should be
fixed in common code.

	Arnd
Michal Simek Jan. 26, 2016, 9:59 a.m. UTC | #2
On 12.1.2016 23:27, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 23:06:11 Bharat Kumar Gogada wrote:
>> Modifying Xilinx AXI PCIe Host Bridge Soft IP driver to work on both
>> Zynq and Microblaze Architectures.
>> With these modifications drivers/pci/host/pcie-xilinx.c, will
>> work on both Zynq and Microblaze Architectures.
>>
>> Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
>> Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> 
> I think this patch should be split into three, as you are doing three
> unrelated things here.
> 
>> ---
>> Changes:
>> Changed Total number of MSI IRQ count logic according to both architectures.
>> Updated MSI assigning functions accordingly to new count.
>> Modified irq_domain_add_linear with new MSI IRQ count.
>> Added #ifdef to pci_fixup_irqs which is ARM specific API.
>> ---
>>  drivers/pci/host/pcie-xilinx.c | 22 +++++++++++++++++-----
>>  1 file changed, 17 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
>> index 3e3757f..1981948 100644
>> --- a/drivers/pci/host/pcie-xilinx.c
>> +++ b/drivers/pci/host/pcie-xilinx.c
>> @@ -92,7 +92,12 @@
>>  #define ECAM_DEV_NUM_SHIFT		12
>>  
>>  /* Number of MSI IRQs */
>> -#define XILINX_NUM_MSI_IRQS		128
>> +#define XILINX_NUM_MSI_IRQS	128
>> +#ifdef CONFIG_ARM
>> +#define TOT_NR_IRQS		XILINX_NUM_MSI_IRQS
>> +#else
>> +#define TOT_NR_IRQS		(NR_IRQS + XILINX_NUM_MSI_IRQS)
>> +#endif
> 
> Something looks wrong here in the microblaze variant. What does NR_IRQS
> have to do with it?

Arnd: What was the story regarding NR_IRQS?
I remember some discussion about it but just forget.

Default value in include/asm-generic/irq.h is 64.
Current value is 32 because microblaze primary interrupt controller is
axi_intc core which has up to 32 input lines.

Thanks,
Michal
Arnd Bergmann Jan. 26, 2016, 12:11 p.m. UTC | #3
On Tuesday 26 January 2016 10:59:12 Michal Simek wrote:
> >> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
> >> index 3e3757f..1981948 100644
> >> --- a/drivers/pci/host/pcie-xilinx.c
> >> +++ b/drivers/pci/host/pcie-xilinx.c
> >> @@ -92,7 +92,12 @@
> >>  #define ECAM_DEV_NUM_SHIFT          12
> >>  
> >>  /* Number of MSI IRQs */
> >> -#define XILINX_NUM_MSI_IRQS         128
> >> +#define XILINX_NUM_MSI_IRQS 128
> >> +#ifdef CONFIG_ARM
> >> +#define TOT_NR_IRQS         XILINX_NUM_MSI_IRQS
> >> +#else
> >> +#define TOT_NR_IRQS         (NR_IRQS + XILINX_NUM_MSI_IRQS)
> >> +#endif
> > 
> > Something looks wrong here in the microblaze variant. What does NR_IRQS
> > have to do with it?
> 
> Arnd: What was the story regarding NR_IRQS?
> I remember some discussion about it but just forget.
> 
> Default value in include/asm-generic/irq.h is 64.
> Current value is 32 because microblaze primary interrupt controller is
> axi_intc core which has up to 32 input lines.

The value in asm-generic is completely arbitrary, it's just something
that happens to work for a number of the simpler architectures.

Traditionally, there is a a fixed NR_IRQS which defines the maximum
number of interrupts that can be used, and each irqchip has a fixed
start offset below that number.

On modern systems, you have CONFIG_SPARSE_IRQ, which lets an irqchip
allocate its own interrupts, without an upper limit. This is more
flexible and avoids preallocating space for all irq_desc instances,
so it saves memory.

This code however doesn't do either of the two on microblaze:

+       irq = pos;
+#ifdef CONFIG_MICROBLAZE
+#define TOT_NR_IRQS            (NR_IRQS + XILINX_NUM_MSI_IRQS)
+       irq = XILINX_NUM_MSI_IRQS + pos;
+#endif
+       if (irq < TOT_NR_IRQS)
                set_bit(pos, msi_irq_in_use);

So you define XILINX_NUM_MSI_IRQS to mean the number of interrupts
that the xilinx_pcie_port can handle itself, but then pick a number
outside of this range by making the hwirq number something between
XILINX_NUM_MSI_IRQS and (2*XILINX_NUM_MSI_IRQS - 1), and in the
end compare it to (NR_IRQS + XILINX_NUM_MSI_IRQS), which is the
sum of two things that are not related: the total number of interrupts
including the MSIs and the number of MSI.

	Arnd
Michal Simek Jan. 26, 2016, 3:21 p.m. UTC | #4
On 26.1.2016 13:11, Arnd Bergmann wrote:
> On Tuesday 26 January 2016 10:59:12 Michal Simek wrote:
>>>> diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
>>>> index 3e3757f..1981948 100644
>>>> --- a/drivers/pci/host/pcie-xilinx.c
>>>> +++ b/drivers/pci/host/pcie-xilinx.c
>>>> @@ -92,7 +92,12 @@
>>>>  #define ECAM_DEV_NUM_SHIFT          12
>>>>  
>>>>  /* Number of MSI IRQs */
>>>> -#define XILINX_NUM_MSI_IRQS         128
>>>> +#define XILINX_NUM_MSI_IRQS 128
>>>> +#ifdef CONFIG_ARM
>>>> +#define TOT_NR_IRQS         XILINX_NUM_MSI_IRQS
>>>> +#else
>>>> +#define TOT_NR_IRQS         (NR_IRQS + XILINX_NUM_MSI_IRQS)
>>>> +#endif
>>>
>>> Something looks wrong here in the microblaze variant. What does NR_IRQS
>>> have to do with it?
>>
>> Arnd: What was the story regarding NR_IRQS?
>> I remember some discussion about it but just forget.
>>
>> Default value in include/asm-generic/irq.h is 64.
>> Current value is 32 because microblaze primary interrupt controller is
>> axi_intc core which has up to 32 input lines.
> 
> The value in asm-generic is completely arbitrary, it's just something
> that happens to work for a number of the simpler architectures.
> 
> Traditionally, there is a a fixed NR_IRQS which defines the maximum
> number of interrupts that can be used, and each irqchip has a fixed
> start offset below that number.
> 
> On modern systems, you have CONFIG_SPARSE_IRQ, which lets an irqchip
> allocate its own interrupts, without an upper limit. This is more
> flexible and avoids preallocating space for all irq_desc instances,
> so it saves memory.

ok. That was the story. I will look if there is any issue to enable
SPARSE_IRQ for Microblaze.

I also need to move intc driver out of arch.

Thanks,
Michal
Bharat Kumar Gogada Jan. 27, 2016, 2:33 p.m. UTC | #5
> Subject: Re: [PATCH V2 3/5] PCI: xilinx: Modifying AXI PCIe Host Bridge driver
> to work on both Zynq and Microblaze
> 
> On Tuesday 12 January 2016 23:06:11 Bharat Kumar Gogada wrote:
> > Modifying Xilinx AXI PCIe Host Bridge Soft IP driver to work on both
> > Zynq and Microblaze Architectures.
> > With these modifications drivers/pci/host/pcie-xilinx.c, will work on
> > both Zynq and Microblaze Architectures.
> >
> > Signed-off-by: Bharat Kumar Gogada <bharatku@xilinx.com>
> > Signed-off-by: Ravi Kiran Gummaluri <rgummal@xilinx.com>
> 
> I think this patch should be split into three, as you are doing three unrelated
> things here.
> 
Agreed, will send as different patches in next series.
> > ---
> > --- a/drivers/pci/host/pcie-xilinx.c
> > +++ b/drivers/pci/host/pcie-xilinx.c
> > @@ -92,7 +92,12 @@
> >  #define ECAM_DEV_NUM_SHIFT		12
> >
> >  /* Number of MSI IRQs */
> > -#define XILINX_NUM_MSI_IRQS		128
> > +#define XILINX_NUM_MSI_IRQS	128
> > +#ifdef CONFIG_ARM
> > +#define TOT_NR_IRQS		XILINX_NUM_MSI_IRQS
> > +#else
> > +#define TOT_NR_IRQS		(NR_IRQS + XILINX_NUM_MSI_IRQS)
> > +#endif
> 
> Something looks wrong here in the microblaze variant. What does NR_IRQS
> have to do with it?
> 
> > @@ -238,15 +243,20 @@ static void xilinx_pcie_destroy_msi(unsigned int
> irq)
> >   */
> >  static int xilinx_pcie_assign_msi(struct xilinx_pcie_port *port)  {
> > +	int irq;
> >  	int pos;
> >
> >  	pos = find_first_zero_bit(msi_irq_in_use, XILINX_NUM_MSI_IRQS);
> > -	if (pos < XILINX_NUM_MSI_IRQS)
> > +	irq = pos;
> > +#ifdef CONFIG_MICROBLAZE
> > +	irq = XILINX_NUM_MSI_IRQS + pos;
> > +#endif
> 
> 
> 	if (IS_ENABLED(CONFIG_MICROBLAZE))
> 		irq = XILINX_NUM_MSI_IRQS + pos;
> 
Agreed.
> > @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct
> > platform_device *pdev)  #endif
> >  	pci_scan_child_bus(bus);
> >  	pci_assign_unassigned_bus_resources(bus);
> > +#ifdef CONFIG_ARM
> >  	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > +#endif
> >  	pci_bus_add_devices(bus);
> >  	platform_set_drvdata(pdev, port);
> 
> Here it looks like microblaze gets it right. I'm not sure why we still need the
> pci_fixup_irqs() on ARM, but my feeling is that this should be fixed in
> common code.
In arm pci_fixup_irqs is called by pci_common_init_dev (arch/arm/kernel/bios32.c), since this API is removed now, I was calling it separately.

Bharat
Bharat Kumar Gogada Jan. 27, 2016, 2:41 p.m. UTC | #6
> 
> On Tuesday 26 January 2016 10:59:12 Michal Simek wrote:
> > >> diff --git a/drivers/pci/host/pcie-xilinx.c
> > >> b/drivers/pci/host/pcie-xilinx.c index 3e3757f..1981948 100644
> > >> --- a/drivers/pci/host/pcie-xilinx.c
> > >> +++ b/drivers/pci/host/pcie-xilinx.c
> > >> @@ -92,7 +92,12 @@
> > >>  #define ECAM_DEV_NUM_SHIFT          12
> > >>
> > >>  /* Number of MSI IRQs */
> > >> -#define XILINX_NUM_MSI_IRQS         128
> > >> +#define XILINX_NUM_MSI_IRQS 128
> > >> +#ifdef CONFIG_ARM
> > >> +#define TOT_NR_IRQS         XILINX_NUM_MSI_IRQS
> > >> +#else
> > >> +#define TOT_NR_IRQS         (NR_IRQS + XILINX_NUM_MSI_IRQS)
> > >> +#endif
> > >
> > > Something looks wrong here in the microblaze variant. What does
> > > NR_IRQS have to do with it?
> >
> > Arnd: What was the story regarding NR_IRQS?
> > I remember some discussion about it but just forget.
> >
> > Default value in include/asm-generic/irq.h is 64.
> > Current value is 32 because microblaze primary interrupt controller is
> > axi_intc core which has up to 32 input lines.
> 
> The value in asm-generic is completely arbitrary, it's just something that
> happens to work for a number of the simpler architectures.
> 
> Traditionally, there is a a fixed NR_IRQS which defines the maximum number
> of interrupts that can be used, and each irqchip has a fixed start offset below
> that number.
> 
> On modern systems, you have CONFIG_SPARSE_IRQ, which lets an irqchip
> allocate its own interrupts, without an upper limit. This is more flexible and
> avoids preallocating space for all irq_desc instances, so it saves memory.
> 
> This code however doesn't do either of the two on microblaze:
> 
> +       irq = pos;
> +#ifdef CONFIG_MICROBLAZE
> +#define TOT_NR_IRQS            (NR_IRQS + XILINX_NUM_MSI_IRQS)
> +       irq = XILINX_NUM_MSI_IRQS + pos; #endif
> +       if (irq < TOT_NR_IRQS)
>                 set_bit(pos, msi_irq_in_use);
> 
> So you define XILINX_NUM_MSI_IRQS to mean the number of interrupts
> that the xilinx_pcie_port can handle itself, but then pick a number
> outside of this range by making the hwirq number something between
> XILINX_NUM_MSI_IRQS and (2*XILINX_NUM_MSI_IRQS - 1), and in the
> end compare it to (NR_IRQS + XILINX_NUM_MSI_IRQS), which is the
> sum of two things that are not related: the total number of interrupts
> including the MSIs and the number of MSI.
> 
Agreed, I have crosschecked something's which I missed previously, NR_IRQS have nothing to do with the count, and also no need to add, irq = XILINX_NUM_MSI_IRQS + pos; in microblaze, I will remove these checks in next patch series.
Arnd Bergmann Jan. 27, 2016, 3:14 p.m. UTC | #7
On Wednesday 27 January 2016 14:33:45 Bharat Kumar Gogada wrote:
> > > @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct
> > > platform_device *pdev)  #endif
> > >     pci_scan_child_bus(bus);
> > >     pci_assign_unassigned_bus_resources(bus);
> > > +#ifdef CONFIG_ARM
> > >     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > +#endif
> > >     pci_bus_add_devices(bus);
> > >     platform_set_drvdata(pdev, port);
> > 
> > Here it looks like microblaze gets it right. I'm not sure why we still need the
> > pci_fixup_irqs() on ARM, but my feeling is that this should be fixed in
> > common code.
> In arm pci_fixup_irqs is called by pci_common_init_dev (arch/arm/kernel/bios32.c), since this API is removed now, I was calling it separately.

But who calls it in microblaze? If it works without the extra call there,
can we make it work the same way for ARM?

	Arnd
Bharat Kumar Gogada Jan. 28, 2016, 1:20 p.m. UTC | #8
> Subject: Re: [PATCH V2 3/5] PCI: xilinx: Modifying AXI PCIe Host Bridge driver
> to work on both Zynq and Microblaze
> 
> On Wednesday 27 January 2016 14:33:45 Bharat Kumar Gogada wrote:
> > > > @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct
> > > > platform_device *pdev)  #endif
> > > >     pci_scan_child_bus(bus);
> > > >     pci_assign_unassigned_bus_resources(bus);
> > > > +#ifdef CONFIG_ARM
> > > >     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > > +#endif
> > > >     pci_bus_add_devices(bus);
> > > >     platform_set_drvdata(pdev, port);
> > >
> > > Here it looks like microblaze gets it right. I'm not sure why we
> > > still need the
> > > pci_fixup_irqs() on ARM, but my feeling is that this should be fixed
> > > in common code.
> > In arm pci_fixup_irqs is called by pci_common_init_dev
> (arch/arm/kernel/bios32.c), since this API is removed now, I was calling it
> separately.
> 
> But who calls it in microblaze? If it works without the extra call there, can we
> make it work the same way for ARM?
> 
In microblaze I have added pcibios_add_device call (similar to call in arch/arm64/kernel/pci.c ) in pci-common.c, which is being invoked by kernel core itself. May be we can add similar on arm and test out, but we might need some cleanup in arch/arm/kernel/bios32.c

Bharat
Arnd Bergmann Jan. 28, 2016, 1:49 p.m. UTC | #9
On Thursday 28 January 2016 13:20:56 Bharat Kumar Gogada wrote:
> > Subject: Re: [PATCH V2 3/5] PCI: xilinx: Modifying AXI PCIe Host Bridge driver
> > to work on both Zynq and Microblaze
> > 
> > On Wednesday 27 January 2016 14:33:45 Bharat Kumar Gogada wrote:
> > > > > @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct
> > > > > platform_device *pdev)  #endif
> > > > >     pci_scan_child_bus(bus);
> > > > >     pci_assign_unassigned_bus_resources(bus);
> > > > > +#ifdef CONFIG_ARM
> > > > >     pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
> > > > > +#endif
> > > > >     pci_bus_add_devices(bus);
> > > > >     platform_set_drvdata(pdev, port);
> > > >
> > > > Here it looks like microblaze gets it right. I'm not sure why we
> > > > still need the
> > > > pci_fixup_irqs() on ARM, but my feeling is that this should be fixed
> > > > in common code.
> > > In arm pci_fixup_irqs is called by pci_common_init_dev
> > (arch/arm/kernel/bios32.c), since this API is removed now, I was calling it
> > separately.
> > 
> > But who calls it in microblaze? If it works without the extra call there, can we
> > make it work the same way for ARM?
> > 
> In microblaze I have added pcibios_add_device call (similar to call in
> arch/arm64/kernel/pci.c ) in pci-common.c, which is being invoked by
> kernel core itself.

I see. In the upstream code you seem to do it in pcibios_setup_bus_devices(),
while arm64 and powerpc do it in pcibios_add_device().

> May be we can add similar on arm and test out, but
> we might need some cleanup in arch/arm/kernel/bios32.c

I think that would still just be a half-baked solution. This should
really be fully automatic. We could do it in the __weak
pcibios_add_device() for all architectures that don't override
it when the bus was probed from DT, or we could do it in
pci_read_irq().

	Arnd
Bharat Kumar Gogada Jan. 28, 2016, 2:18 p.m. UTC | #10
> Subject: Re: [PATCH V2 3/5] PCI: xilinx: Modifying AXI PCIe Host Bridge driver
> to work on both Zynq and Microblaze
> 
> On Thursday 28 January 2016 13:20:56 Bharat Kumar Gogada wrote:
> > > Subject: Re: [PATCH V2 3/5] PCI: xilinx: Modifying AXI PCIe Host
> > > Bridge driver to work on both Zynq and Microblaze
> > >
> > > On Wednesday 27 January 2016 14:33:45 Bharat Kumar Gogada wrote:
> > > > > > @@ -705,7 +715,9 @@ static int xilinx_pcie_probe(struct
> > > > > > platform_device *pdev)  #endif
> > > > > >     pci_scan_child_bus(bus);
> > > > > >     pci_assign_unassigned_bus_resources(bus);
> > > > > > +#ifdef CONFIG_ARM
> > > > > >     pci_fixup_irqs(pci_common_swizzle,
> > > > > > of_irq_parse_and_map_pci);
> > > > > > +#endif
> > > > > >     pci_bus_add_devices(bus);
> > > > > >     platform_set_drvdata(pdev, port);
> > > > >
> > > > > Here it looks like microblaze gets it right. I'm not sure why we
> > > > > still need the
> > > > > pci_fixup_irqs() on ARM, but my feeling is that this should be
> > > > > fixed in common code.
> > > > In arm pci_fixup_irqs is called by pci_common_init_dev
> > > (arch/arm/kernel/bios32.c), since this API is removed now, I was
> > > calling it separately.
> > >
> > > But who calls it in microblaze? If it works without the extra call
> > > there, can we make it work the same way for ARM?
> > >
> > In microblaze I have added pcibios_add_device call (similar to call in
> > arch/arm64/kernel/pci.c ) in pci-common.c, which is being invoked by
> > kernel core itself.
> 
> I see. In the upstream code you seem to do it in
> pcibios_setup_bus_devices(), while arm64 and powerpc do it in
> pcibios_add_device().
> 
No that function is not getting called with generic API's, its getting called with pcibios_init flow which is tightly bound with struct pci_controller microblaze specific structure. So I added pcibios_add_device in pci-common.c.

> > May be we can add similar on arm and test out, but we might need some
> > cleanup in arch/arm/kernel/bios32.c
> 
> I think that would still just be a half-baked solution. This should really be fully
> automatic. We could do it in the __weak
> pcibios_add_device() for all architectures that don't override it when the bus
> was probed from DT, or we could do it in pci_read_irq().
When will pci_read_irq() call get invoked ?

Bharat
Arnd Bergmann Jan. 28, 2016, 2:23 p.m. UTC | #11
On Thursday 28 January 2016 14:18:15 Bharat Kumar Gogada wrote:
> > 
> > I see. In the upstream code you seem to do it in
> > pcibios_setup_bus_devices(), while arm64 and powerpc do it in
> > pcibios_add_device().
> > 
> No that function is not getting called with generic API's, its getting called with pcibios_init flow which is tightly bound with struct pci_controller microblaze specific structure. So I added pcibios_add_device in pci-common.c.

Ok

> > > May be we can add similar on arm and test out, but we might need some
> > > cleanup in arch/arm/kernel/bios32.c
> > 
> > I think that would still just be a half-baked solution. This should really be fully
> > automatic. We could do it in the __weak
> > pcibios_add_device() for all architectures that don't override it when the bus
> > was probed from DT, or we could do it in pci_read_irq().
> When will pci_read_irq() call get invoked ?

This is called early on when a device gets created in pci_setup_device(),
so platforms can still override the value later.

The idea here is that normally a BIOS stores the interrupt number in
the PCI_INTERRUPT_LINE config space byte, and we just read it from
there. Generally speaking though, for non-PC systems we tend to not
have a BIOS that writes these values to start with, and any values
stored in here have no meaning in combination with SPARSE_IRQ
and/or IRQ_DOMAINS because the bootloader or BIOS doesn't know
what IRQ number will refer to hardware IRQ line in Linux.

	Arnd
Lorenzo Pieralisi Jan. 28, 2016, 2:49 p.m. UTC | #12
On Thu, Jan 28, 2016 at 03:23:37PM +0100, Arnd Bergmann wrote:
> On Thursday 28 January 2016 14:18:15 Bharat Kumar Gogada wrote:
> > > 
> > > I see. In the upstream code you seem to do it in
> > > pcibios_setup_bus_devices(), while arm64 and powerpc do it in
> > > pcibios_add_device().
> > > 
> > No that function is not getting called with generic API's, its getting called with pcibios_init flow which is tightly bound with struct pci_controller microblaze specific structure. So I added pcibios_add_device in pci-common.c.
> 
> Ok
> 
> > > > May be we can add similar on arm and test out, but we might need some
> > > > cleanup in arch/arm/kernel/bios32.c
> > > 
> > > I think that would still just be a half-baked solution. This should really be fully
> > > automatic. We could do it in the __weak
> > > pcibios_add_device() for all architectures that don't override it when the bus
> > > was probed from DT, or we could do it in pci_read_irq().
> > When will pci_read_irq() call get invoked ?
> 
> This is called early on when a device gets created in pci_setup_device(),
> so platforms can still override the value later.
> 
> The idea here is that normally a BIOS stores the interrupt number in
> the PCI_INTERRUPT_LINE config space byte, and we just read it from
> there. Generally speaking though, for non-PC systems we tend to not
> have a BIOS that writes these values to start with, and any values
> stored in here have no meaning in combination with SPARSE_IRQ
> and/or IRQ_DOMAINS because the bootloader or BIOS doesn't know
> what IRQ number will refer to hardware IRQ line in Linux.

I think the best way to handle this is through Matthew's series (below),
in the interim a callback in arch code would do, we will clean it up when
Matthew's series goes upstream, it should not take too long.

http://www.spinics.net/lists/linux-pci/msg45950.html

Thanks,
Lorenzo
diff mbox

Patch

diff --git a/drivers/pci/host/pcie-xilinx.c b/drivers/pci/host/pcie-xilinx.c
index 3e3757f..1981948 100644
--- a/drivers/pci/host/pcie-xilinx.c
+++ b/drivers/pci/host/pcie-xilinx.c
@@ -92,7 +92,12 @@ 
 #define ECAM_DEV_NUM_SHIFT		12
 
 /* Number of MSI IRQs */
-#define XILINX_NUM_MSI_IRQS		128
+#define XILINX_NUM_MSI_IRQS	128
+#ifdef CONFIG_ARM
+#define TOT_NR_IRQS		XILINX_NUM_MSI_IRQS
+#else
+#define TOT_NR_IRQS		(NR_IRQS + XILINX_NUM_MSI_IRQS)
+#endif
 
 
 /**
@@ -238,15 +243,20 @@  static void xilinx_pcie_destroy_msi(unsigned int irq)
  */
 static int xilinx_pcie_assign_msi(struct xilinx_pcie_port *port)
 {
+	int irq;
 	int pos;
 
 	pos = find_first_zero_bit(msi_irq_in_use, XILINX_NUM_MSI_IRQS);
-	if (pos < XILINX_NUM_MSI_IRQS)
+	irq = pos;
+#ifdef CONFIG_MICROBLAZE
+	irq = XILINX_NUM_MSI_IRQS + pos;
+#endif
+	if (irq < TOT_NR_IRQS)
 		set_bit(pos, msi_irq_in_use);
 	else
 		return -ENOSPC;
 
-	return pos;
+	return irq;
 }
 
 /**
@@ -520,7 +530,7 @@  static void xilinx_pcie_free_irq_domain(struct xilinx_pcie_port *port)
 
 		free_pages(port->msi_pages, 0);
 
-		num_irqs = XILINX_NUM_MSI_IRQS;
+		num_irqs = TOT_NR_IRQS;
 	} else {
 		/* INTx */
 		num_irqs = 4;
@@ -565,7 +575,7 @@  static int xilinx_pcie_init_irq_domain(struct xilinx_pcie_port *port)
 	/* Setup MSI */
 	if (IS_ENABLED(CONFIG_PCI_MSI)) {
 		port->irq_domain = irq_domain_add_linear(node,
-							 XILINX_NUM_MSI_IRQS,
+							 TOT_NR_IRQS,
 							 &msi_domain_ops,
 							 &xilinx_pcie_msi_chip);
 		if (!port->irq_domain) {
@@ -705,7 +715,9 @@  static int xilinx_pcie_probe(struct platform_device *pdev)
 #endif
 	pci_scan_child_bus(bus);
 	pci_assign_unassigned_bus_resources(bus);
+#ifdef CONFIG_ARM
 	pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci);
+#endif
 	pci_bus_add_devices(bus);
 	platform_set_drvdata(pdev, port);