Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1320127 > unrolled thread

Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Suman Anna <s-anna@ti.com> - 2016-01-28 00:20 +0100
    Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Tony Lindgren <tony@atomide.com> - 2016-01-28 19:40 +0100
      Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Suman Anna <s-anna@ti.com> - 2016-01-28 22:20 +0100
      Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Kishon Vijay Abraham I <kishon@ti.com> - 2016-02-02 11:50 +0100
        Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Kishon Vijay Abraham I <kishon@ti.com> - 2016-02-05 05:30 +0100
        Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform  reset Paul Walmsley <paul@pwsan.com> - 2016-02-08 03:50 +0100
          Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset Suman Anna <s-anna@ti.com> - 2016-02-08 22:00 +0100
            Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform  reset Paul Walmsley <paul@pwsan.com> - 2016-02-09 09:50 +0100

#1320127 — Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset

On 01/27/2016 12:56 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [160127 10:17]:
>> On 01/27/2016 11:31 AM, Tony Lindgren wrote:
>>> Why do you need another reset here? Can't you just implement PM runtime
>>> in the driver and do the usual pm_runtime_put_sync followed by
>>> pm_runtime_disable?
>>
>> The omap_hwmod_enable/disable code does not deal with hardresets (PRCM
>> reset lines) and so the pm_runtime_get_sync/put_sync only end up dealing
>> with clocks, and we need to invoke the reset functions separately.
>> Modules with softresets in SYSCONFIG are ok, as they are dealt with
>> properly.
> 
> Hmm _reset() in omap_hwmod.c has this to call _assert_hardreset:
> 
> 	if (oh->class->reset) {
> 		r = oh->class->reset(oh);
> 	} else {
> 		if (oh->rst_lines_cnt > 0) {
> 			for (i = 0; i < oh->rst_lines_cnt; i++)
> 				_assert_hardreset(oh, oh->rst_lines[i].name);
> 			return 0;
> 		} else {
> 			r = _ocp_softreset(oh);
> 			if (r == -ENOENT)
> 				r = 0;
> 		}
> 	}

Right, hwmod code does the initial reset.

> Care to explain what exactly the problem with the hwmod code not doing
> the reset on init?

And we only need to deassert the reset in probe. Technically, we don't
need to assert first and deassert in probe, and that was a design choice
made by Kishon.

> And why do you need to do another reset in dra7xx_pcie_remove()?

Primarily to restore the reset state back to what it was after the
driver remove gets called. We cannot call deassert twice without calling
a assert in between. Kishon had originally added the assert and deassert
only in probe, but nothing in remove, they ought to be deassert in probe
and assert in remove to match initial hardware state, and to also make
it work across multiple probe/remove.

>>> Basically I'm wondering how come we need these platform data callbacks
>>> at all.
>>
>> The hardresets are controlled through the
>> omap_device_assert(deassert)_hardreset functions, and since these are
>> limited to mach-omap2, we are invoking them through platform data callbacks.
> 
> Right.. But I'm wondering about the why you need to do this in the
> driver at all part :)

The initial reset at init time is okay, but hwmod _enable() bails out if
the resets lines are asserted. This was a change made long time back, I
believe to deal with the problems around the DSP enabling sequences. As
such, pm_runtime_get_sync() and put_sync() do not deassert and assert
the resets.

regards
Suman

[toc] | [next] | [standalone]

#1320940

* Suman Anna <s-anna@ti.com> [160127 15:17]:
> On 01/27/2016 12:56 PM, Tony Lindgren wrote:
> > * Suman Anna <s-anna@ti.com> [160127 10:17]:
> >> On 01/27/2016 11:31 AM, Tony Lindgren wrote:
> >>> Why do you need another reset here? Can't you just implement PM runtime
> >>> in the driver and do the usual pm_runtime_put_sync followed by
> >>> pm_runtime_disable?
> >>
> >> The omap_hwmod_enable/disable code does not deal with hardresets (PRCM
> >> reset lines) and so the pm_runtime_get_sync/put_sync only end up dealing
> >> with clocks, and we need to invoke the reset functions separately.
> >> Modules with softresets in SYSCONFIG are ok, as they are dealt with
> >> properly.
> > 
> > Hmm _reset() in omap_hwmod.c has this to call _assert_hardreset:
> > 
> > 	if (oh->class->reset) {
> > 		r = oh->class->reset(oh);
> > 	} else {
> > 		if (oh->rst_lines_cnt > 0) {
> > 			for (i = 0; i < oh->rst_lines_cnt; i++)
> > 				_assert_hardreset(oh, oh->rst_lines[i].name);
> > 			return 0;
> > 		} else {
> > 			r = _ocp_softreset(oh);
> > 			if (r == -ENOENT)
> > 				r = 0;
> > 		}
> > 	}
> 
> Right, hwmod code does the initial reset.
> 
> > Care to explain what exactly the problem with the hwmod code not doing
> > the reset on init?
> 
> And we only need to deassert the reset in probe. Technically, we don't
> need to assert first and deassert in probe, and that was a design choice
> made by Kishon.

