Hi Sean,
Several comments inline.
On Tue, Sep 10, 2013 at 04:11:15AM +0000, Sean Cross wrote:
> +
> +#define PHY_RX_OVRD_IN_LO 0x1005
> +#define PHY_RX_OVRD_IN_LO_RX_DATA_EN (1<<5)
> +#define PHY_RX_OVRD_IN_LO_RX_PLL_EN (1<<3)
> +
> +
> +
Remove all duplicate empty lines please.
> +static void imx_iomuxc_clrset(struct imx6_pcie *imx6_pcie,
> + u32 mask, u32 val, u32 reg)
> +{
> + u32 tmp;
> + regmap_read(imx6_pcie->iomuxc_gpr, reg, &tmp);
> + tmp &= ~mask;
> + tmp |= (val & mask);
> + regmap_write(imx6_pcie->iomuxc_gpr, reg, tmp);
> +}
We have regmap_update_bits. Use it.
> +
> +
> +static int pcie_phy_ack_polling(void __iomem *dbi_base, int max_iterations,
> + int exp_val)
> +{
> + u32 temp_rd_data;
> + u32 wait_counter = 0;
> +
> + do {
> + temp_rd_data = readl(dbi_base + PCIE_PHY_STAT);
> + temp_rd_data = (temp_rd_data >> PCIE_PHY_STAT_ACK_LOC) & 0x1;
> + wait_counter++;
> + } while ((wait_counter < max_iterations) && (temp_rd_data != exp_val));
> +
> + if (temp_rd_data != exp_val)
> + return 0;
> +
> + return 1;
> +}
You should return 0 for success and a negative error value on error.
> +
> +
> +static int pcie_phy_cap_addr(void __iomem *dbi_base, int addr)
> +{
> + u32 temp_wr_data;
> +
> + /* write addr */
> + temp_wr_data = addr << PCIE_PHY_CTRL_DATA_LOC;
> + writel(temp_wr_data, dbi_base + PCIE_PHY_CTRL);
> +
> + /* capture addr */
> + temp_wr_data |= (0x1 << PCIE_PHY_CTRL_CAP_ADR_LOC);
> + writel(temp_wr_data, dbi_base + PCIE_PHY_CTRL);
> +
> + /* wait for ack */
> + if (!pcie_phy_ack_polling(dbi_base, 100, 1))
> + return 0;
Drop these comments and give the function a more meaningful name, like
pcie_phy_wait_ack().
> +
> + /* deassert cap addr */
> + temp_wr_data = addr << PCIE_PHY_CTRL_DATA_LOC;
> + writel(temp_wr_data, dbi_base + PCIE_PHY_CTRL);
> +
> + /* wait for ack de-assetion */
> + if (!pcie_phy_ack_polling(dbi_base, 100, 0))
> + return 0;
> +
> + return 1;
> +}
> +
> +
> +
> +/* Read from the 16-bt PCIe PHY control registers (not memory-mapped) */
> +static int pcie_phy_read(void __iomem *dbi_base, int addr , int *data)
> +{
> + u32 temp_rd_data, temp_wr_data;
no_need_for_so_long_var_names. 'val' or similar will do it.
> +
> + /* cap addr */
> + if (!pcie_phy_cap_addr(dbi_base, addr))
> + return 0;
> +
> + /* assert rd signal */
> + temp_wr_data = 0x1 << PCIE_PHY_CTRL_RD_LOC;
> + writel(temp_wr_data, dbi_base + PCIE_PHY_CTRL);
> +
> + /* wait for ack */
> + if (!pcie_phy_ack_polling(dbi_base, 100, 1))
> + return 0;
> +
> + /* after got ack return data */
> + temp_rd_data = readl(dbi_base + PCIE_PHY_STAT);
> + *data = (temp_rd_data & (0xffff << PCIE_PHY_STAT_DATA_LOC)) ;
> +
> + /* deassert rd signal */
> + temp_wr_data = 0x0;
> + writel(temp_wr_data, dbi_base + PCIE_PHY_CTRL);
Why not writel(0x0, ...). It's shorter and easier to read.
> +
> + /* wait for ack de-assetion */
> + if (!pcie_phy_ack_polling(dbi_base, 100, 0))
> + return 0;
> +
> + 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);
> +
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR1_PCIE_TEST_PD,
> + 0 << 18, IOMUXC_GPR1);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR1_PCIE_REF_CLK_EN,
> + 1 << 16, IOMUXC_GPR1);
Indentation broken here.
> +
> + /* Enable clocks */
> + 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");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_ref_125m);
> + if (ret) {
> + dev_err(pp->dev, "unable to enable pcie_ref_125m\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->lvds1_gate);
> + if (ret) {
> + dev_err(pp->dev, "unable to enable lvds1_gate\n");
> + return ret;
> + }
> +
> + ret = clk_prepare_enable(imx6_pcie->pcie_axi);
> + if (ret) {
> + dev_err(pp->dev, "unable to enable pcie_axi\n");
> + return ret;
> + }
clock enable/disable must be balanced.
> +
> + /* allow the clocks to stabilize */
> + usleep_range(100, 200);
> +
> + return 0;
> +}
> +
> +static void imx6_pcie_init_phy(struct pcie_port *pp)
> +{
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
> +
> + /* FIXME the field name should be aligned to RM */
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR12_PCIE_CTL_2,
> + 0 << 10, IOMUXC_GPR12);
> +
> + /* configure constant input signal to the pcie ctrl and phy */
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR12_DEVICE_TYPE,
> + PCI_EXP_TYPE_ROOT_PORT << 12, IOMUXC_GPR12);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR12_LOS_LEVEL,
> + 9 << 4, IOMUXC_GPR12);
> +
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR8_TX_DEEMPH_GEN1,
> + 0 << 0, IOMUXC_GPR8);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR8_TX_DEEMPH_GEN2_3P5DB,
> + 0 << 6, IOMUXC_GPR8);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR8_TX_DEEMPH_GEN2_6DB,
> + 20 << 12, IOMUXC_GPR8);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR8_TX_SWING_FULL,
> + 127 << 18, IOMUXC_GPR8);
> + imx_iomuxc_clrset(imx6_pcie, IMX6Q_GPR8_TX_SWING_LOW,
> + 127 << 25, IOMUXC_GPR8);
> +}
> +
> +static int imx6_pcie_establish_link(struct pcie_port *pp)
> +{
> + int count = 0;
> + struct imx6_pcie *imx6_pcie = to_imx6_pcie(pp);
> +
> + /* assert reset signals */
> + imx6_pcie_assert_core_reset(pp);
Please drop
> +
> + /* initialize phy */
> + imx6_pcie_init_phy(pp);
all these
> +
> + /* de-assert core reset */
> + imx6_pcie_deassert_core_reset(pp);
useless
> +
> + /* setup root complex */
> + dw_pcie_setup_rc(pp);
comments. I mean when I read a book I usually don't wear a sign on my
head saying 'I'm reading a book'
> + /* From L0, initiate MAC entry to gen2 if EP/RC supports gen2.
> + * Wait 2ms (LTSSM timeout is 24ms, PHY lock is ~5us in gen2).
> + * If (MAC/LTSSM.state == Recovery.RcvrLock)
> + * && (PHY/rx_valid==0) then pulse PHY/rx_reset. Transition
> + * to gen2 is stuck
> + */
> + pcie_phy_read(pp->dbi_base, PCIE_PHY_RX_ASIC_OUT,
> + &rx_valid);
if (rx_valid & 0x1)
return 0;
> + ltssm = readl(pp->dbi_base + PCIE_PHY_DEBUG_R0) & 0x3F;
if (ltssm != 0xd)
return 0;
/* reset phy here */
> + if ((ltssm == 0x0D) && ((rx_valid & 0x01) == 0)) {
> + dev_err(pp->dev,
> + "transition to gen2 is stuck, reset PHY!\n");
> +
> + pcie_phy_read(pp->dbi_base,
> + PHY_RX_OVRD_IN_LO, &temp);
> + temp |= (PHY_RX_OVRD_IN_LO_RX_DATA_EN
> + | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> + pcie_phy_write(pp->dbi_base,
> + PHY_RX_OVRD_IN_LO, temp);
> +
> + usleep_range(2000, 3000);
> +
> + pcie_phy_read(pp->dbi_base,
> + PHY_RX_OVRD_IN_LO, &temp);
> + temp &= ~(PHY_RX_OVRD_IN_LO_RX_DATA_EN
> + | PHY_RX_OVRD_IN_LO_RX_PLL_EN);
> + pcie_phy_write(pp->dbi_base,
> + PHY_RX_OVRD_IN_LO, temp);
> + }
> + return 0;
> +}
> +
> +static void imx6_pcie_host_init(struct pcie_port *pp)
> +{
> + imx6_pcie_establish_link(pp);
> +}
rename imx6_pcie_establish_link to imx6_pcie_host_init and drop this
function.
> +static int __init imx6_pcie_probe(struct platform_device *pdev)
> +{
> + struct imx6_pcie *imx6_pcie;
> + struct pcie_port *pp;
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *dbi_base;
> + int ret;
> +
> + imx6_pcie = devm_kzalloc(&pdev->dev, sizeof(*imx6_pcie),
> + GFP_KERNEL);
> + if (!imx6_pcie) {
> + dev_err(&pdev->dev, "no memory for imx6 pcie\n");
Drop this message. You'll never see it anyway.
> + return -ENOMEM;
> + }
> +
...
> +
> + /* Fetch GPIOs */
> + imx6_pcie->reset_gpio = of_get_named_gpio(np, "reset-gpio", 0);
> + if (gpio_is_valid(imx6_pcie->reset_gpio))
> + devm_gpio_request_one(&pdev->dev, imx6_pcie->reset_gpio,
> + GPIOF_OUT_INIT_LOW,
> + "PCIe reset");
devm_gpio_request_one can fail.
> +fail_add_port:
> +fail_get_gpr:
> + clk_disable_unprepare(imx6_pcie->sata_ref);
> +fail_sata_ref_clk:
> + clk_disable_unprepare(imx6_pcie->pcie_axi);
> +fail_pcie_axi_clk:
> + clk_disable_unprepare(imx6_pcie->pcie_ref_125m);
> +fail_pcie_ref_125m_clk:
> + clk_disable_unprepare(imx6_pcie->lvds1_gate);
> +fail_lvds_clk:
> + clk_disable_unprepare(imx6_pcie->lvds1_sel);
Why do you disable clocks you never enabled before?
> +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);
> +
The choice of clocks you disable seems rather arbitrary. Also I would
assume that you deregister the pcie controller here.
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
