Re: [XenPPC] [pushed] [ppc] serial port discovery and zilog device driver

[Maria Butrico <butrico@xxxxxxxxxxxxx>]

Hollis Blanchard wrote:
> On Thu, 2006-05-25 at 15:46 -0400, jimix@xxxxxxxxxxxxxx wrote:
>   
> > changeset:   10387:a1a0ff01e7c7e51ee2a1b418e463edbff19d7c55
> > user:        jimix@xxxxxxxxxxxxxxxxxxxxx
> > date:        Thu May 25 15:20:20 2006 -0400
> > files:       xen/arch/ppc/boot_of.c xen/arch/ppc/setup.c 
> > xen/drivers/char/Makefile xen/drivers/char/ns16550.c 
> > xen/drivers/char/pmac_zilog.c xen/drivers/char/pmac_zilog.h 
> > xen/include/asm-ppc/uart.h xen/include/xen/serial.h
> > description:
> > [ppc] serial port discovery and zilog device driver
> >     
> I'm pretty unhappy with this commit. Other than the cosmetic issues,
> which are certainly present, some of my immediate problems are:
>
> - the existence of 'global_serial_port'
>   - Why can't you call e.g. ns16550_init() from
> boot_of_serial_ns16550()? That's the exact same principle I suggested
> for the "platform detection" patch.
>   
I have no opinion on this one.  Feel free to change it one way or 
another.   I tried to minimize the number of changes made to xen, and 
left the initialization in setup.c.  You might not have all the pieces 
you need to initialize the serial port that early.  Also if you change 
this code, please be aware that we are currently having problems in this 
area on the js20 and js21.  On the js20 there is a patch to fix things 
(detect platform and non-hypervisor mode).   On the js21 we are still 
debugging.   We might want wait until both problems are resolved before 
we change invocation of the serial device initialization code. 
>   - 'global_serial_port' seems to store data that's never used, such as
> 'clock'.
>   
Indeed clock is not used.  I can remove it, but in the grand scheme of 
things does not seem like such a big deal.  I would hold off making this 
change though.  I already have one set of changes in the file boot_of.c 
(see above, js20 serial port fix) and more cleanup to do there 
unrelated.  Removing clock would be a good thing to do then and I'll add 
that to my list.
> - boot_of_serial_zilog/ns16550() do lots of getprop() calls to find
> addr_cells and size_cells, then discard the values.
>   
This is debug code and I can assure that as we move from one machine 
type to another is really needed.  Once we stabilize, we can remove the 
code.  Furthermore, I have done the mpic discovery in a slightly 
different way and hope to do dart in the same.  If the different way 
work on all the platforms, the I would advocate doing it in the same way 
for serial port discovery and in that case we would be using the values 
for address-cells. 
> Before I spend too much time hacking at it, Maria, do you have a
> subsequent patch you're working on right now that will clean this code
> up?
>
>   
BTW, I posted various versions of this patch as I was working on it and 
I did not hear from you. 

_______________________________________________
Xen-ppc-devel mailing list
Xen-ppc-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-ppc-devel