From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Johan Hovold <johan+linaro@kernel.org>, Vinod Koul <vkoul@kernel.org> Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@somainline.org>, Kishon Vijay Abraham I <kishon@ti.com>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update Date: Wed, 28 Sep 2022 22:48:40 +0300 [thread overview] Message-ID: <f9d0fb60-3e62-8f55-015e-88ca2e209323@linaro.org> (raw) In-Reply-To: <2bc50ea4-ec29-cacc-216b-1c21d2d2f0bc@linaro.org> 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
WARNING: multiple messages have this Message-ID (diff)
From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> To: Johan Hovold <johan+linaro@kernel.org>, Vinod Koul <vkoul@kernel.org> Cc: Andy Gross <agross@kernel.org>, Bjorn Andersson <andersson@kernel.org>, Konrad Dybcio <konrad.dybcio@somainline.org>, Kishon Vijay Abraham I <kishon@ti.com>, linux-arm-msm@vger.kernel.org, linux-phy@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update Date: Wed, 28 Sep 2022 22:48:40 +0300 [thread overview] Message-ID: <f9d0fb60-3e62-8f55-015e-88ca2e209323@linaro.org> (raw) In-Reply-To: <2bc50ea4-ec29-cacc-216b-1c21d2d2f0bc@linaro.org> 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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2022-09-28 19:48 UTC|newest] Thread overview: 84+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-28 15:28 [PATCH 00/13] phy: qcom-qmp: more fixes and cleanups Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 15:28 ` [PATCH 01/13] phy: qcom-qmp: fix obsolete lane comments Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 15:56 ` Neil Armstrong 2022-09-28 15:56 ` Neil Armstrong 2022-09-28 17:26 ` Dmitry Baryshkov 2022-09-28 17:26 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 02/13] phy: qcom-qmp-combo: drop unused UFS reset Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 16:04 ` Neil Armstrong 2022-09-28 16:04 ` Neil Armstrong 2022-09-28 17:25 ` Dmitry Baryshkov 2022-09-28 17:25 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 03/13] phy: qcom-qmp-pcie: drop unused common-block registers Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 16:03 ` Neil Armstrong 2022-09-28 16:03 ` Neil Armstrong 2022-09-28 17:23 ` Dmitry Baryshkov 2022-09-28 17:23 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 04/13] phy: qcom-qmp-pcie: unify ipq registers Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 15:57 ` Neil Armstrong 2022-09-28 15:57 ` Neil Armstrong 2022-09-28 17:34 ` Dmitry Baryshkov 2022-09-28 17:34 ` Dmitry Baryshkov 2022-09-29 6:53 ` Johan Hovold 2022-09-29 6:53 ` Johan Hovold 2022-09-28 15:28 ` [PATCH 05/13] phy: qcom-qmp-pcie: unify sdm845 registers Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 16:01 ` Neil Armstrong 2022-09-28 16:01 ` Neil Armstrong 2022-09-28 16:03 ` Neil Armstrong 2022-09-28 16:03 ` Neil Armstrong 2022-09-28 19:11 ` Dmitry Baryshkov 2022-09-28 19:11 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 06/13] phy: qcom-qmp-pcie: drop bogus register update Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:10 ` Dmitry Baryshkov 2022-09-28 19:10 ` Dmitry Baryshkov 2022-09-28 19:48 ` Dmitry Baryshkov [this message] 2022-09-28 19:48 ` Dmitry Baryshkov 2022-09-29 7:12 ` Johan Hovold 2022-09-29 7:12 ` Johan Hovold 2022-09-29 7:56 ` Dmitry Baryshkov 2022-09-29 7:56 ` Dmitry Baryshkov 2022-09-29 8:18 ` Johan Hovold 2022-09-29 8:18 ` Johan Hovold 2022-09-29 6:56 ` Johan Hovold 2022-09-29 6:56 ` Johan Hovold 2022-09-28 15:28 ` [PATCH 07/13] phy: qcom-qmp-pcie: clean up power-down handling Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:15 ` Dmitry Baryshkov 2022-09-28 19:15 ` Dmitry Baryshkov 2022-09-29 7:25 ` Johan Hovold 2022-09-29 7:25 ` Johan Hovold 2022-09-29 7:30 ` Dmitry Baryshkov 2022-09-29 7:30 ` Dmitry Baryshkov 2022-09-29 9:04 ` Johan Hovold 2022-09-29 9:04 ` Johan Hovold 2022-09-29 9:07 ` Dmitry Baryshkov 2022-09-29 9:07 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 08/13] phy: qcom-qmp-pcie: move power-down update Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 15:28 ` [PATCH 09/13] phy: qcom-qmp-pcie-msm8996: clean up power-down handling Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:52 ` Dmitry Baryshkov 2022-09-28 19:52 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 10/13] phy: qcom-qmp-combo: " Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 11/13] phy: qcom-qmp-ufs: " Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 12/13] phy: qcom-qmp-usb: " Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 19:53 ` Dmitry Baryshkov 2022-09-28 15:28 ` [PATCH 13/13] phy: qcom-qmp-pcie: clean up clock lists Johan Hovold 2022-09-28 15:28 ` Johan Hovold 2022-09-28 19:54 ` Dmitry Baryshkov 2022-09-28 19:54 ` Dmitry Baryshkov
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=f9d0fb60-3e62-8f55-015e-88ca2e209323@linaro.org \ --to=dmitry.baryshkov@linaro.org \ --cc=agross@kernel.org \ --cc=andersson@kernel.org \ --cc=johan+linaro@kernel.org \ --cc=kishon@ti.com \ --cc=konrad.dybcio@somainline.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=vkoul@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.