| Message ID | 20221011131416.2478-9-johan+linaro@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom-qmp: further prep cleanups | expand |
On 11/10/2022 16:14, Johan Hovold wrote: > The power-down delay was included in the first version of the QMP driver > as an optional delay after powering on the PHY (using > POWER_DOWN_CONTROL) and just before starting it. Later changes modified > this sequence by powering on before initialising the PHY, but the > optional delay stayed where it was (i.e. before starting the PHY). > > The vendor driver does not use a delay before starting the PHY and this > is likely not needed on any platform unless there is a corresponding > delay in the vendor kernel init sequence tables (i.e. in devicetree). > > Let's keep the delay for now, but drop the redundant delay period > configuration while increasing the unnecessarily low timer slack > somewhat. Actually, the vendor driver does this 995..1005 sleep. But contrary to our driver it does that after programming whole PHY init sequence, which includes SW_RESET / START_CTL, but before programming the pipe clocks. I think we can either drop this delay completely, or move it before read_poll_timeout(). > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +----------------------- > 1 file changed, 1 insertion(+), 26 deletions(-) > > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > index fa8bc6aeedf1..315de484f875 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c > @@ -1362,9 +1362,6 @@ struct qmp_phy_cfg { > > /* true, if PHY needs delay after POWER_DOWN */ > bool has_pwrdn_delay; > - /* power_down delay in usec */ > - int pwrdn_delay_min; > - int pwrdn_delay_max; > > /* QMP PHY pipe clock interface rate */ > unsigned long pipe_clock_rate; > @@ -1500,8 +1497,6 @@ static const struct qmp_phy_cfg ipq8074_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = { > @@ -1529,8 +1524,6 @@ static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = { > .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > > .pipe_clock_rate = 250000000, > }; > @@ -1562,8 +1555,6 @@ static const struct qmp_phy_cfg ipq6018_pciephy_cfg = { > .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = { > @@ -1594,8 +1585,6 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sdm845_qhp_pciephy_cfg = { > @@ -1624,8 +1613,6 @@ static const struct qmp_phy_cfg sdm845_qhp_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { > @@ -1666,8 +1653,6 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { > @@ -1708,8 +1693,6 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg msm8998_pciephy_cfg = { > @@ -1765,8 +1748,6 @@ static const struct qmp_phy_cfg sc8180x_pciephy_cfg = { > .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sdx55_qmp_pciephy_cfg = { > @@ -1797,8 +1778,6 @@ static const struct qmp_phy_cfg sdx55_qmp_pciephy_cfg = { > .phy_status = PHYSTATUS_4_20, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sm8450_qmp_gen3x1_pciephy_cfg = { > @@ -1829,8 +1808,6 @@ static const struct qmp_phy_cfg sm8450_qmp_gen3x1_pciephy_cfg = { > .phy_status = PHYSTATUS, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = { > @@ -1876,8 +1853,6 @@ static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = { > .phy_status = PHYSTATUS_4_20, > > .has_pwrdn_delay = true, > - .pwrdn_delay_min = 995, /* us */ > - .pwrdn_delay_max = 1005, /* us */ > }; > > static void qmp_pcie_configure_lane(void __iomem *base, > @@ -2037,7 +2012,7 @@ static int qmp_pcie_power_on(struct phy *phy) > qmp_pcie_pcs_init(qphy, mode_tables); > > if (cfg->has_pwrdn_delay) > - usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max); > + usleep_range(1000, 1200); > > /* Pull PHY out of reset state */ > qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
On 11/10/2022 16:14, Johan Hovold wrote: > The power-down delay was included in the first version of the QMP driver > as an optional delay after powering on the PHY (using > POWER_DOWN_CONTROL) and just before starting it. Later changes modified > this sequence by powering on before initialising the PHY, but the > optional delay stayed where it was (i.e. before starting the PHY). > > The vendor driver does not use a delay before starting the PHY and this > is likely not needed on any platform unless there is a corresponding > delay in the vendor kernel init sequence tables (i.e. in devicetree). > > Let's keep the delay for now, but drop the redundant delay period > configuration while increasing the unnecessarily low timer slack > somewhat. > > Signed-off-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +----------------------- > 1 file changed, 1 insertion(+), 26 deletions(-) > Anyway, we can move the delay later. Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On Tue, Oct 11, 2022 at 04:46:53PM +0300, Dmitry Baryshkov wrote: > On 11/10/2022 16:14, Johan Hovold wrote: > > The power-down delay was included in the first version of the QMP driver > > as an optional delay after powering on the PHY (using > > POWER_DOWN_CONTROL) and just before starting it. Later changes modified > > this sequence by powering on before initialising the PHY, but the > > optional delay stayed where it was (i.e. before starting the PHY). > > > > The vendor driver does not use a delay before starting the PHY and this > > is likely not needed on any platform unless there is a corresponding > > delay in the vendor kernel init sequence tables (i.e. in devicetree). > > > > Let's keep the delay for now, but drop the redundant delay period > > configuration while increasing the unnecessarily low timer slack > > somewhat. > > Actually, the vendor driver does this 995..1005 sleep. But contrary to > our driver it does that after programming whole PHY init sequence, which > includes SW_RESET / START_CTL, but before programming the pipe clocks. Right, it does it after starting the PHY which means that you don't have to poll for as long for the PHY status. It's a different delay entirely. > I think we can either drop this delay completely, or move it before > read_poll_timeout(). It definitely shouldn't be used for any new platforms, but I opted for the conservative route of keeping it in case some of the older platforms actually do need it. My bet is that this is all copy-paste cruft that could be removed, but I'd rather do that as a separate follow-on change. Perhaps after testing some more SoC after removing the delay. SC8280XP certainly doesn't need it. Johan
On 11/10/2022 16:53, Johan Hovold wrote: > On Tue, Oct 11, 2022 at 04:46:53PM +0300, Dmitry Baryshkov wrote: >> On 11/10/2022 16:14, Johan Hovold wrote: >>> The power-down delay was included in the first version of the QMP driver >>> as an optional delay after powering on the PHY (using >>> POWER_DOWN_CONTROL) and just before starting it. Later changes modified >>> this sequence by powering on before initialising the PHY, but the >>> optional delay stayed where it was (i.e. before starting the PHY). >>> >>> The vendor driver does not use a delay before starting the PHY and this >>> is likely not needed on any platform unless there is a corresponding >>> delay in the vendor kernel init sequence tables (i.e. in devicetree). >>> >>> Let's keep the delay for now, but drop the redundant delay period >>> configuration while increasing the unnecessarily low timer slack >>> somewhat. >> >> Actually, the vendor driver does this 995..1005 sleep. But contrary to >> our driver it does that after programming whole PHY init sequence, which >> includes SW_RESET / START_CTL, but before programming the pipe clocks. > > Right, it does it after starting the PHY which means that you don't have > to poll for as long for the PHY status. > > It's a different delay entirely. No-no-no. The 995-1005 delay was added guess for which SoC? For ipq8074, where the config tables contain the ugly CFG_L writes for SW_RESET / START_CTRL. So, it is the same delay, but added by somebody who didn't care enough. The original 10-11 delay is a completely different story, you are correct here. Thus, I'd say, the PCIe delay should be moved after the registers programming. > >> I think we can either drop this delay completely, or move it before >> read_poll_timeout(). > > It definitely shouldn't be used for any new platforms, but I opted for > the conservative route of keeping it in case some of the older platforms > actually do need it. > > My bet is that this is all copy-paste cruft that could be removed, but > I'd rather do that as a separate follow-on change. Perhaps after testing > some more SoC after removing the delay. > > SC8280XP certainly doesn't need it. I think in our case this delay just falls into status polling. We'd probably need it, if we'd add the noretain handling. > > Johan
On Tue, Oct 11, 2022 at 05:04:04PM +0300, Dmitry Baryshkov wrote: > On 11/10/2022 16:53, Johan Hovold wrote: > > On Tue, Oct 11, 2022 at 04:46:53PM +0300, Dmitry Baryshkov wrote: > >> On 11/10/2022 16:14, Johan Hovold wrote: > >>> The power-down delay was included in the first version of the QMP driver > >>> as an optional delay after powering on the PHY (using > >>> POWER_DOWN_CONTROL) and just before starting it. Later changes modified > >>> this sequence by powering on before initialising the PHY, but the > >>> optional delay stayed where it was (i.e. before starting the PHY). > >>> > >>> The vendor driver does not use a delay before starting the PHY and this > >>> is likely not needed on any platform unless there is a corresponding > >>> delay in the vendor kernel init sequence tables (i.e. in devicetree). > >>> > >>> Let's keep the delay for now, but drop the redundant delay period > >>> configuration while increasing the unnecessarily low timer slack > >>> somewhat. > >> > >> Actually, the vendor driver does this 995..1005 sleep. But contrary to > >> our driver it does that after programming whole PHY init sequence, which > >> includes SW_RESET / START_CTL, but before programming the pipe clocks. > > > > Right, it does it after starting the PHY which means that you don't have > > to poll for as long for the PHY status. > > > > It's a different delay entirely. > > No-no-no. The 995-1005 delay was added guess for which SoC? For ipq8074, > where the config tables contain the ugly CFG_L writes for SW_RESET / > START_CTRL. So, it is the same delay, but added by somebody who didn't > care enough. The original 10-11 delay is a completely different story, > you are correct here. Yeah, I noticed that ipq8074 was the first to abuse the prwdn_delay and possibly because of it starting the PHY already in its PCS table (which it never should have). I'm talking about the intent of pwrdn_delay which was to add a delay after powering-on the phy and before starting it. The vendor driver has a 1 ms delay after starting the PHY and before it starts polling as the PHY on newer SoC tend to take > 1 ms before they are ready. So, I still claim that that delay in the vendor driver is a different one entirely. > Thus, I'd say, the PCIe delay should be moved after the registers > programming. No, not necessarily. Again, that's an optimisation in the vendor driver to avoid polling so many times. Since I can say for sure that there are no PHY that start in less than 1 ms, I wouldn't add it unconditionally. Either way, separate change. > >> I think we can either drop this delay completely, or move it before > >> read_poll_timeout(). > > > > It definitely shouldn't be used for any new platforms, but I opted for > > the conservative route of keeping it in case some of the older platforms > > actually do need it. > > > > My bet is that this is all copy-paste cruft that could be removed, but > > I'd rather do that as a separate follow-on change. Perhaps after testing > > some more SoC after removing the delay. > > > > SC8280XP certainly doesn't need it. > > I think in our case this delay just falls into status polling. We'd > probably need it, if we'd add the noretain handling. I'm not sure I understand what you're referring to here ("noretain handling")? Johan
On 11/10/2022 17:17, Johan Hovold wrote: > On Tue, Oct 11, 2022 at 05:04:04PM +0300, Dmitry Baryshkov wrote: >> On 11/10/2022 16:53, Johan Hovold wrote: >>> On Tue, Oct 11, 2022 at 04:46:53PM +0300, Dmitry Baryshkov wrote: >>>> On 11/10/2022 16:14, Johan Hovold wrote: >>>>> The power-down delay was included in the first version of the QMP driver >>>>> as an optional delay after powering on the PHY (using >>>>> POWER_DOWN_CONTROL) and just before starting it. Later changes modified >>>>> this sequence by powering on before initialising the PHY, but the >>>>> optional delay stayed where it was (i.e. before starting the PHY). >>>>> >>>>> The vendor driver does not use a delay before starting the PHY and this >>>>> is likely not needed on any platform unless there is a corresponding >>>>> delay in the vendor kernel init sequence tables (i.e. in devicetree). >>>>> >>>>> Let's keep the delay for now, but drop the redundant delay period >>>>> configuration while increasing the unnecessarily low timer slack >>>>> somewhat. >>>> >>>> Actually, the vendor driver does this 995..1005 sleep. But contrary to >>>> our driver it does that after programming whole PHY init sequence, which >>>> includes SW_RESET / START_CTL, but before programming the pipe clocks. >>> >>> Right, it does it after starting the PHY which means that you don't have >>> to poll for as long for the PHY status. >>> >>> It's a different delay entirely. >> >> No-no-no. The 995-1005 delay was added guess for which SoC? For ipq8074, >> where the config tables contain the ugly CFG_L writes for SW_RESET / >> START_CTRL. So, it is the same delay, but added by somebody who didn't >> care enough. The original 10-11 delay is a completely different story, >> you are correct here. > > Yeah, I noticed that ipq8074 was the first to abuse the prwdn_delay > and possibly because of it starting the PHY already in its PCS table > (which it never should have). > > I'm talking about the intent of pwrdn_delay which was to add a delay > after powering-on the phy and before starting it. > > The vendor driver has a 1 ms delay after starting the PHY and before it > starts polling as the PHY on newer SoC tend to take > 1 ms before they > are ready. > > So, I still claim that that delay in the vendor driver is a different > one entirely. > >> Thus, I'd say, the PCIe delay should be moved after the registers >> programming. > > No, not necessarily. Again, that's an optimisation in the vendor driver > to avoid polling so many times. Since I can say for sure that there are > no PHY that start in less than 1 ms, I wouldn't add it unconditionally. I don't think it's an optimization. For me it looks like some kind of stabilization delay before touching pipe clocks. > > Either way, separate change. > >>>> I think we can either drop this delay completely, or move it before >>>> read_poll_timeout(). >>> >>> It definitely shouldn't be used for any new platforms, but I opted for >>> the conservative route of keeping it in case some of the older platforms >>> actually do need it. >>> >>> My bet is that this is all copy-paste cruft that could be removed, but >>> I'd rather do that as a separate follow-on change. Perhaps after testing >>> some more SoC after removing the delay. >>> >>> SC8280XP certainly doesn't need it. >> >> I think in our case this delay just falls into status polling. We'd >> probably need it, if we'd add the noretain handling. > > I'm not sure I understand what you're referring to here ("noretain > handling")? From what I see in the downstream (4.19 at hand), the sequence is the following: program_phy_config() // including SW_RESET & START_CTRL delay for pipe clocks: clk_set_flags(info->hdl, CLKFLAG_NORETAIN_MEM) clk_set_flags(info->hdl, CLKFLAG_NORETAIN_PERIPH) set clock rates, prepare & enable pipe clocks wmb() poll for the PHY STATUS
On Tue, Oct 11, 2022 at 05:41:28PM +0300, Dmitry Baryshkov wrote: > On 11/10/2022 17:17, Johan Hovold wrote: > > Yeah, I noticed that ipq8074 was the first to abuse the prwdn_delay > > and possibly because of it starting the PHY already in its PCS table > > (which it never should have). > > > > I'm talking about the intent of pwrdn_delay which was to add a delay > > after powering-on the phy and before starting it. > > > > The vendor driver has a 1 ms delay after starting the PHY and before it > > starts polling as the PHY on newer SoC tend to take > 1 ms before they > > are ready. > > > > So, I still claim that that delay in the vendor driver is a different > > one entirely. > > > >> Thus, I'd say, the PCIe delay should be moved after the registers > >> programming. > > > > No, not necessarily. Again, that's an optimisation in the vendor driver > > to avoid polling so many times. Since I can say for sure that there are > > no PHY that start in less than 1 ms, I wouldn't add it unconditionally. > > I don't think it's an optimization. For me it looks like some kind of > stabilization delay before touching pipe clocks. I meant that it's effectively an optimisation as the driver still works without that unconditional delay after starting the PHY and before polling its status. And the mainline poll-loop takes care of waiting also for that 1 ms of "stabilisation". But I guess you could be right in that later contributors have seen that delay in the vendor driver and thought that prwdn_delay corresponds to it without noticing that they are not at all equivalent. As the current delay is pretty much pointless (you can wait for 20 ms if you want, it doesn't matter as the PHY hasn't been started yet) I guess we can move it and avoid a few loops when polling for the status. [ The next batch of clean ups also increases that silly 10 us polling period. ] > > Either way, separate change. > > > >>>> I think we can either drop this delay completely, or move it before > >>>> read_poll_timeout(). > >>> > >>> It definitely shouldn't be used for any new platforms, but I opted for > >>> the conservative route of keeping it in case some of the older platforms > >>> actually do need it. > >>> > >>> My bet is that this is all copy-paste cruft that could be removed, but > >>> I'd rather do that as a separate follow-on change. Perhaps after testing > >>> some more SoC after removing the delay. > >>> > >>> SC8280XP certainly doesn't need it. > >> > >> I think in our case this delay just falls into status polling. We'd > >> probably need it, if we'd add the noretain handling. > > > > I'm not sure I understand what you're referring to here ("noretain > > handling")? > > From what I see in the downstream (4.19 at hand), the sequence is the > following: > > program_phy_config() // including SW_RESET & START_CTRL > > delay > > for pipe clocks: > clk_set_flags(info->hdl, CLKFLAG_NORETAIN_MEM) > clk_set_flags(info->hdl, CLKFLAG_NORETAIN_PERIPH) Heh. Crazy vendor-kernel magic. > set clock rates, prepare & enable pipe clocks > > wmb() > > poll for the PHY STATUS But 5.4 has something similar: program + start delay enable pipe clock poll for status Johan
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c index fa8bc6aeedf1..315de484f875 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp-pcie.c @@ -1362,9 +1362,6 @@ struct qmp_phy_cfg { /* true, if PHY needs delay after POWER_DOWN */ bool has_pwrdn_delay; - /* power_down delay in usec */ - int pwrdn_delay_min; - int pwrdn_delay_max; /* QMP PHY pipe clock interface rate */ unsigned long pipe_clock_rate; @@ -1500,8 +1497,6 @@ static const struct qmp_phy_cfg ipq8074_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = { @@ -1529,8 +1524,6 @@ static const struct qmp_phy_cfg ipq8074_pciephy_gen3_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ .pipe_clock_rate = 250000000, }; @@ -1562,8 +1555,6 @@ static const struct qmp_phy_cfg ipq6018_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = { @@ -1594,8 +1585,6 @@ static const struct qmp_phy_cfg sdm845_qmp_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sdm845_qhp_pciephy_cfg = { @@ -1624,8 +1613,6 @@ static const struct qmp_phy_cfg sdm845_qhp_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { @@ -1666,8 +1653,6 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x1_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { @@ -1708,8 +1693,6 @@ static const struct qmp_phy_cfg sm8250_qmp_gen3x2_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg msm8998_pciephy_cfg = { @@ -1765,8 +1748,6 @@ static const struct qmp_phy_cfg sc8180x_pciephy_cfg = { .pwrdn_ctrl = SW_PWRDN | REFCLK_DRV_DSBL, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sdx55_qmp_pciephy_cfg = { @@ -1797,8 +1778,6 @@ static const struct qmp_phy_cfg sdx55_qmp_pciephy_cfg = { .phy_status = PHYSTATUS_4_20, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sm8450_qmp_gen3x1_pciephy_cfg = { @@ -1829,8 +1808,6 @@ static const struct qmp_phy_cfg sm8450_qmp_gen3x1_pciephy_cfg = { .phy_status = PHYSTATUS, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = { @@ -1876,8 +1853,6 @@ static const struct qmp_phy_cfg sm8450_qmp_gen4x2_pciephy_cfg = { .phy_status = PHYSTATUS_4_20, .has_pwrdn_delay = true, - .pwrdn_delay_min = 995, /* us */ - .pwrdn_delay_max = 1005, /* us */ }; static void qmp_pcie_configure_lane(void __iomem *base, @@ -2037,7 +2012,7 @@ static int qmp_pcie_power_on(struct phy *phy) qmp_pcie_pcs_init(qphy, mode_tables); if (cfg->has_pwrdn_delay) - usleep_range(cfg->pwrdn_delay_min, cfg->pwrdn_delay_max); + usleep_range(1000, 1200); /* Pull PHY out of reset state */ qphy_clrbits(pcs, cfg->regs[QPHY_SW_RESET], SW_RESET);
The power-down delay was included in the first version of the QMP driver as an optional delay after powering on the PHY (using POWER_DOWN_CONTROL) and just before starting it. Later changes modified this sequence by powering on before initialising the PHY, but the optional delay stayed where it was (i.e. before starting the PHY). The vendor driver does not use a delay before starting the PHY and this is likely not needed on any platform unless there is a corresponding delay in the vendor kernel init sequence tables (i.e. in devicetree). Let's keep the delay for now, but drop the redundant delay period configuration while increasing the unnecessarily low timer slack somewhat. Signed-off-by: Johan Hovold <johan+linaro@kernel.org> --- drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 27 +----------------------- 1 file changed, 1 insertion(+), 26 deletions(-)