| Message ID | 20180821143203.29142-12-lokeshvutla@ti.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Initial support Texas Instrument's AM654 Platform | expand |
On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: > From: Andreas Dannenberg <dannenberg@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@ti.com> > Signed-off-by: Lokesh Vutla <lokeshvutla@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! > -- > 2.18.0 > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
+Kever > On 24 Aug 2018, at 16:12, Tom Rini <trini@konsulko.com> wrote: > > On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: > >> From: Andreas Dannenberg <dannenberg@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@ti.com> >> Signed-off-by: Lokesh Vutla <lokeshvutla@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. Maybe we should have a SPL-specific property for the assigned-clocks to be set pre-reloc? > >> -- >> 2.18.0 >> >> _______________________________________________ >> U-Boot mailing list >> U-Boot@lists.denx.de >> https://lists.denx.de/listinfo/u-boot > > -- > Tom
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@konsulko.com> wrote: > > > > On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: > > > >> From: Andreas Dannenberg <dannenberg@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@ti.com> > >> Signed-off-by: Lokesh Vutla <lokeshvutla@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... -- Andreas Dannenberg Texas Instruments Inc
> On 24 Aug 2018, at 17:54, Andreas Dannenberg <dannenberg@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@konsulko.com> wrote: >>> >>> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: >>> >>>> From: Andreas Dannenberg <dannenberg@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@ti.com> >>>> Signed-off-by: Lokesh Vutla <lokeshvutla@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… 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.
Hi Philipp, On Fri, Aug 24, 2018 at 06:00:31PM +0200, Dr. Philipp Tomsich wrote: <snip> > >> 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… > > 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. Ok thanks for the offer. For what it's worth, I'm currently experimenting with not doing any relocation in SPL at all (which is really "only" stack/heap/bss/gd) as there is sufficient on-chip SRAM in our case to do so, if it works out this could be an alternative way to avoid the issue under discussion in the first place. Main reason however wanting to do that is that I started needing to use drivers that have globals and static variables which is when the wheels really came off trying to use those pre-reloc. I suppose should you use TPL for DDR init on RK devices there would also be no need for relocation in your SPL. -- Andreas Dannenberg Texas Instruments Inc
Hi Philipp, Andreas, On 08/25/2018 12:00 AM, Dr. Philipp Tomsich wrote: >> On 24 Aug 2018, at 17:54, Andreas Dannenberg <dannenberg@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@konsulko.com> wrote: >>>> >>>> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: >>>> >>>>> From: Andreas Dannenberg <dannenberg@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@ti.com> >>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@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(). What's the sram size in your SoC? 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@lists.denx.de > https://lists.denx.de/listinfo/u-boot
Hi Philipp, On Friday 24 August 2018 08:12 PM, Dr. Philipp Tomsich wrote: > +Kever > >> On 24 Aug 2018, at 16:12, Tom Rini <trini@konsulko.com> wrote: >> >> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: >> >>> From: Andreas Dannenberg <dannenberg@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@ti.com> >>> Signed-off-by: Lokesh Vutla <lokeshvutla@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. If you don't want to update clock rates in SPL, why can't CONFIG_OF_SPL_REMOVE_PROPS be used to delete not needed properties. > > 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. Right I agree to you point. but we should not restrict everyone to not update default clock rates in SPL. Can you see if OF_SPL_REMOVE_PROPS solves your problem or you have already tried it? Actually I don't mind dropping this patch for now as we are not using any default clock rates in SPL right now. Thanks and regards, Lokesh
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@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@konsulko.com> wrote: > >>>> > >>>> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: > >>>> > >>>>> From: Andreas Dannenberg <dannenberg@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@ti.com> > >>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@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@lists.denx.de > > https://lists.denx.de/listinfo/u-boot > >
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@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@konsulko.com> wrote: >>>>>> >>>>>> On Tue, Aug 21, 2018 at 08:01:49PM +0530, Lokesh Vutla wrote: >>>>>> >>>>>>> From: Andreas Dannenberg <dannenberg@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@ti.com> >>>>>>> Signed-off-by: Lokesh Vutla <lokeshvutla@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
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);