Re: [PATCH v1] phy: qcom-qusb2: Fix HSTX_TRIM tuning with fused value for SDM845

[mgautam]

Hi,

On 2018-10-11 04:06, Doug Anderson wrote:
> Hi,
>
> On Fri, Oct 5, 2018 at 2:09 AM Manu Gautam <mgautam@xxxxxxxxxxxxxx> 
> 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@xxxxxxxxxxxxxx>
> > ---
> >  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