On Sat, Jun 28, 2025 at 04:57:26AM +0530, Manivannan Sadhasivam wrote: > On Fri, Jun 27, 2025 at 05:45:02PM -0500, Bjorn Helgaas wrote: > > On Mon, Jun 16, 2025 at 11:02:09AM +0530, Manivannan Sadhasivam wrote: > > > pci_pwrctrl_create_device() is a PWRCTRL framework API. So it should be > > > built only when CONFIG_PWRCTRL is enabled. Currently, it is built > > > independently of CONFIG_PWRCTRL. This creates enumeration failure on > > > platforms like brcmstb using out-of-tree devicetree that describes the > > > power supplies for endpoints in the PCIe child node, but doesn't use > > > PWRCTRL framework to manage the supplies. The controller driver itself > > > manages the supplies. > > > > > > But in any case, the API should be built only when CONFIG_PWRCTRL is > > > enabled. So move its definition to drivers/pci/pwrctrl/core.c and provide > > > a stub in drivers/pci/pci.h when CONFIG_PWRCTRL is not enabled. This also > > > fixes the enumeration issues on the affected platforms. > > > > Finally circling back to this since I think brcmstb is broken since > > v6.15 and we should fix it for v6.16 final. > > Yes! Sorry for the delay. The fact that I switched the job and had > to attend OSS NA prevented me from reworking this patch. > > > IIUC, v3 is the current patch and needs at least a fix for the build > > issue [1], and I guess the options are: > > > > 1) Make CONFIG_PCI_PWRCTRL bool. On my x86-64 system > > pci-pwrctrl-core.o is 8880 bytes, which seems like kind of a lot > > when only a few systems need it. > > > > 2) Leave pci_pwrctrl_create_device() in probe.c. It gets optimized > > away if CONFIG_OF=n because of_pci_find_child_device() returns > > NULL, but still a little ugly for readers. > > > > 3) Put pci_pwrctrl_create_device() in a separate > > drivers/pci/pwrctrl/ file that is always compiled even if PWRCTRL > > itself is a module. Ugly because then we sort of have two "core" > > files (core.c and whatever new file is always compiled). > > I guess, we could go with option 3 if you prefer. We could rename > the existing pwrctrl/core.c to pwrctrl/pwrctrl.c and move the > definition of pci_pwrctrl_create_device() to new pwrctrl/core.c. The > new file will depend on HAVE_PWRCTRL, which is bool. I think I forgot to mention that option 2 still requires a patch to wrap pci_pwrctrl_create_device() with some sort of #ifdef for CONFIG_PCI_PWRCTRL, right? Seems like we need that regardless of the brcmstb situation so that we don't create pwrctrl devices when CONFIG_OF=y and CONFIG_PCI_PWRCTRL=n. That seems like a straightforward solution and the #ifdef would address my readability concern even though the code stays in probe.c. > > And I guess all of these options still depend on CONFIG_PCI_PWRCTRL > > not being enabled in a kernel that has brcmstb enabled? If so, that > > seems ugly to me. We should be able to enable both PWRCTRL and > > brcmstb at the same time, e.g., for a single kernel image that works > > both on a brcmstb system and a system that needs pwrctrl. > > Right, that would be the end goal. As I explained in the reply to > the bug report [1], this patch will serve as an interim workaround. > Once my pwrctrl rework (which I didn't submit yet) is merged, I will > move this driver to use the pwrctrl framework. OK, so for now, Jim would still need to ensure CONFIG_PCI_PWRCTRL=n when brcmstb is enabled, but we do have a plan to adapt brcmstb work with pwrctrl. > [1] > https://lore.kernel.org/all/vazxuov2hdk5sezrk7a5qfuclv2s3wo5sxhfwuo3o4uedsdlqv@po55ny24ctne/
