[PATCH 0/5] clk: zynqmp: fix CLK_FRAC and various cleanups

[PATCH 0/5] clk: zynqmp: fix CLK_FRAC and various cleanups

[Michael Tretter]

Hello,

These are various patches for the ZynqMP clock driver.

Patch 1 fixes an erroneous test for the CLK_FRAC flag.

Patch 2 fixes a wrong function name in the kernel-doc. However, I think most
of the kernel-doc for the static functions is pretty useless/misleading and
maybe instead of fixing, we should just remove it.

Patches 3 to 5 are cleanup and refactoring and make the macros that are used
to handle the responses from the firmware more consistent.

Michael

Michael Tretter (5):
  clk: zynqmp: fix check for fractional clock
  clk: zynqmp: fix doc of __zynqmp_clock_get_parents
  clk: zynqmp: do not export zynqmp_clk_register_mux
  clk: zynqmp: cleanup sizes of firmware responses
  clk: zynqmp: make field definitions of query responses consistent

 drivers/clk/zynqmp/clk-mux-zynqmp.c |  1 -
 drivers/clk/zynqmp/clk-zynqmp.h     |  6 ---
 drivers/clk/zynqmp/clkc.c           | 71 +++++++++++++++--------------
 drivers/clk/zynqmp/divider.c        |  2 +-
 4 files changed, 38 insertions(+), 42 deletions(-)

-- 
2.20.1

[PATCH 0/5] clk: zynqmp: fix CLK_FRAC and various cleanups

[Michael Tretter]

Hello,

These are various patches for the ZynqMP clock driver.

Patch 1 fixes an erroneous test for the CLK_FRAC flag.

Patch 2 fixes a wrong function name in the kernel-doc. However, I think most
of the kernel-doc for the static functions is pretty useless/misleading and
maybe instead of fixing, we should just remove it.

Patches 3 to 5 are cleanup and refactoring and make the macros that are used
to handle the responses from the firmware more consistent.

Michael

Michael Tretter (5):
  clk: zynqmp: fix check for fractional clock
  clk: zynqmp: fix doc of __zynqmp_clock_get_parents
  clk: zynqmp: do not export zynqmp_clk_register_mux
  clk: zynqmp: cleanup sizes of firmware responses
  clk: zynqmp: make field definitions of query responses consistent

 drivers/clk/zynqmp/clk-mux-zynqmp.c |  1 -
 drivers/clk/zynqmp/clk-zynqmp.h     |  6 ---
 drivers/clk/zynqmp/clkc.c           | 71 +++++++++++++++--------------
 drivers/clk/zynqmp/divider.c        |  2 +-
 4 files changed, 38 insertions(+), 42 deletions(-)

-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

CLK_FRAC is not set in the divider->flags, but in the hw->flags.

The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
When registering the devider, these clkflags are copied to hw->flags.

Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
this check would never work.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/divider.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
index a371c66e72ef..fc70950c1e24 100644
--- a/drivers/clk/zynqmp/divider.c
+++ b/drivers/clk/zynqmp/divider.c
@@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
 	bestdiv = zynqmp_divider_get_val(*prate, rate);
 
 	if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
-	    (divider->flags & CLK_FRAC))
+	    (clk_hw_get_flags(hw) & CLK_FRAC))
 		bestdiv = rate % *prate ? 1 : bestdiv;
 	*prate = rate * bestdiv;
 
-- 
2.20.1

[PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

CLK_FRAC is not set in the divider->flags, but in the hw->flags.

The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
When registering the devider, these clkflags are copied to hw->flags.

Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
this check would never work.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/divider.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
index a371c66e72ef..fc70950c1e24 100644
--- a/drivers/clk/zynqmp/divider.c
+++ b/drivers/clk/zynqmp/divider.c
@@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
 	bestdiv = zynqmp_divider_get_val(*prate, rate);
 
 	if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
-	    (divider->flags & CLK_FRAC))
+	    (clk_hw_get_flags(hw) & CLK_FRAC))
 		bestdiv = rate % *prate ? 1 : bestdiv;
 	*prate = rate * bestdiv;
 
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/5] clk: zynqmp: fix doc of __zynqmp_clock_get_parents

[Michael Tretter]

The kernel-doc refers to __zynqmp_clock_get_topology(), while it actually
documents __zynqmp_clock_get_parents(). Fix the documentation to refer
to the correct function name.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
I think most of the function documentation in this file is pretty
useless and sometimes misleading. Maybe we should drop the documentation
entirely, because all functions are static anyway?
---
 drivers/clk/zynqmp/clkc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index b0908ec62f73..d3d4ce305e71 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -408,7 +408,7 @@ static int zynqmp_clock_get_topology(u32 clk_id,
 }
 
 /**
- * __zynqmp_clock_get_topology() - Get parents info of clock from firmware
+ * __zynqmp_clock_get_parents() - Get parents info of clock from firmware
  *				   response data
  * @parents:		Clock parents
  * @data:		Clock parents data received from firmware
-- 
2.20.1

[PATCH 2/5] clk: zynqmp: fix doc of __zynqmp_clock_get_parents

[Michael Tretter]

The kernel-doc refers to __zynqmp_clock_get_topology(), while it actually
documents __zynqmp_clock_get_parents(). Fix the documentation to refer
to the correct function name.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
I think most of the function documentation in this file is pretty
useless and sometimes misleading. Maybe we should drop the documentation
entirely, because all functions are static anyway?
---
 drivers/clk/zynqmp/clkc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index b0908ec62f73..d3d4ce305e71 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -408,7 +408,7 @@ static int zynqmp_clock_get_topology(u32 clk_id,
 }
 
 /**
- * __zynqmp_clock_get_topology() - Get parents info of clock from firmware
+ * __zynqmp_clock_get_parents() - Get parents info of clock from firmware
  *				   response data
  * @parents:		Clock parents
  * @data:		Clock parents data received from firmware
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Michael Tretter]

zynqmp_clk_register_mux is an internal function of the driver that only
should be used from clkc.c. Therefore, there is no need to export that
function.

Follow the suit of the other files of the driver and do not export the
register function.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clk-mux-zynqmp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clk-mux-zynqmp.c b/drivers/clk/zynqmp/clk-mux-zynqmp.c
index 4143f560c28d..0af8f74c5fa5 100644
--- a/drivers/clk/zynqmp/clk-mux-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-mux-zynqmp.c
@@ -138,4 +138,3 @@ struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
 
 	return hw;
 }
-EXPORT_SYMBOL_GPL(zynqmp_clk_register_mux);
-- 
2.20.1

[PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Michael Tretter]

zynqmp_clk_register_mux is an internal function of the driver that only
should be used from clkc.c. Therefore, there is no need to export that
function.

Follow the suit of the other files of the driver and do not export the
register function.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clk-mux-zynqmp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/zynqmp/clk-mux-zynqmp.c b/drivers/clk/zynqmp/clk-mux-zynqmp.c
index 4143f560c28d..0af8f74c5fa5 100644
--- a/drivers/clk/zynqmp/clk-mux-zynqmp.c
+++ b/drivers/clk/zynqmp/clk-mux-zynqmp.c
@@ -138,4 +138,3 @@ struct clk_hw *zynqmp_clk_register_mux(const char *name, u32 clk_id,
 
 	return hw;
 }
-EXPORT_SYMBOL_GPL(zynqmp_clk_register_mux);
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/5] clk: zynqmp: cleanup sizes of firmware responses

[Michael Tretter]

The queries for the clock name, clock topology, clock parents, and clock
attributes return a specified count of values, whose type and number
depends on the query. Properly separate the number of values per query,
make it dependent on the returned type values and get rid of leftover
hard coded sizes.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clk-zynqmp.h |  6 ------
 drivers/clk/zynqmp/clkc.c       | 32 +++++++++++++++++++-------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-zynqmp.h
index 7ab163b67249..fec9a15c8786 100644
--- a/drivers/clk/zynqmp/clk-zynqmp.h
+++ b/drivers/clk/zynqmp/clk-zynqmp.h
@@ -10,12 +10,6 @@
 
 #include <linux/firmware/xlnx-zynqmp.h>
 
-/* Clock APIs payload parameters */
-#define CLK_GET_NAME_RESP_LEN				16
-#define CLK_GET_TOPOLOGY_RESP_WORDS			3
-#define CLK_GET_PARENTS_RESP_WORDS			3
-#define CLK_GET_ATTR_RESP_WORDS				1
-
 enum topology_type {
 	TYPE_INVALID,
 	TYPE_MUX,
diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index d3d4ce305e71..573c3c58dbbc 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -23,14 +23,11 @@
 
 #define CLK_TYPE_SHIFT			2
 
-#define PM_API_PAYLOAD_LEN		3
-
 #define NA_PARENT			0xFFFFFFFF
 #define DUMMY_PARENT			0xFFFFFFFE
 
 #define CLK_TYPE_FIELD_LEN		4
 #define CLK_TOPOLOGY_NODE_OFFSET	16
-#define NODES_PER_RESP			3
 
 #define CLK_TYPE_FIELD_MASK		0xF
 #define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
@@ -54,6 +51,11 @@
 
 #define CLK_VALID_MASK			0x1
 
+#define CLK_GET_NAME_RESP_LEN		16
+#define CLK_GET_TOPOLOGY_RESP_WORDS	3
+#define CLK_GET_PARENTS_RESP_WORDS	3
+#define CLK_GET_ATTR_RESP_WORDS		1
+
 enum clk_type {
 	CLK_TYPE_OUTPUT,
 	CLK_TYPE_EXTERNAL,
@@ -215,7 +217,8 @@ static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
 	qdata.arg1 = clock_id;
 
 	eemi_ops->query_data(qdata, ret_payload);
-	memcpy(name, ret_payload, CLK_GET_NAME_RESP_LEN);
+	memcpy(name, ret_payload,
+	       CLK_GET_NAME_RESP_LEN * sizeof(*name));
 
 	return 0;
 }
@@ -248,7 +251,8 @@ static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(topology, &ret_payload[1], CLK_GET_TOPOLOGY_RESP_WORDS * 4);
+	memcpy(topology, &ret_payload[1],
+	       CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));
 
 	return ret;
 }
