| Message ID | 20230303-topic-rpmcc_sleep-v2-9-ae80a325fe94@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | SMD RPMCC sleep preparations | expand |
On 08/03/2023 23:35, Konrad Dybcio wrote: > Some bus clock should always have a minimum (19.2 MHz) vote cast on > them, otherwise the platform will fall apart, hang and reboot. > > Add support for specifying which clocks should be kept alive and > always keep a vote on XO_A to make sure the clock tree doesn't > collapse. This removes the need to keep a maximum vote that was > previously guaranteed by clk_smd_rpm_handoff. > > This commit is a combination of existing (not-exactly-upstream) work > by Taniya Das, Shawn Guo and myself. > > Co-developed-by: Shawn Guo <shawn.guo@linaro.org> > Co-developed-by: Taniya Das <quic_tdas@quicinc.com> > Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> > --- > drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c > index eb7781e5c8c1..d89918f9ae60 100644 > --- a/drivers/clk/qcom/clk-smd-rpm.c > +++ b/drivers/clk/qcom/clk-smd-rpm.c > @@ -45,15 +45,17 @@ > }, \ > }; \ > __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \ > - r_id, key, ao_flags) > + r_id, key, ao_flags, false) > > #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \ > - type, r_id, key, ao_flags) \ > + type, r_id, key, ao_flags, \ > + _keep_alive) \ > static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \ > .rpm_res_type = (type), \ > .rpm_clk_id = (r_id), \ > .active_only = true, \ > .rpm_key = (key), \ > + .keep_alive = (_keep_alive), \ > .peer = &clk_smd_rpm_##_prefix##_name, \ > .rate = INT_MAX, \ > .hw.init = &(struct clk_init_data){ \ > @@ -170,6 +172,7 @@ struct clk_smd_rpm { > const bool active_only; > bool enabled; > bool branch; > + bool keep_alive; > struct clk_smd_rpm *peer; > struct clk_hw hw; > unsigned long rate; > @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r) > .value = cpu_to_le32(r->branch ? 1 : INT_MAX), > }; > > + /* Set up keepalive clocks with a minimum bus rate */ > + if (r->keep_alive) > + req.value = cpu_to_le32(19200); /* 19.2 MHz */ Should this be set to cpu_to_le32(max(19200, ...)) ? > + > ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE, > r->rpm_res_type, r->rpm_clk_id, &req, > sizeof(req)); > if (ret) > return ret; > + > ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE, > r->rpm_res_type, r->rpm_clk_id, &req, > sizeof(req)); > @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw) > return r->enabled; > } > > +static int clk_smd_rpm_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > +{ > + struct clk_smd_rpm *r = to_clk_smd_rpm(hw); > + > + /* > + * RPM resolves the rates internally. All we have to do on the kernel > + * side is ensure that we don't accidentally put down the keepalive > + * clocks, which could happen if they received a vote below 19.2 MHz. > + */ > + if (r->keep_alive) > + req->rate = max(req->rate, 19200000UL); > + > + return 0; > +} > + > static const struct clk_ops clk_smd_rpm_ops = { > .prepare = clk_smd_rpm_prepare, > .unprepare = clk_smd_rpm_unprepare, > .set_rate = clk_smd_rpm_set_rate, > .round_rate = clk_smd_rpm_round_rate, > .recalc_rate = clk_smd_rpm_recalc_rate, > + .determine_rate = clk_smd_rpm_determine_rate, > .is_enabled = clk_smd_rpm_is_enabled, > .is_prepared = clk_smd_rpm_is_enabled, > }; >
On 9.03.2023 01:54, Dmitry Baryshkov wrote: > On 08/03/2023 23:35, Konrad Dybcio wrote: >> Some bus clock should always have a minimum (19.2 MHz) vote cast on >> them, otherwise the platform will fall apart, hang and reboot. >> >> Add support for specifying which clocks should be kept alive and >> always keep a vote on XO_A to make sure the clock tree doesn't >> collapse. This removes the need to keep a maximum vote that was >> previously guaranteed by clk_smd_rpm_handoff. >> >> This commit is a combination of existing (not-exactly-upstream) work >> by Taniya Das, Shawn Guo and myself. >> >> Co-developed-by: Shawn Guo <shawn.guo@linaro.org> >> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> >> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> >> --- >> drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++-- >> 1 file changed, 27 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c >> index eb7781e5c8c1..d89918f9ae60 100644 >> --- a/drivers/clk/qcom/clk-smd-rpm.c >> +++ b/drivers/clk/qcom/clk-smd-rpm.c >> @@ -45,15 +45,17 @@ >> }, \ >> }; \ >> __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \ >> - r_id, key, ao_flags) >> + r_id, key, ao_flags, false) >> #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \ >> - type, r_id, key, ao_flags) \ >> + type, r_id, key, ao_flags, \ >> + _keep_alive) \ >> static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \ >> .rpm_res_type = (type), \ >> .rpm_clk_id = (r_id), \ >> .active_only = true, \ >> .rpm_key = (key), \ >> + .keep_alive = (_keep_alive), \ >> .peer = &clk_smd_rpm_##_prefix##_name, \ >> .rate = INT_MAX, \ >> .hw.init = &(struct clk_init_data){ \ >> @@ -170,6 +172,7 @@ struct clk_smd_rpm { >> const bool active_only; >> bool enabled; >> bool branch; >> + bool keep_alive; >> struct clk_smd_rpm *peer; >> struct clk_hw hw; >> unsigned long rate; >> @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r) >> .value = cpu_to_le32(r->branch ? 1 : INT_MAX), >> }; >> + /* Set up keepalive clocks with a minimum bus rate */ >> + if (r->keep_alive) >> + req.value = cpu_to_le32(19200); /* 19.2 MHz */ > > > Should this be set to cpu_to_le32(max(19200, ...)) ? I was debating this. Downstream explicitly sets 19.2 Mhz here and the only regression I can think of is that we'd throttle a bus that was left on by the bootloader and is (ab)used by us.. But then, it's only an active vote, and we're voting INT_MAX on the non-active-only one, so that's a non-issue. So I think 19.2 here is okay as the bare minimum, whatever stupidity the eventual interconnect driver may entail.. Konrad > >> + >> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE, >> r->rpm_res_type, r->rpm_clk_id, &req, >> sizeof(req)); >> if (ret) >> return ret; >> + >> ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE, >> r->rpm_res_type, r->rpm_clk_id, &req, >> sizeof(req)); >> @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw) >> return r->enabled; >> } >> +static int clk_smd_rpm_determine_rate(struct clk_hw *hw, >> + struct clk_rate_request *req) >> +{ >> + struct clk_smd_rpm *r = to_clk_smd_rpm(hw); >> + >> + /* >> + * RPM resolves the rates internally. All we have to do on the kernel >> + * side is ensure that we don't accidentally put down the keepalive >> + * clocks, which could happen if they received a vote below 19.2 MHz. >> + */ >> + if (r->keep_alive) >> + req->rate = max(req->rate, 19200000UL); >> + >> + return 0; >> +} >> + >> static const struct clk_ops clk_smd_rpm_ops = { >> .prepare = clk_smd_rpm_prepare, >> .unprepare = clk_smd_rpm_unprepare, >> .set_rate = clk_smd_rpm_set_rate, >> .round_rate = clk_smd_rpm_round_rate, >> .recalc_rate = clk_smd_rpm_recalc_rate, >> + .determine_rate = clk_smd_rpm_determine_rate, >> .is_enabled = clk_smd_rpm_is_enabled, >> .is_prepared = clk_smd_rpm_is_enabled, >> }; >> >
diff --git a/drivers/clk/qcom/clk-smd-rpm.c b/drivers/clk/qcom/clk-smd-rpm.c index eb7781e5c8c1..d89918f9ae60 100644 --- a/drivers/clk/qcom/clk-smd-rpm.c +++ b/drivers/clk/qcom/clk-smd-rpm.c @@ -45,15 +45,17 @@ }, \ }; \ __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, type, \ - r_id, key, ao_flags) + r_id, key, ao_flags, false) #define __DEFINE_CLK_SMD_RPM_AO_PREFIX(_prefix, _name, _active, \ - type, r_id, key, ao_flags) \ + type, r_id, key, ao_flags, \ + _keep_alive) \ static struct clk_smd_rpm clk_smd_rpm_##_prefix##_active = { \ .rpm_res_type = (type), \ .rpm_clk_id = (r_id), \ .active_only = true, \ .rpm_key = (key), \ + .keep_alive = (_keep_alive), \ .peer = &clk_smd_rpm_##_prefix##_name, \ .rate = INT_MAX, \ .hw.init = &(struct clk_init_data){ \ @@ -170,6 +172,7 @@ struct clk_smd_rpm { const bool active_only; bool enabled; bool branch; + bool keep_alive; struct clk_smd_rpm *peer; struct clk_hw hw; unsigned long rate; @@ -198,11 +201,16 @@ static int clk_smd_rpm_handoff(struct clk_smd_rpm *r) .value = cpu_to_le32(r->branch ? 1 : INT_MAX), }; + /* Set up keepalive clocks with a minimum bus rate */ + if (r->keep_alive) + req.value = cpu_to_le32(19200); /* 19.2 MHz */ + ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_ACTIVE_STATE, r->rpm_res_type, r->rpm_clk_id, &req, sizeof(req)); if (ret) return ret; + ret = qcom_rpm_smd_write(r->rpm, QCOM_SMD_RPM_SLEEP_STATE, r->rpm_res_type, r->rpm_clk_id, &req, sizeof(req)); @@ -438,12 +446,29 @@ static int clk_smd_rpm_is_enabled(struct clk_hw *hw) return r->enabled; } +static int clk_smd_rpm_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) +{ + struct clk_smd_rpm *r = to_clk_smd_rpm(hw); + + /* + * RPM resolves the rates internally. All we have to do on the kernel + * side is ensure that we don't accidentally put down the keepalive + * clocks, which could happen if they received a vote below 19.2 MHz. + */ + if (r->keep_alive) + req->rate = max(req->rate, 19200000UL); + + return 0; +} + static const struct clk_ops clk_smd_rpm_ops = { .prepare = clk_smd_rpm_prepare, .unprepare = clk_smd_rpm_unprepare, .set_rate = clk_smd_rpm_set_rate, .round_rate = clk_smd_rpm_round_rate, .recalc_rate = clk_smd_rpm_recalc_rate, + .determine_rate = clk_smd_rpm_determine_rate, .is_enabled = clk_smd_rpm_is_enabled, .is_prepared = clk_smd_rpm_is_enabled, };
Some bus clock should always have a minimum (19.2 MHz) vote cast on them, otherwise the platform will fall apart, hang and reboot. Add support for specifying which clocks should be kept alive and always keep a vote on XO_A to make sure the clock tree doesn't collapse. This removes the need to keep a maximum vote that was previously guaranteed by clk_smd_rpm_handoff. This commit is a combination of existing (not-exactly-upstream) work by Taniya Das, Shawn Guo and myself. Co-developed-by: Shawn Guo <shawn.guo@linaro.org> Co-developed-by: Taniya Das <quic_tdas@quicinc.com> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org> --- drivers/clk/qcom/clk-smd-rpm.c | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)