Wrong looking statement in cpm_common.c

Wrong looking statement in cpm_common.c

[Benjamin Herrenschmidt]

Hi Scott !

There's this pearl in cpm_common.c :

void __init udbg_init_cpm(void)
{
	if (cpm_udbg_txdesc) {
#ifdef CONFIG_CPM2
		setbat(1, 0xf0000000, 0xf0000000, 1024*1024, PAGE_KERNEL_NCG);
#endif
		udbg_putc = udbg_putc_cpm;
	}
}

Now, last I looked, 0xf0000000 (virtual) lands about right in the middle
of the vmalloc space... so unless there's code somewhere that I missed
that reserves that region of virtual space for use by that crap above,
I think somebody is in trouble :-)

Additionally, that's the last user of setbat that I can find outside
of the linear mapping setup proper, so scott, once you've fixed that
I'll happily make setbat static once for all. We -can- still provide
a facility for using BATs for early ioremap's but that should be done
properly, not by whacking setbat with random hard wired virtual
addresses.

Cheers,
Ben.

Re: Wrong looking statement in cpm_common.c

[Scott Wood]

On Mon, May 25, 2009 at 02:00:33PM +1000, Benjamin Herrenschmidt wrote:
> Hi Scott !
> 
> There's this pearl in cpm_common.c :
> 
> void __init udbg_init_cpm(void)
> {
> 	if (cpm_udbg_txdesc) {
> #ifdef CONFIG_CPM2
> 		setbat(1, 0xf0000000, 0xf0000000, 1024*1024, PAGE_KERNEL_NCG);
> #endif
> 		udbg_putc = udbg_putc_cpm;
> 	}
> }
> 
> Now, last I looked, 0xf0000000 (virtual) lands about right in the middle
> of the vmalloc space... so unless there's code somewhere that I missed
> that reserves that region of virtual space for use by that crap above,
> I think somebody is in trouble :-)

:-(

> Additionally, that's the last user of setbat that I can find outside
> of the linear mapping setup proper, so scott, once you've fixed that
> I'll happily make setbat static once for all. We -can- still provide
> a facility for using BATs for early ioremap's but that should be done
> properly, not by whacking setbat with random hard wired virtual
> addresses.

Any suggestions for how to do it properly?

-Scott

Re: Wrong looking statement in cpm_common.c

[Benjamin Herrenschmidt]

On Tue, 2009-05-26 at 12:56 -0500, Scott Wood wrote:
> 
> Any suggestions for how to do it properly?

ioremap ? :-)

Ben.

Re: Wrong looking statement in cpm_common.c

[Scott Wood]

Benjamin Herrenschmidt wrote:
> On Tue, 2009-05-26 at 12:56 -0500, Scott Wood wrote:
>> Any suggestions for how to do it properly?
> 
> ioremap ? :-)

Does ioremap work that early?

-Scott

Re: Wrong looking statement in cpm_common.c

[Benjamin Herrenschmidt]

On Tue, 2009-05-26 at 17:15 -0500, Scott Wood wrote:
> Benjamin Herrenschmidt wrote:
> > On Tue, 2009-05-26 at 12:56 -0500, Scott Wood wrote:
> >> Any suggestions for how to do it properly?
> > 
> > ioremap ? :-)
> 
> Does ioremap work that early?

It works very early on ppc32 but maybe not -that- early... It works from
start_kernel on. You might be able to do the ioremaps in machine_init
(provided LMBs are up) but they won't be "effective" until MMU_init
returns, but then, neither will setbat anyway.

However, since that's the only user of setbat I can still find and we
want to replace that with something saner, I'd rather kill it.

Ben.

Re: Wrong looking statement in cpm_common.c

[Scott Wood]

Benjamin Herrenschmidt wrote:
> On Tue, 2009-05-26 at 17:15 -0500, Scott Wood wrote:
>> Benjamin Herrenschmidt wrote:
>>> On Tue, 2009-05-26 at 12:56 -0500, Scott Wood wrote:
>>>> Any suggestions for how to do it properly?
>>> ioremap ? :-)
>> Does ioremap work that early?
> 
> It works very early on ppc32 but maybe not -that- early... It works from
> start_kernel on. You might be able to do the ioremaps in machine_init
> (provided LMBs are up) but they won't be "effective" until MMU_init
> returns, but then, neither will setbat anyway.

Before the setbat takes effect, there is a BAT established by 
setup_cpm_bat in head_32.S.

How about something similar to btext_unmap to tell cpm2 udbg that the 
mapping is gone, and to do ioremap at that point (or perhaps it would be 
better to wait until the next print after unmap is called, which should 
be after load_up_mmu assuming nothing crashes in the assembly code)?

-Scott