[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Hello,

This set of patches introduces Message Signaled Interrupt support in
the Marvell EBU PCIe driver. It has been successfully tested on the
Armada XP GP platform and the Armada 370 DB platform with an Intel
e1000e PCIe network card that supports MSI.

This is based on work done by Lior Amsalem <alior@marvell.com>.

The patches do the following:

 * Patches 1, 2 and 3 move the IRQ controller driver of Armada 370/XP
   platforms from arch/arm/mach-mvebu/ into drivers/irqchip/ and use
   the proper irqchip infrastructure. Those changes are not strictly
   needed to add MSI interrupts support, but since we will be touching
   the IRQ controller driver anyway, it sounded like the right time to
   do this move.

 * Patches 4 and 5 do some minor cleanups that will help for the
   remainer of the changes.

 * Patch 6 makes the MPIC node a subnode of the interrupt controller
   node, since the MSI interrupt controller will be added as a second
   subnode. The details on why this is needed are given in the commit
   log. It is important to read those details to understand why such a
   Device Tree representation was used.

 * Patch 7 modifies the IRQ controller driver to support MSI.

 * Patch 8 is a patch from Thierry Reding that introduces a small
   infrastructure based on 'struct msi_chip' which makes the MSI
   support usable on multiplatform kernel.

 * Patch 9 adds the MSI support to the Marvell EBU PCIe driver.

 * Patch 10 and 11 respectively update the Device Tree and the
   defconfig of Armada 370/XP platforms to enable MSI.

It is for now sent as a RFC in order to define whether the Device Tree
representation is correct or not. Again, patch 6 gives the rationale
for this choice.

This patch set is meant to be applied on top of the Marvell PCIe
driver. I keep it distinct from the PCIe driver patch set because the
PCIe driver itself is mostly reading for merging (it has gone through
multiple iterations over the last 3 months), while this patch set is
brand new and will most likely need a bunch of iterations as well.

Thanks,

Thomas

Thierry Reding (1):
  PCI: Introduce new MSI chip infrastructure

Thomas Petazzoni (10):
  arm: mvebu: move L2 cache initialization in init_early()
  irqchip: move IRQ driver for Armada 370/XP
  irqchip: armada-370-xp: move IRQ handler to avoid forward declaration
  irqchip: armada-370-xp: slightly cleanup irq controller driver
  arm: mvebu: do not duplicate the mpic alias
  irqchip: armada-370-xp: use a separate mpic node
  irqchip: armada-370-xp: add MSI support to interrupt controller
    driver
  pci: mvebu: add MSI support
  arm: mvebu: enable MSI support in DT
  arm: mvebu: enable PCI MSI support in defconfig

 .../devicetree/bindings/arm/armada-370-xp-mpic.txt |   39 +++-
 .../devicetree/bindings/pci/mvebu-pci.txt          |    5 +
 arch/arm/boot/dts/armada-370-xp.dtsi               |   15 +-
 arch/arm/boot/dts/armada-370.dtsi                  |    3 +-
 arch/arm/boot/dts/armada-xp-mv78230.dtsi           |    1 +
 arch/arm/boot/dts/armada-xp-mv78260.dtsi           |    1 +
 arch/arm/boot/dts/armada-xp-mv78460.dtsi           |    1 +
 arch/arm/boot/dts/armada-xp.dtsi                   |    2 +-
 arch/arm/configs/mvebu_defconfig                   |    1 +
 arch/arm/mach-mvebu/Kconfig                        |    1 +
 arch/arm/mach-mvebu/Makefile                       |    2 +-
 arch/arm/mach-mvebu/armada-370-xp.c                |    9 +-
 drivers/irqchip/Makefile                           |    1 +
 .../irqchip}/irq-armada-370-xp.c                   |  202 ++++++++++++++------
 drivers/pci/host/pci-mvebu.c                       |  128 +++++++++++++
 drivers/pci/msi.c                                  |   35 +++-
 drivers/pci/probe.c                                |    1 +
 include/linux/msi.h                                |   10 +
 include/linux/pci.h                                |    1 +
 19 files changed, 384 insertions(+), 74 deletions(-)
 rename {arch/arm/mach-mvebu => drivers/irqchip}/irq-armada-370-xp.c (68%)

-- 
1.7.9.5

[RFCv1 01/11] arm: mvebu: move L2 cache initialization in init_early()

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> @@ -72,6 +73,10 @@ void __init armada_370_xp_init_early(void)
>                         ARMADA_370_XP_MBUS_WINS_SIZE,
>                         ARMADA_370_XP_SDRAM_WINS_BASE,
>                         ARMADA_370_XP_SDRAM_WINS_SIZE);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +       l2x0_of_init(0, ~0UL);
> +#endif

I guess you should remove the #ifdef as well here, it's not needed.

	Arnd

[RFCv1 01/11] arm: mvebu: move L2 cache initialization in init_early()

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 16:53:48 +0000, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> > @@ -72,6 +73,10 @@ void __init armada_370_xp_init_early(void)
> >                         ARMADA_370_XP_MBUS_WINS_SIZE,
> >                         ARMADA_370_XP_SDRAM_WINS_BASE,
> >                         ARMADA_370_XP_SDRAM_WINS_SIZE);
> > +
> > +#ifdef CONFIG_CACHE_L2X0
> > +       l2x0_of_init(0, ~0UL);
> > +#endif
> 
> I guess you should remove the #ifdef as well here, it's not needed.

Right, will fix for the next version.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 01/11] arm: mvebu: move L2 cache initialization in init_early()

[Rob Herring]

On 03/26/2013 11:52 AM, Thomas Petazzoni wrote:
> In preparation for moving the IRQ controller driver to
> drivers/irqchip/, we don't want the IRQ controller driver to be
> responsible for initializing the L2 cache. Instead, let's initialize
> the L2 cache at the init_early() level, like mach-exynos/common.c is
> doing.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/mach-mvebu/armada-370-xp.c     |    5 +++++
>  arch/arm/mach-mvebu/irq-armada-370-xp.c |    4 ----
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
> index 12d3655..d98a0ca 100644
> --- a/arch/arm/mach-mvebu/armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/armada-370-xp.c
> @@ -20,6 +20,7 @@
>  #include <linux/clk/mvebu.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/mbus.h>
> +#include <asm/hardware/cache-l2x0.h>
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
>  #include <asm/mach/time.h>
> @@ -72,6 +73,10 @@ void __init armada_370_xp_init_early(void)
>  			ARMADA_370_XP_MBUS_WINS_SIZE,
>  			ARMADA_370_XP_SDRAM_WINS_BASE,
>  			ARMADA_370_XP_SDRAM_WINS_SIZE);
> +
> +#ifdef CONFIG_CACHE_L2X0
> +	l2x0_of_init(0, ~0UL);
> +#endif

Have you actually tested this? I don't think the ioremap in here will
work during init_early.

Rob

