[PATCH 1/3] clk: Add missing <dm/device.h> to include/clk.h

[PATCH 1/3] clk: Add missing <dm/device.h> to include/clk.h

[Tom Rini]

In this header we make direct references to some dm/device.h functions
while not including the header directly. Add the missing include.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
 include/clk.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/clk.h b/include/clk.h
index f94135ff778a..90b42a618675 100644
--- a/include/clk.h
+++ b/include/clk.h
@@ -9,6 +9,7 @@
 #define _CLK_H_
 
 #include <dm/ofnode.h>
+#include <dm/device.h>
 #include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/types.h>
-- 
2.43.0

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

[Tom Rini]

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);
 
 	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 },
 	{ /* sentinel */ },
 };
 
-- 
2.43.0

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

[Quentin Schulz]

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).

>   
>   	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?

Cheers,
Quentin

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

[Tom Rini]

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

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).
> 
> >   	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?

I'll take a harder look. This and maybe 1 or 3 other drivers are
following the above pattern. Thanks.

-- 
Tom

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

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

[Tom Rini]

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

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]

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.

-- 
Tom

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

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

[Quentin Schulz]

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

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

[Tom Rini]

[-- 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 --]

[PATCH 3/3] clk: Tighten some clock driver dependencies

[Tom Rini]

A few clock drivers cannot build without access to some platform
specific header files. Express those requirements in Kconfig as well.

Signed-off-by: Tom Rini <trini@konsulko.com>
---
Cc: Lukasz Majewski <lukma@denx.de>
Cc: Sean Anderson <seanga2@gmail.com>
---
 drivers/clk/Kconfig        | 2 +-
 drivers/clk/at91/Kconfig   | 2 +-
 drivers/clk/sifive/Kconfig | 2 +-
 drivers/clk/ti/Kconfig     | 4 ++--
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
index ef1e5355be87..83f06f50478b 100644
--- a/drivers/clk/Kconfig
+++ b/drivers/clk/Kconfig
@@ -169,7 +169,7 @@ config CLK_K210_SET_RATE
 
 config CLK_MPC83XX
 	bool "Enable MPC83xx clock driver"
-	depends on CLK
+	depends on CLK && MPC83xx
 	help
 	  Support for the clock driver of the MPC83xx series of SoCs.
 
diff --git a/drivers/clk/at91/Kconfig b/drivers/clk/at91/Kconfig
index 4563892647be..faeaa2808b03 100644
--- a/drivers/clk/at91/Kconfig
+++ b/drivers/clk/at91/Kconfig
@@ -1,6 +1,6 @@
 config CLK_AT91
 	bool "AT91 clock drivers"
-	depends on CLK
+	depends on CLK && ARCH_AT91
 	select MISC
 	help
 	  This option is used to enable the AT91 clock driver.
diff --git a/drivers/clk/sifive/Kconfig b/drivers/clk/sifive/Kconfig
index 20fc004b59e9..01e4f33415e1 100644
--- a/drivers/clk/sifive/Kconfig
+++ b/drivers/clk/sifive/Kconfig
@@ -2,7 +2,7 @@
 
 config CLK_SIFIVE
 	bool "SiFive SoC driver support"
-	depends on CLK
+	depends on CLK && RISCV
 	help
 	  SoC drivers for SiFive Linux-capable SoCs.
 
diff --git a/drivers/clk/ti/Kconfig b/drivers/clk/ti/Kconfig
index fbcdefd889ae..e0f15f57f826 100644
--- a/drivers/clk/ti/Kconfig
+++ b/drivers/clk/ti/Kconfig
@@ -5,14 +5,14 @@
 
 config CLK_TI_AM3_DPLL
 	bool "TI AM33XX Digital Phase-Locked Loop (DPLL) clock drivers"
-	depends on CLK && OF_CONTROL
+	depends on CLK && OF_CONTROL && ARCH_OMAP2PLUS
 	help
 	  This enables the DPLL clock drivers support on AM33XX SoCs. The DPLL
 	  provides all interface clocks and functional clocks to the processor.
 
 config CLK_TI_CTRL
 	bool "TI OMAP4 clock controller"
-	depends on CLK && OF_CONTROL
+	depends on CLK && OF_CONTROL && ARCH_OMAP2PLUS
 	help
 	  This enables the clock controller driver support on TI's SoCs.
 
-- 
2.43.0

Re: (subset) [PATCH 1/3] clk: Add missing <dm/device.h> to include/clk.h

[Tom Rini]

On Tue, 01 Jul 2025 19:05:33 -0600, Tom Rini wrote:

> In this header we make direct references to some dm/device.h functions
> while not including the header directly. Add the missing include.
> 
> 

Applied to u-boot/master, thanks!

[1/3] clk: Add missing <dm/device.h> to include/clk.h
      commit: e15dd01422efda76dcb5346b37e9781af5fcb67c
-- 
Tom