Message-ID: <20190329203159.GG24180@google.com>
Date:   Fri, 29 Mar 2019 15:52:49 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Vidya Sagar <vidyas@...dia.com>
Cc:     robh+dt@...nel.org, mark.rutland@....com, thierry.reding@...il.com,
        jonathanh@...dia.com, kishon@...com, catalin.marinas@....com,
        will.deacon@....com, lorenzo.pieralisi@....com,
        jingoohan1@...il.com, gustavo.pimentel@...opsys.com,
        mperttunen@...dia.com, tiwai@...e.de, spujar@...dia.com,
        skomatineni@...dia.com, liviu.dudau@....com, krzk@...nel.org,
        heiko@...ech.de, horms+renesas@...ge.net.au, olof@...om.net,
        maxime.ripard@...tlin.com, andy.gross@...aro.org,
        bjorn.andersson@...aro.org, jagan@...rulasolutions.com,
        enric.balletbo@...labora.com, ezequiel@...labora.com,
        stefan.wahren@...e.com, marc.w.gonzalez@...e.fr,
        l.stach@...gutronix.de, tpiepho@...inj.com,
        hayashi.kunihiko@...ionext.com, yue.wang@...ogic.com,
        shawn.lin@...k-chips.com, xiaowei.bao@....com,
        devicetree@...r.kernel.org, mmaddireddy@...dia.com,
        kthota@...dia.com, linux-pci@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 09/10] PCI: tegra: Add Tegra194 PCIe support

Hi Vidya,

Wow, there's a lot of nice work here!  Thanks for that!

On Tue, Mar 26, 2019 at 08:43:26PM +0530, Vidya Sagar wrote:
> Add support for Synopsys DesignWare core IP based PCIe host controller
> present in Tegra194 SoC.

General comments:

  - There are a few multi-line comments that don't match the
    prevailing style:

        /*
	 * Text...
	 */

  - Comments and dev_info()/dev_err() messages are inconsistent about
    starting with upper-case or lower-case letters.

  - Use "MSI", "IRQ", "PCIe", "CPU", etc in comments and messages.

  - There are a few functions that use "&pdev->dev" many times; can
    you add a "struct device *dev = &pdev->dev" to reduce the
    redundancy?

> +#include "../../pcie/portdrv.h"

What's this for?  I didn't see any obvious uses of things from
portdrv.h, but I didn't actually try to build without it.

> +struct tegra_pcie_dw {
> +	struct device		*dev;
> +	struct resource		*appl_res;
> +	struct resource		*dbi_res;
> +	struct resource		*atu_dma_res;
> +	void __iomem		*appl_base;
> +	struct clk		*core_clk;
> +	struct reset_control	*core_apb_rst;
> +	struct reset_control	*core_rst;
> +	struct dw_pcie		pci;
> +	enum dw_pcie_device_mode mode;
> +
> +	bool disable_clock_request;
> +	bool power_down_en;
> +	u8 init_link_width;
> +	bool link_state;
> +	u32 msi_ctrl_int;
> +	u32 num_lanes;
> +	u32 max_speed;
> +	u32 init_speed;
> +	bool cdm_check;
> +	u32 cid;
> +	int pex_wake;
> +	bool update_fc_fixup;
> +	int n_gpios;
> +	int *gpios;
> +#if defined(CONFIG_PCIEASPM)
> +	u32 cfg_link_cap_l1sub;
> +	u32 event_cntr_ctrl;
> +	u32 event_cntr_data;
> +	u32 aspm_cmrt;
> +	u32 aspm_pwr_on_t;
> +	u32 aspm_l0s_enter_lat;
> +	u32 disabled_aspm_states;
> +#endif

The above could be indented the same as the rest of the struct?

> +	struct regulator	*pex_ctl_reg;
> +
> +	int			phy_count;
> +	struct phy		**phy;
> +
> +	struct dentry		*debugfs;
> +};

