[PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 5734 bytes --]

Hi everybody,

the following patch fixes SMC DPRAM handling on CPM2-based platforms. It makes
the cpm_uart driver independent of any SMC initialisation made by the boot
loader. This is required to be able to reset the CPM in cpm2_reset() which
should be the next step in making CPM2 initialisation independent of the
boot loader.

I was concerned this would break udbg, but udbg doesn't seem to be supported
on PQ2-based platforms.

---
This patch allocates parameter RAM for SMC serial ports without relying on
previous initialisation by a boot loader or a wrapper layer.

SMC parameter RAM on CPM2-based platforms can be allocated anywhere in the
general-purpose areas of the dual-port RAM. The current code relies on the
boot loader to allocate a section of general-purpose CPM RAM and gets the
section address from the device tree.

This patch modifies the device tree address usage to reference the SMC
parameter RAM base pointer instead of a pre-allocated RAM section and
allocates memory from the CPM dual-port RAM when initialising the SMC port.
CPM1-based platforms are not affected.

Signed-off-by: Laurent Pinchart <laurentp@cse-semaphore.com>
---
 drivers/serial/cpm_uart/cpm_uart.h      |    3 ++
 drivers/serial/cpm_uart/cpm_uart_core.c |   19 +++++++--------
 drivers/serial/cpm_uart/cpm_uart_cpm1.c |   12 ++++++++++
 drivers/serial/cpm_uart/cpm_uart_cpm2.c |   37 +++++++++++++++++++++++++++++++
 4 files changed, 61 insertions(+), 10 deletions(-)

diff --git a/drivers/serial/cpm_uart/cpm_uart.h b/drivers/serial/cpm_uart/cpm_uart.h
index 80a7d60..5334653 100644
--- a/drivers/serial/cpm_uart/cpm_uart.h
+++ b/drivers/serial/cpm_uart/cpm_uart.h
@@ -93,6 +93,9 @@ extern struct uart_cpm_port cpm_uart_ports[UART_NR];
 
 /* these are located in their respective files */
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd);
+void __iomem *cpm_uart_map_pram(struct uart_cpm_port *port,
+				struct device_node *np);
+void cpm_uart_unmap_pram(struct uart_cpm_port *port, void __iomem *pram);
 int cpm_uart_init_portdesc(void);
 int cpm_uart_allocbuf(struct uart_cpm_port *pinfo, unsigned int is_con);
 void cpm_uart_freebuf(struct uart_cpm_port *pinfo);
