[PATCH 00/13] phy: qcom-qmp: more fixes and cleanups

[PATCH 00/13] phy: qcom-qmp: more fixes and cleanups

[Johan Hovold]

Here's the next batch of QMP driver fixes and cleanups.

Johan


Johan Hovold (13):
  phy: qcom-qmp: fix obsolete lane comments
  phy: qcom-qmp-combo: drop unused UFS reset
  phy: qcom-qmp-pcie: drop unused common-block registers
  phy: qcom-qmp-pcie: unify ipq registers
  phy: qcom-qmp-pcie: unify sdm845 registers
  phy: qcom-qmp-pcie: drop bogus register update
  phy: qcom-qmp-pcie: clean up power-down handling
  phy: qcom-qmp-pcie: move power-down update
  phy: qcom-qmp-pcie-msm8996: clean up power-down handling
  phy: qcom-qmp-combo: clean up power-down handling
  phy: qcom-qmp-ufs: clean up power-down handling
  phy: qcom-qmp-usb: clean up power-down handling
  phy: qcom-qmp-pcie: clean up clock lists

 drivers/phy/qualcomm/phy-qcom-qmp-combo.c     | 30 ++-------
 .../phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c  | 12 +---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c      | 67 +++++--------------
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c       | 24 +++----
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c       | 22 ++----
 5 files changed, 40 insertions(+), 115 deletions(-)

-- 
2.35.1

[PATCH 01/13] phy: qcom-qmp: fix obsolete lane comments

[Johan Hovold]

All QMP drivers but the MSM8996 and combo ones handle exactly one PHY
and the corresponding memory resources are not per-lane, but per PHY.

