On Wednesday 10 of April 2013 15:45:48 Heiko Stübner wrote:
> Am Mittwoch, 10. April 2013, 14:31:29 schrieb Tomasz Figa:
> > On Wednesday 10 of April 2013 14:20:22 Heiko Stübner wrote:
> > > Hi Tomasz,
> > >
> > > thanks for your comments, more inline.
> > >
> > > Am Mittwoch, 10. April 2013, 12:36:39 schrieb Tomasz Figa:
> > > > Hi Heiko,
> > > >
> > > > Basically looks good to me, but please see my inline comments
> > > > about
> > > > handling of EINT0-3.
> > > >
> > > > On Wednesday 10 of April 2013 01:35:12 Heiko Stübner wrote:
> > > > > The s3c24xx pins follow a similar pattern as the other Samsung
> > > > > SoCs
> > > > > and
> > > > > can therefore reuse the already introduced infrastructure.
> > >
> > > [...]
> > >
> > > > > +struct s3c24xx_eint_data {
> > > > > + struct samsung_pinctrl_drv_data *drvdata;
> > > > > + struct irq_domain *domains[NUM_EINT];
> > > > > + int parents[NUM_EINT_IRQ];
> > > > > +};
> > > > > +
> > > > > +struct s3c24xx_eint_domain_data {
> > > > > + struct samsung_pin_bank *bank;
> > > > > + struct s3c24xx_eint_data *eint_data;
> > > >
> > > > What about:
> > > >
> > > > + bool eint0_3_parent_only;
> > > >
> > > > (or whatever name would be more appropriate), which would store
> > > > the
> > > > information about the s3c24xx-specific quirk in 24xx-specific data
> > > > structure, without the need to add another field to the generic
> > > > samsung_pinctrl_drv_data structure?
> > > >
> > > > See my further comments on how I would see using this field in
> > > > interrupt handling code.
> > >
> > > ok, sounds good, especially gathering the type from the wakeup-int
> > > property
> > >
> > > > > +};
> > >
> > > [...]
> > >
> > > > Now I would split the following 3 functions into two sets of 3
> > > > functions, one set for s3c2412 and other for remaining SoCs and
> > > > make
> > > > separate EINT0-3 IRQ chips for both cases.
> > >
> > > Not doing the decision every time, might bring some very slight
> > > speed
> > > improvements, so is probably the right way to go.
> > >
> > > > > +
> > > > > +static void s3c24xx_eint0_3_ack(struct irq_data *data)
> > > > > +{
> > > > > + struct samsung_pin_bank *bank =
> > > > > irq_data_get_irq_chip_data(data);
> > > > > + struct samsung_pinctrl_drv_data *d = bank->drvdata;
> > > > > + struct s3c24xx_eint_domain_data *ddata = bank->irq_domain-
> > > > >
> > > > >host_data;
> > > > >
> > > > > + struct s3c24xx_eint_data *eint_data = ddata->eint_data;
> > > > > + int parent_irq = eint_data->parents[data->hwirq];
> > > > > + struct irq_chip *parent_chip = irq_get_chip(parent_irq);
> > > > > +
> > > > > + if (d->ctrl->type == S3C2412) {
> > > > > + unsigned long bitval = 1UL << data->hwirq;
> > > > > + writel(bitval, d->virt_base + EINTPEND_REG);
> > > > > + }
> > > > > +
> > > > > + if (parent_chip->irq_ack)
> > > > > + parent_chip-
>irq_ack(irq_get_irq_data(parent_irq));
> > > >
> > > > Btw. Is this parent level acking really needed here?
> > >
> > > Depends. If using chained_irq_* of course not, but if the
> > > irq-handler
> > > should stay in charge of when to ack it might be better this way.
> > >
> > > Generic s3c24xx SoCs need acking in the main controller only, while
> > > s3c2412 needs acking in both the main controller and eintpend.
> > >
> > > > > +}
> > >
> > > [...]
> > >
> > > > > +static void s3c24xx_demux_eint0_3(unsigned int irq, struct
> > > > > irq_desc
> > > > > *desc) +{
> > > > > + struct irq_data *data = irq_desc_get_irq_data(desc);
> > > > > + struct s3c24xx_eint_data *eint_data =
> > > > > irq_get_handler_data(irq);
> > > > > + unsigned int virq;
> > > > > +
> > > >
> > > > Instead of acking the interrupt at parent chip from ack callback
> > > > of
> > > > EINT0_3 chip, I would rather use chained_irq_enter() here...
> > > >
> > > > > + /* the first 4 eints have a simple 1 to 1 mapping */
> > > > > + virq = irq_linear_revmap(eint_data->domains[data->hwirq],
> > > > > data->hwirq); + /* Something must be really wrong if an
unmapped
> > > >
> > > > EINT
> > > >
> > > > > + * was unmasked...
> > > > > + */
> > > > > + BUG_ON(!virq);
> > > > > +
> > > > > + generic_handle_irq(virq);
> > > >
> > > > ...and chained_irq_exit() here.
> > >
> > > If I understand it correctly, the way chained_irq_* works it would
> > > limit the eints to a level style handling. With the way it's
> > > currently the whole determination of when to ack,mask and unmask is
> > > completely for the real handler (edge or level) to decide, as the
> > > original interrupt gets completely forwarded into the irq-domain
> > > without further
> > > constraints.
> > >
> > > So, after the change on regular s3c24xx SoCs when the real irq
> > > handler
> > > wants to ack the irq, it would be a no-op, as it would already have
> > > been acked by chained_irq_enter.
> > >
> > > Masking might be even more interesting. Currently control is
> > > transfered
> > > completely to the pinctrl irq-domain, which then controls the
> > > masking of the interrupt thru the parent-calls - on regular s3c24xx
> > > the masking of these is also only done in the main controller.
> > >
> > > When using chained_irq_* it also wants to mask the interrupt which
> > > might conflict with regular enable_irq/disable_irq calls being done
> > > for example in driver code.
> > >
> > >
> > > So in short I agree with the earlier split of the irqchip, but would
> > > keep the irq operations themself centralized in the pinctrl driver,
> > > instead of using chained_irq_* functions.
> >
> > Right, my solution wouldn't work properly in case of regular s3c24xx
> > and edge triggered interrupts.
> >
> > However I'm still wondering if it's OK to manually call parent chip
> > operations in case of s3c2416. This would imply the same operation
> > calling order as imposed by flow handler of the chained EINT (which
> > can be handle_edge_irq), while the parent chip is probably level
> > triggered, isn't it?
>
> I think imposing the same calling order is the right way to go :-) .
>
> The eints can be set hardware-wise to either level or edge triggered,
> but their flow handler is currently hard set to handle_edge_irq. I
> remember seeing a patch from the Openmoko guys somewhere indicating
> that this does not work at all with level irqs.
Sounds like something broken to me. Level-triggered interrupts need
different handling than edge-triggered handling and so different flow
handlers exist for them.
To make my doubts clear, this is how I'm seeing the hardware on 2412:
edge or level triggered level triggered
| ___________________ | ________________
| | | | | |
EINT0 --> | EINT0 EINT0PEND | --> | IRQ0 ARM_INT | --> to the ARM core
| | | | | |
EINT1 --> | EINT1 EINT1PEND | --> | IRQ1 |
| | | | | |
EINT2 --> | EINT2 EINT2PEND | --> | IRQ2 |
| | | | | |
EINT3 --> | EINT3 EINT3PEND | --> | IRQ3 |
|___________________| |________________|
GPIO controller Interrupt controller
Now EINT0-EINT3 signals of the GPIO controller are directly driven with
external interrupt signals, either edge or level triggered, but the
interrupt status is latched in EINT0PEND-EINT3PEND bits inside the GPIO
controller and state of those bits is exported through EINT0PEND-EINT3PEND
signals to the interrupt controller's IRQ0-IRQ3 signals, which are now
level-triggered, since the interrupt is active only when EINTxPEND bit is
high.
Now, if my vision is wrong, and the hardware has simply EINT0-EINT3 pins
routed to IRQ0-IRQ3 signals of the interrupt controller, then what you're
saying is completely right. In this case just ignore my moaning. ;)
Best regards,
Tomasz
> Only the main interrupts of the real demuxed eints (3_to_7 and 8_to...)
> are always level triggered, and therefore already use chained_irq...
>
> In the current pinctrl driver, the parent eints do not have a flow
> handler of their own (as the chained handler replaces it). So all flow
> handling is done by the flowhandler of the irq in the pinctrl domain.
>
> handle_IRQ(main_eint)
> -> demux_eint0_3
> -> generic_handle_irq(pinctrl_eint)
> -> handle_edge_irq (or handle_level_irq)
> -> call ack, mask or unmask of the pinctrl eint
> -> call ack, mask or unmask of the underlying main eint
>
>
> Heiko
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
