Re: Re: EPIC Vs OpenPIC Vs MontaVista

Re: Re: EPIC Vs OpenPIC Vs MontaVista

[Sarnath Kannan]

> Hi,
>
> Funny you should write about this.!
> I am just trying to fix it in a reasonable way
> for my EPIC-board.

:-) Oh really ! Good. Since I am new to the group
I dint know its an existing bug ! but how can
one tolerate populating reserved areas ? That
sounds very dangerous to me !

> I would like to suggest a global variable ie.
> OpenPIC_RegOffset
> which would be inited to 0 and could be changed to
> 16 for a MPC107 (EPIC).
> This wouldn't (shouldn't) break anything existing
> and will enable the port of a EPIC-board to use
> all the generic openpic routines.
>
 Yeah there are plenty ways u could solve. The
best one is padding 0x200 bytes to the "OpenPIC"
structure before "Source" in case of CONFIG_SANDPOINT.
That should end the story !

> Even if I don't like touching "generic" code, I even
> more dislike code-duplication
>
> I could submit the patches, if your are interested,
> they are noe too big, and a review from the masses
> should do the code good.
>
> BRGDS
 I think I can fix it myself. Thanks for the offer.
Good Luck !

Sarnath, the NATH ;-)


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

Re: EPIC Vs OpenPIC Vs MontaVista

[Mark A. Greer]

Sarnath, Dag, Andrew,

What I think you guys are discovering is an artifact of the inadequate
interface to openpic_init()--its wider than just a 8259/107 problem.  This has
been discussed before and some ideas kick around.

Since you guys seem to have the time & motivation, why don't you go back &
check out the mail archives for that/those discussion(s).  If you like what
was talked about there, go with it or come up with a better solution and post
it here for everyone to comment on.  It would be really nice to have a more
flexible interface to openpic_init(), et. al.

Mark


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

Re: EPIC Vs OpenPIC (RFC)

[Dag Nygren]

> Sarnath, Dag, Andrew,
>
> What I think you guys are discovering is an artifact of the inadequate
> interface to openpic_init()--its wider than just a 8259/107 problem.  This has
> been discussed before and some ideas kick around.
>
> Since you guys seem to have the time & motivation, why don't you go back &
> check out the mail archives for that/those discussion(s).  If you like what
> was talked about there, go with it or come up with a better solution and post
> it here for everyone to comment on.  It would be really nice to have a more
> flexible interface to openpic_init(), et. al.

Would something like this be sufficient/good as interface ?:



typedef enum irq_level_en
    {OP_IRQ_HIGH=1,
     OP_IRQ_LOW=0}
irq_level_type;

typedef enum irq_enable_en
    {OP_IRQ_ON=1,
    OP_IRQ_OFF=0}
irq_enable_type;

typedef struct openpic_irq_def_str {
    u_int           PICIrq;
    u_int           LinuxIrq;
    u_char          Priority;
    irq_level_type  IrqLevel;
    irq_enable_type IrqEnable;
} openpic_irq_def;

typedef struct openpic_def_str {
    struct OpenPIC  *OpenPIC_Addr;
    int             main_pic;
    u_char          RegOffset; /* 0 for "standard", 16 for MPC107 */
    char            *chrp_ack;
    int             programmer_switch_irq; /* Not really needed with the
Priority/IRQ poss. */
    openpic_irq_def *IRQdef;
} openpic_def;

extern void openpic_init(*openpic_def);


If so, I could try to implement it in the current open_pic.c

BRGDS

Dag


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

Re: EPIC Vs OpenPIC (RFC)

[Mark A. Greer]

Dag Nygren wrote:

> > Sarnath, Dag, Andrew,
> >
> > What I think you guys are discovering is an artifact of the inadequate
> > interface to openpic_init()--its wider than just a 8259/107 problem.  This has
> > been discussed before and some ideas kick around.
> >
> > Since you guys seem to have the time & motivation, why don't you go back &
> > check out the mail archives for that/those discussion(s).  If you like what
> > was talked about there, go with it or come up with a better solution and post
> > it here for everyone to comment on.  It would be really nice to have a more
> > flexible interface to openpic_init(), et. al.
>
> Would something like this be sufficient/good as interface ?:
>
> typedef enum irq_level_en
>     {OP_IRQ_HIGH=1,
>      OP_IRQ_LOW=0}
> irq_level_type;
>
> typedef enum irq_enable_en
>     {OP_IRQ_ON=1,
>     OP_IRQ_OFF=0}
> irq_enable_type;
>
> typedef struct openpic_irq_def_str {
>     u_int           PICIrq;
>     u_int           LinuxIrq;
>     u_char          Priority;
>     irq_level_type  IrqLevel;
>     irq_enable_type IrqEnable;
> } openpic_irq_def;
>
> typedef struct openpic_def_str {
>     struct OpenPIC  *OpenPIC_Addr;
>     int             main_pic;
>     u_char          RegOffset; /* 0 for "standard", 16 for MPC107 */
>     char            *chrp_ack;
>     int             programmer_switch_irq; /* Not really needed with the
> Priority/IRQ poss. */
>     openpic_irq_def *IRQdef;
> } openpic_def;
>
> extern void openpic_init(*openpic_def);
>
> If so, I could try to implement it in the current open_pic.c

