Re: [PATCH 0/4] drivers/ata: add low-level I/O calls

Re: [PATCH 0/4] drivers/ata: add low-level I/O calls

[Alan]

On Fri, 12 Jan 2007 19:00:45 +0900
Akira Iguchi <akira2.iguchi@toshiba.co.jp> wrote:

> Dear everyone,
> 
> This is the patchset (based on 2.6.20-rc4) to add low-level I/O calls
> which access the taskfile registers. The idea comes from drivers/ide
> IN*/OUT* calls.

I think this is the wrong approach. The existing code provides methods
for things like "read the status", "write a taskfile" and this is the
style that should be kept.

Alan

Re: [PATCH 0/4] drivers/ata: add low-level I/O calls

[Benjamin Herrenschmidt]

On Fri, 2007-01-12 at 10:24 +0000, Alan wrote:
> On Fri, 12 Jan 2007 19:00:45 +0900
> Akira Iguchi <akira2.iguchi@toshiba.co.jp> wrote:
> 
> > Dear everyone,
> > 
> > This is the patchset (based on 2.6.20-rc4) to add low-level I/O calls
> > which access the taskfile registers. The idea comes from drivers/ide
> > IN*/OUT* calls.
> 
> I think this is the wrong approach. The existing code provides methods
> for things like "read the status", "write a taskfile" and this is the
> style that should be kept.

I agree (Sorry Akira-san for the added work !), I've looked a bit and I
think that what we need is mainly an accessor for the control register
and for the LBA, and an ack for dmdma. It's unclear to me wether the
full taskfile access would be a good way to do the later though, I'm a
bit worried some HW might get upset if not accessing strictly only the
right registers (we all know how "touchy" those IDE controllers can be).

That would give us something like:

 - bmdma_irq_ack
 - dev_ctrl_read/write (unless we want to split that but there are
   quite a few different accesses so maybe not)
 - dev_sig read/write (read/write nsect/lbal, or maybe we want to let
drivers do their own post_reset)

I think that's it, unless I missd something. 'special' drivers can still
replace bmdma_start/stop etc..

That's just a very quick look, I might have missed something.

Ben.
 

Re: [PATCH 0/4] drivers/ata: add low-level I/O calls

[Jeff Garzik]

Akira Iguchi wrote:
> Dear everyone,
> 
> This is the patchset (based on 2.6.20-rc4) to add low-level I/O calls
> which access the taskfile registers. The idea comes from drivers/ide
> IN*/OUT* calls.
> 
> As you know, these calls are unnecessary for most libata drivers.
> But the Celleb PATA driver needs them to use the libata common code.
> And using these calls, it is possible to remove similar code
> about PIO/MMIO access.

Sorry, but NAK.

libata intentionally provides higher level hooks than just I/O accessors.

A low level I/O hook approach makes it difficult to take into account 
platform-specific details like mmiowb(), especially on embedded platforms.

The high level hook approach also enables greater efficiency.  For 
example, an embedded platform could do

	__raw_writeb(datum, mmio_address + ATA_REG_FOO);
	__raw_writeb(datum, mmio_address + ATA_REG_BAR);
	eieio();

to optimize an entire taskfile-read or taskfile-write operation.

	Jeff

Re: [PATCH 0/4] drivers/ata: add low-level I/O calls

[Benjamin Herrenschmidt]

> libata intentionally provides higher level hooks than just I/O accessors.
> 
> A low level I/O hook approach makes it difficult to take into account 
> platform-specific details like mmiowb(), especially on embedded platforms.
> 
> The high level hook approach also enables greater efficiency.  For 
> example, an embedded platform could do
> 
> 	__raw_writeb(datum, mmio_address + ATA_REG_FOO);
> 	__raw_writeb(datum, mmio_address + ATA_REG_BAR);
> 	eieio();
> 
> to optimize an entire taskfile-read or taskfile-write operation.

I've been looking at that for them and for Efika but it's acutally quite
a bit of a mess. I'd be happy if you could have a look as well, but it's
not simple to find the "right" level of abstraction to be able to use
the reset/probe handling with special reset accessors. And hooking at
the toplevel is such a wastage (LOTS of code duplication) that it's
really no fun (it's what Toshiba initial patch does, though you haven't
commented on it).

So some hints as to where you think the right hooks/abstractions would
be would be much welcome.
 
Ben.

[PATCH 0/4] drivers/ata: add low-level I/O calls

[Akira Iguchi]

Dear everyone,

This is the patchset (based on 2.6.20-rc4) to add low-level I/O calls
which access the taskfile registers. The idea comes from drivers/ide
IN*/OUT* calls.

As you know, these calls are unnecessary for most libata drivers.
But the Celleb PATA driver needs them to use the libata common code.
And using these calls, it is possible to remove similar code
about PIO/MMIO access.

Please give me any comments.

Best regards,
Akira Iguchi