Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 27 Jun 2022 09:31:04 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Johan Hovold <johan+linaro@kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Rob Herring <robh@kernel.org>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Stanimir Varbanov <svarbanov@mm-sol.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] PCI: qcom: Add support for modular builds
Message-ID: <YrlcuExtGFp/ASF+@hovoldconsulting.com>
References: <20220519094646.23009-1-johan+linaro@kernel.org>
 <20220623155213.GA1450949@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220623155213.GA1450949@bhelgaas>
Precedence: bulk

On Thu, Jun 23, 2022 at 10:52:13AM -0500, Bjorn Helgaas wrote:
> On Thu, May 19, 2022 at 11:46:46AM +0200, Johan Hovold wrote:
> > Allow the Qualcomm PCIe controller driver to be built as a module, which
> > is useful for multi-platform kernels as well as during development.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---

> > +static void qcom_pcie_host_deinit(struct qcom_pcie *pcie)
> > +{
> > +	qcom_ep_reset_assert(pcie);
> > +	if (pcie->cfg->ops->post_deinit)
> > +		pcie->cfg->ops->post_deinit(pcie);
> > +	phy_power_off(pcie->phy);
> > +	pcie->cfg->ops->deinit(pcie);
> 
> These post_deinit/deinit names look backwards.  Why would we call a
> "post_deinit()" method *before* the "deinit()" method?  It would make
> sense if we called "pre_deinit()" followed by "deinit()".

Yeah, that annoys me as well, but those are the names the driver
currently use.

I considered renaming the deinit callback but instead sent a follow up
patch to remove both of these callbacks now that the pipe clock rework
that depends on them has been merged, but seems like the post_init one
will be needed for the DBI accesses.

I can respin the above mentioned patch to drop or or rename the badly
named one when things settle down a bit.

> >  static const struct dw_pcie_host_ops qcom_pcie_dw_ops = {
> >  	.host_init = qcom_pcie_host_init,
> >  };
> > @@ -1651,6 +1660,22 @@ static int qcom_pcie_probe(struct platform_device *pdev)
> >  	return ret;
> >  }
> >  
> > +static int qcom_pcie_remove(struct platform_device *pdev)
> > +{
> > +	struct qcom_pcie *pcie = platform_get_drvdata(pdev);
> > +	struct device *dev = &pdev->dev;
> > +
> > +	dw_pcie_host_deinit(&pcie->pci->pp);
> > +	qcom_pcie_host_deinit(pcie);
> > +
> > +	phy_exit(pcie->phy);
> > +
> > +	pm_runtime_put_sync(dev);
> > +	pm_runtime_disable(dev);
> 
> Why is this not more symmetric with qcom_pcie_probe()?  Maybe struct
> dw_pcie_host_ops needs a new .host_deinit() pointer that would be
> called from dw_pcie_host_deinit()?

Yeah, I considered that too but decided it's not needed to implement
modular builds for this driver. Instead I did the ground work by adding
a deinit helper function so that it's easier to track any additions to
the host_init() callback and which can later be called by core if
someone decides to clean up core and add a deinit callback.

Looks like someone just posted something along these lines, but this
would conflict with the split MSI series which is otherwise ready to be
merged:

	https://lore.kernel.org/r/20220624143947.8991-9-Sergey.Semin@baikalelectronics.ru

Also note that there are other drivers that implement remove() without
this callback already today.

> In the probe path, we have this:
> 
>   qcom_pcie_probe
>     pm_runtime_enable
>     pm_runtime_get_sync
>     phy_init(pcie->phy)
>     dw_pcie_host_init
>       pp->ops->host_init
>         qcom_pcie_host_init             # .host_init()
>           pcie->cfg->ops->init(pcie)
>           phy_power_on(pcie->phy)
>           pcie->cfg->ops->post_init(pcie)
>           qcom_ep_reset_deassert(pcie)
> 
> The remove path does do things in the opposite order, which makes
> sense, but the call to qcom_pcie_host_deinit() breaks the symmetry:
> 
>   qcom_pcie_remove
>     dw_pcie_host_deinit
>     qcom_pcie_host_deinit
>       qcom_ep_reset_assert
>       pcie->cfg->ops->post_deinit
>       phy_power_off(pcie->phy)
>       pcie->cfg->ops->deinit
>     phy_exit(pcie->phy)
>     pm_runtime_put_sync
>     pm_runtime_disable

Yeah, I didn't want to go rewrite core just for this basic driver
functionality. Especially with so many things already in flux.

As mentioned above, everything is instead prepared to move over to such
a callback if and when it materialises.

Johan