[RFC][DOC] writing IDE driver guidelines

[RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

Feedback is welcomed.


writing IDE driver guidelines


general rules:

- code outside drivers/ide directory shouldn't need to
  include <linux/ide.h> if it does something is wrong

- do not believe in popular myth that driver code
  can be of less quality than core kernel code

- don't copy without thinking ugly and bogus code
  (there is still lot of such in IDE)

- IDE is going into full host driver model
  so write host driver for your interface

- host drivers should request/release IO resource
  themelves and set hwif->mmio to 2

- remember about ordering issues
  (you break device ordering and some machines won't boot)

- use linux-ide@vger.kernel.org mailing list


new architecture:

- add only what is really necessary to <asm/ide.h>
  and put interface specific code into drivers/ide

- ide_init_hwif_ports() is obsolete and dying,
  define IDE_ARCH_NO_OBSOLETE_INIT in <asm/ide.h>

- define ide_default_irq(), ide_init_default_irq()
  and ide_default_io_base() to (0)

- ide_init_default_hwifs() is gone


architecture specific drivers:

- do not abuse ide_default_irq()/ide_default_io_base() from <asm/ide.h>

- your driver should be in drivers/ide/<your_arch>/<your_driver_name>

- see drivers in drivers/ide/arm and drivers/ide/legacy for examples
  (especially m68k and arm specific drivers)

- do not use ide_setup_ports()


PCI drivers:

- no need to split your driver on .c and .h files

- /proc/ide/ interfaces are obsolete


--
Bartlomiej Zolnierkiewicz (15 May 2004)

Re: [RFC][DOC] writing IDE driver guidelines

[Jeff Garzik]

On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> - do not believe in popular myth that driver code
>   can be of less quality than core kernel code

chuckle :)


> - don't copy without thinking ugly and bogus code
>   (there is still lot of such in IDE)

Agreed, but I think most driver authors will not know what is ugly
and bogus code, otherwise they would probably not copy it... (I hope!)


> - host drivers should request/release IO resource
>   themelves and set hwif->mmio to 2

Don't you mean, hwif->mmio==2 for MMIO hardware?


> - ide_init_hwif_ports() is obsolete and dying,
>   define IDE_ARCH_NO_OBSOLETE_INIT in <asm/ide.h>

hmmmm.  Please consider reversing this:

Make ide_init_hwif_ports() present _only_ if IDE_ARCH_OBSOLETE_INIT
is defined.

Then add that define for all arches that still use ide_init_hwif_ports().


> - define ide_default_irq(), ide_init_default_irq()
>   and ide_default_io_base() to (0)

Maybe provide generic definitions, so that new arches don't even
have to care about this?

	Jeff

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Saturday 15 of May 2004 19:34, Jeff Garzik wrote:
> On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > - do not believe in popular myth that driver code
> >   can be of less quality than core kernel code
>
> chuckle :)

hehe :)

> > - don't copy without thinking ugly and bogus code
> >   (there is still lot of such in IDE)
>
> Agreed, but I think most driver authors will not know what is ugly
> and bogus code, otherwise they would probably not copy it... (I hope!)

Some just copy _everything_ what is in other driver, really...

> > - host drivers should request/release IO resource
> >   themelves and set hwif->mmio to 2
>
> Don't you mean, hwif->mmio==2 for MMIO hardware?

It is was historically for MMIO, now it means that driver
handles IO resource itself (per comment in <linux/ide.h>).

> > - ide_init_hwif_ports() is obsolete and dying,
> >   define IDE_ARCH_NO_OBSOLETE_INIT in <asm/ide.h>
>
> hmmmm.  Please consider reversing this:
>
> Make ide_init_hwif_ports() present _only_ if IDE_ARCH_OBSOLETE_INIT
> is defined.
>
> Then add that define for all arches that still use ide_init_hwif_ports().

Well, I started with this idea but it requires adding this define for far
too many archs.  This is a problem especially for arm because we have to
either leave <asm-arm/arch-*/ide.h> or add #ifdef horror <asm-arm/ide.h>.

> > - define ide_default_irq(), ide_init_default_irq()
> >   and ide_default_io_base() to (0)
>
> Maybe provide generic definitions, so that new arches don't even
> have to care about this?

Please explain.

