[PATCH 0/2] Add support for CPM5 controller-1.

[PATCH 0/2] Add support for CPM5 controller-1.

[Thippeswamy Havalige]

This patch series adds support for the Xilinx Versal Premium Controller-1
as a Root Port.

The Versal Premium device supports two controllers, both operating at Gen5.
speed. Only one controller can be used at any given time.

The primary difference between the two controllers are the register offsets
and bits related to platform-specific errors

Thippeswamy Havalige (2):
  dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 host1.
  PCI: xilinx-cpm: Add support for Versal CPM5 Root Port controller 1

 .../bindings/pci/xilinx-versal-cpm.yaml       |  1 +
 drivers/pci/controller/pcie-xilinx-cpm.c      | 39 +++++++++++++++----
 2 files changed, 33 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Thippeswamy Havalige]

The Xilinx Versal premium series has CPM5 block which supports two typeA
Root Port controller functionality at Gen5 speed.

Add compatible string to distinguish between two CPM5 rootport controller1.

Error interrupt register and bits for both the controllers
are at different.

Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
---
 Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
index 989fb0fa2577..b63a759ec2d7 100644
--- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
+++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
@@ -17,6 +17,7 @@ properties:
     enum:
       - xlnx,versal-cpm-host-1.00
       - xlnx,versal-cpm5-host
+      - xlnx,versal-cpm5-host1
 
   reg:
     items:
-- 
2.34.1

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

[Thippeswamy Havalige]

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

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

[Krzysztof Kozlowski]

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

Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Krzysztof Kozlowski]

On 06/09/2024 11:31, Thippeswamy Havalige wrote:
> The Xilinx Versal premium series has CPM5 block which supports two typeA
> Root Port controller functionality at Gen5 speed.
> 
> Add compatible string to distinguish between two CPM5 rootport controller1.

Subjects NEVER end with full stops.
> 
> Error interrupt register and bits for both the controllers
> are at different.
> 
> Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
> ---
>  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> index 989fb0fa2577..b63a759ec2d7 100644
> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> @@ -17,6 +17,7 @@ properties:
>      enum:
>        - xlnx,versal-cpm-host-1.00
>        - xlnx,versal-cpm5-host
> +      - xlnx,versal-cpm5-host1

That's poor naming. "-1.00" and now "1". Get your naming reasonable...

Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Havalige, Thippeswamy]

Hi Krzysztof

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, September 6, 2024 3:26 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> 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
> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string
> for CPM5 controller-1.
> 
> On 06/09/2024 11:31, Thippeswamy Havalige wrote:
> > The Xilinx Versal premium series has CPM5 block which supports two
> > typeA Root Port controller functionality at Gen5 speed.
> >
> > Add compatible string to distinguish between two CPM5 rootport
> controller1.
> 
> Subjects NEVER end with full stops.
Thanks, Update in the next patch series.
> >
> > Error interrupt register and bits for both the controllers are at
> > different.
> >
> > Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
> > ---
> >  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > index 989fb0fa2577..b63a759ec2d7 100644
> > --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> > @@ -17,6 +17,7 @@ properties:
> >      enum:
> >        - xlnx,versal-cpm-host-1.00
> >        - xlnx,versal-cpm5-host
> > +      - xlnx,versal-cpm5-host1
> 
> That's poor naming. "-1.00" and now "1". Get your naming reasonable...
Here 1.00 represents the IP versioning and host1 represents controller-1. 
> 
> Best regards,
> Krzysztof

Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Krzysztof Kozlowski]

On 06/09/2024 12:43, Havalige, Thippeswamy wrote:
> Hi Krzysztof
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Friday, September 6, 2024 3:26 PM
>> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
>> 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
>> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
>> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
>> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string
>> for CPM5 controller-1.
>>
>> On 06/09/2024 11:31, Thippeswamy Havalige wrote:
>>> The Xilinx Versal premium series has CPM5 block which supports two
>>> typeA Root Port controller functionality at Gen5 speed.
>>>
>>> Add compatible string to distinguish between two CPM5 rootport
>> controller1.
>>
>> Subjects NEVER end with full stops.
> Thanks, Update in the next patch series.
>>>
>>> Error interrupt register and bits for both the controllers are at
>>> different.
>>>
>>> Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
>>> ---
>>>  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> index 989fb0fa2577..b63a759ec2d7 100644
>>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>> @@ -17,6 +17,7 @@ properties:
>>>      enum:
>>>        - xlnx,versal-cpm-host-1.00
>>>        - xlnx,versal-cpm5-host
>>> +      - xlnx,versal-cpm5-host1
>>
>> That's poor naming. "-1.00" and now "1". Get your naming reasonable...
> Here 1.00 represents the IP versioning and host1 represents controller-1. 