diff --git a/drivers/serial/cpm_uart/cpm_uart_core.c b/drivers/serial/cpm_uart/cpm_uart_core.c
index af875ad..56f39ca 100644
--- a/drivers/serial/cpm_uart/cpm_uart_core.c
+++ b/drivers/serial/cpm_uart/cpm_uart_core.c
@@ -997,24 +997,23 @@ static int cpm_uart_init_port(struct device_node *np,
 	if (!mem)
 		return -ENOMEM;
 
-	pram = of_iomap(np, 1);
-	if (!pram) {
-		ret = -ENOMEM;
-		goto out_mem;
-	}
-
 	if (of_device_is_compatible(np, "fsl,cpm1-scc-uart") ||
 	    of_device_is_compatible(np, "fsl,cpm2-scc-uart")) {
 		pinfo->sccp = mem;
-		pinfo->sccup = pram;
+		pinfo->sccup = pram = cpm_uart_map_pram(pinfo, np);
 	} else if (of_device_is_compatible(np, "fsl,cpm1-smc-uart") ||
 	           of_device_is_compatible(np, "fsl,cpm2-smc-uart")) {
 		pinfo->flags |= FLAG_SMC;
 		pinfo->smcp = mem;
-		pinfo->smcup = pram;
+		pinfo->smcup = pram = cpm_uart_map_pram(pinfo, np);
 	} else {
 		ret = -ENODEV;
-		goto out_pram;
+		goto out_mem;
+	}
+
+	if (!pram) {
+		ret = -ENOMEM;
+		goto out_mem;
 	}
 
 	pinfo->tx_nrfifos = TX_NUM_FIFO;
@@ -1038,7 +1037,7 @@ static int cpm_uart_init_port(struct device_node *np,
 	return cpm_uart_request_port(&pinfo->port);
 
 out_pram:
-	iounmap(pram);
+	cpm_uart_unmap_pram(pinfo, pram);
 out_mem:
 	iounmap(mem);
 	return ret;
diff --git a/drivers/serial/cpm_uart/cpm_uart_cpm1.c b/drivers/serial/cpm_uart/cpm_uart_cpm1.c
index 52fb044..060f566 100644
--- a/drivers/serial/cpm_uart/cpm_uart_cpm1.c
+++ b/drivers/serial/cpm_uart/cpm_uart_cpm1.c
@@ -58,6 +58,18 @@ void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 	while (in_be16(cpcr) & CPM_CR_FLG)
 		;
 }
+
+void __iomem *cpm_uart_map_pram(struct uart_cpm_port *port,
+				struct device_node *np)
+{
+	return of_iomap(np, 1);
+}
+
+void cpm_uart_unmap_pram(struct uart_cpm_port *port, void __iomem *pram)
+{
+	iounmap(pram);
+}
+
 #else
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 {
diff --git a/drivers/serial/cpm_uart/cpm_uart_cpm2.c b/drivers/serial/cpm_uart/cpm_uart_cpm2.c
index 88daad1..9391dc6 100644
--- a/drivers/serial/cpm_uart/cpm_uart_cpm2.c
+++ b/drivers/serial/cpm_uart/cpm_uart_cpm2.c
@@ -41,6 +41,9 @@
 #include <asm/io.h>
 #include <asm/irq.h>
 #include <asm/fs_pd.h>
+#ifdef CONFIG_PPC_CPM_NEW_BINDING
+#include <asm/prom.h>
+#endif
 
 #include <linux/serial_core.h>
 #include <linux/kernel.h>
@@ -60,6 +63,40 @@ void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 
 	cpm2_unmap(cp);
 }
+
+void __iomem *cpm_uart_map_pram(struct uart_cpm_port *port,
+				struct device_node *np)
+{
+	void __iomem *pram;
+	unsigned long offset;
+
+	/* Don't remap parameter RAM if it has already been initialized
+	 * during console setup.
+	 */
+	if (IS_SMC(port) && port->smcup)
+		return port->smcup;
+	else if (!IS_SMC(port) && port->sccup)
+		return port->sccup;
+
+	pram = of_iomap(np, 1);
+	if (!pram)
+		return NULL;
+
+	if (!IS_SMC(port))
+		return pram;
+
+	offset = cpm_dpalloc(PROFF_SMC_SIZE, 64);
+	out_be16(pram, offset);
+	iounmap(pram);
+	return cpm_muram_addr(offset);
+}
+
+void cpm_uart_unmap_pram(struct uart_cpm_port *port, void __iomem *pram)
+{
+	if (!(port->flags & FLAG_SMC))
+		iounmap(pram);
+}
+
 #else
 void cpm_line_cr_cmd(struct uart_cpm_port *port, int cmd)
 {
-- 
1.5.0


-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Scott Wood]

On Tue, Mar 25, 2008 at 12:24:40PM +0100, Laurent Pinchart wrote:
> I was concerned this would break udbg, but udbg doesn't seem to be supported
> on PQ2-based platforms.

Yes, it is (see arch/powerpc/sysdev/cpm_common.c).

> This patch modifies the device tree address usage to reference the SMC
> parameter RAM base pointer instead of a pre-allocated RAM section and
> allocates memory from the CPM dual-port RAM when initialising the SMC port.
> CPM1-based platforms are not affected.

Please maintain backward compatibility with older device trees (by
checking the length of the second reg resource).  At the very least,
update the device trees that are affected.

> +	offset = cpm_dpalloc(PROFF_SMC_SIZE, 64);
> +	out_be16(pram, offset);

Up to this point, if we don't reset the CPM prior to any dpalloc calls
(and if we do, udbg printk breaks), the SMC could be running and
clobbering some other bit of dpram, which could have been allocated to
something else.

After this point, even if you don't reset the CPM, udbg printk is broken
because you moved pram.  The udbg disabling will have to be moved before
this.

-Scott

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

On Tuesday 25 March 2008 15:58, Scott Wood wrote:
> On Tue, Mar 25, 2008 at 12:24:40PM +0100, Laurent Pinchart wrote:
> > I was concerned this would break udbg, but udbg doesn't seem to be 
supported
> > on PQ2-based platforms.
> 
> Yes, it is (see arch/powerpc/sysdev/cpm_common.c).

I thought that was for CPM1 only. My bad.
 
> > This patch modifies the device tree address usage to reference the SMC
> > parameter RAM base pointer instead of a pre-allocated RAM section and
> > allocates memory from the CPM dual-port RAM when initialising the SMC 
port.
> > CPM1-based platforms are not affected.
> 
> Please maintain backward compatibility with older device trees (by
> checking the length of the second reg resource).  At the very least,
> update the device trees that are affected.

I haven't seen any CPM2-based board using SMC ports in the device trees 
available in arch/powerpc/boot/dts.

Should I still maintain compatibility with older device trees ? Is there any 
out-of-tree PQ2 boards using udbg and SMC ? What about printing a warning if 
the second reg resource has the wrong size ?

> > +	offset = cpm_dpalloc(PROFF_SMC_SIZE, 64);
> > +	out_be16(pram, offset);
> 
> Up to this point, if we don't reset the CPM prior to any dpalloc calls
> (and if we do, udbg printk breaks), the SMC could be running and
> clobbering some other bit of dpram, which could have been allocated to
> something else.

If udbg uses the parameter RAM allocated by the boot loader, that section of 
DPRAM should be removed from the muram node in the device tree. Otherwise the 
SMC DPRAM will eventually be allocated to something else and the system will 
break.

It should thus be safe to reset the CPM if udbg isn't used, and the device 
tree should explicitely exclude the pre-allocated parameter RAM from the 
muram node when udbg is used.

> After this point, even if you don't reset the CPM, udbg printk is broken
> because you moved pram.  The udbg disabling will have to be moved before
> this.

Moving the SMC pram doesn't break udbg printk in itself. What will break it is 
moving the TX BDs, but the end result is the same, moving pram will result in 
udbg being broken.

The cpm_uart driver disable udbg before allocating the new BDs. What about 
moving that right before moving the parameter RAM ? cpm_uart_request_port(), 
which is called in between, already disables RX and TX on the port, so we 
won't loose any debug message.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Scott Wood]

On Tue, Mar 25, 2008 at 04:34:46PM +0100, Laurent Pinchart wrote:
> On Tuesday 25 March 2008 15:58, Scott Wood wrote:
> > Please maintain backward compatibility with older device trees (by
> > checking the length of the second reg resource).  At the very least,
> > update the device trees that are affected.
> 
> I haven't seen any CPM2-based board using SMC ports in the device trees 
> available in arch/powerpc/boot/dts.

ep8248e

> Should I still maintain compatibility with older device trees ? Is there any 
> out-of-tree PQ2 boards using udbg and SMC ?

Yes, I've answered questions on the lists from at least one person using
a custom board with cpm2 smc.

> What about printing a warning if the second reg resource has the wrong
> size ?

The only way you'll see the warning is if udbg is enabled. :-P

Will a CPM reset blow away the portion of muram that holds the SMC pram
pointer?  If not (and I don't think it will), just return the device tree
reg resource as is currently done if the resource is the wrong size.

> > After this point, even if you don't reset the CPM, udbg printk is broken
> > because you moved pram.  The udbg disabling will have to be moved before
> > this.
> 
> Moving the SMC pram doesn't break udbg printk in itself. What will break it is 
> moving the TX BDs, but the end result is the same, moving pram will result in 
> udbg being broken.
> 
> The cpm_uart driver disable udbg before allocating the new BDs. What about 
> moving that right before moving the parameter RAM ? cpm_uart_request_port(), 
> which is called in between, already disables RX and TX on the port, so we 
> won't loose any debug message.

cpm_uart_request_port() returns without doing that if it's a console
port.  I think the current placement of the udbg disable will be fine.

-Scott

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 2452 bytes --]

On Tuesday 25 March 2008 17:03, Scott Wood wrote:
> On Tue, Mar 25, 2008 at 04:34:46PM +0100, Laurent Pinchart wrote:
> > On Tuesday 25 March 2008 15:58, Scott Wood wrote:
> > > Please maintain backward compatibility with older device trees (by
> > > checking the length of the second reg resource).  At the very least,
> > > update the device trees that are affected.
> > 
> > I haven't seen any CPM2-based board using SMC ports in the device trees 
> > available in arch/powerpc/boot/dts.
> 
> ep8248e

I should have checked git head. My bad. I'll include the ep8248e device tree 
in the next patch.

> > Should I still maintain compatibility with older device trees ? Is there 
any 
> > out-of-tree PQ2 boards using udbg and SMC ?
> 
> Yes, I've answered questions on the lists from at least one person using
> a custom board with cpm2 smc.

Are you sure it wasn't me ? ;-)

> > What about printing a warning if the second reg resource has the wrong
> > size ?
> 
> The only way you'll see the warning is if udbg is enabled. :-P
> 
> Will a CPM reset blow away the portion of muram that holds the SMC pram
> pointer?  If not (and I don't think it will), just return the device tree
> reg resource as is currently done if the resource is the wrong size.

Ok I'll do that. Should I add a warning message to tell people to update the 
device tree ?

> > > After this point, even if you don't reset the CPM, udbg printk is broken
> > > because you moved pram.  The udbg disabling will have to be moved before
> > > this.
> > 
> > Moving the SMC pram doesn't break udbg printk in itself. What will break 
it is 
> > moving the TX BDs, but the end result is the same, moving pram will result 
in 
> > udbg being broken.
> > 
> > The cpm_uart driver disable udbg before allocating the new BDs. What about 
> > moving that right before moving the parameter RAM ? 
cpm_uart_request_port(), 
> > which is called in between, already disables RX and TX on the port, so we 
> > won't loose any debug message.
> 
> cpm_uart_request_port() returns without doing that if it's a console
> port.  I think the current placement of the udbg disable will be fine.

Ok. I'll prepare a new patch that maintains compatibility with old device 
trees.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

RE: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports onCPM2-based platforms.

[Rune Torgersen]

>Laurent Pinchart wrote:
> On Tuesday 25 March 2008 17:03, Scott Wood wrote:
>> Yes, I've answered questions on the lists from at least one person
>> using a custom board with cpm2 smc.
>=20
> Are you sure it wasn't me ? ;-)
>=20

Probably me, actually. We have a 8280 with SMC's in use (SMC1 and 2)

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports onCPM2-based platforms.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 605 bytes --]

On Tuesday 25 March 2008 17:27, Rune Torgersen wrote:
> >Laurent Pinchart wrote:
> > On Tuesday 25 March 2008 17:03, Scott Wood wrote:
> >> Yes, I've answered questions on the lists from at least one person
> >> using a custom board with cpm2 smc.
> > 
> > Are you sure it wasn't me ? ;-)
> > 
> 
> Probably me, actually. We have a 8280 with SMC's in use (SMC1 and 2)

Ok.

Do you have any opinion about the proposed patch ?

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

RE: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports onCPM2-based platforms.

[Rune Torgersen]

Laurent Pinchart wrote:
> On Tuesday 25 March 2008 17:27, Rune Torgersen wrote:
>>> Laurent Pinchart wrote:
>>> On Tuesday 25 March 2008 17:03, Scott Wood wrote:
>>>> Yes, I've answered questions on the lists from at least one person
>>>> using a custom board with cpm2 smc.
>>>=20
>>> Are you sure it wasn't me ? ;-)
>>=20
>> Probably me, actually. We have a 8280 with SMC's in use (SMC1 and 2)
> Do you have any opinion about the proposed patch ?

That is about time... :)
I've never really liked that the smc driver in the kernel relies on
parts of the SMC port to be set up by the boot-loader.
It really bit us when we were porting to 2.6.24, and was trying to
enable SMC2, since it was not used by u-boot as a console port, the pram
was uninitialized, and the linux driver then completely borked both
smc's.

