Sean, Several more comments. Shawn, A question about a clk_set_parent inside. On Thu, Sep 12, 2013 at 04:03:31AM +0000, Sean Cross wrote: > + > +#define IMX6_ENTER_RESET 0 > +#define IMX6_EXIT_RESET 1 These are unused > + > +#define IMX6_POWER_OFF 0 > +#define IMX6_POWER_ON 1 unused aswell > +static int pcie_phy_poll_ack(void __iomem *dbi_base, int max_iterations, > + int exp_val) > +{ > + u32 val; > + u32 wait_counter = 0; > + > + do { > + val = readl(dbi_base + PCIE_PHY_STAT); > + val = (val >> PCIE_PHY_STAT_ACK_LOC) & 0x1; > + wait_counter++; > + } while ((wait_counter < max_iterations) && (val != exp_val)); may_iterations is never something else than 100 so you should drop the argument. Also I would prefer a real timeout here rather than a counter. > + > + if (val != exp_val) > + return -1; By saying "A negative error value" I actually meant one of include/uapi/asm-generic/errno* > +static int pcie_phy_write(void __iomem *dbi_base, int addr, int data) > +{ > + u32 var; > + > + /* write addr */ > + /* cap addr */ > + if (pcie_phy_wait_ack(dbi_base, addr)) > + return -1; > + > + var = data << PCIE_PHY_CTRL_DATA_LOC; > + writel(var, dbi_base + PCIE_PHY_CTRL); > + > + /* capture data */ > + var |= (0x1 << PCIE_PHY_CTRL_CAP_DAT_LOC); > + writel(var, dbi_base + PCIE_PHY_CTRL); > + > + /* wait for ack */ Drop this comment. > + if (pcie_phy_poll_ack(dbi_base, 100, 1)) > + return -1; > + ... > +static int imx6_pcie_deassert_core_reset(struct pcie_port *pp) > +{ > + int ret; > + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp); > + > + if (imx6_pcie->power_on_gpio >= 0) > + gpio_set_value(imx6_pcie->power_on_gpio, IMX6_POWER_ON); in probe you used gpio_is_valid(). Why not here? > + > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > + IMX6Q_GPR1_PCIE_TEST_PD, 0 << 18); > + regmap_update_bits(imx6_pcie->iomuxc_gpr, IOMUXC_GPR1, > + IMX6Q_GPR1_PCIE_REF_CLK_EN, 1 << 16); > + > + ret = clk_set_parent(imx6_pcie->lvds1_sel, imx6_pcie->sata_ref); > + if (ret) { > + dev_err(pp->dev, "unable to set lvds1_gate parent\n"); > + goto err_set_parent; > + } I'm unsure this should be done here. Shawn, should we do this in SoC code? > + > + ret = clk_prepare_enable(imx6_pcie->pcie_ref_125m); > + if (ret) { > + dev_err(pp->dev, "unable to enable pcie_ref_125m\n"); > + goto err_pcie_ref; > + } > + > + ret = clk_prepare_enable(imx6_pcie->lvds1_gate); > + if (ret) { > + dev_err(pp->dev, "unable to enable lvds1_gate\n"); > + goto err_lvds1_gate; > + } > + > + ret = clk_prepare_enable(imx6_pcie->pcie_axi); > + if (ret) { > + dev_err(pp->dev, "unable to enable pcie_axi\n"); > + goto err_pcie_axi; > + } > + > + /* allow the clocks to stabilize */ > + usleep_range(100, 200); > + > + return 0; > + > +err_pcie_axi: > + clk_disable(imx6_pcie->lvds1_gate); The counterpart of clk_prepare_enable is clk_disable_unprepare. > + return; > + } > + } > + > + return; unnecessary return > + > + imx6_pcie->power_on_gpio = of_get_named_gpio(np, "power-on-gpio", 0); > + if (gpio_is_valid(imx6_pcie->power_on_gpio)) > + devm_gpio_request_one(&pdev->dev, imx6_pcie->power_on_gpio, > + GPIOF_OUT_INIT_LOW, > + "PCIe power enable"); devm_gpio_request_one can still fail. > + > + imx6_pcie->wake_up_gpio = of_get_named_gpio(np, "wake-up-gpio", 0); > + if (gpio_is_valid(imx6_pcie->wake_up_gpio)) > + devm_gpio_request_one(&pdev->dev, imx6_pcie->wake_up_gpio, > + GPIOF_IN, > + "PCIe wake up"); > + > + imx6_pcie->disable_gpio = of_get_named_gpio(np, "disable-gpio", 0); > + if (gpio_is_valid(imx6_pcie->disable_gpio)) > + devm_gpio_request_one(&pdev->dev, imx6_pcie->disable_gpio, > + GPIOF_OUT_INIT_HIGH, > + "PCIe disable endpoint"); > + > + /* Fetch clocks */ > + imx6_pcie->lvds1_sel = clk_get(&pdev->dev, "lvds1_sel"); > + if (IS_ERR(imx6_pcie->lvds1_sel)) { > + dev_err(&pdev->dev, > + "lvds1_sel clock missing or invalid\n"); > + ret = PTR_ERR(imx6_pcie->lvds1_sel); > + goto err; > + } > + > + imx6_pcie->lvds1_gate = clk_get(&pdev->dev, "lvds1_gate"); > + if (IS_ERR(imx6_pcie->lvds1_gate)) { > + dev_err(&pdev->dev, > + "lvds1_gate clock select missing or invalid\n"); > + ret = PTR_ERR(imx6_pcie->lvds1_gate); > + goto err; > + } devm_clk_get(). Otherwise you need to release the clock un unregister/probe failure. > +static int __exit imx6_pcie_remove(struct platform_device *pdev) > +{ > + struct imx6_pcie *imx6_pcie = platform_get_drvdata(pdev); > + > + clk_disable_unprepare(imx6_pcie->pcie_axi); > + clk_disable_unprepare(imx6_pcie->lvds1_gate); > + clk_disable_unprepare(imx6_pcie->pcie_ref_125m); Still no unregister for the pcie controller? > + > + return 0; > +} > + Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html