[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7b57ccc2-7060-4adf-b896-8992ec05125c@linaro.org>
Date: Wed, 30 Oct 2024 10:28:14 +0200
From: Eugen Hristev <eugen.hristev@...aro.org>
To: Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org
Cc: andersson@...nel.org, konradybcio@...nel.org,
linux-kernel@...r.kernel.org, linux-clk@...r.kernel.org,
linux-pm@...r.kernel.org, djakov@...nel.org, mturquette@...libre.com,
evgreen@...omium.org
Subject: Re: [PATCH v2] soc: qcom: Rework BCM_TCS_CMD 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
Powered by blists - more mailing lists