Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-24 Thread Stephen Boyd 
Quoting Taniya Das (2018-04-23 09:50:22)
> On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-04-13 19:36:41)
> 
> >> +struct clk_rpmh {
> >> +   struct clk_hw hw;
> >> +   const char *res_name;
> >> +   u32 res_addr;
> >> +   u32 res_en_offset;
> > 
> > Why do we store both res_addr and res_en_offset? Can't we just store
> > res_en_offset and then use that all the time? I don't see a user of
> > res_addr anywhere.
> > 
> 
> The res_addr would be the address for the resource returned by the 
> cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN 
> or VRM_EN.

Yes. But why can't we store the combination of the two?

> 
> >> +   u32 res_on_val;
> >> +   u32 res_off_val;
> > 
> > Is this used?
> 
> Yes the above are used.

I just meant res_off_val. Which looks unused.

> > 
> >> +   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;
> >> +   unsigned long rate;
> >> +};
> > 
> > Can you add some kernel-doc on these structure members?
> > 
> Sure will add the same.

Great! Hopefully that clarifies things.

> >> +   /*
> >> +* RPMh clocks have a fixed rate. Return static rate set
> >> +* at init time.
> >> +*/
> >> +   return r->rate;
> > 
> > The rate should come from the parent. In the case of tcxo it would be
> > board_xo clk rate (or maybe some fixed div-2 on the board XO that's also
> > defined in DT because the board_xo seems to be two times 19.2 MHz?).
> > 
> 
> There would not be any parent for the RPMH clock, they would be the 
> parent for other clocks.
> 
> The TCXO is 19.2MHz and once we have the RPMH clocks, we would remove 
> the DT reference for board_xo.

No that's wrong. There is a parent of the RPMh clks, and that's the
board XO clk in the DT file. We will never remove the board clks.

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-24 Thread Stephen Boyd 
Quoting Taniya Das (2018-04-23 09:50:22)
> On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-04-13 19:36:41)
> 
> >> +struct clk_rpmh {
> >> +   struct clk_hw hw;
> >> +   const char *res_name;
> >> +   u32 res_addr;
> >> +   u32 res_en_offset;
> > 
> > Why do we store both res_addr and res_en_offset? Can't we just store
> > res_en_offset and then use that all the time? I don't see a user of
> > res_addr anywhere.
> > 
> 
> The res_addr would be the address for the resource returned by the 
> cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN 
> or VRM_EN.

Yes. But why can't we store the combination of the two?

> 
> >> +   u32 res_on_val;
> >> +   u32 res_off_val;
> > 
> > Is this used?
> 
> Yes the above are used.

I just meant res_off_val. Which looks unused.

> > 
> >> +   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;
> >> +   unsigned long rate;
> >> +};
> > 
> > Can you add some kernel-doc on these structure members?
> > 
> Sure will add the same.

Great! Hopefully that clarifies things.

> >> +   /*
> >> +* RPMh clocks have a fixed rate. Return static rate set
> >> +* at init time.
> >> +*/
> >> +   return r->rate;
> > 
> > The rate should come from the parent. In the case of tcxo it would be
> > board_xo clk rate (or maybe some fixed div-2 on the board XO that's also
> > defined in DT because the board_xo seems to be two times 19.2 MHz?).
> > 
> 
> There would not be any parent for the RPMH clock, they would be the 
> parent for other clocks.
> 
> The TCXO is 19.2MHz and once we have the RPMH clocks, we would remove 
> the DT reference for board_xo.

No that's wrong. There is a parent of the RPMh clks, and that's the
board XO clk in the DT file. We will never remove the board clks.

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-23 Thread Taniya Das 

Thanks Stephen for the review comments.

On 4/16/2018 11:08 PM, Stephen Boyd wrote:

Quoting Taniya Das (2018-04-13 19:36:41)

Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by: David Collins 
Signed-off-by: Amit Nischal 


