[PATCH] pata_it821x: sync with IDE it821x driver

[PATCH] pata_it821x: sync with IDE it821x driver

[Bartlomiej Zolnierkiewicz]

* fix PIO setup for devices with different PIO speeds in passthru mode

  IT821x allows one PIO setting per port se we have to limit maximum
  PIO mode to the one of the slowest device on the port.

* (partially) fix DMA in RAID mode

  Code intended to check DMA status was checking DMA command register.
  Moreover firmware seems to "forget" to set DMA capable bit for the
  slave device (at least in RAID mode but without ITE RAID volumes) so
  check device ID for DMA capable bit when deciding whether to use DMA
  and remove DMA status check completely.

  Thanks to Pavol Šimo for the bugreport and testing the initial fix.

  This change unfortunately still doesn't fix DMA in RAID mode (which
  works fine with IDE it821x) but Alan is working on the missing pieces
  (pata_it821x vs libata EH issues).

Cc: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---

 drivers/ata/pata_it821x.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

Index: b/drivers/ata/pata_it821x.c
===================================================================
--- a/drivers/ata/pata_it821x.c
+++ b/drivers/ata/pata_it821x.c
@@ -2,6 +2,7 @@
  * pata_it821x.c 	- IT821x PATA for new ATA layer
  *			  (C) 2005 Red Hat Inc
  *			  Alan Cox <alan@redhat.com>
+ *			  (C) 2007 Bartlomiej Zolnierkiewicz
  *
  * based upon
  *
@@ -79,7 +80,7 @@
 
 
 #define DRV_NAME "pata_it821x"
