From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756440AbbJVEXl (ORCPT ); Thu, 22 Oct 2015 00:23:41 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:51865 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751363AbbJVEXh (ORCPT ); Thu, 22 Oct 2015 00:23:37 -0400 X-AuditID: cbfee691-f79d66d000001509-16-562864bcb429 Message-id: <56286294.7060907@samsung.com> Date: Thu, 22 Oct 2015 09:44:12 +0530 From: Alim Akhtar User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.5.0 MIME-version: 1.0 To: Javier Martinez Canillas , Krzysztof Kozlowski , linux-kernel@vger.kernel.org Cc: Markus Reichl , Anand Moon , linux-samsung-soc@vger.kernel.org, Marek Szyprowski , linux-mmc@vger.kernel.org, Alexandre Courbot , Ulf Hansson , Douglas Anderson , heiko@sntech.de, enric.balletbo@collabora.com Subject: Re: [PATCH] mmc: pwrseq: Use highest priority for eMMC restart handler References: <1445440540-21525-1-git-send-email-javier@osg.samsung.com> <56282F71.2070408@samsung.com> <562839F7.3040005@osg.samsung.com> <56283F27.9060804@samsung.com> <56284F60.4070802@osg.samsung.com> In-reply-to: <56284F60.4070802@osg.samsung.com> Content-type: text/plain; charset=windows-1252; format=flowed Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrBIsWRmVeSWpSXmKPExsWyRsSkRndvikaYweR7HBbfH55itTi77CCb xZrbhxgt/j96zWrx5u0aJovXLwwtLu+aw2Zx5H8/o8WM8/uYLNZtvMVu8fLID0aLtUfuslsc XxvuwOsxu+Eii8eOu0sYPW7dqffYOesuu8eda3vYPHqb37F5bOkHcvu2rGL02H5tHrPH501y AVxRXDYpqTmZZalF+nYJXBnTt/1mL7jjVfG6aQFrA+Nkmy5GDg4JAROJm9sCuxg5gUwxiQv3 1rN1MXJxCAmsYJQ49rqBFSJhIvHl6E4WEFtIYCmjxJu7YRBFDxgl9p5bAFbEK6AlcWnTC2YQ m0VAVWJ2byNYnE1AW+Lu9C1MIMtEBSIkHl8QgigXlPgx+R4LyBwRgRZGiQ1broE5zAKvmSQ6 5rWBNQsL+EuseNDBDrHtHKPE7hdTwc7gFNCXmPZvD5jNLGArseD9OihbXmLzmrfMIA0SAnM5 JD6tvcgCcZKAxLfJh1ggfpaV2HSAGeI1SYmDK26wTGAUm4XkqllIxs5CMnYBI/MqRtHUguSC 4qT0IlO94sTc4tK8dL3k/NxNjMAIP/3v2cQdjPcPWB9iFOBgVOLh1fivHibEmlhWXJl7iNEU 6IqJzFKiyfnANJJXEm9obGZkYWpiamxkbmmmJM6rI/0zWEggPbEkNTs1tSC1KL6oNCe1+BAj EwenVAOjJl9U3N+NwQpb512OXf31c9jVrTs8ew+W8Rxa9V1OsWoXf9jcMwu1dK9wHy+Ka8tx 4yz5tU8zTu6utMmmiNKyO13brzwRSxZydQ774y3C+mCV0efb71zZTlUVujHW2cYcD1jHKXGs Sbg3J78s7aBt8Z3nF1u6Xz3qPphQs/fWqcItoUntt/SVWIozEg21mIuKEwFkq5x/6wIAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrGKsWRmVeSWpSXmKPExsVy+t9jAd09KRphBpP+WFp8f3iK1eLssoNs FmtuH2K0+P/oNavFm7drmCxevzC0uLxrDpvFkf/9jBYzzu9jsli38Ra7xcsjPxgt1h65y25x fG24A6/H7IaLLB477i5h9Lh1p95j56y77B53ru1h8+htfsfmsaUfyO3bsorRY/u1ecwenzfJ BXBFNTDaZKQmpqQWKaTmJeenZOal2yp5B8c7x5uaGRjqGlpamCsp5CXmptoqufgE6Lpl5gDd rqRQlphTChQKSCwuVtK3wzQhNMRN1wKmMULXNyQIrsfIAA0krGHMmL7tN3vBHa+K100LWBsY J9t0MXJySAiYSHw5upMFwhaTuHBvPRuILSSwlFHizd2wLkYuIPsBo8TecwtYQRK8AloSlza9 YAaxWQRUJWb3NoLF2QS0Je5O38LUxcjBISoQIfH4ghBEuaDEj8n3WEDmiAi0MEps2HINzGEW eM0k0TGvDaxZWMBfYsWDDnaIbecYJXa/mAp2EqeAvsS0f3vAbGYBW4kF79dB2fISm9e8ZZ7A KDALyZZZSMpmISlbwMi8ilEitSC5oDgpPdcwL7Vcrzgxt7g0L10vOT93EyM4jTyT2sF4cJf7 IUYBDkYlHt4Lf9TDhFgTy4orcw8xSnAwK4nw/rPUCBPiTUmsrEotyo8vKs1JLT7EaAoMhonM UqLJ+cAUl1cSb2hsYm5qbGppYmFiZqkkznvjEEOYkEB6YklqdmpqQWoRTB8TB6dUA2OjYHur /7zKN16XCyafb44xWSlRdEGy3cBnj/3clsAq3UWzFC9P4ywPeO/lO4Fdbqbc45l3OpT6/q1I yL00x5eDS2jv9ssCnWunxU5hv292tkpLb/W/mQeqys6GqiwODVKKtJvCv0B9Q+8N46ev4kNY 9aJcJ89s5ZtnvsHkt6DnMt/ehdkMdkosxRmJhlrMRcWJALmmPdQ5AwAA DLP-Filter: Pass X-MTR: 20000000000000000@CPGS X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org CCing Doug, Heiko and Enric Balletbo To help us by testing on rk3288-veyron and am335x-sl50 boards. On 10/22/2015 08:22 AM, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 10/22/2015 03:43 AM, Krzysztof Kozlowski wrote: >> On 22.10.2015 10:20, Javier Martinez Canillas wrote:> Hello Krzysztof, >>> >>> Thanks for your feedback. >>> >>> On 10/22/2015 02:36 AM, Krzysztof Kozlowski wrote: >>>> On 22.10.2015 00:15, Javier Martinez Canillas wrote: >>>>> The pwrseq_emmc driver does a eMMC card reset before a system reboot to >>>>> allow broken or limited ROM boot-loaders (that don't have an eMMC reset >>>>> logic) to be able to read the second stage from the eMMC. >>>>> >>>>> But this has to be called before a system reboot handler and while most >>>>> of them use the priority 128, there are other restart handlers (such as >>>>> the syscon-reboot one) that use a higher priority. So, use the highest >>>>> priority to make sure that the eMMC hw is reset before a system reboot. >>>>> >>>>> Signed-off-by: Javier Martinez Canillas >>>>> Tested-by: Markus Reichl >>>>> Tested-by: Anand Moon >>>>> Reviewed-by: Alim Akhtar >>>>> >>>>> --- >>>>> Hello, >>>>> >>>>> This patch was needed since a recent series from Alim [0] added >>>>> syscon reboot and poweroff support to Exynos SoCs and removed >>>>> the reset handler in the Exynos Power Management Unit (PMU) code. >>>>> >>>>> But the PMU and syscon-reboot restart handler have a different >>>>> priority so [0] breaks restart when eMMC is used on these boards. >>>>> >>>>> [0]: http://www.spinics.net/lists/arm-kernel/msg454396.html >>>>> >>>>> So this patch must be merged before [0] to avoid regressions. >>>>> >>>>> Best regards, >>>>> Javier >>>>> >>>>> drivers/mmc/core/pwrseq_emmc.c | 6 +++--- >>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/mmc/core/pwrseq_emmc.c b/drivers/mmc/core/pwrseq_emmc.c >>>>> index 137c97fb7aa8..ad4f94ec7e8d 100644 >>>>> --- a/drivers/mmc/core/pwrseq_emmc.c >>>>> +++ b/drivers/mmc/core/pwrseq_emmc.c >>>>> @@ -84,11 +84,11 @@ struct mmc_pwrseq *mmc_pwrseq_emmc_alloc(struct mmc_host *host, >>>>> >>>>> /* >>>>> * register reset handler to ensure emmc reset also from >>>>> - * emergency_reboot(), priority 129 schedules it just before >>>>> - * system reboot >>>>> + * emergency_reboot(), priority 255 is the highest priority >>>>> + * so it will be executed before any system reboot handler. >>>>> */ >>>>> pwrseq->reset_nb.notifier_call = mmc_pwrseq_emmc_reset_nb; >>>>> - pwrseq->reset_nb.priority = 129; >>>>> + pwrseq->reset_nb.priority = 255; >>>> >>>> I see the problem which you are trying to solve but this may be tricker >>>> then just kicking the number. Some of restart handlers are registered >>>> with priority 192. I found few of such, like: at91_restart_nb, >>>> zynq_slcr_restart_nb, rmobile_reset_nb (maybe more, I did not grep too >>>> much). >>>> >>> >>> Yes, the syscon-reboot restart handler also uses a priority 192 and that >>> is why reboot with eMMC broke with Alim's patches since the PMU restart >>> handler priority is 128. >>> >>>> I guess they chose the "192" priority on purpose. >>>> >>> >>> I tried to understand what's the policy w.r.t priority numbering for >>> restart handlers but only found this in the register_restart_handler >>> kernel-doc [0]: >>> >>> /** >>> * register_restart_handler - Register function to be called to reset >>> * the system >>> * @nb: Info about handler function to be called >>> * @nb->priority: Handler priority. Handlers should follow the >>> * following guidelines for setting priorities. >>> * 0: Restart handler of last resort, >>> * with limited restart capabilities >>> * 128: Default restart handler; use if no other >>> * restart handler is expected to be available, >>> * and/or if restart functionality is >>> * sufficient to restart the entire system >>> * 255: Highest priority restart handler, will >>> * preempt all other restart handlers >>> >>> So, reading that is not clear to me if only the values 0, 128 and 255 >>> should be used or any value from 0-255. >>> >>> What's clear to me is that restart handlers to reset a specific hw block >>> should be called before the restart handler that resets the whole system. >>> >>> The 192 seems to be used because there are other default restart handlers >>> that are using a prio of 128. See for example the commit that changed the >>> syscon-reboot prio from 128 to 192: >>> >>> b81180b3fd48 power: reset: adjust priority of simple syscon reboot driver >> >> But were are here not talking about syscon handler but the others. Now >> you will be ahead of them. >> > > Yes, I know that. My point was that the platforms were either not using the > mmc-pwrseq-emmc or their system restart handler already had a lower priority > but that is not true for at least rk3288-veyron as you said. > >>> >>> So probably the 192 value was chosen because is in the middle of 128 and >>> 255 but it seems to me a rather arbitrary value and I would prefer it to >>> be documented in some place. >>> >>>> Effectively, now the emmc handler will be executed before their >>>> handlers... is it an issue? Maybe some testing on these platforms is >>>> necessary? >>>> >>> >>> I don't think is an issue, the reason why I chose 255 is that it is >>> a documented value in the kernel-doc and since is the highest prio, >>> it makes sure the eMMC will be reset before any system restart handler. >>> >>> Also, the pwrseq_emmc driver is only used in platforms whose SoC ROM >>> can either leave the eMMC in an unknown state so the kernel needs to >>> hw reset the eMMC or does not have a reset logic so it can only read >>> from an eMMC if is in a known state (i.e: after a reset from kernel). >> >> I think at least one platform may be affected because it used >> mmc-pwrseq-emmc and gpio-restart. >> >> Look at rk3288-veyron.dtsi. >> >> Both of restart handlers had the priority of 129 which means that the >> order of execution depends on probing sequence. Now you will make the >> sequence strict - first mmc then gpio. >> > > The behavior is going to change indeed in that board but no due probe > order but because the gpio-restart handler dev node has priority = <200> > which overrides the default 129 in the gpio-restart driver. > > So before $SUBJECT the eMMC restart handler was not executed but now it > will be after this change. > >> You seems convinced that this is not a problem... I don't know. I would >> prefer test this on affected platforms before risking to break them. >> It's annoying if fix for one SoC breaks another. >> > > Agreed. > >>> >>> Since the current mmc_pwrseq_emmc_reset_nb notifier priority is 129, >>> eMMC reset will not work if one of the platforms you mentioned needs >>> this since the system restart handler with prio 192 will be executed >>> before the eMMC one, leaving the eMMC in an unknown state on reboot. >> >> And now you will "fix this" by making eMMC working correctly. So let's >> make it straight: >> 1. Previously the eMMC could be left on these platforms in an unknown >> state (because emmc handler was not executed). >> 2. No one complained! Which could mean that in fact this was working fine... >> 3. Now you will change it. >> 4. Maybe someone will complain? >> >> Just test it (or get an ack/tested tag). That's all what is needed. >> > > Yes, I never meant that the patch should be merged without testing... > >> >>> And $SUBJECT should not cause any regressions for the platforms that >>> are currently using the pwrseq_emmc, since the restart handler was >>> already being called before the system restart handler so bumping >>> the priority should not cause any effect. >> >> I found at least one platform where the sequence *might* change. There >> could be more of them. >> > > Agreed, I missed that rk3288-veyron is using a restart handler with higher > priority and could be other boards too as you said. > > Let's see what is Marek's opinion since he added the pwrseq_emmc support > and also what Ulf thinks about always doing a eMMC reset before reboot. > > I can't think how doing a eMMC card reset before reboot could affect a > board but you are right that we don't know without testing. > git grep result shows: ====== git grep mmc-pwrseq-emmc Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt:- compatible : contains "mmc-pwrseq-emmc". Documentation/devicetree/bindings/mmc/mmc-pwrseq-emmc.txt: compatible = "mmc-pwrseq-emmc"; arch/arm/boot/dts/am335x-sl50.dts: compatible = "mmc-pwrseq-emmc"; arch/arm/boot/dts/exynos4412-odroid-common.dtsi: compatible = "mmc-pwrseq-emmc"; arch/arm/boot/dts/exynos5422-odroidxu3-common.dtsi: compatible = "mmc-pwrseq-emmc"; arch/arm/boot/dts/rk3288-veyron.dtsi: compatible = "mmc-pwrseq-emmc"; arch/arm64/boot/dts/rockchip/rk3368-r88.dts: compatible = "mmc-pwrseq-emmc"; drivers/mmc/core/pwrseq.c: .compatible = "mmc-pwrseq-emmc", ===== So apart from exynos, there are rk3xxx and am335x which used pwrseq-emmc-restart. So lets wait for the feedback from these guys as well. Thanks. >> Best regards, >> Krzysztof >> > > Best regards, >