[PATCH] pci: cdns : Function to read controller architecture

[PATCH] pci: cdns : Function to read controller architecture

[Manikandan Karunakaran Pillai]

Add support for getting the architecture for Cadence PCIe controllers
Store the architecture type in controller structure.

Signed-off-by: Manikandan K Pillai <mpillai@cadence.com>
---
 .../controller/cadence/pcie-cadence-plat.c    | 20 +++++++++++++++++++
 drivers/pci/controller/cadence/pcie-cadence.h |  8 ++++++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/pci/controller/cadence/pcie-cadence-plat.c b/drivers/pci/controller/cadence/pcie-cadence-plat.c
index 0456845dabb9..d1cfb9847b7c 100644
--- a/drivers/pci/controller/cadence/pcie-cadence-plat.c
+++ b/drivers/pci/controller/cadence/pcie-cadence-plat.c
@@ -37,6 +37,24 @@ static const struct cdns_pcie_ops cdns_plat_ops = {
 	.cpu_addr_fixup = cdns_plat_cpu_addr_fixup,
 };
 
+static void cdns_pcie_ctlr_set_arch(struct cdns_pcie *pcie)
+{
+	/* Read register at offset 0xE4 of the config space
+	 * The value for architecture is in the lower 4 bits
+	 * Legacy-b'0010 and b'1111 for HPA-high performance architecture
+	 */
+	u32 arch, reg;
+
+	reg = cdns_pcie_readl(pcie, CDNS_PCIE_CTRL_ARCH);
+	arch = FIELD_GET(CDNS_PCIE_CTRL_ARCH_MASK, reg);
+
+	if (arch == CDNS_PCIE_CTRL_HPA) {
+		pcie->is_hpa = true;
+	} else {
+		pcie->is_hpa = false;
+	}
+}
+
 static int cdns_plat_pcie_probe(struct platform_device *pdev)
 {
 	const struct cdns_plat_pcie_of_data *data;
@@ -74,6 +92,7 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
 		rc->pcie.ops = &cdns_plat_ops;
 		cdns_plat_pcie->pcie = &rc->pcie;
 
+		cdns_pcie_ctlr_set_arch(&rc->pcie);
 		ret = cdns_pcie_init_phy(dev, cdns_plat_pcie->pcie);
 		if (ret) {
 			dev_err(dev, "failed to init phy\n");
@@ -101,6 +120,7 @@ static int cdns_plat_pcie_probe(struct platform_device *pdev)
 		ep->pcie.ops = &cdns_plat_ops;
 		cdns_plat_pcie->pcie = &ep->pcie;
 
+		cdns_pcie_ctlr_set_arch(&ep->pcie);
 		ret = cdns_pcie_init_phy(dev, cdns_plat_pcie->pcie);
 		if (ret) {
 			dev_err(dev, "failed to init phy\n");
diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
index f5eeff834ec1..2d9ecd923220 100644
--- a/drivers/pci/controller/cadence/pcie-cadence.h
+++ b/drivers/pci/controller/cadence/pcie-cadence.h
@@ -16,6 +16,13 @@
 #define LINK_WAIT_USLEEP_MIN	90000
 #define LINK_WAIT_USLEEP_MAX	100000
 
+/*
+ * Read completion time out reset value to decode controller architecture
+ */
+#define CDNS_PCIE_CTRL_ARCH		0xE4
+#define CDNS_PCIE_CTRL_ARCH_MASK	GENMASK(3, 0)
+#define CDNS_PCIE_CTRL_HPA		0xF
+
 /*
  * Local Management Registers
  */
@@ -305,6 +312,7 @@ struct cdns_pcie {
 	struct resource		*mem_res;
 	struct device		*dev;
 	bool			is_rc;
+	bool			is_hpa;
 	int			phy_count;
 	struct phy		**phy;
 	struct device_link	**link;
-- 
2.27.0

Re: [PATCH] pci: cdns : Function to read controller architecture

[Bjorn Helgaas]

Look at previous subject lines for changes to these files and follow
the pattern.

On Fri, Jan 31, 2025 at 11:58:07AM +0000, Manikandan Karunakaran Pillai wrote:
> Add support for getting the architecture for Cadence PCIe controllers
> Store the architecture type in controller structure.

This needs to be part of a series that uses pcie->is_hpa for
something.  This patch all by itself isn't useful for anything.

Please post the resulting series with a cover letter and the patches
as responses to it:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.13#n333

You can look at previous postings to see the style, e.g.,
https://lore.kernel.org/linux-pci/20250115074301.3514927-1-pandoh@google.com/T/#t

> +static void cdns_pcie_ctlr_set_arch(struct cdns_pcie *pcie)
> +{
> +	/* Read register at offset 0xE4 of the config space
> +	 * The value for architecture is in the lower 4 bits
> +	 * Legacy-b'0010 and b'1111 for HPA-high performance architecture
> +	 */

Don't include the hex register offset in the comment.  That's what
CDNS_PCIE_CTRL_ARCH is for.  It doesn't need the bit values either.

Use the conventional comment style:

  /*
   * Text ...
   */

> +	u32 arch, reg;
> +
> +	reg = cdns_pcie_readl(pcie, CDNS_PCIE_CTRL_ARCH);
> +	arch = FIELD_GET(CDNS_PCIE_CTRL_ARCH_MASK, reg);

Thanks for using GENMASK() and FIELD_GET().

> +	if (arch == CDNS_PCIE_CTRL_HPA) {
> +		pcie->is_hpa = true;
> +	} else {
> +		pcie->is_hpa = false;
> +	}
> +}

> +/*
> + * Read completion time out reset value to decode controller architecture
> + */
> +#define CDNS_PCIE_CTRL_ARCH		0xE4

Is this another name for the PCI_EXP_DEVCTL2 in the PCIe Capability?
Or maybe PCI_EXP_DEVCAP2?  If so, use those existing #defines and the
related masks (if it's DEVCAP2, you'd probably have to add a new one
for the Completion Timeout Ranges Supported field).

There's something similar in cdns_pcie_retrain(), where
CDNS_PCIE_RP_CAP_OFFSET is apparently the config space offset of the
PCIe Capability.

> +#define CDNS_PCIE_CTRL_ARCH_MASK	GENMASK(3, 0)
> +#define CDNS_PCIE_CTRL_HPA		0xF

RE: [PATCH] pci: cdns : Function to read controller architecture

[Manikandan Karunakaran Pillai]

Would like to change the design to get the architecture value from dts, using a bool hpa
And store this value in the is_hpa field in the struct as given.

There would be support for legacy and High performance architecture in different files
And the difference would be basically the registers they write and the offsets of these 
registers. The function names would almost be similar with the tag hpa, embedded in
the function name. 

Would this be an acceptable design for support of these new PCIe cadence controllers ? 


>Look at previous subject lines for changes to these files and follow the pattern.
>
>On Fri, Jan 31, 2025 at 11:58:07AM +0000, Manikandan Karunakaran Pillai
>wrote:
>> Add support for getting the architecture for Cadence PCIe controllers
>> Store the architecture type in controller structure.
>
>This needs to be part of a series that uses pcie->is_hpa for something.  This
>patch all by itself isn't useful for anything.
>
>Please post the resulting series with a cover letter and the patches as
>responses to it:
>
>https://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/t
>orvalds/linux.git/tree/Documentation/process/5.Posting.rst?id=v6.13*n333__;I
>w!!EHscmS1ygiU1lA!AkA94rjfoiZElA3AKt_SRyFA74hygGR-
>X7t7_oZzijqCt3Ojr_UVL2Q9RLTXs4juahroWPLzA6CJFZI$
>
>You can look at previous postings to see the style, e.g.,
>https://urldefense.com/v3/__https://lore.kernel.org/linux-
>pci/20250115074301.3514927-1-
>pandoh@google.com/T/*t__;Iw!!EHscmS1ygiU1lA!AkA94rjfoiZElA3AKt_SRyFA7
>4hygGR-X7t7_oZzijqCt3Ojr_UVL2Q9RLTXs4juahroWPLzuNTbmWU$
>
>> +static void cdns_pcie_ctlr_set_arch(struct cdns_pcie *pcie) {
>> +	/* Read register at offset 0xE4 of the config space
>> +	 * The value for architecture is in the lower 4 bits
>> +	 * Legacy-b'0010 and b'1111 for HPA-high performance architecture
>> +	 */
>
>Don't include the hex register offset in the comment.  That's what
>CDNS_PCIE_CTRL_ARCH is for.  It doesn't need the bit values either.
>
>Use the conventional comment style:
>
>  /*
>   * Text ...
>   */
>
>> +	u32 arch, reg;
>> +
>> +	reg = cdns_pcie_readl(pcie, CDNS_PCIE_CTRL_ARCH);
>> +	arch = FIELD_GET(CDNS_PCIE_CTRL_ARCH_MASK, reg);
>
>Thanks for using GENMASK() and FIELD_GET().
>
>> +	if (arch == CDNS_PCIE_CTRL_HPA) {
>> +		pcie->is_hpa = true;
>> +	} else {
>> +		pcie->is_hpa = false;
>> +	}
>> +}
>
>> +/*
>> + * Read completion time out reset value to decode controller
>> +architecture  */
>> +#define CDNS_PCIE_CTRL_ARCH		0xE4
>
>Is this another name for the PCI_EXP_DEVCTL2 in the PCIe Capability?
>Or maybe PCI_EXP_DEVCAP2?  If so, use those existing #defines and the
>related masks (if it's DEVCAP2, you'd probably have to add a new one for the
>Completion Timeout Ranges Supported field).
>
>There's something similar in cdns_pcie_retrain(), where
>CDNS_PCIE_RP_CAP_OFFSET is apparently the config space offset of the PCIe
>Capability.
>
>> +#define CDNS_PCIE_CTRL_ARCH_MASK	GENMASK(3, 0)
>> +#define CDNS_PCIE_CTRL_HPA		0xF

Re: [PATCH] pci: cdns : Function to read controller architecture

[Bjorn Helgaas]

On Mon, Feb 03, 2025 at 02:04:17PM +0000, Manikandan Karunakaran Pillai wrote:
> Would like to change the design to get the architecture value from
> dts, using a bool hpa And store this value in the is_hpa field in
> the struct as given.
> 
> There would be support for legacy and High performance architecture
> in different files And the difference would be basically the
> registers they write and the offsets of these registers. The
> function names would almost be similar with the tag hpa, embedded in
> the function name. 
> 
> Would this be an acceptable design for support of these new PCIe
> cadence controllers ? 

Look around at other drivers that handle similar issues and use a
similar solution.  drivers/pci/controller/dwc/pcie-qcom.c is one
example, but most drivers support variants with minor differences.

Usual Linux email style is for responses to include only relevant
parts with replies interleaved:
https://people.kernel.org/tglx/notes-about-netiquette

Bjorn