Your signoff chain is very confused. The first signoff should match the
"From:" header but that doesn't seem to be the case here. And the sender
should be the last in the chain.


---
  drivers/clk/qcom/Kconfig|   9 ++
  drivers/clk/qcom/Makefile   |   1 +
  drivers/clk/qcom/clk-rpmh.c | 367 
  3 files changed, 377 insertions(+)
  create mode 100644 drivers/clk/qcom/clk-rpmh.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..63c3372 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
   i2c, USB, SD/eMMC, etc. Select this for the root clock
   of ipq8074.

+config QCOM_CLK_RPMH
+   tristate "RPMh Clock Driver"
+   depends on COMMON_CLK_QCOM && QCOM_RPMH
+   help
+RPMh manages shared resources on some Qualcomm Technologies, Inc.
+SoCs. It accepts requests from other hardware subsystems via RSC.
+Say Y if you want to support the clocks exposed by RPMh on
+platforms such as sdm845.


Capitalize sdm?


+




Would use SDM845.


Can this Kconfig be put into alphabetical order in this file?



Would place it with the QCOM_* configs in this file.


  config MSM_GCC_8660
 tristate "MSM8660 Global Clock Controller"
 depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b2b5babf 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 000..fa61284
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 


Is this include used?



Would remove this include.


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE))


Is there a reason these aren't inlined into the one place they're used?



Would clean the above.


+struct clk_rpmh {
+   struct clk_hw hw;
+   const char *res_name;
+   u32 res_addr;
+   u32 res_en_offset;


Why do we store both res_addr and res_en_offset? Can't we just store
res_en_offset and then use that all the time? I don't see a user of
res_addr anywhere.



The res_addr would be the address for the resource returned by the 
cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN 
or VRM_EN.



+   u32 res_on_val;
+   u32 res_off_val;


Is this used?


Yes the above are used.



+   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;
+   unsigned long rate;
+};


Can you add some kernel-doc on these structure members?


Sure will add the same.

+
+struct rpmh_cc {
+   struct clk_onecell_data data;
+   struct clk *clks[];
+};


Hopefully this structure isn't needed.



Yes, would remove the above.

+
+struct clk_rpmh_desc {
+   struct clk_hw **clks;
+   size_t num_clks;
+};
+

[...]

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


Too many parenthesis here.



Would clean it up.

+}
+
+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 = 0;
+
+   cmd.addr = c->res_addr + c->res_en_offset;
+   cmd_state = c->aggr_state;
+   on_val = c->res_on_val;
+
+   for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
+   if

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-23 Thread Taniya Das 

Thanks Stephen for the review comments.

On 4/16/2018 11:08 PM, Stephen Boyd wrote:

Quoting Taniya Das (2018-04-13 19:36:41)

Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by: David Collins 
Signed-off-by: Amit Nischal 


Your signoff chain is very confused. The first signoff should match the
"From:" header but that doesn't seem to be the case here. And the sender
should be the last in the chain.


---
  drivers/clk/qcom/Kconfig|   9 ++
  drivers/clk/qcom/Makefile   |   1 +
  drivers/clk/qcom/clk-rpmh.c | 367 
  3 files changed, 377 insertions(+)
  create mode 100644 drivers/clk/qcom/clk-rpmh.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..63c3372 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
   i2c, USB, SD/eMMC, etc. Select this for the root clock
   of ipq8074.

+config QCOM_CLK_RPMH
+   tristate "RPMh Clock Driver"
+   depends on COMMON_CLK_QCOM && QCOM_RPMH
+   help
+RPMh manages shared resources on some Qualcomm Technologies, Inc.
+SoCs. It accepts requests from other hardware subsystems via RSC.
+Say Y if you want to support the clocks exposed by RPMh on
+platforms such as sdm845.


Capitalize sdm?


+




Would use SDM845.


Can this Kconfig be put into alphabetical order in this file?



Would place it with the QCOM_* configs in this file.


  config MSM_GCC_8660
 tristate "MSM8660 Global Clock Controller"
 depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b2b5babf 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 000..fa61284
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 


