Re: [PATCH V8 2/7] clk: imx: add scu clock common part

[Stephen Boyd <sboyd@kernel.org>]

Quoting Aisheng DONG (2018-11-21 06:12:23)
> Add scu clock common part which will be used by client clock drivers.
> SCU clocks are totally different from the legacy clocks (No much
> legacy things can be reused). So a new configuration option
> CONFIG_MXC_CLK_SCU is added.

Why are they different? Can you add more details on what has changed
here? I hope to see something like "it's a firmware interface now".

> 
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Sascha Hauer <kernel@pengutronix.de>
> Cc: Fabio Estevam <fabio.estevam@nxp.com>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
> diff --git a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c
> new file mode 100644
> index 0000000..5f9c1f6
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.c
> @@ -0,0 +1,267 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/slab.h>
> +
> +#include "clk-scu.h"
> +
> +struct imx_sc_ipc *ccm_ipc_handle;
> +
> +/*
> + * struct clk_scu - Description of one SCU clock
> + * @hw: the common clk_hw
> + * @rsrc_id: resource ID of this SCU clock
> + * @clk_type: type of this clock resource
> + *
> + * This structure describes one SCU clock

Drop this sentence, it duplicates the first line of this kernel-doc.

> + */
> +struct clk_scu {
> +       struct clk_hw hw;
> +       u32 rsrc_id;
> +       u8 clk_type;
> +};
> +
> +/*
> + * struct imx_sc_msg_req_set_clock_rate - clock set rate protocol
> + * @hdr: SCU protocol header
> + * @rate: rate to set
> + * @resource: clock resource to set rate
> + * @clk: clk type of this resource
> + *
> + * This structure describes the SCU protocol of clock rate set
> + */
> +struct imx_sc_msg_req_set_clock_rate {
> +       struct imx_sc_rpc_msg hdr;
> +       u32 rate;
> +       u16 resource;

These should be __le32 and __le16 presumably because the firmware
interface we're talking to is little endian?

> +       u8 clk;
> +} __packed;
> +
> +struct req_get_clock_rate {
> +       u16 resource;

Same.

> +       u8 clk;
> +} __packed;
> +
> +struct resp_get_clock_rate {
> +       u32 rate;

Same.

> +};
> +
> +/*
> + * struct imx_sc_msg_get_clock_rate - clock get rate protocol
> + * @hdr: SCU protocol header
> + * @req: get rate request protocol
> + * @resp: get rate response protocol
> + *
> + * This structure describes the SCU protocol of clock rate get
> + */
> +struct imx_sc_msg_get_clock_rate {
> +       struct imx_sc_rpc_msg hdr;
> +       union {
> +               struct req_get_clock_rate req;
> +               struct resp_get_clock_rate resp;
> +       } data;
> +} __packed;

Does this need __packed? struct imx_sc_rpc_msg is naturally aligned so I
think we don't need this.

