Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support

From: Johan Hovold <johan@kernel.org>
To: Baruch Siach <baruch@tkos.co.il>
Cc: "Andy Gross" <agross@kernel.org>,
	"Bjorn Andersson" <bjorn.andersson@linaro.org>,
	"Stanimir Varbanov" <svarbanov@mm-sol.com>,
	"Selvam Sathappan Periakaruppan" <quic_speriaka@quicinc.com>,
	"Kathiravan T" <quic_kathirav@quicinc.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Jonathan Hunter" <jonathanh@nvidia.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Robert Marko" <robert.marko@sartura.hr>,
	"Bryan O'Donoghue" <pure.logic@nexus-software.ie>,
	"Pali Rohár" <pali@kernel.org>,
	linux-pci@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-tegra@vger.kernel.org
Subject: Re: [PATCH v7 3/3] PCI: qcom: Add IPQ60xx support
Date: Tue, 21 Jun 2022 09:53:37 +0200	[thread overview]
Message-ID: <YrF5ARelhL5GnOnJ@hovoldconsulting.com> (raw)
In-Reply-To: <87k09aekop.fsf@tarshish>

On Tue, Jun 21, 2022 at 06:39:45AM +0300, Baruch Siach wrote:
> Hi Johan,
> 
> Thanks for your review comments.
> 
> On Mon, Jun 20 2022, Johan Hovold wrote:
> > On Sun, Jun 12, 2022 at 01:18:35PM +0300, Baruch Siach wrote:
> >> From: Selvam Sathappan Periakaruppan <quic_speriaka@quicinc.com>
> >> 
> >> IPQ60xx series of SoCs have one port of PCIe gen 3. Add support for that
> >> platform.
> >> 
> >> The code is based on downstream[1] Codeaurora kernel v5.4 (branch
> >> win.linuxopenwrt.2.0).
> >> 
> >> Split out the DBI registers access part from .init into .post_init. DBI
> >> registers are only accessible after phy_power_on().
> >> 
> >> [1] https://source.codeaurora.org/quic/qsdk/oss/kernel/linux-ipq-5.4/
> >> 
> >> Signed-off-by: Selvam Sathappan Periakaruppan <speriaka@codeaurora.org>
> >> Signed-off-by: Baruch Siach <baruch.siach@siklu.com>
> >> ---
> >
> >> +static void qcom_pcie_deinit_2_9_0(struct qcom_pcie *pcie)
> >> +{
> >> +	struct qcom_pcie_resources_2_9_0 *res = &pcie->res.v2_9_0;
> >> +
> >> +	clk_bulk_disable_unprepare(ARRAY_SIZE(res->clks), res->clks);
> >
> > Assert reset as you do in the init error path?
> 
> Not sure about that. As I understand, the reset assert/deassert sequence
> on init is meant to ensure clean startup state. Deinit most likely does
> not need that. So maybe I should remove reset assert from init error
> path instead?

Yeah, I think the init error path and deinit should at least be
consistent.

> As always, this code sequence is from downstream kernel. I have no
> access to documentation.

I feel your pain. ;)

Johan