From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dwivedi, Avaneesh Kumar (avani)" Subject: Re: [RESEND PATCH v4 1/7] remoteproc: qcom: Add and initialize private data for hexagon dsp. Date: Fri, 9 Dec 2016 17:12:13 +0530 Message-ID: References: <1479981638-32069-1-git-send-email-akdwived@codeaurora.org> <1479981638-32069-2-git-send-email-akdwived@codeaurora.org> <20161208213713.GO30492@tuxbot> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:49616 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752644AbcLILmT (ORCPT ); Fri, 9 Dec 2016 06:42:19 -0500 In-Reply-To: <20161208213713.GO30492@tuxbot> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Bjorn Andersson Cc: sboyd@codeaurora.org, agross@codeaurora.org, linux-arm-msm@vger.kernel.org On 12/9/2016 3:07 AM, Bjorn Andersson wrote: > On Thu 24 Nov 02:00 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> Resource string's specific to version of hexagon chip are initialized >> to be passed to probe for various resource init purpose. >> Also compatible string introduced are added to documentation. >> > Overall I think the series looks good now, will comment on individual > things of each patch, but I'm generally happy about how things look. Thanks Bjorn for reviewing, > > > I would however like to see a slight restructuring between the patches. ok, sure. > > Rather than adding regulators, clocks and version to the res-struct in > patch 1 and then add the code using these later I would like you to > introduce the smallest possible struct here and then add each part in > the relevant patch. And finish off with adding the msm8996 compatible, > once all the pieces are in place. ok. > > So in this first patch I would suggest that you add the msm8974 and > msm8916 compatibles, a res-struct containing only hexagon_mba_image and > the change from patch 2 where you change rproc_alloc() to use the > hexagon_mba_image. ok. so i am going to add additional compatible string while also keeping existing compatible as below. Also compatible string i have changed but is it OK to keep resource instance named as "qdsp6v5_5_0_0_res" as below? +static const struct rproc_hexagon_res qdsp6v5_5_0_0_res = { + .hexagon_mba_image = "mba.mbn", +}; +static const struct rproc_hexagon_res qdsp6v5_5_1_1_res = { + .hexagon_mba_image = "mba.b00", +}; static const struct of_device_id q6v5_of_match[] = { - { .compatible = "qcom,q6v5-pil", }, + { .compatible = "qcom,q6v5-pil", .data = &qdsp6v5_5_0_0_res}, + { .compatible = "qcom,msm8916-mss-pil", .data = &qdsp6v5_5_0_0_res}, + { .compatible = "qcom,msm8974-mss-pil", .data = &qdsp6v5_5_1_1_res}, { }, }; > > Then in the regulator patch add the regulators for msm8974 and msm8916, > same with clocks in the clock patch and then add the hexagon_ver, the > associated changes and the msm8996 compatible in one patch. Ok. > > That way each patch add a single consistent chunk of the changes. Sure. > >> Signed-off-by: Avaneesh Kumar Dwivedi >> --- >> .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 + >> drivers/remoteproc/qcom_q6v5_pil.c | 61 +++++++++++++++++++++- >> 2 files changed, 62 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> index 57cb49e..d4c14f0 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt >> @@ -8,6 +8,8 @@ on the Qualcomm Hexagon core. >> Value type: >> Definition: must be one of: >> "qcom,q6v5-pil" >> + "qcom,q6v5-5-1-1-pil" >> + "qcom,q6v56-1-5-0-pil" > The more I look at these numbers and what's used downstream the more > confused I get and perhaps more important, I can't find any > documentation mentioning these numbers. These versions which i adopted here are as per HPG documents. There was some inconsistency in using the exact version of hexagon chip for compatible string purpose in downstream. If one look documentation in downstream similar naming convention is adopted to indicate version though compatible string is still not in conformance. for example Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v56-1-3: Boolean- Present if the qdsp version is v56 1.3====> applicable for mdm9640 platform Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v56-1-5: Boolean- Present if the qdsp version is v56 1.5 ===> applicable for msm8996 platform Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v56-1-8: Boolean- Present if the qdsp version is v56 1.8 ===> applicable for msm8952 and msm8940 platforms. Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v56-1-10: Boolean- Present if the qdsp version is v56 1.10===>applicable for msm8953 platform Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v61-1-1: Boolean- Present if the qdsp version is v61 1.1 Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v62-1-2: Boolean- Present if the qdsp version is v62 1.2====>applicable for msm8998 Documentation/devicetree/bindings/pil/pil-q6v5-mss.txt:- qcom,qdsp6v62-1-5: Boolean- Present if the qdsp version is v62 1.5 > > As far as I can see these numbers are 1:1 with specific platforms, which > we use as part of other bindings. So I think we should follow the naming > scheme we use for e.g. the ADSP PIL. In very few cases same hexagon chip is used in more than one msm platform i.e. one-to-many example q6v56 1.8 is used on msm8952 as well as on msm8940, but generally it is one to one mapping with msm platform. > > And let's replace the q6v5 part with "mss", as e.g. msm8974 adsp also is > a "q6v5". > > So please add: > "qcom,msm8916-mss-pil", > "qcom,msm8974-mss-pil", > "qcom,msm8996-mss-pil" OK. > > The compatible "qcom,q6v5-pil" is already introduced in the > msm8916.dtsi, so make that compatible be equivalent to > "qcom,msm8916-mss-pil" (but let's have both for clarity). so i will keep "qcom,msm8916-mss-pil" as well as "qcom,q6v5-pil" both in code. > >> >> - reg: >> Usage: required >> diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c >> index 2e0caaa..3360312 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pil.c >> +++ b/drivers/remoteproc/qcom_q6v5_pil.c >> @@ -30,6 +30,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "remoteproc_internal.h" >> #include "qcom_mdt_loader.h" >> @@ -93,6 +94,22 @@ >> #define QDSS_BHS_ON BIT(21) >> #define QDSS_LDO_BYP BIT(22) >> >> +struct rproc_hexagon_res { >> + char *hexagon_mba_image; > const OK. > >> + char **proxy_reg_string; >> + char **active_reg_string; >> + int proxy_uV_uA[3][2]; >> + int active_uV_uA[1][2]; > Let's group these into a: > > struct qcom_mss_reg_res { > const char *supply; > int uA; > int uV; > }; > > Then the res definitions below becomes: > > satic const struct rproc_hexagon_res msm8916_res = { > .proxy_regs = (struct qcom_mss_reg_res[]) { > { > .supply = "mx", > }, > { > .supply = "cx", > .uA = 100000, > }, > { > .supply = "pll", > .uA = 100000, > }, > { NULL } > }, > ... > }; Ok, Sure. > >> + char **proxy_clk_string; >> + char **active_clk_string; >> + int hexagon_ver; >> +}; >> + >> +struct reg_info { >> + struct regulator *reg; >> + int uV; >> + int uA; >> +}; >> struct q6v5 { >> struct device *dev; >> struct rproc *rproc; >> @@ -131,6 +148,12 @@ struct q6v5 { >> }; >> >> enum { >> + Q6V5_5_0_0, /*hexagon on msm8916*/ >> + Q6V5_5_1_1, /*hexagon on msm8974*/ >> + Q5V56_1_5_0, /*hexagon on msm8996*/ > As I said above, this turns out to be confusing. Name them based on > platform instead. Something like Q6V5_MSM8916 Sure. > >> +}; >> + > Regards, > Bjorn -- Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.