From: Bartosz Golaszewski <[email protected]>
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 <[email protected]>
---
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)
--
2.40.1
On 1/4/2024 5:01 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> 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 <[email protected]>
> ---
> 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)
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
On Tue, Jan 9, 2024 at 3:43 PM Lukas Wunner <[email protected]> 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
>
On Wed, Jan 10, 2024 at 01:55:18PM +0100, Bartosz Golaszewski wrote:
> On Tue, Jan 9, 2024 at 3:43???PM Lukas Wunner <[email protected]> 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
On Wed, Jan 10, 2024 at 2:28 PM Lukas Wunner <[email protected]> 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 <[email protected]> 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]
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?
[ 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 <[email protected]> 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
On Wed, Jan 10, 2024 at 02:18:30PM -0600, Bartosz Golaszewski wrote:
> On Wed, 10 Jan 2024 17:41:05 +0100, Lukas Wunner <[email protected]> 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.
On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <[email protected]> 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 <[email protected]> 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
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 <[email protected]> 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
>
--
மணிவண்ணன் சதாசிவம்
On Thu, Jan 11, 2024 at 05:09:09AM -0600, Bartosz Golaszewski wrote:
> On Thu, 11 Jan 2024 11:42:11 +0100, Lukas Wunner <[email protected]> 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 <[email protected]> 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
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
On Thu, Jan 11, 2024 at 4:02 PM Lukas Wunner <[email protected]> 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 <[email protected]> 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 <[email protected]> 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
Hi Bartosz,
On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <[email protected]> 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 -- [email protected]
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
On Thu, Jan 11, 2024 at 10:44 PM Geert Uytterhoeven
<[email protected]> wrote:
>
> Hi Bartosz,
>
> On Thu, Jan 11, 2024 at 5:16 PM Bartosz Golaszewski <[email protected]> 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 -- [email protected]
>
> 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
>
On Thu, Jan 11, 2024 at 05:16:45PM +0100, Bartosz Golaszewski wrote:
> On Thu, Jan 11, 2024 at 4:02???PM Lukas Wunner <[email protected]> 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
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
On Fri, Jan 12, 2024 at 3:43 AM Lukas Wunner <[email protected]> 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 <[email protected]> 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