| Message ID | 20241028163403.522001-1-eugen.hristev@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] soc: qcom: Rework BCM_TCS_CMD macro | expand |
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?
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
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?
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
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(-)