| Message ID | CH2PPF4D26F8E1C5FA4D55D4271BA4F6F0DA2E82@CH2PPF4D26F8E1C.namprd07.prod.outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | pci: cdns : Function to read controller architecture | expand |
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
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
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
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;
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(+)