[V4,03/11] clk: imx: scu: add scu clock divider

Commit Message

Dong Aisheng Oct. 14, 2018, 8:07 a.m. UTC

Add scu based clock divider.

Cc: Shawn Guo <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Signed-off-by: Dong Aisheng <[e-mail]>
---
ChangeLog:
v3->v4:
 * scu headfile path update
v2->v3:
 * structures/enums name update with imx_sc prefix
v1->v2:
 * move SCU clock API implementation into driver
---
 drivers/clk/imx/scu/Makefile          |   3 +-
 drivers/clk/imx/scu/clk-divider-scu.c | 176 ++++++++++++++++++++++++++++++++++
 drivers/clk/imx/scu/clk-scu.h         |  18 ++++
 3 files changed, 196 insertions(+), 1 deletion(-)
 create mode 100644 drivers/clk/imx/scu/clk-divider-scu.c

Comments

Stephen Boyd Oct. 16, 2018, 9:26 p.m. UTC | #1

Quoting A.s. Dong (2018-10-14 01:07:49)
> 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 <[e-mail]>
> + *
> + */
> +#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;
> +}

Dong Aisheng Oct. 17, 2018, 8:56 a.m. UTC | #2

> From: Stephen Boyd [mailto:[e-mail]]
> Sent: Wednesday, October 17, 2018 5:26 AM
> Quoting A.s. Dong (2018-10-14 01:07:49)
> > 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 <[e-mail]>
> > + *
> > + */
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/err.h>
> > +#include <linux/io.h>
> > +#include <linux/jiffies.h>
> 
> Is this used?
> 

Good catch, it's introduced by test code and should be removed.

> > +#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.
> 

Yes, better aligned.

> > +       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

Yes

> 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.
> 

Good catch.
Thanks for the professional explanation.

How about do something like below?
struct req_get_clock_rate {
        u16 resource;
        u8 clk;
} __packed;

struct resp_get_clock_rate {
        u32 rate;
} __packed;

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;

> > +
> > +       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?
> 

I guess the explicit case may not need. Will double check.

> > +       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.
> 

Because on MX8, the clocks are tightly couple with power domain that
once the power domain is off, the clock status will be lost.
Making it NOCACHE helps user to retrieve the real clock status from HW
Instead of using the possible invalid cached rate.

Is that reasonable enough to specify it?

Regards
Dong Aisheng

> > +       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;
> > +}

Stephen Boyd Oct. 17, 2018, 3:17 p.m. UTC | #3

Quoting A.s. Dong (2018-10-17 01:56:35)
> > From: Stephen Boyd [mailto:[e-mail]]
> > Sent: Wednesday, October 17, 2018 5:26 AM
> > Quoting A.s. Dong (2018-10-14 01:07:49)
> > > 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 @@
> > > +       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
> 
> Yes
> 
> > 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.
> > 
> 
> Good catch.
> Thanks for the professional explanation.
> 
> How about do something like below?
> struct req_get_clock_rate {
>         u16 resource;
>         u8 clk;
> } __packed;
> 
> struct resp_get_clock_rate {
>         u32 rate;
> } __packed;

This doesn't need __packed because it's a single u32.

> 
> 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;
> 

Yes something like this would be best. And now I wonder if
imx_scu_call_rpc() is doing endianness swapping? Or does it copy data
into these response structures? I'm saying that the u32/16/8 may need to
be __le32/16/8 and then have the proper accessors.

> 
> > > +       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.
> > 
> 
> Because on MX8, the clocks are tightly couple with power domain that
> once the power domain is off, the clock status will be lost.
> Making it NOCACHE helps user to retrieve the real clock status from HW
> Instead of using the possible invalid cached rate.
> 
> Is that reasonable enough to specify it?

Yes, you can add a comment like that. Or the clk rate can be restored
when the power domain is enabled again?

Dong Aisheng Oct. 17, 2018, 3:45 p.m. UTC | #4

