From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> To: "Yang, Wenyou" <Wenyou.Yang@atmel.com> Cc: "Ferre, Nicolas" <Nicolas.FERRE@atmel.com>, "linux@arm.linux.org.uk" <linux@arm.linux.org.uk>, "linux-arm-kernel@lists.infradead.org" <linux-arm-kernel@lists.infradead.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "alexandre.belloni@free-electrons.com" <alexandre.belloni@free-electrons.com>, "sylvain.rochet@finsecur.com" <sylvain.rochet@finsecur.com>, "peda@axentia.se" <peda@axentia.se>, "Vilchez, Patrice" <Patrice.VILCHEZ@atmel.com> Subject: Re: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 Date: Fri, 30 Jan 2015 10:17:09 +0000 [thread overview] Message-ID: <20150130101709.GB8787@red-moon> (raw) In-Reply-To: <B256D81BAE5131468A838E5D7A243641BFD3A0C2@penmbx01> On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote: [...] > > > > > + * Put the processor to enter the WFI state */ > > > > > + .macro _do_wfi > > > > > > > > You will have to explain why you need this, really. > > > I don't understand your meaning. > > > > I want to understand why this assembly snippet (that can be rewritten in C BTW): > > > > /* Disable the processor's clock */ > > mov tmp1, #AT91_PMC_PCK > > str tmp1, [pmc, #AT91_PMC_SCDR] > > > > + > > > > cpu_do_idle() > > > > is not sufficient for you, or put it differently, why do you need this macro. > This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state. > So, it can't invoke other functions generally. Ok, thanks for explaining, I will have a look again to check where you use the macro to verify the code flow. > > > > > > > > > > > > > > > + > > > > > +#if defined(CONFIG_CPU_V7) > > > > > + /* > > > > > + * Execute an ISB instruction to flush the pipeline to ensure > > > > > + * that all of operations have beem completed. > > > > > > > > s/beem/been > > > Thanks. > > > > > > > > > > > > + */ > > > > > + isb > > > > This isb should not be there, unless you know a reason why it should and you > > explain it to me. > I encountered system lock during verifying the pm function. > Anyway, I will tested again whether it works after removing it. See above, I have to check when you switch to SRAM execution to see if an isb is appropriate there, it is not self-explanatory. Thank you, Lorenzo > > > > > > > > + > > > > > + /* > > > > > + * Execute an ISB instruction to ensure that all of the > > > > > + * CP15 register changes have been committed. > > > > > + */ > > > > > + dsb > > > > > > > > This is a dsb not an isb. > > > Changed in the v2.0 > > > > Yes, but I still do not understand why you want to execute it before disabling the > > clocks (I really hope that by "disabling the clocks" you mean "set the power > > controller to a state when, on wfi execution, the clocks are gated"). > Are you meaning to execute dsb and wfi after disabling the clocks? > > > > > > > > > > > > > > > + dmb > > > > > > > > You have to explain why you need every single one of these barriers, > > > > otherwise I am NAKing this patch. > > > No need this one? > > > > No, remove it. > OK, thanks > > > > > > > > > > > > > > > + > > > > > + /* Disable the processor's clock */ > > > > > + mov tmp1, #AT91_PMC_PCK > > > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > > > > + > > > > > + /* Execute a WFI instruction */ > > > > > + wfi @ Wait For Interrupt > > > > > > > > This one looks ok :) > > > > > > > > > + > > > > > + /* > > > > > + * CPU can specualatively prefetch the instructions > > > > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > > > > > > > So what ? I suspect your issue is related to wfi completion on > > > > pending IRQ. I would like to know the details that describe the > > > > issue you are trying to solve here please. > > > Honestly, I referred to others, I will dig more, and test it. > > > > You should not copy and paste code, because: > > > > 1) it might be broken > > 2) and/or unoptimized > > 3) and/or does not apply to your platform > > > > See my suggestion above, if it does not work for you, you will report the issue and > > we will take it from there. > OK, thanks. > > > > > Thanks, > > Lorenzo > > > > > > > > > > > > > > + */ > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > +#else > > > > > + mcr p15, 0, tmp1, c7, c0, 4 > > > > > +#endif > > > > > > > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > > > > > > > Thanks, > > > > Lorenzo > > > > > > > > > + > > > > > + .endm > > > > > + > > > > > .text > > > > > > > > > > /* > > > > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > > > > > > > skip_disable_main_clock: > > > > > /* Wait for interrupt */ > > > > > - mcr p15, 0, tmp1, c7, c0, 4 > > > > > + _do_wfi > > > > > > > > > > tst mode, #AT91_PM_SLOW_CLOCK > > > > > beq skip_enable_main_clock > > > > > -- > > > > > 1.7.9.5 > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe > > > > > linux-kernel" in the body of a message to > > > > > majordomo@vger.kernel.org More majordomo info at > > > > > http://vger.kernel.org/majordomo-info.html > > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > > > > Best Regards, > > > Wenyou Yang > > > > > Best Regards, > Wenyou Yang >
WARNING: multiple messages have this Message-ID (diff)
From: lorenzo.pieralisi@arm.com (Lorenzo Pieralisi) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 Date: Fri, 30 Jan 2015 10:17:09 +0000 [thread overview] Message-ID: <20150130101709.GB8787@red-moon> (raw) In-Reply-To: <B256D81BAE5131468A838E5D7A243641BFD3A0C2@penmbx01> On Fri, Jan 30, 2015 at 07:23:21AM +0000, Yang, Wenyou wrote: [...] > > > > > + * Put the processor to enter the WFI state */ > > > > > + .macro _do_wfi > > > > > > > > You will have to explain why you need this, really. > > > I don't understand your meaning. > > > > I want to understand why this assembly snippet (that can be rewritten in C BTW): > > > > /* Disable the processor's clock */ > > mov tmp1, #AT91_PMC_PCK > > str tmp1, [pmc, #AT91_PMC_SCDR] > > > > + > > > > cpu_do_idle() > > > > is not sufficient for you, or put it differently, why do you need this macro. > This assembly snippet will be copied and run in the SRAM, in the period the DDR is in the self-refresh state. > So, it can't invoke other functions generally. Ok, thanks for explaining, I will have a look again to check where you use the macro to verify the code flow. > > > > > > > > > > > > > > > + > > > > > +#if defined(CONFIG_CPU_V7) > > > > > + /* > > > > > + * Execute an ISB instruction to flush the pipeline to ensure > > > > > + * that all of operations have beem completed. > > > > > > > > s/beem/been > > > Thanks. > > > > > > > > > > > > + */ > > > > > + isb > > > > This isb should not be there, unless you know a reason why it should and you > > explain it to me. > I encountered system lock during verifying the pm function. > Anyway, I will tested again whether it works after removing it. See above, I have to check when you switch to SRAM execution to see if an isb is appropriate there, it is not self-explanatory. Thank you, Lorenzo > > > > > > > > + > > > > > + /* > > > > > + * Execute an ISB instruction to ensure that all of the > > > > > + * CP15 register changes have been committed. > > > > > + */ > > > > > + dsb > > > > > > > > This is a dsb not an isb. > > > Changed in the v2.0 > > > > Yes, but I still do not understand why you want to execute it before disabling the > > clocks (I really hope that by "disabling the clocks" you mean "set the power > > controller to a state when, on wfi execution, the clocks are gated"). > Are you meaning to execute dsb and wfi after disabling the clocks? > > > > > > > > > > > > > > > + dmb > > > > > > > > You have to explain why you need every single one of these barriers, > > > > otherwise I am NAKing this patch. > > > No need this one? > > > > No, remove it. > OK, thanks > > > > > > > > > > > > > > > + > > > > > + /* Disable the processor's clock */ > > > > > + mov tmp1, #AT91_PMC_PCK > > > > > + str tmp1, [pmc, #AT91_PMC_SCDR] > > > > > + > > > > > + /* Execute a WFI instruction */ > > > > > + wfi @ Wait For Interrupt > > > > > > > > This one looks ok :) > > > > > > > > > + > > > > > + /* > > > > > + * CPU can specualatively prefetch the instructions > > > > > + * so add NOPs after WFI. Sixteen NOPs as Cortex-A5 pipeline. > > > > > > > > So what ? I suspect your issue is related to wfi completion on > > > > pending IRQ. I would like to know the details that describe the > > > > issue you are trying to solve here please. > > > Honestly, I referred to others, I will dig more, and test it. > > > > You should not copy and paste code, because: > > > > 1) it might be broken > > 2) and/or unoptimized > > 3) and/or does not apply to your platform > > > > See my suggestion above, if it does not work for you, you will report the issue and > > we will take it from there. > OK, thanks. > > > > > Thanks, > > Lorenzo > > > > > > > > > > > > > > + */ > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > + nop > > > > > +#else > > > > > + mcr p15, 0, tmp1, c7, c0, 4 > > > > > +#endif > > > > > > > > Tell us what's the problem you have to solve, first, then we will see how to fix it. > > > > > > > > Thanks, > > > > Lorenzo > > > > > > > > > + > > > > > + .endm > > > > > + > > > > > .text > > > > > > > > > > /* > > > > > @@ -181,7 +233,7 @@ sdr_sr_done: > > > > > > > > > > skip_disable_main_clock: > > > > > /* Wait for interrupt */ > > > > > - mcr p15, 0, tmp1, c7, c0, 4 > > > > > + _do_wfi > > > > > > > > > > tst mode, #AT91_PM_SLOW_CLOCK > > > > > beq skip_enable_main_clock > > > > > -- > > > > > 1.7.9.5 > > > > > > > > > > -- > > > > > To unsubscribe from this list: send the line "unsubscribe > > > > > linux-kernel" in the body of a message to > > > > > majordomo at vger.kernel.org More majordomo info at > > > > > http://vger.kernel.org/majordomo-info.html > > > > > Please read the FAQ at http://www.tux.org/lkml/ > > > > > > > > > > > Best Regards, > > > Wenyou Yang > > > > > Best Regards, > Wenyou Yang >
next prev parent reply other threads:[~2015-01-30 10:18 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-01-26 10:03 [PATCH 0/7] AT91 pm improvements for 3.20 Wenyou Yang 2015-01-26 10:03 ` Wenyou Yang 2015-01-26 10:04 ` [PATCH 1/7] pm: at91: achieve the memory controller's type from the dts file Wenyou Yang 2015-01-26 10:04 ` Wenyou Yang 2015-01-26 10:06 ` [PATCH 2/7] pm: at91: pm_suspend: add the WFI support for ARMv7 Wenyou Yang 2015-01-26 10:06 ` Wenyou Yang 2015-01-26 13:05 ` Sergei Shtylyov 2015-01-26 13:05 ` Sergei Shtylyov 2015-01-27 4:44 ` Yang, Wenyou 2015-01-27 4:44 ` Yang, Wenyou 2015-01-28 11:25 ` Lorenzo Pieralisi 2015-01-28 11:25 ` Lorenzo Pieralisi 2015-01-29 2:36 ` Yang, Wenyou 2015-01-29 2:36 ` Yang, Wenyou 2015-01-29 12:22 ` Lorenzo Pieralisi 2015-01-29 12:22 ` Lorenzo Pieralisi 2015-01-30 7:23 ` Yang, Wenyou 2015-01-30 7:23 ` Yang, Wenyou 2015-01-30 10:17 ` Lorenzo Pieralisi [this message] 2015-01-30 10:17 ` Lorenzo Pieralisi 2015-01-30 10:44 ` Lorenzo Pieralisi 2015-01-30 10:44 ` Lorenzo Pieralisi 2015-01-26 10:06 ` [PATCH 3/7] pm: at91: pm_suspend: MOR register KEY was missing Wenyou Yang 2015-01-26 10:06 ` Wenyou Yang 2015-01-26 10:07 ` [PATCH 4/7] ARM: at91: enable the L2 Cache controller Wenyou Yang 2015-01-26 10:07 ` Wenyou Yang 2015-01-26 11:46 ` Mark Rutland 2015-01-26 11:46 ` Mark Rutland 2015-01-26 12:45 ` Russell King - ARM Linux 2015-01-26 12:45 ` Russell King - ARM Linux 2015-01-26 22:36 ` Alexandre Belloni 2015-01-26 22:36 ` Alexandre Belloni 2015-01-27 5:11 ` Yang, Wenyou 2015-01-27 5:11 ` Yang, Wenyou 2015-01-26 10:07 ` [PATCH 5/7] pm: at91: add disable/enable the L1/L2 cache while suspend/resume Wenyou Yang 2015-01-26 10:07 ` Wenyou Yang 2015-01-26 10:08 ` [PATCH 6/7] pm: at91: add achieve the mpddrc peripheral ID and the DDR clock ID support Wenyou Yang 2015-01-26 10:08 ` Wenyou Yang 2015-01-26 11:49 ` Mark Rutland 2015-01-26 11:49 ` Mark Rutland 2015-01-27 5:24 ` Yang, Wenyou 2015-01-27 5:24 ` Yang, Wenyou 2015-01-26 10:08 ` [PATCH 7/7] pm: at91: add disable/enable the mpddrc's clock and DDR clock support Wenyou Yang 2015-01-26 10:08 ` Wenyou Yang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150130101709.GB8787@red-moon \ --to=lorenzo.pieralisi@arm.com \ --cc=Nicolas.FERRE@atmel.com \ --cc=Patrice.VILCHEZ@atmel.com \ --cc=Wenyou.Yang@atmel.com \ --cc=alexandre.belloni@free-electrons.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=peda@axentia.se \ --cc=sylvain.rochet@finsecur.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
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.