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