Message-ID: <f6f171aed953b5ff8080290b1cef9f80.sboyd@kernel.org>
Date: Tue, 19 Nov 2024 15:32:24 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: Eugen Hristev <eugen.hristev@...aro.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

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.

Powered by blists - more mailing lists