Re: [PATCH 2/3] clk: clk-cdce9xx.c: Change from u32 to ulong for addresses

[Quentin Schulz <quentin.schulz@cherry.de>]

Hi Tom,

On 7/17/25 5:08 PM, Tom Rini wrote:
> On Mon, Jul 14, 2025 at 12:35:14PM +0200, Quentin Schulz wrote:
>> Hi Tom,
>>
>> On 7/2/25 3:05 AM, Tom Rini wrote:
>>> For 32/64bit correctness, we need to use ulong and not u32 for casting
>>> for addresses.
>>>
>>> Signed-off-by: Tom Rini <trini@konsulko.com>
>>> ---
>>> Cc: Lukasz Majewski <lukma@denx.de>
>>> Cc: Sean Anderson <seanga2@gmail.com>
>>> ---
>>>    drivers/clk/clk-cdce9xx.c | 10 +++++-----
>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk-cdce9xx.c b/drivers/clk/clk-cdce9xx.c
>>> index e5f74e714d54..afb997c06be5 100644
>>> --- a/drivers/clk/clk-cdce9xx.c
>>> +++ b/drivers/clk/clk-cdce9xx.c
>>> @@ -103,7 +103,7 @@ static int cdce9xx_clk_probe(struct udevice *dev)
>>>    	u32 val;
>>>    	struct clk clk;
>>> -	val = (u32)dev_read_addr_ptr(dev);
>>> +	val = (ulong)dev_read_addr_ptr(dev);
>>
>> The output would be stored in a u32 anyway so not sure this actually helps
>> (see type of val in the git context above).
> 
> Yeah. It's funny. The other example of a driver doing these games is
> drivers/clk/clk_versaclock.c which uses u64 since it's a 64bit system I
> believe.
> 
>>>    	ret = i2c_get_chip(dev->parent, val, 1, &data->i2c);
>>>    	if (ret) {
>>> @@ -226,10 +226,10 @@ static ulong cdce9xx_clk_set_rate(struct clk *clk, ulong rate)
>>>    }
>>>    static const struct udevice_id cdce9xx_clk_of_match[] = {
>>> -	{ .compatible = "ti,cdce913", .data = (u32)&cdce913_chip_info },
>>> -	{ .compatible = "ti,cdce925", .data = (u32)&cdce925_chip_info },
>>> -	{ .compatible = "ti,cdce937", .data = (u32)&cdce937_chip_info },
>>> -	{ .compatible = "ti,cdce949", .data = (u32)&cdce949_chip_info },
>>> +	{ .compatible = "ti,cdce913", .data = (ulong)&cdce913_chip_info },
>>> +	{ .compatible = "ti,cdce925", .data = (ulong)&cdce925_chip_info },
>>> +	{ .compatible = "ti,cdce937", .data = (ulong)&cdce937_chip_info },
>>> +	{ .compatible = "ti,cdce949", .data = (ulong)&cdce949_chip_info },
>>
>> Just get rid of the cast I guess? udevice_id.data being a ulong already the
>> compiler should perform the cast in any case and this improves readability?
> 
> Without some cast we get:
> error: initialization of ‘long unsigned int’ from ‘const struct
> cdce9xx_chip_info *’ makes integer from pointer without a cast
> [-Werror=int-conversion]
> 

My apologies for the mislead.

It seems like places where it's not needed are where the member is of 
type void*. The kernel uses this type for of_device_id struct which I'm 
the most familiar with, but U-Boot's udevice_id actually uses a ulong 
instead, which thus requires the cast I guess.

> That said, I'm just going to drop this patch and make the driver depend
> on ARCH_OMAP2PLUS. My gut feeling is this is another one of the cases
> where we run in to problems because we don't use phys_addr_t for
> physical addresses consistently.
> 

Considering it could be anything, should the type be void* like in the 
kernel for udevice_id.data maybe? Up to the driver to perform the proper 
cast when accessing/dereferencing it?

Cheers,
Quentin