linux-kernel - Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

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] [day] [month] [year] [list]

Date:   Tue, 24 Apr 2018 08:40:09 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...eaurora.org>,
        Taniya Das <tdas@...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, David Collins <collinsd@...eaurora.org>
Subject: Re: [PATCH v3 2/2] clk: qcom: clk-rpmh: Add QCOM RPMh clock driver

Quoting Taniya Das (2018-04-23 09:50:22)
> On 4/16/2018 11:08 PM, Stephen Boyd wrote:
> > Quoting Taniya Das (2018-04-13 19:36:41)
> 
> >> +struct clk_rpmh {
> >> +       struct clk_hw hw;
> >> +       const char *res_name;
> >> +       u32 res_addr;
> >> +       u32 res_en_offset;
> > 
> > Why do we store both res_addr and res_en_offset? Can't we just store
> > res_en_offset and then use that all the time? I don't see a user of
> > res_addr anywhere.
> > 
> 
> The res_addr would be the address for the resource returned by the 
> cmd_db_read_addr. And the res_en_offset would be the offsets of ARC_EN 
> or VRM_EN.

Yes. But why can't we store the combination of the two?

> 
> >> +       u32 res_on_val;
> >> +       u32 res_off_val;
> > 
> > Is this used?
> 
> Yes the above are used.

I just meant res_off_val. Which looks unused.

> > 
> >> +       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;
> >> +};
> > 
> > Can you add some kernel-doc on these structure members?
> > 
> Sure will add the same.

Great! Hopefully that clarifies things.

> >> +       /*
> >> +        * RPMh clocks have a fixed rate. Return static rate set
> >> +        * at init time.
> >> +        */
> >> +       return r->rate;
> > 
> > The rate should come from the parent. In the case of tcxo it would be
> > board_xo clk rate (or maybe some fixed div-2 on the board XO that's also
> > defined in DT because the board_xo seems to be two times 19.2 MHz?).
> > 
> 
> There would not be any parent for the RPMH clock, they would be the 
> parent for other clocks.
> 
> The TCXO is 19.2MHz and once we have the RPMH clocks, we would remove 
> the DT reference for board_xo.

No that's wrong. There is a parent of the RPMh clks, and that's the
board XO clk in the DT file. We will never remove the board clks.