On 10/30/24 02:40, Stephen Boyd wrote:
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?
This review [1] from Evan Green on the original patch submission
requested the use of cpu_to_le32
So that's how it ended up there.
[1]
https://lore.kernel.org/linux-kernel//20180806225252.GQ30024@minitux/T/#mab6b799b3f9b51725c804a65f3580ef8894205f2
