MIME-Version: 1.0
References: <1553281120-22139-1-git-send-email-pozega.tomislav@gmail.com>
 <3337086.qEUs9xMCTV@debian64> <fe853ec8-0dca-d931-8632-fbe4df5cfaf8@broadcom.com>
 <edc38f32-977a-53f5-babd-605775326a20@broadcom.com>
In-Reply-To: <edc38f32-977a-53f5-babd-605775326a20@broadcom.com>
From:   =?UTF-8?Q?Micha=C5=82_Kazior?= <kazikcz@gmail.com>
Date:   Mon, 25 Mar 2019 13:14:20 +0100
Message-ID: <CABvG-CUXHgJ1ZUfsZw6ajzR=4DjnRRNrPmF147mZsoWJBjp-jQ@mail.gmail.com>
Subject: Re: [PATCH] ath10k: reset chip after supported check
To:     Arend Van Spriel <arend.vanspriel@broadcom.com>
Cc:     Christian Lamparter <chunkeey@gmail.com>,
        =?UTF-8?Q?Tomislav_Po=C5=BEega?= <pozega.tomislav@gmail.com>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        openwrt-devel@lists.openwrt.org, Kalle Valo <kvalo@codeaurora.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

On Sat, 23 Mar 2019 at 08:20, Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> * resending with corrected email address from Kalle
> --------------------------------------------------------------------
> + Micha=C5=82

Thanks!


> On 3/22/2019 8:25 PM, Christian Lamparter wrote:
>  > On Friday, March 22, 2019 7:58:40 PM CET Tomislav Po=C5=BEega wrote:
>  >> When chip reset is done before the chip is checked if supported
>  >> there will be crash. Previous behaviour caused bootloops on
>  >> Archer C7 v1 units, this patch allows clean device boot without
>  >> excluding ath10k driver.

Can you elaborate more a bit? What kind of crashes are you seeing?
What does the bootloop look like? Do you have uart connected to
diagnose?

Didn't C7 v1 have the old QCA9880 hw v1 which isn't really supported
by ath10k? I recall the v1 chip was really buggy and required
hammering registers sometimes to get things working.


[...]
> Looking at the commit subject makes me suspicious whether this is a
> proper fix.
[...]
> It seems to me the chip reset was done explicitly *before* reading the
> chipid for a reason.
>
> """
> ath10k: reset chip before reading chip_id in probe
> There are some very rare cases with some hardware
> configuration that the device doesn't init quickly
> enough in which case reading chip_id yielded 0.
> This caused driver to subsequently fail to setup
> the device.
>
> Signed-off-by: Michal Kazior <michal.kazior@tieto.com>
> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
> """

Good catch, Arend!


> Might be the ath10k_pci_chip_reset() function needs to be modified to
> work properly for Archer C7 v1 units.

When driver boots up hardware is in an unknown state. It could've been
power cycled or it could've remained powered via pcie aux while host
cpu went through a warm reset (eg. watchdog). For what it's worth it
could still be beaconing if that is what it was doing before host cpu
reset. Therefore the device can be in a messy state and it should be
reset before talking to it or else you risk reading garbage.

I don't recall details about this particular commit but given the
timeline of commits at the time this change suggests this was related
to supporting qca61x4. In other words your change risks breaking some
qca61x4 chips.


Micha=C5=82