lkml.org 
[lkml]   [2018]   [Oct]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-10-12 07:58    [W:0.088 / U:0.056 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site