[U-Boot-Users] Patch: Support for PQ27e (8247/48/71/72) chips and MPC8272ADS board
Hi,
This patch adds support for the newest additions to Motorola PQII family (MPC8247/8248/8271/8272) known also as PQ27e. They've got different DPRAM layout (as was discussed on the list) so common files were changed. I tested the code on several boards I've got (using both old and new chips). Motorola released new derivative of MPC8260ADS evaluation board supporting these chips: MPC8272ADS. This patch adds support for the new board.
Dear Yuli,
in message 16450.14241.889562.784232@gargle.gargle.HOWL you wrote:
This patch adds support for the newest additions to Motorola PQII family (MPC8247/8248/8271/8272) known also as PQ27e. They've got different DPRAM layout (as was discussed on the list) so common files were changed. I tested the code on several boards I've got (using both old and new chips). Motorola released new derivative of MPC8260ADS evaluation board supporting these chips: MPC8272ADS. This patch adds support for the new board.
Thanks.
Please don't forget the entry for the CHANGELOG file.
Please add the appropriate additions to the CREDITS and MAINTAINERS files, if needed.
I don't like the following parts of your patch:
Index: cpu/mpc8260/commproc.c =================================================================== RCS file: /home/CVS/u-boot/u-boot/cpu/mpc8260/commproc.c,v retrieving revision 1.1.1.3 diff -p -u -r1.1.1.3 commproc.c --- cpu/mpc8260/commproc.c 6 Aug 2003 17:03:01 -0000 1.1.1.3 +++ cpu/mpc8260/commproc.c 29 Feb 2004 18:45:04 -0000 @@ -20,13 +20,6 @@ #include <common.h> #include <asm/cpm_8260.h> -/* - * because we have stack and init data in dual port ram - * we must reduce the size - */ -#undef CPM_DATAONLY_SIZE -#define CPM_DATAONLY_SIZE ((uint)(8 * 1024) - CPM_DATAONLY_BASE)
Are you 100% sure this does not break any existing boards?
void m8260_cpm_reset(void) { @@ -38,7 +31,10 @@ m8260_cpm_reset(void) /* Reclaim the DP memory for our use. */ gd->dp_alloc_base = CPM_DATAONLY_BASE; - gd->dp_alloc_top = gd->dp_alloc_base + CPM_DATAONLY_SIZE; + if (is_pq27e()) + gd->dp_alloc_top = gd->dp_alloc_base + PQ27E_DATAONLY_SIZE; + else + gd->dp_alloc_top = gd->dp_alloc_base + CPM_DATAONLY_SIZE; /* * Reset CPM
Please do not add a board-specific #define PQ27E_DATAONLY_SIZE when we already have a CPM_DATAONLY_SIZE which could be used.
The same happens again in other places, like:
cpu/mpc8260/ether_fcc.c:
PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE
Please clean up and resubmit.
Best regards,
Wolfgang Denk
Wolfgang Denk writes:
Wolfgang> Dear Yuli, in message Wolfgang> 16450.14241.889562.784232@gargle.gargle.HOWL you wrote:
Yuli> This patch adds support for the newest additions to Motorola Yuli> PQII family (MPC8247/8248/8271/8272) known also as Yuli> PQ27e. They've got different DPRAM layout (as was discussed on Yuli> the list) so common files were changed. I tested the code on Yuli> several boards I've got (using both old and new Yuli> chips). Motorola released new derivative of MPC8260ADS Yuli> evaluation board supporting these chips: MPC8272ADS. This Yuli> patch adds support for the new board.
Wolfgang> Thanks.
Wolfgang> Please don't forget the entry for the CHANGELOG file.
Sorry, I promise it was last time I forgot it.
Wolfgang> Please add the appropriate additions to the CREDITS and Wolfgang> MAINTAINERS files, if needed.
Wolfgang> I don't like the following parts of your patch:
Wolfgang> Index: cpu/mpc8260/commproc.c Wolfgang> ======================================== Wolfgang> RCS file: Wolfgang> /home/CVS/u-boot/u-boot/cpu/mpc8260/commproc.c,v Wolfgang> retrieving revision 1.1.1.3 diff -p -u -r1.1.1.3 Wolfgang> commproc.c Wolfgang> --- cpu/mpc8260/commproc.c 6 Aug 2003 17:03:01 -0000 Wolfgang> 1.1.1.3 Wolfgang> +++ cpu/mpc8260/commproc.c 29 Feb 2004 18:45:04 -0000 Wolfgang> @@ -20,13 +20,6 @@ Wolfgang> #include <common.h> include <asm/cpm_8260.h> Wolfgang> -/* Wolfgang> - * because we have stack and init data in dual port ram Wolfgang> - * we must reduce the size Wolfgang> - */ Wolfgang> -#undef CPM_DATAONLY_SIZE Wolfgang> -#define CPM_DATAONLY_SIZE ((uint)(8 * 1024) - CPM_DATAONLY_BASE)
Wolfgang> Are you 100% sure this does not break any existing boards?
I checked it on four different boards with different PQ2 chips. In fact, for older (pre-PQ27e) chips this patch does not change memory map so they aren't affected in any way. Am I missing anything?
Wolfgang> void m8260_cpm_reset(void) { Wolfgang> @@ -38,7 +31,10 @@ m8260_cpm_reset(void) Wolfgang> /* Reclaim the DP memory for our use. */ Wolfgang> gd-> dp_alloc_base = CPM_DATAONLY_BASE; Wolfgang> - gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->CPM_DATAONLY_SIZE; Wolfgang> + if (is_pq27e()) Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->PQ27E_DATAONLY_SIZE; Wolfgang> + else Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->CPM_DATAONLY_SIZE; Wolfgang> /* Reset CPM
Wolfgang> Please do not add a board-specific #define Wolfgang> PQ27E_DATAONLY_SIZE when we already have a Wolfgang> CPM_DATAONLY_SIZE which could be used.
There should be some misunderstanding here. PQ27E_DATAONLY_SIZE is NOT board-specific (I won't add board-specific things into common files). PQ27e is the common name for the 8247/8248/8271/8272 (the `e' stands for `encryption'). These chips have got much less internal memory (DPRAM) and at different addresses so they must have separate #defines. CPM_DATAONLY_SIZE is just incorrect for PQ27e. PQ27e have got only 4K of "data only" RAM and not 8K (and at different base, BTW).
Wolfgang> The same happens again in other places, like:
Wolfgang> cpu/mpc8260/ether_fcc.c:
Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE
It's for the same reason. PQ27e have not got RAM at 0xB000, other PQ2s have not got RAM at 0x9000. There is simply no common base for the two families so I had to #define new constants. Any suggestions for better solution are welcome.
Wolfgang> Please clean up and resubmit.
After the explanations, what changes would you suggest?
In message 16469.31569.535909.246506@gargle.gargle.HOWL you wrote:
Wolfgang> -#undef CPM_DATAONLY_SIZE Wolfgang> -#define CPM_DATAONLY_SIZE ((uint)(8 * 1024) - CPM_DATAONLY_BASE) Wolfgang> Are you 100% sure this does not break any existing boards?I checked it on four different boards with different PQ2 chips. In fact, for older (pre-PQ27e) chips this patch does not change memory map so they aren't affected in any way. Am I missing anything?
I don't know. I just want to understand the consequences.
Wolfgang> + gd->CPM_DATAONLY_SIZE; Wolfgang> + if (is_pq27e()) Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->PQ27E_DATAONLY_SIZE; Wolfgang> + else Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->CPM_DATAONLY_SIZE;
First, ther is at least one redundand "gd->CPM_DATAONLY_SIZE;" here.
Wolfgang> Please do not add a board-specific #define Wolfgang> PQ27E_DATAONLY_SIZE when we already have a Wolfgang> CPM_DATAONLY_SIZE which could be used.There should be some misunderstanding here. PQ27E_DATAONLY_SIZE is NOT board-specific (I won't add board-specific things into common files). PQ27e is the common name for the 8247/8248/8271/8272 (the `e'
Sorry for chosing a poor description.
stands for `encryption'). These chips have got much less internal memory (DPRAM) and at different addresses so they must have separate #defines. CPM_DATAONLY_SIZE is just incorrect for PQ27e. PQ27e have got only 4K of "data only" RAM and not 8K (and at different base, BTW).
Why do you need a separate #define then? Isn't it sufficient to #define a correct value for CPM_DATAONLY_SIZE then?
Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASEIt's for the same reason. PQ27e have not got RAM at 0xB000, other PQ2s
So why not just #define a correct value?
After the explanations, what changes would you suggest?
Use the existing variables and just assign correct values?
Best regards,
Wolfgang Denk
Wolfgang Denk writes:
Wolfgang> In message 16469.31569.535909.246506@gargle.gargle.HOWL Wolfgang> you wrote:
Wolfgang> Are you 100% sure this does not break any existing boards?
Yuli> I checked it on four different boards with different PQ2 Yuli> chips. In fact, for older (pre-PQ27e) chips this patch does Yuli> not change memory map so they aren't affected in any way. Am I Yuli> missing anything?
Wolfgang> I don't know. I just want to understand the consequences.
Wolfgang> + gd->CPM_DATAONLY_SIZE; Wolfgang> + if (is_pq27e()) Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->PQ27E_DATAONLY_SIZE; Wolfgang> + else Wolfgang> + gd->dp_alloc_top = gd->dp_alloc_base Wolfgang> + gd->CPM_DATAONLY_SIZE;
Wolfgang> First, ther is at least one redundand Wolfgang> "gd->CPM_DATAONLY_SIZE;" here.
No, no, it's just E-mail formatting of the excerpt after several replies. There is no redundant code in the patch.
Wolfgang> Please do not add a board-specific #define Wolfgang> PQ27E_DATAONLY_SIZE when we already have a Wolfgang> CPM_DATAONLY_SIZE which could be used.
Yuli> There should be some misunderstanding Yuli> here. PQ27E_DATAONLY_SIZE is NOT board-specific (I won't add Yuli> board-specific things into common files). PQ27e is the common Yuli> name for the 8247/8248/8271/8272 (the `e'
Wolfgang> Sorry for chosing a poor description.
Yuli> stands for `encryption'). These chips have got much less Yuli> internal memory (DPRAM) and at different addresses so they Yuli> must have separate Yuli> #defines. CPM_DATAONLY_SIZE is just incorrect for PQ27e. PQ27e Yuli> have got only 4K of "data only" RAM and not 8K (and at Yuli> different base, BTW).
Wolfgang> Why do you need a separate #define then? Isn't it Wolfgang> sufficient to Wolfgang> #define a correct value for CPM_DATAONLY_SIZE then?
This would require that, for every PQ27e-based board, it would be defined in the board configuration file that it's PQ27e and not just 8260. Then CPM_DATAONLY_SIZE can be defined conditionally on this definition in cpm_8260.h. I personally don't like such compile-time definitions because chip version can be easily detected in run-time.
Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE
Yuli> It's for the same reason. PQ27e have not got RAM at 0xB000, Yuli> other PQ2s
Wolfgang> So why not just #define a correct value?
Yuli> After the explanations, what changes would you suggest?
Wolfgang> Use the existing variables and just assign correct values?
Well, the question is if it's better that the CPU-specific code will silently handle the differences or we'll make it the user's responsibility to define the difference manually. I think automatic handling is better but if you insist on #ifdefs, I can implement it too.
In message 16469.44734.573362.958637@gargle.gargle.HOWL you wrote:
No, no, it's just E-mail formatting of the excerpt after several replies. There is no redundant code in the patch.
You are right. Sorry for the confusion.
This would require that, for every PQ27e-based board, it would be defined in the board configuration file that it's PQ27e and not just 8260. Then CPM_DATAONLY_SIZE can be defined conditionally on this definition in cpm_8260.h. I personally don't like such compile-time definitions because chip version can be easily detected in run-time.
The advantage of compile-time definitions is that they result in smaller code.
Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE
...
Well, the question is if it's better that the CPU-specific code will silently handle the differences or we'll make it the user's responsibility to define the difference manually. I think automatic handling is better but if you insist on #ifdefs, I can implement it too.
Sorry, but I can't follow.
From what I have seen your patches used _two_ #defines instead of
one.
Best regards,
Wolfgang Denk
Wolfgang Denk writes:
Yuli> This would require that, for every PQ27e-based board, it would Yuli> be defined in the board configuration file that it's PQ27e and Yuli> not just Yuli> 8260. Then CPM_DATAONLY_SIZE can be defined conditionally on Yuli> this definition in cpm_8260.h. I personally don't like such Yuli> compile-time definitions because chip version can be easily Yuli> detected in run-time.
Wolfgang> The advantage of compile-time definitions is that they Wolfgang> result in smaller code.
Of course, but here we're speaking about couple of `if's.
Wolfgang> PQ27E_FCC_SPECIAL_BASE vs. CPM_FCC_SPECIAL_BASE ...
Yuli> Well, the question is if it's better that the CPU-specific Yuli> code will silently handle the differences or we'll make it the Yuli> user's responsibility to define the difference manually. I Yuli> think automatic handling is better but if you insist on Yuli> #ifdefs, I can implement it too.
Wolfgang> Sorry, but I can't follow.
Wolfgang> From what I have seen your patches used _two_ #defines Wolfgang> instead of one.
Well, my explanations can be confusing... I'll try to explain it differently. You're right, there are two #defines in one common file and it's all what needed for PQ27e support. If you prefer the compile-time solution, this means that: new CPU type is to be defined (CONFIG_MPC8272 or something like this) in addition to CONFIG_MPC8260, this type must be used for every PQ27e based board instead of CONFIG_MPC8260, and in a common file it must be defined that CONFIG_MPC8272 is a flavour of CONFIG_MPC8260 otherwise the common MPC8260 code won't work for PQ27e. IMHO this means much more work and more possibilities to make a mistake, especially when porting U-Boot to a new board, but you're the maintainer so make your choice.
Sorry to jump into this late, but since I do tons of PQ work I want to add my comments.
.... I personally don't like such Yuli> compile-time definitions because chip version can be easily Yuli> detected in run-time.
No, it can't in all cases. You can determine the core type and can sometimes find something about the CPM rom version, but tracking down and maintaining the permutations isn't trivial. You have great inside information on this stuff, but it doesn't help the rest of us add or maintain this software.
Wolfgang> The advantage of compile-time definitions is that they Wolfgang> result in smaller code.
I agree. We've been doing it this way in Linux for years, the compiler makes it pretty clear when you don't have something #defined. What's run-time code going to do when it can't determine the core or CPM type? Assume some defaults and hope a message gets printed?
Thanks.
-- Dan
Dan Malek writes:
Dan> Sorry to jump into this late, but since I do tons of PQ work I Dan> want to add my comments.
Yuli> .... I personally don't like such Yuli> compile-time definitions because chip version can be easily Yuli> detected in run-time.
Dan> No, it can't in all cases. You can determine the core type and Dan> can sometimes find something about the CPM rom version, but Dan> tracking down and maintaining the permutations isn't trivial.
Well, you can't know if it's 8271 or 8272, for example, but you don't need this info. You can detect the "family" (HiP4, HiP7, etc.) and this is enough for the tasks which we are discussing.
Dan> You have great inside information on this stuff,
No, I don't (and I won't disclose such information in GPL code if I had).
Dan> but it doesn't help the rest of us add or maintain this Dan> software.
Wolfgang> The advantage of compile-time definitions is that they Wolfgang> result in smaller code.
Dan> I agree. We've been doing it this way in Linux for years, the Dan> compiler makes it pretty clear when you don't have something Dan> #defined.
And these #defines were one of the reasons that we developed completely new Linux support for PQs. Now we've got one kernel image running on MPC8260ADS, MPC8266ADS, PQ2FADS, and MPC8272ADS. You would have four different images. Probably that images would be smaller by 2K, maybe 5K, but if the price is maintaining four images instead of one, I vote for one. The same is true for other board families (FADS, etc.) I agree that this can be less important for boot loaders which in many cases contain too much board-specific code to build one-for-all image.
Dan> What's run-time code going to do when it can't determine the Dan> core or CPM type? Assume some defaults and hope a message gets Dan> printed?
Maybe such a thing can happen but in many years of development for Motorola controllers I personally never had a problem with run-time detection of chip features.
In message 16472.27359.566539.641092@gargle.gargle.HOWL Yuli Barcohen wrote:
Dan> I agree. We've been doing it this way in Linux for years, the Dan> compiler makes it pretty clear when you don't have something Dan> #defined.And these #defines were one of the reasons that we developed completely new Linux support for PQs. Now we've got one kernel image running on MPC8260ADS, MPC8266ADS, PQ2FADS, and MPC8272ADS. You would have four different images. Probably that images would be smaller by 2K, maybe 5K, but if the price is maintaining four images instead of one, I vote for one. The same is true for other board families (FADS, etc.) I agree that this can be less important for boot loaders which in many cases contain too much board-specific code to build one-for-all image.
Let's stop this discussion here. We don't need another religious war.
As Dan said, Linux has been using the #define style for a long time, and for good reasons, and U-Boot always followed this style - and if it was only because of the smaller code size.
There are many more situations where run-time code may be very convenient from the software ddeveloper's point of view, but where it is not practical for the intended purpose. Think of memory footprint, boot time, time of error detection etc.
Maybe such a thing can happen but in many years of development for Motorola controllers I personally never had a problem with run-time detection of chip features.
Than you have been just lucky, or not the end user of the solution.
Best regards,
Wolfgang Denk
Wolfgang Denk writes:
Wolfgang> Let's stop this discussion here. We don't need another Wolfgang> religious war.
I hope you believe that religious war is something I want to avoid, not to start. As I wrote in the beginning of this discussion, if the maintainer (you) insists on #ifdefs, I'll send a patch using #ifdefs.
Wolfgang> The universe does not have laws - it has habits, and Wolfgang> habits can be broken.
Even more so with respect to U-Boot and Linux:)
Yuli,
Have you tried this patch with CONFIG_CONS_INDEX = 2? Just wondering because it seems like the IO ports for SCC4 uart are not setup.
- kumar
Kumar Gala writes:
Kumar> Yuli, Have you tried this patch with CONFIG_CONS_INDEX = 2? Kumar> Just wondering because it seems like the IO ports for SCC4 Kumar> uart are not setup.
CONFIG_CONS_INDEX = 2 means SCC2, not SCC4. You have to define CONFIG_CONS_INDEX = 4 to use the lower RS-232 on MPC8272ADS. Ports initialisation is static (I have to make it conditional like I did for FCCs) so the ports table reflects current state (i.e. console on SCC1). I haven't tried the SCC4 in U-Boot, but it works in Linux.
Looking at the patch, I was wondering if there was any reason to not enable both RS232 ports by default.
- kumar
On Feb 29, 2004, at 1:04 PM, Yuli Barcohen wrote:
Hi,
This patch adds support for the newest additions to Motorola PQII family (MPC8247/8248/8271/8272) known also as PQ27e. They've got different DPRAM layout (as was discussed on the list) so common files were changed. I tested the code on several boards I've got (using both old and new chips). Motorola released new derivative of MPC8260ADS evaluation board supporting these chips: MPC8272ADS. This patch adds support for the new board.
--
= Yuli Barcohen | Phone +972-9-765-1788 | Software Project Leader yuli@arabellasw.com | Fax +972-9-765-7494 | Arabella Software, Israel ======================================================================= =
<8272.patch.bz2>
Kumar Gala writes:
Kumar> Looking at the patch, I was wondering if there was any reason Kumar> to not enable both RS232 ports by default.
Only the console port was enabled before the patch. Is there any reason for the patch to change this behaviour? Also, I see no reason to enable any peripheral which isn't going to be used.
Yuli,
I agree with only enabling ports that will be used. I guess I would expect the port to be used in Linux. I'm working on an updated 2.6 SCC uart driver and realized that I needed to enable the 2nd port in BCSR. I had expected that u-boot would have already handled it.
- kumar
On Mar 16, 2004, at 1:23 AM, Yuli Barcohen wrote:
Kumar Gala writes:
Kumar> Looking at the patch, I was wondering if there was anyreason Kumar> to not enable both RS232 ports by default.
Only the console port was enabled before the patch. Is there any reason for the patch to change this behaviour? Also, I see no reason to enable any peripheral which isn't going to be used.
--
= Yuli Barcohen | Phone +972-9-765-1788 | Software Project Leader yuli@arabellasw.com | Fax +972-9-765-7494 | Arabella Software, Israel ======================================================================= =
Kumar Gala writes:
Kumar> Yuli, I agree with only enabling ports that will be used. I Kumar> guess I would expect the port to be used in Linux.
If it's to be used in Linux but not in the U-Boot, let Linux initialise it.
Kumar> I'm working on an updated 2.6 SCC uart driver and realized Kumar> that I needed to enable the 2nd port in BCSR. I had expected Kumar> that u-boot would have already handled it.
I see. In Arabella Linux, there is clear separation between boot and kernel and between chip-specific and board-specific code. ADS-specific file in Linux kernel is the place where the BCSR handling lives, so we've got generic SCC UART driver and perform all the initialisations (including board-specific BCSR) only when some code requests them.
Kumar> Looking at the patch, I was wondering if there was any reason Kumar> to not enable both RS232 ports by default.
Yuli> Only the console port was enabled before the patch. Is there Yuli> any reason for the patch to change this behaviour? Also, I see Yuli> no reason to enable any peripheral which isn't going to be Yuli> used.
On Mar 17, 2004, at 4:54 AM, Yuli Barcohen wrote:
Kumar Gala writes:
Kumar> Yuli, I agree with only enabling ports that will be used. I Kumar> guess I would expect the port to be used in Linux.If it's to be used in Linux but not in the U-Boot, let Linux initialise it.
Kumar> I'm working on an updated 2.6 SCC uart driver and realized Kumar> that I needed to enable the 2nd port in BCSR. I hadexpected Kumar> that u-boot would have already handled it.
I see. In Arabella Linux, there is clear separation between boot and kernel and between chip-specific and board-specific code. ADS-specific file in Linux kernel is the place where the BCSR handling lives, so we've got generic SCC UART driver and perform all the initialisations (including board-specific BCSR) only when some code requests them.
Your correct, I will have the platform code for the board deal with initializing the BCSR in Linux.
Thanks
- kumar
In message 761C62B8-7757-11D8-A4C6-000393DBC2E8@motorola.com you wrote:
I agree with only enabling ports that will be used. I guess I would expect the port to be used in Linux. I'm working on an updated 2.6 SCC uart driver and realized that I needed to enable the 2nd port in BCSR. I had expected that u-boot would have already handled it.
The philosophy is to enable and/or initialize only those devices which are actually used by U-Boot itself. Even the network interfaces get initialized only at the moment when you issue the first command to access the network.
On the other hand a Linux device driver should assume it finds the device in an unknown random (*) state, it should perform all initialization it needs itself, and it should deinitialize the device (if the driver should get unloaded) into an idle state.
(*) "random" is not completely random, of course - it still assumes that the device is well-behaving, i. e. that interrupts are disabled etc.
Best regards,
Wolfgang Denk
participants (4)
-
Dan Malek -
Kumar Gala -
Wolfgang Denk -
Yuli Barcohen