| Message ID | 20240906093148.830452-3-thippesw@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for CPM5 controller-1. | expand |
On 06/09/2024 11:31, Thippeswamy Havalige wrote: > In the CPM5, controller-1 has platform-specific error interrupt bits > located at different offsets compared to controller-0. > > Signed-off-by: Thippeswamy Havalige <thippesw@amd.com> > --- > drivers/pci/controller/pcie-xilinx-cpm.c | 39 +++++++++++++++++++----- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c > index a0f5e1d67b04..d672f620bc4c 100644 > --- a/drivers/pci/controller/pcie-xilinx-cpm.c > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c > @@ -30,10 +30,13 @@ > #define XILINX_CPM_PCIE_REG_IDRN_MASK 0x00000E3C > #define XILINX_CPM_PCIE_MISC_IR_STATUS 0x00000340 > #define XILINX_CPM_PCIE_MISC_IR_ENABLE 0x00000348 > -#define XILINX_CPM_PCIE_MISC_IR_LOCAL BIT(1) > +#define XILINX_CPM_PCIE0_MISC_IR_LOCAL BIT(1) > +#define XILINX_CPM_PCIE1_MISC_IR_LOCAL BIT(2) > > -#define XILINX_CPM_PCIE_IR_STATUS 0x000002A0 > -#define XILINX_CPM_PCIE_IR_ENABLE 0x000002A8 > +#define XILINX_CPM_PCIE0_IR_STATUS 0x000002A0 > +#define XILINX_CPM_PCIE1_IR_STATUS 0x000002B4 > +#define XILINX_CPM_PCIE0_IR_ENABLE 0x000002A8 > +#define XILINX_CPM_PCIE1_IR_ENABLE 0x000002BC > #define XILINX_CPM_PCIE_IR_LOCAL BIT(0) > > #define IMR(x) BIT(XILINX_PCIE_INTR_ ##x) > @@ -280,10 +283,17 @@ static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc) > pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR); > > if (port->variant->version == CPM5) { > - val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_IR_STATUS); > + val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE0_IR_STATUS); > if (val) > writel_relaxed(val, port->cpm_base + > - XILINX_CPM_PCIE_IR_STATUS); > + XILINX_CPM_PCIE0_IR_STATUS); > + } > + There are no blank lines allowed between arms of conditional statements. Please follow coding style. This case is explained there. Best regards, Krzysztof
On Fri, Sep 06, 2024 at 03:01:48PM +0530, Thippeswamy Havalige wrote: > In the CPM5, controller-1 has platform-specific error interrupt bits > located at different offsets compared to controller-0. Technically mentions a difference between controller-0 and controller-1, but doesn't say what the patch does. > Signed-off-by: Thippeswamy Havalige <thippesw@amd.com> > --- > drivers/pci/controller/pcie-xilinx-cpm.c | 39 +++++++++++++++++++----- > 1 file changed, 32 insertions(+), 7 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c > index a0f5e1d67b04..d672f620bc4c 100644 > --- a/drivers/pci/controller/pcie-xilinx-cpm.c > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c > @@ -30,10 +30,13 @@ > #define XILINX_CPM_PCIE_REG_IDRN_MASK 0x00000E3C > #define XILINX_CPM_PCIE_MISC_IR_STATUS 0x00000340 > #define XILINX_CPM_PCIE_MISC_IR_ENABLE 0x00000348 > -#define XILINX_CPM_PCIE_MISC_IR_LOCAL BIT(1) > +#define XILINX_CPM_PCIE0_MISC_IR_LOCAL BIT(1) > +#define XILINX_CPM_PCIE1_MISC_IR_LOCAL BIT(2) > > -#define XILINX_CPM_PCIE_IR_STATUS 0x000002A0 > -#define XILINX_CPM_PCIE_IR_ENABLE 0x000002A8 > +#define XILINX_CPM_PCIE0_IR_STATUS 0x000002A0 > +#define XILINX_CPM_PCIE1_IR_STATUS 0x000002B4 > +#define XILINX_CPM_PCIE0_IR_ENABLE 0x000002A8 > +#define XILINX_CPM_PCIE1_IR_ENABLE 0x000002BC These are all indented with spaces; should use tabs like the other definitions above. > #define XILINX_CPM_PCIE_IR_LOCAL BIT(0) Same here (although you didn't change this one). > #define IMR(x) BIT(XILINX_PCIE_INTR_ ##x) > @@ -280,10 +283,17 @@ static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc) > pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR); > > if (port->variant->version == CPM5) { > - val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_IR_STATUS); > + val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE0_IR_STATUS); > if (val) > writel_relaxed(val, port->cpm_base + > - XILINX_CPM_PCIE_IR_STATUS); > + XILINX_CPM_PCIE0_IR_STATUS); > + } > + > + else if (port->variant->version == CPM5_HOST1) { > + val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE1_IR_STATUS); > + if (val) > + writel_relaxed(val, port->cpm_base + > + XILINX_CPM_PCIE1_IR_STATUS); > } When possible, I think it would be nicer to encode this difference in the data, i.e., in struct xilinx_cpm_variant, than in the code. For example, struct xilinx_cpm_variant { enum xilinx_cpm_version version; + u32 ir_status; } static const struct xilinx_cpm_variant cpm5_host = { .version = CPM5, + .ir_status = XILINX_CPM_PCIE0_IR_STATUS, }; static const struct xilinx_cpm_variant cpm5_host = { .version = CPM5_HOST1, + .ir_status = XILINX_CPM_PCIE1_IR_STATUS, }; Then this code could look like this, where it basically tests a *feature* instead of checking for all the different variants: struct xilinx_cpm_variant *variant = port->variant; if (variant->ir_status) { val = readl_relaxed(port->cpm_base + variant->ir_status); if (val) writel_relaxed(port->cpm_base + variant->ir_status); } (Apparently the CPM variant doesn't require this at all?) > /* > @@ -483,12 +493,19 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port) > * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to > * CPM SLCR block. > */ > - writel(XILINX_CPM_PCIE_MISC_IR_LOCAL, > + writel(XILINX_CPM_PCIE0_MISC_IR_LOCAL, > port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); > > if (port->variant->version == CPM5) { > writel(XILINX_CPM_PCIE_IR_LOCAL, > - port->cpm_base + XILINX_CPM_PCIE_IR_ENABLE); > + port->cpm_base + XILINX_CPM_PCIE0_IR_ENABLE); > + } > + > + else if (port->variant->version == CPM5_HOST1) { > + writel(XILINX_CPM_PCIE1_MISC_IR_LOCAL, > + port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); > + writel(XILINX_CPM_PCIE_IR_LOCAL, > + port->cpm_base + XILINX_CPM_PCIE1_IR_ENABLE); This looks questionable and worth a comment if this is what you intend. CPM - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE CPM5 - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE - sets PCIE_IR_LOCAL in PCIE0_IR_ENABLE CPM5_HOST1 - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE - sets PCIE1_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE (overwrite) - sets PCIE_IR_LOCAL in PCIE1_IR_ENABLE The CPM5_HOST1 overwrite looks either wrong or like the first write was unnecessary. You might be able to simplify this by adding .misc_ir_value, .ir_enable, and .ir_local_value: struct xilinx_cpm_variant *variant = port->variant; writel(variant->misc_ir_value, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); if (variant->ir_enable) writel(variant->ir_local_value, port->cpm_base + ir_enable); > /* Enable the Bridge enable bit */ Unrelated to *this* patch except for being in the diff context, but "enable the enable bit" is unclear because "enable" isn't something you can do to a bit; you can *set* or *clear* a bit. Bjorn
Hi Thippeswamy, kernel test robot noticed the following build errors: [auto build test ERROR on pci/next] [also build test ERROR on pci/for-linus mani-mhi/mhi-next robh/for-next linus/master v6.11-rc6 next-20240906] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Thippeswamy-Havalige/dt-bindings-PCI-xilinx-cpm-Add-compatible-string-for-CPM5-controller-1/20240906-173446 base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next patch link: https://lore.kernel.org/r/20240906093148.830452-3-thippesw%40amd.com patch subject: [PATCH 2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root Port controller-1 config: alpha-randconfig-r051-20240907 (https://download.01.org/0day-ci/archive/20240907/202409071415.6WivnBm0-lkp@intel.com/config) compiler: alpha-linux-gcc (GCC) 13.3.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409071415.6WivnBm0-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202409071415.6WivnBm0-lkp@intel.com/ All errors (new ones prefixed by >>): drivers/pci/controller/pcie-xilinx-cpm.c: In function 'xilinx_cpm_pcie_event_flow': >> drivers/pci/controller/pcie-xilinx-cpm.c:292:44: error: 'CPM5_HOST1' undeclared (first use in this function) 292 | else if (port->variant->version == CPM5_HOST1) { | ^~~~~~~~~~ drivers/pci/controller/pcie-xilinx-cpm.c:292:44: note: each undeclared identifier is reported only once for each function it appears in drivers/pci/controller/pcie-xilinx-cpm.c: In function 'xilinx_cpm_pcie_init_port': drivers/pci/controller/pcie-xilinx-cpm.c:504:44: error: 'CPM5_HOST1' undeclared (first use in this function) 504 | else if (port->variant->version == CPM5_HOST1) { | ^~~~~~~~~~ drivers/pci/controller/pcie-xilinx-cpm.c: At top level: >> drivers/pci/controller/pcie-xilinx-cpm.c:635:40: error: redefinition of 'cpm5_host' 635 | static const struct xilinx_cpm_variant cpm5_host = { | ^~~~~~~~~ drivers/pci/controller/pcie-xilinx-cpm.c:631:40: note: previous definition of 'cpm5_host' with type 'const struct xilinx_cpm_variant' 631 | static const struct xilinx_cpm_variant cpm5_host = { | ^~~~~~~~~ >> drivers/pci/controller/pcie-xilinx-cpm.c:636:20: error: 'CPM5_HOST1' undeclared here (not in a function) 636 | .version = CPM5_HOST1, | ^~~~~~~~~~ >> drivers/pci/controller/pcie-xilinx-cpm.c:650:26: error: 'cpm5_host1' undeclared here (not in a function); did you mean 'cpm5_host'? 650 | .data = &cpm5_host1, | ^~~~~~~~~~ | cpm5_host vim +/CPM5_HOST1 +292 drivers/pci/controller/pcie-xilinx-cpm.c 270 271 static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc) 272 { 273 struct xilinx_cpm_pcie *port = irq_desc_get_handler_data(desc); 274 struct irq_chip *chip = irq_desc_get_chip(desc); 275 unsigned long val; 276 int i; 277 278 chained_irq_enter(chip, desc); 279 val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR); 280 val &= pcie_read(port, XILINX_CPM_PCIE_REG_IMR); 281 for_each_set_bit(i, &val, 32) 282 generic_handle_domain_irq(port->cpm_domain, i); 283 pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR); 284 285 if (port->variant->version == CPM5) { 286 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE0_IR_STATUS); 287 if (val) 288 writel_relaxed(val, port->cpm_base + 289 XILINX_CPM_PCIE0_IR_STATUS); 290 } 291 > 292 else if (port->variant->version == CPM5_HOST1) { 293 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE1_IR_STATUS); 294 if (val) 295 writel_relaxed(val, port->cpm_base + 296 XILINX_CPM_PCIE1_IR_STATUS); 297 } 298 299 /* 300 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to 301 * CPM SLCR block. 302 */ 303 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS); 304 if (val) 305 writel_relaxed(val, 306 port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS); 307 308 chained_irq_exit(chip, desc); 309 } 310
Hi Thippeswamy,
kernel test robot noticed the following build errors:
[auto build test ERROR on pci/next]
[also build test ERROR on pci/for-linus mani-mhi/mhi-next robh/for-next linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thippeswamy-Havalige/dt-bindings-PCI-xilinx-cpm-Add-compatible-string-for-CPM5-controller-1/20240906-173446
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20240906093148.830452-3-thippesw%40amd.com
patch subject: [PATCH 2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root Port controller-1
config: s390-allmodconfig (https://download.01.org/0day-ci/archive/20240907/202409071635.vNwvSZtg-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project 05f5a91d00b02f4369f46d076411c700755ae041)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409071635.vNwvSZtg-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409071635.vNwvSZtg-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from drivers/pci/controller/pcie-xilinx-cpm.c:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
548 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
561 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:37:59: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
| ^
include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
102 | #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
| ^
In file included from drivers/pci/controller/pcie-xilinx-cpm.c:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
574 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/big_endian.h:35:59: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
| ^
include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
115 | #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
| ^
In file included from drivers/pci/controller/pcie-xilinx-cpm.c:10:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:14:
In file included from arch/s390/include/asm/io.h:93:
include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
585 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
595 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
605 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
693 | readsb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
701 | readsw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
709 | readsl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
718 | writesb(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
727 | writesw(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
736 | writesl(PCI_IOBASE + addr, buffer, count);
| ~~~~~~~~~~ ^
In file included from drivers/pci/controller/pcie-xilinx-cpm.c:10:
In file included from include/linux/irq.h:591:
In file included from arch/s390/include/asm/hw_irq.h:6:
In file included from include/linux/pci.h:37:
In file included from include/linux/device.h:32:
In file included from include/linux/device/driver.h:21:
In file included from include/linux/module.h:19:
In file included from include/linux/elf.h:6:
In file included from arch/s390/include/asm/elf.h:181:
In file included from arch/s390/include/asm/mmu_context.h:11:
In file included from arch/s390/include/asm/pgalloc.h:18:
In file included from include/linux/mm.h:2228:
include/linux/vmstat.h:500:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
500 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
501 | item];
| ~~~~
include/linux/vmstat.h:507:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
507 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
508 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
include/linux/vmstat.h:519:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
519 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
520 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
include/linux/vmstat.h:528:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
528 | return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~ ^
529 | NR_VM_NUMA_EVENT_ITEMS +
| ~~~~~~~~~~~~~~~~~~~~~~
>> drivers/pci/controller/pcie-xilinx-cpm.c:292:37: error: use of undeclared identifier 'CPM5_HOST1'
292 | else if (port->variant->version == CPM5_HOST1) {
| ^
drivers/pci/controller/pcie-xilinx-cpm.c:504:37: error: use of undeclared identifier 'CPM5_HOST1'
504 | else if (port->variant->version == CPM5_HOST1) {
| ^
drivers/pci/controller/pcie-xilinx-cpm.c:636:13: error: use of undeclared identifier 'CPM5_HOST1'
636 | .version = CPM5_HOST1,
| ^
>> drivers/pci/controller/pcie-xilinx-cpm.c:650:12: error: use of undeclared identifier 'cpm5_host1'; did you mean 'cpm5_host'?
650 | .data = &cpm5_host1,
| ^~~~~~~~~~
| cpm5_host
drivers/pci/controller/pcie-xilinx-cpm.c:635:40: note: 'cpm5_host' declared here
635 | static const struct xilinx_cpm_variant cpm5_host = {
| ^
17 warnings and 4 errors generated.
vim +/CPM5_HOST1 +292 drivers/pci/controller/pcie-xilinx-cpm.c
270
271 static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc)
272 {
273 struct xilinx_cpm_pcie *port = irq_desc_get_handler_data(desc);
274 struct irq_chip *chip = irq_desc_get_chip(desc);
275 unsigned long val;
276 int i;
277
278 chained_irq_enter(chip, desc);
279 val = pcie_read(port, XILINX_CPM_PCIE_REG_IDR);
280 val &= pcie_read(port, XILINX_CPM_PCIE_REG_IMR);
281 for_each_set_bit(i, &val, 32)
282 generic_handle_domain_irq(port->cpm_domain, i);
283 pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR);
284
285 if (port->variant->version == CPM5) {
286 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE0_IR_STATUS);
287 if (val)
288 writel_relaxed(val, port->cpm_base +
289 XILINX_CPM_PCIE0_IR_STATUS);
290 }
291
> 292 else if (port->variant->version == CPM5_HOST1) {
293 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE1_IR_STATUS);
294 if (val)
295 writel_relaxed(val, port->cpm_base +
296 XILINX_CPM_PCIE1_IR_STATUS);
297 }
298
299 /*
300 * XILINX_CPM_PCIE_MISC_IR_STATUS register is mapped to
301 * CPM SLCR block.
302 */
303 val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
304 if (val)
305 writel_relaxed(val,
306 port->cpm_base + XILINX_CPM_PCIE_MISC_IR_STATUS);
307
308 chained_irq_exit(chip, desc);
309 }
310
Hi Thippeswamy,
kernel test robot noticed the following build warnings:
[auto build test WARNING on pci/next]
[also build test WARNING on pci/for-linus mani-mhi/mhi-next robh/for-next linus/master v6.11-rc6 next-20240906]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thippeswamy-Havalige/dt-bindings-PCI-xilinx-cpm-Add-compatible-string-for-CPM5-controller-1/20240906-173446
base: https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git next
patch link: https://lore.kernel.org/r/20240906093148.830452-3-thippesw%40amd.com
patch subject: [PATCH 2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root Port controller-1
config: um-allyesconfig (https://download.01.org/0day-ci/archive/20240907/202409071713.wrAI0UuK-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409071713.wrAI0UuK-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409071713.wrAI0UuK-lkp@intel.com/
All warnings (new ones prefixed by >>):
drivers/pci/controller/pcie-xilinx-cpm.c: In function 'xilinx_cpm_pcie_event_flow':
drivers/pci/controller/pcie-xilinx-cpm.c:292:44: error: 'CPM5_HOST1' undeclared (first use in this function)
292 | else if (port->variant->version == CPM5_HOST1) {
| ^~~~~~~~~~
drivers/pci/controller/pcie-xilinx-cpm.c:292:44: note: each undeclared identifier is reported only once for each function it appears in
drivers/pci/controller/pcie-xilinx-cpm.c: In function 'xilinx_cpm_pcie_init_port':
drivers/pci/controller/pcie-xilinx-cpm.c:504:44: error: 'CPM5_HOST1' undeclared (first use in this function)
504 | else if (port->variant->version == CPM5_HOST1) {
| ^~~~~~~~~~
drivers/pci/controller/pcie-xilinx-cpm.c: At top level:
drivers/pci/controller/pcie-xilinx-cpm.c:635:40: error: redefinition of 'cpm5_host'
635 | static const struct xilinx_cpm_variant cpm5_host = {
| ^~~~~~~~~
drivers/pci/controller/pcie-xilinx-cpm.c:631:40: note: previous definition of 'cpm5_host' with type 'const struct xilinx_cpm_variant'
631 | static const struct xilinx_cpm_variant cpm5_host = {
| ^~~~~~~~~
drivers/pci/controller/pcie-xilinx-cpm.c:636:20: error: 'CPM5_HOST1' undeclared here (not in a function)
636 | .version = CPM5_HOST1,
| ^~~~~~~~~~
drivers/pci/controller/pcie-xilinx-cpm.c:650:26: error: 'cpm5_host1' undeclared here (not in a function); did you mean 'cpm5_host'?
650 | .data = &cpm5_host1,
| ^~~~~~~~~~
| cpm5_host
>> drivers/pci/controller/pcie-xilinx-cpm.c:631:40: warning: 'cpm5_host' defined but not used [-Wunused-const-variable=]
631 | static const struct xilinx_cpm_variant cpm5_host = {
| ^~~~~~~~~
vim +/cpm5_host +631 drivers/pci/controller/pcie-xilinx-cpm.c
51f1ffc00d95e3 Bharat Kumar Gogada 2022-07-05 630
51f1ffc00d95e3 Bharat Kumar Gogada 2022-07-05 @631 static const struct xilinx_cpm_variant cpm5_host = {
51f1ffc00d95e3 Bharat Kumar Gogada 2022-07-05 632 .version = CPM5,
51f1ffc00d95e3 Bharat Kumar Gogada 2022-07-05 633 };
51f1ffc00d95e3 Bharat Kumar Gogada 2022-07-05 634
Hi Bjorn, Thanks, for your comments. > -----Original Message----- > From: Bjorn Helgaas <helgaas@kernel.org> > Sent: Saturday, September 7, 2024 12:49 AM > To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com> > Cc: manivannan.sadhasivam@linaro.org; robh@kernel.org; linux- > pci@vger.kernel.org; bhelgaas@google.com; linux-arm-kernel@lists.infradead.org; > linux-kernel@vger.kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > devicetree@vger.kernel.org; Gogada, Bharat Kumar > <bharat.kumar.gogada@amd.com>; Simek, Michal <michal.simek@amd.com>; > lpieralisi@kernel.org; kw@linux.com > Subject: Re: [PATCH 2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root Port > controller-1 > > On Fri, Sep 06, 2024 at 03:01:48PM +0530, Thippeswamy Havalige wrote: > > In the CPM5, controller-1 has platform-specific error interrupt bits > > located at different offsets compared to controller-0. > > Technically mentions a difference between controller-0 and > controller-1, but doesn't say what the patch does. I'll update required detailed description in next patch. > > > Signed-off-by: Thippeswamy Havalige <thippesw@amd.com> > > --- > > drivers/pci/controller/pcie-xilinx-cpm.c | 39 +++++++++++++++++++----- > > 1 file changed, 32 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie- > xilinx-cpm.c > > index a0f5e1d67b04..d672f620bc4c 100644 > > --- a/drivers/pci/controller/pcie-xilinx-cpm.c > > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c > > @@ -30,10 +30,13 @@ > > #define XILINX_CPM_PCIE_REG_IDRN_MASK 0x00000E3C > > #define XILINX_CPM_PCIE_MISC_IR_STATUS 0x00000340 > > #define XILINX_CPM_PCIE_MISC_IR_ENABLE 0x00000348 > > -#define XILINX_CPM_PCIE_MISC_IR_LOCAL BIT(1) > > +#define XILINX_CPM_PCIE0_MISC_IR_LOCAL BIT(1) > > +#define XILINX_CPM_PCIE1_MISC_IR_LOCAL BIT(2) > > > > -#define XILINX_CPM_PCIE_IR_STATUS 0x000002A0 > > -#define XILINX_CPM_PCIE_IR_ENABLE 0x000002A8 > > +#define XILINX_CPM_PCIE0_IR_STATUS 0x000002A0 > > +#define XILINX_CPM_PCIE1_IR_STATUS 0x000002B4 > > +#define XILINX_CPM_PCIE0_IR_ENABLE 0x000002A8 > > +#define XILINX_CPM_PCIE1_IR_ENABLE 0x000002BC > > These are all indented with spaces; should use tabs like the other > definitions above. Thanks, will add proper tabs in next patch. > > > #define XILINX_CPM_PCIE_IR_LOCAL BIT(0) > > Same here (although you didn't change this one). > > > #define IMR(x) BIT(XILINX_PCIE_INTR_ ##x) > > @@ -280,10 +283,17 @@ static void xilinx_cpm_pcie_event_flow(struct > irq_desc *desc) > > pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR); > > > > if (port->variant->version == CPM5) { > > - val = readl_relaxed(port->cpm_base + > XILINX_CPM_PCIE_IR_STATUS); > > + val = readl_relaxed(port->cpm_base + > XILINX_CPM_PCIE0_IR_STATUS); > > if (val) > > writel_relaxed(val, port->cpm_base + > > - XILINX_CPM_PCIE_IR_STATUS); > > + XILINX_CPM_PCIE0_IR_STATUS); > > + } > > + > > + else if (port->variant->version == CPM5_HOST1) { > > + val = readl_relaxed(port->cpm_base + > XILINX_CPM_PCIE1_IR_STATUS); > > + if (val) > > + writel_relaxed(val, port->cpm_base + > > + XILINX_CPM_PCIE1_IR_STATUS); > > } > > When possible, I think it would be nicer to encode this difference in > the data, i.e., in struct xilinx_cpm_variant, than in the code. For > example, > > struct xilinx_cpm_variant { > enum xilinx_cpm_version version; > + u32 ir_status; > } > > static const struct xilinx_cpm_variant cpm5_host = { > .version = CPM5, > + .ir_status = XILINX_CPM_PCIE0_IR_STATUS, > }; > > static const struct xilinx_cpm_variant cpm5_host = { > .version = CPM5_HOST1, > + .ir_status = XILINX_CPM_PCIE1_IR_STATUS, > }; > > Then this code could look like this, where it basically tests a > *feature* instead of checking for all the different variants: > > struct xilinx_cpm_variant *variant = port->variant; > > if (variant->ir_status) { > val = readl_relaxed(port->cpm_base + variant->ir_status); > if (val) > writel_relaxed(port->cpm_base + variant->ir_status); > } > > (Apparently the CPM variant doesn't require this at all?) > > > /* > > @@ -483,12 +493,19 @@ static void xilinx_cpm_pcie_init_port(struct > xilinx_cpm_pcie *port) > > * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to > > * CPM SLCR block. > > */ > > - writel(XILINX_CPM_PCIE_MISC_IR_LOCAL, > > + writel(XILINX_CPM_PCIE0_MISC_IR_LOCAL, > > port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); > > > > if (port->variant->version == CPM5) { > > writel(XILINX_CPM_PCIE_IR_LOCAL, > > - port->cpm_base + XILINX_CPM_PCIE_IR_ENABLE); > > + port->cpm_base + XILINX_CPM_PCIE0_IR_ENABLE); > > + } > > + > > + else if (port->variant->version == CPM5_HOST1) { > > + writel(XILINX_CPM_PCIE1_MISC_IR_LOCAL, > > + port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); > > + writel(XILINX_CPM_PCIE_IR_LOCAL, > > + port->cpm_base + XILINX_CPM_PCIE1_IR_ENABLE); > > This looks questionable and worth a comment if this is what you > intend. > > CPM > - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE > > CPM5 > - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE > - sets PCIE_IR_LOCAL in PCIE0_IR_ENABLE > > CPM5_HOST1 > - sets PCIE0_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE > - sets PCIE1_MISC_IR_LOCAL in PCIE_MISC_IR_ENABLE (overwrite) > - sets PCIE_IR_LOCAL in PCIE1_IR_ENABLE > > The CPM5_HOST1 overwrite looks either wrong or like the first write > was unnecessary. Yes, the initial write is unnecessary here. I will fix this in next patch. > > You might be able to simplify this by adding .misc_ir_value, > .ir_enable, and .ir_local_value: > > struct xilinx_cpm_variant *variant = port->variant; > > writel(variant->misc_ir_value, > port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); > if (variant->ir_enable) > writel(variant->ir_local_value, port->cpm_base + ir_enable); > > > /* Enable the Bridge enable bit */ > > Unrelated to *this* patch except for being in the diff context, but > "enable the enable bit" is unclear because "enable" isn't something > you can do to a bit; you can *set* or *clear* a bit. > > Bjorn
diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c b/drivers/pci/controller/pcie-xilinx-cpm.c index a0f5e1d67b04..d672f620bc4c 100644 --- a/drivers/pci/controller/pcie-xilinx-cpm.c +++ b/drivers/pci/controller/pcie-xilinx-cpm.c @@ -30,10 +30,13 @@ #define XILINX_CPM_PCIE_REG_IDRN_MASK 0x00000E3C #define XILINX_CPM_PCIE_MISC_IR_STATUS 0x00000340 #define XILINX_CPM_PCIE_MISC_IR_ENABLE 0x00000348 -#define XILINX_CPM_PCIE_MISC_IR_LOCAL BIT(1) +#define XILINX_CPM_PCIE0_MISC_IR_LOCAL BIT(1) +#define XILINX_CPM_PCIE1_MISC_IR_LOCAL BIT(2) -#define XILINX_CPM_PCIE_IR_STATUS 0x000002A0 -#define XILINX_CPM_PCIE_IR_ENABLE 0x000002A8 +#define XILINX_CPM_PCIE0_IR_STATUS 0x000002A0 +#define XILINX_CPM_PCIE1_IR_STATUS 0x000002B4 +#define XILINX_CPM_PCIE0_IR_ENABLE 0x000002A8 +#define XILINX_CPM_PCIE1_IR_ENABLE 0x000002BC #define XILINX_CPM_PCIE_IR_LOCAL BIT(0) #define IMR(x) BIT(XILINX_PCIE_INTR_ ##x) @@ -280,10 +283,17 @@ static void xilinx_cpm_pcie_event_flow(struct irq_desc *desc) pcie_write(port, val, XILINX_CPM_PCIE_REG_IDR); if (port->variant->version == CPM5) { - val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE_IR_STATUS); + val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE0_IR_STATUS); if (val) writel_relaxed(val, port->cpm_base + - XILINX_CPM_PCIE_IR_STATUS); + XILINX_CPM_PCIE0_IR_STATUS); + } + + else if (port->variant->version == CPM5_HOST1) { + val = readl_relaxed(port->cpm_base + XILINX_CPM_PCIE1_IR_STATUS); + if (val) + writel_relaxed(val, port->cpm_base + + XILINX_CPM_PCIE1_IR_STATUS); } /* @@ -483,12 +493,19 @@ static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port) * XILINX_CPM_PCIE_MISC_IR_ENABLE register is mapped to * CPM SLCR block. */ - writel(XILINX_CPM_PCIE_MISC_IR_LOCAL, + writel(XILINX_CPM_PCIE0_MISC_IR_LOCAL, port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); if (port->variant->version == CPM5) { writel(XILINX_CPM_PCIE_IR_LOCAL, - port->cpm_base + XILINX_CPM_PCIE_IR_ENABLE); + port->cpm_base + XILINX_CPM_PCIE0_IR_ENABLE); + } + + else if (port->variant->version == CPM5_HOST1) { + writel(XILINX_CPM_PCIE1_MISC_IR_LOCAL, + port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE); + writel(XILINX_CPM_PCIE_IR_LOCAL, + port->cpm_base + XILINX_CPM_PCIE1_IR_ENABLE); } /* Enable the Bridge enable bit */ @@ -615,6 +632,10 @@ static const struct xilinx_cpm_variant cpm5_host = { .version = CPM5, }; +static const struct xilinx_cpm_variant cpm5_host = { + .version = CPM5_HOST1, +}; + static const struct of_device_id xilinx_cpm_pcie_of_match[] = { { .compatible = "xlnx,versal-cpm-host-1.00", @@ -624,6 +645,10 @@ static const struct of_device_id xilinx_cpm_pcie_of_match[] = { .compatible = "xlnx,versal-cpm5-host", .data = &cpm5_host, }, + { + .compatible = "xlnx,versal-cpm5-host1", + .data = &cpm5_host1, + }, {} };
In the CPM5, controller-1 has platform-specific error interrupt bits located at different offsets compared to controller-0. Signed-off-by: Thippeswamy Havalige <thippesw@amd.com> --- drivers/pci/controller/pcie-xilinx-cpm.c | 39 +++++++++++++++++++----- 1 file changed, 32 insertions(+), 7 deletions(-)