| Message ID | 1452780672-14339-4-git-send-email-kishon@ti.com |
|---|---|
| State | New |
| Headers | show |
Hi Tony, On 01/27/2016 11:31 AM, Tony Lindgren wrote: > * Kishon Vijay Abraham I <kishon@ti.com> [160114 06:12]: >> Use assert/deassert callbacks populated in the platform data to >> to perform reset of PCIe. >> >> Use these callbacks until a reset controller driver is >> is available in the kernel to reset PCIe. > ... > >> --- a/drivers/pci/host/pci-dra7xx.c >> +++ b/drivers/pci/host/pci-dra7xx.c >> @@ -347,6 +404,10 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) >> enum of_gpio_flags flags; >> unsigned long gpio_flags; >> >> + ret = dra7xx_pcie_reset(pdev); >> + if (ret) >> + return ret; >> + >> dra7xx = devm_kzalloc(dev, sizeof(*dra7xx), GFP_KERNEL); >> if (!dra7xx) >> return -ENOMEM; > > With the hwmod data properly configured the reset already happens > for the device by the bus driver, the hwmod code in this case? > >> @@ -457,6 +518,7 @@ static int __exit dra7xx_pcie_remove(struct platform_device *pdev) >> struct pcie_port *pp = &dra7xx->pp; >> struct device *dev = &pdev->dev; >> int count = dra7xx->phy_count; >> + int ret; >> >> if (pp->irq_domain) >> irq_domain_remove(pp->irq_domain); >> @@ -467,6 +529,10 @@ static int __exit dra7xx_pcie_remove(struct platform_device *pdev) >> phy_exit(dra7xx->phy[count]); >> } >> >> + ret = dra7xx_pcie_assert_reset(pdev); >> + if (ret < 0) >> + return ret; >> + >> return 0; >> } > > 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. > 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. regards Suman
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
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 >
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, > }; > > /* >
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
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
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
Kishon, On 02/12/2016 12:49 AM, Kishon Vijay Abraham I wrote: > Hi, > > On Friday 12 February 2016 12:57 AM, 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? > > I got to try this and looks like even without this series there are other PM > issues possible introduced by Commit 5de85b9d57ab ("PM / runtime: Re-init > runtime PM states at probe error and driver unbind"). > > Now I get this error if I tried to modprobe after rmmod pci-dra7xx. > [ 54.352860] dra7-pcie 51000000.pcie: omap_device: omap_device_enable() > called from invalid state 1 > [ 54.362318] dra7-pcie 51000000.pcie: pm_runtime_get_sync failed > [ 54.368624] dra7-pcie: probe of 51000000.pcie failed with error -22 > > From the thread that fixes this issue [1], looks like drivers that use > *_autosuspend() get this issue. However I don't use *_autosuspend() in > pci-dra7xx. Maybe pci core has this? This has to be debugged further. But I > feel this is not related to the problem that we are trying to solve right now > (dra7 hangs if PCI driver is enabled) and given the fact that pci-dra7xx driver > is now modeled as built-in driver, this can be deferred. > > [1] -> http://www.spinics.net/lists/arm-kernel/msg481845.html > >> >> 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? > > Yeah, that's my understanding. And since this series solves the PCIe problem, > it's proven that hardreset control can be moved to hwmod code. > > For PCIe, it's even okay to do deassert in _reset, but I'm not sure if it'll > have side effects with other modules. > > diff --git a/arch/arm/mach-omap2/omap_hwmod.c b/arch/arm/mach-omap2/omap_hwmod.c > index e9f65fe..24cafd9 100644 > --- a/arch/arm/mach-omap2/omap_hwmod.c > +++ b/arch/arm/mach-omap2/omap_hwmod.c > @@ -1966,8 +1966,11 @@ static int _reset(struct omap_hwmod *oh) > r = oh->class->reset(oh); > } else { > if (oh->rst_lines_cnt > 0) { > - for (i = 0; i < oh->rst_lines_cnt; i++) > + for (i = 0; i < oh->rst_lines_cnt; i++) { > _assert_hardreset(oh, oh->rst_lines[i].name); > + if (!(oh->flags & HWMOD_CUSTOM_HARDRESET)) > + _deassert_hardreset(oh, oh->rst_lines[i].name); > + } Better yet, just add this specific _deassert_hardreset logic to a DRA7 PCIe-specific class->reset function. You won't need adding the HWMOD_CUSTOM_HARDRESET flags either and will satisfy your suspend/resume dilemma, and it won't affect other paths. If that can work for you, that would be simplest patch for this -rc cycle. > return 0; > } else { > r = _ocp_softreset(oh); > > Thanks > Kishon > > P.S. I'll be on vacation till end of next week with no email access till then. > So email response will be delayed. Sorry about that. Sekhar, Will you be following up with above suggestion since Kishon is gonna be out? regards Suman >> >> Dave, any further comments here? >> >> >> - Paul >>
diff --git a/drivers/pci/host/pci-dra7xx.c b/drivers/pci/host/pci-dra7xx.c index 8c36880..f9a3240 100644 --- a/drivers/pci/host/pci-dra7xx.c +++ b/drivers/pci/host/pci-dra7xx.c @@ -25,6 +25,8 @@ #include <linux/resource.h> #include <linux/types.h> +#include <linux/platform_data/pci-dra7xx.h> + #include "pcie-designware.h" /* PCIe controller wrapper DRA7XX configuration registers */ @@ -329,6 +331,61 @@ static int __init dra7xx_add_pcie_port(struct dra7xx_pcie *dra7xx, return 0; } +static int dra7xx_pcie_assert_reset(struct platform_device *pdev) +{ + int ret; + struct device *dev = &pdev->dev; + struct pci_dra7xx_platform_data *pdata = pdev->dev.platform_data; + + if (!(pdata && pdata->assert_reset)) { + dev_err(dev, "platform data for assert reset not found!\n"); + return -EINVAL; + } + + ret = pdata->assert_reset(pdev, pdata->reset_name); + if (ret) { + dev_err(dev, "assert_reset failed: %d\n", ret); + return ret; + } + + return 0; +} + +static int dra7xx_pcie_deassert_reset(struct platform_device *pdev) +{ + int ret; + struct device *dev = &pdev->dev; + struct pci_dra7xx_platform_data *pdata = pdev->dev.platform_data; + + if (!(pdata && pdata->deassert_reset)) { + dev_err(dev, "platform data for deassert reset not found!\n"); + return -EINVAL; + } + + ret = pdata->deassert_reset(pdev, pdata->reset_name); + if (ret) { + dev_err(dev, "deassert_reset failed: %d\n", ret); + return ret; + } + + return 0; +} + +static int dra7xx_pcie_reset(struct platform_device *pdev) +{ + int ret; + + ret = dra7xx_pcie_assert_reset(pdev); + if (ret < 0) + return ret; + + ret = dra7xx_pcie_deassert_reset(pdev); + if (ret < 0) + return ret; + + return 0; +} + static int __init dra7xx_pcie_probe(struct platform_device *pdev) { u32 reg; @@ -347,6 +404,10 @@ static int __init dra7xx_pcie_probe(struct platform_device *pdev) enum of_gpio_flags flags; unsigned long gpio_flags; + ret = dra7xx_pcie_reset(pdev); + if (ret) + return ret; + dra7xx = devm_kzalloc(dev, sizeof(*dra7xx), GFP_KERNEL); if (!dra7xx) return -ENOMEM; @@ -457,6 +518,7 @@ static int __exit dra7xx_pcie_remove(struct platform_device *pdev) struct pcie_port *pp = &dra7xx->pp; struct device *dev = &pdev->dev; int count = dra7xx->phy_count; + int ret; if (pp->irq_domain) irq_domain_remove(pp->irq_domain); @@ -467,6 +529,10 @@ static int __exit dra7xx_pcie_remove(struct platform_device *pdev) phy_exit(dra7xx->phy[count]); } + ret = dra7xx_pcie_assert_reset(pdev); + if (ret < 0) + return ret; + return 0; }