I understand but you repeating the same is not helping. Make it better
and next time upstream "host1-1" compatible.

Number of ports, BTW, comes from ports, right? So no need for the
compatible.

Best regards,
Krzysztof

RE: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Havalige, Thippeswamy]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, September 6, 2024 4:16 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> 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
> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string
> for CPM5 controller-1.
> 
> On 06/09/2024 12:43, Havalige, Thippeswamy wrote:
> > Hi Krzysztof
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Friday, September 6, 2024 3:26 PM
> >> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> >> 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
> >> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> Michal
> >> <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> >> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible
> >> string for CPM5 controller-1.
> >>
> >> On 06/09/2024 11:31, Thippeswamy Havalige wrote:
> >>> The Xilinx Versal premium series has CPM5 block which supports two
> >>> typeA Root Port controller functionality at Gen5 speed.
> >>>
> >>> Add compatible string to distinguish between two CPM5 rootport
> >> controller1.
> >>
> >> Subjects NEVER end with full stops.
> > Thanks, Update in the next patch series.
> >>>
> >>> Error interrupt register and bits for both the controllers are at
> >>> different.
> >>>
> >>> Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
> >>> ---
> >>>  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git
> >>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> index 989fb0fa2577..b63a759ec2d7 100644
> >>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>> @@ -17,6 +17,7 @@ properties:
> >>>      enum:
> >>>        - xlnx,versal-cpm-host-1.00
> >>>        - xlnx,versal-cpm5-host
> >>> +      - xlnx,versal-cpm5-host1
> >>
> >> That's poor naming. "-1.00" and now "1". Get your naming reasonable...
> > Here 1.00 represents the IP versioning and host1 represents controller-1.
> 
> I understand but you repeating the same is not helping. Make it better and
> next time upstream "host1-1" compatible.
> 
> Number of ports, BTW, comes from ports, right? So no need for the
> compatible.

To differentiate between the registers for Controller-0 and Controller-1, I am utilizing a compatible string in the driver. This approach enables the driver to identify and manage the registers associated with each controller based on the specified compatible string.


> Best regards,
> Krzysztof
Regards,
Thippeswamy H

Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Krzysztof Kozlowski]

On 06/09/2024 12:50, Havalige, Thippeswamy wrote:
> Hi Krzysztof,
> 
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzk@kernel.org>
>> Sent: Friday, September 6, 2024 4:16 PM
>> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
>> 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
>> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
>> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
>> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string
>> for CPM5 controller-1.
>>
>> On 06/09/2024 12:43, Havalige, Thippeswamy wrote:
>>> Hi Krzysztof
>>>
>>>> -----Original Message-----
>>>> From: Krzysztof Kozlowski <krzk@kernel.org>
>>>> Sent: Friday, September 6, 2024 3:26 PM
>>>> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
>>>> 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
>>>> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
>> Michal
>>>> <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
>>>> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible
>>>> string for CPM5 controller-1.
>>>>
>>>> On 06/09/2024 11:31, Thippeswamy Havalige wrote:
>>>>> The Xilinx Versal premium series has CPM5 block which supports two
>>>>> typeA Root Port controller functionality at Gen5 speed.
>>>>>
>>>>> Add compatible string to distinguish between two CPM5 rootport
>>>> controller1.
>>>>
>>>> Subjects NEVER end with full stops.
>>> Thanks, Update in the next patch series.
>>>>>
>>>>> Error interrupt register and bits for both the controllers are at
>>>>> different.
>>>>>
>>>>> Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
>>>>>  1 file changed, 1 insertion(+)
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>>>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>>>> index 989fb0fa2577..b63a759ec2d7 100644
>>>>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>>>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
>>>>> @@ -17,6 +17,7 @@ properties:
>>>>>      enum:
>>>>>        - xlnx,versal-cpm-host-1.00
>>>>>        - xlnx,versal-cpm5-host
>>>>> +      - xlnx,versal-cpm5-host1
>>>>
>>>> That's poor naming. "-1.00" and now "1". Get your naming reasonable...
>>> Here 1.00 represents the IP versioning and host1 represents controller-1.
>>
>> I understand but you repeating the same is not helping. Make it better and
>> next time upstream "host1-1" compatible.
>>
>> Number of ports, BTW, comes from ports, right? So no need for the
>> compatible.
> 
> To differentiate between the registers for Controller-0 and Controller-1, I am utilizing a compatible string in the driver. This approach enables the driver to identify and manage the registers associated with each controller based on the specified compatible string.
> 

