| Message ID | 20241028163403.522001-1-eugen.hristev@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] soc: qcom: Rework BCM_TCS_CMD macro | expand |
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?
Quoting Eugen Hristev (2024-10-30 01:28:14) > On 10/30/24 02:40, Stephen Boyd wrote: > > > > 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. > Thanks. I still don't see why this can't just be treated as a u32 and then we have writel() take care of it for us.
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__ */
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(-)