> +
> +/*
> + * struct imx_sc_msg_req_clock_enable - clock gate protocol
> + * @hdr: SCU protocol header
> + * @resource: clock resource to gate
> + * @clk: clk type of this resource
> + * @enable: whether gate off the clock
> + * @autog: HW auto gate enable
> + *
> + * This structure describes the SCU protocol of clock gate
> + */
> +struct imx_sc_msg_req_clock_enable {
> +       struct imx_sc_rpc_msg hdr;
> +       u16 resource;
> +       u8 clk;
> +       u8 enable;
> +       u8 autog;
> +} __packed;
> +
> +static inline struct clk_scu *to_clk_scu(struct clk_hw *hw)
> +{
> +       return container_of(hw, struct clk_scu, hw);
> +}
> +
> +/*
> + * clk_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_scu_recalc_rate(struct clk_hw *hw,
> +                                        unsigned long parent_rate)
> +{
> +       struct clk_scu *clk = to_clk_scu(hw);
> +       struct imx_sc_msg_get_clock_rate msg;

I really hope msg isn't DMAed to the firmware, because the response is
in there!

> +       struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +       int ret;
> +
> +       hdr->ver = IMX_SC_RPC_VERSION;
> +       hdr->svc = IMX_SC_RPC_SVC_PM;
> +       hdr->func = IMX_SC_PM_FUNC_GET_CLOCK_RATE;
> +       hdr->size = 2;
> +
> +       msg.data.req.resource = clk->rsrc_id;
> +       msg.data.req.clk = clk->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;
> +       }
> +
> +       return msg.data.resp.rate;
> +}
> +
> +/*
> + * clk_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
> + *
> + * Returns the current clock rate, or zero in failure.
> + */
> +static long clk_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_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_scu_set_rate(struct clk_hw *hw, unsigned long rate,
> +                           unsigned long parent_rate)
> +{
> +       struct clk_scu *clk = to_clk_scu(hw);
> +       struct imx_sc_msg_req_set_clock_rate msg;
> +       struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +
> +       hdr->ver = IMX_SC_RPC_VERSION;
> +       hdr->svc = IMX_SC_RPC_SVC_PM;
> +       hdr->func = IMX_SC_PM_FUNC_SET_CLOCK_RATE;
> +       hdr->size = 3;
> +
> +       msg.rate = rate;
> +       msg.resource = clk->rsrc_id;
> +       msg.clk = clk->clk_type;
> +
> +       return imx_scu_call_rpc(ccm_ipc_handle, &msg, true);

This function returns normal error codes? Or some firmware special error
code? Please make sure we return normal linux errno values from this
driver.

> +}
> +
> +static int sc_pm_clock_enable(struct imx_sc_ipc *ipc, u32 resource,
> +                             u8 clk, bool enable, bool autog)
> +{
> +       struct imx_sc_msg_req_clock_enable msg;
> +       struct imx_sc_rpc_msg *hdr = &msg.hdr;
> +
> +       hdr->ver = IMX_SC_RPC_VERSION;
> +       hdr->svc = IMX_SC_RPC_SVC_PM;
> +       hdr->func = IMX_SC_PM_FUNC_CLOCK_ENABLE;
> +       hdr->size = 3;
> +
> +       msg.resource = resource;
> +       msg.clk = clk;
> +       msg.enable = enable;
> +       msg.autog = autog;
> +
> +       return imx_scu_call_rpc(ccm_ipc_handle, &msg, true);
> +}
> +
> +/*
> + * clk_scu_prepare - Enable a SCU clock
> + * @hw: clock to enable
> + *
> + * Enable the clock at the DSC slice level
> + */
> +static int clk_scu_prepare(struct clk_hw *hw)
> +{
> +       struct clk_scu *clk = to_clk_scu(hw);
> +
> +       return sc_pm_clock_enable(ccm_ipc_handle, clk->rsrc_id,
> +                                 clk->clk_type, true, false);
> +}
> +
> +/*
> + * clk_scu_unprepare - Disable a SCU clock
> + * @hw: clock to enable
> + *
> + * Disable the clock at the DSC slice level
> + */
> +static void clk_scu_unprepare(struct clk_hw *hw)
> +{
> +       struct clk_scu *clk = to_clk_scu(hw);
> +       int ret;
> +
> +       ret = sc_pm_clock_enable(ccm_ipc_handle, clk->rsrc_id,
> +                                clk->clk_type, false, false);
> +       if (ret)
> +               pr_warn("%s: clk unprepare failed %d\n", clk_hw_get_name(hw),
> +                       ret);
> +}
> +
> +static const struct clk_ops clk_scu_ops = {
> +       .recalc_rate = clk_scu_recalc_rate,
> +       .round_rate = clk_scu_round_rate,
> +       .set_rate = clk_scu_set_rate,
> +       .prepare = clk_scu_prepare,
> +       .unprepare = clk_scu_unprepare,
> +};
> +
> +struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type)
> +{
> +       struct clk_init_data init;
> +       struct clk_scu *clk;
> +       struct clk_hw *hw;
> +       int ret;
> +
> +       clk = kzalloc(sizeof(*clk), GFP_KERNEL);
> +       if (!clk)
> +               return ERR_PTR(-ENOMEM);
> +
> +       clk->rsrc_id = rsrc_id;
> +       clk->clk_type = clk_type;
> +
> +       init.name = name;
> +       init.ops = &clk_scu_ops;
> +       init.num_parents = 0;
> +       /*
> +        * Note on MX8, the clocks are tightly coupled with power domain
> +        * that once the power domain is off, the clock status may be
> +        * lost. So we make it NOCACHE to let user to retrieve the real
> +        * clock status from HW instead of using the possible invalid
> +        * cached rate.
> +        */
> +       init.flags = CLK_GET_RATE_NOCACHE;
> +       clk->hw.init = &init;
> +
> +       hw = &clk->hw;
> +       ret = clk_hw_register(NULL, hw);
> +       if (ret) {
> +               kfree(clk);
> +               hw = ERR_PTR(ret);
> +       }
> +
> +       return hw;
> +}
> diff --git a/drivers/clk/imx/clk-scu.h b/drivers/clk/imx/clk-scu.h
> new file mode 100644
> index 0000000..09f381b
> --- /dev/null
> +++ b/drivers/clk/imx/clk-scu.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright 2018 NXP
> + *   Dong Aisheng <aisheng.dong@nxp.com>
> + */
> +
> +#ifndef __IMX_CLK_SCU_H
> +#define __IMX_CLK_SCU_H
> +
> +#include <linux/firmware/imx/sci.h>
> +
> +extern struct imx_sc_ipc *ccm_ipc_handle;
> +
> +static inline int imx_clk_scu_init(void)

Is this supposed to return EPROBE_DEFER or something? Who calls this?

> +{
> +       return imx_scu_get_handle(&ccm_ipc_handle);
> +}
> +
> +struct clk_hw *imx_clk_scu(const char *name, u32 rsrc_id, u8 clk_type);
> +