linux-kernel - Re: [PATCH v7 1/3] clk: qcom: Configure the RCGs to a safe source as needed

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: <152574189516.138124.5711696582182066656@swboyd.mtv.corp.google.com>
Date:   Mon, 07 May 2018 18:11:35 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Amit Nischal <anischal@...eaurora.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>
Cc:     Andy Gross <andy.gross@...aro.org>,
        David Brown <david.brown@...aro.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Odelu Kukatla <okukatla@...eaurora.org>,
        Taniya Das <tdas@...eaurora.org>,
        linux-arm-msm@...r.kernel.org, linux-soc@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Amit Nischal <anischal@...eaurora.org>
Subject: Re: [PATCH v7 1/3] clk: qcom: Configure the RCGs to a safe source as needed

Quoting Amit Nischal (2018-05-07 03:50:18)
> For some root clock generators, there could be child branches which are
> controlled by an entity other than application processor subsystem. For
> such RCGs, as per application processor subsystem clock driver, all of
> its downstream clocks are disabled and RCG is in disabled state but in
> reality downstream clocks can be left enabled before.
> 
> So in this scenario, when RCG is disabled as per clock driver's point of
> view and when rate scaling request comes before downstream clock enable
> request, then RCG fails to update its configuration because in reality
> RCG is on and it expects its new source to already be in enable state but
> in reality new source is off. In order to avoid having the RCG to go into
> an invalid state, add support to update the CFG, M, N and D registers
> during set_rate() without configuration update and defer the actual RCG
> configuration update to be done during clk_enable() as at this point of
> time, both its new parent and safe source will be already enabled and RCG
> can safely switch to new parent.
> 
> During clk_disable() request, configure it to safe source as both its
> parents, safe source and current parent will be enabled and RCG can
> safely execute a switch.
> 
> Signed-off-by: Taniya Das <tdas@...eaurora.org>
> Signed-off-by: Amit Nischal <anischal@...eaurora.org>
> ---

Applied to clk-next. I squashed some style fixups and logic
simplifications in too.


diff --git a/drivers/clk/qcom/clk-rcg2.c b/drivers/clk/qcom/clk-rcg2.c
index 6827ab32c82c..e63911cbfef9 100644
--- a/drivers/clk/qcom/clk-rcg2.c
+++ b/drivers/clk/qcom/clk-rcg2.c
@@ -797,7 +797,6 @@ static int clk_rcg2_set_force_enable(struct clk_hw *hw)
 	const char *name = clk_hw_get_name(hw);
 	int ret, count;
 
-	/* Force enable bit */
 	ret = regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG,
 				 CMD_ROOT_EN, CMD_ROOT_EN);
 	if (ret)
@@ -808,12 +807,10 @@ static int clk_rcg2_set_force_enable(struct clk_hw *hw)
 		if (clk_rcg2_is_enabled(hw))
 			return 0;
 
-		/* Delay for 1usec and retry polling the status bit */
 		udelay(1);
 	}
-	if (!count)
-		pr_err("%s: RCG did not turn on\n", name);
 
+	pr_err("%s: RCG did not turn on\n", name);
 	return -ETIMEDOUT;
 }
 
@@ -821,7 +818,6 @@ static int clk_rcg2_clear_force_enable(struct clk_hw *hw)
 {
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 
-	/* Clear force enable bit */
 	return regmap_update_bits(rcg->clkr.regmap, rcg->cmd_rcgr + CMD_REG,
 					CMD_ROOT_EN, 0);
 }
@@ -832,9 +828,6 @@ clk_rcg2_shared_force_enable_clear(struct clk_hw *hw, const struct freq_tbl *f)
 	struct clk_rcg2 *rcg = to_clk_rcg2(hw);
 	int ret;
 
-	if (!f)
-		return -EINVAL;
-
 	ret = clk_rcg2_set_force_enable(hw);
 	if (ret)
 		return ret;
@@ -858,10 +851,9 @@ static int clk_rcg2_shared_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	/*
 	 * In case clock is disabled, update the CFG, M, N and D registers
-	 * and do not hit the update bit of CMD register.
+	 * and don't hit the update bit of CMD register.
 	 */
 	if (!__clk_is_enabled(hw->clk))
-		/* Skip the configuration update */
 		return __clk_rcg2_configure(rcg, f);
 
 	return clk_rcg2_shared_force_enable_clear(hw, f);
@@ -879,8 +871,8 @@ static int clk_rcg2_shared_enable(struct clk_hw *hw)
 	int ret;
 
 	/*
-	 * Set the update bit only.
-	 * As required configuration has been already written in set_rate() op.
+	 * Set the update bit because required configuration has already
+	 * been written in set_rate() op.
 	 */
 	ret = clk_rcg2_set_force_enable(hw);
 	if (ret)
@@ -899,8 +891,8 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
 	u32 cfg;
 
 	/*
-	 * Store current configuration as switching to safe source
-	 * would clear the SRC and DIV of CFG register.
+	 * Store current configuration as switching to safe source would clear
+	 * the SRC and DIV of CFG register.
 	 */
 	regmap_read(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG, &cfg);
 
@@ -915,7 +907,7 @@ static void clk_rcg2_shared_disable(struct clk_hw *hw)
 	clk_rcg2_set_force_enable(hw);
 
 	regmap_write(rcg->clkr.regmap, rcg->cmd_rcgr + CFG_REG,
-				rcg->safe_src_index << CFG_SRC_SEL_SHIFT);
+		     rcg->safe_src_index << CFG_SRC_SEL_SHIFT);
 
 	update_config(rcg);