OK so if hwmod code has already done the reset, then why would you need
to deassert reset in the device driver probe?

> > And why do you need to do another reset in dra7xx_pcie_remove()?
> 
> Primarily to restore the reset state back to what it was after the
> driver remove gets called. We cannot call deassert twice without calling
> a assert in between. Kishon had originally added the assert and deassert
> only in probe, but nothing in remove, they ought to be deassert in probe
> and assert in remove to match initial hardware state, and to also make
> it work across multiple probe/remove.

I don't understand this part either.. Usually you just power up and init
the registers to a sane state in a device driver probe and on exit just
power down the device.

> >>> Basically I'm wondering how come we need these platform data callbacks
> >>> at all.
> >>
> >> The hardresets are controlled through the
> >> omap_device_assert(deassert)_hardreset functions, and since these are
> >> limited to mach-omap2, we are invoking them through platform data callbacks.
> > 
> > Right.. But I'm wondering about the why you need to do this in the
> > driver at all part :)
> 
> The initial reset at init time is okay, but hwmod _enable() bails out if
> the resets lines are asserted. This was a change made long time back, I
> believe to deal with the problems around the DSP enabling sequences. As
> such, pm_runtime_get_sync() and put_sync() do not deassert and assert
> the resets.

OK if the hwmod code does not deassert reset lines properly on enable,
then that sounds like a bug that should be fixed instead of adding
device specific work arounds.

Sorry to keep dragging this on a bit longer, but I think we need to
hear Paul's comments on this one.

Regards,

Tony

[toc] | [prev] | [next] | [standalone]

#1321065

On 01/28/2016 12:31 PM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [160127 15:17]:
>> On 01/27/2016 12:56 PM, Tony Lindgren wrote:
>>> * Suman Anna <s-anna@ti.com> [160127 10:17]:
>>>> On 01/27/2016 11:31 AM, Tony Lindgren wrote:
>>>>> Why do you need another reset here? Can't you just implement PM runtime
>>>>> in the driver and do the usual pm_runtime_put_sync followed by
>>>>> pm_runtime_disable?
>>>>
>>>> The omap_hwmod_enable/disable code does not deal with hardresets (PRCM
>>>> reset lines) and so the pm_runtime_get_sync/put_sync only end up dealing
>>>> with clocks, and we need to invoke the reset functions separately.
>>>> Modules with softresets in SYSCONFIG are ok, as they are dealt with
>>>> properly.
>>>
>>> Hmm _reset() in omap_hwmod.c has this to call _assert_hardreset:
>>>
>>> 	if (oh->class->reset) {
>>> 		r = oh->class->reset(oh);
>>> 	} else {
>>> 		if (oh->rst_lines_cnt > 0) {
>>> 			for (i = 0; i < oh->rst_lines_cnt; i++)
>>> 				_assert_hardreset(oh, oh->rst_lines[i].name);
>>> 			return 0;
>>> 		} else {
>>> 			r = _ocp_softreset(oh);
>>> 			if (r == -ENOENT)
>>> 				r = 0;
>>> 		}
>>> 	}
>>
>> Right, hwmod code does the initial reset.
>>
>>> Care to explain what exactly the problem with the hwmod code not doing
>>> the reset on init?
>>
>> And we only need to deassert the reset in probe. Technically, we don't
>> need to assert first and deassert in probe, and that was a design choice
>> made by Kishon.
> 
> OK so if hwmod code has already done the reset, then why would you need
> to deassert reset in the device driver probe?

So the _reset() above asserts the reset for IPs with PRCM reset lines,
but module is not enabled (no register accesses even if clocks enabled).
The _enable() code bails out if there are PRCM reset lines (there are
varied IPs with resets including processors, and we really cannot enable
and idle it without loading some code that would have executed WFI).

> 
>>> And why do you need to do another reset in dra7xx_pcie_remove()?
>>
>> Primarily to restore the reset state back to what it was after the
>> driver remove gets called. We cannot call deassert twice without calling
>> a assert in between. Kishon had originally added the assert and deassert
>> only in probe, but nothing in remove, they ought to be deassert in probe
>> and assert in remove to match initial hardware state, and to also make
>> it work across multiple probe/remove.
> 
> I don't understand this part either.. Usually you just power up and init
> the registers to a sane state in a device driver probe and on exit just
> power down the device.

