arm-lh7a40x IDE support in 2.6.6

arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

I was just porting my patches killing <asm/arch/ide.h> for
ARM to 2.6.6 when noticed that more work is needed now. :-(

arch/arm/mach-lh7a40x/ide-lpd7a40x.c
include/asm-arm/arch-lh7a40x/ide.h

Why it couldn't be done in drivers/ide/arm
(as discussed on linux-ide)?

Code from <asm/ide.h> is inlined into IDE core code in far too
many interesting places which greatly increasing complexity/insanity
to anybody trying to understand or change it.

The rule is simple:
	code outside drivers/ide SHOULDN'T need to know about <linux/ide.h>.

WTF everybody wants to be "smart" and abuses it?
[ and then people complain why IDE is so ugly ]

BTW does it even work as IDE polling code is not merged yet?

Bartlomiej

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

This code won't even compile it the current form:
it uses ide_ioreg_t but I killed all users of ide_ioreg_t
and obsoleted <asm/hdreg.h> braindamage.

On Friday 14 of May 2004 18:40, Bartlomiej Zolnierkiewicz wrote:
> I was just porting my patches killing <asm/arch/ide.h> for
> ARM to 2.6.6 when noticed that more work is needed now. :-(
>
> arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> include/asm-arm/arch-lh7a40x/ide.h

<...>

> BTW does it even work as IDE polling code is not merged yet?
>
> Bartlomiej

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Fri, May 14, 2004 at 06:52:25PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> This code won't even compile it the current form:
> it uses ide_ioreg_t but I killed all users of ide_ioreg_t
> and obsoleted <asm/hdreg.h> braindamage.

I'm not suprised that it doesn't work.

Please point me to a righteous example.

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Fri, May 14, 2004 at 06:40:04PM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> I was just porting my patches killing <asm/arch/ide.h> for
> ARM to 2.6.6 when noticed that more work is needed now. :-(
> 
> arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> include/asm-arm/arch-lh7a40x/ide.h
> 
> Why it couldn't be done in drivers/ide/arm
> (as discussed on linux-ide)?

Your response took look enough for me to switch to another job.  I
haven't yet returned to dealing with this.

> Code from <asm/ide.h> is inlined into IDE core code in far too
> many interesting places which greatly increasing complexity/insanity
> to anybody trying to understand or change it.
> 
> The rule is simple:
> 	code outside drivers/ide SHOULDN'T need to know about <linux/ide.h>.

I am emulating what has come before.  All of my examples look like
what I did.

> WTF everybody wants to be "smart" and abuses it?
> [ and then people complain why IDE is so ugly ]

Where's the model?

> BTW does it even work as IDE polling code is not merged yet?

Huh?

aThe solution isn't really that complex.  I need two things.  First, I
must override the register-level access routines in order to do a
little hardware workaround cha-cha-cha.  There's nothing I can do to
fix the hardware.  Second, I need to be able to poll the interface
after initiating a command.  IIRC, you said that I missed at least one
place where this needed to be done.

There are also some very important code (hacks) in

  arch/arm/mach-lh7a40x/ide-lpd7a40x.c

to deal with the fact that I didn't want to touch the SELECT_DRIVE
call from the IDE driver.  The problem was that the SELECT_DRIVE call
in ide-iops.c does this:

  void SELECT_DRIVE (ide_drive_t *drive)
  {
	  if (HWIF(drive)->selectproc)
		  HWIF(drive)->selectproc(drive);
	  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
  }


instead of this:

  void SELECT_DRIVE (ide_drive_t *drive)
  {
	  if (HWIF(drive)->selectproc)
		  HWIF(drive)->selectproc(drive);
	  else
		  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
  }

The OUTB breaks my interface because I don't really have byte-level
access to the resgisters.  So, is selectproc a pre-select procedure or
should it be a substitute?

Anyway, that is what I did in a nutshell.  I plan to get back to this
in a week or so.  Since Russell King already integrated the lh7a40x
code into the kernel, this stuff should be easy to test.

Cheers.

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Friday 14 of May 2004 19:26, Marc Singer wrote:
> On Fri, May 14, 2004 at 06:40:04PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > I was just porting my patches killing <asm/arch/ide.h> for
> > ARM to 2.6.6 when noticed that more work is needed now. :-(
> >
> > arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> > include/asm-arm/arch-lh7a40x/ide.h
> >
> > Why it couldn't be done in drivers/ide/arm
> > (as discussed on linux-ide)?
>
> Your response took look enough for me to switch to another job.  I
> haven't yet returned to dealing with this.

Yes, it took too long.

Anyway, pushing non-working code to mainline is a bad thing
(I can show some proofs for this statement).

> > Code from <asm/ide.h> is inlined into IDE core code in far too
> > many interesting places which greatly increasing complexity/insanity
> > to anybody trying to understand or change it.
> >
> > The rule is simple:
> > 	code outside drivers/ide SHOULDN'T need to know about <linux/ide.h>.
>
> I am emulating what has come before.  All of my examples look like
> what I did.

Please point me to some (working) code outside drivers/ide
which knows i.e. something about 'ide_hwif_t' type.

[ you used 'struct ide_hwif_s' in arch-lh7a40x/ide.h to workaround this 8) ]

> > WTF everybody wants to be "smart" and abuses it?
> > [ and then people complain why IDE is so ugly ]
>
> Where's the model?

drivers/ide/arm/icside.c

My main concerns are:

- ide-lpd7a40x.c should go into drivers/ide/arm/

- you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
  in many places in generic IDE code - anybody wanting to understand
  interactions with your code + generic code will have serious
  problems (especially if knows _nothing_ about lpd7a40x)

- hwif->mmio is set to 2 but resource handling is missed

- ide_init_default_hwifs() is OBSOLETE and shouldn't be used

> > BTW does it even work as IDE polling code is not merged yet?
>
> Huh?
>
> aThe solution isn't really that complex.  I need two things.  First, I
> must override the register-level access routines in order to do a
> little hardware workaround cha-cha-cha.  There's nothing I can do to
> fix the hardware.  Second, I need to be able to poll the interface
> after initiating a command.  IIRC, you said that I missed at least one
> place where this needed to be done.

Yes, idea isn't complex but spotting all places needing change is.

> There are also some very important code (hacks) in
>
>   arch/arm/mach-lh7a40x/ide-lpd7a40x.c
>
> to deal with the fact that I didn't want to touch the SELECT_DRIVE
> call from the IDE driver.  The problem was that the SELECT_DRIVE call
> in ide-iops.c does this:
>
>   void SELECT_DRIVE (ide_drive_t *drive)
>   {
> 	  if (HWIF(drive)->selectproc)
> 		  HWIF(drive)->selectproc(drive);
> 	  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
>   }
>
>
> instead of this:
>
>   void SELECT_DRIVE (ide_drive_t *drive)
>   {
> 	  if (HWIF(drive)->selectproc)
> 		  HWIF(drive)->selectproc(drive);
> 	  else
> 		  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
>   }
>
> The OUTB breaks my interface because I don't really have byte-level
> access to the resgisters.  So, is selectproc a pre-select procedure or
> should it be a substitute?

pre-select but you can change it to be substitute if you need
(just remember to update all users if you decide to do this)

IMO it is better to fix it correctly than to do hacks like this
in lpd7a40x_ide_outb() (which is minor performance hit btw)

> Anyway, that is what I did in a nutshell.  I plan to get back to this
> in a week or so.  Since Russell King already integrated the lh7a40x
> code into the kernel, this stuff should be easy to test.

That's what I'm talking about - it shouldn't have been integrated. :-)

Cheers.

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Fri, May 14, 2004 at 08:45:51PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday 14 of May 2004 19:26, Marc Singer wrote:
> > On Fri, May 14, 2004 at 06:40:04PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > I was just porting my patches killing <asm/arch/ide.h> for
> > > ARM to 2.6.6 when noticed that more work is needed now. :-(
> > >
> > > arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> > > include/asm-arm/arch-lh7a40x/ide.h
> > >
> > > Why it couldn't be done in drivers/ide/arm
> > > (as discussed on linux-ide)?
> >
> > Your response took look enough for me to switch to another job.  I
> > haven't yet returned to dealing with this.
> 
> Yes, it took too long.
> 
> Anyway, pushing non-working code to mainline is a bad thing
> (I can show some proofs for this statement).

Oh, no don't do that!

> > > Code from <asm/ide.h> is inlined into IDE core code in far too
> > > many interesting places which greatly increasing complexity/insanity
> > > to anybody trying to understand or change it.
> > >
> > > The rule is simple:
> > > 	code outside drivers/ide SHOULDN'T need to know about <linux/ide.h>.
> >
> > I am emulating what has come before.  All of my examples look like
> > what I did.
> 
> Please point me to some (working) code outside drivers/ide
> which knows i.e. something about 'ide_hwif_t' type.

My code works fine in 2.6.4.  QED.

> [ you used 'struct ide_hwif_s' in arch-lh7a40x/ide.h to workaround this 8) ]

Copied from elsewhere.  

Listen.  It is not my intention to be clever.  All I want to do is get
things working and to not break other people's code.  I'm certain that
most people are working with the same assumptions.  Aside from some
naive snippets I've see promulgated, the bulk of the kernel work I've
see is sane given limited information.  Certainly, once one
understands a sybsystem completely then the quality rises.  I hope
you'll admit that the IDE code is overly intricate.
  
> > Where's the model?
> 
> drivers/ide/arm/icside.c

Good.  I'll look at it.

> 
> My main concerns are:
> 
> - ide-lpd7a40x.c should go into drivers/ide/arm/

Easy to do.

> - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
>   in many places in generic IDE code - anybody wanting to understand
>   interactions with your code + generic code will have serious
>   problems (especially if knows _nothing_ about lpd7a40x)

I don't know what you mean.  I grep for that constant and found it
nowhere except for ide-io.c and in my code.  It doesn't take much to
find the references.

What is it that you want changed?
 
> - hwif->mmio is set to 2 but resource handling is missed

Can you be more specific?  Did you notice the comment?  I didn't know
what it was for, but I set it because I thought that that was the
right way to go.

Are you talking about reserving the address space?  At the moment,
this is done in the arch setup.  I can certainly move it to the IDE
driver.

> - ide_init_default_hwifs() is OBSOLETE and shouldn't be used

I expect that your model will show me the way.

> > > BTW does it even work as IDE polling code is not merged yet?
> >
> > Huh?
> >
> > aThe solution isn't really that complex.  I need two things.  First, I
> > must override the register-level access routines in order to do a
> > little hardware workaround cha-cha-cha.  There's nothing I can do to
> > fix the hardware.  Second, I need to be able to poll the interface
> > after initiating a command.  IIRC, you said that I missed at least one
> > place where this needed to be done.
> 
> Yes, idea isn't complex but spotting all places needing change is.

That's what you are for.  %^)

> > There are also some very important code (hacks) in
> >
> >   arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> >
> > to deal with the fact that I didn't want to touch the SELECT_DRIVE
> > call from the IDE driver.  The problem was that the SELECT_DRIVE call
> > in ide-iops.c does this:
> >
> >   void SELECT_DRIVE (ide_drive_t *drive)
> >   {
> > 	  if (HWIF(drive)->selectproc)
> > 		  HWIF(drive)->selectproc(drive);
> > 	  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
> >   }
> >
> >
> > instead of this:
> >
> >   void SELECT_DRIVE (ide_drive_t *drive)
> >   {
> > 	  if (HWIF(drive)->selectproc)
> > 		  HWIF(drive)->selectproc(drive);
> > 	  else
> > 		  HWIF(drive)->OUTB(drive->select.all, IDE_SELECT_REG);
> >   }
> >
> > The OUTB breaks my interface because I don't really have byte-level
> > access to the resgisters.  So, is selectproc a pre-select procedure or
> > should it be a substitute?
> 
> pre-select but you can change it to be substitute if you need
> (just remember to update all users if you decide to do this)

I'll have to search the kernel to see what uses it.  Maybe the better
way would be to define a new select proc that *is* a substitute.


> IMO it is better to fix it correctly than to do hacks like this
> in lpd7a40x_ide_outb() (which is minor performance hit btw)

Of course.

> > Anyway, that is what I did in a nutshell.  I plan to get back to this
> > in a week or so.  Since Russell King already integrated the lh7a40x
> > code into the kernel, this stuff should be easy to test.
> 
> That's what I'm talking about - it shouldn't have been integrated. :-)

I think we are talking about two things here.  I agree that the
ide-lpd7a400 code should not have been integrated.  However, the rest
of the core code is fine.  It is that core code that is necessary to
make the test possible.

Cheers.

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Friday 14 of May 2004 21:47, Marc Singer wrote:

> > [ you used 'struct ide_hwif_s' in arch-lh7a40x/ide.h to workaround this
> > 8) ]

struct hwif_s actually

> Copied from elsewhere.

superio.h, superio.c

EEK, added to TODO

> Listen.  It is not my intention to be clever.  All I want to do is get
> things working and to not break other people's code.  I'm certain that
> most people are working with the same assumptions.  Aside from some
> naive snippets I've see promulgated, the bulk of the kernel work I've
> see is sane given limited information.  Certainly, once one
> understands a sybsystem completely then the quality rises.  I hope
> you'll admit that the IDE code is overly intricate.

With changes like this nobody will ever be able to understand
IDE subsystem completely. ;-)

> > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
> >   in many places in generic IDE code - anybody wanting to understand
> >   interactions with your code + generic code will have serious
> >   problems (especially if knows _nothing_ about lpd7a40x)
>
> I don't know what you mean.  I grep for that constant and found it
> nowhere except for ide-io.c and in my code.  It doesn't take much to
> find the references.

I'm talking about ide_init_hwif_ports() function.

> What is it that you want changed?
>
> > - hwif->mmio is set to 2 but resource handling is missed
>
> Can you be more specific?  Did you notice the comment?  I didn't know
> what it was for, but I set it because I thought that that was the
> right way to go.
>
> Are you talking about reserving the address space?  At the moment,

Yes.

> this is done in the arch setup.  I can certainly move it to the IDE
> driver.

OK

> > > The OUTB breaks my interface because I don't really have byte-level
> > > access to the resgisters.  So, is selectproc a pre-select procedure or
> > > should it be a substitute?
> >
> > pre-select but you can change it to be substitute if you need
> > (just remember to update all users if you decide to do this)
>
> I'll have to search the kernel to see what uses it.  Maybe the better
> way would be to define a new select proc that *is* a substitute.

Nope.

> > IMO it is better to fix it correctly than to do hacks like this
> > in lpd7a40x_ide_outb() (which is minor performance hit btw)
>
> Of course.
>
> > > Anyway, that is what I did in a nutshell.  I plan to get back to this
> > > in a week or so.  Since Russell King already integrated the lh7a40x
> > > code into the kernel, this stuff should be easy to test.
> >
> > That's what I'm talking about - it shouldn't have been integrated. :-)
>
> I think we are talking about two things here.  I agree that the
> ide-lpd7a400 code should not have been integrated.  However, the rest

OK

> of the core code is fine.  It is that core code that is necessary to
> make the test possible.

Stuff in arch-lh7a40x/ide.h is really a driver code but
abuses subsystem code instead - that's my complain.

OK, lets move things forward and just fix it.

Cheers.

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Fri, May 14, 2004 at 11:23:19PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday 14 of May 2004 21:47, Marc Singer wrote:
> 
> > > [ you used 'struct ide_hwif_s' in arch-lh7a40x/ide.h to workaround this
> > > 8) ]
> 
> struct hwif_s actually
> 
> > Copied from elsewhere.
> 
> superio.h, superio.c

