Re: [PATCH v2] soc: qcom: Rework BCM_TCS_CMD macro

[Eugen Hristev]

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