@@ -321,7 +325,8 @@ static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS * 4);
+	memcpy(parents, &ret_payload[1],
+	       CLK_GET_PARENTS_RESP_WORDS * sizeof(*parents));
 
 	return ret;
 }
@@ -345,7 +350,8 @@ static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
 	qdata.arg1 = clock_id;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);
+	memcpy(attr, &ret_payload[1],
+	       CLK_GET_ATTR_RESP_WORDS * sizeof(*attr));
 
 	return ret;
 }
@@ -364,7 +370,7 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 {
 	int i;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
 		if (!(data[i] & CLK_TYPE_FIELD_MASK))
 			return END_OF_TOPOLOGY_NODE;
 		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
@@ -392,10 +398,10 @@ static int zynqmp_clock_get_topology(u32 clk_id,
 				     u32 *num_nodes)
 {
 	int j, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	u32 pm_resp[CLK_GET_TOPOLOGY_RESP_WORDS] = {0};
 
 	*num_nodes = 0;
-	for (j = 0; j <= MAX_NODES; j += 3) {
+	for (j = 0; j <= MAX_NODES; j += CLK_GET_TOPOLOGY_RESP_WORDS) {
 		ret = zynqmp_pm_clock_get_topology(clk_id, j, pm_resp);
 		if (ret)
 			return ret;
@@ -422,7 +428,7 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 	int i;
 	struct clock_parent *parent;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_PARENTS_RESP_WORDS; i++) {
 		if (data[i] == NA_PARENT)
 			return END_OF_PARENTS;
 
@@ -454,7 +460,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 				    u32 *num_parents)
 {
 	int j = 0, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	u32 pm_resp[CLK_GET_PARENTS_RESP_WORDS] = {0};
 
 	*num_parents = 0;
 	do {
@@ -467,7 +473,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 						 num_parents);
 		if (ret == END_OF_PARENTS)
 			return 0;
-		j += PM_API_PAYLOAD_LEN;
+		j += CLK_GET_PARENTS_RESP_WORDS;
 	} while (*num_parents <= MAX_PARENT);
 
 	return 0;
-- 
2.20.1

[PATCH 4/5] clk: zynqmp: cleanup sizes of firmware responses

[Michael Tretter]

The queries for the clock name, clock topology, clock parents, and clock
attributes return a specified count of values, whose type and number
depends on the query. Properly separate the number of values per query,
make it dependent on the returned type values and get rid of leftover
hard coded sizes.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clk-zynqmp.h |  6 ------
 drivers/clk/zynqmp/clkc.c       | 32 +++++++++++++++++++-------------
 2 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/zynqmp/clk-zynqmp.h b/drivers/clk/zynqmp/clk-zynqmp.h
index 7ab163b67249..fec9a15c8786 100644
--- a/drivers/clk/zynqmp/clk-zynqmp.h
+++ b/drivers/clk/zynqmp/clk-zynqmp.h
@@ -10,12 +10,6 @@
 
 #include <linux/firmware/xlnx-zynqmp.h>
 
-/* Clock APIs payload parameters */
-#define CLK_GET_NAME_RESP_LEN				16
-#define CLK_GET_TOPOLOGY_RESP_WORDS			3
-#define CLK_GET_PARENTS_RESP_WORDS			3
-#define CLK_GET_ATTR_RESP_WORDS				1
-
 enum topology_type {
 	TYPE_INVALID,
 	TYPE_MUX,
diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index d3d4ce305e71..573c3c58dbbc 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -23,14 +23,11 @@
 
 #define CLK_TYPE_SHIFT			2
 
-#define PM_API_PAYLOAD_LEN		3
-
 #define NA_PARENT			0xFFFFFFFF
 #define DUMMY_PARENT			0xFFFFFFFE
 
 #define CLK_TYPE_FIELD_LEN		4
 #define CLK_TOPOLOGY_NODE_OFFSET	16
-#define NODES_PER_RESP			3
 
 #define CLK_TYPE_FIELD_MASK		0xF
 #define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
@@ -54,6 +51,11 @@
 
 #define CLK_VALID_MASK			0x1
 
+#define CLK_GET_NAME_RESP_LEN		16
+#define CLK_GET_TOPOLOGY_RESP_WORDS	3
+#define CLK_GET_PARENTS_RESP_WORDS	3
+#define CLK_GET_ATTR_RESP_WORDS		1
+
 enum clk_type {
 	CLK_TYPE_OUTPUT,
 	CLK_TYPE_EXTERNAL,
@@ -215,7 +217,8 @@ static int zynqmp_pm_clock_get_name(u32 clock_id, char *name)
 	qdata.arg1 = clock_id;
 
 	eemi_ops->query_data(qdata, ret_payload);
-	memcpy(name, ret_payload, CLK_GET_NAME_RESP_LEN);
+	memcpy(name, ret_payload,
+	       CLK_GET_NAME_RESP_LEN * sizeof(*name));
 
 	return 0;
 }
@@ -248,7 +251,8 @@ static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(topology, &ret_payload[1], CLK_GET_TOPOLOGY_RESP_WORDS * 4);
+	memcpy(topology, &ret_payload[1],
+	       CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));
 
 	return ret;
 }
@@ -321,7 +325,8 @@ static int zynqmp_pm_clock_get_parents(u32 clock_id, u32 index, u32 *parents)
 	qdata.arg2 = index;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(parents, &ret_payload[1], CLK_GET_PARENTS_RESP_WORDS * 4);
+	memcpy(parents, &ret_payload[1],
+	       CLK_GET_PARENTS_RESP_WORDS * sizeof(*parents));
 
 	return ret;
 }
@@ -345,7 +350,8 @@ static int zynqmp_pm_clock_get_attributes(u32 clock_id, u32 *attr)
 	qdata.arg1 = clock_id;
 
 	ret = eemi_ops->query_data(qdata, ret_payload);
-	memcpy(attr, &ret_payload[1], CLK_GET_ATTR_RESP_WORDS * 4);
+	memcpy(attr, &ret_payload[1],
+	       CLK_GET_ATTR_RESP_WORDS * sizeof(*attr));
 
 	return ret;
 }
