[U-Boot] [PATCH v1 6/6] mpc83xx: Add gdsys hrcon board
Dirk Eibach
dirk.eibach at gdsys.cc
Fri Nov 7 13:56:49 CET 2014
Hi Kim,
2014-11-07 1:18 GMT+01:00 Kim Phillips <kim.phillips at 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
More information about the U-Boot
mailing list