| Message ID | 1436979285-8177-5-git-send-email-ddaney.cavm@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 15/07/15 17:54, David Daney wrote: > From: David Daney <david.daney@cavium.com> > > Needed to map SPI interrupt sources. > > Signed-off-by: David Daney <david.daney@cavium.com> > --- > drivers/irqchip/irq-gic-v3.c | 5 +++++ > include/linux/irqchip/arm-gic-v3.h | 1 + > 2 files changed, 6 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index c52f7ba..0019fed 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; > /* Our default, arbitrary priority value. Linux only uses one anyway. */ > #define DEFAULT_PMR_VALUE 0xf0 > > +struct irq_domain *gic_get_irq_domain(void) > +{ > + return gic_data.domain; > +} > + > static inline unsigned int gic_irq(struct irq_data *d) > { > return d->hwirq; > diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h > index 18e3757..5992224 100644 > --- a/include/linux/irqchip/arm-gic-v3.h > +++ b/include/linux/irqchip/arm-gic-v3.h > @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, > > typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); > void set_its_pci_requester_id(its_pci_requester_id_t fn); > +struct irq_domain *gic_get_irq_domain(void); > #endif > > #endif > Hmmmffff... You need the domain for SPIs?? What is wrong with putting these interrupts in your device tree? M.
On 07/15/2015 10:12 AM, Marc Zyngier wrote: > On 15/07/15 17:54, David Daney wrote: >> From: David Daney <david.daney@cavium.com> >> >> Needed to map SPI interrupt sources. >> >> Signed-off-by: David Daney <david.daney@cavium.com> >> --- >> drivers/irqchip/irq-gic-v3.c | 5 +++++ >> include/linux/irqchip/arm-gic-v3.h | 1 + >> 2 files changed, 6 insertions(+) >> >> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >> index c52f7ba..0019fed 100644 >> --- a/drivers/irqchip/irq-gic-v3.c >> +++ b/drivers/irqchip/irq-gic-v3.c >> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; >> /* Our default, arbitrary priority value. Linux only uses one anyway. */ >> #define DEFAULT_PMR_VALUE 0xf0 >> >> +struct irq_domain *gic_get_irq_domain(void) >> +{ >> + return gic_data.domain; >> +} >> + >> static inline unsigned int gic_irq(struct irq_data *d) >> { >> return d->hwirq; >> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >> index 18e3757..5992224 100644 >> --- a/include/linux/irqchip/arm-gic-v3.h >> +++ b/include/linux/irqchip/arm-gic-v3.h >> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, >> >> typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); >> void set_its_pci_requester_id(its_pci_requester_id_t fn); >> +struct irq_domain *gic_get_irq_domain(void); >> #endif >> >> #endif >> > > Hmmmffff... You need the domain for SPIs?? > > What is wrong with putting these interrupts in your device tree? > There is no device tree node for ECAM based "PCIe" devices, they are discovered in the PCI bus scan, yet they still need to use SPI interrupts. We need a way to be able to map these. David Daney > M. >
On 15/07/15 19:57, David Daney wrote: > On 07/15/2015 10:12 AM, Marc Zyngier wrote: >> On 15/07/15 17:54, David Daney wrote: >>> From: David Daney <david.daney@cavium.com> >>> >>> Needed to map SPI interrupt sources. >>> >>> Signed-off-by: David Daney <david.daney@cavium.com> >>> --- >>> drivers/irqchip/irq-gic-v3.c | 5 +++++ >>> include/linux/irqchip/arm-gic-v3.h | 1 + >>> 2 files changed, 6 insertions(+) >>> >>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>> index c52f7ba..0019fed 100644 >>> --- a/drivers/irqchip/irq-gic-v3.c >>> +++ b/drivers/irqchip/irq-gic-v3.c >>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; >>> /* Our default, arbitrary priority value. Linux only uses one anyway. */ >>> #define DEFAULT_PMR_VALUE 0xf0 >>> >>> +struct irq_domain *gic_get_irq_domain(void) >>> +{ >>> + return gic_data.domain; >>> +} >>> + >>> static inline unsigned int gic_irq(struct irq_data *d) >>> { >>> return d->hwirq; >>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >>> index 18e3757..5992224 100644 >>> --- a/include/linux/irqchip/arm-gic-v3.h >>> +++ b/include/linux/irqchip/arm-gic-v3.h >>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, >>> >>> typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); >>> void set_its_pci_requester_id(its_pci_requester_id_t fn); >>> +struct irq_domain *gic_get_irq_domain(void); >>> #endif >>> >>> #endif >>> >> >> Hmmmffff... You need the domain for SPIs?? >> >> What is wrong with putting these interrupts in your device tree? >> > > There is no device tree node for ECAM based "PCIe" devices, they are > discovered in the PCI bus scan, yet they still need to use SPI interrupts. What is different between these and the normal legacy interrupts that are normally expressed in the RC node using an interrupt map? Sorry if I'm only showing my ignorance here, but I'd like to understand the exact nature of the problem. > We need a way to be able to map these. However you're going to map them, it will not be by just blindly exporting random irqdomains from an unsuspecting interrupt controller. Patch 5 has established that you're using "virtual wire" SPIs, so we need to work on exposing that with the normal kernel abstraction, and not by messing with the internals of the GIC. Thanks, M.
On 07/16/2015 12:38 AM, Marc Zyngier wrote: > On 15/07/15 19:57, David Daney wrote: >> On 07/15/2015 10:12 AM, Marc Zyngier wrote: >>> On 15/07/15 17:54, David Daney wrote: >>>> From: David Daney <david.daney@cavium.com> >>>> >>>> Needed to map SPI interrupt sources. >>>> >>>> Signed-off-by: David Daney <david.daney@cavium.com> >>>> --- >>>> drivers/irqchip/irq-gic-v3.c | 5 +++++ >>>> include/linux/irqchip/arm-gic-v3.h | 1 + >>>> 2 files changed, 6 insertions(+) >>>> >>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>>> index c52f7ba..0019fed 100644 >>>> --- a/drivers/irqchip/irq-gic-v3.c >>>> +++ b/drivers/irqchip/irq-gic-v3.c >>>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; >>>> /* Our default, arbitrary priority value. Linux only uses one anyway. */ >>>> #define DEFAULT_PMR_VALUE 0xf0 >>>> >>>> +struct irq_domain *gic_get_irq_domain(void) >>>> +{ >>>> + return gic_data.domain; >>>> +} >>>> + >>>> static inline unsigned int gic_irq(struct irq_data *d) >>>> { >>>> return d->hwirq; >>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >>>> index 18e3757..5992224 100644 >>>> --- a/include/linux/irqchip/arm-gic-v3.h >>>> +++ b/include/linux/irqchip/arm-gic-v3.h >>>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, >>>> >>>> typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); >>>> void set_its_pci_requester_id(its_pci_requester_id_t fn); >>>> +struct irq_domain *gic_get_irq_domain(void); >>>> #endif [...] > >> We need a way to be able to map these. > > However you're going to map them, it will not be by just blindly > exporting random irqdomains from an unsuspecting interrupt controller. There is nothing random about it. The ARM architects specified that there is exactly One True GIC in a system. If we need to do something with the GIC, it is not a "random ... unsuspecting interrupt controller", it is *The* GIC. > > Patch 5 has established that you're using "virtual wire" SPIs, so we > need to work on exposing that with the normal kernel abstraction, and > not by messing with the internals of the GIC. > Agreed. The MSI system has pci_enable_msix()/pci_disable_msix(). I would propose something like: struct gic_spi_entry { int spi /* SPI number */ int irq; /* kernel irq number mapped to the spi*/ u32 msg; /* message to be written */ u64 assert_addr; u64 deassert_addr; }; /* Fill in the SPI processing information */ int gic_map_spi(int spi, struct gic_spi_entry *data); David Daney > Thanks, > > M. >
On 16/07/15 17:50, David Daney wrote: > On 07/16/2015 12:38 AM, Marc Zyngier wrote: >> On 15/07/15 19:57, David Daney wrote: >>> On 07/15/2015 10:12 AM, Marc Zyngier wrote: >>>> On 15/07/15 17:54, David Daney wrote: >>>>> From: David Daney <david.daney@cavium.com> >>>>> >>>>> Needed to map SPI interrupt sources. >>>>> >>>>> Signed-off-by: David Daney <david.daney@cavium.com> >>>>> --- >>>>> drivers/irqchip/irq-gic-v3.c | 5 +++++ >>>>> include/linux/irqchip/arm-gic-v3.h | 1 + >>>>> 2 files changed, 6 insertions(+) >>>>> >>>>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c >>>>> index c52f7ba..0019fed 100644 >>>>> --- a/drivers/irqchip/irq-gic-v3.c >>>>> +++ b/drivers/irqchip/irq-gic-v3.c >>>>> @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; >>>>> /* Our default, arbitrary priority value. Linux only uses one anyway. */ >>>>> #define DEFAULT_PMR_VALUE 0xf0 >>>>> >>>>> +struct irq_domain *gic_get_irq_domain(void) >>>>> +{ >>>>> + return gic_data.domain; >>>>> +} >>>>> + >>>>> static inline unsigned int gic_irq(struct irq_data *d) >>>>> { >>>>> return d->hwirq; >>>>> diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h >>>>> index 18e3757..5992224 100644 >>>>> --- a/include/linux/irqchip/arm-gic-v3.h >>>>> +++ b/include/linux/irqchip/arm-gic-v3.h >>>>> @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, >>>>> >>>>> typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); >>>>> void set_its_pci_requester_id(its_pci_requester_id_t fn); >>>>> +struct irq_domain *gic_get_irq_domain(void); >>>>> #endif > [...] >> >>> We need a way to be able to map these. >> >> However you're going to map them, it will not be by just blindly >> exporting random irqdomains from an unsuspecting interrupt controller. > > There is nothing random about it. The ARM architects specified that > there is exactly One True GIC in a system. If we need to do something > with the GIC, it is not a "random ... unsuspecting interrupt > controller", it is *The* GIC. Indeed. And The One True GIC Driver will not be butchered just because you can, thank you very much. >> >> Patch 5 has established that you're using "virtual wire" SPIs, so we >> need to work on exposing that with the normal kernel abstraction, and >> not by messing with the internals of the GIC. >> > > Agreed. > > The MSI system has pci_enable_msix()/pci_disable_msix(). > > I would propose something like: > > struct gic_spi_entry { > int spi /* SPI number */ > int irq; /* kernel irq number mapped to the spi*/ > u32 msg; /* message to be written */ > u64 assert_addr; > u64 deassert_addr; > }; > > /* Fill in the SPI processing information */ > int gic_map_spi(int spi, struct gic_spi_entry *data); Neither. The way to do it is to make this a *separate* IRQ domain stacked onto the SPI domain. No funky hook on the side. If it doesn't go through the normal kernel API, it doesn't reach the GIC. M.
On 07/16/2015 10:09 AM, Marc Zyngier wrote: > On 16/07/15 17:50, David Daney wrote: [...] >>> Patch 5 has established that you're using "virtual wire" SPIs, so we >>> need to work on exposing that with the normal kernel abstraction, and >>> not by messing with the internals of the GIC. >>> >> >> Agreed. >> >> The MSI system has pci_enable_msix()/pci_disable_msix(). >> >> I would propose something like: >> >> struct gic_spi_entry { >> int spi /* SPI number */ >> int irq; /* kernel irq number mapped to the spi*/ >> u32 msg; /* message to be written */ >> u64 assert_addr; >> u64 deassert_addr; >> }; >> >> /* Fill in the SPI processing information */ >> int gic_map_spi(int spi, struct gic_spi_entry *data); > > Neither. > > The way to do it is to make this a *separate* IRQ domain stacked onto > the SPI domain. No funky hook on the side. If it doesn't go through the > normal kernel API, it doesn't reach the GIC. Yes, the irqdomain does handle mapping SPI -> irq, and the message can be derived from the SPI. However, the irqdomain infrastructure cannot supply values for either assert_addr or deassert_addr. Those are needed in order to use SPI. How would you suggest that they be obtained? David Daney > > M. >
On 16/07/15 18:14, David Daney wrote: > On 07/16/2015 10:09 AM, Marc Zyngier wrote: >> On 16/07/15 17:50, David Daney wrote: > [...] >>>> Patch 5 has established that you're using "virtual wire" SPIs, so we >>>> need to work on exposing that with the normal kernel abstraction, and >>>> not by messing with the internals of the GIC. >>>> >>> >>> Agreed. >>> >>> The MSI system has pci_enable_msix()/pci_disable_msix(). >>> >>> I would propose something like: >>> >>> struct gic_spi_entry { >>> int spi /* SPI number */ >>> int irq; /* kernel irq number mapped to the spi*/ >>> u32 msg; /* message to be written */ >>> u64 assert_addr; >>> u64 deassert_addr; >>> }; >>> >>> /* Fill in the SPI processing information */ >>> int gic_map_spi(int spi, struct gic_spi_entry *data); >> >> Neither. >> >> The way to do it is to make this a *separate* IRQ domain stacked onto >> the SPI domain. No funky hook on the side. If it doesn't go through the >> normal kernel API, it doesn't reach the GIC. > > Yes, the irqdomain does handle mapping SPI -> irq, and the message can > be derived from the SPI. However, the irqdomain infrastructure cannot > supply values for either assert_addr or deassert_addr. This is why I suggested earlier (in my reply to patch 5) that you have a look at the series I posted a couple of days ago to implement non-PCI MSI support. This would allow you to compose the domains as such: platform-MSI -> message-SPI -> GIC You'd end up with a msi_msg containing the GICD_SETSPI_NSR doorbell, and the SPI as a payload. > Those are needed in order to use SPI. How would you suggest that they > be obtained? Two possibilities: either you derive GICD_CLRSPI_NSR by adding 8 to the doorbell you got from the msi_msg structure (ugly, but limited to your own code), or you extend msi_msg to cater for this case. Thanks, M.
On Thu, 16 Jul 2015 18:32:28 +0100 Marc Zyngier <marc.zyngier@arm.com> wrote: > On 16/07/15 18:14, David Daney wrote: > > On 07/16/2015 10:09 AM, Marc Zyngier wrote: > >> On 16/07/15 17:50, David Daney wrote: > > [...] > >>>> Patch 5 has established that you're using "virtual wire" SPIs, so we > >>>> need to work on exposing that with the normal kernel abstraction, and > >>>> not by messing with the internals of the GIC. > >>>> > >>> > >>> Agreed. > >>> > >>> The MSI system has pci_enable_msix()/pci_disable_msix(). > >>> > >>> I would propose something like: > >>> > >>> struct gic_spi_entry { > >>> int spi /* SPI number */ > >>> int irq; /* kernel irq number mapped to the spi*/ > >>> u32 msg; /* message to be written */ > >>> u64 assert_addr; > >>> u64 deassert_addr; > >>> }; > >>> > >>> /* Fill in the SPI processing information */ > >>> int gic_map_spi(int spi, struct gic_spi_entry *data); > >> > >> Neither. > >> > >> The way to do it is to make this a *separate* IRQ domain stacked onto > >> the SPI domain. No funky hook on the side. If it doesn't go through the > >> normal kernel API, it doesn't reach the GIC. > > > > Yes, the irqdomain does handle mapping SPI -> irq, and the message can > > be derived from the SPI. However, the irqdomain infrastructure cannot > > supply values for either assert_addr or deassert_addr. > > This is why I suggested earlier (in my reply to patch 5) that you have a > look at the series I posted a couple of days ago to implement non-PCI > MSI support. This would allow you to compose the domains as such: > > platform-MSI -> message-SPI -> GIC > > You'd end up with a msi_msg containing the GICD_SETSPI_NSR doorbell, and > the SPI as a payload. > > > Those are needed in order to use SPI. How would you suggest that they > > be obtained? > > Two possibilities: either you derive GICD_CLRSPI_NSR by adding 8 to the > doorbell you got from the msi_msg structure (ugly, but limited to your > own code), or you extend msi_msg to cater for this case. A simpler alternative to the above would be to move all this to firmware, and only expose a *wired* SPI to the kernel. This would have the advantage to use the existing infrastructure for both DT and ACPI. As a side note, that was the intended use of these registers - to provide a "virtual wire" interface that remains mostly invisible to software. Thanks, M.
diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c index c52f7ba..0019fed 100644 --- a/drivers/irqchip/irq-gic-v3.c +++ b/drivers/irqchip/irq-gic-v3.c @@ -58,6 +58,11 @@ static struct gic_chip_data gic_data __read_mostly; /* Our default, arbitrary priority value. Linux only uses one anyway. */ #define DEFAULT_PMR_VALUE 0xf0 +struct irq_domain *gic_get_irq_domain(void) +{ + return gic_data.domain; +} + static inline unsigned int gic_irq(struct irq_data *d) { return d->hwirq; diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h index 18e3757..5992224 100644 --- a/include/linux/irqchip/arm-gic-v3.h +++ b/include/linux/irqchip/arm-gic-v3.h @@ -391,6 +391,7 @@ int its_init(struct device_node *node, struct rdists *rdists, typedef u32 (*its_pci_requester_id_t)(struct pci_dev *, u16); void set_its_pci_requester_id(its_pci_requester_id_t fn); +struct irq_domain *gic_get_irq_domain(void); #endif #endif