Update the obsolete comments.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c        | 2 +-
 drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c | 2 +-
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c         | 2 +-
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c          | 2 +-
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c          | 2 +-
 5 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index ad3b0aa22048..0cf1eb8a62f6 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -2669,7 +2669,7 @@ static int qmp_combo_create(struct device *dev, struct device_node *np, int id,
 	qphy->cfg = cfg;
 	qphy->serdes = serdes;
 	/*
-	 * Get memory resources for each phy lane:
+	 * Get memory resources for each PHY:
 	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
 	 * For dual lane PHYs: tx2 -> 3, rx2 -> 4, pcs_misc (optional) -> 5
 	 * For single lane PHYs: pcs_misc (optional) -> 3.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
index 0f8e739936ab..d9646bf5dc91 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
@@ -777,7 +777,7 @@ static int qmp_pcie_msm8996_create(struct device *dev, struct device_node *np, i
 	qphy->cfg = cfg;
 	qphy->serdes = serdes;
 	/*
-	 * Get memory resources for each phy lane:
+	 * Get memory resources for each PHY:
 	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
 	 */
 	qphy->tx = devm_of_iomap(dev, np, 0, NULL);
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index dde398105f03..abf1cf1abf2c 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -2201,7 +2201,7 @@ static int qmp_pcie_create(struct device *dev, struct device_node *np, int id,
 	qphy->cfg = cfg;
 	qphy->serdes = serdes;
 	/*
-	 * Get memory resources for each phy lane:
+	 * Get memory resources for the PHY:
 	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
 	 * For dual lane PHYs: tx2 -> 3, rx2 -> 4, pcs_misc (optional) -> 5
 	 * For single lane PHYs: pcs_misc (optional) -> 3.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index d21b977850b3..d7b35b715b95 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -1094,7 +1094,7 @@ static int qmp_ufs_create(struct device *dev, struct device_node *np, int id,
 	qphy->cfg = cfg;
 	qphy->serdes = serdes;
 	/*
-	 * Get memory resources for each phy lane:
+	 * Get memory resources for the PHY:
 	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
 	 * For dual lane PHYs: tx2 -> 3, rx2 -> 4, pcs_misc (optional) -> 5
 	 * For single lane PHYs: pcs_misc (optional) -> 3.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
index f01b3022a10d..f8685eddbf80 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
@@ -2581,7 +2581,7 @@ int qmp_usb_create(struct device *dev, struct device_node *np, int id,
 	qphy->cfg = cfg;
 	qphy->serdes = serdes;
 	/*
-	 * Get memory resources for each phy lane:
+	 * Get memory resources for the PHY:
 	 * Resources are indexed as: tx -> 0; rx -> 1; pcs -> 2.
 	 * For dual lane PHYs: tx2 -> 3, rx2 -> 4, pcs_misc (optional) -> 5
 	 * For single lane PHYs: pcs_misc (optional) -> 3.
-- 
2.35.1

Re: [PATCH 01/13] phy: qcom-qmp: fix obsolete lane comments

[Neil Armstrong]

On 28/09/2022 17:28, Johan Hovold wrote:
> All QMP drivers but the MSM8996 and combo ones handle exactly one PHY
> and the corresponding memory resources are not per-lane, but per PHY.
> 
> Update the obsolete comments.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c        | 2 +-
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c | 2 +-
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c         | 2 +-
>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c          | 2 +-
>   drivers/phy/qualcomm/phy-qcom-qmp-usb.c          | 2 +-
>   5 files changed, 5 insertions(+), 5 deletions(-)
> 

<snip>

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 01/13] phy: qcom-qmp: fix obsolete lane comments

[Dmitry Baryshkov]

On 28 September 2022 18:28:10 GMT+03:00, Johan Hovold <johan+linaro@kernel.org> wrote:
>All QMP drivers but the MSM8996 and combo ones handle exactly one PHY
>and the corresponding memory resources are not per-lane, but per PHY.
>
>Update the obsolete comments.
>
>Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c        | 2 +-
> drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c | 2 +-
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c         | 2 +-
> drivers/phy/qualcomm/phy-qcom-qmp-ufs.c          | 2 +-
> drivers/phy/qualcomm/phy-qcom-qmp-usb.c          | 2 +-
> 5 files changed, 5 insertions(+), 5 deletions(-)
>

Reviewed-by: Dmitry Baryshkov<dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 02/13] phy: qcom-qmp-combo: drop unused UFS reset

[Johan Hovold]

Drop the unused UFS reset code which isn't used since the QMP driver
split.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index 0cf1eb8a62f6..e618e675e8ec 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -944,7 +944,6 @@ struct qmp_phy_dp_clks {
  * @phys: array of per-lane phy descriptors
  * @phy_mutex: mutex lock for PHY common block initialization
  * @init_count: phy common block initialization count
- * @ufs_reset: optional UFS PHY reset handle
  */
 struct qcom_qmp {
 	struct device *dev;
@@ -958,8 +957,6 @@ struct qcom_qmp {
 
 	struct mutex phy_mutex;
 	int init_count;
-
-	struct reset_control *ufs_reset;
 };
 
 static void qcom_qmp_v3_phy_dp_aux_init(struct qmp_phy *qphy);
@@ -2027,8 +2024,6 @@ static int qmp_combo_com_exit(struct qmp_phy *qphy)
 		return 0;
 	}
 
-	reset_control_assert(qmp->ufs_reset);
-
 	reset_control_bulk_assert(cfg->num_resets, qmp->resets);
 
 	clk_bulk_disable_unprepare(cfg->num_clks, qmp->clks);
@@ -2103,10 +2098,6 @@ static int qmp_combo_power_on(struct phy *phy)
 	else
 		qmp_combo_configure(pcs, cfg->regs, cfg->pcs_tbl, cfg->pcs_tbl_num);
 
-	ret = reset_control_deassert(qmp->ufs_reset);
-	if (ret)
-		goto err_disable_pipe_clk;
-
 	if (cfg->has_pwrdn_delay)
 		usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
 
-- 
2.35.1

Re: [PATCH 02/13] phy: qcom-qmp-combo: drop unused UFS reset

[Neil Armstrong]

On 28/09/2022 17:28, Johan Hovold wrote:
> Drop the unused UFS reset code which isn't used since the QMP driver
> split.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 9 ---------
>   1 file changed, 9 deletions(-)
> 

<snip>


Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 02/13] phy: qcom-qmp-combo: drop unused UFS reset

[Dmitry Baryshkov]

On 28 September 2022 18:28:11 GMT+03:00, Johan Hovold <johan+linaro@kernel.org> wrote:
>Drop the unused UFS reset code which isn't used since the QMP driver
>split.
>
>Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>---
> drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 9 ---------
> 1 file changed, 9 deletions(-)
>

Reviewed-by: Dmitry Baryshkov<dmitry.baryshkov@linaro.org>

> 

-- 
With best wishes
Dmitry

[PATCH 03/13] phy: qcom-qmp-pcie: drop unused common-block registers

[Johan Hovold]

Drop the common-block register defines that are unused since the QMP
driver split.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index abf1cf1abf2c..ec453f908f1d 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -77,11 +77,6 @@ struct qmp_phy_init_tbl {
 
 /* set of registers with offsets different per-PHY */
 enum qphy_reg_layout {
-	/* Common block control registers */
-	QPHY_COM_SW_RESET,
-	QPHY_COM_POWER_DOWN_CONTROL,
-	QPHY_COM_START_CONTROL,
-	QPHY_COM_PCS_READY_STATUS,
 	/* PCS registers */
 	QPHY_SW_RESET,
 	QPHY_START_CTRL,
@@ -99,10 +94,6 @@ static const unsigned int ipq_pciephy_gen3_regs_layout[QPHY_LAYOUT_SIZE] = {
 };
 
 static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
-	[QPHY_COM_SW_RESET]		= 0x400,
-	[QPHY_COM_POWER_DOWN_CONTROL]	= 0x404,
-	[QPHY_COM_START_CONTROL]	= 0x408,
-	[QPHY_COM_PCS_READY_STATUS]	= 0x448,
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
 	[QPHY_PCS_STATUS]		= 0x174,
-- 
2.35.1

Re: [PATCH 03/13] phy: qcom-qmp-pcie: drop unused common-block registers

[Neil Armstrong]

On 28/09/2022 17:28, Johan Hovold wrote:
> Drop the common-block register defines that are unused since the QMP
> driver split.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 9 ---------
>   1 file changed, 9 deletions(-)
> 

<snip>

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 03/13] phy: qcom-qmp-pcie: drop unused common-block registers

[Dmitry Baryshkov]

On 28 September 2022 18:28:12 GMT+03:00, Johan Hovold <johan+linaro@kernel.org> wrote:
>Drop the common-block register defines that are unused since the QMP
>driver split.

I'd mention that they were only used for msm8996 pcie phy. Nevertheless:

Reviewed-by: Dmitry Baryshkov<dmitry.baryshkov@linaro.org>

>
>Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>---
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 9 ---------
> 1 file changed, 9 deletions(-)
>


-- 
With best wishes
Dmitry

[PATCH 04/13] phy: qcom-qmp-pcie: unify ipq registers

[Johan Hovold]

The IPQ register array is identical to sm8250_pcie_regs_layout so drop
the former.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index ec453f908f1d..7b3f7e42edd5 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -86,13 +86,6 @@ enum qphy_reg_layout {
 	QPHY_LAYOUT_SIZE
 };
 
-static const unsigned int ipq_pciephy_gen3_regs_layout[QPHY_LAYOUT_SIZE] = {
-	[QPHY_SW_RESET]				= 0x00,
-	[QPHY_START_CTRL]			= 0x44,
-	[QPHY_PCS_STATUS]			= 0x14,
-	[QPHY_PCS_POWER_DOWN_CONTROL]		= 0x40,
-};
-
 static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
@@ -1492,7 +1485,7 @@ static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = {
 	.num_resets		= ARRAY_SIZE(ipq8074_pciephy_reset_l),
 	.vreg_list		= NULL,
 	.num_vregs		= 0,
-	.regs			= ipq_pciephy_gen3_regs_layout,
+	.regs			= sm8250_pcie_regs_layout,
 
 	.start_ctrl		= SERDES_START | PCS_START,
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
@@ -1523,7 +1516,7 @@ static const struct qmp_phy_cfg ipq6018_pciephy_cfg = {
 	.num_resets		= ARRAY_SIZE(ipq8074_pciephy_reset_l),
 	.vreg_list		= NULL,
 	.num_vregs		= 0,
-	.regs			= ipq_pciephy_gen3_regs_layout,
+	.regs			= sm8250_pcie_regs_layout,
 
 	.start_ctrl		= SERDES_START | PCS_START,
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
-- 
2.35.1

Re: [PATCH 04/13] phy: qcom-qmp-pcie: unify ipq registers

[Neil Armstrong]

On 28/09/2022 17:28, Johan Hovold wrote:
> The IPQ register array is identical to sm8250_pcie_regs_layout so drop
> the former.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 11 ++---------
>   1 file changed, 2 insertions(+), 9 deletions(-)
> 

<snip>

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 04/13] phy: qcom-qmp-pcie: unify ipq registers

[Dmitry Baryshkov]

On 28 September 2022 18:28:13 GMT+03:00, Johan Hovold <johan+linaro@kernel.org> wrote:
>The IPQ register array is identical to sm8250_pcie_regs_layout so drop
>the former.

I'd not do such merge. They belong to different generations. I'd suggest changing these arrays to use symbolic names defined in corresponding qmp headers.

>
>Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>---
> drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
>diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>index ec453f908f1d..7b3f7e42edd5 100644
>--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>@@ -86,13 +86,6 @@ enum qphy_reg_layout {
> 	QPHY_LAYOUT_SIZE
> };
> 
>-static const unsigned int ipq_pciephy_gen3_regs_layout[QPHY_LAYOUT_SIZE] = {
>-	[QPHY_SW_RESET]				= 0x00,
>-	[QPHY_START_CTRL]			= 0x44,
>-	[QPHY_PCS_STATUS]			= 0x14,
>-	[QPHY_PCS_POWER_DOWN_CONTROL]		= 0x40,
>-};
>-
> static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
> 	[QPHY_SW_RESET]			= 0x00,
> 	[QPHY_START_CTRL]		= 0x08,
>@@ -1492,7 +1485,7 @@ static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = {
> 	.num_resets		= ARRAY_SIZE(ipq8074_pciephy_reset_l),
> 	.vreg_list		= NULL,
> 	.num_vregs		= 0,
>-	.regs			= ipq_pciephy_gen3_regs_layout,
>+	.regs			= sm8250_pcie_regs_layout,
> 
> 	.start_ctrl		= SERDES_START | PCS_START,
> 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
>@@ -1523,7 +1516,7 @@ static const struct qmp_phy_cfg ipq6018_pciephy_cfg = {
> 	.num_resets		= ARRAY_SIZE(ipq8074_pciephy_reset_l),
> 	.vreg_list		= NULL,
> 	.num_vregs		= 0,
>-	.regs			= ipq_pciephy_gen3_regs_layout,
>+	.regs			= sm8250_pcie_regs_layout,
> 
> 	.start_ctrl		= SERDES_START | PCS_START,
> 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,

-- 
With best wishes
Dmitry

Re: [PATCH 04/13] phy: qcom-qmp-pcie: unify ipq registers

[Johan Hovold]

On Wed, Sep 28, 2022 at 08:34:37PM +0300, Dmitry Baryshkov wrote:
> 
> 
> On 28 September 2022 18:28:13 GMT+03:00, Johan Hovold <johan+linaro@kernel.org> wrote:
> >The IPQ register array is identical to sm8250_pcie_regs_layout so drop
> >the former.
> 
> I'd not do such merge. They belong to different generations. I'd
> suggest changing these arrays to use symbolic names defined in
> corresponding qmp headers.

That could be done too, but this is not how these drivers are
implemented currently. Whenever the resource lists match, they end up
being reused. These values need not even change between revisions.

Johan

[PATCH 05/13] phy: qcom-qmp-pcie: unify sdm845 registers

[Johan Hovold]

The SDM845 register array is identical to pciephy_regs_layout so drop
the former.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 7b3f7e42edd5..4146545fdf5f 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -92,12 +92,6 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_PCS_STATUS]		= 0x174,
 };
 
-static const unsigned int sdm845_qmp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
-	[QPHY_SW_RESET]			= 0x00,
-	[QPHY_START_CTRL]		= 0x08,
-	[QPHY_PCS_STATUS]		= 0x174,
-};
-
 static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
@@ -1545,7 +1539,7 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = {
 	.num_resets		= ARRAY_SIZE(sdm845_pciephy_reset_l),
 	.vreg_list		= qmp_phy_vreg_l,
 	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
-	.regs			= sdm845_qmp_pciephy_regs_layout,
+	.regs			= pciephy_regs_layout,
 
 	.start_ctrl		= PCS_START | SERDES_START,
 	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,
-- 
2.35.1

Re: [PATCH 05/13] phy: qcom-qmp-pcie: unify sdm845 registers

[Neil Armstrong]

On 28/09/2022 17:28, Johan Hovold wrote:
> The SDM845 register array is identical to pciephy_regs_layout so drop
> the former.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 7b3f7e42edd5..4146545fdf5f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -92,12 +92,6 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_PCS_STATUS]		= 0x174,
>   };
>   
> -static const unsigned int sdm845_qmp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
> -	[QPHY_SW_RESET]			= 0x00,
> -	[QPHY_START_CTRL]		= 0x08,
> -	[QPHY_PCS_STATUS]		= 0x174,
> -};
> -
>   static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_SW_RESET]			= 0x00,
>   	[QPHY_START_CTRL]		= 0x08,
> @@ -1545,7 +1539,7 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = {
>   	.num_resets		= ARRAY_SIZE(sdm845_pciephy_reset_l),
>   	.vreg_list		= qmp_phy_vreg_l,
>   	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> -	.regs			= sdm845_qmp_pciephy_regs_layout,
> +	.regs			= pciephy_regs_layout,
>   
>   	.start_ctrl		= PCS_START | SERDES_START,
>   	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,

Isn't is an issue if QPHY_COM_* entries are in pciephy_regs_layout and not in sdm845_qmp_pciephy_regs_layout ?

BTW it seems those QPHY_COM_* are never used..

Neil

Re: [PATCH 05/13] phy: qcom-qmp-pcie: unify sdm845 registers

[Neil Armstrong]

On 28/09/2022 18:01, Neil Armstrong wrote:
> On 28/09/2022 17:28, Johan Hovold wrote:
>> The SDM845 register array is identical to pciephy_regs_layout so drop
>> the former.
>>
>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 +-------
>>   1 file changed, 1 insertion(+), 7 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> index 7b3f7e42edd5..4146545fdf5f 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> @@ -92,12 +92,6 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>>       [QPHY_PCS_STATUS]        = 0x174,
>>   };
>> -static const unsigned int sdm845_qmp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>> -    [QPHY_SW_RESET]            = 0x00,
>> -    [QPHY_START_CTRL]        = 0x08,
>> -    [QPHY_PCS_STATUS]        = 0x174,
>> -};
>> -
>>   static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>>       [QPHY_SW_RESET]            = 0x00,
>>       [QPHY_START_CTRL]        = 0x08,
>> @@ -1545,7 +1539,7 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = {
>>       .num_resets        = ARRAY_SIZE(sdm845_pciephy_reset_l),
>>       .vreg_list        = qmp_phy_vreg_l,
>>       .num_vregs        = ARRAY_SIZE(qmp_phy_vreg_l),
>> -    .regs            = sdm845_qmp_pciephy_regs_layout,
>> +    .regs            = pciephy_regs_layout,
>>       .start_ctrl        = PCS_START | SERDES_START,
>>       .pwrdn_ctrl        = SW_PWRDN | REFCLK_DRV_DSBL,
> 
> Isn't is an issue if QPHY_COM_* entries are in pciephy_regs_layout and not in sdm845_qmp_pciephy_regs_layout ?
> 
> BTW it seems those QPHY_COM_* are never used..

Forget this comment, I missed patch 3 removing them...

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>


> 
> Neil

Re: [PATCH 05/13] phy: qcom-qmp-pcie: unify sdm845 registers

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> The SDM845 register array is identical to pciephy_regs_layout so drop
> the former.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Same comment as for the patch 04/13.

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 +-------
>   1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 7b3f7e42edd5..4146545fdf5f 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -92,12 +92,6 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_PCS_STATUS]		= 0x174,
>   };
>   
> -static const unsigned int sdm845_qmp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
> -	[QPHY_SW_RESET]			= 0x00,
> -	[QPHY_START_CTRL]		= 0x08,
> -	[QPHY_PCS_STATUS]		= 0x174,
> -};
> -
>   static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_SW_RESET]			= 0x00,
>   	[QPHY_START_CTRL]		= 0x08,
> @@ -1545,7 +1539,7 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = {
>   	.num_resets		= ARRAY_SIZE(sdm845_pciephy_reset_l),
>   	.vreg_list		= qmp_phy_vreg_l,
>   	.num_vregs		= ARRAY_SIZE(qmp_phy_vreg_l),
> -	.regs			= sdm845_qmp_pciephy_regs_layout,
> +	.regs			= pciephy_regs_layout,
>   
>   	.start_ctrl		= PCS_START | SERDES_START,
>   	.pwrdn_ctrl		= SW_PWRDN | REFCLK_DRV_DSBL,

-- 
With best wishes
Dmitry

[PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Johan Hovold]

Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
PHY is powered on before configuring the registers and only the MSM8996
PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
initialisation table, may possibly require a second update afterwards.

To make things worse, the POWER_DOWN_CONTROL register lies at a
different offset on more recent SoCs so that the second update, which
still used a hard-coded offset, would write to an unrelated register
(e.g. a revision-id register on SC8280XP).

As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
the bogus register update.

Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for sm8150 USB") added support
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 4146545fdf5f..eea66c24cf7e 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -1953,12 +1953,6 @@ static int qmp_pcie_power_on(struct phy *phy)
 	qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl, cfg->pcs_misc_tbl_num);
 	qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl_sec, cfg->pcs_misc_tbl_num_sec);
 
