diff mbox series

[2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root Port controller-1

Message ID 20240906093148.830452-3-thippesw@amd.com (mailing list archive)
State New
Headers show
Series Add support for CPM5 controller-1. | expand

Commit Message

Thippeswamy Havalige Sept. 6, 2024, 9:31 a.m. UTC
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(-)

Comments

Krzysztof Kozlowski Sept. 6, 2024, 9:56 a.m. UTC | #1
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
Bjorn Helgaas Sept. 6, 2024, 7:19 p.m. UTC | #2
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
kernel test robot Sept. 7, 2024, 6:15 a.m. UTC | #3
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
kernel test robot Sept. 7, 2024, 9 a.m. UTC | #4
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
kernel test robot Sept. 7, 2024, 10:01 a.m. UTC | #5
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
Havalige, Thippeswamy Sept. 12, 2024, 9:38 a.m. UTC | #6
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 mbox series

Patch

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,
+	},
 	{}
 };