Re: [U-Boot] [U-Boot, 36/36] rockchip: add common board file for rockchip platform

[Dr. Philipp Tomsich]

Kever,

> On 13 Apr 2018, at 09:51, Kever Yang  wrote:
> 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

[Tom Rini]

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

[Kever Yang]

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

[Dr. Philipp Tomsich]

> On 10 Apr 2018, at 14:32, Tom Rini  wrote:
> 
> 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

[Tom Rini]

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

[Kever Yang]

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

[Dr. Philipp Tomsich]

Kever,

> On 8 Apr 2018, at 03:45, 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,

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

[Dr. Philipp Tomsich]

Kever,

> On 9 Apr 2018, at 00:35, Tom Rini  wrote:
> 
> 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

[Tom Rini]

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

[Kever Yang]

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

[Philipp Tomsich]

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 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/
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

[Philipp Tomsich]

> 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