-	/*
-	 * Pull out PHY from POWER DOWN state.
-	 * This is active low enable signal to power-down PHY.
-	 */
-	qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
-
 	if (cfg->has_pwrdn_delay)
 		usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
 
-- 
2.35.1

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
> PHY is powered on before configuring the registers and only the MSM8996
> PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
> initialisation table, may possibly require a second update afterwards.
> 
> To make things worse, the POWER_DOWN_CONTROL register lies at a
> different offset on more recent SoCs so that the second update, which
> still used a hard-coded offset, would write to an unrelated register
> (e.g. a revision-id register on SC8280XP).
> 
> As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
> the bogus register update.
> 
> Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for sm8150 USB") added support

I'm not sure about the particular fixes tag. Backporting from the split 
driver into old qmp driver would be a complete pain.

> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index 4146545fdf5f..eea66c24cf7e 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -1953,12 +1953,6 @@ static int qmp_pcie_power_on(struct phy *phy)
>   	qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl, cfg->pcs_misc_tbl_num);
>   	qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl_sec, cfg->pcs_misc_tbl_num_sec);
>   
> -	/*
> -	 * Pull out PHY from POWER DOWN state.
> -	 * This is active low enable signal to power-down PHY.
> -	 */
> -	qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
> -
>   	if (cfg->has_pwrdn_delay)
>   		usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
>   