Thanks,
Bartlomiej

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Saturday 15 of May 2004 19:58, Bartlomiej Zolnierkiewicz wrote:
> > > - ide_init_hwif_ports() is obsolete and dying,
> > >   define IDE_ARCH_NO_OBSOLETE_INIT in <asm/ide.h>
> >
> > hmmmm.  Please consider reversing this:
> >
> > Make ide_init_hwif_ports() present _only_ if IDE_ARCH_OBSOLETE_INIT
> > is defined.
> >
> > Then add that define for all arches that still use ide_init_hwif_ports().
>
> Well, I started with this idea but it requires adding this define for far
> too many archs.  This is a problem especially for arm because we have to
> either leave <asm-arm/arch-*/ide.h> or add #ifdef horror <asm-arm/ide.h>.

It was thinko.  I will change this, thanks.

Re: [RFC][DOC] writing IDE driver guidelines

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Saturday 15 of May 2004 19:34, Jeff Garzik wrote:
>>On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
>>>- host drivers should request/release IO resource
>>>  themelves and set hwif->mmio to 2
>>
>>Don't you mean, hwif->mmio==2 for MMIO hardware?
> 
> 
> It is was historically for MMIO, now it means that driver
> handles IO resource itself (per comment in <linux/ide.h>).

Maybe then create a constant HOST_IO_RESOURCES (value==2) to make that 
more obvious?


>>>- define ide_default_irq(), ide_init_default_irq()
>>>  and ide_default_io_base() to (0)
>>
>>Maybe provide generic definitions, so that new arches don't even
>>have to care about this?
> 
> 
> Please explain.

Your document appears to imply that each new arch should define the 
above three symbols.

My suggestion is to devise a method by which new arches don't have to 
care about those symbols at all, unless required to do so by the 
underlying hardware.

	Jeff

Re: [RFC][DOC] writing IDE driver guidelines

[Marc Singer]

On Sat, May 15, 2004 at 07:58:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > - host drivers should request/release IO resource
> > >   themelves and set hwif->mmio to 2
> >
> > Don't you mean, hwif->mmio==2 for MMIO hardware?
> 
> It is was historically for MMIO, now it means that driver
> handles IO resource itself (per comment in <linux/ide.h>).

If you are talking about this

        int             mmio;           /* hosts iomio (0) or custom (2) select 

do you really think that's enough?

Re: [RFC][DOC] writing IDE driver guidelines

[Marc Singer]

On Sat, May 15, 2004 at 06:23:04PM -0400, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> >On Saturday 15 of May 2004 19:34, Jeff Garzik wrote:
> >>On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> >>>- host drivers should request/release IO resource
> >>> themelves and set hwif->mmio to 2
> >>
> >>Don't you mean, hwif->mmio==2 for MMIO hardware?
> >
> >
> >It is was historically for MMIO, now it means that driver
> >handles IO resource itself (per comment in <linux/ide.h>).
> 
> Maybe then create a constant HOST_IO_RESOURCES (value==2) to make that 
> more obvious?
> 

Please allow me to advocate for the naive.

While I do not in favor of lengthy commented discourses within the
code for all of the usual reasons, I do believe that interface
documentation is always welcome.  It encourages everyone to learn and
follow the rules.  It allows the subsystem maintainer to establish a
boundary so that accessing lower-level structures are left alone.

I'm not talking about a HOWTO as we know it.  Let's look at this mmio
flag.  How about writing this at a very minimum.

  	int mmio; /* 0: iommio; <insert appropriate direction */
		  /* 2: custom; driver must reserve & release system resources */
		  

Certainly, I'd rather see something along the lines of a full
description.

	int mmio;
	    /* This field controls whether or not the driver blah,
	       blah.  If the driver needs to reserve system resources,
	       e.g. ports of memory, set the value to 2 and blah, blah. */

It isn't much, but it goes a long way.

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Sunday 16 of May 2004 00:23, Jeff Garzik wrote:

> >>>- define ide_default_irq(), ide_init_default_irq()
> >>>  and ide_default_io_base() to (0)
> >>
> >>Maybe provide generic definitions, so that new arches don't even
> >>have to care about this?
> >
> > Please explain.
>
> Your document appears to imply that each new arch should define the
> above three symbols.
>
> My suggestion is to devise a method by which new arches don't have to
> care about those symbols at all, unless required to do so by the
> underlying hardware.

