| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMRc=McB4Zk8WuSPL=7+7kX4RJbdFBNReWZyiFnH8vfVx3DxAg@mail.gmail.com>
Date: Wed, 5 Nov 2025 17:21:46 +0100
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
Cc: Bjorn Helgaas <bhelgaas@...gle.com>, Manivannan Sadhasivam <mani@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-kernel@...r.kernel.org,
linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-msm@...r.kernel.org, Stephan Gerhold <stephan.gerhold@...aro.org>,
Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH 4/4] power: sequencing: Add the Power Sequencing driver
for the PCIe M.2 connectors
On Wed, Nov 5, 2025 at 10:17 AM Manivannan Sadhasivam
<manivannan.sadhasivam@....qualcomm.com> wrote:
>
> This driver is used to control the PCIe M.2 connectors of different
> Mechanical Keys attached to the host machines and supporting different
> interfaces like PCIe/SATA, USB/UART etc...
>
> Currently, this driver supports only the Mechanical Key M connectors with
> PCIe interface. The driver also only supports driving the mandatory 3.3v
> and optional 1.8v power supplies. The optional signals of the Key M
> connectors are not currently supported.
>
I'm assuming you followed some of the examples from the existing WCN
power sequencing driver. Not all of them are good or matching this
one, please see below.
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> ---
> MAINTAINERS | 7 ++
> drivers/power/sequencing/Kconfig | 8 ++
> drivers/power/sequencing/Makefile | 1 +
> drivers/power/sequencing/pwrseq-pcie-m2.c | 138 ++++++++++++++++++++++++++++++
> 4 files changed, 154 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 46126ce2f968e4f9260263f1574ee29f5ff0de1c..9b3f689d1f50c62afa3772a0c6802f99a98ac2de 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -20474,6 +20474,13 @@ F: Documentation/driver-api/pwrseq.rst
> F: drivers/power/sequencing/
> F: include/linux/pwrseq/
>
> +PCIE M.2 POWER SEQUENCING
> +M: Manivannan Sadhasivam <mani@...nel.org>
> +L: linux-pci@...r.kernel.org
> +S: Maintained
> +F: Documentation/devicetree/bindings/connector/pcie-m2-m-connector.yaml
> +F: drivers/power/sequencing/pwrseq-pcie-m2.c
> +
> POWER STATE COORDINATION INTERFACE (PSCI)
> M: Mark Rutland <mark.rutland@....com>
> M: Lorenzo Pieralisi <lpieralisi@...nel.org>
> diff --git a/drivers/power/sequencing/Kconfig b/drivers/power/sequencing/Kconfig
> index 280f92beb5d0ed524e67a28d1c5dd264bbd6c87e..f5fff84566ba463b55d3cd0c07db34c82f9f1e31 100644
> --- a/drivers/power/sequencing/Kconfig
> +++ b/drivers/power/sequencing/Kconfig
> @@ -35,4 +35,12 @@ config POWER_SEQUENCING_TH1520_GPU
> GPU. This driver handles the complex clock and reset sequence
> required to power on the Imagination BXM GPU on this platform.
>
> +config POWER_SEQUENCING_PCIE_M2
> + tristate "PCIe M.2 connector power sequencing driver"
> + depends on OF || COMPILE_TEST
The OF dependency in the WCN driver is there because we're doing some
phandle parsing and inspecting the parent-child relationships of the
associated nodes. It doesn't look like you need it here. On the other
hand, if you add more logic to the match() callback, this may come
into play.
> + help
> + Say Y here to enable the power sequencing driver for PCIe M.2
> + connectors. This driver handles the power sequencing for the M.2
> + connectors exposing multiple interfaces like PCIe, SATA, UART, etc...
> +
> endif
> diff --git a/drivers/power/sequencing/Makefile b/drivers/power/sequencing/Makefile
> index 96c1cf0a98ac54c9c1d65a4bb4e34289a3550fa1..0911d461829897c5018e26dbe475b28f6fb6914c 100644
> --- a/drivers/power/sequencing/Makefile
> +++ b/drivers/power/sequencing/Makefile
> @@ -5,3 +5,4 @@ pwrseq-core-y := core.o
>
> obj-$(CONFIG_POWER_SEQUENCING_QCOM_WCN) += pwrseq-qcom-wcn.o
> obj-$(CONFIG_POWER_SEQUENCING_TH1520_GPU) += pwrseq-thead-gpu.o
> +obj-$(CONFIG_POWER_SEQUENCING_PCIE_M2) += pwrseq-pcie-m2.o
> diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..b9f68ee9c5a377ce900a88de86a3e269f9c99e51
> --- /dev/null
> +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pwrseq/provider.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/slab.h>
> +
> +struct pwrseq_pcie_m2_pdata {
> + const struct pwrseq_target_data **targets;
> +};
> +
> +struct pwrseq_pcie_m2_ctx {
> + struct pwrseq_device *pwrseq;
> + const struct pwrseq_pcie_m2_pdata *pdata;
> + struct regulator_bulk_data *regs;
> + size_t num_vregs;
> + struct notifier_block nb;
> +};
> +
> +static int pwrseq_pcie_m2_m_vregs_enable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> + return regulator_bulk_enable(ctx->num_vregs, ctx->regs);
> +}
> +
> +static int pwrseq_pcie_m2_m_vregs_disable(struct pwrseq_device *pwrseq)
> +{
> + struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> +
> + return regulator_bulk_disable(ctx->num_vregs, ctx->regs);
> +}
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_vregs_unit_data = {
> + .name = "regulators-enable",
> + .enable = pwrseq_pcie_m2_m_vregs_enable,
> + .disable = pwrseq_pcie_m2_m_vregs_disable,
> +};
> +
> +static const struct pwrseq_unit_data *pwrseq_pcie_m2_m_unit_deps[] = {
> + &pwrseq_pcie_m2_vregs_unit_data,
> + NULL
> +};
> +
> +static const struct pwrseq_unit_data pwrseq_pcie_m2_m_pcie_unit_data = {
> + .name = "pcie-enable",
> + .deps = pwrseq_pcie_m2_m_unit_deps,
> +};
> +
> +static const struct pwrseq_target_data pwrseq_pcie_m2_m_pcie_target_data = {
> + .name = "pcie",
> + .unit = &pwrseq_pcie_m2_m_pcie_unit_data,
> +};
> +
> +static const struct pwrseq_target_data *pwrseq_pcie_m2_m_targets[] = {
> + &pwrseq_pcie_m2_m_pcie_target_data,
> + NULL
> +};
> +
> +static const struct pwrseq_pcie_m2_pdata pwrseq_pcie_m2_m_of_data = {
> + .targets = pwrseq_pcie_m2_m_targets,
> +};
> +
> +static int pwrseq_pcie_m2_match(struct pwrseq_device *pwrseq,
> + struct device *dev)
> +{
> + return PWRSEQ_MATCH_OK;
Eek! That will match any device we check. I'm not sure this is what
you want. Looking at the binding example, I assume struct device *
here will be the endpoint? If so, you should resolve it and confirm
it's the one referenced from the connector node.
> +}
> +
> +static int pwrseq_pcie_m2_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pwrseq_pcie_m2_ctx *ctx;
> + struct pwrseq_config config;
> + int ret;
> +
> + ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> + if (!ctx)
> + return -ENOMEM;
> +
> + ctx->pdata = of_device_get_match_data(dev);
I should probably address it in the WCN driver - you don't need to use
the OF variant, use device_get_match_data().
> + if (!ctx->pdata)
> + return dev_err_probe(dev, -ENODEV,
> + "Failed to obtain platform data\n");
> +
> + ret = of_regulator_bulk_get_all(dev, dev_of_node(dev), &ctx->regs);
Same here, you already have the device, no need to get the regulators
through the OF node. Just use devm_regulator_bulk_get()
> + if (ret < 0)
> + return dev_err_probe(dev, ret,
> + "Failed to get all regulators\n");
> +
> + ctx->num_vregs = ret;
> +
> + memset(&config, 0, sizeof(config));
Just do config = { }; above?
> +
> + config.parent = dev;
> + config.owner = THIS_MODULE;
> + config.drvdata = ctx;
> + config.match = pwrseq_pcie_m2_match;
> + config.targets = ctx->pdata->targets;
> +
> + ctx->pwrseq = devm_pwrseq_device_register(dev, &config);
> + if (IS_ERR(ctx->pwrseq))
> + return dev_err_probe(dev, PTR_ERR(ctx->pwrseq),
> + "Failed to register the power sequencer\n");
> +
> + return 0;
> +}
> +
> +static const struct of_device_id pwrseq_pcie_m2_of_match[] = {
> + {
> + .compatible = "pcie-m2-m-connector",
> + .data = &pwrseq_pcie_m2_m_of_data,
> + },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, pwrseq_pcie_m2_of_match);
> +
> +static struct platform_driver pwrseq_pcie_m2_driver = {
> + .driver = {
> + .name = "pwrseq-pcie-m2",
> + .of_match_table = pwrseq_pcie_m2_of_match,
> + },
> + .probe = pwrseq_pcie_m2_probe,
> +};
> +module_platform_driver(pwrseq_pcie_m2_driver);
> +
> +MODULE_AUTHOR("Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>");
> +MODULE_DESCRIPTION("Power Sequencing driver for PCIe M.2 connector");
> +MODULE_LICENSE("GPL");
>
> --
> 2.48.1
>
Bartosz
Powered by blists - more mailing lists