| Message ID | 20230419133013.2563-2-quic_tdas@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add GCC and RPMHCC support for sdx75 | expand |
Quoting Taniya Das (2023-04-19 06:30:10) > From: Imran Shaik <quic_imrashai@quicinc.com> > > Add support to handle the invert logic for branch2 clocks. > Invert branch halt would indicate the clock ON when CLK_OFF > bit is '1' and OFF when CLK_OFF bit is '0'. > > Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com> > Signed-off-by: Taniya Das <quic_tdas@quicinc.com> > --- > drivers/clk/qcom/clk-branch.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c > index f869fc6aaed6..4b24d45be771 100644 > --- a/drivers/clk/qcom/clk-branch.c > +++ b/drivers/clk/qcom/clk-branch.c > @@ -48,6 +48,7 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) > { > u32 val; > u32 mask; > + bool invert = (br->halt_check == BRANCH_HALT_ENABLE); > > mask = BRANCH_NOC_FSM_STATUS_MASK << BRANCH_NOC_FSM_STATUS_SHIFT; > mask |= BRANCH_CLK_OFF; > @@ -56,9 +57,16 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) > > if (enabling) { > val &= mask; > + > + if (invert) > + return (val & BRANCH_CLK_OFF) == BRANCH_CLK_OFF; > + > return (val & BRANCH_CLK_OFF) == 0 || > val == BRANCH_NOC_FSM_STATUS_ON; Do these clks have a NOC_FSM_STATUS bit? I think it would be better to make a local variable for the val we're looking for, and then test for that. We may need a mask as well, but the idea is to not duplicate the test and return from multiple places. > } else { > + if (invert) > + return (val & BRANCH_CLK_OFF) == 0; > + > return val & BRANCH_CLK_OFF; > } While at it, I'd get rid of this else and de-indent the code because if we're 'enabling' we'll return from the function regardless.
Hello Stephen, Thanks for your review. On 4/20/2023 3:07 AM, Stephen Boyd wrote: > Quoting Taniya Das (2023-04-19 06:30:10) >> From: Imran Shaik <quic_imrashai@quicinc.com> >> >> Add support to handle the invert logic for branch2 clocks. >> Invert branch halt would indicate the clock ON when CLK_OFF >> bit is '1' and OFF when CLK_OFF bit is '0'. >> >> Signed-off-by: Imran Shaik <quic_imrashai@quicinc.com> >> Signed-off-by: Taniya Das <quic_tdas@quicinc.com> >> --- >> drivers/clk/qcom/clk-branch.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c >> index f869fc6aaed6..4b24d45be771 100644 >> --- a/drivers/clk/qcom/clk-branch.c >> +++ b/drivers/clk/qcom/clk-branch.c >> @@ -48,6 +48,7 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) >> { >> u32 val; >> u32 mask; >> + bool invert = (br->halt_check == BRANCH_HALT_ENABLE); >> >> mask = BRANCH_NOC_FSM_STATUS_MASK << BRANCH_NOC_FSM_STATUS_SHIFT; >> mask |= BRANCH_CLK_OFF; >> @@ -56,9 +57,16 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) >> >> if (enabling) { >> val &= mask; >> + >> + if (invert) >> + return (val & BRANCH_CLK_OFF) == BRANCH_CLK_OFF; >> + >> return (val & BRANCH_CLK_OFF) == 0 || >> val == BRANCH_NOC_FSM_STATUS_ON; > > Do these clks have a NOC_FSM_STATUS bit? I think it would be better to > make a local variable for the val we're looking for, and then test for > that. We may need a mask as well, but the idea is to not duplicate the > test and return from multiple places. > Clocks which has invert status doesn't have NOC_FSM_STATUS bit. Will remove the multiple returns in next patch. >> } else { >> + if (invert) >> + return (val & BRANCH_CLK_OFF) == 0; >> + >> return val & BRANCH_CLK_OFF; >> } > > While at it, I'd get rid of this else and de-indent the code because if > we're 'enabling' we'll return from the function regardless. Yes, Stephen, will take care in the next patch.
diff --git a/drivers/clk/qcom/clk-branch.c b/drivers/clk/qcom/clk-branch.c index f869fc6aaed6..4b24d45be771 100644 --- a/drivers/clk/qcom/clk-branch.c +++ b/drivers/clk/qcom/clk-branch.c @@ -48,6 +48,7 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) { u32 val; u32 mask; + bool invert = (br->halt_check == BRANCH_HALT_ENABLE); mask = BRANCH_NOC_FSM_STATUS_MASK << BRANCH_NOC_FSM_STATUS_SHIFT; mask |= BRANCH_CLK_OFF; @@ -56,9 +57,16 @@ static bool clk_branch2_check_halt(const struct clk_branch *br, bool enabling) if (enabling) { val &= mask; + + if (invert) + return (val & BRANCH_CLK_OFF) == BRANCH_CLK_OFF; + return (val & BRANCH_CLK_OFF) == 0 || val == BRANCH_NOC_FSM_STATUS_ON; } else { + if (invert) + return (val & BRANCH_CLK_OFF) == 0; + return val & BRANCH_CLK_OFF; } }