Re: [PATCH 6/7] PCI: cadence: Add callback functions for Root Port and EP controller

[Bjorn Helgaas <helgaas@kernel.org>]

[+cc Frank for .cpu_addr_fixup()]

On Thu, Mar 27, 2025 at 11:42:27AM +0000, Manikandan Karunakaran Pillai wrote:
> Add support for the second generation PCIe controller by adding
> the required callback functions. Update the common functions for
> endpoint and Root port modes. Invoke the relevant callback functions
> for platform probe of PCIe controller using the callback functions

Pick "second generation" or "HPA" and use it consistently so we can
keep this all straight.

s/endpoint/Endpoint/
s/Root port/Root Port/

Add period again.

> @@ -877,7 +877,7 @@ int cdns_pcie_ep_setup(struct cdns_pcie_ep *ep)
>  	set_bit(0, &ep->ob_region_map);
>  
>  	if (ep->quirk_detect_quiet_flag)
> -		cdns_pcie_detect_quiet_min_delay_set(&ep->pcie);
> +		pcie->ops->pcie_detect_quiet_min_delay_set(&ep->pcie);

Maybe the quirk check should go inside .pcie_detect_quiet_min_delay()?
Just an idea, maybe that wouldn't help.

> +void __iomem *cdns_pci_hpa_map_bus(struct pci_bus *bus, unsigned int devfn,
> +				   int where)
> +{
> +	struct pci_host_bridge *bridge = pci_find_host_bridge(bus);
> +	struct cdns_pcie_rc *rc = pci_host_bridge_priv(bridge);
> +	struct cdns_pcie *pcie = &rc->pcie;
> +	unsigned int busn = bus->number;
> +	u32 addr0, desc0, desc1, ctrl0;
> +	u32 regval;
> +
> +	if (pci_is_root_bus(bus)) {
> +		/*
> +		 * Only the root port (devfn == 0) is connected to this bus.
> +		 * All other PCI devices are behind some bridge hence on another
> +		 * bus.
> +		 */
> +		if (devfn)
> +			return NULL;
> +
> +		return pcie->reg_base + (where & 0xfff);
> +	}
> +
> +	/*
> +	 * Clear AXI link-down status
> +	 */
> +	regval = cdns_pcie_hpa_readl(pcie, REG_BANK_AXI_SLAVE, CDNS_PCIE_HPA_AT_LINKDOWN);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE, CDNS_PCIE_HPA_AT_LINKDOWN,
> +			     (regval & GENMASK(0, 0)));
> +
> +	desc1 = 0;
> +	ctrl0 = 0;
> +	/*

Blank line before comment.  You could make this a single-line comment,
e.g.,

  /* Update Output registers for AXI region 0. */

> +	 * Update Output registers for AXI region 0.
> +	 */
> +	addr0 = CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_NBITS(12) |
> +		CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_DEVFN(devfn) |
> +		CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_BUS(busn);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0(0), addr0);
> +
> +	desc1 = cdns_pcie_hpa_readl(pcie, REG_BANK_AXI_SLAVE,
> +				    CDNS_PCIE_HPA_AT_OB_REGION_DESC1(0));
> +	desc1 &= ~CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN_MASK;
> +	desc1 |= CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN(0);
> +	ctrl0 = CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_BUS |
> +		CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_DEV_FN;
> +	/*

Again.

> +	 * The bus number was already set once for all in desc1 by
> +	 * cdns_pcie_host_init_address_translation().

This comment sounds like you only support the root bus and a single
other bus.  But you're not actually setting the *bus number* here;
you're setting up either a Type 0 access (for the Root Port's
secondary bus) or a Type 1 access (for anything else, e.g. things
below a switch).

> +	 */
> +	if (busn == bridge->busnr + 1)

The Root Port's secondary bus number need not be the Root Port's bus
number + 1.  It *might* be, and since you said the current design only
has a single Root Port, it probably *will* be, but that secondary bus
number is writable and can be changed either by the PCI core or by the
user via setpci.  So you shouldn't assume this.  If/when a design
supports more than one Root Port, that assumption will certainly be
broken.

> +		desc0 |= CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_CONF_TYPE0;
> +	else
> +		desc0 |= CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_CONF_TYPE1;
> +
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_DESC0(0), desc0);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_DESC1(0), desc1);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_CTRL0(0), ctrl0);
> +
> +	return rc->cfg_base + (where & 0xfff);
> +}

> +++ b/drivers/pci/controller/cadence/pcie-cadence.c
> @@ -5,9 +5,49 @@
>  
>  #include <linux/kernel.h>
>  #include <linux/of.h>
> -

Spurious change, keep this blank line.

>  #include "pcie-cadence.h"
>  
> +bool cdns_pcie_linkup(struct cdns_pcie *pcie)

Static unless needed elsewhere.  I can't tell whether it is because I
can't download or apply the whole series.