> +static void apply_bad_link_workaround(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct tegra_pcie_dw *pcie = dw_pcie_to_tegra_pcie(pci);
> +	u16 val;
> +
> +	/*
> +	 * NOTE:- Since this scenario is uncommon and link as
> +	 * such is not stable anyway, not waiting to confirm
> +	 * if link is really transiting to Gen-2 speed

s/transiting/transitioning/

I think there are other uses of "transit" when you mean "transition".

> +static int tegra_pcie_dw_rd_own_conf(struct pcie_port *pp, int where, int size,
> +				     u32 *val)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	/*
> +	 * This is an endpoint mode specific register happen to appear even
> +	 * when controller is operating in root port mode and system hangs
> +	 * when it is accessed with link being in ASPM-L1 state.
> +	 * So skip accessing it altogether
> +	 */
> +	if (where == PORT_LOGIC_MSIX_DOORBELL) {
> +		*val = 0x00000000;
> +		return PCIBIOS_SUCCESSFUL;
> +	} else {
> +		return dw_pcie_read(pci->dbi_base + where, size, val);
> +	}
> +}
> +
> +static int tegra_pcie_dw_wr_own_conf(struct pcie_port *pp, int where, int size,
> +				     u32 val)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +
> +	/* This is EP specific register and system hangs when it is
> +	 * accessed with link being in ASPM-L1 state.
> +	 * So skip accessing it altogether
> +	 */
> +	if (where == PORT_LOGIC_MSIX_DOORBELL)
> +		return PCIBIOS_SUCCESSFUL;
> +	else
> +		return dw_pcie_write(pci->dbi_base + where, size, val);

These two functions are almost identical and they could look more
similar.  This one has the wrong multi-line comment style, uses "EP"
instead of "endpoint", etc.  Use this style for the "if" since the
first case is really an error case:

  if (where == PORT_LOGIC_MSIX_DOORBELL) {
    ...
    return ...;
  }

  return dw_pcie_...();

