Hi Kim,
2014-11-07 1:18 GMT+01:00 Kim Phillips kim.phillips@freescale.com:
... sorry for the delay, I bricked a board when going through my queue lately, and haven't been able to fully recover since.
no problem, Thanks for the review, I'm very happy we have some progress now.
arch/powerpc/cpu/mpc83xx/Kconfig | 4 + board/gdsys/405ep/iocon.c | 190 +---------- board/gdsys/common/Makefile | 3 +- board/gdsys/common/ihs_mdio.c | 88 +++++ board/gdsys/common/ihs_mdio.h | 18 ++ board/gdsys/common/phy.c | 280 ++++++++++++++++ board/gdsys/common/phy.h | 14 +
is it me, or should PHY support go under drivers/net/phy/gdsys.c (or something like that)? Even if not immediately ported to the Generic PHY Management layer, still, I think it would be a good idea to get it in the right vicinity.
Taking a closer look, it looks like at least one of the PHYs involved here (the Marvell 88E1518) is already implemented in drivers/phy/marvell.c to a certain degree, so it might be helpful to define CONFIG_PHY_MARVELL as a first step to migrating to the generic PHY subsystem.
The first step was to factor out the common PHY code from iocon.c. The next step is merging this with the PHY subsystem. For the moment I would prefer it going in this way and doing a merge with the PHY subsystem for the next release. There are more boards coming that use this and I will clean it up alltogether.
board/gdsys/mpc8308/Kconfig | 12 + board/gdsys/mpc8308/MAINTAINERS | 6 + board/gdsys/mpc8308/Makefile | 9 + board/gdsys/mpc8308/hrcon.c | 677 +++++++++++++++++++++++++++++++++++++++ board/gdsys/mpc8308/mpc8308.c | 109 +++++++ board/gdsys/mpc8308/mpc8308.h | 10 + board/gdsys/mpc8308/sdram.c | 82 +++++
common/Makefile | 1 + common/cmd_ioloop.c | 295 +++++++++++++++++
IDK what this is (FPGA io-endpoint looper/reflector?), but I'm guessing since it's in common/, it should be separated from this board support patch, otherwise it won't get the intended audience's attention.
Since this is very gdsys FPGA specific I should probably move it to our board directory.
+int last_stage_init(void) +{
int slaves;unsigned int k;unsigned int mux_ch;unsigned char mclink_controllers[] = { 0x24, 0x25, 0x26 };u16 fpga_features;bool hw_type_cat = pca9698_get_value(0x20, 20);bool ch0_rgmii2_present = false;FPGA_GET_REG(0, fpga_features, &fpga_features);/* Turn on Parade DP501 */pca9698_direction_output(0x20, 10, 1);ch0_rgmii2_present = !pca9698_get_value(0x20, 30);/* wait for FPGA done */for (k = 0; k < ARRAY_SIZE(mclink_controllers); ++k) {unsigned int ctr = 0;if (i2c_probe(mclink_controllers[k]))continue;while (!(pca953x_get_val(mclink_controllers[k])& MCFPGA_DONE)) {udelay(100000);if (ctr++ > 5) {printf("no done for mclink_controller %d\n", k);break;}}}if (hw_type_cat) {miiphy_register(bb_miiphy_buses[0].name, bb_miiphy_read,bb_miiphy_write);for (mux_ch = 0; mux_ch < MAX_MUX_CHANNELS; ++mux_ch) {if ((mux_ch == 1) && !ch0_rgmii2_present)continue;setup_88e1514(bb_miiphy_buses[0].name, mux_ch);}}/* give slave-PLLs and Parade DP501 some time to be up and running */udelay(500000);mclink_fpgacount = CONFIG_SYS_MCLINK_MAX;slaves = mclink_probe();mclink_fpgacount = 0;print_fpga_info(0, ch0_rgmii2_present);osd_probe(0);return 0;unless this was left in from debugging (in which case it should be removed), it implies the remaining code in the fn..:
Oops, you are absolutely rigth this is debugcode. Wonder how that crept in...
if (slaves <= 0)return 0;mclink_fpgacount = slaves;for (k = 1; k <= slaves; ++k) {FPGA_GET_REG(k, fpga_features, &fpga_features);print_fpga_info(k, false);osd_probe(k);if (hw_type_cat) {miiphy_register(bb_miiphy_buses[k].name,bb_miiphy_read, bb_miiphy_write);setup_88e1514(bb_miiphy_buses[k].name, 0);}}return 0;+}
..is dead code, and therefore not welcome here.
+int board_early_init_r(void) +{
unsigned k;unsigned ctr;for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k)gd->arch.fpga_state[k] = 0;/** reset FPGA*/mpc8308_init();mpc8308_set_fpga_reset(1);mpc8308_setup_hw();for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) {ctr = 0;while (!mpc8308_get_fpga_done(k)) {udelay(100000);if (ctr++ > 5) {gd->arch.fpga_state[k] |=FPGA_STATE_DONE_FAILED;break;}}}udelay(10);mpc8308_set_fpga_reset(0);for (k = 0; k < CONFIG_SYS_FPGA_COUNT; ++k) {/** wait for fpga out of reset*/ctr = 0;while (1) {u16 val;FPGA_SET_REG(k, reflection_low, REFLECTION_TESTPATTERN);FPGA_GET_REG(k, REFLECTION_TESTREG, &val);if (val == REFLECTION_TESTPATTERN_INV)break;udelay(100000);if (ctr++ > 5) {gd->arch.fpga_state[k] |=FPGA_STATE_REFLECTION_FAILED;break;}}}should these functions have timeouts? not sure if there's anything useful to do if they do timeout...
In fact the ctr code is meant to be a timeout. Or do you address something different?
+/* Fixed sdram init -- doesn't use serial presence detect.
- This is useful for faster booting in configs where the RAM is unlikely
- to be changed, or for things like NAND booting where space is tight.
- */
this is ok if the board has its RAM soldered-on: does it?
It does.
diff --git a/common/Makefile b/common/Makefile index b19d379..b5ed9ae 100644 --- a/common/Makefile +++ b/common/Makefile @@ -119,6 +119,7 @@ obj-$(CONFIG_CMD_HASH) += cmd_hash.o obj-$(CONFIG_CMD_IDE) += cmd_ide.o obj-$(CONFIG_CMD_IMMAP) += cmd_immap.o obj-$(CONFIG_CMD_INI) += cmd_ini.o +COBJS-$(CONFIG_CMD_IOLOOP) += cmd_ioloop.o obj-$(CONFIG_CMD_IRQ) += cmd_irq.o obj-$(CONFIG_CMD_ITEST) += cmd_itest.o obj-$(CONFIG_CMD_JFFS2) += cmd_jffs2.o
hmm, does obj-$... not work?
Sometimes git rebase is simply too smart. Will change to obj-$
Cheers Dirk