This is along the lines that I was thinking except add a struct member to specify
whether edge or level sensitive--i.e., separate members for hi/lo and edge/level.

What exactly is the "IrqEnable" for?  Is this so you can set up the IRQ but not
enable it?  Then the driver can enable it later and its set up correctly (e.g.,
I2C)??

Doesn't the new struct eliminate the need for "RegOffset"?  And as you noted,
"programmer_switch_irq" probably isn't needed either.  Right?

If you're up to it, how about implementing, testing, then posting a patch for
everyone to check out??

Mark


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

Re: EPIC Vs OpenPIC (RFC)

[Dag Nygren]

> Mark A. Greer wrote:
>
>
> This is along the lines that I was thinking except add a struct member to specify
> whether edge or level sensitive--i.e., separate members for hi/lo and edge/level

Actually this evolved after I posted it :-), after I started
to modify open_pic.c

> What exactly is the "IrqEnable" for?  Is this so you can set up the IRQ but not
> enable it?  Then the driver can enable it later and its set up correctly (e.g.,
> I2C)??

Not really, it was more or less a mistake in the first draft. Would there be a
need
for that ?

> Doesn't the new struct eliminate the need for "RegOffset"?

Have to think about that when implementing the stuff.

>  And as you noted,
> "programmer_switch_irq" probably isn't needed either.  Right?

Yep, and I also came up with another scheme for the cascading as the
special handling for chrp is actually.....drum roll.... a cascade!
And it would remove the i8259 refs from the openpic stuff.

> If you're up to it, how about implementing, testing, then posting a patch for
> everyone to check out??

I'm trying, but I am a bit scared of destroying too much of the open_pic
code...

Here is the newest suggestion for that interface:

typedef enum irq_polarity_en {
    OP_IRQ_POS = 1,
    OP_IRQ_NEG = 0
} irq_polarity_type;

typedef enum irq_sense_en {
    OP_IRQ_LEVEL = 1,
    OP_IRQ_EDGE  = 0
} irq_sense_type;

typedef struct openpic_irq_def_str {
    u_int               PICIrq;
    u_int               Vector;
    u_char              Priority;
    irq_sense_type      IrqSense;
    irq_polarity_type   IrqPolarity;
    irq_polarity_type   IrqPolarity;
    int                 (*CascadeHandler)();
} openpic_irq_def;

typedef struct openpic_def_str {
    struct OpenPIC  *OpenPIC_Addr;
    int             main_pic;
    u_char          RegOffset; /* 0 for "standard", 16 for MPC107 */
    int             programmer_switch_irq;
    openpic_irq_def *IRQdef;    /* The lovest vector first in the table !!! */
} openpic_def;

Comments?


Dag


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

Re: EPIC Vs OpenPIC (RFC)

[Dag Nygren]

>
> Somehow I feel that Patching or changing
> the openPIC code would make it shabby.
>  Why should we change open_pic code,
> just because that EPIC is non-conforming ?
> The best is to rewrite open_pic code for
> EPIC. ( ofcourse, we can reuse code. )
>  I dont find anything wrong with open_pic
> interface. If people dont follow standards,
> we dont make standards comply with them. We
> make them comply with standards.

I dont agree,
we would end up duplicating 90% of the open_pic
code, and this is also an excellent opportunity to
move out the cascade-handling from the open_pic stuff.

Just stay tuned, I already did the rewriting of the
open_pic.c, just need to get to work on Monday,
testing the result.
I will then submit this as a suggestion.
It actually looks more clean that the original code,
at last to my eyes ;-)
And it really makes for an even more simple
xxx_setup.c for the different ports.

At the same time I would like to suggest an inclusion
of the registration of the i8259 interrupts to i8259_init().
At the momenty everyone of the drivers are doing exactly
the same thing:
for ( vec = 0 ; vec < NUM_8259_INTERRUPTS  ; vec++ )
        irq_desc[vec].handler = &i8259_pic;

Moving this to i8259_init() and giving the routine a
parameter startvec (even if everyone is starting from 0
at the moment) would get rid of even more code from
the machine-specific stuff.

