[U-Boot,v2,05/21] pmic: Introduce power_board_init() method at ./lib/board.c file

It is necessary to introduce a new system wide function- power_board_init()

It turns out, that power initialization must be done as early as possible.
In the case of PMIC framework redesign, which aims to support multiple
instances of PMIC devices the initialization shall be performed just
after malloc configuration.

The arch_early_init_r() could be used instead, but it doesn't reflect
that the "power" subsystem is going to be initialized.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
Changes for v2:
- None
---
 arch/arm/lib/board.c |    4 ++++
 include/common.h     |    3 +++
 2 files changed, 7 insertions(+), 0 deletions(-)

On 05/10/2012 10:16, Lukasz Majewski wrote:
> It is necessary to introduce a new system wide function- power_board_init()
> 
> It turns out, that power initialization must be done as early as possible.
> In the case of PMIC framework redesign, which aims to support multiple
> instances of PMIC devices the initialization shall be performed just
> after malloc configuration.
> 
> The arch_early_init_r() could be used instead, but it doesn't reflect
> that the "power" subsystem is going to be initialized.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
> Changes for v2:
> - None

Hi Lucasz,

only to make some order. We have already several entries that calls
board specific functions.

There is at least board_init() and board_late_init(). The last one was
use mostly for the pmic initialization up nowm because it is called
after malloc_init().

Now you want to add a new one. This means we have cases where the PMIC
must be initialized before flash, right ?

We use often the "weak" mechanism to avoid that a board maintainer is
constrained to implement an empty function only to make happy the
linker. It is better to declare the function as weak and call it with
the same schema, such as board_power_init() (several board_* are weak)

> ---
>  arch/arm/lib/board.c |    4 ++++
>  include/common.h     |    3 +++
>  2 files changed, 7 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> index 109a1ac..431ef5b 100644
> --- a/arch/arm/lib/board.c
> +++ b/arch/arm/lib/board.c
> @@ -513,6 +513,10 @@ void board_init_r(gd_t *id, ulong dest_addr)
>  	arch_early_init_r();
>  #endif
>  
> +#ifdef CONFIG_POWER_INIT
> +	power_board_init();

Do we need a config option only for calling this ? I think that the
decision can be taken with CONFIG_PMIC, declaring the function as weak.

> +#endif
> +
>  #if !defined(CONFIG_SYS_NO_FLASH)
>  	puts("Flash: ");
>  
> diff --git a/include/common.h b/include/common.h
> index a7fb05e..5cc859f 100644
> --- a/include/common.h
> +++ b/include/common.h
> @@ -487,6 +487,9 @@ int board_late_init (void);
>  int board_postclk_init (void); /* after clocks/timebase, before env/serial */
>  int board_early_init_r (void);
>  void board_poweroff (void);
> +#ifdef CONFIG_POWER_INIT
> +int power_board_init(void);
> +#endif
>  
>  #if defined(CONFIG_SYS_DRAM_TEST)
>  int testdram(void);
> 

Best regards,
Stefano Babic

Hi Stefano,

> On 05/10/2012 10:16, Lukasz Majewski wrote:
> > It is necessary to introduce a new system wide function-
> > power_board_init()
> > 
> > It turns out, that power initialization must be done as early as
> > possible. In the case of PMIC framework redesign, which aims to
> > support multiple instances of PMIC devices the initialization shall
> > be performed just after malloc configuration.
> > 
> > The arch_early_init_r() could be used instead, but it doesn't
> > reflect that the "power" subsystem is going to be initialized.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> > Changes for v2:
> > - None
> 
> Hi Lucasz,
> 
> only to make some order. We have already several entries that calls
> board specific functions.
> 
> There is at least board_init() and board_late_init(). The last one was
> use mostly for the pmic initialization up nowm because it is called
> after malloc_init().
> 
> Now you want to add a new one. This means we have cases where the PMIC
> must be initialized before flash, right ?

The case is that PMIC (and other devices) needs to be setup after
malloc init and before MMC (in our case).
The board_init() is before malloc init and board_late_init() is after
MMC initialization for which working PMIC is necessary.
PMIC_v1 works since there is a single, static instance of PMIC device
(no lists needed).


Filling the above gap was my motivation to define special callback
for power.

In the ./arch/arm/lib/board.c there is 
#ifdef CONFIG_ARCH_EARLY_INIT_R
	arch_early_init_r();
#endif

Which could be used and defined at e.g. ./boards/samsung/trats.c
However it is only ARM specific (not available at PPC) and the name is
a bit misleading. (frankly speaking it is a dead code).



> 
> We use often the "weak" mechanism to avoid that a board maintainer is
> constrained to implement an empty function only to make happy the
> linker. It is better to declare the function as weak and call it with
> the same schema, such as board_power_init() (several board_* are weak)

Correct me if I'm wrong, but weren't we recently trying to remove
functions defined as weak? One of arguments was that weak function will
allow to build a u-boot.bin, which then crash at null pointer
dereference when proper CONFIG not selected.  