-- 
With best wishes
Dmitry

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Dmitry Baryshkov]

On 28/09/2022 22:10, Dmitry Baryshkov wrote:
> On 28/09/2022 18:28, Johan Hovold wrote:
>> Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
>> PHY is powered on before configuring the registers and only the MSM8996
>> PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
>> initialisation table, may possibly require a second update afterwards.
>>
>> To make things worse, the POWER_DOWN_CONTROL register lies at a
>> different offset on more recent SoCs so that the second update, which
>> still used a hard-coded offset, would write to an unrelated register
>> (e.g. a revision-id register on SC8280XP).
>>
>> As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
>> the bogus register update.
>>
>> Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for 
>> sm8150 USB") added support
> 
> I'm not sure about the particular fixes tag. Backporting from the split 
> driver into old qmp driver would be a complete pain.
> 
>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

After digging some more, I stumbled upon the commit 0d58280cf1e6 ("phy: 
Update PHY power control sequence"), which puts explicit register write 
here, telling that 'PCIe PHYs need an extra power control before 
deasserts reset state'.

I can confirm this with the register tables from downstream dtsi. E.g. 
consider sdm845-pcie.dts, pcie0 table. The PCS_POWER_DOWN_CONTROL is the 
register 0x804.

The programmings starts with <0x804 0x1 0x0>, writing 1 to 
PCS_POWER_DOWN_CONTROL (which if I'm not mistaken we do not do at this 
moment). Then after writing all the serdes/tx/rx/pcs/pcs_misc tables 
comes the write <0x804 0x3 0x0> (which you are trying to remove here).

Same sequence applies to the PCIe PHY on msm8998.

Most newer PHYs have the expected sequence (of writing 0x3 to 
PCS_POWER_DOWN_CONTROL) before writing all registers.

As a short summary: unless we get any additional information that 8998 
and sdm845 tables are incorrect, I'd suggest adding a conditional here 
(ugh) and using it here and in qmp_pcie_init() call.

Vinod, Bjorn, do you have any additional info?

> 
>> ---
>>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 6 ------
>>   1 file changed, 6 deletions(-)
>>
>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c 
>> b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> index 4146545fdf5f..eea66c24cf7e 100644
>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>> @@ -1953,12 +1953,6 @@ static int qmp_pcie_power_on(struct phy *phy)
>>       qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl, 
>> cfg->pcs_misc_tbl_num);
>>       qmp_pcie_configure(pcs_misc, cfg->regs, cfg->pcs_misc_tbl_sec, 
>> cfg->pcs_misc_tbl_num_sec);
>> -    /*
>> -     * Pull out PHY from POWER DOWN state.
>> -     * This is active low enable signal to power-down PHY.
>> -     */
>> -    qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL, cfg->pwrdn_ctrl);
>> -
>>       if (cfg->has_pwrdn_delay)
>>           usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max);
> 

-- 
With best wishes
Dmitry

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Johan Hovold]

On Wed, Sep 28, 2022 at 10:48:40PM +0300, Dmitry Baryshkov wrote:
> On 28/09/2022 22:10, Dmitry Baryshkov wrote:
> > On 28/09/2022 18:28, Johan Hovold wrote:
> >> Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
> >> PHY is powered on before configuring the registers and only the MSM8996
> >> PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
> >> initialisation table, may possibly require a second update afterwards.
> >>
> >> To make things worse, the POWER_DOWN_CONTROL register lies at a
> >> different offset on more recent SoCs so that the second update, which
> >> still used a hard-coded offset, would write to an unrelated register
> >> (e.g. a revision-id register on SC8280XP).
> >>
> >> As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
> >> the bogus register update.
> >>
> >> Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for 
> >> sm8150 USB") added support
> > 
> > I'm not sure about the particular fixes tag. Backporting from the split 
> > driver into old qmp driver would be a complete pain.
> > 
> >> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > 
> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> After digging some more, I stumbled upon the commit 0d58280cf1e6 ("phy: 
> Update PHY power control sequence"), which puts explicit register write 
> here, telling that 'PCIe PHYs need an extra power control before 
> deasserts reset state'.

That's the commit I'm referring to above.

> I can confirm this with the register tables from downstream dtsi. E.g. 
> consider sdm845-pcie.dts, pcie0 table. The PCS_POWER_DOWN_CONTROL is the 
> register 0x804.
> 
> The programmings starts with <0x804 0x1 0x0>, writing 1 to 
> PCS_POWER_DOWN_CONTROL (which if I'm not mistaken we do not do at this 
> moment). Then after writing all the serdes/tx/rx/pcs/pcs_misc tables 
> comes the write <0x804 0x3 0x0> (which you are trying to remove here).

The PHY would already have been powered on with the mainline driver, that
write has already happened.

Whether or not PCIe support for SDM845 has been broken since it was
first mainlined almost three years ago is a separate issue. I assume
Bjorn tested it before sending it upstream. 

	421c9a0e9731 ("phy: qcom: qmp: Add SDM845 PCIe QMP PHY support")

> Same sequence applies to the PCIe PHY on msm8998.
> 
> Most newer PHYs have the expected sequence (of writing 0x3 to 
> PCS_POWER_DOWN_CONTROL) before writing all registers.
> 
> As a short summary: unless we get any additional information that 8998 
> and sdm845 tables are incorrect, I'd suggest adding a conditional here 
> (ugh) and using it here and in qmp_pcie_init() call.

I see little point in doing that unless you dig out an SDM845, confirm
that it has never worked with upstream, and update the init sequence
first.

> Vinod, Bjorn, do you have any additional info?

An explanation of the split POWER_DOWN_CONTROL updates on MSM8996 would
be good to have either way.

Johan

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Dmitry Baryshkov]

