RE: mpc555 serial driver hardware queue support

["Steven Clugston" <steven dot clugston at newcastle dot ac dot uk>]

-----Original Message-----
From: Andrew Lunn [mailto:andrew@lunn.ch] 
Sent: 07 September 2008 10:34
To: Steven Clugston
Subject: Re: mpc555 serial driver hardware queue support


On Fri, Sep 05, 2008 at 02:07:01PM +0100, Steven Clugston wrote:
> 
> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: 05 September 2008 07:26
> To: Steven Clugston
> Subject: Re: mpc555 serial driver hardware queue support
> 
> 
> >> I've run out of time today, so I'll pick this up tomorrow and
> >> hopefully resolve it.
> 
> >No rush. Take your time. Look how long it took me to pick up this
> thread after your assignment took place.
> >
> >      Andrew
> 
> 
> Andrew,
> 
> I've made the common ISR change and tested it. Also new in this diff 
> I've modified var_intr.h to correct the interrupt priority defines. 
> I've checked in the hardware manual to confirm that the lowest 
> interrupt number does infact have the highest priority. There are some

> asserts which where inconsistent with this which I've corrected as 
> well.

> I have some new problems now. Attached is the file
mpc555_serial_with_ints.c. It contains your patch plus i wrapped a lot
of the long lines. I personally don't like lines longer than 80
characters.
>
> I then started compiler testing. I picked an arbitrary target which
uses this driver, the ec555.
>
> rm -fr *
> ecosconfig new ec555
> emacs ecos.ecc and set CYGPKG_IO_SERIAL_DEVICES to 1
> ecosconfig tree
> make -s
> headers finished
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c: In function `mpc555_serial_start_xmit':
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:772: structure has no member named
`data_xmt_req'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:790: structure has no member named
`data_xmt_done'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c: In function `mpc555_serial_common_ISR':
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:860: warning: unused variable
`mpc555_chan'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c: In function
`mpc555_serial_tx_queue_DSR':
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1005: structure has no member named
`data_xmt_req'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1016: structure has no member named
`data_xmt_done'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1040: structure has no member named
`data_xmt_done'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c: In function
`mpc555_serial_rx_queue_DSR':
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1073: warning: unused variable `qscicr'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1089: warning: unused variable
`space_avail'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c: In function `mpc555_serial_read_queue':
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1216: structure has no member named
`data_rcv_req'
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:1229: structure has no member named
`data_rcv_done'
> /home/lunn/eCos/work/install/include/cyg/hal/hal_arbiter.h: At top
level:
>
/home/lunn/eCos/anoncvs-clean/packages/devs/serial/powerpc/mpc555/curren
t/src/mpc555_serial_with_ints.c:846: warning: `mpc555_serial_tx_ISR'
defined but not used
>
> The first error is because you use the callback data_xmt_req.  This
only exists when using block transfers. If
CYGINT_IO_SERIAL_BLOCK_TRANSFER is disabled, >which is the default, this
does not exist.
>
> Please could you do some more testing with other configurations and
fix any problems you find.
>
>    Thanks
>        Andrew

Hi Andrew

There was a condition where it was possible in cdl to select the
hardware serial drivers package but not actually select either of the
two serial devices so CYGINT_IO_SERIAL_BLOCK_TRANSFER was never set.
Although this is not a useful selection it is still possible, so I have
#ifdef'd the relevant places to prevent compile errors. I've also added
a trivial fix to re-enable the serial hardware queue after a frame error
occurs which happens repeatably when something is sent at the wrong baud
rate.

I've tested various permutations of different configurations and all
seem to compile and run on my board.

I've wrapped most of the long lines where it is not completely
detremental to code readability.
I also dislike sprawling code that's over 80 chars long, but I was
trying to respect the original code style, or minimise the changes to
it.

Is there an official, or defacto eCos code style?

Thanks,

Steven

Attachment: mpc555_serial.tar.gz
Description: mpc555_serial.tar.gz