@@ -364,7 +370,7 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 {
 	int i;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
 		if (!(data[i] & CLK_TYPE_FIELD_MASK))
 			return END_OF_TOPOLOGY_NODE;
 		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
@@ -392,10 +398,10 @@ static int zynqmp_clock_get_topology(u32 clk_id,
 				     u32 *num_nodes)
 {
 	int j, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	u32 pm_resp[CLK_GET_TOPOLOGY_RESP_WORDS] = {0};
 
 	*num_nodes = 0;
-	for (j = 0; j <= MAX_NODES; j += 3) {
+	for (j = 0; j <= MAX_NODES; j += CLK_GET_TOPOLOGY_RESP_WORDS) {
 		ret = zynqmp_pm_clock_get_topology(clk_id, j, pm_resp);
 		if (ret)
 			return ret;
@@ -422,7 +428,7 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 	int i;
 	struct clock_parent *parent;
 
-	for (i = 0; i < PM_API_PAYLOAD_LEN; i++) {
+	for (i = 0; i < CLK_GET_PARENTS_RESP_WORDS; i++) {
 		if (data[i] == NA_PARENT)
 			return END_OF_PARENTS;
 
@@ -454,7 +460,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 				    u32 *num_parents)
 {
 	int j = 0, ret;
-	u32 pm_resp[PM_API_PAYLOAD_LEN] = {0};
+	u32 pm_resp[CLK_GET_PARENTS_RESP_WORDS] = {0};
 
 	*num_parents = 0;
 	do {
@@ -467,7 +473,7 @@ static int zynqmp_clock_get_parents(u32 clk_id, struct clock_parent *parents,
 						 num_parents);
 		if (ret == END_OF_PARENTS)
 			return 0;
-		j += PM_API_PAYLOAD_LEN;
+		j += CLK_GET_PARENTS_RESP_WORDS;
 	} while (*num_parents <= MAX_PARENT);
 
 	return 0;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Michael Tretter]

The definition of the fields in the firmware responses to the queries is
inconsistent. Some are specified as a mask, some as a shift, and some by
the length of the previous field.

Specify all of them as bit masks and access the fields using the
FIELD_GET macro.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index 573c3c58dbbc..b4e13b8618b2 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -21,20 +21,18 @@
 #define MAX_NODES			6
 #define MAX_NAME_LEN			50
 
-#define CLK_TYPE_SHIFT			2
-
 #define NA_PARENT			0xFFFFFFFF
 #define DUMMY_PARENT			0xFFFFFFFE
 
-#define CLK_TYPE_FIELD_LEN		4
-#define CLK_TOPOLOGY_NODE_OFFSET	16
+#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
+#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
+#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
 
-#define CLK_TYPE_FIELD_MASK		0xF
-#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
-#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
+#define CLK_PARENTS_ID			GENMASK(15, 0)
+#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
 
-#define CLK_PARENTS_ID_LEN		16
-#define CLK_PARENTS_ID_MASK		0xFFFF
+#define CLK_ATTR_VALID			BIT(0)
+#define CLK_ATTR_TYPE			BIT(2)
 
 /* Flags for parents */
 #define PARENT_CLK_SELF			0
@@ -49,8 +47,6 @@
 #define END_OF_PARENTS			1
 #define RESERVED_CLK_NAME		""
 
-#define CLK_VALID_MASK			0x1
-
 #define CLK_GET_NAME_RESP_LEN		16
 #define CLK_GET_TOPOLOGY_RESP_WORDS	3
 #define CLK_GET_PARENTS_RESP_WORDS	3
@@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 				       u32 *data, u32 *nnodes)
 {
 	int i;
+	u32 type;
 
 	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
-		if (!(data[i] & CLK_TYPE_FIELD_MASK))
+		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
+		if (type == TYPE_INVALID)
 			return END_OF_TOPOLOGY_NODE;
-		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
-		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
-						   data[i]);
+		topology[*nnodes].type = type
+		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS, data[i]);
 		topology[*nnodes].type_flag =
-				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
+				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
 		(*nnodes)++;
 	}
 
@@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 			return END_OF_PARENTS;
 
 		parent = &parents[i];
-		parent->id = data[i] & CLK_PARENTS_ID_MASK;
+		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
 		if (data[i] == DUMMY_PARENT) {
 			strcpy(parent->name, "dummy_name");
 			parent->flag = 0;
 		} else {
-			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
+			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS, data[i]);
 			if (zynqmp_get_clock_name(parent->id, parent->name))
 				continue;
 		}
@@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
 		if (ret)
 			continue;
 
-		clock[i].valid = attr & CLK_VALID_MASK;
-		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
-							CLK_TYPE_OUTPUT;
+		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
+		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
+			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
 	}
 
 	/* Get topology of all clock */
-- 
2.20.1

[PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Michael Tretter]

The definition of the fields in the firmware responses to the queries is
inconsistent. Some are specified as a mask, some as a shift, and some by
the length of the previous field.

Specify all of them as bit masks and access the fields using the
FIELD_GET macro.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
 1 file changed, 18 insertions(+), 21 deletions(-)

diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
index 573c3c58dbbc..b4e13b8618b2 100644
--- a/drivers/clk/zynqmp/clkc.c
+++ b/drivers/clk/zynqmp/clkc.c
@@ -21,20 +21,18 @@
 #define MAX_NODES			6
 #define MAX_NAME_LEN			50
 
-#define CLK_TYPE_SHIFT			2
-
 #define NA_PARENT			0xFFFFFFFF
 #define DUMMY_PARENT			0xFFFFFFFE
 
-#define CLK_TYPE_FIELD_LEN		4
-#define CLK_TOPOLOGY_NODE_OFFSET	16
+#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
+#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
+#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
 
-#define CLK_TYPE_FIELD_MASK		0xF
-#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
-#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
+#define CLK_PARENTS_ID			GENMASK(15, 0)
+#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
 
-#define CLK_PARENTS_ID_LEN		16
-#define CLK_PARENTS_ID_MASK		0xFFFF
+#define CLK_ATTR_VALID			BIT(0)
+#define CLK_ATTR_TYPE			BIT(2)
 
 /* Flags for parents */
 #define PARENT_CLK_SELF			0
@@ -49,8 +47,6 @@
 #define END_OF_PARENTS			1
 #define RESERVED_CLK_NAME		""
 
-#define CLK_VALID_MASK			0x1
-
 #define CLK_GET_NAME_RESP_LEN		16
 #define CLK_GET_TOPOLOGY_RESP_WORDS	3
 #define CLK_GET_PARENTS_RESP_WORDS	3
@@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
 				       u32 *data, u32 *nnodes)
 {
 	int i;
+	u32 type;
 
 	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
-		if (!(data[i] & CLK_TYPE_FIELD_MASK))
+		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
+		if (type == TYPE_INVALID)
 			return END_OF_TOPOLOGY_NODE;
-		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
-		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
-						   data[i]);
+		topology[*nnodes].type = type
+		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS, data[i]);
 		topology[*nnodes].type_flag =
-				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
+				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
 		(*nnodes)++;
 	}
 
@@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
 			return END_OF_PARENTS;
 
 		parent = &parents[i];
-		parent->id = data[i] & CLK_PARENTS_ID_MASK;
+		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
 		if (data[i] == DUMMY_PARENT) {
 			strcpy(parent->name, "dummy_name");
 			parent->flag = 0;
 		} else {
-			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
+			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS, data[i]);
 			if (zynqmp_get_clock_name(parent->id, parent->name))
 				continue;
 		}
@@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
 		if (ret)
 			continue;
 
-		clock[i].valid = attr & CLK_VALID_MASK;
-		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
-							CLK_TYPE_OUTPUT;
+		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
+		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
+			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
 	}
 
 	/* Get topology of all clock */
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Michael Tretter]

Oops, somehow this previous incomplete version of the patch slipped through.

I will send a v2 after getting some feedback on the whole series.

Michael

On Tue, 12 Mar 2019 12:00:16 +0100, Michael Tretter wrote:
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
>  
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
>  
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
>  
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
>  
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
>  
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
>  
> -#define CLK_VALID_MASK			0x1
> -
>  #define CLK_GET_NAME_RESP_LEN		16
>  #define CLK_GET_TOPOLOGY_RESP_WORDS	3
>  #define CLK_GET_PARENTS_RESP_WORDS	3
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
>  
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
> +		if (type == TYPE_INVALID)
>  			return END_OF_TOPOLOGY_NODE;
> -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> -						   data[i]);
> +		topology[*nnodes].type = type

Missing semicolon.

> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS, data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
>  		(*nnodes)++;
>  	}
>  
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
>  
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[i] == DUMMY_PARENT) {
>  			strcpy(parent->name, "dummy_name");
>  			parent->flag = 0;
>  		} else {
> -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS, data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent->name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
>  
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?

		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr) ?

> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
>  
>  	/* Get topology of all clock */

Re: [PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Michael Tretter]

Oops, somehow this previous incomplete version of the patch slipped through.

I will send a v2 after getting some feedback on the whole series.

Michael

On Tue, 12 Mar 2019 12:00:16 +0100, Michael Tretter wrote:
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
>  
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
>  
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
>  
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)
> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
>  
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
>  
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
>  
> -#define CLK_VALID_MASK			0x1
> -
>  #define CLK_GET_NAME_RESP_LEN		16
>  #define CLK_GET_TOPOLOGY_RESP_WORDS	3
>  #define CLK_GET_PARENTS_RESP_WORDS	3
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
>  
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
> +		if (type == TYPE_INVALID)
>  			return END_OF_TOPOLOGY_NODE;
> -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> -						   data[i]);
> +		topology[*nnodes].type = type

Missing semicolon.

> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS, data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK, data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS, data[i]);
>  		(*nnodes)++;
>  	}
>  
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
>  
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[i] == DUMMY_PARENT) {
>  			strcpy(parent->name, "dummy_name");
>  			parent->flag = 0;
>  		} else {
> -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS, data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent->name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
>  
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?

		clock[i].type = FIELD_GET(CLK_ATTR_TYPE, attr) ?

> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
>  
>  	/* Get topology of all clock */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:12)
> CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> 
> The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> When registering the devider, these clkflags are copied to hw->flags.
> 
> Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> this check would never work.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/divider.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> index a371c66e72ef..fc70950c1e24 100644
> --- a/drivers/clk/zynqmp/divider.c
> +++ b/drivers/clk/zynqmp/divider.c
> @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
>         bestdiv = zynqmp_divider_get_val(*prate, rate);
>  
>         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> -           (divider->flags & CLK_FRAC))
> +           (clk_hw_get_flags(hw) & CLK_FRAC))

CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
not a clk framework flag so it shouldn't go there. Please fix the user
of this flag to place the CLK_FRAC flag somewhere else. Even adding it
into divider::flags is not a good idea because that numberspace is for
dividers, and this flag seems to be zynqmp driver specific, so maybe
just add a bool to the zynqmp_clk_divider?

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:12)
> CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> 
> The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> When registering the devider, these clkflags are copied to hw->flags.
> 
> Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> this check would never work.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/divider.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> index a371c66e72ef..fc70950c1e24 100644
> --- a/drivers/clk/zynqmp/divider.c
> +++ b/drivers/clk/zynqmp/divider.c
> @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
>         bestdiv = zynqmp_divider_get_val(*prate, rate);
>  
>         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> -           (divider->flags & CLK_FRAC))
> +           (clk_hw_get_flags(hw) & CLK_FRAC))

CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
not a clk framework flag so it shouldn't go there. Please fix the user
of this flag to place the CLK_FRAC flag somewhere else. Even adding it
into divider::flags is not a good idea because that numberspace is for
dividers, and this flag seems to be zynqmp driver specific, so maybe
just add a bool to the zynqmp_clk_divider?


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:14)
> zynqmp_clk_register_mux is an internal function of the driver that only
> should be used from clkc.c. Therefore, there is no need to export that
> function.
> 
> Follow the suit of the other files of the driver and do not export the
> register function.

Does this mean that the divider file shouldn't export the divider
registration function either? Please add a patch for that if it's the
case. Otherwise this looks fine given that the code using this function
is always built in.

Re: [PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:14)
> zynqmp_clk_register_mux is an internal function of the driver that only
> should be used from clkc.c. Therefore, there is no need to export that
> function.
> 
> Follow the suit of the other files of the driver and do not export the
> register function.

Does this mean that the divider file shouldn't export the divider
registration function either? Please add a patch for that if it's the
case. Otherwise this looks fine given that the code using this function
is always built in.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 04:00:12)
> > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > 
> > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > When registering the devider, these clkflags are copied to hw->flags.
> > 
> > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > this check would never work.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/clk/zynqmp/divider.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > index a371c66e72ef..fc70950c1e24 100644
> > --- a/drivers/clk/zynqmp/divider.c
> > +++ b/drivers/clk/zynqmp/divider.c
> > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> >  
> >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > -           (divider->flags & CLK_FRAC))
> > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> 
> CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> not a clk framework flag so it shouldn't go there. Please fix the user
> of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> into divider::flags is not a good idea because that numberspace is for
> dividers, and this flag seems to be zynqmp driver specific, so maybe
> just add a bool to the zynqmp_clk_divider?
> 

Thanks. The driver sets the clk_hw::core::flags based on a response
from the ATF and this response includes this flag with other clk
frameworks flags. I can test for the flag when registering the clock
and set another flag or a bool for the zynqmp_clk_divider and will do
so in v2.

However, this merely sounds like a workaround for an issue in the ATF,
which should not define and use this flag in the first place. 

Michael

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 04:00:12)
> > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > 
> > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > When registering the devider, these clkflags are copied to hw->flags.
> > 
> > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > this check would never work.
> > 
> > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > ---
> >  drivers/clk/zynqmp/divider.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > index a371c66e72ef..fc70950c1e24 100644
> > --- a/drivers/clk/zynqmp/divider.c
> > +++ b/drivers/clk/zynqmp/divider.c
> > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> >  
> >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > -           (divider->flags & CLK_FRAC))
> > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> 
> CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> not a clk framework flag so it shouldn't go there. Please fix the user
> of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> into divider::flags is not a good idea because that numberspace is for
> dividers, and this flag seems to be zynqmp driver specific, so maybe
> just add a bool to the zynqmp_clk_divider?
> 

Thanks. The driver sets the clk_hw::core::flags based on a response
from the ATF and this response includes this flag with other clk
frameworks flags. I can test for the flag when registering the clock
and set another flag or a bool for the zynqmp_clk_divider and will do
so in v2.

However, this merely sounds like a workaround for an issue in the ATF,
which should not define and use this flag in the first place. 

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Michael Tretter]

On Tue, 12 Mar 2019 09:52:36 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 04:00:14)
> > zynqmp_clk_register_mux is an internal function of the driver that only
> > should be used from clkc.c. Therefore, there is no need to export that
> > function.
> > 
> > Follow the suit of the other files of the driver and do not export the
> > register function.  
> 
> Does this mean that the divider file shouldn't export the divider
> registration function either? Please add a patch for that if it's the
> case. Otherwise this looks fine given that the code using this function
> is always built in.
> 

I will fix the divider in v2.

Michael

Re: [PATCH 3/5] clk: zynqmp: do not export zynqmp_clk_register_mux

[Michael Tretter]

On Tue, 12 Mar 2019 09:52:36 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 04:00:14)
> > zynqmp_clk_register_mux is an internal function of the driver that only
> > should be used from clkc.c. Therefore, there is no need to export that
> > function.
> > 
> > Follow the suit of the other files of the driver and do not export the
> > register function.  
> 
> Does this mean that the divider file shouldn't export the divider
> registration function either? Please add a patch for that if it's the
> case. Otherwise this looks fine given that the code using this function
> is always built in.
> 

I will fix the divider in v2.

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/5] clk: zynqmp: fix doc of __zynqmp_clock_get_parents

[Michal Simek]

On 12. 03. 19 12:00, Michael Tretter wrote:
> The kernel-doc refers to __zynqmp_clock_get_topology(), while it actually
> documents __zynqmp_clock_get_parents(). Fix the documentation to refer
> to the correct function name.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> I think most of the function documentation in this file is pretty
> useless and sometimes misleading. Maybe we should drop the documentation
> entirely, because all functions are static anyway?

Interesting is that kernel-doc is not able to find this issue.

drivers/clk/zynqmp/clkc.c:336: info: Scanning doc for
zynqmp_pm_clock_get_attributes
drivers/clk/zynqmp/clkc.c:360: info: Scanning doc for
__zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:388: info: Scanning doc for
zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:418: info: Scanning doc for
__zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:453: info: Scanning doc for
zynqmp_clock_get_parents
drivers/clk/zynqmp/clkc.c:485: info: Scanning doc for zynqmp_get_parent_list

I think that doc is good because it helps with understanding of code.
If there is something misleading that it should be rather fix then remove.

M

Re: [PATCH 2/5] clk: zynqmp: fix doc of __zynqmp_clock_get_parents

[Michal Simek]

On 12. 03. 19 12:00, Michael Tretter wrote:
> The kernel-doc refers to __zynqmp_clock_get_topology(), while it actually
> documents __zynqmp_clock_get_parents(). Fix the documentation to refer
> to the correct function name.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
> I think most of the function documentation in this file is pretty
> useless and sometimes misleading. Maybe we should drop the documentation
> entirely, because all functions are static anyway?

Interesting is that kernel-doc is not able to find this issue.

drivers/clk/zynqmp/clkc.c:336: info: Scanning doc for
zynqmp_pm_clock_get_attributes
drivers/clk/zynqmp/clkc.c:360: info: Scanning doc for
__zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:388: info: Scanning doc for
zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:418: info: Scanning doc for
__zynqmp_clock_get_topology
drivers/clk/zynqmp/clkc.c:453: info: Scanning doc for
zynqmp_clock_get_parents
drivers/clk/zynqmp/clkc.c:485: info: Scanning doc for zynqmp_get_parent_list

I think that doc is good because it helps with understanding of code.
If there is something misleading that it should be rather fix then remove.

M

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/5] clk: zynqmp: fix CLK_FRAC and various cleanups

[Michal Simek]

