* [RFC] Runtime PM on the RZ/A series is never going to work right @ 2017-01-23 19:58 Chris Brandt 2017-01-24 7:52 ` Geert Uytterhoeven 2017-01-24 7:53 ` Geert Uytterhoeven 0 siblings, 2 replies; 11+ messages in thread From: Chris Brandt @ 2017-01-23 19:58 UTC (permalink / raw) To: linux-renesas-soc Hello Renesas SoC people, One difference between R-Car/RZG vs RZA is that there is no status bits for the MSTP clocks. This means even though you enable a clock by clearing the module-stop bit, you're not really guaranteed that the peripheral block is ready to be used. For the most part, the register interface is accessible, but usage of the HW might not be fully ready. Now that clock enable/disable has been fixed for RZ/A1 and actually works, you can start to see things break. For RZ/A1 (r7s72100) you can see this for the I2C driver (i2c-riic.c) and SPI driver (spi-rspi.c). Both of these drivers disable the clock in between EVERY TRANSFER as a means of runtime pm. Since SDHI (tmio) needs to keep the clock running for the card detect logic, it does not implement runtime pm, so it's OK. The only way to really fix this for RZ/A1 is to put an arbitrary delay in the clk-mstp.c driver: spin_lock_irqsave(&group->lock, flags); value = cpg_mstp_read(group, group->smstpcr); if (enable) value &= ~bitmask; else value |= bitmask; cpg_mstp_write(group, value, group->smstpcr); spin_unlock_irqrestore(&group->lock, flags); + if (enable || !group->mstpsr) + udelay(100); + if (!enable || !group->mstpsr) return 0; Or, just remove runtime PM for RZ/A1. * i2c-riic.c: This driver is just for the RZ/A series, so just remove runtime pm completely * spi-rspi.c: Disable runtime pm just for RZ/A series parts Therefore, before I start firing off patches to remove runtime PM for RZ/A, does anyone have an opinion one way of the other???? Regards, Chris ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-23 19:58 [RFC] Runtime PM on the RZ/A series is never going to work right Chris Brandt @ 2017-01-24 7:52 ` Geert Uytterhoeven 2017-01-24 7:53 ` Geert Uytterhoeven 1 sibling, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-24 7:52 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Mon, Jan 23, 2017 at 8:58 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > One difference between R-Car/RZG vs RZA is that there is no status bits for the MSTP clocks. > This means even though you enable a clock by clearing the module-stop bit, you're not really guaranteed that the peripheral block is ready to be used. > > For the most part, the register interface is accessible, but usage of the HW might not be fully ready. > > Now that clock enable/disable has been fixed for RZ/A1 and actually works, you can start to see things break. > > For RZ/A1 (r7s72100) you can see this for the I2C driver (i2c-riic.c) and SPI driver (spi-rspi.c). > Both of these drivers disable the clock in between EVERY TRANSFER as a means of runtime pm. > > Since SDHI (tmio) needs to keep the clock running for the card detect logic, it does not implement runtime pm, so it's OK. > > > The only way to really fix this for RZ/A1 is to put an arbitrary delay in the clk-mstp.c driver: > > spin_lock_irqsave(&group->lock, flags); > > value = cpg_mstp_read(group, group->smstpcr); > if (enable) > value &= ~bitmask; > else > value |= bitmask; > cpg_mstp_write(group, value, group->smstpcr); > > spin_unlock_irqrestore(&group->lock, flags); > > + if (enable || !group->mstpsr) > + udelay(100); > + > if (!enable || !group->mstpsr) > return 0; > > > Or, just remove runtime PM for RZ/A1. > > * i2c-riic.c: This driver is just for the RZ/A series, so just remove runtime pm completely > * spi-rspi.c: Disable runtime pm just for RZ/A series parts > > > Therefore, before I start firing off patches to remove runtime PM for RZ/A, does anyone have an opinion one way of the other???? Please have a look at Section 55.3.5 ("Module Standby Function"), which I had never read myself, apparently... Seems like there is some kind of status register, the STBCR register itself: Canceling Module Standby Function: 1. Clear the MSTP bit to 0. 2. After that, dummy-read the same register. Does it help if you add the dummy read when enabling the clock? However, that section reveals another complicating factor for some modules: if the module has a bit in a STBREQn/STBACKn register, a module standy request must be sent to the module before stopping the module clock. Looks like you're gonna need a whole new RZ/A1-specific clock driver to handle all these details... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-23 19:58 [RFC] Runtime PM on the RZ/A series is never going to work right Chris Brandt 2017-01-24 7:52 ` Geert Uytterhoeven @ 2017-01-24 7:53 ` Geert Uytterhoeven 2017-01-24 14:20 ` Chris Brandt 1 sibling, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-24 7:53 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Mon, Jan 23, 2017 at 8:58 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > One difference between R-Car/RZG vs RZA is that there is no status bits for the MSTP clocks. > This means even though you enable a clock by clearing the module-stop bit, you're not really guaranteed that the peripheral block is ready to be used. > > For the most part, the register interface is accessible, but usage of the HW might not be fully ready. > > Now that clock enable/disable has been fixed for RZ/A1 and actually works, you can start to see things break. > > For RZ/A1 (r7s72100) you can see this for the I2C driver (i2c-riic.c) and SPI driver (spi-rspi.c). > Both of these drivers disable the clock in between EVERY TRANSFER as a means of runtime pm. > > Since SDHI (tmio) needs to keep the clock running for the card detect logic, it does not implement runtime pm, so it's OK. > > > The only way to really fix this for RZ/A1 is to put an arbitrary delay in the clk-mstp.c driver: > > spin_lock_irqsave(&group->lock, flags); > > value = cpg_mstp_read(group, group->smstpcr); > if (enable) > value &= ~bitmask; > else > value |= bitmask; > cpg_mstp_write(group, value, group->smstpcr); > > spin_unlock_irqrestore(&group->lock, flags); > > + if (enable || !group->mstpsr) > + udelay(100); > + > if (!enable || !group->mstpsr) > return 0; > > > Or, just remove runtime PM for RZ/A1. > > * i2c-riic.c: This driver is just for the RZ/A series, so just remove runtime pm completely > * spi-rspi.c: Disable runtime pm just for RZ/A series parts > > > Therefore, before I start firing off patches to remove runtime PM for RZ/A, does anyone have an opinion one way of the other???? Please have a look at Section 55.3.5 ("Module Standby Function"), which I had never read myself, apparently... Seem like there is some kind of status register, the STBCR register itself: Canceling Module Standby Function: 1. Clear the MSTP bit to 0. 2. After that, dummy-read the same register. Does it help if you add the dummy read when enabling the clock? However, that section reveals another complicating factor for some modules: if the module has a bit in a STBREQn/STBACKn register, a module standy request must be sent to the module before stopping the module clock. Looks like you're gonna need a whole new RZ/A1-specific clock driver to handle all those details... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-24 7:53 ` Geert Uytterhoeven @ 2017-01-24 14:20 ` Chris Brandt 2017-01-24 14:40 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Chris Brandt @ 2017-01-24 14:20 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc Hi Geert, On Tuesday, January 24, 2017, Geert Uytterhoeven wrote: > > Therefore, before I start firing off patches to remove runtime PM for > RZ/A, does anyone have an opinion one way of the other???? > > Please have a look at Section 55.3.5 ("Module Standby Function"), which I > had never read myself, apparently... > > Seem like there is some kind of status register, the STBCR register > itself: > > Canceling Module Standby Function: > 1. Clear the MSTP bit to 0. > 2. After that, dummy-read the same register. > > Does it help if you add the dummy read when enabling the clock? Already tried that. And put a barrier() call just to make sure everything was done. It seems that might be enough for the RSPI, but the RIIC still has issues. From what I can tell, that makes the register space readable...but the IP block is not fully functional unless you delay a little. > However, that section reveals another complicating factor for some > modules: > if the module has a bit in a STBREQn/STBACKn register, a module standy > request must be sent to the module before stopping the module clock. > > Looks like you're gonna need a whole new RZ/A1-specific clock driver to > handle all those details... There are only a couple IP blocks called out in the STBREQn/STBACKn registers, and I think they are not really crucial for runtime pm (Ethenret, LCD, Coresight, etc..). So in my opinion it's not worth a new driver just yet. But, I think I'd like to disable runtime pm for RZ/A1 in the drivers for now Because 'functional' is better than 'lower-power-but-broken' Chris ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-24 14:20 ` Chris Brandt @ 2017-01-24 14:40 ` Geert Uytterhoeven 2017-01-24 16:22 ` Chris Brandt 0 siblings, 1 reply; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-24 14:40 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Tue, Jan 24, 2017 at 3:20 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tuesday, January 24, 2017, Geert Uytterhoeven wrote: >> > Therefore, before I start firing off patches to remove runtime PM for >> RZ/A, does anyone have an opinion one way of the other???? >> >> Please have a look at Section 55.3.5 ("Module Standby Function"), which I >> had never read myself, apparently... >> >> Seem like there is some kind of status register, the STBCR register >> itself: >> >> Canceling Module Standby Function: >> 1. Clear the MSTP bit to 0. >> 2. After that, dummy-read the same register. >> >> Does it help if you add the dummy read when enabling the clock? > > Already tried that. And put a barrier() call just to make sure > everything was done. > It seems that might be enough for the RSPI, but the RIIC still has issues. :-( > From what I can tell, that makes the register space readable...but the IP block > is not fully functional unless you delay a little. If you know the minimum delay needed, and it's not too long, it can be added to the enable path. >> However, that section reveals another complicating factor for some >> modules: >> if the module has a bit in a STBREQn/STBACKn register, a module standy >> request must be sent to the module before stopping the module clock. >> >> Looks like you're gonna need a whole new RZ/A1-specific clock driver to >> handle all those details... > > There are only a couple IP blocks called out in the STBREQn/STBACKn registers, > and I think they are not really crucial for runtime pm (Ethenret, LCD, > Coresight, etc..). > So in my opinion it's not worth a new driver just yet. OK. > But, I think I'd like to disable runtime pm for RZ/A1 in the drivers for now > Because 'functional' is better than 'lower-power-but-broken' I prefer not doing that in the individual drivers, as they're shared with other SoCs. I think you can handle that in drivers/clk/renesas/clk-mstp.c: - in cpg_mstp_attach_dev(), add a call to pm_clk_resume(dev) after the call to pm_clk_add_clk(), - in cpg_mstp_detach_dev(), add a call to pm_clk_suspend(dev) before the call to pm_clk_destroy(). Yes, that means the module clocks are enabled all the time. Of course when running on RZ/A1H ;-) Good luck! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-24 14:40 ` Geert Uytterhoeven @ 2017-01-24 16:22 ` Chris Brandt 2017-01-25 8:53 ` Geert Uytterhoeven 0 siblings, 1 reply; 11+ messages in thread From: Chris Brandt @ 2017-01-24 16:22 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc Hi Geert, On Tuesday, January 24, 2017, Geert Uytterhoeven wrote: > > From what I can tell, that makes the register space readable...but the > > IP block is not fully functional unless you delay a little. > > If you know the minimum delay needed, and it's not too long, it can be > added to the enable path. I can play around and see. I know udealy(100) works OK, but then I have to have a delay that's as long as the slowest peripheral. If it was just to turn a clock on once, or once in a while, that's OK. But it seems like runtime pm wants to turn the clocks on/off as much as possible. > > But, I think I'd like to disable runtime pm for RZ/A1 in the drivers > > for now Because 'functional' is better than 'lower-power-but-broken' > > I prefer not doing that in the individual drivers, as they're shared with > other SoCs. What I meant was looking at the compatible string and only doing it for RZ/A1. For example, in rspi_probe(): if (of_device_is_compatible(np, " renesas,rspi-r7s72100")) master->auto_runtime_pm = false; else master->auto_runtime_pm = true; I would do the same kind of thing for riic. > I think you can handle that in drivers/clk/renesas/clk-mstp.c: > - in cpg_mstp_attach_dev(), add a call to pm_clk_resume(dev) after the > call to pm_clk_add_clk(), > - in cpg_mstp_detach_dev(), add a call to pm_clk_suspend(dev) before the > call to pm_clk_destroy(). > Yes, that means the module clocks are enabled all the time. > Of course when running on RZ/A1H ;-) That might be OK. But won't the individual drivers still want to keep turning clocks on and off manually? (unless I'm not understanding that the underlying clock routines will basically just ignore everything). But even if that' the case...that just wasted CPU cycles (remember, I'm only working with a 400MHz single core here running XIP_KERNEL) I think I should at least put the dummy read in cpg_mstp_clock_endisable() first, then maybe I can see what drivers work. Something like this: spin_lock_irqsave(&group->lock, flags); value = cpg_mstp_read(group, group->smstpcr); if (enable) value &= ~bitmask; else value |= bitmask; cpg_mstp_write(group, value, group->smstpcr); + if (!group->mstpsr) { + /* dummy read to ensure write has completed */ + clk_readl(group->smstpcr); + barrier_data(group->smstpcr); + } spin_unlock_irqrestore(&group->lock, flags); Chris ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-24 16:22 ` Chris Brandt @ 2017-01-25 8:53 ` Geert Uytterhoeven 2017-01-25 14:35 ` Chris Brandt 2017-01-27 3:05 ` Chris Brandt 0 siblings, 2 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-25 8:53 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Tue, Jan 24, 2017 at 5:22 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Tuesday, January 24, 2017, Geert Uytterhoeven wrote: >> > From what I can tell, that makes the register space readable...but the >> > IP block is not fully functional unless you delay a little. >> >> If you know the minimum delay needed, and it's not too long, it can be >> added to the enable path. > > I can play around and see. I know udealy(100) works OK, but then I have > to have a delay that's as long as the slowest peripheral. > If it was just to turn a clock on once, or once in a while, that's OK. But > it seems like runtime pm wants to turn the clocks on/off as much as > possible. That's not really true: depending on tuning and/or QoS parameters, pm_runtime_put() may anticipate future use, and may decide not turn off the clock immediately. It may be worth looking into that, and to see how to relax that behavior on RZ/A1. >> > But, I think I'd like to disable runtime pm for RZ/A1 in the drivers >> > for now Because 'functional' is better than 'lower-power-but-broken' >> >> I prefer not doing that in the individual drivers, as they're shared with >> other SoCs. > > What I meant was looking at the compatible string and only doing > it for RZ/A1. > > For example, in rspi_probe(): > > if (of_device_is_compatible(np, " renesas,rspi-r7s72100")) > master->auto_runtime_pm = false; > else > master->auto_runtime_pm = true; > > > I would do the same kind of thing for riic. That needs to be done in individual drivers, ugh... >> I think you can handle that in drivers/clk/renesas/clk-mstp.c: >> - in cpg_mstp_attach_dev(), add a call to pm_clk_resume(dev) after the >> call to pm_clk_add_clk(), >> - in cpg_mstp_detach_dev(), add a call to pm_clk_suspend(dev) before the >> call to pm_clk_destroy(). >> Yes, that means the module clocks are enabled all the time. >> Of course when running on RZ/A1H ;-) > > That might be OK. Forgot to mention: you should also no longer set GENPD_FLAG_PM_CLK in cpg_mstp_add_clk_domain(). The flag won't hurt, it will just cause extra code to be executed. > But won't the individual drivers still want to keep turning clocks on and off manually? > (unless I'm not understanding that the underlying clock routines will basically just > ignore everything). But even if that' the case...that just wasted CPU cycles (remember, > I'm only working with a 400MHz single core here running XIP_KERNEL) If a clock is already enabled, preparing and/or enabling it again will just increase the prepare resp. enable counters. Disabling/unpreparing afterwards will also just decrease the counters. Should be quite cheap. > I think I should at least put the dummy read in cpg_mstp_clock_endisable() first, > then maybe I can see what drivers work. Something like this: > > > spin_lock_irqsave(&group->lock, flags); > > value = cpg_mstp_read(group, group->smstpcr); > if (enable) > value &= ~bitmask; > else > value |= bitmask; > cpg_mstp_write(group, value, group->smstpcr); > > + if (!group->mstpsr) { > + /* dummy read to ensure write has completed */ > + clk_readl(group->smstpcr); > + barrier_data(group->smstpcr); > + } > > spin_unlock_irqrestore(&group->lock, flags); Yes, that's a good first step. The only other supported SoCs without[*] status registers are R-Car M1A and H1. I believe they should handle the additional reads fine. [*] On R-Car Gen1, some MSTP blocks have status registers, some don't. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-25 8:53 ` Geert Uytterhoeven @ 2017-01-25 14:35 ` Chris Brandt 2017-01-25 14:37 ` Geert Uytterhoeven 2017-01-27 3:05 ` Chris Brandt 1 sibling, 1 reply; 11+ messages in thread From: Chris Brandt @ 2017-01-25 14:35 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc Hi Geert, On Wednesday, January 25, 2017, Geert Uytterhoeven wrote: > > I can play around and see. I know udealy(100) works OK, but then I > > have to have a delay that's as long as the slowest peripheral. > > If it was just to turn a clock on once, or once in a while, that's OK. > > But it seems like runtime pm wants to turn the clocks on/off as much > > as possible. > > That's not really true: depending on tuning and/or QoS parameters, > pm_runtime_put() may anticipate future use, and may decide not turn off > the clock immediately. > It may be worth looking into that, and to see how to relax that behavior > on RZ/A1. Yes, if there is a way to relax things, then that would be better. > > But won't the individual drivers still want to keep turning clocks on > and off manually? > > (unless I'm not understanding that the underlying clock routines will > basically just > > ignore everything). But even if that' the case...that just wasted CPU > cycles (remember, > > I'm only working with a 400MHz single core here running XIP_KERNEL) > > If a clock is already enabled, preparing and/or enabling it again will > just > increase the prepare resp. enable counters. Disabling/unpreparing > afterwards > will also just decrease the counters. Should be quite cheap OK, I think I see your point: If I go and double-enable a clock, then the runtime pm won't do much of anything because I'll always be a count higher so a true 'clock disable' will never really occur. Is that correct? #I'm getting side tracked from what I really started to do which was test out PFC for i2c and spi :( Chris ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-25 14:35 ` Chris Brandt @ 2017-01-25 14:37 ` Geert Uytterhoeven 0 siblings, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-25 14:37 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Wed, Jan 25, 2017 at 3:35 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, January 25, 2017, Geert Uytterhoeven wrote: >> > But won't the individual drivers still want to keep turning clocks on >> and off manually? >> > (unless I'm not understanding that the underlying clock routines will >> basically just >> > ignore everything). But even if that' the case...that just wasted CPU >> cycles (remember, >> > I'm only working with a 400MHz single core here running XIP_KERNEL) >> >> If a clock is already enabled, preparing and/or enabling it again will >> just >> increase the prepare resp. enable counters. Disabling/unpreparing >> afterwards >> will also just decrease the counters. Should be quite cheap > > OK, I think I see your point: > > If I go and double-enable a clock, then the runtime pm won't do much > of anything because I'll always be a count higher so a true 'clock disable' > will never really occur. Is that correct? That's correct. > #I'm getting side tracked from what I really started to do which was test > out PFC for i2c and spi :( Welcome to the world of scratching your (increasing number of) itches ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-25 8:53 ` Geert Uytterhoeven 2017-01-25 14:35 ` Chris Brandt @ 2017-01-27 3:05 ` Chris Brandt 2017-01-27 7:51 ` Geert Uytterhoeven 1 sibling, 1 reply; 11+ messages in thread From: Chris Brandt @ 2017-01-27 3:05 UTC (permalink / raw) To: Geert Uytterhoeven; +Cc: linux-renesas-soc Hi Geert, On Wednesday, January 25, 2017, Geert Uytterhoeven wrote: > >> I think you can handle that in drivers/clk/renesas/clk-mstp.c: > >> - in cpg_mstp_attach_dev(), add a call to pm_clk_resume(dev) after > the > >> call to pm_clk_add_clk(), > >> - in cpg_mstp_detach_dev(), add a call to pm_clk_suspend(dev) before > the > >> call to pm_clk_destroy(). > >> Yes, that means the module clocks are enabled all the time. > >> Of course when running on RZ/A1H ;-) > > > > That might be OK. > > Forgot to mention: you should also no longer set GENPD_FLAG_PM_CLK in > cpg_mstp_add_clk_domain(). The flag won't hurt, it will just cause extra > code to be executed. So to be clear before I start hacking away, your suggestion here is to do this ONLY for RZ/A1 so I don't screw up any other SoCs, right? For example: int cpg_mstp_attach_dev() { ... error = pm_clk_add_clk(dev, clk); if (error) { dev_err(dev, "pm_clk_add_clk %pC failed %d\n", clk, error); goto fail_destroy; } + if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks")) + pm_clk_suspend(dev); ... } Then, this would be OK to submit until I find some other way to do stable runtime pm for RZ/A1? (which might not be possible...unless I put in the silly delay) A that point, any peripheral I use (status="okay") will stay on, but the ones I don't use will stay off, correct? Thank you, Chris ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC] Runtime PM on the RZ/A series is never going to work right 2017-01-27 3:05 ` Chris Brandt @ 2017-01-27 7:51 ` Geert Uytterhoeven 0 siblings, 0 replies; 11+ messages in thread From: Geert Uytterhoeven @ 2017-01-27 7:51 UTC (permalink / raw) To: Chris Brandt; +Cc: linux-renesas-soc Hi Chris, On Fri, Jan 27, 2017 at 4:05 AM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Wednesday, January 25, 2017, Geert Uytterhoeven wrote: >> >> I think you can handle that in drivers/clk/renesas/clk-mstp.c: >> >> - in cpg_mstp_attach_dev(), add a call to pm_clk_resume(dev) after >> the >> >> call to pm_clk_add_clk(), >> >> - in cpg_mstp_detach_dev(), add a call to pm_clk_suspend(dev) before >> the >> >> call to pm_clk_destroy(). >> >> Yes, that means the module clocks are enabled all the time. >> >> Of course when running on RZ/A1H ;-) >> > >> > That might be OK. >> >> Forgot to mention: you should also no longer set GENPD_FLAG_PM_CLK in >> cpg_mstp_add_clk_domain(). The flag won't hurt, it will just cause extra >> code to be executed. > > So to be clear before I start hacking away, your suggestion here is > to do this ONLY for RZ/A1 so I don't screw up any other SoCs, right? Correct. > For example: > > int cpg_mstp_attach_dev() > { > ... > > error = pm_clk_add_clk(dev, clk); > if (error) { > dev_err(dev, "pm_clk_add_clk %pC failed %d\n", clk, error); > goto fail_destroy; > } > > + if (of_device_is_compatible(np, "renesas,r7s72100-mstp-clocks")) > + pm_clk_suspend(dev); That should be pm_clk_resume(dev), as is this the attach function ;-) And please drop GENPD_FLAG_PM_CLK on RZ/A1, too. > ... > } > > Then, this would be OK to submit until I find some other way to do stable > runtime pm for RZ/A1? (which might not be possible...unless I put in the > silly delay) > > A that point, any peripheral I use (status="okay") will stay on, but the > ones I don't use will stay off, correct? Correct. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2017-01-27 7:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-01-23 19:58 [RFC] Runtime PM on the RZ/A series is never going to work right Chris Brandt 2017-01-24 7:52 ` Geert Uytterhoeven 2017-01-24 7:53 ` Geert Uytterhoeven 2017-01-24 14:20 ` Chris Brandt 2017-01-24 14:40 ` Geert Uytterhoeven 2017-01-24 16:22 ` Chris Brandt 2017-01-25 8:53 ` Geert Uytterhoeven 2017-01-25 14:35 ` Chris Brandt 2017-01-25 14:37 ` Geert Uytterhoeven 2017-01-27 3:05 ` Chris Brandt 2017-01-27 7:51 ` Geert Uytterhoeven
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.