[PATCH v4 0/7] phy: qcom: Updates for USB PHYs on SDM845

[PATCH v4 0/7] phy: qcom: Updates for USB PHYs on SDM845

[Manu Gautam]

SDM845 has two USB instances each with QUSB2 and QMP PHYs.
One of the QMP PHY is USB-DP (DisplayPort) combo PHY where
as other one is single lane UNI-PHY (without DP support).
Changes are related to PHY configuration for electrical
parameters tuning to improve eye-diagram and some fixes.

Changes since v3:
 - As per Doug's review comments added device tree parameters
   to handle board level differences in PHY tuning values instead
   of adding separate device tree bindings.
 - Replace PHY version specific bindings names with SOC name as
   no one is going to use generic binding names.
 - Update halt_check to not check for pipe clock status that
   allows to simplify pipe_clk handling in QMP driver.

Changes since v2:
 - Use separate phy_ops for USB to not register power_on op.
 - And other minor changes as per review comments from Stephen.

Changes since v1:
 - Updated qusb2 compatibility name as per comment from Vivek.

Manu Gautam (7):
  clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk
  phy: qcom-qmp: Enable pipe_clk before PHY initialization
  phy: qcom-qusb2: Fix crash if nvmem cell not specified
  dt-bindings: phy-qcom-qmp: Update bindings for sdm845
  phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support for sdm845
  dt-bindings: phy-qcom-usb2: Add support to override tuning values
  phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

 .../devicetree/bindings/phy/qcom-qmp-phy.txt       |   3 +-
 .../devicetree/bindings/phy/qcom-qusb2-phy.txt     |  19 ++-
 drivers/clk/qcom/gcc-msm8996.c                     |   4 +
 drivers/phy/qualcomm/phy-qcom-qmp.c                | 169 +++++++++++++++++++--
 drivers/phy/qualcomm/phy-qcom-qmp.h                |   5 +
 drivers/phy/qualcomm/phy-qcom-qusb2.c              | 116 +++++++++++++-
 6 files changed, 298 insertions(+), 18 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Manu Gautam]

