Re: [PATCH v6 1/3] phy: Update PHY power control sequence

2018-06-13 Thread cang 

On 2018-06-12 19:34, Vivek Gautam wrote:

Hi Can,


On 5/29/2018 10:07 AM, Can Guo wrote:
All PHYs should be powered on before register configuration starts. 
And
only PCIe PHYs need an extra power control before deasserts reset 
state.


Signed-off-by: Can Guo 
---
  drivers/phy/qualcomm/phy-qcom-qmp.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c

index 97ef942..f779b0f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -982,6 +982,8 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp 
*qmp)

if (cfg->has_phy_com_ctrl)
qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
 SW_PWRDN);
+   else
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);


No definition of 'pcs' in this function. You are doing that in the 
second patch.

But, we should add this definition here.

Also instead of having the change like this:

+   struct qmp_phy *qphy = qmp->phys[0];
void __iomem *serdes = qmp->serdes;
+   void __iomem *pcs = qphy->pcs;

Let's pass 'struct qmp_phy' to qcom_qmp_phy_com_init(), and then get
'struct qcom_qmp' and 'void __iomem *pcs' from that.

So,

-static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
+static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
 {
+   struct qcom_qmp *qmp = qphy->qmp;
    const struct qmp_phy_cfg *cfg = qmp->cfg;
    void __iomem *serdes = qmp->serdes;
    void __iomem *dp_com = qmp->dp_com;
+   void __iomem *pcs = qphy->pcs;

and

-   ret = qcom_qmp_phy_com_init(qmp);
+   ret = qcom_qmp_phy_com_init(qphy);

That looks cleaner than extracting from the 0th phys.

BRs
Vivek


Sure Vivek


if (cfg->has_phy_dp_com_ctrl) {
qphy_setbits(dp_com, QPHY_V3_DP_COM_POWER_DOWN_CTRL,
@@ -1127,7 +1129,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
 * Pull out PHY from POWER DOWN state.
 * This is active low enable signal to power-down PHY.
 */
-   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+   if (cfg->type == PHY_TYPE_PCIE)
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
if (cfg->has_pwrdn_delay)
usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);

Re: [PATCH v6 1/3] phy: Update PHY power control sequence

2018-06-12 Thread Vivek Gautam 

Hi Can,


On 5/29/2018 10:07 AM, Can Guo wrote:

All PHYs should be powered on before register configuration starts. And
only PCIe PHYs need an extra power control before deasserts reset state.

Signed-off-by: Can Guo 
---
  drivers/phy/qualcomm/phy-qcom-qmp.c | 5 -
  1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 97ef942..f779b0f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -982,6 +982,8 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
if (cfg->has_phy_com_ctrl)
qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
 SW_PWRDN);
+   else
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);


No definition of 'pcs' in this function. You are doing that in the 
second patch.

But, we should add this definition here.

Also instead of having the change like this:

+   struct qmp_phy *qphy = qmp->phys[0];
void __iomem *serdes = qmp->serdes;
+   void __iomem *pcs = qphy->pcs;

Let's pass 'struct qmp_phy' to qcom_qmp_phy_com_init(), and then get
'struct qcom_qmp' and 'void __iomem *pcs' from that.

So,

-static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
+static int qcom_qmp_phy_com_init(struct qmp_phy *qphy)
 {
+   struct qcom_qmp *qmp = qphy->qmp;
    const struct qmp_phy_cfg *cfg = qmp->cfg;
    void __iomem *serdes = qmp->serdes;
    void __iomem *dp_com = qmp->dp_com;
+   void __iomem *pcs = qphy->pcs;

and

-   ret = qcom_qmp_phy_com_init(qmp);
+   ret = qcom_qmp_phy_com_init(qphy);

That looks cleaner than extracting from the 0th phys.

BRs
Vivek
  
  	if (cfg->has_phy_dp_com_ctrl) {

qphy_setbits(dp_com, QPHY_V3_DP_COM_POWER_DOWN_CTRL,
@@ -1127,7 +1129,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
 * Pull out PHY from POWER DOWN state.
 * This is active low enable signal to power-down PHY.
 */
-   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+   if (cfg->type == PHY_TYPE_PCIE)
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
  
  	if (cfg->has_pwrdn_delay)

usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);

Re: [PATCH v6 1/3] phy: Update PHY power control sequence

2018-06-11 Thread cang 

On 2018-06-08 14:45, Manu Gautam wrote:

Hi,

On 5/29/2018 10:07 AM, Can Guo wrote:
All PHYs should be powered on before register configuration starts. 
And
only PCIe PHYs need an extra power control before deasserts reset 
state.


Signed-off-by: Can Guo 
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c

index 97ef942..f779b0f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -982,6 +982,8 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp 
*qmp)

if (cfg->has_phy_com_ctrl)
qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
 SW_PWRDN);
