Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support

[Maciej W. Rozycki]

On Thu, 12 Jul 2007, Andrew Morton wrote:

> On Thu, 12 Jul 2007 18:39:00 +0100 (BST)
> "Maciej W. Rozycki" <macro@xxxxxxxxxxxxxx> wrote:
> 
> >  This is a driver for the SB1250 DUART, a dual serial port implementation 
> > included in the Broadcom family of SOCs descending from the SiByte SB1250 
> > MIPS64 chip multiprocessor.  It is a new implementation replacing the 
> > old-fashioned driver currently present in the linux-mips.org tree.  It 
> > supports all the usual features one would expect from a(n asynchronous) 
> > serial driver, including modem line control (as far as hardware supports 
> > it -- there is edge detection logic missing from the DCD and RI lines and 
> > the driver does not implement polling of these lines at the moment), the 
> > serial console, BREAK transmission and reception, including the magic 
> > SysRq.  The receive FIFO threshold is not maintained though.
> > 
> > ...
> >
> > +
> > +#if defined(CONFIG_SIBYTE_BCM1x55) || defined(CONFIG_SIBYTE_BCM1x80)
> > +#include <asm/sibyte/bcm1480_regs.h>
> > +#include <asm/sibyte/bcm1480_int.h>
> > +
> > +#define SBD_CHANREGS(line)	A_BCM1480_DUART_CHANREG((line), 0)
> > +#define SBD_CTRLREGS(line)	A_BCM1480_DUART_CTRLREG((line), 0)
> > +#define SBD_INT(line)		(K_BCM1480_INT_UART_0 + (line))
> > +
> > +#elif defined(CONFIG_SIBYTE_SB1250) || defined(CONFIG_SIBYTE_BCM112X)
> > +#include <asm/sibyte/sb1250_regs.h>
> > +#include <asm/sibyte/sb1250_int.h>
> > +
> > +#define SBD_CHANREGS(line)	A_DUART_CHANREG((line), 0)
> > +#define SBD_CTRLREGS(line)	A_DUART_CTRLREG(0)
> > +#define SBD_INT(line)		(K_INT_UART_0 + (line))
> > +
> > +#else
> > +#error invalid SB1250 UART configuration
> > +
> > +#endif
> 
> If the #error can trigger, the Kconfig is broken, yes?  (No action is
> required though - it's always good to have checks)

 It should not trigger, because to the best of my knowledge all the chip 
variations we support are covered above.  Ultimately, this should become a 
platform device, where this will become a non-issue.  The conditions have 
been copied verbatim from the original driver.

> > +#define to_sport(uport) container_of(uport, struct sbd_port, port)
> 
> That didn't need to be implemented as a macro.

 Like the last time -- everybody seems to implement it as such.  If you 
think that should be changed, I could see if I find some time to grep the 
tree and reimplement all the to_*() macros.

> > +#define __unused __attribute__((__unused__))
> 
> Please use the generic implementations here.  `grep unused
> include/linux/compiler*.h'.

 Any changes there in the last few days?  Hmm, I must have missed them...  
I'll fix it up next week.

> > +/*
> > + * In bug 1956, we get glitches that can mess up uart registers.  This
> > + * "read-mode-reg after any register access" is an accepted workaround.
> > + */
> 
> <looks in bugzilla.kernel.org>
> 
> <wonders how a uart driver can fix an smbfs bug>
> 
> Perhaps a reference to where that bug number came from?

 Again, copied verbatim from the original driver.  It must be a chip 
erratum.  Which is not publicly available, but I think it is still better 
to have it referred to from here than not at all, in case someone wants to 
pester Broadcom about this one.  I can s/bug/erratum/, but I am not sure 
if it won't confuse them then if contacted. ;-)

> > +static void __war_sbd1956(struct sbd_port *sport)
> > +{
> > +	__read_sbdchn(sport, R_DUART_MODE_REG_1);
> > +	__read_sbdchn(sport, R_DUART_MODE_REG_2);
> > +}
> >
> > ...
> >
> > +static struct uart_ops sbd_ops = {
> 
> I suppose if we made this const, something would blow up.

 No problem at the moment -- I'll change it.  Though I have a strong 
suspicion .startup may have to be overridden at the run time at one point 
because of the way the port #1 is wired on the SWARM board (but nowhere 
else) -- the port is shared with a sound device (if it sounds odd, I can 
provide an overview of the hardware configuration).  Then again, perhaps I 
will do it in a different way...

> There is no power management support in this driver.

 Like nowhere in the chip.

  Maciej
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/