| Message ID | 1309770021-9908-1-git-send-email-grinberg@compulab.co.il |
|---|---|
| State | Changes Requested |
| Headers | show |
On Mon, Jul 04, 2011 at 12:00:19PM +0300, Igor Grinberg wrote: > CONFIG_MACH_TYPE can be used to set the machine type number in the > common arm code instead of setting it in the board code. > > Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> > --- > README | 12 ++++++++++++ > arch/arm/lib/board.c | 5 +++++ > 2 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/README b/README > index 446966d..a9ccb0a 100644 > --- a/README > +++ b/README > @@ -442,6 +442,18 @@ The following options need to be configured: > crash. This is needed for buggy hardware (uc101) where > no pull down resistor is connected to the signal IDE5V_DD7. > > + CONFIG_MACH_TYPE [relevant for ARM only] > + > + This option can be used to specify the machine type number > + as it appears in the ARM machine registry > + (see http://www.arm.linux.org.uk/developer/machines/). > + If this option is not defined, then your board code > + will have to set this up like: > + gd->bd->bi_arch_number = <mach type>; > + Note: This option is not suitable if you have multiple > + boards supported in a single configuration file and the > + machine type is runtime discoverable. > + > - vxWorks boot parameters: > > bootvx constructs a valid bootline using the following > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c > index 169dfeb..ee77d05 100644 > --- a/arch/arm/lib/board.c > +++ b/arch/arm/lib/board.c > @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) > > monitor_flash_len = _end_ofs; > debug ("monitor flash len: %08lX\n", monitor_flash_len); > + > +#ifdef CONFIG_MACH_TYPE > + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ > +#endif > + > board_init(); /* Setup chipselects */ > > #ifdef CONFIG_SERIAL_MULTI > -- > 1.7.3.4 > I'm curious, is it a feature that bd->bi_arch_number can be set at runtime? Do any boards actually make a decision about what value to set this to? If not, then maybe it should be a required value. I've submitted some patches that deal with the same sort of issue, so I'm interested in seeing that happens to this one.
Hi Harvey, Le 04/07/2011 23:06, Christopher Harvey a écrit : > On Mon, Jul 04, 2011 at 12:00:19PM +0300, Igor Grinberg wrote: >> CONFIG_MACH_TYPE can be used to set the machine type number in the >> common arm code instead of setting it in the board code. >> >> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> >> --- >> README | 12 ++++++++++++ >> arch/arm/lib/board.c | 5 +++++ >> 2 files changed, 17 insertions(+), 0 deletions(-) >> >> diff --git a/README b/README >> index 446966d..a9ccb0a 100644 >> --- a/README >> +++ b/README >> @@ -442,6 +442,18 @@ The following options need to be configured: >> crash. This is needed for buggy hardware (uc101) where >> no pull down resistor is connected to the signal IDE5V_DD7. >> >> + CONFIG_MACH_TYPE [relevant for ARM only] >> + >> + This option can be used to specify the machine type number >> + as it appears in the ARM machine registry >> + (see http://www.arm.linux.org.uk/developer/machines/). >> + If this option is not defined, then your board code >> + will have to set this up like: >> + gd->bd->bi_arch_number =<mach type>; >> + Note: This option is not suitable if you have multiple >> + boards supported in a single configuration file and the >> + machine type is runtime discoverable. >> + >> - vxWorks boot parameters: >> >> bootvx constructs a valid bootline using the following >> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >> index 169dfeb..ee77d05 100644 >> --- a/arch/arm/lib/board.c >> +++ b/arch/arm/lib/board.c >> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >> >> monitor_flash_len = _end_ofs; >> debug ("monitor flash len: %08lX\n", monitor_flash_len); >> + >> +#ifdef CONFIG_MACH_TYPE >> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >> +#endif >> + >> board_init(); /* Setup chipselects */ >> >> #ifdef CONFIG_SERIAL_MULTI >> -- >> 1.7.3.4 >> > I'm curious, is it a feature that bd->bi_arch_number can be set at > runtime? Do any boards actually make a decision about what value to > set this to? If not, then maybe it should be a required value. I've > submitted some patches that deal with the same sort of issue, so I'm > interested in seeing that happens to this one. Some boards indeed have a feature to set the mach_type at runtime, for example to run both the mainline linux kernel and a manufacturer one (manufacturers tend to use/expect fancy machine types). Amicalement,
Le 05/07/2011 00:03, Albert ARIBAUD a écrit : > Hi Harvey, Sorry Christopher, I mixed up. > Le 04/07/2011 23:06, Christopher Harvey a écrit : >> On Mon, Jul 04, 2011 at 12:00:19PM +0300, Igor Grinberg wrote: >>> CONFIG_MACH_TYPE can be used to set the machine type number in the >>> common arm code instead of setting it in the board code. >>> >>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> >>> --- >>> README | 12 ++++++++++++ >>> arch/arm/lib/board.c | 5 +++++ >>> 2 files changed, 17 insertions(+), 0 deletions(-) >>> >>> diff --git a/README b/README >>> index 446966d..a9ccb0a 100644 >>> --- a/README >>> +++ b/README >>> @@ -442,6 +442,18 @@ The following options need to be configured: >>> crash. This is needed for buggy hardware (uc101) where >>> no pull down resistor is connected to the signal IDE5V_DD7. >>> >>> + CONFIG_MACH_TYPE [relevant for ARM only] >>> + >>> + This option can be used to specify the machine type number >>> + as it appears in the ARM machine registry >>> + (see http://www.arm.linux.org.uk/developer/machines/). >>> + If this option is not defined, then your board code >>> + will have to set this up like: >>> + gd->bd->bi_arch_number =<mach type>; >>> + Note: This option is not suitable if you have multiple >>> + boards supported in a single configuration file and the >>> + machine type is runtime discoverable. >>> + >>> - vxWorks boot parameters: >>> >>> bootvx constructs a valid bootline using the following >>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >>> index 169dfeb..ee77d05 100644 >>> --- a/arch/arm/lib/board.c >>> +++ b/arch/arm/lib/board.c >>> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >>> >>> monitor_flash_len = _end_ofs; >>> debug ("monitor flash len: %08lX\n", monitor_flash_len); >>> + >>> +#ifdef CONFIG_MACH_TYPE >>> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >>> +#endif >>> + >>> board_init(); /* Setup chipselects */ >>> >>> #ifdef CONFIG_SERIAL_MULTI >>> -- >>> 1.7.3.4 >>> >> I'm curious, is it a feature that bd->bi_arch_number can be set at >> runtime? Do any boards actually make a decision about what value to >> set this to? If not, then maybe it should be a required value. I've >> submitted some patches that deal with the same sort of issue, so I'm >> interested in seeing that happens to this one. > > Some boards indeed have a feature to set the mach_type at runtime, for > example to run both the mainline linux kernel and a manufacturer one > (manufacturers tend to use/expect fancy machine types). > > Amicalement, Amicalement,
Dear Christopher Harvey, In message <20110704210619.GA3218@harvey-pc.matrox.com> you wrote: > > I'm curious, is it a feature that bd->bi_arch_number can be set at > runtime? Do any boards actually make a decision about what value to Yes, this is a feature. It comes in handy in a number of cases. > set this to? If not, then maybe it should be a required value. I've Why? > submitted some patches that deal with the same sort of issue, so I'm > interested in seeing that happens to this one. Sorry, I can't follow... Best regards, Wolfgang Denk
On 07/05/11 00:06, Christopher Harvey wrote: > On Mon, Jul 04, 2011 at 12:00:19PM +0300, Igor Grinberg wrote: >> CONFIG_MACH_TYPE can be used to set the machine type number in the >> common arm code instead of setting it in the board code. >> >> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> >> --- >> README | 12 ++++++++++++ >> arch/arm/lib/board.c | 5 +++++ >> 2 files changed, 17 insertions(+), 0 deletions(-) >> >> diff --git a/README b/README >> index 446966d..a9ccb0a 100644 >> --- a/README >> +++ b/README >> @@ -442,6 +442,18 @@ The following options need to be configured: >> crash. This is needed for buggy hardware (uc101) where >> no pull down resistor is connected to the signal IDE5V_DD7. >> >> + CONFIG_MACH_TYPE [relevant for ARM only] >> + >> + This option can be used to specify the machine type number >> + as it appears in the ARM machine registry >> + (see http://www.arm.linux.org.uk/developer/machines/). >> + If this option is not defined, then your board code >> + will have to set this up like: >> + gd->bd->bi_arch_number = <mach type>; >> + Note: This option is not suitable if you have multiple >> + boards supported in a single configuration file and the >> + machine type is runtime discoverable. >> + >> - vxWorks boot parameters: >> >> bootvx constructs a valid bootline using the following >> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >> index 169dfeb..ee77d05 100644 >> --- a/arch/arm/lib/board.c >> +++ b/arch/arm/lib/board.c >> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >> >> monitor_flash_len = _end_ofs; >> debug ("monitor flash len: %08lX\n", monitor_flash_len); >> + >> +#ifdef CONFIG_MACH_TYPE >> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >> +#endif >> + >> board_init(); /* Setup chipselects */ >> >> #ifdef CONFIG_SERIAL_MULTI >> -- >> 1.7.3.4 >> > I'm curious, is it a feature that bd->bi_arch_number can be set at > runtime? Yes, it is a feature, as Wolfgang already said. > Do any boards actually make a decision about what value to > set this to? Currently, at least, 2 boards (cm_t35 and keymile) use this feature. This does not mean that in the future there will be no more boards using it. > If not, then maybe it should be a required value. I've > submitted some patches that deal with the same sort of issue, so I'm > interested in seeing that happens to this one. If you're talking about this: "Make u-boot a bit easier for newbies to port", then no you are wrong, this patch has nothing to do with your patch, it does completely another thing (see the subject).
On Tue, Jul 05, 2011 at 12:16:12AM +0200, Wolfgang Denk wrote: > Dear Christopher Harvey, > > In message <20110704210619.GA3218@harvey-pc.matrox.com> you wrote: > > > > I'm curious, is it a feature that bd->bi_arch_number can be set at > > runtime? Do any boards actually make a decision about what value to > > Yes, this is a feature. It comes in handy in a number of cases. > > > set this to? If not, then maybe it should be a required value. I've > > Why? Because if every machine sets an essentially static value at runtime then it would be a nice compile-time check to do. But, there is no point since the bi_arch_number isn't fixed for each u-boot configuration. > > > submitted some patches that deal with the same sort of issue, so I'm > > interested in seeing that happens to this one. > > Sorry, I can't follow... I was refering to this patch: http://patchwork.ozlabs.org/patch/103149/ which is similar. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de > The rule on staying alive as a program manager is to give 'em a num- > ber or give 'em a date, but never give 'em both at once. -Chris
On 07/05/11 17:08, charvey@matrox.com wrote: > On Tue, Jul 05, 2011 at 12:16:12AM +0200, Wolfgang Denk wrote: >> Dear Christopher Harvey, >> >> In message <20110704210619.GA3218@harvey-pc.matrox.com> you wrote: >>> I'm curious, is it a feature that bd->bi_arch_number can be set at >>> runtime? Do any boards actually make a decision about what value to >> Yes, this is a feature. It comes in handy in a number of cases. >> >>> set this to? If not, then maybe it should be a required value. I've >> Why? > Because if every machine sets an essentially static value at runtime > then it would be a nice compile-time check to do. But, there is no > point since the bi_arch_number isn't fixed for each u-boot > configuration. Right. >>> submitted some patches that deal with the same sort of issue, so I'm >>> interested in seeing that happens to this one. >> Sorry, I can't follow... > I was refering to this patch: > http://patchwork.ozlabs.org/patch/103149/ > which is similar. No, you are wrong! It is not similar even a bit! It does completely different thing. Your patch: warns about machine type not set. My patch: just adds a configuration _option_ which you can use, but you don't have to. See... It is not the same!
Hi Igor, Le 04/07/2011 11:00, Igor Grinberg a écrit : > CONFIG_MACH_TYPE can be used to set the machine type number in the > common arm code instead of setting it in the board code. > > Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> > --- > README | 12 ++++++++++++ > arch/arm/lib/board.c | 5 +++++ > 2 files changed, 17 insertions(+), 0 deletions(-) > > diff --git a/README b/README > index 446966d..a9ccb0a 100644 > --- a/README > +++ b/README > @@ -442,6 +442,18 @@ The following options need to be configured: > crash. This is needed for buggy hardware (uc101) where > no pull down resistor is connected to the signal IDE5V_DD7. > > + CONFIG_MACH_TYPE [relevant for ARM only] > + > + This option can be used to specify the machine type number > + as it appears in the ARM machine registry > + (see http://www.arm.linux.org.uk/developer/machines/). > + If this option is not defined, then your board code > + will have to set this up like: > + gd->bd->bi_arch_number =<mach type>; > + Note: This option is not suitable if you have multiple > + boards supported in a single configuration file and the > + machine type is runtime discoverable. > + > - vxWorks boot parameters: > > bootvx constructs a valid bootline using the following > diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c > index 169dfeb..ee77d05 100644 > --- a/arch/arm/lib/board.c > +++ b/arch/arm/lib/board.c > @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) > > monitor_flash_len = _end_ofs; > debug ("monitor flash len: %08lX\n", monitor_flash_len); > + > +#ifdef CONFIG_MACH_TYPE > + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ > +#endif > + > board_init(); /* Setup chipselects */ > > #ifdef CONFIG_SERIAL_MULTI I don't really see the added value of having this configuration option. It is used in only one place for one line of code, which is a sign to me that it does not bring any substantial benefits. Amicalement,
On 07/06/11 21:53, Albert ARIBAUD wrote: > Hi Igor, > > Le 04/07/2011 11:00, Igor Grinberg a écrit : >> CONFIG_MACH_TYPE can be used to set the machine type number in the >> common arm code instead of setting it in the board code. >> >> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> >> --- >> README | 12 ++++++++++++ >> arch/arm/lib/board.c | 5 +++++ >> 2 files changed, 17 insertions(+), 0 deletions(-) >> >> diff --git a/README b/README >> index 446966d..a9ccb0a 100644 >> --- a/README >> +++ b/README >> @@ -442,6 +442,18 @@ The following options need to be configured: >> crash. This is needed for buggy hardware (uc101) where >> no pull down resistor is connected to the signal IDE5V_DD7. >> >> + CONFIG_MACH_TYPE [relevant for ARM only] >> + >> + This option can be used to specify the machine type number >> + as it appears in the ARM machine registry >> + (see http://www.arm.linux.org.uk/developer/machines/). >> + If this option is not defined, then your board code >> + will have to set this up like: >> + gd->bd->bi_arch_number =<mach type>; >> + Note: This option is not suitable if you have multiple >> + boards supported in a single configuration file and the >> + machine type is runtime discoverable. >> + >> - vxWorks boot parameters: >> >> bootvx constructs a valid bootline using the following >> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >> index 169dfeb..ee77d05 100644 >> --- a/arch/arm/lib/board.c >> +++ b/arch/arm/lib/board.c >> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >> >> monitor_flash_len = _end_ofs; >> debug ("monitor flash len: %08lX\n", monitor_flash_len); >> + >> +#ifdef CONFIG_MACH_TYPE >> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >> +#endif >> + >> board_init(); /* Setup chipselects */ >> >> #ifdef CONFIG_SERIAL_MULTI > > I don't really see the added value of having this configuration option. It is used in only one place for one line of code, which is a sign to me that it does not bring any substantial benefits. Well, this is something that came up when I tried to get rid of machine_is_* macros (patch 3/3), so we could change/cut/simplify the overgrown mach-types.h if we want/need to... I think there is an added value, may be it is hard to see it right now. If we have this option and it is documented, then any new board can use it instead of thinking (although it is simple) where and how to dereference the bi_arch_number. Also, it can come in handy, in the configuration system (I think it is called Kbuild?), so the value can be chosen from the list or something. The history of how this option was born is here: http://www.mail-archive.com/u-boot@lists.denx.de/msg52349.html and in this thread: http://www.mail-archive.com/u-boot@lists.denx.de/msg52396.html
Hi Igor, Le 06/07/2011 22:05, Igor Grinberg a écrit : > On 07/06/11 21:53, Albert ARIBAUD wrote: > >> Hi Igor, >> >> Le 04/07/2011 11:00, Igor Grinberg a écrit : >>> CONFIG_MACH_TYPE can be used to set the machine type number in the >>> common arm code instead of setting it in the board code. >>> >>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> >>> --- >>> README | 12 ++++++++++++ >>> arch/arm/lib/board.c | 5 +++++ >>> 2 files changed, 17 insertions(+), 0 deletions(-) >>> >>> diff --git a/README b/README >>> index 446966d..a9ccb0a 100644 >>> --- a/README >>> +++ b/README >>> @@ -442,6 +442,18 @@ The following options need to be configured: >>> crash. This is needed for buggy hardware (uc101) where >>> no pull down resistor is connected to the signal IDE5V_DD7. >>> >>> + CONFIG_MACH_TYPE [relevant for ARM only] >>> + >>> + This option can be used to specify the machine type number >>> + as it appears in the ARM machine registry >>> + (see http://www.arm.linux.org.uk/developer/machines/). >>> + If this option is not defined, then your board code >>> + will have to set this up like: >>> + gd->bd->bi_arch_number =<mach type>; >>> + Note: This option is not suitable if you have multiple >>> + boards supported in a single configuration file and the >>> + machine type is runtime discoverable. >>> + >>> - vxWorks boot parameters: >>> >>> bootvx constructs a valid bootline using the following >>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >>> index 169dfeb..ee77d05 100644 >>> --- a/arch/arm/lib/board.c >>> +++ b/arch/arm/lib/board.c >>> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >>> >>> monitor_flash_len = _end_ofs; >>> debug ("monitor flash len: %08lX\n", monitor_flash_len); >>> + >>> +#ifdef CONFIG_MACH_TYPE >>> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >>> +#endif >>> + >>> board_init(); /* Setup chipselects */ >>> >>> #ifdef CONFIG_SERIAL_MULTI >> >> I don't really see the added value of having this configuration option. It is used in only one place for one line of code, which is a sign to me that it does not bring any substantial benefits. > > Well, this is something that came up when I tried to get rid of machine_is_* > macros (patch 3/3), so we could change/cut/simplify the overgrown mach-types.h > if we want/need to... You've read and responded to this thread: http://old.nabble.com/-U-Boot--Update-and-Cut-down-mach-types-td31432177.html, so you know that mach-types.h should not be changed, cut or simplified, but should only be generated from the mach-types list pulled from the ARM Linux machine repository just like Linux' mach-types.h is generated (though we use the full list whereas Linux uses a reduced list, because U-Boot supports some ARM machines which Linux does not). If your goal is reducing the size of mach-types.h, there is a branch in u-boot-ti that is about this (Sandeep: has it been posted as patches for review?) > I think there is an added value, may be it is hard to see it right now. If it is hard to see, then maybe it is too small a value to be worth the effort -- that's what I am trying to sort out. > If we have this option and it is documented, then any new board can use it > instead of thinking (although it is simple) where and how to dereference > the bi_arch_number. Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? > Also, it can come in handy, in the configuration system (I think it is called Kbuild?), > so the value can be chosen from the list or something. AFAICT, the Linux build system does fine with the generated mach-type. Amicalement,
On 07/07/11 19:07, Albert ARIBAUD wrote: > Hi Igor, > > Le 06/07/2011 22:05, Igor Grinberg a écrit : >> On 07/06/11 21:53, Albert ARIBAUD wrote: >> >>> Hi Igor, >>> >>> Le 04/07/2011 11:00, Igor Grinberg a écrit : >>>> CONFIG_MACH_TYPE can be used to set the machine type number in the >>>> common arm code instead of setting it in the board code. >>>> >>>> Signed-off-by: Igor Grinberg<grinberg@compulab.co.il> >>>> --- >>>> README | 12 ++++++++++++ >>>> arch/arm/lib/board.c | 5 +++++ >>>> 2 files changed, 17 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/README b/README >>>> index 446966d..a9ccb0a 100644 >>>> --- a/README >>>> +++ b/README >>>> @@ -442,6 +442,18 @@ The following options need to be configured: >>>> crash. This is needed for buggy hardware (uc101) where >>>> no pull down resistor is connected to the signal IDE5V_DD7. >>>> >>>> + CONFIG_MACH_TYPE [relevant for ARM only] >>>> + >>>> + This option can be used to specify the machine type number >>>> + as it appears in the ARM machine registry >>>> + (see http://www.arm.linux.org.uk/developer/machines/). >>>> + If this option is not defined, then your board code >>>> + will have to set this up like: >>>> + gd->bd->bi_arch_number =<mach type>; >>>> + Note: This option is not suitable if you have multiple >>>> + boards supported in a single configuration file and the >>>> + machine type is runtime discoverable. >>>> + >>>> - vxWorks boot parameters: >>>> >>>> bootvx constructs a valid bootline using the following >>>> diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c >>>> index 169dfeb..ee77d05 100644 >>>> --- a/arch/arm/lib/board.c >>>> +++ b/arch/arm/lib/board.c >>>> @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) >>>> >>>> monitor_flash_len = _end_ofs; >>>> debug ("monitor flash len: %08lX\n", monitor_flash_len); >>>> + >>>> +#ifdef CONFIG_MACH_TYPE >>>> + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ >>>> +#endif >>>> + >>>> board_init(); /* Setup chipselects */ >>>> >>>> #ifdef CONFIG_SERIAL_MULTI >>> >>> I don't really see the added value of having this configuration option. It is used in only one place for one line of code, which is a sign to me that it does not bring any substantial benefits. >> >> Well, this is something that came up when I tried to get rid of machine_is_* >> macros (patch 3/3), so we could change/cut/simplify the overgrown mach-types.h >> if we want/need to... > > You've read and responded to this thread: http://old.nabble.com/-U-Boot--Update-and-Cut-down-mach-types-td31432177.html, so you know that mach-types.h should not be changed, cut or simplified, but should only be generated from the mach-types list pulled from the ARM Linux machine repository just like Linux' mach-types.h is generated (though we use the full list whereas Linux uses a reduced list, because U-Boot supports some ARM machines which Linux does not). If your goal is reducing the size of mach-types.h, there is a branch in u-boot-ti that is about this (Sandeep: has it been posted as patches for review?) Right, I've just tried to explain where it was originated from. It helps to simplify the mach_type logic for omap1610innovator and omap1610h2. Also, variants of it used locally by some other boards (e.g. all nvidia boards, smdk6400). Don't you think that generalizing this stuff is an added value? Instead of having multiple and undocumented variants of the same feature, we'd better provide a common and documented one, no? > >> I think there is an added value, may be it is hard to see it right now. > > If it is hard to see, then maybe it is too small a value to be worth the effort -- that's what I am trying to sort out. I've done it already... The effort is already made... Is there any other effort that I'm not aware of (besides of picking it up and merging)? > > >> If we have this option and it is documented, then any new board can use it >> instead of thinking (although it is simple) where and how to dereference >> the bi_arch_number. > > Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.), If you need Christopher's patch, then there are cases when the machid is not set, right? When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then goes to fix the code, but again he thinks, where is the best place to put it? For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial. With this patch, you get the explanation and also a place to put the machid definition. With this patch, you just define the configuration "variable" and the whole thing will be done for you. Another example would be the board/nvidia/*, the code is shared as much as possible, and the mach_type is set in the common code. That is something I would expect to be done for all ARM boards, not just for nvidia... > > >> Also, it can come in handy, in the configuration system (I think it is called Kbuild?), >> so the value can be chosen from the list or something. > > AFAICT, the Linux build system does fine with the generated mach-type. Here, I'm not talking about the mach_types, but about the config option. It can be set by the build system and no need to do it in board code!
Le 07/07/2011 18:51, Igor Grinberg a écrit : >>> If we have this option and it is documented, then any new board can use it >>> instead of thinking (although it is simple) where and how to dereference >>> the bi_arch_number. >> >> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? > > For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.), > If you need Christopher's patch, then there are cases when the machid is not set, right? > When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then > goes to fix the code, but again he thinks, where is the best place to put it? > For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial. > With this patch, you get the explanation and also a place to put the machid definition. > With this patch, you just define the configuration "variable" and the whole thing will be done for you. > Another example would be the board/nvidia/*, the code is shared as much as possible, > and the mach_type is set in the common code. That is something I would expect to be done > for all ARM boards, not just for nvidia... I see your point. Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(), but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged. IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity. Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either. Any suggestion for ensuring adoption of the feature wherever it can be used? >>> Also, it can come in handy, in the configuration system (I think it is called Kbuild?), >>> so the value can be chosen from the list or something. >> >> AFAICT, the Linux build system does fine with the generated mach-type. > > Here, I'm not talking about the mach_types, but about the config option. > It can be set by the build system and no need to do it in board code! Agreed. Amicalement,
On 07/07/11 20:46, Albert ARIBAUD wrote: > Le 07/07/2011 18:51, Igor Grinberg a écrit : > >>>> If we have this option and it is documented, then any new board can use it >>>> instead of thinking (although it is simple) where and how to dereference >>>> the bi_arch_number. >>> >>> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? >> >> For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.), >> If you need Christopher's patch, then there are cases when the machid is not set, right? >> When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then >> goes to fix the code, but again he thinks, where is the best place to put it? >> For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial. >> With this patch, you get the explanation and also a place to put the machid definition. >> With this patch, you just define the configuration "variable" and the whole thing will be done for you. >> Another example would be the board/nvidia/*, the code is shared as much as possible, >> and the mach_type is set in the common code. That is something I would expect to be done >> for all ARM boards, not just for nvidia... > > I see your point. > > Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(), Vast majority of boards set their bi_arch_number in board_init(). I went through all the boards and there is only one that set it in board_early_init_f() and several that do this in checkboard(). This makes me think of v2 of this patch which will set the bi_arch_number in board_init_f() just before the init_sequence[] array is run. > but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged. Well, I don't like to force people do something by breaking their builds... In general, I think that any change should not break any existing code (at least not on purpose). At least, this is how it works in Linux. > > IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity. > > Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either. Well, I can do the change board/* wide, but it will take some time to accomplish. Also, we still don't have an exact list of boards for removal, so I'd like to wait until the removal takes place, so there will be less boards to consider. > > Any suggestion for ensuring adoption of the feature wherever it can be used? Currently, I can think of: 1) Changing all relevant boards to use it - will eliminate "bad" examples. 2) Pointing to the use of CONFIG_MACH_TYPE during code review. 3) Introduce one more config option, like CONFIG_DYNAMIC_MACH_TYPE and change the patch to something like: #ifndef CONFIG_DYNAMIC_MACH_TYPE bi_arch_number = CONFIG_MACH_TYPE; #endif If we decide to go for 3), it would integrate nicely with Christopher's patch.
Hi Albert, On 07/08/11 00:06, Igor Grinberg wrote: > On 07/07/11 20:46, Albert ARIBAUD wrote: >> Le 07/07/2011 18:51, Igor Grinberg a écrit : >> >>>>> If we have this option and it is documented, then any new board can use it >>>>> instead of thinking (although it is simple) where and how to dereference >>>>> the bi_arch_number. >>>> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? >>> For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.), >>> If you need Christopher's patch, then there are cases when the machid is not set, right? >>> When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then >>> goes to fix the code, but again he thinks, where is the best place to put it? >>> For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial. >>> With this patch, you get the explanation and also a place to put the machid definition. >>> With this patch, you just define the configuration "variable" and the whole thing will be done for you. >>> Another example would be the board/nvidia/*, the code is shared as much as possible, >>> and the mach_type is set in the common code. That is something I would expect to be done >>> for all ARM boards, not just for nvidia... >> I see your point. >> >> Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(), > Vast majority of boards set their bi_arch_number in board_init(). > I went through all the boards and there is only one that set it in board_early_init_f() > and several that do this in checkboard(). > > This makes me think of v2 of this patch which will set the bi_arch_number in board_init_f() > just before the init_sequence[] array is run. > >> but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged. > Well, I don't like to force people do something by breaking their builds... > In general, I think that any change should not break any existing code (at least not on purpose). > At least, this is how it works in Linux. > >> IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity. >> >> Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either. > Well, I can do the change board/* wide, but it will take some time to accomplish. > Also, we still don't have an exact list of boards for removal, so I'd like to wait until > the removal takes place, so there will be less boards to consider. > >> Any suggestion for ensuring adoption of the feature wherever it can be used? > Currently, I can think of: > 1) Changing all relevant boards to use it - will eliminate "bad" examples. > 2) Pointing to the use of CONFIG_MACH_TYPE during code review. > 3) Introduce one more config option, like CONFIG_DYNAMIC_MACH_TYPE > and change the patch to something like: > #ifndef CONFIG_DYNAMIC_MACH_TYPE > bi_arch_number = CONFIG_MACH_TYPE; > #endif > > If we decide to go for 3), it would integrate nicely with Christopher's patch. So, what will it be? If it will be 1 and 2, then I'd like to get this patch in, so I can start working on 1. If it will be 3, then I want to make the change and resubmit, hoping for current merge window...
Hi Igor, Le 13/07/2011 07:52, Igor Grinberg a écrit : > Hi Albert, > > On 07/08/11 00:06, Igor Grinberg wrote: >> On 07/07/11 20:46, Albert ARIBAUD wrote: >>> Le 07/07/2011 18:51, Igor Grinberg a écrit : >>> >>>>>> If we have this option and it is documented, then any new board can use it >>>>>> instead of thinking (although it is simple) where and how to dereference >>>>>> the bi_arch_number. >>>>> Not sure I get you there. Can you elaborate on a more precise example that would show the benefits of it? >>>> For example, if you think of Christopher's patch (ARM: Warn when the machine ID isn't set.), >>>> If you need Christopher's patch, then there are cases when the machid is not set, right? >>>> When someone gets this warning, he thinks: "Ah, I forgot the machid!" and then >>>> goes to fix the code, but again he thinks, where is the best place to put it? >>>> For us, it is trivial, that it should be in board_init() function, but for newbies, it is not that trivial. >>>> With this patch, you get the explanation and also a place to put the machid definition. >>>> With this patch, you just define the configuration "variable" and the whole thing will be done for you. >>>> Another example would be the board/nvidia/*, the code is shared as much as possible, >>>> and the mach_type is set in the common code. That is something I would expect to be done >>>> for all ARM boards, not just for nvidia... >>> I see your point. >>> >>> Now the issue I foresee is that this commonalization has benefits only for boards which currently set their bi_arch_number in board_init_f(), >> Vast majority of boards set their bi_arch_number in board_init(). >> I went through all the boards and there is only one that set it in board_early_init_f() >> and several that do this in checkboard(). >> >> This makes me think of v2 of this patch which will set the bi_arch_number in board_init_f() >> just before the init_sequence[] array is run. >> >>> but has no incentive -- that's a code that will be used only in a few places and could stay that way for quite long, because boards that will not adhere to it will still build unchanged. >> Well, I don't like to force people do something by breaking their builds... >> In general, I think that any change should not break any existing code (at least not on purpose). >> At least, this is how it works in Linux. >> >>> IOW, there is no benefit for e.g. ED Mini V2, to use CONFIG_MACH_TYPE, so why would it? Thus instead of simplifying and commonalizing, this feature will *add* to the code base complexity. >>> >>> Unless the goal is to add this macro *and* change all related board codes in the same patchset? I don't see it as feasible either. >> Well, I can do the change board/* wide, but it will take some time to accomplish. >> Also, we still don't have an exact list of boards for removal, so I'd like to wait until >> the removal takes place, so there will be less boards to consider. >> >>> Any suggestion for ensuring adoption of the feature wherever it can be used? >> Currently, I can think of: >> 1) Changing all relevant boards to use it - will eliminate "bad" examples. >> 2) Pointing to the use of CONFIG_MACH_TYPE during code review. >> 3) Introduce one more config option, like CONFIG_DYNAMIC_MACH_TYPE >> and change the patch to something like: >> #ifndef CONFIG_DYNAMIC_MACH_TYPE >> bi_arch_number = CONFIG_MACH_TYPE; >> #endif >> >> If we decide to go for 3), it would integrate nicely with Christopher's patch. > > So, what will it be? > If it will be 1 and 2, then I'd like to get this patch in, so I can start working on 1. > > If it will be 3, then I want to make the change and resubmit, > hoping for current merge window... I don't think two macros are needed for this. Either the board config file targets a single Linux machine ID and it defines CONFIG_MACH_TYPE, or it targets several and somewhere in the board init code it sets bi_arch_number to one of some MACH_TYPE_xxxx values without defining CONFIG_MACH_TYPE, so this one macro is enough and ...DYNAMIC... would be redundant. Solution 1 would be the most correct IMO but is a lot of work for you as a submitter -- to be clear, I understand it as changing *every* board that sets bi_arch_number in board code to setting it in (lib and) config file. As much as I like it, I myself would hesitate to take on such an effort, so I will not force it upon you. Pragmatism against perfection: let's go for 2, then. Change the boards you intended to change, and from now on reviewers for any change to a board should point out the move to CONFIG_MACH_TYPE as mandatory. Amicalement,
Hi again Igor, Le 14/07/2011 16:10, Albert ARIBAUD a écrit : > Pragmatism against perfection: let's go for 2, then. Change the boards > you intended to change, and from now on reviewers for any change to a > board should point out the move to CONFIG_MACH_TYPE as mandatory. Forgot to mention: That makes patches 2/3 and 3/3 ok for acceptance, but can you post a V2 of patch 1/3 in which documentation of CONFIG_MACH_TYPE describes it as mandatory (and forbids direct setting of bi_arch_number) for board configs with a single machine-type? Thanks in advance, Amicalement,
On 07/14/11 17:20, Albert ARIBAUD wrote: > Hi again Igor, > > Le 14/07/2011 16:10, Albert ARIBAUD a écrit : > >> Pragmatism against perfection: let's go for 2, then. Change the boards >> you intended to change, and from now on reviewers for any change to a >> board should point out the move to CONFIG_MACH_TYPE as mandatory. > > Forgot to mention: > > That makes patches 2/3 and 3/3 ok for acceptance, but can you post a V2 of patch 1/3 in which documentation of CONFIG_MACH_TYPE describes it as mandatory (and forbids direct setting of bi_arch_number) for board configs with a single machine-type? Ok. I have to make it v2 anyway, because the setting of bi_arch_number in the board.c has to be moved to board_init_f() - before the init_sequence[] array is run.
diff --git a/README b/README index 446966d..a9ccb0a 100644 --- a/README +++ b/README @@ -442,6 +442,18 @@ The following options need to be configured: crash. This is needed for buggy hardware (uc101) where no pull down resistor is connected to the signal IDE5V_DD7. + CONFIG_MACH_TYPE [relevant for ARM only] + + This option can be used to specify the machine type number + as it appears in the ARM machine registry + (see http://www.arm.linux.org.uk/developer/machines/). + If this option is not defined, then your board code + will have to set this up like: + gd->bd->bi_arch_number = <mach type>; + Note: This option is not suitable if you have multiple + boards supported in a single configuration file and the + machine type is runtime discoverable. + - vxWorks boot parameters: bootvx constructs a valid bootline using the following diff --git a/arch/arm/lib/board.c b/arch/arm/lib/board.c index 169dfeb..ee77d05 100644 --- a/arch/arm/lib/board.c +++ b/arch/arm/lib/board.c @@ -451,6 +451,11 @@ void board_init_r (gd_t *id, ulong dest_addr) monitor_flash_len = _end_ofs; debug ("monitor flash len: %08lX\n", monitor_flash_len); + +#ifdef CONFIG_MACH_TYPE + bd->bi_arch_number = CONFIG_MACH_TYPE; /* board id for Linux */ +#endif + board_init(); /* Setup chipselects */ #ifdef CONFIG_SERIAL_MULTI
CONFIG_MACH_TYPE can be used to set the machine type number in the common arm code instead of setting it in the board code. Signed-off-by: Igor Grinberg <grinberg@compulab.co.il> --- README | 12 ++++++++++++ arch/arm/lib/board.c | 5 +++++ 2 files changed, 17 insertions(+), 0 deletions(-)