On 12. 03. 19 12:00, Michael Tretter wrote:
> Hello,
> 
> These are various patches for the ZynqMP clock driver.
> 
> Patch 1 fixes an erroneous test for the CLK_FRAC flag.
> 
> Patch 2 fixes a wrong function name in the kernel-doc. However, I think most
> of the kernel-doc for the static functions is pretty useless/misleading and
> maybe instead of fixing, we should just remove it.
> 
> Patches 3 to 5 are cleanup and refactoring and make the macros that are used
> to handle the responses from the firmware more consistent.
> 
> Michael
> 
> Michael Tretter (5):
>   clk: zynqmp: fix check for fractional clock
>   clk: zynqmp: fix doc of __zynqmp_clock_get_parents
>   clk: zynqmp: do not export zynqmp_clk_register_mux
>   clk: zynqmp: cleanup sizes of firmware responses
>   clk: zynqmp: make field definitions of query responses consistent
> 
>  drivers/clk/zynqmp/clk-mux-zynqmp.c |  1 -
>  drivers/clk/zynqmp/clk-zynqmp.h     |  6 ---
>  drivers/clk/zynqmp/clkc.c           | 71 +++++++++++++++--------------
>  drivers/clk/zynqmp/divider.c        |  2 +-
>  4 files changed, 38 insertions(+), 42 deletions(-)
> 

Jolly: Please take a look at these patches.

M

Re: [PATCH 0/5] clk: zynqmp: fix CLK_FRAC and various cleanups

[Michal Simek]

On 12. 03. 19 12:00, Michael Tretter wrote:
> Hello,
> 
> These are various patches for the ZynqMP clock driver.
> 
> Patch 1 fixes an erroneous test for the CLK_FRAC flag.
> 
> Patch 2 fixes a wrong function name in the kernel-doc. However, I think most
> of the kernel-doc for the static functions is pretty useless/misleading and
> maybe instead of fixing, we should just remove it.
> 
> Patches 3 to 5 are cleanup and refactoring and make the macros that are used
> to handle the responses from the firmware more consistent.
> 
> Michael
> 
> Michael Tretter (5):
>   clk: zynqmp: fix check for fractional clock
>   clk: zynqmp: fix doc of __zynqmp_clock_get_parents
>   clk: zynqmp: do not export zynqmp_clk_register_mux
>   clk: zynqmp: cleanup sizes of firmware responses
>   clk: zynqmp: make field definitions of query responses consistent
> 
>  drivers/clk/zynqmp/clk-mux-zynqmp.c |  1 -
>  drivers/clk/zynqmp/clk-zynqmp.h     |  6 ---
>  drivers/clk/zynqmp/clkc.c           | 71 +++++++++++++++--------------
>  drivers/clk/zynqmp/divider.c        |  2 +-
>  4 files changed, 38 insertions(+), 42 deletions(-)
> 

Jolly: Please take a look at these patches.

M

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 10:25:46)
> On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 04:00:12)
> > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > 
> > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > When registering the devider, these clkflags are copied to hw->flags.
> > > 
> > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > this check would never work.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/clk/zynqmp/divider.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > index a371c66e72ef..fc70950c1e24 100644
> > > --- a/drivers/clk/zynqmp/divider.c
> > > +++ b/drivers/clk/zynqmp/divider.c
> > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > >  
> > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > -           (divider->flags & CLK_FRAC))
> > > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> > 
> > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > not a clk framework flag so it shouldn't go there. Please fix the user
> > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > into divider::flags is not a good idea because that numberspace is for
> > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > just add a bool to the zynqmp_clk_divider?
> > 
> 
> Thanks. The driver sets the clk_hw::core::flags based on a response
> from the ATF and this response includes this flag with other clk
> frameworks flags. I can test for the flag when registering the clock
> and set another flag or a bool for the zynqmp_clk_divider and will do
> so in v2.

Cool. Thanks!

> 
> However, this merely sounds like a workaround for an issue in the ATF,
> which should not define and use this flag in the first place. 
> 

What is ATF doing with these flags? Hopefully ATF and the Linux kernel
aren't using the same numberspace to describe these things. For example,
I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
and passing that value from firmware to the kernel, blindly assuming
that the kernel wouldn't change those numbers to be something else.
Obviously that type of kernel change would be invasive but it's not an
ABI that we've ever published so we're free to do these sorts of things.

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 10:25:46)
> On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 04:00:12)
> > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > 
> > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > When registering the devider, these clkflags are copied to hw->flags.
> > > 
> > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > this check would never work.
> > > 
> > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > ---
> > >  drivers/clk/zynqmp/divider.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > index a371c66e72ef..fc70950c1e24 100644
> > > --- a/drivers/clk/zynqmp/divider.c
> > > +++ b/drivers/clk/zynqmp/divider.c
> > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > >  
> > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > -           (divider->flags & CLK_FRAC))
> > > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> > 
> > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > not a clk framework flag so it shouldn't go there. Please fix the user
> > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > into divider::flags is not a good idea because that numberspace is for
> > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > just add a bool to the zynqmp_clk_divider?
> > 
> 
> Thanks. The driver sets the clk_hw::core::flags based on a response
> from the ATF and this response includes this flag with other clk
> frameworks flags. I can test for the flag when registering the clock
> and set another flag or a bool for the zynqmp_clk_divider and will do
> so in v2.

Cool. Thanks!

> 
> However, this merely sounds like a workaround for an issue in the ATF,
> which should not define and use this flag in the first place. 
> 

What is ATF doing with these flags? Hopefully ATF and the Linux kernel
aren't using the same numberspace to describe these things. For example,
I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
and passing that value from firmware to the kernel, blindly assuming
that the kernel wouldn't change those numbers to be something else.
Obviously that type of kernel change would be invasive but it's not an
ABI that we've ever published so we're free to do these sorts of things.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/5] clk: zynqmp: cleanup sizes of firmware responses

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:15)
> @@ -248,7 +251,8 @@ static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
>         qdata.arg2 = index;
>  
>         ret = eemi_ops->query_data(qdata, ret_payload);
> -       memcpy(topology, &ret_payload[1], CLK_GET_TOPOLOGY_RESP_WORDS * 4);
> +       memcpy(topology, &ret_payload[1],
> +              CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));

It would be even clearer to have a set of structs that encoded what the
responses looked like, instead of having to unmarshal the response
somewhat manually on top of a u32 buffer.

So something like

	struct topology_resp {
		u32 node[3];
	};

and then just use sizeof(*topology_resp). Also, it's odd that we make a
stack local variable for the response in this function and then copy it
onto the stack again for the caller of this function
(zynqmp_clock_get_topology()) and then pass it to the unmarshal function
that copies it the third time into the struct clk_topology. It would be
better to do the copy at most once into some structure that has the data
all packed together and then use the macros to access the fields
wherever the response is used.

Re: [PATCH 4/5] clk: zynqmp: cleanup sizes of firmware responses

[Stephen Boyd]

Quoting Michael Tretter (2019-03-12 04:00:15)
> @@ -248,7 +251,8 @@ static int zynqmp_pm_clock_get_topology(u32 clock_id, u32 index, u32 *topology)
>         qdata.arg2 = index;
>  
>         ret = eemi_ops->query_data(qdata, ret_payload);
> -       memcpy(topology, &ret_payload[1], CLK_GET_TOPOLOGY_RESP_WORDS * 4);
> +       memcpy(topology, &ret_payload[1],
> +              CLK_GET_TOPOLOGY_RESP_WORDS * sizeof(*topology));

It would be even clearer to have a set of structs that encoded what the
responses looked like, instead of having to unmarshal the response
somewhat manually on top of a u32 buffer.

So something like

	struct topology_resp {
		u32 node[3];
	};

and then just use sizeof(*topology_resp). Also, it's odd that we make a
stack local variable for the response in this function and then copy it
onto the stack again for the caller of this function
(zynqmp_clock_get_topology()) and then pass it to the unmarshal function
that copies it the third time into the struct clk_topology. It would be
better to do the copy at most once into some structure that has the data
all packed together and then use the macros to access the fields
wherever the response is used.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 10:25:46)
> > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:  
> > > Quoting Michael Tretter (2019-03-12 04:00:12)  
> > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > 
> > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > 
> > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > this check would never work.
> > > > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > index a371c66e72ef..fc70950c1e24 100644
> > > > --- a/drivers/clk/zynqmp/divider.c
> > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > >  
> > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > -           (divider->flags & CLK_FRAC))
> > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))    
> > > 
> > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > into divider::flags is not a good idea because that numberspace is for
> > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > just add a bool to the zynqmp_clk_divider?
> > >   
> > 
> > Thanks. The driver sets the clk_hw::core::flags based on a response
> > from the ATF and this response includes this flag with other clk
> > frameworks flags. I can test for the flag when registering the clock
> > and set another flag or a bool for the zynqmp_clk_divider and will do
> > so in v2.  
> 
> Cool. Thanks!
> 
> > 
> > However, this merely sounds like a workaround for an issue in the ATF,
> > which should not define and use this flag in the first place. 
> >   
> 
> What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> aren't using the same numberspace to describe these things. For example,
> I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> and passing that value from firmware to the kernel, blindly assuming
> that the kernel wouldn't change those numbers to be something else.
> Obviously that type of kernel change would be invasive but it's not an
> ABI that we've ever published so we're free to do these sorts of things.

You mean that the ATF defines macros like

	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate change */
	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-parent */
	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up one level */

in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
response to the Linux driver, and the Linux driver copies the flags
that it got from the ATF to clk_hw::core::flags like

	init.flags = nodes->flag;

where nodes is the response from the ATF? That's exactly what is happening.

So instead of only translating CLK_FRAC, the driver should actually
translate all flags in the ATF response to proper clk framework flags
instead of blindly copying them, right?

Michael

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> Quoting Michael Tretter (2019-03-12 10:25:46)
> > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:  
> > > Quoting Michael Tretter (2019-03-12 04:00:12)  
> > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > 
> > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > 
> > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > this check would never work.
> > > > 
> > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > ---
> > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > index a371c66e72ef..fc70950c1e24 100644
> > > > --- a/drivers/clk/zynqmp/divider.c
> > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > @@ -117,7 +117,7 @@ static long zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > >  
> > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > -           (divider->flags & CLK_FRAC))
> > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))    
> > > 
> > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > into divider::flags is not a good idea because that numberspace is for
> > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > just add a bool to the zynqmp_clk_divider?
> > >   
> > 
> > Thanks. The driver sets the clk_hw::core::flags based on a response
> > from the ATF and this response includes this flag with other clk
> > frameworks flags. I can test for the flag when registering the clock
> > and set another flag or a bool for the zynqmp_clk_divider and will do
> > so in v2.  
> 
> Cool. Thanks!
> 
> > 
> > However, this merely sounds like a workaround for an issue in the ATF,
> > which should not define and use this flag in the first place. 
> >   
> 
> What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> aren't using the same numberspace to describe these things. For example,
> I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> and passing that value from firmware to the kernel, blindly assuming
> that the kernel wouldn't change those numbers to be something else.
> Obviously that type of kernel change would be invasive but it's not an
> ABI that we've ever published so we're free to do these sorts of things.

You mean that the ATF defines macros like

	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate change */
	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-parent */
	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up one level */

in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
response to the Linux driver, and the Linux driver copies the flags
that it got from the ATF to clk_hw::core::flags like

	init.flags = nodes->flag;

where nodes is the response from the ATF? That's exactly what is happening.

So instead of only translating CLK_FRAC, the driver should actually
translate all flags in the ATF response to proper clk framework flags
instead of blindly copying them, right?

Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-14 01:38:36)
> On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 10:25:46)
> > > 
> > > However, this merely sounds like a workaround for an issue in the ATF,
> > > which should not define and use this flag in the first place. 
> > >   
> > 
> > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > aren't using the same numberspace to describe these things. For example,
> > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > and passing that value from firmware to the kernel, blindly assuming
> > that the kernel wouldn't change those numbers to be something else.
> > Obviously that type of kernel change would be invasive but it's not an
> > ABI that we've ever published so we're free to do these sorts of things.
> 
> You mean that the ATF defines macros like
> 
>         #define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate change */
>         #define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-parent */
>         #define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up one level */
> 
> in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> response to the Linux driver, and the Linux driver copies the flags
> that it got from the ATF to clk_hw::core::flags like
> 
>         init.flags = nodes->flag;
> 
> where nodes is the response from the ATF? That's exactly what is happening.

Yes, that looks like a strong coupling between firmware and OS
implemented in a non-obvious way.

> 
> So instead of only translating CLK_FRAC, the driver should actually
> translate all flags in the ATF response to proper clk framework flags
> instead of blindly copying them, right?
> 

Sure. The CLK_FRAC will have to be filtered out anyway it sounds like.

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Stephen Boyd]

Quoting Michael Tretter (2019-03-14 01:38:36)
> On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 10:25:46)
> > > 
> > > However, this merely sounds like a workaround for an issue in the ATF,
> > > which should not define and use this flag in the first place. 
> > >   
> > 
> > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > aren't using the same numberspace to describe these things. For example,
> > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > and passing that value from firmware to the kernel, blindly assuming
> > that the kernel wouldn't change those numbers to be something else.
> > Obviously that type of kernel change would be invasive but it's not an
> > ABI that we've ever published so we're free to do these sorts of things.
> 
> You mean that the ATF defines macros like
> 
>         #define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate change */
>         #define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-parent */
>         #define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up one level */
> 
> in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> response to the Linux driver, and the Linux driver copies the flags
> that it got from the ATF to clk_hw::core::flags like
> 
>         init.flags = nodes->flag;
> 
> where nodes is the response from the ATF? That's exactly what is happening.

Yes, that looks like a strong coupling between firmware and OS
implemented in a non-obvious way.

> 
> So instead of only translating CLK_FRAC, the driver should actually
> translate all flags in the ATF response to proper clk framework flags
> instead of blindly copying them, right?
> 

Sure. The CLK_FRAC will have to be filtered out anyway it sounds like.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Jolly Shah]

Hi Michael,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Tuesday, March 12, 2019 4:00 AM
> To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> Subject: [PATCH 5/5] clk: zynqmp: make field definitions of query responses
> consistent
> 
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
> 
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
> 
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
> 
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)

GENMASK(23,8) here.

Thanks,
Jolly Shah

> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
> 
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
> 
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
> 
> -#define CLK_VALID_MASK			0x1
> -
>  #define CLK_GET_NAME_RESP_LEN		16
>  #define CLK_GET_TOPOLOGY_RESP_WORDS	3
>  #define CLK_GET_PARENTS_RESP_WORDS	3
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct
> clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
> 
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
> +		if (type == TYPE_INVALID)
>  			return END_OF_TOPOLOGY_NODE;
> -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> -						   data[i]);
> +		topology[*nnodes].type = type
> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS,
> data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK,
> data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> data[i]);
>  		(*nnodes)++;
>  	}
> 
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct
> clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
> 
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[i] == DUMMY_PARENT) {
>  			strcpy(parent->name, "dummy_name");
>  			parent->flag = 0;
>  		} else {
> -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS,
> data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent-
> >name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
> 
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
> 
>  	/* Get topology of all clock */
> --
> 2.20.1

RE: [PATCH 5/5] clk: zynqmp: make field definitions of query responses consistent

[Jolly Shah]

Hi Michael,

> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Tuesday, March 12, 2019 4:00 AM
> To: linux-clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org
> Cc: kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Stephen Boyd <sboyd@kernel.org>; Michal Simek <michals@xilinx.com>; Jolly
> Shah <JOLLYS@xilinx.com>; Michael Tretter <m.tretter@pengutronix.de>
> Subject: [PATCH 5/5] clk: zynqmp: make field definitions of query responses
> consistent
> 
> The definition of the fields in the firmware responses to the queries is
> inconsistent. Some are specified as a mask, some as a shift, and some by
> the length of the previous field.
> 
> Specify all of them as bit masks and access the fields using the
> FIELD_GET macro.
> 
> Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> ---
>  drivers/clk/zynqmp/clkc.c | 39 ++++++++++++++++++---------------------
>  1 file changed, 18 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/clk/zynqmp/clkc.c b/drivers/clk/zynqmp/clkc.c
> index 573c3c58dbbc..b4e13b8618b2 100644
> --- a/drivers/clk/zynqmp/clkc.c
> +++ b/drivers/clk/zynqmp/clkc.c
> @@ -21,20 +21,18 @@
>  #define MAX_NODES			6
>  #define MAX_NAME_LEN			50
> 
> -#define CLK_TYPE_SHIFT			2
> -
>  #define NA_PARENT			0xFFFFFFFF
>  #define DUMMY_PARENT			0xFFFFFFFE
> 
> -#define CLK_TYPE_FIELD_LEN		4
> -#define CLK_TOPOLOGY_NODE_OFFSET	16
> +#define CLK_TOPOLOGY_TYPE		GENMASK(3, 0)
> +#define CLK_TOPOLOGY_FLAGS		GENMASK(21, 8)
> +#define CLK_TOPOLOGY_TYPE_FLAGS		GENMASK(31, 24)
> 
> -#define CLK_TYPE_FIELD_MASK		0xF
> -#define CLK_FLAG_FIELD_MASK		GENMASK(21, 8)

GENMASK(23,8) here.

Thanks,
Jolly Shah

> -#define CLK_TYPE_FLAG_FIELD_MASK	GENMASK(31, 24)
> +#define CLK_PARENTS_ID			GENMASK(15, 0)
> +#define CLK_PARENTS_FLAGS		GENMASK(31, 16)
> 
> -#define CLK_PARENTS_ID_LEN		16
> -#define CLK_PARENTS_ID_MASK		0xFFFF
> +#define CLK_ATTR_VALID			BIT(0)
> +#define CLK_ATTR_TYPE			BIT(2)
> 
>  /* Flags for parents */
>  #define PARENT_CLK_SELF			0
> @@ -49,8 +47,6 @@
>  #define END_OF_PARENTS			1
>  #define RESERVED_CLK_NAME		""
> 
> -#define CLK_VALID_MASK			0x1
> -
>  #define CLK_GET_NAME_RESP_LEN		16
>  #define CLK_GET_TOPOLOGY_RESP_WORDS	3
>  #define CLK_GET_PARENTS_RESP_WORDS	3
> @@ -369,15 +365,16 @@ static int __zynqmp_clock_get_topology(struct
> clock_topology *topology,
>  				       u32 *data, u32 *nnodes)
>  {
>  	int i;
> +	u32 type;
> 
>  	for (i = 0; i < CLK_GET_TOPOLOGY_RESP_WORDS; i++) {
> -		if (!(data[i] & CLK_TYPE_FIELD_MASK))
> +		type = FIELD_GET(CLK_TOPOLOGY_TYPE, data[i]);
> +		if (type == TYPE_INVALID)
>  			return END_OF_TOPOLOGY_NODE;
> -		topology[*nnodes].type = data[i] & CLK_TYPE_FIELD_MASK;
> -		topology[*nnodes].flag = FIELD_GET(CLK_FLAG_FIELD_MASK,
> -						   data[i]);
> +		topology[*nnodes].type = type
> +		topology[*nnodes].flag = FIELD_GET(CLK_TOPOLOGY_FLAGS,
> data[i]);
>  		topology[*nnodes].type_flag =
> -				FIELD_GET(CLK_TYPE_FLAG_FIELD_MASK,
> data[i]);
> +				FIELD_GET(CLK_TOPOLOGY_TYPE_FLAGS,
> data[i]);
>  		(*nnodes)++;
>  	}
> 
> @@ -433,12 +430,12 @@ static int __zynqmp_clock_get_parents(struct
> clock_parent *parents, u32 *data,
>  			return END_OF_PARENTS;
> 
>  		parent = &parents[i];
> -		parent->id = data[i] & CLK_PARENTS_ID_MASK;
> +		parent->id = FIELD_GET(CLK_PARENTS_ID, data[i]);
>  		if (data[i] == DUMMY_PARENT) {
>  			strcpy(parent->name, "dummy_name");
>  			parent->flag = 0;
>  		} else {
> -			parent->flag = data[i] >> CLK_PARENTS_ID_LEN;
> +			parent->flag = FIELD_GET(CLK_PARENTS_FLAGS,
> data[i]);
>  			if (zynqmp_get_clock_name(parent->id, parent-
> >name))
>  				continue;
>  		}
> @@ -638,9 +635,9 @@ static void zynqmp_get_clock_info(void)
>  		if (ret)
>  			continue;
> 
> -		clock[i].valid = attr & CLK_VALID_MASK;
> -		clock[i].type = attr >> CLK_TYPE_SHIFT ? CLK_TYPE_EXTERNAL :
> -							CLK_TYPE_OUTPUT;
> +		clock[i].valid = FIELD_GET(CLK_ATTR_VALID, attr);
> +		clock[i].type = FIELD_GET(CLK_ATTR_TYPE) ?
> +			CLK_TYPE_EXTERNAL : CLK_TYPE_OUTPUT;
>  	}
> 
>  	/* Get topology of all clock */
> --
> 2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Jolly Shah]

Hi Michael,


> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Thursday, March 14, 2019 1:39 AM
> To: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>
> Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock
> 
> On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 10:25:46)
> > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> > > > Quoting Michael Tretter (2019-03-12 04:00:12)
> > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > >
> > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > >
> > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > > this check would never work.
> > > > >
> > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > > ---
> > > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > > index a371c66e72ef..fc70950c1e24 100644
> > > > > --- a/drivers/clk/zynqmp/divider.c
> > > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > > @@ -117,7 +117,7 @@ static long
> zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > > >
> > > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > > -           (divider->flags & CLK_FRAC))
> > > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))
> > > >
> > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > > into divider::flags is not a good idea because that numberspace is for
> > > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > > just add a bool to the zynqmp_clk_divider?
> > > >
> > >
> > > Thanks. The driver sets the clk_hw::core::flags based on a response
> > > from the ATF and this response includes this flag with other clk
> > > frameworks flags. I can test for the flag when registering the clock
> > > and set another flag or a bool for the zynqmp_clk_divider and will do
> > > so in v2.
> >
> > Cool. Thanks!
> >
> > >
> > > However, this merely sounds like a workaround for an issue in the ATF,
> > > which should not define and use this flag in the first place.
> > >
> >
> > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > aren't using the same numberspace to describe these things. For example,
> > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > and passing that value from firmware to the kernel, blindly assuming
> > that the kernel wouldn't change those numbers to be something else.
> > Obviously that type of kernel change would be invasive but it's not an
> > ABI that we've ever published so we're free to do these sorts of things.
> 
> You mean that the ATF defines macros like
> 
> 	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate
> change */
> 	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-
> parent */
> 	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up
> one level */
> 
> in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> response to the Linux driver, and the Linux driver copies the flags
> that it got from the ATF to clk_hw::core::flags like
> 
> 	init.flags = nodes->flag;
> 
> where nodes is the response from the ATF? That's exactly what is happening.
> 
> So instead of only translating CLK_FRAC, the driver should actually
> translate all flags in the ATF response to proper clk framework flags
> instead of blindly copying them, right?

Except CLK_FRAC, all flags are defined same as clk framework. I think we should add custom flags field and pass zynqmp specific flags there.

Thanks,
Jolly Shah

> 
> Michael

RE: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Jolly Shah]

Hi Michael,


> -----Original Message-----
> From: Michael Tretter <m.tretter@pengutronix.de>
> Sent: Thursday, March 14, 2019 1:39 AM
> To: Stephen Boyd <sboyd@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>
> Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock
> 
> On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:
> > Quoting Michael Tretter (2019-03-12 10:25:46)
> > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:
> > > > Quoting Michael Tretter (2019-03-12 04:00:12)
> > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > >
> > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > >
> > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > > this check would never work.
> > > > >
> > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > > ---
> > > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > > index a371c66e72ef..fc70950c1e24 100644
> > > > > --- a/drivers/clk/zynqmp/divider.c
> > > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > > @@ -117,7 +117,7 @@ static long
> zynqmp_clk_divider_round_rate(struct clk_hw *hw,
> > > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > > >
> > > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > > -           (divider->flags & CLK_FRAC))
> > > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))
> > > >
> > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > > into divider::flags is not a good idea because that numberspace is for
> > > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > > just add a bool to the zynqmp_clk_divider?
> > > >
> > >
> > > Thanks. The driver sets the clk_hw::core::flags based on a response
> > > from the ATF and this response includes this flag with other clk
> > > frameworks flags. I can test for the flag when registering the clock
> > > and set another flag or a bool for the zynqmp_clk_divider and will do
> > > so in v2.
> >
> > Cool. Thanks!
> >
> > >
> > > However, this merely sounds like a workaround for an issue in the ATF,
> > > which should not define and use this flag in the first place.
> > >
> >
> > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > aren't using the same numberspace to describe these things. For example,
> > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > and passing that value from firmware to the kernel, blindly assuming
> > that the kernel wouldn't change those numbers to be something else.
> > Obviously that type of kernel change would be invasive but it's not an
> > ABI that we've ever published so we're free to do these sorts of things.
> 
> You mean that the ATF defines macros like
> 
> 	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate
> change */
> 	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-
> parent */
> 	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up
> one level */
> 
> in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> response to the Linux driver, and the Linux driver copies the flags
> that it got from the ATF to clk_hw::core::flags like
> 
> 	init.flags = nodes->flag;
> 
> where nodes is the response from the ATF? That's exactly what is happening.
> 
> So instead of only translating CLK_FRAC, the driver should actually
> translate all flags in the ATF response to proper clk framework flags
> instead of blindly copying them, right?

Except CLK_FRAC, all flags are defined same as clk framework. I think we should add custom flags field and pass zynqmp specific flags there.

Thanks,
Jolly Shah

