From: Kalle Pokki <hidden> Date: 2006-11-06 13:29:53
SMC handling is broken in two places when using then platform device
approach with CPM2 devices.
1. The resources in pq2_devices are not named "regs" and "pram", thus they
are not found in cpm_uart_drv_get_platform_data().
2. The code in cpm_uart_drv_get_platform_data() assumes the parameter RAM
is at "pram". With SMCs of CPM2 devices, "pram" is just a pointer to the
actual parameter RAM, which the code should reserve somewhere in DPRAM.
This patch renames these the two existing resources, and introduces a new one,
"pram_base", which is a pointer to the parameter RAM. The parameter RAM for SMC1
and SMC2 is put in the first 128 bytes of the DPRAM. This memory was already
reserved from the DPRAM memory allocator for this purpose.
Signed-off-by: Kalle Pokki <redacted>
---
arch/ppc/syslib/pq2_devices.c | 24 ++++++++++++++++++------
drivers/serial/cpm_uart/cpm_uart_core.c | 8 +++++++-
2 files changed, 25 insertions(+), 7 deletions(-)
@@ -1024,7 +1024,7 @@ int cpm_uart_drv_get_platform_data(strucintidx=pdata->fs_no;/* It is UART_SMCx or UART_SCCx index */structuart_cpm_port*pinfo;intline;-u32mem,pram;+u32mem,pram,pram_base;line=cpm_uart_id2nr(idx);if(line<0){
@@ -1049,6 +1049,12 @@ int cpm_uart_drv_get_platform_data(strucreturn-EINVAL;pram=r->start;+r=platform_get_resource_byname(pdev,IORESOURCE_MEM,"pram_base");+if(r){+pram_base=r->start;+out_be16((u16*)pram_base,pram&0xffff);+}+if(idx>fsid_smc2_uart){pinfo->sccp=(scc_t*)mem;pinfo->sccup=(scc_uart_t*)pram;
On Mon, 6 Nov 2006 15:29:47 +0200 (EET)
Kalle Pokki wrote:
SMC handling is broken in two places when using then platform device
approach with CPM2 devices.
1. The resources in pq2_devices are not named "regs" and "pram", thus
they are not found in cpm_uart_drv_get_platform_data().
2. The code in cpm_uart_drv_get_platform_data() assumes the parameter
RAM is at "pram". With SMCs of CPM2 devices, "pram" is just a pointer
to the actual parameter RAM, which the code should reserve somewhere
in DPRAM.
Right, since I had nothing to probe with pq2/smc, that part of code remain intact since creation.
Nice to see this stuff tuned finally :)
This patch renames these the two existing resources, and introduces a
new one, "pram_base", which is a pointer to the parameter RAM. The
parameter RAM for SMC1 and SMC2 is put in the first 128 bytes of the
DPRAM. This memory was already reserved from the DPRAM memory
allocator for this purpose.
Well just one objection. pram_base should not be a device unless it applies to all the stuff of
SoC family which is not the case.
For this aim, I'd put what you need into the platform_data, or follow the same approach 8xx stuff having.
Thanks, Vitaly
From: Kalle Pokki <hidden> Date: 2006-11-06 20:49:57
On Mon, 6 Nov 2006, Vitaly Bordug wrote:
quoted
This patch renames these the two existing resources, and introduces a
new one, "pram_base", which is a pointer to the parameter RAM. The
parameter RAM for SMC1 and SMC2 is put in the first 128 bytes of the
DPRAM. This memory was already reserved from the DPRAM memory
allocator for this purpose.
Well just one objection. pram_base should not be a device unless it applies to all the stuff of
SoC family which is not the case.
For this aim, I'd put what you need into the platform_data, or follow the same approach 8xx stuff having.
I'm not sure I follow you now. "pram_base" is not a device by itself, but it's just another resource in the PQ2 version of the SMC devices in the platform_data. So the resource is only present when it is needed, and for other platform devices that don't have this resource, the cpm_uart code does nothing (as it should not).
The only difference to the 8xx version is the new "pram_base" resource, and it is required with PQ2, right?
On Mon, 6 Nov 2006 22:49:49 +0200 (EET)
Kalle Pokki [off-list ref] wrote:
On Mon, 6 Nov 2006, Vitaly Bordug wrote:
quoted
quoted
This patch renames these the two existing resources, and introduces a
new one, "pram_base", which is a pointer to the parameter RAM. The
parameter RAM for SMC1 and SMC2 is put in the first 128 bytes of the
DPRAM. This memory was already reserved from the DPRAM memory
allocator for this purpose.
Well just one objection. pram_base should not be a device unless it applies to all the stuff of
SoC family which is not the case.
For this aim, I'd put what you need into the platform_data, or follow the same approach 8xx stuff having.
I'm not sure I follow you now. "pram_base" is not a device by itself, but
it's just another resource in the PQ2 version of the SMC devices in the
platform_data. So the resource is only present when it is needed, and for
other platform devices that don't have this resource, the cpm_uart code
does nothing (as it should not).
Well, yes, but are you _sure_ pram_base will be the same across all the 82xx PQ2, that happen to have smc wired to Ethernet?
If not I am considering storing it in the platform_data is better approach.
The only difference to the 8xx version is the new "pram_base" resource,
and it is required with PQ2, right?
Prolly, and I actually tried the same when introduced 8xx stuff. It was not permitted because such a thing could not be stuck to one value, and as long as we'll have it compiled into the devices table, others will have to use that offset (yet maybe having the specific dpram required for another purpose).
Hope that clarifies my position...
--
Sincerely,
Vitaly
From: Kalle Pokki <hidden> Date: 2006-11-07 13:21:12
On Tue, 7 Nov 2006, Vitaly Bordug wrote:
Well, yes, but are you _sure_ pram_base will be the same across all the 82xx PQ2, that happen to have smc wired to Ethernet?
If not I am considering storing it in the platform_data is better approach.
Yes, pram_base is always 0x87fc for SMC1 and 0x88fc for SMC2. This is
for all PowerQUICC II families (8260, 8272, and 8280). I'm not sure how PQ2 Pro and PQ3 and handled, but I suspect they don't share these definitions.
Anyway, I'm only extending the already existing conventions to the platform device approach. These same decisions have already been made in the past and are used in the cpm_uart compat mode. It may be that Freescale someday releases a microcode patch that relocates the SMC parameter RAM, but even in this case it would be better to use the same approach with compat mode and platform device mode to avoid confusion.
I could have used the numerical address offsets in the resource definition, but I wanted to emphasize the fact that the offsets are already defined by the DPRAM memory allocator (this is a little hackish, yes) instead of hardware directly requiring these exact values.
This snippet is from cpm2.h:
/* Dual Port RAM addresses. The first 16K is available for almost
* any CPM use, so we put the BDs there. The first 128 bytes are
* used for SMC1 and SMC2 parameter RAM, so we start allocating
* BDs above that. All of this must change when we start
* downloading RAM microcode.
*/
#define CPM_DATAONLY_BASE ((uint)128)
My patch puts pram_base exactly here.
On Tue, 7 Nov 2006 15:21:00 +0200 (EET)
Kalle Pokki wrote:
On Tue, 7 Nov 2006, Vitaly Bordug wrote:
quoted
Well, yes, but are you _sure_ pram_base will be the same across all
the 82xx PQ2, that happen to have smc wired to Ethernet?
If not I am considering storing it in the platform_data is better
approach.
Yes, pram_base is always 0x87fc for SMC1 and 0x88fc for SMC2. This is
for all PowerQUICC II families (8260, 8272, and 8280). I'm not sure
how PQ2 Pro and PQ3 and handled, but I suspect they don't share these
definitions.
ok
Anyway, I'm only extending the already existing conventions to the
platform device approach. These same decisions have already been made
in the past and are used in the cpm_uart compat mode. It may be that
Freescale someday releases a microcode patch that relocates the SMC
parameter RAM, but even in this case it would be better to use the
same approach with compat mode and platform device mode to avoid
confusion.
I could have used the numerical address offsets in the resource
definition, but I wanted to emphasize the fact that the offsets are
already defined by the DPRAM memory allocator (this is a little
hackish, yes) instead of hardware directly requiring these exact
values.
Aha, I recall now. There was nearly exactly the same discussion in the past.
The recap was since ppc_platform_devices[] approach is not flexible enough, revisit issue from the
arch/powerpc POV.
This snippet is from cpm2.h:
/* Dual Port RAM addresses. The first 16K is available for
almost
* any CPM use, so we put the BDs there. The first 128
bytes are
* used for SMC1 and SMC2 parameter RAM, so we start
allocating
* BDs above that. All of this must change when we start
* downloading RAM microcode.
*/
#define CPM_DATAONLY_BASE ((uint)128)
My patch puts pram_base exactly here.
I know, the questionable thing was if there is enough "value" to add yet another platform device for that.
Since it improves current ppc being and sort of puts a note for powerpc port, I'm inclined to ACK.
Thanks,
-Vitaly
Thanks for the patch, but I guess it requires a bit of investigation first since
we cannot just drop ioremapped offset and get back to raw one that has been returned by
platform_get_....
We require ioremap stuff for powerpc approach, and I think we'd have to figure out why
ioremapped address did not satisfy smc (and I'll try on my 8xx).
Guessing we are speaking about arch/ppc/ domain upper...
Thanks for the patch, but I guess it requires a bit of investigation first since
we cannot just drop ioremapped offset and get back to raw one that has been returned by
platform_get_....
Ok, "Linux" writes/reads with the ioremapped Address in the Dualported
RAM we get with platform_get_...("pram"), thats Okay, but the CPM needs
the "real" Dualported RAM Address from this Area at offset SMCx_BASE,
which we get with platform_get_...("pram_base"), so it can find the SMC
Parameter RAM, he(CPM) didnt know anything about ioremapped addresses.
We require ioremap stuff for powerpc approach, and I think we'd have to figure out why
ioremapped address did not satisfy smc (and I'll try on my 8xx).
Hmm... you mean the following?:
r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pram_base");
if (r) {
+ pram_base = (u32)ioremap(r->start, r->end - r->start + 1);
out_be16((u16 *)pram_base, base & 0xffff);
+ iounmap(pram_base);
}
But thats not, what I correct with my patch, the error was, that
we wrote the ioremapped address pram in pram_base and not the real
address ... and with this ioremapped address, the CPM cannot do anything
(I think).
Guessing we are speaking about arch/ppc/ domain upper...
Yes.
thanks
Heiko
--
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Thanks for the patch, but I guess it requires a bit of
investigation first since we cannot just drop ioremapped offset and
get back to raw one that has been returned by platform_get_....
Ok, "Linux" writes/reads with the ioremapped Address in the Dualported
RAM we get with platform_get_...("pram"), thats Okay, but the CPM
needs the "real" Dualported RAM Address from this Area at offset
SMCx_BASE, which we get with platform_get_...("pram_base"), so it can
find the SMC Parameter RAM, he(CPM) didnt know anything about
ioremapped addresses.
quoted
We require ioremap stuff for powerpc approach, and I think we'd
have to figure out why ioremapped address did not satisfy smc (and
I'll try on my 8xx).
Hmm... you mean the following?:
r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "pram_base");
if (r) {
+ pram_base = (u32)ioremap(r->start, r->end - r->start + 1);
out_be16((u16 *)pram_base, base & 0xffff);
+ iounmap(pram_base);
}
But thats not, what I correct with my patch, the error was, that
we wrote the ioremapped address pram in pram_base and not the real
address ... and with this ioremapped address, the CPM cannot do
anything (I think).
That is what I am about - I think there is a bit of confusion around the upper. I mean, we'll have to investigate
what's happening in arch/powerpc since the SMC CPM works fine for me in aprch/powerpc with the ioremap stuff. Maybe it has to do something with io_block_map() and the ioremap - I'm not sure. Well it worths looking at it, if it will be unclear we can end up with #ifdef that areas to keep ppc/ happy at least.