| Message ID | 1478522240-11036-2-git-send-email-akdwived@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote: > Embed resources specific to version of hexagon chip in device > structure to avoid conditional check for manipulation of those > resources in driver code. > Please don't rename functions in the same patch as functional changes. [..] > > -static int q6v5_probe(struct platform_device *pdev) > +static int q6_probe(struct platform_device *pdev) > { > struct q6v5 *qproc; > struct rproc *rproc; > + const struct q6_rproc_res *desc; > int ret; > > - rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, > - MBA_FIRMWARE_NAME, sizeof(*qproc)); > + desc = of_device_get_match_data(&pdev->dev); > + if (!desc) > + return -EINVAL; > + > + rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops, If you didn't rename q6v5_ops the patch would look cleaner. > + desc->q6_mba_image, sizeof(*qproc)); > if (!rproc) { > dev_err(&pdev->dev, "failed to allocate rproc\n"); > return -ENOMEM; > } > > - rproc->fw_ops = &q6v5_fw_ops; > + rproc->fw_ops = &q6_fw_ops; > > qproc = (struct q6v5 *)rproc->priv; > qproc->dev = &pdev->dev; > @@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev) > init_completion(&qproc->start_done); > init_completion(&qproc->stop_done); > > + qproc->q6_rproc_res = desc; > ret = q6v5_init_mem(qproc, pdev); > if (ret) > goto free_rproc; > @@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev) > if (ret) > goto free_rproc; > > - ret = q6v5_init_reset(qproc); > + ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev); > if (ret) > goto free_rproc; > > @@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev) > goto free_rproc; > > return 0; > - Please don't do "random" cleanups. > free_rproc: > rproc_free(rproc); > - > return ret; > } > > -static int q6v5_remove(struct platform_device *pdev) > +static int q6_remove(struct platform_device *pdev) > { > struct q6v5 *qproc = platform_get_drvdata(pdev); > > @@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev) > return 0; > } > > -static const struct of_device_id q6v5_of_match[] = { > - { .compatible = "qcom,q6v5-pil", }, > +char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"}; This list is the same in both versions, so no need to define it here. > +int proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} }; This is just a magic matrix, please use a struct with some descriptive names for these. I presume they represents "matching index in reg_load and reg_min_voltage is non-zero and should be used" - in which case it shouldn't be needed at all. > +int proxy_8x96_reg_load[] = {0, 100000, 100000}; > +int proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0}; > +char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"}; > +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk", > + "snoc_axi_clk", "mnoc_axi_clk"}; All these needs to be static, but I would prefer if you just put the values directly into the resource structs below. > + > +static const struct q6_rproc_res msm_8996_res = { > + .proxy_clks = proxy_8x96_clk_str, > + .proxy_clk_cnt = 3, It's common practice to use a NULL terminator in the definition list rather than keeping separate count of the number of items. While acquiring the resources you would have to "calculate" the number and store it in the q6v5 struct, but this would make turn this struct into something only used during probe() - which is nice. > + .active_clks = active_8x96_clk_str, > + .active_clk_cnt = 6, > + .proxy_regs = proxy_8x96_reg_str, > + .active_regs = NULL, > + .proxy_reg_action = (int **)proxy_8x96_reg_action, > + .proxy_reg_load = (int *)proxy_8x96_reg_load, > + .active_reg_action = NULL, > + .active_reg_load = NULL, > + .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage, > + .active_reg_voltage = NULL, > + .proxy_reg_cnt = 3, > + .active_reg_cnt = 0, > + .q6_reset_init = q6v56_init_reset, > + .q6_version = "v56", q6_version would be better to have as a enum. > + .q6_mba_image = "mba.mbn", > +}; > + > +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"}; > +char *active_8x16_reg_str[] = {"mss"}; > +int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} }; > +int active_8x16_reg_action[1][2] = { {1, 1} }; > +int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000}; > +int active_8x16_reg_load[] = {100000}; > +int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0}; > +int active_8x16_reg_min_voltage[] = {1000000}; > +char *proxy_8x16_clk_str[] = {"xo"}; > +char *active_8x16_clk_str[] = {"iface", "bus", "mem"}; > + > +static const struct q6_rproc_res msm_8916_res = { > + .proxy_clks = proxy_8x16_clk_str, > + .proxy_clk_cnt = 1, > + .active_clks = active_8x16_clk_str, > + .active_clk_cnt = 3, > + .proxy_regs = proxy_8x16_reg_str, > + .active_regs = active_8x16_reg_str, > + .proxy_reg_action = (int **)proxy_8x16_reg_action, > + .proxy_reg_load = (int *)proxy_8x16_reg_load, > + .active_reg_action = (int **)active_8x16_reg_action, > + .active_reg_load = (int *)active_8x16_reg_load, > + .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage, > + .active_reg_voltage = active_8x16_reg_min_voltage, > + .proxy_reg_cnt = 3, > + .active_reg_cnt = 1, > + .q6_reset_init = q6v5_init_reset, > + .q6_version = "v5", > + .q6_mba_image = "mba.b00", q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00. > +}; > + > +static const struct of_device_id q6_of_match[] = { > + { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res}, As I was looking in the CAF tree, I believe the correct version for 8916 is 5.5 and v5 was used in 8974. But I presume these versions are not strictly tied to certain platforms, so please name the resource structs based on the q6v5 version, rather than platforms. > + { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res}, > { }, > }; > > -static struct platform_driver q6v5_driver = { > - .probe = q6v5_probe, > - .remove = q6v5_remove, > +static struct platform_driver q6_driver = { > + .probe = q6_probe, > + .remove = q6_remove, > .driver = { > .name = "qcom-q6v5-pil", > - .of_match_table = q6v5_of_match, > + .of_match_table = q6_of_match, > }, > }; > -module_platform_driver(q6v5_driver); > +module_platform_driver(q6_driver); > Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
i have been a little delayed for posting replies to patch comments, hopefully onward it should be quick and fast. On 11/8/2016 12:38 PM, Bjorn Andersson wrote: > On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> Embed resources specific to version of hexagon chip in device >> structure to avoid conditional check for manipulation of those >> resources in driver code. >> > Please don't rename functions in the same patch as functional changes. have corrected > > [..] >> >> -static int q6v5_probe(struct platform_device *pdev) >> +static int q6_probe(struct platform_device *pdev) >> { >> struct q6v5 *qproc; >> struct rproc *rproc; >> + const struct q6_rproc_res *desc; >> int ret; >> >> - rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, >> - MBA_FIRMWARE_NAME, sizeof(*qproc)); >> + desc = of_device_get_match_data(&pdev->dev); >> + if (!desc) >> + return -EINVAL; >> + >> + rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops, > If you didn't rename q6v5_ops the patch would look cleaner. Yes, corrected > >> + desc->q6_mba_image, sizeof(*qproc)); >> if (!rproc) { >> dev_err(&pdev->dev, "failed to allocate rproc\n"); >> return -ENOMEM; >> } >> >> - rproc->fw_ops = &q6v5_fw_ops; >> + rproc->fw_ops = &q6_fw_ops; >> >> qproc = (struct q6v5 *)rproc->priv; >> qproc->dev = &pdev->dev; >> @@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev) >> init_completion(&qproc->start_done); >> init_completion(&qproc->stop_done); >> >> + qproc->q6_rproc_res = desc; >> ret = q6v5_init_mem(qproc, pdev); >> if (ret) >> goto free_rproc; >> @@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev) >> if (ret) >> goto free_rproc; >> >> - ret = q6v5_init_reset(qproc); >> + ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev); >> if (ret) >> goto free_rproc; >> >> @@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev) >> goto free_rproc; >> >> return 0; >> - > Please don't do "random" cleanups. Sure, Corrected. > >> free_rproc: >> rproc_free(rproc); >> - >> return ret; >> } >> >> -static int q6v5_remove(struct platform_device *pdev) >> +static int q6_remove(struct platform_device *pdev) >> { >> struct q6v5 *qproc = platform_get_drvdata(pdev); >> >> @@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev) >> return 0; >> } >> >> -static const struct of_device_id q6v5_of_match[] = { >> - { .compatible = "qcom,q6v5-pil", }, >> +char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"}; > This list is the same in both versions, so no need to define it here. Yes, have reused. > >> +int proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} }; > This is just a magic matrix, please use a struct with some descriptive > names for these. I presume they represents "matching index in reg_load > and reg_min_voltage is non-zero and should be used" - in which case it > shouldn't be needed at all. Yes this represented the flag to set voltage and load. Have removed this and setting voltage and load now depending upon specified voltage and load setting for regulators. > >> +int proxy_8x96_reg_load[] = {0, 100000, 100000}; >> +int proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0}; >> +char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"}; >> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk", >> + "snoc_axi_clk", "mnoc_axi_clk"}; > All these needs to be static, but I would prefer if you just put the > values directly into the resource structs below. If i have to initialize directly into resource struct, i need to declare individual elements as array of fixed size but number of resource lets say number of proxy regulator not being same for different q6 chips, made me to work with double pointer which can be assigned with address of an array of string pointer as per need when new version need to be supported. Though i have made above elements as static in next patch. > >> + >> +static const struct q6_rproc_res msm_8996_res = { >> + .proxy_clks = proxy_8x96_clk_str, >> + .proxy_clk_cnt = 3, > It's common practice to use a NULL terminator in the definition list > rather than keeping separate count of the number of items. > > While acquiring the resources you would have to "calculate" the number > and store it in the q6v5 struct, but this would make turn this struct > into something only used during probe() - which is nice. Yes, have modified as per suggestion. > >> + .active_clks = active_8x96_clk_str, >> + .active_clk_cnt = 6, >> + .proxy_regs = proxy_8x96_reg_str, >> + .active_regs = NULL, >> + .proxy_reg_action = (int **)proxy_8x96_reg_action, >> + .proxy_reg_load = (int *)proxy_8x96_reg_load, >> + .active_reg_action = NULL, >> + .active_reg_load = NULL, >> + .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage, >> + .active_reg_voltage = NULL, >> + .proxy_reg_cnt = 3, >> + .active_reg_cnt = 0, >> + .q6_reset_init = q6v56_init_reset, >> + .q6_version = "v56", > q6_version would be better to have as a enum. have removed this entry. each class of q6 lets say "v56" have again many version of hexagon with minor differences wrt each other. for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence and some other operation differ wrt to this version in terms of order of register programming. so i have introduced one variable in q6v5 struct per q6 chip supported, if this is defined then we can check and carry out version specific instruction. will this be OK? > >> + .q6_mba_image = "mba.mbn", >> +}; >> + >> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"}; >> +char *active_8x16_reg_str[] = {"mss"}; >> +int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} }; >> +int active_8x16_reg_action[1][2] = { {1, 1} }; >> +int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000}; >> +int active_8x16_reg_load[] = {100000}; >> +int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0}; >> +int active_8x16_reg_min_voltage[] = {1000000}; >> +char *proxy_8x16_clk_str[] = {"xo"}; >> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"}; >> + >> +static const struct q6_rproc_res msm_8916_res = { >> + .proxy_clks = proxy_8x16_clk_str, >> + .proxy_clk_cnt = 1, >> + .active_clks = active_8x16_clk_str, >> + .active_clk_cnt = 3, >> + .proxy_regs = proxy_8x16_reg_str, >> + .active_regs = active_8x16_reg_str, >> + .proxy_reg_action = (int **)proxy_8x16_reg_action, >> + .proxy_reg_load = (int *)proxy_8x16_reg_load, >> + .active_reg_action = (int **)active_8x16_reg_action, >> + .active_reg_load = (int *)active_8x16_reg_load, >> + .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage, >> + .active_reg_voltage = active_8x16_reg_min_voltage, >> + .proxy_reg_cnt = 3, >> + .active_reg_cnt = 1, >> + .q6_reset_init = q6v5_init_reset, >> + .q6_version = "v5", >> + .q6_mba_image = "mba.b00", > q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00. Again this is a case where compatible string can not help, we should have single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are from same class of q6 chip with different version. so if we cant initialize mdt name based on compatible string alone. > >> +}; >> + >> +static const struct of_device_id q6_of_match[] = { >> + { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res}, > As I was looking in the CAF tree, I believe the correct version for 8916 > is 5.5 and v5 was used in 8974. > > But I presume these versions are not strictly tied to certain platforms, > so please name the resource structs based on the q6v5 version, rather > than platforms. Yes, resource are tied to compatible and version of q6. have used compatible to initialize major resources but using DT specified version string for minor deviations where needed. Should this be fine? as far as version on 8916 and 8974 are concern they are as below. msm8974 q6v5 core version 5.0.0 msm8916 q6v5 core version 5.1.1 > >> + { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res}, >> { }, >> }; >> >> -static struct platform_driver q6v5_driver = { >> - .probe = q6v5_probe, >> - .remove = q6v5_remove, >> +static struct platform_driver q6_driver = { >> + .probe = q6_probe, >> + .remove = q6_remove, >> .driver = { >> .name = "qcom-q6v5-pil", >> - .of_match_table = q6v5_of_match, >> + .of_match_table = q6_of_match, >> }, >> }; >> -module_platform_driver(q6v5_driver); >> +module_platform_driver(q6_driver); >> > Regards, > Bjorn > -- > To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed 16 Nov 06:41 PST 2016, Avaneesh Kumar Dwivedi wrote: > i have been a little delayed for posting replies to patch comments, > hopefully onward it should be quick and fast. > I would greatly appreciate if you allow for a discussion before posting new revisions of the patchset. I will respond to your comments here and ignore v4 for now. > > On 11/8/2016 12:38 PM, Bjorn Andersson wrote: > >On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote: [..] > >>+char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk", > >>+ "snoc_axi_clk", "mnoc_axi_clk"}; > >All these needs to be static, but I would prefer if you just put the > >values directly into the resource structs below. > If i have to initialize directly into resource struct, i need to declare > individual elements as array of fixed size > but number of resource lets say number of proxy regulator not being same > for different q6 chips, made me to > work with double pointer which can be assigned with address of an array of > string pointer as per need when new version need to be supported. > Using a termination sentinel to indicate end of lists is quite common in the kernel, so you can do this: .active_clks = (char*[]){ "iface", "bus", ..., NULL }, > Though i have made above elements as static in next patch. > > > >>+ > >>+static const struct q6_rproc_res msm_8996_res = { > >>+ .proxy_clks = proxy_8x96_clk_str, > >>+ .proxy_clk_cnt = 3, > >It's common practice to use a NULL terminator in the definition list > >rather than keeping separate count of the number of items. > > > >While acquiring the resources you would have to "calculate" the number > >and store it in the q6v5 struct, but this would make turn this struct > >into something only used during probe() - which is nice. > > Yes, have modified as per suggestion. > > > >>+ .active_clks = active_8x96_clk_str, > >>+ .active_clk_cnt = 6, > >>+ .proxy_regs = proxy_8x96_reg_str, > >>+ .active_regs = NULL, > >>+ .proxy_reg_action = (int **)proxy_8x96_reg_action, > >>+ .proxy_reg_load = (int *)proxy_8x96_reg_load, > >>+ .active_reg_action = NULL, > >>+ .active_reg_load = NULL, > >>+ .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage, > >>+ .active_reg_voltage = NULL, > >>+ .proxy_reg_cnt = 3, > >>+ .active_reg_cnt = 0, > >>+ .q6_reset_init = q6v56_init_reset, > >>+ .q6_version = "v56", > >q6_version would be better to have as a enum. > > have removed this entry. > each class of q6 lets say "v56" have again many version of hexagon with > minor differences wrt each other. Okay, I'm fine with us sticking to classes, but I would like for them to make sense - and be listed as an enum instead of a string, to simplify the code. > for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence In msm-3.18 8996 is listed to be v55. > and some other operation differ wrt to this version in terms of order of > register programming. so i have introduced one variable in q6v5 struct per > q6 chip supported, if this is defined then we can check and carry out > version specific instruction. > will this be OK? > Generally in the Linux kernel it's frowned upon to carry the version information and then do conditional operation on this. It's preferred to carry explicit flags through the implementation, e.g. carrying "mba.mbn" vs "mba.b00" rather than switching based on "version" at the point of use of this data. But I'm not sure if the other differences has reasonable names, e.g. how to we denote the differences in reset sequence? > > > >>+ .q6_mba_image = "mba.mbn", > >>+}; > >>+ > >>+char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"}; > >>+char *active_8x16_reg_str[] = {"mss"}; > >>+int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} }; > >>+int active_8x16_reg_action[1][2] = { {1, 1} }; > >>+int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000}; > >>+int active_8x16_reg_load[] = {100000}; > >>+int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0}; > >>+int active_8x16_reg_min_voltage[] = {1000000}; > >>+char *proxy_8x16_clk_str[] = {"xo"}; > >>+char *active_8x16_clk_str[] = {"iface", "bus", "mem"}; > >>+ > >>+static const struct q6_rproc_res msm_8916_res = { > >>+ .proxy_clks = proxy_8x16_clk_str, > >>+ .proxy_clk_cnt = 1, > >>+ .active_clks = active_8x16_clk_str, > >>+ .active_clk_cnt = 3, > >>+ .proxy_regs = proxy_8x16_reg_str, > >>+ .active_regs = active_8x16_reg_str, > >>+ .proxy_reg_action = (int **)proxy_8x16_reg_action, > >>+ .proxy_reg_load = (int *)proxy_8x16_reg_load, > >>+ .active_reg_action = (int **)active_8x16_reg_action, > >>+ .active_reg_load = (int *)active_8x16_reg_load, > >>+ .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage, > >>+ .active_reg_voltage = active_8x16_reg_min_voltage, > >>+ .proxy_reg_cnt = 3, > >>+ .active_reg_cnt = 1, > >>+ .q6_reset_init = q6v5_init_reset, > >>+ .q6_version = "v5", > >>+ .q6_mba_image = "mba.b00", > >q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00. > Again this is a case where compatible string can not help, we should have > single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are > from same class of q6 chip with different version. > so if we cant initialize mdt name based on compatible string alone. msm-3.4, msm-3.10 and msm-3.18 states that they are not the same and as such I don't think we have a problem. The more I look at this, the more convinced I am that we got 8916 wrong, i.e. we specified the wrong class and it just happens to work. > > > >>+}; > >>+ > >>+static const struct of_device_id q6_of_match[] = { > >>+ { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res}, > >As I was looking in the CAF tree, I believe the correct version for 8916 > >is 5.5 and v5 was used in 8974. > > > >But I presume these versions are not strictly tied to certain platforms, > >so please name the resource structs based on the q6v5 version, rather > >than platforms. > Yes, resource are tied to compatible and version of q6. > have used compatible to initialize major resources but using DT specified > version string for minor deviations where needed. > Should this be fine? > as far as version on 8916 and 8974 are concern they are as below. > > msm8974 q6v5 core version 5.0.0 > msm8916 q6v5 core version 5.1.1 If there are differences within a class then that just forces us to use the version number. There's very little overhead in carrying one compatible per platform, if that's what we need. > > > >>+ { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res}, > >> { }, > >> }; Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 11/19/2016 12:27 AM, Bjorn Andersson wrote: > On Wed 16 Nov 06:41 PST 2016, Avaneesh Kumar Dwivedi wrote: > >> i have been a little delayed for posting replies to patch comments, >> hopefully onward it should be quick and fast. >> > I would greatly appreciate if you allow for a discussion before posting > new revisions of the patchset. I will respond to your comments here and > ignore v4 for now. Ok, i will resend v4 patches with modification after consensus. > >> On 11/8/2016 12:38 PM, Bjorn Andersson wrote: >>> On Mon 07 Nov 04:37 PST 2016, Avaneesh Kumar Dwivedi wrote: > [..] >>>> +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk", >>>> + "snoc_axi_clk", "mnoc_axi_clk"}; >>> All these needs to be static, but I would prefer if you just put the >>> values directly into the resource structs below. >> If i have to initialize directly into resource struct, i need to declare >> individual elements as array of fixed size >> but number of resource lets say number of proxy regulator not being same >> for different q6 chips, made me to >> work with double pointer which can be assigned with address of an array of >> string pointer as per need when new version need to be supported. >> > Using a termination sentinel to indicate end of lists is quite common in > the kernel, so you can do this: > > .active_clks = (char*[]){ > "iface", > "bus", > ..., > NULL > }, OK > >> Though i have made above elements as static in next patch. >>>> + >>>> +static const struct q6_rproc_res msm_8996_res = { >>>> + .proxy_clks = proxy_8x96_clk_str, >>>> + .proxy_clk_cnt = 3, >>> It's common practice to use a NULL terminator in the definition list >>> rather than keeping separate count of the number of items. >>> >>> While acquiring the resources you would have to "calculate" the number >>> and store it in the q6v5 struct, but this would make turn this struct >>> into something only used during probe() - which is nice. >> Yes, have modified as per suggestion. >>>> + .active_clks = active_8x96_clk_str, >>>> + .active_clk_cnt = 6, >>>> + .proxy_regs = proxy_8x96_reg_str, >>>> + .active_regs = NULL, >>>> + .proxy_reg_action = (int **)proxy_8x96_reg_action, >>>> + .proxy_reg_load = (int *)proxy_8x96_reg_load, >>>> + .active_reg_action = NULL, >>>> + .active_reg_load = NULL, >>>> + .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage, >>>> + .active_reg_voltage = NULL, >>>> + .proxy_reg_cnt = 3, >>>> + .active_reg_cnt = 0, >>>> + .q6_reset_init = q6v56_init_reset, >>>> + .q6_version = "v56", >>> q6_version would be better to have as a enum. >> have removed this entry. >> each class of q6 lets say "v56" have again many version of hexagon with >> minor differences wrt each other. > Okay, I'm fine with us sticking to classes, but I would like for them to > make sense - and be listed as an enum instead of a string, to simplify > the code. OK, i will use one compatible string for each msm platform with enum denoting the class and version of hexagon chip. something like .compatible = "qcom,q6v56-1-5-pil", for msm 8996? and version will carry a unique enum value based on compatible. > >> for example msm8996 use "v56" 1.5.0 while MSM8952 uses 1.8.0, reset sequence > In msm-3.18 8996 is listed to be v55. The version and class that i am referring are strictly as per HPG. for 8996, hexagon core for MSS is "v56" with version 1.5.0 > >> and some other operation differ wrt to this version in terms of order of >> register programming. so i have introduced one variable in q6v5 struct per >> q6 chip supported, if this is defined then we can check and carry out >> version specific instruction. >> will this be OK? >> > Generally in the Linux kernel it's frowned upon to carry the version > information and then do conditional operation on this. OK > > It's preferred to carry explicit flags through the implementation, e.g. > carrying "mba.mbn" vs "mba.b00" rather than switching based on "version" > at the point of use of this data. If i have one enum for each version (which will require one compatible for each platform) then i can easily pass firmware binary name to probe. > But I'm not sure if the other differences has reasonable names, e.g. how > to we denote the differences in reset sequence? i have one option that should initialize one function pointer for each compatible to perform reset sequence, but this way there will be huge duplicity of code as more than 50% code in reset sequence remain same, else based on check something like below i can perform certain unique register operations for a particular hexagon core. if (drvdata->hexagon_flag & 0x2) do this else do that where hexagon_flag will be initialized with enum based on compatible.?? > >>>> + .q6_mba_image = "mba.mbn", >>>> +}; >>>> + >>>> +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"}; >>>> +char *active_8x16_reg_str[] = {"mss"}; >>>> +int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} }; >>>> +int active_8x16_reg_action[1][2] = { {1, 1} }; >>>> +int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000}; >>>> +int active_8x16_reg_load[] = {100000}; >>>> +int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0}; >>>> +int active_8x16_reg_min_voltage[] = {1000000}; >>>> +char *proxy_8x16_clk_str[] = {"xo"}; >>>> +char *active_8x16_clk_str[] = {"iface", "bus", "mem"}; >>>> + >>>> +static const struct q6_rproc_res msm_8916_res = { >>>> + .proxy_clks = proxy_8x16_clk_str, >>>> + .proxy_clk_cnt = 1, >>>> + .active_clks = active_8x16_clk_str, >>>> + .active_clk_cnt = 3, >>>> + .proxy_regs = proxy_8x16_reg_str, >>>> + .active_regs = active_8x16_reg_str, >>>> + .proxy_reg_action = (int **)proxy_8x16_reg_action, >>>> + .proxy_reg_load = (int *)proxy_8x16_reg_load, >>>> + .active_reg_action = (int **)active_8x16_reg_action, >>>> + .active_reg_load = (int *)active_8x16_reg_load, >>>> + .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage, >>>> + .active_reg_voltage = active_8x16_reg_min_voltage, >>>> + .proxy_reg_cnt = 3, >>>> + .active_reg_cnt = 1, >>>> + .q6_reset_init = q6v5_init_reset, >>>> + .q6_version = "v5", >>>> + .q6_mba_image = "mba.b00", >>> q6v55 (msm8916) also uses mba.mbn, while q6v5 (msm8974) uses mba.b00. >> Again this is a case where compatible string can not help, we should have >> single compatible string i.e. "q6v5" for msm8916 and msm8974 since both are >> from same class of q6 chip with different version. >> so if we cant initialize mdt name based on compatible string alone. > msm-3.4, msm-3.10 and msm-3.18 states that they are not the same and as > such I don't think we have a problem. > > The more I look at this, the more convinced I am that we got 8916 wrong, > i.e. we specified the wrong class and it just happens to work. > >>>> +}; >>>> + >>>> +static const struct of_device_id q6_of_match[] = { >>>> + { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res}, >>> As I was looking in the CAF tree, I believe the correct version for 8916 >>> is 5.5 and v5 was used in 8974. >>> >>> But I presume these versions are not strictly tied to certain platforms, >>> so please name the resource structs based on the q6v5 version, rather >>> than platforms. >> Yes, resource are tied to compatible and version of q6. >> have used compatible to initialize major resources but using DT specified >> version string for minor deviations where needed. >> Should this be fine? >> as far as version on 8916 and 8974 are concern they are as below. >> >> msm8974 q6v5 core version 5.0.0 >> msm8916 q6v5 core version 5.1.1 > If there are differences within a class then that just forces us to use > the version number. There's very little overhead in carrying one > compatible per platform, if that's what we need. Yes i will use one compatible per msm platform. > >>>> + { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res}, >>>> { }, >>>> }; > Regards, > Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-remoteproc" 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..cbc165c 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt @@ -8,6 +8,7 @@ on the Qualcomm Hexagon core. Value type: <string> Definition: must be one of: "qcom,q6v5-pil" + "qcom,q6v56-pil" - reg: Usage: required diff --git a/drivers/remoteproc/qcom_q6v5_pil.c b/drivers/remoteproc/qcom_q6v5_pil.c index 2e0caaa..b60dff3 100644 --- a/drivers/remoteproc/qcom_q6v5_pil.c +++ b/drivers/remoteproc/qcom_q6v5_pil.c @@ -30,13 +30,13 @@ #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" #include <linux/qcom_scm.h> -#define MBA_FIRMWARE_NAME "mba.b00" #define MPSS_FIRMWARE_NAME "modem.mdt" #define MPSS_CRASH_REASON_SMEM 421 @@ -93,13 +93,32 @@ #define QDSS_BHS_ON BIT(21) #define QDSS_LDO_BYP BIT(22) +struct q6_rproc_res { + char **proxy_clks; + int proxy_clk_cnt; + char **active_clks; + int active_clk_cnt; + char **proxy_regs; + int proxy_reg_cnt; + char **active_regs; + int active_reg_cnt; + int **proxy_reg_action; + int **active_reg_action; + int *proxy_reg_load; + int *active_reg_load; + int *proxy_reg_voltage; + int *active_reg_voltage; + char *q6_version; + char *q6_mba_image; + int (*q6_reset_init)(void *q, void *p); +}; struct q6v5 { struct device *dev; struct rproc *rproc; void __iomem *reg_base; void __iomem *rmb_base; - + void __iomem *restart_reg; struct regmap *halt_map; u32 halt_q6; u32 halt_modem; @@ -111,7 +130,7 @@ struct q6v5 { unsigned stop_bit; struct regulator_bulk_data supply[4]; - + const struct q6_rproc_res *q6_rproc_res; struct clk *ahb_clk; struct clk *axi_clk; struct clk *rom_clk; @@ -198,7 +217,7 @@ static int q6v5_load(struct rproc *rproc, const struct firmware *fw) return 0; } -static const struct rproc_fw_ops q6v5_fw_ops = { +static const struct rproc_fw_ops q6_fw_ops = { .find_rsc_table = qcom_mdt_find_rsc_table, .load = q6v5_load, }; @@ -599,7 +618,7 @@ static void *q6v5_da_to_va(struct rproc *rproc, u64 da, int len) return qproc->mpss_region + offset; } -static const struct rproc_ops q6v5_ops = { +static const struct rproc_ops q6_ops = { .start = q6v5_start, .stop = q6v5_stop, .da_to_va = q6v5_da_to_va, @@ -725,17 +744,36 @@ static int q6v5_init_clocks(struct q6v5 *qproc) return 0; } -static int q6v5_init_reset(struct q6v5 *qproc) +static int q6v5_init_reset(void *q, void *p) { - qproc->mss_restart = devm_reset_control_get(qproc->dev, NULL); + struct q6v5 *qproc = q; + struct platform_device *pdev = p; + + qproc->mss_restart = devm_reset_control_get(&pdev->dev, NULL); if (IS_ERR(qproc->mss_restart)) { - dev_err(qproc->dev, "failed to acquire mss restart\n"); + dev_err(&pdev->dev, "failed to acquire mss restart\n"); return PTR_ERR(qproc->mss_restart); } return 0; } +static int q6v56_init_reset(void *q, void *p) +{ + struct resource *res; + struct q6v5 *qproc = q; + struct platform_device *pdev = p; + + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "restart_reg"); + qproc->restart_reg = devm_ioremap(qproc->dev, res->start, + resource_size(res)); + if (IS_ERR(qproc->restart_reg)) { + dev_err(qproc->dev, "failed to get restart_reg\n"); + return PTR_ERR(qproc->restart_reg); + } + + return 0; +} static int q6v5_request_irq(struct q6v5 *qproc, struct platform_device *pdev, const char *name, @@ -803,20 +841,25 @@ static int q6v5_alloc_memory_region(struct q6v5 *qproc) return 0; } -static int q6v5_probe(struct platform_device *pdev) +static int q6_probe(struct platform_device *pdev) { struct q6v5 *qproc; struct rproc *rproc; + const struct q6_rproc_res *desc; int ret; - rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, - MBA_FIRMWARE_NAME, sizeof(*qproc)); + desc = of_device_get_match_data(&pdev->dev); + if (!desc) + return -EINVAL; + + rproc = rproc_alloc(&pdev->dev, pdev->name, &q6_ops, + desc->q6_mba_image, sizeof(*qproc)); if (!rproc) { dev_err(&pdev->dev, "failed to allocate rproc\n"); return -ENOMEM; } - rproc->fw_ops = &q6v5_fw_ops; + rproc->fw_ops = &q6_fw_ops; qproc = (struct q6v5 *)rproc->priv; qproc->dev = &pdev->dev; @@ -826,6 +869,7 @@ static int q6v5_probe(struct platform_device *pdev) init_completion(&qproc->start_done); init_completion(&qproc->stop_done); + qproc->q6_rproc_res = desc; ret = q6v5_init_mem(qproc, pdev); if (ret) goto free_rproc; @@ -842,7 +886,7 @@ static int q6v5_probe(struct platform_device *pdev) if (ret) goto free_rproc; - ret = q6v5_init_reset(qproc); + ret = qproc->q6_rproc_res->q6_reset_init(qproc, pdev); if (ret) goto free_rproc; @@ -873,14 +917,12 @@ static int q6v5_probe(struct platform_device *pdev) goto free_rproc; return 0; - free_rproc: rproc_free(rproc); - return ret; } -static int q6v5_remove(struct platform_device *pdev) +static int q6_remove(struct platform_device *pdev) { struct q6v5 *qproc = platform_get_drvdata(pdev); @@ -890,20 +932,80 @@ static int q6v5_remove(struct platform_device *pdev) return 0; } -static const struct of_device_id q6v5_of_match[] = { - { .compatible = "qcom,q6v5-pil", }, +char *proxy_8x96_reg_str[] = {"mx", "cx", "vdd_pll"}; +int proxy_8x96_reg_action[3][2] = { {0, 1}, {1, 1}, {1, 0} }; +int proxy_8x96_reg_load[] = {0, 100000, 100000}; +int proxy_8x96_reg_min_voltage[] = {1050000, 1250000, 0}; +char *proxy_8x96_clk_str[] = {"xo", "pnoc", "qdss"}; +char *active_8x96_clk_str[] = {"iface", "bus", "mem", "gpll0_mss_clk", + "snoc_axi_clk", "mnoc_axi_clk"}; + +static const struct q6_rproc_res msm_8996_res = { + .proxy_clks = proxy_8x96_clk_str, + .proxy_clk_cnt = 3, + .active_clks = active_8x96_clk_str, + .active_clk_cnt = 6, + .proxy_regs = proxy_8x96_reg_str, + .active_regs = NULL, + .proxy_reg_action = (int **)proxy_8x96_reg_action, + .proxy_reg_load = (int *)proxy_8x96_reg_load, + .active_reg_action = NULL, + .active_reg_load = NULL, + .proxy_reg_voltage = (int *)proxy_8x96_reg_min_voltage, + .active_reg_voltage = NULL, + .proxy_reg_cnt = 3, + .active_reg_cnt = 0, + .q6_reset_init = q6v56_init_reset, + .q6_version = "v56", + .q6_mba_image = "mba.mbn", +}; + +char *proxy_8x16_reg_str[] = {"mx", "cx", "pll"}; +char *active_8x16_reg_str[] = {"mss"}; +int proxy_8x16_reg_action[4][2] = { {0, 1}, {1, 0}, {1, 0} }; +int active_8x16_reg_action[1][2] = { {1, 1} }; +int proxy_8x16_reg_load[] = {100000, 0, 100000, 100000}; +int active_8x16_reg_load[] = {100000}; +int proxy_8x16_reg_min_voltage[] = {1050000, 0, 0}; +int active_8x16_reg_min_voltage[] = {1000000}; +char *proxy_8x16_clk_str[] = {"xo"}; +char *active_8x16_clk_str[] = {"iface", "bus", "mem"}; + +static const struct q6_rproc_res msm_8916_res = { + .proxy_clks = proxy_8x16_clk_str, + .proxy_clk_cnt = 1, + .active_clks = active_8x16_clk_str, + .active_clk_cnt = 3, + .proxy_regs = proxy_8x16_reg_str, + .active_regs = active_8x16_reg_str, + .proxy_reg_action = (int **)proxy_8x16_reg_action, + .proxy_reg_load = (int *)proxy_8x16_reg_load, + .active_reg_action = (int **)active_8x16_reg_action, + .active_reg_load = (int *)active_8x16_reg_load, + .proxy_reg_voltage = (int *)proxy_8x16_reg_min_voltage, + .active_reg_voltage = active_8x16_reg_min_voltage, + .proxy_reg_cnt = 3, + .active_reg_cnt = 1, + .q6_reset_init = q6v5_init_reset, + .q6_version = "v5", + .q6_mba_image = "mba.b00", +}; + +static const struct of_device_id q6_of_match[] = { + { .compatible = "qcom,q6v5-pil", .data = &msm_8916_res}, + { .compatible = "qcom,q6v56-pil", .data = &msm_8996_res}, { }, }; -static struct platform_driver q6v5_driver = { - .probe = q6v5_probe, - .remove = q6v5_remove, +static struct platform_driver q6_driver = { + .probe = q6_probe, + .remove = q6_remove, .driver = { .name = "qcom-q6v5-pil", - .of_match_table = q6v5_of_match, + .of_match_table = q6_of_match, }, }; -module_platform_driver(q6v5_driver); +module_platform_driver(q6_driver); MODULE_DESCRIPTION("Peripheral Image Loader for Hexagon"); MODULE_LICENSE("GPL v2");
Embed resources specific to version of hexagon chip in device structure to avoid conditional check for manipulation of those resources in driver code. Signed-off-by: Avaneesh Kumar Dwivedi <akdwived@codeaurora.org> --- .../devicetree/bindings/remoteproc/qcom,q6v5.txt | 1 + drivers/remoteproc/qcom_q6v5_pil.c | 148 +++++++++++++++++---- 2 files changed, 126 insertions(+), 23 deletions(-)