Please don't state the obvious... I know how Linux kernel works. But
maybe I wasn't clear - do you have ports property there? I guess not, as
it is PCI.

What I claim here, is that you have exactly the same hardware. Same
hardware, same compatible.

Best regards,
Krzysztof

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

[Bjorn Helgaas]

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

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

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	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

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  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string for CPM5 controller-1.

[Havalige, Thippeswamy]

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Friday, September 6, 2024 5:49 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> 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
> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible string
> for CPM5 controller-1.
> 
> On 06/09/2024 12:50, Havalige, Thippeswamy wrote:
> > Hi Krzysztof,
> >
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzk@kernel.org>
> >> Sent: Friday, September 6, 2024 4:16 PM
> >> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> >> 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
> >> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> >> Michal <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> >> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible
> string
> >> for CPM5 controller-1.
> >>
> >> On 06/09/2024 12:43, Havalige, Thippeswamy wrote:
> >>> Hi Krzysztof
> >>>
> >>>> -----Original Message-----
> >>>> From: Krzysztof Kozlowski <krzk@kernel.org>
> >>>> Sent: Friday, September 6, 2024 3:26 PM
> >>>> To: Havalige, Thippeswamy <thippeswamy.havalige@amd.com>;
> >>>> 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
> >>>> Cc: Gogada, Bharat Kumar <bharat.kumar.gogada@amd.com>; Simek,
> >> Michal
> >>>> <michal.simek@amd.com>; lpieralisi@kernel.org; kw@linux.com
> >>>> Subject: Re: [PATCH 1/2] dt-bindings: PCI: xilinx-cpm: Add compatible
> >>>> string for CPM5 controller-1.
> >>>>
> >>>> On 06/09/2024 11:31, Thippeswamy Havalige wrote:
> >>>>> The Xilinx Versal premium series has CPM5 block which supports two
> >>>>> typeA Root Port controller functionality at Gen5 speed.
> >>>>>
> >>>>> Add compatible string to distinguish between two CPM5 rootport
> >>>> controller1.
> >>>>
> >>>> Subjects NEVER end with full stops.
> >>> Thanks, Update in the next patch series.
> >>>>>
> >>>>> Error interrupt register and bits for both the controllers are at
> >>>>> different.
> >>>>>
> >>>>> Signed-off-by: Thippeswamy Havalige <thippesw@amd.com>
> >>>>> ---
> >>>>>  Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml | 1 +
> >>>>>  1 file changed, 1 insertion(+)
> >>>>>
> >>>>> diff --git
> >>>>> a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>>>> b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>>>> index 989fb0fa2577..b63a759ec2d7 100644
> >>>>> --- a/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/pci/xilinx-versal-cpm.yaml
> >>>>> @@ -17,6 +17,7 @@ properties:
> >>>>>      enum:
> >>>>>        - xlnx,versal-cpm-host-1.00
> >>>>>        - xlnx,versal-cpm5-host
> >>>>> +      - xlnx,versal-cpm5-host1
> >>>>
> >>>> That's poor naming. "-1.00" and now "1". Get your naming
> reasonable...
> >>> Here 1.00 represents the IP versioning and host1 represents controller-
> 1.
> >>
> >> I understand but you repeating the same is not helping. Make it better
> and
> >> next time upstream "host1-1" compatible.
> >>
> >> Number of ports, BTW, comes from ports, right? So no need for the
> >> compatible.
> >
> > To differentiate between the registers for Controller-0 and Controller-1, I
> am utilizing a compatible string in the driver. This approach enables the
> driver to identify and manage the registers associated with each controller
> based on the specified compatible string.
> >
> 
> Please don't state the obvious... I know how Linux kernel works. But
> maybe I wasn't clear - do you have ports property there? I guess not, as
> it is PCI.
> 
> What I claim here, is that you have exactly the same hardware. Same
> hardware, same compatible.


Apologies for the misunderstanding. You're correct—the ports property is not applicable
to PCI devices.
Based on Bjorn's input, I'll follow the recommended process for handling this scenario.
Thank you for the clarification.

> Best regards,
> Krzysztof

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

[Havalige, Thippeswamy]

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