[RFC] Runtime PM on the RZ/A series is never going to work right

[RFC] Runtime PM on the RZ/A series is never going to work right

[Chris Brandt]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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

RE: [RFC] Runtime PM on the RZ/A series is never going to work right

[Chris Brandt]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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

RE: [RFC] Runtime PM on the RZ/A series is never going to work right

[Chris Brandt]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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

RE: [RFC] Runtime PM on the RZ/A series is never going to work right

[Chris Brandt]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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

RE: [RFC] Runtime PM on the RZ/A series is never going to work right

[Chris Brandt]

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

Re: [RFC] Runtime PM on the RZ/A series is never going to work right

[Geert Uytterhoeven]

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