I don't think I ever looked at that code.

> EEK, added to TODO
> 
> > Listen.  It is not my intention to be clever.  All I want to do is get
> > things working and to not break other people's code.  I'm certain that
> > most people are working with the same assumptions.  Aside from some
> > naive snippets I've see promulgated, the bulk of the kernel work I've
> > see is sane given limited information.  Certainly, once one
> > understands a sybsystem completely then the quality rises.  I hope
> > you'll admit that the IDE code is overly intricate.
> 
> With changes like this nobody will ever be able to understand
> IDE subsystem completely. ;-)

I get the feeling that you're blaming me and others for making the IDE
code a mess.  Might I suggest that this isn't a very productive tack?
The most helpful thing to do is to a) provide best-practice examples,
and b) to include some documentation.  I'm not talking about anything
extensive, but statements like

  "All references to linux/ide.h must reside in the ide tree."

Are pretty darn helpful. 

> > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
> > >   in many places in generic IDE code - anybody wanting to understand
> > >   interactions with your code + generic code will have serious
> > >   problems (especially if knows _nothing_ about lpd7a40x)
> >
> > I don't know what you mean.  I grep for that constant and found it
> > nowhere except for ide-io.c and in my code.  It doesn't take much to
> > find the references.
> 
> I'm talking about ide_init_hwif_ports() function.

