Re: [PATCH v3 1/3] dt: psci: Add arm,psci-sys-reset2-vendor-param property

[Elliot Berman <quic_eberman@xxxxxxxxxxx>]

Reviving this very old thread. In case folks don't have in their 
mailbox, here is archive link:

https://lore.kernel.org/all/20200320120105.GA36658@C02TD0UTHF1T.local/

On 3/20/2020 5:01 AM, Mark Rutland wrote:
> Hi Elliot,
>
> On Thu, Mar 05, 2020 at 11:05:27AM -0800, Elliot Berman wrote:
> > Some implementors of PSCI may wish to use a different reset type than
> > SYSTEM_WARM_RESET. For instance, Qualcomm SoCs support an alternate
> > reset_type which may be used in more warm reboot scenarios than
> > SYSTEM_WARM_RESET permits (e.g. to reboot into recovery mode).
>
> To be honest, I'm still confused by this series, and I think that these
> patches indicate a larger problem that we cannot solve generally (e.g.
> on other platofrms and/or with ACPI).
>
> I think the underlying issue is whether the semantics for:
>
> a) Linux's RESET_WARM and RESET_SOFT
> b) PSCI's SYSTEM_RESET2 SYSTEM_WARM_RESET
>
> ... actually align in practice, which this series suggests is not the
> case.
>
> If those don't align, then I think that commit:
>
>    4302e381a870aafb ("firmware/psci: add support for SYSTEM_RESET2")
>
> ... is not actually reliable, and not something we can support by
> default, and we should rethink the code introduce in that commit.
>
> If (a) and (b) are supposed to align, and the behaviour on your platform
> is an erratum, then I think we should treat it as such rather than
> adding a property that is open to abuse.
>
> Thoughts?
I think it's ok for (a) and (b) to align (that invalidates this series). 
PSCI SYSTEM_RESET2 supports vendor-specific reset types and the PSCI 
driver doesn't have any way for these vendor-specific resets to happen.

Would a better way be to export a psci_system_reset2 function? I can 
implement a reboot-mode driver for PSCI system_reset2 for DT-based 
platforms.

Thanks,
Elliot