> +static int tegra_pcie_dw_host_init(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct tegra_pcie_dw *pcie = dw_pcie_to_tegra_pcie(pci);
> +	int count = 200;
> +	u32 val, tmp, offset;
> +	u16 val_w;
> +
> +#if defined(CONFIG_PCIEASPM)
> +	pcie->cfg_link_cap_l1sub =
> +		dw_pcie_find_ext_capability(pci, PCI_EXT_CAP_ID_L1SS) +
> +		PCI_L1SS_CAP;
> +#endif
> +	val = dw_pcie_readl_dbi(pci, PCI_IO_BASE);
> +	val &= ~(IO_BASE_IO_DECODE | IO_BASE_IO_DECODE_BIT8);
> +	dw_pcie_writel_dbi(pci, PCI_IO_BASE, val);
> +
> +	val = dw_pcie_readl_dbi(pci, PCI_PREF_MEMORY_BASE);
> +	val |= CFG_PREF_MEM_LIMIT_BASE_MEM_DECODE;
> +	val |= CFG_PREF_MEM_LIMIT_BASE_MEM_LIMIT_DECODE;
> +	dw_pcie_writel_dbi(pci, PCI_PREF_MEMORY_BASE, val);
> +
> +	dw_pcie_writel_dbi(pci, PCI_BASE_ADDRESS_0, 0);
> +
> +	/* Configure FTS */
> +	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL);
> +	val &= ~(N_FTS_MASK << N_FTS_SHIFT);
> +	val |= N_FTS_VAL << N_FTS_SHIFT;
> +	dw_pcie_writel_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL, val);
> +
> +	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_GEN2_CTRL);
> +	val &= ~FTS_MASK;
> +	val |= FTS_VAL;
> +	dw_pcie_writel_dbi(pci, PORT_LOGIC_GEN2_CTRL, val);
> +
> +	/* Enable as 0xFFFF0001 response for CRS */
> +	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT);
> +	val &= ~(AMBA_ERROR_RESPONSE_CRS_MASK << AMBA_ERROR_RESPONSE_CRS_SHIFT);
> +	val |= (AMBA_ERROR_RESPONSE_CRS_OKAY_FFFF0001 <<
> +		AMBA_ERROR_RESPONSE_CRS_SHIFT);
> +	dw_pcie_writel_dbi(pci, PORT_LOGIC_AMBA_ERROR_RESPONSE_DEFAULT, val);
> +
> +	/* Set MPS to 256 in DEV_CTL */
> +	val = dw_pcie_readl_dbi(pci, CFG_DEV_STATUS_CONTROL);
> +	val &= ~PCI_EXP_DEVCTL_PAYLOAD;
> +	val |= (1 << CFG_DEV_STATUS_CONTROL_MPS_SHIFT);
> +	dw_pcie_writel_dbi(pci, CFG_DEV_STATUS_CONTROL, val);
> +
> +	/* Configure Max Speed from DT */
> +	val = dw_pcie_readl_dbi(pci, CFG_LINK_CAP);
> +	val &= ~PCI_EXP_LNKCAP_SLS;
> +	val |= pcie->max_speed;
> +	dw_pcie_writel_dbi(pci, CFG_LINK_CAP, val);
> +
> +	val = dw_pcie_readw_dbi(pci, CFG_LINK_CONTROL_2);
> +	val &= ~PCI_EXP_LNKCTL2_TLS;
> +	val |= pcie->init_speed;
> +	dw_pcie_writew_dbi(pci, CFG_LINK_CONTROL_2, val);
> +
> +	/* Configure Max lane width from DT */
> +	val = dw_pcie_readl_dbi(pci, CFG_LINK_CAP);
> +	val &= ~PCI_EXP_LNKCAP_MLW;
> +	val |= (pcie->num_lanes << PCI_EXP_LNKSTA_NLW_SHIFT);
> +	dw_pcie_writel_dbi(pci, CFG_LINK_CAP, val);
> +
> +	config_gen3_gen4_eq_presets(pcie);
> +
> +#if defined(CONFIG_PCIEASPM)
> +	/* Enable ASPM counters */
> +	val = EVENT_COUNTER_ENABLE_ALL << EVENT_COUNTER_ENABLE_SHIFT;
> +	val |= EVENT_COUNTER_GROUP_5 << EVENT_COUNTER_GROUP_SEL_SHIFT;
> +	dw_pcie_writel_dbi(pci, pcie->event_cntr_ctrl, val);
> +
> +	/* Program T_cmrt and T_pwr_on values */
> +	val = dw_pcie_readl_dbi(pci, pcie->cfg_link_cap_l1sub);
> +	val &= ~(PCI_L1SS_CAP_CM_RESTORE_TIME | PCI_L1SS_CAP_P_PWR_ON_VALUE);
> +	val |= (pcie->aspm_cmrt << PCI_L1SS_CAP_CM_RTM_SHIFT);
> +	val |= (pcie->aspm_pwr_on_t << PCI_L1SS_CAP_PWRN_VAL_SHIFT);
> +	dw_pcie_writel_dbi(pci, pcie->cfg_link_cap_l1sub, val);
> +
> +	/* Program L0s and L1 entrance latencies */
> +	val = dw_pcie_readl_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL);
> +	val &= ~L0S_ENTRANCE_LAT_MASK;
> +	val |= (pcie->aspm_l0s_enter_lat << L0S_ENTRANCE_LAT_SHIFT);
> +	val |= ENTER_ASPM;
> +	dw_pcie_writel_dbi(pci, PORT_LOGIC_ACK_F_ASPM_CTRL, val);
> +
> +	/* Program what ASPM states sould get advertised */

s/sould/should/