>  }
>  
>  static void __init armada_370_xp_dt_init(void)
> diff --git a/arch/arm/mach-mvebu/irq-armada-370-xp.c b/arch/arm/mach-mvebu/irq-armada-370-xp.c
> index 6a9195e..f6699f3 100644
> --- a/arch/arm/mach-mvebu/irq-armada-370-xp.c
> +++ b/arch/arm/mach-mvebu/irq-armada-370-xp.c
> @@ -25,7 +25,6 @@
>  #include <asm/mach/arch.h>
>  #include <asm/exception.h>
>  #include <asm/smp_plat.h>
> -#include <asm/hardware/cache-l2x0.h>
>  
>  /* Interrupt Controller Registers Map */
>  #define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
> @@ -292,7 +291,4 @@ static const struct of_device_id mpic_of_match[] __initconst = {
>  void __init armada_370_xp_init_irq(void)
>  {
>  	of_irq_init(mpic_of_match);
> -#ifdef CONFIG_CACHE_L2X0
> -	l2x0_of_init(0, ~0UL);
> -#endif
>  }
> 

[RFCv1 02/11] irqchip: move IRQ driver for Armada 370/XP

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> When the Marvell Armada 370/XP support was included in the kernel, the
> drivers/irqchip/ directory didn't exist and the minimal infrastructure
> in it also didn't exist. Now that we have those things in place, we
> move the Armada 370/XP IRQ controller driver from
> arch/arm/mach-mvebu/irq-armada-370-xp.c to
> drivers/irqchip/irq-armada-370-xp.c.
> 
> Note in order to reduce code movement and therefore ease the review of
> this patch, we intentionally introduce a forward declaration of
> armada_370_xp_handle_irq(). It is in fact not needed because this
> handler can now simply be implemented before
> armada_370_xp_mpic_of_init(). That will be done in the next commit.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Arnd Bergmann <arnd@arndb.de>

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Hello,

On Tue, 26 Mar 2013 17:52:15 +0100, Thomas Petazzoni wrote:

> This set of patches introduces Message Signaled Interrupt support in
> the Marvell EBU PCIe driver. It has been successfully tested on the
> Armada XP GP platform and the Armada 370 DB platform with an Intel
> e1000e PCIe network card that supports MSI.

To the readers of LAKML: the mailing list software has, for some
reason, decided that all the e-mails in this series had a "Suspicious
header". They have all been generated by git format-patch and sent with
git send-email, just like the previous set of 17 patches for the PCIe
driver, so I have no idea why the mailing list software changed his
mind.

Hopefully, the next version I'll send will make it to the list.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> To the readers of LAKML: the mailing list software has, for some
> reason, decided that all the e-mails in this series had a "Suspicious
> header". They have all been generated by git format-patch and sent with
> git send-email, just like the previous set of 17 patches for the PCIe
> driver, so I have no idea why the mailing list software changed his
> mind.

The mailing list rejects patches that have an in-reply-to header with
pointing to a different subject as the new email.
It also has an exception for emails that have the work PATCH in brackets,
but not [RFC] or [GIT PULL]. You can work around by using [RFC PATCH 1/11]
or [RFC][PATCH 1/11].

	Arnd

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 17:18:08 +0000, Arnd Bergmann wrote:

> The mailing list rejects patches that have an in-reply-to header with
> pointing to a different subject as the new email.
> It also has an exception for emails that have the work PATCH in
> brackets, but not [RFC] or [GIT PULL]. You can work around by using
> [RFC PATCH 1/11] or [RFC][PATCH 1/11].

Aah, thanks for the tip. Do you think there's a place where we could
write this down for future reference? Maybe
Documentation/arm/mailing-list or whatever?

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> +              mpic: main-intc at d0020000 {
> +                    #interrupt-cells = <1>;
> +                    interrupt-controller;
> +              };
> +
> +              msi: msi-intc at d0020000 {
> +                    #interrupt-cells = <1>;
> +                    interrupt-controller;
> +                    marvell,doorbell = <0xd0020a04>;
> +              };

I think the @d0020000 part needs to be removed for the nodes that
have no reg property.

I think I did not follow the entire discussion. What has led to having
two subnodes in the end, rather than a single node like this?


       interrupt-controller at d0020000 {
              compatible = "marvell,mpic";
              #interrupt-cells = <1>;
              #msi-cells = <1>;
              #address-cells = <1>;
              #size-cells = <1>;
              interrupt-controller;
              msi-controller;
              reg = <0xd0020a00 0x1d0>,
                     <0xd0021070 0x58>;
              marvell,doorbell = <0xd0020a04>;
       };


	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 17:07:41 +0000, Arnd Bergmann wrote:

> I think the @d0020000 part needs to be removed for the nodes that
> have no reg property.

Sure, will fix.

> I think I did not follow the entire discussion. What has led to having
> two subnodes in the end, rather than a single node like this?
> 
> 
>        interrupt-controller at d0020000 {
>               compatible = "marvell,mpic";
>               #interrupt-cells = <1>;
>               #msi-cells = <1>;
>               #address-cells = <1>;
>               #size-cells = <1>;
>               interrupt-controller;
>               msi-controller;
>               reg = <0xd0020a00 0x1d0>,
>                      <0xd0021070 0x58>;
>               marvell,doorbell = <0xd0020a04>;
>        };

I've tried to explain that in the commit log of PATCH 6, which says:

    However, we need the driver to expose two different IRQ domains: one
    for the main interrupt controller itself, and one for the MSI
    interrupt controller. In order to achieve this, we will create two
    subnodes in the interrupt-controller at d0020000 node: one subnode for
    the main interrupt controller, and one subnode for the MSI interrupt
    controller. The two irq domains can't be registered on the same DT
    node, otherwise when irq_find_host() gets used by of_irq_map_one()
    to resolve IRQs of devices, they may find the MSI interrupt
    controller instead of the main interrupt controller.
    
    Note that both the parent and the child node need to have the
    'interrupt-controller' empty property:
    
      * The interrupt-controller property is needed in the main
    interrupt controller node (interrupt-controller at d0020000) because
    the of_irq_init() function skips nodes that are matching the given
       compatible string, but that don't have the interrupt-controller
       property.
    
     * The interrupt-controller property is needed in the child
    interrupt controller node (main-intc at d0020000) otherwise the
    resolution done by of_irq_map_one() doesn't work.

So really, the thing is that irq_domain_add_linear() registers an IRQ
domain on a specific DT node, and then irq_find_host() finds back an
IRQ domain from a given DT node. So if you have two IRQ domains
registered on the same DT node, then you don't know which one will be
used.

So if I do the two irq_domain_add_linear() (one for MPIC, one for MSI)
on one single DT node, when the timer driver will request its
interrupt, it turns out that the MSI IRQ domain is used and not the
MPIC IRQ domain, even though the timer has <&mpic> as its interrupt
parent.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:

> 
> I've tried to explain that in the commit log of PATCH 6, which says:
> 
>     However, we need the driver to expose two different IRQ domains: one
>     for the main interrupt controller itself, and one for the MSI
>     interrupt controller. In order to achieve this, we will create two
>     subnodes in the interrupt-controller at d0020000 node: one subnode for
>     the main interrupt controller, and one subnode for the MSI interrupt
>     controller. The two irq domains can't be registered on the same DT
>     node, otherwise when irq_find_host() gets used by of_irq_map_one()
>     to resolve IRQs of devices, they may find the MSI interrupt
>     controller instead of the main interrupt controller.

Right, I should have read the commit log better ...

>     Note that both the parent and the child node need to have the
>     'interrupt-controller' empty property:
>     
>       * The interrupt-controller property is needed in the main
>     interrupt controller node (interrupt-controller at d0020000) because
>     the of_irq_init() function skips nodes that are matching the given
>        compatible string, but that don't have the interrupt-controller
>        property.
>     
>      * The interrupt-controller property is needed in the child
>     interrupt controller node (main-intc at d0020000) otherwise the
>     resolution done by of_irq_map_one() doesn't work.

If you add compatible properties to the children, that would work
I suppose.

> So really, the thing is that irq_domain_add_linear() registers an IRQ
> domain on a specific DT node, and then irq_find_host() finds back an
> IRQ domain from a given DT node. So if you have two IRQ domains
> registered on the same DT node, then you don't know which one will be
> used.
> 
> So if I do the two irq_domain_add_linear() (one for MPIC, one for MSI)
> on one single DT node, when the timer driver will request its
> interrupt, it turns out that the MSI IRQ domain is used and not the
> MPIC IRQ domain, even though the timer has <&mpic> as its interrupt
> parent.

I still wonder if the real solution shouldn't instead be to make the
irq domain code MSI aware. For instance, you don't really need a
cell to describe an interrupt because the interrupt number is
not a hardware property. So an MSI using device doesn't really
needs an "msis" or "interrupts" property, just an "msi-parent",
and we can add code to handle as a separate domain even if you
have a single device node that can do both level and message
interrupts.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 18:38:22 +0000, Arnd Bergmann wrote:

> >     Note that both the parent and the child node need to have the
> >     'interrupt-controller' empty property:
> >     
> >       * The interrupt-controller property is needed in the main
> >     interrupt controller node (interrupt-controller at d0020000)
> > because the of_irq_init() function skips nodes that are matching
> > the given compatible string, but that don't have the
> > interrupt-controller property.
> >     
> >      * The interrupt-controller property is needed in the child
> >     interrupt controller node (main-intc at d0020000) otherwise the
> >     resolution done by of_irq_map_one() doesn't work.
> 
> If you add compatible properties to the children, that would work
> I suppose.

To which children? Only to the main-intc children? If so,
armada_370_xp_mpic_of_init() would be called with a 'device_node *'
that points to the main-intc, correct? Then it would have to go back up
in the Device Tree to find the msi node? It's doable of course, but
sounds strange, no?

To me, the topology of the hardware is really that a single device
provides two features: the main interrupt controller and the MSI
interrupt controller. But I will adapt to whatever DT binding you
propose.

> > So really, the thing is that irq_domain_add_linear() registers an
> > IRQ domain on a specific DT node, and then irq_find_host() finds
> > back an IRQ domain from a given DT node. So if you have two IRQ
> > domains registered on the same DT node, then you don't know which
> > one will be used.
> > 
> > So if I do the two irq_domain_add_linear() (one for MPIC, one for
> > MSI) on one single DT node, when the timer driver will request its
> > interrupt, it turns out that the MSI IRQ domain is used and not the
> > MPIC IRQ domain, even though the timer has <&mpic> as its interrupt
> > parent.
> 
> I still wonder if the real solution shouldn't instead be to make the
> irq domain code MSI aware. For instance, you don't really need a
> cell to describe an interrupt because the interrupt number is
> not a hardware property. So an MSI using device doesn't really
> needs an "msis" or "interrupts" property, just an "msi-parent",
> and we can add code to handle as a separate domain even if you
> have a single device node that can do both level and message
> interrupts.

So the msi-parent property should point to the single mpic node? But
then the IRQ domain for MSI cannot be registered on this single MPIC
node. Then, what would be the first argument of:

        armada_370_xp_msi_domain =
                irq_domain_add_linear(msi_node, PCI_MSI_DOORBELL_NR,
                                      &armada_370_xp_msi_irq_ops, NULL);

and how would the PCIe driver get a pointer to this IRQ domain? (In the
currently proposed code, it just does a irq_find_host(), which sounded
very simple and straightforward).

To clarify: I really don't mind reworking the code, but unfortunately
"make the IRQ domain code MSI aware" is too vague for me to understand
the direction you're thinking of.

Thanks for your review and comments!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> Dear Arnd Bergmann,
> 
> On Tue, 26 Mar 2013 18:38:22 +0000, Arnd Bergmann wrote:
> 
> > >     Note that both the parent and the child node need to have the
> > >     'interrupt-controller' empty property:
> > >     
> > >       * The interrupt-controller property is needed in the main
> > >     interrupt controller node (interrupt-controller at d0020000)
> > > because the of_irq_init() function skips nodes that are matching
> > > the given compatible string, but that don't have the
> > > interrupt-controller property.
> > >     
> > >      * The interrupt-controller property is needed in the child
> > >     interrupt controller node (main-intc at d0020000) otherwise the
> > >     resolution done by of_irq_map_one() doesn't work.
> > 
> > If you add compatible properties to the children, that would work
> > I suppose.
> 
> To which children? Only to the main-intc children? If so,
> armada_370_xp_mpic_of_init() would be called with a 'device_node *'
> that points to the main-intc, correct? Then it would have to go back up
> in the Device Tree to find the msi node? It's doable of course, but
> sounds strange, no?

I was thinking of registering two init functions as well.

> To me, the topology of the hardware is really that a single device
> provides two features: the main interrupt controller and the MSI
> interrupt controller. But I will adapt to whatever DT binding you
> propose.

My preference would be to have no sub-nodes but rather make the
code deal with an interrupt controller that has an interrupt domain
for regular IRQs but can also handle MSI.

> > I still wonder if the real solution shouldn't instead be to make the
> > irq domain code MSI aware. For instance, you don't really need a
> > cell to describe an interrupt because the interrupt number is
> > not a hardware property. So an MSI using device doesn't really
> > needs an "msis" or "interrupts" property, just an "msi-parent",
> > and we can add code to handle as a separate domain even if you
> > have a single device node that can do both level and message
> > interrupts.
> 
> So the msi-parent property should point to the single mpic node? But
> then the IRQ domain for MSI cannot be registered on this single MPIC
> node. Then, what would be the first argument of:
> 
>         armada_370_xp_msi_domain =
>                 irq_domain_add_linear(msi_node, PCI_MSI_DOORBELL_NR,
>                                       &armada_370_xp_msi_irq_ops, NULL);
> 
> and how would the PCIe driver get a pointer to this IRQ domain? (In the
> currently proposed code, it just does a irq_find_host(), which sounded
> very simple and straightforward).

I guess one way would be to have a single domain that is responsible
for the controller and handles both MSI and legacy interrupts. That
could probably be done without changes to the interrupt domain code.

Another option would be to add an irq_domain_add_msi() function that
creates a domain which is also tied to the same device node but does
not interact with it when going through the legacy interrupts.
You could then add a matching msi_find_host() or irq_find_msi_host()
function to return the domain.

> To clarify: I really don't mind reworking the code, but unfortunately
> "make the IRQ domain code MSI aware" is too vague for me to understand
> the direction you're thinking of.

I don't have specific code in mind yet, mainly playing around with the
possibilities for now.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 21:10:15 +0000, Arnd Bergmann wrote:

> > To which children? Only to the main-intc children? If so,
> > armada_370_xp_mpic_of_init() would be called with a 'device_node *'
> > that points to the main-intc, correct? Then it would have to go back up
> > in the Device Tree to find the msi node? It's doable of course, but
> > sounds strange, no?
> 
> I was thinking of registering two init functions as well.

But then which of the two init functions would do the of_iomap() (or
whatever ioremap()ing function you use) ?

Remember, the very reason what we have *one* driver for both interrupt
controllers is because the registers are completely mixed. So to me
it's really the case of *one* device providing *two* features, like a
device that would be both an Ethernet interface and a SPI controller.
You have a single ->probe() function that gets called when the device
is detected, and this ->probe() function registers both an Ethernet
interface and a SPI controller.

To me, the case we have here is really identical: we have one single set
of registers that provide multiple features.

> > To me, the topology of the hardware is really that a single device
> > provides two features: the main interrupt controller and the MSI
> > interrupt controller. But I will adapt to whatever DT binding you
> > propose.
> 
> My preference would be to have no sub-nodes but rather make the
> code deal with an interrupt controller that has an interrupt domain
> for regular IRQs but can also handle MSI.

Hum, ok.

> > > I still wonder if the real solution shouldn't instead be to make the
> > > irq domain code MSI aware. For instance, you don't really need a
> > > cell to describe an interrupt because the interrupt number is
> > > not a hardware property. So an MSI using device doesn't really
> > > needs an "msis" or "interrupts" property, just an "msi-parent",
> > > and we can add code to handle as a separate domain even if you
> > > have a single device node that can do both level and message
> > > interrupts.
> > 
> > So the msi-parent property should point to the single mpic node? But
> > then the IRQ domain for MSI cannot be registered on this single MPIC
> > node. Then, what would be the first argument of:
> > 
> >         armada_370_xp_msi_domain =
> >                 irq_domain_add_linear(msi_node, PCI_MSI_DOORBELL_NR,
> >                                       &armada_370_xp_msi_irq_ops, NULL);
> > 
> > and how would the PCIe driver get a pointer to this IRQ domain? (In the
> > currently proposed code, it just does a irq_find_host(), which sounded
> > very simple and straightforward).
> 
> I guess one way would be to have a single domain that is responsible
> for the controller and handles both MSI and legacy interrupts. That
> could probably be done without changes to the interrupt domain code.
> 
> Another option would be to add an irq_domain_add_msi() function that
> creates a domain which is also tied to the same device node but does
> not interact with it when going through the legacy interrupts.
> You could then add a matching msi_find_host() or irq_find_msi_host()
> function to return the domain.

This option seems doable. Not sure yet how to do it exactly, but at
least I understand the idea.

> > To clarify: I really don't mind reworking the code, but unfortunately
> > "make the IRQ domain code MSI aware" is too vague for me to understand
> > the direction you're thinking of.
> 
> I don't have specific code in mind yet, mainly playing around with the
> possibilities for now.

Sure, I understand. But I also don't have specific ideas: the current
code works fine for me, and I don't find it really ugly. So if you don't
point me in the direction that you think would make the code less ugly,
then I'm lost.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> On Tue, 26 Mar 2013 21:10:15 +0000, Arnd Bergmann wrote:
> 
> > > To which children? Only to the main-intc children? If so,
> > > armada_370_xp_mpic_of_init() would be called with a 'device_node *'
> > > that points to the main-intc, correct? Then it would have to go back up
> > > in the Device Tree to find the msi node? It's doable of course, but
> > > sounds strange, no?
> > 
> > I was thinking of registering two init functions as well.
> 
> But then which of the two init functions would do the of_iomap() (or
> whatever ioremap()ing function you use) ?

I agree this is where it get a little fishy, hence my preference to
use a single node.

> Remember, the very reason what we have one driver for both interrupt
> controllers is because the registers are completely mixed. So to me
> it's really the case of one device providing two features, like a
> device that would be both an Ethernet interface and a SPI controller.
> You have a single ->probe() function that gets called when the device
> is detected, and this ->probe() function registers both an Ethernet
> interface and a SPI controller.b
> 
> To me, the case we have here is really identical: we have one single set
> of registers that provide multiple features.

The problem here is that the irq subsystem makes certain assumptions
about one device node being the controller and mapping to a single
irq domain, so you'd have to cheat one way or another. Using the
sub-nodes the way you do in your patch is bending the rules for
the device tree binding, which I think is more problematic than
bending the rules for the code. Regarding the call to of_iomap(),
you could work around it by having a static variable in the driver
that remembers the mmio address and gets set by whichever device node
init function gets called first.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Jason Gunthorpe]