On 29/09/2022 10:12, Johan Hovold wrote:
> On Wed, Sep 28, 2022 at 10:48:40PM +0300, Dmitry Baryshkov wrote:
>> On 28/09/2022 22:10, Dmitry Baryshkov wrote:
>>> On 28/09/2022 18:28, Johan Hovold wrote:
>>>> Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
>>>> PHY is powered on before configuring the registers and only the MSM8996
>>>> PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
>>>> initialisation table, may possibly require a second update afterwards.
>>>>
>>>> To make things worse, the POWER_DOWN_CONTROL register lies at a
>>>> different offset on more recent SoCs so that the second update, which
>>>> still used a hard-coded offset, would write to an unrelated register
>>>> (e.g. a revision-id register on SC8280XP).
>>>>
>>>> As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
>>>> the bogus register update.
>>>>
>>>> Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for
>>>> sm8150 USB") added support
>>>
>>> I'm not sure about the particular fixes tag. Backporting from the split
>>> driver into old qmp driver would be a complete pain.
>>>
>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>
>> After digging some more, I stumbled upon the commit 0d58280cf1e6 ("phy:
>> Update PHY power control sequence"), which puts explicit register write
>> here, telling that 'PCIe PHYs need an extra power control before
>> deasserts reset state'.
> 
> That's the commit I'm referring to above.
> 
>> I can confirm this with the register tables from downstream dtsi. E.g.
>> consider sdm845-pcie.dts, pcie0 table. The PCS_POWER_DOWN_CONTROL is the
>> register 0x804.
>>
>> The programmings starts with <0x804 0x1 0x0>, writing 1 to
>> PCS_POWER_DOWN_CONTROL (which if I'm not mistaken we do not do at this
>> moment). Then after writing all the serdes/tx/rx/pcs/pcs_misc tables
>> comes the write <0x804 0x3 0x0> (which you are trying to remove here).
> 
> The PHY would already have been powered on with the mainline driver, that
> write has already happened.
> 
> Whether or not PCIe support for SDM845 has been broken since it was
> first mainlined almost three years ago is a separate issue. I assume
> Bjorn tested it before sending it upstream.
> 
> 	421c9a0e9731 ("phy: qcom: qmp: Add SDM845 PCIe QMP PHY support")

On SDM845 PCIe0 is used for the WiFi, e.g. on the RB3 (dragonboard845) 
device. The PHY definitely works with the upstream kernels.

> 
>> Same sequence applies to the PCIe PHY on msm8998.
>>
>> Most newer PHYs have the expected sequence (of writing 0x3 to
>> PCS_POWER_DOWN_CONTROL) before writing all registers.
>>
>> As a short summary: unless we get any additional information that 8998
>> and sdm845 tables are incorrect, I'd suggest adding a conditional here
>> (ugh) and using it here and in qmp_pcie_init() call.
> 
> I see little point in doing that unless you dig out an SDM845, confirm
> that it has never worked with upstream, and update the init sequence
> first.

Digging out an sdm845 is not a problem per se. It works, but it also has 
an additional regwrite that this patch tries to remove. I will try 
checking whether it works with this register write being removed or not.

> 
>> Vinod, Bjorn, do you have any additional info?
> 
> An explanation of the split POWER_DOWN_CONTROL updates on MSM8996 would
> be good to have either way

-- 
With best wishes
Dmitry

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Johan Hovold]

On Thu, Sep 29, 2022 at 10:56:11AM +0300, Dmitry Baryshkov wrote:
> On 29/09/2022 10:12, Johan Hovold wrote:
> > On Wed, Sep 28, 2022 at 10:48:40PM +0300, Dmitry Baryshkov wrote:
> >> On 28/09/2022 22:10, Dmitry Baryshkov wrote:

> >> The programmings starts with <0x804 0x1 0x0>, writing 1 to
> >> PCS_POWER_DOWN_CONTROL (which if I'm not mistaken we do not do at this
> >> moment). Then after writing all the serdes/tx/rx/pcs/pcs_misc tables
> >> comes the write <0x804 0x3 0x0> (which you are trying to remove here).
> > 
> > The PHY would already have been powered on with the mainline driver, that
> > write has already happened.
> > 
> > Whether or not PCIe support for SDM845 has been broken since it was
> > first mainlined almost three years ago is a separate issue. I assume
> > Bjorn tested it before sending it upstream.
> > 
> > 	421c9a0e9731 ("phy: qcom: qmp: Add SDM845 PCIe QMP PHY support")
> 
> On SDM845 PCIe0 is used for the WiFi, e.g. on the RB3 (dragonboard845) 
> device. The PHY definitely works with the upstream kernels.

Ok, good.

> >> Same sequence applies to the PCIe PHY on msm8998.
> >>
> >> Most newer PHYs have the expected sequence (of writing 0x3 to
> >> PCS_POWER_DOWN_CONTROL) before writing all registers.
> >>
> >> As a short summary: unless we get any additional information that 8998
> >> and sdm845 tables are incorrect, I'd suggest adding a conditional here
> >> (ugh) and using it here and in qmp_pcie_init() call.
> > 
> > I see little point in doing that unless you dig out an SDM845, confirm
> > that it has never worked with upstream, and update the init sequence
> > first.
> 
> Digging out an sdm845 is not a problem per se. It works, but it also has 
> an additional regwrite that this patch tries to remove. I will try 
> checking whether it works with this register write being removed or not.

Right, but as I already mentioned that register already holds the same
value (0x3) when the second redundant write happens. So this should be a
no-op on sdm845 currently.

(Only MSM8996 had an intermediate update of that register in its PCS
table and therefore needs this second update.)

> >> Vinod, Bjorn, do you have any additional info?
> > 
> > An explanation of the split POWER_DOWN_CONTROL updates on MSM8996 would
> > be good to have either way

Johan

Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update

[Johan Hovold]

