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

Commit Message

Felipe Balbi Feb. 14, 2013, 11:15 a.m. UTC

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 <[e-mail]>
---
 arch/arm/mach-omap2/omap_device.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

Comments

Russell King - ARM Linux Feb. 14, 2013, 11:20 a.m. UTC | #1

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;
>  }

Tony Lindgren Feb. 14, 2013, 5:12 p.m. UTC | #2

* Felipe Balbi <[e-mail]> [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 <[e-mail]>
> ---
>  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
>

Felipe Balbi Feb. 14, 2013, 5:56 p.m. UTC | #3

Hi,

On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <[e-mail]> [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

Felipe Balbi Feb. 14, 2013, 5:57 p.m. UTC | #4

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.

Tony Lindgren Feb. 14, 2013, 6:12 p.m. UTC | #5

* Felipe Balbi <[e-mail]> [130214 10:00]:
> Hi,
> 
> On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
> > * Felipe Balbi <[e-mail]> [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

Felipe Balbi Feb. 14, 2013, 7:27 p.m. UTC | #6

Hi,

On Thu, Feb 14, 2013 at 10:12:17AM -0800, Tony Lindgren wrote:
> * Felipe Balbi <[e-mail]> [130214 10:00]:
> > Hi,
> > 
> > On Thu, Feb 14, 2013 at 09:12:53AM -0800, Tony Lindgren wrote:
> > > * Felipe Balbi <[e-mail]> [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@xxxxxxxx {
		l4_per@xxxxxxxx {
			...
		};

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

Tony Lindgren Feb. 14, 2013, 7:39 p.m. UTC | #7

* Felipe Balbi <[e-mail]> [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@xxxxxxxx {
> 		l4_per@xxxxxxxx {
> 			...
> 		};
> 
> 		l4_wakeup@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

Paul Walmsley Feb. 14, 2013, 8:47 p.m. UTC | #8

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

Paul Walmsley Feb. 14, 2013, 9:56 p.m. UTC | #9

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

Tony Lindgren Feb. 14, 2013, 10:22 p.m. UTC | #10

* Paul Walmsley <[e-mail]> [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

Felipe Balbi Feb. 15, 2013, 6:44 a.m. UTC | #11

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.

Vaibhav Bedia Feb. 15, 2013, 7:27 a.m. UTC | #12

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

Russell King - ARM Linux Feb. 15, 2013, 10:16 a.m. UTC | #13

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 <[e-mail]>
    Signed-off-by: Jesse Barnes <[e-mail]>

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

Santosh Shilimkar Feb. 15, 2013, 1:26 p.m. UTC | #14

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

Russell King - ARM Linux Feb. 15, 2013, 1:27 p.m. UTC | #15

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

Santosh Shilimkar Feb. 15, 2013, 1:31 p.m. UTC | #16

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

Kevin Hilman Feb. 15, 2013, 3:28 p.m. UTC | #17

Hi Felipe,

Felipe Balbi <[e-mail]> 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 <[e-mail]>
> ---
>  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

Felipe Balbi Feb. 15, 2013, 4:04 p.m. UTC | #18

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 ;-)

Tony Lindgren Feb. 15, 2013, 4:30 p.m. UTC | #19

* Santosh Shilimkar <[e-mail]> [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

Felipe Balbi Feb. 15, 2013, 4:42 p.m. UTC | #20

Hi,

On Fri, Feb 15, 2013 at 08:30:32AM -0800, Tony Lindgren wrote:
> * Santosh Shilimkar <[e-mail]> [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...

Santosh Shilimkar Feb. 16, 2013, 5:31 a.m. UTC | #21

On Friday 15 February 2013 10:00 PM, Tony Lindgren wrote:
> * Santosh Shilimkar <[e-mail]> [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/[e-mail]/msg85177.html

Nicolas Pitre Feb. 16, 2013, 5:36 a.m. UTC | #22

On Fri, 15 Feb 2013, Tony Lindgren wrote:

> * Santosh Shilimkar <[e-mail]> [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

Santosh Shilimkar Feb. 16, 2013, 5:48 a.m. UTC | #23

On Saturday 16 February 2013 11:06 AM, Nicolas Pitre wrote:
> On Fri, 15 Feb 2013, Tony Lindgren wrote:
>
>> * Santosh Shilimkar <[e-mail]> [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

Santosh Shilimkar Feb. 16, 2013, 6:01 a.m. UTC | #24

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 <[e-mail]> [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

Felipe Balbi Feb. 16, 2013, 8:55 a.m. UTC | #25

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.

Santosh Shilimkar Feb. 16, 2013, 9:17 a.m. UTC | #26

+ 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

Felipe Balbi Feb. 16, 2013, 9:22 a.m. UTC | #27

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.

Santosh Shilimkar Feb. 16, 2013, 9:31 a.m. UTC | #28

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

Vaibhav Bedia Feb. 18, 2013, 8:08 a.m. UTC | #29

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

Santosh Shilimkar Feb. 18, 2013, 8:28 a.m. UTC | #30

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

Kevin Hilman Feb. 18, 2013, 3:27 p.m. UTC | #31

Felipe Balbi <[e-mail]> 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

Paul Walmsley Feb. 19, 2013, 3:27 p.m. UTC | #32

Hi,

On Thu, 14 Feb 2013, Tony Lindgren wrote:

> * Paul Walmsley <[e-mail]> [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

Tony Lindgren Feb. 19, 2013, 4:38 p.m. UTC | #33

* Paul Walmsley <[e-mail]> [130219 07:30]:
> Hi,
> 
> On Thu, 14 Feb 2013, Tony Lindgren wrote:
> 
> > * Paul Walmsley <[e-mail]> [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

Felipe Balbi Feb. 19, 2013, 4:57 p.m. UTC | #34

Hi,

On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote:
> > > * Paul Walmsley <[e-mail]> [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...

Tony Lindgren Feb. 19, 2013, 5:43 p.m. UTC | #35

* Felipe Balbi <[e-mail]> [130219 09:01]:
> Hi,
> 
> On Tue, Feb 19, 2013 at 08:38:20AM -0800, Tony Lindgren wrote:
> > > > * Paul Walmsley <[e-mail]> [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

Kevin Hilman Feb. 19, 2013, 7:16 p.m. UTC | #36

[...]

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

Felipe Balbi Feb. 19, 2013, 7:32 p.m. UTC | #37

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.

Kevin Hilman Feb. 19, 2013, 7:50 p.m. UTC | #38

Felipe Balbi <[e-mail]> 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

Felipe Balbi Feb. 19, 2013, 8:10 p.m. UTC | #39

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

Kevin Hilman Feb. 19, 2013, 8:25 p.m. UTC | #40

Felipe Balbi <[e-mail]> 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

Santosh Shilimkar Feb. 20, 2013, 6:26 a.m. UTC | #41

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

Paul Walmsley Feb. 20, 2013, 5:38 p.m. UTC | #42

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

Felipe Balbi Feb. 20, 2013, 7:16 p.m. UTC | #43

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.

Paul Walmsley Feb. 20, 2013, 8:03 p.m. UTC | #44

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

Russell King - ARM Linux Feb. 20, 2013, 8:37 p.m. UTC | #45

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.

Peter De Schrijver Feb. 21, 2013, 10:16 a.m. UTC | #46

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

Peter Korsgaard Feb. 21, 2013, 12:09 p.m. UTC | #47

>>>>> "Peter" == Peter De Schrijver <[e-mail]> 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.

Patch

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