diff mbox

[RFC] clk: qcom: rpmcc: Add support to XO buffered clocks

Message ID 20180315143724.13859-1-srinivas.kandagatla@linaro.org (mailing list archive)
State RFC, archived
Headers show

Commit Message

Srinivas Kandagatla March 15, 2018, 2:37 p.m. UTC 
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

XO is onchip buffer clock to generate 19.2MHz.

This patch adds support to 5 XO buffer clocks found on PMIC8921,
these buffer clocks can be controlled from external pin or in
manual mode.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/clk/qcom/clk-rpm.c             | 39 +++++++++++++++++++++++++++++++++-
 include/dt-bindings/clock/qcom,rpmcc.h |  5 +++++
 2 files changed, 43 insertions(+), 1 deletion(-)

Comments

Srinivas Kandagatla March 15, 2018, 4:24 p.m. UTC | #1 
On 15/03/18 14:37, srinivas.kandagatla@linaro.org wrote:
>   
>   struct clk_rpm {
>   	const int rpm_clk_id;
> +	const int xo_offset;
> +	u32 xo_buffer_value;
>   	const bool active_only;
Just realized that this would not work when more than one xo are active, 
I will have a closer look and send an new version.

thanks,
srini
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Boyd March 15, 2018, 5:52 p.m. UTC | #2 
Quoting srinivas.kandagatla@linaro.org (2018-03-15 07:37:24)
> diff --git a/drivers/clk/qcom/clk-rpm.c b/drivers/clk/qcom/clk-rpm.c
> index c60f61b10c7f..261f5505e714 100644
> --- a/drivers/clk/qcom/clk-rpm.c
> +++ b/drivers/clk/qcom/clk-rpm.c
> @@ -56,6 +57,19 @@
>                 },                                                            \
>         }
>  
> +#define DEFINE_CLK_RPM_XO_BUFFER(_platform, _name, _active, offset)          \
> +       static struct clk_rpm _platform##_##_name = {                         \
> +               .rpm_clk_id = QCOM_RPM_CXO_BUFFERS,                           \
> +               .xo_offset = (offset),                                        \
> +               .rate = 19200000,                                             \

Shouldn't these rates also come from parent, i.e. pxo_board? So drop
this line?