> 
> Michael

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Tue, 19 Mar 2019 00:56:31 +0000, Jolly Shah wrote:
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@pengutronix.de>
> > Sent: Thursday, March 14, 2019 1:39 AM
> > To: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> > kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> > Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>
> > Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock
> > 
> > On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:  
> > > Quoting Michael Tretter (2019-03-12 10:25:46)  
> > > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:  
> > > > > Quoting Michael Tretter (2019-03-12 04:00:12)  
> > > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > > >
> > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > > >
> > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > > > this check would never work.
> > > > > >
> > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > > > index a371c66e72ef..fc70950c1e24 100644
> > > > > > --- a/drivers/clk/zynqmp/divider.c
> > > > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > > > @@ -117,7 +117,7 @@ static long  
> > zynqmp_clk_divider_round_rate(struct clk_hw *hw,  
> > > > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > > > >
> > > > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > > > -           (divider->flags & CLK_FRAC))
> > > > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> > > > >
> > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > > > into divider::flags is not a good idea because that numberspace is for
> > > > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > > > just add a bool to the zynqmp_clk_divider?
> > > > >  
> > > >
> > > > Thanks. The driver sets the clk_hw::core::flags based on a response
> > > > from the ATF and this response includes this flag with other clk
> > > > frameworks flags. I can test for the flag when registering the clock
> > > > and set another flag or a bool for the zynqmp_clk_divider and will do
> > > > so in v2.  
> > >
> > > Cool. Thanks!
> > >  
> > > >
> > > > However, this merely sounds like a workaround for an issue in the ATF,
> > > > which should not define and use this flag in the first place.
> > > >  
> > >
> > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > > aren't using the same numberspace to describe these things. For example,
> > > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > > and passing that value from firmware to the kernel, blindly assuming
> > > that the kernel wouldn't change those numbers to be something else.
> > > Obviously that type of kernel change would be invasive but it's not an
> > > ABI that we've ever published so we're free to do these sorts of things.  
> > 
> > You mean that the ATF defines macros like
> > 
> > 	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate
> > change */
> > 	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-
> > parent */
> > 	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up
> > one level */
> > 
> > in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> > response to the Linux driver, and the Linux driver copies the flags
> > that it got from the ATF to clk_hw::core::flags like
> > 
> > 	init.flags = nodes->flag;
> > 
> > where nodes is the response from the ATF? That's exactly what is happening.
> > 
> > So instead of only translating CLK_FRAC, the driver should actually
> > translate all flags in the ATF response to proper clk framework flags
> > instead of blindly copying them, right?  
> 
> Except CLK_FRAC, all flags are defined same as clk framework. I think
> we should add custom flags field and pass zynqmp specific flags there.

Using the same flags in the ATF and the common clk framework works now,
because they have the same value. However, the flags are not actually
the same, because the flags that the driver gets from the ATF are
defined in the ATF and the flags that the driver passes to the common
clk framework are defined in the common clk framework. If the flags in
the common clk framework are changed (which is admittedly unlikely), the
ZynqMP clock driver will break, because it still assumes that the
common clk framework uses the same flags as defined in the ATF.

Therefore, the driver should actually decouple the flags by defining
its own flags that correspond to the flags as defined by the ATF (i.e.
platform specific flags) and convert these flags into flags for the
common clk framework when registering the clocks.

The problem is that two things that have the same value are not
necessarily the same thing.

Michael

> 
> Thanks,
> Jolly Shah
> 
> > 
> > Michael  
> 

Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock

[Michael Tretter]

On Tue, 19 Mar 2019 00:56:31 +0000, Jolly Shah wrote:
> > -----Original Message-----
> > From: Michael Tretter <m.tretter@pengutronix.de>
> > Sent: Thursday, March 14, 2019 1:39 AM
> > To: Stephen Boyd <sboyd@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org; linux-clk@vger.kernel.org;
> > kernel@pengutronix.de; Michael Turquette <mturquette@baylibre.com>;
> > Michal Simek <michals@xilinx.com>; Jolly Shah <JOLLYS@xilinx.com>
> > Subject: Re: [PATCH 1/5] clk: zynqmp: fix check for fractional clock
> > 
> > On Wed, 13 Mar 2019 09:24:04 -0700, Stephen Boyd wrote:  
> > > Quoting Michael Tretter (2019-03-12 10:25:46)  
> > > > On Tue, 12 Mar 2019 09:49:21 -0700, Stephen Boyd wrote:  
> > > > > Quoting Michael Tretter (2019-03-12 04:00:12)  
> > > > > > CLK_FRAC is not set in the divider->flags, but in the hw->flags.
> > > > > >
> > > > > > The firmware sets CLK_FRAC for fractional clocks in the clkflag field.
> > > > > > When registering the devider, these clkflags are copied to hw->flags.
> > > > > >
> > > > > > Moreover, divider->flags field is a u8 type, but CLK_FRAG is BIT(13). So
> > > > > > this check would never work.
> > > > > >
> > > > > > Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
> > > > > > ---
> > > > > >  drivers/clk/zynqmp/divider.c | 2 +-
> > > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/drivers/clk/zynqmp/divider.c b/drivers/clk/zynqmp/divider.c
> > > > > > index a371c66e72ef..fc70950c1e24 100644
> > > > > > --- a/drivers/clk/zynqmp/divider.c
> > > > > > +++ b/drivers/clk/zynqmp/divider.c
> > > > > > @@ -117,7 +117,7 @@ static long  
> > zynqmp_clk_divider_round_rate(struct clk_hw *hw,  
> > > > > >         bestdiv = zynqmp_divider_get_val(*prate, rate);
> > > > > >
> > > > > >         if ((clk_hw_get_flags(hw) & CLK_SET_RATE_PARENT) &&
> > > > > > -           (divider->flags & CLK_FRAC))
> > > > > > +           (clk_hw_get_flags(hw) & CLK_FRAC))  
> > > > >
> > > > > CLK_FRAC shouldn't be set in the struct clk_hw::core::flags field. It's
> > > > > not a clk framework flag so it shouldn't go there. Please fix the user
> > > > > of this flag to place the CLK_FRAC flag somewhere else. Even adding it
> > > > > into divider::flags is not a good idea because that numberspace is for
> > > > > dividers, and this flag seems to be zynqmp driver specific, so maybe
> > > > > just add a bool to the zynqmp_clk_divider?
> > > > >  
> > > >
> > > > Thanks. The driver sets the clk_hw::core::flags based on a response
> > > > from the ATF and this response includes this flag with other clk
> > > > frameworks flags. I can test for the flag when registering the clock
> > > > and set another flag or a bool for the zynqmp_clk_divider and will do
> > > > so in v2.  
> > >
> > > Cool. Thanks!
> > >  
> > > >
> > > > However, this merely sounds like a workaround for an issue in the ATF,
> > > > which should not define and use this flag in the first place.
> > > >  
> > >
> > > What is ATF doing with these flags? Hopefully ATF and the Linux kernel
> > > aren't using the same numberspace to describe these things. For example,
> > > I would be concerned if ATF was looking at the CLK_SET_RATE_PARENT flag
> > > and passing that value from firmware to the kernel, blindly assuming
> > > that the kernel wouldn't change those numbers to be something else.
> > > Obviously that type of kernel change would be invasive but it's not an
> > > ABI that we've ever published so we're free to do these sorts of things.  
> > 
> > You mean that the ATF defines macros like
> > 
> > 	#define CLK_SET_RATE_GATE       BIT(0) /* must be gated across rate
> > change */
> > 	#define CLK_SET_PARENT_GATE     BIT(1) /* must be gated across re-
> > parent */
> > 	#define CLK_SET_RATE_PARENT     BIT(2) /* propagate rate change up
> > one level */
> > 
> > in plat/xilinx/zynqmp/pm_service/pm_api_clock.h, sets the flags in the
> > response to the Linux driver, and the Linux driver copies the flags
> > that it got from the ATF to clk_hw::core::flags like
> > 
> > 	init.flags = nodes->flag;
> > 
> > where nodes is the response from the ATF? That's exactly what is happening.
> > 
> > So instead of only translating CLK_FRAC, the driver should actually
> > translate all flags in the ATF response to proper clk framework flags
> > instead of blindly copying them, right?  
> 
> Except CLK_FRAC, all flags are defined same as clk framework. I think
> we should add custom flags field and pass zynqmp specific flags there.

Using the same flags in the ATF and the common clk framework works now,
because they have the same value. However, the flags are not actually
the same, because the flags that the driver gets from the ATF are
defined in the ATF and the flags that the driver passes to the common
clk framework are defined in the common clk framework. If the flags in
the common clk framework are changed (which is admittedly unlikely), the
ZynqMP clock driver will break, because it still assumes that the
common clk framework uses the same flags as defined in the ATF.

Therefore, the driver should actually decouple the flags by defining
its own flags that correspond to the flags as defined by the ATF (i.e.
platform specific flags) and convert these flags into flags for the
common clk framework when registering the clocks.

The problem is that two things that have the same value are not
necessarily the same thing.

Michael

> 
> Thanks,
> Jolly Shah
> 
> > 
> > Michael  
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel