[PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Bartlomiej Zolnierkiewicz]

* Rename ->ide_dma_clear_irq method to ->clear_irq
  and move it from ide_hwif_t to struct ide_port_ops.

* Move ->waiting_for_dma check inside ->clear_irq method.

* Move ->dma_base check inside ->clear_irq method.

piix.c:
* Add ich_port_ops and remove init_hwif_ich() wrapper.

There should be no functional changes caused by this patch.

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/ide/ide-io.c   |   15 ++++-----------
 drivers/ide/pci/piix.c |   39 +++++++++++++++++++++++----------------
 include/linux/ide.h    |    4 ++--
 3 files changed, 29 insertions(+), 29 deletions(-)

Index: b/drivers/ide/ide-io.c
===================================================================
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1418,23 +1418,16 @@ irqreturn_t ide_intr (int irq, void *dev
 	del_timer(&hwgroup->timer);
 	spin_unlock(&ide_lock);
 
-	/* Some controllers might set DMA INTR no matter DMA or PIO;
-	 * bmdma status might need to be cleared even for
-	 * PIO interrupts to prevent spurious/lost irq.
-	 */
-	if (hwif->ide_dma_clear_irq && !(drive->waiting_for_dma))
-		/* ide_dma_end() needs bmdma status for error checking.
-		 * So, skip clearing bmdma status here and leave it
-		 * to ide_dma_end() if this is dma interrupt.
-		 */
-		hwif->ide_dma_clear_irq(drive);
+	if (hwif->port_ops && hwif->port_ops->clear_irq)
+		hwif->port_ops->clear_irq(drive);
 
 	if (drive->dev_flags & IDE_DFLAG_UNMASK)
 		local_irq_enable_in_hardirq();
+
 	/* service this interrupt, may set handler for next interrupt */
 	startstop = handler(drive);
-	spin_lock_irq(&ide_lock);
 
+	spin_lock_irq(&ide_lock);
 	/*
 	 * Note that handler() may have set things up for another
 	 * interrupt to occur soon, but it cannot happen until
Index: b/drivers/ide/pci/piix.c
===================================================================
--- a/drivers/ide/pci/piix.c
+++ b/drivers/ide/pci/piix.c
@@ -215,17 +215,26 @@ static unsigned int init_chipset_ich(str
 }
 
 /**
- *	piix_dma_clear_irq	-	clear BMDMA status
- *	@drive: IDE drive to clear
+ *	ich_clear_irq	-	clear BMDMA status
+ *	@drive: IDE drive
  *
- *	Called from ide_intr() for PIO interrupts
- *	to clear BMDMA status as needed by ICHx
+ *	ICHx contollers set DMA INTR no matter DMA or PIO.
+ *	BMDMA status might need to be cleared even for
+ *	PIO interrupts to prevent spurious/lost IRQ.
  */
-static void piix_dma_clear_irq(ide_drive_t *drive)
+static void ich_clear_irq(ide_drive_t *drive)
 {
 	ide_hwif_t *hwif = HWIF(drive);
 	u8 dma_stat;
 
+	/*
+	 * ide_dma_end() needs BMDMA status for error checking.
+	 * So, skip clearing BMDMA status here and leave it
+	 * to ide_dma_end() if this is DMA interrupt.
+	 */
+	if (drive->waiting_for_dma || hwif->dma_base == 0)
+		return;
+
 	/* clear the INTR & ERROR bits */
 	dma_stat = inb(hwif->dma_base + ATA_DMA_STATUS);
 	/* Should we force the bit as well ? */
@@ -293,21 +302,19 @@ static void __devinit init_hwif_piix(ide
 		hwif->ultra_mask = hwif->mwdma_mask = hwif->swdma_mask = 0;
 }
 
-static void __devinit init_hwif_ich(ide_hwif_t *hwif)
-{
-	init_hwif_piix(hwif);
-
-	/* ICHx need to clear the BMDMA status for all interrupts */
-	if (hwif->dma_base)
-		hwif->ide_dma_clear_irq = &piix_dma_clear_irq;
-}
-
 static const struct ide_port_ops piix_port_ops = {
 	.set_pio_mode		= piix_set_pio_mode,
 	.set_dma_mode		= piix_set_dma_mode,
 	.cable_detect		= piix_cable_detect,
 };
 
+static const struct ide_port_ops ich_port_ops = {
+	.set_pio_mode		= piix_set_pio_mode,
+	.set_dma_mode		= piix_set_dma_mode,
+	.clear_irq		= ich_clear_irq,
+	.cable_detect		= piix_cable_detect,
+};
+
 #ifndef CONFIG_IA64
  #define IDE_HFLAGS_PIIX IDE_HFLAG_LEGACY_IRQS
 #else
@@ -331,9 +338,9 @@ static const struct ide_port_ops piix_po
 	{ \
 		.name		= DRV_NAME, \
 		.init_chipset	= init_chipset_ich, \
-		.init_hwif	= init_hwif_ich, \
+		.init_hwif	= init_hwif_piix, \
 		.enablebits	= {{0x41,0x80,0x80}, {0x43,0x80,0x80}}, \
-		.port_ops	= &piix_port_ops, \
+		.port_ops	= &ich_port_ops, \
 		.host_flags	= IDE_HFLAGS_PIIX, \
 		.pio_mask	= ATA_PIO4, \
 		.swdma_mask	= ATA_SWDMA2_ONLY, \
Index: b/include/linux/ide.h
===================================================================
--- a/include/linux/ide.h
+++ b/include/linux/ide.h
@@ -702,6 +702,7 @@ extern const struct ide_tp_ops default_t
  * @resetproc:		routine to reset controller after a disk reset
  * @maskproc:		special host masking for drive selection
  * @quirkproc:		check host's drive quirk list
+ * @clear_irq:		clear IRQ
  *
  * @mdma_filter:	filter MDMA modes
  * @udma_filter:	filter UDMA modes
@@ -718,6 +719,7 @@ struct ide_port_ops {
 	void	(*resetproc)(ide_drive_t *);
 	void	(*maskproc)(ide_drive_t *, int);
 	void	(*quirkproc)(ide_drive_t *);
+	void	(*clear_irq)(ide_drive_t *);
 
 	u8	(*mdma_filter)(ide_drive_t *);
 	u8	(*udma_filter)(ide_drive_t *);
@@ -780,8 +782,6 @@ typedef struct hwif_s {
 	const struct ide_port_ops	*port_ops;
 	const struct ide_dma_ops	*dma_ops;
 
-	void (*ide_dma_clear_irq)(ide_drive_t *drive);
-
 	/* dma physical region descriptor table (cpu view) */
 	unsigned int	*dmatable_cpu;
 	/* dma physical region descriptor table (dma view) */

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

> * Rename ->ide_dma_clear_irq method to ->clear_irq
>   and move it from ide_hwif_t to struct ide_port_ops.
>
> * Move ->waiting_for_dma check inside ->clear_irq method.
>
> * Move ->dma_base check inside ->clear_irq method.
>
> piix.c:
> * Add ich_port_ops and remove init_hwif_ich() wrapper.
>
> There should be no functional changes caused by this patch.
>   

   Good. I think it's worth implementing this method in at least 
cmd64x.c which actually reads the IDE interrupt latch bits (independent 
from the DMA interrupt status) in the dma_test_irq() methods but never 
clears them, so the latches may reflect a non-current state of the IDE 
interrupt...
   It may also be worth considering turning this method into 
test-and-clear, so that we can get the actual IDE interrupt state on the 
chips that implement this...

> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>

Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

MBR, Sergei

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Sergei Shtylyov]

Hello, I wrote:

>> * Rename ->ide_dma_clear_irq method to ->clear_irq
>>   and move it from ide_hwif_t to struct ide_port_ops.
>>
>> * Move ->waiting_for_dma check inside ->clear_irq method.
>>
>> * Move ->dma_base check inside ->clear_irq method.
>>
>> piix.c:
>> * Add ich_port_ops and remove init_hwif_ich() wrapper.
>>
>> There should be no functional changes caused by this patch.
>>   
>
>   Good. I think it's worth implementing this method in at least 
> cmd64x.c which actually reads the IDE interrupt latch bits 
> (independent from the DMA interrupt status) in the dma_test_irq() 
> methods but never clears them, so the latches may reflect a 
> non-current state of the IDE interrupt...

   I forgot that it does clear them in its dma_end() methods (which I 
myself have reworked :-).
   It seems however that at least for SFF-8038 compatibles, it makes 
sense to leave it that way since INTRQ might be asserted while BMIDE 
interrupt bit is not, so the interrupt latch would need clearing even on 
DMA timeout...

>   It may also be worth considering turning this method into 
> test-and-clear, so that we can get the actual IDE interrupt state on 
> the chips that implement this...

   Probably might add the test_irq() method to be called on 
!hwif->waiting_for_dma. Cleraing the status at once seems impractical...

>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>
> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

   Not feeling sure about this patch -- ->waiting_for_dma probably 
should've been left where it was...

MBR, Sergei

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Bartlomiej Zolnierkiewicz]

On Monday 15 September 2008 15:11:21 Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >> * Rename ->ide_dma_clear_irq method to ->clear_irq
> >>   and move it from ide_hwif_t to struct ide_port_ops.
> >>
> >> * Move ->waiting_for_dma check inside ->clear_irq method.
> >>
> >> * Move ->dma_base check inside ->clear_irq method.
> >>
> >> piix.c:
> >> * Add ich_port_ops and remove init_hwif_ich() wrapper.
> >>
> >> There should be no functional changes caused by this patch.
> >>   
> >
> >   Good. I think it's worth implementing this method in at least 
> > cmd64x.c which actually reads the IDE interrupt latch bits 
> > (independent from the DMA interrupt status) in the dma_test_irq() 
> > methods but never clears them, so the latches may reflect a 
> > non-current state of the IDE interrupt...
> 
>    I forgot that it does clear them in its dma_end() methods (which I 
> myself have reworked :-).
>    It seems however that at least for SFF-8038 compatibles, it makes 
> sense to leave it that way since INTRQ might be asserted while BMIDE 
> interrupt bit is not, so the interrupt latch would need clearing even on 
> DMA timeout...
> 
> >   It may also be worth considering turning this method into 
> > test-and-clear, so that we can get the actual IDE interrupt state on 
> > the chips that implement this...
> 
>    Probably might add the test_irq() method to be called on 
> !hwif->waiting_for_dma. Cleraing the status at once seems impractical...

Or we can test for ->waiting_for_dma inside ->test_irq.
 
> >> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >
> > Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
>    Not feeling sure about this patch -- ->waiting_for_dma probably 
> should've been left where it was...

Well, it doesn't change behavior and I think having ->clear_irq method
independent from the transfer mode is a preffered approach.

Thanks,
Bart

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Sergei Shtylyov]

Hello.

Bartlomiej Zolnierkiewicz wrote:

>>>> * Rename ->ide_dma_clear_irq method to ->clear_irq
>>>>   and move it from ide_hwif_t to struct ide_port_ops.
>>>>
>>>> * Move ->waiting_for_dma check inside ->clear_irq method.
>>>>
>>>> * Move ->dma_base check inside ->clear_irq method.
>>>>
>>>> piix.c:
>>>> * Add ich_port_ops and remove init_hwif_ich() wrapper.
>>>>
>>>> There should be no functional changes caused by this patch.
>>>>   
>>>>         
>>>   Good. I think it's worth implementing this method in at least 
>>> cmd64x.c which actually reads the IDE interrupt latch bits 
>>> (independent from the DMA interrupt status) in the dma_test_irq() 
>>> methods but never clears them, so the latches may reflect a 
>>> non-current state of the IDE interrupt...
>>>       
>>    I forgot that it does clear them in its dma_end() methods (which I 
>> myself have reworked :-).
>>    It seems however that at least for SFF-8038 compatibles, it makes 
>> sense to leave it that way since INTRQ might be asserted while BMIDE 
>> interrupt bit is not, so the interrupt latch would need clearing even on 
>> DMA timeout...
>>     
>>>   It may also be worth considering turning this method into 
>>> test-and-clear, so that we can get the actual IDE interrupt state on 
>>> the chips that implement this...
>>>       
>>    Probably might add the test_irq() method to be called on 
>> !hwif->waiting_for_dma. Cleraing the status at once seems impractical...
>>     
>
> Or we can test for ->waiting_for_dma inside ->test_irq.
>   

   That also will do...

>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
>>>>         
>>> Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
>>>       
>>    Not feeling sure about this patch -- ->waiting_for_dma probably 
>> should've been left where it was...
>>     
>
> Well, it doesn't change behavior and I think having ->clear_irq method
> independent from the transfer mode is a preffered approach.
>   

   But its implementations will have to depend on it anyway. And 
clearing the IDE interrupt in general already depends on the transfer 
mode -- the BMIDE interrupt which is a (delayed) reflection of INTRQ is 
cleared implicitly by the dma_end() method -- except in at least sgiioc4 
driver.
   BTW, sgiioc4 seems another candidate for clear_irq() implementation 
-- currently clearing is done implicitly by the read_status() method (I 
don't quite understand why it clears DMA error interrupt there). 
However, since there's no documentation, I'm not sure how the IDE 
interrupt is latched by IOC4.

> Thanks,
> Bart
>   

MBR, Sergei

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Jeremy Higdon]

On Tue, Sep 16, 2008 at 01:16:54PM +0400, Sergei Shtylyov wrote:
> >>>>Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> >>>>        
> >>>Acked-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> >>>      
> >>   Not feeling sure about this patch -- ->waiting_for_dma probably 
> >>should've been left where it was...
> >>    
> >
> >Well, it doesn't change behavior and I think having ->clear_irq method
> >independent from the transfer mode is a preffered approach.
> >  
> 
>   But its implementations will have to depend on it anyway. And 
> clearing the IDE interrupt in general already depends on the transfer 
> mode -- the BMIDE interrupt which is a (delayed) reflection of INTRQ is 
> cleared implicitly by the dma_end() method -- except in at least sgiioc4 
> driver.
>   BTW, sgiioc4 seems another candidate for clear_irq() implementation 
> -- currently clearing is done implicitly by the read_status() method (I 
> don't quite understand why it clears DMA error interrupt there). 
> However, since there's no documentation, I'm not sure how the IDE 
> interrupt is latched by IOC4.

The person who originally wrote sgiioc4 has been gone for a long time.

I'm guessing that the DMA error is cleared there, because any DMA error
status has already been processed above, and it's easier to clear
unconditionally than to test and clear, and there's no ill effect to
clearing something that's already clear.

jeremy

Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()

[Sergei Shtylyov]

Hello, I wrote:

>>> * Rename ->ide_dma_clear_irq method to ->clear_irq
>>>   and move it from ide_hwif_t to struct ide_port_ops.

>>> * Move ->waiting_for_dma check inside ->clear_irq method.

>>> * Move ->dma_base check inside ->clear_irq method.

>>> piix.c:
>>> * Add ich_port_ops and remove init_hwif_ich() wrapper.

>>> There should be no functional changes caused by this patch.

[...]

>>   It may also be worth considering turning this method into 
>> test-and-clear, so that we can get the actual IDE interrupt state on 
>> the chips that implement this...

>   Probably might add the test_irq() method to be called on 
> !hwif->waiting_for_dma. Cleraing the status at once seems impractical...

    Yet this seems what ack_intr() method is doing already...
    What it does is testing IRQ status and "acknowledging" it (the semantics 
of "acknowledge" is not clear to me, yet it seems that it's clearing the 
interrupt latch in the drivers where it's implemented). And the call site of 
ack_intr() method corresponds to where test_irq() should have been called, 
so it seems we don't need yet another method and probably didn't even need 
clear_irq() method in the first place?..
    Bart, could you clarify about how ack_intr() is supposed to work?

MBR, Sergei

->ack_intr in m68k IDE drivers [was: Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()]

[Bartlomiej Zolnierkiewicz]

On Thursday 21 May 2009 16:07:06 Sergei Shtylyov wrote:
> Hello, I wrote:
> 
> >>> * Rename ->ide_dma_clear_irq method to ->clear_irq
> >>>   and move it from ide_hwif_t to struct ide_port_ops.
> 
> >>> * Move ->waiting_for_dma check inside ->clear_irq method.
> 
> >>> * Move ->dma_base check inside ->clear_irq method.
> 
> >>> piix.c:
> >>> * Add ich_port_ops and remove init_hwif_ich() wrapper.
> 
> >>> There should be no functional changes caused by this patch.
> 
> [...]
> 
> >>   It may also be worth considering turning this method into 
> >> test-and-clear, so that we can get the actual IDE interrupt state on 
> >> the chips that implement this...
> 
> >   Probably might add the test_irq() method to be called on 
> > !hwif->waiting_for_dma. Cleraing the status at once seems impractical...
> 
>     Yet this seems what ack_intr() method is doing already...
>     What it does is testing IRQ status and "acknowledging" it (the semantics 
> of "acknowledge" is not clear to me, yet it seems that it's clearing the 
> interrupt latch in the drivers where it's implemented). And the call site of 
> ack_intr() method corresponds to where test_irq() should have been called, 
> so it seems we don't need yet another method and probably didn't even need 
> clear_irq() method in the first place?..

They have different goals -- the main purpose of ack_intr() (despite its name)
seems to be testing whether the IRQ is ours, OTOH in clear_irq() we know that
already and we just want to clear the pending IRQ status.

So I'm not sure if unification is desirable... though some improvements are
definitely possibly there (less confusing naming at least)...

>     Bart, could you clarify about how ack_intr() is supposed to work?

Good question, m68k list would be the best place to look for an answer..

Re: ->ack_intr in m68k IDE drivers [was: Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()]

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>  It may also be worth considering turning this method into 
>>>>test-and-clear, so that we can get the actual IDE interrupt state on 
>>>>the chips that implement this...

>>>  Probably might add the test_irq() method to be called on 
>>>!hwif->waiting_for_dma. Cleraing the status at once seems impractical...

>>    Yet this seems what ack_intr() method is doing already...
>>    What it does is testing IRQ status and "acknowledging" it (the semantics 
>>of "acknowledge" is not clear to me, yet it seems that it's clearing the 
>>interrupt latch in the drivers where it's implemented). And the call site of 
>>ack_intr() method corresponds to where test_irq() should have been called, 
>>so it seems we don't need yet another method and probably didn't even need 
>>clear_irq() method in the first place?..

> They have different goals -- the main purpose of ack_intr() (despite its name)
> seems to be testing whether the IRQ is ours,

    It does clear some interrupt bit if it sees that IRQ is ours too, hence 
the same.

> OTOH in clear_irq() we know that
> already and we just want to clear the pending IRQ status.

    There seems to be duplication of functionality b/w ack_intr() and 
clear_irq() now...

> So I'm not sure if unification is desirable... though some improvements are
> definitely possibly there (less confusing naming at least)...

>>    Bart, could you clarify about how ack_intr() is supposed to work?

> Good question, m68k list would be the best place to look for an answer..

    Well, I seem to have been able to infer it from the code...

MBR, Sergei

Re: ->ack_intr in m68k IDE drivers [was: Re: [PATCH 2/5] ide: ->ide_dma_clear_irq() -> ->clear_irq()]

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:

>>>>  It may also be worth considering turning this method into 
>>>>test-and-clear, so that we can get the actual IDE interrupt state on 
>>>>the chips that implement this...

>>>  Probably might add the test_irq() method to be called on 
>>>!hwif->waiting_for_dma. Cleraing the status at once seems impractical...

>>    Yet this seems what ack_intr() method is doing already...
>>    What it does is testing IRQ status and "acknowledging" it (the semantics 
>>of "acknowledge" is not clear to me, yet it seems that it's clearing the 
>>interrupt latch in the drivers where it's implemented). And the call site of 
>>ack_intr() method corresponds to where test_irq() should have been called, 
>>so it seems we don't need yet another method and probably didn't even need 
>>clear_irq() method in the first place?..

> They have different goals -- the main purpose of ack_intr() (despite its name)
> seems to be testing whether the IRQ is ours,

    It does clear some interrupt bit if it sees that IRQ is ours too, hence 
the name.

> OTOH in clear_irq() we know that
> already and we just want to clear the pending IRQ status.

    There seems to be duplication of functionality b/w ack_intr() and 
clear_irq() now...

> So I'm not sure if unification is desirable... though some improvements are
> definitely possibly there (less confusing naming at least)...

>>    Bart, could you clarify about how ack_intr() is supposed to work?

> Good question, m68k list would be the best place to look for an answer..

    Well, I seem to have been able to infer it from the code...

MBR, Sergei