[U-Boot-Users] Breakage of board ports on new features.
Hi there,
So I'm cleaning my new 834x board in the time period of just two days.
There's a commit with a bunch of 834x related fixes which totally broke the board port.
I understand that the main code is volatile, but it seems there's a blatant disregard of keeping up a minimum of backwards compatibility.
If someone changes code that's supporting a bunch of board ports, please put in some default macros to ensure past behaviour is preserved.
I.e. in this case the CONFIG_MPC8349 & CFG_FLASH_SIZE could have easily been defined with the defaults values for the non updated board ports to continue working.
Please don't make our lives harder than they are.
/me steps down the soapbox.
Regards
Pantelis
In message CF774E5E-8B40-432B-A6CE-A0788CE20BF3@gmail.com you wrote:
There's a commit with a bunch of 834x related fixes which totally broke the board port.
I understand that the main code is volatile, but it seems there's a blatant disregard of keeping up a minimum of backwards compatibility.
If someone changes code that's supporting a bunch of board ports, please put in some default macros to ensure past behaviour is preserved.
Yes, it would be nice if everybody would work this way, but you know how things are going.
On the other hand, the merge of the 83xx code has been discussed here for a *long* time - you could have raised your concerns a long time ago. I can't help it if you don't participate in this discussions and/or help testing new code.
Best regards,
Wolfgang Denk
On Dec 2, 2006, at 5:42 PM, Wolfgang Denk wrote:
In message CF774E5E-8B40-432B-A6CE-A0788CE20BF3@gmail.com you wrote:
There's a commit with a bunch of 834x related fixes which totally broke the board port.
I understand that the main code is volatile, but it seems there's a blatant disregard of keeping up a minimum of backwards compatibility.
If someone changes code that's supporting a bunch of board ports, please put in some default macros to ensure past behaviour is preserved.
Yes, it would be nice if everybody would work this way, but you know how things are going.
On the other hand, the merge of the 83xx code has been discussed here for a *long* time - you could have raised your concerns a long time ago. I can't help it if you don't participate in this discussions and/or help testing new code.
Best regards,
Wolfgang Denk
I have a patch to remove CONFIG_MPC8349 since its redundant with CONFIG_MPC834X and CONFIG_MPC834X makes more sense to me.
- k
Kumar Gala wrote:
I have a patch to remove CONFIG_MPC8349 since its redundant with CONFIG_MPC834X and CONFIG_MPC834X makes more sense to me.
Grepping the code for CONFIG_MPC8349 brings up dozens of references, most of them in immap_83xx.h. I just received a patch from Dave Liu that eliminates most of those, but it's a huge patch and will take me time to review.
Are you saying that everywhere there's a CONFIG_MPC8349, CONFIG_MPC834X would also work?
On Dec 4, 2006, at 11:54 AM, Timur Tabi wrote:
Kumar Gala wrote:
I have a patch to remove CONFIG_MPC8349 since its redundant with CONFIG_MPC834X and CONFIG_MPC834X makes more sense to me.
Grepping the code for CONFIG_MPC8349 brings up dozens of references, most of them in immap_83xx.h. I just received a patch from Dave Liu that eliminates most of those, but it's a huge patch and will take me time to review.
Are you saying that everywhere there's a CONFIG_MPC8349, CONFIG_MPC834X would also work?
Yes, and I already have that patch and will be posting it shortly.
- k
In message 8978FAC3-D15B-4D54-9AAD-AB84F2E5F182@kernel.crashing.org you wrote:
Are you saying that everywhere there's a CONFIG_MPC8349, CONFIG_MPC834X would also work?
Yes, and I already have that patch and will be posting it shortly.
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x?
What is the exact problem you're addressing?
Best regards,
Wolfgang Denk
Wolfgang Denk wrote:
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x?
What is the exact problem you're addressing?
I think Kumar's point is that the code that's correctly marked with CONFIG_MPC8349 is not 8349-specific. It's 834x-specific, and there already is a macro for 834x. If someone were to add support for an 8343 or 8347, they would need to apply Kumar's patch anyway.
*IF* some of this code is really 8349-specific, then the person adding support for the 8343 or 8347 would need to modify this code again. However, I don't think that's going to happen.
On Dec 4, 2006, at 5:20 PM, Timur Tabi wrote:
Wolfgang Denk wrote:
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x? What is the exact problem you're addressing?
I think Kumar's point is that the code that's correctly marked with CONFIG_MPC8349 is not 8349-specific. It's 834x-specific, and there already is a macro for 834x. If someone were to add support for an 8343 or 8347, they would need to apply Kumar's patch anyway.
*IF* some of this code is really 8349-specific, then the person adding support for the 8343 or 8347 would need to modify this code again. However, I don't think that's going to happen.
This is exactly what I'm saying. The CONFIG_MPC8349 was too specific and really meant CONFIG_MPC834X and thus I changed it. If/when someone's got something that is MPC8349 specific they can re- introduce CONFIG_MPC8349.
The only feature that is unique to 8349 is the 2nd PCI controller which we handle with CONFIG_MPC83XX_PCI2. I'm hard pressed to see anyone actually every needing CONFIG_MPC8349 (but, now that I've stated that someone will come up with one).
- k
Kumar Gala wrote:
On Dec 4, 2006, at 5:20 PM, Timur Tabi wrote:
Wolfgang Denk wrote:
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x? What is the exact problem you're addressing?
I think Kumar's point is that the code that's correctly marked with CONFIG_MPC8349 is not 8349-specific. It's 834x-specific, and there already is a macro for 834x. If someone were to add support for an 8343 or 8347, they would need to apply Kumar's patch anyway.
*IF* some of this code is really 8349-specific, then the person adding support for the 8343 or 8347 would need to modify this code again. However, I don't think that's going to happen.
This is exactly what I'm saying. The CONFIG_MPC8349 was too specific and really meant CONFIG_MPC834X and thus I changed it. If/when someone's got something that is MPC8349 specific they can re- introduce CONFIG_MPC8349.
The AMCC 4XX based boards we define CONFIG_PPC4XX (for the family) as well as CONFIG_PPC405GP (for example) for specific processor support. The X'd version is used to enable common code while the specific version is used whenever divergences exist from the common code. Right now there might not be a specific difference from MPC8349 code but it might be good to define both MPC8349 and MPC834X for sake of consistency and for future proofing. Without defining the specific version when it is time to introduce that divergence you would wonder what boards were using the MPC8349 and what not (unless the board name gives it away).
Tolunay
On Dec 4, 2006, at 7:13 PM, Tolunay Orkun wrote:
Kumar Gala wrote:
On Dec 4, 2006, at 5:20 PM, Timur Tabi wrote:
Wolfgang Denk wrote:
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x? What is the exact problem you're addressing?
I think Kumar's point is that the code that's correctly marked with CONFIG_MPC8349 is not 8349-specific. It's 834x-specific, and there already is a macro for 834x. If someone were to add support for an 8343 or 8347, they would need to apply Kumar's patch anyway.
*IF* some of this code is really 8349-specific, then the person adding support for the 8343 or 8347 would need to modify this code again. However, I don't think that's going to happen.
This is exactly what I'm saying. The CONFIG_MPC8349 was too specific and really meant CONFIG_MPC834X and thus I changed it. If/when someone's got something that is MPC8349 specific they can re- introduce CONFIG_MPC8349.
The AMCC 4XX based boards we define CONFIG_PPC4XX (for the family) as well as CONFIG_PPC405GP (for example) for specific processor support. The X'd version is used to enable common code while the specific version is used whenever divergences exist from the common code. Right now there might not be a specific difference from MPC8349 code but it might be good to define both MPC8349 and MPC834X for sake of consistency and for future proofing. Without defining the specific version when it is time to introduce that divergence you would wonder what boards were using the MPC8349 and what not (unless the board name gives it away).
The concept is nice, but I think we should add it when its needed. The differences between the MPC834X family of processors are handled by features and not by specifying the specific processor. This is because even on the specific processor you can choose to enable or disable the feature (PCI2, 32/64-bit ddr, etc.).
When someone comes up with a good reason for having a CONFIG_MPC8349 then we should add it, until then I think its likely to get used incorrectly (as it already has been).
- k
Kumar Gala wrote:
On Dec 4, 2006, at 7:13 PM, Tolunay Orkun wrote:
Kumar Gala wrote:
On Dec 4, 2006, at 5:20 PM, Timur Tabi wrote:
Wolfgang Denk wrote:
Are you absolutely sure we will *never* want to make a difference between a MPC8349 and any other type of MPC834x? What is the exact problem you're addressing?
I think Kumar's point is that the code that's correctly marked with CONFIG_MPC8349 is not 8349-specific. It's 834x-specific, and there already is a macro for 834x. If someone were to add support for an 8343 or 8347, they would need to apply Kumar's patch anyway.
*IF* some of this code is really 8349-specific, then the person adding support for the 8343 or 8347 would need to modify this code again. However, I don't think that's going to happen.
This is exactly what I'm saying. The CONFIG_MPC8349 was too specific and really meant CONFIG_MPC834X and thus I changed it. If/when someone's got something that is MPC8349 specific they can re- introduce CONFIG_MPC8349.
The AMCC 4XX based boards we define CONFIG_PPC4XX (for the family) as well as CONFIG_PPC405GP (for example) for specific processor support. The X'd version is used to enable common code while the specific version is used whenever divergences exist from the common code. Right now there might not be a specific difference from MPC8349 code but it might be good to define both MPC8349 and MPC834X for sake of consistency and for future proofing. Without defining the specific version when it is time to introduce that divergence you would wonder what boards were using the MPC8349 and what not (unless the board name gives it away).
The concept is nice, but I think we should add it when its needed. The differences between the MPC834X family of processors are handled by features and not by specifying the specific processor. This is because even on the specific processor you can choose to enable or disable the feature (PCI2, 32/64-bit ddr, etc.).
Well, you can have still have the feature enable/disable. The CONFIG_MPC8349 or whatever would make sure the feature could not be applied on a cpu architecture/platform that is not applicable. Basically it prevents configuration errors.
When someone comes up with a good reason for having a CONFIG_MPC8349 then we should add it, until then I think its likely to get used incorrectly (as it already has been).
What I am saying is that you should rename existing CONFIG_MPC8349 to CONFIG_MPC834X and only on the boards that actually use 8349 define the CONFIG_MPC8349 (in addition, not in lieu of). Of course other boards using other member (8343, 8347) need to have their specific CONFIG_MPC8343 and CONFIG_MPC8347 options defined appropriately as well (again in addition to CONFIG_MPC834X). It is better to do this while the number of boards is small.
Anyway, I just pointed to the existing practice in U-Boot. In the end, it's probably Wolfgang's decision as U-Boot project leader.
Tolunay
In message 45750057.3030400@orkun.us you wrote:
What I am saying is that you should rename existing CONFIG_MPC8349 to CONFIG_MPC834X and only on the boards that actually use 8349 define the CONFIG_MPC8349 (in addition, not in lieu of). Of course other boards using other member (8343, 8347) need to have their specific CONFIG_MPC8343 and CONFIG_MPC8347 options defined appropriately as well (again in addition to CONFIG_MPC834X). It is better to do this while the number of boards is small.
Anyway, I just pointed to the existing practice in U-Boot. In the end, it's probably Wolfgang's decision as U-Boot project leader.
You said exactly what I had in mind. Just much better.
Thanks.
Best regards,
Wolfgang Denk
On Dec 4, 2006, at 11:32 PM, Wolfgang Denk wrote:
In message 45750057.3030400@orkun.us you wrote:
What I am saying is that you should rename existing CONFIG_MPC8349 to CONFIG_MPC834X and only on the boards that actually use 8349 define the CONFIG_MPC8349 (in addition, not in lieu of). Of course other boards using other member (8343, 8347) need to have their specific CONFIG_MPC8343 and CONFIG_MPC8347 options defined appropriately as well (again in addition to CONFIG_MPC834X). It is better to do this while the number of boards is small.
Anyway, I just pointed to the existing practice in U-Boot. In the end, it's probably Wolfgang's decision as U-Boot project leader.
You said exactly what I had in mind. Just much better.
Ok, but I think the several of the boards are capable of running with all flavors of MPC834x what do we do for them? I know the MDS is and I'm questing TQM834x is as well?
I understand the desire, I just feel that it doesn't add anything because we've done a decent job of making the differences between them covered by feature configs.
- k
In message F245C92A-F4A7-40AF-A73B-EB453262CB55@kernel.crashing.org you wrote:
Ok, but I think the several of the boards are capable of running with all flavors of MPC834x what do we do for them? I know the MDS is and I'm questing TQM834x is as well?
I think so, but I cannot answer the before I've seen what TQ does on the TQM8347 :-(
I understand the desire, I just feel that it doesn't add anything because we've done a decent job of making the differences between them covered by feature configs.
As Tolunay explained: it's for consistency with existing boards. And I really think it does not hurt ao much to have both CONFIG_MPC834X and CONFIG_MPC8349 in the board config file if this is a 8349 only board, even if CONFIG_MPC8349 should never be used anywhere in the code.
Best regards,
Wolfgang Denk
On Dec 5, 2006, at 9:35 AM, Wolfgang Denk wrote:
In message <F245C92A-F4A7-40AF-A73B- EB453262CB55@kernel.crashing.org> you wrote:
Ok, but I think the several of the boards are capable of running with all flavors of MPC834x what do we do for them? I know the MDS is and I'm questing TQM834x is as well?
I think so, but I cannot answer the before I've seen what TQ does on the TQM8347 :-(
Are you ok with having CONFIG_MPC8349 set in tqm834x.h?
I understand the desire, I just feel that it doesn't add anything because we've done a decent job of making the differences between them covered by feature configs.
As Tolunay explained: it's for consistency with existing boards. And I really think it does not hurt ao much to have both CONFIG_MPC834X and CONFIG_MPC8349 in the board config file if this is a 8349 only board, even if CONFIG_MPC8349 should never be used anywhere in the code.
So how should we set this for TQM834x? Clearly MPC8349EMDS shouldn't have it set, and MPC8349ITX should by your current description.
Also, let me ask, what exactly does CONFIG_MPC8349 mean? Does it mean I'm on the subset of all MPC8349 revisions. Rev3.0 of MPC834x adds DDR2 support so clearly CONFIG_MPC8349 can only be useful to distinguish the subset of MPC8349 revisions.
Personally, I feel that defining it before it's clear there is a user that needs it lends the define to be incorrectly used in the future.
- kumar
In message 14A6A69F-9B58-42AB-985A-89F70DEB05A6@kernel.crashing.org you wrote:
Are you ok with having CONFIG_MPC8349 set in tqm834x.h?
Yes, definitely.
So how should we set this for TQM834x? Clearly MPC8349EMDS shouldn't have it set, and MPC8349ITX should by your current description.
TQM834x in it's current state should set CONFIG_MPC8349 as it was implemented and tested with MPC8349 only in mind.
Also, let me ask, what exactly does CONFIG_MPC8349 mean? Does it
It means that the board has a MPC8349 CPU mounted.
Personally, I feel that defining it before it's clear there is a user that needs it lends the define to be incorrectly used in the future.
For example, I use it for looking for boards with a specific CPU on it (by grepping in include/configs/ :-)
Best regards,
Wolfgang Denk
On Dec 5, 2006, at 10:57 AM, Wolfgang Denk wrote:
In message 14A6A69F-9B58-42AB-985A-89F70DEB05A6@kernel.crashing.org you wrote:
Are you ok with having CONFIG_MPC8349 set in tqm834x.h?
Yes, definitely.
So how should we set this for TQM834x? Clearly MPC8349EMDS shouldn't have it set, and MPC8349ITX should by your current description.
TQM834x in it's current state should set CONFIG_MPC8349 as it was implemented and tested with MPC8349 only in mind.
Also, let me ask, what exactly does CONFIG_MPC8349 mean? Does it
It means that the board has a MPC8349 CPU mounted.
Personally, I feel that defining it before it's clear there is a user that needs it lends the define to be incorrectly used in the future.
For example, I use it for looking for boards with a specific CPU on it (by grepping in include/configs/ :-)
If the intent is for 'documentation' how about doing:
#define CONFIG_MPC8349 ##
So that if someone uses CONFIG_MPC8349 you can a compile error.
- k
Kumar Gala wrote:
If the intent is for 'documentation' how about doing:
#define CONFIG_MPC8349 ##
So that if someone uses CONFIG_MPC8349 you can a compile error.
Eh, I don't like that.
I don't think Wolfgang meant to limit CONFIG_MPC8349 to just documentation purposes. Your idea would require all the board header files to change if 8349-specific is ever introduced.
In message E7593982-8064-4D57-A8D8-83262D2522BA@kernel.crashing.org you wrote:
If the intent is for 'documentation' how about doing:
#define CONFIG_MPC8349 ##
So that if someone uses CONFIG_MPC8349 you can a compile error.
I think that's a pretty dirty trick. Please don;t do such things. If you want to throw an error, then do it explicitely.
Best regards,
Wolfgang Denk
On Dec 5, 2006, at 3:35 PM, Wolfgang Denk wrote:
In message <E7593982-8064-4D57- A8D8-83262D2522BA@kernel.crashing.org> you wrote:
If the intent is for 'documentation' how about doing:
#define CONFIG_MPC8349 ##
So that if someone uses CONFIG_MPC8349 you can a compile error.
I think that's a pretty dirty trick. Please don;t do such things. If you want to throw an error, then do it explicitely.
That's why I suggested it before actually doing it :)
Anyways, I've given in a reverted the changes that remove CONFIG_MPC8349 from my patch.
- k
Tolunay Orkun wrote:
What I am saying is that you should rename existing CONFIG_MPC8349 to CONFIG_MPC834X and only on the boards that actually use 8349 define the CONFIG_MPC8349 (in addition, not in lieu of).
I agree with Kumar. We should not defined CONFIG_MPC8349 anywhere until we know we need it. Technically speaking, with the possible exception of errata, there should never be a need for CONFIG_MPC8349.
In message 457460DF.50200@freescale.com you wrote:
Kumar Gala wrote:
I have a patch to remove CONFIG_MPC8349 since its redundant with CONFIG_MPC834X and CONFIG_MPC834X makes more sense to me.
...
Are you saying that everywhere there's a CONFIG_MPC8349, CONFIG_MPC834X would also work?
I'm not sure about this. There might be features that are only available on some MPC834x systems.
Best regards,
Wolfgang Denk
Pantelis Antoniou wrote:
Hi there,
So I'm cleaning my new 834x board in the time period of just two days.
I've been working on 834x changes for several months now, including adding support for the Freescale 8349 ITX.
There's a commit with a bunch of 834x related fixes which totally broke the board port.
Sorry. That patches were originally submitted back in September. I guess you didn't see them.
I understand that the main code is volatile, but it seems there's a blatant disregard of keeping up a minimum of backwards compatibility.
I wouldn't say it's a blatant disregard. Every time we add support for a new board, we need to consider code consolidation. That often requires changes to core code that affects all the boards.
I have no knowledge of your board and your work. If I had, I would have tried to accommodate you as much as possible.
If someone changes code that's supporting a bunch of board ports, please put in some default macros to ensure past behaviour is preserved.
I.e. in this case the CONFIG_MPC8349 & CFG_FLASH_SIZE could have easily been defined with the defaults values for the non updated board ports to continue working.
CONFIG_MPC8349 should only be defined on 8349 boards, so there's no way to have a default value. Either you have an 8349 or you don't.
CFG_FLASH_SIZE has always been a part of U-Boot, it just that it was never used on 83xx boards. When my change to start.S that used that macro was introduced, we debated whether I should add some kind of default value, but Wolfgang said no. It makes sense, considering the solution was to add this in start.S:
#ifndef CFG_FLASH_SIZE #define CFG_FLASH_SIZE 8 #endif
You definitely do not want to introduce code like that into U-Boot, or any project.
I have a list of about a dozen significant changes that I want to make to U-Boot, many of which will change core files (and break board ports again). The only thing I can do is make my intentions known in advance.
participants (5)
-
Kumar Gala -
Pantelis Antoniou -
Timur Tabi -
Tolunay Orkun -
Wolfgang Denk