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
