| 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
| ||
|
Date: Tue, 1 May 2018 14:10:52 +0530
From: Taniya Das <tdas@...eaurora.org>
To: Stephen Boyd <sboyd@...nel.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>,
Amit Nischal <anischal@...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
Subject: Re: [v4 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver
Hello Stephen,
Thanks for the review comments.
On 4/27/2018 5:10 AM, Stephen Boyd wrote:
> Quoting Taniya Das (2018-04-24 05:23:19)
>> diff --git a/drivers/clk/qcom/clk-rpmh.c b/drivers/clk/qcom/clk-rpmh.c
>> new file mode 100644
>> index 0000000..907a73f
>> --- /dev/null
>> +++ b/drivers/clk/qcom/clk-rpmh.c
>> @@ -0,0 +1,364 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk-provider.h>
>> +#include <linux/err.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <soc/qcom/cmd-db.h>
>> +#include <soc/qcom/rpmh.h>
>> +
>> +#include <dt-bindings/clock/qcom,rpmh.h>
>> +
>> +#define CLK_RPMH_ARC_EN_OFFSET 0
>> +#define CLK_RPMH_VRM_EN_OFFSET 4
>> +
>> +/**
>> + * struct clk_rpmh - individual rpmh clock data structure
>> + * @hw: handle between common and hardware-specific interfaces
>> + * @res_name: resource name for the rpmh clock
>> + * @res_addr: base address of the rpmh resource within the RPMh
>> + * @res_en_offset: clock resource enable offset corresponding to ARC or
>> + * VRM type clock
>
> Can these be combined still? Just do a += on res_addr after we get the
> base of it?
>
I have taken care in the next patch.
>> + * @res_on_val: rpmh clock enable value
>> + * @res_off_val: rpmh clock disable value
>> + * @state: rpmh clock requested state
>> + * @aggr_state: rpmh clock aggregated state
>> + * @last_sent_aggr_state: rpmh clock last aggr state sent to RPMh
>> + * @valid_state_mask: mask to determine the state of the rpmh clock
>> + * @rpmh_client: handle used for rpmh communications
>> + * @dev: device to which it is attached
>> + * @peer: pointer to the clock rpmh sibling
>> + * @rate: rate of the rpmh clock
>> + */
>> +struct clk_rpmh {
>> + struct clk_hw hw;
>> + const char *res_name;
>> + u32 res_addr;
>> + u32 res_en_offset;
>> + u32 res_on_val;
>> + u32 res_off_val;
>> + u32 state;
>> + u32 aggr_state;
>> + u32 last_sent_aggr_state;
>> + u32 valid_state_mask;
>> + struct rpmh_client *rpmh_client;
>> + struct device *dev;
>> + struct clk_rpmh *peer;
>> + unsigned long rate;
>> +};
> [...]
>> +
>> +static inline bool has_state_changed(struct clk_rpmh *c, u32 state)
>> +{
>> + return (c->last_sent_aggr_state & BIT(state))
>> + != (c->aggr_state & BIT(state));
>
> Please drop useless parenthesis.
>
I see a warning in case I remove the parenthesis.
warning: suggest parentheses around comparison in operand of ‘&’
[-Wparentheses]
!= c->aggr_state & BIT(state));
>> +}
>> +
>> +static int clk_rpmh_send_aggregate_command(struct clk_rpmh *c)
>> +{
>> + struct tcs_cmd cmd = { 0 };
>> + u32 cmd_state, on_val;
>> + enum rpmh_state state = RPMH_SLEEP_STATE;
>> + int ret;
>> +
>> + cmd.addr = c->res_addr + c->res_en_offset;
>> + cmd_state = c->aggr_state;
>> + on_val = c->res_on_val;
>> +
>> + for (; state <= RPMH_ACTIVE_ONLY_STATE; state++) {
>> + if (has_state_changed(c, state)) {
>> + cmd.data = (cmd_state & BIT(state)) ? on_val : 0;
>> + ret = rpmh_write_async(c->rpmh_client, state,
>> + &cmd, 1);
>> + if (ret) {
>> + dev_err(c->dev, "set %s state of %s failed: (%d)\n",
>> + (!state) ? "sleep" :
>
> Drop parenthesis please.
>
Dropped.
>> + (state == RPMH_WAKE_ONLY_STATE) ?
>
> Drop parenthesis please.
>
Dropped.
>> + "wake" : "active", c->res_name, ret);
>> + return ret;
>> + }
>> + }
>> + }
>> +
>> + c->last_sent_aggr_state = c->aggr_state;
>> + c->peer->last_sent_aggr_state = c->last_sent_aggr_state;
>> +
>> + return 0;
>> +}
> [...]
>> +
>> +static const struct clk_ops clk_rpmh_ops = {
>> + .prepare = clk_rpmh_prepare,
>> + .unprepare = clk_rpmh_unprepare,
>> + .recalc_rate = clk_rpmh_recalc_rate,
>> +};
>> +
>> +/* Resource name must match resource id present in cmd-db. */
>> +DEFINE_CLK_RPMH_ARC(sdm845, bi_tcxo, bi_tcxo_ao, "xo.lvl", 0x3, 0x0, 19200000);
>> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk2, ln_bb_clk2_ao, "lnbclka2", 19200000);
>> +DEFINE_CLK_RPMH_VRM(sdm845, ln_bb_clk3, ln_bb_clk3_ao, "lnbclka3", 19200000);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 38400000);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 38400000);
>> +DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 38400000);
>
> These rates should come from DT and parents of these clks.
>
Removed the rates from the above and instead I would take the clk-div
values from the DT for the clocks and compute the rates and parents are
fixed to "xo_board".
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation.
--
Powered by blists - more mailing lists