linux-kernel - Re: [PATCH v5 1/3] ARM: rockchip: fix the CPU soft reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <9253141.D1erkaHIPa@diego>
Date:	Tue, 09 Jun 2015 09:46:47 +0200
From:	Heiko Stübner <heiko@...ech.de>
To:	Caesar Wang <wxt@...k-chips.com>
Cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	杨凯 <kever.yang@...k-chips.com>,
	dianders@...omium.org, Dmitry Torokhov <dmitry.torokhov@...il.com>,
	linux-rockchip@...ts.infradead.org,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v5 1/3] ARM: rockchip: fix the CPU soft reset

Am Dienstag, 9. Juni 2015, 08:43:56 schrieb Caesar Wang:
> 在 2015年06月08日 17:43, Russell King - ARM Linux 写道:
> > On Mon, Jun 08, 2015 at 03:11:34PM +0800, Caesar Wang wrote:
> >> diff --git a/arch/arm/mach-rockchip/platsmp.c
> >> b/arch/arm/mach-rockchip/platsmp.c index 5b4ca3c..bd40852 100644
> >> --- a/arch/arm/mach-rockchip/platsmp.c
> >> +++ b/arch/arm/mach-rockchip/platsmp.c
> >> @@ -72,6 +72,7 @@ static struct reset_control
> >> *rockchip_get_core_reset(int cpu)>> 
> >>   static int pmu_set_power_domain(int pd, bool on)
> >>   {
> >>   
> >>   	u32 val = (on) ? 0 : BIT(pd);
> >> 
> >> +	struct reset_control *rstc = rockchip_get_core_reset(pd);
> >> 
> >>   	int ret;
> >>   	
> >>   	/*
> >> 
> >> @@ -80,20 +81,15 @@ static int pmu_set_power_domain(int pd, bool on)
> >> 
> >>   	 * processor is powered down.
> >>   	 */
> >>   	
> >>   	if (read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
> >> 
> >> -		struct reset_control *rstc = rockchip_get_core_reset(pd);
> >> -
> >> +		/* We only require the reset on the RK3288 at the moment */
> >> 
> >>   		if (IS_ERR(rstc)) {
> >>   		
> >>   			pr_err("%s: could not get reset control for core %d\n",
> >>   			
> >>   			       __func__, pd);
> >>   			
> >>   			return PTR_ERR(rstc);
> >>   		
> >>   		}
> >> 
> >> -		if (on)
> >> -			reset_control_deassert(rstc);
> >> -		else
> >> +		if (!on)
> >> 
> >>   			reset_control_assert(rstc);
> >> 
> >> -
> >> -		reset_control_put(rstc);
> >> 
> >>   	}
> > 
> > Do we need the CPU part number check for the assertion/deassertion of
> > the reset control?  Surely the DT provides this where appropriate and
> > omits it where it's inappropriate?
> > 
> > If so, I'd separate out the decision whether to error out from the
> > 
> > decision whether to assert the reset control, like this:
> > 	if (IS_ERR(rstc) && read_cpuid_part() != ARM_CPU_PART_CORTEX_A9) {
> > 	
> > 		pr_err("%s: could not get reset control for core %d\n",
> > 		
> > 		       __func__, pd);
> > 		
> > 		return PTR_ERR(rstc);
> > 	
> > 	}
> > 	
> > 	if (!IS_ERR(rstc) && !on)
> > 	
> > 		reset_control_assert(rstc);
> > 
> > or maybe:
> > 	if (IS_ERR(rstc) && PTR_ERR(rstc) != -ENOENT) {
> > 	
> > 		pr_err("%s: could not get reset control for core %d: %d\n",
> > 		
> > 		       __func__, pd, PTR_ERR(rstc));
> > 		
> > 		return PTR_ERR(rstc);
> > 	
> > 	}
> > 	
> > 	if (!IS_ERR(rstc) && !on)
> > 	
> > 		reset_control_assert(rstc);
> > 
> > which lets you detect whether of_reset_control_get() failed because the
> > reset control was not present (and thus not required) vs failed for some
> > other reason - and this means that the issue of whether the reset
> > control is used is entirely up to the supplied DT file.
> 
> Sound resonable for me.
> 
> I hope get the Heiko and Kever option.
> After all the code is uploaded by Heiko and Kever.

Russell's suggestion is very nice indeed.

We have reset controls for the cpu cores on all Rockchip SoCs but currently 
only absolutely require them on the rk3288, so what Russell suggested handles 
this nicely if we require this later for other SoCs as well (like the A7 ones) 
.


Heiko

> 
> >>   	ret = regmap_update_bits(pmu, PMU_PWRDN_CON, BIT(pd), val);
> >> 
> >> @@ -112,6 +108,12 @@ static int pmu_set_power_domain(int pd, bool on)
> >> 
> >>   		}
> >>   	
> >>   	}
> >> 
> >> +	if (read_cpuid_part() != ARM_CPU_PART_CORTEX_A9 && on)
> >> +		reset_control_deassert(rstc);
> >> +
> >> +	if (!IS_ERR(rstc))
> >> +		reset_control_put(rstc);
> >> +
> > 
> > and here:
> > 	if (!IS_ERR(rstc)) {
> > 	
> > 		if (on)
> > 		
> > 			reset_control_deassert(rstc);
> > 		
> > 		reset_control_put(rstc);
> > 	
> > 	}
> 
> Ditto.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/