Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson]

On 10/28/21 9:28 PM, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Current code has a force clk_set_defaults in multiple stages,
> U-Boot reuse the same device tree and Linux Kernel device tree,
> but we not register all the clks as Linux Kernel, so clk_set_defaults
> will fail and cause the clk provider registeration fail.
> 
> So introduce a new property to ignore the default settings which could
> be used by any node that wanna ignore default settings.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>   Add R-b tag
>   Tom, Simon
>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>     actually Linux specific, however I could not add the new property to Linux,
>     because we are supporting SystemReady-IR, we need the assigned-clock-x property
>     in linux working and ignore it in U-Boot.
> 
>   doc/device-tree-bindings/device.txt | 3 +++
>   drivers/clk/clk-uclass.c            | 3 +++
>   2 files changed, 6 insertions(+)
> 
> diff --git a/doc/device-tree-bindings/device.txt b/doc/device-tree-bindings/device.txt
> index 73ce2a3b5b..fe34ced268 100644
> --- a/doc/device-tree-bindings/device.txt
> +++ b/doc/device-tree-bindings/device.txt
> @@ -28,6 +28,9 @@ the acpi,compatible property.
>       Linux will only load the driver if the device can be detected (e.g. on I2C
>       bus). Note that this is an out-of-tree Linux feature.
>   
> +Common device bindings that could be shared listed below:
> + - u-boot,ignore-clk-defaults : ignore the assigned-clock-parents
> +   and assigned-clock-rates for a device that has the property.
>   
>   Example
>   -------
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> index 493018b33e..6bf3179e7b 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -376,6 +376,9 @@ int clk_set_defaults(struct udevice *dev, enum clk_defaults_stage stage)
>   	if (!dev_has_ofnode(dev))
>   		return 0;
>   
> +	if (ofnode_get_property(dev_ofnode(dev), "u-boot,ignore-clk-defaults", NULL))
> +		return 0;
> +
>   	/*
>   	 * To avoid setting defaults twice, don't set them before relocation.
>   	 * However, still set them for SPL. And still set them if explicitly
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

[PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Peng Fan (OSS)]

From: Peng Fan <peng.fan@nxp.com>

Current code has a force clk_set_defaults in multiple stages,
U-Boot reuse the same device tree and Linux Kernel device tree,
but we not register all the clks as Linux Kernel, so clk_set_defaults
will fail and cause the clk provider registeration fail.

So introduce a new property to ignore the default settings which could
be used by any node that wanna ignore default settings.

Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

V2:
 Add R-b tag
 Tom, Simon
   After a thought, I think still put it as a u-boot thing. assigned-clock-x is 
   actually Linux specific, however I could not add the new property to Linux,
   because we are supporting SystemReady-IR, we need the assigned-clock-x property
   in linux working and ignore it in U-Boot.

 doc/device-tree-bindings/device.txt | 3 +++
 drivers/clk/clk-uclass.c            | 3 +++
 2 files changed, 6 insertions(+)

diff --git a/doc/device-tree-bindings/device.txt b/doc/device-tree-bindings/device.txt
index 73ce2a3b5b..fe34ced268 100644
--- a/doc/device-tree-bindings/device.txt
+++ b/doc/device-tree-bindings/device.txt
@@ -28,6 +28,9 @@ the acpi,compatible property.
     Linux will only load the driver if the device can be detected (e.g. on I2C
     bus). Note that this is an out-of-tree Linux feature.
 
+Common device bindings that could be shared listed below:
+ - u-boot,ignore-clk-defaults : ignore the assigned-clock-parents
+   and assigned-clock-rates for a device that has the property.
 
 Example
 -------
diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
index 493018b33e..6bf3179e7b 100644
--- a/drivers/clk/clk-uclass.c
+++ b/drivers/clk/clk-uclass.c
@@ -376,6 +376,9 @@ int clk_set_defaults(struct udevice *dev, enum clk_defaults_stage stage)
 	if (!dev_has_ofnode(dev))
 		return 0;
 
+	if (ofnode_get_property(dev_ofnode(dev), "u-boot,ignore-clk-defaults", NULL))
+		return 0;
+
 	/*
 	 * To avoid setting defaults twice, don't set them before relocation.
 	 * However, still set them for SPL. And still set them if explicitly
-- 
2.30.0

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Fri, Oct 29, 2021 at 09:28:01AM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> Current code has a force clk_set_defaults in multiple stages,
> U-Boot reuse the same device tree and Linux Kernel device tree,
> but we not register all the clks as Linux Kernel, so clk_set_defaults
> will fail and cause the clk provider registeration fail.
> 
> So introduce a new property to ignore the default settings which could
> be used by any node that wanna ignore default settings.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Add R-b tag
>  Tom, Simon
>    After a thought, I think still put it as a u-boot thing. assigned-clock-x is 
>    actually Linux specific, however I could not add the new property to Linux,
>    because we are supporting SystemReady-IR, we need the assigned-clock-x property
>    in linux working and ignore it in U-Boot.

OK, right, so... it should be a generic property to say what, early
firmware should ignore this clock unless otherwise required and use the
preconfigured value?

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson]

On 10/28/21 10:10 PM, Tom Rini wrote:
> On Fri, Oct 29, 2021 at 09:28:01AM +0800, Peng Fan (OSS) wrote:
>> From: Peng Fan <peng.fan@nxp.com>
>>
>> Current code has a force clk_set_defaults in multiple stages,
>> U-Boot reuse the same device tree and Linux Kernel device tree,
>> but we not register all the clks as Linux Kernel, so clk_set_defaults
>> will fail and cause the clk provider registeration fail.
>>
>> So introduce a new property to ignore the default settings which could
>> be used by any node that wanna ignore default settings.
>>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>> ---
>>
>> V2:
>>   Add R-b tag
>>   Tom, Simon
>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>>     actually Linux specific, however I could not add the new property to Linux,
>>     because we are supporting SystemReady-IR, we need the assigned-clock-x property
>>     in linux working and ignore it in U-Boot.
> 
> OK, right, so... it should be a generic property to say what, early
> firmware should ignore this clock unless otherwise required and use the
> preconfigured value?
> 

The issue is that for platforms sharing devicetrees between U-Boot and
Linux, Linux may add assigned-clocks which are not implemented in
U-Boot. Although assigning a specific frequency or parent to a clock may
not be necessary to use the device, it will fail to probe because the
clock cannot be found. I believe this property is intended for these
cases.

--Sean

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Thu, Oct 28, 2021 at 10:43:01PM -0400, Sean Anderson wrote:
> 
> On 10/28/21 10:10 PM, Tom Rini wrote:
> > On Fri, Oct 29, 2021 at 09:28:01AM +0800, Peng Fan (OSS) wrote:
> > > From: Peng Fan <peng.fan@nxp.com>
> > > 
> > > Current code has a force clk_set_defaults in multiple stages,
> > > U-Boot reuse the same device tree and Linux Kernel device tree,
> > > but we not register all the clks as Linux Kernel, so clk_set_defaults
> > > will fail and cause the clk provider registeration fail.
> > > 
> > > So introduce a new property to ignore the default settings which could
> > > be used by any node that wanna ignore default settings.
> > > 
> > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > ---
> > > 
> > > V2:
> > >   Add R-b tag
> > >   Tom, Simon
> > >     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> > >     actually Linux specific, however I could not add the new property to Linux,
> > >     because we are supporting SystemReady-IR, we need the assigned-clock-x property
> > >     in linux working and ignore it in U-Boot.
> > 
> > OK, right, so... it should be a generic property to say what, early
> > firmware should ignore this clock unless otherwise required and use the
> > preconfigured value?
> > 
> 
> The issue is that for platforms sharing devicetrees between U-Boot and
> Linux, Linux may add assigned-clocks which are not implemented in
> U-Boot. Although assigning a specific frequency or parent to a clock may
> not be necessary to use the device, it will fail to probe because the
> clock cannot be found. I believe this property is intended for these
> cases.

Alright.  An example might help, but, why can't we either make this a
positive general property of the node, or otherwise infer that from
what's present in the node today?

-- 
Tom

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

RE: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Peng Fan (OSS)]

> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> 
> From: Peng Fan <peng.fan@nxp.com>
> 
> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> same device tree and Linux Kernel device tree, but we not register all the clks
> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> registeration fail.
> 
> So introduce a new property to ignore the default settings which could be
> used by any node that wanna ignore default settings.
> 
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V2:
>  Add R-b tag
>  Tom, Simon
>    After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>    actually Linux specific, however I could not add the new property to Linux,
>    because we are supporting SystemReady-IR, we need the
> assigned-clock-x property
>    in linux working and ignore it in U-Boot.

Any more thoughts?

Thanks,
Peng.

> 
>  doc/device-tree-bindings/device.txt | 3 +++
>  drivers/clk/clk-uclass.c            | 3 +++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/doc/device-tree-bindings/device.txt
> b/doc/device-tree-bindings/device.txt
> index 73ce2a3b5b..fe34ced268 100644
> --- a/doc/device-tree-bindings/device.txt
> +++ b/doc/device-tree-bindings/device.txt
> @@ -28,6 +28,9 @@ the acpi,compatible property.
>      Linux will only load the driver if the device can be detected (e.g. on I2C
>      bus). Note that this is an out-of-tree Linux feature.
> 
> +Common device bindings that could be shared listed below:
> + - u-boot,ignore-clk-defaults : ignore the assigned-clock-parents
> +   and assigned-clock-rates for a device that has the property.
> 
>  Example
>  -------
> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index
> 493018b33e..6bf3179e7b 100644
> --- a/drivers/clk/clk-uclass.c
> +++ b/drivers/clk/clk-uclass.c
> @@ -376,6 +376,9 @@ int clk_set_defaults(struct udevice *dev, enum
> clk_defaults_stage stage)
>  	if (!dev_has_ofnode(dev))
>  		return 0;
> 
> +	if (ofnode_get_property(dev_ofnode(dev), "u-boot,ignore-clk-defaults",
> NULL))
> +		return 0;
> +
>  	/*
>  	 * To avoid setting defaults twice, don't set them before relocation.
>  	 * However, still set them for SPL. And still set them if explicitly
> --
> 2.30.0

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > 
> > From: Peng Fan <peng.fan@nxp.com>
> > 
> > Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> > same device tree and Linux Kernel device tree, but we not register all the clks
> > as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> > registeration fail.
> > 
> > So introduce a new property to ignore the default settings which could be
> > used by any node that wanna ignore default settings.
> > 
> > Reviewed-by: Simon Glass <sjg@chromium.org>
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> > 
> > V2:
> >  Add R-b tag
> >  Tom, Simon
> >    After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> >    actually Linux specific, however I could not add the new property to Linux,
> >    because we are supporting SystemReady-IR, we need the
> > assigned-clock-x property
> >    in linux working and ignore it in U-Boot.
> 
> Any more thoughts?

Just my continued request that you treat this as generic and submit the
binding upstream so it can be in the device tree for the platform.

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson]

On 11/20/21 7:57 AM, Tom Rini wrote:
> On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
>>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
>>> same device tree and Linux Kernel device tree, but we not register all the clks
>>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
>>> registeration fail.
>>>
>>> So introduce a new property to ignore the default settings which could be
>>> used by any node that wanna ignore default settings.
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>
>>> V2:
>>>   Add R-b tag
>>>   Tom, Simon
>>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>>>     actually Linux specific, however I could not add the new property to Linux,
>>>     because we are supporting SystemReady-IR, we need the
>>> assigned-clock-x property
>>>     in linux working and ignore it in U-Boot.
>>
>> Any more thoughts?
> 
> Just my continued request that you treat this as generic and submit the
> binding upstream so it can be in the device tree for the platform.
> 

Hmm.

Could we just do

/delete-property/ assigned-clocks;

in our u-boot dtsi?

--Sean

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Sat, Nov 20, 2021 at 10:06:55AM -0500, Sean Anderson wrote:
> On 11/20/21 7:57 AM, Tom Rini wrote:
> > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > 
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > > 
> > > > Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> > > > same device tree and Linux Kernel device tree, but we not register all the clks
> > > > as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> > > > registeration fail.
> > > > 
> > > > So introduce a new property to ignore the default settings which could be
> > > > used by any node that wanna ignore default settings.
> > > > 
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > > 
> > > > V2:
> > > >   Add R-b tag
> > > >   Tom, Simon
> > > >     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> > > >     actually Linux specific, however I could not add the new property to Linux,
> > > >     because we are supporting SystemReady-IR, we need the
> > > > assigned-clock-x property
> > > >     in linux working and ignore it in U-Boot.
> > > 
> > > Any more thoughts?
> > 
> > Just my continued request that you treat this as generic and submit the
> > binding upstream so it can be in the device tree for the platform.
> > 
> 
> Hmm.
> 
> Could we just do
> 
> /delete-property/ assigned-clocks;
> 
> in our u-boot dtsi?

As a fall back?  I'm still unclear as to why the right answer isn't
something along the lines of "work with upstream to get appropriate
bindings accepted".  I think I might have even misread the comment about
SystemReady IR before even.  The assigned-lock-x property isn't in the
upstream binding?  So now we're trying to do what exactly here?

And to be clear, the situation with the layerscape dts files that's just
now getting sorted out has me extra skeptical of "just modify the dts in
U-Boot to ..." changes.  The goal within U-Boot is that our bindings are
accepted upstream (as upstream accepts non-Linux bindings) and I prefer
to start asking "is this a binding that's applicable to other firmwares
too?".

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Mark Kettenis]

> From: Sean Anderson <seanga2@gmail.com>
> Date: Sat, 20 Nov 2021 10:06:55 -0500
> 
> On 11/20/21 7:57 AM, Tom Rini wrote:
> > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> >>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> >>>
> >>> From: Peng Fan <peng.fan@nxp.com>
> >>>
> >>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> >>> same device tree and Linux Kernel device tree, but we not register all the clks
> >>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> >>> registeration fail.
> >>>
> >>> So introduce a new property to ignore the default settings which could be
> >>> used by any node that wanna ignore default settings.
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>
> >>> V2:
> >>>   Add R-b tag
> >>>   Tom, Simon
> >>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> >>>     actually Linux specific, however I could not add the new property to Linux,
> >>>     because we are supporting SystemReady-IR, we need the
> >>> assigned-clock-x property
> >>>     in linux working and ignore it in U-Boot.
> >>
> >> Any more thoughts?
> > 
> > Just my continued request that you treat this as generic and submit the
> > binding upstream so it can be in the device tree for the platform.
> > 
> 
> Hmm.
> 
> Could we just do
> 
> /delete-property/ assigned-clocks;
> 
> in our u-boot dtsi?

No!  Those properties are needed by the OS loaded by U-Boot.

The right answer is probably that U-Boot should use these properties
to set up the clocks correctly.  Paradoxically that means the OS would
no longer have to worry about them and the properties could be deleted ;).

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Sat, Nov 20, 2021 at 04:21:12PM +0100, Mark Kettenis wrote:
> > From: Sean Anderson <seanga2@gmail.com>
> > Date: Sat, 20 Nov 2021 10:06:55 -0500
> > 
> > On 11/20/21 7:57 AM, Tom Rini wrote:
> > > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > >>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > >>>
> > >>> From: Peng Fan <peng.fan@nxp.com>
> > >>>
> > >>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> > >>> same device tree and Linux Kernel device tree, but we not register all the clks
> > >>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> > >>> registeration fail.
> > >>>
> > >>> So introduce a new property to ignore the default settings which could be
> > >>> used by any node that wanna ignore default settings.
> > >>>
> > >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> > >>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > >>> ---
> > >>>
> > >>> V2:
> > >>>   Add R-b tag
> > >>>   Tom, Simon
> > >>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> > >>>     actually Linux specific, however I could not add the new property to Linux,
> > >>>     because we are supporting SystemReady-IR, we need the
> > >>> assigned-clock-x property
> > >>>     in linux working and ignore it in U-Boot.
> > >>
> > >> Any more thoughts?
> > > 
> > > Just my continued request that you treat this as generic and submit the
> > > binding upstream so it can be in the device tree for the platform.
> > > 
> > 
> > Hmm.
> > 
> > Could we just do
> > 
> > /delete-property/ assigned-clocks;
> > 
> > in our u-boot dtsi?
> 
> No!  Those properties are needed by the OS loaded by U-Boot.
> 
> The right answer is probably that U-Boot should use these properties
> to set up the clocks correctly.  Paradoxically that means the OS would
> no longer have to worry about them and the properties could be deleted ;).

Yeah, so this needs a better commit message to explain what the
reasoning / logic is, in a positive manner, rather than negative.  And
the binding portion needs a longer explanation.  And it needs to be sent
upstream.

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Peng Fan (OSS)]

+ Rob

On 2021/11/20 20:57, Tom Rini wrote:
> On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
>>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>
>>> From: Peng Fan <peng.fan@nxp.com>
>>>
>>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
>>> same device tree and Linux Kernel device tree, but we not register all the clks
>>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
>>> registeration fail.
>>>
>>> So introduce a new property to ignore the default settings which could be
>>> used by any node that wanna ignore default settings.
>>>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>> ---
>>>
>>> V2:
>>>   Add R-b tag
>>>   Tom, Simon
>>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>>>     actually Linux specific, however I could not add the new property to Linux,
>>>     because we are supporting SystemReady-IR, we need the
>>> assigned-clock-x property
>>>     in linux working and ignore it in U-Boot.
>>
>> Any more thoughts?
> 
> Just my continued request that you treat this as generic and submit the
> binding upstream so it can be in the device tree for the platform.
> 

As Sean said, this is to serve cast that linux and U-Boot use the same
device tree, I mean U-Boot runtime export device tree to linux for SR-IR
(system-ready IR) booting.

Linux needs assigned-clocks to some reason, but U-Boot not need that 
because the driver not added the support or not a must to have that.

Because assigned-clocks failure in U-Boot will cause probe fail now,
the device driver will report failure.

You mean rename this to "ignore-clk-defaults" or keep 
"u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux 
device tree binding?

I could try to send to linux kernel with "firmware" as a prefix.

Thanks,
Peng.

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Peng Fan (OSS)]

On 2021/11/20 23:06, Sean Anderson wrote:
> On 11/20/21 7:57 AM, Tom Rini wrote:
>> On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
>>>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>>
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
>>>> same device tree and Linux Kernel device tree, but we not register all the clks
>>>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
>>>> registeration fail.
>>>>
>>>> So introduce a new property to ignore the default settings which could be
>>>> used by any node that wanna ignore default settings.
>>>>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>> ---
>>>>
>>>> V2:
>>>>    Add R-b tag
>>>>    Tom, Simon
>>>>      After a thought, I think still put it as a u-boot thing. assigned-clock-x is
>>>>      actually Linux specific, however I could not add the new property to Linux,
>>>>      because we are supporting SystemReady-IR, we need the
>>>> assigned-clock-x property
>>>>      in linux working and ignore it in U-Boot.
>>>
>>> Any more thoughts?
>>
>> Just my continued request that you treat this as generic and submit the
>> binding upstream so it can be in the device tree for the platform.
>>
> 
> Hmm.
> 
> Could we just do
> 
> /delete-property/ assigned-clocks;
> 
> in our u-boot dtsi?

No, in my case, linux will use the runtime exported device tree,
dropping it means linux kernel also not see it.

THanks,
Peng.

> 
> --Sean
> 

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Simon Glass]

Hi Peng,

On Sun, 21 Nov 2021 at 20:33, Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote:
>
> + Rob
>
> On 2021/11/20 20:57, Tom Rini wrote:
> > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> >>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> >>>
> >>> From: Peng Fan <peng.fan@nxp.com>
> >>>
> >>> Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> >>> same device tree and Linux Kernel device tree, but we not register all the clks
> >>> as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> >>> registeration fail.
> >>>
> >>> So introduce a new property to ignore the default settings which could be
> >>> used by any node that wanna ignore default settings.
> >>>
> >>> Reviewed-by: Simon Glass <sjg@chromium.org>
> >>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> >>> ---
> >>>
> >>> V2:
> >>>   Add R-b tag
> >>>   Tom, Simon
> >>>     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> >>>     actually Linux specific, however I could not add the new property to Linux,
> >>>     because we are supporting SystemReady-IR, we need the
> >>> assigned-clock-x property
> >>>     in linux working and ignore it in U-Boot.
> >>
> >> Any more thoughts?
> >
> > Just my continued request that you treat this as generic and submit the
> > binding upstream so it can be in the device tree for the platform.
> >
>
> As Sean said, this is to serve cast that linux and U-Boot use the same
> device tree, I mean U-Boot runtime export device tree to linux for SR-IR
> (system-ready IR) booting.
>
> Linux needs assigned-clocks to some reason, but U-Boot not need that
> because the driver not added the support or not a must to have that.
>
> Because assigned-clocks failure in U-Boot will cause probe fail now,
> the device driver will report failure.
>
> You mean rename this to "ignore-clk-defaults" or keep
> "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> device tree binding?
>
> I could try to send to linux kernel with "firmware" as a prefix.

I'd argue that you are safer with a 'u-boot,' prefix since even U-Boot
might one day implement these clocks if they are needed there. Then we
could drop the property.

This seems like something that is specific to a project.

Regards,
Simon

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> + Rob
> 
> On 2021/11/20 20:57, Tom Rini wrote:
> > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > 
> > > > From: Peng Fan <peng.fan@nxp.com>
> > > > 
> > > > Current code has a force clk_set_defaults in multiple stages, U-Boot reuse the
> > > > same device tree and Linux Kernel device tree, but we not register all the clks
> > > > as Linux Kernel, so clk_set_defaults will fail and cause the clk provider
> > > > registeration fail.
> > > > 
> > > > So introduce a new property to ignore the default settings which could be
> > > > used by any node that wanna ignore default settings.
> > > > 
> > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > ---
> > > > 
> > > > V2:
> > > >   Add R-b tag
> > > >   Tom, Simon
> > > >     After a thought, I think still put it as a u-boot thing. assigned-clock-x is
> > > >     actually Linux specific, however I could not add the new property to Linux,
> > > >     because we are supporting SystemReady-IR, we need the
> > > > assigned-clock-x property
> > > >     in linux working and ignore it in U-Boot.
> > > 
> > > Any more thoughts?
> > 
> > Just my continued request that you treat this as generic and submit the
> > binding upstream so it can be in the device tree for the platform.
> > 
> 
> As Sean said, this is to serve cast that linux and U-Boot use the same
> device tree, I mean U-Boot runtime export device tree to linux for SR-IR
> (system-ready IR) booting.
> 
> Linux needs assigned-clocks to some reason, but U-Boot not need that because
> the driver not added the support or not a must to have that.
> 
> Because assigned-clocks failure in U-Boot will cause probe fail now,
> the device driver will report failure.
> 
> You mean rename this to "ignore-clk-defaults" or keep
> "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> device tree binding?
> 
> I could try to send to linux kernel with "firmware" as a prefix.

What I mean is that first I'm not seeing the description of the property
as being clear enough, either in commit message or the binding itself.
That's why in my mind I keep seeing this as "we set the properties
linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why we
need both.  Is there not something we can do based on seeing
linux,assigned-clocks ?  Showing something that makes use of the property
you're wishing to add would be helpful.  In fact, some specific dts
snippets would be helpful to understand what's going on here.

Second, if we need a new property here then yes, it goes upstream and
you put u-boot,has-not-clk-foo or firmware,has-not-clk-foo or whatever
in the boards dts file.

-- 
Tom

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

RE: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Peng Fan (OSS)]

> Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> 
> On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> > + Rob
> >
> > On 2021/11/20 20:57, Tom Rini wrote:
> > > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > >
> > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > >
> > > > > Current code has a force clk_set_defaults in multiple stages,
> > > > > U-Boot reuse the same device tree and Linux Kernel device tree,
> > > > > but we not register all the clks as Linux Kernel, so
> > > > > clk_set_defaults will fail and cause the clk provider registeration fail.
> > > > >
> > > > > So introduce a new property to ignore the default settings which
> > > > > could be used by any node that wanna ignore default settings.
> > > > >
> > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > ---
> > > > >
> > > > > V2:
> > > > >   Add R-b tag
> > > > >   Tom, Simon
> > > > >     After a thought, I think still put it as a u-boot thing.
> assigned-clock-x is
> > > > >     actually Linux specific, however I could not add the new property
> to Linux,
> > > > >     because we are supporting SystemReady-IR, we need the
> > > > > assigned-clock-x property
> > > > >     in linux working and ignore it in U-Boot.
> > > >
> > > > Any more thoughts?
> > >
> > > Just my continued request that you treat this as generic and submit
> > > the binding upstream so it can be in the device tree for the platform.
> > >
> >
> > As Sean said, this is to serve cast that linux and U-Boot use the same
> > device tree, I mean U-Boot runtime export device tree to linux for
> > SR-IR (system-ready IR) booting.
> >
> > Linux needs assigned-clocks to some reason, but U-Boot not need that
> > because the driver not added the support or not a must to have that.
> >
> > Because assigned-clocks failure in U-Boot will cause probe fail now,
> > the device driver will report failure.
> >
> > You mean rename this to "ignore-clk-defaults" or keep
> > "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> > device tree binding?
> >
> > I could try to send to linux kernel with "firmware" as a prefix.
> 
> What I mean is that first I'm not seeing the description of the property as
> being clear enough, either in commit message or the binding itself.
> That's why in my mind I keep seeing this as "we set the properties
> linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
> we need both.  Is there not something we can do based on seeing
> linux,assigned-clocks ?  Showing something that makes use of the property
> you're wishing to add would be helpful.  In fact, some specific dts snippets
> would be helpful to understand what's going on here.

clk: clock-controller@30380000 {
        compatible = "fsl,imx8mp-ccm";
        reg = <0x30380000 0x10000>;
        #clock-cells = <1>;
        clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
                 <&clk_ext3>, <&clk_ext4>;
        clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
                      "clk_ext3", "clk_ext4";
        assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
                          <&clk IMX8MP_CLK_A53_CORE>,
                          <&clk IMX8MP_CLK_NOC>,
                          <&clk IMX8MP_CLK_NOC_IO>,
                          <&clk IMX8MP_CLK_GIC>,
                          <&clk IMX8MP_CLK_AUDIO_AHB>,
                          <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
                          <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
                          <&clk IMX8MP_AUDIO_PLL1>,
                          <&clk IMX8MP_AUDIO_PLL2>;
        assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
                                 <&clk IMX8MP_ARM_PLL_OUT>,
                                 <&clk IMX8MP_SYS_PLL2_1000M>,
                                 <&clk IMX8MP_SYS_PLL1_800M>,
                                 <&clk IMX8MP_SYS_PLL2_500M>,
                                 <&clk IMX8MP_SYS_PLL1_800M>,
                                 <&clk IMX8MP_SYS_PLL1_800M>;
        assigned-clock-rates = <0>, <0>,
                               <1000000000>,
                               <800000000>,
                               <500000000>,
                               <400000000>,
                               <800000000>,
                               <400000000>,
                               <393216000>,
                               <361267200>;
        u-boot,ignore-clk-defaults;
};

The above node is that I wanna have in U-Boot device tree.
This node is same as the one used in linux device tree except the new
property introduced here.

In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
because U-Boot runtime export the device tree to Linux.

So add a new property here for U-Boot specific usage, if the property
is there, U-Boot ignore the assigned-clock settings for a node.

I hope this is clear and please suggest what to do next.

Thanks,
Peng.
> 
> Second, if we need a new property here then yes, it goes upstream and you
> put u-boot,has-not-clk-foo or firmware,has-not-clk-foo or whatever in the
> boards dts file.
> 
> --
> Tom

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Tue, Nov 23, 2021 at 03:02:45AM +0000, Peng Fan (OSS) wrote:
> > Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > 
> > On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> > > + Rob
> > >
> > > On 2021/11/20 20:57, Tom Rini wrote:
> > > > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > > >
> > > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > >
> > > > > > Current code has a force clk_set_defaults in multiple stages,
> > > > > > U-Boot reuse the same device tree and Linux Kernel device tree,
> > > > > > but we not register all the clks as Linux Kernel, so
> > > > > > clk_set_defaults will fail and cause the clk provider registeration fail.
> > > > > >
> > > > > > So introduce a new property to ignore the default settings which
> > > > > > could be used by any node that wanna ignore default settings.
> > > > > >
> > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > ---
> > > > > >
> > > > > > V2:
> > > > > >   Add R-b tag
> > > > > >   Tom, Simon
> > > > > >     After a thought, I think still put it as a u-boot thing.
> > assigned-clock-x is
> > > > > >     actually Linux specific, however I could not add the new property
> > to Linux,
> > > > > >     because we are supporting SystemReady-IR, we need the
> > > > > > assigned-clock-x property
> > > > > >     in linux working and ignore it in U-Boot.
> > > > >
> > > > > Any more thoughts?
> > > >
> > > > Just my continued request that you treat this as generic and submit
> > > > the binding upstream so it can be in the device tree for the platform.
> > > >
> > >
> > > As Sean said, this is to serve cast that linux and U-Boot use the same
> > > device tree, I mean U-Boot runtime export device tree to linux for
> > > SR-IR (system-ready IR) booting.
> > >
> > > Linux needs assigned-clocks to some reason, but U-Boot not need that
> > > because the driver not added the support or not a must to have that.
> > >
> > > Because assigned-clocks failure in U-Boot will cause probe fail now,
> > > the device driver will report failure.
> > >
> > > You mean rename this to "ignore-clk-defaults" or keep
> > > "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> > > device tree binding?
> > >
> > > I could try to send to linux kernel with "firmware" as a prefix.
> > 
> > What I mean is that first I'm not seeing the description of the property as
> > being clear enough, either in commit message or the binding itself.
> > That's why in my mind I keep seeing this as "we set the properties
> > linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
> > we need both.  Is there not something we can do based on seeing
> > linux,assigned-clocks ?  Showing something that makes use of the property
> > you're wishing to add would be helpful.  In fact, some specific dts snippets
> > would be helpful to understand what's going on here.
> 
> clk: clock-controller@30380000 {
>         compatible = "fsl,imx8mp-ccm";
>         reg = <0x30380000 0x10000>;
>         #clock-cells = <1>;
>         clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>                  <&clk_ext3>, <&clk_ext4>;
>         clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
>                       "clk_ext3", "clk_ext4";
>         assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
>                           <&clk IMX8MP_CLK_A53_CORE>,
>                           <&clk IMX8MP_CLK_NOC>,
>                           <&clk IMX8MP_CLK_NOC_IO>,
>                           <&clk IMX8MP_CLK_GIC>,
>                           <&clk IMX8MP_CLK_AUDIO_AHB>,
>                           <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
>                           <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
>                           <&clk IMX8MP_AUDIO_PLL1>,
>                           <&clk IMX8MP_AUDIO_PLL2>;
>         assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
>                                  <&clk IMX8MP_ARM_PLL_OUT>,
>                                  <&clk IMX8MP_SYS_PLL2_1000M>,
>                                  <&clk IMX8MP_SYS_PLL1_800M>,
>                                  <&clk IMX8MP_SYS_PLL2_500M>,
>                                  <&clk IMX8MP_SYS_PLL1_800M>,
>                                  <&clk IMX8MP_SYS_PLL1_800M>;
>         assigned-clock-rates = <0>, <0>,
>                                <1000000000>,
>                                <800000000>,
>                                <500000000>,
>                                <400000000>,
>                                <800000000>,
>                                <400000000>,
>                                <393216000>,
>                                <361267200>;
>         u-boot,ignore-clk-defaults;
> };
> 
> The above node is that I wanna have in U-Boot device tree.
> This node is same as the one used in linux device tree except the new
> property introduced here.

That property must be in the Linux device tree too.

> In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
> because U-Boot runtime export the device tree to Linux.
> 
> So add a new property here for U-Boot specific usage, if the property
> is there, U-Boot ignore the assigned-clock settings for a node.

OK, so what does U-Boot do instead now?  And why can U-Boot not infer
that it should ignore this node without this property?

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson]

On 11/22/21 10:02 PM, Peng Fan (OSS) wrote:
>> Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>
>> On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
>>> + Rob
>>>
>>> On 2021/11/20 20:57, Tom Rini wrote:
>>>> On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
>>>>>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>>>>
>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>
>>>>>> Current code has a force clk_set_defaults in multiple stages,
>>>>>> U-Boot reuse the same device tree and Linux Kernel device tree,
>>>>>> but we not register all the clks as Linux Kernel, so
>>>>>> clk_set_defaults will fail and cause the clk provider registeration fail.
>>>>>>
>>>>>> So introduce a new property to ignore the default settings which
>>>>>> could be used by any node that wanna ignore default settings.
>>>>>>
>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>>>> ---
>>>>>>
>>>>>> V2:
>>>>>>    Add R-b tag
>>>>>>    Tom, Simon
>>>>>>      After a thought, I think still put it as a u-boot thing.
>> assigned-clock-x is
>>>>>>      actually Linux specific, however I could not add the new property
>> to Linux,
>>>>>>      because we are supporting SystemReady-IR, we need the
>>>>>> assigned-clock-x property
>>>>>>      in linux working and ignore it in U-Boot.
>>>>>
>>>>> Any more thoughts?
>>>>
>>>> Just my continued request that you treat this as generic and submit
>>>> the binding upstream so it can be in the device tree for the platform.
>>>>
>>>
>>> As Sean said, this is to serve cast that linux and U-Boot use the same
>>> device tree, I mean U-Boot runtime export device tree to linux for
>>> SR-IR (system-ready IR) booting.
>>>
>>> Linux needs assigned-clocks to some reason, but U-Boot not need that
>>> because the driver not added the support or not a must to have that.
>>>
>>> Because assigned-clocks failure in U-Boot will cause probe fail now,
>>> the device driver will report failure.
>>>
>>> You mean rename this to "ignore-clk-defaults" or keep
>>> "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
>>> device tree binding?
>>>
>>> I could try to send to linux kernel with "firmware" as a prefix.
>>
>> What I mean is that first I'm not seeing the description of the property as
>> being clear enough, either in commit message or the binding itself.
>> That's why in my mind I keep seeing this as "we set the properties
>> linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
>> we need both.  Is there not something we can do based on seeing
>> linux,assigned-clocks ?  Showing something that makes use of the property
>> you're wishing to add would be helpful.  In fact, some specific dts snippets
>> would be helpful to understand what's going on here.
> 
> clk: clock-controller@30380000 {
>          compatible = "fsl,imx8mp-ccm";
>          reg = <0x30380000 0x10000>;
>          #clock-cells = <1>;
>          clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>                   <&clk_ext3>, <&clk_ext4>;
>          clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
>                        "clk_ext3", "clk_ext4";
>          assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
>                            <&clk IMX8MP_CLK_A53_CORE>,
>                            <&clk IMX8MP_CLK_NOC>,
>                            <&clk IMX8MP_CLK_NOC_IO>,
>                            <&clk IMX8MP_CLK_GIC>,
>                            <&clk IMX8MP_CLK_AUDIO_AHB>,
>                            <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
>                            <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
>                            <&clk IMX8MP_AUDIO_PLL1>,
>                            <&clk IMX8MP_AUDIO_PLL2>;
>          assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
>                                   <&clk IMX8MP_ARM_PLL_OUT>,
>                                   <&clk IMX8MP_SYS_PLL2_1000M>,
>                                   <&clk IMX8MP_SYS_PLL1_800M>,
>                                   <&clk IMX8MP_SYS_PLL2_500M>,
>                                   <&clk IMX8MP_SYS_PLL1_800M>,
>                                   <&clk IMX8MP_SYS_PLL1_800M>;
>          assigned-clock-rates = <0>, <0>,
>                                 <1000000000>,
>                                 <800000000>,
>                                 <500000000>,
>                                 <400000000>,
>                                 <800000000>,
>                                 <400000000>,
>                                 <393216000>,
>                                 <361267200>;
>          u-boot,ignore-clk-defaults;
> };
> 
> The above node is that I wanna have in U-Boot device tree.
> This node is same as the one used in linux device tree except the new
> property introduced here.
> 
> In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
> because U-Boot runtime export the device tree to Linux.
> 
> So add a new property here for U-Boot specific usage, if the property
> is there, U-Boot ignore the assigned-clock settings for a node.
> 
> I hope this is clear and please suggest what to do next.

Hm. Well perhaps we can designate a specific return code to indicate
that we have a valid clock ID but it is just unimplemented. So in your
driver you would do

ulong my_get_rate(struct clk *clk)
{
	switch (clk->id) {
	case MY_CLK:
		...
	default:
		if (clk->id < MY_CLK_MAX)
			return -ENOSYS;
		else
			return -EINVAL;
	}
}

and then the clock rate assignment would see -ENOSYS and go on its merry
way.

--Sean

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Tue, Nov 23, 2021 at 09:16:14PM -0500, Sean Anderson wrote:
> On 11/22/21 10:02 PM, Peng Fan (OSS) wrote:
> > > Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > 
> > > On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> > > > + Rob
> > > > 
> > > > On 2021/11/20 20:57, Tom Rini wrote:
> > > > > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > > > > 
> > > > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > > > 
> > > > > > > Current code has a force clk_set_defaults in multiple stages,
> > > > > > > U-Boot reuse the same device tree and Linux Kernel device tree,
> > > > > > > but we not register all the clks as Linux Kernel, so
> > > > > > > clk_set_defaults will fail and cause the clk provider registeration fail.
> > > > > > > 
> > > > > > > So introduce a new property to ignore the default settings which
> > > > > > > could be used by any node that wanna ignore default settings.
> > > > > > > 
> > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > > ---
> > > > > > > 
> > > > > > > V2:
> > > > > > >    Add R-b tag
> > > > > > >    Tom, Simon
> > > > > > >      After a thought, I think still put it as a u-boot thing.
> > > assigned-clock-x is
> > > > > > >      actually Linux specific, however I could not add the new property
> > > to Linux,
> > > > > > >      because we are supporting SystemReady-IR, we need the
> > > > > > > assigned-clock-x property
> > > > > > >      in linux working and ignore it in U-Boot.
> > > > > > 
> > > > > > Any more thoughts?
> > > > > 
> > > > > Just my continued request that you treat this as generic and submit
> > > > > the binding upstream so it can be in the device tree for the platform.
> > > > > 
> > > > 
> > > > As Sean said, this is to serve cast that linux and U-Boot use the same
> > > > device tree, I mean U-Boot runtime export device tree to linux for
> > > > SR-IR (system-ready IR) booting.
> > > > 
> > > > Linux needs assigned-clocks to some reason, but U-Boot not need that
> > > > because the driver not added the support or not a must to have that.
> > > > 
> > > > Because assigned-clocks failure in U-Boot will cause probe fail now,
> > > > the device driver will report failure.
> > > > 
> > > > You mean rename this to "ignore-clk-defaults" or keep
> > > > "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> > > > device tree binding?
> > > > 
> > > > I could try to send to linux kernel with "firmware" as a prefix.
> > > 
> > > What I mean is that first I'm not seeing the description of the property as
> > > being clear enough, either in commit message or the binding itself.
> > > That's why in my mind I keep seeing this as "we set the properties
> > > linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
> > > we need both.  Is there not something we can do based on seeing
> > > linux,assigned-clocks ?  Showing something that makes use of the property
> > > you're wishing to add would be helpful.  In fact, some specific dts snippets
> > > would be helpful to understand what's going on here.
> > 
> > clk: clock-controller@30380000 {
> >          compatible = "fsl,imx8mp-ccm";
> >          reg = <0x30380000 0x10000>;
> >          #clock-cells = <1>;
> >          clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
> >                   <&clk_ext3>, <&clk_ext4>;
> >          clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
> >                        "clk_ext3", "clk_ext4";
> >          assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
> >                            <&clk IMX8MP_CLK_A53_CORE>,
> >                            <&clk IMX8MP_CLK_NOC>,
> >                            <&clk IMX8MP_CLK_NOC_IO>,
> >                            <&clk IMX8MP_CLK_GIC>,
> >                            <&clk IMX8MP_CLK_AUDIO_AHB>,
> >                            <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
> >                            <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
> >                            <&clk IMX8MP_AUDIO_PLL1>,
> >                            <&clk IMX8MP_AUDIO_PLL2>;
> >          assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
> >                                   <&clk IMX8MP_ARM_PLL_OUT>,
> >                                   <&clk IMX8MP_SYS_PLL2_1000M>,
> >                                   <&clk IMX8MP_SYS_PLL1_800M>,
> >                                   <&clk IMX8MP_SYS_PLL2_500M>,
> >                                   <&clk IMX8MP_SYS_PLL1_800M>,
> >                                   <&clk IMX8MP_SYS_PLL1_800M>;
> >          assigned-clock-rates = <0>, <0>,
> >                                 <1000000000>,
> >                                 <800000000>,
> >                                 <500000000>,
> >                                 <400000000>,
> >                                 <800000000>,
> >                                 <400000000>,
> >                                 <393216000>,
> >                                 <361267200>;
> >          u-boot,ignore-clk-defaults;
> > };
> > 
> > The above node is that I wanna have in U-Boot device tree.
> > This node is same as the one used in linux device tree except the new
> > property introduced here.
> > 
> > In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
> > because U-Boot runtime export the device tree to Linux.
> > 
> > So add a new property here for U-Boot specific usage, if the property
> > is there, U-Boot ignore the assigned-clock settings for a node.
> > 
> > I hope this is clear and please suggest what to do next.
> 
> Hm. Well perhaps we can designate a specific return code to indicate
> that we have a valid clock ID but it is just unimplemented. So in your
> driver you would do
> 
> ulong my_get_rate(struct clk *clk)
> {
> 	switch (clk->id) {
> 	case MY_CLK:
> 		...
> 	default:
> 		if (clk->id < MY_CLK_MAX)
> 			return -ENOSYS;
> 		else
> 			return -EINVAL;
> 	}
> }
> 
> and then the clock rate assignment would see -ENOSYS and go on its merry
> way.

Or even just have the driver that matches on "fsl,imx8mp-ccm" have a
log_debug("Leaving clocks as-is, OS will handle them\n");
and returning success?

-- 
Tom

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

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson]

On 11/24/21 9:10 AM, Tom Rini wrote:
> On Tue, Nov 23, 2021 at 09:16:14PM -0500, Sean Anderson wrote:
>> On 11/22/21 10:02 PM, Peng Fan (OSS) wrote:
>>>> Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>>
>>>> On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
>>>>> + Rob
>>>>>
>>>>> On 2021/11/20 20:57, Tom Rini wrote:
>>>>>> On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
>>>>>>>> Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
>>>>>>>>
>>>>>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>>>>>
>>>>>>>> Current code has a force clk_set_defaults in multiple stages,
>>>>>>>> U-Boot reuse the same device tree and Linux Kernel device tree,
>>>>>>>> but we not register all the clks as Linux Kernel, so
>>>>>>>> clk_set_defaults will fail and cause the clk provider registeration fail.
>>>>>>>>
>>>>>>>> So introduce a new property to ignore the default settings which
>>>>>>>> could be used by any node that wanna ignore default settings.
>>>>>>>>
>>>>>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>>>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> V2:
>>>>>>>>     Add R-b tag
>>>>>>>>     Tom, Simon
>>>>>>>>       After a thought, I think still put it as a u-boot thing.
>>>> assigned-clock-x is
>>>>>>>>       actually Linux specific, however I could not add the new property
>>>> to Linux,
>>>>>>>>       because we are supporting SystemReady-IR, we need the
>>>>>>>> assigned-clock-x property
>>>>>>>>       in linux working and ignore it in U-Boot.
>>>>>>>
>>>>>>> Any more thoughts?
>>>>>>
>>>>>> Just my continued request that you treat this as generic and submit
>>>>>> the binding upstream so it can be in the device tree for the platform.
>>>>>>
>>>>>
>>>>> As Sean said, this is to serve cast that linux and U-Boot use the same
>>>>> device tree, I mean U-Boot runtime export device tree to linux for
>>>>> SR-IR (system-ready IR) booting.
>>>>>
>>>>> Linux needs assigned-clocks to some reason, but U-Boot not need that
>>>>> because the driver not added the support or not a must to have that.
>>>>>
>>>>> Because assigned-clocks failure in U-Boot will cause probe fail now,
>>>>> the device driver will report failure.
>>>>>
>>>>> You mean rename this to "ignore-clk-defaults" or keep
>>>>> "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
>>>>> device tree binding?
>>>>>
>>>>> I could try to send to linux kernel with "firmware" as a prefix.
>>>>
>>>> What I mean is that first I'm not seeing the description of the property as
>>>> being clear enough, either in commit message or the binding itself.
>>>> That's why in my mind I keep seeing this as "we set the properties
>>>> linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
>>>> we need both.  Is there not something we can do based on seeing
>>>> linux,assigned-clocks ?  Showing something that makes use of the property
>>>> you're wishing to add would be helpful.  In fact, some specific dts snippets
>>>> would be helpful to understand what's going on here.
>>>
>>> clk: clock-controller@30380000 {
>>>           compatible = "fsl,imx8mp-ccm";
>>>           reg = <0x30380000 0x10000>;
>>>           #clock-cells = <1>;
>>>           clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
>>>                    <&clk_ext3>, <&clk_ext4>;
>>>           clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
>>>                         "clk_ext3", "clk_ext4";
>>>           assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
>>>                             <&clk IMX8MP_CLK_A53_CORE>,
>>>                             <&clk IMX8MP_CLK_NOC>,
>>>                             <&clk IMX8MP_CLK_NOC_IO>,
>>>                             <&clk IMX8MP_CLK_GIC>,
>>>                             <&clk IMX8MP_CLK_AUDIO_AHB>,
>>>                             <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
>>>                             <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
>>>                             <&clk IMX8MP_AUDIO_PLL1>,
>>>                             <&clk IMX8MP_AUDIO_PLL2>;
>>>           assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
>>>                                    <&clk IMX8MP_ARM_PLL_OUT>,
>>>                                    <&clk IMX8MP_SYS_PLL2_1000M>,
>>>                                    <&clk IMX8MP_SYS_PLL1_800M>,
>>>                                    <&clk IMX8MP_SYS_PLL2_500M>,
>>>                                    <&clk IMX8MP_SYS_PLL1_800M>,
>>>                                    <&clk IMX8MP_SYS_PLL1_800M>;
>>>           assigned-clock-rates = <0>, <0>,
>>>                                  <1000000000>,
>>>                                  <800000000>,
>>>                                  <500000000>,
>>>                                  <400000000>,
>>>                                  <800000000>,
>>>                                  <400000000>,
>>>                                  <393216000>,
>>>                                  <361267200>;
>>>           u-boot,ignore-clk-defaults;
>>> };
>>>
>>> The above node is that I wanna have in U-Boot device tree.
>>> This node is same as the one used in linux device tree except the new
>>> property introduced here.
>>>
>>> In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
>>> because U-Boot runtime export the device tree to Linux.
>>>
>>> So add a new property here for U-Boot specific usage, if the property
>>> is there, U-Boot ignore the assigned-clock settings for a node.
>>>
>>> I hope this is clear and please suggest what to do next.
>>
>> Hm. Well perhaps we can designate a specific return code to indicate
>> that we have a valid clock ID but it is just unimplemented. So in your
>> driver you would do
>>
>> ulong my_get_rate(struct clk *clk)
>> {
>> 	switch (clk->id) {
>> 	case MY_CLK:
>> 		...
>> 	default:
>> 		if (clk->id < MY_CLK_MAX)
>> 			return -ENOSYS;
>> 		else
>> 			return -EINVAL;
>> 	}
>> }
>>
>> and then the clock rate assignment would see -ENOSYS and go on its merry
>> way.
> 
> Or even just have the driver that matches on "fsl,imx8mp-ccm" have a
> log_debug("Leaving clocks as-is, OS will handle them\n");
> and returning success?
> 

assigned-clocks is handled by the uclass, so that seems a bit invasive for me.

--Sean

Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults

[Tom Rini]

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

On Fri, Nov 26, 2021 at 01:17:10PM -0500, Sean Anderson wrote:
> On 11/24/21 9:10 AM, Tom Rini wrote:
> > On Tue, Nov 23, 2021 at 09:16:14PM -0500, Sean Anderson wrote:
> > > On 11/22/21 10:02 PM, Peng Fan (OSS) wrote:
> > > > > Subject: Re: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > > 
> > > > > On Mon, Nov 22, 2021 at 11:33:27AM +0800, Peng Fan (OSS) wrote:
> > > > > > + Rob
> > > > > > 
> > > > > > On 2021/11/20 20:57, Tom Rini wrote:
> > > > > > > On Sat, Nov 20, 2021 at 12:10:54PM +0000, Peng Fan (OSS) wrote:
> > > > > > > > > Subject: [PATCH V2] clk: introduce u-boot,ignore-clk-defaults
> > > > > > > > > 
> > > > > > > > > From: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > 
> > > > > > > > > Current code has a force clk_set_defaults in multiple stages,
> > > > > > > > > U-Boot reuse the same device tree and Linux Kernel device tree,
> > > > > > > > > but we not register all the clks as Linux Kernel, so
> > > > > > > > > clk_set_defaults will fail and cause the clk provider registeration fail.
> > > > > > > > > 
> > > > > > > > > So introduce a new property to ignore the default settings which
> > > > > > > > > could be used by any node that wanna ignore default settings.
> > > > > > > > > 
> > > > > > > > > Reviewed-by: Simon Glass <sjg@chromium.org>
> > > > > > > > > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > > > > > > > > ---
> > > > > > > > > 
> > > > > > > > > V2:
> > > > > > > > >     Add R-b tag
> > > > > > > > >     Tom, Simon
> > > > > > > > >       After a thought, I think still put it as a u-boot thing.
> > > > > assigned-clock-x is
> > > > > > > > >       actually Linux specific, however I could not add the new property
> > > > > to Linux,
> > > > > > > > >       because we are supporting SystemReady-IR, we need the
> > > > > > > > > assigned-clock-x property
> > > > > > > > >       in linux working and ignore it in U-Boot.
> > > > > > > > 
> > > > > > > > Any more thoughts?
> > > > > > > 
> > > > > > > Just my continued request that you treat this as generic and submit
> > > > > > > the binding upstream so it can be in the device tree for the platform.
> > > > > > > 
> > > > > > 
> > > > > > As Sean said, this is to serve cast that linux and U-Boot use the same
> > > > > > device tree, I mean U-Boot runtime export device tree to linux for
> > > > > > SR-IR (system-ready IR) booting.
> > > > > > 
> > > > > > Linux needs assigned-clocks to some reason, but U-Boot not need that
> > > > > > because the driver not added the support or not a must to have that.
> > > > > > 
> > > > > > Because assigned-clocks failure in U-Boot will cause probe fail now,
> > > > > > the device driver will report failure.
> > > > > > 
> > > > > > You mean rename this to "ignore-clk-defaults" or keep
> > > > > > "u-boot,ignore-clk-defauls" or "firmware,ignore-clk-defaults" to linux
> > > > > > device tree binding?
> > > > > > 
> > > > > > I could try to send to linux kernel with "firmware" as a prefix.
> > > > > 
> > > > > What I mean is that first I'm not seeing the description of the property as
> > > > > being clear enough, either in commit message or the binding itself.
> > > > > That's why in my mind I keep seeing this as "we set the properties
> > > > > linux,assigned-clocks and u-boot,ignore-clk-defaults" and I don't know why
> > > > > we need both.  Is there not something we can do based on seeing
> > > > > linux,assigned-clocks ?  Showing something that makes use of the property
> > > > > you're wishing to add would be helpful.  In fact, some specific dts snippets
> > > > > would be helpful to understand what's going on here.
> > > > 
> > > > clk: clock-controller@30380000 {
> > > >           compatible = "fsl,imx8mp-ccm";
> > > >           reg = <0x30380000 0x10000>;
> > > >           #clock-cells = <1>;
> > > >           clocks = <&osc_32k>, <&osc_24m>, <&clk_ext1>, <&clk_ext2>,
> > > >                    <&clk_ext3>, <&clk_ext4>;
> > > >           clock-names = "osc_32k", "osc_24m", "clk_ext1", "clk_ext2",
> > > >                         "clk_ext3", "clk_ext4";
> > > >           assigned-clocks = <&clk IMX8MP_CLK_A53_SRC>,
> > > >                             <&clk IMX8MP_CLK_A53_CORE>,
> > > >                             <&clk IMX8MP_CLK_NOC>,
> > > >                             <&clk IMX8MP_CLK_NOC_IO>,
> > > >                             <&clk IMX8MP_CLK_GIC>,
> > > >                             <&clk IMX8MP_CLK_AUDIO_AHB>,
> > > >                             <&clk IMX8MP_CLK_AUDIO_AXI_SRC>,
> > > >                             <&clk IMX8MP_CLK_IPG_AUDIO_ROOT>,
> > > >                             <&clk IMX8MP_AUDIO_PLL1>,
> > > >                             <&clk IMX8MP_AUDIO_PLL2>;
> > > >           assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
> > > >                                    <&clk IMX8MP_ARM_PLL_OUT>,
> > > >                                    <&clk IMX8MP_SYS_PLL2_1000M>,
> > > >                                    <&clk IMX8MP_SYS_PLL1_800M>,
> > > >                                    <&clk IMX8MP_SYS_PLL2_500M>,
> > > >                                    <&clk IMX8MP_SYS_PLL1_800M>,
> > > >                                    <&clk IMX8MP_SYS_PLL1_800M>;
> > > >           assigned-clock-rates = <0>, <0>,
> > > >                                  <1000000000>,
> > > >                                  <800000000>,
> > > >                                  <500000000>,
> > > >                                  <400000000>,
> > > >                                  <800000000>,
> > > >                                  <400000000>,
> > > >                                  <393216000>,
> > > >                                  <361267200>;
> > > >           u-boot,ignore-clk-defaults;
> > > > };
> > > > 
> > > > The above node is that I wanna have in U-Boot device tree.
> > > > This node is same as the one used in linux device tree except the new
> > > > property introduced here.
> > > > 
> > > > In i.MX U-Boot, we not support the clks here in the clk driver, but linux needs the assigned-clocks property, so I could not delete it
> > > > because U-Boot runtime export the device tree to Linux.
> > > > 
> > > > So add a new property here for U-Boot specific usage, if the property
> > > > is there, U-Boot ignore the assigned-clock settings for a node.
> > > > 
> > > > I hope this is clear and please suggest what to do next.
> > > 
> > > Hm. Well perhaps we can designate a specific return code to indicate
> > > that we have a valid clock ID but it is just unimplemented. So in your
> > > driver you would do
> > > 
> > > ulong my_get_rate(struct clk *clk)
> > > {
> > > 	switch (clk->id) {
> > > 	case MY_CLK:
> > > 		...
> > > 	default:
> > > 		if (clk->id < MY_CLK_MAX)
> > > 			return -ENOSYS;
> > > 		else
> > > 			return -EINVAL;
> > > 	}
> > > }
> > > 
> > > and then the clock rate assignment would see -ENOSYS and go on its merry
> > > way.
> > 
> > Or even just have the driver that matches on "fsl,imx8mp-ccm" have a
> > log_debug("Leaving clocks as-is, OS will handle them\n");
> > and returning success?
> > 
> 
> assigned-clocks is handled by the uclass, so that seems a bit invasive for me.

I'm probably missing something still, sorry.  We'd get kicked over to
drivers/clk/imx/clk-imx8mp.c in this case, from the uclass figuring out
who should own these clocks, right?  I'm wondering why / if clk-imx8mp.c
can't figure out what's appropriate to do (or not do, or otherwise just
ignore an error).

-- 
Tom

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