Re: [PATCH 6/6] clk: rockchip: pll: fix overflow and drop manual two's complement in rk3588_pll_get_rate

[Quentin Schulz via U-Boot <u-boot@lists.u-boot-project.org>]

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 <quentin.schulz@cherry.de> 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"?

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?

Cheers,
Quentin