-#define DRV_VERSION "0.3.6"
+#define DRV_VERSION "0.3.7"
 
 struct it821x_dev
 {
@@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
 	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
 
 	struct it821x_dev *itdev = ap->private_data;
+	struct ata_device *pair = ata_dev_pair(adev);
 	int unit = adev->devno;
-	int mode_wanted = adev->pio_mode - XFER_PIO_0;
+	int mode_wanted = adev->pio_mode;
+
+	if (pair && adev->pio_mode > pair->pio_mode)
+		mode_wanted = pair->pio_mode;
+
+	mode_wanted -= XFER_PIO_0;
 
 	/* We prefer 66Mhz clock for PIO 0-3, don't care for PIO4 */
 	itdev->want[unit][1] = pio_want[mode_wanted];
@@ -460,14 +467,8 @@ static unsigned int it821x_passthru_qc_i
 
 static int it821x_smart_set_mode(struct ata_port *ap, struct ata_device **unused)
 {
-	int dma_enabled = 0;
 	int i;
 
-	/* Bits 5 and 6 indicate if DMA is active on master/slave */
-	/* It is possible that BMDMA isn't allocated */
-	if (ap->ioaddr.bmdma_addr)
-		dma_enabled = ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
-
 	for (i = 0; i < ATA_MAX_DEVICES; i++) {
 		struct ata_device *dev = &ap->device[i];
 		if (ata_dev_enabled(dev)) {
@@ -476,7 +477,7 @@ static int it821x_smart_set_mode(struct 
 			dev->dma_mode = XFER_MW_DMA_0;
 			/* We do need the right mode information for DMA or PIO
 			   and this comes from the current configuration flags */
-			if (dma_enabled & (1 << (5 + i))) {
+			if (ata_id_has_dma(dev->id)) {
 				ata_dev_printk(dev, KERN_INFO, "configured for DMA\n");
 				dev->xfer_mode = XFER_MW_DMA_0;
 				dev->xfer_shift = ATA_SHIFT_MWDMA;

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Tejun Heo]

Hello,

Bartlomiej Zolnierkiewicz wrote:
> * (partially) fix DMA in RAID mode
> 
>   Code intended to check DMA status was checking DMA command register.
>   Moreover firmware seems to "forget" to set DMA capable bit for the
>   slave device (at least in RAID mode but without ITE RAID volumes) so
>   check device ID for DMA capable bit when deciding whether to use DMA
>   and remove DMA status check completely.
> 
>   Thanks to Pavol Šimo for the bugreport and testing the initial fix.

Ah... This is the mysterious mwdma configuration in smart mode, right?
Thanks for fixing this.

>   This change unfortunately still doesn't fix DMA in RAID mode (which
>   works fine with IDE it821x) but Alan is working on the missing pieces
>   (pata_it821x vs libata EH issues).

This is the lbal/nsect SRST problem, right?

> @@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
>  	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
>  
>  	struct it821x_dev *itdev = ap->private_data;
> +	struct ata_device *pair = ata_dev_pair(adev);
>  	int unit = adev->devno;
> -	int mode_wanted = adev->pio_mode - XFER_PIO_0;
> +	int mode_wanted = adev->pio_mode;
> +
> +	if (pair && adev->pio_mode > pair->pio_mode)
> +		mode_wanted = pair->pio_mode;
> +
> +	mode_wanted -= XFER_PIO_0;

I think this is better done by mode_filter callback which is guaranteed
to be called before any actual mode configuration is performed and in
device order (master then slave).

Thanks.

-- 
tejun

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Bartlomiej Zolnierkiewicz]

Hello,

On Saturday 09 June 2007, Tejun Heo wrote:
> Hello,
> 
> Bartlomiej Zolnierkiewicz wrote:
> > * (partially) fix DMA in RAID mode
> > 
> >   Code intended to check DMA status was checking DMA command register.
> >   Moreover firmware seems to "forget" to set DMA capable bit for the
> >   slave device (at least in RAID mode but without ITE RAID volumes) so
> >   check device ID for DMA capable bit when deciding whether to use DMA
> >   and remove DMA status check completely.
> > 
> >   Thanks to Pavol Šimo for the bugreport and testing the initial fix.
> 
> Ah... This is the mysterious mwdma configuration in smart mode, right?

Yep.

> Thanks for fixing this.
> 
> >   This change unfortunately still doesn't fix DMA in RAID mode (which
> >   works fine with IDE it821x) but Alan is working on the missing pieces
> >   (pata_it821x vs libata EH issues).
> 
> This is the lbal/nsect SRST problem, right?

I think so, due to lack of time I'm not following libata discussions closely.

> > @@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
> >  	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
> >  
> >  	struct it821x_dev *itdev = ap->private_data;
> > +	struct ata_device *pair = ata_dev_pair(adev);
> >  	int unit = adev->devno;
> > -	int mode_wanted = adev->pio_mode - XFER_PIO_0;
> > +	int mode_wanted = adev->pio_mode;
> > +
> > +	if (pair && adev->pio_mode > pair->pio_mode)
> > +		mode_wanted = pair->pio_mode;
> > +
> > +	mode_wanted -= XFER_PIO_0;
> 
> I think this is better done by mode_filter callback which is guaranteed
> to be called before any actual mode configuration is performed and in
> device order (master then slave).

I was thinking about using ->mode_filter but since all other PATA host
drivers are doing PIO filtering in ->set_piomode methods and also since it
seemed that using ->mode_filter method would result in slightly more complex
code I opted for coherency / simplicity.  However if you (or somebody else)
want to make a follow-up change to this driver (and preferably other PATA
host drivers ie. pata_sil680) to use ->mode_filter I'm also fine with that.

Thanks,
Bart

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Tejun Heo]

Bartlomiej Zolnierkiewicz wrote:
>>> @@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
>>>  	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
>>>  
>>>  	struct it821x_dev *itdev = ap->private_data;
>>> +	struct ata_device *pair = ata_dev_pair(adev);
>>>  	int unit = adev->devno;
>>> -	int mode_wanted = adev->pio_mode - XFER_PIO_0;
>>> +	int mode_wanted = adev->pio_mode;
>>> +
>>> +	if (pair && adev->pio_mode > pair->pio_mode)
>>> +		mode_wanted = pair->pio_mode;
>>> +
>>> +	mode_wanted -= XFER_PIO_0;
>> I think this is better done by mode_filter callback which is guaranteed
>> to be called before any actual mode configuration is performed and in
>> device order (master then slave).
> 
> I was thinking about using ->mode_filter but since all other PATA host
> drivers are doing PIO filtering in ->set_piomode methods and also since it
> seemed that using ->mode_filter method would result in slightly more complex
> code I opted for coherency / simplicity.  However if you (or somebody else)
> want to make a follow-up change to this driver (and preferably other PATA
> host drivers ie. pata_sil680) to use ->mode_filter I'm also fine with that.

Hmmm... indeed.  Alan, is there any reason we do that in ->set_piomode
not ->mode_filter?  We end up with mismatching configuration between the
controller and the higher speed drive.

-- 
tejun

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Alan Cox]

> Hmmm... indeed.  Alan, is there any reason we do that in ->set_piomode
> not ->mode_filter?  We end up with mismatching configuration between the
> controller and the higher speed drive.

We always talk more slowly than the drive which is just fine. Just about
every device on every controller does some variant of this, usually just
for the address setup timings. Our current behaviour (both old and new
IDE) may actually not be conservative enough for register rather than
data timings, which quite possibly should always be at the lower speed.

Currently the PATA drivers use mode_filter to remove modes not allowed
due to hardware issues/flaws/etc and a mix of their own timer merging and
the intelligence in the ata_timing functions to work out what timing
pattern should be used.

Beyond looking further into the register load timings and maybe making
the 8bit timings more conseratively merged (as Sergei I think feels they
should be) I don't plan to change this.

Alan

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Alan Cox]

> @@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
>  	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
>  
>  	struct it821x_dev *itdev = ap->private_data;
> +	struct ata_device *pair = ata_dev_pair(adev);
>  	int unit = adev->devno;
> -	int mode_wanted = adev->pio_mode - XFER_PIO_0;
> +	int mode_wanted = adev->pio_mode;
> +
> +	if (pair && adev->pio_mode > pair->pio_mode)
> +		mode_wanted = pair->pio_mode;
> +
> +	mode_wanted -= XFER_PIO_0;

