From: Vivek Gautam <vivek.gautam@codeaurora.org>
To: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: robh+dt@kernel.org, kishon@ti.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, mark.rutland@arm.com,
sboyd@codeaurora.org, srinivas.kandagatla@linaro.org
Subject: Re: [PATCH v5 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips
Date: Fri, 10 Mar 2017 14:12:04 +0530 [thread overview]
Message-ID: <d394f667-f0e3-944f-9afc-5b6d645fcac8@codeaurora.org> (raw)
In-Reply-To: <20170309110147.GA53510@Bjorns-MacBook-Pro-2.local>
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
next prev parent reply other threads:[~2017-03-10 8:42 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-09 9:07 [PATCH v5 0/4] phy: USB and PCIe phy drivers for Qcom chipsets Vivek Gautam
2017-03-09 9:07 ` [PATCH v5 1/4] dt-bindings: phy: Add support for QUSB2 phy Vivek Gautam
2017-03-09 9:07 ` [PATCH v5 2/4] phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips Vivek Gautam
2017-03-09 11:01 ` Bjorn Andersson
2017-03-10 8:42 ` Vivek Gautam [this message]
2017-03-09 9:07 ` [PATCH v5 3/4] dt-bindings: phy: Add support for QMP phy Vivek Gautam
2017-03-09 11:07 ` Bjorn Andersson
2017-03-10 6:13 ` Vivek Gautam
2017-03-15 20:34 ` Rob Herring
2017-03-20 8:26 ` Vivek Gautam
2017-03-09 9:07 ` [PATCH v5 4/4] phy: qcom-qmp: new qmp phy driver for qcom-chipsets Vivek Gautam
2017-03-09 11:32 ` Bjorn Andersson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d394f667-f0e3-944f-9afc-5b6d645fcac8@codeaurora.org \
--to=vivek.gautam@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=devicetree@vger.kernel.org \
--cc=kishon@ti.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=robh+dt@kernel.org \
--cc=sboyd@codeaurora.org \
--cc=srinivas.kandagatla@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).