> +               .hw.init = &(struct clk_init_data){                           \
> +                       .ops = &clk_rpm_fixed_ops,                            \
> +                       .name = #_name,                                       \
> +                       .parent_names = (const char *[]){ "pxo_board" },      \
> +                       .num_parents = 1,                                     \
> +               },                                                            \
> +       }
> +
>  #define DEFINE_CLK_RPM_FIXED(_platform, _name, _active, r_id, r)             \
>         static struct clk_rpm _platform##_##_name = {                         \
>                 .rpm_clk_id = (r_id),                                         \
> @@ -128,6 +142,8 @@
>  
>  struct clk_rpm {
>         const int rpm_clk_id;
> +       const int xo_offset;
> +       u32 xo_buffer_value;
>         const bool active_only;
>         unsigned long rate;
>         bool enabled;
> @@ -308,6 +330,11 @@ static void clk_rpm_fixed_unprepare(struct clk_hw *hw)
>         u32 value = 0;
>         int ret;
>  
> +       if (r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS) {
> +               r->xo_buffer_value &= ~(QCOM_RPM_XO_MODE_ON << r->xo_offset);
> +               value = r->xo_buffer_value;
> +       }
> +

I seem to recall that the xo buffers are within the same "word" or
something like that for each of the buffers. So we would need to make
sure we don't overwrite the bits in there between clks that are
enabling/disabling stuff. Maybe make some sort of shared variable they
all point to and then put a mutex around it? May also be worth making
more clk_ops at that point to not conflate with the simpler on/off code.
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas Kandagatla March 15, 2018, 6:04 p.m. UTC | #3 
Thanks for the review comments,

On 15/03/18 17:52, Stephen Boyd wrote:
> Quoting srinivas.kandagatla@linaro.org (2018-03-15 07:37:24)
>> diff --git a/drivers/clk/qcom/clk-rpm.c b/drivers/clk/qcom/clk-rpm.c
>> index c60f61b10c7f..261f5505e714 100644
>> --- a/drivers/clk/qcom/clk-rpm.c
>> +++ b/drivers/clk/qcom/clk-rpm.c
>> @@ -56,6 +57,19 @@
>>                  },                                                            \
>>          }
>>   
>> +#define DEFINE_CLK_RPM_XO_BUFFER(_platform, _name, _active, offset)          \
>> +       static struct clk_rpm _platform##_##_name = {                         \
>> +               .rpm_clk_id = QCOM_RPM_CXO_BUFFERS,                           \
>> +               .xo_offset = (offset),                                        \
>> +               .rate = 19200000,                                             \
> 
> Shouldn't these rates also come from parent, i.e. pxo_board? So drop
> this line?
Yep, will remove this.
> 
>> +               .hw.init = &(struct clk_init_data){                           \
>> +                       .ops = &clk_rpm_fixed_ops,                            \
>> +                       .name = #_name,                                       \
>> +                       .parent_names = (const char *[]){ "pxo_board" },      \
>> +                       .num_parents = 1,                                     \
>> +               },                                                            \
>> +       }
>> +
>>   #define DEFINE_CLK_RPM_FIXED(_platform, _name, _active, r_id, r)             \
>>          static struct clk_rpm _platform##_##_name = {                         \
>>                  .rpm_clk_id = (r_id),                                         \
>> @@ -128,6 +142,8 @@
>>   
>>   struct clk_rpm {
>>          const int rpm_clk_id;
>> +       const int xo_offset;
>> +       u32 xo_buffer_value;
>>          const bool active_only;
>>          unsigned long rate;
>>          bool enabled;
>> @@ -308,6 +330,11 @@ static void clk_rpm_fixed_unprepare(struct clk_hw *hw)
>>          u32 value = 0;
>>          int ret;
>>   
>> +       if (r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS) {
>> +               r->xo_buffer_value &= ~(QCOM_RPM_XO_MODE_ON << r->xo_offset);
>> +               value = r->xo_buffer_value;
>> +       }
>> +
> 
> I seem to recall that the xo buffers are within the same "word" or
> something like that for each of the buffers. So we would need to make
> sure we don't overwrite the bits in there between clks that are
Code as it is will not work.
> enabling/disabling stuff. Maybe make some sort of shared variable they
> all point to and then put a mutex around it? May also be worth making
> more clk_ops at that point to not conflate with the simpler on/off code.
Yep, I will give something like that a try in next version.


-srini

> 
--
To unsubscribe from this list: send the line "unsubscribe linux-clk" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/clk/qcom/clk-rpm.c b/drivers/clk/qcom/clk-rpm.c
index c60f61b10c7f..261f5505e714 100644
--- a/drivers/clk/qcom/clk-rpm.c
+++ b/drivers/clk/qcom/clk-rpm.c
@@ -29,6 +29,7 @@ 
 
 #define QCOM_RPM_MISC_CLK_TYPE				0x306b6c63
 #define QCOM_RPM_SCALING_ENABLE_ID			0x2
+#define QCOM_RPM_XO_MODE_ON				0x2
 
 #define DEFINE_CLK_RPM(_platform, _name, _active, r_id)			      \
 	static struct clk_rpm _platform##_##_active;			      \
@@ -56,6 +57,19 @@ 
 		},							      \
 	}
 
+#define DEFINE_CLK_RPM_XO_BUFFER(_platform, _name, _active, offset)	      \
+	static struct clk_rpm _platform##_##_name = {			      \
+		.rpm_clk_id = QCOM_RPM_CXO_BUFFERS,			      \
+		.xo_offset = (offset),					      \
+		.rate = 19200000,					      \
+		.hw.init = &(struct clk_init_data){			      \
+			.ops = &clk_rpm_fixed_ops,			      \
+			.name = #_name,					      \
+			.parent_names = (const char *[]){ "pxo_board" },      \
+			.num_parents = 1,				      \
+		},							      \
+	}
+
 #define DEFINE_CLK_RPM_FIXED(_platform, _name, _active, r_id, r)	      \
 	static struct clk_rpm _platform##_##_name = {			      \
 		.rpm_clk_id = (r_id),					      \