NAK this too

Please see it821x_program.

it821x_passthru_set_piomode computes the required mode and the preferred
clock for the mode
it821x_clock_strategy the computes the best clock to use
it821x_program the programs the device

Whenever we issue a command it821x_passthru_dev_select then does a timing
mode switch and loads the PIO timing before doing the dev_select. The
same is also done when doing a qc_issue_prot so each qc_issue will have
the right timings loaded for the device.

So if you've got a problem case your diagnosis isn't one that makes a lot
of sense, and the fix is clearly wrong but I've not seen most of the
discussion between you and whoever you worked with to judge why.

One obvious possibility is that we are not currently doing timing
clipping for the address setup and merge for the 8bit timings. However
the docs I have strongly imply that the IT821x PIO/MWDMA timing register
is for data cycles only.

The other would be that there is some kind of pattern of interaction
between the ATA midlayer and the driver causing the wrong timings to get
loaded at some point.

Your "fix" would happen to work for both of these cases, but is wrong for
both of them.

If the register is switching all the timings then the fix is to switch
the PIO timing loaded to the lowest of the two initially (in _program)
and flip it back to the correct value at the start of the data transfer
sequence. We can then flip it back next qc_issue or indeed at the end of
the data xfer

If we've got an interaction problem then we need proper traces with
LIBATA debug turned right up so we can fix the actual case where the
switching occurs wrongly.

Alan

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Bartlomiej Zolnierkiewicz]

Hi Alan!

On Sunday 10 June 2007, Alan Cox wrote:
> > @@ -258,8 +259,14 @@ static void it821x_passthru_set_piomode(
> >  	static const u8 pio_want[]    = { ATA_66, ATA_66, ATA_66, ATA_66, ATA_ANY };
> >  
> >  	struct it821x_dev *itdev = ap->private_data;
> > +	struct ata_device *pair = ata_dev_pair(adev);
> >  	int unit = adev->devno;
> > -	int mode_wanted = adev->pio_mode - XFER_PIO_0;
> > +	int mode_wanted = adev->pio_mode;
> > +
> > +	if (pair && adev->pio_mode > pair->pio_mode)
> > +		mode_wanted = pair->pio_mode;
> > +
> > +	mode_wanted -= XFER_PIO_0;
> 
> NAK this too

OK, I'm able to understand the meaning of "NAK" [1] but "this too"?

[1] http://ozlabs.org/~rusty/index.cgi/2007/05/04

;)

> Please see it821x_program.
> 
> it821x_passthru_set_piomode computes the required mode and the preferred
> clock for the mode
> it821x_clock_strategy the computes the best clock to use
> it821x_program the programs the device
> 
> Whenever we issue a command it821x_passthru_dev_select then does a timing
> mode switch and loads the PIO timing before doing the dev_select. The
> same is also done when doing a qc_issue_prot so each qc_issue will have
> the right timings loaded for the device.
> 
> So if you've got a problem case your diagnosis isn't one that makes a lot
> of sense, and the fix is clearly wrong but I've not seen most of the
> discussion between you and whoever you worked with to judge why.
> 
> One obvious possibility is that we are not currently doing timing
> clipping for the address setup and merge for the 8bit timings. However
> the docs I have strongly imply that the IT821x PIO/MWDMA timing register
> is for data cycles only.
> 
> The other would be that there is some kind of pattern of interaction
> between the ATA midlayer and the driver causing the wrong timings to get
> loaded at some point.
> 
> Your "fix" would happen to work for both of these cases, but is wrong for
> both of them.
> 
> If the register is switching all the timings then the fix is to switch
> the PIO timing loaded to the lowest of the two initially (in _program)
> and flip it back to the correct value at the start of the data transfer
> sequence. We can then flip it back next qc_issue or indeed at the end of
> the data xfer
> 
> If we've got an interaction problem then we need proper traces with
> LIBATA debug turned right up so we can fix the actual case where the
> switching occurs wrongly.

PIO fix was directly ported from my it821x.c patch.  I now see that thanks
to it821x_passthru_dev_select() it is not really needeed in pata_it821x.c.

OTOH DMA fixes are pata_it821x.c unique, I will respin the patch.

Bart

Re: [PATCH] pata_it821x: sync with IDE it821x driver

[Alan Cox]

> > NAK this too
> 
> OK, I'm able to understand the meaning of "NAK" [1] but "this too"?

That was me being confusing with something else I NAKked that wasn't
from you. It may have made sense to Jeff but not you - sorry

> PIO fix was directly ported from my it821x.c patch.  I now see that thanks
> to it821x_passthru_dev_select() it is not really needeed in pata_it821x.c.

I don't think its needed but if we see problems with some drive mixes its
worth further debug. One nice thing about libata is the qc interfaces
make it possibly to do clock switches very cleanly.

Alan