On Wed, Sep 28, 2022 at 10:10:02PM +0300, Dmitry Baryshkov wrote:
> On 28/09/2022 18:28, Johan Hovold wrote:
> > Since commit 0d58280cf1e6 ("phy: Update PHY power control sequence") the
> > PHY is powered on before configuring the registers and only the MSM8996
> > PCIe PHY, which includes the POWER_DOWN_CONTROL register in its PCS
> > initialisation table, may possibly require a second update afterwards.
> > 
> > To make things worse, the POWER_DOWN_CONTROL register lies at a
> > different offset on more recent SoCs so that the second update, which
> > still used a hard-coded offset, would write to an unrelated register
> > (e.g. a revision-id register on SC8280XP).
> > 
> > As the MSM8996 PCIe PHY is now handled by a separate driver, simply drop
> > the bogus register update.
> > 
> > Fixes: e4d8b05ad5f9 ("phy: qcom-qmp: Use proper PWRDOWN offset for sm8150 USB") added support
> 
> I'm not sure about the particular fixes tag. Backporting from the split 
> driver into old qmp driver would be a complete pain.

That a separate issue. The fixes tag point at the commit introducing the
bug. And I didn't add a stable tag on purpose.

> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Johan

[PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Johan Hovold]

Always define the POWER_DOWN_CONTROL register instead of falling back to
the v2 offset during power on and power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index eea66c24cf7e..47cdb9ed80cd 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
 	[QPHY_PCS_STATUS]		= 0x174,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
 	[QPHY_PCS_STATUS]		= 0x2ac,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int sm8250_pcie_regs_layout[QPHY_LAYOUT_SIZE] = {
@@ -1872,13 +1874,8 @@ static int qmp_pcie_init(struct phy *phy)
 	if (ret)
 		goto err_assert_reset;
 
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
-		qphy_setbits(pcs,
-				cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-				cfg->pwrdn_ctrl);
-	else
-		qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 
@@ -1995,13 +1992,8 @@ static int qmp_pcie_power_off(struct phy *phy)
 	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
 	/* Put PHY into POWER DOWN state: active low */
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
-		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	} else {
-		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
-	}
+	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Always define the POWER_DOWN_CONTROL register instead of falling back to
> the v2 offset during power on and power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> index eea66c24cf7e..47cdb9ed80cd 100644
> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> @@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_SW_RESET]			= 0x00,
>   	[QPHY_START_CTRL]		= 0x08,
>   	[QPHY_PCS_STATUS]		= 0x174,
> +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
>   };

Without symbolic names it's not obvious that 0x04 (and thus this 
regs_layout) can be used for v2 and v3, but not for v4.

>   
>   static const unsigned int sdm845_qhp_pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>   	[QPHY_SW_RESET]			= 0x00,
>   	[QPHY_START_CTRL]		= 0x08,
>   	[QPHY_PCS_STATUS]		= 0x2ac,
> +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
>   };
>   
>   static const unsigned int sm8250_pcie_regs_layout[QPHY_LAYOUT_SIZE] = {
> @@ -1872,13 +1874,8 @@ static int qmp_pcie_init(struct phy *phy)
>   	if (ret)
>   		goto err_assert_reset;
>   
> -	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> -		qphy_setbits(pcs,
> -				cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> -				cfg->pwrdn_ctrl);
> -	else
> -		qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
> -				cfg->pwrdn_ctrl);
> +	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +			cfg->pwrdn_ctrl);
>   
>   	return 0;
>   
> @@ -1995,13 +1992,8 @@ static int qmp_pcie_power_off(struct phy *phy)
>   	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
>   
>   	/* Put PHY into POWER DOWN state: active low */
> -	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
> -		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> -			     cfg->pwrdn_ctrl);
> -	} else {
> -		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
> -				cfg->pwrdn_ctrl);
> -	}
> +	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +			cfg->pwrdn_ctrl);
>   
>   	return 0;
>   }

-- 
With best wishes
Dmitry

Re: [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Johan Hovold]

On Wed, Sep 28, 2022 at 10:15:46PM +0300, Dmitry Baryshkov wrote:
> On 28/09/2022 18:28, Johan Hovold wrote:
> > Always define the POWER_DOWN_CONTROL register instead of falling back to
> > the v2 offset during power on and power off.
> > 
> > Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> > ---
> >   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
> >   1 file changed, 6 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > index eea66c24cf7e..47cdb9ed80cd 100644
> > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> > @@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
> >   	[QPHY_SW_RESET]			= 0x00,
> >   	[QPHY_START_CTRL]		= 0x08,
> >   	[QPHY_PCS_STATUS]		= 0x174,
> > +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
> >   };
> 
> Without symbolic names it's not obvious that 0x04 (and thus this 
> regs_layout) can be used for v2 and v3, but not for v4.

It's no less obvious than it was when we were falling back to the v2
define when it wasn't in the table.

> @@ -1872,13 +1874,8 @@ static int qmp_pcie_init(struct phy *phy)
>       if (ret)
>               goto err_assert_reset;
> -     if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
> -             qphy_setbits(pcs,
> -                             cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> -                             cfg->pwrdn_ctrl);
> -     else
> -             qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
> -                             cfg->pwrdn_ctrl);
> +     qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
> +                     cfg->pwrdn_ctrl);

This is the cruft I'm getting rid of.

Johan

Re: [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Dmitry Baryshkov]

On 29/09/2022 10:25, Johan Hovold wrote:
> On Wed, Sep 28, 2022 at 10:15:46PM +0300, Dmitry Baryshkov wrote:
>> On 28/09/2022 18:28, Johan Hovold wrote:
>>> Always define the POWER_DOWN_CONTROL register instead of falling back to
>>> the v2 offset during power on and power off.
>>>
>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>> ---
>>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
>>>    1 file changed, 6 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> index eea66c24cf7e..47cdb9ed80cd 100644
>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>> @@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>>>    	[QPHY_SW_RESET]			= 0x00,
>>>    	[QPHY_START_CTRL]		= 0x08,
>>>    	[QPHY_PCS_STATUS]		= 0x174,
>>> +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
>>>    };
>>
>> Without symbolic names it's not obvious that 0x04 (and thus this
>> regs_layout) can be used for v2 and v3, but not for v4.
> 
> It's no less obvious than it was when we were falling back to the v2
> define when it wasn't in the table.

Yes, that's without doubts. Anyway, I've sent my view on the regs 
layouts standing on top of your six patches from this series. Could you 
please take a glance?

> 
>> @@ -1872,13 +1874,8 @@ static int qmp_pcie_init(struct phy *phy)
>>        if (ret)
>>                goto err_assert_reset;
>> -     if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
>> -             qphy_setbits(pcs,
>> -                             cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>> -                             cfg->pwrdn_ctrl);
>> -     else
>> -             qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
>> -                             cfg->pwrdn_ctrl);
>> +     qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
>> +                     cfg->pwrdn_ctrl);
> 
> This is the cruft I'm getting rid of.
> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Johan Hovold]

