On Fri, Aug 22, 2025 at 07:00:25AM +0000, Jacky Chou wrote: > > 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: > > > 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. > > > +#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. > > > +#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? > > > +static int aspeed_ast2700_child_config(struct pci_bus *bus, unsigned int > > devfn, > > > + 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; > > > + 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
