All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] irqchip/xilinx: Expose Kconfig option
@ 2021-04-15 23:32 Robert Hancock
  2021-04-16 13:41 ` Marc Zyngier
  2021-04-18  8:26   ` kernel test robot
  0 siblings, 2 replies; 9+ messages in thread
From: Robert Hancock @ 2021-04-15 23:32 UTC (permalink / raw)
  To: tglx, maz; +Cc: michal.simek, linux-kernel, Robert Hancock

Previously the XILINX_INTC config option was hidden and only
auto-selected on the MicroBlaze platform. However, this IP can also be
used on other platforms. Allow this option to be user-enabled.

Signed-off-by: Robert Hancock <robert.hancock@calian.com>
---
 drivers/irqchip/Kconfig | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 15536e321df5..cc24f1bd3ca7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -279,8 +279,12 @@ config XTENSA_MX
 	select GENERIC_IRQ_EFFECTIVE_AFF_MASK
 
 config XILINX_INTC
-	bool
+	bool "Xilinx Interrupt Controller IP"
 	select IRQ_DOMAIN
+	help
+	  Support for the Xilinx Interrupt Controller IP core.
+	  This is used as a primary controller with MicroBlaze and can also
+	  be used as a secondary chained controller on other platforms.
 
 config IRQ_CROSSBAR
 	bool
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-15 23:32 [PATCH] irqchip/xilinx: Expose Kconfig option Robert Hancock
@ 2021-04-16 13:41 ` Marc Zyngier
  2021-04-16 16:05   ` Robert Hancock
  2021-04-18  8:26   ` kernel test robot
  1 sibling, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2021-04-16 13:41 UTC (permalink / raw)
  To: Robert Hancock; +Cc: tglx, michal.simek, linux-kernel

On Fri, 16 Apr 2021 00:32:50 +0100,
Robert Hancock <robert.hancock@calian.com> wrote:
> 
> Previously the XILINX_INTC config option was hidden and only
> auto-selected on the MicroBlaze platform. However, this IP can also be
> used on other platforms. Allow this option to be user-enabled.
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>

I don't think this is a good idea. In general, people have no idea
which interrupt controller they need to select. So you either end-up
with a missing interrupt controller, or a bunch you really don't need.

This is essentially a platform constraint, and this should directly be
selected by the platform if you have this IP in your system.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-16 13:41 ` Marc Zyngier
@ 2021-04-16 16:05   ` Robert Hancock
  2021-04-16 17:53     ` Marc Zyngier
  0 siblings, 1 reply; 9+ messages in thread
From: Robert Hancock @ 2021-04-16 16:05 UTC (permalink / raw)
  To: maz; +Cc: tglx, linux-kernel, michal.simek

On Fri, 2021-04-16 at 14:41 +0100, Marc Zyngier wrote:
> On Fri, 16 Apr 2021 00:32:50 +0100,
> Robert Hancock <robert.hancock@calian.com> wrote:
> > Previously the XILINX_INTC config option was hidden and only
> > auto-selected on the MicroBlaze platform. However, this IP can also be
> > used on other platforms. Allow this option to be user-enabled.
> > 
> > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> 
> I don't think this is a good idea. In general, people have no idea
> which interrupt controller they need to select. So you either end-up
> with a missing interrupt controller, or a bunch you really don't need.
> 
> This is essentially a platform constraint, and this should directly be
> selected by the platform if you have this IP in your system.
> 
> Thanks,
> 
> 	M.

The problem is essentially that at the platform level, we don't know, other
than in the MicroBlaze case where we know it will be used as the platform's
primary interrupt controller. In our case, we are using this IP core on the
ZynqMP platform as a secondary cascaded interrupt controller in the FPGA
portion of the device. But many ZynqMP configurations wouldn't have this device
present, it depends on what the user instantiates in the programmable logic.
Also, this core could just as easily be instantiated in standalone Xilinx FPGAs
which could be connected to many different platforms over a PCIe, AXI, etc.
bus. So I don't think having this as a platform constraint makes sense.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-16 16:05   ` Robert Hancock
@ 2021-04-16 17:53     ` Marc Zyngier
  2021-04-16 18:14       ` Robert Hancock
  0 siblings, 1 reply; 9+ messages in thread
From: Marc Zyngier @ 2021-04-16 17:53 UTC (permalink / raw)
  To: Robert Hancock; +Cc: tglx, linux-kernel, michal.simek

