Re: [PATCH 4/7] PCI: cadence: Add support for PCIe Endpoint HPA controllers

[Bjorn Helgaas]

On Thu, Mar 27, 2025 at 11:40:36AM +0000, Manikandan Karunakaran Pillai wrote:
> Add support for the second generation(HPA) Cadence PCIe endpoint
> controller by adding the required functions based on the HPA registers
> and register bit definitions

Add period.

> @@ -93,7 +93,10 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>  	 * for 64bit values.
>  	 */
>  	sz = 1ULL << fls64(sz - 1);
> -	aperture = ilog2(sz) - 7; /* 128B -> 0, 256B -> 1, 512B -> 2, ... */
> +	/*
> +	 * 128B -> 0, 256B -> 1, 512B -> 2, ...
> +	 */
> +	aperture = ilog2(sz) - 7;

Unclear exactly how this is related to HPA and whether it affects
non-HPA.

> @@ -121,7 +124,7 @@ static int cdns_pcie_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>  		reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG(bar, fn);
>  	else
>  		reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG(bar, fn);
> -	b = (bar < BAR_4) ? bar : bar - BAR_4;
> +	b = (bar < BAR_3) ? bar : bar - BAR_3;

Unclear what's going on here because this doesn't look specific to
HPA.  Should this be a separate patch that fixes an existing defect?

>  	if (vfn == 0 || vfn == 1) {
>  		cfg = cdns_pcie_readl(pcie, reg);
> @@ -158,7 +161,7 @@ static void cdns_pcie_ep_clear_bar(struct pci_epc *epc, u8 fn, u8 vfn,
>  		reg = CDNS_PCIE_LM_EP_VFUNC_BAR_CFG(bar, fn);
>  	else
>  		reg = CDNS_PCIE_LM_EP_FUNC_BAR_CFG(bar, fn);
> -	b = (bar < BAR_4) ? bar : bar - BAR_4;
> +	b = (bar < BAR_3) ? bar : bar - BAR_3;

And here.

> @@ -569,7 +572,11 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
>  	 * and can't be disabled anyway.
>  	 */
> -	cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, epc->function_num_map);
> +	if (pcie->is_hpa)
> +		cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG,
> +				     CDNS_PCIE_HPA_LM_EP_FUNC_CFG, epc->function_num_map);
> +	else
> +		cdns_pcie_writel(pcie, CDNS_PCIE_LM_EP_FUNC_CFG, epc->function_num_map);

Sprinkling tests of "is_hpa" around is not very extensible.  When the
next generation after HPA shows up, then it gets really messy.
Sometimes generation-specific function pointers can make this simpler.

> +static int cdns_pcie_hpa_ep_set_bar(struct pci_epc *epc, u8 fn, u8 vfn,
> +				    struct pci_epf_bar *epf_bar)
> +{
> +	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> +	struct cdns_pcie_epf *epf = &ep->epf[fn];
> +	struct cdns_pcie *pcie = &ep->pcie;
> +	dma_addr_t bar_phys = epf_bar->phys_addr;
> +	enum pci_barno bar = epf_bar->barno;
> +	int flags = epf_bar->flags;
> +	u32 addr0, addr1, reg, cfg, b, aperture, ctrl;
> +	u64 sz;
> +
> +	/*
> +	 * BAR size is 2^(aperture + 7)
> +	 */
> +	sz = max_t(size_t, epf_bar->size, CDNS_PCIE_EP_MIN_APERTURE);
> +	/*

Add blank line between code and comment.

> +	 * roundup_pow_of_two() returns an unsigned long, which is not suited
> +	 * for 64bit values.
> +	 */
> +	sz = 1ULL << fls64(sz - 1);
> +	/*

Again.  Check for other places in this series.

> +	 * 128B -> 0, 256B -> 1, 512B -> 2, ...
> +	 */
> +	aperture = ilog2(sz) - 7;