+   else
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);


We should power-up PHYs after following dp_com_ctrl programming which
powers-off USB-DP combo PHY when it brings DP_COM block out of reset 
reset.





Sure Manu



if (cfg->has_phy_dp_com_ctrl) {
qphy_setbits(dp_com, QPHY_V3_DP_COM_POWER_DOWN_CTRL,
@@ -1127,7 +1129,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
 * Pull out PHY from POWER DOWN state.
 * This is active low enable signal to power-down PHY.
 */
-   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+   if (cfg->type == PHY_TYPE_PCIE)
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);

if (cfg->has_pwrdn_delay)
usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);

Re: [PATCH v6 1/3] phy: Update PHY power control sequence

2018-06-07 Thread Manu Gautam 
Hi,

On 5/29/2018 10:07 AM, Can Guo wrote:
> All PHYs should be powered on before register configuration starts. And
> only PCIe PHYs need an extra power control before deasserts reset state.
>
> Signed-off-by: Can Guo 
> ---
>  drivers/phy/qualcomm/phy-qcom-qmp.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
> b/drivers/phy/qualcomm/phy-qcom-qmp.c
> index 97ef942..f779b0f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
> @@ -982,6 +982,8 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
>   if (cfg->has_phy_com_ctrl)
>   qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
>SW_PWRDN);
> + else
> + qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);

We should power-up PHYs after following dp_com_ctrl programming which
powers-off USB-DP combo PHY when it brings DP_COM block out of reset reset.


>  
>   if (cfg->has_phy_dp_com_ctrl) {
>   qphy_setbits(dp_com, QPHY_V3_DP_COM_POWER_DOWN_CTRL,
> @@ -1127,7 +1129,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
>* Pull out PHY from POWER DOWN state.
>* This is active low enable signal to power-down PHY.
>*/
> - qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
> + if (cfg->type == PHY_TYPE_PCIE)
> + qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
>  
>   if (cfg->has_pwrdn_delay)
>   usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);

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

[PATCH v6 1/3] phy: Update PHY power control sequence

2018-05-28 Thread Can Guo 
All PHYs should be powered on before register configuration starts. And
only PCIe PHYs need an extra power control before deasserts reset state.

Signed-off-by: Can Guo 
---
 drivers/phy/qualcomm/phy-qcom-qmp.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c 
b/drivers/phy/qualcomm/phy-qcom-qmp.c
index 97ef942..f779b0f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp.c
@@ -982,6 +982,8 @@ static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp)
if (cfg->has_phy_com_ctrl)
qphy_setbits(serdes, cfg->regs[QPHY_COM_POWER_DOWN_CONTROL],
 SW_PWRDN);
+   else
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
 
if (cfg->has_phy_dp_com_ctrl) {
qphy_setbits(dp_com, QPHY_V3_DP_COM_POWER_DOWN_CTRL,
@@ -1127,7 +1129,8 @@ static int qcom_qmp_phy_init(struct phy *phy)
 * Pull out PHY from POWER DOWN state.
 * This is active low enable signal to power-down PHY.
 */
-   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
+   if (cfg->type == PHY_TYPE_PCIE)
+   qphy_setbits(pcs, QPHY_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
 
if (cfg->has_pwrdn_delay)
usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project