Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency

[Tony Lindgren <tony@atomide.com>]

* Felipe Balbi <balbi@ti.com> [130214 10:00]:
> Hi,
> 
> On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
> > * Felipe Balbi <balbi@ti.com> [130214 03:19]:
> > > Currently the omap-serial driver will not
> > > work properly if booted via DT with CPUIDLE
> > > enabled because it depends on function pointers
> > > provided by hwmod to change its own SYSCONFIG
> > > register.
> > > 
> > > Remove that relyance on hwmod by moving SYSCONFIG
> > > handling to driver itself. Note that this also
> > > fixes a possible corner case bug where we could
> > > be putting UART in Force Idle mode if we called
> > > omap_serial_enable_wakeup(up, false) after setting
> > > NOIDLE to the idle mode. This is because hwmod
> > > has no protection against that situation.
> 
> Any comments to these last two sentences ?

Well I think all the driver handling should be done in
the driver since we now have runtime PM.
 
> > I agree the sysconfig stuff should be handled in the drivers.
> > But considering that we need to also reset or idle hardware
> > that may not have a driver loaded, it's best to put the
> > reset/idle function in to the driver specific header as
> > an inline function.
> 
> that is likely to cause quite a few isues, don't you think?
> 
> hwmod depending on driver code to reset particular IPs ? Seems like we
> need a generic device_reset() hook which can be called by platform code,
> no ? Not sure if that would help a lot though.
> 
> > This way the hwmod code can idle or reset the unclaimed
> > hardware in a late_initcall.
> 
> The problem with this:
> 
> my_driver.h:
> 
> static inline void my_driver_reset(struct my_driver *drv)
> {
> 	writel(drv->regs, SYSS, RESET);
> }
> 
> how will hwmod provide drv pointer ? Even if we use void __iomem * it
> would mean that hwmod would have to a) access driver's ioremaped area or
> b) ioremap() the same area again.

Well the ioremapping should only be done in the driver ideally.
Currently we have a mess of both hwmod and driver doing the
ioremapping.

So in the long run, let's assume only the driver ioremaps for
most of the cases where there is a driver available.

And only in the case there is no driver, hwmod can parse the address
space from DT for the unclaimed hardware in the late_initcall.

So the shared inline function should just take the __iomem * and
size instead of *drv so both the driver and hwmod code can tinker
with the autoidle bit.
 
> Note sure if any of those are acceptable.

Hmm what issues do you see in the above suggestion?
 
> > And probably the handling of sysconfig bits should be
> > done using a common header in include/linux/omap-hwmod.h
> > or something so it can be shared between drivers.
> 
> That could be done, though I think the header name could be different.
> Maybe it could match the document which define those registers, let me
> just check if we're allowed to use that publicly :-p

Cool yeah I have no preference where that header should be. Maybe
something bus specific like include/linux/bus/omap-hwmod.h?

Regards,

Tony