> +	if (pcie->disabled_aspm_states & 0x1)
> +		disable_aspm_l0s(pcie); /* Disable L0s */
> +	if (pcie->disabled_aspm_states & 0x2) {
> +		disable_aspm_l10(pcie); /* Disable L1 */
> +		disable_aspm_l11(pcie); /* Disable L1.1 */
> +		disable_aspm_l12(pcie); /* Disable L1.2 */
> +	}
> +	if (pcie->disabled_aspm_states & 0x4)
> +		disable_aspm_l11(pcie); /* Disable L1.1 */
> +	if (pcie->disabled_aspm_states & 0x8)
> +		disable_aspm_l12(pcie); /* Disable L1.2 */
> +#endif
> +	val = dw_pcie_readl_dbi(pci, GEN3_RELATED_OFF);
> +	val &= ~GEN3_RELATED_OFF_GEN3_ZRXDC_NONCOMPL;
> +	dw_pcie_writel_dbi(pci, GEN3_RELATED_OFF, val);
> +
> +	if (pcie->update_fc_fixup) {
> +		val = dw_pcie_readl_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF);
> +		val |= 0x1 << CFG_TIMER_CTRL_ACK_NAK_SHIFT;
> +		dw_pcie_writel_dbi(pci, CFG_TIMER_CTRL_MAX_FUNC_NUM_OFF, val);
> +	}
> +
> +	/* CDM check enable */
> +	if (pcie->cdm_check) {
> +		val = dw_pcie_readl_dbi(pci,
> +					PORT_LOGIC_PL_CHK_REG_CONTROL_STATUS);
> +		val |= PORT_LOGIC_PL_CHK_REG_CHK_REG_CONTINUOUS;
> +		val |= PORT_LOGIC_PL_CHK_REG_CHK_REG_START;
> +		dw_pcie_writel_dbi(pci, PORT_LOGIC_PL_CHK_REG_CONTROL_STATUS,
> +				   val);
> +	}
> +
> +	dw_pcie_setup_rc(pp);
> +
> +	clk_set_rate(pcie->core_clk, GEN4_CORE_CLK_FREQ);
> +
> +	/* assert RST */
> +	val = readl(pcie->appl_base + APPL_PINMUX);
> +	val &= ~APPL_PINMUX_PEX_RST;
> +	writel(val, pcie->appl_base + APPL_PINMUX);
> +
> +	usleep_range(100, 200);
> +
> +	/* enable LTSSM */
> +	val = readl(pcie->appl_base + APPL_CTRL);
> +	val |= APPL_CTRL_LTSSM_EN;
> +	writel(val, pcie->appl_base + APPL_CTRL);
> +
> +	/* de-assert RST */
> +	val = readl(pcie->appl_base + APPL_PINMUX);
> +	val |= APPL_PINMUX_PEX_RST;
> +	writel(val, pcie->appl_base + APPL_PINMUX);
> +
> +	msleep(100);
> +
> +	val_w = dw_pcie_readw_dbi(pci, CFG_LINK_STATUS);
> +	while (!(val_w & PCI_EXP_LNKSTA_DLLLA)) {
> +		if (!count) {
> +			val = readl(pcie->appl_base + APPL_DEBUG);
> +			val &= APPL_DEBUG_LTSSM_STATE_MASK;
> +			val >>= APPL_DEBUG_LTSSM_STATE_SHIFT;
> +			tmp = readl(pcie->appl_base + APPL_LINK_STATUS);
> +			tmp &= APPL_LINK_STATUS_RDLH_LINK_UP;
> +			if (val == 0x11 && !tmp) {
> +				dev_info(pci->dev, "link is down in DLL");
> +				dev_info(pci->dev,
> +					 "trying again with DLFE disabled\n");
> +				/* disable LTSSM */
> +				val = readl(pcie->appl_base + APPL_CTRL);
> +				val &= ~APPL_CTRL_LTSSM_EN;
> +				writel(val, pcie->appl_base + APPL_CTRL);
> +
> +				reset_control_assert(pcie->core_rst);
> +				reset_control_deassert(pcie->core_rst);
> +
> +				offset =
> +				dw_pcie_find_ext_capability(pci,
> +							    PCI_EXT_CAP_ID_DLF)
> +				+ PCI_DLF_CAP;

This capability offset doesn't change, does it?  Could it be computed
outside the loop?

> +				val = dw_pcie_readl_dbi(pci, offset);
> +				val &= ~DL_FEATURE_EXCHANGE_EN;
> +				dw_pcie_writel_dbi(pci, offset, val);
> +
> +				tegra_pcie_dw_host_init(&pcie->pci.pp);

This looks like some sort of "wait for link up" retry loop, but a
recursive call seems a little unusual.  My 5 second analysis is that
the loop could run this 200 times, and you sure don't want the
possibility of a 200-deep call chain.  Is there way to split out the
host init from the link-up polling?

> +				return 0;
> +			}
> +			dev_info(pci->dev, "link is down\n");
> +			return 0;
> +		}
> +		dev_dbg(pci->dev, "polling for link up\n");
> +		usleep_range(1000, 2000);
> +		val_w = dw_pcie_readw_dbi(pci, CFG_LINK_STATUS);
> +		count--;
> +	}
> +	dev_info(pci->dev, "link is up\n");
> +
> +	tegra_pcie_enable_interrupts(pp);
> +
> +	return 0;
> +}

