mbox series

[RFC,0/9] power: sequencing: implement the subsystem and add first users

Message ID 20240201155532.49707-1-brgl@bgdev.pl
Headers show
Series power: sequencing: implement the subsystem and add first users | expand

Message

Bartosz Golaszewski Feb. 1, 2024, 3:55 p.m. UTC
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

Comments

Rob Herring Feb. 1, 2024, 10:18 p.m. UTC | #1
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/
Rob Herring Feb. 1, 2024, 10:25 p.m. UTC | #2
On Thu, Feb 1, 2024 at 9:55 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
>
> Implement the power sequencing subsystem allowing devices to share
> complex powering-up and down procedures. It's split into the consumer
> and provider parts but does not implement any new DT bindings so that
> the actual power sequencing is never revealed in the DT representation.
>
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>

> +++ b/drivers/power/sequencing/Kconfig
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: GPL-2.0-only


> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> new file mode 100644
> index 000000000000..dcdf8c0c159e
> --- /dev/null
> +++ b/drivers/power/sequencing/Makefile
> @@ -0,0 +1,4 @@
> +# SPDX-License-Identifier: GPL-2.0

GPL-2.0-only to be consistent.

> +
> +obj-$(CONFIG_POWER_SEQUENCING)         += pwrseq-core.o
> +pwrseq-core-y                          := core.o
> diff --git a/drivers/power/sequencing/core.c b/drivers/power/sequencing/core.c
> new file mode 100644
> index 000000000000..f035caed0e4e
> --- /dev/null
> +++ b/drivers/power/sequencing/core.c
> @@ -0,0 +1,482 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later

Why the deviation from the kernel's default license?

Rob
Bjorn Andersson Feb. 2, 2024, 12:40 a.m. UTC | #3
On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 

We now have 3 RFC and 1 PATCH versions of these patches on the list in
under a month. Please at least add a version to your subject...

> 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.
> 

How can we tell? There are still no Documentation/dt-bindings changes in
your series.

Regards,
Bjorn

