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

[Bjorn Helgaas <helgaas@kernel.org>]

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