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....@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 <s...@chromium.org>
Signed-off-by: Peng Fan <peng....@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
