Re: Re: EPIC Vs OpenPIC (RFC)

Re: Re: EPIC Vs OpenPIC (RFC)

[Sarnath Kannan]

> 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.
>
  Moving the cascading part away from open_pic is
a good one. I dont know, why on earth it assumes
that only IRQ0 would be cascaded. Its really
very innocent.

> 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 ;-)

             Good Work !

> 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.

Do the ISA drivers assume the
vector number starts from 0 and register IRQs ?
Even if it is so, its fine. See below.


> 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.

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


Good Luck

Sarnath


** 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/

Re: Re: EPIC Vs OpenPIC (RFC)

[Sarnath Kannan]

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.

Sarnath

On Sat, 03 Nov 2001 Dag Nygren wrote :
>
> > 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 op
         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/