[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[Felipe Balbi]

Eventually, we need to be able to remove
ti,hwmods DT attribute (or at a minimum
ignore it).

For new platforms, this patch could enable
the transition by not relying on ti,hwmods
to have functioning PM and Idle implementation.

Notice that this poses no differences for
platforms which are already supported, it
just gives us means of dropping the relyance
on hwmod for new platforms.

NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 arch/arm/mach-omap2/omap_device.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index e065daa..305eeb4 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -796,13 +796,18 @@ static int __init omap_early_device_register(struct platform_device *pdev)
 static int _od_runtime_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
 	int ret;
 
 	ret = pm_generic_runtime_suspend(dev);
 
+	if (!od)
+		goto out;
+
 	if (!ret)
 		omap_device_idle(pdev);
 
+out:
 	return ret;
 }
 
@@ -814,9 +819,14 @@ static int _od_runtime_idle(struct device *dev)
 static int _od_runtime_resume(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
+	struct omap_device *od = to_omap_device(pdev);
+
+	if (!od)
+		goto out;
 
 	omap_device_enable(pdev);
 
+out:
 	return pm_generic_runtime_resume(dev);
 }
 #endif
@@ -828,6 +838,9 @@ static int _od_suspend_noirq(struct device *dev)
 	struct omap_device *od = to_omap_device(pdev);
 	int ret;
 
+	if (!od)
+		return pm_generic_suspend_noirq(dev);
+
 	/* Don't attempt late suspend on a driver that is not bound */
 	if (od->_driver_status != BUS_NOTIFY_BOUND_DRIVER)
 		return 0;
@@ -850,6 +863,9 @@ static int _od_resume_noirq(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct omap_device *od = to_omap_device(pdev);
 
+	if (!od)
+		goto out;
+
 	if ((od->flags & OMAP_DEVICE_SUSPENDED) &&
 	    !pm_runtime_status_suspended(dev)) {
 		od->flags &= ~OMAP_DEVICE_SUSPENDED;
@@ -858,6 +874,7 @@ static int _od_resume_noirq(struct device *dev)
 		pm_generic_runtime_resume(dev);
 	}
 
+out:
 	return pm_generic_resume_noirq(dev);
 }
 #else
-- 
1.8.1.rc1.5.g7e0651a

[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[Russell King - ARM Linux]

On Thu, Feb 14, 2013 at 01:15:52PM +0200, Felipe Balbi wrote:
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *od = to_omap_device(pdev);
>  	int ret;
>  
>  	ret = pm_generic_runtime_suspend(dev);
>  
> +	if (!od)
> +		goto out;
> +
>  	if (!ret)

What's wrong with the single line change per function of:

-	if (!ret)
+	if (!ret && to_omap_device(pdev))

?

>  		omap_device_idle(pdev);
>  
> +out:
>  	return ret;
>  }

[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[Felipe Balbi]

On Thu, Feb 14, 2013 at 11:20:43AM +0000, Russell King - ARM Linux wrote:
> On Thu, Feb 14, 2013 at 01:15:52PM +0200, Felipe Balbi wrote:
> >  	struct platform_device *pdev = to_platform_device(dev);
> > +	struct omap_device *od = to_omap_device(pdev);
> >  	int ret;
> >  
> >  	ret = pm_generic_runtime_suspend(dev);
> >  
> > +	if (!od)
> > +		goto out;
> > +
> >  	if (!ret)
> 
> What's wrong with the single line change per function of:
> 
> -	if (!ret)
> +	if (!ret && to_omap_device(pdev))
> 
> ?

nothing, will change. Let me just check if the other folks are ok with
the patchset.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130214/7bc41cb4/attachment.sig>

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

[Tony Lindgren]

* 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.

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.

This way the hwmod code can idle or reset the unclaimed
hardware in a late_initcall.

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.

Regards,

Tony
 
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  drivers/tty/serial/omap-serial.c | 67 +++++++++++++++++++---------------------
>  1 file changed, 32 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 57d6b29..078beb1 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -96,6 +96,18 @@
>  
>  #define OMAP_UART_TCR_TRIG	0x0F
>  
> +#define OMAP_UART_SYSC		0x54
> +#define OMAP_UART_AUTOIDLE	(1 << 0)
> +#define OMAP_UART_SOFTRESET	(1 << 1)
> +#define OMAP_UART_ENAWAKEUP	(1 << 2)
> +
> +#define OMAP_UART_IDLEMODE(n)	(((n) & 3) << 3)
> +#define OMAP_UART_FORCE_IDLE	OMAP_UART_IDLEMODE(0)
> +#define OMAP_UART_NO_IDLE	OMAP_UART_IDLEMODE(1)
> +#define OMAP_UART_SMART_IDLE	OMAP_UART_IDLEMODE(2)
> +#define OMAP_UART_SMART_IDLE_WKUP OMAP_UART_IDLEMODE(3)
> +#define OMAP_UART_IDLEMODE_MASK	OMAP_UART_SMART_IDLE_WKUP
> +
>  struct uart_omap_dma {
>  	u8			uart_dma_tx;
>  	u8			uart_dma_rx;
> @@ -191,44 +203,39 @@ static inline void serial_omap_clear_fifos(struct uart_omap_port *up)
>  	serial_out(up, UART_FCR, 0);
>  }
>  
> -static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
> -{
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->get_context_loss_count)
> -		return 0;
> -
> -	return pdata->get_context_loss_count(up->dev);
> -}
> -
>  static void serial_omap_set_forceidle(struct uart_omap_port *up)
>  {
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> -
> -	if (!pdata || !pdata->set_forceidle)
> -		return;
> +	u32 reg;
>  
> -	pdata->set_forceidle(up->dev);
> +	reg = serial_in(up, OMAP_UART_SYSC);
> +	reg &= ~OMAP_UART_IDLEMODE_MASK;
> +	reg |= OMAP_UART_FORCE_IDLE;
> +	serial_out(up, OMAP_UART_SYSC, reg);
>  }
>  
>  static void serial_omap_set_noidle(struct uart_omap_port *up)
>  {
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> +	u32 reg;
>  
> -	if (!pdata || !pdata->set_noidle)
> -		return;
> -
> -	pdata->set_noidle(up->dev);
> +	reg = serial_in(up, OMAP_UART_SYSC);
> +	reg &= ~OMAP_UART_IDLEMODE_MASK;
> +	reg |= OMAP_UART_NO_IDLE;
> +	serial_out(up, OMAP_UART_SYSC, reg);
>  }
>  
>  static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
>  {
> -	struct omap_uart_port_info *pdata = up->dev->platform_data;
> +	u32 reg;
>  
> -	if (!pdata || !pdata->enable_wakeup)
> -		return;
> +	reg = serial_in(up, OMAP_UART_SYSC);
> +	reg &= ~OMAP_UART_IDLEMODE_MASK;
> +
> +	if (enable)
> +		reg |= OMAP_UART_SMART_IDLE_WKUP;
> +	else
> +		reg |= OMAP_UART_SMART_IDLE;
>  
> -	pdata->enable_wakeup(up->dev, enable);
> +	serial_out(up, OMAP_UART_SYSC, reg);
>  }
>  
>  /*
> @@ -1596,8 +1603,6 @@ static int serial_omap_runtime_suspend(struct device *dev)
>  	if (!pdata)
>  		return 0;
>  
> -	up->context_loss_cnt = serial_omap_get_context_loss_count(up);
> -
>  	if (device_may_wakeup(dev)) {
>  		if (!up->wakeups_enabled) {
>  			serial_omap_enable_wakeup(up, true);
> @@ -1620,15 +1625,7 @@ static int serial_omap_runtime_resume(struct device *dev)
>  {
>  	struct uart_omap_port *up = dev_get_drvdata(dev);
>  
> -	int loss_cnt = serial_omap_get_context_loss_count(up);
> -
> -	if (loss_cnt < 0) {
> -		dev_err(dev, "serial_omap_get_context_loss_count failed : %d\n",
> -			loss_cnt);
> -		serial_omap_restore_context(up);
> -	} else if (up->context_loss_cnt != loss_cnt) {
> -		serial_omap_restore_context(up);
> -	}
> +	serial_omap_restore_context(up);
>  	up->latency = up->calc_latency;
>  	schedule_work(&up->qos_work);
>  
> -- 
> 1.8.1.rc1.5.g7e0651a
> 

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

[Felipe Balbi]

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 ?

> 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.

Note sure if any of those are acceptable.

> 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

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130214/64fab5bb/attachment-0001.sig>

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

[Tony Lindgren]

* 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

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

[Felipe Balbi]

Hi,

On Thu, Feb 14, 2013 at 10:12:17AM -0800, Tony Lindgren wrote:
> * 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.

cool.

> > > 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.

right... and it only works because hwmod never calls
request_mem_region().

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

makes sense.

> 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.

sounds good. But then it means our DTS files should be 100% complete,
meaning that even for the IPs which we don't have drivers at all, we
should still provide DTS nodes.

In that case, does it make sense to teach DT about the actual structure
of OMAP's interconnect ? I mean:

ocp {
	l3 at xxxxxxxx {
		l4_per at xxxxxxxx {
			...
		};

		l4_wakeup at xxxxxxxx {
			...
		};

		...
	};
};

That would mean that even interconnect PM could move to a driver under
drivers/bus/{l3,l4_per,l4_wakeup,..}.c

> 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.

Should hwmod be touching that in the long run ? It _does_ belong in the
device's address space

> > Note sure if any of those are acceptable.
> 
> Hmm what issues do you see in the above suggestion?

driver and hwmod accessing SYSC simultaneously for instance. Imagine
something like:

hwmod						driver
------						-------
1. starts device reset
2. polls RESET bit with X ms timeout		X' ms later starts reset
3. times out since reset restarts		polls RESET bit with X ms timeout
4. error!					reset completes

In such cases, what do we do ? There can be many such cases, don't
you think ?

> > > 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?

perhaps...

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130214/e04cee95/attachment-0001.sig>

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

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [130214 11:31]:
> On Thu, Feb 14, 2013 at 10:12:17AM -0800, Tony Lindgren wrote:
> 
> > 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.
> 
> sounds good. But then it means our DTS files should be 100% complete,
> meaning that even for the IPs which we don't have drivers at all, we
> should still provide DTS nodes.

Yes, eventually. It's still better to have only one set of that data
rather than number of supported SoCs times that data.. Parsing it
should not be too bad if it's done one driver at a time during the
driver probe.

> In that case, does it make sense to teach DT about the actual structure
> of OMAP's interconnect ? I mean:
> 
> ocp {
> 	l3 at xxxxxxxx {
> 		l4_per at xxxxxxxx {
> 			...
> 		};
> 
> 		l4_wakeup at xxxxxxxx {
> 			...
> 		};
> 
> 		...
> 	};
> };
> 
> That would mean that even interconnect PM could move to a driver under
> drivers/bus/{l3,l4_per,l4_wakeup,..}.c

Yes makes sense to me.
 
> > 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.
> 
> Should hwmod be touching that in the long run ? It _does_ belong in the
> device's address space
> 
> > > Note sure if any of those are acceptable.
> > 
> > Hmm what issues do you see in the above suggestion?
> 
> driver and hwmod accessing SYSC simultaneously for instance. Imagine
> something like:
> 
> hwmod						driver
> ------						-------
> 1. starts device reset
> 2. polls RESET bit with X ms timeout		X' ms later starts reset
> 3. times out since reset restarts		polls RESET bit with X ms timeout
> 4. error!					reset completes
> 
> In such cases, what do we do ? There can be many such cases, don't
> you think ?

I don't think so as hwmod should only touch the sysconfig space
when no driver has claimed it. If hwmod sysconfig tinkering is
only done in late_initcall, I don't think any drivers are probing
at that time? Well there may be some deferred probe related issues
there though, so we need some atomic way at that point to know if
some hardware is being claimed by a driver.

Regards,

Tony 

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

[Paul Walmsley]

Hi,

On Thu, 14 Feb 2013, Tony Lindgren wrote:

> I don't think so as hwmod should only touch the sysconfig space
> when no driver has claimed it.

hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
and resume operations, and during device enable and disable operations.  
Here's the relevant code:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195

Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 
have anything specifically to do with the underlying device IP block, but 
with the SoC integration, even though they live in the device's 
memory-mapped address range.  It's unfortunate that TI didn't allocate a 
separate, small address space for these registers, translated by the bus, 
similar to the PCI-E Enhanced Configuration Access Mechanism:

http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space

... but that's unfortunately the reality of the OMAP hardware.

Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as 
long as the implementation of ioremap() can be overridden by the device's 
bus.  PCI device drivers already do this -- albeit in a PCI-specific way 
-- with pci_ioremap_bar():

http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git&a=search&h=HEAD&st=grep&s=pci_ioremap_bar

So instead of something bus-specific like that, a better way would be to 
use something like:

va = dev->bus->ioremap( ... );
va = dev->bus->iounmap( ... );

Any driver using such a mechanism would be intrinsically generic. 
platform_bus could simply set its bus->ioremap to the existing ioremap() 
function, while buses like a omap_hwmod_bus could use a function that 
returned the address range that omap_hwmod_bus has already ioremap()ped.

regards,

- Paul

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

[Paul Walmsley]

Hi,

On Thu, 14 Feb 2013, Paul Walmsley wrote:

> So instead of something bus-specific like that, a better way would be to 
> use something like:
> 
> va = dev->bus->ioremap( ... );
> va = dev->bus->iounmap( ... );

Something like this is what I was thinking.  Obviously there would be more
patches needed, for the various arches, etc.; and I'm not convinced that
the function pointer init is done at the right time yet. Comments welcome.


- Paul


From: Paul Walmsley <paul@pwsan.com>
Date: Thu, 14 Feb 2013 13:49:58 -0700
Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
 and iounmap()

On some hardware, such as OMAP, the bus abstraction code needs to call
ioremap(), since some SoC-integration registers are located in the
device address space.  But generic device drivers should be able to
call ioremap() from driver code, for the majority of situations where
this isn't necessary.  This experimental patch allows Linux bus abstraction
code to override all of the ioremap*() and iounmap() functions.  In the OMAP
case, this would be used to return the previously-mapped address range from
the bus code, when called for the device's register area.  This would avoid
a duplicate TLB mapping for that space.

This might also be useful as a generic replacement for pci_ioremap_bar().

Compile-tested only.

---
 arch/arm/include/asm/io.h |   10 +++++-----
 arch/arm/mm/ioremap.c     |   30 ++++++++++++++++++++++++++++++
 arch/arm/mm/mmu.c         |    8 ++++++++
 include/linux/device.h    |   26 ++++++++++++++++++++++++++
 4 files changed, 69 insertions(+), 5 deletions(-)

diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
index 652b560..22cc085 100644
--- a/arch/arm/include/asm/io.h
+++ b/arch/arm/include/asm/io.h
@@ -325,11 +325,11 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
  * Documentation/io-mapping.txt.
  *
  */