On Tue, Mar 26, 2013 at 09:46:13PM +0100, Thomas Petazzoni wrote:

> To me, the topology of the hardware is really that a single device
> provides two features: the main interrupt controller and the MSI
> interrupt controller. But I will adapt to whatever DT binding you
> propose.

No.. the HW is a single device that provides an interrupt on register
write capability, so it ideally should be a single DT node..

The need to distinguish MSI vs IPI vs other usage is completely a side
effect of how Linux's IRQ and PCI layers are hooked together today.

> > I still wonder if the real solution shouldn't instead be to make the
> > irq domain code MSI aware. For instance, you don't really need a
> > cell to describe an interrupt because the interrupt number is

Some kind of generic way for an irq chip driver to say 'here, I can
provide some MSI interrupts' and then for the PCI layer to say 'irq
layer, find me a driver that can provision a MSI with XXX properties' ?

This need to stack an irq chip under a MSI is not something I think
the kernel has had to support before, so new common code is probably
needed...

The interrupt chip should not need to know what the ultimate consumer
of the interrupt capability will be, it just needs to tell the
consumer 'write D to physical address A and irq I will trigger'.

Jason

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 26 Mar 2013 15:14:03 -0600, Jason Gunthorpe wrote:
> On Tue, Mar 26, 2013 at 09:46:13PM +0100, Thomas Petazzoni wrote:
> 
> > To me, the topology of the hardware is really that a single device
> > provides two features: the main interrupt controller and the MSI
> > interrupt controller. But I will adapt to whatever DT binding you
> > propose.
> 
> No.. the HW is a single device that provides an interrupt on register
> write capability, so it ideally should be a single DT node..

No, here you're only talking about the interrupt on register write
capability (used for doorbells), but the interrupt controller is also
used for all other devices. Not only IPIs and MSIs are handled by this
piece of code.

> The need to distinguish MSI vs IPI vs other usage is completely a side
> effect of how Linux's IRQ and PCI layers are hooked together today.

Yes. And since I live in today's world, I try to adapt somewhat to
it :-)

> > > I still wonder if the real solution shouldn't instead be to make the
> > > irq domain code MSI aware. For instance, you don't really need a
> > > cell to describe an interrupt because the interrupt number is
> 
> Some kind of generic way for an irq chip driver to say 'here, I can
> provide some MSI interrupts' and then for the PCI layer to say 'irq
> layer, find me a driver that can provision a MSI with XXX properties' ?
> 
> This need to stack an irq chip under a MSI is not something I think
> the kernel has had to support before, so new common code is probably
> needed...
> 
> The interrupt chip should not need to know what the ultimate consumer
> of the interrupt capability will be, it just needs to tell the
> consumer 'write D to physical address A and irq I will trigger'.

I honestly don't see much difference between what you're saying here
and what the proposed code is doing. The IRQ driver says "hay, I
provide two IRQ domains, one is named mpic, one is named msi". And then
all the regular devices have interrupt-parent = <&mpic> to say "I use
legacy interrupts from that controller", and the PCIe controller has
msi-parent = <&msi> to say "I use MSI interrupt from that controller".

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Jason Gunthorpe]

On Tue, Mar 26, 2013 at 05:52:22PM +0100, Thomas Petazzoni wrote:

> This commit introduces the support for the MSI interrupts in the
> armada-370-xp interrupt controller driver. It registers an IRQ domain
> associated with the 'msi' node in the Device Tree, which the PCI
> controller will refer to in a followup commit.

I've got some MSI stuff working on Kirkwood using the doorbells, so I
can confirm this general approach works in HW.

What do you think it would take to extend this to other Marvell SOCs?

BTW, in my testing on Kirkwood I found that register ..40010 in the
PEX had to be set to the internal register base to make this work. Did
you find something similar?

> The MSI interrupts use the 16 high doorbells, and are therefore
> notified using IRQ1 of the main interrupt controller.

I was initially a bit surprised this was the high doorbell bits, but I
checked and it is right, the 16 bit MSI maps to the high two bytes in
the 32 bit MemWr TLP.

FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
the PCI layer if it is setting up MSI or MSI-X you could allocate low
bits first to MSI-X and high bits first to MSI, increasing the number
of available MSI/MSI-X vectors.

> +   - marvell,doorbell: Gives the physical address at which PCIe
> +     devices should write to signal an MSI interrupt.

Why is this necessary? Can't the doorbell register physical address be
computed by the driver? AFAIK there is no possibility for address
translation on SOC inbound TLPs.

Thinking about it a bit, maybe less magic code is needed here, be
explicit about the available interrupts in the DT:

pcie-controller {
   msi-interrupts = <0xd0020a04 (1<<16) &msi 16
                     0xd0020a04 (1<<17) &msi 17
                     [..]
   msi-x-interrupts = <0xd0020a04 (1<<1) &msi 1
                       0xd0020a04 (1<<2) &msi 2
                       [..]

There is a better chance of that supporting other Marvell SOCs.. Not
sure, just throwing it out there.

Regarding the sub node, I'm not sure it should be necessary. You
don't need to differentiate the MSI vs non-MSI case in the doorbell
register@the interrupt controller level. Eg this ..
  
> +#ifdef CONFIG_PCI_MSI
> +static struct irq_chip armada_370_xp_msi_irq_chip = {
> +	.name = "armada_370_xp_msi_irq",
> +	.irq_enable = unmask_msi_irq,
> +	.irq_disable = mask_msi_irq,
> +	.irq_mask = mask_msi_irq,
> +	.irq_unmask = unmask_msi_irq,
> +};

.. isn't necessary for doorbell. With MSI cases on other archs there
is no local MSI interrupt controller, the MSI MemWr TLP directly
creates an interrupt at the CPU. This requires using the *_msi_irq
functions to control the interrupt at the source, since there is no
control at the destination.

However, Marvell's doorbell can be controlled at the destination, so
it is better to handle it that way, especially since it creates
symmetry with the IPI usage.

I used:
       priv->gc = irq_alloc_generic_chip("kirkwood_doorbell_irq", 1,
                                   irq_base, priv->base,
                                   handle_fasteoi_irq);
       [..]				  
       ct->regs.mask = 4;
       ct->regs.eoi = 0;
       ct->chip.irq_eoi = irq_gc_eoi_inv;
       ct->chip.irq_mask = irq_gc_mask_clr_bit;
       ct->chip.irq_unmask = irq_gc_mask_set_bit;

Which should work correctly for the IPI and MSI cases. The handle_fasteoi_irq
is important.

>  #ifdef CONFIG_SMP
>  void armada_mpic_send_doorbell(const struct cpumask *mask, unsigned int irq)
>  {
> @@ -220,12 +280,39 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
>  		if (irqnr > 1022)
>  			break;
>  
> -		if (irqnr > 0) {
> +		if (irqnr > 1) {
>  			irqnr =	irq_find_mapping(armada_370_xp_mpic_domain,
>  					irqnr);
>  			handle_IRQ(irqnr, regs);
>  			continue;
>  		}
> +
> +#ifdef CONFIG_PCI_MSI
> +		/* MSI handling */
> +		if (irqnr == 1) {
> +			u32 msimask, msinr;
> +
> +			msimask = readl_relaxed(per_cpu_int_base +
> +					ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
> +				& PCI_MSI_DOORBELL_MASK;
> +
> +			writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base +
> +			       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);

Be careful here, the ordering of the EOI in relation to the handler is
important. If you use my stuff from above then the MSI and IPI cases
are identical and the core code handles ack'ing the doorbell via
irq_eoi at the right moment, you can just uniformly iterate over all
32 bits and there is no need to care if it is MSI or IPI.

Also, I'm not super familiar with the irq stuff, but is
irq_find_mapping the best way? Most of the drivers I looked at used
irq_alloc_descs to get a contiguous range of irq numbers and then just
used a simple offset in the handle_irq...

Jason

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 26 Mar 2013 12:02:45 -0600, Jason Gunthorpe wrote:

> > This commit introduces the support for the MSI interrupts in the
> > armada-370-xp interrupt controller driver. It registers an IRQ
> > domain associated with the 'msi' node in the Device Tree, which the
> > PCI controller will refer to in a followup commit.
> 
> I've got some MSI stuff working on Kirkwood using the doorbells, so I
> can confirm this general approach works in HW.
> 
> What do you think it would take to extend this to other Marvell SOCs?

A bit of time. It is on my list to use the PCIe driver on Kirkwood and
other Marvell SoCs as well.

> BTW, in my testing on Kirkwood I found that register ..40010 in the
> PEX had to be set to the internal register base to make this work. Did
> you find something similar?

The original code I used was prepared by Lior Amsalem, and it seems
like it was not needed, maybe because the bootloader had already
configured the PCIe interfaces. I'll have a look to see if those
registers already contain the right values, but certainly it would be
safer if the PCIe driver was setting their value. Not a big deal to
fix, I believe.

> > The MSI interrupts use the 16 high doorbells, and are therefore
> > notified using IRQ1 of the main interrupt controller.
> 
> I was initially a bit surprised this was the high doorbell bits, but I
> checked and it is right, the 16 bit MSI maps to the high two bytes in
> the 32 bit MemWr TLP.

Right. The low 8 bits are used for the IPIs, and the high 16 bits are
used for MSIs. I think it has been chosen to be like this because then
we have two different interrupt numbers for the IPIs and the MSIs,
which make the thing easy to handle in the IRQ controller driver.

> FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
> the PCI layer if it is setting up MSI or MSI-X you could allocate low
> bits first to MSI-X and high bits first to MSI, increasing the number
> of available MSI/MSI-X vectors.

This could be an improvement. There are also other, non-per-CPU,
doorbell interrupts that could potentially be used. Can we consider
this a possible improvement, and not something that is fundamentally
necessary? For now, I'm trying to get the current feature set merged,
and not necessarily to extend it to cover all possible features of the
hardware.

> > +   - marvell,doorbell: Gives the physical address at which PCIe
> > +     devices should write to signal an MSI interrupt.
> 
> Why is this necessary? Can't the doorbell register physical address be
> computed by the driver? AFAIK there is no possibility for address
> translation on SOC inbound TLPs.

It is the responsibility of the PCIe driver to prepare the 'struct
msi_msg', which contains the physical address at which the PCIe device
should write to trigger an MSI. But this physical address is part of
the interrupt controller registers, so there is no way for the PCIe
driver to magically know about it.

Please see the code where we're using this:
https://github.com/MISL-EBU-System-SW/mainline-public/blob/marvell-pcie-msi-v1/drivers/pci/host/pci-mvebu.c#L844.

If you think this physical address can be "computed" by the driver, I'm
all open to suggestions.

> Thinking about it a bit, maybe less magic code is needed here, be
> explicit about the available interrupts in the DT:
> 
> pcie-controller {
>    msi-interrupts = <0xd0020a04 (1<<16) &msi 16
>                      0xd0020a04 (1<<17) &msi 17
>                      [..]
>    msi-x-interrupts = <0xd0020a04 (1<<1) &msi 1
>                        0xd0020a04 (1<<2) &msi 2
>                        [..]
> 
> There is a better chance of that supporting other Marvell SOCs.. Not
> sure, just throwing it out there.

Isn't that very verbose, to list each and every MSI interrupt, bit per
bit? I'm fine with doing that (except maybe implement both MSI and
MSI-X support, I'd like to stick with the current feature set for now),
but it sounds like a lot more code in the DT and a lot more code in the
driver to parse this... just to get the exact same feature.

Arnd, what is your feeling about this suggestion?

> Regarding the sub node, I'm not sure it should be necessary. You
> don't need to differentiate the MSI vs non-MSI case in the doorbell
> register at the interrupt controller level. Eg this ..
>   
> > +#ifdef CONFIG_PCI_MSI
> > +static struct irq_chip armada_370_xp_msi_irq_chip = {
> > +	.name = "armada_370_xp_msi_irq",
> > +	.irq_enable = unmask_msi_irq,
> > +	.irq_disable = mask_msi_irq,
> > +	.irq_mask = mask_msi_irq,
> > +	.irq_unmask = unmask_msi_irq,
> > +};
> 
> .. isn't necessary for doorbell. With MSI cases on other archs there
> is no local MSI interrupt controller, the MSI MemWr TLP directly
> creates an interrupt at the CPU. This requires using the *_msi_irq
> functions to control the interrupt at the source, since there is no
> control at the destination.
> 
> However, Marvell's doorbell can be controlled at the destination, so
> it is better to handle it that way, especially since it creates
> symmetry with the IPI usage.

Hum, ok. But the MSI and IPI are handled quite differently: MSIs have
to call handle_IRQ(), while IPIs have to call handle_IPI(), so you'd
still have to distinguish between IPIs and MSIs. In the current driver,
IPIs don't have an associated irq_chip structure.

> I used:
>        priv->gc = irq_alloc_generic_chip("kirkwood_doorbell_irq", 1,
>                                    irq_base, priv->base,
>                                    handle_fasteoi_irq);
>        [..]				  
>        ct->regs.mask = 4;
>        ct->regs.eoi = 0;
>        ct->chip.irq_eoi = irq_gc_eoi_inv;
>        ct->chip.irq_mask = irq_gc_mask_clr_bit;
>        ct->chip.irq_unmask = irq_gc_mask_set_bit;
> 
> Which should work correctly for the IPI and MSI cases. The
> handle_fasteoi_irq is important.

I don't understand how this can end up calling handle_IPI() when IPIs
are raised. There are no IPIs on Kirkwood, so are you sure the Kirkwood
logic can apply to the SMP case of Armada XP, which requires IPIs?

> >  #ifdef CONFIG_SMP
> >  void armada_mpic_send_doorbell(const struct cpumask *mask,
> > unsigned int irq) {
> > @@ -220,12 +280,39 @@ armada_370_xp_handle_irq(struct pt_regs *regs)
> >  		if (irqnr > 1022)
> >  			break;
> >  
> > -		if (irqnr > 0) {
> > +		if (irqnr > 1) {
> >  			irqnr =
> > irq_find_mapping(armada_370_xp_mpic_domain, irqnr);
> >  			handle_IRQ(irqnr, regs);
> >  			continue;
> >  		}
> > +
> > +#ifdef CONFIG_PCI_MSI
> > +		/* MSI handling */
> > +		if (irqnr == 1) {
> > +			u32 msimask, msinr;
> > +
> > +			msimask = readl_relaxed(per_cpu_int_base +
> > +
> > ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
> > +				& PCI_MSI_DOORBELL_MASK;
> > +
> > +			writel(~PCI_MSI_DOORBELL_MASK,
> > per_cpu_int_base +
> > +			       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
> 
> Be careful here, the ordering of the EOI in relation to the handler is
> important. If you use my stuff from above then the MSI and IPI cases
> are identical and the core code handles ack'ing the doorbell via
> irq_eoi at the right moment, you can just uniformly iterate over all
> 32 bits and there is no need to care if it is MSI or IPI.

Again, I don't see how it's possible to not care whether it's MSI or
IPI. IPIs have to call handle_IPI() which is a ARM-specific call, and I
don't understand how handle_fasteoi_irq() would end up calling
handle_IPI().

> Also, I'm not super familiar with the irq stuff, but is
> irq_find_mapping the best way? Most of the drivers I looked at used
> irq_alloc_descs to get a contiguous range of irq numbers and then just
> used a simple offset in the handle_irq...

I'll let Arnd answer this one, but I'm pretty sure that using IRQ
domains is the way to go. The fact that a number of drivers don't yet
use IRQ domains is maybe just because they haven't been converted yet.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> > FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
> > the PCI layer if it is setting up MSI or MSI-X you could allocate low
> > bits first to MSI-X and high bits first to MSI, increasing the number
> > of available MSI/MSI-X vectors.
> 
> This could be an improvement. There are also other, non-per-CPU,
> doorbell interrupts that could potentially be used. Can we consider
> this a possible improvement, and not something that is fundamentally
> necessary? For now, I'm trying to get the current feature set merged,
> and not necessarily to extend it to cover all possible features of the
> hardware.

If we are extending the DT binding for the current feature, we should
at least think about how it would look like for future extensions, to
make sure it won't be fundamentally incompatible.

> > > +   - marvell,doorbell: Gives the physical address at which PCIe
> > > +     devices should write to signal an MSI interrupt.
> > 
> > Why is this necessary? Can't the doorbell register physical address be
> > computed by the driver? AFAIK there is no possibility for address
> > translation on SOC inbound TLPs.
> 
> It is the responsibility of the PCIe driver to prepare the 'struct
> msi_msg', which contains the physical address at which the PCIe device
> should write to trigger an MSI. But this physical address is part of
> the interrupt controller registers, so there is no way for the PCIe
> driver to magically know about it.

If we introduce an irq_find_msi_host() interface, we can also introduce
an interface to return the doorbell register, or more. I suppose
we could actually have a generic version of your mvebu_pcie_setup_msi_irq()
function that looks up the domain from the device node and calls
a new irq_domain_ops function, which allocates a free MSI hwirq number,
creates a mapping for it, and fills out a struct msi_msg with the
doorbell register and data.

> > Thinking about it a bit, maybe less magic code is needed here, be
> > explicit about the available interrupts in the DT:
> > 
> > pcie-controller {
> >    msi-interrupts = <0xd0020a04 (1<<16) &msi 16
> >                      0xd0020a04 (1<<17) &msi 17
> >                      [..]
> >    msi-x-interrupts = <0xd0020a04 (1<<1) &msi 1
> >                        0xd0020a04 (1<<2) &msi 2
> >                        [..]
> > 
> > There is a better chance of that supporting other Marvell SOCs.. Not
> > sure, just throwing it out there.
> 
> Isn't that very verbose, to list each and every MSI interrupt, bit per
> bit? I'm fine with doing that (except maybe implement both MSI and
> MSI-X support, I'd like to stick with the current feature set for now),
> but it sounds like a lot more code in the DT and a lot more code in the
> driver to parse this... just to get the exact same feature.
> 
> Arnd, what is your feeling about this suggestion?

I think we should only need an msi-parent property and let the details
be handled by the irq driver.
 
> > Also, I'm not super familiar with the irq stuff, but is
> > irq_find_mapping the best way? Most of the drivers I looked at used
> > irq_alloc_descs to get a contiguous range of irq numbers and then just
> > used a simple offset in the handle_irq...
> 
> I'll let Arnd answer this one, but I'm pretty sure that using IRQ
> domains is the way to go. The fact that a number of drivers don't yet
> use IRQ domains is maybe just because they haven't been converted yet.

Yes, irq_find_mapping is what we should be using here.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 21:31:45 +0000, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> > > FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
> > > the PCI layer if it is setting up MSI or MSI-X you could allocate low
> > > bits first to MSI-X and high bits first to MSI, increasing the number
> > > of available MSI/MSI-X vectors.
> > 
> > This could be an improvement. There are also other, non-per-CPU,
> > doorbell interrupts that could potentially be used. Can we consider
> > this a possible improvement, and not something that is fundamentally
> > necessary? For now, I'm trying to get the current feature set merged,
> > and not necessarily to extend it to cover all possible features of the
> > hardware.
> 
> If we are extending the DT binding for the current feature, we should
> at least think about how it would look like for future extensions, to
> make sure it won't be fundamentally incompatible.

Sure.

> > > > +   - marvell,doorbell: Gives the physical address at which PCIe
> > > > +     devices should write to signal an MSI interrupt.
> > > 
> > > Why is this necessary? Can't the doorbell register physical address be
> > > computed by the driver? AFAIK there is no possibility for address
> > > translation on SOC inbound TLPs.
> > 
> > It is the responsibility of the PCIe driver to prepare the 'struct
> > msi_msg', which contains the physical address at which the PCIe device
> > should write to trigger an MSI. But this physical address is part of
> > the interrupt controller registers, so there is no way for the PCIe
> > driver to magically know about it.
> 
> If we introduce an irq_find_msi_host() interface, we can also introduce
> an interface to return the doorbell register, or more. I suppose
> we could actually have a generic version of your mvebu_pcie_setup_msi_irq()
> function that looks up the domain from the device node and calls
> a new irq_domain_ops function, which allocates a free MSI hwirq number,
> creates a mapping for it, and fills out a struct msi_msg with the
> doorbell register and data.

Ok, sounds like a plan. I must admit I'm not very familiar with the IRQ
domain code, but I guess I should take this as an opportunity to become
a little bit more familiar :)

> > > Thinking about it a bit, maybe less magic code is needed here, be
> > > explicit about the available interrupts in the DT:
> > > 
> > > pcie-controller {
> > >    msi-interrupts = <0xd0020a04 (1<<16) &msi 16
> > >                      0xd0020a04 (1<<17) &msi 17
> > >                      [..]
> > >    msi-x-interrupts = <0xd0020a04 (1<<1) &msi 1
> > >                        0xd0020a04 (1<<2) &msi 2
> > >                        [..]
> > > 
> > > There is a better chance of that supporting other Marvell SOCs.. Not
> > > sure, just throwing it out there.
> > 
> > Isn't that very verbose, to list each and every MSI interrupt, bit per
> > bit? I'm fine with doing that (except maybe implement both MSI and
> > MSI-X support, I'd like to stick with the current feature set for now),
> > but it sounds like a lot more code in the DT and a lot more code in the
> > driver to parse this... just to get the exact same feature.
> > 
> > Arnd, what is your feeling about this suggestion?
> 
> I think we should only need an msi-parent property and let the details
> be handled by the irq driver.

Ok. Having the irq driver allocate the MSI interrupt number as you
suggested above seems a good idea.

> > > Also, I'm not super familiar with the irq stuff, but is
> > > irq_find_mapping the best way? Most of the drivers I looked at used
> > > irq_alloc_descs to get a contiguous range of irq numbers and then just
> > > used a simple offset in the handle_irq...
> > 
> > I'll let Arnd answer this one, but I'm pretty sure that using IRQ
> > domains is the way to go. The fact that a number of drivers don't yet
> > use IRQ domains is maybe just because they haven't been converted yet.
> 
> Yes, irq_find_mapping is what we should be using here.

Ok, thanks.

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Jason Gunthorpe]

On Tue, Mar 26, 2013 at 10:16:54PM +0100, Thomas Petazzoni wrote:

> > FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
> > the PCI layer if it is setting up MSI or MSI-X you could allocate low
> > bits first to MSI-X and high bits first to MSI, increasing the number
> > of available MSI/MSI-X vectors.
> 
> This could be an improvement. There are also other, non-per-CPU,
> doorbell interrupts that could potentially be used. Can we consider
> this a possible improvement, and not something that is fundamentally
> necessary? For now, I'm trying to get the current feature set merged,
> and not necessarily to extend it to cover all possible features of the
> hardware.

A major point of MSI is to be able to direct interrupts on a CPU by
CPU basis. Looks like XP has per-cpu and all-cpu doorbell bits?

Combined with this:

> It is the responsibility of the PCIe driver to prepare the 'struct
> msi_msg', which contains the physical address at which the PCIe device
> should write to trigger an MSI. But this physical address is part of

Makes me think the split of responsibility created by moving the MSI
ops into the PCI host structure is not correct.

The PCIe host driver just seems to get in the way, it has no knowledge
it is adding to the process.

irqchip knows:
 - what the physical address of the doorbell is
 - how to construct an address that is per-cpu or all-cpu
 - which bits in the doorbell registers are allocated and which are
   free

pci has none of that info.

Looking at this some more, there is tonnes of stuff in linux that when
a PCI MSI is allocated a special IRQ number is created for it that has
special properties - eg set_affinity on that number actually goes into
the MSI table and changes it.

The cleanest would be to keep the doorbell driver purely related to
the doorbell and when a request for a PCI MSI comes in allocate a new
irq_chip (like arch/x86/kernel/apic/io_apic.c does) that has all the
special PCI stuff and chain it to the proper bit in the doorbell. 

Optimizing to remove function calls from the interrupt stack could
happen later.

> > However, Marvell's doorbell can be controlled at the destination, so
> > it is better to handle it that way, especially since it creates
> > symmetry with the IPI usage.
> 
> Hum, ok. But the MSI and IPI are handled quite differently: MSIs have
> to call handle_IRQ(), while IPIs have to call handle_IPI(), so you'd
> still have to distinguish between IPIs and MSIs. In the current driver,
> IPIs don't have an associated irq_chip structure.

Once you have a proper generic interrupt driver you can go ahead and
use request_irq to grab N bits of the doorbell register and assign
them to a handler that only calls handle_IPI(ipinr,get_irq_regs()).

It is not necessary to keep IPI and the irqchip driver convoluted
together.

> Again, I don't see how it's possible to not care whether it's MSI or
> IPI. IPIs have to call handle_IPI() which is a ARM-specific call, and I
> don't understand how handle_fasteoi_irq() would end up calling
> handle_IPI().

- The main IRQ vector is entered, it decodes the main cause register
  and calls the handler for the doorbell
- The doorbell handler is setup as a chained handler and it uses
  handle_fasteoi_irq to enter into armada_370_xp_handle_irq
- armada_370_xp_handle_irq then runs through all bits and calls their
  handlers
- the handler for IPI bits is associated with the IPI handler that
  simply calls handle_IPI(...)

handle_fasteoi_irq acks's and clears the handled bits in the doorbell
register at the proper time.

> I'll let Arnd answer this one, but I'm pretty sure that using IRQ
> domains is the way to go. The fact that a number of drivers don't yet
> use IRQ domains is maybe just because they haven't been converted yet.

Maybe, but they have irq domain code as well.. I'm curious about the
answer too :)

Jason

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Jason Gunthorpe wrote:
> > I'll let Arnd answer this one, but I'm pretty sure that using IRQ
> > domains is the way to go. The fact that a number of drivers don't yet
> > use IRQ domains is maybe just because they haven't been converted yet.
> 
> Maybe, but they have irq domain code as well.. I'm curious about the
> answer too :)

To expand on my previous answer: irq_alloc_descs can require a lot of
memory if you have a lot of MSIs. If the driver only has 16 MSIs,
there is probably no reason to not assign all of them at once, but if
you have 2048 MSIs, creating the mapping only when you need it is
much more space efficient.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Jason Gunthorpe,

On Tue, 26 Mar 2013 15:55:46 -0600, Jason Gunthorpe wrote:

> > > FWIW, MSI-X is not restricted to 16 bits, so if you can detect from
> > > the PCI layer if it is setting up MSI or MSI-X you could allocate low
> > > bits first to MSI-X and high bits first to MSI, increasing the number
> > > of available MSI/MSI-X vectors.
> > 
> > This could be an improvement. There are also other, non-per-CPU,
> > doorbell interrupts that could potentially be used. Can we consider
> > this a possible improvement, and not something that is fundamentally
> > necessary? For now, I'm trying to get the current feature set merged,
> > and not necessarily to extend it to cover all possible features of the
> > hardware.
> 
> A major point of MSI is to be able to direct interrupts on a CPU by
> CPU basis. Looks like XP has per-cpu and all-cpu doorbell bits?

Yes. Two per-CPU interrupts, IRQ 0 for the low 16 bits of the first
doorbell register, IRQ 1 for the high 16 bits of the first doorbell
register. And then, three global interrupts, each with a 32 bits
register associated to trigger the interrupt. At least, that's my
understanding of the datasheet.

> Combined with this:
> 
> > It is the responsibility of the PCIe driver to prepare the 'struct
> > msi_msg', which contains the physical address at which the PCIe device
> > should write to trigger an MSI. But this physical address is part of
> 
> Makes me think the split of responsibility created by moving the MSI
> ops into the PCI host structure is not correct.
> 
> The PCIe host driver just seems to get in the way, it has no knowledge
> it is adding to the process.
> 
> irqchip knows:
>  - what the physical address of the doorbell is
>  - how to construct an address that is per-cpu or all-cpu
>  - which bits in the doorbell registers are allocated and which are
>    free
> 
> pci has none of that info.

Wait, wait. Did you look at the Tegra PCIe driver? In the Tegra case,
the MSI stuff is handled completely by the PCIe unit, and does not need
the special interaction with the IRQ controller driver that I need.

The fact that PCIe and MSI handling are completely separate matters on
Marvell may just be a specific situation. On other platforms, the
physical register that gets written to by PCIe devices to trigger a MSI
may well be located within the PCIe interface registers, and therefore
the MSI thing should be handled by the PCIe driver.

> Looking at this some more, there is tonnes of stuff in linux that when
> a PCI MSI is allocated a special IRQ number is created for it that has
> special properties - eg set_affinity on that number actually goes into
> the MSI table and changes it.
> 
> The cleanest would be to keep the doorbell driver purely related to
> the doorbell and when a request for a PCI MSI comes in allocate a new
> irq_chip (like arch/x86/kernel/apic/io_apic.c does) that has all the
> special PCI stuff and chain it to the proper bit in the doorbell. 
> 
> Optimizing to remove function calls from the interrupt stack could
> happen later.
> 
> > > However, Marvell's doorbell can be controlled at the destination, so
> > > it is better to handle it that way, especially since it creates
> > > symmetry with the IPI usage.
> > 
> > Hum, ok. But the MSI and IPI are handled quite differently: MSIs have
> > to call handle_IRQ(), while IPIs have to call handle_IPI(), so you'd
> > still have to distinguish between IPIs and MSIs. In the current driver,
> > IPIs don't have an associated irq_chip structure.
> 
> Once you have a proper generic interrupt driver you can go ahead and
> use request_irq to grab N bits of the doorbell register and assign
> them to a handler that only calls handle_IPI(ipinr,get_irq_regs()).
> 
> It is not necessary to keep IPI and the irqchip driver convoluted
> together.
> 
> > Again, I don't see how it's possible to not care whether it's MSI or
> > IPI. IPIs have to call handle_IPI() which is a ARM-specific call, and I
> > don't understand how handle_fasteoi_irq() would end up calling
> > handle_IPI().
> 
> - The main IRQ vector is entered, it decodes the main cause register
>   and calls the handler for the doorbell
> - The doorbell handler is setup as a chained handler and it uses
>   handle_fasteoi_irq to enter into armada_370_xp_handle_irq
> - armada_370_xp_handle_irq then runs through all bits and calls their
>   handlers
> - the handler for IPI bits is associated with the IPI handler that
>   simply calls handle_IPI(...)
> 
> handle_fasteoi_irq acks's and clears the handled bits in the doorbell
> register at the proper time.

Could you propose some code that implements this? The existing code
works fine, and is modeled after what the GIC IRQ driver is doing. I
don't say what you're proposing is not interesting, but just that it
looks like every time I come with some code, you're suggesting me to
'reinvent' the whole world, and you've never come up with some code to
help in a direction or another.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Jason Gunthorpe]

On Tue, Mar 26, 2013 at 11:06:56PM +0100, Thomas Petazzoni wrote:

> > The PCIe host driver just seems to get in the way, it has no knowledge
> > it is adding to the process.
> > 
> > irqchip knows:
> >  - what the physical address of the doorbell is
> >  - how to construct an address that is per-cpu or all-cpu
> >  - which bits in the doorbell registers are allocated and which are
> >    free
> > 
> > pci has none of that info.
> 
> Wait, wait. Did you look at the Tegra PCIe driver? In the Tegra case,
> the MSI stuff is handled completely by the PCIe unit, and does not need
> the special interaction with the IRQ controller driver that I need.

Yes, but only briefly. It doesn't matter if the mechanisms are in the
PCI-E register block or someplace else. If irqchip is providing the
interface then the PCI-E driver would have to also instantiate an
irqchip block.

This is no different from what would be required to handle INTx on
Marvell, the PCI-E driver would have to create a chained irqchip from
the PEX interrupt and decode the cause register into INTA/B/C/D.

> > > Again, I don't see how it's possible to not care whether it's MSI or
> > > IPI. IPIs have to call handle_IPI() which is a ARM-specific call, and I
> > > don't understand how handle_fasteoi_irq() would end up calling
> > > handle_IPI().
> > 
> > - The main IRQ vector is entered, it decodes the main cause register
> >   and calls the handler for the doorbell
> > - The doorbell handler is setup as a chained handler and it uses
> >   handle_fasteoi_irq to enter into armada_370_xp_handle_irq
> > - armada_370_xp_handle_irq then runs through all bits and calls their
> >   handlers
> > - the handler for IPI bits is associated with the IPI handler that
> >   simply calls handle_IPI(...)
> > 
> > handle_fasteoi_irq acks's and clears the handled bits in the doorbell
> > register at the proper time.
> 
> Could you propose some code that implements this? The existing code
> works fine, and is modeled after what the GIC IRQ driver is doing. I
> don't say what you're proposing is not interesting, but just that it
> looks like every time I come with some code, you're suggesting me to
> 'reinvent' the whole world, and you've never come up with some code to
> help in a direction or another.

I'll send you my doorbell driver for Kirkwood - it doesn't attempt to
generically solve the MSI problem, but the basic template shows how to
wire up the doorbell concept to irqchip's generic code, and it works
in the MSI role with lots of rather ugly hacking..

The XP driver is taking some small shortcuts because it only uses the
doorbell for IPI..

Jason

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Arnd Bergmann]

On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> +                       msimask = readl_relaxed(per_cpu_int_base +
> +                                       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
> +                               & PCI_MSI_DOORBELL_MASK;
> +
> +                       writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base +
> +                              ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
> +

Upon reading this code again, I stumbled over the barriers. You use
a readl_relaxed() without barrier but a writel() with barrier. Is
that intentional? Are you sure that you don't need a full readl()
to guarantee that all inbound DMA that was sent by the device before
the MSI message has arrived by the time the interrupt handler function
is called? It depends on the implementation of the MSI controller whether
that guarantee is already made by the fact that you are handling the
interrupt.

	Arnd

[RFCv1 07/11] irqchip: armada-370-xp: add MSI support to interrupt controller driver

[Thomas Petazzoni]

Dear Arnd Bergmann,

On Tue, 26 Mar 2013 21:15:40 +0000, Arnd Bergmann wrote:
> On Tuesday 26 March 2013, Thomas Petazzoni wrote:
> > +                       msimask = readl_relaxed(per_cpu_int_base +
> > +                                       ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS)
> > +                               & PCI_MSI_DOORBELL_MASK;
> > +
> > +                       writel(~PCI_MSI_DOORBELL_MASK, per_cpu_int_base +
> > +                              ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS);
> > +
> 
> Upon reading this code again, I stumbled over the barriers. You use
> a readl_relaxed() without barrier but a writel() with barrier. Is
> that intentional? Are you sure that you don't need a full readl()
> to guarantee that all inbound DMA that was sent by the device before
> the MSI message has arrived by the time the interrupt handler function
> is called? It depends on the implementation of the MSI controller whether
> that guarantee is already made by the fact that you are handling the
> interrupt.

This question I will have to raise to the Marvell HW engineers, the
datasheet does not go into these considerations. For now, I'm rather
trying to validate the 'architecture' of the code: DT binding,
interaction between the IRQ controller driver and the PCIe driver, etc.

Thanks,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Ezequiel Garcia]

Thomas,

On Tue, Mar 26, 2013 at 05:52:15PM +0100, Thomas Petazzoni wrote:
> 
> This set of patches introduces Message Signaled Interrupt support in
> the Marvell EBU PCIe driver. It has been successfully tested on the
> Armada XP GP platform and the Armada 370 DB platform with an Intel
> e1000e PCIe network card that supports MSI.
> 
> This is based on work done by Lior Amsalem <alior@marvell.com>.
> 
> The patches do the following:
> 
>  * Patches 1, 2 and 3 move the IRQ controller driver of Armada 370/XP
>    platforms from arch/arm/mach-mvebu/ into drivers/irqchip/ and use
>    the proper irqchip infrastructure. Those changes are not strictly
>    needed to add MSI interrupts support, but since we will be touching
>    the IRQ controller driver anyway, it sounded like the right time to
>    do this move.
> 

Given the IRQ controller move to drivers/irqchip is independent of MSI
work, and that we've already agreed this move is fine (Arnd has acked
patch 2), may I suggest that you resend these three first patches
(and perhaps the fourth?) as a separate patchset to be included in v3.10.

This has the advantage that further development on IRQ controller can be
done directly on its proper place, and also the MSI patchset can be
simplified.

What do you think?
-- 
Ezequiel Garc?a, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com

[RFCv1 00/11] MSI support for Marvell EBU PCIe driver

[Thomas Petazzoni]

Dear Ezequiel Garcia,

On Thu, 4 Apr 2013 06:16:40 -0300, Ezequiel Garcia wrote:

> Given the IRQ controller move to drivers/irqchip is independent of MSI
> work, and that we've already agreed this move is fine (Arnd has acked
> patch 2), may I suggest that you resend these three first patches
> (and perhaps the fourth?) as a separate patchset to be included in v3.10.
> 
> This has the advantage that further development on IRQ controller can be
> done directly on its proper place, and also the MSI patchset can be
> simplified.
> 
> What do you think?

Sounds like a good idea. I'll cook a patch set later today and I'll
send it.

Thanks for the suggestion!

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[RFCv1 08/11] PCI: Introduce new MSI chip infrastructure

[Bjorn Helgaas]

On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> From: Thierry Reding <thierry.reding@avionic-design.de>
>
> The new struct msi_chip is used to associated an MSI controller with a
> PCI bus. It is automatically handed down from the root to its children
> during bus enumeration.
>
> This patch provides default (weak) implementations for the architecture-
> specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> and arch_msi_check_device()) which check if a PCI device's bus has an
> attached MSI chip and forward the call appropriately.
>
> Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/pci/msi.c   |   35 +++++++++++++++++++++++++++++++----
>  drivers/pci/probe.c |    1 +
>  include/linux/msi.h |   10 ++++++++++
>  include/linux/pci.h |    1 +
>  4 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 00cc78c..fce3549 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -26,14 +26,41 @@
>
>  static int pci_msi_enable = 1;
>
> -/* Arch hooks */
> +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> +{
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->setup_irq) {
> +               int err;
> +
> +               err = chip->setup_irq(chip, dev, desc);
> +               if (err < 0)
> +                       return err;
> +
> +               irq_set_chip_data(desc->irq, chip);
> +               return err;
> +       }
> +
> +       return -EINVAL;
> +}
>
> -#ifndef arch_msi_check_device
> -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> +void __weak arch_teardown_msi_irq(unsigned int irq)
>  {
> +       struct msi_chip *chip = irq_get_chip_data(irq);
> +
> +       if (chip && chip->teardown_irq)
> +               chip->teardown_irq(chip, irq);
> +}
> +
> +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)

I like the replacement of "#ifndef arch_msi_check_device()" with a
weak implementation here, but this only does half the job -- shouldn't
we remove the powerpc "#define arch_msi_check_device
arch_msi_check_device" at the same time?

And since we're touching all the check_device() implementations, maybe
we could come up with a better name.  "check_device()" conveys
absolutely no information about what we're checking or what the sense
of the result is.  arch_msi_supported()?  pcibios_msi_supported()?  I
guess it should be consistent with the other arch interfaces, so
arch_*() is probably better.

Maybe the ugly #ifdef-ery around arch_setup_msi_irqs,
arch_teardown_msi_irqs, and arch_restore_msi_irqs could be cleaned up
similarly?  Somebody worked pretty hard to obfuscate all that,
probably before weak functions were available.

> +{
> +       struct msi_chip *chip = dev->bus->msi;
> +
> +       if (chip && chip->check_device)
> +               return chip->check_device(chip, dev, nvec, type);
> +
>         return 0;
>  }
> -#endif
>
>  #ifndef arch_setup_msi_irqs
>  # define arch_setup_msi_irqs default_setup_msi_irqs
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b494066..9307550 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>
>         child->parent = parent;
>         child->ops = parent->ops;
> +       child->msi = parent->msi;
>         child->sysdata = parent->sysdata;
>         child->bus_flags = parent->bus_flags;
>
> diff --git a/include/linux/msi.h b/include/linux/msi.h
> index ce93a34..ea4a5be 100644
> --- a/include/linux/msi.h
> +++ b/include/linux/msi.h
> @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
>  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
>  extern int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
>
> +struct msi_chip {
> +       struct module *owner;
> +       struct device *dev;
> +
> +       int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> +                        struct msi_desc *desc);
> +       void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> +       int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> +                           int nvec, int type);

