| Message ID | 1522321466-21755-3-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Manu, On Thu, Mar 29, 2018 at 4:06 AM Manu Gautam <mgautam@codeaurora.org> wrote: > QMP PHY for USB/PCIE requires pipe_clk for locking of > retime buffers at the pipe interface. Driver checks for > PHY_STATUS without enabling pipe_clk due to which > phy_init() fails with initialization timeout. > Though pipe_clk is output from PHY (after PLL is programmed > during initialization sequence) to GCC clock_ctl and then fed > back to PHY but for PHY_STATUS register to reflect successful > initialization pipe_clk from GCC must be present. > Since, clock driver now ignores status_check for pipe_clk on > clk_enable/disable, driver can safely enable/disable pipe_clk > from phy_init/exit. > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) > diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c > index 6470c5d..fddb1c9 100644 > --- a/drivers/phy/qualcomm/phy-qcom-qmp.c > +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c > @@ -793,19 +793,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, > } > } > -static int qcom_qmp_phy_poweron(struct phy *phy) > -{ > - struct qmp_phy *qphy = phy_get_drvdata(phy); > - struct qcom_qmp *qmp = qphy->qmp; > - int ret; > - > - ret = clk_prepare_enable(qphy->pipe_clk); > - if (ret) > - dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret); > - > - return ret; > -} > - > static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) > { This change looks good to me, it has much nicer symmetry. It did get me looking at qcom_qmp_phy_com_init though. This isn't directly related to your change, but I think there might be an issue with qmp->init_count. We increment it at the start of the function, but then if init fails we don't decrement it. So if we then try init again later, it magically succeeds without actually initing anything. The other side, qcom_qmp_phy_com_exit doesn't have this problem because exit doesn't fail (which begs the question of why it has a return value). I don't need to hold up this series, since this isn't strictly related to your changes, but if you do spin again, you could include it. Otherwise you or I could send it along as a followup. -Evan -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote: > QMP PHY for USB/PCIE requires pipe_clk for locking of > retime buffers at the pipe interface. Driver checks for > PHY_STATUS without enabling pipe_clk due to which > phy_init() fails with initialization timeout. > Though pipe_clk is output from PHY (after PLL is programmed > during initialization sequence) to GCC clock_ctl and then fed > back to PHY but for PHY_STATUS register to reflect successful > initialization pipe_clk from GCC must be present. > Since, clock driver now ignores status_check for pipe_clk on > clk_enable/disable, driver can safely enable/disable pipe_clk > from phy_init/exit. > > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- > 1 file changed, 8 insertions(+), 14 deletions(-) Overall this looks much better than the previous version. Thanks! :) I wonder one thing though. You describe the original problem as this: 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY never sets the "ready" status. 2. If you don't have the PHY powered on / out of reset (which happens in qcom_qmp_phy_init()) then when you enable/disable the clock it doesn't properly update the status. That's why you needed patch #1 in this series. I wonder: could you solve the above _without_ needing to use BRANCH_HALT_DELAY in the clock driver? Specifically, can you tell me what happens if you put the clk_prepare_enable() after you've powered on the PHY and taken it out of reset but before you check the status? Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call right before the "readl_poll_timeout" of the ready status? If you do that, you'll turn everything on. Then you'll check that the clock's status is OK and then that the PHY's status is OK. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote: >> QMP PHY for USB/PCIE requires pipe_clk for locking of >> retime buffers at the pipe interface. Driver checks for >> PHY_STATUS without enabling pipe_clk due to which >> phy_init() fails with initialization timeout. >> Though pipe_clk is output from PHY (after PLL is programmed >> during initialization sequence) to GCC clock_ctl and then fed >> back to PHY but for PHY_STATUS register to reflect successful >> initialization pipe_clk from GCC must be present. >> Since, clock driver now ignores status_check for pipe_clk on >> clk_enable/disable, driver can safely enable/disable pipe_clk >> from phy_init/exit. >> >> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >> --- >> drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- >> 1 file changed, 8 insertions(+), 14 deletions(-) > > Overall this looks much better than the previous version. Thanks! :) > > I wonder one thing though. You describe the original problem as this: > > 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY > never sets the "ready" status. > > 2. If you don't have the PHY powered on / out of reset (which happens > in qcom_qmp_phy_init()) then when you enable/disable the clock it > doesn't properly update the status. That's why you needed patch #1 in > this series. > > > I wonder: could you solve the above _without_ needing to use > BRANCH_HALT_DELAY in the clock driver? Specifically, can you tell me > what happens if you put the clk_prepare_enable() after you've powered > on the PHY and taken it out of reset but before you check the status? > Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call > right before the "readl_poll_timeout" of the ready status? > > > If you do that, you'll turn everything on. Then you'll check that the > clock's status is OK and then that the PHY's status is OK. Oh! This is what you did in the previous version of the patch, then you said: "That is still needed as PHY might take some time to generate pipe_clk after its PLL is locked". It's really going to take more than the 200 us that the clock driver is giving it? If so, I'd prefer to increase the amount of time waited in the clock driver, or adding a fixed delay _before_ the clk_enable() so that the 200 us that the clock driver gives it would be enough. I'm just not a fan of ignoring status bits if it can be helped. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 3/30/2018 2:24 AM, Doug Anderson wrote: > Hi, > > On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote: >> Hi, >> >> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote: >>> QMP PHY for USB/PCIE requires pipe_clk for locking of >>> retime buffers at the pipe interface. Driver checks for >>> PHY_STATUS without enabling pipe_clk due to which >>> phy_init() fails with initialization timeout. >>> Though pipe_clk is output from PHY (after PLL is programmed >>> during initialization sequence) to GCC clock_ctl and then fed >>> back to PHY but for PHY_STATUS register to reflect successful >>> initialization pipe_clk from GCC must be present. >>> Since, clock driver now ignores status_check for pipe_clk on >>> clk_enable/disable, driver can safely enable/disable pipe_clk >>> from phy_init/exit. >>> >>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >>> --- >>> drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- >>> 1 file changed, 8 insertions(+), 14 deletions(-) >> Overall this looks much better than the previous version. Thanks! :) >> >> I wonder one thing though. You describe the original problem as this: >> >> 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY >> never sets the "ready" status. >> >> 2. If you don't have the PHY powered on / out of reset (which happens >> in qcom_qmp_phy_init()) then when you enable/disable the clock it >> doesn't properly update the status. That's why you needed patch #1 in >> this series. >> >> >> I wonder: could you solve the above _without_ needing to use >> BRANCH_HALT_DELAY in the clock driver? Specifically, can you tell me >> what happens if you put the clk_prepare_enable() after you've powered >> on the PHY and taken it out of reset but before you check the status? >> Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call >> right before the "readl_poll_timeout" of the ready status? >> >> >> If you do that, you'll turn everything on. Then you'll check that the >> clock's status is OK and then that the PHY's status is OK. > Oh! This is what you did in the previous version of the patch, then you said: > > "That is still needed as PHY might take some time to generate pipe_clk > after its PLL is locked". > > It's really going to take more than the 200 us that the clock driver > is giving it? If so, I'd prefer to increase the amount of time waited > in the clock driver, or adding a fixed delay _before_ the clk_enable() > so that the 200 us that the clock driver gives it would be enough. > > I'm just not a fan of ignoring status bits if it can be helped. I too would want to do that but it is not just about the delay. As per QMP PHY hardware designers, pipe_clk should be enabled in GCC as first thing in the PHY initialization sequence. Same sequence also has been used in downstream phy driver always. Changing the sequence might work but I would like to stick to the HPG recommendation and avoid any deviation as PHY issues are very hard to debug.
Hi, On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote: > Hi, > > > On 3/30/2018 2:24 AM, Doug Anderson wrote: >> Hi, >> >> On Thu, Mar 29, 2018 at 11:44 AM, Doug Anderson <dianders@chromium.org> wrote: >>> Hi, >>> >>> On Thu, Mar 29, 2018 at 4:04 AM, Manu Gautam <mgautam@codeaurora.org> wrote: >>>> QMP PHY for USB/PCIE requires pipe_clk for locking of >>>> retime buffers at the pipe interface. Driver checks for >>>> PHY_STATUS without enabling pipe_clk due to which >>>> phy_init() fails with initialization timeout. >>>> Though pipe_clk is output from PHY (after PLL is programmed >>>> during initialization sequence) to GCC clock_ctl and then fed >>>> back to PHY but for PHY_STATUS register to reflect successful >>>> initialization pipe_clk from GCC must be present. >>>> Since, clock driver now ignores status_check for pipe_clk on >>>> clk_enable/disable, driver can safely enable/disable pipe_clk >>>> from phy_init/exit. >>>> >>>> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >>>> --- >>>> drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- >>>> 1 file changed, 8 insertions(+), 14 deletions(-) >>> Overall this looks much better than the previous version. Thanks! :) >>> >>> I wonder one thing though. You describe the original problem as this: >>> >>> 1. If you don't turn the clock on in qcom_qmp_phy_init() then the PHY >>> never sets the "ready" status. >>> >>> 2. If you don't have the PHY powered on / out of reset (which happens >>> in qcom_qmp_phy_init()) then when you enable/disable the clock it >>> doesn't properly update the status. That's why you needed patch #1 in >>> this series. >>> >>> >>> I wonder: could you solve the above _without_ needing to use >>> BRANCH_HALT_DELAY in the clock driver? Specifically, can you tell me >>> what happens if you put the clk_prepare_enable() after you've powered >>> on the PHY and taken it out of reset but before you check the status? >>> Said another way, put the "clk_prepare_enable(qphy->pipe_clk)" call >>> right before the "readl_poll_timeout" of the ready status? >>> >>> >>> If you do that, you'll turn everything on. Then you'll check that the >>> clock's status is OK and then that the PHY's status is OK. >> Oh! This is what you did in the previous version of the patch, then you said: >> >> "That is still needed as PHY might take some time to generate pipe_clk >> after its PLL is locked". >> >> It's really going to take more than the 200 us that the clock driver >> is giving it? If so, I'd prefer to increase the amount of time waited >> in the clock driver, or adding a fixed delay _before_ the clk_enable() >> so that the 200 us that the clock driver gives it would be enough. >> >> I'm just not a fan of ignoring status bits if it can be helped. > > I too would want to do that but it is not just about the delay. > As per QMP PHY hardware designers, pipe_clk should be enabled in GCC > as first thing in the PHY initialization sequence. Same sequence also has > been used in downstream phy driver always. > Changing the sequence might work but I would like to stick to the HPG > recommendation and avoid any deviation as PHY issues are very hard to > debug. So hardware guys tell you that you're _supposed to_ ignore the clock ready bit for that clock and just hope it turns on and settles in time once power comes on for the clock? That doesn't seem ideal. My guess is that it's a bug in the specification that the QMP PHY hardware designers gave you. Stephen can feel free to override me if he disagrees since he's in charge of the clock part of this, but IMHO we should get the specification fixed and turn things on in the order that lets us check the status bits. -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Doug Anderson (2018-04-10 08:05:27) > On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote: > > On 3/30/2018 2:24 AM, Doug Anderson wrote: > >> Oh! This is what you did in the previous version of the patch, then you said: > >> > >> "That is still needed as PHY might take some time to generate pipe_clk > >> after its PLL is locked". > >> > >> It's really going to take more than the 200 us that the clock driver > >> is giving it? If so, I'd prefer to increase the amount of time waited > >> in the clock driver, or adding a fixed delay _before_ the clk_enable() > >> so that the 200 us that the clock driver gives it would be enough. > >> > >> I'm just not a fan of ignoring status bits if it can be helped. > > > > I too would want to do that but it is not just about the delay. > > As per QMP PHY hardware designers, pipe_clk should be enabled in GCC > > as first thing in the PHY initialization sequence. Same sequence also has > > been used in downstream phy driver always. > > Changing the sequence might work but I would like to stick to the HPG > > recommendation and avoid any deviation as PHY issues are very hard to > > debug. > > So hardware guys tell you that you're _supposed to_ ignore the clock > ready bit for that clock and just hope it turns on and settles in time > once power comes on for the clock? That doesn't seem ideal. My guess > is that it's a bug in the specification that the QMP PHY hardware > designers gave you. > > Stephen can feel free to override me if he disagrees since he's in > charge of the clock part of this, but IMHO we should get the > specification fixed and turn things on in the order that lets us check > the status bits. > Presumably there's a PLL "enable" bit in the QMP phy init sequence that we can use to enable the PLL output at a bypass rate and then enable the clk in GCC and then resume the PLL enable sequence. That would allow us to make sure the clk is working. Are the branches in GCC ever turned on or off at runtime though? Or do we just turn them on because they're defaulted off out of reset and then leave them on? I ask because it may be easier to never expose these clks in Linux, hit the enable bits in the branches during clk driver probe, and then act like they never exist because we don't really use them. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 4/11/2018 12:02 AM, Stephen Boyd wrote: > Quoting Doug Anderson (2018-04-10 08:05:27) >> On Mon, Apr 9, 2018 at 11:36 PM, Manu Gautam <mgautam@codeaurora.org> wrote: >>> On 3/30/2018 2:24 AM, Doug Anderson wrote: >>>> Oh! This is what you did in the previous version of the patch, then you said: >>>> >>>> "That is still needed as PHY might take some time to generate pipe_clk >>>> after its PLL is locked". >>>> >>>> It's really going to take more than the 200 us that the clock driver >>>> is giving it? If so, I'd prefer to increase the amount of time waited >>>> in the clock driver, or adding a fixed delay _before_ the clk_enable() >>>> so that the 200 us that the clock driver gives it would be enough. >>>> >>>> I'm just not a fan of ignoring status bits if it can be helped. >>> I too would want to do that but it is not just about the delay. >>> As per QMP PHY hardware designers, pipe_clk should be enabled in GCC >>> as first thing in the PHY initialization sequence. Same sequence also has >>> been used in downstream phy driver always. >>> Changing the sequence might work but I would like to stick to the HPG >>> recommendation and avoid any deviation as PHY issues are very hard to >>> debug. >> So hardware guys tell you that you're _supposed to_ ignore the clock >> ready bit for that clock and just hope it turns on and settles in time >> once power comes on for the clock? That doesn't seem ideal. My guess >> is that it's a bug in the specification that the QMP PHY hardware >> designers gave you. It could be a bug in specification. But same sequence has been well tested on both msm8996 and sdm845, so I would for now like to stick with that for now. >> Stephen can feel free to override me if he disagrees since he's in >> charge of the clock part of this, but IMHO we should get the >> specification fixed and turn things on in the order that lets us check >> the status bits. >> > Presumably there's a PLL "enable" bit in the QMP phy init sequence that > we can use to enable the PLL output at a bypass rate and then enable the > clk in GCC and then resume the PLL enable sequence. That would allow us > to make sure the clk is working. > > Are the branches in GCC ever turned on or off at runtime though? Or do > we just turn them on because they're defaulted off out of reset and then > leave them on? It can be left on always. > > I ask because it may be easier to never expose these clks in Linux, hit > the enable bits in the branches during clk driver probe, and then act > like they never exist because we don't really use them. This sounds better idea. Let me check if I can get a patch for same in msm8996 and sdm845 clock drivers.
Quoting Manu Gautam (2018-04-11 08:37:38) > > > > I ask because it may be easier to never expose these clks in Linux, hit > > the enable bits in the branches during clk driver probe, and then act > > like they never exist because we don't really use them. > This sounds better idea. Let me check if I can get a patch for same in msm8996 > and sdm845 clock drivers. > Ok! Presumably the PHY has a way to tell if it failed to turn on right? Put another way, I'm hoping these branch bits aren't there to help us debug and figure out when the PHY PLL fails to lock. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 4/13/2018 2:08 AM, Stephen Boyd wrote: > Quoting Manu Gautam (2018-04-11 08:37:38) >>> I ask because it may be easier to never expose these clks in Linux, hit >>> the enable bits in the branches during clk driver probe, and then act >>> like they never exist because we don't really use them. >> This sounds better idea. Let me check if I can get a patch for same in msm8996 >> and sdm845 clock drivers. >> > Ok! Presumably the PHY has a way to tell if it failed to turn on right? > Put another way, I'm hoping these branch bits aren't there to help us > debug and figure out when the PHY PLL fails to lock. Yes, PHY has a PCS_STATUS3 register that indicates whether pipe_clk got enabled or not.
diff --git a/drivers/phy/qualcomm/phy-qcom-qmp.c b/drivers/phy/qualcomm/phy-qcom-qmp.c index 6470c5d..fddb1c9 100644 --- a/drivers/phy/qualcomm/phy-qcom-qmp.c +++ b/drivers/phy/qualcomm/phy-qcom-qmp.c @@ -793,19 +793,6 @@ static void qcom_qmp_phy_configure(void __iomem *base, } } -static int qcom_qmp_phy_poweron(struct phy *phy) -{ - struct qmp_phy *qphy = phy_get_drvdata(phy); - struct qcom_qmp *qmp = qphy->qmp; - int ret; - - ret = clk_prepare_enable(qphy->pipe_clk); - if (ret) - dev_err(qmp->dev, "pipe_clk enable failed, err=%d\n", ret); - - return ret; -} - static int qcom_qmp_phy_com_init(struct qcom_qmp *qmp) { const struct qmp_phy_cfg *cfg = qmp->cfg; @@ -974,6 +961,12 @@ static int qcom_qmp_phy_init(struct phy *phy) } } + ret = clk_prepare_enable(qphy->pipe_clk); + if (ret) { + dev_err(qmp->dev, "pipe_clk enable failed err=%d\n", ret); + goto err_clk_enable; + } + /* Tx, Rx, and PCS configurations */ qcom_qmp_phy_configure(tx, cfg->regs, cfg->tx_tbl, cfg->tx_tbl_num); /* Configuration for other LANE for USB-DP combo PHY */ @@ -1019,6 +1012,8 @@ static int qcom_qmp_phy_init(struct phy *phy) return ret; err_pcs_ready: + clk_disable_unprepare(qphy->pipe_clk); +err_clk_enable: if (cfg->has_lane_rst) reset_control_assert(qphy->lane_rst); err_lane_rst: @@ -1283,7 +1278,6 @@ static int phy_pipe_clk_register(struct qcom_qmp *qmp, struct device_node *np) static const struct phy_ops qcom_qmp_phy_gen_ops = { .init = qcom_qmp_phy_init, .exit = qcom_qmp_phy_exit, - .power_on = qcom_qmp_phy_poweron, .set_mode = qcom_qmp_phy_set_mode, .owner = THIS_MODULE, };
QMP PHY for USB/PCIE requires pipe_clk for locking of retime buffers at the pipe interface. Driver checks for PHY_STATUS without enabling pipe_clk due to which phy_init() fails with initialization timeout. Though pipe_clk is output from PHY (after PLL is programmed during initialization sequence) to GCC clock_ctl and then fed back to PHY but for PHY_STATUS register to reflect successful initialization pipe_clk from GCC must be present. Since, clock driver now ignores status_check for pipe_clk on clk_enable/disable, driver can safely enable/disable pipe_clk from phy_init/exit. Signed-off-by: Manu Gautam <mgautam@codeaurora.org> --- drivers/phy/qualcomm/phy-qcom-qmp.c | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-)