On Thu, Sep 29, 2022 at 10:30:20AM +0300, Dmitry Baryshkov wrote:
> On 29/09/2022 10:25, Johan Hovold wrote:
> > On Wed, Sep 28, 2022 at 10:15:46PM +0300, Dmitry Baryshkov wrote:
> >> On 28/09/2022 18:28, Johan Hovold wrote:
> >>> Always define the POWER_DOWN_CONTROL register instead of falling back to
> >>> the v2 offset during power on and power off.
> >>>
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>    drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
> >>>    1 file changed, 6 insertions(+), 14 deletions(-)
> >>>
> >>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> index eea66c24cf7e..47cdb9ed80cd 100644
> >>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
> >>> @@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
> >>>    	[QPHY_SW_RESET]			= 0x00,
> >>>    	[QPHY_START_CTRL]		= 0x08,
> >>>    	[QPHY_PCS_STATUS]		= 0x174,
> >>> +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
> >>>    };
> >>
> >> Without symbolic names it's not obvious that 0x04 (and thus this
> >> regs_layout) can be used for v2 and v3, but not for v4.
> > 
> > It's no less obvious than it was when we were falling back to the v2
> > define when it wasn't in the table.
> 
> Yes, that's without doubts. Anyway, I've sent my view on the regs 
> layouts standing on top of your six patches from this series. Could you 
> please take a glance?

Sure, but I don't think doing that separate change should be a blocker
for this series. Especially since you run into issues like it not
always being clear which version of the IP is being used (IPQ).

I'd rather respin this series and drop the two patches that merged the
two redundant layout structs.

Then you can work on further clean ups on top for 6.2 since that's going
to require some more careful review and thought.

Johan

Re: [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling

[Dmitry Baryshkov]

On 29/09/2022 12:04, Johan Hovold wrote:
> On Thu, Sep 29, 2022 at 10:30:20AM +0300, Dmitry Baryshkov wrote:
>> On 29/09/2022 10:25, Johan Hovold wrote:
>>> On Wed, Sep 28, 2022 at 10:15:46PM +0300, Dmitry Baryshkov wrote:
>>>> On 28/09/2022 18:28, Johan Hovold wrote:
>>>>> Always define the POWER_DOWN_CONTROL register instead of falling back to
>>>>> the v2 offset during power on and power off.
>>>>>
>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>> ---
>>>>>     drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 20 ++++++--------------
>>>>>     1 file changed, 6 insertions(+), 14 deletions(-)
>>>>>
>>>>> diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> index eea66c24cf7e..47cdb9ed80cd 100644
>>>>> --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
>>>>> @@ -90,12 +90,14 @@ static const unsigned int pciephy_regs_layout[QPHY_LAYOUT_SIZE] = {
>>>>>     	[QPHY_SW_RESET]			= 0x00,
>>>>>     	[QPHY_START_CTRL]		= 0x08,
>>>>>     	[QPHY_PCS_STATUS]		= 0x174,
>>>>> +	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
>>>>>     };
>>>>
>>>> Without symbolic names it's not obvious that 0x04 (and thus this
>>>> regs_layout) can be used for v2 and v3, but not for v4.
>>>
>>> It's no less obvious than it was when we were falling back to the v2
>>> define when it wasn't in the table.
>>
>> Yes, that's without doubts. Anyway, I've sent my view on the regs
>> layouts standing on top of your six patches from this series. Could you
>> please take a glance?
> 
> Sure, but I don't think doing that separate change should be a blocker
> for this series. Especially since you run into issues like it not
> always being clear which version of the IP is being used (IPQ).
> 
> I'd rather respin this series and drop the two patches that merged the
> two redundant layout structs.

I'm fine either way.

> 
> Then you can work on further clean ups on top for 6.2 since that's going
> to require some more careful review and thought.
> 
> Johan

-- 
With best wishes
Dmitry

[PATCH 08/13] phy: qcom-qmp-pcie: move power-down update

[Johan Hovold]

Move the power-down-control register update that powers on the PHY to
the power-on handler so that it matches the power-off handler.

Note that the power-on handler is currently always called directly
after init.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 47cdb9ed80cd..150dc58147ce 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -1848,7 +1848,6 @@ static int qmp_pcie_init(struct phy *phy)
 	struct qmp_phy *qphy = phy_get_drvdata(phy);
 	struct qcom_qmp *qmp = qphy->qmp;
 	const struct qmp_phy_cfg *cfg = qphy->cfg;
-	void __iomem *pcs = qphy->pcs;
 	int ret;
 
 	/* turn on regulator supplies */
@@ -1874,9 +1873,6 @@ static int qmp_pcie_init(struct phy *phy)
 	if (ret)
 		goto err_assert_reset;
 
-	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			cfg->pwrdn_ctrl);
-
 	return 0;
 
 err_assert_reset:
@@ -1915,6 +1911,9 @@ static int qmp_pcie_power_on(struct phy *phy)
 	unsigned int mask, val, ready;
 	int ret;
 
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
+
 	qmp_pcie_serdes_init(qphy);
 
 	ret = clk_prepare_enable(qphy->pipe_clk);
-- 
2.35.1

[PATCH 09/13] phy: qcom-qmp-pcie-msm8996: clean up power-down handling

[Johan Hovold]

This driver uses v2 registers only so drop the unnecessary
POWER_DOWN_CONTROL override.

Note that this register is already hard-coded when powering on the PHY.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
index d9646bf5dc91..1960bd0513e9 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c
@@ -91,7 +91,6 @@ enum qphy_reg_layout {
 	QPHY_SW_RESET,
 	QPHY_START_CTRL,
 	QPHY_PCS_STATUS,
-	QPHY_PCS_POWER_DOWN_CONTROL,
 	/* Keep last to ensure regs_layout arrays are properly initialized */
 	QPHY_LAYOUT_SIZE
 };
@@ -591,13 +590,8 @@ static int qmp_pcie_msm8996_power_off(struct phy *phy)
 	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
 	/* Put PHY into POWER DOWN state: active low */
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
-		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	} else {
-		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
-	}
+	qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
+			cfg->pwrdn_ctrl);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 09/13] phy: qcom-qmp-pcie-msm8996: clean up power-down handling

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> This driver uses v2 registers only so drop the unnecessary
> POWER_DOWN_CONTROL override.
> 
> Note that this register is already hard-coded when powering on the PHY.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie-msm8996.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 10/13] phy: qcom-qmp-combo: clean up power-down handling

[Johan Hovold]

Always define the POWER_DOWN_CONTROL register instead of falling back to
the v2 (and v3) offset during power on and power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
index e618e675e8ec..5665eb3c1556 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-combo.c
@@ -121,6 +121,7 @@ static const unsigned int qmp_v3_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_SW_RESET]			= 0x00,
 	[QPHY_START_CTRL]		= 0x08,
 	[QPHY_PCS_STATUS]		= 0x174,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 	[QPHY_PCS_AUTONOMOUS_MODE_CTRL]	= 0x0d8,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR]  = 0x0dc,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_STATUS] = 0x170,