If we do end up adding interfaces like this (I'm not sure it will work
-- see below), I think it would be better to pass just the pci_dev,
not the "msi_chip, pci_dev" pair.  Passing both pointers avoids a
cheap lookup in the caller, but it adds a way that two inseparable
things can get out of sync.

> +};
>
>  #endif /* LINUX_MSI_H */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 2461033a..6aca43ea 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -416,6 +416,7 @@ struct pci_bus {
>         struct resource busn_res;       /* bus numbers routed to this bus */
>
>         struct pci_ops  *ops;           /* configuration access functions */
> +       struct msi_chip *msi;           /* MSI controller */

"msi" seems like a too-generic name here; it suggests an interrupt or
IRQ, not a controller.

I'm not sure this is the correct place for it.  Having it in the
struct pci_bus means you need arch code to fill it in, e.g., you added
it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
that for arches that use pci_scan_root_bus(), which is the direction
I'd like to go.

I think it probably should go in sysdata instead.  That would mean you
can't really do generic weak setup/tear-down functions, because they
wouldn't know how to pull the MSI controller info out of the
arch-specific sysdata.  But there are so many levels of weak-ness
going on there, maybe it would be a good thing to get rid of one :)

Bjorn

>         void            *sysdata;       /* hook for sys-specific extension */
>         struct proc_dir_entry *procdir; /* directory entry in /proc/bus/pci */
>
> --
> 1.7.9.5
>

