diff mbox series

[v2] soc: qcom: Rework BCM_TCS_CMD macro

Message ID 20241028163403.522001-1-eugen.hristev@linaro.org
State New
Headers show
Series [v2] soc: qcom: Rework BCM_TCS_CMD macro | expand

Commit Message

Eugen Hristev Oct. 28, 2024, 4:34 p.m. UTC
Reworked BCM_TCS_CMD macro in order to fix warnings from sparse:

drivers/clk/qcom/clk-rpmh.c:270:28: warning: restricted __le32 degrades to integer
drivers/clk/qcom/clk-rpmh.c:270:28: warning: restricted __le32 degrades to integer

While at it, used le32_encode_bits which made the code easier to
follow and removed unnecessary shift definitions.

Signed-off-by: Eugen Hristev <eugen.hristev@linaro.org>
---
 drivers/clk/qcom/clk-rpmh.c           |  2 +-
 drivers/interconnect/qcom/bcm-voter.c |  2 +-
 include/soc/qcom/tcs.h                | 28 +++++++++++++--------------
 3 files changed, 16 insertions(+), 16 deletions(-)

Comments

Stephen Boyd Oct. 30, 2024, 12:40 a.m. UTC | #1
Quoting Eugen Hristev (2024-10-29 06:12:12)
> On 10/28/24 19:56, Stephen Boyd wrote:
> > Quoting Eugen Hristev (2024-10-28 09:34:03)
> >> diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
> >> index 3acca067c72b..152947a922c0 100644
> >> --- a/include/soc/qcom/tcs.h
> >> +++ b/include/soc/qcom/tcs.h
[....]
> >>   /* Construct a Bus Clock Manager (BCM) specific TCS command */
> >>   #define BCM_TCS_CMD(commit, valid, vote_x, vote_y)             \
> >> -       (((commit) << BCM_TCS_CMD_COMMIT_SHFT) |                \
> >> -       ((valid) << BCM_TCS_CMD_VALID_SHFT) |                   \
> >> -       ((cpu_to_le32(vote_x) &                                 \
> >> -       BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_X_SHFT) |    \
> >> -       ((cpu_to_le32(vote_y) &                                 \
> >> -       BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_Y_SHFT))
> >> +       (le32_encode_bits(commit, BCM_TCS_CMD_COMMIT_MASK) |    \
> >> +       le32_encode_bits(valid, BCM_TCS_CMD_VALID_MASK) |       \
> >> +       le32_encode_bits(vote_x,        \
> >> +                       BCM_TCS_CMD_VOTE_X_MASK) |              \
> >> +       le32_encode_bits(vote_y,        \
> >> +                       BCM_TCS_CMD_VOTE_Y_MASK))
> > 
> > Why is cpu_to_le32() inside BCM_TCS_CMD at all? Is struct tcs_cmd::data
> > supposed to be marked as __le32?
> > 
> > Can the whole u32 be constructed and turned into an __le32 after setting
> > all the bit fields instead of using le32_encode_bits() multiple times?
> 
> I believe no. The fields inside the constructed TCS command should be 
> little endian. If we construct the whole u32 and then convert it from 
> cpu endinaness to little endian, this might prove to be incorrect as it 
> would swap the bytes at the u32 level, while originally, the bytes for 
> each field that was longer than 1 byte were swapped before being added 
> to the constructed u32.
> So I would say that the fields inside the constructed item are indeed 
> le32, but the result as a whole is an u32 which would be sent to the 
> hardware using an u32 container , and no byte swapping should be done 
> there, as the masks already place the fields at the required offsets.
> So the tcs_cmd.data is not really a le32, at least my acception of it.
> Does this make sense ?
> 

Sort of? But I thought that the RPMh hardware was basically 32-bit
little-endian registers. That's why write_tcs_*() APIs in
drivers/soc/qcom/rpmh-rsc.c use writel() and readl(), right? The
cpu_to_le32() code that's there today is doing nothing, because the CPU
is little-endian 99% of the time. It's likely doing the wrong thing on
big-endian machines. Looking at commit 6311b6521bcc ("drivers: qcom: Add
BCM vote macro to header") it seems to have picked the macro version
from interconnect vs. clk subsystem. And commit b5d2f741077a
("interconnect: qcom: Add sdm845 interconnect provider driver") used
cpu_to_le32() but I can't figure out why.

If the rpmh-rsc code didn't use writel() or readl() I'd believe that the
data member is simply a u32 container. But those writel() and readl()
functions are doing a byte swap, which seems to imply that the data
member is a native CPU endian u32 that needs to be converted to
little-endian. Sounds like BCM_TCS_CMD() should just pack things into a
u32 and we can simply remove the cpu_to_l32() stuff in the macro?
diff mbox series

Patch

diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
index 4acde937114a..4929893b09c2 100644
--- a/drivers/clk/qcom/clk-rpmh.c
+++ b/drivers/clk/qcom/clk-rpmh.c
@@ -267,7 +267,7 @@  static int clk_rpmh_bcm_send_cmd(struct clk_rpmh *c, bool enable)
 
 	if (c->last_sent_aggr_state != cmd_state) {
 		cmd.addr = c->res_addr;
-		cmd.data = BCM_TCS_CMD(1, enable, 0, cmd_state);
+		cmd.data = (__force u32)BCM_TCS_CMD(1, enable, 0, cmd_state);
 
 		/*
 		 * Send only an active only state request. RPMh continues to
diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
index a2d437a05a11..ce9091cf122b 100644
--- a/drivers/interconnect/qcom/bcm-voter.c
+++ b/drivers/interconnect/qcom/bcm-voter.c
@@ -144,7 +144,7 @@  static inline void tcs_cmd_gen(struct tcs_cmd *cmd, u64 vote_x, u64 vote_y,
 		vote_y = BCM_TCS_CMD_VOTE_MASK;
 
 	cmd->addr = addr;
-	cmd->data = BCM_TCS_CMD(commit, valid, vote_x, vote_y);
+	cmd->data = (__force u32)BCM_TCS_CMD(commit, valid, vote_x, vote_y);
 
 	/*
 	 * Set the wait for completion flag on command that need to be completed
diff --git a/include/soc/qcom/tcs.h b/include/soc/qcom/tcs.h
index 3acca067c72b..152947a922c0 100644
--- a/include/soc/qcom/tcs.h
+++ b/include/soc/qcom/tcs.h
@@ -6,6 +6,9 @@ 
 #ifndef __SOC_QCOM_TCS_H__
 #define __SOC_QCOM_TCS_H__
 
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+
 #define MAX_RPMH_PAYLOAD	16
 
 /**
@@ -60,22 +63,19 @@  struct tcs_request {
 	struct tcs_cmd *cmds;
 };
 
-#define BCM_TCS_CMD_COMMIT_SHFT		30
-#define BCM_TCS_CMD_COMMIT_MASK		0x40000000
-#define BCM_TCS_CMD_VALID_SHFT		29
-#define BCM_TCS_CMD_VALID_MASK		0x20000000
-#define BCM_TCS_CMD_VOTE_X_SHFT		14
-#define BCM_TCS_CMD_VOTE_MASK		0x3fff
-#define BCM_TCS_CMD_VOTE_Y_SHFT		0
-#define BCM_TCS_CMD_VOTE_Y_MASK		0xfffc000
+#define BCM_TCS_CMD_COMMIT_MASK		BIT(30)
+#define BCM_TCS_CMD_VALID_MASK		BIT(29)
+#define BCM_TCS_CMD_VOTE_MASK		GENMASK(13, 0)
+#define BCM_TCS_CMD_VOTE_Y_MASK		GENMASK(13, 0)
+#define BCM_TCS_CMD_VOTE_X_MASK		GENMASK(27, 14)
 
 /* Construct a Bus Clock Manager (BCM) specific TCS command */
 #define BCM_TCS_CMD(commit, valid, vote_x, vote_y)		\
-	(((commit) << BCM_TCS_CMD_COMMIT_SHFT) |		\
-	((valid) << BCM_TCS_CMD_VALID_SHFT) |			\
-	((cpu_to_le32(vote_x) &					\
-	BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_X_SHFT) |	\
-	((cpu_to_le32(vote_y) &					\
-	BCM_TCS_CMD_VOTE_MASK) << BCM_TCS_CMD_VOTE_Y_SHFT))
+	(le32_encode_bits(commit, BCM_TCS_CMD_COMMIT_MASK) |	\
+	le32_encode_bits(valid, BCM_TCS_CMD_VALID_MASK) |	\
+	le32_encode_bits(vote_x,	\
+			BCM_TCS_CMD_VOTE_X_MASK) |		\
+	le32_encode_bits(vote_y,	\
+			BCM_TCS_CMD_VOTE_Y_MASK))
 
 #endif /* __SOC_QCOM_TCS_H__ */