@@ -1991,13 +1992,8 @@ static int qmp_combo_com_init(struct qmp_phy *qphy)
 	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SWI_CTRL, 0x03);
 	qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
 
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
-		qphy_setbits(pcs,
-				cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-				cfg->pwrdn_ctrl);
-	else
-		qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	mutex_unlock(&qmp->phy_mutex);
 
@@ -2144,13 +2140,8 @@ static int qmp_combo_power_off(struct phy *phy)
 		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
 		/* Put PHY into POWER DOWN state: active low */
-		if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
-			qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-				     cfg->pwrdn_ctrl);
-		} else {
-			qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-					cfg->pwrdn_ctrl);
-		}
+		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+				cfg->pwrdn_ctrl);
 	}
 
 	return 0;
-- 
2.35.1

Re: [PATCH 10/13] phy: qcom-qmp-combo: clean up power-down handling

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Always define the POWER_DOWN_CONTROL register instead of falling back to
> the v2 (and v3) offset during power on and power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-combo.c | 19 +++++--------------
>   1 file changed, 5 insertions(+), 14 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 11/13] phy: qcom-qmp-ufs: clean up power-down handling

[Johan Hovold]

Always define the POWER_DOWN_CONTROL register instead of falling back to
the v2 (and v4) offset during power on and power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 ++++++++--------------
 1 file changed, 8 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
index d7b35b715b95..c8d86aecfe74 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-ufs.c
@@ -89,22 +89,26 @@ enum qphy_reg_layout {
 static const unsigned int msm8996_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_START_CTRL]		= 0x00,
 	[QPHY_PCS_READY_STATUS]		= 0x168,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int sdm845_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_START_CTRL]		= 0x00,
 	[QPHY_PCS_READY_STATUS]		= 0x160,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int sm6115_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_START_CTRL]		= 0x00,
 	[QPHY_PCS_READY_STATUS]		= 0x168,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int sm8150_ufsphy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_START_CTRL]		= QPHY_V4_PCS_UFS_PHY_START,
 	[QPHY_PCS_READY_STATUS]		= QPHY_V4_PCS_UFS_READY_STATUS,
 	[QPHY_SW_RESET]			= QPHY_V4_PCS_UFS_SW_RESET,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= QPHY_V4_PCS_UFS_POWER_DOWN_CONTROL,
 };
 
 static const struct qmp_phy_init_tbl msm8996_ufs_serdes_tbl[] = {
@@ -856,13 +860,8 @@ static int qmp_ufs_com_init(struct qmp_phy *qphy)
 	if (ret)
 		goto err_disable_regulators;
 
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
-		qphy_setbits(pcs,
-			     cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	else
-		qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-			     cfg->pwrdn_ctrl);
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 
@@ -996,13 +995,8 @@ static int qmp_ufs_power_off(struct phy *phy)
 	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
 	/* Put PHY into POWER DOWN state: active low */
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
-		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	} else {
-		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
-	}
+	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 11/13] phy: qcom-qmp-ufs: clean up power-down handling

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Always define the POWER_DOWN_CONTROL register instead of falling back to
> the v2 (and v4) offset during power on and power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-ufs.c | 22 ++++++++--------------
>   1 file changed, 8 insertions(+), 14 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH 12/13] phy: qcom-qmp-usb: clean up power-down handling

[Johan Hovold]

Always define the POWER_DOWN_CONTROL register instead of falling back to
the v2 (and v3) offset during power on and power off.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
index f8685eddbf80..8acb5a3aeb95 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-usb.c
@@ -126,6 +126,7 @@ static const unsigned int usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_PCS_AUTONOMOUS_MODE_CTRL]	= 0x0d4,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR]  = 0x0d8,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_STATUS] = 0x178,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int qmp_v3_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
@@ -135,6 +136,7 @@ static const unsigned int qmp_v3_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
 	[QPHY_PCS_AUTONOMOUS_MODE_CTRL]	= 0x0d8,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_CLEAR]  = 0x0dc,
 	[QPHY_PCS_LFPS_RXTERM_IRQ_STATUS] = 0x170,
+	[QPHY_PCS_POWER_DOWN_CONTROL]	= 0x04,
 };
 
 static const unsigned int qmp_v4_usb3phy_regs_layout[QPHY_LAYOUT_SIZE] = {
@@ -2164,13 +2166,8 @@ static int qmp_usb_init(struct phy *phy)
 		qphy_clrbits(dp_com, QPHY_V3_DP_COM_SW_RESET, SW_RESET);
 	}
 
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL])
-		qphy_setbits(pcs,
-			     cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	else
-		qphy_setbits(pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-			     cfg->pwrdn_ctrl);
+	qphy_setbits(pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 
@@ -2277,13 +2274,8 @@ static int qmp_usb_power_off(struct phy *phy)
 	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_START_CTRL], cfg->start_ctrl);
 
 	/* Put PHY into POWER DOWN state: active low */
-	if (cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL]) {
-		qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
-			     cfg->pwrdn_ctrl);
-	} else {
-		qphy_clrbits(qphy->pcs, QPHY_V2_PCS_POWER_DOWN_CONTROL,
-				cfg->pwrdn_ctrl);
-	}
+	qphy_clrbits(qphy->pcs, cfg->regs[QPHY_PCS_POWER_DOWN_CONTROL],
+			cfg->pwrdn_ctrl);
 
 	return 0;
 }
-- 
2.35.1

Re: [PATCH 12/13] phy: qcom-qmp-usb: clean up power-down handling

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Always define the POWER_DOWN_CONTROL register instead of falling back to
> the v2 (and v3) offset during power on and power off.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-usb.c | 20 ++++++--------------
>   1 file changed, 6 insertions(+), 14 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH 13/13] phy: qcom-qmp-pcie: clean up clock lists

[Johan Hovold]

Keep the clock lists together and sorted by symbol name.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
index 150dc58147ce..0b048e32f39b 100644
--- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
+++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c
@@ -1409,6 +1409,10 @@ static inline void qphy_clrbits(void __iomem *base, u32 offset, u32 val)
 }
 
 /* list of clocks required by phy */
+static const char * const ipq8074_pciephy_clk_l[] = {
+	"aux", "cfg_ahb",
+};
+
 static const char * const msm8996_phy_clk_l[] = {
 	"aux", "cfg_ahb", "ref",
 };
@@ -1423,10 +1427,6 @@ static const char * const qmp_phy_vreg_l[] = {
 	"vdda-phy", "vdda-pll",
 };
 
-static const char * const ipq8074_pciephy_clk_l[] = {
-	"aux", "cfg_ahb",
-};
-
 /* list of resets */
 static const char * const ipq8074_pciephy_reset_l[] = {
 	"phy", "common",
-- 
2.35.1

Re: [PATCH 13/13] phy: qcom-qmp-pcie: clean up clock lists

[Dmitry Baryshkov]

On 28/09/2022 18:28, Johan Hovold wrote:
> Keep the clock lists together and sorted by symbol name.
> 
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>   drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry