Re: [PATCH] Memec Virtex-II Pro and PowerPC 405 watchdog support

[Wouter Cloetens <wouter at mind dot be>]

On Fri, Oct 15, 2004 at 01:48:44PM +0200, Andrew Lunn wrote:
> On Fri, Oct 15, 2004 at 12:29:37PM +0200, Wouter Cloetens wrote:
> > Mind has decided to release its platform support for the Memec Virtex-II
> > Pro FG456 evaluation board.
> 
> Would you like this incorperating into the anoncvs repository? You do

Yes.

> not appear to have a copyright assignment on file. Please could you
> take a look at http://ecos.sourceware.org/assign.html.

I know and I have. We're thinking about it.

> > +// This file is part of eCos, the Embedded Configurable Operating System.
> > +// Copyright (C) 1998, 1999, 2000, 2001, 2002 Red Hat, Inc.
> > +// Copyright (C) 2002, 2004 Mind n.v. <v2p@mind.be>
> 
> When this is committed to anoncvs, this Mind Copyright will have to be
> removed. 

That is a problem. Though we may be prepared to assign the copyright to
the code to someone else, we also want to retain the (non-exclusive) copyright
and the right to use our code as we see fit, as well as a permanent
reference to Mind as the original contributor.

> > +++ ecos-mv2p/ecos/packages/devs/eth/powerpc/mv2p/current/src/if_mv2p.c	2004-10-14 17:44:27.000000000 +0200
> > +// HACK
> > +#define MV2P_ETH_PHY 0
> > +// HACK
> 
> I start to worry when i see comments like this!

Gary must've taken a shortcut here. Our Linux driver scans for the PHY
address. I can fix and test that if I get the time.

> > +// LED activity [exclusive of hardware bits]
> > +#ifndef _get_leds
> > +externC int  _mv2p_get_leds(void);
> > +externC void _mv2p_set_leds(int);
> 
> These should be called something like hal_get_leds() and
> hal_set_leds() so you do not polute the namespace. There should also
> be prototypes in something like hal_diag.h.

Thanks for the tip, I'll look into it.

> > +static void 
> > +mv2p_eth_send(struct eth_drv_sc *sc, struct eth_drv_sg *sg_list, int sg_len, 
> > +               int total_len, unsigned long key)
> > +{
> > +    struct mv2p_eth_info *qi = (struct mv2p_eth_info *)sc->driver_private;
> > +    volatile struct eth_bd *txbd, *txfirst;
> > +    char *bp;
> > +    int i, txindex;
> > +
> > +    // Find a free buffer
> > +    txbd = txfirst = qi->txbd;
> > +    if ((txbd->status & BD_FREE) == 0) {
> > +#ifdef CYGPKG_NET
> > +        panic ("No free xmit buffers");
> 
> Is panicing the right thing to do here? I would of thought discarding
> the packet would be a much better solution, so that you can keep on
> running and let the higher layers do a retry.

Ouch. Will fix.

> > +        cdl_option CYGNUM_IO_SERIAL_POWERPC_MV2P_SERIAL0_BAUD {
> > +            display       "Baud rate for the Memec Virtex-II/Pro serial port 0 driver"
> > +            flavor        data
> > +            legal_values  { 115200 }
> > +            default_value 115200
> 
> Does it only support one fixed baud rate?

Yes. The UART Lite is a very simple IP implementation of an UART. All
the communication settings are configured in the design. So for all
intents and purposes, from the software's point of view, it's been
configured in the hardware.

> > +    cdl_component CYGPKG_IO_SERIAL_POWERPC_MV2P_TESTING {
> > +        display    "Testing parameters"
> > +        flavor     bool
> > +        calculated 1
> > +
> > +        implements CYGINT_IO_SERIAL_TEST_SKIP_38400
> > +        implements CYGINT_IO_SERIAL_TEST_SKIP_57600
> > +        implements CYGINT_IO_SERIAL_TEST_SKIP_PARITY_EVEN
> 
> I suspend there should be more skip interfaces here, but i've not
> checked.
> 
> > +    cdl_component CYGPKG_DEVICES_WATCHDOG_POWERPC_PPC405_OPTIONS {
> > +        display "ppc405 watchdog build options"
> > +        flavor  none
> > +        description   "
> > +            Package specific build options including control over
> > +            compiler flags used only in building this package,
> > +            and details of which tests are built."
> > +
> 
> There should be no need for this component. You can have the
> CFLAGS_ADD and CLAGS_REMOVE a level up.
> 
> > +        cdl_option CYGPKG_DEVICES_WATCHDOG_POWERPC_PPC405_CFLAGS_ADD {
> > +            display "Additional compiler flags"
> > +            flavor  data
> > +            no_define
> > +            default_value { "" }
> > +            description   "
> > +                This option modifies the set of compiler flags for
> > +                building the watchdog device. These flags are used in addition
> > +                to the set of global flags."
> > +        }
> > +
> > +        cdl_option CYGPKG_DEVICES_WATCHDOG_POWERPC_PPC405_CFLAGS_REMOVE {
> > +            display "Suppressed compiler flags"
> > +            flavor  data
> > +            no_define
> > +            default_value { "" }
> > +            description   "
> > +                This option modifies the set of compiler flags for
> > +                building the watchdog device. These flags are removed from
> > +                the set of global flags if present."
> > +        }
> > +
> > +    }
> > +}
> 
> > +## FIXME
> > +##   This is a workaround to a problem which is totally unexplainable.
> > +##   If ^C support is enabled, eCos applications make a call into RedBoot
> > +##   to enable the serial port interrupts.  However, the call into RedBoot
> > +##   fails with an illegal instruction exception every time.  There is no
> > +##   understandable reason for this, but at the moment the only safe thing
> > +##   is to not support this feature.
> > +    requires      { (CYGDBG_HAL_DEBUG_GDB_CTRLC_SUPPORT == 0) }
> > +## FIXME
> 
> Any new ideas why this is?

Perhaps Gary can shed some light on this based on his more recent experience
with other 405-based platforms?

> > +    cdl_option CYGHWR_HAL_POWERPC_CPU_SPEED {
> > +        display          "Development board clock speed (MHz)"
> > +        flavor           data
> > +        legal_values     { 100 300 }
> > +        default_value    100
> > +        description      "
> > +           MV2P Development Boards have various system clock speeds
> > +           depending on the processor fitted.  Select the clock speed
> > +           appropriate for your board so that the system can set the serial
> > +           baud rate correctly, amongst other things."
> > +    }
> > +
> > +    cdl_option CYGHWR_HAL_POWERPC_MEM_SPEED {
> > +        display          "Development board memory bus speed (MHz)"
> > +        flavor           data
> > +        legal_values     100
> > +        default_value    100
> > +        description      "
> > +           MV2P Development Boards have various system clock speeds
> > +           depending on the processor fitted.  Select the clock speed
> > +           appropriate for your board so that the system can set the serial
> > +           baud rate correctly, amongst other things."
> 
> This Description does not seem correct.

The CPU speed description is, but the memory bus speed value only seems
to be used by RedBoot to pass as a boot value to Linux. I'll fix it.

Thanks for the feedback! I'll work on an update when time permits.

bfn, Wouter

Attachment: signature.asc
Description: Digital signature