Re: [Linux-galileo] ev64260 bi_rec patch

Re: [Linux-galileo] ev64260 bi_rec patch

[Mark A. Greer]

Michael Sokolov wrote:

> Mark A. Greer <mgreer@mvista.com> wrote:
>
> > I have other things to do so I won't be able to finish today.  In case
> > someone wants to work on this over the weekend, attached is a patch of
> > what I have so far.  It implements the example bi_rec's emailed before
> > for the ev64260 board.  Feel free to use it or do a better job.
>
> Looks OK except for 2 major problems:
>
> 1. You are using find_bootinfo() in EV-64260 platform_init to find the bi_recs.
> This is bad, as the magic location where find_bootinfo looks for the bi_recs is
> unavailable to bootloaders outside the linux source tree. Instead I have
> adopted the convention (which Tom Rini has accepted for the K2 port and
> implemented in the wrapper) to pass the physical address of bi_recs to the
> kernel in R3. You should use r3 + KERNELBASE in platform_init to find the
> bi_recs. This works with the wrapper just as well as find_bootinfo(), but is
> much nicer for external bootloaders.

It *is* in the kernel source (and not the bootloader).   Also, 15 of the *_setup.c
files in arch/ppc/platforms call find_bootinfo including ev64260_setup.c before I
modified it.  I think the R3 way of doing things is out of date now but maybe Tom
can elaborate.

> 2. You are also using find_bootinfo() in the gt64260_eth driver to find its
> bi_recs. In addition to the problems mentioned above, this has the extra
> problem in that find_bootinfo's magic location, or any other bootloader's
> location for that matter, has certainly been overwritten at this point. If you
> want to peek at bi_recs after platform_init(), you have to have setup.c or
> something copy them into memory that the kernel will retain.
>
> Because of problem #2 your code will most certainly not work as it is right
> now.

You may have a point here.  I thought the bi_rec's were left around but I need to
verify.

Mark


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Mark A. Greer]

"Mark A. Greer" wrote:

> Michael Sokolov wrote:
>
> > Because of problem #2 your code will most certainly not work as it is right
> > now.
>
> You may have a point here.  I thought the bi_rec's were left around but I need to
> verify.

I took a quick look and didn't see it being reused but I may have missed it.

Mark


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Mark A. Greer]

"Mark A. Greer" wrote:

> "Mark A. Greer" wrote:
>
> > Michael Sokolov wrote:
> >
> > > Because of problem #2 your code will most certainly not work as it is right
> > > now.
> >
> > You may have a point here.  I thought the bi_rec's were left around but I need to
> > verify.
>
> I took a quick look and didn't see it being reused but I may have missed it.

Okay, the bi_recs need to be copied.  Something for you to do over the weekend ;)


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Mark A. Greer <mgreer@mvista.com> wrote:

> I have other things to do so I won't be able to finish today.  In case
> someone wants to work on this over the weekend, attached is a patch of
> what I have so far.  It implements the example bi_rec's emailed before
> for the ev64260 board.  Feel free to use it or do a better job.

Looks OK except for 2 major problems:

1. You are using find_bootinfo() in EV-64260 platform_init to find the bi_recs.
This is bad, as the magic location where find_bootinfo looks for the bi_recs is
unavailable to bootloaders outside the linux source tree. Instead I have
adopted the convention (which Tom Rini has accepted for the K2 port and
implemented in the wrapper) to pass the physical address of bi_recs to the
kernel in R3. You should use r3 + KERNELBASE in platform_init to find the
bi_recs. This works with the wrapper just as well as find_bootinfo(), but is
much nicer for external bootloaders.

2. You are also using find_bootinfo() in the gt64260_eth driver to find its
bi_recs. In addition to the problems mentioned above, this has the extra
problem in that find_bootinfo's magic location, or any other bootloader's
location for that matter, has certainly been overwritten at this point. If you
want to peek at bi_recs after platform_init(), you have to have setup.c or
something copy them into memory that the kernel will retain.

