mhi.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To: Kalle Valo <kvalo@kernel.org>
Cc: ath11k@lists.infradead.org, mhi@lists.linux.dev,
	linux-wireless@vger.kernel.org, robert.marko@sartura.hr
Subject: Re: [PATCH RFC] ath11k: Add multiple QCN9074 devices support
Date: Thu, 12 Jan 2023 16:08:24 +0530	[thread overview]
Message-ID: <20230112103824.GD4782@thinkpad> (raw)
In-Reply-To: <20230111170033.32454-1-kvalo@kernel.org>

On Wed, Jan 11, 2023 at 07:00:33PM +0200, Kalle Valo wrote:
> From: P Praneesh <quic_ppranees@quicinc.com>
> 
> On platforms with two or more QCN9074 devices, the QMI service will run with
> identical QRTR ids. qmi_add_lookup() is called with same qmi.service_ins_id.

identical QRTR instance ID.

> Because of this identical id, the QMI .new_server is received on all ath11k QMI
> clients and the QMI state machine fails.
> 
> To solve this generate a unique instance id from PCIe domain number and bus
> number, write to the register ATH11K_PCIE_LOCAL_REG_PCIE_LOCAL_RSV0 before
> power up. It is available for firmware when the QMI service is spawned. As not
> all firmware releases support this feature add a new firmware feature flag
> ATH11K_FW_FEATURE_MULTI_QRTR_ID to detect if it's supported.
> 
> With this change, qrtr-lookup is shown in below example.
> 
> Example:
> root@OpenWrt:/# qrtr-lookup
> Service Version Instance Node Port
> 15 1 0 23 1 Test service
> 69 1 23 23 2 ATH10k WLAN firmware service
> 15 1 0 24 1 Test service
> 69 1 24 24 2 ATH10k WLAN firmware service
> 
> Here 23 and 24 on column 3 (QMI Instance ID) and column 4 (Node ID) are the
> node IDs that is unique per ath11k device.
> 
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.6.0.1-00760-QCAHKSWPL_SILICONZ-1
> 
> Co-developed-by: Anilkumar Kolli <quic_akolli@quicinc.com>
> Signed-off-by: Anilkumar Kolli <quic_akolli@quicinc.com>
> Signed-off-by: P Praneesh <quic_ppranees@quicinc.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>
> ---
> 
> Kalle's comments:
> 
> Depends on: https://patchwork.kernel.org/project/linux-wireless/list/?series=710862&order=date
> 
> It's also possible to do a simple test by just changing the test
> "test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" to true, no
> extra patches should be needed.
> 
> I don't have a test setup for this so compile tested only. But I assume Praneesh has tested this.
> 
> This is alternative approach to Robert's patchset:
> 
> https://patchwork.kernel.org/project/linux-wireless/list/?series=692423&state=*&order=date
> 

For what devices the compatible firmware is available? Any plan to fix the
devices with old firmware?

