Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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!
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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!
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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 7500 Kbytes/s. Regards, Bjorn
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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 7500 Kbytes/s. Regards, Bjorn
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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.
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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.
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
---
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_MASK0x4000
+#define BCM_TCS_CMD_VALID_SHIFT29
+#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, , 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(_clk_lock);
+ ret = clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+
+ return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, false);
+ mutex_unlock(_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(_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(_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=
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
---
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_MASK0x4000
+#define BCM_TCS_CMD_VALID_SHIFT29
+#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, , 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(_clk_lock);
+ ret = clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+
+ return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, false);
+ mutex_unlock(_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(_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(_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=
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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. -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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.
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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.
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
>> ---
>> 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_MASK0x4000
>> +#define BCM_TCS_CMD_VALID_SHIFT29
>> +#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, , 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(_clk_lock);
>> + ret = clk_rpmh_bcm_send_cmd(c, true);
>> + mutex_unlock(_clk_lock);
>> +
>> + return ret;
>> +};
>> +
>> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
>> +{
>> + struct clk_rpmh *c = to_clk_rpmh(hw);
>> +
>> + mutex_lock(_clk_lock);
>> + clk_rpmh_bcm_send_cmd(c, false);
>> + mutex_unlock(_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(_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(_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)
>> +{
>> +
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
>> ---
>> 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_MASK0x4000
>> +#define BCM_TCS_CMD_VALID_SHIFT29
>> +#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, , 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(_clk_lock);
>> + ret = clk_rpmh_bcm_send_cmd(c, true);
>> + mutex_unlock(_clk_lock);
>> +
>> + return ret;
>> +};
>> +
>> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
>> +{
>> + struct clk_rpmh *c = to_clk_rpmh(hw);
>> +
>> + mutex_lock(_clk_lock);
>> + clk_rpmh_bcm_send_cmd(c, false);
>> + mutex_unlock(_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(_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(_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)
>> +{
>> +
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
> ---
> 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_MASK0x4000
> +#define BCM_TCS_CMD_VALID_SHIFT29
> +#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, , 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(_clk_lock);
> + ret = clk_rpmh_bcm_send_cmd(c, true);
> + mutex_unlock(_clk_lock);
> +
> + return ret;
> +};
> +
> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> + mutex_lock(_clk_lock);
> + clk_rpmh_bcm_send_cmd(c, false);
> + mutex_unlock(_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(_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(_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,
Re: [RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
> ---
> 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_MASK0x4000
> +#define BCM_TCS_CMD_VALID_SHIFT29
> +#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, , 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(_clk_lock);
> + ret = clk_rpmh_bcm_send_cmd(c, true);
> + mutex_unlock(_clk_lock);
> +
> + return ret;
> +};
> +
> +static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
> +{
> + struct clk_rpmh *c = to_clk_rpmh(hw);
> +
> + mutex_lock(_clk_lock);
> + clk_rpmh_bcm_send_cmd(c, false);
> + mutex_unlock(_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(_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(_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,
[RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
---
drivers/clk/qcom/clk-rpmh.c | 142 ++
include/dt-bindings/clock/qcom,rpmh.h | 1 +
2 files changed, 143 insertions(+)
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_MASK0x4000
+#define BCM_TCS_CMD_VALID_SHIFT29
+#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 = _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, , 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(_clk_lock);
+ ret = clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+
+ return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, false);
+ mutex_unlock(_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(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+ }
+
+ return 0;
+};
+
+static long clk_rpmh_round_rate(struct
[RFC PATCH] clk: qcom: clk-rpmh: Add IPA clock support
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
---
drivers/clk/qcom/clk-rpmh.c | 142 ++
include/dt-bindings/clock/qcom,rpmh.h | 1 +
2 files changed, 143 insertions(+)
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_MASK0x4000
+#define BCM_TCS_CMD_VALID_SHIFT29
+#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 = _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, , 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(_clk_lock);
+ ret = clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+
+ return ret;
+};
+
+static void clk_rpmh_bcm_unprepare(struct clk_hw *hw)
+{
+ struct clk_rpmh *c = to_clk_rpmh(hw);
+
+ mutex_lock(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, false);
+ mutex_unlock(_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(_clk_lock);
+ clk_rpmh_bcm_send_cmd(c, true);
+ mutex_unlock(_clk_lock);
+ }
+
+ return 0;
+};
+
+static long clk_rpmh_round_rate(struct