Because of problem #2 your code will most certainly not work as it is right
now.

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Mark A. Greer <mgreer@mvista.com> wrote:

> It *is* in the kernel source (and not the bootloader).

I don't understand what you are talking about.

find_bootinfo looks for bi_recs in a magic location in memory. An external
bootloader has no way of knowing where in memory that is and thus can't put
bi_recs in a place where the kernel will find them. A port that uses
find_bootinfo() can't be booted by anything except the wrapper.

> Also, 15 of the *_setup.c
> files in arch/ppc/platforms call find_bootinfo

That's because no one has bothered so far to boot any of those 15 ports with
any bootloader other than the wrapper. And that's in turn because I haven't
worked on any of those ports. :-) See k2_setup.c and adir_setup.c for the
difference.

> including ev64260_setup.c before I
> modified it.

In my patch I changed it.

> I think the R3 way of doing things is out of date now but maybe Tom
> can elaborate.

It can't be out of date w.r.t. find_bootinfo(), as first everyone used the
latter then I screamed and screamed and screamed and got the K2 port changed.

As the wrapper now puts its bi_recs in the magic location *and* sets R3 to
point to them, either way will work for you, but the R3 way makes it work for
me too.

> You may have a point here.

This is not a may, it's certain.

> I thought the bi_rec's were left around but I need to
> verify.

They aren't.

> I took a quick look and didn't see it being reused but I may have missed it.

You won't see the kernel explicitly reusing the bi_recs memory because it
doesn't even know where that memory is! The bootloader can put the bi_recs
absolutely anywhere in memory. They can be parsed in platform_init because
Linux memory management hasn't started yet, but once it's started the memory
belongs to Linux and you can't expect some random bytes in the middle of memory
to be preserved for eternity.

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Mark A. Greer <mgreer@mvista.com> wrote:

> Okay, the bi_recs need to be copied.  Something for you to do over the weekend ;)

I don't know how to do that in a way acceptable to you. I only know how to do
things my way, which I've done in my patch which you've pocket-vetoed.

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Mark A. Greer]

Tom Rini wrote:

> OK.  The problem is that the original PPC location of the bi_recs was

Acutally, my problem was that I misread his respone.  I thought he was trying to
say the find_bootinfo was in the bootloader or something like that.

> So the always-correct call for platform_init() is:
> parse_bootinfo((struct bi_record *) (r3 + KERNELBASE));

Is there a reason we shouldn't just fix find_bootinfo then?

Mark


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Tom Rini]

On Fri, Mar 29, 2002 at 04:45:43PM -0800, Michael Sokolov wrote:
>
> Mark A. Greer <mgreer@mvista.com> wrote:
>
> > Okay, the bi_recs need to be copied.  Something for you to do over the weekend ;)
>
> I don't know how to do that in a way acceptable to you. I only know how to do
> things my way, which I've done in my patch which you've pocket-vetoed.
>

IIRC, your original patch did the parsing in parse_bootinfo() or so in
arch/ppc/kernel/setup.c which means each new driver bi_rec needs to
modify arch/ppc/kernel/setup.c, which isn't nice.  What we need is
something like how we save Motorola residual data or bd_info stuffs
(memcpy in platform_init).

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Tom Rini]

On Fri, Mar 29, 2002 at 05:24:11PM -0500, Mark A. Greer wrote:
>
> Michael Sokolov wrote:
>
> > Mark A. Greer <mgreer@mvista.com> wrote:
> >
> > > I have other things to do so I won't be able to finish today.  In case
> > > someone wants to work on this over the weekend, attached is a patch of
> > > what I have so far.  It implements the example bi_rec's emailed before
> > > for the ev64260 board.  Feel free to use it or do a better job.
> >
> > Looks OK except for 2 major problems:
> >
> > 1. You are using find_bootinfo() in EV-64260 platform_init to find the
> > bi_recs. This is bad, as the magic location where find_bootinfo looks for
> > the bi_recs is
> > unavailable to bootloaders outside the linux source tree. Instead I have
> > adopted the convention (which Tom Rini has accepted for the K2 port and
> > implemented in the wrapper) to pass the physical address of bi_recs to the
> > kernel in R3. You should use r3 + KERNELBASE in platform_init to find the
> > bi_recs. This works with the wrapper just as well as find_bootinfo(), but is
> > much nicer for external bootloaders.
>
> It *is* in the kernel source (and not the bootloader).   Also, 15 of
> the *_setup.c files in arch/ppc/platforms call find_bootinfo including
> ev64260_setup.c before I modified it.  I think the R3 way of doing things
> is out of date now but maybe Tom can elaborate.

OK.  The problem is that the original PPC location of the bi_recs was
kernel BSS + some.  This posed 2 problems.  First, a big enough kernel
could overwrite the bi_recs when they're here.  The second problem is
that anything out side of the kernel shouldn't have to know where the
bss is (in-kernel does because of linker magic).  So r3 is where the
bi_rec location can be.  This might be changed later (or we'll just pass
along the PReP residual data (not Motorola like I said in my last email,
whoops) location in a bi_rec. :)

So the always-correct call for platform_init() is:
parse_bootinfo((struct bi_record *) (r3 + KERNELBASE));

But, since Michael has only worked on a few boards, only a few boards do
that right now.

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Mark A. Greer]

Michael Sokolov wrote:

> > Is there a reason we shouldn't just fix find_bootinfo then?
>
> How are you going to fix it? How is it going to find out where in the vast
> expanses of system memory the bootloader has stuffed the bi_recs? How do you
> code ESP in C?

I guess you didn't get it.  What I meant was to change find_bootinfo to use
r3+KERNELBASE.

Anyway, I don't care.  I'm done with this conversation.


** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Tom Rini]

On Mon, Apr 01, 2002 at 10:09:37AM -0500, Mark A. Greer wrote:
> Tom Rini wrote:
>
> > OK.  The problem is that the original PPC location of the bi_recs was
>
> Acutally, my problem was that I misread his respone.  I thought he was trying to
> say the find_bootinfo was in the bootloader or something like that.
>
> > So the always-correct call for platform_init() is:
> > parse_bootinfo((struct bi_record *) (r3 + KERNELBASE));
>
> Is there a reason we shouldn't just fix find_bootinfo then?

The intent of parse_bootinfo taking a location was to allow it to be a
per-platform thing.  Eg pmac/chrp/prep don't use r3, and can't always
anyhow.  Everything else uses r3 just because it's where the
bootloader/wrapper caneasily put things.

--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Tom Rini <trini@kernel.crashing.org> wrote:

> So the always-correct call for platform_init() is:
> parse_bootinfo((struct bi_record *) (r3 + KERNELBASE));

Yes!

Mark A. Greer <mgreer@mvista.com> responded:

> Is there a reason we shouldn't just fix find_bootinfo then?

How are you going to fix it? How is it going to find out where in the vast
expanses of system memory the bootloader has stuffed the bi_recs? How do you
code ESP in C?

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Tom Rini <trini@kernel.crashing.org> wrote:

> IIRC, your original patch did the parsing in parse_bootinfo() or so in
> arch/ppc/kernel/setup.c which means each new driver bi_rec needs to
> modify arch/ppc/kernel/setup.c,

Yes.

> which isn't nice.

Agreed.

> What we need is
> something like how we save Motorola residual data or bd_info stuffs
> (memcpy in platform_init).

Both of those structures are fixed-length and are copied to static variables.
The bi_recs list has a variable length and can't be copied to a static variable
(unless we agree on a maximum size for it and make a static buffer of that
size, but someone other than me has to make that decision), and I don't know
how to make a dynamic memory allocation that early.

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Mark A. Greer <mgreer@mvista.com> wrote:

> Anyway, I don't care.  I'm done with this conversation.

Responding for the benefit of others on the list who aren't...

