Re: Commit 1.20 ofw_machdep.c breaks PowerMac G5 in NetBSD HEAD

[Michael <macallan%netbsd.org@localhost>]

Hello,

On Fri, 05 Apr 2013 19:34:27 +0200
Phileas Fogg <phileas-fogg%mail.ru@localhost> wrote:

> On 04/04/2013 07:13 PM, Michael wrote:
> > Hello,
> >
> > On Thu, 04 Apr 2013 16:58:11 +0200
> > Phileas Fogg <phileas-fogg%mail.ru@localhost> wrote:
> >
> >> On 04/04/2013 03:31 AM, Michael wrote:
> >>
> >>> What's in your /memory properties? Especially "available" and "reg"?
> >>> Also, what's in / "#size-cells" and "#address-cells" ?
> >
> >> #address-cells          00000002
> >> #size-cells             00000001
> > Ok, 64bit addresses, 32bit sizes - just like mine.
> >
> >> reg                     00000000 00000000  00000000
> >>                           00000000 00000000  00000000
> >>                           00000000 00000000  40000000
> >>                           00000000 00000000  00000000
> >>                           00000000 00000000  00000000
> >>                           00000000 00000000  00000000
> >>                           00000000 40000000  40000000
> >>                           00000001 00000000  40000000
> >
> > Hmm, a few zero entries but they should be ignored.
> >
> >> available               pseudo=00003000 7f5ed000
> > ... and this should not trip the code below...
> >
> >>> Finally, does it work if you remove this:
> >>>           if (((regcnt >> 2) % (acells + scells)) != 0) {
> >>>                   aprint_normal("messed up 'available' property 
> >>> detected\n");
> >>>                   acells = 1;
> >>>           }
> >>> ... and just set acells to 1 there?
> >>>
> >>
> >> It didn't help.
> >
> > Yeah, it would screw up if "available" had a size of a multiple of 3 which 
> > yours doesn't ( and neither does mine ).
> > There should be some debug output, like listing the memory regions found. 
> > Does that look sane?
> >
> > Wait a minute.
> >
> > Does this help?
> >
> > Index: powerpc.h
> > ===================================================================
> > RCS file: /cvsroot/src/sys/arch/macppc/include/powerpc.h,v
> > retrieving revision 1.7
> > diff -u -w -r1.7 powerpc.h
> > --- powerpc.h   14 Mar 2009 14:46:02 -0000      1.7
> > +++ powerpc.h   4 Apr 2013 17:12:12 -0000
> > @@ -34,7 +34,7 @@
> >   #define        _MACHINE_POWERPC_H_
> >
> >   struct mem_region {
> > -#if defined (PMAC_G5)
> > +#if defined (PMAC_G5___)
> >          paddr_t start_hi;
> >   #endif
> >          paddr_t start;
> >
> > start_hi isn't used anywhere as fas as I can tell, and shouldn't be
> > used in a 32bit kernel anyway. And on LP64 paddr_t would be 64bit so no
> > need for an additional field there.
> >
> > have fun
> > Michael
> >
> 
> I found the problem.
> Any kind of printf in mem_regions causes this problem.
> I removed all DEBUG printfs and aprints and it works now.
> But no idea why, i'm using rascons as the boot cons.

This is weird, but not unheard of. I'll change that then.

> One more question.
> 
> for (i=0, cnt=0; i <= numregs; i++) {
> 
> shouldn't it be
> 
> for (i=0, cnt=0; i < numregs; i++) {

Good catch, you're right.

have fun
Michael