Most of the ARM arch's use it.  Perhaps all of them need a good once
over.

> > What is it that you want changed?
> >
> > > - hwif->mmio is set to 2 but resource handling is missed
> >
> > Can you be more specific?  Did you notice the comment?  I didn't know
> > what it was for, but I set it because I thought that that was the
> > right way to go.
> >
> > Are you talking about reserving the address space?  At the moment,
> 
> Yes.

Easily done.

> > this is done in the arch setup.  I can certainly move it to the IDE
> > driver.
> 
> OK
> 
> > > > The OUTB breaks my interface because I don't really have byte-level
> > > > access to the resgisters.  So, is selectproc a pre-select procedure or
> > > > should it be a substitute?
> > >
> > > pre-select but you can change it to be substitute if you need
> > > (just remember to update all users if you decide to do this)
> >
> > I'll have to search the kernel to see what uses it.  Maybe the better
> > way would be to define a new select proc that *is* a substitute.
> 
> Nope.

So then we break anyone who is using the selectproc as a pre-select
proc?  I don't understand what you mean here.  There are several
drivers that use this function.  How do you propose that we provide
both types of behavior with one entry point?

> > of the core code is fine.  It is that core code that is necessary to
> > make the test possible.
> 
> Stuff in arch-lh7a40x/ide.h is really a driver code but
> abuses subsystem code instead - that's my complain.

