On Tue, Jul 21, 2026 at 6:05 PM Quentin Schulz <[email protected]> wrote: > > Hi Alexey, > > On 7/21/26 3:34 PM, Alexey Charkov wrote: > > Hi Quentin, > > > > On Tue, Jul 21, 2026 at 4:56 PM Quentin Schulz <[email protected]> > > wrote: > >> > >> Hi Alexey, > >> > >> On 7/13/26 8:35 PM, Alexey Charkov wrote: > >>> Current code calculates the fractional component in 32 bits before > >>> assigning it to a 64-bit holding variable, causing overflow for real-world > >>> values of k, given that OSC_HZ is 24000000U. It also does bitwise manual > >>> massaging of an unsigned representation of what is actually a two's > >>> complement signed value, which is confusing and makes the code harder to > >>> read. > >>> > >>> Read k into a properly signed type and promote operands to avoid overflow, > >> > >> I'm not sure this is correct? Converting an unsigned integer (readl > >> returns an u32 and con is a u32) to signed (k is s16) is > >> implementation-defined as far as I understood (c.f. > >> https://en.cppreference.com/c/language/conversion). I'm sure I > >> misunderstood the spec but considering signed integer overflow is > >> undefined (and I guess one could understand casting a u32 storing a > >> number bigger than S16_MAX into an s16 to be some kind of overflow), I'm > >> a bit concerned here. I probably forgot important stuff I learned a > >> decade ago :) Can you point me where/what I misunderstood? > > > > It is indeed implementation defined in the C standard, but given that > > U-Boot enforces the gnu11 convention, it's defined to be reduction > > modulo 2^16 [1] along with two's complement representation, which is > > exactly what we need here. > > > > That is helpful thank you! I was sure I was missing something as I don't > think we explicitly handle this anywhere in U-Boot or the Linux kernel > :) A few more related questions though if you don't mind :) > > I'm assuming we're simply masking the bits 16+ to be 0 (modulo 2**16) as > stored in the u32 (which already carries an s16 value just "as" u32) and > not care about signedness when doing that (otherwise we would have an > issue since the MSB of the u32 is necessarily a 0, thus a positive > value, since the register returns 0 for [31:16] according to the TRM). > Is that what you meant by "along with two's complement representation"?
The masking that the driver does is technically redundant, but doesn't hurt. It's the same modulo operation that the compiler does upon assignment of a u32 value to a narrower type. Two's complement representation ensures that bit(15) is the sign bit and that 0x8000..0xffff map to -32768..-1, so assigning a masked value to an s16 variable correctly interprets the value as what the hardware implies for both positive and negative values. > We also build with clang, but I'm assuming it respects the GNU > implementation with the -std=gnu11 argument we have in KBUILD_CFLAGS. > May I ask how you know this implementation is part of gnu11 from that > webpage? I guess I stand corrected here: the webpage defines the specific "implementation-defined" behavior for this case for the specific implementation in GNU GCC, although it's not one of the GNU extensions to the C11 per se (and shouldn't be, given that C11 explicitly leaves this part to the implementation). Clang tries to maintain compatibility with GCC, so I assume it handles it the same way, but I haven't found any official documentation to confirm this. For a two's complement signed representation this is perhaps the only sane approach IMO, so I'd be surprised if any mainstream compilers chose to do it differently :) FWIW, Exynos does a similar thing in drivers/clk/exynos/clk-pll.c:88 Best regards, Alexey
