hwif->rw_disk() method

hwif->rw_disk() method

[Sergei Shtylyov]

Hello.

    Alan/Bart/anybody, could you enlighten me why hwif->rw_disk() method came 
into being at all?
    IIUC, hwif->dma_setup() method could have been used instead which is 
called from the ATAPI drivers and the ide-taskfile.c, while hwif->rw_disk() is 
only called from ide-disk.c.  I'm thinking about replacing this hook in 
hpt366.c (the only its user) an getting rid of it altogether...

MBR, Sergei

Re: hwif->rw_disk() method

[Sergei Shtylyov]

Hello.

Alan wrote:

>>    Alan/Bart/anybody, could you enlighten me why hwif->rw_disk() method came 
>>into being at all?

> When you needed to wrap entire disk operations. The ->dma_ methods only
> wrap DMA commands.

    Yeah, good point. But it's not likely that the HighPoint driver actually 
needs this wrapper for PIO or even MW DMA commands...

> For the current support hardware it can probably go, no idea if anyone
> embedded uses it any more and can't be bothered to check 8)

    Nobody does, as far I can see, except this wretched driver... :-)

MBR, Sergei

Re: hwif->rw_disk() method

[Alan]

>     Alan/Bart/anybody, could you enlighten me why hwif->rw_disk() method came 
> into being at all?

When you needed to wrap entire disk operations. The ->dma_ methods only
wrap DMA commands.

For the current support hardware it can probably go, no idea if anyone
embedded uses it any more and can't be bothered to check 8)

Re: hwif->rw_disk() method

[Bartlomiej Zolnierkiewicz]

On 1/10/07, Sergei Shtylyov <sshtylyov@ru.mvista.com> wrote:
> Hello.

Hi,

> Alan wrote:
>
> >>    Alan/Bart/anybody, could you enlighten me why hwif->rw_disk() method came
> >>into being at all?
>
> > When you needed to wrap entire disk operations. The ->dma_ methods only
> > wrap DMA commands.

Exactly.  IIRC ->rw_disk method was used for PIO by (removed) pdc4030 driver.

>     Yeah, good point. But it's not likely that the HighPoint driver actually
> needs this wrapper for PIO or even MW DMA commands...
>
> > For the current support hardware it can probably go, no idea if anyone
> > embedded uses it any more and can't be bothered to check 8)
>
>     Nobody does, as far I can see, except this wretched driver... :-)

Yes, so if HPT hardware doesn't need clock switch for PIO ->rw_disk can go.

Bart

Re: hwif->rw_disk() method

[Sergei Shtylyov]

Hello.

Alan wrote:

>>    Alan/Bart/anybody, could you enlighten me why hwif->rw_disk() method came 
>>into being at all?

> When you needed to wrap entire disk operations. The ->dma_ methods only
> wrap DMA commands.

    BTW, I've looked at the clock turnaround code both in drivers/ide/hpt366.c 
and drivers/ata/pata_hpt3x2n.c and compared it to the HighPoint driver and I'm 
dazed and confused (again?):  contrary to what HighPoint does (I'm not sure 
they're still doing it at all due to confused nature of their drivers :-), 
Linux uses DPLL on reads and PCI clock on writes.  The libata driver seem to 
have an explicit contradiction:

static int hpt3x2n_use_dpll(struct ata_port *ap, int reading)
{
         long flags = (long)ap->host->private_data;
         /* See if we should use the DPLL */
         if (reading == 0)
                 return USE_DPLL;        /* Needed for write */
         if (flags & PCI66)
                 return USE_DPLL;        /* Needed at 66Mhz */
         return 0;
}

static unsigned int hpt3x2n_qc_issue_prot(struct ata_queued_cmd *qc)
{
         struct ata_taskfile *tf = &qc->tf;
         struct ata_port *ap = qc->ap;
         int flags = (long)ap->host->private_data;

         if (hpt3x2n_pair_idle(ap)) {
                 int dpll = hpt3x2n_use_dpll(ap, (tf->flags & ATA_TFLAG_WRITE));
                 if ((flags & USE_DPLL) != dpll) {
                         if (dpll == 1)
                                 hpt3x2n_set_clock(ap, 0x21);
                         else
                                 hpt3x2n_set_clock(ap, 0x23);
                 }
         }
         return ata_qc_issue_prot(qc);
}

    ATA_TFLAG_WRITE indicates a write transfer (host-to-device) while 
hpt3x2n_use_dpll() contrarywise expects the read transfer indication. Comments?

WBR, Sergei

Re: hwif->rw_disk() method

[Alan]

>     BTW, I've looked at the clock turnaround code both in drivers/ide/hpt366.c 
> and drivers/ata/pata_hpt3x2n.c and compared it to the HighPoint driver and I'm 
> dazed and confused (again?):  contrary to what HighPoint does (I'm not sure 

Thats how I usually feel after looking at highpoints driver code.


>     ATA_TFLAG_WRITE indicates a write transfer (host-to-device) while 
> hpt3x2n_use_dpll() contrarywise expects the read transfer indication. Comments?

That looks like a bug. The code seems to get the DPLL choice right but
then the caller as you say is incorrect.

Will fix