| Message ID | 20240201155532.49707-1-brgl@bgdev.pl |
|---|---|
| Headers | show |
| Series | power: sequencing: implement the subsystem and add first users | expand |
On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote: [..] > > > + break; > > > + } > > > + > > > + return NOTIFY_DONE; > > > +} > > > + > > > +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl) > > > > This function doesn't really "enable the device", looking at the example > > driver it's rather "device_enabled" than "device_enable"... > > > > I was also thinking about pci_pwrctl_device_ready() or > pci_pwrctl_device_prepared(). I like both of these. I guess the bigger question is how the flow would look like in the event that we need to power-cycle the attached PCIe device, e.g. because firmware has gotten into a really bad state. Will we need an operation that removes the device first, and then cut the power, or do we cut the power and then call unprepared()? Regards, Bjorn > > Bart > > [snip!]
On Thu, Feb 1, 2024 at 11:18 PM Rob Herring <robh@kernel.org> wrote: > > On Thu, Feb 1, 2024 at 9:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> > > > > Allow to use __free() to automatically put references to OF nodes. > > Jonathan has already been working on this[1]. > > Rob > > [1] https://lore.kernel.org/all/20240128160542.178315-1-jic23@kernel.org/ Thanks, I will watch this but for now I'll have to stick to carrying it in my series until it gets upstream. Bart
On Fri, Feb 2, 2024 at 5:52 PM Bjorn Andersson <andersson@kernel.org> wrote: > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote: > [..] > > > > + break; > > > > + } > > > > + > > > > + return NOTIFY_DONE; > > > > +} > > > > + > > > > +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl) > > > > > > This function doesn't really "enable the device", looking at the example > > > driver it's rather "device_enabled" than "device_enable"... > > > > > > > I was also thinking about pci_pwrctl_device_ready() or > > pci_pwrctl_device_prepared(). > > I like both of these. > > I guess the bigger question is how the flow would look like in the event > that we need to power-cycle the attached PCIe device, e.g. because > firmware has gotten into a really bad state. > > Will we need an operation that removes the device first, and then cut > the power, or do we cut the power and then call unprepared()? > How would the core be notified about this power-cycle from the PCI subsystem? I honestly don't know. Is there a notifier we could subscribe to? Is the device unbound and rebound in such case? Bart
On Fri, Feb 02, 2024 at 10:52:11AM -0600, Bjorn Andersson wrote: > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote: > [..] > > > > + break; > > > > + } > > > > + > > > > + return NOTIFY_DONE; > > > > +} > > > > + > > > > +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl) > > > > > > This function doesn't really "enable the device", looking at the example > > > driver it's rather "device_enabled" than "device_enable"... > > > > > > > I was also thinking about pci_pwrctl_device_ready() or > > pci_pwrctl_device_prepared(). > > I like both of these. > > I guess the bigger question is how the flow would look like in the event > that we need to power-cycle the attached PCIe device, e.g. because > firmware has gotten into a really bad state. > > Will we need an operation that removes the device first, and then cut > the power, or do we cut the power and then call unprepared()? > Currently, we don't power cycle while resetting the devices. Most of the drivers just do a software reset using some register writes. Part of the reason for that is, the drivers themselves don't control the power to the devices and there would be no way to let the parent know about the firmware crash. - Mani
On Wed, Feb 07, 2024 at 05:26:16PM +0100, Bartosz Golaszewski wrote: > On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <andersson@kernel.org> wrote: > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > > > I was also thinking about pci_pwrctl_device_ready() or > > > pci_pwrctl_device_prepared(). > > > > I like both of these. > > > > I guess the bigger question is how the flow would look like in the event > > that we need to power-cycle the attached PCIe device, e.g. because > > firmware has gotten into a really bad state. > > > > Will we need an operation that removes the device first, and then cut > > the power, or do we cut the power and then call unprepared()? > > How would the core be notified about this power-cycle from the PCI > subsystem? I honestly don't know. Is there a notifier we could > subscribe to? Is the device unbound and rebound in such case? To power-manage the PCI device for runtime PM (suspend to D3cold) or system sleep, you need to amend: platform_pci_power_manageable() platform_pci_set_power_state() platform_pci_get_power_state() platform_pci_refresh_power_state() platform_pci_choose_state() E.g. platform_pci_power_manageable() would check for presence of a regulator in the DT and platform_pci_set_power_state() would disable or enable the regulator. To reset the device by power cycling it, amend pci_reset_fn_methods[] to provide a reset method which disables and re-enables the regulator. Then you can choose that reset method via sysfs and power-cycle the device. The PCI core will also automatically use that reset method if there's nothing else available (e.g. if no Secondary Bus Reset is available because the device has siblings or children, or if FLR is not supported). Thanks, Lukas
On Fri, Feb 09, 2024 at 10:04:33AM +0100, Lukas Wunner wrote: > On Wed, Feb 07, 2024 at 05:26:16PM +0100, Bartosz Golaszewski wrote: > > On Fri, Feb 2, 2024 at 5:52???PM Bjorn Andersson <andersson@kernel.org> wrote: > > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > > > > I was also thinking about pci_pwrctl_device_ready() or > > > > pci_pwrctl_device_prepared(). > > > > > > I like both of these. > > > > > > I guess the bigger question is how the flow would look like in the event > > > that we need to power-cycle the attached PCIe device, e.g. because > > > firmware has gotten into a really bad state. > > > > > > Will we need an operation that removes the device first, and then cut > > > the power, or do we cut the power and then call unprepared()? > > > > How would the core be notified about this power-cycle from the PCI > > subsystem? I honestly don't know. Is there a notifier we could > > subscribe to? Is the device unbound and rebound in such case? > > To power-manage the PCI device for runtime PM (suspend to D3cold) > or system sleep, you need to amend: > > platform_pci_power_manageable() > platform_pci_set_power_state() > platform_pci_get_power_state() > platform_pci_refresh_power_state() > platform_pci_choose_state() > > E.g. platform_pci_power_manageable() would check for presence of a > regulator in the DT and platform_pci_set_power_state() would disable > or enable the regulator. > This will work if the sole control of the resources lies in these platform_*() APIs. But in reality, the controller drivers are the ones controlling the power supply to the devices and with this series, the control would be shifted partly to pwrctl driver. I think what we need is to call in the callbacks of the drivers in a hierarchial order. - Mani > To reset the device by power cycling it, amend pci_reset_fn_methods[] > to provide a reset method which disables and re-enables the regulator. > Then you can choose that reset method via sysfs and power-cycle the > device. The PCI core will also automatically use that reset method > if there's nothing else available (e.g. if no Secondary Bus Reset > is available because the device has siblings or children, or if FLR > is not supported). > > Thanks, > > Lukas
On Thu, Feb 08, 2024 at 05:02:01PM +0530, Manivannan Sadhasivam wrote: > On Fri, Feb 02, 2024 at 10:52:11AM -0600, Bjorn Andersson wrote: > > On Fri, Feb 02, 2024 at 10:11:42AM +0100, Bartosz Golaszewski wrote: > > > On Fri, Feb 2, 2024 at 4:53 AM Bjorn Andersson <andersson@kernel.org> wrote: > > [..] > > > > > + break; > > > > > + } > > > > > + > > > > > + return NOTIFY_DONE; > > > > > +} > > > > > + > > > > > +int pci_pwrctl_device_enable(struct pci_pwrctl *pwrctl) > > > > > > > > This function doesn't really "enable the device", looking at the example > > > > driver it's rather "device_enabled" than "device_enable"... > > > > > > > > > > I was also thinking about pci_pwrctl_device_ready() or > > > pci_pwrctl_device_prepared(). > > > > I like both of these. > > > > I guess the bigger question is how the flow would look like in the event > > that we need to power-cycle the attached PCIe device, e.g. because > > firmware has gotten into a really bad state. > > > > Will we need an operation that removes the device first, and then cut > > the power, or do we cut the power and then call unprepared()? > > > > Currently, we don't power cycle while resetting the devices. Most of the drivers > just do a software reset using some register writes. Part of the reason for > that is, the drivers themselves don't control the power to the devices and there > would be no way to let the parent know about the firmware crash. > I don't know what the appropriate design for this is, but we do have a need for being able to recover from this state by the means of power-cycling the device. If it's not possible to let the device do it (in any fashion), then perhaps a user-space-assisted model is needed? Turning on power is an important first step, but please do consider the full scope of the known problem space. Regards, Bjorn
From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org> I'd like to preface the cover letter by saying right away that this series is not complete. It's an RFC that presents my approach and is sent to the list for discussion. There are no DT bindings nor docs in Documentation/ yet. Please review it as an RFC and not an upstreambound series. If the approach is accepted as correct, I'll add missing bits. The RFC[1] presenting my proposed device-tree representation of the QCA6391 package present on the RB5 board - while not really officially accepted - was not outright rejected which is a good sign. This series incorporates it and builds a proposed power sequencing subsystem together with the first dedicated driver around it. Then it adds first two users: the Bluetooth and WLAN modules of the QCA6391. The Bluetooth part is pretty straightforward. The WLAN however is a PCIe device and as such needs to be powered-up *before* it's detected on the PCI bus. To that end, we modify the PCI core to instantiate platform devices for existing DT child nodes of the PCIe ports. For those nodes for which a power-sequencing driver exists, we bind it and let it probe. The driver then triggers a rescan of the PCI bus with the aim of detecting the now powered-on device. The device will consume the same DT node as the platform, power-sequencing device. We use device links to make the latter become the parent of the former. The main advantage of the above approach (both for PCI as well as generic power sequencers) is that we don't introduce significant changes in DT bindings and don't introduce new properties. We merely define new resources. [1] https://lore.kernel.org/all/CAMRc=MckG32DQv7b1AQL-mbnYdx4fsdYWtLwCyXc5Ma7EeSAKw@mail.gmail.com/T/#md5dc62007d12f6833d4e51658b14e0493954ba68 Bartosz Golaszewski (9): of: provide a cleanup helper for OF nodes arm64: dts: qcom: qrb5165-rb5: model the PMU of the QCA6391 power: sequencing: new subsystem power: pwrseq: add a driver for the QCA6390 PMU module Bluetooth: qca: use the power sequencer for QCA6390 PCI: create platform devices for child OF nodes of the port node PCI: hold the rescan mutex when scanning for the first time PCI/pwrctl: add PCI power control core code PCI/pwrctl: add a PCI power control driver for power sequenced devices arch/arm64/boot/dts/qcom/qrb5165-rb5.dts | 128 +++++- arch/arm64/boot/dts/qcom/sm8250.dtsi | 10 + drivers/bluetooth/hci_qca.c | 30 ++ drivers/pci/Kconfig | 1 + drivers/pci/Makefile | 1 + drivers/pci/bus.c | 9 +- drivers/pci/probe.c | 2 + drivers/pci/pwrctl/Kconfig | 17 + drivers/pci/pwrctl/Makefile | 4 + drivers/pci/pwrctl/core.c | 82 ++++ drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 83 ++++ drivers/pci/remove.c | 2 + drivers/power/Kconfig | 1 + drivers/power/Makefile | 1 + drivers/power/sequencing/Kconfig | 28 ++ drivers/power/sequencing/Makefile | 6 + drivers/power/sequencing/core.c | 482 ++++++++++++++++++++++ drivers/power/sequencing/pwrseq-qca6390.c | 232 +++++++++++ include/linux/of.h | 4 + include/linux/pci-pwrctl.h | 24 ++ include/linux/pwrseq/consumer.h | 53 +++ include/linux/pwrseq/provider.h | 41 ++ 22 files changed, 1229 insertions(+), 12 deletions(-) create mode 100644 drivers/pci/pwrctl/Kconfig create mode 100644 drivers/pci/pwrctl/Makefile create mode 100644 drivers/pci/pwrctl/core.c create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c create mode 100644 drivers/power/sequencing/Kconfig create mode 100644 drivers/power/sequencing/Makefile create mode 100644 drivers/power/sequencing/core.c create mode 100644 drivers/power/sequencing/pwrseq-qca6390.c create mode 100644 include/linux/pci-pwrctl.h create mode 100644 include/linux/pwrseq/consumer.h create mode 100644 include/linux/pwrseq/provider.h