On Thu, 2010-05-27 at 20:32 +0200, Peter Stuge wrote:
> mbertens wrote:
> > Here is the patch just for the pirq_routing() function. Its made
> > specific to the CONFIG_NORTHBRIDGE_INTEL_440BX if that is to generic
> > please replace by CONFIG_BOARD_NOKIA_IP530.
>
> _440BX is good, but there are still a few issues.
>
>
> > - added the correction for the i440BX by AND the link value with 0x5F
> > so that always the value is kept below 0x5F. That AND value should be
> > 0x03 i think because the link value cannot be greater than 3. But i'm
> > not sure about that, thats why i used the current solution.
>
> This is different from my code in your last patch. Why did it change?
> I'd love to know if there was a problem with using subtraction, in
> order to understand the work you and others are doing.
I changed it because i saw some implementations that have even higher
values in the PIRQ table (above 0x63) and i think that the link value
can never be above 3 anyway (0..3), so for that the the AND value should
even 0x03, to enforce that.
>
> My original if(link>0x5f) link-=0x5f; is a really crude hack that
> makes some wild assumptions in order to translate data at hand into
> data that is needed. I think this must not be coded into coreboot in
> this particular manner.
Correct but for now until a better solution is there i think we should
put it in, other i440BX users should have the same type of problem that
i've had.
>
> > +++ src/arch/i386/boot/pirq_routing.c (working copy)
> > - printk(BIOS_INFO, "Verifing copy of Interrupt Routing Table at
> > 0x%08x... ", addr);
> > + /**
> > + * fix made by Marc Bertens <mbert...@xs4all.nl>
> > + *
> > + * The compiler was putting out a warning that the 'addr' value
> > + * was of the unsigned int long type but the printk() was using '%08x'
> > + */
> > + printk(BIOS_INFO, "Verifing copy of Interrupt Routing Table at
> > 0x%08lx... ", addr);
>
> These comments are not ok. Please consider what the source code would
> look like if every commit that touched something small like a printk
> argument came with 6 lines of comments.
>
> Also, don't try to document who made changes to the code *within the
> code* because it will not work over time and more importantly because
> the source control system that we are using (subversion) keeps track
> of this for us! You sign-off, someone commits, the commit gets a
> revision number, SVN logs the commit message (which includes
> sign-off) and SVN can also find the revision number that is
> responsible for every line of source code in the project, so there is
> already perfect traceability both to the commiter (svn username) and
> to the original author(s) (Signed-off-by) without trying to store
> that in comments in the code.
Ok i did'nt know that, i was doing this kind of comments all the time in
my own projects and i was just trying to explain why i did the fix.
there.
>
> Finally, comments about changes that have been made go into the
> commit message and not into a comment in the source code.
>
> Documentation in comments is not at all forbidden, but I certainly
> think that it's important to not have *too many* comments in the code
> either. Some code certainly deserves to be commented, but not all,
> then it is probably so weird that it should be reworked a bit to be
> clear also without comments.
For understanding the code i think its quiet nice to have a lot of
comments, what the programmer is doing. I find it conviniant to put in
information about specicifations and that kind of thing. But if you
think its better to keep the comments short no problem i will take it
out.
>
>
> > @@ -121,7 +147,24 @@
> >
> > printk(BIOS_DEBUG, "INT: %c link: %x bitmap: %x ",
> > 'A' + j, link, bitmap);
> > -
> > +#if CONFIG_NORTHBRIDGE_INTEL_I440BX == 1
> > + /**
> > + * fix made by Marc Bertens <mbert...@xs4all.nl>
> > + *
> > + * this was done for the Northbridge i440BX, due to
> > the fact
> > + * that the values in the PIRQ table needs to be 60,
> > 61, 62
> > + * and 63. This was passed to me by Idwer Vollering
> > <vid...@gmail.com>
> > + * and Peter Stuge <pe...@stuge.se> helped with this
> > + * fix, so that the IRQ routing is done.
> > + */
> > + if (link > 0x5f) {
> > + /**
> > + * as if the maximum value can be 0x5F we should
> > + * AND it instead of substracting, my opinion.
> > + */
> > + link &= 0x5F;
> > + }
> > +#endif
>
> NAK for this. Even if it works, it's not the right way to solve this
> problem. I don't think this can be fixed without some more
> investigation and possibly central changes in coreboot. I don't think
> they will be very large changes, but still.
>
Even so that this is not the way to do it correctly, i like to put it in
for now. By doing it this way basically nobody should have a problem
with this addition. And i like to solve it the correct way, but i
started two months ago with de coreboot project (and i like it alot) and
overhauling the code for the i440BX is just a little to abitieus at the
moment. Certainly due to that there are many boards with this chipset.
>
> //Peter
>
And finally i like to keep working on this and fixing this at a later
time so that it fits all. I dont think its handy solution that i need to
keep a patch local to put this every time in when the code of the
pirq_routing object changes. And others cannot benefit from this and
they need to keep this patch locally too.
Let me know what you think, then i'll make a new patch with less
comments. And the part of adjusting the link value for the i440BX will
be a TODO comment.
Marc
--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