Is this include used?



Would remove this include.


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE))


Is there a reason these aren't inlined into the one place they're used?



Would clean the above.


+struct clk_rpmh {
+   struct clk_hw hw;
+   const char *res_name;
+   u32 res_addr;
+   u32 res_en_offset;


Why do we store both res_addr and res_en_offset? Can't we just store
res_en_offset and then use that all the time? I don't see a user of
res_addr anywhere.



The res_addr would be the address for the resource returned by the 
cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN 
or VRM_EN.



+   u32 res_on_val;
+   u32 res_off_val;


Is this used?


Yes the above are used.



+   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;
+   unsigned long rate;
+};


Can you add some kernel-doc on these structure members?


Sure will add the same.

+
+struct rpmh_cc {
+   struct clk_onecell_data data;
+   struct clk *clks[];
+};


Hopefully this structure isn't needed.



Yes, would remove the above.

+
+struct clk_rpmh_desc {
+   struct clk_hw **clks;
+   size_t num_clks;
+};
+

[...]

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


Too many parenthesis here.



Would clean it up.

+}
+
+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 = 0;
+
+   cmd.addr = c->res_addr + c->res_en_offset;
+   cmd_state = c->aggr_state;
+   on_val = c->res_on_val;
+
+   for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
+   if (has_state_changed(c, state)) {
+

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-16 Thread David Collins 
Hello Taniya,

On 04/16/2018 10:38 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-04-13 19:36:41)
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins 
>> Signed-off-by: Amit Nischal 
> 
> Your signoff chain is very confused. The first signoff should match the
> "From:" header but that doesn't seem to be the case here. And the sender
> should be the last in the chain.

It looks like the provenance of this driver go muddled during downstream
propagation between branches.  The original downstream version of the
clk-rpmh driver was written by Osvaldo Banuelos [1].  I think that the
best course of action here would be to remove my Signed-off-by, add
Osvaldo's, and add yours.

Thanks,
David

[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?id=11d3623c37afb945ce36755501812a44efeb6cd9

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-16 Thread David Collins 
Hello Taniya,

On 04/16/2018 10:38 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-04-13 19:36:41)
>> Add the RPMh clock driver to control the RPMh managed clock resources on
>> some of the Qualcomm Technologies, Inc. SoCs.
>>
>> Signed-off-by: David Collins 
>> Signed-off-by: Amit Nischal 
> 
> Your signoff chain is very confused. The first signoff should match the
> "From:" header but that doesn't seem to be the case here. And the sender
> should be the last in the chain.

It looks like the provenance of this driver go muddled during downstream
propagation between branches.  The original downstream version of the
clk-rpmh driver was written by Osvaldo Banuelos [1].  I think that the
best course of action here would be to remove my Signed-off-by, add
Osvaldo's, and add yours.

Thanks,
David

[1]:
https://source.codeaurora.org/quic/la/kernel/msm-4.9/commit/?id=11d3623c37afb945ce36755501812a44efeb6cd9

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-16 Thread Stephen Boyd 
Quoting Taniya Das (2018-04-13 19:36:41)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> 
> Signed-off-by: David Collins 
> Signed-off-by: Amit Nischal 

Your signoff chain is very confused. The first signoff should match the
"From:" header but that doesn't seem to be the case here. And the sender
should be the last in the chain.

> ---
>  drivers/clk/qcom/Kconfig|   9 ++
>  drivers/clk/qcom/Makefile   |   1 +
>  drivers/clk/qcom/clk-rpmh.c | 367 
> 
>  3 files changed, 377 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-rpmh.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
>   i2c, USB, SD/eMMC, etc. Select this for the root clock
>   of ipq8074.
> 
> +config QCOM_CLK_RPMH
> +   tristate "RPMh Clock Driver"
> +   depends on COMMON_CLK_QCOM && QCOM_RPMH
> +   help
> +RPMh manages shared resources on some Qualcomm Technologies, Inc.
> +SoCs. It accepts requests from other hardware subsystems via RSC.
> +Say Y if you want to support the clocks exposed by RPMh on
> +platforms such as sdm845.

