lkml.org 
[lkml]   [2011]   [Oct]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 2/4] ata_piix: unify code for programming PIO and MWDMA timings
Hello.

On 13-10-2011 17:39, Bartlomiej Zolnierkiewicz wrote:

> From: Bartlomiej Zolnierkiewicz<bzolnier@gmail.com>
> Subject: [PATCH v2] ata_piix: unify code for programming PIO and MWDMA timings

> Besides making things noticably simpler it results in ~2% decrease in
> the driver LOC count and also ~2% decrease in the driver binary size
> (as measured on x86-32).

> Fix piix_set_piomode() documentation while at it.

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

I'm still inclined to NAK this patch.

> ---
> v2: remove use_mwdma variable and fix comment style

> earlier references:
> https://lkml.org/lkml/2011/2/8/99
>
> drivers/ata/ata_piix.c | 111 ++++++++++++++++---------------------------------
> 1 file changed, 38 insertions(+), 73 deletions(-)

> Index: b/drivers/ata/ata_piix.c
> ===================================================================
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
[...]
> @@ -811,31 +822,20 @@ static void do_pata_set_dmamode(struct a
[...]
> - if (ap->udma_mask)
> - pci_read_config_byte(dev, 0x48,&udma_enable);

Don't think you should remove this.

> -
> if (speed>= XFER_UDMA_0) {
> - unsigned int udma = adev->dma_mode - XFER_UDMA_0;
> + unsigned int udma = speed - XFER_UDMA_0;
> u16 udma_timing;
> u16 ideconf;
> int u_clock, u_speed;
>
> + spin_lock_irqsave(&piix_lock, flags);
> +
> + pci_read_config_byte(dev, 0x48, &udma_enable);

But you should also clear the UDMA enable bit when setting an MWDMA mode, no?

> +
> /*
> * UDMA is handled by a combination of clock switching and
> * selection of dividers
> @@ -868,56 +868,21 @@ static void do_pata_set_dmamode(struct a
> performance (WR_PingPong_En) */
> pci_write_config_word(dev, 0x54, ideconf);
> }
> +
> + pci_write_config_byte(dev, 0x48, udma_enable);
> +
> + spin_unlock_irqrestore(&piix_lock, flags);
> } else {
> - /*
> - * MWDMA is driven by the PIO timings. We must also enable
> - * IORDY unconditionally along with TIME1. PPE has already
> - * been set when the PIO timing was set.
> - */
> - unsigned int mwdma = adev->dma_mode - XFER_MW_DMA_0;
> - unsigned int control;
> - u8 slave_data;
> + /* MWDMA is driven by the PIO timings. */
> + unsigned int mwdma = speed - XFER_MW_DMA_0;
> const unsigned int needed_pio[3] = {
> XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
> };
> int pio = needed_pio[mwdma] - XFER_PIO_0;
>
[...]
> - if (ap->udma_mask)
> - udma_enable&= ~(1<< devid);

Still don't understand why you're removing this code.

> -
> - pci_write_config_word(dev, master_port, master_data);
> + /* XFER_PIO_0 is never used currently */
> + piix_set_timings(ap, adev, pio);
> }
> - /* Don't scribble on 0x48 if the controller does not support UDMA */
> - if (ap->udma_mask)
> - pci_write_config_byte(dev, 0x48, udma_enable);

Should have kept this one too...

MBR, Sergei


\
 
 \ /
  Last update: 2011-10-14 14:05    [W:0.039 / U:2.944 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site