| Message ID | 20240530-x1e80100-clk-gcc-fix-halt-check-for-usb-phy-pipe-clks-v1-1-16c6f4dccbd5@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | clk: qcom: gcc-x1e80100: Fix halt_check for all 3 USB PHY pipe clocks | expand |
On Thu, May 30, 2024 at 07:48:44PM +0300, Abel Vesa wrote: > Since the pipe clocks are fed by the QMP PHYs, they are not under the > GCC control, therefore the halt bit might not get. This will lead to > the clock driver reporting the clock as stuck, but that is inaccurate. > So instead of waiting for the halt bit to get set, just use the > HALT_DELAY flag. > Isn't this behavior applicable to PCIe pipe clocks also? - Mani > Fixes: 161b7c401f4b ("clk: qcom: Add Global Clock controller (GCC) driver for X1E80100") > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/gcc-x1e80100.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/clk/qcom/gcc-x1e80100.c b/drivers/clk/qcom/gcc-x1e80100.c > index 1404017be918..afff7cd14848 100644 > --- a/drivers/clk/qcom/gcc-x1e80100.c > +++ b/drivers/clk/qcom/gcc-x1e80100.c > @@ -5186,7 +5186,7 @@ static struct clk_regmap_mux gcc_usb3_prim_phy_pipe_clk_src = { > > static struct clk_branch gcc_usb3_prim_phy_pipe_clk = { > .halt_reg = 0x39068, > - .halt_check = BRANCH_HALT_VOTED, > + .halt_check = BRANCH_HALT_DELAY, > .hwcg_reg = 0x39068, > .hwcg_bit = 1, > .clkr = { > @@ -5257,7 +5257,7 @@ static struct clk_regmap_mux gcc_usb3_sec_phy_pipe_clk_src = { > > static struct clk_branch gcc_usb3_sec_phy_pipe_clk = { > .halt_reg = 0xa1068, > - .halt_check = BRANCH_HALT_VOTED, > + .halt_check = BRANCH_HALT_DELAY, > .hwcg_reg = 0xa1068, > .hwcg_bit = 1, > .clkr = { > @@ -5327,7 +5327,7 @@ static struct clk_regmap_mux gcc_usb3_tert_phy_pipe_clk_src = { > > static struct clk_branch gcc_usb3_tert_phy_pipe_clk = { > .halt_reg = 0xa2068, > - .halt_check = BRANCH_HALT_VOTED, > + .halt_check = BRANCH_HALT_DELAY, > .hwcg_reg = 0xa2068, > .hwcg_bit = 1, > .clkr = { > > --- > base-commit: 9d99040b1bc8dbf385a8aa535e9efcdf94466e19 > change-id: 20240530-x1e80100-clk-gcc-fix-halt-check-for-usb-phy-pipe-clks-ef8cf5b5631b > > Best regards, > -- > Abel Vesa <abel.vesa@linaro.org> > >
On Thu, May 30, 2024 at 07:48:44PM +0300, Abel Vesa wrote: > Since the pipe clocks are fed by the QMP PHYs, they are not under the > GCC control, therefore the halt bit might not get. This will lead to Nit: might not get... set? cleared? > the clock driver reporting the clock as stuck, but that is inaccurate. > So instead of waiting for the halt bit to get set, just use the > HALT_DELAY flag. > > Fixes: 161b7c401f4b ("clk: qcom: Add Global Clock controller (GCC) driver for X1E80100") > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/clk/qcom/gcc-x1e80100.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
On 30.05.2024 6:48 PM, Abel Vesa wrote: > Since the pipe clocks are fed by the QMP PHYs, they are not under the > GCC control, therefore the halt bit might not get. This will lead to > the clock driver reporting the clock as stuck, but that is inaccurate. > So instead of waiting for the halt bit to get set, just use the > HALT_DELAY flag. I can see this being a good fix, however the commit message could use some massaging. The issue you're facing is that you can't toggle these branch clocks before the QMPPHY has been fully initialized (as that initialization sequence contains, among other things, setting up the internal PLL). We're doing the HALT_SKIP thing to hack away the complexity of coming back to this. In a perfect world, we'd "defer" the toggle requests and all code that makes these, but it sounds overly complex for what it is. So in essence, the clock could really be stuck. And FWIW, you missed gcc_usb3_mp_phy_pipe_[01]_clk, and these are orphans which could use some fixing up as well. Konrad
diff --git a/drivers/clk/qcom/gcc-x1e80100.c b/drivers/clk/qcom/gcc-x1e80100.c index 1404017be918..afff7cd14848 100644 --- a/drivers/clk/qcom/gcc-x1e80100.c +++ b/drivers/clk/qcom/gcc-x1e80100.c @@ -5186,7 +5186,7 @@ static struct clk_regmap_mux gcc_usb3_prim_phy_pipe_clk_src = { static struct clk_branch gcc_usb3_prim_phy_pipe_clk = { .halt_reg = 0x39068, - .halt_check = BRANCH_HALT_VOTED, + .halt_check = BRANCH_HALT_DELAY, .hwcg_reg = 0x39068, .hwcg_bit = 1, .clkr = { @@ -5257,7 +5257,7 @@ static struct clk_regmap_mux gcc_usb3_sec_phy_pipe_clk_src = { static struct clk_branch gcc_usb3_sec_phy_pipe_clk = { .halt_reg = 0xa1068, - .halt_check = BRANCH_HALT_VOTED, + .halt_check = BRANCH_HALT_DELAY, .hwcg_reg = 0xa1068, .hwcg_bit = 1, .clkr = { @@ -5327,7 +5327,7 @@ static struct clk_regmap_mux gcc_usb3_tert_phy_pipe_clk_src = { static struct clk_branch gcc_usb3_tert_phy_pipe_clk = { .halt_reg = 0xa2068, - .halt_check = BRANCH_HALT_VOTED, + .halt_check = BRANCH_HALT_DELAY, .hwcg_reg = 0xa2068, .hwcg_bit = 1, .clkr = {
Since the pipe clocks are fed by the QMP PHYs, they are not under the GCC control, therefore the halt bit might not get. This will lead to the clock driver reporting the clock as stuck, but that is inaccurate. So instead of waiting for the halt bit to get set, just use the HALT_DELAY flag. Fixes: 161b7c401f4b ("clk: qcom: Add Global Clock controller (GCC) driver for X1E80100") Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/clk/qcom/gcc-x1e80100.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) --- base-commit: 9d99040b1bc8dbf385a8aa535e9efcdf94466e19 change-id: 20240530-x1e80100-clk-gcc-fix-halt-check-for-usb-phy-pipe-clks-ef8cf5b5631b Best regards,