The USB and PCIE pipe clocks are sourced from external clocks
inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
clocks is dependent on PHY initialization sequence hence
update halt_check to BRANCH_HALT_DELAY for these clocks so
that clock status bit is not polled when enabling or disabling
the clocks. It allows to simplify PHY client driver code which
is both user and source of the pipe_clk and avoid error logging
related status check on clk_disable/enable.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/clk/qcom/gcc-msm8996.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 5d74512..336d12d 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -1418,6 +1418,7 @@ enum {
 
 static struct clk_branch gcc_usb3_phy_pipe_clk = {
 	.halt_reg = 0x50004,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x50004,
 		.enable_mask = BIT(0),
@@ -2472,6 +2473,7 @@ enum {
 
 static struct clk_branch gcc_pcie_0_pipe_clk = {
 	.halt_reg = 0x6b018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6b018,
 		.enable_mask = BIT(0),
@@ -2547,6 +2549,7 @@ enum {
 
 static struct clk_branch gcc_pcie_1_pipe_clk = {
 	.halt_reg = 0x6d018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6d018,
 		.enable_mask = BIT(0),
@@ -2622,6 +2625,7 @@ enum {
 
 static struct clk_branch gcc_pcie_2_pipe_clk = {
 	.halt_reg = 0x6e018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6e018,
 		.enable_mask = BIT(0),
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Manu Gautam]

The USB and PCIE pipe clocks are sourced from external clocks
inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
clocks is dependent on PHY initialization sequence hence
update halt_check to BRANCH_HALT_DELAY for these clocks so
that clock status bit is not polled when enabling or disabling
the clocks. It allows to simplify PHY client driver code which
is both user and source of the pipe_clk and avoid error logging
related status check on clk_disable/enable.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/clk/qcom/gcc-msm8996.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/clk/qcom/gcc-msm8996.c b/drivers/clk/qcom/gcc-msm8996.c
index 5d74512..336d12d 100644
--- a/drivers/clk/qcom/gcc-msm8996.c
+++ b/drivers/clk/qcom/gcc-msm8996.c
@@ -1418,6 +1418,7 @@ enum {
 
 static struct clk_branch gcc_usb3_phy_pipe_clk = {
 	.halt_reg = 0x50004,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x50004,
 		.enable_mask = BIT(0),
@@ -2472,6 +2473,7 @@ enum {
 
 static struct clk_branch gcc_pcie_0_pipe_clk = {
 	.halt_reg = 0x6b018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6b018,
 		.enable_mask = BIT(0),
@@ -2547,6 +2549,7 @@ enum {
 
 static struct clk_branch gcc_pcie_1_pipe_clk = {
 	.halt_reg = 0x6d018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6d018,
 		.enable_mask = BIT(0),
@@ -2622,6 +2625,7 @@ enum {
 
 static struct clk_branch gcc_pcie_2_pipe_clk = {
 	.halt_reg = 0x6e018,
+	.halt_check = BRANCH_HALT_DELAY,
 	.clkr = {
 		.enable_reg = 0x6e018,
 		.enable_mask = BIT(0),
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> The USB and PCIE pipe clocks are sourced from external clocks
> inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
> clocks is dependent on PHY initialization sequence hence
> update halt_check to BRANCH_HALT_DELAY for these clocks so
> that clock status bit is not polled when enabling or disabling
> the clocks. It allows to simplify PHY client driver code which
> is both user and source of the pipe_clk and avoid error logging
> related status check on clk_disable/enable.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  drivers/clk/qcom/gcc-msm8996.c | 4 ++++
>  1 file changed, 4 insertions(+)

As per my feedback on <https://patchwork.kernel.org/patch/10314937/>,
I'm not a fan of this.  Hopefully we can adjust the PHY driver so it's
not needed.

-Doug

Re: [PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Stephen Boyd]

Quoting Doug Anderson (2018-03-29 13:55:55)
> Hi,
> 
> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> > The USB and PCIE pipe clocks are sourced from external clocks
> > inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
> > clocks is dependent on PHY initialization sequence hence
> > update halt_check to BRANCH_HALT_DELAY for these clocks so
> > that clock status bit is not polled when enabling or disabling
> > the clocks. It allows to simplify PHY client driver code which
> > is both user and source of the pipe_clk and avoid error logging
> > related status check on clk_disable/enable.
> >
> > Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> > ---
> >  drivers/clk/qcom/gcc-msm8996.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> 
> As per my feedback on <https://patchwork.kernel.org/patch/10314937/>,
> I'm not a fan of this.  Hopefully we can adjust the PHY driver so it's
> not needed.
> 

Agreed. We should be able to enable the clks at the right time and halt
bits should work. From what I can recall we had that working before on
db820c, so has something changed?

Re: [PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Stephen Boyd]

Quoting Doug Anderson (2018-03-29 13:55:55)
> Hi,
> =

> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wro=
te:
> > The USB and PCIE pipe clocks are sourced from external clocks
> > inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
> > clocks is dependent on PHY initialization sequence hence
> > update halt_check to BRANCH_HALT_DELAY for these clocks so
> > that clock status bit is not polled when enabling or disabling
> > the clocks. It allows to simplify PHY client driver code which
> > is both user and source of the pipe_clk and avoid error logging
> > related status check on clk_disable/enable.
> >
> > Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> > ---
> >  drivers/clk/qcom/gcc-msm8996.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> =

> As per my feedback on <https://patchwork.kernel.org/patch/10314937/>,
> I'm not a fan of this.  Hopefully we can adjust the PHY driver so it's
> not needed.
> =


Agreed. We should be able to enable the clks at the right time and halt
bits should work. From what I can recall we had that working before on
db820c, so has something changed?

Re: [PATCH v4 1/7] clk: msm8996-gcc: change halt check for USB/PCIE pipe_clk

[Manu Gautam]

Hi,


On 4/6/2018 1:37 AM, Stephen Boyd wrote:
> Quoting Doug Anderson (2018-03-29 13:55:55)
>> Hi,
>>
>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>> The USB and PCIE pipe clocks are sourced from external clocks
>>> inside the QMP USB/PCIE PHYs. Enabling or disabling of PIPE RCG
>>> clocks is dependent on PHY initialization sequence hence
>>> update halt_check to BRANCH_HALT_DELAY for these clocks so
>>> that clock status bit is not polled when enabling or disabling
>>> the clocks. It allows to simplify PHY client driver code which
>>> is both user and source of the pipe_clk and avoid error logging
>>> related status check on clk_disable/enable.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>> ---
>>>  drivers/clk/qcom/gcc-msm8996.c | 4 ++++
>>>  1 file changed, 4 insertions(+)
>> As per my feedback on <https://patchwork.kernel.org/patch/10314937/>,
>> I'm not a fan of this.  Hopefully we can adjust the PHY driver so it's
>> not needed.
>>
> Agreed. We should be able to enable the clks at the right time and halt
> bits should work. From what I can recall we had that working before on
> db820c, so has something changed?

As replied in other patch IMO it is better to stick to the recommended
sequence and have this change as it would allow to cleanup PHY driver
and align with HPG. One reason I can think of why it works
on db820c is that there is some code in bootloader that enables
all USB clocks (including pipe_clk). Same is the case with SDM845.
And I start seeing errors if bootloader is changed or skip pipe_clk enable.



-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Manu Gautam]

QMP PHY for USB/PCIE requires pipe_clk for locking of
retime buffers at the pipe interface. Driver checks for
PHY_STATUS without enabling pipe_clk due to which
phy_init() fails with initialization timeout.
Though pipe_clk is output from PHY (after PLL is programmed
during initialization sequence) to GCC clock_ctl and then fed
back to PHY but for PHY_STATUS register to reflect successful
initialization pipe_clk from GCC must be present.
Since, clock driver now ignores status_check for pipe_clk on
clk_enable/disable, driver can safely enable/disable pipe_clk
from phy_init/exit.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 6470c5d..fddb1c9 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -793,19 +793,6 @@ static void qcom_qmp_phy_configure(void __iomem *base,
 	}
 }
 
-static int qcom_qmp_phy_poweron(struct phy *phy)
-{
-	struct qmp_phy *qphy = phy_get_drvdata(phy);
-	struct qcom_qmp *qmp = qphy->qmp;
-	int ret;
-
-	ret = clk_prepare_enable(qphy->pipe_clk);
-	if (ret)
-		dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret);
-
-	return ret;
-}
-
 static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
 {
 	const struct qmp_phy_cfg *cfg = qmp->cfg;
@@ -974,6 +961,12 @@ static int qcom_qmp_phy_init(struct phy *phy)
 		}
 	}
 
+	ret = clk_prepare_enable(qphy->pipe_clk);
+	if (ret) {
+		dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret);
+		goto err_clk_enable;
+	}
+
 	/* Tx, Rx, and PCS configurations */
 	qcom_qmp_phy_configure(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num);
 	/* Configuration for other LANE for USB-DP combo PHY */
@@ -1019,6 +1012,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
 	return ret;
 
 err_pcs_ready:
+	clk_disable_unprepare(qphy->pipe_clk);
+err_clk_enable:
 	if (cfg->has_lane_rst)
 		reset_control_assert(qphy->lane_rst);
 err_lane_rst:
@@ -1283,7 +1278,6 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np)
 static const struct phy_ops qcom_qmp_phy_gen_ops = {
 	.init		= qcom_qmp_phy_init,
 	.exit		= qcom_qmp_phy_exit,
-	.power_on	= qcom_qmp_phy_poweron,
 	.set_mode	= qcom_qmp_phy_set_mode,
 	.owner		= THIS_MODULE,
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Evan Green]

Hi Manu,

On Thu, Mar 29, 2018 at 4:06 AM Manu Gautam <mgautam@codeaurora.org> wrote:

> QMP PHY for USB/PCIE requires pipe_clk for locking of
> retime buffers at the pipe interface. Driver checks for
> PHY_STATUS without enabling pipe_clk due to which
> phy_init() fails with initialization timeout.
> Though pipe_clk is output from PHY (after PLL is programmed
> during initialization sequence) to GCC clock_ctl and then fed
> back to PHY but for PHY_STATUS register to reflect successful
> initialization pipe_clk from GCC must be present.
> Since, clock driver now ignores status_check for pipe_clk on
> clk_enable/disable, driver can safely enable/disable pipe_clk
> from phy_init/exit.

> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)

> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c
b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 6470c5d..fddb1c9 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -793,19 +793,6 @@ static void qcom_qmp_phy_configure(void __iomem
*base,
>          }
>   }

> -static int qcom_qmp_phy_poweron(struct phy *phy)
> -{
> -       struct qmp_phy *qphy = phy_get_drvdata(phy);
> -       struct qcom_qmp *qmp = qphy->qmp;
> -       int ret;
> -
> -       ret = clk_prepare_enable(qphy->pipe_clk);
> -       if (ret)
> -               dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n",
ret);
> -
> -       return ret;
> -}
> -
>   static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>   {

This change looks good to me, it has much nicer symmetry. It did get me
looking at qcom_qmp_phy_com_init though. This isn't directly related to
your change, but I think there might be an issue with qmp->init_count. We
increment it at the start of the function, but then if init fails we don't
decrement it. So if we then try init again later, it magically succeeds
without actually initing anything. The other side, qcom_qmp_phy_com_exit
doesn't have this problem because exit doesn't fail (which begs the
question of why it has a return value).

I don't need to hold up this series, since this isn't strictly related to
your changes, but if you do spin again, you could include it. Otherwise you
or I could send it along as a followup.

-Evan

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> QMP PHY for USB/PCIE requires pipe_clk for locking of
> retime buffers at the pipe interface. Driver checks for
> PHY_STATUS without enabling pipe_clk due to which
> phy_init() fails with initialization timeout.
> Though pipe_clk is output from PHY (after PLL is programmed
> during initialization sequence) to GCC clock_ctl and then fed
> back to PHY but for PHY_STATUS register to reflect successful
> initialization pipe_clk from GCC must be present.
> Since, clock driver now ignores status_check for pipe_clk on
> clk_enable/disable, driver can safely enable/disable pipe_clk
> from phy_init/exit.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
>  1 file changed, 8 insertions(+), 14 deletions(-)

Overall this looks much better than the previous version.  Thanks!  :)

I wonder one thing though.  You describe the original problem as this:

1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY
never sets the "ready" status.

2. If you don't have the PHY powered on / out of reset (which happens
in qcom_qmp_phy_init()) then when you enable/disable the clock it
doesn't properly update the status.  That's why you needed patch #1 in
this series.


I wonder: could you solve the above _without_ needing to use
BRANCH_HALT_DELAY in the clock driver?  Specifically, can you tell me
what happens if you put the clk_prepare_enable() after you've powered
on the PHY and taken it out of reset but before you check the status?
Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call
right before the "readl_poll_timeout" of the ready status?


If you do that, you'll turn everything on.  Then you'll check that the
clock's status is OK and then that the PHY's status is OK.


-Doug

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> QMP PHY for USB/PCIE requires pipe_clk for locking of
>> retime buffers at the pipe interface. Driver checks for
>> PHY_STATUS without enabling pipe_clk due to which
>> phy_init() fails with initialization timeout.
>> Though pipe_clk is output from PHY (after PLL is programmed
>> during initialization sequence) to GCC clock_ctl and then fed
>> back to PHY but for PHY_STATUS register to reflect successful
>> initialization pipe_clk from GCC must be present.
>> Since, clock driver now ignores status_check for pipe_clk on
>> clk_enable/disable, driver can safely enable/disable pipe_clk
>> from phy_init/exit.
>>
>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>> ---
>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
>>  1 file changed, 8 insertions(+), 14 deletions(-)
>
> Overall this looks much better than the previous version.  Thanks!  :)
>
> I wonder one thing though.  You describe the original problem as this:
>
> 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY
> never sets the "ready" status.
>
> 2. If you don't have the PHY powered on / out of reset (which happens
> in qcom_qmp_phy_init()) then when you enable/disable the clock it
> doesn't properly update the status.  That's why you needed patch #1 in
> this series.
>
>
> I wonder: could you solve the above _without_ needing to use
> BRANCH_HALT_DELAY in the clock driver?  Specifically, can you tell me
> what happens if you put the clk_prepare_enable() after you've powered
> on the PHY and taken it out of reset but before you check the status?
> Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call
> right before the "readl_poll_timeout" of the ready status?
>
>
> If you do that, you'll turn everything on.  Then you'll check that the
> clock's status is OK and then that the PHY's status is OK.

Oh!  This is what you did in the previous version of the patch, then you said:

"That is still needed as PHY might take some time to generate pipe_clk
after its PLL is locked".

It's really going to take more than the 200 us that the clock driver
is giving it?  If so, I'd prefer to increase the amount of time waited
in the clock driver, or adding a fixed delay _before_ the clk_enable()
so that the 200 us that the clock driver gives it would be enough.

I'm just not a fan of ignoring status bits if it can be helped.


-Doug

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Manu Gautam]

Hi,


On 3/30/2018 2:24 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>> QMP PHY for USB/PCIE requires pipe_clk for locking of
>>> retime buffers at the pipe interface. Driver checks for
>>> PHY_STATUS without enabling pipe_clk due to which
>>> phy_init() fails with initialization timeout.
>>> Though pipe_clk is output from PHY (after PLL is programmed
>>> during initialization sequence) to GCC clock_ctl and then fed
>>> back to PHY but for PHY_STATUS register to reflect successful
>>> initialization pipe_clk from GCC must be present.
>>> Since, clock driver now ignores status_check for pipe_clk on
>>> clk_enable/disable, driver can safely enable/disable pipe_clk
>>> from phy_init/exit.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>> ---
>>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
>>>  1 file changed, 8 insertions(+), 14 deletions(-)
>> Overall this looks much better than the previous version.  Thanks!  :)
>>
>> I wonder one thing though.  You describe the original problem as this:
>>
>> 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY
>> never sets the "ready" status.
>>
>> 2. If you don't have the PHY powered on / out of reset (which happens
>> in qcom_qmp_phy_init()) then when you enable/disable the clock it
>> doesn't properly update the status.  That's why you needed patch #1 in
>> this series.
>>
>>
>> I wonder: could you solve the above _without_ needing to use
>> BRANCH_HALT_DELAY in the clock driver?  Specifically, can you tell me
>> what happens if you put the clk_prepare_enable() after you've powered
>> on the PHY and taken it out of reset but before you check the status?
>> Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call
>> right before the "readl_poll_timeout" of the ready status?
>>
>>
>> If you do that, you'll turn everything on.  Then you'll check that the
>> clock's status is OK and then that the PHY's status is OK.
> Oh!  This is what you did in the previous version of the patch, then you said:
>
> "That is still needed as PHY might take some time to generate pipe_clk
> after its PLL is locked".
>
> It's really going to take more than the 200 us that the clock driver
> is giving it?  If so, I'd prefer to increase the amount of time waited
> in the clock driver, or adding a fixed delay _before_ the clk_enable()
> so that the 200 us that the clock driver gives it would be enough.
>
> I'm just not a fan of ignoring status bits if it can be helped.

I too would want to do that but it is not just about the delay.
As per QMP PHY hardware designers, pipe_clk should be enabled in GCC
as first thing in the PHY initialization sequence. Same sequence also has
been used in downstream phy driver always.
Changing the sequence might work but I would like to stick to the HPG
recommendation and avoid any deviation as PHY issues are very hard to
debug.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Doug Anderson]

Hi,

On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Hi,
>
>
> On 3/30/2018 2:24 AM, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote:
>>> Hi,
>>>
>>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>>> QMP PHY for USB/PCIE requires pipe_clk for locking of
>>>> retime buffers at the pipe interface. Driver checks for
>>>> PHY_STATUS without enabling pipe_clk due to which
>>>> phy_init() fails with initialization timeout.
>>>> Though pipe_clk is output from PHY (after PLL is programmed
>>>> during initialization sequence) to GCC clock_ctl and then fed
>>>> back to PHY but for PHY_STATUS register to reflect successful
>>>> initialization pipe_clk from GCC must be present.
>>>> Since, clock driver now ignores status_check for pipe_clk on
>>>> clk_enable/disable, driver can safely enable/disable pipe_clk
>>>> from phy_init/exit.
>>>>
>>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>>> ---
>>>>  drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++--------------
>>>>  1 file changed, 8 insertions(+), 14 deletions(-)
>>> Overall this looks much better than the previous version.  Thanks!  :)
>>>
>>> I wonder one thing though.  You describe the original problem as this:
>>>
>>> 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY
>>> never sets the "ready" status.
>>>
>>> 2. If you don't have the PHY powered on / out of reset (which happens
>>> in qcom_qmp_phy_init()) then when you enable/disable the clock it
>>> doesn't properly update the status.  That's why you needed patch #1 in
>>> this series.
>>>
>>>
>>> I wonder: could you solve the above _without_ needing to use
>>> BRANCH_HALT_DELAY in the clock driver?  Specifically, can you tell me
>>> what happens if you put the clk_prepare_enable() after you've powered
>>> on the PHY and taken it out of reset but before you check the status?
>>> Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call
>>> right before the "readl_poll_timeout" of the ready status?
>>>
>>>
>>> If you do that, you'll turn everything on.  Then you'll check that the
>>> clock's status is OK and then that the PHY's status is OK.
>> Oh!  This is what you did in the previous version of the patch, then you said:
>>
>> "That is still needed as PHY might take some time to generate pipe_clk
>> after its PLL is locked".
>>
>> It's really going to take more than the 200 us that the clock driver
>> is giving it?  If so, I'd prefer to increase the amount of time waited
>> in the clock driver, or adding a fixed delay _before_ the clk_enable()
>> so that the 200 us that the clock driver gives it would be enough.
>>
>> I'm just not a fan of ignoring status bits if it can be helped.
>
> I too would want to do that but it is not just about the delay.
> As per QMP PHY hardware designers, pipe_clk should be enabled in GCC
> as first thing in the PHY initialization sequence. Same sequence also has
> been used in downstream phy driver always.
> Changing the sequence might work but I would like to stick to the HPG
> recommendation and avoid any deviation as PHY issues are very hard to
> debug.