> +{
> +	u32 pl_reg_val;
> +
> +	pl_reg_val = cdns_pcie_readl(pcie, CDNS_PCIE_LM_BASE);
> +	if (pl_reg_val & GENMASK(0, 0))
> +		return true;
> +	else
> +		return false;

Drop the else:

  if (pl_reg_val & GENMASK(0, 0))
    return true;

  return false;

> +}
> +
> +bool cdns_pcie_hpa_linkup(struct cdns_pcie *pcie)
> +{
> +	u32 pl_reg_val;
> +
> +	pl_reg_val = cdns_pcie_hpa_readl(pcie, REG_BANK_IP_REG, CDNS_PCIE_HPA_PHY_DBG_STS_REG0);
> +	if (pl_reg_val & GENMASK(0, 0))
> +		return true;
> +	else
> +		return false;

Ditto.

> +}
> +
> +int cdns_pcie_hpa_startlink(struct cdns_pcie *pcie)

s/cdns_pcie_hpa_startlink/cdns_pcie_hpa_start_link/

> +{
> +	u32 pl_reg_val;
> +
> +	pl_reg_val = cdns_pcie_hpa_readl(pcie, REG_BANK_IP_REG, CDNS_PCIE_HPA_PHY_LAYER_CFG0);
> +	pl_reg_val |= CDNS_PCIE_HPA_LINK_TRNG_EN_MASK;
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_IP_REG, CDNS_PCIE_HPA_PHY_LAYER_CFG0, pl_reg_val);
> +	return 1;

This should return 0 for success.

> +}

> +void cdns_pcie_hpa_set_outbound_region(struct cdns_pcie *pcie, u8 busnr, u8 fn,
> +				       u32 r, bool is_io,
> +				       u64 cpu_addr, u64 pci_addr, size_t size)
> +{
> +	/*
> +	 * roundup_pow_of_two() returns an unsigned long, which is not suited
> +	 * for 64bit values.
> +	 */
> +	u64 sz = 1ULL << fls64(size - 1);
> +	int nbits = ilog2(sz);
> +	u32 addr0, addr1, desc0, desc1, ctrl0;
> +
> +	if (nbits < 8)
> +		nbits = 8;
> +
> +	/*
> +	 * Set the PCI address
> +	 */

Could be a single line comment:

  /* Set the PCI address */

like many others in this series.

> +	addr0 = CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0_NBITS(nbits) |
> +		(lower_32_bits(pci_addr) & GENMASK(31, 8));
> +	addr1 = upper_32_bits(pci_addr);
> +
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR0(r), addr0);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_PCI_ADDR1(r), addr1);
> +
> +	/*
> +	 * Set the PCIe header descriptor
> +	 */
> +	if (is_io)
> +		desc0 = CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_IO;
> +	else
> +		desc0 = CDNS_PCIE_HPA_AT_OB_REGION_DESC0_TYPE_MEM;
> +	desc1 = 0;
> +
> +	/*
> +	 * Whatever Bit [26] is set or not inside DESC0 register of the outbound
> +	 * PCIe descriptor, the PCI function number must be set into
> +	 * Bits [31:24] of DESC1 anyway.

s/Whatever/Whether/ (I think)

> +	 * In Root Complex mode, the function number is always 0 but in Endpoint
> +	 * mode, the PCIe controller may support more than one function. This
> +	 * function number needs to be set properly into the outbound PCIe
> +	 * descriptor.
> +	 *
> +	 * Besides, setting Bit [26] is mandatory when in Root Complex mode:
> +	 * then the driver must provide the bus, resp. device, number in
> +	 * Bits [31:24] of DESC1, resp. Bits[23:16] of DESC0. Like the function
> +	 * number, the device number is always 0 in Root Complex mode.
> +	 *
> +	 * However when in Endpoint mode, we can clear Bit [26] of DESC0, hence
> +	 * the PCIe controller will use the captured values for the bus and
> +	 * device numbers.
> +	 */
> +	if (pcie->is_rc) {
> +		/* The device and function numbers are always 0. */
> +		desc1 = CDNS_PCIE_HPA_AT_OB_REGION_DESC1_BUS(busnr) |
> +			CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN(0);
> +		ctrl0 = CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_BUS |
> +			CDNS_PCIE_HPA_AT_OB_REGION_CTRL0_SUPPLY_DEV_FN;
> +	} else {
> +		/*
> +		 * Use captured values for bus and device numbers but still
> +		 * need to set the function number.
> +		 */
> +		desc1 |= CDNS_PCIE_HPA_AT_OB_REGION_DESC1_DEVFN(fn);
> +	}
> +
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_DESC0(r), desc0);
> +	cdns_pcie_hpa_writel(pcie, REG_BANK_AXI_SLAVE,
> +			     CDNS_PCIE_HPA_AT_OB_REGION_DESC1(r), desc1);
> +
> +	/*
> +	 * Set the CPU address
> +	 */
> +	if (pcie->ops->cpu_addr_fixup)
> +		cpu_addr = pcie->ops->cpu_addr_fixup(pcie, cpu_addr);

Oops, we can't add any more .cpu_addr_fixup() functions or uses.  This
must be done via the devicetree description.  If we add a new
.cpu_addr_fixup(), it may cover up defects in the devicetree.

You can see Frank Li's nice work to fix this for some of the dwc
drivers on the branch ending at 07ae413e169d ("PCI: intel-gw: Remove
intel_pcie_cpu_addr()"):

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/?h=07ae413e169d

This one is the biggest issue so far.

Bjorn