OK will work on this

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Sunday 16 of May 2004 02:51, Marc Singer wrote:
> On Sat, May 15, 2004 at 07:58:03PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > - host drivers should request/release IO resource
> > > >   themelves and set hwif->mmio to 2
> > >
> > > Don't you mean, hwif->mmio==2 for MMIO hardware?
> >
> > It is was historically for MMIO, now it means that driver
> > handles IO resource itself (per comment in <linux/ide.h>).
>
> If you are talking about this
>
>         int             mmio;           /* hosts iomio (0) or custom (2)
> select
>
> do you really think that's enough?

No and I didn't say this.  I tried to describe current state of affairs.
Don't blame me for cryptic comments in <linux/ide.h>.

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Sunday 16 of May 2004 03:00, Marc Singer wrote:
> On Sat, May 15, 2004 at 06:23:04PM -0400, Jeff Garzik wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > >On Saturday 15 of May 2004 19:34, Jeff Garzik wrote:
> > >>On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > >>>- host drivers should request/release IO resource
> > >>> themelves and set hwif->mmio to 2
> > >>
> > >>Don't you mean, hwif->mmio==2 for MMIO hardware?
> > >
> > >It is was historically for MMIO, now it means that driver
> > >handles IO resource itself (per comment in <linux/ide.h>).
> >
> > Maybe then create a constant HOST_IO_RESOURCES (value==2) to make that
> > more obvious?
>
> Please allow me to advocate for the naive.
>
> While I do not in favor of lengthy commented discourses within the
> code for all of the usual reasons, I do believe that interface
> documentation is always welcome.  It encourages everyone to learn and
> follow the rules.  It allows the subsystem maintainer to establish a
> boundary so that accessing lower-level structures are left alone.
>
> I'm not talking about a HOWTO as we know it.  Let's look at this mmio
> flag.  How about writing this at a very minimum.
>
>   	int mmio; /* 0: iommio; <insert appropriate direction */
> 		  /* 2: custom; driver must reserve & release system resources */

I think this is good for ide.h

> Certainly, I'd rather see something along the lines of a full
> description.
>
> 	int mmio;
> 	    /* This field controls whether or not the driver blah,
> 	       blah.  If the driver needs to reserve system resources,
> 	       e.g. ports of memory, set the value to 2 and blah, blah. */

and this is good for documentation file.

> It isn't much, but it goes a long way.

Thanks.

Re: [RFC][DOC] writing IDE driver guidelines

[Marc Singer]

On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> - IDE is going into full host driver model
>   so write host driver for your interface

Can you tell me what this means?  Is arm/rapide.c an example?  If not,
what *is* and example?  Is the arm/ide_arm.c *not* the right way?

> - see drivers in drivers/ide/arm and drivers/ide/legacy for examples
>   (especially m68k and arm specific drivers)
> 
> - do not use ide_setup_ports()

Hmm.  macide in legacy does.  Is this, then, a bad bad example legacy?

The piece I'm missing is how to replace the ioops functions for my
platform.  I could implement something in ide-iops.c.  Is that the way?

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Saturday 05 of June 2004 21:49, Marc Singer wrote:
> On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > - IDE is going into full host driver model
> >   so write host driver for your interface
>
> Can you tell me what this means?  Is arm/rapide.c an example?  If not,
> what *is* and example?  Is the arm/ide_arm.c *not* the right way?

This was supposed to mean to not hide your code in <asm/ide.h> and abuse
ide_init_default_hwifs() (which is gone).  I'll make this more verbose.

arm/rapide.c and arm/ide_arm.c are both okay.
Bad example is <asm-arm/arch-sa1100/ide.h>.

> > - see drivers in drivers/ide/arm and drivers/ide/legacy for examples
> >   (especially m68k and arm specific drivers)
> >
> > - do not use ide_setup_ports()
>
> Hmm.  macide in legacy does.  Is this, then, a bad bad example legacy?
>
> The piece I'm missing is how to replace the ioops functions for my
> platform.  I could implement something in ide-iops.c.  Is that the way?

No, your host driver should do it:

If you call ide_register_hw(&hw, &hwif) you should get a valid pointer to
hwif so you can then replace standard ioops by your custom ones if needed.

hwif->OUT* = ...
hwif->IN* = ...