So hardware guys tell you that you're _supposed to_ ignore the clock
ready bit for that clock and just hope it turns on and settles in time
once power comes on for the clock?  That doesn't seem ideal.  My guess
is that it's a bug in the specification that the QMP PHY hardware
designers gave you.

Stephen can feel free to override me if he disagrees since he's in
charge of the clock part of this, but IMHO we should get the
specification fixed and turn things on in the order that lets us check
the status bits.


-Doug

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Stephen Boyd]

Quoting Doug Anderson (2018-04-10 08:05:27)
> On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
> > On 3/30/2018 2:24 AM, Doug Anderson wrote:
> >> Oh!  This is what you did in the previous version of the patch, then you said:
> >>
> >> "That is still needed as PHY might take some time to generate pipe_clk
> >> after its PLL is locked".
> >>
> >> It's really going to take more than the 200 us that the clock driver
> >> is giving it?  If so, I'd prefer to increase the amount of time waited
> >> in the clock driver, or adding a fixed delay _before_ the clk_enable()
> >> so that the 200 us that the clock driver gives it would be enough.
> >>
> >> I'm just not a fan of ignoring status bits if it can be helped.
> >
> > I too would want to do that but it is not just about the delay.
> > As per QMP PHY hardware designers, pipe_clk should be enabled in GCC
> > as first thing in the PHY initialization sequence. Same sequence also has
> > been used in downstream phy driver always.
> > Changing the sequence might work but I would like to stick to the HPG
> > recommendation and avoid any deviation as PHY issues are very hard to
> > debug.
> 
> So hardware guys tell you that you're _supposed to_ ignore the clock
> ready bit for that clock and just hope it turns on and settles in time
> once power comes on for the clock?  That doesn't seem ideal.  My guess
> is that it's a bug in the specification that the QMP PHY hardware
> designers gave you.
> 
> Stephen can feel free to override me if he disagrees since he's in
> charge of the clock part of this, but IMHO we should get the
> specification fixed and turn things on in the order that lets us check
> the status bits.
> 

Presumably there's a PLL "enable" bit in the QMP phy init sequence that
we can use to enable the PLL output at a bypass rate and then enable the
clk in GCC and then resume the PLL enable sequence. That would allow us
to make sure the clk is working.

Are the branches in GCC ever turned on or off at runtime though? Or do
we just turn them on because they're defaulted off out of reset and then
leave them on?

I ask because it may be easier to never expose these clks in Linux, hit
the enable bits in the branches during clk driver probe, and then act
like they never exist because we don't really use them.

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Manu Gautam]

Hi,


On 4/11/2018 12:02 AM, Stephen Boyd wrote:
> Quoting Doug Anderson (2018-04-10 08:05:27)
>> On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>> On 3/30/2018 2:24 AM, Doug Anderson wrote:
>>>> Oh!  This is what you did in the previous version of the patch, then you said:
>>>>
>>>> "That is still needed as PHY might take some time to generate pipe_clk
>>>> after its PLL is locked".
>>>>
>>>> It's really going to take more than the 200 us that the clock driver
>>>> is giving it?  If so, I'd prefer to increase the amount of time waited
>>>> in the clock driver, or adding a fixed delay _before_ the clk_enable()
>>>> so that the 200 us that the clock driver gives it would be enough.
>>>>
>>>> I'm just not a fan of ignoring status bits if it can be helped.
>>> I too would want to do that but it is not just about the delay.
>>> As per QMP PHY hardware designers, pipe_clk should be enabled in GCC
>>> as first thing in the PHY initialization sequence. Same sequence also has
>>> been used in downstream phy driver always.
>>> Changing the sequence might work but I would like to stick to the HPG
>>> recommendation and avoid any deviation as PHY issues are very hard to
>>> debug.
>> So hardware guys tell you that you're _supposed to_ ignore the clock
>> ready bit for that clock and just hope it turns on and settles in time
>> once power comes on for the clock?  That doesn't seem ideal.  My guess
>> is that it's a bug in the specification that the QMP PHY hardware
>> designers gave you.
It could be a bug in specification. But same sequence has been well tested on both
msm8996 and sdm845, so I would for now like to stick with that for now.
>> Stephen can feel free to override me if he disagrees since he's in
>> charge of the clock part of this, but IMHO we should get the
>> specification fixed and turn things on in the order that lets us check
>> the status bits.
>>
> Presumably there's a PLL "enable" bit in the QMP phy init sequence that
> we can use to enable the PLL output at a bypass rate and then enable the
> clk in GCC and then resume the PLL enable sequence. That would allow us
> to make sure the clk is working.
>
> Are the branches in GCC ever turned on or off at runtime though? Or do
> we just turn them on because they're defaulted off out of reset and then
> leave them on?
It can be left on always.
>
> I ask because it may be easier to never expose these clks in Linux, hit
> the enable bits in the branches during clk driver probe, and then act
> like they never exist because we don't really use them.
This sounds better idea. Let me check if I can get a patch for same in msm8996
and sdm845 clock drivers.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Stephen Boyd]

Quoting Manu Gautam (2018-04-11 08:37:38)
> >
> > I ask because it may be easier to never expose these clks in Linux, hit
> > the enable bits in the branches during clk driver probe, and then act
> > like they never exist because we don't really use them.
> This sounds better idea. Let me check if I can get a patch for same in msm8996
> and sdm845 clock drivers.
> 

Ok! Presumably the PHY has a way to tell if it failed to turn on right?
Put another way, I'm hoping these branch bits aren't there to help us
debug and figure out when the PHY PLL fails to lock.

Re: [PATCH v4 2/7] phy: qcom-qmp: Enable pipe_clk before PHY initialization

[Manu Gautam]

Hi,


On 4/13/2018 2:08 AM, Stephen Boyd wrote:
> Quoting Manu Gautam (2018-04-11 08:37:38)
>>> I ask because it may be easier to never expose these clks in Linux, hit
>>> the enable bits in the branches during clk driver probe, and then act
>>> like they never exist because we don't really use them.
>> This sounds better idea. Let me check if I can get a patch for same in msm8996
>> and sdm845 clock drivers.
>>
> Ok! Presumably the PHY has a way to tell if it failed to turn on right?
> Put another way, I'm hoping these branch bits aren't there to help us
> debug and figure out when the PHY PLL fails to lock.

Yes, PHY has a PCS_STATUS3 register that indicates whether pipe_clk got enabled or
not.


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 3/7] phy: qcom-qusb2: Fix crash if nvmem cell not specified

[Manu Gautam]

Driver currently crashes due to NULL pointer deference
while updating PHY tune register if nvmem cell is NULL.
Since, fused value for Tune1/2 register is optional,
we'd rather bail out.

Fixes: ca04d9d3e1b1 ("phy: qcom-qusb2: New driver for QUSB2 PHY on Qcom chips")
Reviewed-by: Vivek Gautam <vivek.gautam@codeaurora.org>
Reviewed-by: Evan Green <evgreen@chromium.org>
Cc: stable <stable@vger.kernel.org> # 4.14+
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 94afeac..40fdef8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -315,6 +315,10 @@ static void qusb2_phy_set_tune2_param(struct qusb2_phy *qphy)
 	const struct qusb2_phy_cfg *cfg = qphy->cfg;
 	u8 *val;
 