> -----Original Message-----
> From: Stephen Boyd [mailto:[e-mail]]
> Sent: Wednesday, October 17, 2018 11:17 PM
> Quoting A.s. Dong (2018-10-17 01:56:35)
> > > From: Stephen Boyd [mailto:[e-mail]]
> > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong
> > > (2018-10-14 01:07:49)
> > > > 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 @@
> > > > +       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
> >
> > Yes
> >
> > > 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.
> > >
> >
> > Good catch.
> > Thanks for the professional explanation.
> >
> > How about do something like below?
> > struct req_get_clock_rate {
> >         u16 resource;
> >         u8 clk;
> > } __packed;
> >
> > struct resp_get_clock_rate {
> >         u32 rate;
> > } __packed;
> 
> This doesn't need __packed because it's a single u32.
> 

It's a safe writing, but yes, can be removed.

> >
> > 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;
> >
> 
> Yes something like this would be best. And now I wonder if
> imx_scu_call_rpc() is doing endianness swapping? Or does it copy data into
> these response structures? I'm saying that the u32/16/8 may need to be
> __le32/16/8 and then have the proper accessors.
> 

No endianness swapping. It's fixed little endian.
SCU protocol isn't aware of these structures. The structures are defined
according to SCU protocol definition to make sure each field position and size
are correct. Then SCU IPC driver just send and receive them sequentially.
Client driver uses the structures to retrieve the responding filed values.

We do this like drivers/firmware/ti_sci.h
Do you think we still need specify __le32/16/8 for this case?

> >
> > > > +       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.
> > >
> >
> > Because on MX8, the clocks are tightly couple with power domain that
> > once the power domain is off, the clock status will be lost.
> > Making it NOCACHE helps user to retrieve the real clock status from HW
> > Instead of using the possible invalid cached rate.
> >
> > Is that reasonable enough to specify it?
> 
> Yes, you can add a comment like that. Or the clk rate can be restored when the
> power domain is enabled again?

No restore when power domain is enabled again. That needs driver to do special
handling. 

Can we add the comment in commit message as there're still many other places
using this?

Regards
Dong Aisheng

Stephen Boyd Oct. 17, 2018, 4:05 p.m. UTC | #5

Quoting A.s. Dong (2018-10-17 08:45:05)
> > -----Original Message-----
> > From: Stephen Boyd [mailto:[e-mail]]
> > Sent: Wednesday, October 17, 2018 11:17 PM
> > Quoting A.s. Dong (2018-10-17 01:56:35)
> > > > From: Stephen Boyd [mailto:[e-mail]]
> > > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong
> > > > (2018-10-14 01:07:49)
> > >
> > > How about do something like below?
> > > struct req_get_clock_rate {
> > >         u16 resource;
> > >         u8 clk;
> > > } __packed;
> > >
> > > struct resp_get_clock_rate {
> > >         u32 rate;
> > > } __packed;
> > 
> > This doesn't need __packed because it's a single u32.
> > 
> 
> It's a safe writing, but yes, can be removed.
> 
> > >
> > > 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;
> > >
> > 
> > Yes something like this would be best. And now I wonder if
> > imx_scu_call_rpc() is doing endianness swapping? Or does it copy data into
> > these response structures? I'm saying that the u32/16/8 may need to be
> > __le32/16/8 and then have the proper accessors.
> > 
> 
> No endianness swapping. It's fixed little endian.
> SCU protocol isn't aware of these structures. The structures are defined
> according to SCU protocol definition to make sure each field position and size
> are correct. Then SCU IPC driver just send and receive them sequentially.
> Client driver uses the structures to retrieve the responding filed values.
> 
> We do this like drivers/firmware/ti_sci.h
> Do you think we still need specify __le32/16/8 for this case?

Probably, because the CPU in linux could be big endian or little endian.
It doesn't hurt to do it right to begin with and then you get the
support for free if it's ever used later on.

> 
> > >
> > > > > +       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.
> > > >
> > >
> > > Because on MX8, the clocks are tightly couple with power domain that
> > > once the power domain is off, the clock status will be lost.
> > > Making it NOCACHE helps user to retrieve the real clock status from HW
> > > Instead of using the possible invalid cached rate.
> > >
> > > Is that reasonable enough to specify it?
> > 
> > Yes, you can add a comment like that. Or the clk rate can be restored when the
> > power domain is enabled again?
> 
> No restore when power domain is enabled again. That needs driver to do special
> handling. 
> 
> Can we add the comment in commit message as there're still many other places
> using this?
> 

Sure. Please also add the comment into the code so we don't have to dig
it out of commit text.

Dong Aisheng Oct. 18, 2018, 2:35 a.m. UTC | #6

