[PATCH 2/4][SSB] EXTIF serial port initialization

[PATCH 2/4][SSB] EXTIF serial port initialization

[Aurelien Jarno]

The patch below against 2.6.23-rc1-mm2 implements EXTIF serial
initialization, currently marked as TODO.

It originally comes from the OpenWrt patches.

Cc: Felix Fietkau <nbd@openwrt.org>
Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>

--- a/drivers/ssb/driver_mipscore.c
+++ b/drivers/ssb/driver_mipscore.c
@@ -128,51 +128,52 @@
 	ssb_write32(mdev, SSB_IPSFLAG, irqflag);
 }
 
-/* XXX: leave here or move into separate extif driver? */
-static int ssb_extif_serial_init(struct ssb_device *dev, struct ssb_serial_ports *ports)
+static inline bool serial_exists(u8 *regs)
 {
+	u8 save_mcr, msr = 0;
 
+	if (regs) {
+		save_mcr = regs[UART_MCR];
+		regs[UART_MCR] = (UART_MCR_LOOP | UART_MCR_OUT2 | UART_MCR_RTS);
+		msr = regs[UART_MSR] & (UART_MSR_DCD | UART_MSR_RI \
+					| UART_MSR_CTS | UART_MSR_DSR);
+		regs[UART_MCR] = save_mcr;
+	}
+	return (msr == (UART_MSR_DCD | UART_MSR_CTS));
 }
 
-
-static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
+static int ssb_extif_serial_init(struct ssb_extif *dev, struct ssb_serial_port *ports)
 {
-	struct ssb_bus *bus = mcore->dev->bus;
+	u32 i, nr_ports = 0;
 
-	//TODO if (EXTIF available
-#if 0
-		extifregs_t *eir = (extifregs_t *) regs;
-		sbconfig_t *sb;
-
-		/* Determine external UART register base */
-		sb = (sbconfig_t *)((ulong) eir + SBCONFIGOFF);
-		base = EXTIF_CFGIF_BASE(sb_base(R_REG(&sb->sbadmatch1)));
-
-		/* Determine IRQ */
-		irq = sb_irq(sbh);
-
-		/* Disable GPIO interrupt initially */
-		W_REG(&eir->gpiointpolarity, 0);
-		W_REG(&eir->gpiointmask, 0);
-
-		/* Search for external UARTs */
-		n = 2;
-		for (i = 0; i < 2; i++) {
-			regs = (void *) REG_MAP(base + (i * 8), 8);
-			if (BCMINIT(serial_exists)(regs)) {
-				/* Set GPIO 1 to be the external UART IRQ */
-				W_REG(&eir->gpiointmask, 2);
-				if (add)
-					add(regs, irq, 13500000, 0);
-			}
+	/* Disable GPIO interrupt initially */
+	extif_write32(dev, SSB_EXTIF_GPIO_INTPOL, 0);
+	extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 0);
+
+	for (i = 0; i < 2; i++) {
+		void __iomem *uart_regs;
+
+		uart_regs = ioremap_nocache(SSB_EUART, 16);
+		uart_regs += (i * 8);
+
+		if (serial_exists(uart_regs) && ports) {
+			extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 2);
+
+			nr_ports++;
+			ports[i].regs = uart_regs;
+			ports[i].irq = 2;
+			ports[i].baud_base = 13500000;
+			ports[i].reg_shift = 0;
 		}
+		iounmap(uart_regs);
+	}
+	return nr_ports;
+}
 
