Re: [PATCH v1] clk: qcom: clk-rpmh: Add IPA clock support

From: Stephen Boyd <sboyd@kernel.org>
To: David Dai <daidavid1@codeaurora.org>,
	linux-arm-msm@vger.kernel.org, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org
Cc: georgi.djakov@linaro.org, bjorn.andersson@linaro.org,
	evgreen@google.com, tdas@codeaurora.org, elder@linaro.org
Subject: Re: [PATCH v1] clk: qcom: clk-rpmh: Add IPA clock support
Date: Mon, 14 Jan 2019 08:47:02 -0800	[thread overview]
Message-ID: <154748442276.169631.994019688641603420@swboyd.mtv.corp.google.com> (raw)
In-Reply-To: <11f948dc-3045-762f-b6bf-63c32d51772a@codeaurora.org>

Quoting David Dai (2019-01-11 16:56:14)
> 
> On 1/9/2019 11:28 AM, Stephen Boyd wrote:
> > Quoting David Dai (2018-12-13 18:35:04)
> >> +
> >> +#define BCM_TCS_CMD(valid, vote)                               \
> >> +       (BCM_TCS_CMD_COMMIT_MASK |                              \
> >> +       ((valid) << BCM_TCS_CMD_VALID_SHIFT) |                  \
> >> +       ((cpu_to_le32(vote) &                                   \
> > Why?
> Sorry, could you clarify this question? If you're referring to the 
> cpu_to_le32, shouldn't we be explicit about converting endianness even 
> if it might be redundant for this particular qcom platform?

Is only the vote part of the message in little endian format and the
rest is native CPU endianess? It's very odd to see that jammed in the
middle of a bit packing statement like that. Typically the whole u32
would be in one or the other endianness. Also, sparse right complains
about this macro and it's usage, so something is wrong.

I think one other problem is that rpmh API doesn't really talk about
endianness here and that's busted. So if you want to fix endianness
issues that needs to be fixed first.

> >> @@ -29,6 +54,7 @@
> >>    * @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
> >> + * @aux_data:          data specific to the bcm rpmh resource
> > But there isn't aux_data. Is this supposed to be unit? And what is unit
> > going to be? 1000?
> Supposed to be unit, the value is on the order of Khz.

Ok, hopefully the kernel-doc comes out easy to understand.

> >> @@ -217,6 +340,7 @@ static unsigned long clk_rpmh_recalc_rate(struct clk_hw *hw,
> >>   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk1, rf_clk1_ao, "rfclka1", 1);
> >>   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk2, rf_clk2_ao, "rfclka2", 1);
> >>   DEFINE_CLK_RPMH_VRM(sdm845, rf_clk3, rf_clk3_ao, "rfclka3", 1);
> >> +DEFINE_CLK_RPMH_BCM(sdm845, ipa, "IP0");
> > It's really IP0?! What was wrong with IPA?
> Yeah... convention seems to be 2 letters and then a number for most BCM 
> resources.

OK.