Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220207093023.10605-1-quic_bqiang@quicinc.com>
In-Reply-To: <20220207093023.10605-1-quic_bqiang@quicinc.com>
From:   Abhishek Kumar <kuabhs@chromium.org>
Date:   Wed, 9 Feb 2022 17:05:23 -0800
Message-ID: <CACTWRwugAHA87-AuEkC7tXEEuOhef_FrWBjjk4pQXZvpmHQ65A@mail.gmail.com>
Subject: Re: [PATCH] ath11k: Reset PCIE_GPIO_CFG_REG register when power on
To:     Baochen Qiang <quic_bqiang@quicinc.com>
Cc:     ath11k@lists.infradead.org, linux-wireless@vger.kernel.org,
        Abhishek Kumar <kuabhs@chromium.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Mon, Feb 7, 2022 at 1:41 AM Baochen Qiang <quic_bqiang@quicinc.com> wrote:
>
> On some modules, PCIE_GPIO_CFG_REG is not initialized to right value,
> this will cause WCN6855 hardware fails to be enumerated.
>
> Fix it by force writing the correct value to this register when power
> on.
>
> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1
Can you elaborate how you tested this ? I can see due to this patch
shows resource temporarily unavailable after running simulated wifi
crash in a loop.
>
> Signed-off-by: Baochen Qiang <quic_bqiang@quicinc.com>
> ---
>  drivers/net/wireless/ath/ath11k/pci.c | 11 +++++++++++
>  drivers/net/wireless/ath/ath11k/pci.h |  3 +++
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index d73b522a0081..06968ad488b0 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
> @@ -445,6 +445,16 @@ static void ath11k_pci_force_wake(struct ath11k_base *ab)
>         mdelay(5);
>  }
>
> +static void ath11k_pci_gpio_reset(struct ath11k_base *ab)
> +{
> +       int val;
> +
> +       ath11k_pci_write32(ab, PCIE_GPIO_CFG_REG, PCIE_GPIO_RESET_VAL);
> +       mdelay(10);
> +       val = ath11k_pci_read32(ab, PCIE_GPIO_CFG_REG);
> +       ath11k_dbg(ab, ATH11K_DBG_PCI, "gpio cfg: 0x%x\n", val);
> +}
Looks like you have added delay before reading which gets printed as a
debug log. In this case, I don't think you should add the
unconditional delay and read the register unconditionally but rather
should do only if debug is enabled. Thought ?
> +
>  static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
>  {
>         mdelay(100);
> @@ -461,6 +471,7 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
>         ath11k_pci_clear_dbg_registers(ab);
>         ath11k_pci_soc_global_reset(ab);
>         ath11k_mhi_set_mhictrl_reset(ab);
> +       ath11k_pci_gpio_reset(ab);
>  }
>
>  int ath11k_pci_get_msi_irq(struct device *dev, unsigned int vector)
> diff --git a/drivers/net/wireless/ath/ath11k/pci.h b/drivers/net/wireless/ath/ath11k/pci.h
> index 61d67b20a0eb..2716fc7745d6 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.h
> +++ b/drivers/net/wireless/ath/ath11k/pci.h
> @@ -52,6 +52,9 @@
>  #define WLAON_QFPROM_PWR_CTRL_REG              0x01f8031c
>  #define QFPROM_PWR_CTRL_VDD4BLOW_MASK          0x4
>
> +#define PCIE_GPIO_CFG_REG                      0x0180e000
> +#define PCIE_GPIO_RESET_VAL                    0xc5
> +
>  struct ath11k_msi_user {
>         char *name;
>         int num_vectors;
> --
> 2.25.1
>

Thanks
Abhishek