Right.  We agree.  I am talking about the core code.  Not the ide
code.  The core is what supports the CPU.

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Friday 14 of May 2004 23:33, Marc Singer wrote:

> > > Listen.  It is not my intention to be clever.  All I want to do is get
> > > things working and to not break other people's code.  I'm certain that
> > > most people are working with the same assumptions.  Aside from some
> > > naive snippets I've see promulgated, the bulk of the kernel work I've
> > > see is sane given limited information.  Certainly, once one
> > > understands a sybsystem completely then the quality rises.  I hope
> > > you'll admit that the IDE code is overly intricate.
> >
> > With changes like this nobody will ever be able to understand
> > IDE subsystem completely. ;-)
>
> I get the feeling that you're blaming me and others for making the IDE
> code a mess.  Might I suggest that this isn't a very productive tack?

Yep, sorry.

> The most helpful thing to do is to a) provide best-practice examples,
> and b) to include some documentation.  I'm not talking about anything
> extensive, but statements like
>
>   "All references to linux/ide.h must reside in the ide tree."
>
> Are pretty darn helpful.

OK, will do (this can take a while).  Can I ask you to review it later?
I think your comments will be very valuable.

> > > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
> > > >   in many places in generic IDE code - anybody wanting to understand
> > > >   interactions with your code + generic code will have serious
> > > >   problems (especially if knows _nothing_ about lpd7a40x)
> > >
> > > I don't know what you mean.  I grep for that constant and found it
> > > nowhere except for ide-io.c and in my code.  It doesn't take much to
> > > find the references.
> >
> > I'm talking about ide_init_hwif_ports() function.
>
> Most of the ARM arch's use it.  Perhaps all of them need a good once
> over.