Yes, in the case of IPs with hard-reset lines, that init is left to the
drivers.

> 
>>>>> Basically I'm wondering how come we need these platform data callbacks
>>>>> at all.
>>>>
>>>> The hardresets are controlled through the
>>>> omap_device_assert(deassert)_hardreset functions, and since these are
>>>> limited to mach-omap2, we are invoking them through platform data callbacks.
>>>
>>> Right.. But I'm wondering about the why you need to do this in the
>>> driver at all part :)
>>
>> The initial reset at init time is okay, but hwmod _enable() bails out if
>> the resets lines are asserted. This was a change made long time back, I
>> believe to deal with the problems around the DSP enabling sequences. As
>> such, pm_runtime_get_sync() and put_sync() do not deassert and assert
>> the resets.
> 
> OK if the hwmod code does not deassert reset lines properly on enable,
> then that sounds like a bug that should be fixed instead of adding
> device specific work arounds.

As I said above, not all IPs with hard-reset lines have the same power
on/power off sequences.. IPs with only SYSCONFIG based soft-reset all
pretty much follow the PRCM HW_Auto idling, so dealing with them is
rather straightforward in the common hwmod code. I have had to do rather
funky stuff in our product kernels when doing suspend/resume on IOMMUs,
remoteprocs.

> Sorry to keep dragging this on a bit longer, but I think we need to
> hear Paul's comments on this one.

Yeah, it would be good to restart this discussion, as I will be adding
the DT support for the remoteproc devices a bit later.

regards
Suman

[toc] | [prev] | [next] | [standalone]

#1323964

Hi,

On Friday 29 January 2016 12:01 AM, Tony Lindgren wrote:
> * Suman Anna <s-anna@ti.com> [160127 15:17]:
>> On 01/27/2016 12:56 PM, Tony Lindgren wrote:
>>> * Suman Anna <s-anna@ti.com> [160127 10:17]:
>>>> On 01/27/2016 11:31 AM, Tony Lindgren wrote:
>>>>> Why do you need another reset here? Can't you just implement PM runtime
>>>>> in the driver and do the usual pm_runtime_put_sync followed by
>>>>> pm_runtime_disable?
>>>>
>>>> The omap_hwmod_enable/disable code does not deal with hardresets (PRCM
>>>> reset lines) and so the pm_runtime_get_sync/put_sync only end up dealing
>>>> with clocks, and we need to invoke the reset functions separately.
>>>> Modules with softresets in SYSCONFIG are ok, as they are dealt with
>>>> properly.
>>>
>>> Hmm _reset() in omap_hwmod.c has this to call _assert_hardreset:
>>>
>>> 	if (oh->class->reset) {
>>> 		r = oh->class->reset(oh);
>>> 	} else {
>>> 		if (oh->rst_lines_cnt > 0) {
>>> 			for (i = 0; i < oh->rst_lines_cnt; i++)
>>> 				_assert_hardreset(oh, oh->rst_lines[i].name);
>>> 			return 0;
>>> 		} else {
>>> 			r = _ocp_softreset(oh);
>>> 			if (r == -ENOENT)
>>> 				r = 0;
>>> 		}
>>> 	}
>>
>> Right, hwmod code does the initial reset.
>>
>>> Care to explain what exactly the problem with the hwmod code not doing
>>> the reset on init?
>>
>> And we only need to deassert the reset in probe. Technically, we don't
>> need to assert first and deassert in probe, and that was a design choice
>> made by Kishon.
> 
> OK so if hwmod code has already done the reset, then why would you need
> to deassert reset in the device driver probe?

The hwmod code only asserts the reset lines and that is not enough to access
the PCI registers. The reset lines must be de-asserted before accessing the
PCIe registers.
> 
>>> And why do you need to do another reset in dra7xx_pcie_remove()?
>>
>> Primarily to restore the reset state back to what it was after the
>> driver remove gets called. We cannot call deassert twice without calling
>> a assert in between. Kishon had originally added the assert and deassert
>> only in probe, but nothing in remove, they ought to be deassert in probe
>> and assert in remove to match initial hardware state, and to also make
>> it work across multiple probe/remove.

