| Message ID | 1479981638-32069-2-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
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. I would however like to see a slight restructuring between the patches. 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. 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. 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. That way each patch add a single consistent chunk of the changes. > Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> > --- > .../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: <string> > 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. 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. 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" 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). > > - 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 <linux/reset.h> > #include <linux/soc/qcom/smem.h> > #include <linux/soc/qcom/smem_state.h> > +#include <linux/of_device.h> > > #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 > + 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 } }, ... }; > + 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 > +}; > + Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <akdwived@codeaurora.org> >> --- >> .../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: <string> >> 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 <linux/reset.h> >> #include <linux/soc/qcom/smem.h> >> #include <linux/soc/qcom/smem_state.h> >> +#include <linux/of_device.h> >> >> #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
On Fri 09 Dec 03:42 PST 2016, Dwivedi, Avaneesh Kumar (avani) wrote: > >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. Yes, we need to keep the qcom,q6v5-pil and your change below looks to get it right. > Also compatible string i have changed but is it OK to keep > resource instance named as "qdsp6v5_5_0_0_res" as below? > Please update their naming based on the platform name as well, it will cause less confusion in the future. > +static const struct rproc_hexagon_res qdsp6v5_5_0_0_res = { > + .hexagon_mba_image = "mba.mbn", > +}; A single empty line between each struct makes things easier to read. > +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}, > { }, > }; [..] > >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. Okay, thanks for the summary. In these rare cases we can have two compatibles referencing the same struct. > > > >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. Sound good. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: <string> Definition: must be one of: "qcom,q6v5-pil" + "qcom,q6v5-5-1-1-pil" + "qcom,q6v56-1-5-0-pil" - 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 <linux/reset.h> #include <linux/soc/qcom/smem.h> #include <linux/soc/qcom/smem_state.h> +#include <linux/of_device.h> #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; + char **proxy_reg_string; + char **active_reg_string; + int proxy_uV_uA[3][2]; + int active_uV_uA[1][2]; + 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*/ +}; + +enum { Q6V5_SUPPLY_CX, Q6V5_SUPPLY_MX, Q6V5_SUPPLY_MSS, @@ -890,8 +913,44 @@ static int q6v5_remove(struct platform_device *pdev) return 0; } +static const struct rproc_hexagon_res q6v56_1_5_0_res = { + .hexagon_mba_image = "mba.mbn", + .proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL}, + .active_reg_string = NULL, + .proxy_uV_uA = { {0, 0}, {0, 100000}, {0, 100000} }, + .active_uV_uA = { {0} }, + .proxy_clk_string = (char*[]){"xo", "pnoc", "qdss", NULL}, + .active_clk_string = (char*[]){"iface", "bus", "mem", + "gpll0_mss_clk", "snoc_axi_clk", "mnoc_axi_clk", NULL}, + .hexagon_ver = Q5V56_1_5_0, +}; + +static const struct rproc_hexagon_res q6v5_5_0_0_res = { + .hexagon_mba_image = "mba.mbn", + .proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL}, + .proxy_uV_uA = { {1050000, 0}, {0, 100000}, {0, 100000} }, + .active_reg_string = (char*[]){"mss", NULL}, + .active_uV_uA = { {1000000, 100000} }, + .proxy_clk_string = (char*[]){"xo", NULL}, + .active_clk_string = (char*[]){"iface", "bus", "mem", NULL}, + .hexagon_ver = Q6V5_5_0_0, +}; + +static const struct rproc_hexagon_res q6v5_5_1_1_res = { + .hexagon_mba_image = "mba.b00", + .proxy_reg_string = (char*[]){"mx", "cx", "pll", NULL}, + .proxy_uV_uA = { {1050000, 0}, {0, 100000}, {0, 100000} }, + .active_reg_string = (char*[]){"mss", NULL}, + .active_uV_uA = { {1000000, 100000} }, + .proxy_clk_string = (char*[]){"xo", NULL}, + .active_clk_string = (char*[]){"iface", "bus", "mem", NULL}, + .hexagon_ver = Q6V5_5_1_1, +}; + static const struct of_device_id q6v5_of_match[] = { - { .compatible = "qcom,q6v5-pil", }, + { .compatible = "qcom,q6v5-pil", .data = &q6v5_5_0_0_res}, + { .compatible = "qcom,q6v5-5-1-1-pil", .data = &q6v5_5_1_1_res}, + { .compatible = "qcom,q6v56-1-5-0-pil", .data = &q6v56_1_5_0_res}, { }, };
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. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 2 + drivers/remoteproc/qcom_q6v5_pil.c | 61 +++++++++++++++++++++- 2 files changed, 62 insertions(+), 1 deletion(-)