[PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Brian Norris]

ath10k_wcn3990_clk_info and ath10k_wcn3990_vreg_info are not
WCN3990-specific structures. They hold generic data. So don't name them
with wcn3990 specifics.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/snoc.c | 34 +++++++++++++-------------
 drivers/net/wireless/ath/ath10k/snoc.h |  8 +++---
 2 files changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 8d3d9bca410f..c6254db17dab 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -46,14 +46,14 @@ static char *const ce_name[] = {
 	"WLAN_CE_11",
 };
 
-static struct ath10k_wcn3990_vreg_info vreg_cfg[] = {
+static struct ath10k_vreg_info vreg_cfg[] = {
 	{NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false},
 	{NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false},
 	{NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false},
 	{NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false},
 };
 
-static struct ath10k_wcn3990_clk_info clk_cfg[] = {
+static struct ath10k_clk_info clk_cfg[] = {
 	{NULL, "cxo_ref_clk_pin", 0, false},
 };
 
@@ -1246,7 +1246,7 @@ static void ath10k_snoc_release_resource(struct ath10k *ar)
 }
 
 static int ath10k_get_vreg_info(struct ath10k *ar, struct device *dev,
-				struct ath10k_wcn3990_vreg_info *vreg_info)
+				struct ath10k_vreg_info *vreg_info)
 {
 	struct regulator *reg;
 	int ret = 0;
@@ -1284,7 +1284,7 @@ static int ath10k_get_vreg_info(struct ath10k *ar, struct device *dev,
 }
 
 static int ath10k_get_clk_info(struct ath10k *ar, struct device *dev,
-			       struct ath10k_wcn3990_clk_info *clk_info)
+			       struct ath10k_clk_info *clk_info)
 {
 	struct clk *handle;
 	int ret = 0;
@@ -1311,10 +1311,10 @@ static int ath10k_get_clk_info(struct ath10k *ar, struct device *dev,
 	return ret;
 }
 
-static int ath10k_wcn3990_vreg_on(struct ath10k *ar)
+static int ath10k_snoc_vreg_on(struct ath10k *ar)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
-	struct ath10k_wcn3990_vreg_info *vreg_info;
+	struct ath10k_vreg_info *vreg_info;
 	int ret = 0;
 	int i;
 
@@ -1376,10 +1376,10 @@ static int ath10k_wcn3990_vreg_on(struct ath10k *ar)
 	return ret;
 }
 
-static int ath10k_wcn3990_vreg_off(struct ath10k *ar)
+static int ath10k_snoc_vreg_off(struct ath10k *ar)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
-	struct ath10k_wcn3990_vreg_info *vreg_info;
+	struct ath10k_vreg_info *vreg_info;
 	int ret = 0;
 	int i;
 
@@ -1412,10 +1412,10 @@ static int ath10k_wcn3990_vreg_off(struct ath10k *ar)
 	return ret;
 }
 
-static int ath10k_wcn3990_clk_init(struct ath10k *ar)
+static int ath10k_snoc_clk_init(struct ath10k *ar)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
-	struct ath10k_wcn3990_clk_info *clk_info;
+	struct ath10k_clk_info *clk_info;
 	int ret = 0;
 	int i;
 
@@ -1461,10 +1461,10 @@ static int ath10k_wcn3990_clk_init(struct ath10k *ar)
 	return ret;
 }
 
-static int ath10k_wcn3990_clk_deinit(struct ath10k *ar)
+static int ath10k_snoc_clk_deinit(struct ath10k *ar)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
-	struct ath10k_wcn3990_clk_info *clk_info;
+	struct ath10k_clk_info *clk_info;
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(clk_cfg); i++) {
@@ -1488,18 +1488,18 @@ static int ath10k_hw_power_on(struct ath10k *ar)
 
 	ath10k_dbg(ar, ATH10K_DBG_SNOC, "soc power on\n");
 
-	ret = ath10k_wcn3990_vreg_on(ar);
+	ret = ath10k_snoc_vreg_on(ar);
 	if (ret)
 		return ret;
 
-	ret = ath10k_wcn3990_clk_init(ar);
+	ret = ath10k_snoc_clk_init(ar);
 	if (ret)
 		goto vreg_off;
 
 	return ret;
 
 vreg_off:
-	ath10k_wcn3990_vreg_off(ar);
+	ath10k_snoc_vreg_off(ar);
 	return ret;
 }
 
