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

[Tom Rini <trini@konsulko.com>]

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