diff mbox series

[RFC,3/9] PCI/portdrv: create platform devices for child OF nodes

Message ID 20240104130123.37115-4-brgl@bgdev.pl (mailing list archive)
State New, archived
Headers show
Series PCI: introduce the concept of power sequencing of PCIe devices | expand

Commit Message

Bartosz Golaszewski Jan. 4, 2024, 1:01 p.m. UTC 
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

In order to introduce PCIe power-sequencing, we need to create platform
devices for child nodes of the port driver node. They will get matched
against the pwrseq drivers (if one exists) and then the actuak PCIe
device will reuse the node once it's detected on the bus.

Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
---
 drivers/pci/pcie/portdrv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Jeff Johnson Jan. 6, 2024, 1:05 a.m. UTC | #1 
On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> In order to introduce PCIe power-sequencing, we need to create platform
> devices for child nodes of the port driver node. They will get matched
> against the pwrseq drivers (if one exists) and then the actuak PCIe

nit if you re-spin: s/actuak /actual /

> device will reuse the node once it's detected on the bus.
> 
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/pcie/portdrv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
> index 14a4b89a3b83..401fb731009d 100644
> --- a/drivers/pci/pcie/portdrv.c
> +++ b/drivers/pci/pcie/portdrv.c
> @@ -13,6 +13,7 @@
>  #include <linux/pci.h>
>  #include <linux/kernel.h>
>  #include <linux/errno.h>
> +#include <linux/of_platform.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/string.h>
> @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
>  		pm_runtime_allow(&dev->dev);
>  	}
>  
> -	return 0;
> +	return devm_of_platform_populate(&dev->dev);
>  }
>  
>  static void pcie_portdrv_remove(struct pci_dev *dev)
Bartosz Golaszewski Jan. 10, 2024, 12:55 p.m. UTC | #2 
On Tue, Jan 9, 2024 at 3:43 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > In order to introduce PCIe power-sequencing, we need to create platform
> > devices for child nodes of the port driver node. They will get matched
> > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > device will reuse the node once it's detected on the bus.
> [...]
> > --- a/drivers/pci/pcie/portdrv.c
> > +++ b/drivers/pci/pcie/portdrv.c
> > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> >               pm_runtime_allow(&dev->dev);
> >       }
> >
> > -     return 0;
> > +     return devm_of_platform_populate(&dev->dev);
> >  }
>
> I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> the wrong place.  Note that you're currently calling this for RCECs
> (Root Complex Event Collectors) as well, which is likely not what
> you want.
>

of_pci_make_dev_node() is only called when the relevant PCI device is
instantiated which doesn't happen until it's powered-up and scanned -
precisely the problem I'm trying to address.

Calling this for whomever isn't really a problem though, is it? We
will create a platform device alright - if it's defined on the DT -
and at worst it won't match against any driver. It seems harmless IMO.

> devm functions can't be used in the PCI core, so symmetrically call
> of_platform_unpopulate() from of_pci_remove_node().

I don't doubt what you're saying is true (I've seen worse things) but
this is the probe() callback of a driver using the driver model. Why
wouldn't devres work?

Bart

>
> Thanks,
>
> Lukas
>
Bartosz Golaszewski Jan. 10, 2024, 4:26 p.m. UTC | #3 
On Wed, Jan 10, 2024 at 2:28 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > devices for child nodes of the port driver node. They will get matched
> > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > device will reuse the node once it's detected on the bus.
> > > [...]
> > > > --- a/drivers/pci/pcie/portdrv.c
> > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > >               pm_runtime_allow(&dev->dev);
> > > >       }
> > > >
> > > > -     return 0;
> > > > +     return devm_of_platform_populate(&dev->dev);
> > > >  }
> > >
> > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > the wrong place.  Note that you're currently calling this for RCECs
> > > (Root Complex Event Collectors) as well, which is likely not what
> > > you want.
> > >
> >
> > of_pci_make_dev_node() is only called when the relevant PCI device is
> > instantiated which doesn't happen until it's powered-up and scanned -
> > precisely the problem I'm trying to address.
>
> No, of_pci_make_dev_node() is called *before* device_attach(),
> i.e. before portdrv has even probed.  So it seems this should
> work perfectly well for your use case.
>

Seems like the following must be true but isn't in my case (from
pci_bus_add_device()):

    if (pci_is_bridge(dev))
        of_pci_make_dev_node(dev);

Shouldn't it evaluate to true for ports?

Bartosz

[snip]
Bartosz Golaszewski Jan. 10, 2024, 8:18 p.m. UTC | #4 
On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
>> Seems like the following must be true but isn't in my case (from
>> pci_bus_add_device()):
>>
>>     if (pci_is_bridge(dev))
>>         of_pci_make_dev_node(dev);
>>
>> Shouldn't it evaluate to true for ports?
>
> It should.
>
> What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
>

I cut out the hexdump part, let me know if you really need it. Output follows.

Bart

--

# lspci -vvvvxxxx -s  0000:00:00.0
0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
(prog-if 00 [Normal decode])
	Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR+ FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 176
	IOMMU group: 8
	Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K]
	Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
	I/O behind bridge: f000-0fff [disabled] [16-bit]
	Memory behind bridge: 60400000-604fffff [size=1M] [32-bit]
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
[disabled] [64-bit]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000a1c3f000  Data: 0000
		Masking: fffffffe  Pending: 00000000
	Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency
L0s <1us, L1 <64us
			ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 128 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 5GT/s, Width x1
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+
		SltCap:	AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+
			Slot #0, PowerLimit 0W; Interlock+ NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Off, PwrInd Off, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCap: CRSVisible+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+
10BitTagReq- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS-
		LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
			 Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-
EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [100 v2] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+
MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
		RootCmd: CERptEn+ NFERptEn+ FERptEn+
		RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
			 FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0
		ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000
	Capabilities: [148 v1] Secondary PCI Express
		LnkCtl3: LnkEquIntrruptEn- PerformEqu-
		LaneErrStat: 0
	Capabilities: [168 v1] Transaction Processing Hints
		No steering table available
	Capabilities: [1fc v1] L1 PM Substates
		L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
			  PortCommonModeRestoreTime=70us PortTPowerOnTime=0us
		L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
			   T_CommonMode=70us LTR1.2_Threshold=76800ns
		L1SubCtl2: T_PwrOn=0us
	Kernel driver in use: pcieport
Dan Williams Jan. 10, 2024, 8:41 p.m. UTC | #5 
[ add Terry ]


Lukas Wunner wrote:
> On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > devices for child nodes of the port driver node. They will get matched
> > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > device will reuse the node once it's detected on the bus.
> > > [...]
> > > > --- a/drivers/pci/pcie/portdrv.c
> > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > >               pm_runtime_allow(&dev->dev);
> > > >       }
> > > >
> > > > -     return 0;
> > > > +     return devm_of_platform_populate(&dev->dev);
> > > >  }
> > >
> > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > the wrong place.  Note that you're currently calling this for RCECs
> > > (Root Complex Event Collectors) as well, which is likely not what
> > > you want.
> > >
> > 
> > of_pci_make_dev_node() is only called when the relevant PCI device is
> > instantiated which doesn't happen until it's powered-up and scanned -
> > precisely the problem I'm trying to address.
> 
> No, of_pci_make_dev_node() is called *before* device_attach(),
> i.e. before portdrv has even probed.  So it seems this should
> work perfectly well for your use case.
> 
> 
> > > devm functions can't be used in the PCI core, so symmetrically call
> > > of_platform_unpopulate() from of_pci_remove_node().
> > 
> > I don't doubt what you're saying is true (I've seen worse things) but
> > this is the probe() callback of a driver using the driver model. Why
> > wouldn't devres work?
> 
> The long term plan is to move the functionality in portdrv to
> the PCI core.  Because devm functions can't be used in the PCI
> core, adding new ones to portdrv will *add* a new roadblock to
> migrating portdrv to the PCI core.  In other words, it makes
> future maintenance more difficult.
> 
> Generally, only PCIe port services which share the same interrupt
> (hotplug, PME, bandwith notification, flit error counter, ...)
> need to live in portdrv.  Arbitrary other stuff should not be
> shoehorned into portdrv.

I came here to say the same thing. It is already the case that portdrv
is not a good model to build new functionality upon [1], and PCI core
enlightenment should be considered first.

The portdrv model is impeding Terry's CXL Port error handling effort, so
I am on the lookout for portdrv growing new entanglements to unwind
later.

[1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas
Bartosz Golaszewski Jan. 11, 2024, 11:09 a.m. UTC | #6 
On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said:
> On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
>> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
>> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
>> > > Seems like the following must be true but isn't in my case (from
>> > > pci_bus_add_device()):
>> > >
>> > >     if (pci_is_bridge(dev))
>> > >         of_pci_make_dev_node(dev);
>> > >
>> > > Shouldn't it evaluate to true for ports?
>> >
>> > It should.
>> >
>> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
>>
>> I cut out the hexdump part, let me know if you really need it.
>
> I really need it.
>

Ok, one more:

# lspci -vvvvxxxx -s 0000:00:00
0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
(prog-if 00 [Normal decode])
	Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
	Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping- SERR+ FastB2B- DisINTx+
	Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
	Latency: 0
	Interrupt: pin A routed to IRQ 188
	IOMMU group: 7
	Region 0: Memory at 60300000 (32-bit, non-prefetchable) [size=4K]
	Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
	I/O behind bridge: f000-0fff [disabled] [16-bit]
	Memory behind bridge: 60400000-604fffff [size=1M] [32-bit]
	Prefetchable memory behind bridge: 00000000fff00000-00000000000fffff
[disabled] [64-bit]
	Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
	BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset- FastB2B-
		PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
	Capabilities: [40] Power Management version 3
		Flags: PMEClk- DSI- D1- D2- AuxCurrent=0mA PME(D0+,D1-,D2-,D3hot+,D3cold+)
		Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
	Capabilities: [50] MSI: Enable+ Count=1/32 Maskable+ 64bit+
		Address: 00000000a1c00000  Data: 0000
		Masking: fffffffe  Pending: 00000000
	Capabilities: [70] Express (v2) Root Port (Slot+), MSI 00
		DevCap:	MaxPayload 128 bytes, PhantFunc 0
			ExtTag- RBE+
		DevCtl:	CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
			RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
			MaxPayload 128 bytes, MaxReadReq 512 bytes
		DevSta:	CorrErr- NonFatalErr- FatalErr- UnsupReq- AuxPwr+ TransPend-
		LnkCap:	Port #0, Speed 8GT/s, Width x1, ASPM L0s L1, Exit Latency
L0s <1us, L1 <64us
			ClockPM- Surprise+ LLActRep+ BwNot+ ASPMOptComp+
		LnkCtl:	ASPM Disabled; RCB 128 bytes, Disabled- CommClk+
			ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt-
		LnkSta:	Speed 5GT/s, Width x1
			TrErr- Train- SlotClk+ DLActive+ BWMgmt+ ABWMgmt+
		SltCap:	AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug- Surprise+
			Slot #0, PowerLimit 0W; Interlock+ NoCompl-
		SltCtl:	Enable: AttnBtn- PwrFlt- MRL- PresDet- CmdCplt- HPIrq- LinkChg-
			Control: AttnInd Off, PwrInd Off, Power- Interlock-
		SltSta:	Status: AttnBtn- PowerFlt- MRL- CmdCplt- PresDet- Interlock-
			Changed: MRL- PresDet- LinkState-
		RootCap: CRSVisible+
		RootCtl: ErrCorrectable- ErrNon-Fatal- ErrFatal- PMEIntEna+ CRSVisible+
		RootSta: PME ReqID 0000, PMEStatus- PMEPending-
		DevCap2: Completion Timeout: Range ABCD, TimeoutDis+ NROPrPrP+ LTR+
			 10BitTagComp- 10BitTagReq- OBFF Not Supported, ExtFmt- EETLPPrefix-
			 EmergencyPowerReduction Not Supported, EmergencyPowerReductionInit-
			 FRS- LN System CLS Not Supported, TPHComp+ ExtTPHComp- ARIFwd-
			 AtomicOpsCap: Routing- 32bit- 64bit- 128bitCAS-
		DevCtl2: Completion Timeout: 50us to 50ms, TimeoutDis- LTR+
10BitTagReq- OBFF Disabled, ARIFwd-
			 AtomicOpsCtl: ReqEn- EgressBlck-
		LnkCap2: Supported Link Speeds: 2.5-8GT/s, Crosslink- Retimer- 2Retimers- DRS-
		LnkCtl2: Target Link Speed: 8GT/s, EnterCompliance- SpeedDis-
			 Transmit Margin: Normal Operating Range, EnterModifiedCompliance-
ComplianceSOS-
			 Compliance Preset/De-emphasis: -6dB de-emphasis, 0dB preshoot
		LnkSta2: Current De-emphasis Level: -6dB, EqualizationComplete-
EqualizationPhase1-
			 EqualizationPhase2- EqualizationPhase3- LinkEqualizationRequest-
			 Retimer- 2Retimers- CrosslinkRes: unsupported
	Capabilities: [100 v2] Advanced Error Reporting
		UESta:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UEMsk:	DLP- SDES- TLP- FCP- CmpltTO- CmpltAbrt- UnxCmplt- RxOF-
MalfTLP- ECRC- UnsupReq- ACSViol-
		UESvrt:	DLP+ SDES+ TLP- FCP+ CmpltTO- CmpltAbrt- UnxCmplt- RxOF+
MalfTLP+ ECRC- UnsupReq- ACSViol-
		CESta:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr-
		CEMsk:	RxErr- BadTLP- BadDLLP- Rollover- Timeout- AdvNonFatalErr+
		AERCap:	First Error Pointer: 00, ECRCGenCap+ ECRCGenEn- ECRCChkCap+ ECRCChkEn-
			MultHdrRecCap- MultHdrRecEn- TLPPfxPres- HdrLogCap-
		HeaderLog: 00000000 00000000 00000000 00000000
		RootCmd: CERptEn+ NFERptEn+ FERptEn+
		RootSta: CERcvd- MultCERcvd- UERcvd- MultUERcvd-
			 FirstFatal- NonFatalMsg- FatalMsg- IntMsg 0
		ErrorSrc: ERR_COR: 0000 ERR_FATAL/NONFATAL: 0000
	Capabilities: [148 v1] Secondary PCI Express
		LnkCtl3: LnkEquIntrruptEn- PerformEqu-
		LaneErrStat: 0
	Capabilities: [168 v1] Transaction Processing Hints
		No steering table available
	Capabilities: [1fc v1] L1 PM Substates
		L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ L1_PM_Substates+
			  PortCommonModeRestoreTime=70us PortTPowerOnTime=0us
		L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
			   T_CommonMode=70us LTR1.2_Threshold=76800ns
		L1SubCtl2: T_PwrOn=0us
	Kernel driver in use: pcieport
00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00
10: 00 00 30 60 00 00 00 00 00 01 ff 00 f0 00 00 00
20: f0 ff 00 00 f1 ff 01 00 00 00 00 00 00 00 00 00
30: 00 00 00 00 40 00 00 00 00 00 00 00 bb 01 02 00
40: 01 50 03 c8 08 00 00 00 00 00 00 00 00 00 00 00
50: 05 70 8b 01 00 00 c0 a1 00 00 00 00 00 00 00 00
60: fe ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00
70: 10 00 42 01 00 80 00 00 1f 28 10 00 13 4c 7b 00
80: 48 00 12 f0 3f 00 02 00 c0 03 00 00 18 00 01 00
90: 00 00 00 00 1f 1c 00 00 00 04 00 00 0e 00 00 00
a0: 03 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
100: 01 00 82 14 00 00 00 00 00 00 40 00 30 20 46 00
110: 00 00 00 00 00 e0 00 00 a0 00 00 00 00 00 00 00
120: 00 00 00 00 00 00 00 00 00 00 00 00 07 00 00 00
130: 00 00 00 000 00 00 00 00 00 00 00 00 00 00 00
140: 00 00 00 00 00 00 00 00 19 00 81 16 00 00 00 00
150: 00 00 00 00 7f 6f 00 00 00 00 00 00 00 00 00 00
160: 00 00 00 00 00 00 00 00 17 00 c1 1f 01 00 00 00
170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
1f0: 00 00 00 00 18 00 c1 1f 00 00 00 00 1e 00 01 00
200: 1f 46 00 00 00 46 4b 40 00 00 00 00 00 00 00 00
210: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
220: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
230: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
240: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
250: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
260: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
270: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
280: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
290: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
2f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
300: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
310: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
320: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
330: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
340: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
350: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
360: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
370: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
380: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
390: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
3f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
400: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
410: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
420: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
430: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
440: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
450: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
460: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
470: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
480: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
490: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
4f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
500: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
520: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
530: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
540: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
550: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
560: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
570: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
5f0: 00 ��0 00 00 00 00 00 00 00 00 00 00 00 00 00
600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
620: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
630: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
640: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
650: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
660: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
670: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
680: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
690: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
6f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
700: 3b 00 b1 00 ff ff ff ff 04 00 80 00 00 90 90 1b
710: 20 01 01 00 00 00 00 00 00 40 01 00 40 01 00 00
720: 00 00 00 00 01 00 00 00 11 74 40 03 10 00 00 08
730: 60 00 01 00 07 a0 01 00 ff ff 0f 00 00 00 00 00
740: 0f 00 00 00 00 00 00 00 60 00 21 45 07 a0 21 05
750: 00 00 20 05 00 00 00 00 00 00 00 00 00 00 00 00
760: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
770: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
7f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
800: 00 00 00 00 00 00 00 00 00 00 00 00 90 01 00 00
810: 00 00 00 00 00 00 00 00 00 00 00 00 7f 00 00 00
820: 00 00 c0 a1 00 00 00 00 ff ff ff ff fe ff ff ff
830: 00 00 00 00 ff ff ff ff 00 3c 02 fe 00 00 00 00
840: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff
850: ff ff ff ff 00 00 00 00 ff ff ff ff ff ff ff ff
860: 00 00 00 00 ff ff ff ff ff ff ff ff 00 00 00 00
870: ff ff ff ff ff ff ff ff 00 00 00 00 ff ff ff ff
880: ff ff ff ff 00 00 00 00 00 00 00 00 01 00 00 00
890: 01 20 00 00 00 00 00 00 00 00 00 00 00 00 00 00
8a0: 00 00 00 00 00 00 00 00 60 00 00 04 a0 55 01 00
8b0: 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00
8c0: 00 00 00 00 44 00 00 00 00 00 00 00 01 00 00 ff
8d0: 00 9c 00 00 32 00 00 00 00 00 00 00 00 00 00 00
8e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
8f0: 00 00 00 00 00 00 00 00 2a 30 30 35 39 32 61 65
900: ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00
910: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
920: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
930: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
940: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
950: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
960: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
970: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
980: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
990: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9a0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9b0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9c0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9d0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9e0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
9f0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
a90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
aa0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ab0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ac0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ad0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ae0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
af0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
b40: 14 00 00 00 d2 00 00 00 00 00 00 00 00 00 00 00
b50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
b90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ba0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
be0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
bf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
c90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ca0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ce0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
cf0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
d90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
da0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
db0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
dc0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
dd0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
de0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
df0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e20: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e30: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e40: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e50: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e60: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e70: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e80: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
e90: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ea0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
eb0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ec0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ed0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ee0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
ef0: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f00: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f10: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff
f20: 90 01 00 00 00 00 00 00 01 14 01 00 00 00 00 00
f30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00� 00
f70: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
f90: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fa0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fb0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fc0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fd0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
fe0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
ff0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00

Bartosz
Manivannan Sadhasivam Jan. 11, 2024, 12:40 p.m. UTC | #7 
On Wed, Jan 10, 2024 at 12:41:17PM -0800, Dan Williams wrote:
> [ add Terry ]
> 
> 
> Lukas Wunner wrote:
> > On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> > > On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > > On Thu, Jan 04, 2024 at 02:01:17PM +0100, Bartosz Golaszewski wrote:
> > > > > In order to introduce PCIe power-sequencing, we need to create platform
> > > > > devices for child nodes of the port driver node. They will get matched
> > > > > against the pwrseq drivers (if one exists) and then the actuak PCIe
> > > > > device will reuse the node once it's detected on the bus.
> > > > [...]
> > > > > --- a/drivers/pci/pcie/portdrv.c
> > > > > +++ b/drivers/pci/pcie/portdrv.c
> > > > > @@ -715,7 +716,7 @@ static int pcie_portdrv_probe(struct pci_dev *dev,
> > > > >               pm_runtime_allow(&dev->dev);
> > > > >       }
> > > > >
> > > > > -     return 0;
> > > > > +     return devm_of_platform_populate(&dev->dev);
> > > > >  }
> > > >
> > > > I think this belongs in of_pci_make_dev_node(), portdrv seems totally
> > > > the wrong place.  Note that you're currently calling this for RCECs
> > > > (Root Complex Event Collectors) as well, which is likely not what
> > > > you want.
> > > >
> > > 
> > > of_pci_make_dev_node() is only called when the relevant PCI device is
> > > instantiated which doesn't happen until it's powered-up and scanned -
> > > precisely the problem I'm trying to address.
> > 
> > No, of_pci_make_dev_node() is called *before* device_attach(),
> > i.e. before portdrv has even probed.  So it seems this should
> > work perfectly well for your use case.
> > 
> > 
> > > > devm functions can't be used in the PCI core, so symmetrically call
> > > > of_platform_unpopulate() from of_pci_remove_node().
> > > 
> > > I don't doubt what you're saying is true (I've seen worse things) but
> > > this is the probe() callback of a driver using the driver model. Why
> > > wouldn't devres work?
> > 
> > The long term plan is to move the functionality in portdrv to
> > the PCI core.  Because devm functions can't be used in the PCI
> > core, adding new ones to portdrv will *add* a new roadblock to
> > migrating portdrv to the PCI core.  In other words, it makes
> > future maintenance more difficult.
> > 
> > Generally, only PCIe port services which share the same interrupt
> > (hotplug, PME, bandwith notification, flit error counter, ...)
> > need to live in portdrv.  Arbitrary other stuff should not be
> > shoehorned into portdrv.
> 
> I came here to say the same thing. It is already the case that portdrv
> is not a good model to build new functionality upon [1], and PCI core
> enlightenment should be considered first.
> 

The primary reason for plugging the power sequencing into portdrv is due to
portdrv binding with all the bridge devices and acting as management driver for
the bridges. This is where exactly the power sequencing part needs to be plugged
in IMO. But if the idea of the portdrv is just to expose services based on
interrupts, then please suggest a better place to plug this power sequencing
part.

- Mani

> The portdrv model is impeding Terry's CXL Port error handling effort, so
> I am on the lookout for portdrv growing new entanglements to unwind
> later.
> 
> [1]: http://lore.kernel.org/r/20221025232535.GA579167@bhelgaas
>
Bartosz Golaszewski Jan. 11, 2024, 4:16 p.m. UTC | #8 
On Thu, Jan 11, 2024 at 4:02 PM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote:
> > On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <lukas@wunner.de> said:
> > > On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
> > >> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <lukas@wunner.de> said:
> > >> > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > >> > > Seems like the following must be true but isn't in my case (from
> > >> > > pci_bus_add_device()):
> > >> > >
> > >> > >     if (pci_is_bridge(dev))
> > >> > >         of_pci_make_dev_node(dev);
> > >> > >
> > >> > > Shouldn't it evaluate to true for ports?
> > >> >
> > >> > It should.
> > >> >
> > >> > What does "lspci -vvvvxxxx -s BB:DD.F" say for the port in question?
> >
> > # lspci -vvvvxxxx -s 0000:00:00
> > 0000:00:00.0 PCI bridge: Qualcomm Technologies, Inc Device 010b
> > (prog-if 00 [Normal decode])
> >       Device tree node: /sys/firmware/devicetree/base/soc@0/pcie@1c00000/pcie@0
> [...]
> > 00: cb 17 0b 01 07 05 10 00 00 00 04 06 00 00 01 00
>                                                 ^^
> The Header Type in config space is 0x1, i.e. PCI_HEADER_TYPE_BRIDGE.
>
> So pci_is_bridge(dev) does return true (unlike what you write above)
> and control flow enters of_pci_make_dev_node().
>
> But perhaps of_pci_make_dev_node() returns immediately because:
>

No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
which requires OF_UNITTEST (!).

We definitely don't need to enable dynamic OF nodes. We don't want to
modify the DT, we want to create devices for existing nodes. Also:
with the approach in this RFC we maintain a clear hierarchy of devices
with the port device being the parent of the power sequencing device
which becomes the parent of the actual PCIe device (the port stays the
parent of this device too).

Bartosz

>         /*
>          * If there is already a device tree node linked to this device,
>          * return immediately.
>          */
>         if (pci_device_to_OF_node(pdev))
>                 return;
>
> ...and lspci does list a devicetree node for that Root Port.
>
> In any case, of_pci_make_dev_node() is the right place to add
> the call to of_platform_populate().  Just make sure it's called
> even if there is already a DT node for the Root Port itself.
>
> Thanks,
>
> Lukas
Geert Uytterhoeven Jan. 11, 2024, 9:43 p.m. UTC | #9 
Hi Bartosz,

On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> which requires OF_UNITTEST (!).

Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Bartosz Golaszewski Jan. 12, 2024, 9:43 a.m. UTC | #10 
On Thu, Jan 11, 2024 at 10:44 PM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
>
> Hi Bartosz,
>
> On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> > which requires OF_UNITTEST (!).
>
> Huh? Config PCI_DYNAMIC_OF_NODES does select OF_DYNAMIC.
>

Indeed, I got something wrong.

But in any case: we *don't* need dynamic OF nodes as we don't create
new ones. We use the ones that already exist. This is logically a
wrong place to add this.

Lukas, Terry: am I getting this right - is the port driver supposed to
go away at some point? Because I'm not sure I understand what the
problem is here. To me it seems that when we create a real device for
the PCIe port, then it's only normal to populate its child devices
from the port driver.

Bartosz

> Gr{oetje,eeting}s,
>
>                         Geert
>
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
>
Rob Herring Jan. 17, 2024, 11:38 p.m. UTC | #11 
On Fri, Jan 12, 2024 at 3:43 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote:
> > On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <lukas@wunner.de> wrote:
> > > On Wed, Jan 10, 2024 at 05:26:52PM +0100, Bartosz Golaszewski wrote:
> > > >     if (pci_is_bridge(dev))
> > > >         of_pci_make_dev_node(dev);
> > >
> > > But perhaps of_pci_make_dev_node() returns immediately because:
> >
> > No, it was actually a no-op due to CONFIG_PCI_DYNAMIC_OF_NODES not
> > being set. But this is only available if CONFIG_OF_DYNAMIC is enabled
> > which requires OF_UNITTEST (!).
> >
> > We definitely don't need to enable dynamic OF nodes. We don't want to
> > modify the DT, we want to create devices for existing nodes.
>
> Consider refactoring of_pci_make_dev_node() to suit your needs or
> add a separate function call inside the "if (pci_is_bridge(dev))"
> clause which populates the child OF nodes.

The latter because of_pci_make_dev_node() has absolutely nothing to do
with the issue this series solves. The uses are pretty much mutually
exclusive. If we have a DT node with power related properties, there
is no need to create that node because it already exists.

Rob
diff mbox series

Patch

diff --git a/drivers/pci/pcie/portdrv.c b/drivers/pci/pcie/portdrv.c
index 14a4b89a3b83..401fb731009d 100644
--- a/drivers/pci/pcie/portdrv.c
+++ b/drivers/pci/pcie/portdrv.c
@@ -13,6 +13,7 @@ 
 #include <linux/pci.h>
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/of_platform.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/string.h>
@@ -715,7 +716,7 @@  static int pcie_portdrv_probe(struct pci_dev *dev,
 		pm_runtime_allow(&dev->dev);
 	}
 
-	return 0;
+	return devm_of_platform_populate(&dev->dev);
 }
 
 static void pcie_portdrv_remove(struct pci_dev *dev)