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

[Lokesh Vutla]

On Monday 27 August 2018 09:36 PM, Andreas Dannenberg wrote:
> 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?

Right. I still feel that $patch as such is fine.
CONFIG_OF_SPL_REMOVE_PROPS should be used to not set the clock rates in SPL.

Any opinions on this? We can repost this patch alone.

Thanks and regards,
Lokesh