Messages in this thread | | | Subject | Re: [v5 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver | From | Taniya Das <> | Date | Wed, 2 May 2018 15:52:12 +0530 |
| |
Thanks Stephen for the comments.
On 5/2/2018 2:57 AM, Stephen Boyd wrote: > Quoting Taniya Das (2018-05-01 01:41:33) >> Add the RPMh clock driver to control the RPMh managed clock resources on >> some of the Qualcomm Technologies, Inc. SoCs. >> >> Signed-off-by: Amit Nischal <anischal@codeaurora.org> >> Signed-off-by: Taniya Das <tdas@codeaurora.org> > > Drop Amit's signoff, or make it "Co-authored-by" please. First signoff > should be the author and there should be a "From:" line either from the > email header of the sender or explicitly here before the commit text > body to indicate the authorship. > > Overall this is looking close. Maybe one more round. > >> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c >> new file mode 100644 >> index 0000000..9cb7aa4 >> --- /dev/null >> +++ b/drivers/clk/qcom/clk-rpmh.c >> @@ -0,0 +1,387 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2018, The Linux Foundation. All rights reserved. >> + */ >> + >> +#include <linux/clk-provider.h> >> +#include <linux/err.h> >> +#include <linux/kernel.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> >> +#include <linux/platform_device.h> >> +#include <soc/qcom/cmd-db.h> >> +#include <soc/qcom/rpmh.h> >> + >> +#include <dt-bindings/clock/qcom,rpmh.h> >> + >> +#define CLK_RPMH_ARC_EN_OFFSET 0 >> +#define CLK_RPMH_VRM_EN_OFFSET 4 >> + >> +/** >> + * struct clk_rpmh - individual rpmh clock data structure >> + * @hw: handle between common and hardware-specific interfaces >> + * @res_name: resource name for the rpmh clock >> + * @div: clock divider to compute the clock rate >> + * @res_addr: base address of the rpmh resource within the RPMh >> + * @res_on_val: rpmh clock enable value >> + * @res_off_val: rpmh clock disable value > > It's still unused. >
It is unused as the currently the resource off value is '0'. I would remove, will add it back if any different value is required.
>> + * @state: rpmh clock requested state >> + * @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 >> + * @rpmh_client: handle used for rpmh communications >> + * @dev: device to which it is attached >> + * @peer: pointer to the clock rpmh sibling >> + */ >> +struct clk_rpmh { >> + struct clk_hw hw; >> + const char *res_name; >> + u8 div; > > Weird space here. > This is no longer required, removed.
>> + u32 res_addr; >> + u32 res_on_val; >> + u32 res_off_val; >> + u32 state; >> + u32 aggr_state; >> + u32 last_sent_aggr_state; >> + u32 valid_state_mask; >> + struct rpmh_client *rpmh_client; >> + struct device *dev; >> + struct clk_rpmh *peer; >> +}; >> + >> +struct clk_rpmh_desc { >> + struct clk_hw **clks; >> + size_t num_clks; >> +}; >> + >> +static DEFINE_MUTEX(rpmh_clk_lock); >> + >> +#define __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ >> + _res_en_offset, _res_on, _res_off) \ >> + static struct clk_rpmh _platform##_##_name_active; \ >> + static struct clk_rpmh _platform##_##_name = { \ >> + .res_name = _res_name, \ >> + .res_addr = _res_en_offset, \ >> + .res_on_val = _res_on, \ >> + .res_off_val = _res_off, \ >> + .peer = &_platform##_##_name_active, \ >> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \ >> + BIT(RPMH_ACTIVE_ONLY_STATE) | \ >> + BIT(RPMH_SLEEP_STATE)), \ >> + .hw.init = &(struct clk_init_data){ \ >> + .ops = &clk_rpmh_ops, \ >> + .name = #_name, \ >> + .parent_names = (const char *[]){ "xo_board" }, \ > > This would be different for the xo_board and the xo_board / 2 clk names. >
Would add parents "xo_board" & "xo_board_div".
>> + .num_parents = 1, \ >> + }, \ >> + }; \ >> + static struct clk_rpmh _platform##_##_name_active = { \ >> + .res_name = _res_name, \ >> + .res_addr = _res_en_offset, \ >> + .res_on_val = _res_on, \ >> + .res_off_val = _res_off, \ >> + .peer = &_platform##_##_name, \ >> + .valid_state_mask = (BIT(RPMH_WAKE_ONLY_STATE) | \ >> + BIT(RPMH_ACTIVE_ONLY_STATE)), \ >> + .hw.init = &(struct clk_init_data){ \ >> + .ops = &clk_rpmh_ops, \ >> + .name = #_name_active, \ >> + .parent_names = (const char *[]){ "xo_board" }, \ >> + .num_parents = 1, \ >> + }, \ >> + } >> + >> +#define DEFINE_CLK_RPMH_ARC(_platform, _name, _name_active, _res_name, \ >> + _res_on, _res_off) \ >> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ >> + CLK_RPMH_ARC_EN_OFFSET, _res_on, _res_off) >> + >> +#define DEFINE_CLK_RPMH_VRM(_platform, _name, _name_active, _res_name) \ >> + __DEFINE_CLK_RPMH(_platform, _name, _name_active, _res_name, \ >> + CLK_RPMH_VRM_EN_OFFSET, true, false) > > s/true, false/1, 0/ ? >
Would take care of this too.
>> + >> +static inline struct clk_rpmh *to_clk_rpmh(struct clk_hw *_hw) >> +{ >> + return container_of(_hw, struct clk_rpmh, hw); >> +} >> + >> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state) >> +{ >> + return (c->last_sent_aggr_state & BIT(state)) >> + != (c->aggr_state & BIT(state)); >> +} >> + >> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c) >> +{ >> + struct tcs_cmd cmd = { 0 }; >> + u32 cmd_state, on_val; >> + enum rpmh_state state = RPMH_SLEEP_STATE; >> + int ret; >> + >> + cmd.addr = c->res_addr; >> + cmd_state = c->aggr_state; >> + on_val = c->res_on_val; >> + >> + for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) { >> + if (has_state_changed(c, state)) { >> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0; > > Make this into an "if" please. Default is already 0 so just set bit if > cmd_state & BIT(state). >
Would take care of it.
>> + ret = rpmh_write_async(c->rpmh_client, state, >> + &cmd, 1); >> + if (ret) { >> + dev_err(c->dev, "set %s state of %s failed: (%d)\n", >> + !state ? "sleep" : >> + state == RPMH_WAKE_ONLY_STATE ? >> + "wake" : "active", c->res_name, ret); >> + return ret; >> + } >> + } >> + } >> + >> + c->last_sent_aggr_state = c->aggr_state; >> + c->peer->last_sent_aggr_state = c->last_sent_aggr_state; >> + >> + return 0; >> +} >> + >> +/* >> + * Update state and aggregate state values based on enable value. >> + */ >> +static int clk_rpmh_aggregate_state_send_command(struct clk_rpmh *c, >> + bool enable) >> +{ >> + int ret; >> + >> + c->state = enable ? c->valid_state_mask : 0; >> + c->aggr_state = c->state | c->peer->state; >> + c->peer->aggr_state = c->aggr_state; >> + >> + ret = clk_rpmh_send_aggregate_command(c); >> + if (ret && enable) { >> + c->state = 0; > > Why don't we print a WARN if it fails to enable? > I would add a WARN for enable failure too.
>> + } else if (ret) { >> + c->state = c->valid_state_mask; >> + WARN(1, "clk: %s failed to disable\n", c->res_name); >> + } >> + >> + return ret; >> +} >> + >> +static int clk_rpmh_prepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + int ret = 0; >> + >> + mutex_lock(&rpmh_clk_lock); >> + >> + if (!c->state) > > Can we push this into clk_rpmh_aggregate_state_send_command()? Something > like: > > /* Nothing to do if already off or on */ > if (enable == c->state) > return 0; >
Thanks, would add these changes.
>> + ret = clk_rpmh_aggregate_state_send_command(c, true); >> + >> + mutex_unlock(&rpmh_clk_lock); >> + return ret; >> +}; >> + >> +static void clk_rpmh_unprepare(struct clk_hw *hw) >> +{ >> + struct clk_rpmh *c = to_clk_rpmh(hw); >> + >> + mutex_lock(&rpmh_clk_lock); >> + >> + if (c->state) >> + clk_rpmh_aggregate_state_send_command(c, false); >> + >> + mutex_unlock(&rpmh_clk_lock); >> +}; >> + >> +static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw, >> + unsigned long prate) >> +{ >> + struct clk_rpmh *r = to_clk_rpmh(hw); >> + unsigned long rate; >> + >> + /* >> + * RPMh clocks have a fixed rate. >> + */ >> + rate = prate; >> + do_div(rate, r->div); >> + >> + return rate; >> +} > > You shouldn't need any recalc rate in this driver. >
Yes, these would be removed.
>> + >> +static int clk_rpmh_probe(struct platform_device *pdev) >> +{ >> + struct clk *clk; >> + struct clk_hw_onecell_data *hw_data; >> + struct clk_hw **hw_clks; >> + struct clk_rpmh *rpmh_clk; >> + const struct clk_rpmh_desc *desc; >> + struct rpmh_client *rpmh_client; >> + struct device *dev; >> + size_t num_clks, i; >> + int ret; >> + >> + dev = &pdev->dev; > > Just do this when dev is defined please. > >> + >> + ret = cmd_db_ready(); >> + if (ret) { >> + if (ret != -EPROBE_DEFER) >> + dev_err(dev, "Command DB not available (%d)\n", ret); >> + return ret; >> + } >> + >> + rpmh_client = rpmh_get_client(pdev); >> + if (IS_ERR(rpmh_client)) { >> + ret = PTR_ERR(rpmh_client); >> + dev_err(dev, "Failed to request RPMh client, ret=%d\n", ret); >> + return ret; >> + } >> + >> + desc = of_device_get_match_data(&pdev->dev); >> + hw_clks = desc->clks; >> + num_clks = desc->num_clks; >> + >> + hw_data = devm_kzalloc(dev, >> + sizeof(*hw_data) + num_clks * sizeof(struct clk_hw *), >> + GFP_KERNEL); >> + if (!hw_data) >> + return -ENOMEM; >> + >> + hw_data->num = num_clks; >> + >> + for (i = 0; i < num_clks; i++) { >> + const char *clk_name; >> + struct property *prop; >> + const __be32 *p; >> + u32 res_addr; >> + int div, j = 0; >> + >> + rpmh_clk = to_clk_rpmh(hw_clks[i]); >> + res_addr = cmd_db_read_addr(rpmh_clk->res_name); >> + if (!res_addr) { >> + dev_err(dev, "missing RPMh resource address for %s\n", >> + rpmh_clk->res_name); >> + ret = -ENODEV; >> + goto err; >> + } >> + rpmh_clk->res_addr += res_addr; > > Thanks! > >> + >> + rpmh_clk->rpmh_client = rpmh_client; >> + >> + /* default div for all clocks */ >> + if (!rpmh_clk->div) >> + rpmh_clk->div = 1; >> + >> + of_property_for_each_u32(pdev->dev.of_node, "assigned-clk-divs", >> + prop, p, div) { >> + of_property_read_string_index(pdev->dev.of_node, >> + "clk-output-names", j, &clk_name); >> + if (!strcmp(clk_name, hw_clks[i]->init->name)) { >> + rpmh_clk->div = div; >> + rpmh_clk->peer->div = div; >> + } >> + j++; >> + } >> + >> + clk = devm_clk_register(&pdev->dev, hw_clks[i]); >> + if (IS_ERR(clk)) { >> + ret = PTR_ERR(clk); >> + dev_err(dev, "failed to register %s\n", >> + hw_clks[i]->init->name); >> + goto err; >> + } >> + >> + rpmh_clk->dev = &pdev->dev; >> + } >> + >> + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, > > Why not roll your own "getter" function and then just index directly > into your array of rpmh clks for sdm845? Then we don't have to copy > things from one place to another. >
Added a new function.
>> + hw_data); >> + if (ret) { >> + dev_err(dev, "Failed to add clock provider\n"); >> + goto err; >> + } >> + >> + dev_dbg(dev, "Registered RPMh clocks\n"); >> + >> + return 0; >> +err: >> + if (rpmh_client) >> + rpmh_release(rpmh_client); >> + >> + return ret; >> +} >> + > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation.
--
| |