Re: [PATCH] Incorrect segment 0 initialization for PMAC G5

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

Hello,

On Tue, 02 Apr 2013 20:59:23 +0200
Phileas Fogg <phileas-fogg%mail.ru@localhost> wrote:

> On 04/02/2013 09:36 PM, Michael wrote:
> > Hello,
> >
> > On Tue, 02 Apr 2013 20:11:32 +0200
> > Phileas Fogg <phileas-fogg%mail.ru@localhost> wrote:
> >
> >> On 04/02/2013 08:59 PM, Michael wrote:
> >>> Hello,
> >>>
> >>> On Tue, 02 Apr 2013 19:33:16 +0200
> >>> Phileas Fogg <phileas-fogg%mail.ru@localhost> wrote:
> >>>
> >>>> we need to extend/rework the OpenPIC interface because
> >>>> currently the implementation assumes the little-endian
> >>>> architecture which is not true on PowerMAC G5 11,2.
> >>>
> >>> I'm fairly sure there are at least some ofppc boxes with the same problem.
> >>>
> >>>> And if we want to reuse some parts of the current OpenPIC
> >>>> implementation then we need to extend it and support
> >>>> the big-endian architecture too.
> >>>>
> >>>> I thought of a global variable "openpic_be" which can be set
> >>>> to 0 or 1. And "openpic_read" and "openpic_write" methods
> >>>> will check it.
> >>>>
> >>>> And then rename the 2nd parameter of "setup_openpic" to "flags"
> >>>> and define 2 flags:
> >>>>
> >>>> OPENPIC_FLAG_PASSTHROUGH and OPENPIC_FLAG_BIGENDIAN
> >>>>
> >>>> What do you think ?
> >>>
> >>> I wonder how likely it is that we'll ever need both in the same kernel
> >>> image. If that likelihood is zero then we'd be better off with a macro
> >>> and a build time option. Do you think 32bit OEA and 64bit bridge mode
> >>> will ever be supported by the same kernel?
> >>>
> >>> Otherwise, I'd avoid checking flags in openpic_read() and openpic_write(),
> >>> instead make them function pointers, have variants with and
> >>> without byte order twiddling, and decide which one to use in
> >>> setup_openpic().
> >
> >> Yeah, you are right, checking the variable every time an IRQ is acked is
> >> not a good idea :) But you can't know till run time if it's little
> >> or big-endian architecture because we get this info from OFW.
> >> Linux and FreeBSD do it at run-time on PowerMac.
> >
> > I'm sure they've got their reasons ;)
> > What I was thinking about is this - all 32bit PowerMacs that use
> > OpenPICs use the little endian variant. All(?) G5 PowerMacs use the big
> > endian variant. Kernels that support only one of those don't need the
> > distinction, and introducing additional overhead, even if it's tiny, on
> > comparably slow hardware is something we should ( in my opinion at
> > least ) try to avoid.
> >
> > Since the OpenPIC code is used on other architectures as well, where
> > the logic above doesn't necessarily apply, it's probably best to
> > decide at runtime and I'll add an optional fast path for 32bit Macs,
> > which avoids the indirection, later on.

> Dumb question, how do we add an indirection to openpic_write and 
> openpic_read ?
> They do not accept any kind of this pointer or context.
> Change interface or use global function pointers which can be set in 
> setup_openpic ?

Ugh, I forgot about that - you're right, openpic_eoi() and friends call
openpic_write() and if that's using indirection we need context there.
On the other hand, there are only four functions in openpic.h, all extremely 
short.
Hmm, there are two more openpic variants which use varying amounts of the 
common code, they all would need to be changed although these changes would be 
more or less mechanical. I'd just add openpic_read() and _write() methods to 
struct openpic_ops, add indirection-aware versions of openpic_eoi(), 
openpic_readirq() and openpic_set_priority(), and copy/paste that over 
pic_mpcsoc.c, pic_distopenpic.c, pic_openpic.c etc., that way openpic.h can 
stay as it is ( I'm fairly sure there's nothing outside powerpc/pic that still 
uses it but I'm not 100% sure )
I'll do it in the next few hours ( whenever real life lets me ) unless you want 
to beat me to it.

have fun
Michael