>[+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.
Ok
>
>> @@ -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.
After going through the code, the check requires to be outside.
>
>> +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.,
>
Ok
> /* 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.
>
Ok
>> + * 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).
>
Removed the comment in here and in existing code for the next patch series
>> + */
>> + 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.
>
Ok
>> #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.
Used in other file.
>
>> +{
>> + 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.
OK
>
>> +}
>> +
>> +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.
>
>> +}
>
Ok
>> +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)
>
Ok
>> + * 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.
>
cpu_addr_fixup is removed.
>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://urldefense.com/v3/__https://git.kernel.org/pub/scm/linux/kernel/git/t
>orvalds/linux.git/log/?h=07ae413e169d__;!!EHscmS1ygiU1lA!HirY7Jqq13s65vE
>sm8Xtx9gMxZHrQDFP83kcJl16q69IqZNwZ8uRfTlSISvAIeG6vWCI2sIr8sP4N64$
>
>This one is the biggest issue so far.
>
>Bjorn
