On Fri, Sep 14, 2007 at 12:21:14PM +0400, Vitaly Bordug wrote:
> Hello David,
>
> On Fri, 14 Sep 2007 14:09:34 +1000
> David Gibson wrote:
>
> > On Thu, Sep 13, 2007 at 12:16:40PM +0400, Vitaly Bordug wrote:
> > [snip]
> > > > This looks bogus. You're replacing the old crap immr_map() functions,
> > > > which ioremap()ed the registers every time, with a much simpler
> > > > version which uses an established-once mapping of the register
> > > > region. AFAICT, anywah.
> > > >
> > > > So far, so good - but your immr_unmap() still does an iounmap() which
> > > > is surely wrong - it should now be a no-op, leaving the mpc8xx_immr
> > > > mapping intact. You probably get away with it by accident, because I
> > > > imagine attempting to unmap an unaligned chunk of the region will just
> > > > fail.
> > > >
> > >
> > > yes, it should do nop instead of iounmap.
> > > > In fact, with this patch in place, I'd like to see another patch which
> > > > removes all calls to immr_map() and immr_unmap(), simply accessing the
> > > > common mapping directly.
> > > >
> > > Sorry, but originally, that stuff was created to get rid of BSP
> > > ifdefs in drivers. For PQ family, it is a common practice to have
> > > single driver handling all 3 CPU families, which use the same logic,
> > > but immr structure differs a little bit.
> > >
> > > At this point it's clear case-by-case ioremapping does not have firm
> > > benefit, but getting back to the way it was is useless either. In
> > > ideal world, we'd have all those stuff put into dts and have
> > > specific drivers be a shim layer between core hw and IO drivers.
> >
> > Err.. I don't understand what you're getting at. As the code stands
> > after Scott's cleanup, the map() and unmap() calls can certainly be
> > trivially removed, regardless of the history for them.
> >
> I don't argue if they can be removed, but if we aught to do
> that. Direct immr dereference adds plenty of mess into driver
Um... better one line of mess than the 3 lines it is now
(map/access/unmap).
> code. I would like to keep the situation when immr accesses factored
> out as a starting point, rather then turn them back to &immap-> or
> cpm2_immr-> refs.
I can see no advantage to the current "factoring".
> > And, yes, the drivers should certainly uses addresses from the device
> > tree, rather than that revolting structure covering all the inbuilt
> > device retgisters.
> hehe, then you prolly know, that this structure does not fin well
> into device/driver model, either platform_ or of_device. And I am
> going to sort it out at some point...
Well, that's good to know.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
