[PATCH V4 03/11] clk: imx: scu: add scu clock divider
From: sboyd@kernel.org (Stephen Boyd)
Date: 2018-10-16 21:26:26
Also in:
linux-clk
Quoting A.s. Dong (2018-10-14 01:07:49)
quoted hunk ↗ jump to hunk
diff --git a/drivers/clk/imx/scu/clk-divider-scu.c b/drivers/clk/imx/scu/clk-divider-scu.c new file mode 100644 index 0000000..51cb816 --- /dev/null +++ b/drivers/clk/imx/scu/clk-divider-scu.c@@ -0,0 +1,176 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2016 Freescale Semiconductor, Inc. + * Copyright 2017~2018 NXP + * Dong Aisheng <aisheng.dong@nxp.com> + * + */ +#include <linux/clk.h> +#include <linux/clk-provider.h> +#include <linux/err.h> +#include <linux/io.h> +#include <linux/jiffies.h>
Is this used?
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+
+struct clk_divider_scu {
+ struct clk_hw hw;Nitpick: No idea why this is spaced out from clk_hw.
+ u32 rsrc_id;
+ u8 clk_type;
+};
+
+/* SCU Clock Protocol definitions */
+struct imx_sc_msg_req_set_clock_rate {
+ struct imx_sc_rpc_msg hdr;
+ u32 rate;
+ u16 resource;
+ u8 clk;
+} __packed;
+
+struct imx_sc_msg_req_get_clock_rate {
+ struct imx_sc_rpc_msg hdr;
+ u16 resource;
+ u8 clk;
+} __packed;
+
+struct imx_sc_msg_resp_get_clock_rate {
+ struct imx_sc_rpc_msg hdr;
+ u32 rate;
+} __packed;
+
+
+static inline struct clk_divider_scu *to_clk_divider_scu(struct clk_hw *hw)
+{
+ return container_of(hw, struct clk_divider_scu, hw);
+}
+
+/*
+ * clk_divider_scu_recalc_rate - Get clock rate for a SCU clock
+ * @hw: clock to get rate for
+ * @parent_rate: parent rate provided by common clock framework, not used
+ *
+ * Gets the current clock rate of a SCU clock. Returns the current
+ * clock rate, or zero in failure.
+ */
+static unsigned long clk_divider_scu_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ struct clk_divider_scu *div = to_clk_divider_scu(hw);
+ struct imx_sc_msg_req_get_clock_rate msg;
+ struct imx_sc_msg_resp_get_clock_rate *resp;
+ struct imx_sc_rpc_msg *hdr = &msg.hdr;
+ int ret;
+
+ hdr->ver = IMX_SC_RPC_VERSION;
+ hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+ hdr->func = (uint8_t)IMX_SC_PM_FUNC_GET_CLOCK_RATE;
+ hdr->size = 2;
+
+ msg.resource = div->rsrc_id;
+ msg.clk = div->clk_type;
+
+ ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+ if (ret) {
+ pr_err("%s: failed to get clock rate %d\n",
+ clk_hw_get_name(hw), ret);
+ return 0;
+ }
+
+ resp = (struct imx_sc_msg_resp_get_clock_rate *)&msg;Does the response get written to the same place that the message is written? If so, it would be better to combine the different structs into a union that's always large enough to handle this? For example, it looks like there are only 16 + 8 bytes for the get_clock_rate structure, but then the response is returning the rate in 32 bytes. When we cast that here we're now getting an extra 8 bytes of stack, aren't we? Combining the different structures into one bigger structure would alleviate this and avoid the need for casting.
+
+ return resp->rate;
+}
+
+/*
+ * clk_divider_scu_round_rate - Round clock rate for a SCU clock
+ * @hw: clock to round rate for
+ * @rate: rate to round
+ * @parent_rate: parent rate provided by common clock framework, not used
+ *
+ * Gets the current clock rate of a SCU clock. Returns the current
+ * clock rate, or zero in failure.
+ */
+static long clk_divider_scu_round_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long *parent_rate)
+{
+ /*
+ * Assume we support all the requested rate and let the SCU firmware
+ * to handle the left work
+ */
+ return rate;
+}
+
+/*
+ * clk_divider_scu_set_rate - Set rate for a SCU clock
+ * @hw: clock to change rate for
+ * @rate: target rate for the clock
+ * @parent_rate: rate of the clock parent, not used for SCU clocks
+ *
+ * Sets a clock frequency for a SCU clock. Returns the SCU
+ * protocol status.
+ */
+static int clk_divider_scu_set_rate(struct clk_hw *hw, unsigned long rate,
+ unsigned long parent_rate)
+{
+ struct clk_divider_scu *div = to_clk_divider_scu(hw);
+ struct imx_sc_msg_req_set_clock_rate msg;
+ struct imx_sc_rpc_msg *hdr = &msg.hdr;
+ int ret;
+
+ hdr->ver = IMX_SC_RPC_VERSION;
+ hdr->svc = (uint8_t)IMX_SC_RPC_SVC_PM;
+ hdr->func = (uint8_t)IMX_SC_PM_FUNC_SET_CLOCK_RATE;Are these casts necessary?
+ hdr->size = 3;
+
+ msg.rate = rate;
+ msg.resource = div->rsrc_id;
+ msg.clk = div->clk_type;
+
+ ret = imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
+ if (ret)
+ pr_err("%s: failed to set clock rate %ld : ret %d\n",
+ clk_hw_get_name(hw), rate, ret);
+
+ return 0;
+}
+
+static const struct clk_ops clk_divider_scu_ops = {
+ .recalc_rate = clk_divider_scu_recalc_rate,
+ .round_rate = clk_divider_scu_round_rate,
+ .set_rate = clk_divider_scu_set_rate,
+};
+
+struct clk_hw *imx_clk_register_divider_scu(const char *name,
+ const char *parent_name,
+ u32 rsrc_id,
+ u8 clk_type)
+{
+ struct clk_divider_scu *div;
+ struct clk_init_data init;
+ struct clk_hw *hw;
+ int ret;
+
+ div = kzalloc(sizeof(*div), GFP_KERNEL);
+ if (!div)
+ return ERR_PTR(-ENOMEM);
+
+ div->rsrc_id = rsrc_id;
+ div->clk_type = clk_type;
+
+ init.name = name;
+ init.ops = &clk_divider_scu_ops;
+ init.flags = CLK_GET_RATE_NOCACHE;Why nocache? Please have a good reason and add a comment indicating why.
+ init.parent_names = parent_name ? &parent_name : NULL;
+ init.num_parents = parent_name ? 1 : 0;
+ div->hw.init = &init;
+
+ hw = &div->hw;
+ ret = clk_hw_register(NULL, hw);
+ if (ret) {
+ kfree(div);
+ hw = ERR_PTR(ret);
+ }
+
+ return hw;
+}