Since some time I have a patch killing <asm/arch-*/ide.h>. :)

> > > > > The OUTB breaks my interface because I don't really have byte-level
> > > > > access to the resgisters.  So, is selectproc a pre-select procedure
> > > > > or should it be a substitute?
> > > >
> > > > pre-select but you can change it to be substitute if you need
> > > > (just remember to update all users if you decide to do this)
> > >
> > > I'll have to search the kernel to see what uses it.  Maybe the better
> > > way would be to define a new select proc that *is* a substitute.
> >
> > Nope.
>
> So then we break anyone who is using the selectproc as a pre-select
> proc?  I don't understand what you mean here.  There are several
> drivers that use this function.  How do you propose that we provide
> both types of behavior with one entry point?

You can add last line of SELECT_DRIVE() to all ->selectproc()
implementations and add 'else' to SELECT_DRIVE().

> > > of the core code is fine.  It is that core code that is necessary to
> > > make the test possible.
> >
> > Stuff in arch-lh7a40x/ide.h is really a driver code but
> > abuses subsystem code instead - that's my complain.
>
> Right.  We agree.  I am talking about the core code.  Not the ide
> code.  The core is what supports the CPU.

Good.

Cheers,
Bartlomiej

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Sat, May 15, 2004 at 12:19:46AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > The most helpful thing to do is to a) provide best-practice examples,
> > and b) to include some documentation.  I'm not talking about anything
> > extensive, but statements like
> >
> >   "All references to linux/ide.h must reside in the ide tree."
> >
> > Are pretty darn helpful.
> 
> OK, will do (this can take a while).  Can I ask you to review it later?
> I think your comments will be very valuable.

