Re: [U-Boot] [PATCH] Add mpc5125ads board and processor to the mpc512x family
Yes but the default has constants like CONFIG_SYS_MICRON_INIT_DEV_OP ... must I then declare this if I am using CONFIG_SYS_ELPIDA_INIT_DEV_OP ?
Well, the ideas was that this was the default setting that fits most boards, and if it doesn't fit it will be overwritten by another array. Adding #ifdef's here is a strict No-No.
How do I override a default array of 30 ulong elements that is declared static in common code from my board code ? I declare my own and use it exclusively so the one in common code is a waste of space. Besides which it puts constraints that all the elements need a declaration. Why if I'm not using it ?
The default constants are a large mem array that just plain doesn't need to be there if you must override it anyway. I don't understand the impetus to save on printf strings, for example, and not wanting to save here ???
Feel free to implement something that needs less memory, but do not add #ifdef's here.
+#ifdef CONFIG_MPC5125
out_8(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR);+#else out_be32(&im->io_ctrl.io_control_mem, IOCTRL_MUX_DDR); +#endif
This is something which happens a lot in the remaining code - so often that it is plain unacceptable. As mentioned above, I know that you are just a victim here, but we need a less ugly implementation.
Actually .. since I redid the entire iopin_initialize function to a separate one for the mpc5125 this is the only place where an ugly #ifdef'ed iopin init occurs now.
I think it's not the only place, and it's just a symptom of the problem. I think we should try to avoid duplicating structs that are more or less the same, except for the data type.
If they had been left as an array with indices as when the 5121 was first implemented these differences would be trivial ... now that each iopin is an elemnet in a struct there are over 50 different namesbesides the ordering by the 3rd element is off for the few names at the beginning that are the same. Also the init code still treats it like an array. I believe you NAKed me on doing this a week or two ago. Perhaps I should take it back to that implementation ???
As I said .. since I redid the iopin_initialize (there are now 2 different functions) I don't think this is necessary ... it's not just a size difference ... there is also a bit configuration difference. I redid the #define for this too. Also .. the elements within the struct are all different.
It's primarily and issue of being able to read and maintain the code. Duplicating the structs makes no sense if they are essentially the same except for the u8 versus u32 difference.
You claim the elements are all different? I didn't get this impression from reading either your code or the RefMan.
I've read and reread the manual Wolfgang ... I could recite page numbers at this point ... I believe you 've glanced at it and have the wrong impression.
...
+#ifndef CONFIG_MPC5125 /* set MR register to point to MR1 */ out_8(&psc->command, PSC_SEL_MODE_REG_1);
@@ -93,12 +93,25 @@ int serial_init(void) /* switch to UART mode */ out_be32(&psc->sicr, 0);
/* mode register points to mr1 */ /* configure parity, bit length and so on in mode register 1*/
/* mode register points to mr1 */ out_8(&psc->mode, PSC_MODE_8_BITS | PSC_MODE_PARNONE); /* now, mode register points to mr2 */ out_8(&psc->mode, PSC_MODE_1_STOPBIT);+#else
/* disable Tx/Rx */out_8(&psc->command, PSC_TX_DISABLE | PSC_RX_DISABLE);/* choose the prescaler the Tx/Rx clock generation */out_8(&psc->psc_clock_select, 0xdd);/* switch to UART mode */out_be32(&psc->sicr, 0);/* configure parity, bit length and so on in mode registers */out_8(&psc->mr1, PSC_MODE_8_BITS | PSC_MODE_PARNONE);out_8(&psc->mr2, PSC_MODE_1_STOPBIT);+#endif /* set baudrate */ serial_setbrg();
I think we should move the differing code into separate functions.
Fine .. but I'll have to check the processor type to see which one to call at some point if I take out the ifdefs ???
No. You can build either one implementation of the function or the other one, depending of the config settings.
Are you talking about pulling one or the other in at the make ? SO to share the 8 functions or so like putc and getc I have one file, 5121 init another and 5125 init a third -- all that to bypass an if statement ?? You're not making any sense to me.
diff --git a/drivers/net/mpc512x_fec.c b/drivers/net/mpc512x_fec.c
index fb2c19a..9f839a1 100644 --- a/drivers/net/mpc512x_fec.c +++ b/drivers/net/mpc512x_fec.c @@ -41,7 +41,12 @@ static int rx_buff_idx = 0; static void mpc512x_fec_phydump (char *devname) { u16 phyStatus, i;
u8 phyAddr = CONFIG_PHY_ADDR;+#ifdef CONFIG_MPC5125
uint8 phyAddr = ((devname[3] == '2') ? CONFIG_PHY2_ADDR :CONFIG_PHY_ADDR);+#else
uint8 phyAddr = CONFIG_PHY_ADDR;+#endif
Can we not do without this #ifdef here?
I'm open to suggestions ... there are possibly two active FECS and this seemed to be the best solution ... The MPC5125 code will work
I'm not sure what you mean. In U-Boot, there cannot be 'two active FECs'; at any point of time, no more than one network interface will be enabled - if any at all.
I'm not talking about any printed messages here. I smell a basic misunderstaning in your comments - there cannot be more than one network interface active and in use in U-Boot.
Can't I initialize both and switch between them ? I do it now and want to keep this functionality.
As far as the Clock on the 2nd FEC .. I check it to see if it's ON .. Only the 1st FEC is necessary in u-boot while the 2nd is optional. ...
Umm... Why should we care about this? See bullet # 2 at http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
U-Boot should not care about these things, unless you run a network command on the second Ethernet interface.
Yes but since you can switch midstream they both need to be initialized and the code needs to determine which one it's using ???
Very Best, Martha
Dear Martha,
can you _please_ fix your mailer and make sure to keep the In-reply-to: and References: mail headers in place and working, so threading of messages works? Thanks.
In message <200910200953803.SM05928@[206.180.163.89]> you wrote:
Well, the ideas was that this was the default setting that fits most boards, and if it doesn't fit it will be overwritten by another array. Adding #ifdef's here is a strict No-No.
How do I override a default array of 30 ulong elements that is declared static in common code from my board code ? I declare my own and use it exclusively so the one in common code is a waste of space. Besides which it puts constraints that all the elements need a declaration. Why if I'm not using it ?
If the assumption that the default code is useful on all boards is wrong, it has the be revised. We can implement this is a way that it can be completel enabled/ disabled, but I will not accept any #ifdef's right in the middle of that array init code.
No. You can build either one implementation of the function or the other one, depending of the config settings.
Are you talking about pulling one or the other in at the make ? SO to share the 8 functions or so like putc and getc I have one file, 5121 init another and 5125 init a third -- all that to bypass an if statement ??
Why compile code for two incompatible architectures when you know at compile time which one you have? You don't attempt to have one U-Boot binary image that runs both on a MPC5121 and on a MPC5125 board, or do you? [I would be interested to see how this could be done in a halfway clean way.] Whether you split this in several files or not is a different question.
You're not making any sense to me.
I'm sorry to hear that.
I'm not talking about any printed messages here. I smell a basic misunderstaning in your comments - there cannot be more than one network interface active and in use in U-Boot.
Can't I initialize both and switch between them ? I do it now and want to keep this functionality.
No, you cannot do that. You must not initialize _any_ network interface unless U-Boot issues a network related command. And when switching to another interface, you have to disable the previouly used one. Same before you boot Linux.
Umm... Why should we care about this? See bullet # 2 at http://www.denx.de/wiki/view/U-Boot/DesignPrinciples#2_Keep_it_Fast
U-Boot should not care about these things, unless you run a network command on the second Ethernet interface.
Yes but since you can switch midstream they both need to be initialized and the code needs to determine which one it's using ???
You cannot switch "midstream". You can use a new network interface with a new network command - and there it does not make a difference whether you ran a command on another interface before (except that you have to make sure to disable such other interface first). There is no and cannot be any switching while any command (say, a TFTP download) is running.
Initialization must not be done before actual use, and then only for the network interface that is actually going to be used.
Best regards,
Wolfgang Denk
participants (2)
-
m stan -
Wolfgang Denk