Re: [PATCH v2 2/3] firmware: psci: Add support for dt-supplied SYSTEM_RESET2 type

From: Elliot Berman <eberman@codeaurora.org>
To: Sudeep Holla <sudeep.holla@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
	Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Trilok Soni <tsoni@codeaurora.org>,
	Prasad Sodagudi <psodagud@codeaurora.org>,
	David Collins <collinsd@codeaurora.org>,
	linux-arm-kernel@lists.infradead.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 2/3] firmware: psci: Add support for dt-supplied SYSTEM_RESET2 type
Date: Tue, 25 Feb 2020 17:37:53 -0800	[thread overview]
Message-ID: <1d7fecf8-3a7f-57e5-5c13-73de89d52aa2@codeaurora.org> (raw)
In-Reply-To: <20200225110346.GF32784@bogus>

On 2/25/2020 3:03 AM, Sudeep Holla wrote:
> On Mon, Feb 24, 2020 at 12:57:37PM -0800, Elliot Berman wrote:
>> @@ -493,6 +494,7 @@ typedef int (*psci_initcall_t)(const struct device_node *);
>>  static int __init psci_0_2_init(struct device_node *np)
>>  {
>>  	int err;
>> +	u32 param;
>>
>>  	err = get_set_conduit_method(np);
>>  	if (err)
>> @@ -505,7 +507,19 @@ static int __init psci_0_2_init(struct device_node *np)
>>  	 * can be carried out according to the specific version reported
>>  	 * by firmware
>>  	 */
>> -	return psci_probe();
>> +	err = psci_probe();
>> +	if (err)
>> +		return err;
>> +
>> +	if (psci_system_reset2_supported &&
>> +	    !of_property_read_u32(np, "arm,psci-sys-reset2-param", &param)) {
>> +		if ((s32)param > 0)
> 
> What is the point on signed comparison here ? You are assuming all vendor
> reset also as architecture by doing so which is wrong.
> 
>> +			pr_warn("%08x is an invalid architectural reset type.\n",
>> +				param);
> 
> I thought the point was to have vendor reset here. Based on the 3/3 you
> see to have vendor reset bit set, you ignore that by doing signed comparison
> which is wrong and even the message is wrong. Specification defines only
> one architectural reset(WARM RESET) and all others need to be vendor specific.
> 
> --
> Regards,
> Sudeep
> 
I might've gone crazy, but all vendor-specific reset types would be
negative when cast as s32. Thus the check returns true only for an invalid
architectural reset type. I can switch to checking bits instead of using 
cast in v3 to avoid the confusion.

Alternatively, I could rename the DT property to
"arm,psci-sys-reset2-vendor-param" and then always set the 31st bit so that
it is impossible to provide an invalid architectural reset type in DT.

Let me know what is preferred.

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project