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

[Doug Anderson]

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.

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