> Thanks,
> Mark.
>
> > Reviewed-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Signed-off-by: Elliot Berman <eberman@xxxxxxxxxxxxxx>
> > ---
> >   Documentation/devicetree/bindings/arm/psci.yaml | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > index 8ef8542..1a9d2dd 100644
> > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > @@ -102,6 +102,13 @@ properties:
> >         [1] Kernel documentation - ARM idle states bindings
> >           Documentation/devicetree/bindings/arm/idle-states.txt
> >   
> > +  arm,psci-sys-reset2-vendor-param:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +        Vendor-specific reset type parameter to use for SYSTEM_RESET2 during
> > +        a warm or soft reboot. If no value is provided, then architectural
> > +        reset type SYSTEM_WARM_RESET is used.
> > +
> >     "#power-domain-cells":
> >       description:
> >         The number of cells in a PM domain specifier as per binding in [3].
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
>
>  From mboxrd@z Thu Jan  1 00:00:00 1970
> Return-Path: <SRS0=iAha=5F=lists.infradead.org=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@xxxxxxxxxx>
> X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on
> 	aws-us-west-2-korg-lkml-1.web.codeaurora.org
> X-Spam-Level:
> X-Spam-Status: No, score=-6.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
> 	DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,
> 	SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
> 	version=3.4.0
> Received: from mail.kernel.org (mail.kernel.org [198.145.29.99])
> 	by smtp.lore.kernel.org (Postfix) with ESMTP id 4F3C9C43333
> 	for <infradead-linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Fri, 20 Mar 2020 12:01:17 +0000 (UTC)
> Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133])
> 	(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
> 	(No client certificate requested)
> 	by mail.kernel.org (Postfix) with ESMTPS id 1C65120732
> 	for <infradead-linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>; Fri, 20 Mar 2020 12:01:16 +0000 (UTC)
> Authentication-Results: mail.kernel.org;
> 	dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="AoWzGqG2"
> DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1C65120732
> Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arm.com
> Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@xxxxxxxxxxxxxxxxxxx
> DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed;
> 	d=lists.infradead.org; s=bombadil.20170209; h=Sender:
> 	Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post:
> 	List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References:
> 	Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description:
> 	Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:
> 	List-Owner; bh=15BYqh1L1ALv71IjF7NXoGOSeeS0sbsBf/ahe6hUVYY=; b=AoWzGqG2lL+PnT
> 	eHgf22G62sLTs4kKNimymlbuMng5lx29kBuft3E15X+B1hWZr4smk8/XlIARAMD2tVef89z6wBHTq
> 	Uj3FBiZzghCEKhiW+yJA2h/DxfBmBWBDmS2iPBQlehFOpftLMApK6uSGvHKbh4CjuYVS1FFjzli2g
> 	10leZnncASBmeyiZl/17d3H286+dalis6nDW6GfOYMzg/zhlZJi630/CPEsmlt/4TkIr6SQYPsfpI
> 	DtjiDGeQYCZEqNvtu22etrtCleRfqtsoLyLYBSD1zn2haDJTzhhDQgoZ8+8xr2dzNR9lnhfONmyHC
> 	8Q65eWvNSwTvJlb55XxQ==;
> Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org)
> 	by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux))
> 	id 1jFGKy-0004HY-Gg; Fri, 20 Mar 2020 12:01:16 +0000
> Received: from foss.arm.com ([217.140.110.172])
>   by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux))
>   id 1jFGKt-0004Gl-QZ
>   for linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Fri, 20 Mar 2020 12:01:14 +0000
> Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
>   by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7AA5D31B;
>   Fri, 20 Mar 2020 05:01:10 -0700 (PDT)
> Received: from C02TD0UTHF1T.local (usa-sjc-imap-foss1.foss.arm.com
>   [10.121.207.14])
>   by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 1E89A3F85E;
>   Fri, 20 Mar 2020 05:01:07 -0700 (PDT)
> Date: Fri, 20 Mar 2020 12:01:05 +0000
> From: Mark Rutland <mark.rutland@xxxxxxx>
> To: Elliot Berman <eberman@xxxxxxxxxxxxxx>
> Subject: Re: [PATCH v3 1/3] dt: psci: Add arm,psci-sys-reset2-vendor-param
>   property
> Message-ID: <20200320120105.GA36658@C02TD0UTHF1T.local>
> References: <1583435129-31356-1-git-send-email-eberman@xxxxxxxxxxxxxx>
>   <1583435129-31356-2-git-send-email-eberman@xxxxxxxxxxxxxx>
> MIME-Version: 1.0
> Content-Disposition: inline
> In-Reply-To: <1583435129-31356-2-git-send-email-eberman@xxxxxxxxxxxxxx>
> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3
> X-CRM114-CacheID: sfid-20200320_050112_965539_E60DC8A3
> X-CRM114-Status: GOOD (  19.35  )
> X-BeenThere: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> X-Mailman-Version: 2.1.29
> Precedence: list
> List-Id: <linux-arm-kernel.lists.infradead.org>
> List-Unsubscribe: <http://lists.infradead.org/mailman/options/linux-arm-kernel>,
>   <mailto:linux-arm-kernel-request@xxxxxxxxxxxxxxxxxxx?subject=unsubscribe>
> List-Archive: <http://lists.infradead.org/pipermail/linux-arm-kernel/>
> List-Post: <mailto:linux-arm-kernel@xxxxxxxxxxxxxxxxxxx>
> List-Help: <mailto:linux-arm-kernel-request@xxxxxxxxxxxxxxxxxxx?subject=help>
> List-Subscribe: <http://lists.infradead.org/mailman/listinfo/linux-arm-kernel>,
>   <mailto:linux-arm-kernel-request@xxxxxxxxxxxxxxxxxxx?subject=subscribe>
> Cc: Trilok Soni <tsoni@xxxxxxxxxxxxxx>,
>   Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>,
>   David Collins <collinsd@xxxxxxxxxxxxxx>, linux-arm-msm@xxxxxxxxxxxxxxx,
>   linux-kernel@xxxxxxxxxxxxxxx, Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>,
>   Sudeep Holla <sudeep.holla@xxxxxxx>, Prasad Sodagudi <psodagud@xxxxxxxxxxxxxx>,
>   linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Content-Type: text/plain; charset="us-ascii"
> Content-Transfer-Encoding: 7bit
> Sender: "linux-arm-kernel" <linux-arm-kernel-bounces@xxxxxxxxxxxxxxxxxxx>
> Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@xxxxxxxxxxxxxxxxxxx
>
> Hi Elliot,
>
> On Thu, Mar 05, 2020 at 11:05:27AM -0800, Elliot Berman wrote:
> > Some implementors of PSCI may wish to use a different reset type than
> > SYSTEM_WARM_RESET. For instance, Qualcomm SoCs support an alternate
> > reset_type which may be used in more warm reboot scenarios than
> > SYSTEM_WARM_RESET permits (e.g. to reboot into recovery mode).
>
> To be honest, I'm still confused by this series, and I think that these
> patches indicate a larger problem that we cannot solve generally (e.g.
> on other platofrms and/or with ACPI).
>
> I think the underlying issue is whether the semantics for:
>
> a) Linux's RESET_WARM and RESET_SOFT
> b) PSCI's SYSTEM_RESET2 SYSTEM_WARM_RESET
>
> ... actually align in practice, which this series suggests is not the
> case.
>
> If those don't align, then I think that commit:
>
>    4302e381a870aafb ("firmware/psci: add support for SYSTEM_RESET2")
>
> ... is not actually reliable, and not something we can support by
> default, and we should rethink the code introduce in that commit.
>
> If (a) and (b) are supposed to align, and the behaviour on your platform
> is an erratum, then I think we should treat it as such rather than
> adding a property that is open to abuse.
>
> Thoughts?
>
> Thanks,
> Mark.
>
> > Reviewed-by: Sudeep Holla <sudeep.holla@xxxxxxx>
> > Signed-off-by: Elliot Berman <eberman@xxxxxxxxxxxxxx>
> > ---
> >   Documentation/devicetree/bindings/arm/psci.yaml | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/arm/psci.yaml b/Documentation/devicetree/bindings/arm/psci.yaml
> > index 8ef8542..1a9d2dd 100644
> > --- a/Documentation/devicetree/bindings/arm/psci.yaml
> > +++ b/Documentation/devicetree/bindings/arm/psci.yaml
> > @@ -102,6 +102,13 @@ properties:
> >         [1] Kernel documentation - ARM idle states bindings
> >           Documentation/devicetree/bindings/arm/idle-states.txt
> >   
> > +  arm,psci-sys-reset2-vendor-param:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description: |
> > +        Vendor-specific reset type parameter to use for SYSTEM_RESET2 during
> > +        a warm or soft reboot. If no value is provided, then architectural
> > +        reset type SYSTEM_WARM_RESET is used.
> > +
> >     "#power-domain-cells":
> >       description:
> >         The number of cells in a PM domain specifier as per binding in [3].
> > --
> > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> > a Linux Foundation Collaborative Project
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel