diff mbox series

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

Message ID 20240104130123.37115-4-brgl@bgdev.pl
State New
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)
Lukas Wunner Jan. 9, 2024, 2:43 p.m. UTC | #2 
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.

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

Thanks,

Lukas
Bartosz Golaszewski Jan. 10, 2024, 12:55 p.m. UTC | #3 
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
>
Lukas Wunner Jan. 10, 2024, 1:28 p.m. UTC | #4 
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.

Thanks,

Lukas
Bartosz Golaszewski Jan. 10, 2024, 4:26 p.m. UTC | #5 
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]
Lukas Wunner Jan. 10, 2024, 4:41 p.m. UTC | #6 
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?
Bartosz Golaszewski Jan. 10, 2024, 8:18 p.m. UTC | #7 
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 | #8 
[ 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
Lukas Wunner Jan. 11, 2024, 10:42 a.m. UTC | #9 
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.
Lukas Wunner Jan. 11, 2024, 3:02 p.m. UTC | #10 
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:

	/*
	 * 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
Lukas Wunner Jan. 11, 2024, 3:06 p.m. UTC | #11 
On Thu, Jan 11, 2024 at 06:10:09PM +0530, Manivannan Sadhasivam wrote:
> 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.

As I've said before, portdrv not only binds to bridges but also
Root Complex Event Collectors.  And you most likely don't want to
populate child DT nodes for those.

> 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.

Again, I'm suggesting to put this into of_pci_make_dev_node().

Thanks,

Lukas
Bartosz Golaszewski Jan. 11, 2024, 4:16 p.m. UTC | #12 
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
Lukas Wunner Jan. 12, 2024, 9:43 a.m. UTC | #13 
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.

Thanks,

Lukas
Lukas Wunner Jan. 12, 2024, 9:47 a.m. UTC | #14 
On Fri, Jan 12, 2024 at 10:43:04AM +0100, Bartosz Golaszewski wrote:
> Lukas, Terry: am I getting this right - is the port driver supposed to
> go away at some point?

Yes, that's the plan.

> 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.

portdrv is not creating a real device for the PCIe port.
It *binds* to that device.  The device is created much earlier.

NAK for adding this to portdrv.

Thanks,

Lukas
Rob Herring Jan. 17, 2024, 11:38 p.m. UTC | #15 
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)