Re: 回覆: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

[Bjorn Helgaas <helgaas@kernel.org>]

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