+	/* efuse register is optional */
+	if (!qphy->cell)
+		return;
+
 	/*
 	 * Read efuse register having TUNE2/1 parameter's high nibble.
 	 * If efuse register shows value as 0x0, or if we fail to find
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 4/7] dt-bindings: phy-qcom-qmp: Update bindings for sdm845

[Manu Gautam]

Update compatible strings for USB3 PHYs on SDM845.
One is QMPv3 DisplayPort-USB combo PHY and other one
is USB UNI PHY which is single lane USB3 PHY without
DP capability. While at it also remove "qcom,qmp-v3-usb3-phy"
compatible string which was earlier added for sdm845
only as there wouldn't be any user of same.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
index dcf1b8f..266a1bb 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt
@@ -9,7 +9,8 @@ Required properties:
 	       "qcom,ipq8074-qmp-pcie-phy" for PCIe phy on IPQ8074
 	       "qcom,msm8996-qmp-pcie-phy" for 14nm PCIe phy on msm8996,
 	       "qcom,msm8996-qmp-usb3-phy" for 14nm USB3 phy on msm8996,
-	       "qcom,qmp-v3-usb3-phy" for USB3 QMP V3 phy.
+	       "qcom,sdm845-qmp-usb3-phy" for USB3 QMP V3 phy on sdm845,
+	       "qcom,sdm845-qmp-usb3-uni-phy" for USB3 QMP V3 UNI phy on sdm845.
 
  - reg: offset and length of register set for PHY's common serdes block.
 
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 4/7] dt-bindings: phy-qcom-qmp: Update bindings for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> Update compatible strings for USB3 PHYs on SDM845.
> One is QMPv3 DisplayPort-USB combo PHY and other one
> is USB UNI PHY which is single lane USB3 PHY without
> DP capability. While at it also remove "qcom,qmp-v3-usb3-phy"
> compatible string which was earlier added for sdm845
> only as there wouldn't be any user of same.
>
> Reviewed-by: Rob Herring <robh@kernel.org>

IMHO you should remove Rob's Review until he re-reviews.  I want to
make sure he's OK with the removal of the "qcom,qmp-v3-usb3-phy".
Rob: that compatible string was _just_ added and was intended for
SDM845.  It's not expected that there are any users yet.  IMHO that
means it's OK to remove, but it would be good to get your confirmation
that you agree.  See the discussion at
<https://patchwork.kernel.org/patch/10302761/>.


> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  Documentation/devicetree/bindings/phy/qcom-qmp-phy.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

[PATCH v4 5/7] phy: qcom-qmp: Add QMP V3 USB3 UNI PHY support for sdm845

[Manu Gautam]

QMP V3 UNI PHY is a single lane USB3 PHY without support
for DisplayPort (DP).
Main difference from DP combo QMPv3 PHY is that UNI PHY
doesn't have dual RX/TX lanes and no separate DP_COM
block for configuration related to type-c or DP.
Also remove "qcom,qmp-v3-usb3-phy" compatible string which
was earlier added for sdm845 only as there wouldn't be
any user of same.
While at it, fix has_pwrdn_delay attribute for USB-DP
PHY configuration and.

Reviewed-by: Evan Green <evgreen@chromium.org>
Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 147 +++++++++++++++++++++++++++++++++++-
 drivers/phy/qualcomm/phy-qcom-qmp.h |   5 ++
 2 files changed, 151 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c
index fddb1c9..4c47010 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -490,6 +490,118 @@ enum qphy_reg_layout {
 	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_RUN_TIME, 0x13),
 };
 
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_serdes_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_IVCO, 0x07),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_EN_SEL, 0x14),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_BIAS_EN_CLKBUFLR_EN, 0x04),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CLK_SELECT, 0x30),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYS_CLK_CTRL, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_RESETSM_CNTRL2, 0x08),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CMN_CONFIG, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SVS_MODE_CLK_SEL, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_HSCLK_SEL, 0x80),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DEC_START_MODE0, 0x82),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START1_MODE0, 0xab),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START2_MODE0, 0xea),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_DIV_FRAC_START3_MODE0, 0x02),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CP_CTRL_MODE0, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_RCTRL_MODE0, 0x16),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_PLL_CCTRL_MODE0, 0x36),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN1_MODE0, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_INTEGLOOP_GAIN0_MODE0, 0x3f),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE2_MODE0, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE1_MODE0, 0xc9),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CORECLK_DIV_MODE0, 0x0a),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP3_MODE0, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP2_MODE0, 0x34),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP1_MODE0, 0x15),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP_EN, 0x04),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_CORE_CLK_EN, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_LOCK_CMP_CFG, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_VCO_TUNE_MAP, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SYSCLK_BUF_ENABLE, 0x0a),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_EN_CENTER, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_PER1, 0x31),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_PER2, 0x01),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_ADJ_PER1, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_ADJ_PER2, 0x00),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_STEP_SIZE1, 0x85),
+	QMP_PHY_INIT_CFG(QSERDES_V3_COM_SSC_STEP_SIZE2, 0x07),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_tx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_HIGHZ_DRVR_EN, 0x10),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RCV_DETECT_LVL_2, 0x12),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_LANE_MODE_1, 0xc6),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_RX, 0x06),
+	QMP_PHY_INIT_CFG(QSERDES_V3_TX_RES_CODE_LANE_OFFSET_TX, 0x06),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_rx_tbl[] = {
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_VGA_CAL_CNTRL2, 0x0c),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_MODE_00, 0x50),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN, 0x0b),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL2, 0x0e),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL3, 0x4e),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL4, 0x18),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_EQ_OFFSET_ADAPTOR_CNTRL1, 0x77),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_RX_OFFSET_ADAPTOR_CNTRL2, 0x80),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_CNTRL, 0x03),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL, 0x1c),
+	QMP_PHY_INIT_CFG(QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE, 0x75),
+};
+
+static const struct qmp_phy_init_tbl qmp_v3_usb3_uniphy_pcs_tbl[] = {
+	/* FLL settings */
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL2, 0x83),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_L, 0x09),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNT_VAL_H_TOL, 0xa2),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_MAN_CODE, 0x40),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_FLL_CNTRL1, 0x02),
+
+	/* Lock Det settings */
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG1, 0xd1),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG2, 0x1f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LOCK_DETECT_CONFIG3, 0x47),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_POWER_STATE_CONFIG2, 0x1b),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RX_SIGDET_LVL, 0xba),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V0, 0x9f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V1, 0x9f),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V2, 0xb5),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V3, 0x4c),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_V4, 0x64),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXMGN_LS, 0x6a),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V0, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V0, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V1, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V1, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V2, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V2, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V3, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V3, 0x1d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_V4, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_V4, 0x0d),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M6DB_LS, 0x15),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TXDEEMPH_M3P5DB_LS, 0x0d),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RATE_SLEW_CNTRL, 0x02),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_PWRUP_RESET_DLY_TIME_AUXCLK, 0x04),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_TSYNC_RSYNC_TIME, 0x44),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_PWRUP_RESET_DLY_TIME_AUXCLK, 0x04),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_L, 0xe7),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_P1U2_H, 0x03),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_L, 0x40),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RCVR_DTCT_DLY_U3_H, 0x00),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_WAIT_TIME, 0x75),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_LFPS_TX_ECSTART_EQTLOCK, 0x86),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_RXEQTRAINING_RUN_TIME, 0x13),
+
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_REFGEN_REQ_CONFIG1, 0x21),
+	QMP_PHY_INIT_CFG(QPHY_V3_PCS_REFGEN_REQ_CONFIG2, 0x60),
+};
+
+
 /* struct qmp_phy_cfg - per-PHY initialization config */
 struct qmp_phy_cfg {
 	/* phy-type - PCIE/UFS/USB */
@@ -766,6 +878,7 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
 	.pwrdn_ctrl		= SW_PWRDN,
 	.mask_pcs_ready		= PHYSTATUS,
 
+	.has_pwrdn_delay	= true,
 	.pwrdn_delay_min	= POWER_DOWN_DELAY_US_MIN,
 	.pwrdn_delay_max	= POWER_DOWN_DELAY_US_MAX,
 
@@ -774,6 +887,35 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
 	.rx_b_lane_offset	= 0x400,
 };
 
+static const struct qmp_phy_cfg qmp_v3_usb3_uniphy_cfg = {
+	.type			= PHY_TYPE_USB3,
+	.nlanes			= 1,
+
+	.serdes_tbl		= qmp_v3_usb3_uniphy_serdes_tbl,
+	.serdes_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_serdes_tbl),
+	.tx_tbl			= qmp_v3_usb3_uniphy_tx_tbl,
+	.tx_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_tx_tbl),
+	.rx_tbl			= qmp_v3_usb3_uniphy_rx_tbl,
+	.rx_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_rx_tbl),
+	.pcs_tbl		= qmp_v3_usb3_uniphy_pcs_tbl,
+	.pcs_tbl_num		= ARRAY_SIZE(qmp_v3_usb3_uniphy_pcs_tbl),
+	.clk_list		= qmp_v3_phy_clk_l,
+	.num_clks		= ARRAY_SIZE(qmp_v3_phy_clk_l),
+	.reset_list		= msm8996_usb3phy_reset_l,
+	.num_resets		= ARRAY_SIZE(msm8996_usb3phy_reset_l),
+	.vreg_list		= msm8996_phy_vreg_l,
+	.num_vregs		= ARRAY_SIZE(msm8996_phy_vreg_l),
+	.regs			= qmp_v3_usb3phy_regs_layout,
+
+	.start_ctrl		= SERDES_START | PCS_START,
+	.pwrdn_ctrl		= SW_PWRDN,
+	.mask_pcs_ready		= PHYSTATUS,
+
+	.has_pwrdn_delay	= true,
+	.pwrdn_delay_min	= POWER_DOWN_DELAY_US_MIN,
+	.pwrdn_delay_max	= POWER_DOWN_DELAY_US_MAX,
+};
+
 static void qcom_qmp_phy_configure(void __iomem *base,
 				   const unsigned int *regs,
 				   const struct qmp_phy_init_tbl tbl[],
@@ -1375,8 +1517,11 @@ int qcom_qmp_phy_create(struct device *dev, struct device_node *np, int id)
 		.compatible = "qcom,ipq8074-qmp-pcie-phy",
 		.data = &ipq8074_pciephy_cfg,
 	}, {
-		.compatible = "qcom,qmp-v3-usb3-phy",
+		.compatible = "qcom,sdm845-qmp-usb3-phy",
 		.data = &qmp_v3_usb3phy_cfg,
+	}, {
+		.compatible = "qcom,sdm845-qmp-usb3-uni-phy",
+		.data = &qmp_v3_usb3_uniphy_cfg,
 	},
 	{ },
 };
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.h b/drivers/phy/qualcomm/phy-qcom-qmp.h
index d1c6905..5d78d43 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.h
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.h
@@ -214,6 +214,8 @@
 #define QSERDES_V3_RX_UCDR_FASTLOCK_FO_GAIN		0x030
 #define QSERDES_V3_RX_UCDR_SO_SATURATION_AND_ENABLE	0x034
 #define QSERDES_V3_RX_RX_TERM_BW			0x07c
+#define QSERDES_V3_RX_VGA_CAL_CNTRL1			0x0bc
+#define QSERDES_V3_RX_VGA_CAL_CNTRL2			0x0c0
 #define QSERDES_V3_RX_RX_EQ_GAIN2_LSB			0x0c8
 #define QSERDES_V3_RX_RX_EQ_GAIN2_MSB			0x0cc
 #define QSERDES_V3_RX_RX_EQU_ADAPTOR_CNTRL2		0x0d4
@@ -227,6 +229,7 @@
 #define QSERDES_V3_RX_SIGDET_DEGLITCH_CNTRL		0x10c
 #define QSERDES_V3_RX_RX_BAND				0x110
 #define QSERDES_V3_RX_RX_INTERFACE_MODE			0x11c
+#define QSERDES_V3_RX_RX_MODE_00			0x164
 
 /* Only for QMP V3 PHY - PCS registers */
 #define QPHY_V3_PCS_POWER_DOWN_CONTROL			0x004
@@ -273,6 +276,8 @@
 #define QPHY_V3_PCS_FLL_CNT_VAL_H_TOL			0x0d0
 #define QPHY_V3_PCS_FLL_MAN_CODE			0x0d4
 #define QPHY_V3_PCS_RX_SIGDET_LVL			0x1d8
+#define QPHY_V3_PCS_REFGEN_REQ_CONFIG1			0x20c
+#define QPHY_V3_PCS_REFGEN_REQ_CONFIG2			0x210
 
 /* Only for QMP V3 PHY - PCS_MISC registers */
 #define QPHY_V3_PCS_MISC_CLAMP_ENABLE			0x0c
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Manu Gautam]

