| Message ID | alpine.DEB.2.02.1602080208070.2182@utopia.booyaka.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, > }; > > /* >
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
Hi Paul, On 02/09/2016 02:49 AM, Paul Walmsley wrote: > 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? The current OMAP IOMMU code already leverages the pdata ops for performing the resets, so not adding the flags would also require additional changes in the driver. Also, the reset lines controlling the MMUs actually also manage the reset for all the other sub-modules other than the processor cores within the sub-systems. We have currently different issues (see [1] for eg. around the IPU sub-system entering RET in between), so from a PM point of view, we do prefer to place the MMUs also in reset when we are runtime suspended. regards Suman [1] http://git.ti.com/gitweb/?p=rpmsg/rpmsg.git;a=commit;h=a7db749a8a0fdfe7baa185db9f5071789a889061
Hi Suman On Tue, 9 Feb 2016, Suman Anna wrote: > On 02/09/2016 02:49 AM, Paul Walmsley wrote: > > 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? > > The current OMAP IOMMU code already leverages the pdata ops for > performing the resets, so not adding the flags would also require > additional changes in the driver. > > Also, the reset lines controlling the MMUs actually also manage the > reset for all the other sub-modules other than the processor cores > within the sub-systems. We have currently different issues (see [1] for > eg. around the IPU sub-system entering RET in between), so from a PM > point of view, we do prefer to place the MMUs also in reset when we are > runtime suspended. Should we reassert hardreset in _idle() for IP blocks that don't have HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this mechanism for the uncore hardreset lines, or are there other quirks? Also - would that address the potential issue that you mentioned with the PCIe block, or is that a different issue? - Paul
Hi Paul, On 02/09/2016 01:36 PM, Paul Walmsley wrote: > Hi Suman > > On Tue, 9 Feb 2016, Suman Anna wrote: > >> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>> 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? >> >> The current OMAP IOMMU code already leverages the pdata ops for >> performing the resets, so not adding the flags would also require >> additional changes in the driver. >> >> Also, the reset lines controlling the MMUs actually also manage the >> reset for all the other sub-modules other than the processor cores >> within the sub-systems. We have currently different issues (see [1] for >> eg. around the IPU sub-system entering RET in between), so from a PM >> point of view, we do prefer to place the MMUs also in reset when we are >> runtime suspended. > > Should we reassert hardreset in _idle() for IP blocks that don't have > HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this > mechanism for the uncore hardreset lines, or are there other quirks? > > Also - would that address the potential issue that you mentioned with the > PCIe block, or is that a different issue? Yeah, I think that would address the PCIe block issue in terms of reset state balancing between pm_runtime_get_sync() and pm_runtime_put() calls. Right now, they are unbalanced. The PCIe block is using these only in probe and remove, so it should work for that IP. The IPUs and DSPs in general would also place the reset lines asserted when suspended, as the power up sequence almost always involves releasing a reset line with the boot-up code on the processor detecting that it is a power restore boot. regards Suman
On Wednesday 10 February 2016 01:06 AM, Paul Walmsley wrote: > Hi Suman > > On Tue, 9 Feb 2016, Suman Anna wrote: > >> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>> 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? >> >> The current OMAP IOMMU code already leverages the pdata ops for >> performing the resets, so not adding the flags would also require >> additional changes in the driver. >> >> Also, the reset lines controlling the MMUs actually also manage the >> reset for all the other sub-modules other than the processor cores >> within the sub-systems. We have currently different issues (see [1] for >> eg. around the IPU sub-system entering RET in between), so from a PM >> point of view, we do prefer to place the MMUs also in reset when we are >> runtime suspended. > > Should we reassert hardreset in _idle() for IP blocks that don't have > HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this > mechanism for the uncore hardreset lines, or are there other quirks? IIUC _idle() will be called on pm_runtime_put. That would mean we perform reset on every suspend/resume cycle which is not desired. (suspend/resume support for PCIe is not yet in mainline but once we have it runtime_put and runtime_get will be invoked during suspend/resume cycle). Let me know if my understanding is wrong. Thanks Kishon > > Also - would that address the potential issue that you mentioned with the > PCIe block, or is that a different issue? > > > - Paul >
Hi, On Wednesday 10 February 2016 07:12 AM, Suman Anna wrote: > Hi Paul, > > On 02/09/2016 01:36 PM, Paul Walmsley wrote: >> Hi Suman >> >> On Tue, 9 Feb 2016, Suman Anna wrote: >> >>> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>>> 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? >>> >>> The current OMAP IOMMU code already leverages the pdata ops for >>> performing the resets, so not adding the flags would also require >>> additional changes in the driver. >>> >>> Also, the reset lines controlling the MMUs actually also manage the >>> reset for all the other sub-modules other than the processor cores >>> within the sub-systems. We have currently different issues (see [1] for >>> eg. around the IPU sub-system entering RET in between), so from a PM >>> point of view, we do prefer to place the MMUs also in reset when we are >>> runtime suspended. >> >> Should we reassert hardreset in _idle() for IP blocks that don't have >> HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this >> mechanism for the uncore hardreset lines, or are there other quirks? >> >> Also - would that address the potential issue that you mentioned with the >> PCIe block, or is that a different issue? > > Yeah, I think that would address the PCIe block issue in terms of reset > state balancing between pm_runtime_get_sync() and pm_runtime_put() > calls. Right now, they are unbalanced. The PCIe block is using these > only in probe and remove, so it should work for that IP. As I mentioned before this would result in undesired behavior during suspend/resume cycle in PCIe. (This should be okay for the current mainline code but would break once we add suspend/resume support for PCIe). Thanks Kishon
Hi Kishon, Suman, On Wed, 10 Feb 2016, Kishon Vijay Abraham I wrote: > On Wednesday 10 February 2016 07:12 AM, Suman Anna wrote: > > On 02/09/2016 01:36 PM, Paul Walmsley wrote: > >> On Tue, 9 Feb 2016, Suman Anna wrote: > >>> On 02/09/2016 02:49 AM, Paul Walmsley wrote: > >>>> 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? > >>> > >>> The current OMAP IOMMU code already leverages the pdata ops for > >>> performing the resets, so not adding the flags would also require > >>> additional changes in the driver. > >>> > >>> Also, the reset lines controlling the MMUs actually also manage the > >>> reset for all the other sub-modules other than the processor cores > >>> within the sub-systems. We have currently different issues (see [1] for > >>> eg. around the IPU sub-system entering RET in between), so from a PM > >>> point of view, we do prefer to place the MMUs also in reset when we are > >>> runtime suspended. > >> > >> Should we reassert hardreset in _idle() for IP blocks that don't have > >> HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this > >> mechanism for the uncore hardreset lines, or are there other quirks? > >> > >> Also - would that address the potential issue that you mentioned with the > >> PCIe block, or is that a different issue? > > > > Yeah, I think that would address the PCIe block issue in terms of reset > > state balancing between pm_runtime_get_sync() and pm_runtime_put() > > calls. Right now, they are unbalanced. The PCIe block is using these > > only in probe and remove, so it should work for that IP. > > As I mentioned before this would result in undesired behavior during > suspend/resume cycle in PCIe. (This should be okay for the current mainline > code but would break once we add suspend/resume support for PCIe). I'd like to understand where we're currently at here. It looks like we're waiting for testing from Suman, and we're waiting for Kishon to try using the bind/unbind driver model hook to see if that wedges PCIe? Does this match your collective understanding of the status here? Thinking about the question of what to do about hardreset assertion in idle, if we need it, we could add a hwmod flag to control that mode. I would consider it a temporary workaround until we have the hwmod code moved into a bus driver and the bus driver/hwmod code can hook into the LDM .remove operation (and connect it to .shutdown, etc.) Suman/Kishon: is it your understanding that we could remove the existing hardreset control in the IOMMU drivers and the PCIe driver if we had these options in the hwmod code? Dave, any further comments here? - Paul
On 02/09/2016 11:38 PM, Kishon Vijay Abraham I wrote: > Hi, > > On Wednesday 10 February 2016 07:12 AM, Suman Anna wrote: >> Hi Paul, >> >> On 02/09/2016 01:36 PM, Paul Walmsley wrote: >>> Hi Suman >>> >>> On Tue, 9 Feb 2016, Suman Anna wrote: >>> >>>> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>>>> 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? >>>> >>>> The current OMAP IOMMU code already leverages the pdata ops for >>>> performing the resets, so not adding the flags would also require >>>> additional changes in the driver. >>>> >>>> Also, the reset lines controlling the MMUs actually also manage the >>>> reset for all the other sub-modules other than the processor cores >>>> within the sub-systems. We have currently different issues (see [1] for >>>> eg. around the IPU sub-system entering RET in between), so from a PM >>>> point of view, we do prefer to place the MMUs also in reset when we are >>>> runtime suspended. >>> >>> Should we reassert hardreset in _idle() for IP blocks that don't have >>> HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this >>> mechanism for the uncore hardreset lines, or are there other quirks? >>> >>> Also - would that address the potential issue that you mentioned with the >>> PCIe block, or is that a different issue? >> >> Yeah, I think that would address the PCIe block issue in terms of reset >> state balancing between pm_runtime_get_sync() and pm_runtime_put() >> calls. Right now, they are unbalanced. The PCIe block is using these >> only in probe and remove, so it should work for that IP. > > As I mentioned before this would result in undesired behavior during > suspend/resume cycle in PCIe. (This should be okay for the current mainline > code but would break once we add suspend/resume support for PCIe). Yeah, I was wondering if some peripheral would want only the clock to be controlled during _idle() and not reset. Even then for the PCIe case that you are talking about, going through a pm_runtime_get_sync(), pm_runtime_put_sync()/pm_runtime_put() deasserts the resets everytime _enable() is called. Right now, the code block has ignored the return value from the _hardreset_deassert(), but if you check it and bail out, then your get_sync() would start failing from the second invocation. Can you elaborate more on what kind of issues you will see on suspend/resume cycle with PCIe? Do note that _idle() gets called through _od_suspend_no_irq() in omap_device.c if your runtime status is not suspended. I had to manage the runtime status in the IPU/DSP suspend/resume code to deal with the reset (omap_device_assert_hardreset) and clock sequences in _idle()/omap_device_idle() regards Suman
On 02/11/2016 01:27 PM, Paul Walmsley wrote: > Hi Kishon, Suman, > > On Wed, 10 Feb 2016, Kishon Vijay Abraham I wrote: > >> On Wednesday 10 February 2016 07:12 AM, Suman Anna wrote: >>> On 02/09/2016 01:36 PM, Paul Walmsley wrote: >>>> On Tue, 9 Feb 2016, Suman Anna wrote: >>>>> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>>>>> 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? >>>>> >>>>> The current OMAP IOMMU code already leverages the pdata ops for >>>>> performing the resets, so not adding the flags would also require >>>>> additional changes in the driver. >>>>> >>>>> Also, the reset lines controlling the MMUs actually also manage the >>>>> reset for all the other sub-modules other than the processor cores >>>>> within the sub-systems. We have currently different issues (see [1] for >>>>> eg. around the IPU sub-system entering RET in between), so from a PM >>>>> point of view, we do prefer to place the MMUs also in reset when we are >>>>> runtime suspended. >>>> >>>> Should we reassert hardreset in _idle() for IP blocks that don't have >>>> HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this >>>> mechanism for the uncore hardreset lines, or are there other quirks? >>>> >>>> Also - would that address the potential issue that you mentioned with the >>>> PCIe block, or is that a different issue? >>> >>> Yeah, I think that would address the PCIe block issue in terms of reset >>> state balancing between pm_runtime_get_sync() and pm_runtime_put() >>> calls. Right now, they are unbalanced. The PCIe block is using these >>> only in probe and remove, so it should work for that IP. >> >> As I mentioned before this would result in undesired behavior during >> suspend/resume cycle in PCIe. (This should be okay for the current mainline >> code but would break once we add suspend/resume support for PCIe). > > I'd like to understand where we're currently at here. It looks like we're > waiting for testing from Suman, and we're waiting for Kishon to try using > the bind/unbind driver model hook to see if that wedges PCIe? Does this > match your collective understanding of the status here? Matches mine :) For MMUs and (out of tree) OMAP remoteprocs, my current sequence is omap_device_deassert_hardreset() followed by pm_runtime_get_sync() or omap_device_enable() during booting, and pm_runtime_put_sync() or omap_device_idle() followed by omap_device_assert_hardreset(). Atleast they are bunched together. So, the current code does _deassert_hardreset twice when invoking the pm_runtime_get_sync() in my driver since the check for _are_all_hardreset_lines_asserted(oh) would fail. > > Thinking about the question of what to do about hardreset assertion in > idle, if we need it, we could add a hwmod flag to control that mode. I > would consider it a temporary workaround until we have the hwmod code > moved into a bus driver and the bus driver/hwmod code can hook into the > LDM .remove operation (and connect it to .shutdown, etc.) Suman/Kishon: > is it your understanding that we could remove the existing hardreset > control in the IOMMU drivers and the PCIe driver if we had these options > in the hwmod code? For MMUs/processors, the position where we deassert the reset becomes important. It has to be after the clocks are enabled (which is why half of the _deassert_hardreset code looks like the code sequence in _enable()). regards Suman > > Dave, any further comments here?
Hi Suman, On Friday 12 February 2016 02:13 AM, Suman Anna wrote: > On 02/09/2016 11:38 PM, Kishon Vijay Abraham I wrote: >> Hi, >> >> On Wednesday 10 February 2016 07:12 AM, Suman Anna wrote: >>> Hi Paul, >>> >>> On 02/09/2016 01:36 PM, Paul Walmsley wrote: >>>> Hi Suman >>>> >>>> On Tue, 9 Feb 2016, Suman Anna wrote: >>>> >>>>> On 02/09/2016 02:49 AM, Paul Walmsley wrote: >>>>>> 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? >>>>> >>>>> The current OMAP IOMMU code already leverages the pdata ops for >>>>> performing the resets, so not adding the flags would also require >>>>> additional changes in the driver. >>>>> >>>>> Also, the reset lines controlling the MMUs actually also manage the >>>>> reset for all the other sub-modules other than the processor cores >>>>> within the sub-systems. We have currently different issues (see [1] for >>>>> eg. around the IPU sub-system entering RET in between), so from a PM >>>>> point of view, we do prefer to place the MMUs also in reset when we are >>>>> runtime suspended. >>>> >>>> Should we reassert hardreset in _idle() for IP blocks that don't have >>>> HWMOD_CUSTOM_HARDRESET set on them? Would that allow us to use this >>>> mechanism for the uncore hardreset lines, or are there other quirks? >>>> >>>> Also - would that address the potential issue that you mentioned with the >>>> PCIe block, or is that a different issue? >>> >>> Yeah, I think that would address the PCIe block issue in terms of reset >>> state balancing between pm_runtime_get_sync() and pm_runtime_put() >>> calls. Right now, they are unbalanced. The PCIe block is using these >>> only in probe and remove, so it should work for that IP. >> >> As I mentioned before this would result in undesired behavior during >> suspend/resume cycle in PCIe. (This should be okay for the current mainline >> code but would break once we add suspend/resume support for PCIe). > > Yeah, I was wondering if some peripheral would want only the clock to be > controlled during _idle() and not reset. Even then for the PCIe case > that you are talking about, going through a pm_runtime_get_sync(), > pm_runtime_put_sync()/pm_runtime_put() deasserts the resets everytime right. But it'll deassert a line which is already deasserted. So it actually doesn't do a reset again. > _enable() is called. Right now, the code block has ignored the return > value from the _hardreset_deassert(), but if you check it and bail out, > then your get_sync() would start failing from the second invocation. hmm.. yeah. > > Can you elaborate more on what kind of issues you will see on > suspend/resume cycle with PCIe? Do note that _idle() gets called through At this point there are other issues w.r.t suspend/resume in PCI-dra7xx but as such reset of the controller is not desired during suspend/resume cycle and it'll result in the register contents being reset (haven't tested it though). Thanks Kishon
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, }; /*
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(-)