> I guess you didn't get it.  What I meant was to change find_bootinfo to use
> r3+KERNELBASE.

Then why do you need find_bootinfo at all? Just pass r3+KERNELBASE as the
argument to parse_bootinfo().

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Tom Rini]

On Mon, Apr 01, 2002 at 11:52:16AM -0800, Michael Sokolov wrote:
>
> Tom Rini <trini@kernel.crashing.org> wrote:
>
> > What we need is
> > something like how we save Motorola residual data or bd_info stuffs
> > (memcpy in platform_init).
>
> Both of those structures are fixed-length and are copied to static variables.
> The bi_recs list has a variable length and can't be copied to a static variable
> (unless we agree on a maximum size for it and make a static buffer of that
> size, but someone other than me has to make that decision), and I don't know
> how to make a dynamic memory allocation that early.

We do it on OF boxes to save the device-tree.
--
Tom Rini (TR1265)
http://gate.crashing.org/~trini/

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Paul Mackerras]

Michael Sokolov writes:

> The bi_recs list has a variable length and can't be copied to a static variable
> (unless we agree on a maximum size for it and make a static buffer of that
> size, but someone other than me has to make that decision), and I don't know
> how to make a dynamic memory allocation that early.

Use the memory pointed to by klimit and increment klimit by the amount
you use.  You should keep klimit aligned on a 4-byte boundary, I think.

Paul.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Paul Mackerras]

Mark A. Greer writes:

> > So the always-correct call for platform_init() is:
> > parse_bootinfo((struct bi_record *) (r3 + KERNELBASE));
>
> Is there a reason we shouldn't just fix find_bootinfo then?

The reason why I split the original parse_bootinfo into two pieces,
find_bootinfo and parse_bootinfo, was to allow for the case where the
bootloader passes in the address of the bi_recs.  In other words,
if you know where the bi_recs are, you don't need to call
find_bootinfo, just parse_bootinfo.

Paul.

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Michael Sokolov]

Paul Mackerras <paulus@samba.org> wrote:

> Use the memory pointed to by klimit and increment klimit by the amount
> you use.  You should keep klimit aligned on a 4-byte boundary, I think.

Yeah, I've already figured this out myself by looking at what you do for OF.

I'm working on one really big changeset right now that'll add a
CONFIG_GENERIC_PPC32 configuration, have presistent and searchable bi_recs, a
new and much cleaner GT-64260 implementation, etc. I'll push it into
linuxppc_2_4_alt when it's ready to see the light of day (hopefully tomorrow or
Wednesday).

MS

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/

Re: [Linux-galileo] ev64260 bi_rec patch

[Troy Benjegerdes]

On Mon, Apr 01, 2002 at 07:35:50PM -0800, Michael Sokolov wrote:
> Paul Mackerras <paulus@samba.org> wrote:
>
> > Use the memory pointed to by klimit and increment klimit by the amount
> > you use.  You should keep klimit aligned on a 4-byte boundary, I think.
>
> Yeah, I've already figured this out myself by looking at what you do for OF.
>
> I'm working on one really big changeset right now that'll add a
> CONFIG_GENERIC_PPC32 configuration, have presistent and searchable bi_recs, a
> new and much cleaner GT-64260 implementation, etc. I'll push it into
> linuxppc_2_4_alt when it's ready to see the light of day (hopefully tomorrow or
> Wednesday).

Would you mind detailing exactly what's in your 'much cleaner' gt64260
implementation? Like, do you have an SMP-safe gt ethernet driver
perchance? ;)

--
Troy Benjegerdes | master of mispeeling | 'da hozer' |  hozer@drgw.net
-----"If this message isn't misspelled, I didn't write it" -- Me -----
"Why do musicians compose symphonies and poets write poems? They do it
because life wouldn't have any meaning for them if they didn't. That's
why I draw cartoons. It's my life." -- Charles Schulz

** Sent via the linuxppc-dev mail list. See http://lists.linuxppc.org/