> -----Original Message-----
> From: Stephen Boyd [mailto:[e-mail]]
> Sent: Thursday, October 18, 2018 12:06 AM
> Quoting A.s. Dong (2018-10-17 08:45:05)
> > > -----Original Message-----
> > > From: Stephen Boyd [mailto:[e-mail]]
> > > Sent: Wednesday, October 17, 2018 11:17 PM Quoting A.s. Dong
> > > (2018-10-17 01:56:35)
> > > > > From: Stephen Boyd [mailto:[e-mail]]
> > > > > Sent: Wednesday, October 17, 2018 5:26 AM Quoting A.s. Dong
> > > > > (2018-10-14 01:07:49)
> > > >
> > > > How about do something like below?
> > > > struct req_get_clock_rate {
> > > >         u16 resource;
> > > >         u8 clk;
> > > > } __packed;
> > > >
> > > > struct resp_get_clock_rate {
> > > >         u32 rate;
> > > > } __packed;
> > >
> > > This doesn't need __packed because it's a single u32.
> > >
> >
> > It's a safe writing, but yes, can be removed.
> >
> > > >
> > > > 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;
> > > >
> > >
> > > Yes something like this would be best. And now I wonder if
> > > imx_scu_call_rpc() is doing endianness swapping? Or does it copy
> > > data into these response structures? I'm saying that the u32/16/8
> > > may need to be
> > > __le32/16/8 and then have the proper accessors.
> > >
> >
> > No endianness swapping. It's fixed little endian.
> > SCU protocol isn't aware of these structures. The structures are
> > defined according to SCU protocol definition to make sure each field
> > position and size are correct. Then SCU IPC driver just send and receive them
> sequentially.
> > Client driver uses the structures to retrieve the responding filed values.
> >
> > We do this like drivers/firmware/ti_sci.h Do you think we still need
> > specify __le32/16/8 for this case?
> 
> Probably, because the CPU in linux could be big endian or little endian.
> It doesn't hurt to do it right to begin with and then you get the support for free
> if it's ever used later on.
> 

All right for me. Thanks for the suggestion.

Regards
Dong Aisheng

Patch

diff --git a/drivers/clk/imx/scu/Makefile b/drivers/clk/imx/scu/Makefile
index 7dead13..7e360e2 100644
--- a/drivers/clk/imx/scu/Makefile
+++ b/drivers/clk/imx/scu/Makefile
@@ -1,4 +1,5 @@ 
 # SPDX-License-Identifier: GPL-2.0
 
 obj-$(CONFIG_MXC_CLK_SCU) += \
-	clk-scu.o
+	clk-scu.o \
+	clk-divider-scu.o
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 <[e-mail]>
+ *
+ */
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/jiffies.h>
+#include <linux/slab.h>
+
+#include "clk-scu.h"
+
+struct clk_divider_scu {
+	struct clk_hw	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;
+
+	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;
+	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;
+	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;
+}
diff --git a/drivers/clk/imx/scu/clk-scu.h b/drivers/clk/imx/scu/clk-scu.h
index b964f35..e99af63 100644
--- a/drivers/clk/imx/scu/clk-scu.h
+++ b/drivers/clk/imx/scu/clk-scu.h
@@ -7,6 +7,7 @@ 
 #ifndef __IMX_CLK_SCU_H
 #define __IMX_CLK_SCU_H
 
+#include <linux/clk-provider.h>
 #include <linux/firmware/imx/sci.h>
 #include <linux/spinlock.h>
 
@@ -15,4 +16,21 @@  extern struct imx_sc_ipc *ccm_ipc_handle;
 
 int imx_clk_scu_init(void);
 
+struct clk_hw *imx_clk_register_divider_scu(const char *name,
+				const char *parent_name, u32 rsrc_id,
+				u8 clk_type);
+
+static inline struct clk_hw *imx_clk_divider_scu(const char *name,
+				u32 rsrc_id, u8 clk_type)
+{
+	return imx_clk_register_divider_scu(name, NULL, rsrc_id, clk_type);
+}
+
+static inline struct clk_hw *imx_clk_divider2_scu(const char *name,
+				const char *parent_name,
+				u32 rsrc_id, u8 clk_type)
+{
+	return imx_clk_register_divider_scu(name, parent_name, rsrc_id, clk_type);
+}
+
 #endif