right. I thought if some program like the bootloader requires the reset lines
to be in initial hw state, then it might break on 'reboot'. So restored it back
to the initial hw state.
> 
> I don't understand this part either.. Usually you just power up and init
> the registers to a sane state in a device driver probe and on exit just
> power down the device.
> 
>>>>> Basically I'm wondering how come we need these platform data callbacks
>>>>> at all.
>>>>
>>>> The hardresets are controlled through the
>>>> omap_device_assert(deassert)_hardreset functions, and since these are
>>>> limited to mach-omap2, we are invoking them through platform data callbacks.
>>>
>>> Right.. But I'm wondering about the why you need to do this in the
>>> driver at all part :)
>>
>> The initial reset at init time is okay, but hwmod _enable() bails out if
>> the resets lines are asserted. This was a change made long time back, I
>> believe to deal with the problems around the DSP enabling sequences. As
>> such, pm_runtime_get_sync() and put_sync() do not deassert and assert
>> the resets.
> 
> OK if the hwmod code does not deassert reset lines properly on enable,
> then that sounds like a bug that should be fixed instead of adding
> device specific work arounds.

I think some devices require the reset lines to be asserted and some devices
require it to be de-asserted and hwmod was designed when there was only the
first type of devices. I'm not sure though.
> 
> Sorry to keep dragging this on a bit longer, but I think we need to
> hear Paul's comments on this one.

I agree.
Paul, what do you think is the best way forward to perform reset?

Thanks
Kishon

[toc] | [prev] | [next] | [standalone]

#1327473

Hi Paul,

On Tuesday 02 February 2016 04:10 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Friday 29 January 2016 12:01 AM, Tony Lindgren wrote:
>> * Suman Anna <s-anna@ti.com> [160127 15:17]:
>>> On 01/27/2016 12:56 PM, Tony Lindgren wrote:
>>>> * Suman Anna <s-anna@ti.com> [160127 10:17]:
>>>>> On 01/27/2016 11:31 AM, Tony Lindgren wrote:
>>>>>> Why do you need another reset here? Can't you just implement PM runtime
>>>>>> in the driver and do the usual pm_runtime_put_sync followed by
>>>>>> pm_runtime_disable?
>>>>>
>>>>> The omap_hwmod_enable/disable code does not deal with hardresets (PRCM
>>>>> reset lines) and so the pm_runtime_get_sync/put_sync only end up dealing
>>>>> with clocks, and we need to invoke the reset functions separately.
>>>>> Modules with softresets in SYSCONFIG are ok, as they are dealt with
>>>>> properly.
>>>>
>>>> Hmm _reset() in omap_hwmod.c has this to call _assert_hardreset:
>>>>
>>>> 	if (oh->class->reset) {
>>>> 		r = oh->class->reset(oh);
>>>> 	} else {
>>>> 		if (oh->rst_lines_cnt > 0) {
>>>> 			for (i = 0; i < oh->rst_lines_cnt; i++)
>>>> 				_assert_hardreset(oh, oh->rst_lines[i].name);
>>>> 			return 0;
>>>> 		} else {
>>>> 			r = _ocp_softreset(oh);
>>>> 			if (r == -ENOENT)
>>>> 				r = 0;
>>>> 		}
>>>> 	}
>>>
>>> Right, hwmod code does the initial reset.
>>>
>>>> Care to explain what exactly the problem with the hwmod code not doing
>>>> the reset on init?
>>>
>>> And we only need to deassert the reset in probe. Technically, we don't
>>> need to assert first and deassert in probe, and that was a design choice
>>> made by Kishon.
>>
>> OK so if hwmod code has already done the reset, then why would you need
>> to deassert reset in the device driver probe?
> 
> The hwmod code only asserts the reset lines and that is not enough to access
> the PCI registers. The reset lines must be de-asserted before accessing the
> PCIe registers.
>>
>>>> And why do you need to do another reset in dra7xx_pcie_remove()?
>>>
>>> Primarily to restore the reset state back to what it was after the
>>> driver remove gets called. We cannot call deassert twice without calling
>>> a assert in between. Kishon had originally added the assert and deassert
>>> only in probe, but nothing in remove, they ought to be deassert in probe
>>> and assert in remove to match initial hardware state, and to also make
>>> it work across multiple probe/remove.
> 
> right. I thought if some program like the bootloader requires the reset lines
> to be in initial hw state, then it might break on 'reboot'. So restored it back
> to the initial hw state.
>>
>> I don't understand this part either.. Usually you just power up and init
>> the registers to a sane state in a device driver probe and on exit just
>> power down the device.
>>
>>>>>> Basically I'm wondering how come we need these platform data callbacks
>>>>>> at all.
>>>>>
>>>>> The hardresets are controlled through the
>>>>> omap_device_assert(deassert)_hardreset functions, and since these are
>>>>> limited to mach-omap2, we are invoking them through platform data callbacks.
>>>>
>>>> Right.. But I'm wondering about the why you need to do this in the
>>>> driver at all part :)
>>>
>>> The initial reset at init time is okay, but hwmod _enable() bails out if
>>> the resets lines are asserted. This was a change made long time back, I
>>> believe to deal with the problems around the DSP enabling sequences. As
>>> such, pm_runtime_get_sync() and put_sync() do not deassert and assert
>>> the resets.
>>
>> OK if the hwmod code does not deassert reset lines properly on enable,
>> then that sounds like a bug that should be fixed instead of adding
>> device specific work arounds.
> 
> I think some devices require the reset lines to be asserted and some devices
> require it to be de-asserted and hwmod was designed when there was only the
> first type of devices. I'm not sure though.
>>
>> Sorry to keep dragging this on a bit longer, but I think we need to
>> hear Paul's comments on this one.
> 
> I agree.
> Paul, what do you think is the best way forward to perform reset?

