lkml.org 
[lkml]   [2016]   [Oct]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 1/3] NCR5380: Use probe_irq_*() for IRQ probing

    On Mon, 31 Oct 2016, Ondrej Zary wrote:

    > On Monday 31 October 2016, Finn Thain wrote:
    > > On Sun, 30 Oct 2016, Ondrej Zary wrote:
    > > > Use standard probe_irq_on() and probe_irq_off() functions instead of own
    > > > implementation.
    > >
    > > Thanks for doing this.
    > >
    > > > This prevents warning messages like this in the kernel log:
    > > > genirq: Flags mismatch irq 1. 00000000 (NCR-probe) vs. 00000080 (i8042)
    > > >
    > > > Move the IRQ trigger code to a separate function so it can be used for
    > > > other purposes (testing if the IRQ works).
    > > >
    > > > Also add missing IRQ reset after probe.
    > > >
    > > > Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
    > > > ---
    > > > drivers/scsi/NCR5380.c | 72
    > > > +++++++++++++++++----------------------------- drivers/scsi/NCR5380.h |
    > > > 3 +-
    > > > drivers/scsi/g_NCR5380.c | 2 +-
    > > > 3 files changed, 29 insertions(+), 48 deletions(-)
    > > >
    > > > diff --git a/drivers/scsi/NCR5380.c b/drivers/scsi/NCR5380.c
    > > > index d849ffa..01c0027 100644
    > > > --- a/drivers/scsi/NCR5380.c
    > > > +++ b/drivers/scsi/NCR5380.c
    > > > @@ -351,50 +351,13 @@ static void NCR5380_print_phase(struct Scsi_Host
    > > > *instance) }
    > > > #endif
    > > >
    > > > -
    > > > -static int probe_irq;
    > > > -
    > > > -/**
    > > > - * probe_intr - helper for IRQ autoprobe
    > > > - * @irq: interrupt number
    > > > - * @dev_id: unused
    > > > - * @regs: unused
    > > > - *
    > > > - * Set a flag to indicate the IRQ in question was received. This is
    > > > - * used by the IRQ probe code.
    > > > - */
    > > > -
    > > > -static irqreturn_t probe_intr(int irq, void *dev_id)
    > > > -{
    > > > - probe_irq = irq;
    > > > - return IRQ_HANDLED;
    > > > -}
    > > > -
    > > > -/**
    > > > - * NCR5380_probe_irq - find the IRQ of an NCR5380
    > > > - * @instance: NCR5380 controller
    > > > - * @possible: bitmask of ISA IRQ lines
    > > > - *
    > > > - * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
    > > > - * and then looking to see what interrupt actually turned up.
    > > > - */
    > > > -
    > > > -static int __maybe_unused NCR5380_probe_irq(struct Scsi_Host *instance,
    > > > - int possible)
    > > > +static void NCR5380_trigger_irq(struct Scsi_Host *instance)
    > > > {
    > > > struct NCR5380_hostdata *hostdata = shost_priv(instance);
    > > > - unsigned long timeout;
    > > > - int trying_irqs, i, mask;
    > > > -
    > > > - for (trying_irqs = 0, i = 1, mask = 2; i < 16; ++i, mask <<= 1)
    > > > - if ((mask & possible) && (request_irq(i, &probe_intr, 0, "NCR-probe",
    > > > NULL) == 0)) - trying_irqs |= mask;
    > > > -
    > > > - timeout = jiffies + msecs_to_jiffies(250);
    > > > - probe_irq = NO_IRQ;
    > > > + unsigned long timeout = jiffies + msecs_to_jiffies(2500);
    > > >
    > > > /*
    > > > - * A interrupt is triggered whenever BSY = false, SEL = true
    > > > + * An interrupt is triggered whenever BSY = false, SEL = true
    > > > * and a bit set in the SELECT_ENABLE_REG is asserted on the
    > > > * SCSI bus.
    > > > *
    > > > @@ -402,23 +365,40 @@ static int __maybe_unused NCR5380_probe_irq(struct
    > > > Scsi_Host *instance, * (I/O, C/D, and MSG) match those in the TCR, so we
    > > > must reset that * to zero.
    > > > */
    > > > -
    > > > NCR5380_write(TARGET_COMMAND_REG, 0);
    > > > NCR5380_write(SELECT_ENABLE_REG, hostdata->id_mask);
    > > > NCR5380_write(OUTPUT_DATA_REG, hostdata->id_mask);
    > > > NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE | ICR_ASSERT_DATA |
    > > > ICR_ASSERT_SEL);
    > > >
    > > > - while (probe_irq == NO_IRQ && time_before(jiffies, timeout))
    > > > + while (!(NCR5380_read(BUS_AND_STATUS_REG) & BASR_IRQ) &&
    > > > + time_before(jiffies, timeout))
    > > > schedule_timeout_uninterruptible(1);
    > >
    > > You don't need a 2.5 second timeout here. To raise the interrupt the chip
    > > should not need more than a bus settle delay (400ns). Calling msleep(1)
    > > should be plenty.
    >
    > Yes, 2.5s is a mistake (leftover from testing), sorry.
    >
    > > Better still, please drop the loop, the delay and the BASR_IRQ test here,
    > > and if need be, put the delay in the caller. This routine discards the
    > > result of that sequence anyway.
    >
    > Will if work if the following lines are executed before the delay?

    I think so... but you're right, it is better if the delay remains here.

    >
    > >
    > > > NCR5380_write(SELECT_ENABLE_REG, 0);
    > > > NCR5380_write(INITIATOR_COMMAND_REG, ICR_BASE);
    > > > +}
    > > >
    > > > - for (i = 1, mask = 2; i < 16; ++i, mask <<= 1)
    > > > - if (trying_irqs & mask)
    > > > - free_irq(i, NULL);
    > > > +/**
    > > > + * NCR5380_probe_irq - find the IRQ of an NCR5380
    > > > + * @instance: NCR5380 controller
    > > > + *
    > > > + * Autoprobe for the IRQ line used by the NCR5380 by triggering an IRQ
    > > > + * and then looking to see what interrupt actually turned up.
    > > > + */
    > > > +
    > > > +static int NCR5380_probe_irq(struct Scsi_Host *instance)
    > > > +{
    > > > + struct NCR5380_hostdata *hostdata = shost_priv(instance);
    > > > + int irqs, irq;
    > > > +
    > >
    > > Please rename irqs to irq_mask.
    > >
    > > Other drivers like tty/cyclades.c begin with,
    > >
    > > /* forget possible initially masked and pending IRQ */
    > > irq = probe_irq_off(probe_irq_on());
    > >
    > > Perhaps we need that here too?
    >
    > Only cyclades and 8250_port do that, other drivers don't.
    > Don't know what exact problem should that solve.

    OK. I'm no expert on ISA so I'll leave it up to you.

    --

    >
    > >
    > > > + irqs = probe_irq_on();
    > >
    > > You should clear any pending interrupt on the chip before you try to
    > > trigger another one.
    > >
    > > > + NCR5380_trigger_irq(instance);
    > >
    > > Please do the msleep(1) here.
    > >
    > > > + irq = probe_irq_off(irqs);
    > > > + NCR5380_read(RESET_PARITY_INTERRUPT_REG);
    > > > + if (irq < 0)
    > > > + irq = 0;
    > >
    > > Please use NO_IRQ instead of 0.
    > >
    > > > - return probe_irq;
    > > > + return irq;
    > > > }
    > > >
    > > > /**
    > > > diff --git a/drivers/scsi/NCR5380.h b/drivers/scsi/NCR5380.h
    > > > index 3c6ce54..b2fca52 100644
    > > > --- a/drivers/scsi/NCR5380.h
    > > > +++ b/drivers/scsi/NCR5380.h
    > > > @@ -290,7 +290,8 @@ static inline struct scsi_cmnd
    > > > *NCR5380_to_scmd(struct NCR5380_cmd *ncmd_ptr) #define
    > > > NCR5380_dprint_phase(flg, arg) do {} while (0)
    > > > #endif
    > > >
    > > > -static int NCR5380_probe_irq(struct Scsi_Host *instance, int possible);
    > > > +static void NCR5380_trigger_irq(struct Scsi_Host *instance);
    > > > +static int NCR5380_probe_irq(struct Scsi_Host *instance);
    > > > static int NCR5380_init(struct Scsi_Host *instance, int flags);
    > > > static int NCR5380_maybe_reset_bus(struct Scsi_Host *);
    > > > static void NCR5380_exit(struct Scsi_Host *instance);
    > >
    > > Please move NCR5380_probe_irq and NCR5380_trigger_irq implementations
    > > and prototypes out of the core driver and into g_NCR5380.c. The reason
    > > is, ISA cards are probably the only 5380 cards ever likely to need
    > > manual IRQ configuration. It would better to have g_NCR5380 support
    > > more ISA cards than to add more ISA drivers, therefore this code isn't
    > > likely to be needed in the core driver.
    > >
    > > Thanks.
    >
    >
    >
    >

    \
     
     \ /
      Last update: 2016-10-31 10:41    [W:3.675 / U:0.396 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site