On Fri, 16 Apr 2021 17:05:49 +0100,
Robert Hancock <robert.hancock@calian.com> wrote:
> 
> On Fri, 2021-04-16 at 14:41 +0100, Marc Zyngier wrote:
> > On Fri, 16 Apr 2021 00:32:50 +0100,
> > Robert Hancock <robert.hancock@calian.com> wrote:
> > > Previously the XILINX_INTC config option was hidden and only
> > > auto-selected on the MicroBlaze platform. However, this IP can also be
> > > used on other platforms. Allow this option to be user-enabled.
> > > 
> > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > 
> > I don't think this is a good idea. In general, people have no idea
> > which interrupt controller they need to select. So you either end-up
> > with a missing interrupt controller, or a bunch you really don't need.
> > 
> > This is essentially a platform constraint, and this should directly be
> > selected by the platform if you have this IP in your system.
> > 
> > Thanks,
> > 
> > 	M.
> 
> The problem is essentially that at the platform level, we don't know, other
> than in the MicroBlaze case where we know it will be used as the platform's
> primary interrupt controller. In our case, we are using this IP core on the
> ZynqMP platform as a secondary cascaded interrupt controller in the FPGA
> portion of the device.
> But many ZynqMP configurations wouldn't have this device present, it
> depends on what the user instantiates in the programmable logic.
> Also, this core could just as easily be instantiated in standalone
> Xilinx FPGAs which could be connected to many different platforms
> over a PCIe, AXI, etc.  bus.

Not compiling it for some users is great if you happen to *know* what
you have to select, which is probably a single digit percentage of the
people that build their own kernel. At least having it to depend on
ZYNQMP (or some other FPGA platform) would narrow it down.

And if you have some other HW in your FPGA, you can make the config
fragment for this HW select the right interrupt controller. But I'm
definitely not keen on making this a universally user-selectable
driver.

> So I don't think having this as a platform constraint makes sense.