Can you give your feedback as we are at the risk of PCIe driver being removed?

Thanks
Kishon

> 
> Thanks
> Kishon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[toc] | [prev] | [next] | [standalone]

#1328721 — Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset

On Tue, 2 Feb 2016, Kishon Vijay Abraham I wrote:

> Paul, what do you think is the best way forward to perform reset?

Many of the IP blocks with PRM hardreset lines are processor IP blocks. 
Those often need special reset handling to ensure that WFI/HLT-like 
instructions are executed after reset.  This special handling ensures that 
the IP blocks' bus initiator interfaces indicate that they are in standby 
to the PRCM - thus allowing power management for the rest of the chip to 
work correctly.

But that doesn't seem to be the case with PCIe - and maybe others - 
possibly some of the MMUs?  So how about just creating a new hwmod flag to 
mark all of the initiator IP blocks that require driver-based special 
handling during _enable() - i.e., most of the processor IP blocks. Then 
for the rest, like PCIe, implement a default behavior in the hwmod code to 
automatically release the IP block's hardreset lines in 
omap_hwmod.c:_enable()?  Something similar to what's enclosed at the 
bottom of this message.  I've annotated what will be needed in the 
OMAP44xx file; similar flags will be needed in any other hwmod data file 
that contains IP blocks with hard reset lines defined.

Either that - or you could write custom reset handlers for all of the 
processor IP blocks that put them into WFI/HLT.

I leave it to you TI folks to write and test the actual patches, since as 
you probably know, I don't have any DRA7xx/AM57xx boards in the testbed.  

As far as reasserting hardreset in *remove(), there's already hwmod code 
to do that in omap_hwmod.c:_shutdown().  I don't recall any more if we 
currently have code in the stack that calls it.  Ideally the device model 
code should call that during or after a .remove() call.


- Paul


---
 arch/arm/mach-omap2/omap_hwmod.c           | 16 +++++++++++-----
 arch/arm/mach-omap2/omap_hwmod.h           | 12 ++++++++++++
 arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  6 ++++++
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
index e9f65fec55c0..ab66dd988709 100644
--- a/arch/arm/mach-omap2/omap_hwmod.c
+++ b/arch/arm/mach-omap2/omap_hwmod.c
@@ -2077,7 +2077,7 @@ static int _enable_preprogram(struct omap_hwmod *oh)
  */
 static int _enable(struct omap_hwmod *oh)
 {
-	int r;
+	int r, i;
 	int hwsup = 0;
 
 	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
@@ -2109,17 +2109,23 @@ static int _enable(struct omap_hwmod *oh)
 	}
 
 	/*
-	 * If an IP block contains HW reset lines and all of them are
-	 * asserted, we let integration code associated with that
-	 * block handle the enable.  We've received very little
+	 * If an IP block contains HW reset lines, all of them are
+	 * asserted, and the IP block is marked as requiring a custom
+	 * hardreset handler, we let integration code associated with
+	 * that block handle the enable.  We've received very little
 	 * information on what those driver authors need, and until
 	 * detailed information is provided and the driver code is
 	 * posted to the public lists, this is probably the best we
 	 * can do.
 	 */
-	if (_are_all_hardreset_lines_asserted(oh))
+	if ((oh->flags & HWMOD_CUSTOM_HARDRESET) &&
+	    _are_all_hardreset_lines_asserted(oh))
 		return 0;
 