hwif->ata_[input,output]_data = ...
hwif->atapi_[input,output]_bytes = ...

Thanks,
Bartlomiej

Re: [RFC][DOC] writing IDE driver guidelines

[Marc Singer]

On Sat, Jun 05, 2004 at 10:36:32PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 05 of June 2004 21:49, Marc Singer wrote:
> > On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > - IDE is going into full host driver model
> > >   so write host driver for your interface
> >
> > Can you tell me what this means?  Is arm/rapide.c an example?  If not,
> > what *is* and example?  Is the arm/ide_arm.c *not* the right way?
> 
> This was supposed to mean to not hide your code in <asm/ide.h> and abuse
> ide_init_default_hwifs() (which is gone).  I'll make this more verbose.
> 
> arm/rapide.c and arm/ide_arm.c are both okay.
> Bad example is <asm-arm/arch-sa1100/ide.h>.
> 
> > > - see drivers in drivers/ide/arm and drivers/ide/legacy for examples
> > >   (especially m68k and arm specific drivers)
> > >
> > > - do not use ide_setup_ports()
> >
> > Hmm.  macide in legacy does.  Is this, then, a bad bad example legacy?
> >
> > The piece I'm missing is how to replace the ioops functions for my
> > platform.  I could implement something in ide-iops.c.  Is that the way?
> 
> No, your host driver should do it:
> 
> If you call ide_register_hw(&hw, &hwif) you should get a valid pointer to
> hwif so you can then replace standard ioops by your custom ones if needed.
> 
> hwif->OUT* = ...
> hwif->IN* = ...
> 
> hwif->ata_[input,output]_data = ...
> hwif->atapi_[input,output]_bytes = ...
> 

OK.  I see how this can work.  What chafes me about ide_arm.c is that
it takes an overly generic view of *all* ARM platforms.  I'd prefer to
see a generic version and the platform specific versions that replace
the generic when appropriate.

Thanks for the guidance.

Re: [RFC][DOC] writing IDE driver guidelines

[Bartlomiej Zolnierkiewicz]

On Sunday 06 of June 2004 00:13, Marc Singer wrote:
> On Sat, Jun 05, 2004 at 10:36:32PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 05 of June 2004 21:49, Marc Singer wrote:
> > > On Sat, May 15, 2004 at 07:23:50PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > - IDE is going into full host driver model
> > > >   so write host driver for your interface
> > >
> > > Can you tell me what this means?  Is arm/rapide.c an example?  If not,
> > > what *is* and example?  Is the arm/ide_arm.c *not* the right way?
> >
> > This was supposed to mean to not hide your code in <asm/ide.h> and abuse
> > ide_init_default_hwifs() (which is gone).  I'll make this more verbose.
> >
> > arm/rapide.c and arm/ide_arm.c are both okay.
> > Bad example is <asm-arm/arch-sa1100/ide.h>.
> >
> > > > - see drivers in drivers/ide/arm and drivers/ide/legacy for examples
> > > >   (especially m68k and arm specific drivers)
> > > >
> > > > - do not use ide_setup_ports()
> > >
> > > Hmm.  macide in legacy does.  Is this, then, a bad bad example legacy?
> > >
> > > The piece I'm missing is how to replace the ioops functions for my
> > > platform.  I could implement something in ide-iops.c.  Is that the way?
> >
> > No, your host driver should do it:
> >
> > If you call ide_register_hw(&hw, &hwif) you should get a valid pointer to
> > hwif so you can then replace standard ioops by your custom ones if
> > needed.
> >
> > hwif->OUT* = ...
> > hwif->IN* = ...
> >
> > hwif->ata_[input,output]_data = ...
> > hwif->atapi_[input,output]_bytes = ...
>
> OK.  I see how this can work.  What chafes me about ide_arm.c is that
> it takes an overly generic view of *all* ARM platforms.  I'd prefer to

ide_arm is only used on A5K, CLPS7500, RPC and SHARK.
See drivers/ide/Kconfig "option IDE_ARM".

It is named ide_arm because I didn't have a better name. :-)

Hm, I see it now, comment on top of the ide_arm.c is a misleading
and needs one/two more lines of explanation.

Thanks.

> see a generic version and the platform specific versions that replace
> the generic when appropriate.
>
> Thanks for the guidance.