| Message ID | 1543895413-1553-2-git-send-email-daidavid1@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | [RFC] clk: qcom: clk-rpmh: Add IPA clock support | expand |
Quoting David Dai (2018-12-03 19:50:13) > Add IPA clock support by extending the current clk rpmh driver to support > clocks that are managed by a different type of RPMh resource known as > Bus Clock Manager(BCM). Yes, but why? Does the IPA driver need to set clk rates and that somehow doesn't work as a bandwidth request? > > Signed-off-by: David Dai <daidavid1@codeaurora.org> > --- > diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c > index 9f4fc77..42e2cd2 100644 > --- a/drivers/clk/qcom/clk-rpmh.c > +++ b/drivers/clk/qcom/clk-rpmh.c > @@ -18,6 +18,32 @@ > #define CLK_RPMH_ARC_EN_OFFSET 0 > #define CLK_RPMH_VRM_EN_OFFSET 4 > > +#define BCM_TCS_CMD_COMMIT_MASK 0x40000000 > +#define BCM_TCS_CMD_VALID_SHIFT 29 > +#define BCM_TCS_CMD_VOTE_MASK 0x3fff > +#define BCM_TCS_CMD_VOTE_SHIFT 0 > + > +#define BCM_TCS_CMD(valid, vote) \ > + (BCM_TCS_CMD_COMMIT_MASK |\ Nitpick: Add space before the \ and align them all up on the right side of the page. > + ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\ > + ((cpu_to_le32(vote) &\ > + BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT)) > + > +/** > + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM) > + * @unit: divisor used to convert Hz value to an RPMh msg > + * @width: multiplier used to convert Hz value to an RPMh msg > + * @vcd: virtual clock domain that this bcm belongs to > + * @reserved: reserved to pad the struct > + */ > + Nitpick: Please remove the newline between comment and struct. > +struct bcm_db { > + u32 unit; > + u16 width; > + u8 vcd; > + u8 reserved; These would need __le32 and __le16 for the byte swap. > +}; > + > /** > * struct clk_rpmh - individual rpmh clock data structure > * @hw: handle between common and hardware-specific interfaces > @@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, > .recalc_rate = clk_rpmh_recalc_rate, > }; > > +static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable) > +{ > + struct tcs_cmd cmd = { 0 }; > + u32 cmd_state; > + int ret; > + > + cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0; > + > + if (c->last_sent_aggr_state == cmd_state) > + return 0; > + > + cmd.addr = c->res_addr; > + cmd.data = BCM_TCS_CMD(enable, cmd_state); > + > + ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1); > + if (ret) { > + dev_err(c->dev, "set active state of %s failed: (%d)\n", > + c->res_name, ret); > + return ret; > + } > + > + c->last_sent_aggr_state = cmd_state; > + > + return 0; > +} > + > +static int clk_rpmh_bcm_prepare(struct clk_hw *hw) > +{ > + struct clk_rpmh *c = to_clk_rpmh(hw); > + int ret = 0; Don't initialize variables and then reassign them right after. > + > + mutex_lock(&rpmh_clk_lock); > + ret = clk_rpmh_bcm_send_cmd(c, true); > + mutex_unlock(&rpmh_clk_lock); > + > + return ret; > +}; > + > +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw) > +{ > + struct clk_rpmh *c = to_clk_rpmh(hw); > + > + mutex_lock(&rpmh_clk_lock); > + clk_rpmh_bcm_send_cmd(c, false); > + mutex_unlock(&rpmh_clk_lock); > +}; > + > +static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long parent_rate) > +{ > + struct clk_rpmh *c = to_clk_rpmh(hw); > + > + c->aggr_state = rate / (c->aux_data.unit * 1000); > + > + if (clk_hw_is_prepared(hw)) { Why do we need to check is_prepared? Add a comment indicating we can't send the request when the clk is disabled? > + mutex_lock(&rpmh_clk_lock); > + clk_rpmh_bcm_send_cmd(c, true); This function is always inside mutex_lock()/unlock() pair, so push the lock into the function? > + mutex_unlock(&rpmh_clk_lock); > + } > + > + return 0; > +}; > + > +static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate, > + unsigned long *parent_rate) > +{ > + return rate; > +} > + > +static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw, > + unsigned long prate) > +{ > + struct clk_rpmh *c = to_clk_rpmh(hw); > + > + return c->aggr_state * c->aux_data.unit * 1000; What is 1000 for? > +} > + > +static const struct clk_ops clk_rpmh_bcm_ops = { > + .prepare = clk_rpmh_bcm_prepare, > + .unprepare = clk_rpmh_bcm_unprepare, > + .set_rate = clk_rpmh_bcm_set_rate, > + .round_rate = clk_rpmh_round_rate, > + .recalc_rate = clk_rpmh_bcm_recalc_rate, > +}; > + > /* Resource name must match resource id present in cmd-db. */ > DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2); > DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2); > @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev) > rpmh_clk->res_name); > return -ENODEV; > } Nitpick: Please add newline here. > + aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name); > + if (aux_data_len == sizeof(struct bcm_db)) { > + ret = cmd_db_read_aux_data(rpmh_clk->res_name, > + (u8 *)&rpmh_clk->aux_data, Is the cast necessary? And I would just pick out the data you need from the aux_data instead of storing it forever (with the padding) in the rpmh_clk structure. > + sizeof(struct bcm_db)); > + if (ret < 0) { > + dev_err(&pdev->dev, "aux data read failure for %s (%d)\n", > + rpmh_clk->res_name, ret); > + return ret; > + } > + rpmh_clk->aux_data.unit = > + le32_to_cpu(rpmh_clk->aux_data.unit); > + rpmh_clk->aux_data.width = > + le16_to_cpu(rpmh_clk->aux_data.width); > + } Nitpick: Newline here too. > rpmh_clk->res_addr += res_addr; > rpmh_clk->dev = &pdev->dev; >
On 12/4/18 1:24 PM, Stephen Boyd wrote: > Quoting David Dai (2018-12-03 19:50:13) >> Add IPA clock support by extending the current clk rpmh driver to support >> clocks that are managed by a different type of RPMh resource known as >> Bus Clock Manager(BCM). > > Yes, but why? Does the IPA driver need to set clk rates and that somehow > doesn't work as a bandwidth request? The IPA core clock is a *clock*, not a bus. Representing it as if it were a bus, abusing the interconnect interface--pretending a bandwidth request is really a clock rate request--is kind of kludgy. I think Bjorn and David (and maybe Georgi? I don't know) decided a long time ago that exposing this as a clock is the right way to do it. I agree with that. -Alex >> Signed-off-by: David Dai <daidavid1@codeaurora.org> >> --- >> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c >> index 9f4fc77..42e2cd2 100644 >> --- a/drivers/clk/qcom/clk-rpmh.c >> +++ b/drivers/clk/qcom/clk-rpmh.c >> @@ -18,6 +18,32 @@ >> #define CLK_RPMH_ARC_EN_OFFSET 0 >> #define CLK_RPMH_VRM_EN_OFFSET 4 >> >> +#define BCM_TCS_CMD_COMMIT_MASK 0x40000000 >> +#define BCM_TCS_CMD_VALID_SHIFT 29 >> +#define BCM_TCS_CMD_VOTE_MASK 0x3fff >> +#define BCM_TCS_CMD_VOTE_SHIFT 0 >> + >> +#define BCM_TCS_CMD(valid, vote) \ >> + (BCM_TCS_CMD_COMMIT_MASK |\ > > Nitpick: Add space before the \ and align them all up on the right side > of the page. > >> + ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\ >> + ((cpu_to_le32(vote) &\ >> + BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT)) >> + >> +/** >> + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM) >> + * @unit: divisor used to convert Hz value to an RPMh msg >> + * @width: multiplier used to convert Hz value to an RPMh msg >> + * @vcd: virtual clock domain that this bcm belongs to >> + * @reserved: reserved to pad the struct >> + */ >> + > > Nitpick: Please remove the newline between comment and struct. > >> +struct bcm_db { >> + u32 unit; >> + u16 width; >> + u8 vcd; >> + u8 reserved; > > These would need __le32 and __le16 for the byte swap. > >> +}; >> + >> /** >> * struct clk_rpmh - individual rpmh clock data structure >> * @hw: handle between common and hardware-specific interfaces >> @@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, >> .recalc_rate = clk_rpmh_recalc_rate, >> }; >> >> +static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable) >> +{ >> + struct tcs_cmd cmd = { 0 }; >> + u32 cmd_state; >> + int ret; >> + >> + cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0; >> + >> + if (c->last_sent_aggr_state == cmd_state) >> + return 0; >> + >> + cmd.addr = c->res_addr; >> + cmd.data = BCM_TCS_CMD(enable, cmd_state); >> + >> + ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1); >> + if (ret) { >> + dev_err(c->dev, "set active state of %s failed: (%d)\n", >> + c->res_name, ret); >> + return ret; >> + } >> + >> + c->last_sent_aggr_state = cmd_state; >> + >> + return 0; >> +} >> + >> +static int clk_rpmh_bcm_prepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + int ret = 0; > > Don't initialize variables and then reassign them right after. > >> + >> + mutex_lock(&rpmh_clk_lock); >> + ret = clk_rpmh_bcm_send_cmd(c, true); >> + mutex_unlock(&rpmh_clk_lock); >> + >> + return ret; >> +}; >> + >> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + mutex_lock(&rpmh_clk_lock); >> + clk_rpmh_bcm_send_cmd(c, false); >> + mutex_unlock(&rpmh_clk_lock); >> +}; >> + >> +static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + c->aggr_state = rate / (c->aux_data.unit * 1000); >> + >> + if (clk_hw_is_prepared(hw)) { > > Why do we need to check is_prepared? Add a comment indicating we can't > send the request when the clk is disabled? > >> + mutex_lock(&rpmh_clk_lock); >> + clk_rpmh_bcm_send_cmd(c, true); > > This function is always inside mutex_lock()/unlock() pair, so push the > lock into the function? > >> + mutex_unlock(&rpmh_clk_lock); >> + } >> + >> + return 0; >> +}; >> + >> +static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long *parent_rate) >> +{ >> + return rate; >> +} >> + >> +static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw, >> + unsigned long prate) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + return c->aggr_state * c->aux_data.unit * 1000; > > What is 1000 for? > >> +} >> + >> +static const struct clk_ops clk_rpmh_bcm_ops = { >> + .prepare = clk_rpmh_bcm_prepare, >> + .unprepare = clk_rpmh_bcm_unprepare, >> + .set_rate = clk_rpmh_bcm_set_rate, >> + .round_rate = clk_rpmh_round_rate, >> + .recalc_rate = clk_rpmh_bcm_recalc_rate, >> +}; >> + >> /* Resource name must match resource id present in cmd-db. */ >> DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2); >> DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2); >> @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev) >> rpmh_clk->res_name); >> return -ENODEV; >> } > > Nitpick: Please add newline here. > >> + aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name); >> + if (aux_data_len == sizeof(struct bcm_db)) { >> + ret = cmd_db_read_aux_data(rpmh_clk->res_name, >> + (u8 *)&rpmh_clk->aux_data, > > Is the cast necessary? And I would just pick out the data you need from > the aux_data instead of storing it forever (with the padding) in the > rpmh_clk structure. > >> + sizeof(struct bcm_db)); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "aux data read failure for %s (%d)\n", >> + rpmh_clk->res_name, ret); >> + return ret; >> + } >> + rpmh_clk->aux_data.unit = >> + le32_to_cpu(rpmh_clk->aux_data.unit); >> + rpmh_clk->aux_data.width = >> + le16_to_cpu(rpmh_clk->aux_data.width); >> + } > > Nitpick: Newline here too. > >> rpmh_clk->res_addr += res_addr; >> rpmh_clk->dev = &pdev->dev; >>
Quoting Alex Elder (2018-12-04 13:41:47) > On 12/4/18 1:24 PM, Stephen Boyd wrote: > > Quoting David Dai (2018-12-03 19:50:13) > >> Add IPA clock support by extending the current clk rpmh driver to support > >> clocks that are managed by a different type of RPMh resource known as > >> Bus Clock Manager(BCM). > > > > Yes, but why? Does the IPA driver need to set clk rates and that somehow > > doesn't work as a bandwidth request? > > The IPA core clock is a *clock*, not a bus. Representing it as if > it were a bus, abusing the interconnect interface--pretending a bandwidth > request is really a clock rate request--is kind of kludgy. I think Bjorn > and David (and maybe Georgi? I don't know) decided a long time ago that > exposing this as a clock is the right way to do it. I agree with that. > But then we translate that clock rate into a bandwidth request to the BCM hardware? Seems really weird because it's doing the opposite of what you say is abusive. What does the IPA driver plan to do with this clk? Calculate a frequency by knowing that it really boils down to some bandwidth that then gets converted back into some clock frequency? Do we have the user somewhere that can be pointed to? Of course, none of these details are in the commit text so it's really hard for me as a bystander to figure this all out. So again, please add these sorts of details to the commit text so we can be "sold" on the idea of the patch instead of stating what the patch does.
On 12/4/2018 2:34 PM, Stephen Boyd wrote: > Quoting Alex Elder (2018-12-04 13:41:47) >> On 12/4/18 1:24 PM, Stephen Boyd wrote: >>> Quoting David Dai (2018-12-03 19:50:13) >>>> Add IPA clock support by extending the current clk rpmh driver to support >>>> clocks that are managed by a different type of RPMh resource known as >>>> Bus Clock Manager(BCM). >>> Yes, but why? Does the IPA driver need to set clk rates and that somehow >>> doesn't work as a bandwidth request? >> The IPA core clock is a *clock*, not a bus. Representing it as if >> it were a bus, abusing the interconnect interface--pretending a bandwidth >> request is really a clock rate request--is kind of kludgy. I think Bjorn >> and David (and maybe Georgi? I don't know) decided a long time ago that >> exposing this as a clock is the right way to do it. I agree with that. >> > But then we translate that clock rate into a bandwidth request to the > BCM hardware? Seems really weird because it's doing the opposite of what > you say is abusive. What does the IPA driver plan to do with this clk? > Calculate a frequency by knowing that it really boils down to some > bandwidth that then gets converted back into some clock frequency? Do we > have the user somewhere that can be pointed to? The clock rate is translated into a unitless threshold value sent as part of the rpmh msg that BCM takes to select a performance. In this case, the unit conversion is based on the unit value read from the aux data which is in Khz. I understand that this wasn't explicitly mentioned anywhere and I'll improve on that next patch. Here's a link to the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220 > > Of course, none of these details are in the commit text so it's really > hard for me as a bystander to figure this all out. So again, please add > these sorts of details to the commit text so we can be "sold" on the > idea of the patch instead of stating what the patch does. Understood, I'll be as detailed and as explicit as I can in the future.
Thanks for the quick feedback! On 12/4/2018 11:24 AM, Stephen Boyd wrote: > Quoting David Dai (2018-12-03 19:50:13) >> Add IPA clock support by extending the current clk rpmh driver to support >> clocks that are managed by a different type of RPMh resource known as >> Bus Clock Manager(BCM). > Yes, but why? Does the IPA driver need to set clk rates and that somehow > doesn't work as a bandwidth request? > >> Signed-off-by: David Dai <daidavid1@codeaurora.org> >> --- >> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c >> index 9f4fc77..42e2cd2 100644 >> --- a/drivers/clk/qcom/clk-rpmh.c >> +++ b/drivers/clk/qcom/clk-rpmh.c >> @@ -18,6 +18,32 @@ >> #define CLK_RPMH_ARC_EN_OFFSET 0 >> #define CLK_RPMH_VRM_EN_OFFSET 4 >> >> +#define BCM_TCS_CMD_COMMIT_MASK 0x40000000 >> +#define BCM_TCS_CMD_VALID_SHIFT 29 >> +#define BCM_TCS_CMD_VOTE_MASK 0x3fff >> +#define BCM_TCS_CMD_VOTE_SHIFT 0 >> + >> +#define BCM_TCS_CMD(valid, vote) \ >> + (BCM_TCS_CMD_COMMIT_MASK |\ > Nitpick: Add space before the \ and align them all up on the right side > of the page. Ok. > >> + ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\ >> + ((cpu_to_le32(vote) &\ >> + BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT)) >> + >> +/** >> + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM) >> + * @unit: divisor used to convert Hz value to an RPMh msg >> + * @width: multiplier used to convert Hz value to an RPMh msg >> + * @vcd: virtual clock domain that this bcm belongs to >> + * @reserved: reserved to pad the struct >> + */ >> + > Nitpick: Please remove the newline between comment and struct. Ok. >> +struct bcm_db { >> + u32 unit; >> + u16 width; >> + u8 vcd; >> + u8 reserved; > These would need __le32 and __le16 for the byte swap. Ok. >> +}; >> + >> /** >> * struct clk_rpmh - individual rpmh clock data structure >> * @hw: handle between common and hardware-specific interfaces >> @@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, >> .recalc_rate = clk_rpmh_recalc_rate, >> }; >> >> +static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable) >> +{ >> + struct tcs_cmd cmd = { 0 }; >> + u32 cmd_state; >> + int ret; >> + >> + cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0; >> + >> + if (c->last_sent_aggr_state == cmd_state) >> + return 0; >> + >> + cmd.addr = c->res_addr; >> + cmd.data = BCM_TCS_CMD(enable, cmd_state); >> + >> + ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1); >> + if (ret) { >> + dev_err(c->dev, "set active state of %s failed: (%d)\n", >> + c->res_name, ret); >> + return ret; >> + } >> + >> + c->last_sent_aggr_state = cmd_state; >> + >> + return 0; >> +} >> + >> +static int clk_rpmh_bcm_prepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + int ret = 0; > Don't initialize variables and then reassign them right after. Ok. >> + >> + mutex_lock(&rpmh_clk_lock); >> + ret = clk_rpmh_bcm_send_cmd(c, true); >> + mutex_unlock(&rpmh_clk_lock); >> + >> + return ret; >> +}; >> + >> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + mutex_lock(&rpmh_clk_lock); >> + clk_rpmh_bcm_send_cmd(c, false); >> + mutex_unlock(&rpmh_clk_lock); >> +}; >> + >> +static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long parent_rate) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + c->aggr_state = rate / (c->aux_data.unit * 1000); >> + >> + if (clk_hw_is_prepared(hw)) { > Why do we need to check is_prepared? Add a comment indicating we can't > send the request when the clk is disabled? Since the BCM only takes in numerical values as part of its msg (as opposed to enable or disable of some sort), this is so that we don't actually enable the clock if it hasn't been prepared yet. Sending a value of zero vs a non-zero would be the equivalent turning the clocks on and off and the behavior I'm trying to emulate here is to only change the rate in the system if the clock is on. > >> + mutex_lock(&rpmh_clk_lock); >> + clk_rpmh_bcm_send_cmd(c, true); > This function is always inside mutex_lock()/unlock() pair, so push the > lock into the function? Makes sense, done. > >> + mutex_unlock(&rpmh_clk_lock); >> + } >> + >> + return 0; >> +}; >> + >> +static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate, >> + unsigned long *parent_rate) >> +{ >> + return rate; >> +} >> + >> +static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw, >> + unsigned long prate) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + return c->aggr_state * c->aux_data.unit * 1000; > What is 1000 for? The unit is in order of Khz, I can do the conversion for KHz to Hz the time when we fetch the aux data. > >> +} >> + >> +static const struct clk_ops clk_rpmh_bcm_ops = { >> + .prepare = clk_rpmh_bcm_prepare, >> + .unprepare = clk_rpmh_bcm_unprepare, >> + .set_rate = clk_rpmh_bcm_set_rate, >> + .round_rate = clk_rpmh_round_rate, >> + .recalc_rate = clk_rpmh_bcm_recalc_rate, >> +}; >> + >> /* Resource name must match resource id present in cmd-db. */ >> DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2); >> DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2); >> @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev) >> rpmh_clk->res_name); >> return -ENODEV; >> } > Nitpick: Please add newline here. Ok. >> + aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name); >> + if (aux_data_len == sizeof(struct bcm_db)) { >> + ret = cmd_db_read_aux_data(rpmh_clk->res_name, >> + (u8 *)&rpmh_clk->aux_data, > Is the cast necessary? And I would just pick out the data you need from > the aux_data instead of storing it forever (with the padding) in the > rpmh_clk structure. Sounds reasonable, the only thing we need from the aux_data is just the unit in this case. >> + sizeof(struct bcm_db)); >> + if (ret < 0) { >> + dev_err(&pdev->dev, "aux data read failure for %s (%d)\n", >> + rpmh_clk->res_name, ret); >> + return ret; >> + } >> + rpmh_clk->aux_data.unit = >> + le32_to_cpu(rpmh_clk->aux_data.unit); >> + rpmh_clk->aux_data.width = >> + le16_to_cpu(rpmh_clk->aux_data.width); >> + } > Nitpick: Newline here too. Ok. >> rpmh_clk->res_addr += res_addr; >> rpmh_clk->dev = &pdev->dev; >>
Quoting David Dai (2018-12-04 17:14:10) > > On 12/4/2018 2:34 PM, Stephen Boyd wrote: > > Quoting Alex Elder (2018-12-04 13:41:47) > >> On 12/4/18 1:24 PM, Stephen Boyd wrote: > >>> Quoting David Dai (2018-12-03 19:50:13) > >>>> Add IPA clock support by extending the current clk rpmh driver to support > >>>> clocks that are managed by a different type of RPMh resource known as > >>>> Bus Clock Manager(BCM). > >>> Yes, but why? Does the IPA driver need to set clk rates and that somehow > >>> doesn't work as a bandwidth request? > >> The IPA core clock is a *clock*, not a bus. Representing it as if > >> it were a bus, abusing the interconnect interface--pretending a bandwidth > >> request is really a clock rate request--is kind of kludgy. I think Bjorn > >> and David (and maybe Georgi? I don't know) decided a long time ago that > >> exposing this as a clock is the right way to do it. I agree with that. > >> > > But then we translate that clock rate into a bandwidth request to the > > BCM hardware? Seems really weird because it's doing the opposite of what > > you say is abusive. What does the IPA driver plan to do with this clk? > > Calculate a frequency by knowing that it really boils down to some > > bandwidth that then gets converted back into some clock frequency? Do we > > have the user somewhere that can be pointed to? > The clock rate is translated into a unitless threshold value sent as > part of the rpmh msg > that BCM takes to select a performance. In this case, the unit > conversion is based on > the unit value read from the aux data which is in Khz. I understand that > this wasn't > explicitly mentioned anywhere and I'll improve on that next patch. How is this different from bus bandwidth requests? In those cases the bandwidth is calculated in bits per second or something like that, and written to the hardware so it can convert that bandwidth into kHz and set a bus clk frequency in the clock controller? So in the IPA case we've skipped the bps to kHz conversion step and gone straight to the clk frequency setting part? Is a BCM able to aggregate units of bandwidth or kHz depending on how it's configured and this BCM is configured for kHz? > Here's a link to > the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220 Thanks for the link. It looks like the IPA driver hard codes a rate of 75 MHz.
On 12/4/2018 11:15 PM, Stephen Boyd wrote: > Quoting David Dai (2018-12-04 17:14:10) >> On 12/4/2018 2:34 PM, Stephen Boyd wrote: >>> Quoting Alex Elder (2018-12-04 13:41:47) >>>> On 12/4/18 1:24 PM, Stephen Boyd wrote: >>>>> Quoting David Dai (2018-12-03 19:50:13) >>>>>> Add IPA clock support by extending the current clk rpmh driver to support >>>>>> clocks that are managed by a different type of RPMh resource known as >>>>>> Bus Clock Manager(BCM). >>>>> Yes, but why? Does the IPA driver need to set clk rates and that somehow >>>>> doesn't work as a bandwidth request? >>>> The IPA core clock is a *clock*, not a bus. Representing it as if >>>> it were a bus, abusing the interconnect interface--pretending a bandwidth >>>> request is really a clock rate request--is kind of kludgy. I think Bjorn >>>> and David (and maybe Georgi? I don't know) decided a long time ago that >>>> exposing this as a clock is the right way to do it. I agree with that. >>>> >>> But then we translate that clock rate into a bandwidth request to the >>> BCM hardware? Seems really weird because it's doing the opposite of what >>> you say is abusive. What does the IPA driver plan to do with this clk? >>> Calculate a frequency by knowing that it really boils down to some >>> bandwidth that then gets converted back into some clock frequency? Do we >>> have the user somewhere that can be pointed to? >> The clock rate is translated into a unitless threshold value sent as >> part of the rpmh msg >> that BCM takes to select a performance. In this case, the unit >> conversion is based on >> the unit value read from the aux data which is in Khz. I understand that >> this wasn't >> explicitly mentioned anywhere and I'll improve on that next patch. > How is this different from bus bandwidth requests? In those cases the > bandwidth is calculated in bits per second or something like that, and > written to the hardware so it can convert that bandwidth into kHz and > set a bus clk frequency in the clock controller? So in the IPA case > we've skipped the bps to kHz conversion step and gone straight to the > clk frequency setting part? Is a BCM able to aggregate units of > bandwidth or kHz depending on how it's configured and this BCM is > configured for kHz? The data written to the hardware is just a 14bit scalar value that it takes to select a performance/frequency from a preset table. It's not really doing any sort of conversion in hardware in this case, instead the value is computed by software based on the aux data given. Think of it as a generic aggregator as opposed to being strictly bandwidth and the aggregator itself does not care what type of value it is(be it Khz or BW/s). > >> Here's a link to >> the IPA driver implementation: https://lkml.org/lkml/2018/11/7/220 > Thanks for the link. It looks like the IPA driver hard codes a rate of > 75 MHz. >
On Tue 04 Dec 23:15 PST 2018, Stephen Boyd wrote: > Quoting David Dai (2018-12-04 17:14:10) > > > > On 12/4/2018 2:34 PM, Stephen Boyd wrote: > > > Quoting Alex Elder (2018-12-04 13:41:47) > > >> On 12/4/18 1:24 PM, Stephen Boyd wrote: > > >>> Quoting David Dai (2018-12-03 19:50:13) > > >>>> Add IPA clock support by extending the current clk rpmh driver to support > > >>>> clocks that are managed by a different type of RPMh resource known as > > >>>> Bus Clock Manager(BCM). > > >>> Yes, but why? Does the IPA driver need to set clk rates and that somehow > > >>> doesn't work as a bandwidth request? > > >> The IPA core clock is a *clock*, not a bus. Representing it as if > > >> it were a bus, abusing the interconnect interface--pretending a bandwidth > > >> request is really a clock rate request--is kind of kludgy. I think Bjorn > > >> and David (and maybe Georgi? I don't know) decided a long time ago that > > >> exposing this as a clock is the right way to do it. I agree with that. > > >> > > > But then we translate that clock rate into a bandwidth request to the > > > BCM hardware? Seems really weird because it's doing the opposite of what > > > you say is abusive. What does the IPA driver plan to do with this clk? > > > Calculate a frequency by knowing that it really boils down to some > > > bandwidth that then gets converted back into some clock frequency? Do we > > > have the user somewhere that can be pointed to? > > The clock rate is translated into a unitless threshold value sent as > > part of the rpmh msg > > that BCM takes to select a performance. In this case, the unit > > conversion is based on > > the unit value read from the aux data which is in Khz. I understand that > > this wasn't > > explicitly mentioned anywhere and I'll improve on that next patch. > > How is this different from bus bandwidth requests? In those cases the > bandwidth is calculated in bits per second or something like that, and > written to the hardware so it can convert that bandwidth into kHz and > set a bus clk frequency in the clock controller? So in the IPA case > we've skipped the bps to kHz conversion step and gone straight to the > clk frequency setting part? Is a BCM able to aggregate units of > bandwidth or kHz depending on how it's configured and this BCM is > configured for kHz? > My objection to the use of msm_bus vs clock framework is not related to how the actual interface of configuring the hardware looks like. It's simply a matter of how this is represented in software, between some provider and the IPA driver. The IPA driver wants the IPA block to tick at 75MHz and I do not think it's appropriate to achieve that by requesting a path between IPA Core and IPA Core of 75000000 Kbytes/s. Regards, Bjorn
Quoting David Dai (2018-12-05 17:24:18) > > > On 12/4/2018 11:15 PM, Stephen Boyd wrote: > > Quoting David Dai (2018-12-04 17:14:10) > >> On 12/4/2018 2:34 PM, Stephen Boyd wrote: > >>> Quoting Alex Elder (2018-12-04 13:41:47) > >>>> > >>> But then we translate that clock rate into a bandwidth request to the > >>> BCM hardware? Seems really weird because it's doing the opposite of what > >>> you say is abusive. What does the IPA driver plan to do with this clk? > >>> Calculate a frequency by knowing that it really boils down to some > >>> bandwidth that then gets converted back into some clock frequency? Do we > >>> have the user somewhere that can be pointed to? > >> The clock rate is translated into a unitless threshold value sent as > >> part of the rpmh msg > >> that BCM takes to select a performance. In this case, the unit > >> conversion is based on > >> the unit value read from the aux data which is in Khz. I understand that > >> this wasn't > >> explicitly mentioned anywhere and I'll improve on that next patch. > > How is this different from bus bandwidth requests? In those cases the > > bandwidth is calculated in bits per second or something like that, and > > written to the hardware so it can convert that bandwidth into kHz and > > set a bus clk frequency in the clock controller? So in the IPA case > > we've skipped the bps to kHz conversion step and gone straight to the > > clk frequency setting part? Is a BCM able to aggregate units of > > bandwidth or kHz depending on how it's configured and this BCM is > > configured for kHz? > > The data written to the hardware is just a 14bit scalar value that it > takes to select a performance/frequency from a preset table. It's not > really doing any sort of conversion in hardware in this case, instead > the value is computed by software based on the aux data given. Think of > it as a generic aggregator as opposed to being strictly bandwidth and > the aggregator itself does not care what type of value it is(be it Khz > or BW/s). > Got it. Thanks!
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c index 9f4fc77..42e2cd2 100644 --- a/drivers/clk/qcom/clk-rpmh.c +++ b/drivers/clk/qcom/clk-rpmh.c @@ -18,6 +18,32 @@ #define CLK_RPMH_ARC_EN_OFFSET 0 #define CLK_RPMH_VRM_EN_OFFSET 4 +#define BCM_TCS_CMD_COMMIT_MASK 0x40000000 +#define BCM_TCS_CMD_VALID_SHIFT 29 +#define BCM_TCS_CMD_VOTE_MASK 0x3fff +#define BCM_TCS_CMD_VOTE_SHIFT 0 + +#define BCM_TCS_CMD(valid, vote) \ + (BCM_TCS_CMD_COMMIT_MASK |\ + ((valid) << BCM_TCS_CMD_VALID_SHIFT) |\ + ((cpu_to_le32(vote) &\ + BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_SHIFT)) + +/** + * struct bcm_db - Auxiliary data pertaining to each Bus Clock Manager(BCM) + * @unit: divisor used to convert Hz value to an RPMh msg + * @width: multiplier used to convert Hz value to an RPMh msg + * @vcd: virtual clock domain that this bcm belongs to + * @reserved: reserved to pad the struct + */ + +struct bcm_db { + u32 unit; + u16 width; + u8 vcd; + u8 reserved; +}; + /** * struct clk_rpmh - individual rpmh clock data structure * @hw: handle between common and hardware-specific interfaces @@ -29,6 +55,7 @@ * @aggr_state: rpmh clock aggregated state * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh * @valid_state_mask: mask to determine the state of the rpmh clock + * @aux_data: data specific to the bcm rpmh resource * @dev: device to which it is attached * @peer: pointer to the clock rpmh sibling */ @@ -42,6 +69,7 @@ struct clk_rpmh { u32 aggr_state; u32 last_sent_aggr_state; u32 valid_state_mask; + struct bcm_db aux_data; struct device *dev; struct clk_rpmh *peer; }; @@ -98,6 +126,17 @@ struct clk_rpmh_desc { __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ CLK_RPMH_VRM_EN_OFFSET, 1, _div) +#define DEFINE_CLK_RPMH_BCM(_platform, _name, _res_name) \ + static struct clk_rpmh _platform##_##_name = { \ + .res_name = _res_name, \ + .valid_state_mask = BIT(RPMH_ACTIVE_ONLY_STATE), \ + .div = 1, \ + .hw.init = &(struct clk_init_data){ \ + .ops = &clk_rpmh_bcm_ops, \ + .name = #_name, \ + }, \ + } + static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw) { return container_of(_hw, struct clk_rpmh, hw); @@ -210,6 +249,91 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, .recalc_rate = clk_rpmh_recalc_rate, }; +static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable) +{ + struct tcs_cmd cmd = { 0 }; + u32 cmd_state; + int ret; + + cmd_state = enable ? (c->aggr_state ? c->aggr_state : 1) : 0; + + if (c->last_sent_aggr_state == cmd_state) + return 0; + + cmd.addr = c->res_addr; + cmd.data = BCM_TCS_CMD(enable, cmd_state); + + ret = rpmh_write_async(c->dev, RPMH_ACTIVE_ONLY_STATE, &cmd, 1); + if (ret) { + dev_err(c->dev, "set active state of %s failed: (%d)\n", + c->res_name, ret); + return ret; + } + + c->last_sent_aggr_state = cmd_state; + + return 0; +} + +static int clk_rpmh_bcm_prepare(struct clk_hw *hw) +{ + struct clk_rpmh *c = to_clk_rpmh(hw); + int ret = 0; + + mutex_lock(&rpmh_clk_lock); + ret = clk_rpmh_bcm_send_cmd(c, true); + mutex_unlock(&rpmh_clk_lock); + + return ret; +}; + +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw) +{ + struct clk_rpmh *c = to_clk_rpmh(hw); + + mutex_lock(&rpmh_clk_lock); + clk_rpmh_bcm_send_cmd(c, false); + mutex_unlock(&rpmh_clk_lock); +}; + +static int clk_rpmh_bcm_set_rate(struct clk_hw *hw, unsigned long rate, + unsigned long parent_rate) +{ + struct clk_rpmh *c = to_clk_rpmh(hw); + + c->aggr_state = rate / (c->aux_data.unit * 1000); + + if (clk_hw_is_prepared(hw)) { + mutex_lock(&rpmh_clk_lock); + clk_rpmh_bcm_send_cmd(c, true); + mutex_unlock(&rpmh_clk_lock); + } + + return 0; +}; + +static long clk_rpmh_round_rate(struct clk_hw *hw, unsigned long rate, + unsigned long *parent_rate) +{ + return rate; +} + +static unsigned long clk_rpmh_bcm_recalc_rate(struct clk_hw *hw, + unsigned long prate) +{ + struct clk_rpmh *c = to_clk_rpmh(hw); + + return c->aggr_state * c->aux_data.unit * 1000; +} + +static const struct clk_ops clk_rpmh_bcm_ops = { + .prepare = clk_rpmh_bcm_prepare, + .unprepare = clk_rpmh_bcm_unprepare, + .set_rate = clk_rpmh_bcm_set_rate, + .round_rate = clk_rpmh_round_rate, + .recalc_rate = clk_rpmh_bcm_recalc_rate, +}; + /* Resource name must match resource id present in cmd-db. */ DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 2); DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 2); @@ -217,6 +341,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1); DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1); DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1); +DEFINE_CLK_RPMH_BCM(sdm845, ipa, "IP0"); static struct clk_hw *sdm845_rpmh_clocks[] = { [RPMH_CXO_CLK] = &sdm845_bi_tcxo.hw, @@ -231,6 +356,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, [RPMH_RF_CLK2_A] = &sdm845_rf_clk2_ao.hw, [RPMH_RF_CLK3] = &sdm845_rf_clk3.hw, [RPMH_RF_CLK3_A] = &sdm845_rf_clk3_ao.hw, + [RPMH_IPA_CLK] = &sdm845_ipa.hw, }; static const struct clk_rpmh_desc clk_rpmh_sdm845 = { @@ -267,6 +393,7 @@ static int clk_rpmh_probe(struct platform_device *pdev) for (i = 0; i < desc->num_clks; i++) { u32 res_addr; + u32 aux_data_len; rpmh_clk = to_clk_rpmh(hw_clks[i]); res_addr = cmd_db_read_addr(rpmh_clk->res_name); @@ -275,6 +402,21 @@ static int clk_rpmh_probe(struct platform_device *pdev) rpmh_clk->res_name); return -ENODEV; } + aux_data_len = cmd_db_read_aux_data_len(rpmh_clk->res_name); + if (aux_data_len == sizeof(struct bcm_db)) { + ret = cmd_db_read_aux_data(rpmh_clk->res_name, + (u8 *)&rpmh_clk->aux_data, + sizeof(struct bcm_db)); + if (ret < 0) { + dev_err(&pdev->dev, "aux data read failure for %s (%d)\n", + rpmh_clk->res_name, ret); + return ret; + } + rpmh_clk->aux_data.unit = + le32_to_cpu(rpmh_clk->aux_data.unit); + rpmh_clk->aux_data.width = + le16_to_cpu(rpmh_clk->aux_data.width); + } rpmh_clk->res_addr += res_addr; rpmh_clk->dev = &pdev->dev; diff --git a/include/dt-bindings/clock/qcom,rpmh.h b/include/dt-bindings/clock/qcom,rpmh.h index f48fbd6..edcab3f 100644 --- a/include/dt-bindings/clock/qcom,rpmh.h +++ b/include/dt-bindings/clock/qcom,rpmh.h @@ -18,5 +18,6 @@ #define RPMH_RF_CLK2_A 9 #define RPMH_RF_CLK3 10 #define RPMH_RF_CLK3_A 11 +#define RPMH_IPA_CLK 12 #endif
Add IPA clock support by extending the current clk rpmh driver to support clocks that are managed by a different type of RPMh resource known as Bus Clock Manager(BCM). Signed-off-by: David Dai <daidavid1@codeaurora.org> --- drivers/clk/qcom/clk-rpmh.c | 142 ++++++++++++++++++++++++++++++++++ include/dt-bindings/clock/qcom,rpmh.h | 1 + 2 files changed, 143 insertions(+)