| Message ID | 20201228135625.4971-1-shawn.guo@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc: qcom: geni: shield ICC calls for ACPI boot | expand |
Il 28/12/20 14:56, Shawn Guo ha scritto: > Currently, GENI devices like i2c-qcom-geni fails to probe in ACPI boot, > if interconnect support is enabled. That's because interconnect driver > only supports DT right now. As interconnect is not necessarily required > for basic function of GENI devices, let's shield those ICC calls to get > GENI devices probe for ACPI boot. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/soc/qcom/qcom-geni-se.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > Hello! To avoid developers booting on ACPI getting "strange slownesses" and possibly timeouts with no apparent reason, wouldn't it be a bit more proper and friendly to actually send a kernel message to advertise that there is *no interconnect support* and that this may lead to the aforementioned potential issues? --Angelo > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index f42954e2c98e..9feb1d78a5df 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -760,6 +760,9 @@ int geni_icc_get(struct geni_se *se, const char *icc_ddr) > int i, err; > const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > if (!icc_names[i]) > continue; > @@ -785,6 +788,9 @@ int geni_icc_set_bw(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_set_bw(se->icc_paths[i].path, > se->icc_paths[i].avg_bw, se->icc_paths[i].avg_bw); > @@ -803,6 +809,9 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag) > { > int i; > > + if (has_acpi_companion(se->dev)) > + return; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) > icc_set_tag(se->icc_paths[i].path, tag); > } > @@ -813,6 +822,9 @@ int geni_icc_enable(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_enable(se->icc_paths[i].path); > if (ret) { > @@ -830,6 +842,9 @@ int geni_icc_disable(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_disable(se->icc_paths[i].path); > if (ret) { >
On Thu, Jan 14, 2021 at 12:17:23AM +0100, AngeloGioacchino Del Regno wrote: > Il 28/12/20 14:56, Shawn Guo ha scritto: > > Currently, GENI devices like i2c-qcom-geni fails to probe in ACPI boot, > > if interconnect support is enabled. That's because interconnect driver > > only supports DT right now. As interconnect is not necessarily required > > for basic function of GENI devices, let's shield those ICC calls to get > > GENI devices probe for ACPI boot. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > drivers/soc/qcom/qcom-geni-se.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > Hello! > > To avoid developers booting on ACPI getting "strange slownesses" and > possibly timeouts with no apparent reason, wouldn't it be a bit more proper > and friendly to actually send a kernel message to advertise that there is > *no interconnect support* and that this may lead to the aforementioned > potential issues? Thanks for the comment. The commit 58ffbba6a399 ("soc: qcom: geni: Support for ICC voting") was landed just recently, and we have been running those geni-se device drivers without ICC voting for years. I doubt it would have been the case if missing ICC from any of those geni-se will cause any strange slowness or timeout issues. There are only a limited number of drivers on Snapdragon that support ACPI boot right now. We are running it with distro installer, and haven't seen any slowness or timeout. In short, I prefer to add warnings that are really helpful rather than just something noisy. Shawn
On Mon 28 Dec 07:56 CST 2020, Shawn Guo wrote: > Currently, GENI devices like i2c-qcom-geni fails to probe in ACPI boot, > if interconnect support is enabled. That's because interconnect driver > only supports DT right now. As interconnect is not necessarily required > for basic function of GENI devices, let's shield those ICC calls to get > GENI devices probe for ACPI boot. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > drivers/soc/qcom/qcom-geni-se.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > index f42954e2c98e..9feb1d78a5df 100644 > --- a/drivers/soc/qcom/qcom-geni-se.c > +++ b/drivers/soc/qcom/qcom-geni-se.c > @@ -760,6 +760,9 @@ int geni_icc_get(struct geni_se *se, const char *icc_ddr) > int i, err; > const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > if (!icc_names[i]) > continue; > @@ -785,6 +788,9 @@ int geni_icc_set_bw(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) Can't we simply rely on the fact that icc_set_bw(), icc_enable() and icc_disable() all return successfully when passed a path of NULL? That would reduce this patch to only modifying geni_icc_get(), which presumably would be the place the modify once there is a mechanism to acquire paths in ACPI (if that ends up look anything like the of-based case). Regards, Bjorn > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_set_bw(se->icc_paths[i].path, > se->icc_paths[i].avg_bw, se->icc_paths[i].avg_bw); > @@ -803,6 +809,9 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag) > { > int i; > > + if (has_acpi_companion(se->dev)) > + return; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) > icc_set_tag(se->icc_paths[i].path, tag); > } > @@ -813,6 +822,9 @@ int geni_icc_enable(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_enable(se->icc_paths[i].path); > if (ret) { > @@ -830,6 +842,9 @@ int geni_icc_disable(struct geni_se *se) > { > int i, ret; > > + if (has_acpi_companion(se->dev)) > + return 0; > + > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > ret = icc_disable(se->icc_paths[i].path); > if (ret) { > -- > 2.17.1 >
On Wed, Jan 13, 2021 at 10:12:51PM -0600, Bjorn Andersson wrote: > On Mon 28 Dec 07:56 CST 2020, Shawn Guo wrote: > > > Currently, GENI devices like i2c-qcom-geni fails to probe in ACPI boot, > > if interconnect support is enabled. That's because interconnect driver > > only supports DT right now. As interconnect is not necessarily required > > for basic function of GENI devices, let's shield those ICC calls to get > > GENI devices probe for ACPI boot. > > > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > drivers/soc/qcom/qcom-geni-se.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c > > index f42954e2c98e..9feb1d78a5df 100644 > > --- a/drivers/soc/qcom/qcom-geni-se.c > > +++ b/drivers/soc/qcom/qcom-geni-se.c > > @@ -760,6 +760,9 @@ int geni_icc_get(struct geni_se *se, const char *icc_ddr) > > int i, err; > > const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; > > > > + if (has_acpi_companion(se->dev)) > > + return 0; > > + > > for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { > > if (!icc_names[i]) > > continue; > > @@ -785,6 +788,9 @@ int geni_icc_set_bw(struct geni_se *se) > > { > > int i, ret; > > > > + if (has_acpi_companion(se->dev)) > > Can't we simply rely on the fact that icc_set_bw(), icc_enable() and > icc_disable() all return successfully when passed a path of NULL? Yes, we can. Thanks for the tip! I will send v2 shortly. Shawn
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index f42954e2c98e..9feb1d78a5df 100644 --- a/drivers/soc/qcom/qcom-geni-se.c +++ b/drivers/soc/qcom/qcom-geni-se.c @@ -760,6 +760,9 @@ int geni_icc_get(struct geni_se *se, const char *icc_ddr) int i, err; const char *icc_names[] = {"qup-core", "qup-config", icc_ddr}; + if (has_acpi_companion(se->dev)) + return 0; + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { if (!icc_names[i]) continue; @@ -785,6 +788,9 @@ int geni_icc_set_bw(struct geni_se *se) { int i, ret; + if (has_acpi_companion(se->dev)) + return 0; + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { ret = icc_set_bw(se->icc_paths[i].path, se->icc_paths[i].avg_bw, se->icc_paths[i].avg_bw); @@ -803,6 +809,9 @@ void geni_icc_set_tag(struct geni_se *se, u32 tag) { int i; + if (has_acpi_companion(se->dev)) + return; + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) icc_set_tag(se->icc_paths[i].path, tag); } @@ -813,6 +822,9 @@ int geni_icc_enable(struct geni_se *se) { int i, ret; + if (has_acpi_companion(se->dev)) + return 0; + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { ret = icc_enable(se->icc_paths[i].path); if (ret) { @@ -830,6 +842,9 @@ int geni_icc_disable(struct geni_se *se) { int i, ret; + if (has_acpi_companion(se->dev)) + return 0; + for (i = 0; i < ARRAY_SIZE(se->icc_paths); i++) { ret = icc_disable(se->icc_paths[i].path); if (ret) {
Currently, GENI devices like i2c-qcom-geni fails to probe in ACPI boot, if interconnect support is enabled. That's because interconnect driver only supports DT right now. As interconnect is not necessarily required for basic function of GENI devices, let's shield those ICC calls to get GENI devices probe for ACPI boot. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- drivers/soc/qcom/qcom-geni-se.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)