[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