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

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

[Manu Gautam]

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(-)

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);
 }
 
 static int qusb2_phy_set_mode(struct phy *phy, enum phy_mode mode)
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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@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.

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

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@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

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

[Doug Anderson]

Hi,

On Thu, Oct 11, 2018 at 10:57 PM <mgautam@codeaurora.org> wrote:
> 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.

OK cool.  Just wanted to make sure.  ...so the fusing must be written
after the SoC is placed on a board.  I wonder if it makes sense to
document this in a comment so I don't ask about it again.  ;-)

In any case, once you've got the patch split in 2 (and added the
"Fixes" tag to each patch) feel free to add my Reviewed-by tag to each
patch.

-Doug