To improve eye diagram for PHYs on different boards of same SOC,
some parameters may need to be changed. Provide device tree
properties to override these from board specific device tree
files. While at it, replace "qcom,qusb2-v2-phy" with compatible
string for USB2 PHY on sdm845 which was earlier added for
sdm845 only.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 .../devicetree/bindings/phy/qcom-qusb2-phy.txt        | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
index 42c9742..0ed140a 100644
--- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
+++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
@@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
 Required properties:
  - compatible: compatible list, contains
 	       "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
-	       "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
+	       "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
 
  - reg: offset and length of the PHY register set.
  - #phy-cells: must be 0.
@@ -27,6 +27,23 @@ Optional properties:
 		tuning parameter value for qusb2 phy.
 
  - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
+ - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be
+		added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
+		tuning paramter that may vary for different boards of same SOC.
+ - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
+		output current. 0x0 value corresponding to 24mA which is maximum
+		current and 0xf corresponds to lowest current which is 15mA.
+ - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level.
+		Possible values are:
+		00: NONE
+		01: +5%
+		10: +10%
+		11: +15%
+- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX
+		pre-emphasis (specified using qcom,preemphasis-level) must be in
+		effect. Possible values are:
+		0: Full-bit width
+		1: Half-bit width
 
 Example:
 	hsusb_phy: phy@7411000 {
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> To improve eye diagram for PHYs on different boards of same SOC,
> some parameters may need to be changed. Provide device tree
> properties to override these from board specific device tree
> files. While at it, replace "qcom,qusb2-v2-phy" with compatible
> string for USB2 PHY on sdm845 which was earlier added for
> sdm845 only.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt        | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> index 42c9742..0ed140a 100644
> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>  Required properties:
>   - compatible: compatible list, contains
>                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
> -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
> +              "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
>
>   - reg: offset and length of the PHY register set.
>   - #phy-cells: must be 0.
> @@ -27,6 +27,23 @@ Optional properties:
>                 tuning parameter value for qusb2 phy.
>
>   - qcom,tcsr-syscon: Phandle to TCSR syscon register region.

Just to confirm: the new properties below work just fine on the old
msm8996 PHY too, right?


> + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be
> +               added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
> +               tuning paramter that may vary for different boards of same SOC.
> + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
> +               output current. 0x0 value corresponding to 24mA which is maximum
> +               current and 0xf corresponds to lowest current which is 15mA.
> + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level.
> +               Possible values are:
> +               00: NONE
> +               01: +5%
> +               10: +10%
> +               11: +15%

The user of the device tree will expect to specify this in decimal,
right?  So list the above as 0, 1, 2, 3.  ...not 00, 01, 10, 11.
(though below I suggest that specifying 0, 1, 2, 3 is probably not
quite the right way to describe this property).


> +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX
> +               pre-emphasis (specified using qcom,preemphasis-level) must be in
> +               effect. Possible values are:
> +               0: Full-bit width
> +               1: Half-bit width

Perhaps just make this a boolean property.  If it exists then you get
the non-default case.  AKA: if the default is full bit width, then
you'd allow a boolean property "qcom,preemphasis-half-width" to
override.  If the default is half bit width then you'd allow
"qcom,preemphasis-full-width" to override.


For all the above optional bits, please indicate what the default is
if they aren't specified.  If you have to specify a different default
for sdm845 vs. msm8996 then so be it (though it would be nice to avoid
it by changing the default for sdm845 unless that's totally crazy).


Overall this looks pretty good to me.  Certainly the descriptions are
very understandable and this will make tuning on a per-board /
per-port basis much easier!  Thanks!


One last overall comment is that ideally we could make the device tree
a little bit more human readable.  Right now we'll have a set of
properties that looks like this (numbers made up):

qcom,imp-res-offset-value = <0x13>;
qcom,hstx-trim-value = <0x7>;
qcom,preemphasis-level = <1>;
qcom,preemphasis-width = <0>;


One option to make that more readable would be to change the units, AKA:

qcom,hstx-trim-ma = <18>;
qcom,preemphasis-percent = <5>;

...then the code would translate from these human-readable values to
the real numbers.


Another option would be to add a #include to the bindings.  I'd defer
to the wisdom of the bindings guys about if this is better or worse
than adding the units, but personally I like it better because:
* You get a compile-time error if you use an unsupported value.
* You don't need to add the code to translate.

So you'd add a ".h" file as part of this bindings patch (see git
history for other examples) and you'd have stuff like:

#define SDM845_QUSB2_TRIM_24MA 0
...
#define SDM845_QUSB2_TRIM_15MA 0xf

#define SDM845_QUSB2_PREEMPHASIS_NONE 0
#define SDM845_QUSB2_PREEMPHASIS_5_PERCENT 1
#define SDM845_QUSB2_PREEMPHASIS_10_PERCENT 2
#define SDM845_QUSB2_PREEMPHASIS_15_PERCENT 3

...then you'd use those #defines in the description of the properties.

--

One last note: from the current code
<https://patchwork.kernel.org/patch/10314925/> all of these new
properties need to be specified as 8-bit numbers, not 32-bit.  AKA:

qcom,imp-res-offset-value = /bits/ 8 <0x13>;
qcom,hstx-trim-value = /bits/ 8 <0x7>;
qcom,preemphasis-level = /bits/ 8 <1>;
qcom,preemphasis-width = /bits/ 8 <0>;


If we decide to keep it that way then it needs to be documented in the
bindings file.  IMHO we should just specify that these are normal
(32-bit) properties even if we expect that they'll always be 8-bits.
That seems to be commonplace elsewhere.



-Doug

Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Rob Herring]

On Thu, Mar 29, 2018 at 01:38:23PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> > To improve eye diagram for PHYs on different boards of same SOC,
> > some parameters may need to be changed. Provide device tree
> > properties to override these from board specific device tree
> > files. While at it, replace "qcom,qusb2-v2-phy" with compatible
> > string for USB2 PHY on sdm845 which was earlier added for
> > sdm845 only.
> >
> > Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> > ---
> >  .../devicetree/bindings/phy/qcom-qusb2-phy.txt        | 19 ++++++++++++++++++-
> >  1 file changed, 18 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> > index 42c9742..0ed140a 100644
> > --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> > +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
> > @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
> >  Required properties:
> >   - compatible: compatible list, contains
> >                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
> > -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
> > +              "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
> >
> >   - reg: offset and length of the PHY register set.
> >   - #phy-cells: must be 0.
> > @@ -27,6 +27,23 @@ Optional properties:
> >                 tuning parameter value for qusb2 phy.
> >
> >   - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
> 
> Just to confirm: the new properties below work just fine on the old
> msm8996 PHY too, right?
> 
> 
> > + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be
> > +               added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
> > +               tuning paramter that may vary for different boards of same SOC.
> > + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
> > +               output current. 0x0 value corresponding to 24mA which is maximum
> > +               current and 0xf corresponds to lowest current which is 15mA.
> > + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level.
> > +               Possible values are:
> > +               00: NONE
> > +               01: +5%
> > +               10: +10%
> > +               11: +15%
> 
> The user of the device tree will expect to specify this in decimal,
> right?  So list the above as 0, 1, 2, 3.  ...not 00, 01, 10, 11.
> (though below I suggest that specifying 0, 1, 2, 3 is probably not
> quite the right way to describe this property).
> 
> 
> > +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX
> > +               pre-emphasis (specified using qcom,preemphasis-level) must be in
> > +               effect. Possible values are:
> > +               0: Full-bit width
> > +               1: Half-bit width
> 
> Perhaps just make this a boolean property.  If it exists then you get
> the non-default case.  AKA: if the default is full bit width, then
> you'd allow a boolean property "qcom,preemphasis-half-width" to
> override.  If the default is half bit width then you'd allow
> "qcom,preemphasis-full-width" to override.
> 
> 
> For all the above optional bits, please indicate what the default is
> if they aren't specified.  If you have to specify a different default
> for sdm845 vs. msm8996 then so be it (though it would be nice to avoid
> it by changing the default for sdm845 unless that's totally crazy).
> 
> 
> Overall this looks pretty good to me.  Certainly the descriptions are
> very understandable and this will make tuning on a per-board /
> per-port basis much easier!  Thanks!
> 
> 
> One last overall comment is that ideally we could make the device tree
> a little bit more human readable.  Right now we'll have a set of
> properties that looks like this (numbers made up):
> 
> qcom,imp-res-offset-value = <0x13>;
> qcom,hstx-trim-value = <0x7>;
> qcom,preemphasis-level = <1>;
> qcom,preemphasis-width = <0>;
> 
> 
> One option to make that more readable would be to change the units, AKA:
> 
> qcom,hstx-trim-ma = <18>;
> qcom,preemphasis-percent = <5>;
> 
> ...then the code would translate from these human-readable values to
> the real numbers.

Yes, we often do that. However, there properties are very specific to 
this device and making the driver translate back to h/w values doesn't 
add much.

> Another option would be to add a #include to the bindings.  I'd defer
> to the wisdom of the bindings guys about if this is better or worse
> than adding the units, but personally I like it better because:
> * You get a compile-time error if you use an unsupported value.
> * You don't need to add the code to translate.

This is fine for me. Really, I'm fine with it as-is, but #defines make 
some people happy.

Rob

Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Manu Gautam]

On 4/10/2018 1:48 AM, Rob Herring wrote:
> On Thu, Mar 29, 2018 at 01:38:23PM -0700, Doug Anderson wrote:
>> Hi,
>>
>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>> To improve eye diagram for PHYs on different boards of same SOC,
>>> some parameters may need to be changed. Provide device tree
>>> properties to override these from board specific device tree
>>> files. While at it, replace "qcom,qusb2-v2-phy" with compatible
>>> string for USB2 PHY on sdm845 which was earlier added for
>>> sdm845 only.
>>>
>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>> ---
>>>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt        | 19 ++++++++++++++++++-
>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>> index 42c9742..0ed140a 100644
>>> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>> @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>>>  Required properties:
>>>   - compatible: compatible list, contains
>>>                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
>>> -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
>>> +              "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
>>>
>>>   - reg: offset and length of the PHY register set.
>>>   - #phy-cells: must be 0.
>>> @@ -27,6 +27,23 @@ Optional properties:
>>>                 tuning parameter value for qusb2 phy.
>>>
>>>   - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
>> Just to confirm: the new properties below work just fine on the old
>> msm8996 PHY too, right?
No it won't as register layouts and bit fields are different. Also IMP_CTRL
register doesnt exist for msm8996. I will mention that explicitly here.

