Message-ID: <5wbwpr7ivnvpttacyl7b5fsexfda2uvoqau7yaaxuavskka4z6@vvntbnakzrjb>
Date: Thu, 6 Nov 2025 20:02:01 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>, 
	Bjorn Helgaas <bhelgaas@...gle.com>, 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 Thu, Nov 06, 2025 at 10:53:05AM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 5, 2025 at 6:46 PM Manivannan Sadhasivam <mani@...nel.org> wrote:
> >
> > On Wed, Nov 05, 2025 at 05:21:46PM +0100, Bartosz Golaszewski wrote:
> > > 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.
> > >
> >
> > For sure the driver will build fine for !CONFIG_OF, but it is not going to work.
> > And for the build coverage, COMPILE_TEST is already present. Maybe I was wrong
> > to enforce functional dependency in Kconfig.
> >
> 
> Given what you said below for the regulator API, let's keep it as is.
> 
> > > > +
> > > > +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.
> > >
> >
> > I was expecting this question, so returned PWRSEQ_MATCH_OK on purpose. I feel it
> > is redundant to have match callback that just does link resolution and matches
> > the of_node of the caller. Can't we have a default match callback that does just
> > this?
> >
> 
> To be clear: the above is certainly wrong. Any power sequencing
> consumer would match against this device.
> 
> To answer your question: sure, there is nothing wrong with having a
> default match callback but first: I'd like to see more than one user
> before we generalize it, and second: it still needs some logic. What
> is the relationship between the firmware nodes of dev and pwrseq here
> exactly?
> 

The 'dev' belongs to the PCIe Root Port node where the graph port is defined:

&pcie6_port0 {
	...
	port {
		pcie6a_port0_ep: endpoint {
			remote-endpoint = <&m2_pcie_ep>;
		};
	};
};

So I have to do remote-endpoint lookup from the pwrseq and compare the of_node
of the parent with 'dev->of_node', I believe. If so, this looks like a common
pattern.

> > > > +       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()
> > >
> >
> > I used it on purpose. This is the only regulator API that just gets all
> > regulators defined in the devicetree node without complaining. Here, 3.3v is
> > mandatory and 1.8v is optional. There could be other supplies in the future and
> > I do not want to hardcode the supply names in the driver. IMO, the driver should
> > trust devicetree to supply enough supplies and it should just consume them
> > instead of doing validation. I proposed to add a devm_ variant for this, but
> > Mark was against that idea.
> >
> 
> What was the reason for being against it?

Mark doesn't want the drivers to trust DT for regulators. It was an IRC
discussion and the conclusion was the drivers had to specify the supplies
manually and not trust DT to provide required regulators. But then we already
have this API that does the same.

> Anyway: in that case, would
> you mind adding a comment containing what you wrote here so that
> people don't mindlessly try convert it to the regular variant in the
> future?
> 

Sure.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

Powered by blists - more mailing lists