It would be a pleasure.

> > > > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is used
> > > > >   in many places in generic IDE code - anybody wanting to understand
> > > > >   interactions with your code + generic code will have serious
> > > > >   problems (especially if knows _nothing_ about lpd7a40x)
> > > >
> > > > I don't know what you mean.  I grep for that constant and found it
> > > > nowhere except for ide-io.c and in my code.  It doesn't take much to
> > > > find the references.
> > >
> > > I'm talking about ide_init_hwif_ports() function.
> >
> > Most of the ARM arch's use it.  Perhaps all of them need a good once
> > over.
> 
> Since some time I have a patch killing <asm/arch-*/ide.h>. :)

OK.  That raises an interesting question.  If a) you as the IDE
maintainer want to make a policy change, and b) you have a concrete
action to take, then how do you go about it so that the right thing
(tm) happens?

One tack would be to post to the ARM list stating that there is
such-and-such, a new policy, and this requires a change to the
way-things-work (tm).  Then effect a patch that breaks the bad stuff
so that the users of such bad stuff must cope.

For example, the patch could edit each of the arm ide files so that
they don't compile when IDE is enabled.  This wouldn't clobber the
files as much as make them useless until the such-and-such policy is
followed.

> > So then we break anyone who is using the selectproc as a pre-select
> > proc?  I don't understand what you mean here.  There are several
> > drivers that use this function.  How do you propose that we provide
> > both types of behavior with one entry point?
> 
> You can add last line of SELECT_DRIVE() to all ->selectproc()
> implementations and add 'else' to SELECT_DRIVE().

Ah.  I see where you're going.  I can do that.  I cannot test it, but
I can do it.

BTW, thanks for you help and input.

Cheers.

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Saturday 15 of May 2004 00:49, Marc Singer wrote:
> On Sat, May 15, 2004 at 12:19:46AM +0200, Bartlomiej Zolnierkiewicz wrote:

> > > > > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is
> > > > > > used in many places in generic IDE code - anybody wanting to
> > > > > > understand interactions with your code + generic code will have
> > > > > > serious problems (especially if knows _nothing_ about lpd7a40x)
> > > > >
> > > > > I don't know what you mean.  I grep for that constant and found it
> > > > > nowhere except for ide-io.c and in my code.  It doesn't take much
> > > > > to find the references.
> > > >
> > > > I'm talking about ide_init_hwif_ports() function.
> > >
> > > Most of the ARM arch's use it.  Perhaps all of them need a good once
> > > over.
> >
> > Since some time I have a patch killing <asm/arch-*/ide.h>. :)
>
> OK.  That raises an interesting question.  If a) you as the IDE
> maintainer want to make a policy change, and b) you have a concrete
> action to take, then how do you go about it so that the right thing
> (tm) happens?

