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
@@ -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))
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 ?
Eugen
