Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, Jul 12, 2007 at 11:47:12AM -0700, Andrew Morton wrote: > There is no power management support in this driver. There is hardly anything software could do to safe power on Sibyte SOCs. Just like most other MIPS SOCs power managment is mostly left to the hardware which does it by using extensive clockgating and other design techniques. As a SW person I can't protest that approach :-) Ralf - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, 12 Jul 2007, Andrew Morton wrote:
> On Thu, 12 Jul 2007 18:39:00 +0100 (BST)
> "Maciej W. Rozycki" <[EMAIL PROTECTED]> 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
> > +#include
> > +
> > +#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
> > +#include
> > +
> > +#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.
> > + */
>
>
>
>
>
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, 12 Jul 2007 11:47:12 -0700 Andrew Morton
<[EMAIL PROTECTED]> wrote:
> On Thu, 12 Jul 2007 18:39:00 +0100 (BST)
> "Maciej W. Rozycki" <[EMAIL PROTECTED]> 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
> > +#include
> > +
> > +#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
> > +#include
> > +
> > +#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)
>
> > +#define to_sport(uport) container_of(uport, struct sbd_port, port)
>
> That didn't need to be implemented as a macro.
>
> > +#define __unused __attribute__((__unused__))
>
> Please use the generic implementations here. `grep unused
> include/linux/compiler*.h'.
>
> > +/*
> > + * In bug 1956, we get glitches that can mess up uart registers.
> > This
> > + * "read-mode-reg after any register access" is an accepted
> > workaround.
> > + */
>
>
>
>
>
> Perhaps a reference to where that bug number came from?
It's a Sibyte errata/WAR number as I recall.
> > +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.
>
>
>
>
> There is no power management support in this driver.
>
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, 12 Jul 2007 20:15:11 +0100 Alistair John Strachan <[EMAIL PROTECTED]> wrote: > On Thursday 12 July 2007 19:16:20 Maciej W. Rozycki wrote: > > On Thu, 12 Jul 2007, Andy Whitcroft wrote: > [snip] > > > WARNING: declaring multiple variables together should be avoided > > > #372: FILE: drivers/serial/sb1250-duart.c:246: > > > + unsigned int mctrl, status; > > > > Well, this is probably superfluous -- why would anyone prefer: > > > > int r0; > > int r1; > > int r2; > > int r3; > > int r4; > > > > to: > > > > int r0, r1, r2, r3, r4; > > > > unconditionally? > > Imagine you're working on a piece of kernel code that has a lot of parallel > churn. Conflicts on lines like "int a,b,c,d;" are more likely to cause Andrew > et al pain, which I guess is the rationale for discouraging it. Conversely, > if the variables are kept separate, diff handles it fine. That, plus the first style leaves room for useful code comments. The lack of which is often a maintainability bug. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thursday 12 July 2007 19:16:20 Maciej W. Rozycki wrote: > On Thu, 12 Jul 2007, Andy Whitcroft wrote: [snip] > > WARNING: declaring multiple variables together should be avoided > > #372: FILE: drivers/serial/sb1250-duart.c:246: > > + unsigned int mctrl, status; > > Well, this is probably superfluous -- why would anyone prefer: > > int r0; > int r1; > int r2; > int r3; > int r4; > > to: > > int r0, r1, r2, r3, r4; > > unconditionally? Imagine you're working on a piece of kernel code that has a lot of parallel churn. Conflicts on lines like "int a,b,c,d;" are more likely to cause Andrew et al pain, which I guess is the rationale for discouraging it. Conversely, if the variables are kept separate, diff handles it fine. I think as long as the variables are logically grouped, the pain is minimised, but there's a few good reasons for the verbose style. -- Cheers, Alistair. 137/1 Warrender Park Road, Edinburgh, UK. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, 12 Jul 2007 18:39:00 +0100 (BST)
"Maciej W. Rozycki" <[EMAIL PROTECTED]> 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
> +#include
> +
> +#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
> +#include
> +
> +#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)
> +#define to_sport(uport) container_of(uport, struct sbd_port, port)
That didn't need to be implemented as a macro.
> +#define __unused __attribute__((__unused__))
Please use the generic implementations here. `grep unused
include/linux/compiler*.h'.
> +/*
> + * In bug 1956, we get glitches that can mess up uart registers. This
> + * "read-mode-reg after any register access" is an accepted workaround.
> + */
Perhaps a reference to where that bug number came from?
> +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.
There is no power management support in this driver.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, 12 Jul 2007, Andy Whitcroft wrote: > > printk() should include KERN_ facility level > > #750: FILE: drivers/serial/sb1250-duart.c:675: > > + printk(err); > > Heh, yeah Ingo pointed this style out. This is a wrapper where the > facility will be supplied by the caller (I assume). The thought there Actually "err" is "static const char *", except it is used twice in the function, so rather than cluttering the source with two identical strings and relying on GCC merging them I did it explicitly. > was that only complain on printks which had a string literal as their > first arguement. That gets us very high accuracy and eliminates these > falsies. That would be my suggestion too, if you asked me. But as did not, I do not either. > I think I tend to agree that the MAKEMASK ones are separate. Good to > see someone using their common sense in the face of whinging by the tool. Thanks for appreciation. ;-) > WARNING: declaring multiple variables together should be avoided > #372: FILE: drivers/serial/sb1250-duart.c:246: > + unsigned int mctrl, status; Well, this is probably superfluous -- why would anyone prefer: int r0; int r1; int r2; int r3; int r4; to: int r0, r1, r2, r3, r4; unconditionally? I agree clustering variable declarations may obfuscate the code, but then again, a bit of common sense should be used. It usually makes sense to group related variables together and declare other ones separately. And obviously if somebody writes unreadable code, then it is hard to change the habit with a script no matter how much you try. Maciej - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
On Thu, Jul 12, 2007 at 06:39:00PM +0100, Maciej W. Rozycki 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. > > Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]> I'm all in favor of this patch sine the old drivers/char/ serial driver has been slowly decaying while the kernel around it is changing. It would also make the kernel.org kernel finally usable for the Sibyte machines, so it's an improvment even if it doesn't have half a decade of testing under the belt, thus ACK. Ralf - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] sb1250-duart.c: SB1250 DUART serial support
Maciej W. Rozycki 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. > > Signed-off-by: Maciej W. Rozycki <[EMAIL PROTECTED]> > --- > Hi, > > The driver was tested with a SWARM board which uses a BCM1250 SOC (which > is dual MIPS64 CMP) and has both ports of the single DUART implemented > wired externally. Both were tested. Testing included using the ports as > terminal lines at 1200bps (which is the ports minimum), 115200bps and a > couple of random speeds inbetween. The modem lines were verified to > operate correctly. No testing was performed with a use as a network > interface, like with SLIP or PPP. > > The driver builds succesfully and without warnings both as integrated and > as modular. There are a couple of -W warnings, but they are results of > some inconsistencies (signedness mismatches) in the serial core. It > produces no sparse warnings. There are a few benign warnings from > patchcheck.pl, one of which is, I believe, a bug in the script itself > (maintainers cc-ed): > > printk() should include KERN_ facility level > #750: FILE: drivers/serial/sb1250-duart.c:675: > + printk(err); Heh, yeah Ingo pointed this style out. This is a wrapper where the facility will be supplied by the caller (I assume). The thought there was that only complain on printks which had a string literal as their first arguement. That gets us very high accuracy and eliminates these falsies. > > The warning is, I gather, not a problem and warnings about the > _SB_MAKEMASK() macro should be addressed as a separate change. I think I tend to agree that the MAKEMASK ones are separate. Good to see someone using their common sense in the face of whinging by the tool. You will be pleased to know that the latest version of the tool is throwing a new batch of errors on your patch :) Included at the end of the email. > The driver runs correctly with a 64-bit SMP kernel in a big- and > little-endian configuration (with spinlock debugging enabled). Modular > configuration was not tested at the run time. UP configuration was not > tested at all, but is not expected to give any troubles. > > I have asked for testing at the linux-mips list, but rather than results > I have received some pressure to push the patch regardless. So here it > goes. ;-) -apw WARNING: declaring multiple variables together should be avoided #372: FILE: drivers/serial/sb1250-duart.c:246: + unsigned int mctrl, status; WARNING: declaring multiple variables together should be avoided #386: FILE: drivers/serial/sb1250-duart.c:260: + unsigned int clr = 0, set = 0, mode2; WARNING: declaring multiple variables together should be avoided #464: FILE: drivers/serial/sb1250-duart.c:338: + unsigned int status, ch, flag; WARNING: declaring multiple variables together should be avoided #667: FILE: drivers/serial/sb1250-duart.c:541: + unsigned int mode1 = 0, mode2 = 0, aux = 0; WARNING: declaring multiple variables together should be avoided #668: FILE: drivers/serial/sb1250-duart.c:542: + unsigned int mode1mask = 0, mode2mask = 0, auxmask = 0; WARNING: declaring multiple variables together should be avoided #669: FILE: drivers/serial/sb1250-duart.c:543: + unsigned int oldmode1, oldmode2, oldaux; WARNING: declaring multiple variables together should be avoided #670: FILE: drivers/serial/sb1250-duart.c:544: + unsigned int baud, brg; WARNING: declaring multiple variables together should be avoided #907: FILE: drivers/serial/sb1250-duart.c:781: + int chip, side; WARNING: declaring multiple variables together should be avoided #908: FILE: drivers/serial/sb1250-duart.c:782: + int max_lines, line; WARNING: declaring multiple variables together should be avoided #1060: FILE: drivers/serial/sb1250-duart.c:934: + int i, ret; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
