Messages in this thread | | | Date | Fri, 12 Oct 2018 11:27:30 +0530 | From | mgautam@codeauro ... | Subject | Re: [PATCH v1] phy: qcom-qusb2: Fix HSTX_TRIM tuning with fused value for SDM845 |
| |
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
| |