I don't think imposing this on *everyone*, across all supported
architectures and platforms makes any sense. Surely, people who build
their own HW (because that's what we are talking about here) can be
bothered to add the small Kconfig fragment that is required to their
kernel build.

	M.

-- 
Without deviation from the norm, progress is not possible.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-16 17:53     ` Marc Zyngier
@ 2021-04-16 18:14       ` Robert Hancock
  2021-04-19 11:23         ` Michal Simek
  0 siblings, 1 reply; 9+ messages in thread
From: Robert Hancock @ 2021-04-16 18:14 UTC (permalink / raw)
  To: maz; +Cc: tglx, linux-kernel, michal.simek

On Fri, 2021-04-16 at 18:53 +0100, Marc Zyngier wrote:
> On Fri, 16 Apr 2021 17:05:49 +0100,
> Robert Hancock <robert.hancock@calian.com> wrote:
> > On Fri, 2021-04-16 at 14:41 +0100, Marc Zyngier wrote:
> > > On Fri, 16 Apr 2021 00:32:50 +0100,
> > > Robert Hancock <robert.hancock@calian.com> wrote:
> > > > Previously the XILINX_INTC config option was hidden and only
> > > > auto-selected on the MicroBlaze platform. However, this IP can also be
> > > > used on other platforms. Allow this option to be user-enabled.
> > > > 
> > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > 
> > > I don't think this is a good idea. In general, people have no idea
> > > which interrupt controller they need to select. So you either end-up
> > > with a missing interrupt controller, or a bunch you really don't need.
> > > 
> > > This is essentially a platform constraint, and this should directly be
> > > selected by the platform if you have this IP in your system.
> > > 
> > > Thanks,
> > > 
> > > 	M.
> > 
> > The problem is essentially that at the platform level, we don't know, other
> > than in the MicroBlaze case where we know it will be used as the platform's
> > primary interrupt controller. In our case, we are using this IP core on the
> > ZynqMP platform as a secondary cascaded interrupt controller in the FPGA
> > portion of the device.
> > But many ZynqMP configurations wouldn't have this device present, it
> > depends on what the user instantiates in the programmable logic.
> > Also, this core could just as easily be instantiated in standalone
> > Xilinx FPGAs which could be connected to many different platforms
> > over a PCIe, AXI, etc.  bus.
> 
> Not compiling it for some users is great if you happen to *know* what
> you have to select, which is probably a single digit percentage of the
> people that build their own kernel. At least having it to depend on
> ZYNQMP (or some other FPGA platform) would narrow it down.
> 
> And if you have some other HW in your FPGA, you can make the config
> fragment for this HW select the right interrupt controller. But I'm
> definitely not keen on making this a universally user-selectable
> driver.

In general there is no specific or unique config option for what is
instantiated in an FPGA, it is completely up to the whims of whoever set it up.
You can instantiate whatever logic cores you want and there is no guarantee
whether they will or won't end up using this interrupt controller in the path
somewhere, so having a dependency there doesn't make much sense. For FPGA logic
it's ultimately up to the user to ensure the kernel config they are using has
the right drivers enabled for the cores they are using. Kconfig doesn't and
can't really help in this regard.

There's some precedent on this issue with drivers for various other FPGA-based
IP cores for SPI, I2C, Ethernet etc. Often they started out with an
architecture constraint which limited them to the platform they were originally
developed with, but which was later removed because the ability to use them in
standalone FPGAs means that the platforms they could potentially be used with
are basically unconstrained.

> 
> > So I don't think having this as a platform constraint makes sense.
> 
> I don't think imposing this on *everyone*, across all supported
> architectures and platforms makes any sense. Surely, people who build
> their own HW (because that's what we are talking about here) can be
> bothered to add the small Kconfig fragment that is required to their
> kernel build.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-15 23:32 [PATCH] irqchip/xilinx: Expose Kconfig option Robert Hancock
@ 2021-04-18  8:26   ` kernel test robot
  2021-04-18  8:26   ` kernel test robot
  1 sibling, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-18  8:26 UTC (permalink / raw)
  To: Robert Hancock, tglx, maz
  Cc: kbuild-all, michal.simek, linux-kernel, Robert Hancock

[-- Attachment #1: Type: text/plain, Size: 3308 bytes --]

Hi Robert,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v5.12-rc7 next-20210416]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Robert-Hancock/irqchip-xilinx-Expose-Kconfig-option/20210416-080610
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 7c07012eb1be8b4a95d3502fd30795849007a40e
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4ece2fff79c8d47de22ecca7c8d18d96525bfa43
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robert-Hancock/irqchip-xilinx-Expose-Kconfig-option/20210416-080610
        git checkout 4ece2fff79c8d47de22ecca7c8d18d96525bfa43
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   s390-linux-ld: kernel/dma/coherent.o: in function `dma_init_coherent_memory':
   coherent.c:(.text+0x3bc): undefined reference to `memremap'
   s390-linux-ld: coherent.c:(.text+0x500): undefined reference to `memunmap'
   s390-linux-ld: kernel/dma/coherent.o: in function `dma_declare_coherent_memory':
   coherent.c:(.text+0xae6): undefined reference to `memunmap'
   s390-linux-ld: drivers/irqchip/irq-al-fic.o: in function `al_fic_init_dt':
   irq-al-fic.c:(.init.text+0x98): undefined reference to `of_iomap'
   s390-linux-ld: irq-al-fic.c:(.init.text+0x596): undefined reference to `iounmap'
   s390-linux-ld: drivers/irqchip/irq-xilinx-intc.o: in function `xilinx_intc_of_init':
>> irq-xilinx-intc.c:(.init.text+0x9e): undefined reference to `of_iomap'
>> s390-linux-ld: irq-xilinx-intc.c:(.init.text+0x5f6): undefined reference to `iounmap'
   s390-linux-ld: drivers/clk/clk-fixed-mmio.o: in function `fixed_mmio_clk_setup':
   clk-fixed-mmio.c:(.text+0x9a): undefined reference to `of_iomap'
   s390-linux-ld: clk-fixed-mmio.c:(.text+0xe6): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-of.o: in function `timer_of_init':
   timer-of.c:(.init.text+0xcc): undefined reference to `of_iomap'
   s390-linux-ld: timer-of.c:(.init.text+0x8ee): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-of.o: in function `timer_of_cleanup':
   timer-of.c:(.init.text+0xb9a): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-microchip-pit64b.o: in function `mchp_pit64b_dt_init_timer':
   timer-microchip-pit64b.c:(.init.text+0x150): undefined reference to `of_iomap'
   s390-linux-ld: timer-microchip-pit64b.c:(.init.text+0xc78): undefined reference to `iounmap'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 27988 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
@ 2021-04-18  8:26   ` kernel test robot
  0 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2021-04-18  8:26 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 3365 bytes --]

Hi Robert,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/irq/core]
[also build test ERROR on v5.12-rc7 next-20210416]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Robert-Hancock/irqchip-xilinx-Expose-Kconfig-option/20210416-080610
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 7c07012eb1be8b4a95d3502fd30795849007a40e
config: s390-allmodconfig (attached as .config)
compiler: s390-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/4ece2fff79c8d47de22ecca7c8d18d96525bfa43
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Robert-Hancock/irqchip-xilinx-Expose-Kconfig-option/20210416-080610
        git checkout 4ece2fff79c8d47de22ecca7c8d18d96525bfa43
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross W=1 ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   s390-linux-ld: kernel/dma/coherent.o: in function `dma_init_coherent_memory':
   coherent.c:(.text+0x3bc): undefined reference to `memremap'
   s390-linux-ld: coherent.c:(.text+0x500): undefined reference to `memunmap'
   s390-linux-ld: kernel/dma/coherent.o: in function `dma_declare_coherent_memory':
   coherent.c:(.text+0xae6): undefined reference to `memunmap'
   s390-linux-ld: drivers/irqchip/irq-al-fic.o: in function `al_fic_init_dt':
   irq-al-fic.c:(.init.text+0x98): undefined reference to `of_iomap'
   s390-linux-ld: irq-al-fic.c:(.init.text+0x596): undefined reference to `iounmap'
   s390-linux-ld: drivers/irqchip/irq-xilinx-intc.o: in function `xilinx_intc_of_init':
>> irq-xilinx-intc.c:(.init.text+0x9e): undefined reference to `of_iomap'
>> s390-linux-ld: irq-xilinx-intc.c:(.init.text+0x5f6): undefined reference to `iounmap'
   s390-linux-ld: drivers/clk/clk-fixed-mmio.o: in function `fixed_mmio_clk_setup':
   clk-fixed-mmio.c:(.text+0x9a): undefined reference to `of_iomap'
   s390-linux-ld: clk-fixed-mmio.c:(.text+0xe6): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-of.o: in function `timer_of_init':
   timer-of.c:(.init.text+0xcc): undefined reference to `of_iomap'
   s390-linux-ld: timer-of.c:(.init.text+0x8ee): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-of.o: in function `timer_of_cleanup':
   timer-of.c:(.init.text+0xb9a): undefined reference to `iounmap'
   s390-linux-ld: drivers/clocksource/timer-microchip-pit64b.o: in function `mchp_pit64b_dt_init_timer':
   timer-microchip-pit64b.c:(.init.text+0x150): undefined reference to `of_iomap'
   s390-linux-ld: timer-microchip-pit64b.c:(.init.text+0xc78): undefined reference to `iounmap'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 27988 bytes --]

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-16 18:14       ` Robert Hancock
@ 2021-04-19 11:23         ` Michal Simek
  2021-04-19 17:54           ` Robert Hancock
  0 siblings, 1 reply; 9+ messages in thread
From: Michal Simek @ 2021-04-19 11:23 UTC (permalink / raw)
  To: Robert Hancock, maz, Anirudha Sarangi; +Cc: tglx, linux-kernel, michal.simek

Hi Marc and Robert, +Anirudha

On 4/16/21 8:14 PM, Robert Hancock wrote:
> On Fri, 2021-04-16 at 18:53 +0100, Marc Zyngier wrote:
>> On Fri, 16 Apr 2021 17:05:49 +0100,
>> Robert Hancock <robert.hancock@calian.com> wrote:
>>> On Fri, 2021-04-16 at 14:41 +0100, Marc Zyngier wrote:
>>>> On Fri, 16 Apr 2021 00:32:50 +0100,
>>>> Robert Hancock <robert.hancock@calian.com> wrote:
>>>>> Previously the XILINX_INTC config option was hidden and only
>>>>> auto-selected on the MicroBlaze platform. However, this IP can also be
>>>>> used on other platforms. Allow this option to be user-enabled.
>>>>>
>>>>> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
>>>>
>>>> I don't think this is a good idea. In general, people have no idea
>>>> which interrupt controller they need to select. So you either end-up
>>>> with a missing interrupt controller, or a bunch you really don't need.
>>>>
>>>> This is essentially a platform constraint, and this should directly be
>>>> selected by the platform if you have this IP in your system.
>>>>
>>>> Thanks,
>>>>
>>>> 	M.
>>>
>>> The problem is essentially that at the platform level, we don't know, other
>>> than in the MicroBlaze case where we know it will be used as the platform's
>>> primary interrupt controller. In our case, we are using this IP core on the
>>> ZynqMP platform as a secondary cascaded interrupt controller in the FPGA
>>> portion of the device.
>>> But many ZynqMP configurations wouldn't have this device present, it
>>> depends on what the user instantiates in the programmable logic.
>>> Also, this core could just as easily be instantiated in standalone
>>> Xilinx FPGAs which could be connected to many different platforms
>>> over a PCIe, AXI, etc.  bus.
>>
>> Not compiling it for some users is great if you happen to *know* what
>> you have to select, which is probably a single digit percentage of the
>> people that build their own kernel. At least having it to depend on
>> ZYNQMP (or some other FPGA platform) would narrow it down.
>>
>> And if you have some other HW in your FPGA, you can make the config
>> fragment for this HW select the right interrupt controller. But I'm
>> definitely not keen on making this a universally user-selectable
>> driver.
> 
> In general there is no specific or unique config option for what is
> instantiated in an FPGA, it is completely up to the whims of whoever set it up.
> You can instantiate whatever logic cores you want and there is no guarantee
> whether they will or won't end up using this interrupt controller in the path
> somewhere, so having a dependency there doesn't make much sense. For FPGA logic
> it's ultimately up to the user to ensure the kernel config they are using has
> the right drivers enabled for the cores they are using. Kconfig doesn't and
> can't really help in this regard.
> 
> There's some precedent on this issue with drivers for various other FPGA-based
> IP cores for SPI, I2C, Ethernet etc. Often they started out with an
> architecture constraint which limited them to the platform they were originally
> developed with, but which was later removed because the ability to use them in
> standalone FPGAs means that the platforms they could potentially be used with
> are basically unconstrained.
> 
>>
>>> So I don't think having this as a platform constraint makes sense.
>>
>> I don't think imposing this on *everyone*, across all supported
>> architectures and platforms makes any sense. Surely, people who build
>> their own HW (because that's what we are talking about here) can be
>> bothered to add the small Kconfig fragment that is required to their
>> kernel build.
> 

The same interrupt controller was used by ppc405 and ppc440 xilinx
platform in past. Unfortunately it was separate copy of this microblaze
one and ppc copy should be already removed by removing support for these
platforms.

The first(default) configuration which was mentioned above is primary
interrupt controller for Microblaze.

That being said this controller can be used and actually is used by
Xilinx Zynq(arm32) ZynqMP(arm64) and Versal(arm64) SOCs as a cascate
intc controller. I have also seen couple of different cpus in PL which
can use (and actually use it) this soft IP. But limiting it for
Zynq/ZynqMP should be fine.

Recently my colleague (Anirudha) did some experiments with binding and
unbinding this driver for cases where you are using it as cascade
interrupt controller in a programmable logic(PL) (primary controller is
GIC) and then you want to exchange the whole PL by different
configuration that you need to have option to remove this chip.

Thanks,
Michal

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] irqchip/xilinx: Expose Kconfig option
  2021-04-19 11:23         ` Michal Simek
@ 2021-04-19 17:54           ` Robert Hancock
  0 siblings, 0 replies; 9+ messages in thread
From: Robert Hancock @ 2021-04-19 17:54 UTC (permalink / raw)
  To: maz, michal.simek, anirudh; +Cc: tglx, linux-kernel

On Mon, 2021-04-19 at 13:23 +0200, Michal Simek wrote:
> Hi Marc and Robert, +Anirudha
> 
> On 4/16/21 8:14 PM, Robert Hancock wrote:
> > On Fri, 2021-04-16 at 18:53 +0100, Marc Zyngier wrote:
> > > On Fri, 16 Apr 2021 17:05:49 +0100,
> > > Robert Hancock <robert.hancock@calian.com> wrote:
> > > > On Fri, 2021-04-16 at 14:41 +0100, Marc Zyngier wrote:
> > > > > On Fri, 16 Apr 2021 00:32:50 +0100,
> > > > > Robert Hancock <robert.hancock@calian.com> wrote:
> > > > > > Previously the XILINX_INTC config option was hidden and only
> > > > > > auto-selected on the MicroBlaze platform. However, this IP can also
> > > > > > be
> > > > > > used on other platforms. Allow this option to be user-enabled.
> > > > > > 
> > > > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> > > > > 
> > > > > I don't think this is a good idea. In general, people have no idea
> > > > > which interrupt controller they need to select. So you either end-up
> > > > > with a missing interrupt controller, or a bunch you really don't
> > > > > need.
> > > > > 
> > > > > This is essentially a platform constraint, and this should directly
> > > > > be
> > > > > selected by the platform if you have this IP in your system.
> > > > > 
> > > > > Thanks,
> > > > > 
> > > > > 	M.
> > > > 
> > > > The problem is essentially that at the platform level, we don't know,
> > > > other
> > > > than in the MicroBlaze case where we know it will be used as the
> > > > platform's
> > > > primary interrupt controller. In our case, we are using this IP core on
> > > > the
> > > > ZynqMP platform as a secondary cascaded interrupt controller in the
> > > > FPGA
> > > > portion of the device.
> > > > But many ZynqMP configurations wouldn't have this device present, it
> > > > depends on what the user instantiates in the programmable logic.
> > > > Also, this core could just as easily be instantiated in standalone
> > > > Xilinx FPGAs which could be connected to many different platforms
> > > > over a PCIe, AXI, etc.  bus.
> > > 
> > > Not compiling it for some users is great if you happen to *know* what
> > > you have to select, which is probably a single digit percentage of the
> > > people that build their own kernel. At least having it to depend on
> > > ZYNQMP (or some other FPGA platform) would narrow it down.
> > > 
> > > And if you have some other HW in your FPGA, you can make the config
> > > fragment for this HW select the right interrupt controller. But I'm
> > > definitely not keen on making this a universally user-selectable
> > > driver.
> > 
> > In general there is no specific or unique config option for what is
> > instantiated in an FPGA, it is completely up to the whims of whoever set it
> > up.
> > You can instantiate whatever logic cores you want and there is no guarantee
> > whether they will or won't end up using this interrupt controller in the
> > path
> > somewhere, so having a dependency there doesn't make much sense. For FPGA
> > logic
> > it's ultimately up to the user to ensure the kernel config they are using
> > has
> > the right drivers enabled for the cores they are using. Kconfig doesn't and
> > can't really help in this regard.
> > 
> > There's some precedent on this issue with drivers for various other FPGA-
> > based
> > IP cores for SPI, I2C, Ethernet etc. Often they started out with an
> > architecture constraint which limited them to the platform they were
> > originally
> > developed with, but which was later removed because the ability to use them
> > in
> > standalone FPGAs means that the platforms they could potentially be used
> > with
> > are basically unconstrained.
> > 
> > > > So I don't think having this as a platform constraint makes sense.
> > > 
> > > I don't think imposing this on *everyone*, across all supported
> > > architectures and platforms makes any sense. Surely, people who build
> > > their own HW (because that's what we are talking about here) can be
> > > bothered to add the small Kconfig fragment that is required to their
> > > kernel build.
> 
> The same interrupt controller was used by ppc405 and ppc440 xilinx
> platform in past. Unfortunately it was separate copy of this microblaze
> one and ppc copy should be already removed by removing support for these
> platforms.
> 
> The first(default) configuration which was mentioned above is primary
> interrupt controller for Microblaze.
> 
> That being said this controller can be used and actually is used by
> Xilinx Zynq(arm32) ZynqMP(arm64) and Versal(arm64) SOCs as a cascate
> intc controller. I have also seen couple of different cpus in PL which
> can use (and actually use it) this soft IP. But limiting it for
> Zynq/ZynqMP should be fine.
> 
> Recently my colleague (Anirudha) did some experiments with binding and
> unbinding this driver for cases where you are using it as cascade
> interrupt controller in a programmable logic(PL) (primary controller is
> GIC) and then you want to exchange the whole PL by different
> configuration that you need to have option to remove this chip.

It sounds like limiting to Microblaze, Zynq and ZynqMP for now is less
controversial and meets our current use case, so will submit v2 with that
dependency in place.

-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-04-19 18:05 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-15 23:32 [PATCH] irqchip/xilinx: Expose Kconfig option Robert Hancock
2021-04-16 13:41 ` Marc Zyngier
2021-04-16 16:05   ` Robert Hancock
2021-04-16 17:53     ` Marc Zyngier
2021-04-16 18:14       ` Robert Hancock
2021-04-19 11:23         ` Michal Simek
2021-04-19 17:54           ` Robert Hancock
2021-04-18  8:26 ` kernel test robot
2021-04-18  8:26   ` kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.