@@ -1509,9 +1509,9 @@ static int ath10k_hw_power_off(struct ath10k *ar)
 
 	ath10k_dbg(ar, ATH10K_DBG_SNOC, "soc power off\n");
 
-	ath10k_wcn3990_clk_deinit(ar);
+	ath10k_snoc_clk_deinit(ar);
 
-	ret = ath10k_wcn3990_vreg_off(ar);
+	ret = ath10k_snoc_vreg_off(ar);
 
 	return ret;
 }
diff --git a/drivers/net/wireless/ath/ath10k/snoc.h b/drivers/net/wireless/ath/ath10k/snoc.h
index e1d2d6675556..4a3837b5c68d 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.h
+++ b/drivers/net/wireless/ath/ath10k/snoc.h
@@ -53,7 +53,7 @@ struct ath10k_snoc_ce_irq {
 	u32 irq_line;
 };
 
-struct ath10k_wcn3990_vreg_info {
+struct ath10k_vreg_info {
 	struct regulator *reg;
 	const char *name;
 	u32 min_v;
@@ -63,7 +63,7 @@ struct ath10k_wcn3990_vreg_info {
 	bool required;
 };
 
-struct ath10k_wcn3990_clk_info {
+struct ath10k_clk_info {
 	struct clk *handle;
 	const char *name;
 	u32 freq;
@@ -81,8 +81,8 @@ struct ath10k_snoc {
 	struct ath10k_snoc_ce_irq ce_irqs[CE_COUNT_MAX];
 	struct ath10k_ce ce;
 	struct timer_list rx_post_retry;
-	struct ath10k_wcn3990_vreg_info *vreg;
-	struct ath10k_wcn3990_clk_info *clk;
+	struct ath10k_vreg_info *vreg;
+	struct ath10k_clk_info *clk;
 	struct ath10k_qmi *qmi;
 };
 
-- 
2.19.1.331.ge82ca0e54c-goog

[PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling

[Brian Norris]

If a regulator fails to set its voltage, we end up with an unbalanced
call to regulator_disable(), because the error path starts with the
current regulator (which was never enabled).

Factor out the "on" function to perform (and unwind if failed) a single
regulator at a time, and then main loop (ath10k_snoc_vreg_on()) can just
worry about unwinding the regulators that were already enabled.

It also helps to factor out the "off" function, to avoid repeating some
code here.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/snoc.c | 129 ++++++++++++++-----------
 1 file changed, 75 insertions(+), 54 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index c6254db17dab..b63ae8b006b4 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1311,6 +1311,76 @@ static int ath10k_get_clk_info(struct ath10k *ar, struct device *dev,
 	return ret;
 }
 
+static int __ath10k_snoc_vreg_on(struct ath10k *ar,
+				 struct ath10k_vreg_info *vreg_info)
+{
+	int ret;
+
+	ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being enabled\n",
+		   vreg_info->name);
+
+	ret = regulator_set_voltage(vreg_info->reg, vreg_info->min_v,
+				    vreg_info->max_v);
+	if (ret) {
+		ath10k_err(ar,
+			   "failed to set regulator %s voltage-min: %d voltage-max: %d\n",
+			   vreg_info->name, vreg_info->min_v, vreg_info->max_v);
+		return ret;
+	}
+
+	if (vreg_info->load_ua) {
+		ret = regulator_set_load(vreg_info->reg, vreg_info->load_ua);
+		if (ret < 0) {
+			ath10k_err(ar, "failed to set regulator %s load: %d\n",
+				   vreg_info->name, vreg_info->load_ua);
+			goto err_set_load;
+		}
+	}
+
+	ret = regulator_enable(vreg_info->reg);
+	if (ret) {
+		ath10k_err(ar, "failed to enable regulator %s\n",
+			   vreg_info->name);
+		goto err_enable;
+	}
+
+	if (vreg_info->settle_delay)
+		udelay(vreg_info->settle_delay);
+
+	return 0;
+
+err_enable:
+	regulator_set_load(vreg_info->reg, 0);
+err_set_load:
+	regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
+
+	return ret;
+}
+
+static int __ath10k_snoc_vreg_off(struct ath10k *ar,
+				  struct ath10k_vreg_info *vreg_info)
+{
+	int ret;
+
+	ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n",
+		   vreg_info->name);
+
+	ret = regulator_disable(vreg_info->reg);
+	if (ret)
+		ath10k_err(ar, "failed to disable regulator %s\n",
+			   vreg_info->name);
+
+	ret = regulator_set_load(vreg_info->reg, 0);
+	if (ret < 0)
+		ath10k_err(ar, "failed to set load %s\n", vreg_info->name);
+
+	ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
+	if (ret)
+		ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name);
+
+	return ret;
+}
+
 static int ath10k_snoc_vreg_on(struct ath10k *ar)
 {
 	struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
@@ -1324,53 +1394,21 @@ static int ath10k_snoc_vreg_on(struct ath10k *ar)
 		if (!vreg_info->reg)
 			continue;
 
-		ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being enabled\n",
-			   vreg_info->name);
-
-		ret = regulator_set_voltage(vreg_info->reg, vreg_info->min_v,
-					    vreg_info->max_v);
-		if (ret) {
-			ath10k_err(ar,
-				   "failed to set regulator %s voltage-min: %d voltage-max: %d\n",
-				   vreg_info->name, vreg_info->min_v, vreg_info->max_v);
-			goto err_reg_config;
-		}
-
-		if (vreg_info->load_ua) {
-			ret = regulator_set_load(vreg_info->reg,
-						 vreg_info->load_ua);
-			if (ret < 0) {
-				ath10k_err(ar,
-					   "failed to set regulator %s load: %d\n",
-					   vreg_info->name,
-					   vreg_info->load_ua);
-				goto err_reg_config;
-			}
-		}
-
-		ret = regulator_enable(vreg_info->reg);
-		if (ret) {
-			ath10k_err(ar, "failed to enable regulator %s\n",
-				   vreg_info->name);
+		ret = __ath10k_snoc_vreg_on(ar, vreg_info);
+		if (ret)
 			goto err_reg_config;
-		}
-
-		if (vreg_info->settle_delay)
-			udelay(vreg_info->settle_delay);
 	}
 
 	return 0;
 
 err_reg_config:
-	for (; i >= 0; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		vreg_info = &ar_snoc->vreg[i];
 
 		if (!vreg_info->reg)
 			continue;
 
-		regulator_disable(vreg_info->reg);
-		regulator_set_load(vreg_info->reg, 0);
-		regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
+		__ath10k_snoc_vreg_off(ar, vreg_info);
 	}
 
 	return ret;
@@ -1389,24 +1427,7 @@ static int ath10k_snoc_vreg_off(struct ath10k *ar)
 		if (!vreg_info->reg)
 			continue;
 
-		ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n",
-			   vreg_info->name);
-
-		ret = regulator_disable(vreg_info->reg);
-		if (ret)
-			ath10k_err(ar, "failed to disable regulator %s\n",
-				   vreg_info->name);
-
-		ret = regulator_set_load(vreg_info->reg, 0);
-		if (ret < 0)
-			ath10k_err(ar, "failed to set load %s\n",
-				   vreg_info->name);
-
-		ret = regulator_set_voltage(vreg_info->reg, 0,
-					    vreg_info->max_v);
-		if (ret)
-			ath10k_err(ar, "failed to set voltage %s\n",
-				   vreg_info->name);
+		ret = __ath10k_snoc_vreg_off(ar, vreg_info);
 	}
 
 	return ret;
