Message ID | 20200706081106.25125-1-alexandre.torgue@st.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | irqchip/stm32-exti: map direct event to irq parent | expand |
Hi Alexandre, I love your patch! Perhaps something to improve: [auto build test WARNING on stm32/stm32-next] [also build test WARNING on soc/for-next v5.8-rc4 next-20200707] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use as documented in https://git-scm.com/docs/git-format-patch] url: https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327 base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next config: arm-randconfig-s031-20200707 (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # apt-get install sparse # sparse version: v0.6.2-31-gabbfd661-dirty # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): In file included from include/linux/build_bug.h:5, from include/linux/bits.h:23, from include/linux/bitops.h:5, from drivers/irqchip/irq-stm32-exti.c:8: drivers/irqchip/irq-stm32-exti.c: In function 'stm32_exti_h_domain_alloc': drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] 683 | if (desc->irq_parent >= 0) { | ^~ include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' 683 | if (desc->irq_parent >= 0) { | ^~ drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] 683 | if (desc->irq_parent >= 0) { | ^~ include/linux/compiler.h:58:61: note: in definition of macro '__trace_if_var' 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) | ^~~~ >> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' 683 | if (desc->irq_parent >= 0) { | ^~ drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] 683 | if (desc->irq_parent >= 0) { | ^~ include/linux/compiler.h:69:3: note: in definition of macro '__trace_if_value' 69 | (cond) ? \ | ^~~~ include/linux/compiler.h:56:28: note: in expansion of macro '__trace_if_var' 56 | #define if(cond, ...) if ( __trace_if_var( !!(cond , ## __VA_ARGS__) ) ) | ^~~~~~~~~~~~~~ >> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' 683 | if (desc->irq_parent >= 0) { | ^~ vim +/if +683 drivers/irqchip/irq-stm32-exti.c 659 660 static int stm32_exti_h_domain_alloc(struct irq_domain *dm, 661 unsigned int virq, 662 unsigned int nr_irqs, void *data) 663 { 664 struct stm32_exti_host_data *host_data = dm->host_data; 665 struct stm32_exti_chip_data *chip_data; 666 const struct stm32_desc_irq *desc; 667 struct irq_fwspec *fwspec = data; 668 struct irq_fwspec p_fwspec; 669 irq_hw_number_t hwirq; 670 int bank; 671 672 hwirq = fwspec->param[0]; 673 bank = hwirq / IRQS_PER_BANK; 674 chip_data = &host_data->chips_data[bank]; 675 676 677 desc = stm32_exti_get_desc(host_data->drv_data, hwirq); 678 if (!desc) 679 return -EINVAL; 680 681 irq_domain_set_hwirq_and_chip(dm, virq, hwirq, desc->chip, 682 chip_data); > 683 if (desc->irq_parent >= 0) { 684 p_fwspec.fwnode = dm->parent->fwnode; 685 p_fwspec.param_count = 3; 686 p_fwspec.param[0] = GIC_SPI; 687 p_fwspec.param[1] = desc->irq_parent; 688 p_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; 689 690 return irq_domain_alloc_irqs_parent(dm, virq, 1, &p_fwspec); 691 } 692 693 return 0; 694 } 695 --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Alexandre, On Wed, 08 Jul 2020 05:57:24 +0100, kernel test robot <lkp@intel.com> wrote: > > [1 <text/plain; us-ascii (7bit)>] > Hi Alexandre, > > I love your patch! Perhaps something to improve: > > [auto build test WARNING on stm32/stm32-next] > [also build test WARNING on soc/for-next v5.8-rc4 next-20200707] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327 > base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next > config: arm-randconfig-s031-20200707 (attached as .config) > compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # apt-get install sparse > # sparse version: v0.6.2-31-gabbfd661-dirty > # save the attached .config to linux build tree > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm > > If you fix the issue, kindly add following tag as appropriate > Reported-by: kernel test robot <lkp@intel.com> > > All warnings (new ones prefixed by >>): > > In file included from include/linux/build_bug.h:5, > from include/linux/bits.h:23, > from include/linux/bitops.h:5, > from drivers/irqchip/irq-stm32-exti.c:8: > drivers/irqchip/irq-stm32-exti.c: In function 'stm32_exti_h_domain_alloc': > drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] > 683 | if (desc->irq_parent >= 0) { > | ^~ > include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > | ^~~~ > >> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' > 683 | if (desc->irq_parent >= 0) { Do you plan to address this? Looks like an actual bug to me. M.
Hi Marc, On 7/10/20 11:31 AM, Marc Zyngier wrote: > Alexandre, > > On Wed, 08 Jul 2020 05:57:24 +0100, > kernel test robot <lkp@intel.com> wrote: >> >> [1 <text/plain; us-ascii (7bit)>] >> Hi Alexandre, >> >> I love your patch! Perhaps something to improve: >> >> [auto build test WARNING on stm32/stm32-next] >> [also build test WARNING on soc/for-next v5.8-rc4 next-20200707] >> [If your patch is applied to the wrong git tree, kindly drop us a note. >> And when submitting patch, we suggest to use as documented in >> https://git-scm.com/docs/git-format-patch] >> >> url: https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327 >> base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next >> config: arm-randconfig-s031-20200707 (attached as .config) >> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 >> reproduce: >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >> chmod +x ~/bin/make.cross >> # apt-get install sparse >> # sparse version: v0.6.2-31-gabbfd661-dirty >> # save the attached .config to linux build tree >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm >> >> If you fix the issue, kindly add following tag as appropriate >> Reported-by: kernel test robot <lkp@intel.com> >> >> All warnings (new ones prefixed by >>): >> >> In file included from include/linux/build_bug.h:5, >> from include/linux/bits.h:23, >> from include/linux/bitops.h:5, >> from drivers/irqchip/irq-stm32-exti.c:8: >> drivers/irqchip/irq-stm32-exti.c: In function 'stm32_exti_h_domain_alloc': >> drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] >> 683 | if (desc->irq_parent >= 0) { >> | ^~ >> include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' >> 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) >> | ^~~~ >>>> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' >> 683 | if (desc->irq_parent >= 0) { > > Do you plan to address this? Looks like an actual bug to me. I'll fix it in v2, I was just waiting for other comments before sending a v2. Do you prefer I send a v2 with this fix, and you'll do your review on this v2 ? regards alex > > M. >
On Fri, 10 Jul 2020 10:37:47 +0100, Alexandre Torgue <alexandre.torgue@st.com> wrote: > > Hi Marc, > > On 7/10/20 11:31 AM, Marc Zyngier wrote: > > Alexandre, > > > > On Wed, 08 Jul 2020 05:57:24 +0100, > > kernel test robot <lkp@intel.com> wrote: > >> > >> [1 <text/plain; us-ascii (7bit)>] > >> Hi Alexandre, > >> > >> I love your patch! Perhaps something to improve: > >> > >> [auto build test WARNING on stm32/stm32-next] > >> [also build test WARNING on soc/for-next v5.8-rc4 next-20200707] > >> [If your patch is applied to the wrong git tree, kindly drop us a note. > >> And when submitting patch, we suggest to use as documented in > >> https://git-scm.com/docs/git-format-patch] > >> > >> url: https://github.com/0day-ci/linux/commits/Alexandre-Torgue/irqchip-stm32-exti-map-direct-event-to-irq-parent/20200706-161327 > >> base: https://git.kernel.org/pub/scm/linux/kernel/git/atorgue/stm32.git stm32-next > >> config: arm-randconfig-s031-20200707 (attached as .config) > >> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0 > >> reproduce: > >> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > >> chmod +x ~/bin/make.cross > >> # apt-get install sparse > >> # sparse version: v0.6.2-31-gabbfd661-dirty > >> # save the attached .config to linux build tree > >> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=arm > >> > >> If you fix the issue, kindly add following tag as appropriate > >> Reported-by: kernel test robot <lkp@intel.com> > >> > >> All warnings (new ones prefixed by >>): > >> > >> In file included from include/linux/build_bug.h:5, > >> from include/linux/bits.h:23, > >> from include/linux/bitops.h:5, > >> from drivers/irqchip/irq-stm32-exti.c:8: > >> drivers/irqchip/irq-stm32-exti.c: In function 'stm32_exti_h_domain_alloc': > >> drivers/irqchip/irq-stm32-exti.c:683:23: warning: comparison of unsigned expression >= 0 is always true [-Wtype-limits] > >> 683 | if (desc->irq_parent >= 0) { > >> | ^~ > >> include/linux/compiler.h:58:52: note: in definition of macro '__trace_if_var' > >> 58 | #define __trace_if_var(cond) (__builtin_constant_p(cond) ? (cond) : __trace_if_value(cond)) > >> | ^~~~ > >>>> drivers/irqchip/irq-stm32-exti.c:683:2: note: in expansion of macro 'if' > >> 683 | if (desc->irq_parent >= 0) { > > > > Do you plan to address this? Looks like an actual bug to me. > > I'll fix it in v2, I was just waiting for other comments before > sending a v2. Do you prefer I send a v2 with this fix, and you'll do > your review on this v2 ? I'm certainly not going to review something that has such a glaring issue. M.
diff --git a/drivers/irqchip/irq-stm32-exti.c b/drivers/irqchip/irq-stm32-exti.c index faa8482c8246..26aaaae812aa 100644 --- a/drivers/irqchip/irq-stm32-exti.c +++ b/drivers/irqchip/irq-stm32-exti.c @@ -42,6 +42,7 @@ struct stm32_exti_bank { struct stm32_desc_irq { u32 exti; u32 irq_parent; + struct irq_chip *chip; }; struct stm32_exti_drv_data { @@ -166,27 +167,41 @@ static const struct stm32_exti_bank *stm32mp1_exti_banks[] = { &stm32mp1_exti_b3, }; +static struct irq_chip stm32_exti_h_chip; +static struct irq_chip stm32_exti_h_chip_direct; + static const struct stm32_desc_irq stm32mp1_desc_irq[] = { - { .exti = 0, .irq_parent = 6 }, - { .exti = 1, .irq_parent = 7 }, - { .exti = 2, .irq_parent = 8 }, - { .exti = 3, .irq_parent = 9 }, - { .exti = 4, .irq_parent = 10 }, - { .exti = 5, .irq_parent = 23 }, - { .exti = 6, .irq_parent = 64 }, - { .exti = 7, .irq_parent = 65 }, - { .exti = 8, .irq_parent = 66 }, - { .exti = 9, .irq_parent = 67 }, - { .exti = 10, .irq_parent = 40 }, - { .exti = 11, .irq_parent = 42 }, - { .exti = 12, .irq_parent = 76 }, - { .exti = 13, .irq_parent = 77 }, - { .exti = 14, .irq_parent = 121 }, - { .exti = 15, .irq_parent = 127 }, - { .exti = 16, .irq_parent = 1 }, - { .exti = 65, .irq_parent = 144 }, - { .exti = 68, .irq_parent = 143 }, - { .exti = 73, .irq_parent = 129 }, + { .exti = 0, .irq_parent = 6, .chip = &stm32_exti_h_chip }, + { .exti = 1, .irq_parent = 7, .chip = &stm32_exti_h_chip }, + { .exti = 2, .irq_parent = 8, .chip = &stm32_exti_h_chip }, + { .exti = 3, .irq_parent = 9, .chip = &stm32_exti_h_chip }, + { .exti = 4, .irq_parent = 10, .chip = &stm32_exti_h_chip }, + { .exti = 5, .irq_parent = 23, .chip = &stm32_exti_h_chip }, + { .exti = 6, .irq_parent = 64, .chip = &stm32_exti_h_chip }, + { .exti = 7, .irq_parent = 65, .chip = &stm32_exti_h_chip }, + { .exti = 8, .irq_parent = 66, .chip = &stm32_exti_h_chip }, + { .exti = 9, .irq_parent = 67, .chip = &stm32_exti_h_chip }, + { .exti = 10, .irq_parent = 40, .chip = &stm32_exti_h_chip }, + { .exti = 11, .irq_parent = 42, .chip = &stm32_exti_h_chip }, + { .exti = 12, .irq_parent = 76, .chip = &stm32_exti_h_chip }, + { .exti = 13, .irq_parent = 77, .chip = &stm32_exti_h_chip }, + { .exti = 14, .irq_parent = 121, .chip = &stm32_exti_h_chip }, + { .exti = 15, .irq_parent = 127, .chip = &stm32_exti_h_chip }, + { .exti = 16, .irq_parent = 1, .chip = &stm32_exti_h_chip }, + { .exti = 19, .irq_parent = 3, .chip = &stm32_exti_h_chip_direct }, + { .exti = 21, .irq_parent = 31, .chip = &stm32_exti_h_chip_direct }, + { .exti = 22, .irq_parent = 33, .chip = &stm32_exti_h_chip_direct }, + { .exti = 23, .irq_parent = 72, .chip = &stm32_exti_h_chip_direct }, + { .exti = 24, .irq_parent = 95, .chip = &stm32_exti_h_chip_direct }, + { .exti = 25, .irq_parent = 107, .chip = &stm32_exti_h_chip_direct }, + { .exti = 30, .irq_parent = 52, .chip = &stm32_exti_h_chip_direct }, + { .exti = 47, .irq_parent = 93, .chip = &stm32_exti_h_chip_direct }, + { .exti = 54, .irq_parent = 135, .chip = &stm32_exti_h_chip_direct }, + { .exti = 61, .irq_parent = 100, .chip = &stm32_exti_h_chip_direct }, + { .exti = 65, .irq_parent = 144, .chip = &stm32_exti_h_chip }, + { .exti = 68, .irq_parent = 143, .chip = &stm32_exti_h_chip }, + { .exti = 70, .irq_parent = 62, .chip = &stm32_exti_h_chip_direct }, + { .exti = 73, .irq_parent = 129, .chip = &stm32_exti_h_chip }, }; static const struct stm32_exti_drv_data stm32mp1_drv_data = { @@ -196,22 +211,23 @@ static const struct stm32_exti_drv_data stm32mp1_drv_data = { .irq_nr = ARRAY_SIZE(stm32mp1_desc_irq), }; -static int stm32_exti_to_irq(const struct stm32_exti_drv_data *drv_data, - irq_hw_number_t hwirq) +static const struct +stm32_desc_irq *stm32_exti_get_desc(const struct stm32_exti_drv_data *drv_data, + irq_hw_number_t hwirq) { - const struct stm32_desc_irq *desc_irq; + const struct stm32_desc_irq *desc = NULL; int i; if (!drv_data->desc_irqs) - return -EINVAL; + return NULL; for (i = 0; i < drv_data->irq_nr; i++) { - desc_irq = &drv_data->desc_irqs[i]; - if (desc_irq->exti == hwirq) - return desc_irq->irq_parent; + desc = &drv_data->desc_irqs[i]; + if (desc->exti == hwirq) + break; } - return -EINVAL; + return desc; } static unsigned long stm32_exti_pending(struct irq_chip_generic *gc) @@ -628,30 +644,47 @@ static struct irq_chip stm32_exti_h_chip = { .irq_set_affinity = IS_ENABLED(CONFIG_SMP) ? stm32_exti_h_set_affinity : NULL, }; +static struct irq_chip stm32_exti_h_chip_direct = { + .name = "stm32-exti-h-direct", + .irq_eoi = irq_chip_eoi_parent, + .irq_ack = irq_chip_ack_parent, + .irq_mask = irq_chip_mask_parent, + .irq_unmask = irq_chip_unmask_parent, + .irq_retrigger = irq_chip_retrigger_hierarchy, + .irq_set_type = irq_chip_set_type_parent, + .irq_set_wake = stm32_exti_h_set_wake, + .flags = IRQCHIP_MASK_ON_SUSPEND, + .irq_set_affinity = IS_ENABLED(CONFIG_SMP) ? irq_chip_set_affinity_parent : NULL, +}; + static int stm32_exti_h_domain_alloc(struct irq_domain *dm, unsigned int virq, unsigned int nr_irqs, void *data) { struct stm32_exti_host_data *host_data = dm->host_data; struct stm32_exti_chip_data *chip_data; + const struct stm32_desc_irq *desc; struct irq_fwspec *fwspec = data; struct irq_fwspec p_fwspec; irq_hw_number_t hwirq; - int p_irq, bank; + int bank; hwirq = fwspec->param[0]; bank = hwirq / IRQS_PER_BANK; chip_data = &host_data->chips_data[bank]; - irq_domain_set_hwirq_and_chip(dm, virq, hwirq, - &stm32_exti_h_chip, chip_data); - p_irq = stm32_exti_to_irq(host_data->drv_data, hwirq); - if (p_irq >= 0) { + desc = stm32_exti_get_desc(host_data->drv_data, hwirq); + if (!desc) + return -EINVAL; + + irq_domain_set_hwirq_and_chip(dm, virq, hwirq, desc->chip, + chip_data); + if (desc->irq_parent >= 0) { p_fwspec.fwnode = dm->parent->fwnode; p_fwspec.param_count = 3; p_fwspec.param[0] = GIC_SPI; - p_fwspec.param[1] = p_irq; + p_fwspec.param[1] = desc->irq_parent; p_fwspec.param[2] = IRQ_TYPE_LEVEL_HIGH; return irq_domain_alloc_irqs_parent(dm, virq, 1, &p_fwspec);
EXTI lines are mainly used to wake-up system from CStop low power mode. Currently, if a device wants to use a EXTI (direct) line as wakeup line, it has to declare 2 interrupts: - one for EXTI used to wake-up system (with dedicated_wake_irq api). - one for GIC used to get the wake up reason inside the concerned IP. This split is not really needed as each EXTI line is actually "linked " to a GIC. So to avoid this useless double interrupt management in each wake-up driver, this patch lets the STM32 EXTI driver abstract it by mapping each EXTI line to his corresponding GIC. Signed-off-by: Alexandre Torgue <alexandre.torgue@st.com>