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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/