This should take care of that, it looks like.

No driver should rely on the boot loader to initialize the device...
(With the exception of pin mappinmgs, PCI and memory...)

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports on CPM2-based platforms.

[Scott Wood]

Laurent Pinchart wrote:
> On Tuesday 25 March 2008 17:03, Scott Wood wrote:
>> Yes, I've answered questions on the lists from at least one person using
>> a custom board with cpm2 smc.
> 
> Are you sure it wasn't me ? ;-)

Yes. :-)

>> Will a CPM reset blow away the portion of muram that holds the SMC pram
>> pointer?  If not (and I don't think it will), just return the device tree
>> reg resource as is currently done if the resource is the wrong size.
> 
> Ok I'll do that. Should I add a warning message to tell people to update the 
> device tree ?

Sure.

-Scott

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports onCPM2-based platforms.

[Sergej Stepanov]

Am Dienstag, den 25.03.2008, 17:32 +0100 schrieb Laurent Pinchart:

> Do you have any opinion about the proposed patch ?
> 

I have to say, it could be some off-topic.
But if you would use the cpm_uart-driver for cpm2(or cpm1) as kernel
module, at linking you can get warnings about:
	- cpm_setbrg (cpm_set_brg): something with section mismatch
	-  __alloc_bootmem (cpm_uart_allocbuf): the same

The symbols are not exported.
Is it ok, about kernel modules?

Sergej.

Re: [PATCH] cpm_uart: Allocate DPRAM memory for SMC ports onCPM2-based platforms.

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 856 bytes --]

On Wednesday 26 March 2008 09:31, Sergej Stepanov wrote:
> 
> Am Dienstag, den 25.03.2008, 17:32 +0100 schrieb Laurent Pinchart:
> 
> > Do you have any opinion about the proposed patch ?
> > 
> 
> I have to say, it could be some off-topic.
> But if you would use the cpm_uart-driver for cpm2(or cpm1) as kernel
> module, at linking you can get warnings about:
> 	- cpm_setbrg (cpm_set_brg): something with section mismatch
> 	-  __alloc_bootmem (cpm_uart_allocbuf): the same
> 
> The symbols are not exported.
> Is it ok, about kernel modules?

I get the same section-mismatch warning for __alloc_bootmem in 
cpm_uart_allocbuf. cpm_setbrg doesn't cause any warning here.

Best regards,

-- 
Laurent Pinchart
CSE Semaphore Belgium

Chaussée de Bruxelles, 732A
B-1410 Waterloo
Belgium

T +32 (2) 387 42 59
F +32 (2) 387 42 75

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]