[U-Boot] [PATCH 11/25] clk: Allow clock defaults to be set also during re-reloc state

Mon Aug 27 16:06:14 UTC 2018

Hi Kever,

On Mon, Aug 27, 2018 at 11:26:52AM +0800, Kever Yang wrote:
> Hi Philipp, Andreas,
> 
> 
> On 08/25/2018 12:00 AM, Dr. Philipp Tomsich wrote:
> >> On 24 Aug 2018, at 17:54, Andreas Dannenberg <dannenberg at ti.com> wrote:
> >>
> >> Philipp,
> >>
> >> On Fri, Aug 24, 2018 at 04:42:15PM +0200, Dr. Philipp Tomsich wrote:
> >>> +Kever
> >>>
> >>>> On 24 Aug 2018, at 16:12, Tom Rini <trini at konsulko.com> wrote:
> >>>>
> >>>> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote:
> >>>>
> >>>>> From: Andreas Dannenberg <dannenberg at ti.com>
> >>>>>
> >>>>> The earlier commit f4fcba5c5ba ("clk: implement clk_set_defaults()")
> >>>>> which introduced the functionality for setting clock defaults such as
> >>>>> rates and parents will skip the processing when executing in a re-reloc
> >>>>> state. This for example can prevent the assigning of clock parents
> >>>>> when running in SPL code. Go ahead and remove this limitation.
> >>>>>
> >>>>> Signed-off-by: Andreas Dannenberg <dannenberg at ti.com>
> >>>>> Signed-off-by: Lokesh Vutla <lokeshvutla at ti.com>
> >>>>> ---
> >>>>> drivers/clk/clk-uclass.c | 4 ----
> >>>>> 1 file changed, 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
> >>>>> index 2b15978e14..04b369aa5a 100644
> >>>>> --- a/drivers/clk/clk-uclass.c
> >>>>> +++ b/drivers/clk/clk-uclass.c
> >>>>> @@ -243,10 +243,6 @@ int clk_set_defaults(struct udevice *dev)
> >>>>> {
> >>>>> 	int ret;
> >>>>>
> >>>>> -	/* If this is running pre-reloc state, don't take any action. */
> >>>>> -	if (!(gd->flags & GD_FLG_RELOC))
> >>>>> -		return 0;
> >>>>> -
> >>>>> 	debug("%s(%s)\n", __func__, dev_read_name(dev));
> >>>>>
> >>>>> 	ret = clk_set_default_parents(dev);
> >>>> Philipp? David?  Comments?  Thanks!
> >>> If I remember correctly, David had a concern regarding an increase in
> >>> boottime if we ran this twice… adding Kever, as he was also involved
> >>> in the discussion.
> >>>
> >>> I settled on skipping it for pre-reloc, but it’s an imperfect solution: the
> >>> boottime increase comes from the fact that some devices have a large
> >>> number of assigned-clocks, that the device-tree processing has a cost,
> >>> and that we don’t have a way of synchronising between SPL and full
> >>> U-Boot to avoid redoing the complete init-flow.
> >> Good to know some of the background; when I did this patch initially it
> >> was not really clear why this was removed and it obviously was an issue
> >> for what I was doing that I had to overcome and re-adding this was the
> >> simpliest thing to do at that time.
> >>
> >>> Maybe we should have a SPL-specific property for the assigned-clocks
> >>> to be set pre-reloc?
> >> Need to think about this some more. Generally we probably want to do as
> >> little as possible before relocation. Unfortunately for the K3 family of
> >> SoCs much is dependent on loading/installing the system firmware (SYSFW)
> >> image including to get DDR operational which itself requires us to use a
> >> lot of DT/DM stuff pre-reloc. So a little bit of a chicken and egg
> >> problem…
> 'pre-reloc' is used in SPL and U-Boot before relocate, I think this
> should be
> clear to be as simple as possible, but now it's going to much like U-Boot
> proper. It have 2 problem: code size and boot time．
> Every feature will increase the code size, in Rockchip platform, we run SPL
> in sram which have size limit, and the common code of upstream feature
> update often break the size limit. In this case I want to add TPL for
> all Rockchip
> SoCs and used for ddr init only so that SPL will not have the size limit.
> For boot time, you need to understand that the module tag as 'pre-reloc'
> will
> be binded twice in U-Boot proper and also twice in SPL. I hope we only
> do things
> we need for SPL and U-Boot proper, but not too much overhead by framework.
> The API like clk_set_defaults() is able to used in every module, better
> to use in
> U-Boot Proper while the SPL should have a clear white list modules.
> 
> For example, we get everything from DTS for ddr node, but not using the
> clk_set_defaults().

As Lokesh noted in the other email, we technically no longer need the
patch to undo the setting of the clock defaults as we since hard-coded
the UART clock frequency which in an earlier (pre-public) version of our
tree was derived from the central "System Management Controller" and
was dependent on this patch.

Yes doing the same thing twice or three times is not efficient also from
a boot time POV. In addition, parsing the DT over and over takes a
significant amount of time (I work on "simulated" devices in a silicon
design environment so I know exactly how many minutes in that case are
spent parsing the DT... :)

But I think it would be cleaner to not abort clk_set_defaults() if
GD_FLG_RELOC is not set, but rather what Lokesh suggested to use
CONFIG_OF_SPL_REMOVE_PROPS? Would that work for RK?

> What's the sram size in your SoC?

There are multiple sections and blocks of SRAM [1] but for what U-Boot
SPL executing on the R5 is concerned there are essentially 512KB that
can be used which is pretty workable. Unlike most people I suppose, the
challenge we have is less with the size of SRAM, but rather with the
dependency to be able to do pretty much anything on the system controller
that needs to be brought up in the limited context of R5 SPL...

--
Andreas Dannenberg
Texas Instruments Inc

[1] http://www.ti.com/lit/pdf/spruid7

> 
> Thanks,
> - Kever
> > If you need someone to throw your thoughts at, feel free to share them with
> > me.  With the newer Rockchip devices that I am focused on, we also do a lot
> > DT/DM stuff pre-reloc (I have a 100+KB SPL stage on the RK3399 at the 
> > moment) and this trend is likely to increase … e.g. I may split the RK3399
> > to use TPL for DRAM-init and will then have the SPL stage running from
> > DRAM which will remove the last remaining size constraints.
> >
> > --Philipp.
> >
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> 
> 