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@xxxxxxxxxxxxxx>
---
.../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
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
