Re: [PATCH v8 10/11] PCI: qcom: Add SM8550 PCIe support

From: Johan Hovold <johan@kernel.org>
To: Abel Vesa <abel.vesa@linaro.org>
Cc: "Andy Gross" <agross@kernel.org>,
	"Bjorn Andersson" <andersson@kernel.org>,
	"Konrad Dybcio" <konrad.dybcio@linaro.org>,
	"Rob Herring" <robh@kernel.org>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"vkoul@kernel.org" <vkoul@kernel.org>,
	"Kishon Vijay Abraham I" <kishon@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Johan Hovold" <johan+linaro@kernel.org>,
	linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-phy@lists.infradead.org, devicetree@vger.kernel.org,
	"Linux Kernel Mailing List" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v8 10/11] PCI: qcom: Add SM8550 PCIe support
Date: Wed, 8 Feb 2023 17:58:35 +0100	[thread overview]
Message-ID: <Y+PUuy0qbO9EWOXb@hovoldconsulting.com> (raw)
In-Reply-To: <20230206212619.3218741-11-abel.vesa@linaro.org>

On Mon, Feb 06, 2023 at 11:26:18PM +0200, Abel Vesa wrote:
> Add compatible for both PCIe found on SM8550.
> Also add the noc_aggr and cnoc_sf_axi clocks needed by the SM8550.
> 
> Signed-off-by: Abel Vesa <abel.vesa@linaro.org>
> Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
> ---

> @@ -1237,17 +1239,17 @@ static int qcom_pcie_init_2_7_0(struct qcom_pcie *pcie)
>  	if (ret < 0)
>  		goto err_disable_regulators;
>  
> -	ret = reset_control_assert(res->pci_reset);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot assert pci reset\n");
> +	ret = reset_control_assert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset assert failed (%d)\n", ret);
>  		goto err_disable_clocks;
>  	}
>  
>  	usleep_range(1000, 1500);
>  
> -	ret = reset_control_deassert(res->pci_reset);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot deassert pci reset\n");
> +	ret = reset_control_deassert(res->rst);
> +	if (ret) {
> +		dev_err(dev, "reset deassert failed (%d)\n", ret);
>  		goto err_disable_clocks;
>  	}

I'd still like to know if it's indeed ok the deassert the 'pci' reset
before the new 'link_down' reset here which is what this code currently
would do (i.e. same order as for assert, while if you had used the bulk
reset API it would have deasserted in the reverse order).

Other than that:

Reviewed-by: Johan Hovold <johan+linaro@kernel.org>