I posted patch to linux-arm-kernel (rmk, I can't find it in l-a-k archives
and I also can't find any mail about it being rejected?) and linux-ide.
[ and to affected arch maintainers of course ]

> One tack would be to post to the ARM list stating that there is
> such-and-such, a new policy, and this requires a change to the
> way-things-work (tm).  Then effect a patch that breaks the bad stuff
> so that the users of such bad stuff must cope.

It is stable kernel series so I paid 'stable kernel' price
and made sure that it shouldn't break anything.

Cheers,
Bartlomiej

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Sat, May 15, 2004 at 01:26:33AM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Saturday 15 of May 2004 00:49, Marc Singer wrote:
> > On Sat, May 15, 2004 at 12:19:46AM +0200, Bartlomiej Zolnierkiewicz wrote:
> 
> > > > > > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which is
> > > > > > > used in many places in generic IDE code - anybody wanting to
> > > > > > > understand interactions with your code + generic code will have
> > > > > > > serious problems (especially if knows _nothing_ about lpd7a40x)
> > > > > >
> > > > > > I don't know what you mean.  I grep for that constant and found it
> > > > > > nowhere except for ide-io.c and in my code.  It doesn't take much
> > > > > > to find the references.
> > > > >
> > > > > I'm talking about ide_init_hwif_ports() function.
> > > >
> > > > Most of the ARM arch's use it.  Perhaps all of them need a good once
> > > > over.
> > >
> > > Since some time I have a patch killing <asm/arch-*/ide.h>. :)
> >
> > OK.  That raises an interesting question.  If a) you as the IDE
> > maintainer want to make a policy change, and b) you have a concrete
> > action to take, then how do you go about it so that the right thing
> > (tm) happens?
> 
> I posted patch to linux-arm-kernel (rmk, I can't find it in l-a-k archives
> and I also can't find any mail about it being rejected?) and linux-ide.
> [ and to affected arch maintainers of course ]

There is a web page for posting kernel patches.  It is most likely to
receive attention than posting to the mailing list.

  <http://www.arm.linux.org.uk/developer/patches/>

> > One tack would be to post to the ARM list stating that there is
> > such-and-such, a new policy, and this requires a change to the
> > way-things-work (tm).  Then effect a patch that breaks the bad stuff
> > so that the users of such bad stuff must cope.
> 
> It is stable kernel series so I paid 'stable kernel' price
> and made sure that it shouldn't break anything.

Fair enough.  We'll start with lh and see if we can get others to
follow.

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Saturday 15 of May 2004 02:10, Marc Singer wrote:
> On Sat, May 15, 2004 at 01:26:33AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Saturday 15 of May 2004 00:49, Marc Singer wrote:
> > > On Sat, May 15, 2004 at 12:19:46AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > > > > > - you are setting IDE_NO_IRQ in ide_init_hwif_ports() which
> > > > > > > > is used in many places in generic IDE code - anybody wanting
> > > > > > > > to understand interactions with your code + generic code will
> > > > > > > > have serious problems (especially if knows _nothing_ about
> > > > > > > > lpd7a40x)
> > > > > > >
> > > > > > > I don't know what you mean.  I grep for that constant and found
> > > > > > > it nowhere except for ide-io.c and in my code.  It doesn't take
> > > > > > > much to find the references.
> > > > > >
> > > > > > I'm talking about ide_init_hwif_ports() function.
> > > > >
> > > > > Most of the ARM arch's use it.  Perhaps all of them need a good
> > > > > once over.
> > > >
> > > > Since some time I have a patch killing <asm/arch-*/ide.h>. :)
> > >
> > > OK.  That raises an interesting question.  If a) you as the IDE
> > > maintainer want to make a policy change, and b) you have a concrete
> > > action to take, then how do you go about it so that the right thing
> > > (tm) happens?
> >
> > I posted patch to linux-arm-kernel (rmk, I can't find it in l-a-k
> > archives and I also can't find any mail about it being rejected?) and
> > linux-ide. [ and to affected arch maintainers of course ]
>
> There is a web page for posting kernel patches.  It is most likely to
> receive attention than posting to the mailing list.
>
>   <http://www.arm.linux.org.uk/developer/patches/>

It is for -rmk tree.

> > > One tack would be to post to the ARM list stating that there is
> > > such-and-such, a new policy, and this requires a change to the
> > > way-things-work (tm).  Then effect a patch that breaks the bad stuff
> > > so that the users of such bad stuff must cope.
> >
> > It is stable kernel series so I paid 'stable kernel' price
> > and made sure that it shouldn't break anything.
>
> Fair enough.  We'll start with lh and see if we can get others to
> follow.

lh is non-buildable in the mainline so I shouldn't care about it
(from kill ide_init_hwif_ports() patch standpoint).

We will change mainline and others will follow. 8)