-- 
2.19.1.331.ge82ca0e54c-goog

[PATCH 3/4] ath10k: snoc: relax voltage requirements

[Brian Norris]

I rarely see drivers specify precise voltage requirements like this, but
if we really have to...let's at least give a little wiggle room. Board
designs (and accompanying device trees) may not provide exactly the
voltage listed here, and we shouldn't fail to probe just because of
this.

Round these ranges down to the nearest volt, and provide a 0.05V margin.
The regulator should provide its own supported ranges, which will
helpfully intersect with these ranges.

I would just as well remove these ranges entirely, but if I understand
correctly, there's some reason that QCOM SoC's like to set zero /
non-zero voltages.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/snoc.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index b63ae8b006b4..5a8e87339df2 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -47,10 +47,10 @@ static char *const ce_name[] = {
 };
 
 static struct ath10k_vreg_info vreg_cfg[] = {
-	{NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false},
-	{NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false},
-	{NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false},
-	{NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false},
+	{NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false},
+	{NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false},
+	{NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false},
+	{NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false},
 };
 
 static struct ath10k_clk_info clk_cfg[] = {
-- 
2.19.1.331.ge82ca0e54c-goog

[PATCH 4/4] ath10k: snoc: fix unbalanced clock error handling

[Brian Norris]

Similar to regulator error handling, we should only start tearing down
the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
end up with an unbalanced clock, where we never successfully enabled the
clock, but we try to disable it anyway.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
 drivers/net/wireless/ath/ath10k/snoc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
index 5a8e87339df2..a835599a8d55 100644
--- a/drivers/net/wireless/ath/ath10k/snoc.c
+++ b/drivers/net/wireless/ath/ath10k/snoc.c
@@ -1470,7 +1470,7 @@ static int ath10k_snoc_clk_init(struct ath10k *ar)
 	return 0;
 
 err_clock_config:
-	for (; i >= 0; i--) {
+	for (i = i - 1; i >= 0; i--) {
 		clk_info = &ar_snoc->clk[i];
 
 		if (!clk_info->handle)
-- 
2.19.1.331.ge82ca0e54c-goog

Re: [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Doug Anderson]

Hi,

On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>
> ath10k_wcn3990_clk_info and ath10k_wcn3990_vreg_info are not
> WCN3990-specific structures. They hold generic data. So don't name them
> with wcn3990 specifics.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 34 +++++++++++++-------------
>  drivers/net/wireless/ath/ath10k/snoc.h |  8 +++---
>  2 files changed, 21 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 8d3d9bca410f..c6254db17dab 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -46,14 +46,14 @@ static char *const ce_name[] = {
>         "WLAN_CE_11",
>  };
>
> -static struct ath10k_wcn3990_vreg_info vreg_cfg[] = {
> +static struct ath10k_vreg_info vreg_cfg[] = {

Ironically, you could sorta make the argument that this should be:

static struct ath10k_vreg_info wcn3990_vreg_cfg

AKA the "wcn3990" shouldn't be in the name of the structure (since all
snoc devices can have the concept of an array of regulators) but
wcn3990 could be in the name of the variable since it's possible that
different snoc devices could have different arrays.  However I'm OK w/
waiting to do that part until we actually see a different snoc device
with a different array.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Brian Norris]

On Tue, Oct 16, 2018 at 4:43 PM Doug Anderson <dianders@chromium.org> wrote:
> On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
> >
> > ath10k_wcn3990_clk_info and ath10k_wcn3990_vreg_info are not
> > WCN3990-specific structures. They hold generic data. So don't name them
> > with wcn3990 specifics.
> >

> > -static struct ath10k_wcn3990_vreg_info vreg_cfg[] = {
> > +static struct ath10k_vreg_info vreg_cfg[] = {
>
> Ironically, you could sorta make the argument that this should be:
>
> static struct ath10k_vreg_info wcn3990_vreg_cfg

Hehe, good point.

> AKA the "wcn3990" shouldn't be in the name of the structure (since all
> snoc devices can have the concept of an array of regulators) but
> wcn3990 could be in the name of the variable since it's possible that
> different snoc devices could have different arrays.  However I'm OK w/
> waiting to do that part until we actually see a different snoc device
> with a different array.

But I think that is a pretty reasonable conclusion.

There's still some other strange stuff in this driver too, like the
fact that this is NOT a const array -- we assign things dynamically to
the regulator fields within the array -- that we would probably want
to fix if this is really supposed to be a generic multi-IP driver.

> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks,
Brian

Re: [PATCH 4/4] ath10k: snoc: fix unbalanced clock error handling

[Doug Anderson]

Hi,
On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Similar to regulator error handling, we should only start tearing down
> the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
> end up with an unbalanced clock, where we never successfully enabled the
> clock, but we try to disable it anyway.
>
> Signed-off-by: Brian Norris <briannorris@chromium.org>

Presumably you could have a Fixes tag just to help folks, like:

Fixes: a6a793f98786 ("ath10k: vote for hardware resources for WCN3990")

> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index 5a8e87339df2..a835599a8d55 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1470,7 +1470,7 @@ static int ath10k_snoc_clk_init(struct ath10k *ar)
>         return 0;
>
>  err_clock_config:
> -       for (; i >= 0; i--) {
> +       for (i = i - 1; i >= 0; i--) {

I see no problem with this and it's a good / easy to backport fix.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

For extra credit, though, you could change this to not duplicate the
"clk_bulk" APIs and just use 'em.  I already mentioned to someone else
that maybe the clk_bulk APIs could probably be improved to handle
optional clocks, but I don't think anyone has posted a patch for it.
Bonus points if you remember that "NULL" is a valid clock so you
really only need special case code in clk_bulk_get().  :-P


-Doug

Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling

[Doug Anderson]

Hi,

On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
> +       if (vreg_info->settle_delay)
> +               udelay(vreg_info->settle_delay);

Not new to your patch, but this seems like a duplication of what the
regulator framework is doing for you.  There are plenty of regulator
properties describing lots of different types delays and that would be
the place to put it.  Doing so makes it automatically easy for boards
to specify a different delay if they have different ramp
characteristics (like someone put a bigger capacitor on the line or
somesuch).

At the moment it would be easy for someone to submit a patch to kill
the settle delay in this driver this since the entire vreg_cfg table
has 0 for the settle delay.


> +static int __ath10k_snoc_vreg_off(struct ath10k *ar,
> +                                 struct ath10k_vreg_info *vreg_info)
> +{
> +       int ret;
> +
> +       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n",
> +                  vreg_info->name);
> +
> +       ret = regulator_disable(vreg_info->reg);
> +       if (ret)
> +               ath10k_err(ar, "failed to disable regulator %s\n",
> +                          vreg_info->name);
> +
> +       ret = regulator_set_load(vreg_info->reg, 0);
> +       if (ret < 0)
> +               ath10k_err(ar, "failed to set load %s\n", vreg_info->name);
> +
> +       ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
> +       if (ret)
> +               ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name);

Not new to your patch, but ick, forcing someone to manually set the
load / voltage of a regulator that they've turned off is silly.  It's
only list to try to send a patch to remedy this situation.  Let me try
to put that higher on my plate.


...just like with the clock patch I suspect that some of this is
duplicating the "regulator_bulk" APIs.  ...though I guess maybe we
can't use those too easily until we can avoid setting voltage and
current so much...  In any case, your patch overall looks good and an
improvement.


Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 3/4] ath10k: snoc: relax voltage requirements

[Doug Anderson]

Hi,
On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>
> I rarely see drivers specify precise voltage requirements like this, but
> if we really have to...let's at least give a little wiggle room. Board
> designs (and accompanying device trees) may not provide exactly the
> voltage listed here, and we shouldn't fail to probe just because of
> this.
>
> Round these ranges down to the nearest volt, and provide a 0.05V margin.
> The regulator should provide its own supported ranges, which will
> helpfully intersect with these ranges.
>
> I would just as well remove these ranges entirely, but if I understand
> correctly, there's some reason that QCOM SoC's like to set zero /
> non-zero voltages.

Yeah, I'll try to up-prioritize working on making that better
(assuming others like my ideas in that area).


> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>  drivers/net/wireless/ath/ath10k/snoc.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c b/drivers/net/wireless/ath/ath10k/snoc.c
> index b63ae8b006b4..5a8e87339df2 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -47,10 +47,10 @@ static char *const ce_name[] = {
>  };
>
>  static struct ath10k_vreg_info vreg_cfg[] = {
> -       {NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false},
> -       {NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false},
> -       {NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false},
> -       {NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false},
> +       {NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false},
> +       {NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false},
> +       {NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false},
> +       {NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false},

These look fine to me.  I find it really funny that this array has all
those load values and they're all 0, but that's not new to your patch.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 3/4] ath10k: snoc: relax voltage requirements

[Brian Norris]

On Thu, Oct 18, 2018 at 10:56 AM Doug Anderson <dianders@chromium.org> wrote:
> On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
> >
> > I rarely see drivers specify precise voltage requirements like this, but
> > if we really have to...let's at least give a little wiggle room. Board
> > designs (and accompanying device trees) may not provide exactly the
> > voltage listed here, and we shouldn't fail to probe just because of
> > this.
> >
> > Round these ranges down to the nearest volt, and provide a 0.05V margin.
> > The regulator should provide its own supported ranges, which will
> > helpfully intersect with these ranges.
> >
> > I would just as well remove these ranges entirely, but if I understand
> > correctly, there's some reason that QCOM SoC's like to set zero /
> > non-zero voltages.
>
> Yeah, I'll try to up-prioritize working on making that better
> (assuming others like my ideas in that area).

Ah, OK, so my understanding is correct? (I feel like I've bumped into
this multiple times, but it probably didn't stick because it makes so
little sense to me.)

> >  static struct ath10k_vreg_info vreg_cfg[] = {
> > -       {NULL, "vdd-0.8-cx-mx", 800000, 800000, 0, 0, false},
> > -       {NULL, "vdd-1.8-xo", 1800000, 1800000, 0, 0, false},
> > -       {NULL, "vdd-1.3-rfa", 1304000, 1304000, 0, 0, false},
> > -       {NULL, "vdd-3.3-ch0", 3312000, 3312000, 0, 0, false},
> > +       {NULL, "vdd-0.8-cx-mx", 800000, 850000, 0, 0, false},
> > +       {NULL, "vdd-1.8-xo", 1800000, 1850000, 0, 0, false},
> > +       {NULL, "vdd-1.3-rfa", 1300000, 1350000, 0, 0, false},
> > +       {NULL, "vdd-3.3-ch0", 3300000, 3350000, 0, 0, false},
>
> These look fine to me.  I find it really funny that this array has all
> those load values and they're all 0, but that's not new to your patch.

Indeed, funny. It's also funny to have that 'required' field, which is
all 'false' -- but that kinda goes to your binding review too: there's
an overabundant use of "optional", to avoid defining real requirements
on a per-IP basis.

Brian

Re: [PATCH 2/4] ath10k: snoc: fix unabalanced regulator error handling

[Brian Norris]

Hi,

On Thu, Oct 18, 2018 at 10:54:39AM -0700, Doug Anderson wrote:
> On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
> > +       if (vreg_info->settle_delay)
> > +               udelay(vreg_info->settle_delay);
> 
> Not new to your patch, but this seems like a duplication of what the
> regulator framework is doing for you.  There are plenty of regulator
> properties describing lots of different types delays and that would be
> the place to put it.  Doing so makes it automatically easy for boards
> to specify a different delay if they have different ramp
> characteristics (like someone put a bigger capacitor on the line or
> somesuch).
> 
> At the moment it would be easy for someone to submit a patch to kill
> the settle delay in this driver this since the entire vreg_cfg table
> has 0 for the settle delay.

Thanks! I'll probably take a stab at killing the excessive
specifications in these tables, in a later patch. Would be nice to get
the bugfixes landed first though.

> > +static int __ath10k_snoc_vreg_off(struct ath10k *ar,
> > +                                 struct ath10k_vreg_info *vreg_info)
> > +{
> > +       int ret;
> > +
> > +       ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc regulator %s being disabled\n",
> > +                  vreg_info->name);
> > +
> > +       ret = regulator_disable(vreg_info->reg);
> > +       if (ret)
> > +               ath10k_err(ar, "failed to disable regulator %s\n",
> > +                          vreg_info->name);
> > +
> > +       ret = regulator_set_load(vreg_info->reg, 0);
> > +       if (ret < 0)
> > +               ath10k_err(ar, "failed to set load %s\n", vreg_info->name);
> > +
> > +       ret = regulator_set_voltage(vreg_info->reg, 0, vreg_info->max_v);
> > +       if (ret)
> > +               ath10k_err(ar, "failed to set voltage %s\n", vreg_info->name);
> 
> Not new to your patch, but ick, forcing someone to manually set the
> load / voltage of a regulator that they've turned off is silly.  It's
> only list to try to send a patch to remedy this situation.  Let me try

I'm guessing you meant:

s/only/on my/

> to put that higher on my plate.
> 
> 
> ...just like with the clock patch I suspect that some of this is
> duplicating the "regulator_bulk" APIs.  ...though I guess maybe we
> can't use those too easily until we can avoid setting voltage and
> current so much...  In any case, your patch overall looks good and an
> improvement.

I'll admit I've never used the bulk APIs. But I might give them a try as
a follow-up cleanup. (As before: would be nice to have the simpler
bugfix first.)

> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks,
Brian

Re: [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Kalle Valo]

Brian Norris <briannorris@chromium.org> wrote:

> ath10k_wcn3990_clk_info and ath10k_wcn3990_vreg_info are not
> WCN3990-specific structures. They hold generic data. So don't name them
> with wcn3990 specifics.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

This patchset (I think it was with the first patch, but not sure) had some
conflicts but 3-way merge was able to automatically solve them. Please check
the end result anyway:

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=77adda81e264e0c93af9570ac4ada82a5d0f5d99

-- 
https://patchwork.kernel.org/patch/10639795/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Re: [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Brian Norris]

On Mon, Nov 5, 2018 at 5:04 AM Kalle Valo <kvalo@codeaurora.org> wrote:
> This patchset (I think it was with the first patch, but not sure) had some
> conflicts but 3-way merge was able to automatically solve them. Please check
> the end result anyway:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=77adda81e264e0c93af9570ac4ada82a5d0f5d99

I see the same. There were some unrelated changes to this file that
you merged to your pending tree in the meantime. The end result looks
fine to me. Thanks for the heads up.

Brian

Re: [PATCH 4/4] ath10k: snoc: fix unbalanced clock error handling

[Kalle Valo]

Doug Anderson <dianders@chromium.org> writes:

> Hi,
> On Fri, Oct 12, 2018 at 5:55 PM Brian Norris <briannorris@chromium.org> wrote:
>>
>> Similar to regulator error handling, we should only start tearing down
>> the 'i - 1' clock when clock 'i' fails to enable. Otherwise, we might
>> end up with an unbalanced clock, where we never successfully enabled the
>> clock, but we try to disable it anyway.
>>
>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>
> Presumably you could have a Fixes tag just to help folks, like:
>
> Fixes: a6a793f98786 ("ath10k: vote for hardware resources for WCN3990")

Thanks, I added that in the pending branch.

-- 
Kalle Valo

Re: [PATCH 1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling

[Kalle Valo]

Brian Norris <briannorris@chromium.org> wrote:

> ath10k_wcn3990_clk_info and ath10k_wcn3990_vreg_info are not
> WCN3990-specific structures. They hold generic data. So don't name them
> with wcn3990 specifics.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>

4 patches applied to ath-next branch of ath.git, thanks.

887a3dcf5893 ath10k: snoc: remove 'wcn3990' from generic resource handling
1a1a0d5ccefc ath10k: snoc: fix unabalanced regulator error handling
bfe57a6ac75a ath10k: snoc: relax voltage requirements
82e60d920e8a ath10k: snoc: fix unbalanced clock error handling

-- 
https://patchwork.kernel.org/patch/10639795/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches