Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Marc Gonzalez]

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.

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Jeffrey Hugo]

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),
> 


-- 
Jeffrey Hugo
Qualcomm Datacenter Technologies as an affiliate of Qualcomm 
Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Stephen Boyd]

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

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Manu Gautam]

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.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Stephen Boyd]

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.

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Manu Gautam]

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().


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Stephen Boyd]

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?

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Manu Gautam]

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.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v1] clk: qcom: Skip halt checks on gcc_pcie_0_pipe_clk for 8998

[Stephen Boyd]

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.