[RFCv1 08/11] PCI: Introduce new MSI chip infrastructure

[Andrew Murray]

On Mon, Apr 08, 2013 at 11:28:58PM +0100, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > From: Thierry Reding <thierry.reding@avionic-design.de>
> >
> >  #endif /* LINUX_MSI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2461033a..6aca43ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -416,6 +416,7 @@ struct pci_bus {
> >         struct resource busn_res;       /* bus numbers routed to this bus */
> >
> >         struct pci_ops  *ops;           /* configuration access functions */
> > +       struct msi_chip *msi;           /* MSI controller */
> 
> "msi" seems like a too-generic name here; it suggests an interrupt or
> IRQ, not a controller.
> 
> I'm not sure this is the correct place for it.  Having it in the
> struct pci_bus means you need arch code to fill it in, e.g., you added
> it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
> that for arches that use pci_scan_root_bus(), which is the direction
> I'd like to go.
> 
> I think it probably should go in sysdata instead.  That would mean you
> can't really do generic weak setup/tear-down functions, because they
> wouldn't know how to pull the MSI controller info out of the
> arch-specific sysdata.  But there are so many levels of weak-ness
> going on there, maybe it would be a good thing to get rid of one :)

But generic setup/tear-down functions would allow for architecture
independent MSI controllers. This would be useful for MSI controllers that
are unrelated to PCI host controllers or a specific architecture. It would
make drivers sit more comfortably in drivers/irqchip or drivers/pci/host. Also
having the msi_chip in struct pci_bus could allow there to exist multiple
MSI controllers on a PCI fabric and is consistent with pci_ops.

Assuming the MSI controller is represented in the device tree and there is a
relationship between the controller and the host bridge
(phandle/interrupt-parent) then as Thierry suggested[1] previously you could call
something like of_find_msi_chip_by_node(node) to locate an msi_chip from a
device node. Could this look up exist in pci_scan_root_bus via its struct
device.of_node? Perhaps pci_create_root_bus can be changed to take a parameter
for msi_chip alongside the ops parameter? The of_find_msi_chip_by_node could
walk up the device tree until it finds an MSI controller.

In the case where device tree isn't used - then I guess the weakly defined
arch_ callbacks would be replaced with the architectures existing
implementation. Or perhaps if MSI controllers are registered (msi_chip_add)
then pci_scan_root_bus could use the first controller it finds.

Also I believe pci_alloc_child_bus function would need to be changed to add
"b->msi = msi" to inherit msi_chip for child buses in the above patch?

Andrew Murray

[1] http://lkml.org/lkml/2013/3/25/67

[RFCv1 08/11] PCI: Introduce new MSI chip infrastructure

[Thierry Reding]

On Tue, Apr 09, 2013 at 09:11:19AM +0100, Andrew Murray wrote:
[...]
> Also I believe pci_alloc_child_bus function would need to be changed to add
> "b->msi = msi" to inherit msi_chip for child buses in the above patch?

The patch already does:

	child->msi = parent->msi;

in pci_alloc_child_bus(), the same way that ops is inherited. I have
admittedly only tested this code on Tegra, but I don't see why that
wouldn't be enough. Or maybe I haven't understood your question?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130409/99b8cee1/attachment.sig>

[RFCv1 08/11] PCI: Introduce new MSI chip infrastructure

[Andrew Murray]

On Tue, Apr 09, 2013 at 09:22:33AM +0100, Thierry Reding wrote:
> On Tue, Apr 09, 2013 at 09:11:19AM +0100, Andrew Murray wrote:
> [...]
> > Also I believe pci_alloc_child_bus function would need to be changed to add
> > "b->msi = msi" to inherit msi_chip for child buses in the above patch?
> 
> The patch already does:
> 
> 	child->msi = parent->msi;
> 
> in pci_alloc_child_bus(), the same way that ops is inherited. I have
> admittedly only tested this code on Tegra, but I don't see why that
> wouldn't be enough. Or maybe I haven't understood your question?

Sorry I missed that part of your patch - that was exacatly what I was
hoping to see.

Andrew Murray

[RFCv1 08/11] PCI: Introduce new MSI chip infrastructure

[Thierry Reding]