Re: arm-lh7a40x IDE support in 2.6.6

[Marc Singer]

On Sat, May 15, 2004 at 02:25:28AM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > I posted patch to linux-arm-kernel (rmk, I can't find it in l-a-k
> > > archives and I also can't find any mail about it being rejected?) and
> > > linux-ide. [ and to affected arch maintainers of course ]
> >
> > There is a web page for posting kernel patches.  It is most likely to
> > receive attention than posting to the mailing list.
> >
> >   <http://www.arm.linux.org.uk/developer/patches/>
> 
> It is for -rmk tree.

Right.  That web page is for arm patches.  For the -rmk tree.  Instead
of posting patches to linux-arm-kernel, you can post them to that
page.

Re: arm-lh7a40x IDE support in 2.6.6

[Russell King]

On Fri, May 14, 2004 at 08:45:51PM +0200, Bartlomiej Zolnierkiewicz wrote:
> On Friday 14 of May 2004 19:26, Marc Singer wrote:
> > On Fri, May 14, 2004 at 06:40:04PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > I was just porting my patches killing <asm/arch/ide.h> for
> > > ARM to 2.6.6 when noticed that more work is needed now. :-(
> > >
> > > arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> > > include/asm-arm/arch-lh7a40x/ide.h
> > >
> > > Why it couldn't be done in drivers/ide/arm
> > > (as discussed on linux-ide)?
> >
> > Your response took look enough for me to switch to another job.  I
> > haven't yet returned to dealing with this.
> 
> Yes, it took too long.
> 
> Anyway, pushing non-working code to mainline is a bad thing
> (I can show some proofs for this statement).

It was a necessary step.  To get around this, we're going to ask people
to submit new machine support on a file by file basis, and that's just
not practical, and you can't expect me to be able to track _every_
_single_ fscking change to the kernel, and pick up every one in a
review.

There will always be a delay between changes happening between two people
and we have to live with it.  Not everyone works on Linus' latest kernel.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: arm-lh7a40x IDE support in 2.6.6

[Bartlomiej Zolnierkiewicz]

On Friday 14 of May 2004 21:52, Russell King wrote:
> On Fri, May 14, 2004 at 08:45:51PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Friday 14 of May 2004 19:26, Marc Singer wrote:
> > > On Fri, May 14, 2004 at 06:40:04PM +0200, Bartlomiej Zolnierkiewicz wrote:
> > > > I was just porting my patches killing <asm/arch/ide.h> for
> > > > ARM to 2.6.6 when noticed that more work is needed now. :-(
> > > >
> > > > arch/arm/mach-lh7a40x/ide-lpd7a40x.c
> > > > include/asm-arm/arch-lh7a40x/ide.h
> > > >
> > > > Why it couldn't be done in drivers/ide/arm
> > > > (as discussed on linux-ide)?
> > >
> > > Your response took look enough for me to switch to another job.  I
> > > haven't yet returned to dealing with this.
> >
> > Yes, it took too long.
> >
> > Anyway, pushing non-working code to mainline is a bad thing
> > (I can show some proofs for this statement).
>
> It was a necessary step.  To get around this, we're going to ask people
> to submit new machine support on a file by file basis, and that's just
> not practical, and you can't expect me to be able to track _every_
> _single_ fscking change to the kernel, and pick up every one in a
> review.

Nobody expects this but expecting people to try building they patches
against latest & greatest kernel before pushing to Linus is sane & practical.
-> ask people to submit patches which are buildable.

> There will always be a delay between changes happening between two people
> and we have to live with it.  Not everyone works on Linus' latest kernel.