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 linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
