| Message ID | 20181005090920.26108-1-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | [v1] phy: qcom-qusb2: Fix HSTX_TRIM tuning with fused value for SDM845 | expand |
Hi, On Fri, Oct 5, 2018 at 2:09 AM Manu Gautam <mgautam@codeaurora.org> wrote: > > Tune1 register on sdm845 is used to update HSTX_TRIM with fused > setting. Enable same by specifying update_tune1_with_efuse flag > for sdm845, otherwise driver ends up programming tune2 register. > While at it, also fix HSTX_TRIM tuning logic which instead of > using fused value as HSTX_TRIM, incorrectly performs bitwise OR > operation with existing default value. > > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/phy/qualcomm/phy-qcom-qusb2.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) It's a little weird that the old code wasn't causing more problems. Any idea why? On SDM845 it looks like the old code was clobbering the "fstx slew rate control" bits. In any case, this looks like it fixes several commits: 1. The bitwise OR vs. setting the bits w/ mask fixes the original driver at commit ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips"). It'll be slightly annoying to backport past commit c71dabf27c3a ("phy: qcom-qusb2: Add support for different register layouts") but you should still tag "Fixes" with the original commit in case anyone wants to do it. 2. On sdm845 it was updating the wrong register (tune2 instead of tune1). This fixes commit ef17f6e212ca ("phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845"). ...because of the above I'd suggest splitting this into two commits: one that fixes the qusb2_write_mask() and one that fixes sdm845. Then add the "Fixes:" tag. This will really help in the backports to linux-stable. > diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c > index e70e425f26f5..defeb0b7cfa0 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c > +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c > @@ -231,6 +231,7 @@ static const struct qusb2_phy_cfg sdm845_phy_cfg = { > .mask_core_ready = CORE_READY_STATUS, > .has_pll_override = true, > .autoresume_en = BIT(0), > + .update_tune1_with_efuse = true, > }; > > static const char * const qusb2_phy_vreg_names[] = { > @@ -415,12 +416,13 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy) > > /* Fused TUNE1/2 value is the higher nibble only */ > if (cfg->update_tune1_with_efuse) > - qusb2_setbits(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1], > - val[0] << 0x4); > + qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1], > + val[0] << HSTX_TRIM_SHIFT, > + HSTX_TRIM_MASK); > else > - qusb2_setbits(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE2], > - val[0] << 0x4); > - > + qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE2], > + val[0] << HSTX_TRIM_SHIFT, > + HSTX_TRIM_MASK); In general your patch seems like something we should take. ...but it made me look a bit more at the code and I think your patch will tickle another bug that we probably need to fix first. Specifically there are two ways to set HSTX_TRIM. One is in qusb2_phy_set_tune2_param() and the other is in qusb2_phy_override_phy_params(). At the moment we first call qusb2_phy_override_phy_params() and then we call qusb2_phy_set_tune2_param(). That means that (now that we've fixed qusb2_phy_set_tune2_param()) we'll _always_ set the tuning based on qusb2_phy_set_tune2_param() assuming that the nvmem is specified (and non-zero). ...and it's specified in sdm845.dtsi so that means that on SDM845 it's _always_ specified. Said another way: the 'qcom,hstx-trim-value' in sdm845-mtp.dts is totally useless because it will always be overridden by the fuse which is specified in sdm845.dtsi. I have no idea how the fused value vs. the device tree value are supposed to interact, but that doesn't seem right. ...or is the fused value really supposed to override and it'll be 0 if it's not supposed to? -Doug
Hi, On 2018-10-11 04:06, Doug Anderson wrote: > Hi, > > On Fri, Oct 5, 2018 at 2:09 AM Manu Gautam <mgautam@codeaurora.org> > wrote: >> >> Tune1 register on sdm845 is used to update HSTX_TRIM with fused >> setting. Enable same by specifying update_tune1_with_efuse flag >> for sdm845, otherwise driver ends up programming tune2 register. >> While at it, also fix HSTX_TRIM tuning logic which instead of >> using fused value as HSTX_TRIM, incorrectly performs bitwise OR >> operation with existing default value. >> >> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qusb2.c | 12 +++++++----- >> 1 file changed, 7 insertions(+), 5 deletions(-) > > It's a little weird that the old code wasn't causing more problems. > Any idea why? On SDM845 it looks like the old code was clobbering the > "fstx slew rate control" bits. Thanks for reviewing it. I am assuming changing 'fstx slew rc' didn't have major impact other than some interoperability or electrical compliance issues. > > In any case, this looks like it fixes several commits: > > 1. The bitwise OR vs. setting the bits w/ mask fixes the original > driver at commit ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 > PHY on Qcom chips"). It'll be slightly annoying to backport past > commit c71dabf27c3a ("phy: qcom-qusb2: Add support for different > register layouts") but you should still tag "Fixes" with the original > commit in case anyone wants to do it. > > 2. On sdm845 it was updating the wrong register (tune2 instead of > tune1). This fixes commit ef17f6e212ca ("phy: qcom-qusb2: Add QUSB2 > PHYs support for sdm845"). > > ...because of the above I'd suggest splitting this into two commits: > one that fixes the qusb2_write_mask() and one that fixes sdm845. Then > add the "Fixes:" tag. This will really help in the backports to > linux-stable. Sure, will split in two. > > >> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c >> b/drivers/phy/qualcomm/phy-qcom-qusb2.c >> index e70e425f26f5..defeb0b7cfa0 100644 >> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c >> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c >> @@ -231,6 +231,7 @@ static const struct qusb2_phy_cfg sdm845_phy_cfg = >> { >> .mask_core_ready = CORE_READY_STATUS, >> .has_pll_override = true, >> .autoresume_en = BIT(0), >> + .update_tune1_with_efuse = true, >> }; >> >> static const char * const qusb2_phy_vreg_names[] = { >> @@ -415,12 +416,13 @@ static void qusb2_phy_set_tune2_param(struct >> qusb2_phy *qphy) >> >> /* Fused TUNE1/2 value is the higher nibble only */ >> if (cfg->update_tune1_with_efuse) >> - qusb2_setbits(qphy->base, >> cfg->regs[QUSB2PHY_PORT_TUNE1], >> - val[0] << 0x4); >> + qusb2_write_mask(qphy->base, >> cfg->regs[QUSB2PHY_PORT_TUNE1], >> + val[0] << HSTX_TRIM_SHIFT, >> + HSTX_TRIM_MASK); >> else >> - qusb2_setbits(qphy->base, >> cfg->regs[QUSB2PHY_PORT_TUNE2], >> - val[0] << 0x4); >> - >> + qusb2_write_mask(qphy->base, >> cfg->regs[QUSB2PHY_PORT_TUNE2], >> + val[0] << HSTX_TRIM_SHIFT, >> + HSTX_TRIM_MASK); > > In general your patch seems like something we should take. ...but it > made me look a bit more at the code and I think your patch will tickle > another bug that we probably need to fix first. > > Specifically there are two ways to set HSTX_TRIM. One is in > qusb2_phy_set_tune2_param() and the other is in > qusb2_phy_override_phy_params(). At the moment we first call > qusb2_phy_override_phy_params() and then we call > qusb2_phy_set_tune2_param(). That means that (now that we've fixed > qusb2_phy_set_tune2_param()) we'll _always_ set the tuning based on > qusb2_phy_set_tune2_param() assuming that the nvmem is specified (and > non-zero). ...and it's specified in sdm845.dtsi so that means that on > SDM845 it's _always_ specified. > > Said another way: the 'qcom,hstx-trim-value' in sdm845-mtp.dts is > totally useless because it will always be overridden by the fuse which > is specified in sdm845.dtsi. > > I have no idea how the fused value vs. the device tree value are > supposed to interact, but that doesn't seem right. ...or is the fused > value really supposed to override and it'll be 0 if it's not supposed > to? Fused value is supposed to always override. If value is not fused for some parts (which I believe is case with some early samples), then driver will read it is '0' from nvmem and should use hstx-trim value passed from DT. -Manu
Hi, On Thu, Oct 11, 2018 at 10:57 PM <mgautam@codeaurora.org> wrote: > Fused value is supposed to always override. If value is not fused for > some > parts (which I believe is case with some early samples), then driver > will > read it is '0' from nvmem and should use hstx-trim value passed from DT. OK cool. Just wanted to make sure. ...so the fusing must be written after the SoC is placed on a board. I wonder if it makes sense to document this in a comment so I don't ask about it again. ;-) In any case, once you've got the patch split in 2 (and added the "Fixes" tag to each patch) feel free to add my Reviewed-by tag to each patch. -Doug
diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c index e70e425f26f5..defeb0b7cfa0 100644 --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c @@ -231,6 +231,7 @@ static const struct qusb2_phy_cfg sdm845_phy_cfg = { .mask_core_ready = CORE_READY_STATUS, .has_pll_override = true, .autoresume_en = BIT(0), + .update_tune1_with_efuse = true, }; static const char * const qusb2_phy_vreg_names[] = { @@ -415,12 +416,13 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy) /* Fused TUNE1/2 value is the higher nibble only */ if (cfg->update_tune1_with_efuse) - qusb2_setbits(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1], - val[0] << 0x4); + qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1], + val[0] << HSTX_TRIM_SHIFT, + HSTX_TRIM_MASK); else - qusb2_setbits(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE2], - val[0] << 0x4); - + qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE2], + val[0] << HSTX_TRIM_SHIFT, + HSTX_TRIM_MASK); } static int qusb2_phy_set_mode(struct phy *phy, enum phy_mode mode)
Tune1 register on sdm845 is used to update HSTX_TRIM with fused setting. Enable same by specifying update_tune1_with_efuse flag for sdm845, otherwise driver ends up programming tune2 register. While at it, also fix HSTX_TRIM tuning logic which instead of using fused value as HSTX_TRIM, incorrectly performs bitwise OR operation with existing default value. Signed-off-by: Manu Gautam <mgautam@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qusb2.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)