[v2] soc: qcom: Rework BCM_TCS_CMD macro

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(-)

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.

Quoting Eugen Hristev (2024-11-11 05:05:02)
> 
> 
> On 11/8/24 21:00, Stephen Boyd wrote:
> > 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.
> 
> If the values are in the wrong endianness, e.g. 0xff11 instead of 
> 0x11ff, the corresponding field would be filled up wrongly, even 
> possibly writing unwanted bits. vote_x and vote_y have a mask of length 
> 14, so there is one byte and another 6 more bits. If the endianness of 
> the value is not correct, the one byte might end up written over the 6 
> bits and 2 extra bits which are supposed to be for another field.
> In my example 0x11 should be in the first 6 bits and the 0xff in the 
> next byte, but if the endianness of the cpu is different, we might write 
> 0xff on the 6 bit field.
> So we must ensure that the multi-byte fields are in the correct 
> endianness that the hardware expects.

The vote_x field looks like it goes from bits 14 to 27. No matter the
endian format of the _word_, bits 14 to 27 are for the vote_x field. So
if I set bit 15 and 17 in a big-endian format word (vote_x is decimal
10), pass that 32-bit word to writel(), swap the bytes, it is still set
as bit 15 and 17 in little-endian format. That's because when I read the
32-bit word as a little-endian machine, the first byte is for bits 0 to
7, second byte is for bits 8 to 15, third byte is for 16 to 23, and
fourth byte is for bits 24 to 31. The hardware assembles the 32-bit word
out of that byte order, knowing that bits are mapped that way. Once I
have the 32-bit word, it can be shifted right 14 times so that the
vote_x field is from 0 to 13, and bits 1 and 3 are set, i.e. decimal 10.

Fields that span bytes don't matter here. The hardware is going to read
the word in the format that it is in, which is the order of bytes, and
assemble a full 32-bit word out of it. We're just setting bits in the
field that's shifted so many bits because it's part of a word. The order
of the bits isn't changing.

> 
> In other words, writel does not know about the multi-byte fields inside 
> this u32 which have a specific bit shift, and those fields are expected 
> to be in le32 order written to the hardware. Whether or not the cpu is 
> le32 is not important because using cpu_to_le32 will make it safe either 
> way.
> 
> I apologize for my not so great explanation
> 

I hope my explanation has helped. Long story short, the cpu_to_le32()
usage here is wrong. Typically we try to operate with a type that's the
same size and native format for as long as possible (u32), partially for
performance reasons but also to make it easier to understand. When it
comes time to write that value to the hardware, write it in the hardware
format (writel), and read from the hardware in the hardware format
(readl). Doing this lets you avoid thinking about the endianness almost
entirely, and makes it so that code doesn't have to be rewritten when
running on a different endian CPU to avoid suffering performance
penalties with all the byte swapping.