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

[Jacky Chou]

> > > > +#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