> 
> > ---
> >  arch/arm/lib/board.c |    4 ++++
> >  include/common.h     |    3 +++
> >  2 files changed, 7 insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c
> > index 109a1ac..431ef5b 100644
> > --- a/arch/arm/lib/board.c
> > +++ b/arch/arm/lib/board.c
> > @@ -513,6 +513,10 @@ void board_init_r(gd_t *id, ulong dest_addr)
> >  	arch_early_init_r();
> >  #endif
> >  
> > +#ifdef CONFIG_POWER_INIT
> > +	power_board_init();
> 
> Do we need a config option only for calling this ? I think that the
> decision can be taken with CONFIG_PMIC, declaring the function as
> weak.

This is one of options -> __weak power_board_init() would be defined
when no CONFIG_PMIC defined.
Good idea, since one CONFIG_POWER_* option would be removed. 

> 
> > +#endif
> > +
> >  #if !defined(CONFIG_SYS_NO_FLASH)
> >  	puts("Flash: ");
> >  
> > diff --git a/include/common.h b/include/common.h
> > index a7fb05e..5cc859f 100644
> > --- a/include/common.h
> > +++ b/include/common.h
> > @@ -487,6 +487,9 @@ int board_late_init (void);
> >  int board_postclk_init (void); /* after clocks/timebase, before
> > env/serial */ int board_early_init_r (void);
> >  void board_poweroff (void);
> > +#ifdef CONFIG_POWER_INIT
> > +int power_board_init(void);
> > +#endif
> >  
> >  #if defined(CONFIG_SYS_DRAM_TEST)
> >  int testdram(void);
> > 
> 
> Best regards,
> Stefano Babic
>

On 09/10/2012 12:25, Lukasz Majewski wrote:

>> Now you want to add a new one. This means we have cases where the PMIC
>> must be initialized before flash, right ?
> 
> The case is that PMIC (and other devices) needs to be setup after
> malloc init and before MMC (in our case).
> The board_init() is before malloc init and board_late_init() is after
> MMC initialization for which working PMIC is necessary.
> PMIC_v1 works since there is a single, static instance of PMIC device
> (no lists needed).

Ok

> 
> 
> Filling the above gap was my motivation to define special callback
> for power.
> 
> In the ./arch/arm/lib/board.c there is 
> #ifdef CONFIG_ARCH_EARLY_INIT_R
> 	arch_early_init_r();
> #endif

arch_early_init_r has nothing to do with power management

> 
> Which could be used and defined at e.g. ./boards/samsung/trats.c
> However it is only ARM specific (not available at PPC) and the name is
> a bit misleading. (frankly speaking it is a dead code).

Agree

>>
>> We use often the "weak" mechanism to avoid that a board maintainer is
>> constrained to implement an empty function only to make happy the
>> linker. It is better to declare the function as weak and call it with
>> the same schema, such as board_power_init() (several board_* are weak)
> 
> Correct me if I'm wrong, but weren't we recently trying to remove
> functions defined as weak?

I am not aware of it. I see several patches in last days adding weak
function. Several weak function are present in the recent added SPL
framework.

> One of arguments was that weak function will
> allow to build a u-boot.bin, which then crash at null pointer
> dereference when proper CONFIG not selected.  

I think that if there is a general agreement to drop the weak functions,
this must be done coherent for the whole code. However, I have not seen
that weak functions are rejected, and they are currently used in many parts.


>> Do we need a config option only for calling this ? I think that the
>> decision can be taken with CONFIG_PMIC, declaring the function as
>> weak.
> 
> This is one of options -> __weak power_board_init() would be defined
> when no CONFIG_PMIC defined.
> Good idea, since one CONFIG_POWER_* option would be removed. 

Right


Best regards,
Stefano Babic

On Tue, Oct 09, 2012 at 01:34:35PM +0200, Stefano Babic wrote:
> On 09/10/2012 12:25, Lukasz Majewski wrote:
[snip]
> >> We use often the "weak" mechanism to avoid that a board maintainer is
> >> constrained to implement an empty function only to make happy the
> >> linker. It is better to declare the function as weak and call it with
> >> the same schema, such as board_power_init() (several board_* are weak)
> > 
> > Correct me if I'm wrong, but weren't we recently trying to remove
> > functions defined as weak?
> 
> I am not aware of it. I see several patches in last days adding weak
> function. Several weak function are present in the recent added SPL
> framework.

We should use weak functions when we can provide a functioning in many
cases default but need to allow for overrides in some cases.  We
shouldn't use them when everyone needs to define the function and it has
to do something.

Hi Tom,

> On Tue, Oct 09, 2012 at 01:34:35PM +0200, Stefano Babic wrote:
> > On 09/10/2012 12:25, Lukasz Majewski wrote:
> [snip]
> > >> We use often the "weak" mechanism to avoid that a board
> > >> maintainer is constrained to implement an empty function only to
> > >> make happy the linker. It is better to declare the function as
> > >> weak and call it with the same schema, such as
> > >> board_power_init() (several board_* are weak)
> > > 
> > > Correct me if I'm wrong, but weren't we recently trying to remove
> > > functions defined as weak?
> > 
> > I am not aware of it. I see several patches in last days adding weak
> > function. Several weak function are present in the recent added SPL
> > framework.
> 
> We should use weak functions when we can provide a functioning in many
> cases default but need to allow for overrides in some cases.  We
> shouldn't use them when everyone needs to define the function and it
> has to do something.
> 

Ok, thanks for clarification :-).

I will define and use board_power_init() as a __weak function.