| Message ID | 20220929050209.1464526-2-sireeshkodali1@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | remoteproc: qcom: Add support for pronto-v3 | expand |
On Thu, Sep 29, 2022 at 10:32:05AM +0530, Sireesh Kodali wrote: > From: Vladimir Lypak <vladimir.lypak@gmail.com> > > Pronto-v3 is similar to pronto-v2. It requires two power domains, and it > requires the xo clock. It is used on the MSM8953 platform. > > Signed-off-by: Vladimir Lypak <vladimir.lypak@gmail.com> > Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com> > --- > drivers/remoteproc/qcom_wcnss.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c > index 68f37296b151..ff18bfae5eb6 100644 > --- a/drivers/remoteproc/qcom_wcnss.c > +++ b/drivers/remoteproc/qcom_wcnss.c > @@ -141,6 +141,18 @@ static const struct wcnss_data pronto_v2_data = { > .num_vregs = 1, > }; > > +static const struct wcnss_data pronto_v3_data = { > + .pmu_offset = 0x1004, > + .spare_offset = 0x1088, > + > + .pd_names = { "mx", "cx" }, > + .vregs = (struct wcnss_vreg_info[]) { > + { "vddpx", 1800000, 1800000, 0 }, > + }, > + .num_pd_vregs = 2, Can you try dropping this line? num_pd_vregs = 2 means: "If power domains are specified in the device tree, skip the first two entries in 'vregs'." For pronto-v1/v2 it would skip the "vddmx" and "vddcx" entry, since those are already covered by the power domains. However, since pronto-v3 should always have power domains in DT and "vregs" has just a single entry this means that it would always access the array out of bounds at runtime and request some garbage regulator. I'm confused why this does not crash more spectacularly... Thanks, Stephan > + .num_vregs = 1, > +}; > + > static int wcnss_load(struct rproc *rproc, const struct firmware *fw) > { > struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv; > @@ -675,6 +687,7 @@ static const struct of_device_id wcnss_of_match[] = { > { .compatible = "qcom,riva-pil", &riva_data }, > { .compatible = "qcom,pronto-v1-pil", &pronto_v1_data }, > { .compatible = "qcom,pronto-v2-pil", &pronto_v2_data }, > + { .compatible = "qcom,pronto-v3-pil", &pronto_v3_data }, > { }, > }; > MODULE_DEVICE_TABLE(of, wcnss_of_match); > -- > 2.37.3 >
On Thu Sep 29, 2022 at 3:09 PM IST, Stephan Gerhold wrote: > On Thu, Sep 29, 2022 at 10:32:05AM +0530, Sireesh Kodali wrote: > > From: Vladimir Lypak <vladimir.lypak@gmail.com> > > > > Pronto-v3 is similar to pronto-v2. It requires two power domains, and it > > requires the xo clock. It is used on the MSM8953 platform. > > > > Signed-off-by: Vladimir Lypak <vladimir.lypak@gmail.com> > > Signed-off-by: Sireesh Kodali <sireeshkodali1@gmail.com> > > --- > > drivers/remoteproc/qcom_wcnss.c | 13 +++++++++++++ > > 1 file changed, 13 insertions(+) > > > > diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c > > index 68f37296b151..ff18bfae5eb6 100644 > > --- a/drivers/remoteproc/qcom_wcnss.c > > +++ b/drivers/remoteproc/qcom_wcnss.c > > @@ -141,6 +141,18 @@ static const struct wcnss_data pronto_v2_data = { > > .num_vregs = 1, > > }; > > > > +static const struct wcnss_data pronto_v3_data = { > > + .pmu_offset = 0x1004, > > + .spare_offset = 0x1088, > > + > > + .pd_names = { "mx", "cx" }, > > + .vregs = (struct wcnss_vreg_info[]) { > > + { "vddpx", 1800000, 1800000, 0 }, > > + }, > > + .num_pd_vregs = 2, > > Can you try dropping this line? num_pd_vregs = 2 means: > "If power domains are specified in the device tree, skip the first two > entries in 'vregs'." For pronto-v1/v2 it would skip the "vddmx" and > "vddcx" entry, since those are already covered by the power domains. > > However, since pronto-v3 should always have power domains in DT and > "vregs" has just a single entry this means that it would always access > the array out of bounds at runtime and request some garbage regulator. > I'm confused why this does not crash more spectacularly... Indeed it should have crashed, thanks for catching this. Will be fixed in v6 > > Thanks, > Stephan > > > + .num_vregs = 1, > > +}; > > + > > static int wcnss_load(struct rproc *rproc, const struct firmware *fw) > > { > > struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv; > > @@ -675,6 +687,7 @@ static const struct of_device_id wcnss_of_match[] = { > > { .compatible = "qcom,riva-pil", &riva_data }, > > { .compatible = "qcom,pronto-v1-pil", &pronto_v1_data }, > > { .compatible = "qcom,pronto-v2-pil", &pronto_v2_data }, > > + { .compatible = "qcom,pronto-v3-pil", &pronto_v3_data }, > > { }, > > }; > > MODULE_DEVICE_TABLE(of, wcnss_of_match); > > -- > > 2.37.3 > >
diff --git a/drivers/remoteproc/qcom_wcnss.c b/drivers/remoteproc/qcom_wcnss.c index 68f37296b151..ff18bfae5eb6 100644 --- a/drivers/remoteproc/qcom_wcnss.c +++ b/drivers/remoteproc/qcom_wcnss.c @@ -141,6 +141,18 @@ static const struct wcnss_data pronto_v2_data = { .num_vregs = 1, }; +static const struct wcnss_data pronto_v3_data = { + .pmu_offset = 0x1004, + .spare_offset = 0x1088, + + .pd_names = { "mx", "cx" }, + .vregs = (struct wcnss_vreg_info[]) { + { "vddpx", 1800000, 1800000, 0 }, + }, + .num_pd_vregs = 2, + .num_vregs = 1, +}; + static int wcnss_load(struct rproc *rproc, const struct firmware *fw) { struct qcom_wcnss *wcnss = (struct qcom_wcnss *)rproc->priv; @@ -675,6 +687,7 @@ static const struct of_device_id wcnss_of_match[] = { { .compatible = "qcom,riva-pil", &riva_data }, { .compatible = "qcom,pronto-v1-pil", &pronto_v1_data }, { .compatible = "qcom,pronto-v2-pil", &pronto_v2_data }, + { .compatible = "qcom,pronto-v3-pil", &pronto_v3_data }, { }, }; MODULE_DEVICE_TABLE(of, wcnss_of_match);