[3/4] ath10k: snoc: relax voltage requirements
diff mbox series

Message ID 20181013005504.46399-3-briannorris@chromium.org
State New
Headers show
Series
  • [1/4] ath10k: snoc: remove 'wcn3990' from generic resource handling
Related show

Commit Message

Brian Norris Oct. 13, 2018, 12:55 a.m. UTC
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(-)

Comments

Doug Anderson Oct. 18, 2018, 5:56 p.m. UTC | #1
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>
Brian Norris Oct. 18, 2018, 6:14 p.m. UTC | #2
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

Patch
diff mbox series

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[] = {