> [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
> 
> -- 
> 2.40.1
>
Bjorn Andersson Feb. 2, 2024, 4:03 a.m. UTC | #4
On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Add a PCI power control driver that's capable of correctly powering up
> devices using the power sequencing subsystem. For now we support the
> ath11k module on QCA6390.
> 

For a PCI device which doesn't share resources with something on another
bus, the whole power sequencing would be implemented in a driver like
this - without the involvement of the power sequence framework.

I think it would be nice to see this series introduce a simple
pci_pwrctl driver, and then (in the same series) introduce the power
sequence framework and your PMU driver.

One case where such model would be appropriate is the XHCI controller
(uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on
the PCI controller, but it should have been vdd33-supply, vdd10-supply,
avdd33-supply on the PCI device.

That would provide an example for how a simple PCI power control driver
can/should look like, and we can discuss the PCI pieces separate from
the introduction of the new power sequence framework (which is unrelated
to PCI).

Regards,
Bjorn

> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/pci/pwrctl/Kconfig             |  9 +++
>  drivers/pci/pwrctl/Makefile            |  1 +
>  drivers/pci/pwrctl/pci-pwrctl-pwrseq.c | 83 ++++++++++++++++++++++++++
>  3 files changed, 93 insertions(+)
>  create mode 100644 drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> 
> diff --git a/drivers/pci/pwrctl/Kconfig b/drivers/pci/pwrctl/Kconfig
> index e2dc5e5d2af1..bca72dc08e79 100644
> --- a/drivers/pci/pwrctl/Kconfig
> +++ b/drivers/pci/pwrctl/Kconfig
> @@ -5,4 +5,13 @@ menu "PCI Power control drivers"
>  config PCI_PWRCTL
>  	bool
>  
> +config PCI_PWRCTL_PWRSEQ
> +	tristate "PCI Power Control driver using the Power Sequencing subsystem"
> +	select POWER_SEQUENCING
> +	select PCI_PWRCTL
> +	default m if (ATH11K_PCI && ARCH_QCOM)
> +	help
> +	  Enable support for the PCI power control driver for device
> +	  drivers using the Power Sequencing subsystem.
> +
>  endmenu
> diff --git a/drivers/pci/pwrctl/Makefile b/drivers/pci/pwrctl/Makefile
> index 4381cfbf3f21..919c0f704ee9 100644
> --- a/drivers/pci/pwrctl/Makefile
> +++ b/drivers/pci/pwrctl/Makefile
> @@ -1,3 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  obj-$(CONFIG_PCI_PWRCTL)		+= core.o
> +obj-$(CONFIG_PCI_PWRCTL_PWRSEQ)		+= pci-pwrctl-pwrseq.o
> diff --git a/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> new file mode 100644
> index 000000000000..510598c4edc4
> --- /dev/null
> +++ b/drivers/pci/pwrctl/pci-pwrctl-pwrseq.c
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023-2024 Linaro Ltd.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pci-pwrctl.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwrseq/consumer.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +
> +struct pci_pwrctl_pwrseq_data {
> +	struct pci_pwrctl ctx;
> +	struct pwrseq_desc *pwrseq;
> +};
> +
> +static void devm_pci_pwrctl_pwrseq_power_off(void *data)
> +{
> +	struct pwrseq_desc *pwrseq = data;
> +
> +	pwrseq_power_off(pwrseq);
> +}
> +
> +static int pci_pwrctl_pwrseq_probe(struct platform_device *pdev)
> +{
> +	struct pci_pwrctl_pwrseq_data *data;
> +	struct device *dev = &pdev->dev;
> +	int ret;
> +
> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	data->pwrseq = devm_pwrseq_get(dev);
> +	if (IS_ERR(data->pwrseq))
> +		return dev_err_probe(dev, PTR_ERR(data->pwrseq),
> +				     "Failed to get the power sequencer\n");
> +
> +	ret = pwrseq_power_on(data->pwrseq);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to power-on the device\n");
> +
> +	ret = devm_add_action_or_reset(dev, devm_pci_pwrctl_pwrseq_power_off,
> +				       data->pwrseq);
> +	if (ret)
> +		return ret;
> +
> +	data->ctx.dev = dev;
> +
> +	ret = devm_pci_pwrctl_device_enable(dev, &data->ctx);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register the pwrctl wrapper\n");
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id pci_pwrctl_pwrseq_of_match[] = {
> +	{
> +		/* ATH11K in QCA6390 package. */
> +		.compatible = "pci17cb,1101",
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, pci_pwrctl_pwrseq_of_match);
> +
> +static struct platform_driver pci_pwrctl_pwrseq_driver = {
> +	.driver = {
> +		.name = "pci-pwrctl-pwrseq",
> +		.of_match_table = pci_pwrctl_pwrseq_of_match,
> +	},
> +	.probe = pci_pwrctl_pwrseq_probe,
> +};
> +module_platform_driver(pci_pwrctl_pwrseq_driver);
> +
> +MODULE_AUTHOR("Bartosz Golaszewski <bartosz.golaszewski@linaro.org>");
> +MODULE_DESCRIPTION("Generic PCI Power Control module for power sequenced devices");
> +MODULE_LICENSE("GPL");
> -- 
> 2.40.1
>
Bjorn Andersson Feb. 2, 2024, 4:10 a.m. UTC | #5
On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> 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

FWIW, booting RB5 with this patch series does give me working working
WiFI. But I do see the following splat during boot:

[    5.880411] sysfs: cannot create duplicate filename '/devices/platform/soc@0/1c00000.pcie/pci0000:00/0000:00:00.0/resource0'
[    5.891938] CPU: 5 PID: 68 Comm: kworker/u16:4 Not tainted 6.8.0-rc2-next-20240131-00009-g079fdad54c8f #199
[    5.901927] Hardware name: Qualcomm Technologies, Inc. Robotics RB5 (DT)
[    5.908808] Workqueue: events_unbound async_run_entry_fn
[    5.914274] Call trace:
[    5.916794]  dump_backtrace+0xec/0x108
[    5.920649]  show_stack+0x18/0x24
[    5.924062]  dump_stack_lvl+0x50/0x68
[    5.927826]  dump_stack+0x18/0x24
[    5.931238]  sysfs_warn_dup+0x64/0x80
[    5.935004]  sysfs_create_bin_file+0xf4/0x130
[    5.939480]  pci_create_attr+0x100/0x168
[    5.943509]  pci_create_sysfs_dev_files+0x6c/0xc0
[    5.948337]  pci_bus_add_device+0x60/0x114
[    5.952551]  pci_bus_add_devices+0x4c/0x7c
[    5.956762]  pci_host_probe+0x138/0x188
[    5.960700]  dw_pcie_host_init+0x290/0x334
[    5.964914]  qcom_pcie_probe+0x1f8/0x23c
[    5.968942]  platform_probe+0xa8/0xd0
[    5.972707]  really_probe+0x130/0x2e4
[    5.976469]  __driver_probe_device+0xa0/0x128
[    5.980944]  driver_probe_device+0x3c/0x1f8
[    5.985245]  __device_attach_driver+0x118/0x140
[    5.989897]  bus_for_each_drv+0xf4/0x14c
[    5.993923]  __device_attach_async_helper+0x78/0xd0
[    5.998937]  async_run_entry_fn+0x24/0xdc
[    6.003051]  process_scheduled_works+0x210/0x328
[    6.007793]  worker_thread+0x28c/0x450
[    6.011642]  kthread+0xfc/0x184
[    6.014865]  ret_from_fork+0x10/0x20
[    6.018572] ------------[ cut here ]------------
[    6.023339] proc_dir_entry '0000:00/00.0' already registered

Regards,
Bjorn
Bartosz Golaszewski Feb. 2, 2024, 8:53 a.m. UTC | #6
On Fri, Feb 2, 2024 at 1:40 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:23PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
>
> We now have 3 RFC and 1 PATCH versions of these patches on the list in
> under a month. Please at least add a version to your subject...
>

So there was an RFC for the PCI power sequencing (now renamed to PCI
power control - pci_pwrctl), then a proper series for it (with changes
listed). Then an RFC with just proposed DT changes (sent mostly to DT
maintainers to clear it with them) and now an RFC with power
sequencing + PCI power control. Hard to figure out how to version it
if these are pretty much separate developments.

I'll provide links to everything next time.

> > 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.
> >
>
> How can we tell? There are still no Documentation/dt-bindings changes in
> your series.

Noted.

Bartosz

>
> Regards,
> Bjorn
>
> > [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
> >
> > --
> > 2.40.1
> >
Bartosz Golaszewski Feb. 2, 2024, 1:05 p.m. UTC | #7
On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
>
> On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> >
> > Add a PCI power control driver that's capable of correctly powering up
> > devices using the power sequencing subsystem. For now we support the
> > ath11k module on QCA6390.
> >
>
> For a PCI device which doesn't share resources with something on another
> bus, the whole power sequencing would be implemented in a driver like
> this - without the involvement of the power sequence framework.
>

Yes, this is what I did in the previous incarnation of this code[1].

(I know, I should have linked it here. My bad, I will do it next time).

> I think it would be nice to see this series introduce a simple
> pci_pwrctl driver, and then (in the same series) introduce the power
> sequence framework and your PMU driver.
>

I disagree. I was initially annoyed by Dmitry asking me to do a lot
more work than anticipated but he's right after all. WLAN and BT
consuming what is really the PMU's inputs is simply not the actual
representation. That's why we should make it a pwrseq user IMO.

> One case where such model would be appropriate is the XHCI controller
> (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on
> the PCI controller, but it should have been vdd33-supply, vdd10-supply,
> avdd33-supply on the PCI device.

Sounds like a good second user then!

>
> That would provide an example for how a simple PCI power control driver
> can/should look like, and we can discuss the PCI pieces separate from
> the introduction of the new power sequence framework (which is unrelated
> to PCI).

I agree it's unrelated and it could possibly go upstream separately
but the particular use-case on RB5 (and other Qcom platforms) requires
both the PCI and generic power sequencing to be addressed.

Bart

[snip]

[1] https://lore.kernel.org/netdev/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1
Bartosz Golaszewski Feb. 4, 2024, 7:18 p.m. UTC | #8
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
Bjorn Andersson Feb. 9, 2024, 11:37 p.m. UTC | #9
On Fri, Feb 02, 2024 at 02:05:59PM +0100, Bartosz Golaszewski wrote:
> On Fri, Feb 2, 2024 at 5:03 AM Bjorn Andersson <andersson@kernel.org> wrote:
> >
> > On Thu, Feb 01, 2024 at 04:55:32PM +0100, Bartosz Golaszewski wrote:
> > > From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > >
> > > Add a PCI power control driver that's capable of correctly powering up
> > > devices using the power sequencing subsystem. For now we support the
> > > ath11k module on QCA6390.
> > >
> >
> > For a PCI device which doesn't share resources with something on another
> > bus, the whole power sequencing would be implemented in a driver like
> > this - without the involvement of the power sequence framework.
> >
> 
> Yes, this is what I did in the previous incarnation of this code[1].
> 
> (I know, I should have linked it here. My bad, I will do it next time).
> 
> > I think it would be nice to see this series introduce a simple
> > pci_pwrctl driver, and then (in the same series) introduce the power
> > sequence framework and your PMU driver.
> >
> 
> I disagree. I was initially annoyed by Dmitry asking me to do a lot
> more work than anticipated but he's right after all. WLAN and BT
> consuming what is really the PMU's inputs is simply not the actual
> representation. That's why we should make it a pwrseq user IMO.
> 

If the PMU registers the "internal" output regulators, then PCI device
would consume the PCI outputs of the PMU, the BT device would consume
the BT outputs of the PMU. The PMU requests inputs enabled and drives
BT_EN and WLAN_EN according to which subset of these output regulators
are enabled.

Pretty much exactly as "regulator-fixes" isn't a pwrseq device.

Regards,
Bjorn

> > One case where such model would be appropriate is the XHCI controller
> > (uPD720201) on db845c. Today we describe vddpe-3p3-supply as a supply on
> > the PCI controller, but it should have been vdd33-supply, vdd10-supply,
> > avdd33-supply on the PCI device.
> 
> Sounds like a good second user then!
> 
> >
> > That would provide an example for how a simple PCI power control driver
> > can/should look like, and we can discuss the PCI pieces separate from
> > the introduction of the new power sequence framework (which is unrelated
> > to PCI).
> 
> I agree it's unrelated and it could possibly go upstream separately
> but the particular use-case on RB5 (and other Qcom platforms) requires
> both the PCI and generic power sequencing to be addressed.
> 
> Bart
> 
> [snip]
> 
> [1] https://lore.kernel.org/netdev/20240117160748.37682-7-brgl@bgdev.pl/T/#m72f52254a52fcb8a8a44de0702cad1087d4bcfa1