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

[mgautam@codeaurora.org]

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