>  drivers/net/wireless/ath/ath11k/fw.h  |  5 +++++
>  drivers/net/wireless/ath/ath11k/pci.c | 31 +++++++++++++++++++++++++++
>  drivers/net/wireless/ath/ath11k/pci.h |  1 +
>  3 files changed, 37 insertions(+)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/fw.h b/drivers/net/wireless/ath/ath11k/fw.h
> index e33b0f78b571..94f50d675f8d 100644
> --- a/drivers/net/wireless/ath/ath11k/fw.h
> +++ b/drivers/net/wireless/ath/ath11k/fw.h
> @@ -17,6 +17,11 @@ enum ath11k_fw_ie_type {
>  };
>  
>  enum ath11k_fw_features {
> +	/* The firmware supports setting the QRTR id via register
> +	 * ATH11K_PCIE_LOCAL_REG_PCIE_LOCAL_RSV0.
> +	 */
> +	ATH11K_FW_FEATURE_MULTI_QRTR_ID = 0,
> +
>  	/* keep last */
>  	ATH11K_FW_FEATURE_COUNT,
>  };
> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index 776362d151cb..c42e1e8da624 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
> @@ -27,6 +27,8 @@
>  #define QCN9074_DEVICE_ID		0x1104
>  #define WCN6855_DEVICE_ID		0x1103
>  
> +#define ATH11K_PCIE_LOCAL_REG_PCIE_LOCAL_RSV0  0x1E03164
> +
>  static const struct pci_device_id ath11k_pci_id_table[] = {
>  	{ PCI_VDEVICE(QCOM, QCA6390_DEVICE_ID) },
>  	{ PCI_VDEVICE(QCOM, WCN6855_DEVICE_ID) },
> @@ -367,9 +369,14 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
>  	ath11k_mhi_set_mhictrl_reset(ab);
>  }
>  
> +#define ATH11K_QRTR_INSTANCE_PCI_DOMAIN		GENMASK(3, 0)
> +#define ATH11K_QRTR_INSTANCE_PCI_BUS_NUM	GENMASK(7, 4)
> +
>  static void ath11k_pci_init_qmi_ce_config(struct ath11k_base *ab)
>  {
>  	struct ath11k_qmi_ce_cfg *cfg = &ab->qmi.ce_cfg;
> +	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
> +	struct pci_bus *bus = ab_pci->pdev->bus;
>  
>  	cfg->tgt_ce = ab->hw_params.target_ce_config;
>  	cfg->tgt_ce_len = ab->hw_params.target_ce_count;
> @@ -380,6 +387,15 @@ static void ath11k_pci_init_qmi_ce_config(struct ath11k_base *ab)
>  
>  	ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
>  				    &cfg->shadow_reg_v2_len);
> +
> +	if (test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)) {
> +		ab_pci->instance_id =
> +			FIELD_PREP(ATH11K_QRTR_INSTANCE_PCI_DOMAIN,
> +				   pci_domain_nr(bus)) |
> +			FIELD_PREP(ATH11K_QRTR_INSTANCE_PCI_BUS_NUM,
> +				   bus->number);
> +		ab->qmi.service_ins_id += ab_pci->instance_id;
> +	}
>  }
>  
>  static void ath11k_pci_msi_config(struct ath11k_pci *ab_pci, bool enable)
> @@ -597,6 +613,18 @@ static void ath11k_pci_aspm_restore(struct ath11k_pci *ab_pci)
>  					   ab_pci->link_ctl);
>  }
>  
> +static void ath11k_pci_update_qrtr_node_id(struct ath11k_base *ab)

The function name says update node_id but instance_id is what getting updated.

Thanks,
Mani

> +{
> +	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
> +	u32 reg;
> +
> +	reg = ATH11K_PCIE_LOCAL_REG_PCIE_LOCAL_RSV0 & ATH11K_PCI_WINDOW_RANGE_MASK;
> +	ath11k_pcic_write32(ab, reg, ab_pci->instance_id);
> +
> +	ath11k_dbg(ab, ATH11K_DBG_PCI, "pci reg 0x%x instance_id 0x%x read val 0x%x\n",
> +		   reg, ab_pci->instance_id, ath11k_pcic_read32(ab, reg));
> +}
> +
>  static int ath11k_pci_power_up(struct ath11k_base *ab)
>  {
>  	struct ath11k_pci *ab_pci = ath11k_pci_priv(ab);
> @@ -613,6 +641,9 @@ static int ath11k_pci_power_up(struct ath11k_base *ab)
>  
>  	ath11k_pci_msi_enable(ab_pci);
>  
> +	if (test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features))
> +		ath11k_pci_update_qrtr_node_id(ab);
> +
>  	ret = ath11k_mhi_start(ab_pci);
>  	if (ret) {
>  		ath11k_err(ab, "failed to start mhi: %d\n", ret);
> diff --git a/drivers/net/wireless/ath/ath11k/pci.h b/drivers/net/wireless/ath/ath11k/pci.h
> index e9a01f344ec6..b9d4018d53c6 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.h
> +++ b/drivers/net/wireless/ath/ath11k/pci.h
> @@ -72,6 +72,7 @@ struct ath11k_pci {
>  	/* enum ath11k_pci_flags */
>  	unsigned long flags;
>  	u16 link_ctl;
> +	u32 instance_id;
>  };
>  
>  static inline struct ath11k_pci *ath11k_pci_priv(struct ath11k_base *ab)
> -- 
> 2.30.2
> 
> 

-- 
மணிவண்ணன் சதாசிவம்

  reply	other threads:[~2023-01-12 10:38 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-01-11 17:00 [PATCH RFC] ath11k: Add multiple QCN9074 devices support Kalle Valo
2023-01-12 10:38 ` Manivannan Sadhasivam [this message]
2023-01-13 12:34   ` Kalle Valo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20230112103824.GD4782@thinkpad \
    --to=manivannan.sadhasivam@linaro.org \
    --cc=ath11k@lists.infradead.org \
    --cc=kvalo@kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=mhi@lists.linux.dev \
    --cc=robert.marko@sartura.hr \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).