> +static void tegra_pcie_dw_scan_bus(struct pcie_port *pp)
> +{
> +	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> +	struct tegra_pcie_dw *pcie = dw_pcie_to_tegra_pcie(pci);
> +	u32 speed;
> +
> +	if (!tegra_pcie_dw_link_up(pci))
> +		return;
> +
> +	speed = (dw_pcie_readw_dbi(pci, CFG_LINK_STATUS) & PCI_EXP_LNKSTA_CLS);
> +	clk_set_rate(pcie->core_clk, pcie_gen_freq[speed - 1]);

I don't understand what's happening here.  This is named
tegra_pcie_dw_scan_bus(), but it doesn't actually scan anything.
Maybe it's just a bad name for the dw_pcie_host_ops hook
(ks_pcie_v3_65_scan_bus() is the only other .scan_bus()
implementation, and it doesn't scan anything either).

dw_pcie_host_init() calls pci_scan_root_bus_bridge(), which actually
*does* scan the bus, but it does it before calling
pp->ops->scan_bus().  I'd say by the time we get to
pci_scan_root_bus_bridge(), the device-specific init should be all
done and we should be using only generic PCI core interfaces.

Maybe this stuff could/should be done in the ->host_init() hook?  The
code between ->host_init() and ->scan_bus() is all generic code with
no device-specific stuff, so I don't know why we need both hooks.

> +static int tegra_pcie_enable_phy(struct tegra_pcie_dw *pcie)
> +{
> +	int phy_count = pcie->phy_count;
> +	int ret;
> +	int i;
> +
> +	for (i = 0; i < phy_count; i++) {
> +		ret = phy_init(pcie->phy[i]);
> +		if (ret < 0)
> +			goto err_phy_init;
> +
> +		ret = phy_power_on(pcie->phy[i]);
> +		if (ret < 0) {
> +			phy_exit(pcie->phy[i]);
> +			goto err_phy_power_on;
> +		}
> +	}
> +
> +	return 0;
> +
> +	while (i >= 0) {
> +		phy_power_off(pcie->phy[i]);
> +err_phy_power_on:
> +		phy_exit(pcie->phy[i]);
> +err_phy_init:
> +		i--;
> +	}

Wow, jumping into the middle of that loop is clever ;)  Can't decide
what I think of it, but it's certainly clever!

> +	return ret;
> +}
> +
> +static int tegra_pcie_dw_parse_dt(struct tegra_pcie_dw *pcie)
> +{
> +	struct device_node *np = pcie->dev->of_node;
> +	int ret;
> +
> +#if defined(CONFIG_PCIEASPM)
> +	ret = of_property_read_u32(np, "nvidia,event-cntr-ctrl",
> +				   &pcie->event_cntr_ctrl);
> +	if (ret < 0) {
> +		dev_err(pcie->dev, "fail to read event-cntr-ctrl: %d\n", ret);
> +		return ret;
> +	}

The fact that you return error here if DT lacks the
"nvidia,event-cntr-ctrl" property, but only if CONFIG_PCIEASPM=y,
means that you have a revlock between the DT and the kernel: if you
update the kernel to enable CONFIG_PCIEASPM, you may also have to
update your DT.

Maybe that's OK, but I think it'd be nicer if you always required the
presence of these properties, even if you only actually *use* them
when CONFIG_PCIEASPM=y.

> +static int tegra_pcie_dw_probe(struct platform_device *pdev)
> +{
> +	struct tegra_pcie_dw *pcie;
> +	struct pcie_port *pp;
> +	struct dw_pcie *pci;
> +	struct phy **phy;
> +	struct resource	*dbi_res;
> +	struct resource	*atu_dma_res;
> +	const struct of_device_id *match;
> +	const struct tegra_pcie_of_data *data;
> +	char *name;
> +	int ret, i;
> +
> +	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> +	if (!pcie)
> +		return -ENOMEM;
> +
> +	pci = &pcie->pci;
> +	pci->dev = &pdev->dev;
> +	pci->ops = &tegra_dw_pcie_ops;
> +	pp = &pci->pp;
> +	pcie->dev = &pdev->dev;
> +
> +	match = of_match_device(of_match_ptr(tegra_pcie_dw_of_match),
> +				&pdev->dev);
> +	if (!match)
> +		return -EINVAL;

Logically could be the first thing in the function since it doesn't
depend on anything.

> +	data = (struct tegra_pcie_of_data *)match->data;
> +	pcie->mode = (enum dw_pcie_device_mode)data->mode;
> +
> +	ret = tegra_pcie_dw_parse_dt(pcie);
> +	if (ret < 0) {
> +		dev_err(pcie->dev, "device tree parsing failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (gpio_is_valid(pcie->pex_wake)) {
> +		ret = devm_gpio_request(pcie->dev, pcie->pex_wake,
> +					"pcie_wake");
> +		if (ret < 0) {
> +			if (ret == -EBUSY) {
> +				dev_err(pcie->dev,
> +					"pex_wake already in use\n");
> +				pcie->pex_wake = -EINVAL;

This looks strange.  "pex_wake == -EINVAL" doesn't look right, and
you're about to pass it to gpio_direction_input(), which looks wrong.

> +			} else {
> +				dev_err(pcie->dev,
> +					"pcie_wake gpio_request failed %d\n",
> +					ret);
> +				return ret;
> +			}
> +		}
> +
> +		ret = gpio_direction_input(pcie->pex_wake);
> +		if (ret < 0) {
> +			dev_err(pcie->dev,
> +				"setting pcie_wake input direction failed %d\n",
> +				ret);
> +			return ret;
> +		}
> +		device_init_wakeup(pcie->dev, true);
> +	}
> +
> +	pcie->pex_ctl_reg = devm_regulator_get(&pdev->dev, "vddio-pex-ctl");
> +	if (IS_ERR(pcie->pex_ctl_reg)) {
> +		dev_err(&pdev->dev, "fail to get regulator: %ld\n",
> +			PTR_ERR(pcie->pex_ctl_reg));
> +		return PTR_ERR(pcie->pex_ctl_reg);
> +	}
> +
> +	pcie->core_clk = devm_clk_get(&pdev->dev, "core_clk");
> +	if (IS_ERR(pcie->core_clk)) {
> +		dev_err(&pdev->dev, "Failed to get core clock\n");
> +		return PTR_ERR(pcie->core_clk);
> +	}
> +
> +	pcie->appl_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						      "appl");
> +	if (!pcie->appl_res) {
> +		dev_err(&pdev->dev, "missing appl space\n");
> +		return PTR_ERR(pcie->appl_res);
> +	}
> +	pcie->appl_base = devm_ioremap_resource(&pdev->dev, pcie->appl_res);
> +	if (IS_ERR(pcie->appl_base)) {
> +		dev_err(&pdev->dev, "mapping appl space failed\n");
> +		return PTR_ERR(pcie->appl_base);
> +	}
> +
> +	pcie->core_apb_rst = devm_reset_control_get(pcie->dev, "core_apb_rst");
> +	if (IS_ERR(pcie->core_apb_rst)) {
> +		dev_err(pcie->dev, "PCIE : core_apb_rst reset is missing\n");

This error message looks different from the others ("PCIE :" prefix).

> +		return PTR_ERR(pcie->core_apb_rst);
> +	}
> +
> +	phy = devm_kcalloc(pcie->dev, pcie->phy_count, sizeof(*phy),
> +			   GFP_KERNEL);
> +	if (!phy)
> +		return PTR_ERR(phy);
> +
> +	for (i = 0; i < pcie->phy_count; i++) {
> +		name = kasprintf(GFP_KERNEL, "pcie-p2u-%u", i);
> +		if (!name) {
> +			dev_err(pcie->dev, "failed to create p2u string\n");
> +			return -ENOMEM;
> +		}
> +		phy[i] = devm_phy_get(pcie->dev, name);
> +		kfree(name);
> +		if (IS_ERR(phy[i])) {
> +			ret = PTR_ERR(phy[i]);
> +			dev_err(pcie->dev, "phy_get error: %d\n", ret);
> +			return ret;
> +		}
> +	}
> +
> +	pcie->phy = phy;
> +
> +	dbi_res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dbi");
> +	if (!dbi_res) {
> +		dev_err(&pdev->dev, "missing config space\n");
> +		return PTR_ERR(dbi_res);
> +	}
> +	pcie->dbi_res = dbi_res;
> +
> +	pci->dbi_base = devm_ioremap_resource(&pdev->dev, dbi_res);
> +	if (IS_ERR(pci->dbi_base)) {
> +		dev_err(&pdev->dev, "mapping dbi space failed\n");
> +		return PTR_ERR(pci->dbi_base);
> +	}
> +
> +	/* Tegra HW locates DBI2 at a fixed offset from DBI */
> +	pci->dbi_base2 = pci->dbi_base + 0x1000;
> +
> +	atu_dma_res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   "atu_dma");
> +	if (!atu_dma_res) {
> +		dev_err(&pdev->dev, "missing atu_dma space\n");
> +		return PTR_ERR(atu_dma_res);
> +	}
> +	pcie->atu_dma_res = atu_dma_res;
> +	pci->atu_base = devm_ioremap_resource(&pdev->dev, atu_dma_res);
> +	if (IS_ERR(pci->atu_base)) {
> +		dev_err(&pdev->dev, "mapping atu space failed\n");
> +		return PTR_ERR(pci->atu_base);
> +	}
> +
> +	pcie->core_rst = devm_reset_control_get(pcie->dev, "core_rst");
> +	if (IS_ERR(pcie->core_rst)) {
> +		dev_err(pcie->dev, "PCIE : core_rst reset is missing\n");

Different message format again.

> +		return PTR_ERR(pcie->core_rst);
> +	}
> +
> +	pp->irq = platform_get_irq_byname(pdev, "intr");
> +	if (!pp->irq) {
> +		dev_err(pcie->dev, "failed to get intr interrupt\n");
> +		return -ENODEV;
> +	}
> +
> +	ret = devm_request_irq(&pdev->dev, pp->irq, tegra_pcie_irq_handler,
> +			       IRQF_SHARED, "tegra-pcie-intr", pcie);
> +	if (ret) {
> +		dev_err(pcie->dev, "failed to request \"intr\" irq\n");

It'd be nice to include the actual IRQ, i.e., "IRQ %d", pp->irq.

> +		return ret;
> +	}
> +
> +	platform_set_drvdata(pdev, pcie);
> +
> +	if (pcie->mode == DW_PCIE_RC_TYPE) {
> +		ret = tegra_pcie_config_rp(pcie);
> +		if (ret == -ENOMEDIUM)
> +			ret = 0;
> +	}
> +
> +	return ret;
> +}

> +static void tegra_pcie_downstream_dev_to_D0(struct tegra_pcie_dw *pcie)
> +{
> +	struct pci_dev *pdev = NULL;

Unnecessary initialization.

> +	struct pci_bus *child;
> +	struct pcie_port *pp = &pcie->pci.pp;
> +
> +	list_for_each_entry(child, &pp->bus->children, node) {
> +		/* Bring downstream devices to D0 if they are not already in */
> +		if (child->parent == pp->bus) {
> +			pdev = pci_get_slot(child, PCI_DEVFN(0, 0));
> +			pci_dev_put(pdev);
> +			if (!pdev)
> +				break;

I don't really like this dance with iterating over the bus children,
comparing parent to pp->bus, pci_get_slot(), pci_dev_put(), etc.

I guess the idea is to bring only the directly-downstream devices to
D0, not to do it for things deeper in the hierarchy?

Is this some Tegra-specific wrinkle?  I don't think other drivers do
this.

I see that an earlier patch added "bus" to struct pcie_port.  I think
it would be better to somehow connect to the pci_host_bridge struct.
Several other drivers already do this; see uses of
pci_host_bridge_from_priv().

That would give you the bus, as well as flags like no_ext_tags,
native_aer, etc, which this driver, being a host bridge driver that's
responsible for this part of the firmware/OS interface, may
conceivably need.

Rather than pci_get_slot(), couldn't you iterate over bus->devices and
just skip the non-PCI_DEVFN(0, 0) devices?

> +
> +			if (pci_set_power_state(pdev, PCI_D0))
> +				dev_err(pcie->dev, "D0 transition failed\n");
> +		}
> +	}
> +}

> +static int tegra_pcie_dw_remove(struct platform_device *pdev)
> +{
> +	struct tegra_pcie_dw *pcie = platform_get_drvdata(pdev);
> +
> +	if (pcie->mode == DW_PCIE_RC_TYPE) {

Return early if it's not RC and unindent the rest of the function.

> +		if (!pcie->link_state && pcie->power_down_en)
> +			return 0;
> +
> +		debugfs_remove_recursive(pcie->debugfs);
> +		pm_runtime_put_sync(pcie->dev);
> +		pm_runtime_disable(pcie->dev);
> +	}
> +
> +	return 0;
> +}

> +static int tegra_pcie_dw_runtime_suspend(struct device *dev)
> +{
> +	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> +
> +	tegra_pcie_downstream_dev_to_D0(pcie);
> +
> +	pci_stop_root_bus(pcie->pci.pp.bus);
> +	pci_remove_root_bus(pcie->pci.pp.bus);

Why are you calling these?  No other drivers do this except in their
.remove() methods.  Is there something special about Tegra, or is this
something the other drivers *should* be doing?

> +	tegra_pcie_dw_pme_turnoff(pcie);
> +
> +	reset_control_assert(pcie->core_rst);
> +	tegra_pcie_disable_phy(pcie);
> +	reset_control_assert(pcie->core_apb_rst);
> +	clk_disable_unprepare(pcie->core_clk);
> +	regulator_disable(pcie->pex_ctl_reg);
> +	config_plat_gpio(pcie, 0);
> +
> +	if (pcie->cid != CTRL_5)
> +		tegra_pcie_bpmp_set_ctrl_state(pcie, false);
> +
> +	return 0;
> +}
> +
> +static int tegra_pcie_dw_runtime_resume(struct device *dev)
> +{
> +	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> +	struct dw_pcie *pci = &pcie->pci;
> +	struct pcie_port *pp = &pci->pp;
> +	int ret = 0;
> +
> +	ret = tegra_pcie_config_controller(pcie, false);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* program to use MPS of 256 whereever possible */

s/whereever/wherever/

> +	pcie_bus_config = PCIE_BUS_SAFE;
> +
> +	pp->root_bus_nr = -1;
> +	pp->ops = &tegra_pcie_dw_host_ops;
> +
> +	/* Disable MSI interrupts for PME messages */

Superfluous comment; it repeats the function name.

> +static int tegra_pcie_dw_suspend_noirq(struct device *dev)
> +{
> +	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> +	int ret = 0;
> +
> +	if (!pcie->link_state)
> +		return 0;
> +
> +	/* save MSI interrutp vector*/

s/interrutp/interrupt/
s/vector/vector /

> +static int tegra_pcie_dw_resume_noirq(struct device *dev)
> +{
> +	struct tegra_pcie_dw *pcie = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (!pcie->link_state)
> +		return 0;
> +
> +	if (gpio_is_valid(pcie->pex_wake) && device_may_wakeup(dev)) {
> +		ret = disable_irq_wake(gpio_to_irq(pcie->pex_wake));
> +		if (ret < 0)
> +			dev_err(dev, "disable wake irq failed: %d\n", ret);
> +	}
> +
> +	ret = tegra_pcie_config_controller(pcie, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tegra_pcie_dw_host_init(&pcie->pci.pp);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to init host: %d\n", ret);
> +		goto fail_host_init;
> +	}
> +
> +	/* restore MSI interrutp vector*/

s/interrutp/interrupt/
s/vector/vector /

> +static void tegra_pcie_dw_shutdown(struct platform_device *pdev)
> +{
> +	struct tegra_pcie_dw *pcie = platform_get_drvdata(pdev);
> +
> +	if (pcie->mode == DW_PCIE_RC_TYPE) {

  if (pcie->mode != DW_PCIE_RC_TYPE)
    return;

Then you can unindent the whole function.

> +		if (!pcie->link_state && pcie->power_down_en)
> +			return;
> +
> +		debugfs_remove_recursive(pcie->debugfs);
> +		tegra_pcie_downstream_dev_to_D0(pcie);
> +
> +		/* Disable interrupts */

Superfluous comment.

> +		disable_irq(pcie->pci.pp.irq);

Powered by blists - more mailing lists