+	/* If the IP block is an initiator, release it from hardreset */
+	for (i = 0; i < oh->rst_lines_cnt; i++)
+		_deassert_hardreset(oh, oh->rst_lines[i].name);
+
 	/* Mux pins for device runtime if populated */
 	if (oh->mux && (!oh->mux->enabled ||
 			((oh->_state == _HWMOD_STATE_IDLE) &&
diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
index 76bce11c85a4..4198829551a4 100644
--- a/arch/arm/mach-omap2/omap_hwmod.h
+++ b/arch/arm/mach-omap2/omap_hwmod.h
@@ -525,6 +525,17 @@ struct omap_hwmod_omap4_prcm {
  *     or idled.
  * HWMOD_OPT_CLKS_NEEDED: The optional clocks are needed for the module to
  *     operate and they need to be handled at the same time as the main_clk.
+ * HWMOD_CUSTOM_HARDRESET: By default, if a hwmod has PRCM hardreset
+ *     lines associated with it (i.e., a populated .rst_lines field in
+ *     the hwmod), the hwmod code will assert the hardreset lines when
+ *     the IP block is initially reset, deassert the hardreset lines
+ *     in _enable(), and reassert them in _shutdown().  If this flag
+ *     is set, the hwmod code will not deassert the hardreset lines in
+ *     _enable(), leaving this responsibility to the driver code.  This flag may
+ *     be needed for processor IP blocks that must be put into a WFI/HLT
+ *     state after reset is deasserted, lest the processor leave its MSTANDBY
+ *     signal deasserted, thus blocking the chip from entering a system-wide
+ *     low power state.
  */
 #define HWMOD_SWSUP_SIDLE			(1 << 0)
 #define HWMOD_SWSUP_MSTANDBY			(1 << 1)
@@ -541,6 +552,7 @@ struct omap_hwmod_omap4_prcm {
 #define HWMOD_SWSUP_SIDLE_ACT			(1 << 12)
 #define HWMOD_RECONFIG_IO_CHAIN			(1 << 13)
 #define HWMOD_OPT_CLKS_NEEDED			(1 << 14)
+#define HWMOD_CUSTOM_HARDRESET			(1 << 15)
 
 /*
  * omap_hwmod._int_flags definitions
diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
index dad871a4cd96..20501f0e3c6c 100644
--- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
@@ -553,6 +553,7 @@ static struct omap_hwmod omap44xx_dsp_hwmod = {
 			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
+	.flags		= HWMOD_CUSTOM_HARDRESET,
 };
 
 /*
@@ -1433,6 +1434,7 @@ static struct omap_hwmod omap44xx_ipu_hwmod = {
 			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
+	.flags		= HWMOD_CUSTOM_HARDRESET,
 };
 
 /*
@@ -1517,6 +1519,7 @@ static struct omap_hwmod omap44xx_iva_hwmod = {
 			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
+	.flags		= HWMOD_CUSTOM_HARDRESET,
 };
 
 /*
@@ -2115,6 +2118,7 @@ static struct omap_hwmod omap44xx_mmu_ipu_hwmod = {
 			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
+	.flags		= HWMOD_CUSTOM_HARDRESET, /* XXX doublecheck */
 };
 
 /* mmu dsp */
@@ -2147,6 +2151,7 @@ static struct omap_hwmod omap44xx_mmu_dsp_hwmod = {
 			.modulemode   = MODULEMODE_HWCTRL,
 		},
 	},
+	.flags		= HWMOD_CUSTOM_HARDRESET, /* XXX doublecheck */
 };
 
 /*
@@ -2299,6 +2304,7 @@ static struct omap_hwmod omap44xx_prm_hwmod = {
 	.class		= &omap44xx_prcm_hwmod_class,
 	.rst_lines	= omap44xx_prm_resets,
 	.rst_lines_cnt	= ARRAY_SIZE(omap44xx_prm_resets),
+	.flags		= HWMOD_CUSTOM_HARDRESET,
 };
 
 /*
-- 
2.7.0

[toc] | [prev] | [next] | [standalone]

#1329564

Hi Paul,

On 02/07/2016 08:48 PM, Paul Walmsley wrote:
> On Tue, 2 Feb 2016, Kishon Vijay Abraham I wrote:
> 
>> Paul, what do you think is the best way forward to perform reset?
> 
> Many of the IP blocks with PRM hardreset lines are processor IP blocks. 
> Those often need special reset handling to ensure that WFI/HLT-like 
> instructions are executed after reset.  This special handling ensures that 
> the IP blocks' bus initiator interfaces indicate that they are in standby 
> to the PRCM - thus allowing power management for the rest of the chip to 
> work correctly.
> 
> But that doesn't seem to be the case with PCIe - and maybe others - 
> possibly some of the MMUs?  

Yeah, the sequencing between clocks and resets would still be the same
for MMUs, so, adding the custom flags for MMUs is fine.

So how about just creating a new hwmod flag to
> mark all of the initiator IP blocks that require driver-based special 
> handling during _enable() - i.e., most of the processor IP blocks. Then 
> for the rest, like PCIe, implement a default behavior in the hwmod code to 
> automatically release the IP block's hardreset lines in 
> omap_hwmod.c:_enable()?  Something similar to what's enclosed at the 
> bottom of this message.  I've annotated what will be needed in the 
> OMAP44xx file; similar flags will be needed in any other hwmod data file 
> that contains IP blocks with hard reset lines defined.
> 
> Either that - or you could write custom reset handlers for all of the 
> processor IP blocks that put them into WFI/HLT.
> 
> I leave it to you TI folks to write and test the actual patches, since as 
> you probably know, I don't have any DRA7xx/AM57xx boards in the testbed.  
> 
> As far as reasserting hardreset in *remove(), there's already hwmod code 
> to do that in omap_hwmod.c:_shutdown().  I don't recall any more if we 
> currently have code in the stack that calls it.  Ideally the device model 
> code should call that during or after a .remove() call.

Yeah, don't think there is any code that exercises
omap_hwmod_shutdown(). We used to have an omap_device_shutdown() but
that function has been removed in commit c1d1cd597fc7 ("ARM: OMAP2+:
omap_device: remove obsolete pm_lats and early_device code"). We used to
exercise this using custom pm_lats replacing idle with shutdown in the
out-of-tree processor drivers.

> 
> 
> - Paul
> 
> 
> ---
>  arch/arm/mach-omap2/omap_hwmod.c           | 16 +++++++++++-----
>  arch/arm/mach-omap2/omap_hwmod.h           | 12 ++++++++++++
>  arch/arm/mach-omap2/omap_hwmod_44xx_data.c |  6 ++++++
>  3 files changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c
> index e9f65fec55c0..ab66dd988709 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.c
> +++ b/arch/arm/mach-omap2/omap_hwmod.c
> @@ -2077,7 +2077,7 @@ static int _enable_preprogram(struct omap_hwmod *oh)
>   */
>  static int _enable(struct omap_hwmod *oh)
>  {
> -	int r;
> +	int r, i;
>  	int hwsup = 0;
>  
>  	pr_debug("omap_hwmod: %s: enabling\n", oh->name);
> @@ -2109,17 +2109,23 @@ static int _enable(struct omap_hwmod *oh)
>  	}
>  
>  	/*
> -	 * If an IP block contains HW reset lines and all of them are
> -	 * asserted, we let integration code associated with that
> -	 * block handle the enable.  We've received very little
> +	 * If an IP block contains HW reset lines, all of them are
> +	 * asserted, and the IP block is marked as requiring a custom
> +	 * hardreset handler, we let integration code associated with
> +	 * that block handle the enable.  We've received very little
>  	 * information on what those driver authors need, and until
>  	 * detailed information is provided and the driver code is
>  	 * posted to the public lists, this is probably the best we
>  	 * can do.
>  	 */
> -	if (_are_all_hardreset_lines_asserted(oh))
> +	if ((oh->flags & HWMOD_CUSTOM_HARDRESET) &&
> +	    _are_all_hardreset_lines_asserted(oh))
>  		return 0;
>  
> +	/* If the IP block is an initiator, release it from hardreset */
> +	for (i = 0; i < oh->rst_lines_cnt; i++)
> +		_deassert_hardreset(oh, oh->rst_lines[i].name);

I believe this will cause a problem as typically we release the reset
and then call pm_runtime_get_sync() to enable the clock. We are not
checking error code, but if were, I do think _deassert_hardreset would
return a failure.

regards
Suman

> +
>  	/* Mux pins for device runtime if populated */
>  	if (oh->mux && (!oh->mux->enabled ||
>  			((oh->_state == _HWMOD_STATE_IDLE) &&
> diff --git a/arch/arm/mach-omap2/omap_hwmod.h b/arch/arm/mach-omap2/omap_hwmod.h
> index 76bce11c85a4..4198829551a4 100644
> --- a/arch/arm/mach-omap2/omap_hwmod.h
> +++ b/arch/arm/mach-omap2/omap_hwmod.h
> @@ -525,6 +525,17 @@ struct omap_hwmod_omap4_prcm {
>   *     or idled.
>   * HWMOD_OPT_CLKS_NEEDED: The optional clocks are needed for the module to
>   *     operate and they need to be handled at the same time as the main_clk.
> + * HWMOD_CUSTOM_HARDRESET: By default, if a hwmod has PRCM hardreset
> + *     lines associated with it (i.e., a populated .rst_lines field in
> + *     the hwmod), the hwmod code will assert the hardreset lines when
> + *     the IP block is initially reset, deassert the hardreset lines
> + *     in _enable(), and reassert them in _shutdown().  If this flag
> + *     is set, the hwmod code will not deassert the hardreset lines in
> + *     _enable(), leaving this responsibility to the driver code.  This flag may
> + *     be needed for processor IP blocks that must be put into a WFI/HLT
> + *     state after reset is deasserted, lest the processor leave its MSTANDBY
> + *     signal deasserted, thus blocking the chip from entering a system-wide
> + *     low power state.
>   */
>  #define HWMOD_SWSUP_SIDLE			(1 << 0)
>  #define HWMOD_SWSUP_MSTANDBY			(1 << 1)
> @@ -541,6 +552,7 @@ struct omap_hwmod_omap4_prcm {
>  #define HWMOD_SWSUP_SIDLE_ACT			(1 << 12)
>  #define HWMOD_RECONFIG_IO_CHAIN			(1 << 13)
>  #define HWMOD_OPT_CLKS_NEEDED			(1 << 14)
> +#define HWMOD_CUSTOM_HARDRESET			(1 << 15)
>  
>  /*
>   * omap_hwmod._int_flags definitions
> diff --git a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> index dad871a4cd96..20501f0e3c6c 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_44xx_data.c
> @@ -553,6 +553,7 @@ static struct omap_hwmod omap44xx_dsp_hwmod = {
>  			.modulemode   = MODULEMODE_HWCTRL,
>  		},
>  	},
> +	.flags		= HWMOD_CUSTOM_HARDRESET,
>  };
>  
>  /*
> @@ -1433,6 +1434,7 @@ static struct omap_hwmod omap44xx_ipu_hwmod = {
>  			.modulemode   = MODULEMODE_HWCTRL,
>  		},
>  	},
> +	.flags		= HWMOD_CUSTOM_HARDRESET,
>  };
>  
>  /*
> @@ -1517,6 +1519,7 @@ static struct omap_hwmod omap44xx_iva_hwmod = {
>  			.modulemode   = MODULEMODE_HWCTRL,
>  		},
>  	},
> +	.flags		= HWMOD_CUSTOM_HARDRESET,
>  };
>  
>  /*
> @@ -2115,6 +2118,7 @@ static struct omap_hwmod omap44xx_mmu_ipu_hwmod = {
>  			.modulemode   = MODULEMODE_HWCTRL,
>  		},
>  	},
> +	.flags		= HWMOD_CUSTOM_HARDRESET, /* XXX doublecheck */
>  };
>  
>  /* mmu dsp */
> @@ -2147,6 +2151,7 @@ static struct omap_hwmod omap44xx_mmu_dsp_hwmod = {
>  			.modulemode   = MODULEMODE_HWCTRL,
>  		},
>  	},
> +	.flags		= HWMOD_CUSTOM_HARDRESET, /* XXX doublecheck */
>  };
>  
>  /*
> @@ -2299,6 +2304,7 @@ static struct omap_hwmod omap44xx_prm_hwmod = {
>  	.class		= &omap44xx_prcm_hwmod_class,
>  	.rst_lines	= omap44xx_prm_resets,
>  	.rst_lines_cnt	= ARRAY_SIZE(omap44xx_prm_resets),
> +	.flags		= HWMOD_CUSTOM_HARDRESET,
>  };
>  
>  /*
> 

[toc] | [prev] | [next] | [standalone]

#1329968 — Re: [PATCH v3 3/3] pci: dra7xx: use pdata callbacks to perform reset

On Mon, 8 Feb 2016, Suman Anna wrote:

> On 02/07/2016 08:48 PM, Paul Walmsley wrote:
> > On Tue, 2 Feb 2016, Kishon Vijay Abraham I wrote:
> > 
> >> Paul, what do you think is the best way forward to perform reset?
> > 
> > Many of the IP blocks with PRM hardreset lines are processor IP blocks. 
> > Those often need special reset handling to ensure that WFI/HLT-like 
> > instructions are executed after reset.  This special handling ensures that 
> > the IP blocks' bus initiator interfaces indicate that they are in standby 
> > to the PRCM - thus allowing power management for the rest of the chip to 
> > work correctly.
> > 
> > But that doesn't seem to be the case with PCIe - and maybe others - 
> > possibly some of the MMUs?  
> 
> Yeah, the sequencing between clocks and resets would still be the same
> for MMUs, so, adding the custom flags for MMUs is fine.

I'm curious as to whether HWMOD_CUSTOM_HARDRESET is needed for the MMUs.  
We've stated that the main point of the custom hardreset code is to handle 
processors that need to be placed into WFI/HLT, but it doesn't seem like 
there would be an equivalent for MMUs.  Thoughts?


- Paul

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web