Messages in this thread | | | Subject | Re: [PATCH v5 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips | From | Vivek Gautam <> | Date | Fri, 10 Mar 2017 14:12:04 +0530 |
| |
Hi,
On 03/09/2017 04:31 PM, Bjorn Andersson wrote: > On Thu 09 Mar 10:07 CET 2017, Vivek Gautam wrote: > > [..] >> +static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val) >> +{ >> + u32 reg; >> + >> + reg = readl_relaxed(base + offset); >> + reg |= val; >> + writel_relaxed(reg, base + offset); >> + >> + /* Ensure above write is completed */ >> + mb(); > mb() does not ensure that the operation is completed, it only ensures > ordering between this write and subsequent memory accesses. > > Unless you have specific reasons to you should not use the _relaxed() > versions of read and write operations. Using the non-relaxed versions > will ensure that your memory operations are not reordered.
Right, I think this is something i took from downstream and didn't change. I should rather be using readl()/writel() and then readl() again to ensure writes are through. Thanks for pointing out.
> >> +} >> + >> +static inline void qusb2_clrbits(void __iomem *base, u32 offset, u32 val) >> +{ >> + u32 reg; >> + >> + reg = readl_relaxed(base + offset); >> + reg &= ~val; >> + writel_relaxed(reg, base + offset); >> + >> + /* Ensure above write is completed */ >> + mb(); > Dito.
Sure, will change it.
> >> +} >> + >> +static inline void qcom_qusb2_phy_configure(void __iomem *base, >> + const struct qusb2_phy_init_tbl tbl[], >> + int num) >> +{ >> + int i; >> + >> + for (i = 0; i < num; i++) >> + writel_relaxed(tbl[i].val, base + tbl[i].offset); >> + >> + /* flush buffered writes */ >> + mb(); > Dito.
will correct it.
> >> +} >> + >> +static int qusb2_phy_toggle_power(struct qusb2_phy *qphy, bool on) >> +{ >> + struct device *dev = &qphy->phy->dev; >> + int ret; >> + >> + if (!on) { >> + ret = 0; >> + goto disable_vdda_phy_dpdm; >> + } > This is an ugly hack.
Right, it sort of minimized the code. :) It just jumps to disable code for an 'off' call.
But, will amend it to use regulator_bulk_*() apis.
> >> + >> + ret = regulator_enable(qphy->vdd_phy); >> + if (ret) { >> + dev_err(dev, "failed to enable vdd-phy, %d\n", ret); >> + goto err_vdd_phy; >> + } >> + >> + ret = regulator_enable(qphy->vdda_pll); >> + if (ret) { >> + dev_err(dev, "failed to enable vdda-pll, %d\n", ret); >> + goto disable_vdd_phy; >> + } >> + >> + ret = regulator_enable(qphy->vdda_phy_dpdm); >> + if (ret) { >> + dev_err(dev, "failed to enable vdda-phy-dpdm, %d\n", ret); >> + goto disable_vdda_pll; >> + } >> + >> + dev_vdbg(dev, "%s(): regulators are turned on\n", __func__); >> + >> + return ret; >> + >> +disable_vdda_phy_dpdm: >> + regulator_disable(qphy->vdda_phy_dpdm); >> +disable_vdda_pll: >> + regulator_disable(qphy->vdda_pll); >> +disable_vdd_phy: >> + regulator_disable(qphy->vdd_phy); >> +err_vdd_phy: >> + dev_vdbg(dev, "%s(): regulators are turned off\n", __func__); >> + return ret; >> +} > If you use the regulator_bulk interface for your regulators you can > replace this entire function with a call to regulator_bulk_enable() and > regulator_bulk_disable().
Sure, will change this as suggested.
> >> + >> +/* >> + * Fetches HS Tx tuning value from nvmem and sets the >> + * QUSB2PHY_PORT_TUNE2 register. >> + * For error case, skip setting the value and use the default value. >> + */ >> +static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy) >> +{ >> + struct device *dev = &qphy->phy->dev; >> + u8 *val; >> + >> + /* >> + * Read efuse register having TUNE2 parameter's high nibble. >> + * If efuse register shows value as 0x0, or if we fail to find >> + * a valid efuse register settings, then use default value >> + * as 0xB for high nibble that we have already set while >> + * configuring phy. >> + */ >> + val = nvmem_cell_read(qphy->cell, NULL); >> + if (IS_ERR(val) || !val[0]) { >> + dev_dbg(dev, "failed to read a valid hs-tx trim value\n"); >> + return; >> + } >> + >> + /* Fused TUNE2 value is the higher nibble only */ >> + qusb2_setbits(qphy->base, QUSB2PHY_PORT_TUNE2, val[0] << 0x4); >> +} >> + >> +static int qusb2_phy_poweron(struct phy *phy) >> +{ >> + struct qusb2_phy *qphy = phy_get_drvdata(phy); >> + int ret; >> + >> + dev_vdbg(&phy->dev, "%s(): Powering-on QUSB2 phy\n", __func__); >> + >> + /* turn on regulator supplies */ >> + ret = qusb2_phy_toggle_power(qphy, true); > Call regulator_bulk_enable()
Sure.
> >> + if (ret) >> + return ret; >> + >> + ret = clk_prepare_enable(qphy->iface_clk); >> + if (ret) >> + dev_err(&phy->dev, "failed to enable iface_clk, %d\n", ret); >> + >> + return ret; >> +} >> + >> +static int qusb2_phy_poweroff(struct phy *phy) >> +{ >> + struct qusb2_phy *qphy = phy_get_drvdata(phy); >> + >> + clk_disable_unprepare(qphy->iface_clk); >> + >> + qusb2_phy_toggle_power(qphy, false); > Call regulator_bulk_disable()
Right, will do that.
> >> + >> + return 0; >> +} >> + >> +static int qusb2_phy_init(struct phy *phy) >> +{ >> + struct qusb2_phy *qphy = phy_get_drvdata(phy); >> + unsigned int val; >> + unsigned int clk_scheme; >> + int ret; >> + >> + dev_vdbg(&phy->dev, "%s(): Initializing QUSB2 phy\n", __func__); >> + >> + /* enable ahb interface clock to program phy */ >> + ret = clk_prepare_enable(qphy->cfg_ahb_clk); >> + if (ret) { >> + dev_err(&phy->dev, "failed to enable cfg ahb clock, %d\n", ret); >> + return ret; >> + } >> + >> + /* Perform phy reset */ >> + ret = reset_control_assert(qphy->phy_reset); >> + if (ret) { >> + dev_err(&phy->dev, "failed to assert phy_reset, %d\n", ret); >> + goto disable_ahb_clk; >> + } >> + >> + /* 100 us delay to keep PHY in reset mode */ >> + usleep_range(100, 150); >> + >> + ret = reset_control_deassert(qphy->phy_reset); >> + if (ret) { >> + dev_err(&phy->dev, "failed to de-assert phy_reset, %d\n", ret); >> + goto disable_ahb_clk; >> + } >> + >> + /* Disable the PHY */ >> + qusb2_setbits(qphy->base, QUSB2PHY_PORT_POWERDOWN, >> + CLAMP_N_EN | FREEZIO_N | POWER_DOWN); >> + >> + /* save reset value to override reference clock scheme later */ >> + val = readl_relaxed(qphy->base + QUSB2PHY_PLL_TEST); >> + >> + qcom_qusb2_phy_configure(qphy->base, qphy->cfg->tbl, >> + qphy->cfg->tbl_num); >> + >> + /* Set efuse value for tuning the PHY */ >> + qusb2_phy_set_tune2_param(qphy); >> + >> + /* Enable the PHY */ >> + qusb2_clrbits(qphy->base, QUSB2PHY_PORT_POWERDOWN, POWER_DOWN); >> + > The mb() in clrbits will not ensure that the write is finished before > the sleep, it will only ensure that above write is done before any later > memory operations. If you want to ensure that the value has hit the > hardware before the sleep you should simply read back the value from the > register.
Right. Will add a read back in qusb2_clrbits() and qusb2_setbits().
> >> + /* Required to get phy pll lock successfully */ >> + usleep_range(150, 160); >> + >> + /* Default is single-ended clock on msm8996 */ >> + qphy->has_se_clk_scheme = true; >> + /* >> + * read TCSR_PHY_CLK_SCHEME register to check if single-ended >> + * clock scheme is selected. If yes, then disable differential >> + * ref_clk and use single-ended clock, otherwise use differential >> + * ref_clk only. >> + */ >> + if (qphy->tcsr) { >> + ret = regmap_read(qphy->tcsr, qphy->cfg->clk_scheme_offset, >> + &clk_scheme); >> + if (ret) { >> + dev_err(&phy->dev, "failed to read clk scheme reg\n"); >> + goto assert_phy_reset; >> + } >> + >> + /* is it a differential clock scheme ? */ >> + if (!(clk_scheme & PHY_CLK_SCHEME_SEL)) { >> + dev_vdbg(&phy->dev, "%s(): select differential clk\n", >> + __func__); >> + qphy->has_se_clk_scheme = false; >> + } else { >> + dev_vdbg(&phy->dev, "%s(): select single-ended clk\n", >> + __func__); >> + } >> + } >> + >> + if (!qphy->has_se_clk_scheme) { >> + val &= ~CLK_REF_SEL; >> + ret = clk_prepare_enable(qphy->ref_clk); >> + if (ret) { >> + dev_err(&phy->dev, "failed to enable ref clk, %d\n", >> + ret); >> + goto assert_phy_reset; >> + } >> + } else { >> + val |= CLK_REF_SEL; >> + } >> + >> + writel_relaxed(val, qphy->base + QUSB2PHY_PLL_TEST); >> + >> + /* Make sure that above write is completed to get PLL source clock */ >> + wmb(); >> + > As above.
Sure, will change this.
>> + /* Required to get phy pll lock successfully */ >> + usleep_range(100, 110); >> + > Regards, > Bjorn
Regards Vivek
-- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
| |