On Tue, May 20, 2025 at 7:40 PM Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote: > > On Tue, May 20, 2025 at 11:06:33AM -0400, Jim Quinlan wrote: > > [...] > > > > > > What systems does the regression affect? > > > > > > > > All Broadcom STB chips, including the RPi sister chips. Now is there > > > > anyone but our team who runs upstream Linux on our boards? Probably > > > > not. > > > > > > > > > > I forgot to ask you this question. Is your devicetree upstreamed? Because, while > > > introducing the pwrctrl knobs, I made sure that there are no upstream users > > > using the supply properties in child nodes. All our existing users are using the > > > properties in controller nodes, so they are not impacted. > > Hello Mani, > > > > Our device tree is constructed on the fly by our custom bootloader > > and passed to Linux, so it does not make sense (IMO) to put them > > upstream as long as we strictly follow our upstreamed YAML bindings > > file. As I mentioned before, our brcm,stb-pcie.yaml example, which > > is upstream, contains a "vpcie3v3-supply" property under the pci@0,0 > > node. > > > > Okay, thanks for the pointer. I was not aware that you have bootloader > constructed DTB. > > > > > > > Here it looks like you are running out-of-tree dts, which we do not support > > > unfortunately. > > The brcmstb YAML file is upstream, and a grep for the standard pcie > > regulator names would have led you to it. I don't see how you can say > > you don't support a DT that follows the upstream YAML file(s). > > > > I didn't say that exactly. I thought you were using some out-of-tree vendor DTS > which doesn't adhere to the DT bindings, but I was wrong. Your usecase is > completely valid. > > > But it doesn't mean that I do not care about the issue you > > > reported. I'm flying back home from vacation tomorrow and it will be the first > > > thing I'm goona look into. > > > > > > Adding suspend/resume support is pretty much what I'm going to work on the > > > upcoming weeks (combined with some rework). So until then, I request you to > > > revert the changes in your downstream tree and bear with me. > > > > I would rather our systems peacefully coexist, and take your changes > > voluntarily and on my own schedule, rather than wait for you to add > > future features. I'm a little surprised that the CONFIG_PCI_PWRCTL > > code seems to act on the PCI regulators even when its driver is not > > present. > > > > I overlooked at that part. Would the below diff help you? > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 4b8693ec9e4c..b38a62f40448 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2514,6 +2514,9 @@ static struct platform_device *pci_pwrctrl_create_device(struct pci_bus *bus, in > struct platform_device *pdev; > struct device_node *np; > > + if (!IS_ENABLED(CONFIG_PCI_PWRCTRL)) > + return NULL; > + > np = of_pci_find_child_device(dev_of_node(&bus->dev), devfn); > if (!np || of_find_device_by_node(np)) > return NULL; > Hi Mani, Yes that works but I have to set CONFIG_ATH11K=n CONFIG_ATH12K=n CONFIG_ATH11K_PCI=n in order to get CONFIG_PCI_PWRCTRL=n This would be a problem if we had a customer system that required ATH1[2]. We do have one with ATH9k but we are safe for now. > > > In addition, I need the ability to cancel at runtime the > > suspend/resume off/on of the regulators if the downstream device. > > > > I don't get what you mean by 'ability to cancel at runtime'. Like I said before, > I'm going to rework pwrctrl little bit and probably add suspend/resume (system > PM first) on the way, if that's what you are referring to. If you look at drivers/pci/controller/pcie-brcmstb.c and search for brcm_pcie_suspend_noirq(),, you will see that we only turn off/on the regulators on suspend/resume if the downstream device has device_may_wakeup(dev)==false. We discover this at runtime by walking the bus. The idea here is to support an optional WOL scenario. I'm currently inquiring whether we can lessen this requirement somehow and will let you know when I get the answer. > > > That being said, I don't mind if the series stays as long as you > > promise to work with me to have our systems coexist. But I do not > > want to wait for future features to be added for our code to work. > > > > > > Bjorn, FYI: We do not need any revert in mainline since the issue is not > > > affecting upstream users. > > > > So is it a rule that all DT must be upstreamed, or is it sufficient > > to have a bindings YAML file that defines the DT for the driver? > > > > The latter IMO. If the diff I supplied above works fine, I'll submit a patch for > that. Eventually, we do like to control the supplies from the pwrctrl driver > instead of from controller drivers. That's the whole point of introducing this > framework. But since it is not enabled in defconfig yet, your driver should > continue working in the meantime with the diff. Later on, I will modify brcmstb > driver to adapt to pwrctrl framework. Since the binding is same, the driver is > going to be backwards compatible also. > > Please let me know if it works of not! Yes it does work. Not a perfect situation, but it will keep me going as we move toward having the pcie-brcmstb driver move to the pwrctrl framework. Thanks, Jim Quinlan Broadcom STB/CM > > - Mani > > -- > மணிவண்ணன் சதாசிவம்
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