Capitalize sdm?

> +

Can this Kconfig be put into alphabetical order in this file?

>  config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
>  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
>  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
> +obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
>  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 000..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include 

Is this include used?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> +BIT(RPMH_ACTIVE_ONLY_STATE))
> +#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE))

Is there a reason these aren't inlined into the one place they're used?

> +struct clk_rpmh {
> +   struct clk_hw hw;
> +   const char *res_name;
> +   u32 res_addr;
> +   u32 res_en_offset;

Why do we store both res_addr and res_en_offset? Can't we just store
res_en_offset and then use that all the time? I don't see a user of
res_addr anywhere.

> +   u32 res_on_val;
> +   u32 res_off_val;

Is this used?

> +   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;
> +   unsigned long rate;
> +};

Can you add some kernel-doc on these structure members?

> +
> +struct rpmh_cc {
> +   struct clk_onecell_data data;
> +   struct clk *clks[];
> +};

Hopefully this structure isn't needed.

> +
> +struct clk_rpmh_desc {
> +   struct clk_hw **clks;
> +   size_t num_clks;
> +};
> +
[...]
> +
> +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)));

Too many parenthesis here.

> +}
> +
> +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 = 0;
> +
> +   cmd.addr = c->res_addr + c->res_en_offset;
> +   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;
> +   ret = rpmh_write_async(c->rpmh_client, state,
> +   , 1);
> +   if (ret) {
> +

Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-16 Thread Stephen Boyd 
Quoting Taniya Das (2018-04-13 19:36:41)
> Add the RPMh clock driver to control the RPMh managed clock resources on
> some of the Qualcomm Technologies, Inc. SoCs.
> 
> Signed-off-by: David Collins 
> Signed-off-by: Amit Nischal 

Your signoff chain is very confused. The first signoff should match the
"From:" header but that doesn't seem to be the case here. And the sender
should be the last in the chain.

> ---
>  drivers/clk/qcom/Kconfig|   9 ++
>  drivers/clk/qcom/Makefile   |   1 +
>  drivers/clk/qcom/clk-rpmh.c | 367 
> 
>  3 files changed, 377 insertions(+)
>  create mode 100644 drivers/clk/qcom/clk-rpmh.c
> 
> diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
> index fbf4532..63c3372 100644
> --- a/drivers/clk/qcom/Kconfig
> +++ b/drivers/clk/qcom/Kconfig
> @@ -112,6 +112,15 @@ config IPQ_GCC_8074
>   i2c, USB, SD/eMMC, etc. Select this for the root clock
>   of ipq8074.
> 
> +config QCOM_CLK_RPMH
> +   tristate "RPMh Clock Driver"
> +   depends on COMMON_CLK_QCOM && QCOM_RPMH
> +   help
> +RPMh manages shared resources on some Qualcomm Technologies, Inc.
> +SoCs. It accepts requests from other hardware subsystems via RSC.
> +Say Y if you want to support the clocks exposed by RPMh on
> +platforms such as sdm845.

Capitalize sdm?

> +

Can this Kconfig be put into alphabetical order in this file?

>  config MSM_GCC_8660
> tristate "MSM8660 Global Clock Controller"
> depends on COMMON_CLK_QCOM
> diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
> index 230332c..b2b5babf 100644
> --- a/drivers/clk/qcom/Makefile
> +++ b/drivers/clk/qcom/Makefile
> @@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
>  obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
>  obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
>  obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
> +obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
>  obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
>  obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
> new file mode 100644
> index 000..fa61284
> --- /dev/null
> +++ b/drivers/clk/qcom/clk-rpmh.c
> @@ -0,0 +1,367 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include 

Is this include used?

> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#define CLK_RPMH_ARC_EN_OFFSET 0
> +#define CLK_RPMH_VRM_EN_OFFSET 4
> +#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> +BIT(RPMH_ACTIVE_ONLY_STATE))
> +#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
> + BIT(RPMH_ACTIVE_ONLY_STATE) | \
> + BIT(RPMH_SLEEP_STATE))

