[PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[Shawn Guo]

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))
+		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

Re: [PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[AngeloGioacchino Del Regno]

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) {
> 

Re: [PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[Shawn Guo]

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

Re: [PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[Bjorn Andersson]

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
> 

Re: [PATCH] soc: qcom: geni: shield ICC calls for ACPI boot

[Shawn Guo]

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