-		/* Add internal UART if enabled */
-		if (R_REG(&eir->corecontrol) & CC_UE)
-			if (add)
-				add((void *) &eir->uartdata, irq, sb_clock(sbh), 2);
+static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
+{
+	struct ssb_bus *bus = mcore->dev->bus;
 
-#endif
 	if (bus->extif.dev)
 		mcore->nr_serial_ports = ssb_extif_serial_init(&bus->extif, mcore->serial_ports);
 	else if (bus->chipco.dev)

-- 
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   aurel32@debian.org         | aurelien@aurel32.net
   `-    people.debian.org/~aurel32 | www.aurel32.net

Re: [PATCH 2/4][SSB] EXTIF serial port initialization

[Arnaldo Carvalho de Melo]

Em Mon, Aug 06, 2007 at 01:19:59AM +0200, Aurelien Jarno escreveu:
> The patch below against 2.6.23-rc1-mm2 implements EXTIF serial
> initialization, currently marked as TODO.
> 
> It originally comes from the OpenWrt patches.

Comments below
 
> Cc: Felix Fietkau <nbd@openwrt.org>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> --- a/drivers/ssb/driver_mipscore.c
> +++ b/drivers/ssb/driver_mipscore.c
> @@ -128,51 +128,52 @@
>  	ssb_write32(mdev, SSB_IPSFLAG, irqflag);
>  }
>  
> -/* XXX: leave here or move into separate extif driver? */
> -static int ssb_extif_serial_init(struct ssb_device *dev, struct ssb_serial_ports *ports)
> +static inline bool serial_exists(u8 *regs)
>  {
> +	u8 save_mcr, msr = 0;

Why declare save_mcr here...

>  
> +	if (regs) {

if it is just used here?

> +		save_mcr = regs[UART_MCR];
> +		regs[UART_MCR] = (UART_MCR_LOOP | UART_MCR_OUT2 | UART_MCR_RTS);
> +		msr = regs[UART_MSR] & (UART_MSR_DCD | UART_MSR_RI \
> +					| UART_MSR_CTS | UART_MSR_DSR);
> +		regs[UART_MCR] = save_mcr;
> +	}
> +	return (msr == (UART_MSR_DCD | UART_MSR_CTS));
>  }
>  
> -
> -static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
> +static int ssb_extif_serial_init(struct ssb_extif *dev, struct ssb_serial_port *ports)
>  {
> -	struct ssb_bus *bus = mcore->dev->bus;
> +	u32 i, nr_ports = 0;
>  
> -	//TODO if (EXTIF available
> -#if 0
> -		extifregs_t *eir = (extifregs_t *) regs;
> -		sbconfig_t *sb;
> -
> -		/* Determine external UART register base */
> -		sb = (sbconfig_t *)((ulong) eir + SBCONFIGOFF);
> -		base = EXTIF_CFGIF_BASE(sb_base(R_REG(&sb->sbadmatch1)));
> -
> -		/* Determine IRQ */
> -		irq = sb_irq(sbh);
> -
> -		/* Disable GPIO interrupt initially */
> -		W_REG(&eir->gpiointpolarity, 0);
> -		W_REG(&eir->gpiointmask, 0);
> -
> -		/* Search for external UARTs */
> -		n = 2;
> -		for (i = 0; i < 2; i++) {
> -			regs = (void *) REG_MAP(base + (i * 8), 8);
> -			if (BCMINIT(serial_exists)(regs)) {
> -				/* Set GPIO 1 to be the external UART IRQ */
> -				W_REG(&eir->gpiointmask, 2);
> -				if (add)
> -					add(regs, irq, 13500000, 0);
> -			}
> +	/* Disable GPIO interrupt initially */
> +	extif_write32(dev, SSB_EXTIF_GPIO_INTPOL, 0);
> +	extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 0);
> +
> +	for (i = 0; i < 2; i++) {
> +		void __iomem *uart_regs;
> +
> +		uart_regs = ioremap_nocache(SSB_EUART, 16);

ioremap_nocache can fail, even if just on, say, an instrumented kernel
that inserts failures in such functions to catch possible errors not
being handled.

> +		uart_regs += (i * 8);
> +
> +		if (serial_exists(uart_regs) && ports) {
> +			extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 2);
> +
> +			nr_ports++;
> +			ports[i].regs = uart_regs;
> +			ports[i].irq = 2;
> +			ports[i].baud_base = 13500000;
> +			ports[i].reg_shift = 0;
>  		}
> +		iounmap(uart_regs);
> +	}
> +	return nr_ports;
> +}
>  
> -		/* Add internal UART if enabled */
> -		if (R_REG(&eir->corecontrol) & CC_UE)
> -			if (add)
> -				add((void *) &eir->uartdata, irq, sb_clock(sbh), 2);
> +static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
> +{
> +	struct ssb_bus *bus = mcore->dev->bus;
>  
> -#endif
>  	if (bus->extif.dev)
>  		mcore->nr_serial_ports = ssb_extif_serial_init(&bus->extif, mcore->serial_ports);
>  	else if (bus->chipco.dev)

Re: [PATCH 2/4][SSB] EXTIF serial port initialization

[Michael Buesch]

On Monday 06 August 2007, Arnaldo Carvalho de Melo wrote:
> Em Mon, Aug 06, 2007 at 01:19:59AM +0200, Aurelien Jarno escreveu:
> > The patch below against 2.6.23-rc1-mm2 implements EXTIF serial
> > initialization, currently marked as TODO.
> > 
> > It originally comes from the OpenWrt patches.
> 
> Comments below
>  
> > Cc: Felix Fietkau <nbd@openwrt.org>
> > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > 
> > --- a/drivers/ssb/driver_mipscore.c
> > +++ b/drivers/ssb/driver_mipscore.c
> > @@ -128,51 +128,52 @@
> >  	ssb_write32(mdev, SSB_IPSFLAG, irqflag);
> >  }
> >  
> > -/* XXX: leave here or move into separate extif driver? */
> > -static int ssb_extif_serial_init(struct ssb_device *dev, struct ssb_serial_ports *ports)
> > +static inline bool serial_exists(u8 *regs)
> >  {
> > +	u8 save_mcr, msr = 0;
> 
> Why declare save_mcr here...
> 
> >  
> > +	if (regs) {
> 
> if it is just used here?

There is almost never an advantage when declaring variables at the beginning
of a statement block instead of the function start.
The only thing you "gain" is that you don't easily see anymore how much
stackspace is used by the funtions.
So I really suggest to declare variables at the beginning of functions,
except for some rare circumstances maybe. Which is not the case here.

But anyway. Just saw this: Drop the inline statement. There is no
need to inline this function.

Re: [PATCH 2/4][SSB] EXTIF serial port initialization

[Arnaldo Carvalho de Melo]

Em Mon, Aug 06, 2007 at 10:51:05AM +0200, Michael Buesch escreveu:
> On Monday 06 August 2007, Arnaldo Carvalho de Melo wrote:
> > Em Mon, Aug 06, 2007 at 01:19:59AM +0200, Aurelien Jarno escreveu:
> > > The patch below against 2.6.23-rc1-mm2 implements EXTIF serial
> > > initialization, currently marked as TODO.
> > > 
> > > It originally comes from the OpenWrt patches.
> > 
> > Comments below
> >  
> > > Cc: Felix Fietkau <nbd@openwrt.org>
> > > Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> > > 
> > > --- a/drivers/ssb/driver_mipscore.c
> > > +++ b/drivers/ssb/driver_mipscore.c
> > > @@ -128,51 +128,52 @@
> > >  	ssb_write32(mdev, SSB_IPSFLAG, irqflag);
> > >  }
> > >  
> > > -/* XXX: leave here or move into separate extif driver? */
> > > -static int ssb_extif_serial_init(struct ssb_device *dev, struct ssb_serial_ports *ports)
> > > +static inline bool serial_exists(u8 *regs)
> > >  {
> > > +	u8 save_mcr, msr = 0;
> > 
> > Why declare save_mcr here...
> > 
> > >  
> > > +	if (regs) {
> > 
> > if it is just used here?
> 
> There is almost never an advantage when declaring variables at the beginning
> of a statement block instead of the function start.
> The only thing you "gain" is that you don't easily see anymore how much
> stackspace is used by the funtions.

Well, I disagree, your argument is flawed because if the function is too
big to have so many variables it should be split in smaller functions
anyway, and when it is not that big, like in the above case one can see
the stack usage just fine on the same screen.

> So I really suggest to declare variables at the beginning of functions,
> except for some rare circumstances maybe. Which is not the case here.

I'd recommend the reverse, but anyway, it is just a suggestion.
 
- Arnaldo

Re: [PATCH 2/4][SSB] EXTIF serial port initialization

[Michael Buesch]

On Monday 06 August 2007, Aurelien Jarno wrote:
> The patch below against 2.6.23-rc1-mm2 implements EXTIF serial
> initialization, currently marked as TODO.
> 
> It originally comes from the OpenWrt patches.
> 
> Cc: Felix Fietkau <nbd@openwrt.org>
> Signed-off-by: Aurelien Jarno <aurelien@aurel32.net>
> 
> --- a/drivers/ssb/driver_mipscore.c
> +++ b/drivers/ssb/driver_mipscore.c

Also to driver_extif.c, please.

> @@ -128,51 +128,52 @@
>  	ssb_write32(mdev, SSB_IPSFLAG, irqflag);
>  }
>  
> -/* XXX: leave here or move into separate extif driver? */
> -static int ssb_extif_serial_init(struct ssb_device *dev, struct ssb_serial_ports *ports)
> +static inline bool serial_exists(u8 *regs)
>  {
> +	u8 save_mcr, msr = 0;
>  
> +	if (regs) {
> +		save_mcr = regs[UART_MCR];
> +		regs[UART_MCR] = (UART_MCR_LOOP | UART_MCR_OUT2 | UART_MCR_RTS);
> +		msr = regs[UART_MSR] & (UART_MSR_DCD | UART_MSR_RI \
> +					| UART_MSR_CTS | UART_MSR_DSR);
> +		regs[UART_MCR] = save_mcr;
> +	}
> +	return (msr == (UART_MSR_DCD | UART_MSR_CTS));
>  }
>  
> -
> -static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
> +static int ssb_extif_serial_init(struct ssb_extif *dev, struct ssb_serial_port *ports)
>  {
> -	struct ssb_bus *bus = mcore->dev->bus;
> +	u32 i, nr_ports = 0;
>  
> -	//TODO if (EXTIF available
> -#if 0
> -		extifregs_t *eir = (extifregs_t *) regs;
> -		sbconfig_t *sb;
> -
> -		/* Determine external UART register base */
> -		sb = (sbconfig_t *)((ulong) eir + SBCONFIGOFF);
> -		base = EXTIF_CFGIF_BASE(sb_base(R_REG(&sb->sbadmatch1)));
> -
> -		/* Determine IRQ */
> -		irq = sb_irq(sbh);
> -
> -		/* Disable GPIO interrupt initially */
> -		W_REG(&eir->gpiointpolarity, 0);
> -		W_REG(&eir->gpiointmask, 0);
> -
> -		/* Search for external UARTs */
> -		n = 2;
> -		for (i = 0; i < 2; i++) {
> -			regs = (void *) REG_MAP(base + (i * 8), 8);
> -			if (BCMINIT(serial_exists)(regs)) {
> -				/* Set GPIO 1 to be the external UART IRQ */
> -				W_REG(&eir->gpiointmask, 2);
> -				if (add)
> -					add(regs, irq, 13500000, 0);
> -			}
> +	/* Disable GPIO interrupt initially */
> +	extif_write32(dev, SSB_EXTIF_GPIO_INTPOL, 0);
> +	extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 0);
> +
> +	for (i = 0; i < 2; i++) {
> +		void __iomem *uart_regs;
> +
> +		uart_regs = ioremap_nocache(SSB_EUART, 16);
> +		uart_regs += (i * 8);
> +
> +		if (serial_exists(uart_regs) && ports) {
> +			extif_write32(dev, SSB_EXTIF_GPIO_INTMASK, 2);
> +
> +			nr_ports++;
> +			ports[i].regs = uart_regs;
> +			ports[i].irq = 2;
> +			ports[i].baud_base = 13500000;
> +			ports[i].reg_shift = 0;
>  		}
> +		iounmap(uart_regs);
> +	}
> +	return nr_ports;
> +}
>  
> -		/* Add internal UART if enabled */
> -		if (R_REG(&eir->corecontrol) & CC_UE)
> -			if (add)
> -				add((void *) &eir->uartdata, irq, sb_clock(sbh), 2);
> +static void ssb_mips_serial_init(struct ssb_mipscore *mcore)
> +{
> +	struct ssb_bus *bus = mcore->dev->bus;
>  
> -#endif
>  	if (bus->extif.dev)
>  		mcore->nr_serial_ports = ssb_extif_serial_init(&bus->extif, mcore->serial_ports);
>  	else if (bus->chipco.dev)
>