>>
>>
>>> + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be
>>> +               added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
>>> +               tuning paramter that may vary for different boards of same SOC.
>>> + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
>>> +               output current. 0x0 value corresponding to 24mA which is maximum
>>> +               current and 0xf corresponds to lowest current which is 15mA.
>>> + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level.
>>> +               Possible values are:
>>> +               00: NONE
>>> +               01: +5%
>>> +               10: +10%
>>> +               11: +15%
>> The user of the device tree will expect to specify this in decimal,
>> right?  So list the above as 0, 1, 2, 3.  ...not 00, 01, 10, 11.
Fine.
>> (though below I suggest that specifying 0, 1, 2, 3 is probably not
>> quite the right way to describe this property).
>>
>>
>>> +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX
>>> +               pre-emphasis (specified using qcom,preemphasis-level) must be in
>>> +               effect. Possible values are:
>>> +               0: Full-bit width
>>> +               1: Half-bit width
>> Perhaps just make this a boolean property.  If it exists then you get
>> the non-default case.  AKA: if the default is full bit width, then
>> you'd allow a boolean property "qcom,preemphasis-half-width" to
>> override.  If the default is half bit width then you'd allow
>> "qcom,preemphasis-full-width" to override.

Default property value for an SOC is specified in driver and could vary from
soc to soc. Hence, from board devicetree for different SOCs we might need
to select separate widths overriding default driver values.
Alternative is to have two bool properties each for half and full-width. Did
you actually mean that?

>>
>>
>> For all the above optional bits, please indicate what the default is
>> if they aren't specified.  If you have to specify a different default
>> for sdm845 vs. msm8996 then so be it (though it would be nice to avoid
>> it by changing the default for sdm845 unless that's totally crazy).
Sure.
>>
>>
>> Overall this looks pretty good to me.  Certainly the descriptions are
>> very understandable and this will make tuning on a per-board /
>> per-port basis much easier!  Thanks!
>>
>>
>> One last overall comment is that ideally we could make the device tree
>> a little bit more human readable.  Right now we'll have a set of
>> properties that looks like this (numbers made up):
>>
>> qcom,imp-res-offset-value = <0x13>;
>> qcom,hstx-trim-value = <0x7>;
>> qcom,preemphasis-level = <1>;
>> qcom,preemphasis-width = <0>;
>>
>>
>> One option to make that more readable would be to change the units, AKA:
>>
>> qcom,hstx-trim-ma = <18>;
>> qcom,preemphasis-percent = <5>;
>>
>> ...then the code would translate from these human-readable values to
>> the real numbers.
> Yes, we often do that. However, there properties are very specific to 
> this device and making the driver translate back to h/w values doesn't 
> add much.

Thanks for review Rob. I too agree with both the viewpoints.
Doug, if it is not of much concern then can I stick with current approach?

>
>> Another option would be to add a #include to the bindings.  I'd defer
>> to the wisdom of the bindings guys about if this is better or worse
>> than adding the units, but personally I like it better because:
>> * You get a compile-time error if you use an unsupported value.
>> * You don't need to add the code to translate.
> This is fine for me. Really, I'm fine with it as-is, but #defines make 
> some people happy.
>
> Rob

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Doug Anderson]

Hi,

On Tue, Apr 10, 2018 at 12:16 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>
>
> On 4/10/2018 1:48 AM, Rob Herring wrote:
>> On Thu, Mar 29, 2018 at 01:38:23PM -0700, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>>>> To improve eye diagram for PHYs on different boards of same SOC,
>>>> some parameters may need to be changed. Provide device tree
>>>> properties to override these from board specific device tree
>>>> files. While at it, replace "qcom,qusb2-v2-phy" with compatible
>>>> string for USB2 PHY on sdm845 which was earlier added for
>>>> sdm845 only.
>>>>
>>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
>>>> ---
>>>>  .../devicetree/bindings/phy/qcom-qusb2-phy.txt        | 19 ++++++++++++++++++-
>>>>  1 file changed, 18 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>>> index 42c9742..0ed140a 100644
>>>> --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>>> +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt
>>>> @@ -6,7 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets.
>>>>  Required properties:
>>>>   - compatible: compatible list, contains
>>>>                "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996,
>>>> -              "qcom,qusb2-v2-phy" for QUSB2 V2 PHY.
>>>> +              "qcom,sdm845-qusb2-phy" for 10nm PHY on sdm845.
>>>>
>>>>   - reg: offset and length of the PHY register set.
>>>>   - #phy-cells: must be 0.
>>>> @@ -27,6 +27,23 @@ Optional properties:
>>>>                 tuning parameter value for qusb2 phy.
>>>>
>>>>   - qcom,tcsr-syscon: Phandle to TCSR syscon register region.
>>> Just to confirm: the new properties below work just fine on the old
>>> msm8996 PHY too, right?
> No it won't as register layouts and bit fields are different. Also IMP_CTRL
> register doesnt exist for msm8996. I will mention that explicitly here.

Thanks!  Yeah, definitely document which properties only work on
sdm845 and which ones work everywhere.


>>>> + - qcom,imp-res-offset-value: It is a 6 bit value that specifies offset to be
>>>> +               added to PHY refgen RESCODE via IMP_CTRL1 register. It is a PHY
>>>> +               tuning paramter that may vary for different boards of same SOC.
>>>> + - qcom,hstx-trim-value: It is a 4 bit value that specifies tuning for HSTX
>>>> +               output current. 0x0 value corresponding to 24mA which is maximum
>>>> +               current and 0xf corresponds to lowest current which is 15mA.
>>>> + - qcom,preemphasis-level: It is a 2 bit value that specifies pre-emphasis level.
>>>> +               Possible values are:
>>>> +               00: NONE
>>>> +               01: +5%
>>>> +               10: +10%
>>>> +               11: +15%
>>> The user of the device tree will expect to specify this in decimal,
>>> right?  So list the above as 0, 1, 2, 3.  ...not 00, 01, 10, 11.
> Fine.
>>> (though below I suggest that specifying 0, 1, 2, 3 is probably not
>>> quite the right way to describe this property).
>>>
>>>
>>>> +- qcom,preemphasis-width: It is a 1 bit value that specifies how long the HSTX
>>>> +               pre-emphasis (specified using qcom,preemphasis-level) must be in
>>>> +               effect. Possible values are:
>>>> +               0: Full-bit width
>>>> +               1: Half-bit width
>>> Perhaps just make this a boolean property.  If it exists then you get
>>> the non-default case.  AKA: if the default is full bit width, then
>>> you'd allow a boolean property "qcom,preemphasis-half-width" to
>>> override.  If the default is half bit width then you'd allow
>>> "qcom,preemphasis-full-width" to override.
>
> Default property value for an SOC is specified in driver and could vary from
> soc to soc. Hence, from board devicetree for different SOCs we might need
> to select separate widths overriding default driver values.
> Alternative is to have two bool properties each for half and full-width. Did
> you actually mean that?

It's OK to keep it as-is then.  ...but please document what the
default is for every SoC/port combination that supports this property.


>>> For all the above optional bits, please indicate what the default is
>>> if they aren't specified.  If you have to specify a different default
>>> for sdm845 vs. msm8996 then so be it (though it would be nice to avoid
>>> it by changing the default for sdm845 unless that's totally crazy).
> Sure.
>>>
>>>
>>> Overall this looks pretty good to me.  Certainly the descriptions are
>>> very understandable and this will make tuning on a per-board /
>>> per-port basis much easier!  Thanks!
>>>
>>>
>>> One last overall comment is that ideally we could make the device tree
>>> a little bit more human readable.  Right now we'll have a set of
>>> properties that looks like this (numbers made up):
>>>
>>> qcom,imp-res-offset-value = <0x13>;
>>> qcom,hstx-trim-value = <0x7>;
>>> qcom,preemphasis-level = <1>;
>>> qcom,preemphasis-width = <0>;
>>>
>>>
>>> One option to make that more readable would be to change the units, AKA:
>>>
>>> qcom,hstx-trim-ma = <18>;
>>> qcom,preemphasis-percent = <5>;
>>>
>>> ...then the code would translate from these human-readable values to
>>> the real numbers.
>> Yes, we often do that. However, there properties are very specific to
>> this device and making the driver translate back to h/w values doesn't
>> add much.
>
> Thanks for review Rob. I too agree with both the viewpoints.
> Doug, if it is not of much concern then can I stick with current approach?

I certainly would appreciate the #defines and believe they add to the
readability, but if you're dead set against it and Rob says it's OK, I
won't yell too loudly.


>>> Another option would be to add a #include to the bindings.  I'd defer
>>> to the wisdom of the bindings guys about if this is better or worse
>>> than adding the units, but personally I like it better because:
>>> * You get a compile-time error if you use an unsupported value.
>>> * You don't need to add the code to translate.
>> This is fine for me. Really, I'm fine with it as-is, but #defines make
>> some people happy.
>>
>> Rob

Re: [PATCH v4 6/7] dt-bindings: phy-qcom-usb2: Add support to override tuning values

[Manu Gautam]

Hi,


On 4/13/2018 2:17 AM, Doug Anderson wrote:
>> Thanks for review Rob. I too agree with both the viewpoints.
>> Doug, if it is not of much concern then can I stick with current approach?
> I certainly would appreciate the #defines and believe they add to the
> readability, but if you're dead set against it and Rob says it's OK, I
> won't yell too loudly.
>
Agree to the readability part. Will add #defines as you suggested.
-thanks

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Manu Gautam]

There are two QUSB2 PHYs present on sdm845. In order
to improve eye diagram for both the PHYs some parameters
need to be changed. Provide device tree properties to
override these from board specific device tree files.

Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
---
 drivers/phy/qualcomm/phy-qcom-qusb2.c | 112 +++++++++++++++++++++++++++++++++-
 1 file changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
index 40fdef8..adcc3f8 100644
--- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
+++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
@@ -60,6 +60,17 @@
 #define CORE_RESET				BIT(5)
 #define CORE_RESET_MUX				BIT(6)
 
+/* QUSB2PHY_IMP_CTRL1 register bits */
+#define IMP_RES_OFFSET_MASK			GENMASK(5, 0)
+#define IMP_RES_OFFSET_SHIFT			0x0
+
+/* QUSB2PHY_PORT_TUNE1 register bits */
+#define HSTX_TRIM_MASK				GENMASK(7, 4)
+#define HSTX_TRIM_SHIFT				0x4
+#define PREEMPH_HALF_WIDTH			BIT(2)
+#define PREEMPHASIS_EN_MASK			GENMASK(1, 0)
+#define PREEMPHASIS_EN_SHIFT			0x0
+
 #define QUSB2PHY_PLL_ANALOG_CONTROLS_TWO	0x04
 #define QUSB2PHY_PLL_CLOCK_INVERTERS		0x18c
 #define QUSB2PHY_PLL_CMODE			0x2c
