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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3676 bytes --]

On Thu, Jul 17, 2025 at 05:19:39PM +0200, Quentin Schulz wrote:
> 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?

Off-hand, I don't know. That's the kind of thing that perhaps someone
will take an interest in looking at and thinking on now that as a
community we're spending some more time cleaning up code.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]