> > > > +#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.
>
Thank you for pointing this define.
I change to this define in next version.
> > > > +#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?
>
Agreed.
> > > > +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.
>
Agreed.
I have checked our design and verified in different root bus number.
I will modify this part in next version.
Thanks,
Jacky
