MIME-Version: 1.0
In-Reply-To: <3337086.qEUs9xMCTV@debian64>
References: <1553281120-22139-1-git-send-email-pozega.tomislav@gmail.com> <3337086.qEUs9xMCTV@debian64>
From:   Tom Psyborg <pozega.tomislav@gmail.com>
Date:   Wed, 17 Apr 2019 05:52:20 +0200
Message-ID: <CAKR_QVKvg3O19gz6zQRrcCjK=k=hvdmL4pys2KkQxAYAHs2qQQ@mail.gmail.com>
Subject: Re: [PATCH] ath10k: reset chip after supported check
To:     linux-wireless <linux-wireless@vger.kernel.org>
Cc:     openwrt-devel@lists.openwrt.org,
        Ben Greear <greearb@candelatech.com>,
        =?UTF-8?Q?Micha=C5=82_Kazior?= <kazikcz@gmail.com>,
        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

These replies never reached my inbox. Anyway scraped conversation off
the mailing list archive and CCing relevant people.

>> >>> 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.
>> >>
>> >> The crash is related to the v1 chip.  Is there a good way to detect
>> >> that this is the chip in question and only apply this work-around
>> >> for the problem chip?

yes there is. but the provided solution should be enough. which device
really depend on reset to probe sueccesfully? that device need other
fixes!

>> >
>> > I don't know of any good way to do that.
>> >
>> > You could consider device-tree but that would be no different from
>> > having a module blacklist in the C7v1 build recipe, or to not build
>> > the module at all. That is unless you actually want to make v1 chip
>> > work with ath10k at which point there's more fun to be had before it
>> > can actually work.

this is also intention. can it work with v2.0 fw?

>>
>> I remember v1, and I have no interest in trying to make it work :)
>>
>> If we could blacklist certain pci slots in the ath10k driver, I guess
>> that would work?
>>
>> I think the goal is to not use the v1 chip, but allow users to add a
>> v2 NIC to the platform, so driver still needs to load.
>
>That makes sense, but I don't see how blacklisting pci slots would
>help someone putting v2 nic into C7v1 mobo? Won't the slot be the same
>regardless what nic is put?
>
>The best thing I can come up with is something like this:
>
>--- a/drivers/net/wireless/ath/ath10k/pci.c
>+++ b/drivers/net/wireless/ath/ath10k/pci.c
>@@ -3629,6 +3629,19 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
>                goto err_deinit_irq;
>        }
>
>+       if (hw_rev =3D=3D ATH10K_HW_QCA988X) {
>+               /* v1 can crash the system on chip_reset()
>+                * so all we can do is keep our fingers
>+                * crossed v2 never reports 0 without a
>+                * chip_reset()
>+                */
>+               if (ath10k_pci_soc_read32(ar, SOC_CHIP_ID_ADDRESS) =3D=3D =
0) {
>+                       ath10k_err(ar, "qca9880 v1 is chip not supported")=
;
>+                       ret =3D -ENOTSUP;
>+                       goto err_free_irq;
>+               }
>+       }
>+
>        ret =3D ath10k_pci_chip_reset(ar);
>        if (ret) {
>                ath10k_err(ar, "failed to reset chip: %d\n", ret);
>
>I didn't test it. Someone needs to compile and test and make sure v2
>doesn't regress when fw hangs and cold & warm host cpu resets are
>mixed in.
>
>
>Micha=C5=82

this doesn't even fix the bootloop with v1