linux-pci.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Krzysztof Wilczyński" <kw@linux.com>
To: Greentime Hu <greentime.hu@sifive.com>
Cc: paul.walmsley@sifive.com, hes@sifive.com, erik.danie@sifive.com,
	zong.li@sifive.com, bhelgaas@google.com, robh+dt@kernel.org,
	palmer@dabbelt.com, aou@eecs.berkeley.edu,
	mturquette@baylibre.com, sboyd@kernel.org,
	lorenzo.pieralisi@arm.com, p.zabel@pengutronix.de,
	alex.dewar90@gmail.com, khilman@baylibre.com,
	hayashi.kunihiko@socionext.com, vidyas@nvidia.com,
	jh80.chung@samsung.com, linux-pci@vger.kernel.org,
	devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
	linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
	helgaas@kernel.org
Subject: Re: [PATCH v2 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver
Date: Fri, 19 Mar 2021 05:37:58 +0100	[thread overview]
Message-ID: <YFQqpojmJyX0l6lx@rocinante> (raw)
In-Reply-To: <27f491c113e1bb369d25aca02571b34422986142.1615954046.git.greentime.hu@sifive.com>

Hi,

[...]
> +static void fu740_phyregwrite(const uint8_t phy, const uint16_t addr,
> +			      const uint16_t wrdata, struct fu740_pcie *afp)
> +{
> +	struct device *dev = afp->pci.dev;
> +	void __iomem *phy_cr_para_addr;
> +	void __iomem *phy_cr_para_wr_data;
> +	void __iomem *phy_cr_para_wr_en;
> +	void __iomem *phy_cr_para_ack;
> +	int ret, val;
> +
> +	/* Setup */
> +	if (phy) {
> +		phy_cr_para_addr = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_ADDR;
> +		phy_cr_para_wr_data = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_WR_DATA;
> +		phy_cr_para_wr_en = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_WR_EN;
> +		phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY1_CR_PARA_ACK;
> +	} else {
> +		phy_cr_para_addr = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_ADDR;
> +		phy_cr_para_wr_data = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_WR_DATA;
> +		phy_cr_para_wr_en = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_WR_EN;
> +		phy_cr_para_ack = afp->mgmt_base + PCIEX8MGMT_PHY0_CR_PARA_ACK;
> +	}
> +
> +	writel_relaxed(addr, phy_cr_para_addr);
> +	writel_relaxed(wrdata, phy_cr_para_wr_data);
> +	writel_relaxed(1, phy_cr_para_wr_en);
> +
> +	/* Wait for wait_idle */
> +	ret = readl_poll_timeout(phy_cr_para_ack, val, val, 10, 5000);
> +	if (ret)
> +		dev_err(dev, "Wait for wait_ilde state failed!\n");

It would be "wait_idle" rather than "wait_idle".

[...]
> +	/* Wait for ~wait_idle */
> +	ret = readl_poll_timeout(phy_cr_para_ack, val, !val, 10, 5000);
> +	if (ret)
> +		dev_err(dev, "Wait for !wait_ilde state failed!\n");
[...]

Same as above, it would be "wait_idle" in the above.

> +static void fu740_pcie_ltssm_enable(struct device *dev)
> +{
> +	struct fu740_pcie *afp = dev_get_drvdata(dev);
> +
> +	/* Enable LTSSM */
> +	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_LTSSM_ENABLE);
> +}
> +
> +static int fu740_pcie_start_link(struct dw_pcie *pci)
> +{
> +	struct device *dev = pci->dev;
> +
> +	/* Start LTSSM. */

Nitpick.  No need for a dot in this comment to keep it consistent with
the comment in the function above this one.

> +static int fu740_pcie_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct fu740_pcie *afp = to_fu740_pcie(pci);
> +	struct device *dev = pci->dev;
> +	int ret;
> +
> +	/* Power on reset */
> +	fu740_pcie_drive_perstn(afp);
> +
> +	/* Enable pcieauxclk */
> +	ret = clk_prepare_enable(afp->pcie_aux);
> +	if (ret)
> +		dev_err(dev, "unable to enable pcie_aux clock\n");
> +
> +	/*
> +	 * Assert hold_phy_rst (hold the controller LTSSM in reset after
> +	 * power_up_rst_n for register programming with cr_para)
> +	 */
> +	writel_relaxed(0x1, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +
> +	/* Deassert power_up_rst_n */
> +	ret = reset_control_deassert(afp->rst);
> +	if (ret)
> +		dev_err(dev, "unable to deassert pcie_power_up_rst_n\n");
> +
> +	fu740_pcie_init_phy(afp);
> +
> +	/* Disable pcieauxclk */
> +	clk_disable_unprepare(afp->pcie_aux);
> +	/* Clear hold_phy_rst */
> +	writel_relaxed(0x0, afp->mgmt_base + PCIEX8MGMT_APP_HOLD_PHY_RST);
> +	/* Enable pcieauxclk */
> +	ret = clk_prepare_enable(afp->pcie_aux);
> +	/* Set RC mode */
> +	writel_relaxed(0x4, afp->mgmt_base + PCIEX8MGMT_DEVICE_TYPE);
> +
> +	return 0;
> +}
[...]

It seems that the error handling is somewhat broken in the above
function, especially when you look at how the "ret" variables does not
seem to be used for anything once there was an error.

Krzysztof

  reply	other threads:[~2021-03-19  4:38 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-03-18  6:08 [PATCH v2 0/6] Add SiFive FU740 PCIe host controller driver support Greentime Hu
2021-03-18  6:08 ` [PATCH v2 1/6] clk: sifive: Add pcie_aux clock in prci driver for PCIe driver Greentime Hu
2021-03-18  6:08 ` [PATCH v2 2/6] clk: sifive: Use reset-simple " Greentime Hu
2021-03-29 19:14   ` Stephen Boyd
2021-03-30  3:36     ` Greentime Hu
2021-03-31  0:24       ` Palmer Dabbelt
2021-03-18  6:08 ` [PATCH v2 3/6] MAINTAINERS: Add maintainers for SiFive FU740 " Greentime Hu
2021-03-18  6:08 ` [PATCH v2 4/6] dt-bindings: PCI: Add SiFive FU740 PCIe host controller Greentime Hu
2021-03-19  3:56   ` Krzysztof Wilczyński
2021-03-19 21:49   ` Rob Herring
2021-03-23 20:35   ` Rob Herring
2021-03-29  3:39     ` Greentime Hu
2021-03-18  6:08 ` [PATCH v2 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver Greentime Hu
2021-03-19  4:37   ` Krzysztof Wilczyński [this message]
2021-03-19  4:42     ` Krzysztof Wilczyński
2021-03-18  6:08 ` [PATCH v2 6/6] riscv: dts: Add PCIe support for the SiFive FU740-C000 SoC Greentime Hu
2021-03-31  0:24   ` Palmer Dabbelt
2021-04-19  2:43     ` Greentime Hu
2021-04-19  2:48       ` Greentime Hu
2021-03-29 19:12 ` [PATCH v2 0/6] Add SiFive FU740 PCIe host controller driver support Stephen Boyd
2021-04-01  6:16   ` Greentime Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YFQqpojmJyX0l6lx@rocinante \
    --to=kw@linux.com \
    --cc=alex.dewar90@gmail.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=bhelgaas@google.com \
    --cc=devicetree@vger.kernel.org \
    --cc=erik.danie@sifive.com \
    --cc=greentime.hu@sifive.com \
    --cc=hayashi.kunihiko@socionext.com \
    --cc=helgaas@kernel.org \
    --cc=hes@sifive.com \
    --cc=jh80.chung@samsung.com \
    --cc=khilman@baylibre.com \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=mturquette@baylibre.com \
    --cc=p.zabel@pengutronix.de \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=vidyas@nvidia.com \
    --cc=zong.li@sifive.com \
    --subject='Re: [PATCH v2 5/6] PCI: fu740: Add SiFive FU740 PCIe host controller driver' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).