On Mon, Apr 08, 2013 at 04:28:58PM -0600, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > From: Thierry Reding <thierry.reding@avionic-design.de>
> >
> > The new struct msi_chip is used to associated an MSI controller with a
> > PCI bus. It is automatically handed down from the root to its children
> > during bus enumeration.
> >
> > This patch provides default (weak) implementations for the architecture-
> > specific MSI functions (arch_setup_msi_irq(), arch_teardown_msi_irq()
> > and arch_msi_check_device()) which check if a PCI device's bus has an
> > attached MSI chip and forward the call appropriately.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
> > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> > ---
> >  drivers/pci/msi.c   |   35 +++++++++++++++++++++++++++++++----
> >  drivers/pci/probe.c |    1 +
> >  include/linux/msi.h |   10 ++++++++++
> >  include/linux/pci.h |    1 +
> >  4 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index 00cc78c..fce3549 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -26,14 +26,41 @@
> >
> >  static int pci_msi_enable = 1;
> >
> > -/* Arch hooks */
> > +int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc)
> > +{
> > +       struct msi_chip *chip = dev->bus->msi;
> > +
> > +       if (chip && chip->setup_irq) {
> > +               int err;
> > +
> > +               err = chip->setup_irq(chip, dev, desc);
> > +               if (err < 0)
> > +                       return err;
> > +
> > +               irq_set_chip_data(desc->irq, chip);
> > +               return err;
> > +       }
> > +
> > +       return -EINVAL;
> > +}
> >
> > -#ifndef arch_msi_check_device
> > -int arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> > +void __weak arch_teardown_msi_irq(unsigned int irq)
> >  {
> > +       struct msi_chip *chip = irq_get_chip_data(irq);
> > +
> > +       if (chip && chip->teardown_irq)
> > +               chip->teardown_irq(chip, irq);
> > +}
> > +
> > +int __weak arch_msi_check_device(struct pci_dev *dev, int nvec, int type)
> 
> I like the replacement of "#ifndef arch_msi_check_device()" with a
> weak implementation here, but this only does half the job -- shouldn't
> we remove the powerpc "#define arch_msi_check_device
> arch_msi_check_device" at the same time?

Yes, there are a few things that can be done to cleanup the "mess"
around this subsequently. For instance I have a local patch which
completely removes the ARCH_SUPPORTS_MSI symbol because it isn't
useful after making the symbols weak. One other obvious item is to
convert more architectures to implement an MSI chip.

> And since we're touching all the check_device() implementations, maybe
> we could come up with a better name.  "check_device()" conveys
> absolutely no information about what we're checking or what the sense
> of the result is.  arch_msi_supported()?  pcibios_msi_supported()?  I
> guess it should be consistent with the other arch interfaces, so
> arch_*() is probably better.

At least on PowerPC the arch_msi_check_device() hook also refuses to
setup multiple MSI per device because it isn't supported. I can't think
of a better alternative than arch_msi_supported(), though so that's fine
with me.

Perhaps pcibios_msi_supported() wouldn't be so bad either. Other
architecture-specific functions already use that prefix (see the OF
support functions) and it might be good to settle on one convention for
consistency.

That said, the goal is to eventually get rid of all the arch_msi_*()
functions. The only reason they are still there is because I didn't want
to convert everything in one go. So I wanted to put the infrastructure
in place that we need to support multi-platform on ARM (which is given
by this generic MSI chip infrastructure). Later the remaining PCI
architectures can be converted to provide an msi_chip as well, at which
point the now weak implementations can become non-weak and be renamed to
something like pci_{setup,teardown,check}_msi() to make it more obvious
that they are generic.

> Maybe the ugly #ifdef-ery around arch_setup_msi_irqs,
> arch_teardown_msi_irqs, and arch_restore_msi_irqs could be cleaned up
> similarly?  Somebody worked pretty hard to obfuscate all that,
> probably before weak functions were available.

I think Andrew or Jason at one point commented whether they should be
allowed to be implemented by an MSI chip. If so we could use the same
approach as for the other functions.

> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index b494066..9307550 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -634,6 +634,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
> >
> >         child->parent = parent;
> >         child->ops = parent->ops;
> > +       child->msi = parent->msi;
> >         child->sysdata = parent->sysdata;
> >         child->bus_flags = parent->bus_flags;
> >
> > diff --git a/include/linux/msi.h b/include/linux/msi.h
> > index ce93a34..ea4a5be 100644
> > --- a/include/linux/msi.h
> > +++ b/include/linux/msi.h
> > @@ -58,5 +58,15 @@ extern int arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type);
> >  extern void arch_teardown_msi_irqs(struct pci_dev *dev);
> >  extern int arch_msi_check_device(struct pci_dev* dev, int nvec, int type);
> >
> > +struct msi_chip {
> > +       struct module *owner;
> > +       struct device *dev;
> > +
> > +       int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev,
> > +                        struct msi_desc *desc);
> > +       void (*teardown_irq)(struct msi_chip *chip, unsigned int irq);
> > +       int (*check_device)(struct msi_chip *chip, struct pci_dev *dev,
> > +                           int nvec, int type);
> 
> If we do end up adding interfaces like this (I'm not sure it will work
> -- see below), I think it would be better to pass just the pci_dev,
> not the "msi_chip, pci_dev" pair.  Passing both pointers avoids a
> cheap lookup in the caller, but it adds a way that two inseparable
> things can get out of sync.

Yes, that's a good idea and we can easily do that.

> > +};
> >
> >  #endif /* LINUX_MSI_H */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 2461033a..6aca43ea 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -416,6 +416,7 @@ struct pci_bus {
> >         struct resource busn_res;       /* bus numbers routed to this bus */
> >
> >         struct pci_ops  *ops;           /* configuration access functions */
> > +       struct msi_chip *msi;           /* MSI controller */
> 
> "msi" seems like a too-generic name here; it suggests an interrupt or
> IRQ, not a controller.

It's short and to the point. The bus abstraction doesn't really have any
interrupt functionality, has it? Even for PCI devices the MSI interrupt
number is actually stored in the .irq field, so I don't think it's too
generic. But if you insist I'll think of another name.

> I'm not sure this is the correct place for it.  Having it in the
> struct pci_bus means you need arch code to fill it in, e.g., you added
> it in mvebu_pcie_scan_bus() in patch 09/11.  There's no good way to do
> that for arches that use pci_scan_root_bus(), which is the direction
> I'd like to go.

We could add an argument to pci_scan_root_bus() to pass this information
in. It's a bit ugly but see below...

> I think it probably should go in sysdata instead.  That would mean you
> can't really do generic weak setup/tear-down functions, because they
> wouldn't know how to pull the MSI controller info out of the
> arch-specific sysdata.  But there are so many levels of weak-ness
> going on there, maybe it would be a good thing to get rid of one :)

Isn't putting more data into sysdata a step in the opposite direction of
where we want to go? I seem to remember some discussion about wanting to
consolidate the various architecture-specific implementations and move
more common code into the PCI core. If we handle the whole MSI business
in architecture-specific code we gain little to nothing compared to the
current situation.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130409/e25780af/attachment-0001.sig>

[RFCv1 09/11] pci: mvebu: add MSI support

[Andrew Murray]

On Tue, Mar 26, 2013 at 04:52:24PM +0000, Thomas Petazzoni wrote:
> This commit adds the MSI support for the Marvell EBU PCIe driver. The
> driver now looks at the 'msi-parent' property of the PCIe controller
> DT node, and if it exists, it gets the associated IRQ domain, which
> should be the MSI interrupt controller registered by the IRQ
> controller driver.
> 
> Using this, the PCIe driver registers the ->setup_irq() and
> ->teardown_irq() callbacks using the newly introduced msi_chip
> infrastructure, which allows the kernel PCI core to use the MSI
> functionality.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/pci/mvebu-pci.txt          |    5 +
>  drivers/pci/host/pci-mvebu.c                       |  128 ++++++++++++++++++++
>  2 files changed, 133 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> index 192bdfb..53cc437 100644
> --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> @@ -14,6 +14,10 @@ Mandatory properties:
>    corresponding registers
>  - ranges: ranges for the PCI memory and I/O regions
>  
> +Optional properties:
> +- msi-parent: a phandle pointing to the interrupt controller that
> +  handles the MSI interrupts.
> +
>  In addition, the Device Tree node must have sub-nodes describing each
>  PCIe interface, having the following mandatory properties:
>  - reg: used only for interrupt mapping, so only the first four bytes
> @@ -43,6 +47,7 @@ pcie-controller {
>  	#address-cells = <3>;
>  	#size-cells = <2>;
>  
> +	msi-parent = <&msi>;
>  	bus-range = <0x00 0xff>;
>  
>  	reg = <0xd0040000 0x2000>, <0xd0042000 0x2000>,
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 9e6b137..b46fab8 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -7,17 +7,23 @@
>   */
>  
>  #include <linux/kernel.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci.h>
> +#include <linux/msi.h>
>  #include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/mbus.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/interrupt.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>  
> +#define INT_PCI_MSI_NR (16)
> +
>  /*
>   * PCIe unit register offsets.
>   */
> @@ -101,10 +107,19 @@ struct mvebu_sw_pci_bridge {
>  
>  struct mvebu_pcie_port;
>

This structure...
 
> +struct mvebu_pcie_msi {
> +	DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> +	struct irq_domain *domain;
> +	struct mutex lock;
> +	struct msi_chip chip;
> +	phys_addr_t doorbell;
> +};

And these functions...

> +#if defined(CONFIG_PCI_MSI)
> +static int mvebu_pcie_msi_alloc(struct mvebu_pcie_msi *chip)
> +{
> +	int msi;
> +
> +	mutex_lock(&chip->lock);
> +
> +	msi = find_first_zero_bit(chip->used, INT_PCI_MSI_NR);
> +
> +	if (msi < INT_PCI_MSI_NR)
> +		set_bit(msi, chip->used);
> +	else
> +		msi = -ENOSPC;
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return msi;
> +}
> +
> +static void mvebu_pcie_msi_free(struct mvebu_pcie_msi *chip, unsigned long irq)
> +{
> +	struct device *dev = chip->chip.dev;
> +
> +	mutex_lock(&chip->lock);
> +
> +	if (!test_bit(irq, chip->used))
> +		dev_err(dev, "trying to free unused MSI#%lu\n", irq);
> +	else
> +		clear_bit(irq, chip->used);
> +
> +	mutex_unlock(&chip->lock);
> +}
> +
> +static int mvebu_pcie_setup_msi_irq(struct msi_chip *chip,
> +				    struct pci_dev *pdev,
> +				    struct msi_desc *desc)
> +{
> +	struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);
> +	struct msi_msg msg;
> +	unsigned int irq;
> +	int hwirq;
> +
> +	hwirq = mvebu_pcie_msi_alloc(msi);
> +	if (hwirq < 0)
> +		return hwirq;
> +
> +	irq = irq_create_mapping(msi->domain, hwirq);
> +	if (!irq)
> +		return -EINVAL;
> +
> +	irq_set_msi_desc(irq, desc);
> +
> +	msg.address_lo = msi->doorbell;
> +	msg.address_hi = 0;
> +	msg.data = 0xf00 | (hwirq + 16);
> +
> +	write_msi_msg(irq, &msg);
> +
> +	return 0;
> +}
> +
> +static void mvebu_pcie_teardown_msi_irq(struct msi_chip *chip,
> +					unsigned int irq)
> +{
> +	struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);
> +	struct irq_data *d = irq_get_irq_data(irq);
> +
> +	mvebu_pcie_msi_free(msi, d->hwirq);
> +}

...are not related to your PCIe driver or PCI. I remember you said that the MSI
registers are all inter-mixed with the interrupt controller. Therefore as the
above don't interact with PCIe controller registers can you move this out to an
irqchip driver or include it inside your interrupt controller?

> +
> +static int mvebu_pcie_enable_msi(struct mvebu_pcie *pcie)
> +{
> +	struct device_node *msi_node;
> +	struct mvebu_pcie_msi *msi;
> +
> +	msi = &pcie->msi;
> +
> +	mutex_init(&msi->lock);
> +
> +	msi_node = of_parse_phandle(pcie->pdev->dev.of_node,
> +				    "msi-parent", 0);
> +	if (!msi_node)
> +		return -EINVAL;
> +
> +	msi->domain = irq_find_host(msi_node);
> +	if (!msi->domain)
> +		return -EINVAL;
> +
> +	if (of_property_read_u32(msi_node, "marvell,doorbell", &msi->doorbell))
> +		return -EINVAL;
> +
> +	msi->chip.dev = &pcie->pdev->dev;
> +	msi->chip.setup_irq = mvebu_pcie_setup_msi_irq;
> +	msi->chip.teardown_irq = mvebu_pcie_teardown_msi_irq;
> +
> +	return 0;
> +}

This can change too, as per Thierry's suggestion you can call something like:

msi_chip_add(&msi)

from your interrupt controller. And then in this file you can call something
like:

of_find_msi_chip_by_node(msi_node)