@@ -241,6 +252,18 @@ struct qusb2_phy_cfg {
  * @tcsr: TCSR syscon register map
  * @cell: nvmem cell containing phy tuning value
  *
+ * @override_imp_res_offset: PHY should use different rescode offset
+ * @imp_res_offset_value: rescode offset to be updated in IMP_CTRL1 register
+ *
+ * @override_hstx_trim: PHY should use different HSTX o/p current value
+ * @hstx_trim_value: HSTX_TRIM value to be updated in TUNE1 register
+ *
+ * @override_preemphasis: PHY should use different pre-amphasis amplitude
+ * @preemphasis_level: Amplitude Pre-Emphasis to be updated in TUNE1 register
+ *
+ * @override_preemphasis_width: PHY should use different pre-emphasis duration
+ * @preemphasis_width: half/full-width Pre-Emphasis updated via TUNE1
+ *
  * @cfg: phy config data
  * @has_se_clk_scheme: indicate if PHY has single-ended ref clock scheme
  * @phy_initialized: indicate if PHY has been initialized
@@ -259,12 +282,35 @@ struct qusb2_phy {
 	struct regmap *tcsr;
 	struct nvmem_cell *cell;
 
+	bool override_imp_res_offset;
+	u8 imp_res_offset_value;
+	bool override_hstx_trim;
+	u8 hstx_trim_value;
+	bool override_preemphasis;
+	u8 preemphasis_level;
+	bool override_preemphasis_width;
+	u8 preemphasis_width;
+
 	const struct qusb2_phy_cfg *cfg;
 	bool has_se_clk_scheme;
 	bool phy_initialized;
 	enum phy_mode mode;
 };
 
+static inline void qusb2_write_mask(void __iomem *base, u32 offset,
+				    u32 val, u32 mask)
+{
+	u32 reg;
+
+	reg = readl(base + offset);
+	reg &= ~mask;
+	reg |= val;
+	writel(reg, base + offset);
+
+	/* Ensure above write is completed */
+	readl(base + offset);
+}
+
 static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
 {
 	u32 reg;
@@ -305,6 +351,42 @@ void qcom_qusb2_phy_configure(void __iomem *base,
 }
 
 /*
+ * Update board specific PHY tuning override values if specified from
+ * device tree.
+ *
+ */
+static void qusb2_phy_override_phy_params(struct qusb2_phy *qphy)
+{
+	const struct qusb2_phy_cfg *cfg = qphy->cfg;
+
+	if (qphy->override_imp_res_offset)
+		qusb2_write_mask(qphy->base, QUSB2PHY_IMP_CTRL1,
+			     qphy->imp_res_offset_value << IMP_RES_OFFSET_SHIFT,
+			     IMP_RES_OFFSET_MASK);
+
+	if (qphy->override_hstx_trim)
+		qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
+				 qphy->hstx_trim_value << HSTX_TRIM_SHIFT,
+				 HSTX_TRIM_MASK);
+
+	if (qphy->override_preemphasis)
+		qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
+				qphy->preemphasis_level << PREEMPHASIS_EN_SHIFT,
+				PREEMPHASIS_EN_MASK);
+
+	if (qphy->override_preemphasis_width) {
+		if (qphy->preemphasis_width)
+			qusb2_setbits(qphy->base,
+				      cfg->regs[QUSB2PHY_PORT_TUNE1],
+				      PREEMPH_HALF_WIDTH);
+		else
+			qusb2_clrbits(qphy->base,
+				      cfg->regs[QUSB2PHY_PORT_TUNE1],
+				      PREEMPH_HALF_WIDTH);
+	}
+}
+
+/*
  * Fetches HS Tx tuning value from nvmem and sets the
  * QUSB2PHY_PORT_TUNE1/2 register.
  * For error case, skip setting the value and use the default value.
@@ -525,6 +607,9 @@ static int qusb2_phy_init(struct phy *phy)
 	qcom_qusb2_phy_configure(qphy->base, cfg->regs, cfg->tbl,
 				 cfg->tbl_num);
 
+	/* Override board specific PHY tuning values */
+	qusb2_phy_override_phy_params(qphy);
+
 	/* Set efuse value for tuning the PHY */
 	qusb2_phy_set_tune2_param(qphy);
 
@@ -647,7 +732,7 @@ static int qusb2_phy_exit(struct phy *phy)
 		.compatible	= "qcom,msm8996-qusb2-phy",
 		.data		= &msm8996_phy_cfg,
 	}, {
-		.compatible	= "qcom,qusb2-v2-phy",
+		.compatible	= "qcom,sdm845-qusb2-phy",
 		.data		= &qusb2_v2_phy_cfg,
 	},
 	{ },
@@ -736,6 +821,31 @@ static int qusb2_phy_probe(struct platform_device *pdev)
 		qphy->cell = NULL;
 		dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
 	}