@@ -128,6 +142,8 @@ 
 
 struct clk_rpm {
 	const int rpm_clk_id;
+	const int xo_offset;
+	u32 xo_buffer_value;
 	const bool active_only;
 	unsigned long rate;
 	bool enabled;
@@ -159,7 +175,8 @@  static int clk_rpm_handoff(struct clk_rpm *r)
 	 * The vendor tree simply reads the status for this
 	 * RPM clock.
 	 */
-	if (r->rpm_clk_id == QCOM_RPM_PLL_4)
+	if (r->rpm_clk_id == QCOM_RPM_PLL_4 ||
+		r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS)
 		return 0;
 
 	ret = qcom_rpm_write(r->rpm, QCOM_RPM_ACTIVE_STATE,
@@ -294,6 +311,11 @@  static int clk_rpm_fixed_prepare(struct clk_hw *hw)
 	u32 value = 1;
 	int ret;
 
+	if (r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS) {
+		r->xo_buffer_value |= (QCOM_RPM_XO_MODE_ON << r->xo_offset);
+		value = r->xo_buffer_value;
+	}
+
 	ret = qcom_rpm_write(r->rpm, QCOM_RPM_ACTIVE_STATE,
 			     r->rpm_clk_id, &value, 1);
 	if (!ret)
@@ -308,6 +330,11 @@  static void clk_rpm_fixed_unprepare(struct clk_hw *hw)
 	u32 value = 0;
 	int ret;
 
+	if (r->rpm_clk_id == QCOM_RPM_CXO_BUFFERS) {
+		r->xo_buffer_value &= ~(QCOM_RPM_XO_MODE_ON << r->xo_offset);
+		value = r->xo_buffer_value;
+	}
+
 	ret = qcom_rpm_write(r->rpm, QCOM_RPM_ACTIVE_STATE,
 			     r->rpm_clk_id, &value, 1);
 	if (!ret)
@@ -449,6 +476,11 @@  DEFINE_CLK_RPM(apq8064, mmfpb_clk, mmfpb_a_clk, QCOM_RPM_MMFPB_CLK);
 DEFINE_CLK_RPM(apq8064, sfab_clk, sfab_a_clk, QCOM_RPM_SYS_FABRIC_CLK);
 DEFINE_CLK_RPM(apq8064, sfpb_clk, sfpb_a_clk, QCOM_RPM_SFPB_CLK);
 DEFINE_CLK_RPM(apq8064, qdss_clk, qdss_a_clk, QCOM_RPM_QDSS_CLK);
+DEFINE_CLK_RPM_XO_BUFFER(apq8064, xo_d0_clk, xo_d0_a_clk, 0);
+DEFINE_CLK_RPM_XO_BUFFER(apq8064, xo_d1_clk, xo_d1_a_clk, 8);
+DEFINE_CLK_RPM_XO_BUFFER(apq8064, xo_a0_clk, xo_a0_a_clk, 16);
+DEFINE_CLK_RPM_XO_BUFFER(apq8064, xo_a1_clk, xo_a1_a_clk, 24);
+DEFINE_CLK_RPM_XO_BUFFER(apq8064, xo_a2_clk, xo_a2_a_clk, 28);
 
 static struct clk_rpm *apq8064_clks[] = {
 	[RPM_APPS_FABRIC_CLK] = &apq8064_afab_clk,
@@ -469,6 +501,11 @@  static struct clk_rpm *apq8064_clks[] = {
 	[RPM_SFPB_A_CLK] = &apq8064_sfpb_a_clk,
 	[RPM_QDSS_CLK] = &apq8064_qdss_clk,
 	[RPM_QDSS_A_CLK] = &apq8064_qdss_a_clk,
+	[RPM_XO_D0] = &apq8064_xo_d0_clk,
+	[RPM_XO_D1] = &apq8064_xo_d1_clk,
+	[RPM_XO_A0] = &apq8064_xo_a0_clk,
+	[RPM_XO_A1] = &apq8064_xo_a1_clk,
+	[RPM_XO_A2] = &apq8064_xo_a2_clk,
 };
 
 static const struct rpm_clk_desc rpm_clk_apq8064 = {
diff --git a/include/dt-bindings/clock/qcom,rpmcc.h b/include/dt-bindings/clock/qcom,rpmcc.h
index b8337a5fa347..c585b82b9c05 100644
--- a/include/dt-bindings/clock/qcom,rpmcc.h
+++ b/include/dt-bindings/clock/qcom,rpmcc.h
@@ -40,6 +40,11 @@ 
 #define RPM_SMI_CLK				22
 #define RPM_SMI_A_CLK				23
 #define RPM_PLL4_CLK				24
+#define RPM_XO_D0				25
+#define RPM_XO_D1				26
+#define RPM_XO_A0				27
+#define RPM_XO_A1				28
+#define RPM_XO_A2				29
 
 /* SMD RPM clocks */
 #define RPM_SMD_XO_CLK_SRC				0