On Fri, Jul 25, 2025 at 01:53:02PM GMT, Brian Norris wrote: > Hi Manivannan, > > Sorry for some delay. Things get busy, and I don't get the time for > proper review/reply sometimes... > > On Sat, Jul 12, 2025 at 11:50:43AM +0530, Manivannan Sadhasivam wrote: > > On Fri, Jul 11, 2025 at 04:42:47PM GMT, Brian Norris wrote: > > > Hi, > > > > > > On Mon, Jul 07, 2025 at 11:48:40PM +0530, Manivannan Sadhasivam wrote: > > > > Since the Qcom platforms rely on pwrctrl framework to control the power > > > > supplies, allow it to control PERST# also. PERST# should be toggled during > > > > the power-on and power-off scenarios. > > > > > > > > But the controller driver still need to assert PERST# during the controller > > > > initialization. So only skip the deassert if pwrctrl usage is detected. The > > > > pwrctrl framework will deassert PERST# after turning on the supplies. > > > > > > > > The usage of pwrctrl framework is detected based on the new DT binding > > > > i.e., with the presence of PERST# and PHY properties in the Root Port node > > > > instead of the host bridge node. > > > > > > I just noticed what this paragraph means. IIUC, this implies that in > > > your new binding, one *must* describe one or more *-supply in the port > > > or endpoint device(s). Otherwise, no pwrctrl devices will be created, > > > and no one will deassert PERST# for you. My understanding is that > > > *-supply is optional, and so this is a poor requirement. > > > > > > > Your understanding is correct. But the problem is, you thought that pwrctrl > > would work across all platforms without any modifications, which unfortunately > > I do not think this. Of course there's some modification needed on > occasion, especially when drivers assume they can poll for the link to > come up when power isn't ready, or if they want to get PERST# right > (i.e., $subject). > > OTOH, I don't think you can claim that platforms *don't* support > pwrctrl. If a driver has a well-behaved start_link() behavior and > doesn't otherwise manage slot/endpoint *-supply properties (a la > pcie-brcmstb), it should mostly work without further involvement. > I agree. As I dig more, there seems to be a gazellion of these combinations exist. In the next version, I'll try to accomodate most of them. > But crucially, that changes with PERST#. And I think you're making > very narrow assumptions when you do that. > This series does indeed. I wanted to start small, but I realized that going too simplistic won't work for everyone. > > is not true and is the main source of confusion. And I never claim anywhere that > > pwrctrl is ready for all platforms. I just want platforms to start showing > > interest towards it and we will collectively solve the issues. Or I'll be happy > > to solve the issues if platform maintainers show interest towards it. This is > > what currently happening with brcmstb. I signed up for the transition to > > pwrctrl as their out-of-tree is breaking with pwrctrl. > > > > Right now, we indeed create pwrctrl device based on the presence of power > > supplies as that's how the sole user of pwrctrl (Qcom platforms) behave. But > > I don't see how this is really Qualcomm specific, unless you simply > require that all new Qcom DTs specify external *-supply. I don't see > that in your Documentation/devicetree/bindings/pci/qcom*.yaml though, > and I don't think that's reasonable. > > > sure, for some other platforms we might have only 'reset-gpios'. When we have to > > support those platforms, we will extend the logic. > > The thing is, you don't have 100% control over this. You sound like you > only want to support device trees that are shipped in the upstream > kernel, but that's not how they work -- it's totally valid to ship > non-upstream device trees, if you follow the DT bindings. And you've > already hit that pitfall with brcmstb. > > Suppose you have a Qcom platform today, with pwrctrl support, and: > > 1. it has GPIO PERST# > 2. some boards have external power controls for the endpoint. *-supply > nodes are described for the endpoint, and pwrctrl is in use. > 3. some boards have hardwired power that is always-on / on at boot (no > *-supply node, no pwrctrl). > > As you've written it today, #3 will no longer work, since you're > deferring PERST# to pwrctrl, but pwrctrl never gets involved. > > Crucially, you can't read the driver source to tell the difference > between #2 and #3, and it's not even in the binding schema. Now magnify > this across other drivers that might support this. > > I have boards like #2 and #3, and I don't know how I'm supposed to > develop my driver. > pci_pwrctrl_create_device() should be really smart to figure out these combinations. Let me see how smart it becomes. > > > And even if all QCOM device trees manage to have external regulators > > > described in their device trees, there are certainly other systems where > > > the driver might (optionally) use pwrctrl for some devices, but others > > > will establish power on their own (e.g., PCIe tied to some other system > > > power rail). > > > > > > I think you either need the PCI core to tell you whether pwrctrl is in > > > use, or else you need to disconnect this PERST# support from pwrctrl. > > > > > > > It is not straightforward for the PCI core to tell whether pwrctrl is in use or > > not. > > Yes, well this seems like a fundamental recurring problem at the root > here. This agnostic design just causes more problems, IMO. > > > pwrctrl has no devicetree representation as it is not a separate hardware > > entity. It just reuses the PCI device DT node. So I used the -supply properties > > to assume that the pwrctrl device will be used. And almost none of the upstream > > DTS has -supply properties in the PCI child node (only exception is brcmstb > > where they define -supply properties in the PCI child node, but only in the DT > > binding). So that added up. > > You gotta work off DT bindings and schema to make assertions. You can't > just guess based on in-tree device trees, and so you can't prove > non-existence, if it's not explicit in the bindings. > This is the actual problem here. We cannot introduce any changes in the binding for the sake of pwrctrl. Pwrctrl is just a kernel framework which is supposed to work with the existing bindings and DTS. For sure we can ammend the binding to make it strict. Like, mandating the -supply property even if it is always on as the endpoint definitely needs atleast one supply from the host (well from the system PMIC atleast). But nothing more. - Mani -- மணிவண்ணன் சதாசிவம்
