On Fri, Aug 22, 2025 at 07:00:25AM +0000, Jacky Chou wrote:

quoted

v1 posting was
https://lore.kernel.org/r/20250613033001.3153637-1-jacky_chou@aspeedtech
.com
Links to previous postings are helpful in the cover letter.

On Tue, Jul 15, 2025 at 11:43:19AM +0800, Jacky Chou wrote:

quoted

Introduce PCIe Root Complex driver for ASPEED SoCs. Support RC
initialization, reset, clock, IRQ domain, and MSI domain setup.
Implement platform-specific setup and register configuration for
ASPEED. And provide PCI config space read/write and INTx/MSI interrupt
handling.

quoted

quoted

+#define MAX_MSI_HOST_IRQS	64
+#define PCIE_RESET_CONFIG_DEVICE_WAIT_MS	500

Where does this value come from?  Is there a generic value from
drivers/pci/pci.h you can use?

We check the PCIe specification to find these contents.

"With a Downstream Port that supports Link speeds greater than 5.0
GT/s, software must wait a minimum of 100 ms after Link training
completes before sending a Configuration Request to the device
immediately below that Port."

So, we think delay 500ms to let kernel issue the first configuration
command is enough after deassert PERST.

Isn't this PCIE_RESET_CONFIG_WAIT_MS?

I prefer to use #defines from the PCI core whenever possible because
it makes it easier to ensure that all drivers include the required
delays.

quoted

quoted

+#define PCIE_RESET_CONFIG_RC_WAIT_MS		10

Ditto.  If it's an Aspeed-specific value, can you point to the
source in the Aspeed datasheet?

This delay is set to ensure that the RC internal settings are
completely reset.  We do not put its usage in our datasheet.

The "PCIE_" prefix suggests something required by the PCIe base spec.
If this is an Aspeed-specific value, perhaps remove the "PCIE_"
prefix?

quoted

quoted

+static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int

devfn,

quoted

+				       int where, int size, u32 *val,
+				       bool write)
+{
+	struct aspeed_pcie *pcie = bus->sysdata;
+	u32 bdf_offset, status, cfg_val;
+	int ret;
+
+	bdf_offset = aspeed_pcie_get_bdf_offset(bus, devfn, where);
+
+	cfg_val = CRG_PAYLOAD_SIZE;
+	if (write)
+		cfg_val |= (bus->number == 1) ? CRG0_WRITE_FMTTYPE :

CRG1_WRITE_FMTTYPE;

quoted

+	else
+		cfg_val |= (bus->number == 1) ? CRG0_READ_FMTTYPE :
+CRG1_READ_FMTTYPE;

I don't think you should assume that bus 0 is the root bus.  The root bus
number should come from the DT bus-range.

Just making sure you saw this part since you didn't mention it.

Bjorn