[PATCH] usb: dwc3: core: fix clk_get_rate returning negative value

[Eugen Hristev]

On 6/16/23 12:22, Marek Vasut wrote:
> On 6/16/23 09:31, Eugen Hristev wrote:
>> On 6/15/23 23:59, Marek Vasut wrote:
>>> On 6/15/23 16:59, Eugen Hristev wrote:
>>>> Unlike it's Linux counterpart, clk_get_rate can return a negative 
>>>> value, -ve.
>>>> The driver does not take that into account, stores the rate into
>>>> an unsigned long, and if clk_get_rate fails, it will take into 
>>>> consideration
>>>> the actual value and wrongly program the hardware.
>>>> E.g. on error -2 (no such entry), the rate will be 18446744073709551614
>>>> and this will be subsequently used by the driver to program the DWC3
>>>> To fix this, exit the function if the value is negative.
>>>>
>>>> Fixes: 6bae0eb5b8bd ("usb: dwc3: Calculate REFCLKPER based on 
>>>> reference clock")
>>>> Signed-off-by: Eugen Hristev <eugen.hristev at collabora.com>
>>>> ---
>>>>   drivers/usb/dwc3/core.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 49f6a1900b01..5a8c29424578 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>> @@ -138,7 +138,7 @@ static void dwc3_ref_clk_period(struct dwc3 *dwc)
>>>>       if (dwc->ref_clk) {
>>>>           rate = clk_get_rate(dwc->ref_clk);
>>>
>>> This ^ assigns to unsigned long .
>>
>> That's right, because clk_get_rate returns an unsigned long
>>>
>>> Please just change the data type of the $rate to 'long' (drop the 
>>> unsigned).
>>
>> clk_get_rate returns unsigned long, so it's not normal to assign it to 
>> a signed long.
>>
>> It is not normal that clk_get_rate returns negative values in an 
>> unsigned, but oh well, that's how the clock API is in U-boot
> 
> Ah, I see. Then why not do this diff, and fix any clock driver which 
> returns negative from clk_get_rate() instead ?
> 
> diff --git a/include/clk.h b/include/clk.h
> index d91285235f7..4e5c1fc90f6 100644
> --- a/include/clk.h
> +++ b/include/clk.h
> @@ -453,8 +453,7 @@ void clk_free(struct clk *clk);
>    * @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.
> + * Return: clock rate in Hz on success, 0 for invalid clock.
>    */
>   ulong clk_get_rate(struct clk *clk);
> 

Because clk_get_rate really returns negative numbers, I cannot update 
the documentation for it when in fact it still can return errors.

This patch which I sent, fixes the driver from the perspective of 
'clk_get_rate is like this, driver should cope with it'

If you disagree with my patch, the the only way forward that I see right 
now, is to modify the clk_get_rate to no longer return negative numbers 
as errors. But this would mean modifying the clk subsystem, and it's no 
longer a simple fix rather a change in the API. (a simple fix which I 
intended with this patch )