| Message ID | a7e27415-02d9-bfe9-c0ea-59dc236a7f91@free.fr (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998 | expand |
On 25/03/2019 14:49, Marc Gonzalez wrote: > See similar issue solved by commit 5f2420ed2189 > ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") > > Without this patch, PCIe PHY init fails: > > qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 > phy phy-1c06000.phy.0: phy init failed --> -16 > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > drivers/clk/qcom/gcc-msm8998.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > index c240fba794c7..033688264c7b 100644 > --- a/drivers/clk/qcom/gcc-msm8998.c > +++ b/drivers/clk/qcom/gcc-msm8998.c > @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { > > static struct clk_branch gcc_pcie_0_pipe_clk = { > .halt_reg = 0x6b018, > - .halt_check = BRANCH_HALT, > + .halt_check = BRANCH_HALT_SKIP, > .clkr = { > .enable_reg = 0x6b018, > .enable_mask = BIT(0), Actually, 5f2420ed2189 is not the only similar instance. $ git log --oneline -G BRANCH_HALT_SKIP drivers/clk/qcom | grep -v controller 924a86bf9793 clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998 5f2420ed2189 clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998 2abf856202fd clk: qcom: gcc-msm8998: Disable halt check of UFS clocks 5f75b78d3d67 clk: qcom: gcc-msm8996: Disable halt check on UFS tx clock 12d807cd34b8 clk: qcom: gcc-msm8996: Disable halt check on UFS clocks 096abdc296f1 clk: msm8996-gcc: Mark halt check as no-op for USB/PCIE pipe_clk 7d99ced8f4c6 clk: qcom: Add support for BRANCH_HALT_SKIP flag for branch clocks Regards.
Quoting Marc Gonzalez (2019-03-28 09:26:59) > On 25/03/2019 14:49, Marc Gonzalez wrote: > > > See similar issue solved by commit 5f2420ed2189 > > ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") > > > > Without this patch, PCIe PHY init fails: > > > > qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 > > phy phy-1c06000.phy.0: phy init failed --> -16 > > > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > > --- > > drivers/clk/qcom/gcc-msm8998.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > > index c240fba794c7..033688264c7b 100644 > > --- a/drivers/clk/qcom/gcc-msm8998.c > > +++ b/drivers/clk/qcom/gcc-msm8998.c > > @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { > > > > static struct clk_branch gcc_pcie_0_pipe_clk = { > > .halt_reg = 0x6b018, > > - .halt_check = BRANCH_HALT, > > + .halt_check = BRANCH_HALT_SKIP, > > .clkr = { > > .enable_reg = 0x6b018, > > .enable_mask = BIT(0), > > Actually, 5f2420ed2189 is not the only similar instance. > > $ git log --oneline -G BRANCH_HALT_SKIP drivers/clk/qcom | grep -v controller > 924a86bf9793 clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998 > 5f2420ed2189 clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998 > 2abf856202fd clk: qcom: gcc-msm8998: Disable halt check of UFS clocks > 5f75b78d3d67 clk: qcom: gcc-msm8996: Disable halt check on UFS tx clock > 12d807cd34b8 clk: qcom: gcc-msm8996: Disable halt check on UFS clocks > 096abdc296f1 clk: msm8996-gcc: Mark halt check as no-op for USB/PCIE pipe_clk > 7d99ced8f4c6 clk: qcom: Add support for BRANCH_HALT_SKIP flag for branch clocks > I keep asking Qualcomm engineers everytime this comes up why they can't fix their phy initialization sequence. Too bad they don't care anymore!
On 29/03/2019 23:32, Stephen Boyd wrote: > I keep asking Qualcomm engineers every time this comes up why they can't > fix their phy initialization sequence. If I had the clock-tree documentation, I'd give it a shot. But qcom sits on their "internal" documentation like a dragon on his pile o' gold. In the mean time, will you take the patch? :-) Regards.
On 25/03/2019 14:49, Marc Gonzalez wrote: > See similar issue solved by commit 5f2420ed2189 > ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") > > Without this patch, PCIe PHY init fails: > > qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 > phy phy-1c06000.phy.0: phy init failed --> -16 > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- > drivers/clk/qcom/gcc-msm8998.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > index c240fba794c7..033688264c7b 100644 > --- a/drivers/clk/qcom/gcc-msm8998.c > +++ b/drivers/clk/qcom/gcc-msm8998.c > @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { > > static struct clk_branch gcc_pcie_0_pipe_clk = { > .halt_reg = 0x6b018, > - .halt_check = BRANCH_HALT, > + .halt_check = BRANCH_HALT_SKIP, > .clkr = { > .enable_reg = 0x6b018, > .enable_mask = BIT(0), Bjorn, Jeffrey, et al, Could someone review this patch and confirm it is required for functional PCIe? Regards.
On 3/25/2019 7:49 AM, Marc Gonzalez wrote: > See similar issue solved by commit 5f2420ed2189 > ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") > > Without this patch, PCIe PHY init fails: > > qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 > phy phy-1c06000.phy.0: phy init failed --> -16 > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> Reviewed-by: Jeffrey Hugo <jhugo@codeaurora.org> > --- > drivers/clk/qcom/gcc-msm8998.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c > index c240fba794c7..033688264c7b 100644 > --- a/drivers/clk/qcom/gcc-msm8998.c > +++ b/drivers/clk/qcom/gcc-msm8998.c > @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { > > static struct clk_branch gcc_pcie_0_pipe_clk = { > .halt_reg = 0x6b018, > - .halt_check = BRANCH_HALT, > + .halt_check = BRANCH_HALT_SKIP, > .clkr = { > .enable_reg = 0x6b018, > .enable_mask = BIT(0), >
Quoting Marc Gonzalez (2019-03-25 06:49:54) > See similar issue solved by commit 5f2420ed2189 > ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") > > Without this patch, PCIe PHY init fails: > > qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 > phy phy-1c06000.phy.0: phy init failed --> -16 > > Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> > --- Applied to clk-next
Hi Steve, On 3/30/2019 4:02 AM, Stephen Boyd wrote: > Quoting Marc Gonzalez (2019-03-28 09:26:59) >> On 25/03/2019 14:49, Marc Gonzalez wrote: >> >>> See similar issue solved by commit 5f2420ed2189 >>> ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") >>> >>> Without this patch, PCIe PHY init fails: >>> >>> qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 >>> phy phy-1c06000.phy.0: phy init failed --> -16 >>> >>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> >>> --- >>> drivers/clk/qcom/gcc-msm8998.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c >>> index c240fba794c7..033688264c7b 100644 >>> --- a/drivers/clk/qcom/gcc-msm8998.c >>> +++ b/drivers/clk/qcom/gcc-msm8998.c >>> @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { >>> >>> static struct clk_branch gcc_pcie_0_pipe_clk = { >>> .halt_reg = 0x6b018, >>> - .halt_check = BRANCH_HALT, >>> + .halt_check = BRANCH_HALT_SKIP, >>> .clkr = { >>> .enable_reg = 0x6b018, >>> .enable_mask = BIT(0), >> Actually, 5f2420ed2189 is not the only similar instance. >> >> $ git log --oneline -G BRANCH_HALT_SKIP drivers/clk/qcom | grep -v controller >> 924a86bf9793 clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998 >> 5f2420ed2189 clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998 >> 2abf856202fd clk: qcom: gcc-msm8998: Disable halt check of UFS clocks >> 5f75b78d3d67 clk: qcom: gcc-msm8996: Disable halt check on UFS tx clock >> 12d807cd34b8 clk: qcom: gcc-msm8996: Disable halt check on UFS clocks >> 096abdc296f1 clk: msm8996-gcc: Mark halt check as no-op for USB/PCIE pipe_clk >> 7d99ced8f4c6 clk: qcom: Add support for BRANCH_HALT_SKIP flag for branch clocks >> > I keep asking Qualcomm engineers everytime this comes up why they can't > fix their phy initialization sequence. > > Too bad they don't care anymore! I have followed this up with QMP PHY hardware designers and they have confirmed that QMP PHY must have pipe clock enabled at the beginning of initialization sequence i.e. before bringing it out of reset and starting it. Otherwise there is possibility of incorrect locking of pipe_interface/ retime buffers in PHY. Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag.
Quoting Manu Gautam (2019-10-09 01:31:09) > On 3/30/2019 4:02 AM, Stephen Boyd wrote: > > Quoting Marc Gonzalez (2019-03-28 09:26:59) > >> On 25/03/2019 14:49, Marc Gonzalez wrote: > >> > >>> .enable_mask = BIT(0), > >> Actually, 5f2420ed2189 is not the only similar instance. > >> > >> $ git log --oneline -G BRANCH_HALT_SKIP drivers/clk/qcom | grep -v controller > >> 924a86bf9793 clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998 > >> 5f2420ed2189 clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998 > >> 2abf856202fd clk: qcom: gcc-msm8998: Disable halt check of UFS clocks > >> 5f75b78d3d67 clk: qcom: gcc-msm8996: Disable halt check on UFS tx clock > >> 12d807cd34b8 clk: qcom: gcc-msm8996: Disable halt check on UFS clocks > >> 096abdc296f1 clk: msm8996-gcc: Mark halt check as no-op for USB/PCIE pipe_clk > >> 7d99ced8f4c6 clk: qcom: Add support for BRANCH_HALT_SKIP flag for branch clocks > >> > > I keep asking Qualcomm engineers everytime this comes up why they can't > > fix their phy initialization sequence. > > > > Too bad they don't care anymore! > > > I have followed this up with QMP PHY hardware designers and they have > confirmed that QMP PHY must have pipe clock enabled at the beginning > of initialization sequence i.e. before bringing it out of reset and starting it. Awesome, thanks for following up. > > Otherwise there is possibility of incorrect locking of pipe_interface/ > retime buffers in PHY. > Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag. Does anything go wrong if we just leave these clks enabled forever out of boot? I'm inclined to rip the clks out and just slam the branch enable bit on all the time in gcc driver probe and return NULL to the callers of clk_get() for these clks. I don't see how this would be a problem because when the upstream phy is disabled this clk is disabled and so we aren't wasting power. It should also save us time and memory because now we don't have to call into the clk framework to turn it on and sequence that just right in the phy driver.
Hi, On 10/10/2019 9:45 AM, Stephen Boyd wrote: > Quoting Manu Gautam (2019-10-09 01:31:09) >> [snip] >> I have followed this up with QMP PHY hardware designers and they have >> confirmed that QMP PHY must have pipe clock enabled at the beginning >> of initialization sequence i.e. before bringing it out of reset and starting it. > Awesome, thanks for following up. > >> Otherwise there is possibility of incorrect locking of pipe_interface/ >> retime buffers in PHY. >> Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag. > Does anything go wrong if we just leave these clks enabled forever out > of boot? I'm inclined to rip the clks out and just slam the branch > enable bit on all the time in gcc driver probe and return NULL to the > callers of clk_get() for these clks. I don't see how this would be a > problem because when the upstream phy is disabled this clk is disabled > and so we aren't wasting power. It should also save us time and memory > because now we don't have to call into the clk framework to turn it on > and sequence that just right in the phy driver. That might work, however on some platforms gcc_pipe_clk parent is changed to XO and back to phy_pipe_clk across low power mode. It requires PHY driver to use clk_set_parent().
Quoting Manu Gautam (2019-10-10 00:33:32) > Hi, > > On 10/10/2019 9:45 AM, Stephen Boyd wrote: > > Quoting Manu Gautam (2019-10-09 01:31:09) > >> > [snip] > >> I have followed this up with QMP PHY hardware designers and they have > >> confirmed that QMP PHY must have pipe clock enabled at the beginning > >> of initialization sequence i.e. before bringing it out of reset and starting it. > > Awesome, thanks for following up. > > > >> Otherwise there is possibility of incorrect locking of pipe_interface/ > >> retime buffers in PHY. > >> Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag. > > Does anything go wrong if we just leave these clks enabled forever out > > of boot? I'm inclined to rip the clks out and just slam the branch > > enable bit on all the time in gcc driver probe and return NULL to the > > callers of clk_get() for these clks. I don't see how this would be a > > problem because when the upstream phy is disabled this clk is disabled > > and so we aren't wasting power. It should also save us time and memory > > because now we don't have to call into the clk framework to turn it on > > and sequence that just right in the phy driver. > > That might work, however on some platforms gcc_pipe_clk parent is changed to > XO and back to phy_pipe_clk across low power mode. > It requires PHY driver to use clk_set_parent(). > Hm ok. Where is the call to clk_set_parent()? I don't see this in the kernel. $ git grep clk_set_parent -- drivers/usb/phy drivers/phy | wc -l 0 What platforms do this? Are they upstream?
On 10/11/2019 1:18 AM, Stephen Boyd wrote: > Quoting Manu Gautam (2019-10-10 00:33:32) >> Hi, >> >> On 10/10/2019 9:45 AM, Stephen Boyd wrote: >>> Quoting Manu Gautam (2019-10-09 01:31:09) >> [snip] >>>> I have followed this up with QMP PHY hardware designers and they have >>>> confirmed that QMP PHY must have pipe clock enabled at the beginning >>>> of initialization sequence i.e. before bringing it out of reset and starting it. >>> Awesome, thanks for following up. >>> >>>> Otherwise there is possibility of incorrect locking of pipe_interface/ >>>> retime buffers in PHY. >>>> Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag. >>> Does anything go wrong if we just leave these clks enabled forever out >>> of boot? I'm inclined to rip the clks out and just slam the branch >>> enable bit on all the time in gcc driver probe and return NULL to the >>> callers of clk_get() for these clks. I don't see how this would be a >>> problem because when the upstream phy is disabled this clk is disabled >>> and so we aren't wasting power. It should also save us time and memory >>> because now we don't have to call into the clk framework to turn it on >>> and sequence that just right in the phy driver. >> That might work, however on some platforms gcc_pipe_clk parent is changed to >> XO and back to phy_pipe_clk across low power mode. >> It requires PHY driver to use clk_set_parent(). >> > Hm ok. Where is the call to clk_set_parent()? I don't see this in the > kernel. > > $ git grep clk_set_parent -- drivers/usb/phy drivers/phy | wc -l > 0 > > What platforms do this? Are they upstream? They are some recent platforms and not on upstream yet.
Quoting Manu Gautam (2019-10-10 20:47:12) > > On 10/11/2019 1:18 AM, Stephen Boyd wrote: > > Quoting Manu Gautam (2019-10-10 00:33:32) > >> Hi, > >> > >> On 10/10/2019 9:45 AM, Stephen Boyd wrote: > >>> Quoting Manu Gautam (2019-10-09 01:31:09) > >> [snip] > >>>> I have followed this up with QMP PHY hardware designers and they have > >>>> confirmed that QMP PHY must have pipe clock enabled at the beginning > >>>> of initialization sequence i.e. before bringing it out of reset and starting it. > >>> Awesome, thanks for following up. > >>> > >>>> Otherwise there is possibility of incorrect locking of pipe_interface/ > >>>> retime buffers in PHY. > >>>> Hence, for both USB and PCIe we have to continue to use HALT_SKIP flag. > >>> Does anything go wrong if we just leave these clks enabled forever out > >>> of boot? I'm inclined to rip the clks out and just slam the branch > >>> enable bit on all the time in gcc driver probe and return NULL to the > >>> callers of clk_get() for these clks. I don't see how this would be a > >>> problem because when the upstream phy is disabled this clk is disabled > >>> and so we aren't wasting power. It should also save us time and memory > >>> because now we don't have to call into the clk framework to turn it on > >>> and sequence that just right in the phy driver. > >> That might work, however on some platforms gcc_pipe_clk parent is changed to > >> XO and back to phy_pipe_clk across low power mode. > >> It requires PHY driver to use clk_set_parent(). > >> > > Hm ok. Where is the call to clk_set_parent()? I don't see this in the > > kernel. > > > > $ git grep clk_set_parent -- drivers/usb/phy drivers/phy | wc -l > > 0 > > > > What platforms do this? Are they upstream? > > They are some recent platforms and not on upstream yet. > Cool so we can remove it on the platforms that are upstream right now and get to this problem later.
diff --git a/drivers/clk/qcom/gcc-msm8998.c b/drivers/clk/qcom/gcc-msm8998.c index c240fba794c7..033688264c7b 100644 --- a/drivers/clk/qcom/gcc-msm8998.c +++ b/drivers/clk/qcom/gcc-msm8998.c @@ -2161,7 +2161,7 @@ static struct clk_branch gcc_pcie_0_mstr_axi_clk = { static struct clk_branch gcc_pcie_0_pipe_clk = { .halt_reg = 0x6b018, - .halt_check = BRANCH_HALT, + .halt_check = BRANCH_HALT_SKIP, .clkr = { .enable_reg = 0x6b018, .enable_mask = BIT(0),
See similar issue solved by commit 5f2420ed2189 ("clk: qcom: Skip halt checks on gcc_usb3_phy_pipe_clk for 8998") Without this patch, PCIe PHY init fails: qcom-qmp-phy 1c06000.phy: pipe_clk enable failed err=-16 phy phy-1c06000.phy.0: phy init failed --> -16 Signed-off-by: Marc Gonzalez <marc.w.gonzalez@free.fr> --- drivers/clk/qcom/gcc-msm8998.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)