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, -- Albert. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
