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
>
>
> --
> மணிவண்ணன் சதாசிவம்
