[PATCH 08/24] clk: rockchip: Remove negative error returns from clk_get_rate

[Andrew Goodbody]

On 20/10/2025 16:03, Jonas Karlman wrote:
> Hi Andrew,
> 
> On 10/20/2025 2:03 PM, Andrew Goodbody wrote:
>> On 19/10/2025 21:28, Jonas Karlman wrote:
>>> Hi Andrew,
>>>
>>> On 10/15/2025 4:32 PM, Andrew Goodbody wrote:
>>>> clk_get_rate() returns a ulong so do not attempt to pass negative error
>>>> codes through it.
>>>
>>> On Rockchip the clk drivers only implement support for a subset of all
>>> clocks supported by the hardware, mostly to save space for TPL/SPL and
>>> not having to implement logic for clocks not really needed by U-Boot.
>>>
>>> Because support for all clocks is not implemented the existing working
>>> error handling actually help us catch clock issues on the Rockchip
>>> platform. E.g. when a clk is referenced by assigned-clock-rates in DT we
>>> get an error and can fix the issue, or decide if we can safely ignore
>>> setting the clk rate.
>>>
>>> I do not understand why you need to change the way the clk ops work,
>>> just check with IS_ERR_VALUE() inside the clk-uclass or similar if you
>>> really must remove working error handling from the callers.
>>
>> OK, so here we have a question and it is one that I wish had come up in
>> the original discussion. IS_ERR_VALUE() and other related macros are
>> from include/linux/err.h and are introduced with this comment
> 
> I have no idea what the original discussion this relates to.

https://lore.kernel.org/u-boot/f5c94319-8ef8-459d-88b2-836702779cfb@linaro.org/

Andrew

> In U-Boot most/all? uclass functions is documented to return >= 0 on
> success and -ve on error, I do not really see a reason why clk funcs
> should be changed away from this documented behavior.
> 
>>
>> /*
>>    * Kernel pointers have redundant information, so we can use a
>>    * scheme where we can return either an error code or a dentry
>>    * pointer with the same return value.
>>    *
>>    * This should be a per-architecture thing, to allow different
>>    * error and pointer decisions.
>>    */
>>
>> So they are clearly intended to be unambiguous and used to add
>> information about errors into kernel pointers. It also suggests that
>> there may be other implementations of these macros for other architectures.
>>
>> Much of the error checking of the return value from clk_get_rate() is
>> done with IS_ERR_VALUE() despite the return value not being a kernel
>> pointer and with this default implementation it does happen to work just
>> as long as your clock rate does not collide with an error. OK collisions
>> are very unlikely but it does still feel like an abuse of the macro to me.
> 
> The U-Boot documentation for clk_get_rate() clearly state that we should
> use -ve error code for errors. And the safest way to test for error
> codes will be to use something like IS_ERR_VALUE(), so why should we not use it as
> it does the job very well?
> 
> Also for Rockchip the clock rates do not really collide with error codes
> as the typical max clock rate go up to 2-3 GHz, and in U-Boot we
> typically only reference clocks with less than 1 GHz rates.
> 
> /**
>   * clk_get_rate() - Get current clock rate.
>   * @clk:	A clock struct that was previously successfully requested by
>   *		clk_request/get_by_*().
>   *
>   * Return: clock rate in Hz on success, 0 for invalid clock, or -ve error code
>   *	   for other errors.
>   */
> 
> To me this patch/series just look to break this documented and working
> error handling without any justified benefit or replacement.
> 
> Regards,
> Jonas
> 
>>
>> Is there any official guidance on this use of IS_ERR_VALUE()?
>>
>> Andrew
>>
>>> The current working clk error handling has really helped me time and
>>> time again on Rockchip platform during soc and board bring-up.
>>>
>>> This is a NAK for me without first having a proper replacement for all
>>> existing working error handling that is just being left ignored after
>>> this patch/series.
>>>
>>> Regards,
>>> Jonas