Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Kever, > On 13 Apr 2018, at 09:51, Kever Yangwrote: > On 04/08/2018 09:45 AM, Kever Yang wrote: > +__weak int arch_cpu_init(void) +{ +return 0; +} + +__weak int rk_board_init_f(void) +{ +return 0; +} >>> This doesn't really help in modularising our board-support and I am >>> not a fan of adding something like 'rk_board_init_f' in the first place. >>> >>> Instead this should be implemented in a way that actually makes the >>> code structure easier and more resilient for the future (having __weak >>> functions at the architecture-level doesn't really help)... in fact >>> the only other uses of __weak in the U-Boot source-base are within >>> SPL, as there's no other way to provide hooks there. >> I know your proposal is to use DM for board init, then could you make it >> more >> clear about how to handle this in your solution? >> We need to do: >> - same board init flow for all rockchip platform; >> - something different but common in soc level; >> - something different in board level; > > I didn't see your response for this, could you send out your patches? This isn’t at the stage of a patch-set yet… I had asked for comments to this, so we could design this in a way that benefits all platforms. > I admit that I'm not very clear about the limitation of '__weak' function, > but I do see there are many '__weak' function in common/board_f/r.c, > and my common board file is connect to the board_r.c. I like __weak as a way to provide a hook for something that is part of the common API (so it’s ok, if spl.c uses this). However, I don’t want individual platforms to suddenly expose new extension points. And with the two examples above (arch_cpu_init and rk_board_init_f), you basically highlight what’s wrong about using __weak at this level: 1 arch_cpu_init is an extension point to do low-level initialisation for a CPU (not a board). Implementation for it usually live below arch/arm/cpu and takes care of things like MMU maintenance. Now we suddenly provide this below arch/arm/mach-rockchip … and using a __weak function. This goes against everything that users will expect. So just move it to arch/arm/cpu (you’ll probably need to have 2 separate ones for armv7 and armv8) and nothing unexpected will ever happen. 2 If we rk_board_init_f here, we are again changing the extension API of U-Boot: board_init_f belongs to each board (i.e. any board can expect to override it w/o ill effect), but now you’d suddenly create a link error. Instead users need to override rk_board_init_f. This is a documentation nightmare (and the current solution would be to provide a common function that all board_init_f implementations could call as their head or tail…). My question—as to whether the DM could/should be extended to CPUs, SoCs, architectures and boards—was meant to discuss exactly these observed issues in how boards and SoCs today interact. I usually don’t mind to touch APIs and extend them (or the driver model), but a solution for how to handle board/SoC/CPU init sequences is nothing I want to start before getting an actual design discussion going and reaching something resembling a consensus of how this aspect of U-Boot should be structured in a year’s (or two years’) time. > > @Simon, @Tom, > Could you kindly give some comment here? > > Thanks, > - Kever ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
On Fri, Apr 13, 2018 at 03:51:07PM +0800, Kever Yang wrote: > Hi Philipp, > > > On 04/08/2018 09:45 AM, Kever Yang wrote: > >>> +__weak int arch_cpu_init(void) > >>> +{ > >>> + return 0; > >>> +} > >>> + > >>> +__weak int rk_board_init_f(void) > >>> +{ > >>> + return 0; > >>> +} > >> This doesn't really help in modularising our board-support and I am > >> not a fan of adding something like 'rk_board_init_f' in the first place. > >> > >> Instead this should be implemented in a way that actually makes the > >> code structure easier and more resilient for the future (having __weak > >> functions at the architecture-level doesn't really help)... in fact > >> the only other uses of __weak in the U-Boot source-base are within > >> SPL, as there's no other way to provide hooks there. > > I know your proposal is to use DM for board init, then could you make it > > more > > clear about how to handle this in your solution? > > We need to do: > > - same board init flow for all rockchip platform; > > - something different but common in soc level; > > - something different in board level; > > I didn't see your response for this, could you send out your patches? > > I admit that I'm not very clear about the limitation of '__weak' function, > but I do see there are many '__weak' function in common/board_f/r.c, > and my common board file is connect to the board_r.c. > > @Simon, @Tom, > Could you kindly give some comment here? I am perhaps more of a fan of using weak functions than other people are. The problem with weak functions is that you must know when designing the code that it can safely only be overridden in one place per build. Otherwise the results are not predictable. -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Hi Philipp, On 04/08/2018 09:45 AM, Kever Yang wrote: >>> +__weak int arch_cpu_init(void) >>> +{ >>> + return 0; >>> +} >>> + >>> +__weak int rk_board_init_f(void) >>> +{ >>> + return 0; >>> +} >> This doesn't really help in modularising our board-support and I am >> not a fan of adding something like 'rk_board_init_f' in the first place. >> >> Instead this should be implemented in a way that actually makes the >> code structure easier and more resilient for the future (having __weak >> functions at the architecture-level doesn't really help)... in fact >> the only other uses of __weak in the U-Boot source-base are within >> SPL, as there's no other way to provide hooks there. > I know your proposal is to use DM for board init, then could you make it > more > clear about how to handle this in your solution? > We need to do: > - same board init flow for all rockchip platform; > - something different but common in soc level; > - something different in board level; I didn't see your response for this, could you send out your patches? I admit that I'm not very clear about the limitation of '__weak' function, but I do see there are many '__weak' function in common/board_f/r.c, and my common board file is connect to the board_r.c. @Simon, @Tom, Could you kindly give some comment here? Thanks, - Kever ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
> On 10 Apr 2018, at 14:32, Tom Riniwrote: > > On Tue, Apr 10, 2018 at 02:54:09PM +0800, Kever Yang wrote: >> Hi Tom, >> >> >> On 04/09/2018 06:35 AM, Tom Rini wrote: I have do a lot of test and re-work in my local branch and at last make it landed in rockchip vendor U-Boot, with testing in most of SoCs(not including rk3066/rk3188). Well, I do try to split it into pieces, but I found that actually not help very much except waste much more time: - The target is(very clear) to make rockchip soc board file in a good shape with common files, instead of copy-paste for each soc(more than 10 of them now) - then we need to identify what's common and what should go to soc and board; - remove using common rockchip timer and use arm generic timer instead for armv7 SoCs(rk3066 and rk3188 need still using rockchip timer) - most soc need to do uart init, boot order select, and some arch_cpu_init(). - don't break the boards already working, so I still leave some code which not so common in board file, but I would like to remove or move them into right place if I got a board to verify; @Simon, @Tom, This patch set is to remove some common files and add some other common files for all Rockchip SoCs, I have to make sure the whole patch set can running good for all SoCs, but it's really hard to make every patch to build and work perfect for all SoCs, is there any mandatory rules for this? >>> So you mean possibly breaking some existing platforms? I don't like the >>> idea of doing that... >> >> No, I'm not intent to to breaking some existing platforms, >> this patch set including 36 patches, all the platform should work fine >> after apply all these patches, but if only some of them applied, >> there is compile error or running error because of feature missing. > > OK. Similar to the Linux kernel, it's not a good thing to break > buildability of things during a patch series. Independent of this: this is not a single series, but multiple series rolled into one. Once the commit messages are reworked to convey what’s changed in a more meaningful way, this will be even more apparent than it already is today. Thanks, Philipp. ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
On Tue, Apr 10, 2018 at 02:54:09PM +0800, Kever Yang wrote: > Hi Tom, > > > On 04/09/2018 06:35 AM, Tom Rini wrote: > >> I have do a lot of test and re-work in my local branch and at last make > >> it landed in > >> rockchip vendor U-Boot, with testing in most of SoCs(not including > >> rk3066/rk3188). > >> Well, I do try to split it into pieces, but I found that actually not > >> help very much > >> except waste much more time: > >> - The target is(very clear) to make rockchip soc board file in a good > >> shape with common files, > >> instead of copy-paste for each soc(more than 10 of them now) > >> - then we need to identify what's common and what should go to soc and > >> board; > >> - remove using common rockchip timer and use arm generic timer instead > >> for armv7 > >> SoCs(rk3066 and rk3188 need still using rockchip timer) > >> - most soc need to do uart init, boot order select, and some > >> arch_cpu_init(). > >> - don't break the boards already working, so I still leave some code > >> which not so common > >> in board file, but I would like to remove or move them into right > >> place if I got a board > >> to verify; > >> > >> @Simon, @Tom, > >> This patch set is to remove some common files and add some other common > >> files for > >> all Rockchip SoCs, I have to make sure the whole patch set can running > >> good for all SoCs, > >> but it's really hard to make every patch to build and work perfect for > >> all SoCs, is there > >> any mandatory rules for this? > > So you mean possibly breaking some existing platforms? I don't like the > > idea of doing that... > > No, I'm not intent to to breaking some existing platforms, > this patch set including 36 patches, all the platform should work fine > after apply all these patches, but if only some of them applied, > there is compile error or running error because of feature missing. OK. Similar to the Linux kernel, it's not a good thing to break buildability of things during a patch series. -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Hi Tom, On 04/09/2018 06:35 AM, Tom Rini wrote: >> I have do a lot of test and re-work in my local branch and at last make >> it landed in >> rockchip vendor U-Boot, with testing in most of SoCs(not including >> rk3066/rk3188). >> Well, I do try to split it into pieces, but I found that actually not >> help very much >> except waste much more time: >> - The target is(very clear) to make rockchip soc board file in a good >> shape with common files, >> instead of copy-paste for each soc(more than 10 of them now) >> - then we need to identify what's common and what should go to soc and >> board; >> - remove using common rockchip timer and use arm generic timer instead >> for armv7 >> SoCs(rk3066 and rk3188 need still using rockchip timer) >> - most soc need to do uart init, boot order select, and some >> arch_cpu_init(). >> - don't break the boards already working, so I still leave some code >> which not so common >> in board file, but I would like to remove or move them into right >> place if I got a board >> to verify; >> >> @Simon, @Tom, >> This patch set is to remove some common files and add some other common >> files for >> all Rockchip SoCs, I have to make sure the whole patch set can running >> good for all SoCs, >> but it's really hard to make every patch to build and work perfect for >> all SoCs, is there >> any mandatory rules for this? > So you mean possibly breaking some existing platforms? I don't like the > idea of doing that... No, I'm not intent to to breaking some existing platforms, this patch set including 36 patches, all the platform should work fine after apply all these patches, but if only some of them applied, there is compile error or running error because of feature missing. Thanks, - Kever ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Kever, > On 8 Apr 2018, at 03:45, Kever Yangwrote: > > Philipp, > > > On 04/02/2018 05:28 AM, Philipp Tomsich wrote: >> >> >> On Tue, 27 Mar 2018, Kever Yang wrote: >> >>> We use common board/spl/tpl file for all rockchip SoCs, >>> - all the SoC spec setting should move into SoC file like rk3288.c; >>> - tpl is option and only purpose to init DRAM, clock, uart(option); >>> - spl do secure relate one time init, boot device select, boot into >>> U-Boot or trust or OS in falcon mode; >>> - board do boot mode detect, enable regulator, usb init and so on. >> >> There's too much going on in a single commit/single series. >> This needs to be split up into multiple, independent steps (e.g. one >> for the timer changes, another one for the UART changes)... > > I understand review the patches piece by piece is much more comfortable, In fact I do not like to do these reviews, as they are a tiresome chore… …but they need to be done, as some issues are best caught at this stage. Note, that a good commit message (i.e. one that summarises the status quo/presents the motivation for the specific change; then summarises what is changed how and to what effect; finally notes on anything that the reviewer/someone debugging in the future should know) helps very much in reviewing. However, a review can not catch all issues and once a patch-set gets to a certain level of complexity, it is likely to introduce unnecessary breakage that could have been avoided in a review (if there simply hadn’t been too many changes at once). Consequently, we need a clean history consisting of orthogonal changes so we can bisect and revert if necessary (and I’d prefer not to revert an entire series)… which again requires patches that are (a) in a healthy application-order and (b) do a well-defined number of things well. > and this patch including "too much" things. And I never expect this > patch set > can be merge quickly, but we have to do this ASAP before more SoC coming. The quickest way to get at least some of this merged quickly (e.g. the UART changes) is to have smaller series for these. > I have do a lot of test and re-work in my local branch and at last make > it landed in > rockchip vendor U-Boot, with testing in most of SoCs(not including > rk3066/rk3188). > Well, I do try to split it into pieces, but I found that actually not > help very much > except waste much more time: > - The target is(very clear) to make rockchip soc board file in a good > shape with common files, > instead of copy-paste for each soc(more than 10 of them now) > - then we need to identify what's common and what should go to soc and > board; > - remove using common rockchip timer and use arm generic timer instead > for armv7 > SoCs(rk3066 and rk3188 need still using rockchip timer) > - most soc need to do uart init, boot order select, and some > arch_cpu_init(). > - don't break the boards already working, so I still leave some code > which not so common > in board file, but I would like to remove or move them into right > place if I got a board > to verify; > > @Simon, @Tom, > This patch set is to remove some common files and add some other common > files for > all Rockchip SoCs, I have to make sure the whole patch set can running > good for all SoCs, > but it's really hard to make every patch to build and work perfect for > all SoCs, is there > any mandatory rules for this? > > I have to do a lot of temporary work for this like add temporary MACRO > for those SoCs > convert to use common code, and remove it after all the SoCs have > convert to use common > code, which have no any help for what we get at last, but it really cost > a lot of time. > >> >>> >>> Signed-off-by: Kever Yang >> >> See below for requested changes (beyond splitting this up). >> Reviewing this in this state is a real chore, so I'll probably have >> more comments, once I see this presented in more manageable parcels. >> >>> --- >>> >>> arch/arm/mach-rockchip/Makefile | 23 + >>> arch/arm/mach-rockchip/board.c | 136 >>> arch/arm/mach-rockchip/spl.c| 195 >>> >>> arch/arm/mach-rockchip/tpl.c| 111 +++ >>> 4 files changed, 445 insertions(+), 20 deletions(-) >>> create mode 100644 arch/arm/mach-rockchip/board.c >>> create mode 100644 arch/arm/mach-rockchip/spl.c >>> create mode 100644 arch/arm/mach-rockchip/tpl.c >>> >>> diff --git a/arch/arm/mach-rockchip/Makefile >>> b/arch/arm/mach-rockchip/Makefile >>> index e1b0519..3aba66c 100644 >>> --- a/arch/arm/mach-rockchip/Makefile >>> +++ b/arch/arm/mach-rockchip/Makefile >>> @@ -11,15 +11,8 @@ >>> obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o >>> obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o >>> >>> -obj-tpl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-tpl.o >>> -obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o >>> - >>>
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Kever, > On 9 Apr 2018, at 00:35, Tom Riniwrote: > > On Sun, Apr 08, 2018 at 09:45:22AM +0800, Kever Yang wrote: >> Philipp, >> >> >> On 04/02/2018 05:28 AM, Philipp Tomsich wrote: >>> >>> >>> On Tue, 27 Mar 2018, Kever Yang wrote: >>> We use common board/spl/tpl file for all rockchip SoCs, - all the SoC spec setting should move into SoC file like rk3288.c; - tpl is option and only purpose to init DRAM, clock, uart(option); - spl do secure relate one time init, boot device select, boot into U-Boot or trust or OS in falcon mode; - board do boot mode detect, enable regulator, usb init and so on. >>> >>> There's too much going on in a single commit/single series. >>> This needs to be split up into multiple, independent steps (e.g. one >>> for the timer changes, another one for the UART changes)... >> >> I understand review the patches piece by piece is much more comfortable, >> and this patch including "too much" things. And I never expect this >> patch set >> can be merge quickly, but we have to do this ASAP before more SoC coming. >> I have do a lot of test and re-work in my local branch and at last make >> it landed in >> rockchip vendor U-Boot, with testing in most of SoCs(not including >> rk3066/rk3188). >> Well, I do try to split it into pieces, but I found that actually not >> help very much >> except waste much more time: >> - The target is(very clear) to make rockchip soc board file in a good >> shape with common files, >> instead of copy-paste for each soc(more than 10 of them now) >> - then we need to identify what's common and what should go to soc and >> board; >> - remove using common rockchip timer and use arm generic timer instead >> for armv7 >> SoCs(rk3066 and rk3188 need still using rockchip timer) >> - most soc need to do uart init, boot order select, and some >> arch_cpu_init(). >> - don't break the boards already working, so I still leave some code >> which not so common >> in board file, but I would like to remove or move them into right >> place if I got a board >> to verify; Having a clean commit-history and the ability to bisect and revert individual patches is an important requirement for the overall project. I thought to have already provided you the needed guidance on how to get most fo this merged and highlighted where architectural discussions will be needed. So to get most of this merged quickly, you will need to break this up into series that are more manageable (this is likely to not be a full list): * the changes for split out the UART configs * the timer changes * adding a common board-file and switching boards over For the common board-file, you should add this first and then start moving SoCs over onto this new file. I believe (my memory may be wrong) to have commented so in some of the individual reviews. Finally, there’s a few architectural issues to discuss: 1. You are merging the “board”-files, but these are now merged across multiple SoCs and across multiple boards per SoC. This is already causing some fraying at the edges (e.g. the number of rk_* hooks and the weak functions added). We should very carefully consider how this will affect adding boards (which may have a specific market segment in mind and may—for lack of a better example—not want to have rockusb included) in the future. 2. The new weak-functions are causing a major headache: they are at odds with us trying to move to DM across the entire tree. I strongly believe that these weak functions will cause added debug issues in the short term and that they will eventually be replaced with more DM-like model in the long term. Thanks, Philipp. >> @Simon, @Tom, >> This patch set is to remove some common files and add some other common >> files for >> all Rockchip SoCs, I have to make sure the whole patch set can running >> good for all SoCs, >> but it's really hard to make every patch to build and work perfect for >> all SoCs, is there >> any mandatory rules for this? > > So you mean possibly breaking some existing platforms? I don't like the > idea of doing that... > > -- > Tom ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
On Sun, Apr 08, 2018 at 09:45:22AM +0800, Kever Yang wrote: > Philipp, > > > On 04/02/2018 05:28 AM, Philipp Tomsich wrote: > > > > > > On Tue, 27 Mar 2018, Kever Yang wrote: > > > >> We use common board/spl/tpl file for all rockchip SoCs, > >> - all the SoC spec setting should move into SoC file like rk3288.c; > >> - tpl is option and only purpose to init DRAM, clock, uart(option); > >> - spl do secure relate one time init, boot device select, boot into > >> U-Boot or trust or OS in falcon mode; > >> - board do boot mode detect, enable regulator, usb init and so on. > > > > There's too much going on in a single commit/single series. > > This needs to be split up into multiple, independent steps (e.g. one > > for the timer changes, another one for the UART changes)... > > I understand review the patches piece by piece is much more comfortable, > and this patch including "too much" things. And I never expect this > patch set > can be merge quickly, but we have to do this ASAP before more SoC coming. > I have do a lot of test and re-work in my local branch and at last make > it landed in > rockchip vendor U-Boot, with testing in most of SoCs(not including > rk3066/rk3188). > Well, I do try to split it into pieces, but I found that actually not > help very much > except waste much more time: > - The target is(very clear) to make rockchip soc board file in a good > shape with common files, > instead of copy-paste for each soc(more than 10 of them now) > - then we need to identify what's common and what should go to soc and > board; > - remove using common rockchip timer and use arm generic timer instead > for armv7 > SoCs(rk3066 and rk3188 need still using rockchip timer) > - most soc need to do uart init, boot order select, and some > arch_cpu_init(). > - don't break the boards already working, so I still leave some code > which not so common > in board file, but I would like to remove or move them into right > place if I got a board > to verify; > > @Simon, @Tom, > This patch set is to remove some common files and add some other common > files for > all Rockchip SoCs, I have to make sure the whole patch set can running > good for all SoCs, > but it's really hard to make every patch to build and work perfect for > all SoCs, is there > any mandatory rules for this? So you mean possibly breaking some existing platforms? I don't like the idea of doing that... -- Tom signature.asc Description: PGP signature ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
Philipp, On 04/02/2018 05:28 AM, Philipp Tomsich wrote: > > > On Tue, 27 Mar 2018, Kever Yang wrote: > >> We use common board/spl/tpl file for all rockchip SoCs, >> - all the SoC spec setting should move into SoC file like rk3288.c; >> - tpl is option and only purpose to init DRAM, clock, uart(option); >> - spl do secure relate one time init, boot device select, boot into >> U-Boot or trust or OS in falcon mode; >> - board do boot mode detect, enable regulator, usb init and so on. > > There's too much going on in a single commit/single series. > This needs to be split up into multiple, independent steps (e.g. one > for the timer changes, another one for the UART changes)... I understand review the patches piece by piece is much more comfortable, and this patch including "too much" things. And I never expect this patch set can be merge quickly, but we have to do this ASAP before more SoC coming. I have do a lot of test and re-work in my local branch and at last make it landed in rockchip vendor U-Boot, with testing in most of SoCs(not including rk3066/rk3188). Well, I do try to split it into pieces, but I found that actually not help very much except waste much more time: - The target is(very clear) to make rockchip soc board file in a good shape with common files, instead of copy-paste for each soc(more than 10 of them now) - then we need to identify what's common and what should go to soc and board; - remove using common rockchip timer and use arm generic timer instead for armv7 SoCs(rk3066 and rk3188 need still using rockchip timer) - most soc need to do uart init, boot order select, and some arch_cpu_init(). - don't break the boards already working, so I still leave some code which not so common in board file, but I would like to remove or move them into right place if I got a board to verify; @Simon, @Tom, This patch set is to remove some common files and add some other common files for all Rockchip SoCs, I have to make sure the whole patch set can running good for all SoCs, but it's really hard to make every patch to build and work perfect for all SoCs, is there any mandatory rules for this? I have to do a lot of temporary work for this like add temporary MACRO for those SoCs convert to use common code, and remove it after all the SoCs have convert to use common code, which have no any help for what we get at last, but it really cost a lot of time. > >> >> Signed-off-by: Kever Yang> > See below for requested changes (beyond splitting this up). > Reviewing this in this state is a real chore, so I'll probably have > more comments, once I see this presented in more manageable parcels. > >> --- >> >> arch/arm/mach-rockchip/Makefile | 23 + >> arch/arm/mach-rockchip/board.c | 136 >> arch/arm/mach-rockchip/spl.c | 195 >> >> arch/arm/mach-rockchip/tpl.c | 111 +++ >> 4 files changed, 445 insertions(+), 20 deletions(-) >> create mode 100644 arch/arm/mach-rockchip/board.c >> create mode 100644 arch/arm/mach-rockchip/spl.c >> create mode 100644 arch/arm/mach-rockchip/tpl.c >> >> diff --git a/arch/arm/mach-rockchip/Makefile >> b/arch/arm/mach-rockchip/Makefile >> index e1b0519..3aba66c 100644 >> --- a/arch/arm/mach-rockchip/Makefile >> +++ b/arch/arm/mach-rockchip/Makefile >> @@ -11,15 +11,8 @@ >> obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o >> obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o >> >> -obj-tpl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-tpl.o >> -obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o >> - >> -obj-spl-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board-spl.o >> -obj-spl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-spl.o >> -obj-spl-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board-spl.o >> -obj-spl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-spl.o >> -obj-spl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-spl.o >> spl-boot-order.o >> -obj-spl-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board-spl.o >> spl-boot-order.o >> +obj-tpl-y += tpl.o >> +obj-spl-y += spl.o spl-boot-order.o >> >> ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) >> >> @@ -28,21 +21,11 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) >> # we can have the preprocessor correctly recognise both 0x0 and 0 >> # meaning "turn it off". >> obj-y += boot_mode.o >> - >> -obj-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board.o >> -obj-$(CONFIG_ROCKCHIP_RK3128) += rk3128-board.o >> -obj-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board.o >> -obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board.o >> -obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board.o >> -obj-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board.o >> +obj-y += board.o >> endif >> >> obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram_common.o >> >> -ifndef CONFIG_ARM64 >> -obj-y += rk_timer.o >> -endif > > This would need to have gone with the rk_timer.c removal. > Otherwise things don't build between the earlier patch and here. > >> - >> obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036/ >>
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
On Tue, 27 Mar 2018, Kever Yang wrote: We use common board/spl/tpl file for all rockchip SoCs, - all the SoC spec setting should move into SoC file like rk3288.c; - tpl is option and only purpose to init DRAM, clock, uart(option); - spl do secure relate one time init, boot device select, boot into U-Boot or trust or OS in falcon mode; - board do boot mode detect, enable regulator, usb init and so on. There's too much going on in a single commit/single series. This needs to be split up into multiple, independent steps (e.g. one for the timer changes, another one for the UART changes)... Signed-off-by: Kever YangSee below for requested changes (beyond splitting this up). Reviewing this in this state is a real chore, so I'll probably have more comments, once I see this presented in more manageable parcels. --- arch/arm/mach-rockchip/Makefile | 23 + arch/arm/mach-rockchip/board.c | 136 arch/arm/mach-rockchip/spl.c| 195 arch/arm/mach-rockchip/tpl.c| 111 +++ 4 files changed, 445 insertions(+), 20 deletions(-) create mode 100644 arch/arm/mach-rockchip/board.c create mode 100644 arch/arm/mach-rockchip/spl.c create mode 100644 arch/arm/mach-rockchip/tpl.c diff --git a/arch/arm/mach-rockchip/Makefile b/arch/arm/mach-rockchip/Makefile index e1b0519..3aba66c 100644 --- a/arch/arm/mach-rockchip/Makefile +++ b/arch/arm/mach-rockchip/Makefile @@ -11,15 +11,8 @@ obj-spl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o obj-tpl-$(CONFIG_ROCKCHIP_BROM_HELPER) += bootrom.o -obj-tpl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-tpl.o -obj-tpl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-tpl.o - -obj-spl-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board-spl.o -obj-spl-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board-spl.o -obj-spl-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board-spl.o -obj-spl-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board-spl.o -obj-spl-$(CONFIG_ROCKCHIP_RK3368) += rk3368-board-spl.o spl-boot-order.o -obj-spl-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board-spl.o spl-boot-order.o +obj-tpl-y += tpl.o +obj-spl-y += spl.o spl-boot-order.o ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) @@ -28,21 +21,11 @@ ifeq ($(CONFIG_SPL_BUILD)$(CONFIG_TPL_BUILD),) # we can have the preprocessor correctly recognise both 0x0 and 0 # meaning "turn it off". obj-y += boot_mode.o - -obj-$(CONFIG_ROCKCHIP_RK3188) += rk3188-board.o -obj-$(CONFIG_ROCKCHIP_RK3128) += rk3128-board.o -obj-$(CONFIG_ROCKCHIP_RK322X) += rk322x-board.o -obj-$(CONFIG_ROCKCHIP_RK3288) += rk3288-board.o -obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036-board.o -obj-$(CONFIG_ROCKCHIP_RK3399) += rk3399-board.o +obj-y += board.o endif obj-$(CONFIG_$(SPL_TPL_)RAM) += sdram_common.o -ifndef CONFIG_ARM64 -obj-y += rk_timer.o -endif This would need to have gone with the rk_timer.c removal. Otherwise things don't build between the earlier patch and here. - obj-$(CONFIG_ROCKCHIP_RK3036) += rk3036/ obj-$(CONFIG_ROCKCHIP_RK3128) += rk3128/ ifndef CONFIG_TPL_BUILD diff --git a/arch/arm/mach-rockchip/board.c b/arch/arm/mach-rockchip/board.c new file mode 100644 index 000..52c6f66 --- /dev/null +++ b/arch/arm/mach-rockchip/board.c @@ -0,0 +1,136 @@ +/* + * (C) Copyright 2017 Rockchip Electronics Co., Ltd. + * + * SPDX-License-Identifier: GPL-2.0+ + */ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#ifdef CONFIG_DM_REGULATOR +#include +#endif + +DECLARE_GLOBAL_DATA_PTR; + +#if defined(CONFIG_USB_FUNCTION_FASTBOOT) +int fb_set_reboot_flag(void) +{ + printf("Setting reboot to fastboot flag ...\n"); + /* Set boot mode to fastboot */ + writel(BOOT_FASTBOOT, CONFIG_ROCKCHIP_BOOT_MODE_REG); + + return 0; +} + +#define FASTBOOT_KEY_GPIO 43 /* GPIO1_B3 */ +static int fastboot_key_pressed(void) +{ + gpio_request(FASTBOOT_KEY_GPIO, "fastboot_key"); + gpio_direction_input(FASTBOOT_KEY_GPIO); + return !gpio_get_value(FASTBOOT_KEY_GPIO); +} +#endif + +__weak int rk_board_init(void) +{ + return 0; +} + +__weak int rk_board_late_init(void) +{ + return 0; +} + +int board_late_init(void) +{ +#if defined(CONFIG_USB_FUNCTION_FASTBOOT) + if (fastboot_key_pressed()) { + printf("fastboot key pressed!\n"); + fb_set_reboot_flag(); + } +#endif + +#if (CONFIG_ROCKCHIP_BOOT_MODE_REG > 0) + setup_boot_mode(); +#endif + + return rk_board_late_init(); +} + +int board_init(void) +{ + int ret; + +#if !defined(CONFIG_SUPPORT_SPL) + board_debug_uart_init(); +#endif +#ifdef CONFIG_DM_REGULATOR + ret = regulators_enable_boot_on(false); + if (ret) + debug("%s: Cannot enable boot on regulator\n", __func__); +#endif + + return rk_board_init(); +} + +#if !defined(CONFIG_SYS_DCACHE_OFF) && !defined(CONFIG_ARM64) +void enable_caches(void) +{ + /* Enable D-cache. I-cache is
Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform
> We use common board/spl/tpl file for all rockchip SoCs, > - all the SoC spec setting should move into SoC file like rk3288.c; > - tpl is option and only purpose to init DRAM, clock, uart(option); > - spl do secure relate one time init, boot device select, boot into > U-Boot or trust or OS in falcon mode; > - board do boot mode detect, enable regulator, usb init and so on. > > Signed-off-by: Kever Yang> --- > > arch/arm/mach-rockchip/Makefile | 23 + > arch/arm/mach-rockchip/board.c | 136 > arch/arm/mach-rockchip/spl.c| 195 > > arch/arm/mach-rockchip/tpl.c| 111 +++ > 4 files changed, 445 insertions(+), 20 deletions(-) > create mode 100644 arch/arm/mach-rockchip/board.c > create mode 100644 arch/arm/mach-rockchip/spl.c > create mode 100644 arch/arm/mach-rockchip/tpl.c > Acked-by: Philipp Tomsich ___ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot