[U-Boot-Users] Altera Stratix II support

[eran liberty]

Most of what you have chosen to comment on is part of my new board.
some of it is still dirty work as it is all on progress and would love
to get code review on, but keep in mind its fairly young code.

On 6/6/07, Andy Fleming <afleming at gmail.com> wrote:
> +/* FIXME: if tsec does not work try thorwing this in (remove the if 0)*/
> +int last_stage_init(void)
> +{
> +       /* TSEC0 register the first phy so we dont */
> +       tsec_initialize(gd->bd, 1, "2");
> +       tsec_initialize(gd->bd, 2, "3");
> +       tsec_initialize(gd->bd, 3, "4");
> +       tsec_initialize(gd->bd, 4, "5");
> +       tsec_initialize(gd->bd, 5, "6");
> +       tsec_initialize(gd->bd, 6, "7");
> +       tsec_initialize(gd->bd, 7, "8");
> +       tsec_initialize(gd->bd, 8, "9");
> +       tsec_initialize(gd->bd, 9, "10");
> +       tsec_initialize(gd->bd, 10, "11");
> +
> +       return 0;
> +}
>
> Huh?  How can you be initializing 10 tsecs?  What part is this?
>
I can and i do. :)

> +static void cds_pci_fixup(void *blob)
> +{
>
> Um...you probably want to rename this.  Also, are you sure this is a
> function you want?  This is only if your cpu is hooked up to the pci
> devices through a PCI slot.
>
> * reads a little further.  Does some math*
>
> +       slot = 1;
> +
> +       for (i=0;i<len;i+=7) {
> +               /* We rotate the interrupt pins so that the mapping
> +                * changes depending on the slot the carrier card is in.
> +                */
> +               map[3] = ((map[3] + slot - 2) % 4) + 1;
> +
> +               map+=7;
> +       }
>
> Do you realize the net effect of this is nothing?  Pins 1-4 will
> remain 1-4, respectively.  Delete this function altogether and (of
> course) the call to it.
>
> +#define LAWBAR1 ((CFG_PCI1_MEM_BASE>>12) & 0xfffff)
> +#define LAWAR1         (LAWAR_EN | LAWAR_TRGT_IF_PCI1 | (LAWAR_SIZE &
> LAWAR_SIZE_512M))
> +
> +#define LAWBAR2 ((CFG_PCI2_MEM_BASE>>12) & 0xfffff)
> +#define LAWAR2         (LAWAR_EN | LAWAR_TRGT_IF_PCI2 | (LAWAR_SIZE &
> LAWAR_SIZE_512M))
>
> These should be CFG_PCIn_MEM_PHYS instead of CFG_PCIn_MEM_BASE.
> They're defined to be the same thing, but it is the cpu-physical
> address we want, not the PCI address.
>

this code is not mine (i have not wrote it), I will re look if i need it...

>
> +COBJS  := $(BOARD).o \
> +           ft_board.o \
> +           fpga.o \
> +          ../../cds/common/eeprom.o \
> +          ../../cds/common/via.o
>

I need to remove both eeprom.o via.o. I do not use them.. but as it is
right now, removing them breaks the build. It is in my todo list.
>
> Um....that seems shady.  Maybe we need to move the via.c code into
> drivers/  Are you sure you have a VIA chip?
>
>  static struct tsec_info_struct tsec_info[] = {
> +#if defined CONFIG_EXSW6000
> +       {TSEC1_PHY_ADDR, TSEC_GIGABIT, TSEC1_PHYIDX},
> +       {2, TSEC_GIGABIT, 1},
> +       {3, TSEC_GIGABIT, 2},
> +       {4, TSEC_GIGABIT, 3},
> +       {5, TSEC_GIGABIT, 4},
> +       {6, TSEC_GIGABIT, 5},
> +       {7, TSEC_GIGABIT, 6},
> +       {8, TSEC_GIGABIT, 7},
> +       {9, TSEC_GIGABIT, 8},
> +       {10, TSEC_GIGABIT, 9},
> +       {11, TSEC_GIGABIT, 10},
> +#else
>

I thought i am doing something shady as well stacking all mdio on the
register of tsec0... but then i moved on to linux and saw they do
exactly the same. :)

>
> Are you sure about this?  You are claiming that you have 11 TSECs, and
> that all of them use their own registers to access their PHY.
>
>
> +#if defined(CONFIG_EXSW6000)
> +       priv->regs = (volatile tsec_t *)(TSEC_BASE_ADDR);
> +       priv->phyregs = (volatile tsec_t *)(TSEC_BASE_ADDR);
> +#else
>        priv->regs = (volatile tsec_t *)(TSEC_BASE_ADDR + index * TSEC_SIZE);
>        priv->phyregs = (volatile tsec_t *)(TSEC_BASE_ADDR +
> -                                           tsec_info[index].phyregidx *
> -                                           TSEC_SIZE);
> +                                                                tsec_info[index].phyregidx *
> +                                                                TSEC_SIZE);
> +#endif
>
> 1) Random, bad whitespace change.

would fix this

> 2) I am *very* confused by what you did, here.  Now it looks like you
> have set the phyregs back to what would happen if your phyregidx had
> been 0.  And all of your TSECs share a single register set.  It looks
> like you have one TSEC and 11 PHYs.  What's going on?
>

very simple. All mdio are accessed via the same registers. the one
that are in the memory space of eTSEC0

thanks for the in depth review. I appreciate the time you took to
provide these insight.

Liberty