-#define ioremap(cookie,size)		__arm_ioremap((cookie), (size), MT_DEVICE)
-#define ioremap_nocache(cookie,size)	__arm_ioremap((cookie), (size), MT_DEVICE)
-#define ioremap_cached(cookie,size)	__arm_ioremap((cookie), (size), MT_DEVICE_CACHED)
-#define ioremap_wc(cookie,size)		__arm_ioremap((cookie), (size), MT_DEVICE_WC)
-#define iounmap				__arm_iounmap
+extern void __iomem *ioremap(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_nocache(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_cached(unsigned long cookie, size_t size);
+extern void __iomem *ioremap_wc(unsigned long cookie, size_t size);
+extern void iounmap(volatile void __iomem *va);
 
 /*
  * io{read,write}{8,16,32} macros
diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
index 88fd86c..6507e69 100644
--- a/arch/arm/mm/ioremap.c
+++ b/arch/arm/mm/ioremap.c
@@ -398,3 +398,33 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
 }
 EXPORT_SYMBOL_GPL(pci_ioremap_io);
 #endif
+
+void __iomem *ioremap(unsigned long cookie, size_t size)
+{
+	return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap);
+
+void __iomem *ioremap_nocache(unsigned long cookie, size_t size)
+{
+	return __arm_ioremap(cookie, size, MT_DEVICE);
+}
+EXPORT_SYMBOL(ioremap_nocache);
+
+void __iomem *ioremap_cached(unsigned long cookie, size_t size)
+{
+	return __arm_ioremap(cookie, size, MT_DEVICE_CACHED);
+}
+EXPORT_SYMBOL(ioremap_cached);
+
+void __iomem *ioremap_wc(unsigned long cookie, size_t size)
+{
+	return __arm_ioremap(cookie, size, MT_DEVICE_WC);
+}
+EXPORT_SYMBOL(ioremap_wc);
+
+void iounmap(volatile void __iomem *va)
+{
+	return __arm_iounmap(va);
+}
+EXPORT_SYMBOL(iounmap);
diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
index ce328c7..303dba0 100644
--- a/arch/arm/mm/mmu.c
+++ b/arch/arm/mm/mmu.c
@@ -17,6 +17,7 @@
 #include <linux/fs.h>
 #include <linux/vmalloc.h>
 #include <linux/sizes.h>
+#include <linux/platform_device.h>
 
 #include <asm/cp15.h>
 #include <asm/cputype.h>
@@ -28,6 +29,7 @@
 #include <asm/highmem.h>
 #include <asm/system_info.h>
 #include <asm/traps.h>
+#include <asm/io.h>
 
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
@@ -1246,4 +1248,10 @@ void __init paging_init(struct machine_desc *mdesc)
 
 	empty_zero_page = virt_to_page(zero_page);
 	__flush_dcache_page(NULL, empty_zero_page);
+
+	platform_bus_type.ioremap = ioremap;
+	platform_bus_type.ioremap_nocache = ioremap_nocache;
+	platform_bus_type.ioremap_cached = ioremap_cached;
+	platform_bus_type.ioremap_wc = ioremap_wc;
+	platform_bus_type.iounmap = iounmap;
 }
diff --git a/include/linux/device.h b/include/linux/device.h
index 43dcda9..48c35e2 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -71,6 +71,26 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
  * @shutdown:	Called at shut-down time to quiesce the device.
  * @suspend:	Called when a device on this bus wants to go to sleep mode.
  * @resume:	Called to bring a device on this bus out of sleep mode.
+ * @ioremap:	Function pointer returning a virtual address used to
+ *		interact with a device on this bus.  Generally
+ *		implemented via @ioremap_nocache.
+ * @ioremap_nocache: Function pointer returning a virtual address used to
+ *		interact with a device on this bus.  Reads and writes
+ *		to the returned address space are not cached by the
+ *		CPU, so are suitable for memory-mapped I/O regions.
+ * @ioremap_cached: Function pointer returning a virtual address used to
+ *		interact with private memory located on this bus.  Reads and
+ *		writes to the returned address space may be cached by the
+ *		CPU, so this is suitable for I/O-mapped memory where all
+ *		accesses are via the same cache.
+ * @ioremap_wc: Function pointer returning a virtual address used to
+ *		interact with memory located on this bus.  Writes to
+ *		the returned address space may be combined by the CPU,
+ *		so this is suitable for I/O-mapped memory such as
+ *		framebuffers.
+ * @iounmap:	Function pointer called to indicate that a caller is done
+ *		with the virtual address mapping returned by @ioremap,
+ *		@ioremap_nocache, @ioremap_cached, or @ioremap_wc.
  * @pm:		Power management operations of this bus, callback the specific
  *		device driver's pm-ops.
  * @iommu_ops:  IOMMU specific operations for this bus, used to attach IOMMU
@@ -105,6 +125,12 @@ struct bus_type {
 	int (*suspend)(struct device *dev, pm_message_t state);
 	int (*resume)(struct device *dev);
 
+	void __iomem *(*ioremap)(unsigned long phys_addr, size_t size);
+	void __iomem *(*ioremap_nocache)(unsigned long phys_addr, size_t size);
+	void __iomem *(*ioremap_cached)(unsigned long phys_addr, size_t size);
+	void __iomem *(*ioremap_wc)(unsigned long phys_addr, size_t size);
+	void (*iounmap)(volatile void __iomem *va);
+
 	const struct dev_pm_ops *pm;
 
 	struct iommu_ops *iommu_ops;
-- 
1.7.10.4

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

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [130214 13:44]:
> Hi,
> 
> On Thu, 14 Feb 2013, Paul Walmsley wrote:
> 
> > So instead of something bus-specific like that, a better way would be to 
> > use something like:
> > 
> > va = dev->bus->ioremap( ... );
> > va = dev->bus->iounmap( ... );
> 
> Something like this is what I was thinking.  Obviously there would be more
> patches needed, for the various arches, etc.; and I'm not convinced that
> the function pointer init is done at the right time yet. Comments welcome.
> 
> 
> - Paul
> 
> 
> From: Paul Walmsley <paul@pwsan.com>
> Date: Thu, 14 Feb 2013 13:49:58 -0700
> Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
>  and iounmap()
> 
> On some hardware, such as OMAP, the bus abstraction code needs to call
> ioremap(), since some SoC-integration registers are located in the
> device address space.  But generic device drivers should be able to
> call ioremap() from driver code, for the majority of situations where
> this isn't necessary.  This experimental patch allows Linux bus abstraction
> code to override all of the ioremap*() and iounmap() functions.  In the OMAP
> case, this would be used to return the previously-mapped address range from
> the bus code, when called for the device's register area.  This would avoid
> a duplicate TLB mapping for that space.
> 
> This might also be useful as a generic replacement for pci_ioremap_bar().
> 
> Compile-tested only.

The other option could be to allow custom ioremap function pointers 
based on address range in __arm_ioremap_pfn_caller() the same way as
the SoC specific static mappings are allowed. That would require adding
a function pointer to struct map_desc.

Maybe that would do the trick?

Regards,

Tony

> ---
>  arch/arm/include/asm/io.h |   10 +++++-----
>  arch/arm/mm/ioremap.c     |   30 ++++++++++++++++++++++++++++++
>  arch/arm/mm/mmu.c         |    8 ++++++++
>  include/linux/device.h    |   26 ++++++++++++++++++++++++++
>  4 files changed, 69 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/include/asm/io.h b/arch/arm/include/asm/io.h
> index 652b560..22cc085 100644
> --- a/arch/arm/include/asm/io.h
> +++ b/arch/arm/include/asm/io.h
> @@ -325,11 +325,11 @@ extern void _memset_io(volatile void __iomem *, int, size_t);
>   * Documentation/io-mapping.txt.
>   *
>   */
> -#define ioremap(cookie,size)		__arm_ioremap((cookie), (size), MT_DEVICE)
j> -#define ioremap_nocache(cookie,size)	__arm_ioremap((cookie), (size), MT_DEVICE)
> -#define ioremap_cached(cookie,size)	__arm_ioremap((cookie), (size), MT_DEVICE_CACHED)
> -#define ioremap_wc(cookie,size)		__arm_ioremap((cookie), (size), MT_DEVICE_WC)
> -#define iounmap				__arm_iounmap
> +extern void __iomem *ioremap(unsigned long cookie, size_t size);
> +extern void __iomem *ioremap_nocache(unsigned long cookie, size_t size);
> +extern void __iomem *ioremap_cached(unsigned long cookie, size_t size);
> +extern void __iomem *ioremap_wc(unsigned long cookie, size_t size);
> +extern void iounmap(volatile void __iomem *va);
>  
>  /*
>   * io{read,write}{8,16,32} macros
> diff --git a/arch/arm/mm/ioremap.c b/arch/arm/mm/ioremap.c
> index 88fd86c..6507e69 100644
> --- a/arch/arm/mm/ioremap.c
> +++ b/arch/arm/mm/ioremap.c
> @@ -398,3 +398,33 @@ int pci_ioremap_io(unsigned int offset, phys_addr_t phys_addr)
>  }
>  EXPORT_SYMBOL_GPL(pci_ioremap_io);
>  #endif
> +
> +void __iomem *ioremap(unsigned long cookie, size_t size)
> +{
> +	return __arm_ioremap(cookie, size, MT_DEVICE);
> +}
> +EXPORT_SYMBOL(ioremap);
> +
> +void __iomem *ioremap_nocache(unsigned long cookie, size_t size)
> +{
> +	return __arm_ioremap(cookie, size, MT_DEVICE);
> +}
> +EXPORT_SYMBOL(ioremap_nocache);
> +
> +void __iomem *ioremap_cached(unsigned long cookie, size_t size)
> +{
> +	return __arm_ioremap(cookie, size, MT_DEVICE_CACHED);
> +}
> +EXPORT_SYMBOL(ioremap_cached);
> +
> +void __iomem *ioremap_wc(unsigned long cookie, size_t size)
> +{
> +	return __arm_ioremap(cookie, size, MT_DEVICE_WC);
> +}
> +EXPORT_SYMBOL(ioremap_wc);
> +
> +void iounmap(volatile void __iomem *va)
> +{
> +	return __arm_iounmap(va);
> +}
> +EXPORT_SYMBOL(iounmap);
> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index ce328c7..303dba0 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -17,6 +17,7 @@
>  #include <linux/fs.h>
>  #include <linux/vmalloc.h>
>  #include <linux/sizes.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/cp15.h>
>  #include <asm/cputype.h>
> @@ -28,6 +29,7 @@
>  #include <asm/highmem.h>
>  #include <asm/system_info.h>
>  #include <asm/traps.h>
> +#include <asm/io.h>
>  
>  #include <asm/mach/arch.h>
>  #include <asm/mach/map.h>
> @@ -1246,4 +1248,10 @@ void __init paging_init(struct machine_desc *mdesc)
>  
>  	empty_zero_page = virt_to_page(zero_page);
>  	__flush_dcache_page(NULL, empty_zero_page);
> +
> +	platform_bus_type.ioremap = ioremap;
> +	platform_bus_type.ioremap_nocache = ioremap_nocache;
> +	platform_bus_type.ioremap_cached = ioremap_cached;
> +	platform_bus_type.ioremap_wc = ioremap_wc;
> +	platform_bus_type.iounmap = iounmap;
>  }
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 43dcda9..48c35e2 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -71,6 +71,26 @@ extern void bus_remove_file(struct bus_type *, struct bus_attribute *);
>   * @shutdown:	Called at shut-down time to quiesce the device.
>   * @suspend:	Called when a device on this bus wants to go to sleep mode.
>   * @resume:	Called to bring a device on this bus out of sleep mode.
> + * @ioremap:	Function pointer returning a virtual address used to
> + *		interact with a device on this bus.  Generally
> + *		implemented via @ioremap_nocache.
> + * @ioremap_nocache: Function pointer returning a virtual address used to
> + *		interact with a device on this bus.  Reads and writes
> + *		to the returned address space are not cached by the
> + *		CPU, so are suitable for memory-mapped I/O regions.
> + * @ioremap_cached: Function pointer returning a virtual address used to
> + *		interact with private memory located on this bus.  Reads and
> + *		writes to the returned address space may be cached by the
> + *		CPU, so this is suitable for I/O-mapped memory where all
> + *		accesses are via the same cache.
> + * @ioremap_wc: Function pointer returning a virtual address used to
> + *		interact with memory located on this bus.  Writes to
> + *		the returned address space may be combined by the CPU,
> + *		so this is suitable for I/O-mapped memory such as
> + *		framebuffers.
> + * @iounmap:	Function pointer called to indicate that a caller is done
> + *		with the virtual address mapping returned by @ioremap,
> + *		@ioremap_nocache, @ioremap_cached, or @ioremap_wc.
>   * @pm:		Power management operations of this bus, callback the specific
>   *		device driver's pm-ops.
>   * @iommu_ops:  IOMMU specific operations for this bus, used to attach IOMMU
> @@ -105,6 +125,12 @@ struct bus_type {
>  	int (*suspend)(struct device *dev, pm_message_t state);
>  	int (*resume)(struct device *dev);
>  
> +	void __iomem *(*ioremap)(unsigned long phys_addr, size_t size);
> +	void __iomem *(*ioremap_nocache)(unsigned long phys_addr, size_t size);
> +	void __iomem *(*ioremap_cached)(unsigned long phys_addr, size_t size);
> +	void __iomem *(*ioremap_wc)(unsigned long phys_addr, size_t size);
> +	void (*iounmap)(volatile void __iomem *va);
> +
>  	const struct dev_pm_ops *pm;
>  
>  	struct iommu_ops *iommu_ops;
> -- 
> 1.7.10.4
> 

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

[Felipe Balbi]

On Thu, Feb 14, 2013 at 02:47:10PM -0800, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [130214 13:44]:
> > Hi,
> > 
> > On Thu, 14 Feb 2013, Paul Walmsley wrote:
> > 
> > > So instead of something bus-specific like that, a better way would be to 
> > > use something like:
> > > 
> > > va = dev->bus->ioremap( ... );
> > > va = dev->bus->iounmap( ... );
> > 
> > Something like this is what I was thinking.  Obviously there would be more
> > patches needed, for the various arches, etc.; and I'm not convinced that
> > the function pointer init is done at the right time yet. Comments welcome.
> > 
> > 
> > - Paul
> > 
> > 
> > From: Paul Walmsley <paul@pwsan.com>
> > Date: Thu, 14 Feb 2013 13:49:58 -0700
> > Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
> >  and iounmap()
> > 
> > On some hardware, such as OMAP, the bus abstraction code needs to call
> > ioremap(), since some SoC-integration registers are located in the
> > device address space.  But generic device drivers should be able to
> > call ioremap() from driver code, for the majority of situations where
> > this isn't necessary.  This experimental patch allows Linux bus abstraction
> > code to override all of the ioremap*() and iounmap() functions.  In the OMAP
> > case, this would be used to return the previously-mapped address range from
> > the bus code, when called for the device's register area.  This would avoid
> > a duplicate TLB mapping for that space.
> > 
> > This might also be useful as a generic replacement for pci_ioremap_bar().
> > 
> > Compile-tested only.
> 
> The other option could be to allow custom ioremap function pointers 
> based on address range in __arm_ioremap_pfn_caller() the same way as
> the SoC specific static mappings are allowed. That would require adding
> a function pointer to struct map_desc.
> 
> Maybe that would do the trick?

I'm not sure we should even try that. I mean, eventually (maybe in 100
years) we wouldn't mach-* at all and even struct map_desc would be
dynamically initialized by data passed in via DTB, right ? Just consider
Arnd's patch for the "machine_desc-less" DT boot. If we add a function
pointer to struct map_desc, it will just become yet another function
pointer to be removed later.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/9d963284/attachment-0001.sig>

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

[Santosh Shilimkar]

+ Tero, Rajendra, Kevin

On Friday 15 February 2013 12:16 PM, Felipe Balbi wrote:
> On Thu, Feb 14, 2013 at 02:47:10PM -0800, Tony Lindgren wrote:
>> * Paul Walmsley <paul@pwsan.com> [130214 13:44]:
>>> Hi,
>>>
>>> On Thu, 14 Feb 2013, Paul Walmsley wrote:
>>>
>>>> So instead of something bus-specific like that, a better way would be to
>>>> use something like:
>>>>
>>>> va = dev->bus->ioremap( ... );
>>>> va = dev->bus->iounmap( ... );
>>>
>>> Something like this is what I was thinking.  Obviously there would be more
>>> patches needed, for the various arches, etc.; and I'm not convinced that
>>> the function pointer init is done at the right time yet. Comments welcome.
>>>
>>>
>>> - Paul
>>>
>>>
>>> From: Paul Walmsley <paul@pwsan.com>
>>> Date: Thu, 14 Feb 2013 13:49:58 -0700
>>> Subject: [PATCH] EXPERIMENTAL: device/ARM: allow buses to override ioremap*()
>>>   and iounmap()
>>>
>>> On some hardware, such as OMAP, the bus abstraction code needs to call
>>> ioremap(), since some SoC-integration registers are located in the
>>> device address space.  But generic device drivers should be able to
>>> call ioremap() from driver code, for the majority of situations where
>>> this isn't necessary.  This experimental patch allows Linux bus abstraction
>>> code to override all of the ioremap*() and iounmap() functions.  In the OMAP
>>> case, this would be used to return the previously-mapped address range from
>>> the bus code, when called for the device's register area.  This would avoid
>>> a duplicate TLB mapping for that space.
>>>
>>> This might also be useful as a generic replacement for pci_ioremap_bar().
>>>
>>> Compile-tested only.
>>
>> The other option could be to allow custom ioremap function pointers
>> based on address range in __arm_ioremap_pfn_caller() the same way as
>> the SoC specific static mappings are allowed. That would require adding
>> a function pointer to struct map_desc.
>>
>> Maybe that would do the trick?
>
> I'm not sure we should even try that. I mean, eventually (maybe in 100
> years) we wouldn't mach-* at all and even struct map_desc would be
> dynamically initialized by data passed in via DTB, right ? Just consider
> Arnd's patch for the "machine_desc-less" DT boot. If we add a function
> pointer to struct map_desc, it will just become yet another function
> pointer to be removed later.
>
On the same thread, it was also mentioned that, "machine_desc-less" DT
boot is for simple machines. I have looked that aspect bit more.
Considering the amount of callbacks OMAP uses, it is not practical.
Ofcourse am also not in favor of adding another function pointer
stuff here.

We should get rid of dual ioremap() and the method suggested by Paul
seems to be reasonable.

Sysconfig handling is very tightly coupled with PRCM. I agree with Paul
that hardware should have mapped it in separate address space. So
whichever framework manages the PRCM, should also manage sysconfig.
Refer all the issues around sysconfig for IP's like USB, UART
and they are directly PRCM issues.

We should be able to work around the IP integration issues
around problematic IPs like UART, USB with some profile knowledge
which can be handled by runtime PM transparently. It should have
been done that way in first place instead of functions pointers
which today break the Device Tree builds directly.

As I mentioned in OMAP5 data series, we are now able to remove the
IO_resource information ( Address space, IRQ data, DMA lines) from
from hwmod data and extract it from device Tree. Thats a good
point.

Next one is getting rid off sysconfig function pointers which drivers
are using and handle those issues using runtime_pm APIs. Idea is, IP
gives profile like...
e.g
McSPI1 -> Smart Idle always
UART -> no_idle(runtime_get) and  smart_idle(runtime_put)
mUSB --> no_idle(runtime_get) and force_idle(runtime_put)

Then the last one is custom reset function for broken IPs. For this
one as well if we can add a hook in runtime framework, then it
can be handled without direct function calls.

Regards,
Santosh



Regards
Santosh

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

[Paul Walmsley]

Hi,

On Thu, 14 Feb 2013, Tony Lindgren wrote:

> The other option could be to allow custom ioremap function pointers 
> based on address range in __arm_ioremap_pfn_caller() the same way as
> the SoC specific static mappings are allowed. That would require adding
> a function pointer to struct map_desc.
> 
> Maybe that would do the trick?

That sounds fine to me too.

To me it makes sense to eventually handle the I/O mappings automatically 
from data in the DT -- hence the association with a bus device, which 
should be present in DT data.


- Paul

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

[Russell King - ARM Linux]

On Tue, Feb 19, 2013 at 03:30:01PM +0000, Paul Walmsley wrote:
> Hi,
> 
> On Thu, 14 Feb 2013, Tony Lindgren wrote:
> 
> > The other option could be to allow custom ioremap function pointers 
> > based on address range in __arm_ioremap_pfn_caller() the same way as
> > the SoC specific static mappings are allowed. That would require adding
> > a function pointer to struct map_desc.
> > 
> > Maybe that would do the trick?
> 
> That sounds fine to me too.
> 
> To me it makes sense to eventually handle the I/O mappings automatically 
> from data in the DT -- hence the association with a bus device, which 
> should be present in DT data.

No.  I really don't get why OMAP thinks it's soo special that it needs
to go around adding lots and lots of new abstractions all over the
place.

Indirect ioremap() through a function pointer so you can overload it with
OMAP specific crap?  No way.  Function pointers in map_desc - what the hell
for?

You must be totally mad if you think that's a way forward, I really don't
see how you think this kind of crap would be remotely acceptable.

Ok, so you have this problem that you need hwmod to touch a register which
is also contained within the device resources as well.  That's fine.  You
can do it one of two ways:

1. Call out from the driver at the appropriate points (which seems to be
   _after_ you've ioremapped it) to access the register.

2. Find the resource, temporarily map the register, access it, and then
   unmap it.

No requirement what so ever to override ioremap().  AT ALL.  So stop this
madness now.

As for a function pointer in struct map_desc.  You're bonkers, the lot of
you.  map_desc is a structure describing the boot time static mappings
which gets discarded.  Nothing keeps any references around to it, and it
is used at a time when *NOTHING ELSE* should be going on in the kernel
other than building those static mappings.  Not even any arch code poking
about at devices.  Or anything like that.  Because if you think that's
acceptable, then we'll need to flush the cache and TLB after each and
every map_desc entry is touched - which brings a whole load of new pain
and slowness to the kernel boot.

So please, stop this idiotic madness now.

If you want such things as pci_enable_device(), then what you're actually
asking for is omap_enable_device() for OMAP devices.  OMAP devices are
already specific enough to OMAP SoCs (god knows, they have really complex
and obscure behaviours that no one else in their right mind would want
to copy) that calling out to omap specific functions would never really
be a problem.

No need for any of this crazy dev->bus shit either.

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

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [130219 07:49]:
> On Tue, Feb 19, 2013 at 03:30:01PM +0000, Paul Walmsley wrote:
> > Hi,
> > 
> > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > 
> > > The other option could be to allow custom ioremap function pointers 
> > > based on address range in __arm_ioremap_pfn_caller() the same way as
> > > the SoC specific static mappings are allowed. That would require adding
> > > a function pointer to struct map_desc.
> > > 
> > > Maybe that would do the trick?
> > 
> > That sounds fine to me too.
> > 
> > To me it makes sense to eventually handle the I/O mappings automatically 
> > from data in the DT -- hence the association with a bus device, which 
> > should be present in DT data.
> 
> No.  I really don't get why OMAP thinks it's soo special that it needs
> to go around adding lots and lots of new abstractions all over the
> place.

Hey it's actually quite the opposite. We're trying to find a generic
way of doing things in order to avoid omap specific driver hacks.
 
> Indirect ioremap() through a function pointer so you can overload it with
> OMAP specific crap?  No way.  Function pointers in map_desc - what the hell
> for?
> 
> You must be totally mad if you think that's a way forward, I really don't
> see how you think this kind of crap would be remotely acceptable.
> 
> Ok, so you have this problem that you need hwmod to touch a register which
> is also contained within the device resources as well.  That's fine.  You
> can do it one of two ways:
> 
> 1. Call out from the driver at the appropriate points (which seems to be
>    _after_ you've ioremapped it) to access the register.
> 
> 2. Find the resource, temporarily map the register, access it, and then
>    unmap it.
> 
> No requirement what so ever to override ioremap().  AT ALL.  So stop this
> madness now.
> 
> As for a function pointer in struct map_desc.  You're bonkers, the lot of
> you.  map_desc is a structure describing the boot time static mappings
> which gets discarded.  Nothing keeps any references around to it, and it
> is used at a time when *NOTHING ELSE* should be going on in the kernel
> other than building those static mappings.  Not even any arch code poking
> about at devices.  Or anything like that.  Because if you think that's
> acceptable, then we'll need to flush the cache and TLB after each and
> every map_desc entry is touched - which brings a whole load of new pain
> and slowness to the kernel boot.
> 
> So please, stop this idiotic madness now.

OK good point. Also it would be risky for things going wrong trying to try
set up more complex stuff that early as then debugging it again depends on
DEBUG_LL.
 
> If you want such things as pci_enable_device(), then what you're actually
> asking for is omap_enable_device() for OMAP devices.  OMAP devices are
> already specific enough to OMAP SoCs (god knows, they have really complex
> and obscure behaviours that no one else in their right mind would want
> to copy) that calling out to omap specific functions would never really
> be a problem.

I'd rather avoid adding omap_enable_device() calls to drivers as we really
want to keep the drivers generic. But maybe there could be some generic
bus_enable_device() function pointer that could be populated by the bus
code during init.

> No need for any of this crazy dev->bus shit either.

Paul's suggestion on configuring things with pm_runtime_get()
seems doable and is generic.

Regards,

Tony

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

[Russell King - ARM Linux]

On Tue, Feb 19, 2013 at 08:30:53AM -0800, Tony Lindgren wrote:
> * Russell King - ARM Linux <linux@arm.linux.org.uk> [130219 07:49]:
> > If you want such things as pci_enable_device(), then what you're actually
> > asking for is omap_enable_device() for OMAP devices.  OMAP devices are
> > already specific enough to OMAP SoCs (god knows, they have really complex
> > and obscure behaviours that no one else in their right mind would want
> > to copy) that calling out to omap specific functions would never really
> > be a problem.
> 
> I'd rather avoid adding omap_enable_device() calls to drivers as we really
> want to keep the drivers generic. But maybe there could be some generic
> bus_enable_device() function pointer that could be populated by the bus
> code during init.

What you're not getting is that pci_enable_device() is a PCI thing which
is mostly a no-op - and where it isn't a no-op, it's something that must
be done _before_ PCI resources are used or even reserved (because
conceptually, this is to do with getting the resources correct.)

The PCI case is:

pci_device_probe(pci_dev)
{
	pci_enable_device(pci_dev);

	pci_request_regions(pci_dev);

	regs = pci_iomap(pci_dev, BAR, size);
...
}

That's different from what you're wanting on OMAP - what you're wanting
there is some way to record the platform device has been ioremapped, so
that you can then fiddle with its idle/reset register from "bus" code.

If you think about it in light of the above sequence, the "enable device"
stage *doesn't* suit your needs because that happens before the driver
has done anything.

However... if you think you're going to get away with another total
rewrite of OMAP device support away from hwmod to a new scheme with a
new load of huge churn, think again.  Remember, churn is evil.  I've
complained to you about the amount of churn that OMAP manufactures
in the past.  Linus has complained about it too.  You can't continue
like this.

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

[Tony Lindgren]

* Russell King - ARM Linux <linux@arm.linux.org.uk> [130219 10:26]:
> On Tue, Feb 19, 2013 at 08:30:53AM -0800, Tony Lindgren wrote:
> > * Russell King - ARM Linux <linux@arm.linux.org.uk> [130219 07:49]:
> > > If you want such things as pci_enable_device(), then what you're actually
> > > asking for is omap_enable_device() for OMAP devices.  OMAP devices are
> > > already specific enough to OMAP SoCs (god knows, they have really complex
> > > and obscure behaviours that no one else in their right mind would want
> > > to copy) that calling out to omap specific functions would never really
> > > be a problem.
> > 
> > I'd rather avoid adding omap_enable_device() calls to drivers as we really
> > want to keep the drivers generic. But maybe there could be some generic
> > bus_enable_device() function pointer that could be populated by the bus
> > code during init.
> 
> What you're not getting is that pci_enable_device() is a PCI thing which
> is mostly a no-op - and where it isn't a no-op, it's something that must
> be done _before_ PCI resources are used or even reserved (because
> conceptually, this is to do with getting the resources correct.)
> 
> The PCI case is:
> 
> pci_device_probe(pci_dev)
> {
> 	pci_enable_device(pci_dev);
> 
> 	pci_request_regions(pci_dev);
> 
> 	regs = pci_iomap(pci_dev, BAR, size);
> ...
> }
> 
> That's different from what you're wanting on OMAP - what you're wanting
> there is some way to record the platform device has been ioremapped, so
> that you can then fiddle with its idle/reset register from "bus" code.
> 
> If you think about it in light of the above sequence, the "enable device"
> stage *doesn't* suit your needs because that happens before the driver
> has done anything.

Right.. that won't help then. It sounds like the proper place would
be something like pm_runtime_init() as these register manage things
like autoidle etc.
 
> However... if you think you're going to get away with another total
> rewrite of OMAP device support away from hwmod to a new scheme with a
> new load of huge churn, think again.  Remember, churn is evil.  I've
> complained to you about the amount of churn that OMAP manufactures
> in the past.  Linus has complained about it too.  You can't continue
> like this.

I don't think there's any churn needed here if done properly. It's
mostly a question of dropping duplicate data from hwmod that we
already have available in device tree. That means we can shrink the
hwmod data needed.

Regards,

Tony

hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
> > However... if you think you're going to get away with another total
> > rewrite of OMAP device support away from hwmod to a new scheme with a
> > new load of huge churn, think again.  Remember, churn is evil.  I've
> > complained to you about the amount of churn that OMAP manufactures
> > in the past.  Linus has complained about it too.  You can't continue
> > like this.
> 
> I don't think there's any churn needed here if done properly. It's
> mostly a question of dropping duplicate data from hwmod that we
> already have available in device tree. That means we can shrink the
> hwmod data needed.

how about starting by removing register addresses and interrupt numbers
which are passed by devicetree today ? If you want to move to DT-only
now even without all drivers DT-adapted, you can have something like
what Marvel folks did for kirkwood:

static void __init kirkwood_dt_init(void)
{

[ ... ]

	if (of_machine_is_compatible("globalscale,dreamplug"))
		dreamplug_init();

	if (of_machine_is_compatible("dlink,dns-kirkwood"))
		dnskw_init();

	if (of_machine_is_compatible("iom,iconnect"))
		iconnect_init();

[ ... ]

	of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
}

those machine-based inits are there just to initialize drivers which
aren't converted to DT.

this would let you remove all board-files except board-generic.c and
remove data which is already passed in via DT. It has the benefit of
showing Linus (or whoever cares) that we are working to remove the
"churn" while also removing some of the pressure of DT conversion, since
there are still details which need to be sorted out (UART function
pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
nodes, etc etc).

Only on board-files we're talking about over 13K lines:

 arch/arm/mach-omap2/board-2430sdp.c          |  289 ------
 arch/arm/mach-omap2/board-3430sdp.c          |  602 ------------
 arch/arm/mach-omap2/board-3630sdp.c          |  216 -----
 arch/arm/mach-omap2/board-4430sdp.c          |  730 ---------------
 arch/arm/mach-omap2/board-am3517crane.c      |   97 --
 arch/arm/mach-omap2/board-am3517evm.c        |  398 --------
 arch/arm/mach-omap2/board-apollon.c          |  342 -------
 arch/arm/mach-omap2/board-cm-t35.c           |  769 ----------------
 arch/arm/mach-omap2/board-cm-t3517.c         |  302 ------
 arch/arm/mach-omap2/board-devkit8000.c       |  648 -------------
 arch/arm/mach-omap2/board-flash.c            |  242 -----
 arch/arm/mach-omap2/board-flash.h            |   62 --
 arch/arm/mach-omap2/board-h4.c               |  347 -------
 arch/arm/mach-omap2/board-igep0020.c         |  673 --------------
 arch/arm/mach-omap2/board-ldp.c              |  440 ---------
 arch/arm/mach-omap2/board-n8x0.c             |  762 ---------------
 arch/arm/mach-omap2/board-omap3beagle.c      |  549 -----------
 arch/arm/mach-omap2/board-omap3evm.c         |  762 ---------------
 arch/arm/mach-omap2/board-omap3logic.c       |  249 -----
 arch/arm/mach-omap2/board-omap3pandora.c     |  623 -------------
 arch/arm/mach-omap2/board-omap3stalker.c     |  432 ---------
 arch/arm/mach-omap2/board-omap3touchbook.c   |  391 --------
 arch/arm/mach-omap2/board-omap4panda.c       |  467 ----------
 arch/arm/mach-omap2/board-overo.c            |  556 -----------
 arch/arm/mach-omap2/board-rm680.c            |  165 ----
 arch/arm/mach-omap2/board-rx51-peripherals.c | 1276 --------------------------
 arch/arm/mach-omap2/board-rx51-video.c       |   89 --
 arch/arm/mach-omap2/board-rx51.c             |  128 ---
 arch/arm/mach-omap2/board-rx51.h             |   11 -
 arch/arm/mach-omap2/board-ti8168evm.c        |   62 --
 arch/arm/mach-omap2/board-zoom-debugboard.c  |  139 ---
 arch/arm/mach-omap2/board-zoom-display.c     |  147 ---
 arch/arm/mach-omap2/board-zoom-peripherals.c |  304 ------
 arch/arm/mach-omap2/board-zoom.c             |  155 ----
 arch/arm/mach-omap2/board-zoom.h             |   10 -
 35 files changed, 13434 deletions(-)

If we remove all addresses and interrupts, numbers look even better.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130219/2e26099e/attachment.sig>

hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [130219 11:47]:
> Hi,
> 
> On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
> > > However... if you think you're going to get away with another total
> > > rewrite of OMAP device support away from hwmod to a new scheme with a
> > > new load of huge churn, think again.  Remember, churn is evil.  I've
> > > complained to you about the amount of churn that OMAP manufactures
> > > in the past.  Linus has complained about it too.  You can't continue
> > > like this.
> > 
> > I don't think there's any churn needed here if done properly. It's
> > mostly a question of dropping duplicate data from hwmod that we
> > already have available in device tree. That means we can shrink the
> > hwmod data needed.
> 
> how about starting by removing register addresses and interrupt numbers
> which are passed by devicetree today ? If you want to move to DT-only
> now even without all drivers DT-adapted, you can have something like
> what Marvel folks did for kirkwood:
> 
> static void __init kirkwood_dt_init(void)
> {
> 
> [ ... ]
> 
> 	if (of_machine_is_compatible("globalscale,dreamplug"))
> 		dreamplug_init();
> 
> 	if (of_machine_is_compatible("dlink,dns-kirkwood"))
> 		dnskw_init();
> 
> 	if (of_machine_is_compatible("iom,iconnect"))
> 		iconnect_init();
> 
> [ ... ]
> 
> 	of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
> }
> 
> those machine-based inits are there just to initialize drivers which
> aren't converted to DT.

Yes we could do that at least partially, but..
 
> this would let you remove all board-files except board-generic.c and
> remove data which is already passed in via DT. It has the benefit of
> showing Linus (or whoever cares) that we are working to remove the
> "churn" while also removing some of the pressure of DT conversion, since
> there are still details which need to be sorted out (UART function
> pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
> nodes, etc etc).

..that means massive amount of churn in the board-*.c files to convert
them to various init functions to be called from board-generic.c and
removing the ones that are working with DT.

I think we're better off making first sure things are usable with
DT, then just dropping the board-*.c files as we go.

And omap4 is the place to start as we only have blaze and panda
board files. Once DSS, USB and WLAN work with the .dts files, we
can just drop those board files and make omap4 DT only.

We may be able to drop omap4 board-*.c files faster than going full
DT with few selected legacy init functions in board-generic.c for
things like LCD panel configuration etc.
 
> Only on board-files we're talking about over 13K lines:
> 
>  arch/arm/mach-omap2/board-2430sdp.c          |  289 ------
>  arch/arm/mach-omap2/board-3430sdp.c          |  602 ------------
>  arch/arm/mach-omap2/board-3630sdp.c          |  216 -----
>  arch/arm/mach-omap2/board-4430sdp.c          |  730 ---------------
>  arch/arm/mach-omap2/board-am3517crane.c      |   97 --
>  arch/arm/mach-omap2/board-am3517evm.c        |  398 --------
>  arch/arm/mach-omap2/board-apollon.c          |  342 -------
>  arch/arm/mach-omap2/board-cm-t35.c           |  769 ----------------
>  arch/arm/mach-omap2/board-cm-t3517.c         |  302 ------
>  arch/arm/mach-omap2/board-devkit8000.c       |  648 -------------
>  arch/arm/mach-omap2/board-flash.c            |  242 -----
>  arch/arm/mach-omap2/board-flash.h            |   62 --
>  arch/arm/mach-omap2/board-h4.c               |  347 -------
>  arch/arm/mach-omap2/board-igep0020.c         |  673 --------------
>  arch/arm/mach-omap2/board-ldp.c              |  440 ---------
>  arch/arm/mach-omap2/board-n8x0.c             |  762 ---------------
>  arch/arm/mach-omap2/board-omap3beagle.c      |  549 -----------
>  arch/arm/mach-omap2/board-omap3evm.c         |  762 ---------------
>  arch/arm/mach-omap2/board-omap3logic.c       |  249 -----
>  arch/arm/mach-omap2/board-omap3pandora.c     |  623 -------------
>  arch/arm/mach-omap2/board-omap3stalker.c     |  432 ---------
>  arch/arm/mach-omap2/board-omap3touchbook.c   |  391 --------
>  arch/arm/mach-omap2/board-omap4panda.c       |  467 ----------
>  arch/arm/mach-omap2/board-overo.c            |  556 -----------
>  arch/arm/mach-omap2/board-rm680.c            |  165 ----
>  arch/arm/mach-omap2/board-rx51-peripherals.c | 1276 --------------------------
>  arch/arm/mach-omap2/board-rx51-video.c       |   89 --
>  arch/arm/mach-omap2/board-rx51.c             |  128 ---
>  arch/arm/mach-omap2/board-rx51.h             |   11 -
>  arch/arm/mach-omap2/board-ti8168evm.c        |   62 --
>  arch/arm/mach-omap2/board-zoom-debugboard.c  |  139 ---
>  arch/arm/mach-omap2/board-zoom-display.c     |  147 ---
>  arch/arm/mach-omap2/board-zoom-peripherals.c |  304 ------
>  arch/arm/mach-omap2/board-zoom.c             |  155 ----
>  arch/arm/mach-omap2/board-zoom.h             |   10 -
>  35 files changed, 13434 deletions(-)
> 
> If we remove all addresses and interrupts, numbers look even better.

Yeah. Let's start with omap4 first when DSS + USB + WLAN work.

Regards,

Tony

hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote:
> > On Tue, Feb 19, 2013 at 11:31:22AM -0800, Tony Lindgren wrote:
> > > > However... if you think you're going to get away with another total
> > > > rewrite of OMAP device support away from hwmod to a new scheme with a
> > > > new load of huge churn, think again.  Remember, churn is evil.  I've
> > > > complained to you about the amount of churn that OMAP manufactures
> > > > in the past.  Linus has complained about it too.  You can't continue
> > > > like this.
> > > 
> > > I don't think there's any churn needed here if done properly. It's
> > > mostly a question of dropping duplicate data from hwmod that we
> > > already have available in device tree. That means we can shrink the
> > > hwmod data needed.
> > 
> > how about starting by removing register addresses and interrupt numbers
> > which are passed by devicetree today ? If you want to move to DT-only
> > now even without all drivers DT-adapted, you can have something like
> > what Marvel folks did for kirkwood:
> > 
> > static void __init kirkwood_dt_init(void)
> > {
> > 
> > [ ... ]
> > 
> > 	if (of_machine_is_compatible("globalscale,dreamplug"))
> > 		dreamplug_init();
> > 
> > 	if (of_machine_is_compatible("dlink,dns-kirkwood"))
> > 		dnskw_init();
> > 
> > 	if (of_machine_is_compatible("iom,iconnect"))
> > 		iconnect_init();
> > 
> > [ ... ]
> > 
> > 	of_platform_populate(NULL, kirkwood_dt_match_table, NULL, NULL);
> > }
> > 
> > those machine-based inits are there just to initialize drivers which
> > aren't converted to DT.
> 
> Yes we could do that at least partially, but..
>  
> > this would let you remove all board-files except board-generic.c and
> > remove data which is already passed in via DT. It has the benefit of
> > showing Linus (or whoever cares) that we are working to remove the
> > "churn" while also removing some of the pressure of DT conversion, since
> > there are still details which need to be sorted out (UART function
> > pointers, clock nodes via DT - see Roger's discussion with Rajendra, DMA
> > nodes, etc etc).
> 
> ..that means massive amount of churn in the board-*.c files to convert
> them to various init functions to be called from board-generic.c and
> removing the ones that are working with DT.

why ? I meant that only what's not converted to DT today should be
handled this way. Also, most of the "churn" is already there
(usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called
from a different place. We don't need to have one function for each
board, however, maybe we could target by-soc:

if (of_is_compatible("omap3"))
	omap3_init_devices(); /* or whatever you wanna call it */

omap_init_devices() has initialization for everything which isn't DT
adapted today and as we move things to DT, there's a single place to
remove code from.

> I think we're better off making first sure things are usable with
> DT, then just dropping the board-*.c files as we go.
> 
> And omap4 is the place to start as we only have blaze and panda
> board files. Once DSS, USB and WLAN work with the .dts files, we
> can just drop those board files and make omap4 DT only.

fair enough.

> We may be able to drop omap4 board-*.c files faster than going full
> DT with few selected legacy init functions in board-generic.c for
> things like LCD panel configuration etc.
>  
> > Only on board-files we're talking about over 13K lines:
> > 
> >  arch/arm/mach-omap2/board-2430sdp.c          |  289 ------
> >  arch/arm/mach-omap2/board-3430sdp.c          |  602 ------------
> >  arch/arm/mach-omap2/board-3630sdp.c          |  216 -----
> >  arch/arm/mach-omap2/board-4430sdp.c          |  730 ---------------
> >  arch/arm/mach-omap2/board-am3517crane.c      |   97 --
> >  arch/arm/mach-omap2/board-am3517evm.c        |  398 --------
> >  arch/arm/mach-omap2/board-apollon.c          |  342 -------
> >  arch/arm/mach-omap2/board-cm-t35.c           |  769 ----------------
> >  arch/arm/mach-omap2/board-cm-t3517.c         |  302 ------
> >  arch/arm/mach-omap2/board-devkit8000.c       |  648 -------------
> >  arch/arm/mach-omap2/board-flash.c            |  242 -----
> >  arch/arm/mach-omap2/board-flash.h            |   62 --
> >  arch/arm/mach-omap2/board-h4.c               |  347 -------
> >  arch/arm/mach-omap2/board-igep0020.c         |  673 --------------
> >  arch/arm/mach-omap2/board-ldp.c              |  440 ---------
> >  arch/arm/mach-omap2/board-n8x0.c             |  762 ---------------
> >  arch/arm/mach-omap2/board-omap3beagle.c      |  549 -----------
> >  arch/arm/mach-omap2/board-omap3evm.c         |  762 ---------------
> >  arch/arm/mach-omap2/board-omap3logic.c       |  249 -----
> >  arch/arm/mach-omap2/board-omap3pandora.c     |  623 -------------
> >  arch/arm/mach-omap2/board-omap3stalker.c     |  432 ---------
> >  arch/arm/mach-omap2/board-omap3touchbook.c   |  391 --------
> >  arch/arm/mach-omap2/board-omap4panda.c       |  467 ----------
> >  arch/arm/mach-omap2/board-overo.c            |  556 -----------
> >  arch/arm/mach-omap2/board-rm680.c            |  165 ----
> >  arch/arm/mach-omap2/board-rx51-peripherals.c | 1276 --------------------------
> >  arch/arm/mach-omap2/board-rx51-video.c       |   89 --
> >  arch/arm/mach-omap2/board-rx51.c             |  128 ---
> >  arch/arm/mach-omap2/board-rx51.h             |   11 -
> >  arch/arm/mach-omap2/board-ti8168evm.c        |   62 --
> >  arch/arm/mach-omap2/board-zoom-debugboard.c  |  139 ---
> >  arch/arm/mach-omap2/board-zoom-display.c     |  147 ---
> >  arch/arm/mach-omap2/board-zoom-peripherals.c |  304 ------
> >  arch/arm/mach-omap2/board-zoom.c             |  155 ----
> >  arch/arm/mach-omap2/board-zoom.h             |   10 -
> >  35 files changed, 13434 deletions(-)
> > 
> > If we remove all addresses and interrupts, numbers look even better.
> 
> Yeah. Let's start with omap4 first when DSS + USB + WLAN work.

USB is going to be ready for v3.10, likely Wlan too.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130220/c373cf44/attachment.sig>

hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [130219 14:26]:
> On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote:
> > 
> > ..that means massive amount of churn in the board-*.c files to convert
> > them to various init functions to be called from board-generic.c and
> > removing the ones that are working with DT.
> 
> why ? I meant that only what's not converted to DT today should be
> handled this way. Also, most of the "churn" is already there
> (usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called
> from a different place. We don't need to have one function for each
> board, however, maybe we could target by-soc:
> 
> if (of_is_compatible("omap3"))
> 	omap3_init_devices(); /* or whatever you wanna call it */
> 
> omap_init_devices() has initialization for everything which isn't DT
> adapted today and as we move things to DT, there's a single place to
> remove code from.

And the pdata for that comes from where? :) I think that means
converting each board-*.c to device init functions, which leads to
the churn I was mentioning..
 
> > I think we're better off making first sure things are usable with
> > DT, then just dropping the board-*.c files as we go.
> > 
> > And omap4 is the place to start as we only have blaze and panda
> > board files. Once DSS, USB and WLAN work with the .dts files, we
> > can just drop those board files and make omap4 DT only.
> 
> fair enough.
> 
> > We may be able to drop omap4 board-*.c files faster than going full
> > DT with few selected legacy init functions in board-generic.c for
> > things like LCD panel configuration etc.
> >  
> > > Only on board-files we're talking about over 13K lines:
> > >  35 files changed, 13434 deletions(-)
...

> > > If we remove all addresses and interrupts, numbers look even better.
> > 
> > Yeah. Let's start with omap4 first when DSS + USB + WLAN work.
> 
> USB is going to be ready for v3.10, likely Wlan too.

That's good news. Maybe we can then have a legacy device pdata
init for DSS, and make omap4 DT only for v3.10.

Regards,

Tony

hwmod data duplication (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 02:31:28PM -0800, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [130219 14:26]:
> > On Tue, Feb 19, 2013 at 02:09:33PM -0800, Tony Lindgren wrote:
> > > 
> > > ..that means massive amount of churn in the board-*.c files to convert
> > > them to various init functions to be called from board-generic.c and
> > > removing the ones that are working with DT.
> > 
> > why ? I meant that only what's not converted to DT today should be
> > handled this way. Also, most of the "churn" is already there
> > (usb_musb_init(), usb_ehci_init(), etc etc), it just needs to be called
> > from a different place. We don't need to have one function for each
> > board, however, maybe we could target by-soc:
> > 
> > if (of_is_compatible("omap3"))
> > 	omap3_init_devices(); /* or whatever you wanna call it */
> > 
> > omap_init_devices() has initialization for everything which isn't DT
> > adapted today and as we move things to DT, there's a single place to
> > remove code from.
> 
> And the pdata for that comes from where? :) I think that means
> converting each board-*.c to device init functions, which leads to
> the churn I was mentioning..

fair enough, still a lot less churn ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130220/84e34824/attachment-0001.sig>

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

[Russell King - ARM Linux]

On Thu, Feb 14, 2013 at 09:40:29PM +0000, Paul Walmsley wrote:
> Hi,
> 
> On Thu, 14 Feb 2013, Paul Walmsley wrote:
> 
> > So instead of something bus-specific like that, a better way would be to 
> > use something like:
> > 
> > va = dev->bus->ioremap( ... );
> > va = dev->bus->iounmap( ... );
> 
> Something like this is what I was thinking.  Obviously there would be more
> patches needed, for the various arches, etc.; and I'm not convinced that
> the function pointer init is done at the right time yet. Comments welcome.

Not only does this break the "ioremap caller" stuff, allowing the caller
of ioremap() to be recorded, I believe this to be a total abonimation.

> This might also be useful as a generic replacement for pci_ioremap_bar().

No it isn't.  Read the comments about why pci_ioremap_bar() was introduced
in its original commit.  It's there to reduce the number of common errors
with mapping a PCI bar, so it takes the PCI device and the BAR index.

It has a totally different interface to standard ioremap().  It can't be
twisted into the standard ioremap() interface in any shape or form.

> diff --git a/arch/arm/mm/mmu.c b/arch/arm/mm/mmu.c
> index ce328c7..303dba0 100644
> --- a/arch/arm/mm/mmu.c
> +++ b/arch/arm/mm/mmu.c
> @@ -17,6 +17,7 @@
>  #include <linux/fs.h>
>  #include <linux/vmalloc.h>
>  #include <linux/sizes.h>
> +#include <linux/platform_device.h>
>  
>  #include <asm/cp15.h>
>  #include <asm/cputype.h>
> @@ -28,6 +29,7 @@
>  #include <asm/highmem.h>
>  #include <asm/system_info.h>
>  #include <asm/traps.h>
> +#include <asm/io.h>

How many more bloody times do I have to tell people about using asm/io.h
directly?  You've been around for _long_ enough that you well know this
by now.  There is no excuse for this kind of thing other than shere
laziness or down-right sloppyness on your part.

If I make a big enough thing about it, hopefully people will become scared
to post their un-self-reviewed patches and instead start taking a little
more care in future.

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

[Paul Walmsley]

On Thu, 14 Feb 2013, Paul Walmsley wrote:

> va = dev->bus->iounmap( ... );

And this should simply be

dev->bus->iounmap( ... );

I regret the error...

- Paul

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

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> Hi,
> 
> On Thu, 14 Feb 2013, Tony Lindgren wrote:
> 
> > I don't think so as hwmod should only touch the sysconfig space
> > when no driver has claimed it.
> 
> hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> and resume operations, and during device enable and disable operations.  
> Here's the relevant code:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195

But that's triggered by runtime PM from the device driver, right?

I think it's fine for hwmod to do that as requested by the device
driver via runtime PM since hwmod is the bus glue making the drivers arch
independent.

I think the only piece we're missing for that is for driver to run
something like pm_runtime_init() that initializes the address space
to hwmod. Or just bus specific ioremap like you're suggesting later
on.

Just to recap what we've discussed earlier, the reasons why we want
reset and idle functions should be in the driver specific header are:

1. There's often driver specific logic needed in addition to the
   syconfig tinkering in the reset/idle functions.

2. We need to be able to reset and idle some hardware even if the
   driver is not compiled in.
 
> Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 
> have anything specifically to do with the underlying device IP block, but 
> with the SoC integration, even though they live in the device's 
> memory-mapped address range.  It's unfortunate that TI didn't allocate a 
> separate, small address space for these registers, translated by the bus, 
> similar to the PCI-E Enhanced Configuration Access Mechanism:
> 
> http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space
> 
> ... but that's unfortunately the reality of the OMAP hardware.

Yes I think we all agree on accessing sysconfig in a standardized
way via runtime PM triggered by the driver.
 
> Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as 
> long as the implementation of ioremap() can be overridden by the device's 
> bus.  PCI device drivers already do this -- albeit in a PCI-specific way 
> -- with pci_ioremap_bar():
> 
> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git&a=search&h=HEAD&st=grep&s=pci_ioremap_bar
> 
> So instead of something bus-specific like that, a better way would be to 
> use something like:
> 
> va = dev->bus->ioremap( ... );
> va = dev->bus->iounmap( ... );
> 
> Any driver using such a mechanism would be intrinsically generic. 
> platform_bus could simply set its bus->ioremap to the existing ioremap() 
> function, while buses like a omap_hwmod_bus could use a function that 
> returned the address range that omap_hwmod_bus has already ioremap()ped.

Yeah makes sense to me.

Tony

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

[Felipe Balbi]

On Thu, Feb 14, 2013 at 02:22:47PM -0800, Tony Lindgren wrote:
> * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > Hi,
> > 
> > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > 
> > > I don't think so as hwmod should only touch the sysconfig space
> > > when no driver has claimed it.
> > 
> > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > and resume operations, and during device enable and disable operations.  
> > Here's the relevant code:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> 
> But that's triggered by runtime PM from the device driver, right?
> 
> I think it's fine for hwmod to do that as requested by the device
> driver via runtime PM since hwmod is the bus glue making the drivers arch
> independent.
> 
> I think the only piece we're missing for that is for driver to run
> something like pm_runtime_init() that initializes the address space
> to hwmod. Or just bus specific ioremap like you're suggesting later
> on.
> 
> Just to recap what we've discussed earlier, the reasons why we want
> reset and idle functions should be in the driver specific header are:
> 
> 1. There's often driver specific logic needed in addition to the
>    syconfig tinkering in the reset/idle functions.

how about introducing generic device_reset() and device_idle() hooks
which driver can implement and can be called by bus glue ?

Something like:


diff --git a/include/linux/pm.h b/include/linux/pm.h
index 03d7bb1..9c921e2 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -256,6 +256,8 @@ typedef struct pm_message {
  *	these conditions and handle the device as appropriate, possibly queueing
  *	a suspend request for it.  The return value is ignored by the PM core.
  *
+ * @runtime_reset: Resets the device and puts it in a known state.
+ *
  * Refer to Documentation/power/runtime_pm.txt for more information about the
  * role of the above callbacks in device runtime power management.
  *
@@ -285,6 +287,7 @@ struct dev_pm_ops {
 	int (*runtime_suspend)(struct device *dev);
 	int (*runtime_resume)(struct device *dev);
 	int (*runtime_idle)(struct device *dev);
+	int (*runtime_reset)(struct device *dev);
 };
 
 #ifdef CONFIG_PM_SLEEP


We already have ->runtime_idle() which we could be using...

> 2. We need to be able to reset and idle some hardware even if the
>    driver is not compiled in.

yeah, this will be tricky. Even if you try late_initcall() there could
still be issues with -EPROBE_DEFER. Maybe you don't need to reset the
IPs but rather put those which have status = "disabled" to FORCE_IDLE.

Wouldn't that be enough ? If a driver claims the device later, then
pm_runtime_enable() + pm_runtime_get() will change that.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/83010646/attachment.sig>

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

[Bedia, Vaibhav]

On Fri, Feb 15, 2013 at 12:23:08, Balbi, Felipe wrote:
> On Thu, Feb 14, 2013 at 02:22:47PM -0800, Tony Lindgren wrote:
> > * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > > Hi,
> > > 
> > > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > > 
> > > > I don't think so as hwmod should only touch the sysconfig space
> > > > when no driver has claimed it.
> > > 
> > > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > > and resume operations, and during device enable and disable operations.  
> > > Here's the relevant code:
> > > 
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > > 
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > 
> > But that's triggered by runtime PM from the device driver, right?
> > 
> > I think it's fine for hwmod to do that as requested by the device
> > driver via runtime PM since hwmod is the bus glue making the drivers arch
> > independent.
> > 
> > I think the only piece we're missing for that is for driver to run
> > something like pm_runtime_init() that initializes the address space
> > to hwmod. Or just bus specific ioremap like you're suggesting later
> > on.
> > 
> > Just to recap what we've discussed earlier, the reasons why we want
> > reset and idle functions should be in the driver specific header are:
> > 
> > 1. There's often driver specific logic needed in addition to the
> >    syconfig tinkering in the reset/idle functions.
> 
> how about introducing generic device_reset() and device_idle() hooks
> which driver can implement and can be called by bus glue ?
> 
> Something like:
> 
> 
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index 03d7bb1..9c921e2 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -256,6 +256,8 @@ typedef struct pm_message {
>   *	these conditions and handle the device as appropriate, possibly queueing
>   *	a suspend request for it.  The return value is ignored by the PM core.
>   *
> + * @runtime_reset: Resets the device and puts it in a known state.
> + *
>   * Refer to Documentation/power/runtime_pm.txt for more information about the
>   * role of the above callbacks in device runtime power management.
>   *
> @@ -285,6 +287,7 @@ struct dev_pm_ops {
>  	int (*runtime_suspend)(struct device *dev);
>  	int (*runtime_resume)(struct device *dev);
>  	int (*runtime_idle)(struct device *dev);
> +	int (*runtime_reset)(struct device *dev);
>  };
>  

I am not a runtime PM expert but runtime_reset() looks a good option to me.
I expect most of the drivers won't need to do anything different from what
the hwmod code already does. IPs which have special reset requirements
can either extend the defaults ops or just override it. On AM33xx there
are some special requirements for CPSW, DCAN, PWMSS reset handling but
AFAIK none of the other IPs need to do anything special and we don't want
to duplicate the reset code in all of them.

Regards,
Vaibhav

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

[Paul Walmsley]

Hi,

On Thu, 14 Feb 2013, Tony Lindgren wrote:

> * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > 
> > > I don't think so as hwmod should only touch the sysconfig space
> > > when no driver has claimed it.
> > 
> > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > and resume operations, and during device enable and disable operations.  
> > Here's the relevant code:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> 
> But that's triggered by runtime PM from the device driver, right?

Yes - for devices with drivers, it will hopefully be called by the 
driver.

> I think it's fine for hwmod to do that as requested by the device
> driver via runtime PM since hwmod is the bus glue making the drivers arch
> independent.
> 
> I think the only piece we're missing for that is for driver to run
> something like pm_runtime_init() that initializes the address space
> to hwmod. 

In the current design, we might be able to do this during the driver's 
first call to pm_runtime_get*().  I think that's the first point that we 
can hook into the PM runtime code.

Once the hwmod code is moved out to be a bus, I'm hoping we can do this 
through the driver making a dev->bus->enable_device() call - similar to 
the way PCI drivers call pci_enable_device(), but not bus-specific.  That 
should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices 
to work correctly :-(

> Just to recap what we've discussed earlier, the reasons why we want
> reset and idle functions should be in the driver specific header are:
> 
> 1. There's often driver specific logic needed in addition to the
>    syconfig tinkering in the reset/idle functions.

It's been a while since I last tabulated this.  But my recollection was 
that some kind of IP block-specific reset code is needed for about 7% to 
10% of the OMAP IP blocks.

One thing that's unclear to me for the DT case is how we'll bind the 
driver-specific parts of the reset code to the device, particularly in the 
case where there's no driver present.  It should be possible to place an 
initcall in the driver-specific header, but that seems like a hack.  Any 
thoughts on this?

> 2. We need to be able to reset and idle some hardware even if the
>    driver is not compiled in.

Yep.


- Paul

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

[Tony Lindgren]

* Paul Walmsley <paul@pwsan.com> [130219 07:30]:
> Hi,
> 
> On Thu, 14 Feb 2013, Tony Lindgren wrote:
> 
> > * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > > 
> > > > I don't think so as hwmod should only touch the sysconfig space
> > > > when no driver has claimed it.
> > > 
> > > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > > and resume operations, and during device enable and disable operations.  
> > > Here's the relevant code:
> > > 
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > > 
> > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > 
> > But that's triggered by runtime PM from the device driver, right?
> 
> Yes - for devices with drivers, it will hopefully be called by the 
> driver.
> 
> > I think it's fine for hwmod to do that as requested by the device
> > driver via runtime PM since hwmod is the bus glue making the drivers arch
> > independent.
> > 
> > I think the only piece we're missing for that is for driver to run
> > something like pm_runtime_init() that initializes the address space
> > to hwmod. 
> 
> In the current design, we might be able to do this during the driver's 
> first call to pm_runtime_get*().  I think that's the first point that we 
> can hook into the PM runtime code.

Sounds doable and generic for now.
 
> Once the hwmod code is moved out to be a bus, I'm hoping we can do this 
> through the driver making a dev->bus->enable_device() call - similar to 
> the way PCI drivers call pci_enable_device(), but not bus-specific.  That 
> should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices 
> to work correctly :-(
> 
> > Just to recap what we've discussed earlier, the reasons why we want
> > reset and idle functions should be in the driver specific header are:
> > 
> > 1. There's often driver specific logic needed in addition to the
> >    syconfig tinkering in the reset/idle functions.
> 
> It's been a while since I last tabulated this.  But my recollection was 
> that some kind of IP block-specific reset code is needed for about 7% to 
> 10% of the OMAP IP blocks.

Yes it's not too many, but the issue there is the driver specific code
that's duplicated in both places. And sounds like the solution to that
is to make driver specific reset code a static inline function in the
driver header as discussed earlier so bus code can call it when there's
no driver loaded.
 
> One thing that's unclear to me for the DT case is how we'll bind the 
> driver-specific parts of the reset code to the device, particularly in the 
> case where there's no driver present.  It should be possible to place an 
> initcall in the driver-specific header, but that seems like a hack.  Any 
> thoughts on this?

I think we can just initialize the driver-specific reset functions
in hwmod code and those can just call the driver specific inline functions
as needed in the driver specific header files.
 
> > 2. We need to be able to reset and idle some hardware even if the
> >    driver is not compiled in.
> 
> Yep.

Regards,

Tony

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

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote:
> > > * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > > > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > > > 
> > > > > I don't think so as hwmod should only touch the sysconfig space
> > > > > when no driver has claimed it.
> > > > 
> > > > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > > > and resume operations, and during device enable and disable operations.  
> > > > Here's the relevant code:
> > > > 
> > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > > > 
> > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > > 
> > > But that's triggered by runtime PM from the device driver, right?
> > 
> > Yes - for devices with drivers, it will hopefully be called by the 
> > driver.
> > 
> > > I think it's fine for hwmod to do that as requested by the device
> > > driver via runtime PM since hwmod is the bus glue making the drivers arch
> > > independent.
> > > 
> > > I think the only piece we're missing for that is for driver to run
> > > something like pm_runtime_init() that initializes the address space
> > > to hwmod. 
> > 
> > In the current design, we might be able to do this during the driver's 
> > first call to pm_runtime_get*().  I think that's the first point that we 
> > can hook into the PM runtime code.
> 
> Sounds doable and generic for now.

this isn't really a nice way IMHO. You will end up with drivers assuming
that certain details will be initialized whenever it calls
pm_runtime_get() for the first time. Then, for devices such as UART
which, when used as console, will not get pm_domain->runtime_resume()
called because pm_runtime_set_active() has been called right before.

IOW, this will not work for all cases and very soon we will have bug
reports.

> > Once the hwmod code is moved out to be a bus, I'm hoping we can do this 
> > through the driver making a dev->bus->enable_device() call - similar to 
> > the way PCI drivers call pci_enable_device(), but not bus-specific.  That 
> > should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices 
> > to work correctly :-(
> > 
> > > Just to recap what we've discussed earlier, the reasons why we want
> > > reset and idle functions should be in the driver specific header are:
> > > 
> > > 1. There's often driver specific logic needed in addition to the
> > >    syconfig tinkering in the reset/idle functions.
> > 
> > It's been a while since I last tabulated this.  But my recollection was 
> > that some kind of IP block-specific reset code is needed for about 7% to 
> > 10% of the OMAP IP blocks.
> 
> Yes it's not too many, but the issue there is the driver specific code
> that's duplicated in both places. And sounds like the solution to that
> is to make driver specific reset code a static inline function in the
> driver header as discussed earlier so bus code can call it when there's
> no driver loaded.

this wouldn't work. How would you write a reset for i2c which can be
called for all instances even when there is no driver loaded ? What
would be the arguments to such a call ?

If you have something like:

static inline void omap_i2c_reset(int bus_nr);

you would need to keep track of the bus numbers even when there is no
driver around and if there is a driver, you would have to ask i2c-omap.c
to track all available instances and convert that number to the proper
struct omap_i2c pointer. None of those sound like a good plan IMO.

>From a driver perspective, we want all our calls to have our private
structure or a *dev as argument. Those are only avaiable when driver is
around and anything which doesn't take those as argument will force
driver to add extra unnecessary churn.

> > One thing that's unclear to me for the DT case is how we'll bind the 
> > driver-specific parts of the reset code to the device, particularly in the 
> > case where there's no driver present.  It should be possible to place an 
> > initcall in the driver-specific header, but that seems like a hack.  Any 
> > thoughts on this?
> 
> I think we can just initialize the driver-specific reset functions
> in hwmod code and those can just call the driver specific inline functions
> as needed in the driver specific header files.

please don't. This will not work as you expect, we will just add more
special cases to drivers which will be even more complex to maintain.

What you want is a generic reset callback somewhere. For cases where
there is no driver around, then I'm not sure what to do, but doing early
reset sounds like a better plan than late reset, so resetting on
omap_device_build_from_dt() sounds better IMHO, although it concerns me
cases of -EPROBE_DEFER and what that would mean in the long run...

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130219/20fd25e4/attachment.sig>

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

[Tony Lindgren]

* Felipe Balbi <balbi@ti.com> [130219 09:01]:
> Hi,
> 
> On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote:
> > > > * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > > > > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > > > > 
> > > > > > I don't think so as hwmod should only touch the sysconfig space
> > > > > > when no driver has claimed it.
> > > > > 
> > > > > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > > > > and resume operations, and during device enable and disable operations.  
> > > > > Here's the relevant code:
> > > > > 
> > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > > > > 
> > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > > > 
> > > > But that's triggered by runtime PM from the device driver, right?
> > > 
> > > Yes - for devices with drivers, it will hopefully be called by the 
> > > driver.
> > > 
> > > > I think it's fine for hwmod to do that as requested by the device
> > > > driver via runtime PM since hwmod is the bus glue making the drivers arch
> > > > independent.
> > > > 
> > > > I think the only piece we're missing for that is for driver to run
> > > > something like pm_runtime_init() that initializes the address space
> > > > to hwmod. 
> > > 
> > > In the current design, we might be able to do this during the driver's 
> > > first call to pm_runtime_get*().  I think that's the first point that we 
> > > can hook into the PM runtime code.
> > 
> > Sounds doable and generic for now.
> 
> this isn't really a nice way IMHO. You will end up with drivers assuming
> that certain details will be initialized whenever it calls
> pm_runtime_get() for the first time. Then, for devices such as UART
> which, when used as console, will not get pm_domain->runtime_resume()
> called because pm_runtime_set_active() has been called right before.
> 
> IOW, this will not work for all cases and very soon we will have bug
> reports.

Some generic way to init this from drivers is needed, I'm out of ideas
here though.
 
> > > Once the hwmod code is moved out to be a bus, I'm hoping we can do this 
> > > through the driver making a dev->bus->enable_device() call - similar to 
> > > the way PCI drivers call pci_enable_device(), but not bus-specific.  That 
> > > should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices 
> > > to work correctly :-(
> > > 
> > > > Just to recap what we've discussed earlier, the reasons why we want
> > > > reset and idle functions should be in the driver specific header are:
> > > > 
> > > > 1. There's often driver specific logic needed in addition to the
> > > >    syconfig tinkering in the reset/idle functions.
> > > 
> > > It's been a while since I last tabulated this.  But my recollection was 
> > > that some kind of IP block-specific reset code is needed for about 7% to 
> > > 10% of the OMAP IP blocks.
> > 
> > Yes it's not too many, but the issue there is the driver specific code
> > that's duplicated in both places. And sounds like the solution to that
> > is to make driver specific reset code a static inline function in the
> > driver header as discussed earlier so bus code can call it when there's
> > no driver loaded.
> 
> this wouldn't work. How would you write a reset for i2c which can be
> called for all instances even when there is no driver loaded ? What
> would be the arguments to such a call ?
> 
> If you have something like:
> 
> static inline void omap_i2c_reset(int bus_nr);
> 
> you would need to keep track of the bus numbers even when there is no
> driver around and if there is a driver, you would have to ask i2c-omap.c
> to track all available instances and convert that number to the proper
> struct omap_i2c pointer. None of those sound like a good plan IMO.
> 
> From a driver perspective, we want all our calls to have our private
> structure or a *dev as argument. Those are only avaiable when driver is
> around and anything which doesn't take those as argument will force
> driver to add extra unnecessary churn.

If we cannot find a clean way to reset unclaimed devices without duplicating
driver specific code at the bus level, then let's just assume the drivers 
need to probe to reset them even if the device is not used on a particular
board.
 
> > > One thing that's unclear to me for the DT case is how we'll bind the 
> > > driver-specific parts of the reset code to the device, particularly in the 
> > > case where there's no driver present.  It should be possible to place an 
> > > initcall in the driver-specific header, but that seems like a hack.  Any 
> > > thoughts on this?
> > 
> > I think we can just initialize the driver-specific reset functions
> > in hwmod code and those can just call the driver specific inline functions
> > as needed in the driver specific header files.
> 
> please don't. This will not work as you expect, we will just add more
> special cases to drivers which will be even more complex to maintain.
> 
> What you want is a generic reset callback somewhere. For cases where
> there is no driver around, then I'm not sure what to do, but doing early
> reset sounds like a better plan than late reset, so resetting on
> omap_device_build_from_dt() sounds better IMHO, although it concerns me
> cases of -EPROBE_DEFER and what that would mean in the long run...

Well this problem too will disappear if we require the drivers to probe
to reset things properly.

Then the hwmod late_init would just be a printk warning about unclaimed
devices.

In any case, we should not reset things early unless requested from driver
probe. Too many things can go wrong before we have a proper console
available.

Regards,

Tony

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

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 09:43:36AM -0800, Tony Lindgren wrote:
> > On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote:
> > > > > * Paul Walmsley <paul@pwsan.com> [130214 12:51]:
> > > > > > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > > > > > 
> > > > > > > I don't think so as hwmod should only touch the sysconfig space
> > > > > > > when no driver has claimed it.
> > > > > > 
> > > > > > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > > > > > and resume operations, and during device enable and disable operations.  
> > > > > > Here's the relevant code:
> > > > > > 
> > > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > > > > > 
> > > > > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > > > > 
> > > > > But that's triggered by runtime PM from the device driver, right?
> > > > 
> > > > Yes - for devices with drivers, it will hopefully be called by the 
> > > > driver.
> > > > 
> > > > > I think it's fine for hwmod to do that as requested by the device
> > > > > driver via runtime PM since hwmod is the bus glue making the drivers arch
> > > > > independent.
> > > > > 
> > > > > I think the only piece we're missing for that is for driver to run
> > > > > something like pm_runtime_init() that initializes the address space
> > > > > to hwmod. 
> > > > 
> > > > In the current design, we might be able to do this during the driver's 
> > > > first call to pm_runtime_get*().  I think that's the first point that we 
> > > > can hook into the PM runtime code.
> > > 
> > > Sounds doable and generic for now.
> > 
> > this isn't really a nice way IMHO. You will end up with drivers assuming
> > that certain details will be initialized whenever it calls
> > pm_runtime_get() for the first time. Then, for devices such as UART
> > which, when used as console, will not get pm_domain->runtime_resume()
> > called because pm_runtime_set_active() has been called right before.
> > 
> > IOW, this will not work for all cases and very soon we will have bug
> > reports.
> 
> Some generic way to init this from drivers is needed, I'm out of ideas
> here though.

that's fair, but exposing driver-specific details via a static inline in
a header which might or might not be called from hwmod isn't the best
choice IMHO.

the problems this could cause are numerous, specially when you consider
drivers which are shared among multiple architectures (think musb, dwc3,
ehci, ohci, xhci, ahci_platform, etc).

> > > > Once the hwmod code is moved out to be a bus, I'm hoping we can do this 
> > > > through the driver making a dev->bus->enable_device() call - similar to 
> > > > the way PCI drivers call pci_enable_device(), but not bus-specific.  That 
> > > > should remove our current dependency on CONFIG_PM_RUNTIME for OMAP devices 
> > > > to work correctly :-(
> > > > 
> > > > > Just to recap what we've discussed earlier, the reasons why we want
> > > > > reset and idle functions should be in the driver specific header are:
> > > > > 
> > > > > 1. There's often driver specific logic needed in addition to the
> > > > >    syconfig tinkering in the reset/idle functions.
> > > > 
> > > > It's been a while since I last tabulated this.  But my recollection was 
> > > > that some kind of IP block-specific reset code is needed for about 7% to 
> > > > 10% of the OMAP IP blocks.
> > > 
> > > Yes it's not too many, but the issue there is the driver specific code
> > > that's duplicated in both places. And sounds like the solution to that
> > > is to make driver specific reset code a static inline function in the
> > > driver header as discussed earlier so bus code can call it when there's
> > > no driver loaded.
> > 
> > this wouldn't work. How would you write a reset for i2c which can be
> > called for all instances even when there is no driver loaded ? What
> > would be the arguments to such a call ?
> > 
> > If you have something like:
> > 
> > static inline void omap_i2c_reset(int bus_nr);
> > 
> > you would need to keep track of the bus numbers even when there is no
> > driver around and if there is a driver, you would have to ask i2c-omap.c
> > to track all available instances and convert that number to the proper
> > struct omap_i2c pointer. None of those sound like a good plan IMO.
> > 
> > From a driver perspective, we want all our calls to have our private
> > structure or a *dev as argument. Those are only avaiable when driver is
> > around and anything which doesn't take those as argument will force
> > driver to add extra unnecessary churn.
> 
> If we cannot find a clean way to reset unclaimed devices without duplicating
> driver specific code at the bus level, then let's just assume the drivers 
> need to probe to reset them even if the device is not used on a particular
> board.

that wouldn't work either. Whenever you have a product which wants to
optimize memory footprint, will start by disabling unused modules, then
all of your problems are back and we will start to receive bug reports
because of an artificial requirement created by OMAP ;-)

hwmod already has access to all device addresses, so why not reset early
when you build the omap device ?

How about this completely untested patch ? I guess it won't work because
I'm not making sure clocks are enabled for the particular hwmod to be
accessible, but it shows the idea:

diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
index e065daa..58de594 100644
--- a/arch/arm/mach-omap2/omap_device.c
+++ b/arch/arm/mach-omap2/omap_device.c
@@ -354,6 +354,13 @@ static int omap_device_build_from_dt(struct platform_device *pdev)
 			goto odbfd_exit1;
 		}
 		hwmods[i] = oh;
+
+		/*
+		 * reset all hwmods early on, but even if it fails to reset
+		 * let it go through, driver might still be able to recover.
+		 */
+		ret = omap_hwmod_reset(oh);
+		WARN(ret < 0, "%s failed to reset\n", oh->name);
 	}
 
 	od = omap_device_alloc(pdev, hwmods, oh_cnt, NULL, 0);

> > > > One thing that's unclear to me for the DT case is how we'll bind the 
> > > > driver-specific parts of the reset code to the device, particularly in the 
> > > > case where there's no driver present.  It should be possible to place an 
> > > > initcall in the driver-specific header, but that seems like a hack.  Any 
> > > > thoughts on this?
> > > 
> > > I think we can just initialize the driver-specific reset functions
> > > in hwmod code and those can just call the driver specific inline functions
> > > as needed in the driver specific header files.
> > 
> > please don't. This will not work as you expect, we will just add more
> > special cases to drivers which will be even more complex to maintain.
> > 
> > What you want is a generic reset callback somewhere. For cases where
> > there is no driver around, then I'm not sure what to do, but doing early
> > reset sounds like a better plan than late reset, so resetting on
> > omap_device_build_from_dt() sounds better IMHO, although it concerns me
> > cases of -EPROBE_DEFER and what that would mean in the long run...
> 
> Well this problem too will disappear if we require the drivers to probe
> to reset things properly.

see above, you can't guarantee that, not to mention that this is a too
vague requirement. If a platform doesn't use a driver, why would they
enable it ? And how do you make sure that platform A needs the full
driver but platform B only needs it for a quick reset ? Why would we
allocate memory, IRQ, i/o memory space (ioremap), enable clocks and so
many more resources just for that ? Besides, that would already cause
problems since every DT node with status = "disabled" will not probe.

Changing DT data today to status = enabled by default won't cut since we
already lost control over DT data. I mean, we don't know which
bootloaders are supporting which DT version and relying on appended DTB
only will not work in the long run since the image will get too large
when you want to support multiplatform.

> Then the hwmod late_init would just be a printk warning about unclaimed
> devices.

is that even necessary ? No other architecture (not even other ARM SoCs)
make such distinction, why would we be different ?

> In any case, we should not reset things early unless requested from driver
> probe. Too many things can go wrong before we have a proper console
> available.

OTOH, initializing from a known state would be much, much better and
would force us to remove *all* bootloader dependencies.

We just need to figure out how to handle console until omap uart is
reset, but I guess 8250 is already taking care of that for us, right ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130219/9d7f61fa/attachment-0001.sig>

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

[Kevin Hilman]

[...]

>> > Just to recap what we've discussed earlier, the reasons why we want
>> > reset and idle functions should be in the driver specific header are:
>> > 
>> > 1. There's often driver specific logic needed in addition to the
>> >    syconfig tinkering in the reset/idle functions.
>> 
>> It's been a while since I last tabulated this.  But my recollection was 
>> that some kind of IP block-specific reset code is needed for about 7% to 
>> 10% of the OMAP IP blocks.
>
> Yes it's not too many, but the issue there is the driver specific code
> that's duplicated in both places. And sounds like the solution to that
> is to make driver specific reset code a static inline function in the
> driver header as discussed earlier so bus code can call it when there's
> no driver loaded.

This thread is going in many directions and I've lost track.  

I think we need to separate out the specific reset cases and look at
examples, since there are not very many (in theory).  Specifically, IMO,
we need to separate out the init-time reset needs from the runtime reset
needs.

It seems to me that init-time reset issues can (and should) be handled
by omap_device/omap_hwmod.  For devices with drivers, this can be done
easily before driver probe (using bus notifiers).  Any remaining devices
that specifically need reset can be reset later (which devices are
these?)

The other problem is the where reset is need during runtime.  Again,
what are the specific examples here?  The one I can think of off the top
of my head is I2C, where it's needed in certain error recovery
scenarios.

Here, we need a way for the driver itself to initiate a full reset.
Maybe a new runtime PM hook like ->runtime_reset() as Felipe has
suggested (though I'm not yet sure runtime PM is the right place for
this, since it's not strictly related to runtime PM).  Or, if these are
mostly corner-case, error recovery scenarios, maybe a a way force the
driver to remove itself and re-probe (which would trigger the
bus-notifier based reset) as Tony has suggested is the better approach.

Either way, it would be good to summarize the devices that need this to
be sure what the actual needs are.

The OMAP I2C driver is doing a reset and fully re-initializing itself,
so it seems the forced remove/probe is the right approach there.  Any
one have the other use cases handy?

Kevin

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

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 11:16:38AM -0800, Kevin Hilman wrote:

[ ... ]

> The other problem is the where reset is need during runtime.  Again,
> what are the specific examples here?  The one I can think of off the top
> of my head is I2C, where it's needed in certain error recovery
> scenarios.

right, I still have a theory that it's not needed in that case either,
though I haven't had time to try that out.

> Here, we need a way for the driver itself to initiate a full reset.
> Maybe a new runtime PM hook like ->runtime_reset() as Felipe has
> suggested (though I'm not yet sure runtime PM is the right place for
> this, since it's not strictly related to runtime PM).  Or, if these are

right, I agree with you but I couldn't think of a better place. Maybe we
need a reset hook in struct device itself (as I suggested on another
mail) but I'm not sure Greg would take it, unless we have a damn good
reason.

> mostly corner-case, error recovery scenarios, maybe a a way force the
> driver to remove itself and re-probe (which would trigger the
> bus-notifier based reset) as Tony has suggested is the better approach.

I don't think so. We certainly don't need to go through all memory
allocations again. That will just cause unnecessary memory
fragmentation.

> The OMAP I2C driver is doing a reset and fully re-initializing itself,
> so it seems the forced remove/probe is the right approach there.  Any

I must disagree here. Doing a reprobe of I2C every time there's an error
condition is overkill. It means freeing struct omap_i2c_dev, freeing its
IRQ, freeing the ioremapped area, changing mux configuration (due to
pinctrl calls) just to do it all over again. Besides omap_i2c_probe()
calls omap_i2c_init() which will do the full IP reset anyway.

The difference is that calling omap_i2c_init() directly doesn't force us
to free and reallocate all resources all over again.

> one have the other use cases handy?

dwc3 gets reset during probe() too, but that has an IP specific reset
which doesn't need SYSCONFIG register for that.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130219/4311af94/attachment.sig>

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

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Tue, Feb 19, 2013 at 11:16:38AM -0800, Kevin Hilman wrote:
>
> [ ... ]
>
>> The other problem is the where reset is need during runtime.  Again,
>> what are the specific examples here?  The one I can think of off the top
>> of my head is I2C, where it's needed in certain error recovery
>> scenarios.
>
> right, I still have a theory that it's not needed in that case either,
> though I haven't had time to try that out.

Great, I hope it's not needed.  I've asked several times in past as this
driver was converted to runtime PM, but was told it was required for
various reasons (which I can't remember anymore.)

>> Here, we need a way for the driver itself to initiate a full reset.
>> Maybe a new runtime PM hook like ->runtime_reset() as Felipe has
>> suggested (though I'm not yet sure runtime PM is the right place for
>> this, since it's not strictly related to runtime PM).  Or, if these are
>
> right, I agree with you but I couldn't think of a better place. Maybe we
> need a reset hook in struct device itself (as I suggested on another
> mail) but I'm not sure Greg would take it, unless we have a damn good
> reason.

I'm starting to think a ->hardreset() method in struct dev_pm_ops is the
place to put this.  

IMO, it needs to be in dev_pm_ops, so it can be selectively overridden
by PM domains.  On OMAP, we'd just hook it up to omap_device_shutdown()
for all omap_devices, and we'd be done.

>> mostly corner-case, error recovery scenarios, maybe a a way force the
>> driver to remove itself and re-probe (which would trigger the
>> bus-notifier based reset) as Tony has suggested is the better approach.
>
> I don't think so. We certainly don't need to go through all memory
> allocations again. That will just cause unnecessary memory
> fragmentation.
>
>> The OMAP I2C driver is doing a reset and fully re-initializing itself,
>> so it seems the forced remove/probe is the right approach there.  Any
>
> I must disagree here. Doing a reprobe of I2C every time there's an error
> condition is overkill. It means freeing struct omap_i2c_dev, freeing its
> IRQ, freeing the ioremapped area, changing mux configuration (due to
> pinctrl calls) just to do it all over again. Besides omap_i2c_probe()
> calls omap_i2c_init() which will do the full IP reset anyway.
>
> The difference is that calling omap_i2c_init() directly doesn't force us
> to free and reallocate all resources all over again.

I agree, that would be a lot of churn.

>> one have the other use cases handy?
>
> dwc3 gets reset during probe() too, but that has an IP specific reset
> which doesn't need SYSCONFIG register for that.

OK, but that's still an init-time reset issue

Do you know of any other runtime reset cases?  If I2C is the only one,
then maybe seeing if it can be removed is the right first step.  If that
works, we don't need any of this.

Kevin

OMAP reset requirements (was: Re: [RFC/NOT FOR MERGING 2/3] serial: omap: remove hwmod dependency)^[:x

[Felipe Balbi]

Hi,

On Tue, Feb 19, 2013 at 11:50:37AM -0800, Kevin Hilman wrote:
> >> The other problem is the where reset is need during runtime.  Again,
> >> what are the specific examples here?  The one I can think of off the top
> >> of my head is I2C, where it's needed in certain error recovery
> >> scenarios.
> >
> > right, I still have a theory that it's not needed in that case either,
> > though I haven't had time to try that out.
> 
> Great, I hope it's not needed.  I've asked several times in past as this
> driver was converted to runtime PM, but was told it was required for
> various reasons (which I can't remember anymore.)

:-) just like I2C's revision register was broken and had no useful data
or MUSB mode1 couldn't be made to work... right. Looking at the
functional spec, there's no mention that we need to driver reset signal
anywhere, we _do_ need to reinitialize the transfer-related registers
(I2C_CON, I2C_CNT, I2C_SA), other than that, I can't think of a need for
driving the reset signal.

The bus specification isn't that stupid right ? Arbitration Lost and
NACK are both expected bus conditions. My only concern is write
underflow and read overflow, but I haven't been able to force that to
happen yet. Even in those cases, I'd be very surprised if we really
needed to driver the reset signal.

> >> Here, we need a way for the driver itself to initiate a full reset.
> >> Maybe a new runtime PM hook like ->runtime_reset() as Felipe has
> >> suggested (though I'm not yet sure runtime PM is the right place for
> >> this, since it's not strictly related to runtime PM).  Or, if these are
> >
> > right, I agree with you but I couldn't think of a better place. Maybe we
> > need a reset hook in struct device itself (as I suggested on another
> > mail) but I'm not sure Greg would take it, unless we have a damn good
> > reason.
> 
> I'm starting to think a ->hardreset() method in struct dev_pm_ops is the
> place to put this.  
> 
> IMO, it needs to be in dev_pm_ops, so it can be selectively overridden
> by PM domains.  On OMAP, we'd just hook it up to omap_device_shutdown()
> for all omap_devices, and we'd be done.

do you mean omap_device_shutdown(); omap_device_enable(); ?

> >> one have the other use cases handy?
> >
> > dwc3 gets reset during probe() too, but that has an IP specific reset
> > which doesn't need SYSCONFIG register for that.
> 
> OK, but that's still an init-time reset issue

yeah, and this is pretty mandatory otherwise dwc3 and its PHYs (USB3
pair plus USB2) won't synchronize and we'll have all sorts of issues :-)

> Do you know of any other runtime reset cases?  If I2C is the only one,
> then maybe seeing if it can be removed is the right first step.  If that
> works, we don't need any of this.

I don't know of any other, but there might be a few... On the other
hand, I doubt there's any of them which is truly necessary, unless
there's a bug in the IP which we can solve by issuing a reset, other
than that, driving the reset signal shouldn't be necessary outside of
probe() (or something before probe()).

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130219/cb672f14/attachment.sig>

OMAP reset requirements

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Tue, Feb 19, 2013 at 11:50:37AM -0800, Kevin Hilman wrote:
>> >> The other problem is the where reset is need during runtime.  Again,
>> >> what are the specific examples here?  The one I can think of off the top
>> >> of my head is I2C, where it's needed in certain error recovery
>> >> scenarios.
>> >
>> > right, I still have a theory that it's not needed in that case either,
>> > though I haven't had time to try that out.
>> 
>> Great, I hope it's not needed.  I've asked several times in past as this
>> driver was converted to runtime PM, but was told it was required for
>> various reasons (which I can't remember anymore.)
>
> :-) just like I2C's revision register was broken and had no useful data
> or MUSB mode1 couldn't be made to work... right. Looking at the
> functional spec, there's no mention that we need to driver reset signal
> anywhere, we _do_ need to reinitialize the transfer-related registers
> (I2C_CON, I2C_CNT, I2C_SA), other than that, I can't think of a need for
> driving the reset signal.
>
> The bus specification isn't that stupid right ? Arbitration Lost and
> NACK are both expected bus conditions. My only concern is write
> underflow and read overflow, but I haven't been able to force that to
> happen yet. Even in those cases, I'd be very surprised if we really
> needed to driver the reset signal.

IIRC, it's definitely related to recovering from underflow/overflow.

>> >> Here, we need a way for the driver itself to initiate a full reset.
>> >> Maybe a new runtime PM hook like ->runtime_reset() as Felipe has
>> >> suggested (though I'm not yet sure runtime PM is the right place for
>> >> this, since it's not strictly related to runtime PM).  Or, if these are
>> >
>> > right, I agree with you but I couldn't think of a better place. Maybe we
>> > need a reset hook in struct device itself (as I suggested on another
>> > mail) but I'm not sure Greg would take it, unless we have a damn good
>> > reason.
>> 
>> I'm starting to think a ->hardreset() method in struct dev_pm_ops is the
>> place to put this.  
>> 
>> IMO, it needs to be in dev_pm_ops, so it can be selectively overridden
>> by PM domains.  On OMAP, we'd just hook it up to omap_device_shutdown()
>> for all omap_devices, and we'd be done.
>
> do you mean omap_device_shutdown(); omap_device_enable(); ?

Possibly, depending on the current state of the device.  That method
would have to be smart enough to know the current state of the device,
and return with it in the same state so as not to confuse runtime PM
usecounting.

>> >> one have the other use cases handy?
>> >
>> > dwc3 gets reset during probe() too, but that has an IP specific reset
>> > which doesn't need SYSCONFIG register for that.
>> 
>> OK, but that's still an init-time reset issue
>
> yeah, and this is pretty mandatory otherwise dwc3 and its PHYs (USB3
> pair plus USB2) won't synchronize and we'll have all sorts of issues :-)
>
>> Do you know of any other runtime reset cases?  If I2C is the only one,
>> then maybe seeing if it can be removed is the right first step.  If that
>> works, we don't need any of this.
>
> I don't know of any other, but there might be a few... On the other
> hand, I doubt there's any of them which is truly necessary, unless
> there's a bug in the IP which we can solve by issuing a reset, other
> than that, driving the reset signal shouldn't be necessary outside of
> probe() (or something before probe()).

If that's the case, then the approach should be to focus on sorting out
the drivers that actually need to reset outside of init/probe before
going and trying to extend the driver model.

Combined with solving the init/probe time reset using bus notifiers (for
devices with drivers) and late initcall for the rest should solve all
the problems, I think.  

Am I missing anything?

Kevin

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

[Santosh Shilimkar]

On Wednesday 20 February 2013 12:46 AM, Kevin Hilman wrote:
> [...]
>
>>>> Just to recap what we've discussed earlier, the reasons why we want
>>>> reset and idle functions should be in the driver specific header are:
>>>>
>>>> 1. There's often driver specific logic needed in addition to the
>>>>     syconfig tinkering in the reset/idle functions.
>>>
>>> It's been a while since I last tabulated this.  But my recollection was
>>> that some kind of IP block-specific reset code is needed for about 7% to
>>> 10% of the OMAP IP blocks.
>>
>> Yes it's not too many, but the issue there is the driver specific code
>> that's duplicated in both places. And sounds like the solution to that
>> is to make driver specific reset code a static inline function in the
>> driver header as discussed earlier so bus code can call it when there's
>> no driver loaded.
>
> This thread is going in many directions and I've lost track.
>
Indeed. I lost track too and almost gave up further reading.

Regards,
Santosh

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

[Felipe Balbi]

Hi,

On Thu, Feb 14, 2013 at 08:47:53PM +0000, Paul Walmsley wrote:
> Hi,
> 
> On Thu, 14 Feb 2013, Tony Lindgren wrote:
> 
> > I don't think so as hwmod should only touch the sysconfig space
> > when no driver has claimed it.
> 
> hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> and resume operations, and during device enable and disable operations.  
> Here's the relevant code:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> 
> Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 

why not ? It's part of the driver's address space anyway. It's not part
of the IP in question (usb, ethernet, etc) but it's part of the TI
wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the
TI-specific integration registers (revision, sysc, syss...).

So they're not part of the licensed IP, but they're part of the TI
wrapper around those.

> have anything specifically to do with the underlying device IP block, but 

of course they do, soft reset touches the underlying IP, so does force
idle, no idle, etc. But I agree that part is more changing the the
asynchronous idle request handshake.

> with the SoC integration, even though they live in the device's 
> memory-mapped address range.  It's unfortunate that TI didn't allocate a 
> separate, small address space for these registers, translated by the bus, 
> similar to the PCI-E Enhanced Configuration Access Mechanism:
> 
> http://wiki.osdev.org/PCI_Express#Extended_Configuration_Space
> 
> ... but that's unfortunately the reality of the OMAP hardware.

right.

> Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as 
> long as the implementation of ioremap() can be overridden by the device's 
> bus.  PCI device drivers already do this -- albeit in a PCI-specific way 
> -- with pci_ioremap_bar():
> 
> http://git.kernel.org/?p=linux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git&a=search&h=HEAD&st=grep&s=pci_ioremap_bar
> 
> So instead of something bus-specific like that, a better way would be to 
> use something like:
> 
> va = dev->bus->ioremap( ... );
> va = dev->bus->iounmap( ... );
> 
> Any driver using such a mechanism would be intrinsically generic. 
> platform_bus could simply set its bus->ioremap to the existing ioremap() 
> function, while buses like a omap_hwmod_bus could use a function that 
> returned the address range that omap_hwmod_bus has already ioremap()ped.

although I'm not sure I like the idea exposed here, ioremap() is the
least of our problems :-) The biggest problem is having functioning PM
for all drivers on a DT boot with CPUIDLE enabled.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/8ce7c5cc/attachment.sig>

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

[Bedia, Vaibhav]

Hi,

On Fri, Feb 15, 2013 at 12:14:29, Balbi, Felipe wrote:
> Hi,
> 
> On Thu, Feb 14, 2013 at 08:47:53PM +0000, Paul Walmsley wrote:
> > Hi,
> > 
> > On Thu, 14 Feb 2013, Tony Lindgren wrote:
> > 
> > > I don't think so as hwmod should only touch the sysconfig space
> > > when no driver has claimed it.
> > 
> > hwmod does need to touch the SYSCONFIG register during pm_runtime suspend 
> > and resume operations, and during device enable and disable operations.  
> > Here's the relevant code:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2157
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=arch/arm/mach-omap2/omap_hwmod.c;h=4653efb87a2721ea20a9fe06a30a6c204d6d2282;hb=HEAD#l2195
> > 
> > Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 
> 
> why not ? It's part of the driver's address space anyway. It's not part
> of the IP in question (usb, ethernet, etc) but it's part of the TI
> wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the
> TI-specific integration registers (revision, sysc, syss...).
> 
> So they're not part of the licensed IP, but they're part of the TI
> wrapper around those.
> 

That's all the more reason to not allow the drivers to touch the SYSCONFIG register.
We have IPs like EDMA, PWMSS, McASP etc which are common to Davinci and OMAP.
The integration approach was different in the past and now if we want the same
driver to work on both the platforms we have to keep the code for taking care
of the integration details out of the drivers.

IMO omap_hwmod does an excellent job of taking care of all or rather most of
the IP integration idiosyncrasies. I really don't understand why people make
a big fuss about hwmod. With the right SoC data things just work. Most of the
driver authors don't take the pains to understand how the SoC PM gets impacted
by toggling a few bits in SYSCONFIG and hence it's best to abstract away all
the critical pieces out of drivers.

For specific cases like custom reset handling I think it make much more sense to
extend runtime PM that already build upon the hwmod code for OMAP. The drivers
shouldn't have to worry about the integration details. Trying to shove a common
piece of code into all the drivers is equivalent to taking a huge step backwards
in the ongoing consolidation not just across OMAP and AMxx parts but also across
Davinci and OMAP.
 
Regards,
Vaibhav

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

[Paul Walmsley]

Hi,

I think Vaibhav has covered most of the ground here, but just a few 
comments:

On Fri, 15 Feb 2013, Felipe Balbi wrote:

> On Thu, Feb 14, 2013 at 08:47:53PM +0000, Paul Walmsley wrote:
>
> > Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 
> 
> why not ? It's part of the driver's address space anyway. It's not part
> of the IP in question (usb, ethernet, etc) but it's part of the TI
> wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the
> TI-specific integration registers (revision, sysc, syss...).
> 
> So they're not part of the licensed IP, but they're part of the TI
> wrapper around those.

Ideally Linux drivers would be as independent as possible from their SoC- 
or board-specific integration.  This includes bus integration, etc.  That 
way, an IP block like UART, where most or all of the registers are shared 
with the common 8250 code, might be able to use a shared driver.

Also, it should be noted that the TI wrapper isn't necessarily common to 
all TI SoCs :-)  The wrapper bits and functions can change from TI SoC to 
SoC.

> > have anything specifically to do with the underlying device IP block, but 
> 
> of course they do, soft reset touches the underlying IP, so does force
> idle, no idle, etc. 

Force idle, no idle, etc. only affects what's reported to the PRCM via 
the SIdleAck/MStandbyReq lines.  It shouldn't affect anything in the IP 
block itself.

As far as reset goes, the chip-wide reset bits affect the IP block too, 
but we wouldn't put that code into the drivers :-)


- Paul

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

[Felipe Balbi]

Hi,

On Wed, Feb 20, 2013 at 05:38:49PM +0000, Paul Walmsley wrote:
> > > Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 

just now I noticed this fun fact:

"driver's should touch *their* SYSCONFIG registers"

You're stating yourself that SYSCONFIG belongs to the IP and the IP is
fully controlled by its driver. ;-)

> > why not ? It's part of the driver's address space anyway. It's not part
> > of the IP in question (usb, ethernet, etc) but it's part of the TI
> > wrapper which usually involves a bridge (ocp2scp, ocp2axi, etc) plus the
> > TI-specific integration registers (revision, sysc, syss...).
> > 
> > So they're not part of the licensed IP, but they're part of the TI
> > wrapper around those.
> 
> Ideally Linux drivers would be as independent as possible from their SoC- 
> or board-specific integration.  This includes bus integration, etc.  That 
> way, an IP block like UART, where most or all of the registers are shared 
> with the common 8250 code, might be able to use a shared driver.

right, then why did we even bother adding omap-serial ? If the whole
idea was to hide integration, what was the point in introducing
omap-serial ?

> Also, it should be noted that the TI wrapper isn't necessarily common to 
> all TI SoCs :-)  The wrapper bits and functions can change from TI SoC to 
> SoC.

yeah, that's because of the chassis architecture definition and has
little value to current discussion ;-)

> > > have anything specifically to do with the underlying device IP block, but 
> > 
> > of course they do, soft reset touches the underlying IP, so does force
> > idle, no idle, etc. 
> 
> Force idle, no idle, etc. only affects what's reported to the PRCM via 
> the SIdleAck/MStandbyReq lines.  It shouldn't affect anything in the IP 
> block itself.

It does touch the IP block. After the asynchronous PM handshake
(Ack-Req) is successful the IP block isn't in a usable state.

> As far as reset goes, the chip-wide reset bits affect the IP block too, 
> but we wouldn't put that code into the drivers :-)

why not ? What happens when drivers need to reset the IP ? For cases
where we need "reset in runtime", drivers are the best entities to know
exactly where the IP must be reset.

Also, IMHO reset should always be done during probe() so driver can be
dead sure that we're starting from a known state. This is even more
important for the complex IPs which are licensed from RTL vendors and
are used in different SoCs. While arch/arm/mach-abc/ resets every IP
pior to probe() being called, we can't be sure that arch/arm/mach-xyz/
will do the same thing and imposing such requirement is too difficult.

Problem just gets worse when you consider SoCs from completely different
architectures (say ARM and Blackfin) using the same IP licensed from
a particular RTL vendor.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130220/8a737f8f/attachment.sig>

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

[Paul Walmsley]

Hi,

On Wed, 20 Feb 2013, Felipe Balbi wrote:

> On Wed, Feb 20, 2013 at 05:38:49PM +0000, Paul Walmsley wrote:
> > > > Drivers shouldn't touch their IP block's SYSCONFIG registers.  They don't 
> 
> just now I noticed this fun fact:
> 
> "driver's should touch *their* SYSCONFIG registers"
> 
> You're stating yourself that SYSCONFIG belongs to the IP and the IP is
> fully controlled by its driver. ;-)

Hehe.  I should have used some more convoluted expression :-)

> > Ideally Linux drivers would be as independent as possible from their SoC- 
> > or board-specific integration.  This includes bus integration, etc.  That 
> > way, an IP block like UART, where most or all of the registers are shared 
> > with the common 8250 code, might be able to use a shared driver.
> 
> right, then why did we even bother adding omap-serial ? If the whole
> idea was to hide integration, what was the point in introducing
> omap-serial ?

My distant recollection was that the main impetus at the time was DMA 
support.  Which might be moot, now.  Not really my parish.

> > Also, it should be noted that the TI wrapper isn't necessarily common to 
> > all TI SoCs :-)  The wrapper bits and functions can change from TI SoC to 
> > SoC.
> 
> yeah, that's because of the chassis architecture definition and has
> little value to current discussion ;-)

The point was that we don't want to duplicate that code in each driver for 
each SoC ... rather, it can live in one place, in some separate 
integration layer.

> It does touch the IP block. After the asynchronous PM handshake
> (Ack-Req) is successful the IP block isn't in a usable state.

Usually that's just because the PRCM has cut some clock to the device.
You can chat with Beno?t about this if you want; we had a discussion last 
year about it in relation to the 32K_SYNCTIMER.

> > As far as reset goes, the chip-wide reset bits affect the IP block too, 
> > but we wouldn't put that code into the drivers :-)
> 
> why not ? 

In the OMAP case, the SoC-wide reset bits are in the PRCM somewhere, which 
in turn assert reset for a wide set of IP blocks.  So there's not much 
point to putting code for that in the drivers.

> What happens when drivers need to reset the IP ? For cases
> where we need "reset in runtime", drivers are the best entities to know
> exactly where the IP must be reset.

We might be miscommunicating.  I was mostly referring to SoC-wide reset in 
my (somewhat silly) counterfactual.

So there are at least three kinds of reset in this context, that I think 
about:

1. SoC-level reset
2. Bus-level reset
3. Device-level reset

Ideally these are all independent, even though they might share some of 
the same hardware mechanisms.  The case that's causing us so much hassle 
is bus-level reset.  The problem is that some of the OMAP devices - say, 
7-10% - can't be reset and idled simply with a bus-level reset.  I'd 
consider the exceptions as hardware bugs, but we still have to deal with 
them.

> Also, IMHO reset should always be done during probe() so driver can be
> dead sure that we're starting from a known state. This is even more
> important for the complex IPs which are licensed from RTL vendors and
> are used in different SoCs. While arch/arm/mach-abc/ resets every IP
> pior to probe() being called, we can't be sure that arch/arm/mach-xyz/
> will do the same thing and imposing such requirement is too difficult.
> 
> Problem just gets worse when you consider SoCs from completely different
> architectures (say ARM and Blackfin) using the same IP licensed from
> a particular RTL vendor.

Or if there's no driver for the device at all :-(


- Paul

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

[Russell King - ARM Linux]

On Wed, Feb 20, 2013 at 09:16:38PM +0200, Felipe Balbi wrote:
> Also, IMHO reset should always be done during probe() so driver can be
> dead sure that we're starting from a known state. This is even more
> important for the complex IPs which are licensed from RTL vendors and
> are used in different SoCs. While arch/arm/mach-abc/ resets every IP
> pior to probe() being called, we can't be sure that arch/arm/mach-xyz/
> will do the same thing and imposing such requirement is too difficult.

And that's where you're wrong.  Virtually nothing in arch/arm/mach-*
does any manipulation of hardware resets for individual IP blocks.
This just doesn't feature.  Mainly because many SoCs generally do not
have a way to reset individual IP blocks - normally you find that you
get the option of resetting the entire platform including the CPU or
nothing at all.

And what that means is that we expect to take over hardware in a
not-well-defined state and start using it.

This is even more complicated by kexec() where we can end up booting
a kernel from a previously crashed kernel where the devices are still
running from the crashed kernel - and we _hope_ that it works.  (It
doesn't always - think about devices which DMA into the kernel's
memory.)

So really... get over the fact that you have reset bits that you can
twiddle on your SoC - that's a cool additional feature.  Many SoCs
that Linux runs on doesn't have that, and you have to start using the
hardware from whatever state you find it in.  And that means a probe
function has to do things like:

1. Mask all interrupts on the device and clean out any pending
   interrupts.
2. Initialize the settings that it needs on the device to the values
   it requires.
etc.

I'm not saying to you to forego the reset - I'm saying that your assertion
that everything in Linux starts from a known state is definitely false.

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

[Peter De Schrijver]

> 
> Also, IMHO reset should always be done during probe() so driver can be
> dead sure that we're starting from a known state. This is even more

Depends on the IP block. Eg: you might want to keep the screen showing the
contents drawn by the bootloader while booting the kernel and smoothly
change to a new image when userspace comes online. In that case you probably
don't want to blindly reset the display controller for example.

Cheers,

Peter.

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

[Peter Korsgaard]

>>>>> "Peter" == Peter De Schrijver <pdeschrijver@nvidia.com> writes:

 >> 
 >> Also, IMHO reset should always be done during probe() so driver can be
 >> dead sure that we're starting from a known state. This is even more

 Peter> Depends on the IP block. Eg: you might want to keep the screen
 Peter> showing the contents drawn by the bootloader while booting the
 Peter> kernel and smoothly change to a new image when userspace comes
 Peter> online. In that case you probably don't want to blindly reset
 Peter> the display controller for example.

Indeed. Another recent example was gpio pins (controlling stuff like
reset signals) where the bootloader had already set things up correctly
and the reset caused pins to toggle.

-- 
Bye, Peter Korsgaard

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

[Russell King - ARM Linux]

On Thu, Feb 14, 2013 at 08:47:53PM +0000, Paul Walmsley wrote:
> Regarding ioremap(), it seems reasonable for drivers to call ioremap(), as 
> long as the implementation of ioremap() can be overridden by the device's 
> bus.  PCI device drivers already do this -- albeit in a PCI-specific way 
> -- with pci_ioremap_bar():

Err no, that's just a helper to assist checking the resource type and
remove all those pesky resource size mistakes - see the commit text:

    PCI: introduce an pci_ioremap(pdev, barnr) function

    A common thing in many PCI drivers is to ioremap() an entire bar.  This
    is a slightly fragile thing right now, needing both an address and a
    size, and many driver writers do.. various things there.

    This patch introduces an pci_ioremap() function taking just a PCI device
    struct and the bar number as arguments, and figures this all out itself,
    in one place.  In addition, we can add various sanity checks to this
    function (the patch already checks to make sure that the bar in question
    really is a MEM bar; few to no drivers do that sort of thing).

    Hopefully with this type of API we get less chance of mistakes in
    drivers with ioremap() operations.

    Signed-off-by: Arjan van de Ven <arjan@linux.intel.com>
    Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

> So instead of something bus-specific like that, a better way would be to 
> use something like:
> 
> va = dev->bus->ioremap( ... );
> va = dev->bus->iounmap( ... );

No.  ioremap() is already generic.  We don't need additional layers of
indirection per bus type.

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

[Santosh Shilimkar]

Russell,

On Friday 15 February 2013 03:46 PM, Russell King - ARM Linux wrote:
> On Thu, Feb 14, 2013 at 08:47:53PM +0000, Paul Walmsley wrote:

[..]

>
>> So instead of something bus-specific like that, a better way would be to
>> use something like:
>>
>> va = dev->bus->ioremap( ... );
>> va = dev->bus->iounmap( ... );
>
> No.  ioremap() is already generic.  We don't need additional layers of
> indirection per bus type.
>
Whats your view on use of arch_ioremap_caller() hook ? This can allow
us to avoid the dual ioremap() issue discussed here if the hook
maintains the list of mapped ios.

I was even thinking of having such intelligence within the core
ioremap code but thought that might be too invasive.

Regards,
Santosh

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

[Russell King - ARM Linux]

On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
> Whats your view on use of arch_ioremap_caller() hook ? This can allow
> us to avoid the dual ioremap() issue discussed here if the hook
> maintains the list of mapped ios.
>
> I was even thinking of having such intelligence within the core
> ioremap code but thought that might be too invasive.

Why do you even need it?  There's no problem with ioremapping the same
space multiple times (you end up with multiple mappings but that
shouldn't be a problem, except for the additional space used.)

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

[Santosh Shilimkar]

On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
> On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
>> Whats your view on use of arch_ioremap_caller() hook ? This can allow
>> us to avoid the dual ioremap() issue discussed here if the hook
>> maintains the list of mapped ios.
>>
>> I was even thinking of having such intelligence within the core
>> ioremap code but thought that might be too invasive.
>
> Why do you even need it?  There's no problem with ioremapping the same
> space multiple times (you end up with multiple mappings but that
> shouldn't be a problem, except for the additional space used.)
>
It just waste of iospace and Tony insisted to have just single ioremap()
hence all this discussion

regards
Santosh

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

[Tony Lindgren]

* Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
> On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
> >On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
> >>Whats your view on use of arch_ioremap_caller() hook ? This can allow
> >>us to avoid the dual ioremap() issue discussed here if the hook
> >>maintains the list of mapped ios.
> >>
> >>I was even thinking of having such intelligence within the core
> >>ioremap code but thought that might be too invasive.
> >
> >Why do you even need it?  There's no problem with ioremapping the same
> >space multiple times (you end up with multiple mappings but that
> >shouldn't be a problem, except for the additional space used.)
> >
> It just waste of iospace and Tony insisted to have just single ioremap()
> hence all this discussion

The main goal is to avoid duplicating data both in hwmod and DT.
That's pretty much solved as we can have the driver probe populate
the common data for hwmod from DT as Santosh has already demonstrated.

Then we also want the driver specific idle and reset code to be done
in the drivers rather than in hwmod and glue it together with hwmod
using runtime PM. The biggest issue there is how do we reset and idle
some piece of hardware for PM purposes when there's no driver loaded.

For the duplicate ioremapping, I don't think there's any need to
do it if we get things right.

Regards,

Tony

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

[Felipe Balbi]

Hi,

On Fri, Feb 15, 2013 at 08:30:32AM -0800, Tony Lindgren wrote:
> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
> > On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
> > >On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
> > >>Whats your view on use of arch_ioremap_caller() hook ? This can allow
> > >>us to avoid the dual ioremap() issue discussed here if the hook
> > >>maintains the list of mapped ios.
> > >>
> > >>I was even thinking of having such intelligence within the core
> > >>ioremap code but thought that might be too invasive.
> > >
> > >Why do you even need it?  There's no problem with ioremapping the same
> > >space multiple times (you end up with multiple mappings but that
> > >shouldn't be a problem, except for the additional space used.)
> > >
> > It just waste of iospace and Tony insisted to have just single ioremap()
> > hence all this discussion
> 
> The main goal is to avoid duplicating data both in hwmod and DT.
> That's pretty much solved as we can have the driver probe populate
> the common data for hwmod from DT as Santosh has already demonstrated.
> 
> Then we also want the driver specific idle and reset code to be done
> in the drivers rather than in hwmod and glue it together with hwmod
> using runtime PM. The biggest issue there is how do we reset and idle
> some piece of hardware for PM purposes when there's no driver loaded.

right, this will be a tough nut to crack.

I guess the only way would be reset all IPs early in the boot process,
before even creating the platform-devices. Can we do that ? I mean, from
omap_device_build_from_dt() we have access to address space of all
devices, through ti,hwmods we can figure out which hwmods are linked to
that particular device, so whenever you build a device, you could just
call _reset().

The only problem is that now omap_device_build_from_dt() is called after
we notify that a new device/driver pair has been registered with the
platform_bus, right ? So we would still need a way to call _reset() for
those which are on DTS but don't have a driver bound to them...

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/844a239f/attachment.sig>

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

[Santosh Shilimkar]

On Friday 15 February 2013 10:12 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 15, 2013 at 08:30:32AM -0800, Tony Lindgren wrote:
>> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
>>> On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
>>>> On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
>>>>> Whats your view on use of arch_ioremap_caller() hook ? This can allow
>>>>> us to avoid the dual ioremap() issue discussed here if the hook
>>>>> maintains the list of mapped ios.
>>>>>
>>>>> I was even thinking of having such intelligence within the core
>>>>> ioremap code but thought that might be too invasive.
>>>>
>>>> Why do you even need it?  There's no problem with ioremapping the same
>>>> space multiple times (you end up with multiple mappings but that
>>>> shouldn't be a problem, except for the additional space used.)
>>>>
>>> It just waste of iospace and Tony insisted to have just single ioremap()
>>> hence all this discussion
>>
>> The main goal is to avoid duplicating data both in hwmod and DT.
>> That's pretty much solved as we can have the driver probe populate
>> the common data for hwmod from DT as Santosh has already demonstrated.
>>
>> Then we also want the driver specific idle and reset code to be done
>> in the drivers rather than in hwmod and glue it together with hwmod
>> using runtime PM. The biggest issue there is how do we reset and idle
>> some piece of hardware for PM purposes when there's no driver loaded.
>
> right, this will be a tough nut to crack.
>
> I guess the only way would be reset all IPs early in the boot process,
> before even creating the platform-devices. Can we do that ? I mean, from
> omap_device_build_from_dt() we have access to address space of all
> devices, through ti,hwmods we can figure out which hwmods are linked to
> that particular device, so whenever you build a device, you could just
> call _reset().
>
Thats what we do today and it works perfectly. As per Tony's suggestion,
we need to move the non-probed devices reset and idle setup to late_init
which is also doable.

In that case when probed driver calls runtime_get(), we reset that
device and setup the idle settings. And remainder of the devices
are managed in late_init().

> The only problem is that now omap_device_build_from_dt() is called after
> we notify that a new device/driver pair has been registered with the
> platform_bus, right ? So we would still need a way to call _reset() for
> those which are on DTS but don't have a driver bound to them...
>
The only special requirement for reset remains(which today handled by
hwmod function calls) is for devices which needs specific reset
sequence. And this one can be handled through a runtime_reset()
kind of hook.

Does that sound reasonable ?

Regards,
Santosh

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

[Felipe Balbi]

Hi,

On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote:
> >>The main goal is to avoid duplicating data both in hwmod and DT.
> >>That's pretty much solved as we can have the driver probe populate
> >>the common data for hwmod from DT as Santosh has already demonstrated.
> >>
> >>Then we also want the driver specific idle and reset code to be done
> >>in the drivers rather than in hwmod and glue it together with hwmod
> >>using runtime PM. The biggest issue there is how do we reset and idle
> >>some piece of hardware for PM purposes when there's no driver loaded.
> >
> >right, this will be a tough nut to crack.
> >
> >I guess the only way would be reset all IPs early in the boot process,
> >before even creating the platform-devices. Can we do that ? I mean, from
> >omap_device_build_from_dt() we have access to address space of all
> >devices, through ti,hwmods we can figure out which hwmods are linked to
> >that particular device, so whenever you build a device, you could just
> >call _reset().
> >
> Thats what we do today and it works perfectly. As per Tony's suggestion,
> we need to move the non-probed devices reset and idle setup to late_init
> which is also doable.
> 
> In that case when probed driver calls runtime_get(), we reset that
> device and setup the idle settings. And remainder of the devices
> are managed in late_init().

what's the point in moving it to late_initcall() ? It makes no
difference, if no driver binds to that device it will stay in reset
anyway. Maybe what we're missing is properly idling (not exactly) all
devices before driver probe kicks in.

> >The only problem is that now omap_device_build_from_dt() is called after
> >we notify that a new device/driver pair has been registered with the
> >platform_bus, right ? So we would still need a way to call _reset() for
> >those which are on DTS but don't have a driver bound to them...
> >
> The only special requirement for reset remains(which today handled by
> hwmod function calls) is for devices which needs specific reset
> sequence. And this one can be handled through a runtime_reset()
> kind of hook.
> 
> Does that sound reasonable ?

Depends on Rafael. If he says no to the ->runtime_reset() I suggested
earlier, another aproach needs to be found. In any case,
->runtime_reset() is only for devices which actually have a driver, so
it matters little.

The difficult part is handling special reset requirements for devices
without drivers as there'd be no ->runtime_reset() to call.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130216/d001032e/attachment.sig>

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

[Santosh Shilimkar]

+ Rafael,

On Saturday 16 February 2013 02:25 PM, Felipe Balbi wrote:
> Hi,
>
> On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote:
>>>> The main goal is to avoid duplicating data both in hwmod and DT.
>>>> That's pretty much solved as we can have the driver probe populate
>>>> the common data for hwmod from DT as Santosh has already demonstrated.
>>>>
>>>> Then we also want the driver specific idle and reset code to be done
>>>> in the drivers rather than in hwmod and glue it together with hwmod
>>>> using runtime PM. The biggest issue there is how do we reset and idle
>>>> some piece of hardware for PM purposes when there's no driver loaded.
>>>
>>> right, this will be a tough nut to crack.
>>>
>>> I guess the only way would be reset all IPs early in the boot process,
>>> before even creating the platform-devices. Can we do that ? I mean, from
>>> omap_device_build_from_dt() we have access to address space of all
>>> devices, through ti,hwmods we can figure out which hwmods are linked to
>>> that particular device, so whenever you build a device, you could just
>>> call _reset().
>>>
>> Thats what we do today and it works perfectly. As per Tony's suggestion,
>> we need to move the non-probed devices reset and idle setup to late_init
>> which is also doable.
>>
>> In that case when probed driver calls runtime_get(), we reset that
>> device and setup the idle settings. And remainder of the devices
>> are managed in late_init().
>
> what's the point in moving it to late_initcall() ? It makes no
> difference, if no driver binds to that device it will stay in reset
> anyway. Maybe what we're missing is properly idling (not exactly) all
> devices before driver probe kicks in.
>
I think it is largely reducing the early init dependencies and also
reducing the role of platform code who today takes care of every
device idle and reset initialization. That way late_init() will
only have to care about the devices which are not probed by
drivers.

Tony, Is that right ?


>>> The only problem is that now omap_device_build_from_dt() is called after
>>> we notify that a new device/driver pair has been registered with the
>>> platform_bus, right ? So we would still need a way to call _reset() for
>>> those which are on DTS but don't have a driver bound to them...
>>>
>> The only special requirement for reset remains(which today handled by
>> hwmod function calls) is for devices which needs specific reset
>> sequence. And this one can be handled through a runtime_reset()
>> kind of hook.
>>
>> Does that sound reasonable ?
>
> Depends on Rafael. If he says no to the ->runtime_reset() I suggested
> earlier, another aproach needs to be found. In any case,
> ->runtime_reset() is only for devices which actually have a driver, so
> it matters little.
>
I agree. Looping Rafael to hear from him.

To add a bit of context,
On OMAP we do have few IP blocks which needs a special reset sequence
during init as well as working state of the driver. Some of this
was worked around such cases by populating function pointers from
platform data which drivers can use. This obviously doesn't scale
and won't work with DT based builds. We have been thinking of
having a runtime_reset() generic callback which drivers can use.

Whats you take on it ?

> The difficult part is handling special reset requirements for devices
> without drivers as there'd be no ->runtime_reset() to call.
>
I don't think that requirement exists so if we address the driver
requirement, we are good. Even otherwise also, it can be managed
from platform code.

Regards,
Sasntosh

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

[Felipe Balbi]

Hi,

On Sat, Feb 16, 2013 at 02:47:45PM +0530, Santosh Shilimkar wrote:
> >On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote:
> >>>>The main goal is to avoid duplicating data both in hwmod and DT.
> >>>>That's pretty much solved as we can have the driver probe populate
> >>>>the common data for hwmod from DT as Santosh has already demonstrated.
> >>>>
> >>>>Then we also want the driver specific idle and reset code to be done
> >>>>in the drivers rather than in hwmod and glue it together with hwmod
> >>>>using runtime PM. The biggest issue there is how do we reset and idle
> >>>>some piece of hardware for PM purposes when there's no driver loaded.
> >>>
> >>>right, this will be a tough nut to crack.
> >>>
> >>>I guess the only way would be reset all IPs early in the boot process,
> >>>before even creating the platform-devices. Can we do that ? I mean, from
> >>>omap_device_build_from_dt() we have access to address space of all
> >>>devices, through ti,hwmods we can figure out which hwmods are linked to
> >>>that particular device, so whenever you build a device, you could just
> >>>call _reset().
> >>>
> >>Thats what we do today and it works perfectly. As per Tony's suggestion,
> >>we need to move the non-probed devices reset and idle setup to late_init
> >>which is also doable.
> >>
> >>In that case when probed driver calls runtime_get(), we reset that
> >>device and setup the idle settings. And remainder of the devices
> >>are managed in late_init().
> >
> >what's the point in moving it to late_initcall() ? It makes no
> >difference, if no driver binds to that device it will stay in reset
> >anyway. Maybe what we're missing is properly idling (not exactly) all
> >devices before driver probe kicks in.
> >
> I think it is largely reducing the early init dependencies and also
> reducing the role of platform code who today takes care of every
> device idle and reset initialization. That way late_init() will
> only have to care about the devices which are not probed by
> drivers.
> 
> Tony, Is that right ?

Makes not much difference, except that you will have to keep track of
which devices have gotten a driver probed and which haven't.

IMO, it sounds a lot better to reset everything early on, so we know
we're starting at a known stage (and thus drop all bootloader
dependencies) then to follow the other route.

> >The difficult part is handling special reset requirements for devices
> >without drivers as there'd be no ->runtime_reset() to call.
> >
> I don't think that requirement exists so if we address the driver
> requirement, we are good. Even otherwise also, it can be managed

Look back at what you want to do at late_initcall() time. You want to
reset all devices which haven't gotten a driver bound to them.

> from platform code.

right, the you will need even more data in hwmod to let it know about
the special devices. /me wonders when the amount of data will actually
decrease.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130216/6506aeec/attachment.sig>

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

[Santosh Shilimkar]

On Saturday 16 February 2013 02:52 PM, Felipe Balbi wrote:
> Hi,
>
> On Sat, Feb 16, 2013 at 02:47:45PM +0530, Santosh Shilimkar wrote:
>>> On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote:
>>>>>> The main goal is to avoid duplicating data both in hwmod and DT.
>>>>>> That's pretty much solved as we can have the driver probe populate
>>>>>> the common data for hwmod from DT as Santosh has already demonstrated.
>>>>>>
>>>>>> Then we also want the driver specific idle and reset code to be done
>>>>>> in the drivers rather than in hwmod and glue it together with hwmod
>>>>>> using runtime PM. The biggest issue there is how do we reset and idle
>>>>>> some piece of hardware for PM purposes when there's no driver loaded.
>>>>>
>>>>> right, this will be a tough nut to crack.
>>>>>
>>>>> I guess the only way would be reset all IPs early in the boot process,
>>>>> before even creating the platform-devices. Can we do that ? I mean, from
>>>>> omap_device_build_from_dt() we have access to address space of all
>>>>> devices, through ti,hwmods we can figure out which hwmods are linked to
>>>>> that particular device, so whenever you build a device, you could just
>>>>> call _reset().
>>>>>
>>>> Thats what we do today and it works perfectly. As per Tony's suggestion,
>>>> we need to move the non-probed devices reset and idle setup to late_init
>>>> which is also doable.
>>>>
>>>> In that case when probed driver calls runtime_get(), we reset that
>>>> device and setup the idle settings. And remainder of the devices
>>>> are managed in late_init().
>>>
>>> what's the point in moving it to late_initcall() ? It makes no
>>> difference, if no driver binds to that device it will stay in reset
>>> anyway. Maybe what we're missing is properly idling (not exactly) all
>>> devices before driver probe kicks in.
>>>
>> I think it is largely reducing the early init dependencies and also
>> reducing the role of platform code who today takes care of every
>> device idle and reset initialization. That way late_init() will
>> only have to care about the devices which are not probed by
>> drivers.
>>
>> Tony, Is that right ?
>
> Makes not much difference, except that you will have to keep track of
> which devices have gotten a driver probed and which haven't.
>
> IMO, it sounds a lot better to reset everything early on, so we know
> we're starting at a known stage (and thus drop all bootloader
> dependencies) then to follow the other route.
>
I tend to agree with you. This was exactly the reason Paul and Benoit
added that support first up as part of early init code.

>>> The difficult part is handling special reset requirements for devices
>>> without drivers as there'd be no ->runtime_reset() to call.
>>>
>> I don't think that requirement exists so if we address the driver
>> requirement, we are good. Even otherwise also, it can be managed
>
> Look back at what you want to do at late_initcall() time. You want to
> reset all devices which haven't gotten a driver bound to them.
>
>> from platform code.
>
> right, the you will need even more data in hwmod to let it know about
> the special devices. /me wonders when the amount of data will actually
> decrease.
>
Well that is already supported. There is no need to add any additional
information. Device which are initialized, there state is set as
initialized. So the late_init() will just have to iterate over
un-initialised devices.

Just to be clear, I am also in favor of just keeping that part as it
is today but we were exploring other options based on comments from
Tony during OMAP5 data review.

Regards,
Santosh

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

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> Hi,
>
> On Sat, Feb 16, 2013 at 11:31:21AM +0530, Santosh Shilimkar wrote:
>> >>The main goal is to avoid duplicating data both in hwmod and DT.
>> >>That's pretty much solved as we can have the driver probe populate
>> >>the common data for hwmod from DT as Santosh has already demonstrated.
>> >>
>> >>Then we also want the driver specific idle and reset code to be done
>> >>in the drivers rather than in hwmod and glue it together with hwmod
>> >>using runtime PM. The biggest issue there is how do we reset and idle
>> >>some piece of hardware for PM purposes when there's no driver loaded.
>> >
>> >right, this will be a tough nut to crack.
>> >
>> >I guess the only way would be reset all IPs early in the boot process,
>> >before even creating the platform-devices. Can we do that ? I mean, from
>> >omap_device_build_from_dt() we have access to address space of all
>> >devices, through ti,hwmods we can figure out which hwmods are linked to
>> >that particular device, so whenever you build a device, you could just
>> >call _reset().
>> >
>> Thats what we do today and it works perfectly. As per Tony's suggestion,
>> we need to move the non-probed devices reset and idle setup to late_init
>> which is also doable.
>> 
>> In that case when probed driver calls runtime_get(), we reset that
>> device and setup the idle settings. And remainder of the devices
>> are managed in late_init().
>
> what's the point in moving it to late_initcall() ? It makes no
> difference, if no driver binds to that device it will stay in reset
> anyway. Maybe what we're missing is properly idling (not exactly) all
> devices before driver probe kicks in.
>
>> >The only problem is that now omap_device_build_from_dt() is called after
>> >we notify that a new device/driver pair has been registered with the
>> >platform_bus, right ? So we would still need a way to call _reset() for
>> >those which are on DTS but don't have a driver bound to them...
>> >
>> The only special requirement for reset remains(which today handled by
>> hwmod function calls) is for devices which needs specific reset
>> sequence. And this one can be handled through a runtime_reset()
>> kind of hook.
>> 
>> Does that sound reasonable ?
>
> Depends on Rafael. If he says no to the ->runtime_reset() I suggested
> earlier, another aproach needs to be found. In any case,
> ->runtime_reset() is only for devices which actually have a driver, so
> it matters little.

Has somone tried this to see if it works on devices with the custom
reset hooks?

It should be relatively easy to add a ->runtime_reset hook, and hook it
up to omap_device_shutdown() at the PM domain level.

If we have a patch, showing how we use it in some drivers and making the
case for why it is needed in some drivers, it will be easier to take
this to Rafael and make a case for it.

> The difficult part is handling special reset requirements for devices
> without drivers as there'd be no ->runtime_reset() to call.

IMO, this should not be difficult, as omap_device_shutdown() ->
omap_hwmod_shutdown() does the heavy lifting here.  Also, We already
track whether omap_devices have drivers bound, so adding support to
reset/shutdown unbound drivers in late init will be straight forward.

Kevin

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

[Santosh Shilimkar]

On Friday 15 February 2013 10:00 PM, Tony Lindgren wrote:
> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
>> On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
>>> On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
>>>> Whats your view on use of arch_ioremap_caller() hook ? This can allow
>>>> us to avoid the dual ioremap() issue discussed here if the hook
>>>> maintains the list of mapped ios.
>>>>
>>>> I was even thinking of having such intelligence within the core
>>>> ioremap code but thought that might be too invasive.
>>>
>>> Why do you even need it?  There's no problem with ioremapping the same
>>> space multiple times (you end up with multiple mappings but that
>>> shouldn't be a problem, except for the additional space used.)
>>>
>> It just waste of iospace and Tony insisted to have just single ioremap()
>> hence all this discussion
>
> The main goal is to avoid duplicating data both in hwmod and DT.
> That's pretty much solved as we can have the driver probe populate
> the common data for hwmod from DT as Santosh has already demonstrated.
>
Right.

> Then we also want the driver specific idle and reset code to be done
> in the drivers rather than in hwmod and glue it together with hwmod
> using runtime PM. The biggest issue there is how do we reset and idle
> some piece of hardware for PM purposes when there's no driver loaded.
>
Driver idle functions is getting sorted out. There is no need for driver
to fiddle around it since PM runtime back-end can take care of of it.
One attempt to clean that is here [1]

For the reset, we need to have a generic runtime hook as discussed and
that should take care of reset as well. That way driver just calls
generic

> For the duplicate ioremapping, I don't think there's any need to
> do it if we get things right.
>
Good to know. So overall, I think we do have a way to move forward
and get right things done.

Regards,
Santosh

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg85177.html

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

[Nicolas Pitre]

On Fri, 15 Feb 2013, Tony Lindgren wrote:

> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
> > On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
> > >On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
> > >>Whats your view on use of arch_ioremap_caller() hook ? This can allow
> > >>us to avoid the dual ioremap() issue discussed here if the hook
> > >>maintains the list of mapped ios.
> > >>
> > >>I was even thinking of having such intelligence within the core
> > >>ioremap code but thought that might be too invasive.
> > >
> > >Why do you even need it?  There's no problem with ioremapping the same
> > >space multiple times (you end up with multiple mappings but that
> > >shouldn't be a problem, except for the additional space used.)
> > >
> > It just waste of iospace and Tony insisted to have just single ioremap()
> > hence all this discussion
> 
> The main goal is to avoid duplicating data both in hwmod and DT.
> That's pretty much solved as we can have the driver probe populate
> the common data for hwmod from DT as Santosh has already demonstrated.
> 
> Then we also want the driver specific idle and reset code to be done
> in the drivers rather than in hwmod and glue it together with hwmod
> using runtime PM. The biggest issue there is how do we reset and idle
> some piece of hardware for PM purposes when there's no driver loaded.
> 
> For the duplicate ioremapping, I don't think there's any need to
> do it if we get things right.

Note that if the ioremap matches a static map area there is no cost to 
ioremap it multiple times.


Nicolas

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

[Santosh Shilimkar]

On Saturday 16 February 2013 11:06 AM, Nicolas Pitre wrote:
> On Fri, 15 Feb 2013, Tony Lindgren wrote:
>
>> * Santosh Shilimkar <santosh.shilimkar@ti.com> [130215 05:34]:
>>> On Friday 15 February 2013 06:57 PM, Russell King - ARM Linux wrote:
>>>> On Fri, Feb 15, 2013 at 06:56:47PM +0530, Santosh Shilimkar wrote:
>>>>> Whats your view on use of arch_ioremap_caller() hook ? This can allow
>>>>> us to avoid the dual ioremap() issue discussed here if the hook
>>>>> maintains the list of mapped ios.
>>>>>
>>>>> I was even thinking of having such intelligence within the core
>>>>> ioremap code but thought that might be too invasive.
>>>>
>>>> Why do you even need it?  There's no problem with ioremapping the same
>>>> space multiple times (you end up with multiple mappings but that
>>>> shouldn't be a problem, except for the additional space used.)
>>>>
>>> It just waste of iospace and Tony insisted to have just single ioremap()
>>> hence all this discussion
>>
>> The main goal is to avoid duplicating data both in hwmod and DT.
>> That's pretty much solved as we can have the driver probe populate
>> the common data for hwmod from DT as Santosh has already demonstrated.
>>
>> Then we also want the driver specific idle and reset code to be done
>> in the drivers rather than in hwmod and glue it together with hwmod
>> using runtime PM. The biggest issue there is how do we reset and idle
>> some piece of hardware for PM purposes when there's no driver loaded.
>>
>> For the duplicate ioremapping, I don't think there's any need to
>> do it if we get things right.
>
> Note that if the ioremap matches a static map area there is no cost to
> ioremap it multiple times.
>
>
Thats true though now on OMAP we removed most of the static mappings.
The main issue is waste of IO space because, we end up mapping same
area two times for all the OMAP drivers. This can be optimized with
a arch ioremap caller hook but as discussed here, its nice to have
rather than something important.

Regards,
Santosh

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

[Bedia, Vaibhav]

On Sat, Feb 16, 2013 at 11:18:36, Shilimkar, Santosh wrote:
[...]
> >>
> >> For the duplicate ioremapping, I don't think there's any need to
> >> do it if we get things right.
> >
> > Note that if the ioremap matches a static map area there is no cost to
> > ioremap it multiple times.
> >
> >
> Thats true though now on OMAP we removed most of the static mappings.
> The main issue is waste of IO space because, we end up mapping same
> area two times for all the OMAP drivers. This can be optimized with
> a arch ioremap caller hook but as discussed here, its nice to have
> rather than something important.
> 

Err.. I was looking at the iotable_init for OMAPx in mainline and it looks
like most (all?) of the peripherals are already covered in the static mappings?

Regards,
Vaibhav

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

[Santosh Shilimkar]

On Monday 18 February 2013 01:38 PM, Bedia, Vaibhav wrote:
> On Sat, Feb 16, 2013 at 11:18:36, Shilimkar, Santosh wrote:
> [...]
>>>>
>>>> For the duplicate ioremapping, I don't think there's any need to
>>>> do it if we get things right.
>>>
>>> Note that if the ioremap matches a static map area there is no cost to
>>> ioremap it multiple times.
>>>
>>>
>> Thats true though now on OMAP we removed most of the static mappings.
>> The main issue is waste of IO space because, we end up mapping same
>> area two times for all the OMAP drivers. This can be optimized with
>> a arch ioremap caller hook but as discussed here, its nice to have
>> rather than something important.
>>
>
Mostly L3 and L4 OCP slaves ones are covered because of L3/L4 map.
If you boot a full blown kernel, you will see many dynamically allocated 
as well.

Regards,
Santosh

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

[Kevin Hilman]

Felipe Balbi <balbi@ti.com> writes:

> 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.
>
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>

Here's another approach to getting rid of the sysconfig twiddling in the
driver.  I wrote this some time ago at my former company ;) but don't
think I ever got around to posting it.

It doesn't solve the whole problem (e.g. doesn't address the
context_loss or enable_wakeup func pointers), but at least gets rid of
the need for any SYSCONFIG access in the driver for the idle modes.

Needs more thorough testing.

Kevin


>From 3d3956472d2375b5ed939d1d0165e439aa47262f Mon Sep 17 00:00:00 2001
From: Kevin Hilman <khilman@ti.com>
Date: Wed, 26 Sep 2012 18:36:43 -0700
Subject: [PATCH] ARM: OMAP2+: serial/hwmod: use SW supported slave idle for
 all UARTs

Due to various errata, we have several hacks involving modifying the
slave idle mode of the UARTs at runtime.  Since the UART driver has
been using the autosuspend feature of runtime PM, each UART is kept in
NO_IDLE during activity, and SMART_IDLE during activity.

The omap_hwmod layer has the per-device HWMOD_SWSUP_SIDLE flag which
will have the exact same behavior, with the benefit of simplifying the
device and driver layers.  Switch to using hwmod to automatically
manage the slave idle mode.

This also enables the removal of a couple pdata function pointers from
the driver, which facilitates the conversion to device tree.

Signed-off-by: Kevin Hilman <khilman@ti.com>
---
 arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c |  3 ++
 arch/arm/mach-omap2/omap_hwmod_33xx_data.c         |  4 +++
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |  4 +++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c         |  4 +++
 arch/arm/mach-omap2/serial.c                       | 35 ----------------------
 arch/arm/plat-omap/include/plat/omap-serial.h      |  2 --
 drivers/tty/serial/omap-serial.c                   | 23 --------------
 7 files changed, 15 insertions(+), 60 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
index bd9220e..0d3c1a6 100644
--- a/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_2xxx_ipblock_data.c
@@ -509,6 +509,7 @@ struct omap_hwmod omap2xxx_uart1_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART2 */
@@ -528,6 +529,7 @@ struct omap_hwmod omap2xxx_uart2_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART3 */
@@ -547,6 +549,7 @@ struct omap_hwmod omap2xxx_uart3_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* dss */
diff --git a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
index 59d5c1c..1a41f93 100644
--- a/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_33xx_data.c
@@ -1903,6 +1903,7 @@ static struct omap_hwmod am33xx_uart1_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 static struct omap_hwmod_irq_info am33xx_uart2_irqs[] = {
@@ -1923,6 +1924,7 @@ static struct omap_hwmod am33xx_uart2_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* uart3 */
@@ -1950,6 +1952,7 @@ static struct omap_hwmod am33xx_uart3_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 static struct omap_hwmod_irq_info am33xx_uart4_irqs[] = {
@@ -1970,6 +1973,7 @@ static struct omap_hwmod am33xx_uart4_hwmod = {
 			.modulemode	= MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 static struct omap_hwmod_irq_info am33xx_uart5_irqs[] = {
diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index f67b7ee..99ada6a 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -497,6 +497,7 @@ static struct omap_hwmod omap3xxx_uart1_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART2 */
@@ -515,6 +516,7 @@ static struct omap_hwmod omap3xxx_uart2_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART3 */
@@ -533,6 +535,7 @@ static struct omap_hwmod omap3xxx_uart3_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* UART4 */
@@ -562,6 +565,7 @@ static struct omap_hwmod omap36xx_uart4_hwmod = {
 		},
 	},
 	.class		= &omap2_uart_class,
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 static struct omap_hwmod_irq_info am35xx_uart4_mpu_irqs[] = {
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index 652d028..5373e58 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -3397,6 +3397,7 @@ static struct omap_hwmod omap44xx_uart1_hwmod = {
 			.modulemode   = MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* uart2 */
@@ -3425,6 +3426,7 @@ static struct omap_hwmod omap44xx_uart2_hwmod = {
 			.modulemode   = MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* uart3 */
@@ -3454,6 +3456,7 @@ static struct omap_hwmod omap44xx_uart3_hwmod = {
 			.modulemode   = MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /* uart4 */
@@ -3482,6 +3485,7 @@ static struct omap_hwmod omap44xx_uart4_hwmod = {
 			.modulemode   = MODULEMODE_SWCTRL,
 		},
 	},
+	.flags		= HWMOD_SWSUP_SIDLE,
 };
 
 /*
diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index a507cd6..e9d69a9 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -94,39 +94,6 @@ static void omap_uart_enable_wakeup(struct device *dev, bool enable)
 	else
 		omap_hwmod_disable_wakeup(od->hwmods[0]);
 }
-
-/*
- * Errata i291: [UART]:Cannot Acknowledge Idle Requests
- * in Smartidle Mode When Configured for DMA Operations.
- * WA: configure uart in force idle mode.
- */
-static void omap_uart_set_noidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], HWMOD_IDLEMODE_NO);
-}
-
-static void omap_uart_set_smartidle(struct device *dev)
-{
-	struct platform_device *pdev = to_platform_device(dev);
-	struct omap_device *od = to_omap_device(pdev);
-	u8 idlemode;
-
-	if (od->hwmods[0]->class->sysc->idlemodes & SIDLE_SMART_WKUP)
-		idlemode = HWMOD_IDLEMODE_SMART_WKUP;
-	else
-		idlemode = HWMOD_IDLEMODE_SMART;
-
-	omap_hwmod_set_slave_idlemode(od->hwmods[0], idlemode);
-}
-
-#else
-static void omap_uart_enable_wakeup(struct device *dev, bool enable)
-{}
-static void omap_uart_set_noidle(struct device *dev) {}
-static void omap_uart_set_smartidle(struct device *dev) {}
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_OMAP_MUX
@@ -299,8 +266,6 @@ void __init omap_serial_init_port(struct omap_board_data *bdata,
 	omap_up.uartclk = OMAP24XX_BASE_BAUD * 16;
 	omap_up.flags = UPF_BOOT_AUTOCONF;
 	omap_up.get_context_loss_count = omap_pm_get_dev_context_loss_count;
-	omap_up.set_forceidle = omap_uart_set_smartidle;
-	omap_up.set_noidle = omap_uart_set_noidle;
 	omap_up.enable_wakeup = omap_uart_enable_wakeup;
 	omap_up.dma_rx_buf_size = info->dma_rx_buf_size;
 	omap_up.dma_rx_timeout = info->dma_rx_timeout;
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 1957a85..b9a712c 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -72,8 +72,6 @@ struct omap_uart_port_info {
 	int			DTR_present;
 
 	int (*get_context_loss_count)(struct device *);
-	void (*set_forceidle)(struct device *);
-	void (*set_noidle)(struct device *);
 	void (*enable_wakeup)(struct device *, bool);
 };
 
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 6d3d26a..8e4e39f 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -151,26 +151,6 @@ static int serial_omap_get_context_loss_count(struct uart_omap_port *up)
 	return pdata->get_context_loss_count(up->dev);
 }
 
-static void serial_omap_set_forceidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_forceidle)
-		return;
-
-	pdata->set_forceidle(up->dev);
-}
-
-static void serial_omap_set_noidle(struct uart_omap_port *up)
-{
-	struct omap_uart_port_info *pdata = up->dev->platform_data;
-
-	if (!pdata || !pdata->set_noidle)
-		return;
-
-	pdata->set_noidle(up->dev);
-}
-
 static void serial_omap_enable_wakeup(struct uart_omap_port *up, bool enable)
 {
 	struct omap_uart_port_info *pdata = up->dev->platform_data;
@@ -229,8 +209,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		serial_out(up, UART_IER, up->ier);
 	}
 
-	serial_omap_set_forceidle(up);
-
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
@@ -298,7 +276,6 @@ static void serial_omap_start_tx(struct uart_port *port)
 
 	pm_runtime_get_sync(up->dev);
 	serial_omap_enable_ier_thri(up);
-	serial_omap_set_noidle(up);
 	pm_runtime_mark_last_busy(up->dev);
 	pm_runtime_put_autosuspend(up->dev);
 }
-- 
1.8.1.2

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

[Felipe Balbi]

Hi,

On Fri, Feb 15, 2013 at 07:40:04AM -0800, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
> 
> > 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.
> >
> > NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> 
> Here's another approach to getting rid of the sysconfig twiddling in the
> driver.  I wrote this some time ago at my former company ;) but don't
> think I ever got around to posting it.
> 
> It doesn't solve the whole problem (e.g. doesn't address the
> context_loss or enable_wakeup func pointers), but at least gets rid of
> the need for any SYSCONFIG access in the driver for the idle modes.

wakeup is still done by setting smart_idle_wakeup right, at least to
some IPs. Following omap_serial_enable_wakeup() calls, I can see that it
turns out to fiddle with sysconfig ;-)

> @@ -229,8 +209,6 @@ static void serial_omap_stop_tx(struct uart_port *port)
>  		serial_out(up, UART_IER, up->ier);
>  	}
>  
> -	serial_omap_set_forceidle(up);
> -
>  	pm_runtime_mark_last_busy(up->dev);
>  	pm_runtime_put_autosuspend(up->dev);
>  }
> @@ -298,7 +276,6 @@ static void serial_omap_start_tx(struct uart_port *port)
>  
>  	pm_runtime_get_sync(up->dev);
>  	serial_omap_enable_ier_thri(up);
> -	serial_omap_set_noidle(up);

this is no different then the patches Santosh has sent just a few
minutes ago, what gives ? It's exactly the same patch...

Anyway, I have exposed a problem with this aproach.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/36013e1f/attachment.sig>

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

[Santosh Shilimkar]

On Friday 15 February 2013 09:10 PM, Kevin Hilman wrote:
> Felipe Balbi <balbi@ti.com> writes:
>
>> 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.
>>
>> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
>
> Here's another approach to getting rid of the sysconfig twiddling in the
> driver.  I wrote this some time ago at my former company ;) but don't
> think I ever got around to posting it.
>
> It doesn't solve the whole problem (e.g. doesn't address the
> context_loss or enable_wakeup func pointers), but at least gets rid of
> the need for any SYSCONFIG access in the driver for the idle modes.
>
> Needs more thorough testing.
>
I posted similar patch series[1] yesterday after testing it on OMAP4/5
devices. OMAP3 testing seems to be ok as well. AM3XXX and OMAP2 test
results is what am waiting for.

Good to know that you had similar idea in mind to get rid of
UART sysc hackery.

Regards,
Santosh

[1] http://www.mail-archive.com/linux-omap at vger.kernel.org/msg85177.html

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

[Kevin Hilman]

Santosh Shilimkar <santosh.shilimkar@ti.com> writes:

> On Friday 15 February 2013 09:10 PM, Kevin Hilman wrote:
>> Felipe Balbi <balbi@ti.com> writes:
>>
>>> 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.
>>>
>>> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
>>
>> Here's another approach to getting rid of the sysconfig twiddling in the
>> driver.  I wrote this some time ago at my former company ;) but don't
>> think I ever got around to posting it.
>>
>> It doesn't solve the whole problem (e.g. doesn't address the
>> context_loss or enable_wakeup func pointers), but at least gets rid of
>> the need for any SYSCONFIG access in the driver for the idle modes.
>>
>> Needs more thorough testing.
>>
> I posted similar patch series[1] yesterday after testing it on OMAP4/5
> devices. OMAP3 testing seems to be ok as well. AM3XXX and OMAP2 test
> results is what am waiting for.

Yeah, I saw yours after I posted mine.  Just continue with yours, I'll
be glad to have someone else take this on.

> Good to know that you had similar idea in mind to get rid of
> UART sysc hackery.

Likewise.

Kevin

[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[Kevin Hilman]

Hi Felipe,

Felipe Balbi <balbi@ti.com> writes:

> Eventually, we need to be able to remove
> ti,hwmods DT attribute (or at a minimum
> ignore it).
>
> For new platforms, this patch could enable
> the transition by not relying on ti,hwmods
> to have functioning PM and Idle implementation.
>
> Notice that this poses no differences for
> platforms which are already supported, it
> just gives us means of dropping the relyance
> on hwmod for new platforms.
>
> NYET-Signed-off-by: Felipe Balbi <balbi@ti.com>
> ---
>  arch/arm/mach-omap2/omap_device.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/arch/arm/mach-omap2/omap_device.c b/arch/arm/mach-omap2/omap_device.c
> index e065daa..305eeb4 100644
> --- a/arch/arm/mach-omap2/omap_device.c
> +++ b/arch/arm/mach-omap2/omap_device.c
> @@ -796,13 +796,18 @@ static int __init omap_early_device_register(struct platform_device *pdev)
>  static int _od_runtime_suspend(struct device *dev)
>  {
>  	struct platform_device *pdev = to_platform_device(dev);
> +	struct omap_device *od = to_omap_device(pdev);
>  	int ret;
>  
>  	ret = pm_generic_runtime_suspend(dev);
>  
> +	if (!od)
> +		goto out;
> +

Rather than adding a check for every function, I think you will get the
effect by simply not hooking up the PM domain.

IOW, in omap_device_build_from_dt(), conditionalize:

	pdev->dev.pm_domain = &omap_device_pm_domain;

then none of the callbacks will be called in the first place, so they
won't need to be conditionalized.

Without a PM domain, they will fallback to the bus level hooks, which in
this case will be the platform_bus hooks, which will just call the
pm_generic functions.

Kevin

[RFC/NOT FOR MERGING 1/3] arm: omap: use generic implementation if !od

[Felipe Balbi]

Hi Kevin,

On Fri, Feb 15, 2013 at 07:28:22AM -0800, Kevin Hilman wrote:
> > @@ -796,13 +796,18 @@ static int __init omap_early_device_register(struct platform_device *pdev)
> >  static int _od_runtime_suspend(struct device *dev)
> >  {
> >  	struct platform_device *pdev = to_platform_device(dev);
> > +	struct omap_device *od = to_omap_device(pdev);
> >  	int ret;
> >  
> >  	ret = pm_generic_runtime_suspend(dev);
> >  
> > +	if (!od)
> > +		goto out;
> > +
> 
> Rather than adding a check for every function, I think you will get the
> effect by simply not hooking up the PM domain.
> 
> IOW, in omap_device_build_from_dt(), conditionalize:
> 
> 	pdev->dev.pm_domain = &omap_device_pm_domain;
> 
> then none of the callbacks will be called in the first place, so they
> won't need to be conditionalized.
> 
> Without a PM domain, they will fallback to the bus level hooks, which in
> this case will be the platform_bus hooks, which will just call the
> pm_generic functions.

good point, let's just sort out the other comments first though ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130215/b39b0723/attachment.sig>