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@xxxxxxxxxx/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