BRGDS

Dag


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

Re: EPIC Vs OpenPIC (RFC)

[Dag Nygren]

>
>
> > And it really makes for an even more simple
> > xxx_setup.c for the different ports.
> >
> > At the same time I would like to suggest an inclusion
> > of the registration of the i8259 interrupts to
> > i8259_init().
> > At the momenty everyone of the drivers are doing exactly
> > the same thing:
> > for ( vec = 0 ; vec < NUM_8259_INTERRUPTS  ; vec++ )
> >         irq_desc[vec].handler = &i8259_pic;
> >
>   Why should all drivers populate irq_desc ??
> I dont get this. It is done once in life time,
> right ? That too, even before driver's init
> routine come into picture.

Sorry for talking about drivers, did get a little
confusing, what I meant is that every
port (see the xxx_setup.c stuff) is doing
the same thing.

> > Moving this to i8259_init() and giving the routine a
> > parameter startvec (even if everyone is starting from 0
> > at the moment) would get rid of even more code from
> > the machine-specific stuff.
> >
>    The best thing is allow the 8259 to retain
> its vector space from 0-15. Program EPIC
> to generate interrupts from 16-40. ( By
> passing "offset" value of 16 to openpic_init)
> That should clear things up and seperate the
> vector spaces of EPIC and PIC.

I am not sure it the i8259 (or similar) will always
have vectors 0-15, what if we have two cascades ?
I think we need to make sure not limiting this
too much by giving the i8259() routing the extra
argument for the first vector.

>    May I know whats the semantics of the
> "regOffset" ?

Well, that was from the first draft, when I wasn't
sure if we still needed it.

Now the interface looks like this:

typedef enum irq_polarity_en {
    OP_IRQ_POS = 1,
    OP_IRQ_NEG = 0
} irq_polarity_type;

typedef enum irq_sense_en {
    OP_IRQ_LEVEL = 1,
    OP_IRQ_EDGE  = 0
} irq_sense_type;

typedef struct openpic_irq_def_str {
    u_int               PICIrq;
    u_int               Vector;
    u_char              Priority;
    irq_sense_type      IrqSense;
    irq_polarity_type   IrqPolarity;
    /* This can be used for cascade, but should work well for the chrp
specials too */
    int                 (*CascadeAckHandler)(int);
} openpic_irq_def;

typedef struct openpic_def_str {
    volatile struct OpenPIC *OpenPIC_Addr;
    int             slave_pic;
    openpic_irq_def *IRQdef;    /* The lovest vector first in the table !!! */
} openpic_def;

> Good Luck

Thanks

Dag


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

Re: EPIC Vs OpenPIC (RFC)

[Dag Nygren]

BTW,

here is the corresponding routine for the xxxx_setup.c
in my port:

    /* Using MPC107(EPIC) means that the LinuxVector numbers start from 16
     * as 0-15 is reserved for the possible cascade
     * Remember that the LinuxVector of the first entry is used as an offset
     * for the Timer and IPI vectors */

static openpic_irq_def vg4_openpic_irq_defs[] __initdata = {
    /* PICirq, LinuxVector, Priority, Level/Edge, Polarity, CascadeAck */
    {  0, 16, 8, OP_IRQ_LEVEL, OP_IRQ_POS, &i8259_irq} ,    /* 8259 cascade */
    {  1, 17, 8, OP_IRQ_LEVEL, OP_IRQ_NEG, NULL      } ,    /* INT_PCI_INTA */
    {  2, 18, 8, OP_IRQ_LEVEL, OP_IRQ_NEG, NULL      } ,    /* INT_PCI_INTB */
    {  3, 19, 8, OP_IRQ_LEVEL, OP_IRQ_NEG, NULL      } ,    /* INT_PCI_INTC */
    {  4, 20, 8, OP_IRQ_LEVEL, OP_IRQ_NEG, NULL      } ,    /* INT_PCI_INTD */
    { -1, -1, 8, OP_IRQ_LEVEL, OP_IRQ_NEG, NULL      }      /* End of table
marker */
};

ALLOC_OPENPIC_DEF(vg4_openpic_def);

static void __init
vg4_init_IRQ(void)
{
    int vec;

    vg4_openpic_def.IRQdef    = vg4_openpic_irq_defs;

    /* The int:s behind the OpenPIC in MPC107 */

    openpic_init(&vg4_openpic_def);

    /* These are the ints (0-15) behind the 8259 in the ALI chip */
    for ( vec = 0 ; vec < NUM_8259_INTERRUPTS  ; vec++ )
        irq_desc[vec].handler = &i8259_pic;

    i8259_init();
}


BRGDS

Dag


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