+
+	if (of_find_property(dev->of_node, "qcom,imp-res-offset-value", NULL)) {
+		qphy->override_imp_res_offset = true;
+		of_property_read_u8(dev->of_node, "qcom,imp-res-offset-value",
+				    &qphy->imp_res_offset_value);
+	}
+
+	if (of_find_property(dev->of_node, "qcom,hstx-trim-value", NULL)) {
+		qphy->override_hstx_trim = true;
+		of_property_read_u8(dev->of_node, "qcom,hstx-trim-value",
+				    &qphy->hstx_trim_value);
+	}
+
+	if (of_find_property(dev->of_node, "qcom,preemphasis-level", NULL)) {
+		qphy->override_preemphasis = true;
+		of_property_read_u8(dev->of_node, "qcom,preemphasis-level",
+				    &qphy->preemphasis_level);
+	}
+
+	if (of_find_property(dev->of_node, "qcom,preemphasis-width", NULL)) {
+		qphy->override_preemphasis_width = true;
+		of_property_read_u8(dev->of_node, "qcom,preemphasis-width",
+				    &qphy->preemphasis_width);
+	}
+
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 	/*
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Doug Anderson]

Hi,

On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
> There are two QUSB2 PHYs present on sdm845. In order
> to improve eye diagram for both the PHYs some parameters
> need to be changed. Provide device tree properties to
> override these from board specific device tree files.
>
> Signed-off-by: Manu Gautam <mgautam@codeaurora.org>
> ---
>  drivers/phy/qualcomm/phy-qcom-qusb2.c | 112 +++++++++++++++++++++++++++++++++-
>  1 file changed, 111 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qusb2.c b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> index 40fdef8..adcc3f8 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qusb2.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qusb2.c
> @@ -60,6 +60,17 @@
>  #define CORE_RESET                             BIT(5)
>  #define CORE_RESET_MUX                         BIT(6)
>
> +/* QUSB2PHY_IMP_CTRL1 register bits */
> +#define IMP_RES_OFFSET_MASK                    GENMASK(5, 0)
> +#define IMP_RES_OFFSET_SHIFT                   0x0
> +
> +/* QUSB2PHY_PORT_TUNE1 register bits */
> +#define HSTX_TRIM_MASK                         GENMASK(7, 4)
> +#define HSTX_TRIM_SHIFT                                0x4
> +#define PREEMPH_HALF_WIDTH                     BIT(2)
> +#define PREEMPHASIS_EN_MASK                    GENMASK(1, 0)
> +#define PREEMPHASIS_EN_SHIFT                   0x0
> +
>  #define QUSB2PHY_PLL_ANALOG_CONTROLS_TWO       0x04
>  #define QUSB2PHY_PLL_CLOCK_INVERTERS           0x18c
>  #define QUSB2PHY_PLL_CMODE                     0x2c
> @@ -241,6 +252,18 @@ struct qusb2_phy_cfg {
>   * @tcsr: TCSR syscon register map
>   * @cell: nvmem cell containing phy tuning value
>   *
> + * @override_imp_res_offset: PHY should use different rescode offset
> + * @imp_res_offset_value: rescode offset to be updated in IMP_CTRL1 register
> + *
> + * @override_hstx_trim: PHY should use different HSTX o/p current value
> + * @hstx_trim_value: HSTX_TRIM value to be updated in TUNE1 register
> + *
> + * @override_preemphasis: PHY should use different pre-amphasis amplitude
> + * @preemphasis_level: Amplitude Pre-Emphasis to be updated in TUNE1 register
> + *
> + * @override_preemphasis_width: PHY should use different pre-emphasis duration
> + * @preemphasis_width: half/full-width Pre-Emphasis updated via TUNE1
> + *

nit: spacing here doesn't match spacing in the structure.  AKA: you've
smashed together all 8 properties in the structure but not in the
description.


>   * @cfg: phy config data
>   * @has_se_clk_scheme: indicate if PHY has single-ended ref clock scheme
>   * @phy_initialized: indicate if PHY has been initialized
> @@ -259,12 +282,35 @@ struct qusb2_phy {
>         struct regmap *tcsr;
>         struct nvmem_cell *cell;
>
> +       bool override_imp_res_offset;
> +       u8 imp_res_offset_value;
> +       bool override_hstx_trim;
> +       u8 hstx_trim_value;
> +       bool override_preemphasis;
> +       u8 preemphasis_level;
> +       bool override_preemphasis_width;
> +       u8 preemphasis_width;
> +
>         const struct qusb2_phy_cfg *cfg;
>         bool has_se_clk_scheme;
>         bool phy_initialized;
>         enum phy_mode mode;
>  };
>
> +static inline void qusb2_write_mask(void __iomem *base, u32 offset,
> +                                   u32 val, u32 mask)
> +{
> +       u32 reg;
> +
> +       reg = readl(base + offset);
> +       reg &= ~mask;
> +       reg |= val;

"reg |= (val & mask)" instead of just "reg |= val"

You don't do any bounds checking of the device tree entries and this
will at least make sure that a bad value for a field won't screw up
other fields (and so, presumably, it will be easier to find the bug).


> +       writel(reg, base + offset);
> +
> +       /* Ensure above write is completed */
> +       readl(base + offset);

You're using readl() and writel() which have barriers.  Why do you
need this extra readl()?


> +}
> +
>  static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
>  {
>         u32 reg;
> @@ -305,6 +351,42 @@ void qcom_qusb2_phy_configure(void __iomem *base,
>  }
>
>  /*
> + * Update board specific PHY tuning override values if specified from
> + * device tree.
> + *

nit: remove extra comment line with just a "*" on it.


> + */
> +static void qusb2_phy_override_phy_params(struct qusb2_phy *qphy)
> +{
> +       const struct qusb2_phy_cfg *cfg = qphy->cfg;
> +
> +       if (qphy->override_imp_res_offset)
> +               qusb2_write_mask(qphy->base, QUSB2PHY_IMP_CTRL1,
> +                            qphy->imp_res_offset_value << IMP_RES_OFFSET_SHIFT,
> +                            IMP_RES_OFFSET_MASK);
> +
> +       if (qphy->override_hstx_trim)
> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
> +                                qphy->hstx_trim_value << HSTX_TRIM_SHIFT,
> +                                HSTX_TRIM_MASK);
> +
> +       if (qphy->override_preemphasis)
> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
> +                               qphy->preemphasis_level << PREEMPHASIS_EN_SHIFT,
> +                               PREEMPHASIS_EN_MASK);
> +
> +       if (qphy->override_preemphasis_width) {
> +               if (qphy->preemphasis_width)
> +                       qusb2_setbits(qphy->base,
> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
> +                                     PREEMPH_HALF_WIDTH);
> +               else
> +                       qusb2_clrbits(qphy->base,
> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
> +                                     PREEMPH_HALF_WIDTH);
> +       }
> +}
> +
> +/*
>   * Fetches HS Tx tuning value from nvmem and sets the
>   * QUSB2PHY_PORT_TUNE1/2 register.
>   * For error case, skip setting the value and use the default value.
> @@ -525,6 +607,9 @@ static int qusb2_phy_init(struct phy *phy)
>         qcom_qusb2_phy_configure(qphy->base, cfg->regs, cfg->tbl,
>                                  cfg->tbl_num);
>
> +       /* Override board specific PHY tuning values */
> +       qusb2_phy_override_phy_params(qphy);
> +
>         /* Set efuse value for tuning the PHY */
>         qusb2_phy_set_tune2_param(qphy);
>
> @@ -647,7 +732,7 @@ static int qusb2_phy_exit(struct phy *phy)
>                 .compatible     = "qcom,msm8996-qusb2-phy",
>                 .data           = &msm8996_phy_cfg,
>         }, {
> -               .compatible     = "qcom,qusb2-v2-phy",
> +               .compatible     = "qcom,sdm845-qusb2-phy",
>                 .data           = &qusb2_v2_phy_cfg,

Can you change the name of the structure to match (AKA include sdm845?)


>         },
>         { },
> @@ -736,6 +821,31 @@ static int qusb2_phy_probe(struct platform_device *pdev)
>                 qphy->cell = NULL;
>                 dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
>         }
> +
> +       if (of_find_property(dev->of_node, "qcom,imp-res-offset-value", NULL)) {

Get rid of the extra of_find_property().  Just use the error code from
the property_read() to know if it was there and you've done two steps
in one (read it and know if it was there).


> +               qphy->override_imp_res_offset = true;
> +               of_property_read_u8(dev->of_node, "qcom,imp-res-offset-value",
> +                                   &qphy->imp_res_offset_value);

You probably don't want of_property_read_u8(), even if you intend the
property to bit in 8 bits.  You probably want to use
of_property_read_u32() to read into a temporary value and then copy
that to your 8-bit structure element.

If you use of_property_read_u8 then you have to "/bits/ 8" in the
device tree and that's ugly and doesn't seem to be what's done very
often.  People just always stick a u32 in the device tree.



-Doug

Re: [PATCH v4 7/7] phy: qcom-qusb2: Add QUSB2 PHYs support for sdm845

[Manu Gautam]

Hi,


On 3/30/2018 2:08 AM, Doug Anderson wrote:
> Hi,
>
> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote:
>> @@ -241,6 +252,18 @@ struct qusb2_phy_cfg {
>>   * @tcsr: TCSR syscon register map
>>   * @cell: nvmem cell containing phy tuning value
>>   *
>> + * @override_imp_res_offset: PHY should use different rescode offset
>> + * @imp_res_offset_value: rescode offset to be updated in IMP_CTRL1 register
>> + *
>> + * @override_hstx_trim: PHY should use different HSTX o/p current value
>> + * @hstx_trim_value: HSTX_TRIM value to be updated in TUNE1 register
>> + *
>> + * @override_preemphasis: PHY should use different pre-amphasis amplitude
>> + * @preemphasis_level: Amplitude Pre-Emphasis to be updated in TUNE1 register
>> + *
>> + * @override_preemphasis_width: PHY should use different pre-emphasis duration
>> + * @preemphasis_width: half/full-width Pre-Emphasis updated via TUNE1
>> + *
> nit: spacing here doesn't match spacing in the structure.  AKA: you've
> smashed together all 8 properties in the structure but not in the
> description.
Will fix it.
>
>
>>   * @cfg: phy config data
>>   * @has_se_clk_scheme: indicate if PHY has single-ended ref clock scheme
>>   * @phy_initialized: indicate if PHY has been initialized
>> @@ -259,12 +282,35 @@ struct qusb2_phy {
>>         struct regmap *tcsr;
>>         struct nvmem_cell *cell;
>>
>> +       bool override_imp_res_offset;
>> +       u8 imp_res_offset_value;
>> +       bool override_hstx_trim;
>> +       u8 hstx_trim_value;
>> +       bool override_preemphasis;
>> +       u8 preemphasis_level;
>> +       bool override_preemphasis_width;
>> +       u8 preemphasis_width;
>> +
>>         const struct qusb2_phy_cfg *cfg;
>>         bool has_se_clk_scheme;
>>         bool phy_initialized;
>>         enum phy_mode mode;
>>  };
>>
>> +static inline void qusb2_write_mask(void __iomem *base, u32 offset,
>> +                                   u32 val, u32 mask)
>> +{
>> +       u32 reg;
>> +
>> +       reg = readl(base + offset);
>> +       reg &= ~mask;
>> +       reg |= val;
> "reg |= (val & mask)" instead of just "reg |= val"
>
> You don't do any bounds checking of the device tree entries and this
> will at least make sure that a bad value for a field won't screw up
> other fields (and so, presumably, it will be easier to find the bug).
>
It makes sense. Will change accordingly.

>
>> +       writel(reg, base + offset);
>> +
>> +       /* Ensure above write is completed */
>> +       readl(base + offset);
> You're using readl() and writel() which have barriers.  Why do you
> need this extra readl()?

This requirement comes from AHB2PHY wrapper designers and HPG.
Also existing qusb2_setbits/clrbits() wrapper already have similar handling.

>
>
>> +}
>> +
>>  static inline void qusb2_setbits(void __iomem *base, u32 offset, u32 val)
>>  {
>>         u32 reg;
>> @@ -305,6 +351,42 @@ void qcom_qusb2_phy_configure(void __iomem *base,
>>  }
>>
>>  /*
>> + * Update board specific PHY tuning override values if specified from
>> + * device tree.
>> + *
> nit: remove extra comment line with just a "*" on it.
Sure.
>
>
>> + */
>> +static void qusb2_phy_override_phy_params(struct qusb2_phy *qphy)
>> +{
>> +       const struct qusb2_phy_cfg *cfg = qphy->cfg;
>> +
>> +       if (qphy->override_imp_res_offset)
>> +               qusb2_write_mask(qphy->base, QUSB2PHY_IMP_CTRL1,
>> +                            qphy->imp_res_offset_value << IMP_RES_OFFSET_SHIFT,
>> +                            IMP_RES_OFFSET_MASK);
>> +
>> +       if (qphy->override_hstx_trim)
>> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                qphy->hstx_trim_value << HSTX_TRIM_SHIFT,
>> +                                HSTX_TRIM_MASK);
>> +
>> +       if (qphy->override_preemphasis)
>> +               qusb2_write_mask(qphy->base, cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                               qphy->preemphasis_level << PREEMPHASIS_EN_SHIFT,
>> +                               PREEMPHASIS_EN_MASK);
>> +
>> +       if (qphy->override_preemphasis_width) {
>> +               if (qphy->preemphasis_width)
>> +                       qusb2_setbits(qphy->base,
>> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                     PREEMPH_HALF_WIDTH);
>> +               else
>> +                       qusb2_clrbits(qphy->base,
>> +                                     cfg->regs[QUSB2PHY_PORT_TUNE1],
>> +                                     PREEMPH_HALF_WIDTH);
>> +       }
>> +}
>> +
>> +/*
>>   * Fetches HS Tx tuning value from nvmem and sets the
>>   * QUSB2PHY_PORT_TUNE1/2 register.
>>   * For error case, skip setting the value and use the default value.
>> @@ -525,6 +607,9 @@ static int qusb2_phy_init(struct phy *phy)
>>         qcom_qusb2_phy_configure(qphy->base, cfg->regs, cfg->tbl,
>>                                  cfg->tbl_num);
>>
>> +       /* Override board specific PHY tuning values */
>> +       qusb2_phy_override_phy_params(qphy);
>> +
>>         /* Set efuse value for tuning the PHY */
>>         qusb2_phy_set_tune2_param(qphy);
>>
>> @@ -647,7 +732,7 @@ static int qusb2_phy_exit(struct phy *phy)
>>                 .compatible     = "qcom,msm8996-qusb2-phy",
>>                 .data           = &msm8996_phy_cfg,
>>         }, {
>> -               .compatible     = "qcom,qusb2-v2-phy",
>> +               .compatible     = "qcom,sdm845-qusb2-phy",
>>                 .data           = &qusb2_v2_phy_cfg,
> Can you change the name of the structure to match (AKA include sdm845?)

OK
>
>
>>         },
>>         { },
>> @@ -736,6 +821,31 @@ static int qusb2_phy_probe(struct platform_device *pdev)
>>                 qphy->cell = NULL;
>>                 dev_dbg(dev, "failed to lookup tune2 hstx trim value\n");
>>         }
>> +
>> +       if (of_find_property(dev->of_node, "qcom,imp-res-offset-value", NULL)) {
> Get rid of the extra of_find_property().  Just use the error code from
> the property_read() to know if it was there and you've done two steps
> in one (read it and know if it was there).
>
That is better. Thanks.

>> +               qphy->override_imp_res_offset = true;
>> +               of_property_read_u8(dev->of_node, "qcom,imp-res-offset-value",
>> +                                   &qphy->imp_res_offset_value);
> You probably don't want of_property_read_u8(), even if you intend the
> property to bit in 8 bits.  You probably want to use
> of_property_read_u32() to read into a temporary value and then copy
> that to your 8-bit structure element.
>
> If you use of_property_read_u8 then you have to "/bits/ 8" in the
> device tree and that's ugly and doesn't seem to be what's done very
> often.  People just always stick a u32 in the device tree.

Sure, will do that.

>
>
> -Doug

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project