Is there a reason these aren't inlined into the one place they're used?

> +struct clk_rpmh {
> +   struct clk_hw hw;
> +   const char *res_name;
> +   u32 res_addr;
> +   u32 res_en_offset;

Why do we store both res_addr and res_en_offset? Can't we just store
res_en_offset and then use that all the time? I don't see a user of
res_addr anywhere.

> +   u32 res_on_val;
> +   u32 res_off_val;

Is this used?

> +   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;
> +   unsigned long rate;
> +};

Can you add some kernel-doc on these structure members?

> +
> +struct rpmh_cc {
> +   struct clk_onecell_data data;
> +   struct clk *clks[];
> +};

Hopefully this structure isn't needed.

> +
> +struct clk_rpmh_desc {
> +   struct clk_hw **clks;
> +   size_t num_clks;
> +};
> +
[...]
> +
> +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)));

Too many parenthesis here.

> +}
> +
> +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 = 0;
> +
> +   cmd.addr = c->res_addr + c->res_en_offset;
> +   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;
> +   ret = rpmh_write_async(c->rpmh_client, state,
> +   , 1);
> +   if (ret) {
> +   dev_err(c->dev, "set %s

[PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-13 Thread Taniya Das 
Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by: David Collins 
Signed-off-by: Amit Nischal 
---
 drivers/clk/qcom/Kconfig|   9 ++
 drivers/clk/qcom/Makefile   |   1 +
 drivers/clk/qcom/clk-rpmh.c | 367 
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..63c3372 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
  i2c, USB, SD/eMMC, etc. Select this for the root clock
  of ipq8074.

+config QCOM_CLK_RPMH
+   tristate "RPMh Clock Driver"
+   depends on COMMON_CLK_QCOM && QCOM_RPMH
+   help
+RPMh manages shared resources on some Qualcomm Technologies, Inc.
+SoCs. It accepts requests from other hardware subsystems via RSC.
+Say Y if you want to support the clocks exposed by RPMh on
+platforms such as sdm845.
+
 config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b2b5babf 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 000..fa61284
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+   struct clk_hw hw;
+   const char *res_name;
+   u32 res_addr;
+   u32 res_en_offset;
+   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;
+   unsigned long rate;
+};
+
+struct rpmh_cc {
+   struct clk_onecell_data data;
+   struct clk *clks[];
+};
+
+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, _rate) \
+   static struct clk_rpmh _platform##_##_name_active;\
+   static struct clk_rpmh _platform##_##_name = {\
+   .res_name = _res_name,\
+   .res_en_offset = _res_en_offset,  \
+   .res_on_val = _res_on,\
+   .res_off_val = _res_off,  \
+   .rate = _rate,\
+   .peer = &_platform##_##_name_active,  \
+   .valid_state_mask = CLK_RPMH_APPS_RSC_STATE_MASK, \
+   .hw.init = &(struct clk_init_data){   \
+   .ops = _rpmh_ops, \
+   .name = #_name,   \
+   },\
+   };\
+   static struct clk_rpmh _platform##_##_name_active = { \
+   .res_name = _res_name,\
+   .res_en_offset = _res_en_offset,  \
+   .res_on_val = _res_on,\
+   .res_off_val = _res_off,  \
+   .rate = _rate,\
+   .peer = &_platform##_##_name,

[PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

2018-04-13 Thread Taniya Das 
Add the RPMh clock driver to control the RPMh managed clock resources on
some of the Qualcomm Technologies, Inc. SoCs.

Signed-off-by: David Collins 
Signed-off-by: Amit Nischal 
---
 drivers/clk/qcom/Kconfig|   9 ++
 drivers/clk/qcom/Makefile   |   1 +
 drivers/clk/qcom/clk-rpmh.c | 367 
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/clk/qcom/clk-rpmh.c

diff --git a/drivers/clk/qcom/Kconfig b/drivers/clk/qcom/Kconfig
index fbf4532..63c3372 100644
--- a/drivers/clk/qcom/Kconfig
+++ b/drivers/clk/qcom/Kconfig
@@ -112,6 +112,15 @@ config IPQ_GCC_8074
  i2c, USB, SD/eMMC, etc. Select this for the root clock
  of ipq8074.

+config QCOM_CLK_RPMH
+   tristate "RPMh Clock Driver"
+   depends on COMMON_CLK_QCOM && QCOM_RPMH
+   help
+RPMh manages shared resources on some Qualcomm Technologies, Inc.
+SoCs. It accepts requests from other hardware subsystems via RSC.
+Say Y if you want to support the clocks exposed by RPMh on
+platforms such as sdm845.
+
 config MSM_GCC_8660
tristate "MSM8660 Global Clock Controller"
depends on COMMON_CLK_QCOM
diff --git a/drivers/clk/qcom/Makefile b/drivers/clk/qcom/Makefile
index 230332c..b2b5babf 100644
--- a/drivers/clk/qcom/Makefile
+++ b/drivers/clk/qcom/Makefile
@@ -36,5 +36,6 @@ obj-$(CONFIG_MSM_MMCC_8996) += mmcc-msm8996.o
 obj-$(CONFIG_QCOM_A53PLL) += a53-pll.o
 obj-$(CONFIG_QCOM_CLK_APCS_MSM8916) += apcs-msm8916.o
 obj-$(CONFIG_QCOM_CLK_RPM) += clk-rpm.o
+obj-$(CONFIG_QCOM_CLK_RPMH) += clk-rpmh.o
 obj-$(CONFIG_QCOM_CLK_SMD_RPM) += clk-smd-rpm.o
 obj-$(CONFIG_SPMI_PMIC_CLKDIV) += clk-spmi-pmic-div.o
diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
new file mode 100644
index 000..fa61284
--- /dev/null
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -0,0 +1,367 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+#define CLK_RPMH_ARC_EN_OFFSET 0
+#define CLK_RPMH_VRM_EN_OFFSET 4
+#define CLK_RPMH_APPS_RSC_AO_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+BIT(RPMH_ACTIVE_ONLY_STATE))
+#define CLK_RPMH_APPS_RSC_STATE_MASK (BIT(RPMH_WAKE_ONLY_STATE) | \
+ BIT(RPMH_ACTIVE_ONLY_STATE) | \
+ BIT(RPMH_SLEEP_STATE))
+struct clk_rpmh {
+   struct clk_hw hw;
+   const char *res_name;
+   u32 res_addr;
+   u32 res_en_offset;
+   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;
+   unsigned long rate;
+};
+
+struct rpmh_cc {
+   struct clk_onecell_data data;
+   struct clk *clks[];
+};
+
+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, _rate) \
+   static struct clk_rpmh _platform##_##_name_active;\
+   static struct clk_rpmh _platform##_##_name = {\
+   .res_name = _res_name,\
+   .res_en_offset = _res_en_offset,  \
+   .res_on_val = _res_on,\
+   .res_off_val = _res_off,  \
+   .rate = _rate,\
+   .peer = &_platform##_##_name_active,  \
+   .valid_state_mask = CLK_RPMH_APPS_RSC_STATE_MASK, \
+   .hw.init = &(struct clk_init_data){   \
+   .ops = _rpmh_ops, \
+   .name = #_name,   \
+   },\
+   };\
+   static struct clk_rpmh _platform##_##_name_active = { \
+   .res_name = _res_name,\
+   .res_en_offset = _res_en_offset,  \
+   .res_on_val = _res_on,\
+   .res_off_val = _res_off,  \
+   .rate = _rate,\
+   .peer = &_platform##_##_name, \
+