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

[Havalige, Thippeswamy]

Hi Manivannan Sadhasivam,
> -----Original Message-----
> From: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> Sent: Tuesday, September 17, 2024 8:40 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>
> Cc: robh@xxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; bhelgaas@xxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Gogada, Bharat Kumar
> <bharat.kumar.gogada@xxxxxxx>; Simek, Michal <michal.simek@xxxxxxx>;
> lpieralisi@xxxxxxxxxx; kw@xxxxxxxxx
> Subject: Re: [PATCH v2 2/2] PCI: xilinx-cpm: Add support for Versal CPM5 Root
> Port controller 1
> 
> On Mon, Sep 16, 2024 at 10:07:48PM +0530, Thippeswamy Havalige wrote:
> 
> For some reason, this patch is not threaded and not part of the series:
> [PATCH v2 0/2] Add support for CPM5 controller 1
> 
> > This patch adds support for the Xilinx Versal CPM5 Root Port Controller 1.
> 
> s/patch/commit
> 
> Once this patch gets merged, it will become a commit.
Thanks for review, will update this in next patch.
> 
> > The key difference between Controller 0 and Controller 1 lies in the
> > platform-specific error interrupt bits, which are located at different
> > register offsets.
> >
> > To handle these differences, a variant structure is introduced that
> > holds the following platform-specific details:
> >
> 
> The variant structure is already present in the driver. Hence not introduced in *this*
> patch.
Thanks for review, will update this in next patch.

> > - Interrupt status register offset (ir_status)
> > - Interrupt enable register offset (ir_enable)
> > - Miscellaneous interrupt values (ir_misc_value)
> >
> > The driver differentiates between Controller 0 and Controller 1 using
> > the compatible string in the device tree. This ensures that the
> > appropriate register offsets are used for each controller, allowing
> > for correct handling of platform-specific interrupts and initialization.
> >
> > Signed-off-by: Thippeswamy Havalige <thippesw@xxxxxxx>
> > ---
> > changes in v2:
> > --------------
> > 1. Introduced new constants for Controller 1.
> > 2. Extended the xilinx_cpm_variant structure to support
> > 	a. ir_status,
> > 	b. ir_enable, and
> > 	c. ir_misc_value for different controllers.
> > 3. Updated IRQ handling and initialization to use the variant structure.
> > 4. Added a new device tree match entry for Controller 1.
> > ---
> >  drivers/pci/controller/pcie-xilinx-cpm.c | 47
> > ++++++++++++++++++------
> >  1 file changed, 36 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pcie-xilinx-cpm.c
> > b/drivers/pci/controller/pcie-xilinx-cpm.c
> > index a0f5e1d67b04..b783fff27c9d 100644
> > --- a/drivers/pci/controller/pcie-xilinx-cpm.c
> > +++ b/drivers/pci/controller/pcie-xilinx-cpm.c
> > @@ -30,11 +30,14 @@
> >  #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_PCIE_IR_LOCAL        BIT(0)
> > +#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)
> >
> > @@ -80,6 +83,7 @@
> >  enum xilinx_cpm_version {
> >  	CPM,
> >  	CPM5,
> > +	CPM5_HOST1,
> >  };
> >
> >  /**
> > @@ -88,6 +92,9 @@ enum xilinx_cpm_version {
> >   */
> >  struct xilinx_cpm_variant {
> >  	enum xilinx_cpm_version version;
> > +	u32 ir_status;
> > +	u32 ir_enable;
> > +	u32 ir_misc_value;
> 
> Kdoc comments missing for these members.
Thanks for review, will update this in next patch.
> 
> >  };
> >
> >  /**
> > @@ -269,6 +276,7 @@ static void xilinx_cpm_pcie_event_flow(struct
> > irq_desc *desc)  {
> >  	struct xilinx_cpm_pcie *port = irq_desc_get_handler_data(desc);
> >  	struct irq_chip *chip = irq_desc_get_chip(desc);
> > +	const struct xilinx_cpm_variant *variant = port->variant;
> >  	unsigned long val;
> >  	int i;
> >
> > @@ -279,11 +287,11 @@ static void xilinx_cpm_pcie_event_flow(struct irq_desc
> *desc)
> >  		generic_handle_domain_irq(port->cpm_domain, i);
> >  	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);
> > +	if (variant->ir_status) {
> > +		val = readl_relaxed(port->cpm_base + variant->ir_status);
> >  		if (val)
> >  			writel_relaxed(val, port->cpm_base +
> > -					    XILINX_CPM_PCIE_IR_STATUS);
> > +				       variant->ir_status);
> >  	}
> >
> >  	/*
> > @@ -465,6 +473,8 @@ static int xilinx_cpm_setup_irq(struct xilinx_cpm_pcie
> *port)
> >   */
> >  static void xilinx_cpm_pcie_init_port(struct xilinx_cpm_pcie *port)
> > {
> > +	const struct xilinx_cpm_variant *variant = port->variant;
> > +
> >  	if (cpm_pcie_link_up(port))
> >  		dev_info(port->dev, "PCIe Link is UP\n");
> >  	else
> > @@ -483,15 +493,15 @@ 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(variant->ir_misc_value,
> >  	       port->cpm_base + XILINX_CPM_PCIE_MISC_IR_ENABLE);
> >
> > -	if (port->variant->version == CPM5) {
> > +	if (variant->ir_enable) {
> >  		writel(XILINX_CPM_PCIE_IR_LOCAL,
> > -		       port->cpm_base + XILINX_CPM_PCIE_IR_ENABLE);
> > +		       port->cpm_base + variant->ir_enable);
> >  	}
> >
> > -	/* Enable the Bridge enable bit */
> > +	/* Set Bridge enable bit */
> 
> This changes doesn't belong to this patch.
Thanks for review, will update this in next patch.
> 
> >  	pcie_write(port, pcie_read(port, XILINX_CPM_PCIE_REG_RPSC) |
> >  		   XILINX_CPM_PCIE_REG_RPSC_BEN,
> >  		   XILINX_CPM_PCIE_REG_RPSC);
> > @@ -609,10 +619,21 @@ static int xilinx_cpm_pcie_probe(struct
> > platform_device *pdev)
> >
> >  static const struct xilinx_cpm_variant cpm_host = {
> >  	.version = CPM,
> > +	.ir_misc_value = XILINX_CPM_PCIE0_MISC_IR_LOCAL,
> >  };
> >
> >  static const struct xilinx_cpm_variant cpm5_host = {
> >  	.version = CPM5,
> > +	.ir_misc_value = XILINX_CPM_PCIE0_MISC_IR_LOCAL,
> > +	.ir_status = XILINX_CPM_PCIE0_IR_STATUS,
> > +	.ir_enable = XILINX_CPM_PCIE0_IR_ENABLE, };
> > +
> > +static const struct xilinx_cpm_variant cpm5_host1 = {
> > +	.version = CPM5_HOST1,
> > +	.ir_misc_value = XILINX_CPM_PCIE1_MISC_IR_LOCAL,
> > +	.ir_status = XILINX_CPM_PCIE1_IR_STATUS,
> > +	.ir_enable = XILINX_CPM_PCIE1_IR_ENABLE,
> >  };
> >
> >  static const struct of_device_id xilinx_cpm_pcie_of_match[] = { @@
> > -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-1",
> 
> This doesn't look like a valid compatible name. Please use the compatible as per
> the IP version.
Thanks, Here we don't have any IP versioning for CPM5, so I ll make compatible string as xlnx,versal-cpm5-host1 as updated to Krzysztof Kozlowski. 
> 
> - Mani
> 
> 
> --
> மணிவண்ணன் சதாசிவம்