and use the returned chip to assign to pcie->msi (and register with the pci_bus).
Perhaps then you may not need to expose the doorbell register as that would be
knowledge that the interrupt controller may already have (i.e. offset from base
address already provided in DT).

Andrew Murray

> +#endif /* CONFIG_PCI_MSI */
> +
>  static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  {
>  	struct mvebu_pcie *pcie;
> @@ -903,6 +1024,13 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  
>  	mvebu_pcie_enable(pcie);
>  
> +#ifdef CONFIG_PCI_MSI
> +	ret = mvebu_pcie_enable_msi(pcie);
> +	if (ret)
> +		dev_warn(&pdev->dev, "could not enable MSI support: %d\n",
> +			 ret);
> +#endif
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.9.5
> 
> 

[RFCv1 09/11] pci: mvebu: add MSI support

[Bjorn Helgaas]

On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> This commit adds the MSI support for the Marvell EBU PCIe driver. The
> driver now looks at the 'msi-parent' property of the PCIe controller
> DT node, and if it exists, it gets the associated IRQ domain, which
> should be the MSI interrupt controller registered by the IRQ
> controller driver.
>
> Using this, the PCIe driver registers the ->setup_irq() and
> ->teardown_irq() callbacks using the newly introduced msi_chip
> infrastructure, which allows the kernel PCI core to use the MSI
> functionality.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  .../devicetree/bindings/pci/mvebu-pci.txt          |    5 +
>  drivers/pci/host/pci-mvebu.c                       |  128 ++++++++++++++++++++
>  2 files changed, 133 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/pci/mvebu-pci.txt b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> index 192bdfb..53cc437 100644
> --- a/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> +++ b/Documentation/devicetree/bindings/pci/mvebu-pci.txt
> @@ -14,6 +14,10 @@ Mandatory properties:
>    corresponding registers
>  - ranges: ranges for the PCI memory and I/O regions
>
> +Optional properties:
> +- msi-parent: a phandle pointing to the interrupt controller that
> +  handles the MSI interrupts.
> +
>  In addition, the Device Tree node must have sub-nodes describing each
>  PCIe interface, having the following mandatory properties:
>  - reg: used only for interrupt mapping, so only the first four bytes
> @@ -43,6 +47,7 @@ pcie-controller {
>         #address-cells = <3>;
>         #size-cells = <2>;
>
> +       msi-parent = <&msi>;
>         bus-range = <0x00 0xff>;
>
>         reg = <0xd0040000 0x2000>, <0xd0042000 0x2000>,
> diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
> index 9e6b137..b46fab8 100644
> --- a/drivers/pci/host/pci-mvebu.c
> +++ b/drivers/pci/host/pci-mvebu.c
> @@ -7,17 +7,23 @@
>   */
>
>  #include <linux/kernel.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
>  #include <linux/pci.h>
> +#include <linux/msi.h>
>  #include <linux/clk.h>
>  #include <linux/module.h>
>  #include <linux/mbus.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/interrupt.h>
>  #include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_platform.h>
>
> +#define INT_PCI_MSI_NR (16)
> +
>  /*
>   * PCIe unit register offsets.
>   */
> @@ -101,10 +107,19 @@ struct mvebu_sw_pci_bridge {
>
>  struct mvebu_pcie_port;
>
> +struct mvebu_pcie_msi {
> +       DECLARE_BITMAP(used, INT_PCI_MSI_NR);
> +       struct irq_domain *domain;
> +       struct mutex lock;
> +       struct msi_chip chip;
> +       phys_addr_t doorbell;
> +};
> +
>  /* Structure representing all PCIe interfaces */
>  struct mvebu_pcie {
>         struct platform_device *pdev;
>         struct mvebu_pcie_port *ports;
> +       struct mvebu_pcie_msi msi;
>         struct resource io;
>         struct resource realio;
>         struct resource mem;
> @@ -546,6 +561,11 @@ static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys)
>         return sys->private_data;
>  }
>
> +static inline struct mvebu_pcie_msi *to_mvebu_msi(struct msi_chip *chip)
> +{
> +       return container_of(chip, struct mvebu_pcie_msi, chip);
> +}
> +
>  /* Find the PCIe interface that corresponds to the given bus */
>  static struct mvebu_pcie_port *mvebu_find_port_from_bus(struct mvebu_pcie *pcie,
>                                                         int bus)
> @@ -710,6 +730,8 @@ static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys)
>         if (!bus)
>                 return NULL;
>
> +       bus->msi = &pcie->msi.chip;

As I mentioned in the 08/11 patch, I'd like to keep arch-specific
stuff out of the PCI scanning path to preserve the ability to move
toward pci_scan_root_bus() eventually.

>         pci_scan_child_bus(bus);
>
>         return bus;
> @@ -786,6 +808,105 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev,
>         return devm_request_and_ioremap(&pdev->dev, &regs);
>  }
>
> +#if defined(CONFIG_PCI_MSI)
> +static int mvebu_pcie_msi_alloc(struct mvebu_pcie_msi *chip)
> +{
> +       int msi;
> +
> +       mutex_lock(&chip->lock);
> +
> +       msi = find_first_zero_bit(chip->used, INT_PCI_MSI_NR);
> +
> +       if (msi < INT_PCI_MSI_NR)
> +               set_bit(msi, chip->used);
> +       else
> +               msi = -ENOSPC;
> +
> +       mutex_unlock(&chip->lock);
> +
> +       return msi;
> +}
> +
> +static void mvebu_pcie_msi_free(struct mvebu_pcie_msi *chip, unsigned long irq)
> +{
> +       struct device *dev = chip->chip.dev;
> +
> +       mutex_lock(&chip->lock);
> +
> +       if (!test_bit(irq, chip->used))
> +               dev_err(dev, "trying to free unused MSI#%lu\n", irq);
> +       else
> +               clear_bit(irq, chip->used);
> +
> +       mutex_unlock(&chip->lock);
> +}
> +
> +static int mvebu_pcie_setup_msi_irq(struct msi_chip *chip,
> +                                   struct pci_dev *pdev,
> +                                   struct msi_desc *desc)
> +{
> +       struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);

If this took only the pci_dev (not the msi_chip), I think you could do this:

  struct mvebu_pcie_msi *msi = &pdev->bus->sysdata->msi;

> +       struct msi_msg msg;
> +       unsigned int irq;
> +       int hwirq;
> +
> +       hwirq = mvebu_pcie_msi_alloc(msi);
> +       if (hwirq < 0)
> +               return hwirq;
> +
> +       irq = irq_create_mapping(msi->domain, hwirq);
> +       if (!irq)
> +               return -EINVAL;
> +
> +       irq_set_msi_desc(irq, desc);
> +
> +       msg.address_lo = msi->doorbell;
> +       msg.address_hi = 0;
> +       msg.data = 0xf00 | (hwirq + 16);
> +
> +       write_msi_msg(irq, &msg);
> +
> +       return 0;
> +}
> +
> +static void mvebu_pcie_teardown_msi_irq(struct msi_chip *chip,
> +                                       unsigned int irq)
> +{
> +       struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);
> +       struct irq_data *d = irq_get_irq_data(irq);
> +
> +       mvebu_pcie_msi_free(msi, d->hwirq);
> +}
> +
> +static int mvebu_pcie_enable_msi(struct mvebu_pcie *pcie)
> +{
> +       struct device_node *msi_node;
> +       struct mvebu_pcie_msi *msi;
> +
> +       msi = &pcie->msi;
> +
> +       mutex_init(&msi->lock);
> +
> +       msi_node = of_parse_phandle(pcie->pdev->dev.of_node,
> +                                   "msi-parent", 0);
> +       if (!msi_node)
> +               return -EINVAL;
> +
> +       msi->domain = irq_find_host(msi_node);
> +       if (!msi->domain)
> +               return -EINVAL;
> +
> +       if (of_property_read_u32(msi_node, "marvell,doorbell", &msi->doorbell))
> +               return -EINVAL;
> +
> +       msi->chip.dev = &pcie->pdev->dev;
> +       msi->chip.setup_irq = mvebu_pcie_setup_msi_irq;
> +       msi->chip.teardown_irq = mvebu_pcie_teardown_msi_irq;
> +
> +       return 0;
> +}
> +#endif /* CONFIG_PCI_MSI */
> +
>  static int __init mvebu_pcie_probe(struct platform_device *pdev)
>  {
>         struct mvebu_pcie *pcie;
> @@ -903,6 +1024,13 @@ static int __init mvebu_pcie_probe(struct platform_device *pdev)
>
>         mvebu_pcie_enable(pcie);
>
> +#ifdef CONFIG_PCI_MSI
> +       ret = mvebu_pcie_enable_msi(pcie);
> +       if (ret)
> +               dev_warn(&pdev->dev, "could not enable MSI support: %d\n",
> +                        ret);
> +#endif
> +
>         return 0;
>  }
>
> --
> 1.7.9.5
>

[RFCv1 09/11] pci: mvebu: add MSI support

[Thierry Reding]

On Mon, Apr 08, 2013 at 04:29:07PM -0600, Bjorn Helgaas wrote:
> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
[...]
> > +static int mvebu_pcie_setup_msi_irq(struct msi_chip *chip,
> > +                                   struct pci_dev *pdev,
> > +                                   struct msi_desc *desc)
> > +{
> > +       struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);
> 
> If this took only the pci_dev (not the msi_chip), I think you could do this:
> 
>   struct mvebu_pcie_msi *msi = &pdev->bus->sysdata->msi;

That would mean that the arch_setup_msi_irq() and friends could still be
architecture-agnostic because they only pass around pci_dev, and the
driver specific implementations would know how to lookup sysdata and
from there the MSI chip.

So I was almost convinced that putting the struct msi_chip pointer into
sysdata is a good idea. However that also means that each PCI host
bridge driver becomes architecture-specific. If we ever get a driver
that can be used on multiple architectures (however unlikely), the only
way to make it work would be to #ifdef those parts. We could make that
easier to deal with by providing an accessor (pci_sysdata_set_msi_chip()
or similar), though.

But maybe it's something we don't need to be concerned about because no
PCI host bridge driver will ever support two different architecture?

One related point is compile coverage. If the drivers are completely
architecture-agnostic it makes it a lot easier to compile-test all
drivers, which might come in useful when doing core changes and such.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130530/062557b1/attachment.sig>

[RFCv1 09/11] pci: mvebu: add MSI support

[Bjorn Helgaas]

On Thu, May 30, 2013 at 6:15 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> On Mon, Apr 08, 2013 at 04:29:07PM -0600, Bjorn Helgaas wrote:
>> On Tue, Mar 26, 2013 at 10:52 AM, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:
> [...]
>> > +static int mvebu_pcie_setup_msi_irq(struct msi_chip *chip,
>> > +                                   struct pci_dev *pdev,
>> > +                                   struct msi_desc *desc)
>> > +{
>> > +       struct mvebu_pcie_msi *msi = to_mvebu_msi(chip);
>>
>> If this took only the pci_dev (not the msi_chip), I think you could do this:
>>
>>   struct mvebu_pcie_msi *msi = &pdev->bus->sysdata->msi;
>
> That would mean that the arch_setup_msi_irq() and friends could still be
> architecture-agnostic because they only pass around pci_dev, and the
> driver specific implementations would know how to lookup sysdata and
> from there the MSI chip.
>
> So I was almost convinced that putting the struct msi_chip pointer into
> sysdata is a good idea. However that also means that each PCI host
> bridge driver becomes architecture-specific. If we ever get a driver
> that can be used on multiple architectures (however unlikely), the only
> way to make it work would be to #ifdef those parts. We could make that
> easier to deal with by providing an accessor (pci_sysdata_set_msi_chip()
> or similar), though.
>
> But maybe it's something we don't need to be concerned about because no
> PCI host bridge driver will ever support two different architecture?
>
> One related point is compile coverage. If the drivers are completely
> architecture-agnostic it makes it a lot easier to compile-test all
> drivers, which might come in useful when doing core changes and such.

Hmm, I've forgotten what I was thinking here.  (And I'm the *worst*
about reviving ancient threads myself).  So I drop whatever objection
I had :)

Bjorn