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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SEYPR06MB513422076CE2B82FAB2EBBA19D38A@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Wed, 27 Aug 2025 03:35:40 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: "bhelgaas@...gle.com" <bhelgaas@...gle.com>, "lpieralisi@...nel.org"
	<lpieralisi@...nel.org>, "kwilczynski@...nel.org" <kwilczynski@...nel.org>,
	"mani@...nel.org" <mani@...nel.org>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>, "joel@....id.au" <joel@....id.au>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "openbmc@...ts.ozlabs.org"
	<openbmc@...ts.ozlabs.org>, "linux-gpio@...r.kernel.org"
	<linux-gpio@...r.kernel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, "p.zabel@...gutronix.de"
	<p.zabel@...gutronix.de>, BMC-SW <BMC-SW@...eedtech.com>
Subject: [PATCH v2 09/10] PCI: aspeed: Add ASPEED PCIe RC driver

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