[2/2] wifi: ath11k: use unique QRTR instance ID

Commit Message

Robert Marko Nov. 5, 2022, 7:49 p.m. UTC

Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
will cause a clash in the QRTR instance node ID and prevent the driver
from talking via QMI to the card and thus initializing it with:
[    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
[    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22

So, in order to allow for this combination of cards, especially AHB + PCI
cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
QRTR instance ID offset by calculating a unique one based on PCI domain
and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
using the SBL state callback that is added as part of the series.
We also have to make sure that new QRTR offset is added on top of the
default QRTR instance ID-s that are currently used in the driver.

This finally allows using AHB + PCI or multiple PCI cards on the same
system.

Before:
root@OpenWrt:/# qrtr-lookup
  Service Version Instance Node  Port
     1054       1        0    7     1 <unknown>
       69       1        2    7     3 ATH10k WLAN firmware service

After:
root@OpenWrt:/# qrtr-lookup
  Service Version Instance Node  Port
     1054       1        0    7     1 <unknown>
       69       1        2    7     3 ATH10k WLAN firmware service
       15       1        0    8     1 Test service
       69       1        8    8     2 ATH10k WLAN firmware service

Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1

Signed-off-by: Robert Marko <[e-mail]>
---
 drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
 drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
 drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
 3 files changed, 38 insertions(+), 17 deletions(-)

Comments

Jeffrey Hugo Nov. 7, 2022, 3:09 p.m. UTC | #1

On 11/5/2022 1:49 PM, Robert Marko wrote:
> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
> will cause a clash in the QRTR instance node ID and prevent the driver
> from talking via QMI to the card and thus initializing it with:
> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
> 
> So, in order to allow for this combination of cards, especially AHB + PCI
> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
> QRTR instance ID offset by calculating a unique one based on PCI domain
> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
> using the SBL state callback that is added as part of the series.
> We also have to make sure that new QRTR offset is added on top of the
> default QRTR instance ID-s that are currently used in the driver.
> 
> This finally allows using AHB + PCI or multiple PCI cards on the same
> system.
> 
> Before:
> root@OpenWrt:/# qrtr-lookup
>    Service Version Instance Node  Port
>       1054       1        0    7     1 <unknown>
>         69       1        2    7     3 ATH10k WLAN firmware service
> 
> After:
> root@OpenWrt:/# qrtr-lookup
>    Service Version Instance Node  Port
>       1054       1        0    7     1 <unknown>
>         69       1        2    7     3 ATH10k WLAN firmware service
>         15       1        0    8     1 Test service
>         69       1        8    8     2 ATH10k WLAN firmware service
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Robert Marko <[e-mail]>
> ---
>   drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
>   drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
>   drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
>   3 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> index 86995e8dc913..23e85ea902f5 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.c
> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
>   {
>   }
>   
> +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> +				  void __iomem *addr,
> +				  u32 *out)
> +{
> +	*out = readl(addr);
> +
> +	return 0;
> +}
> +
> +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> +				    void __iomem *addr,
> +				    u32 val)
> +{
> +	writel(val, addr);
> +}
> +
> +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
> +{
> +	struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
> +
> +	ath11k_mhi_op_write_reg(mhi_cntrl,
> +				mhi_cntrl->bhi + BHI_ERRDBG2,
> +				FIELD_PREP(QRTR_INSTANCE_MASK,
> +				ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
> +}

What kind of synchronization is there for this?

Does SBL spin until this is set?

What would prevent SBL from booting, sending the notification to the 
host, and then quickly entering runtime mode before the host had a 
chance to get here?


> +
>   static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>   {
>   	switch (reason) {
> @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>   		return "MHI_CB_FATAL_ERROR";
>   	case MHI_CB_BW_REQ:
>   		return "MHI_CB_BW_REQ";
> +	case MHI_CB_EE_SBL_MODE:
> +		return "MHI_CB_EE_SBL_MODE";
>   	default:
>   		return "UNKNOWN";
>   	}
> @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
>   		if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
>   			queue_work(ab->workqueue_aux, &ab->reset_work);
>   		break;
> +	case MHI_CB_EE_SBL_MODE:
> +		ath11k_mhi_qrtr_instance_set(mhi_cntrl);
> +		break;
>   	default:
>   		break;
>   	}
>   }
>   
> -static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> -				  void __iomem *addr,
> -				  u32 *out)
> -{
> -	*out = readl(addr);
> -
> -	return 0;
> -}
> -
> -static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> -				    void __iomem *addr,
> -				    u32 val)
> -{
> -	writel(val, addr);
> -}
> -
>   static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
>   {
>   	struct device_node *np;
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
> index 8d9f852da695..0db308bc3047 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.h
> +++ b/drivers/net/wireless/ath/ath11k/mhi.h
> @@ -16,6 +16,9 @@
>   #define MHICTRL					0x38
>   #define MHICTRL_RESET_MASK			0x2
>   
> +#define BHI_ERRDBG2				0x38
> +#define QRTR_INSTANCE_MASK			GENMASK(7, 0)
> +
>   int ath11k_mhi_start(struct ath11k_pci *ar_pci);
>   void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
>   int ath11k_mhi_register(struct ath11k_pci *ar_pci);
> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index 99cf3357c66e..cd26c1567415 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
> @@ -370,13 +370,16 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
>   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;
>   
>   	cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
>   	cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
> -	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> +	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
> +	(((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
>   
>   	ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
>   				    &cfg->shadow_reg_v2_len);

Robert Marko Nov. 7, 2022, 5:15 p.m. UTC | #2

On Mon, 7 Nov 2022 at 16:10, Jeffrey Hugo <[e-mail]> wrote:
>
> On 11/5/2022 1:49 PM, Robert Marko wrote:
> > Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
> > will cause a clash in the QRTR instance node ID and prevent the driver
> > from talking via QMI to the card and thus initializing it with:
> > [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
> > [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
> >
> > So, in order to allow for this combination of cards, especially AHB + PCI
> > cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
> > QRTR instance ID offset by calculating a unique one based on PCI domain
> > and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
> > using the SBL state callback that is added as part of the series.
> > We also have to make sure that new QRTR offset is added on top of the
> > default QRTR instance ID-s that are currently used in the driver.
> >
> > This finally allows using AHB + PCI or multiple PCI cards on the same
> > system.
> >
> > Before:
> > root@OpenWrt:/# qrtr-lookup
> >    Service Version Instance Node  Port
> >       1054       1        0    7     1 <unknown>
> >         69       1        2    7     3 ATH10k WLAN firmware service
> >
> > After:
> > root@OpenWrt:/# qrtr-lookup
> >    Service Version Instance Node  Port
> >       1054       1        0    7     1 <unknown>
> >         69       1        2    7     3 ATH10k WLAN firmware service
> >         15       1        0    8     1 Test service
> >         69       1        8    8     2 ATH10k WLAN firmware service
> >
> > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> >
> > Signed-off-by: Robert Marko <[e-mail]>
> > ---
> >   drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
> >   drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
> >   drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
> >   3 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> > index 86995e8dc913..23e85ea902f5 100644
> > --- a/drivers/net/wireless/ath/ath11k/mhi.c
> > +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> > @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
> >   {
> >   }
> >
> > +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> > +                               void __iomem *addr,
> > +                               u32 *out)
> > +{
> > +     *out = readl(addr);
> > +
> > +     return 0;
> > +}
> > +
> > +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> > +                                 void __iomem *addr,
> > +                                 u32 val)
> > +{
> > +     writel(val, addr);
> > +}
> > +
> > +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
> > +{
> > +     struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
> > +
> > +     ath11k_mhi_op_write_reg(mhi_cntrl,
> > +                             mhi_cntrl->bhi + BHI_ERRDBG2,
> > +                             FIELD_PREP(QRTR_INSTANCE_MASK,
> > +                             ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
> > +}
>
> What kind of synchronization is there for this?

None from what I could tell.
>
> Does SBL spin until this is set?

No, the default value is 0x0 and it will boot with that.
>
> What would prevent SBL from booting, sending the notification to the
> host, and then quickly entering runtime mode before the host had a
> chance to get here?

As far as I know nothing really, but this is a question for QCA.
I have tried to make a generic solution from various bits and pieces they
are using downstream but which are really not suitable for upstream.

I agree it's not ideal, but the worst that could happen is that card
won't work which is current
behavior anyway.

Not being able to use AHB + PCI or multiple PCI cards is really
annoying as I am not able
to utilize most of the 5GHz spectrum on my router due to this.

Regards,
Robert
>
>
> > +
> >   static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
> >   {
> >       switch (reason) {
> > @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
> >               return "MHI_CB_FATAL_ERROR";
> >       case MHI_CB_BW_REQ:
> >               return "MHI_CB_BW_REQ";
> > +     case MHI_CB_EE_SBL_MODE:
> > +             return "MHI_CB_EE_SBL_MODE";
> >       default:
> >               return "UNKNOWN";
> >       }
> > @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
> >               if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
> >                       queue_work(ab->workqueue_aux, &ab->reset_work);
> >               break;
> > +     case MHI_CB_EE_SBL_MODE:
> > +             ath11k_mhi_qrtr_instance_set(mhi_cntrl);
> > +             break;
> >       default:
> >               break;
> >       }
> >   }
> >
> > -static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> > -                               void __iomem *addr,
> > -                               u32 *out)
> > -{
> > -     *out = readl(addr);
> > -
> > -     return 0;
> > -}
> > -
> > -static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> > -                                 void __iomem *addr,
> > -                                 u32 val)
> > -{
> > -     writel(val, addr);
> > -}
> > -
> >   static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
> >   {
> >       struct device_node *np;
> > diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
> > index 8d9f852da695..0db308bc3047 100644
> > --- a/drivers/net/wireless/ath/ath11k/mhi.h
> > +++ b/drivers/net/wireless/ath/ath11k/mhi.h
> > @@ -16,6 +16,9 @@
> >   #define MHICTRL                                     0x38
> >   #define MHICTRL_RESET_MASK                  0x2
> >
> > +#define BHI_ERRDBG2                          0x38
> > +#define QRTR_INSTANCE_MASK                   GENMASK(7, 0)
> > +
> >   int ath11k_mhi_start(struct ath11k_pci *ar_pci);
> >   void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
> >   int ath11k_mhi_register(struct ath11k_pci *ar_pci);
> > diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> > index 99cf3357c66e..cd26c1567415 100644
> > --- a/drivers/net/wireless/ath/ath11k/pci.c
> > +++ b/drivers/net/wireless/ath/ath11k/pci.c
> > @@ -370,13 +370,16 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
> >   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;
> >
> >       cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
> >       cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
> > -     ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> > +     ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
> > +     (((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
> >
> >       ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
> >                                   &cfg->shadow_reg_v2_len);
>

Manivannan Sadhasivam Nov. 7, 2022, 5:47 p.m. UTC | #3

On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
> will cause a clash in the QRTR instance node ID and prevent the driver
> from talking via QMI to the card and thus initializing it with:
> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
> 

There is still an outstanding issue where you cannot connect two WLAN modules
with same node id.

> So, in order to allow for this combination of cards, especially AHB + PCI
> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
> QRTR instance ID offset by calculating a unique one based on PCI domain
> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
> using the SBL state callback that is added as part of the series.
> We also have to make sure that new QRTR offset is added on top of the
> default QRTR instance ID-s that are currently used in the driver.
> 

Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
So I'm not sure if this solution is going to work on all ath11k supported
chipsets.

Kalle, can you confirm?

> This finally allows using AHB + PCI or multiple PCI cards on the same
> system.
> 
> Before:
> root@OpenWrt:/# qrtr-lookup
>   Service Version Instance Node  Port
>      1054       1        0    7     1 <unknown>
>        69       1        2    7     3 ATH10k WLAN firmware service
> 
> After:
> root@OpenWrt:/# qrtr-lookup
>   Service Version Instance Node  Port
>      1054       1        0    7     1 <unknown>
>        69       1        2    7     3 ATH10k WLAN firmware service
>        15       1        0    8     1 Test service
>        69       1        8    8     2 ATH10k WLAN firmware service
> 
> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> 
> Signed-off-by: Robert Marko <[e-mail]>
> ---
>  drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
>  drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
>  drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
>  3 files changed, 38 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> index 86995e8dc913..23e85ea902f5 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.c
> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
>  {
>  }
>  
> +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> +				  void __iomem *addr,
> +				  u32 *out)
> +{
> +	*out = readl(addr);
> +
> +	return 0;
> +}
> +
> +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> +				    void __iomem *addr,
> +				    u32 val)
> +{
> +	writel(val, addr);
> +}
> +
> +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
> +{
> +	struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
> +
> +	ath11k_mhi_op_write_reg(mhi_cntrl,
> +				mhi_cntrl->bhi + BHI_ERRDBG2,
> +				FIELD_PREP(QRTR_INSTANCE_MASK,
> +				ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
> +}
> +
>  static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>  {
>  	switch (reason) {
> @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>  		return "MHI_CB_FATAL_ERROR";
>  	case MHI_CB_BW_REQ:
>  		return "MHI_CB_BW_REQ";
> +	case MHI_CB_EE_SBL_MODE:
> +		return "MHI_CB_EE_SBL_MODE";
>  	default:
>  		return "UNKNOWN";
>  	}
> @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
>  		if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
>  			queue_work(ab->workqueue_aux, &ab->reset_work);
>  		break;
> +	case MHI_CB_EE_SBL_MODE:
> +		ath11k_mhi_qrtr_instance_set(mhi_cntrl);

I still don't understand how SBL could make use of this information during
boot without waiting for an update.

Thanks,
Mani

> +		break;
>  	default:
>  		break;
>  	}
>  }
>  
> -static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> -				  void __iomem *addr,
> -				  u32 *out)
> -{
> -	*out = readl(addr);
> -
> -	return 0;
> -}
> -
> -static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> -				    void __iomem *addr,
> -				    u32 val)
> -{
> -	writel(val, addr);
> -}
> -
>  static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
>  {
>  	struct device_node *np;
> diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
> index 8d9f852da695..0db308bc3047 100644
> --- a/drivers/net/wireless/ath/ath11k/mhi.h
> +++ b/drivers/net/wireless/ath/ath11k/mhi.h
> @@ -16,6 +16,9 @@
>  #define MHICTRL					0x38
>  #define MHICTRL_RESET_MASK			0x2
>  
> +#define BHI_ERRDBG2				0x38
> +#define QRTR_INSTANCE_MASK			GENMASK(7, 0)
> +
>  int ath11k_mhi_start(struct ath11k_pci *ar_pci);
>  void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
>  int ath11k_mhi_register(struct ath11k_pci *ar_pci);
> diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> index 99cf3357c66e..cd26c1567415 100644
> --- a/drivers/net/wireless/ath/ath11k/pci.c
> +++ b/drivers/net/wireless/ath/ath11k/pci.c
> @@ -370,13 +370,16 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
>  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;
>  
>  	cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
>  	cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
> -	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> +	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
> +	(((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
>  
>  	ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
>  				    &cfg->shadow_reg_v2_len);
> -- 
> 2.38.1
> 
>

Robert Marko Nov. 7, 2022, 5:52 p.m. UTC | #4

On Mon, 7 Nov 2022 at 18:47, Manivannan Sadhasivam <[e-mail]> wrote:
>
> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
> > Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
> > will cause a clash in the QRTR instance node ID and prevent the driver
> > from talking via QMI to the card and thus initializing it with:
> > [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
> > [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
> >
>
> There is still an outstanding issue where you cannot connect two WLAN modules
> with same node id.

Yes, but as far as I can understand QRTR that is never gonna be
possible, node ID-s
must be different, but I dont have any docs at all.

>
> > So, in order to allow for this combination of cards, especially AHB + PCI
> > cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
> > QRTR instance ID offset by calculating a unique one based on PCI domain
> > and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
> > using the SBL state callback that is added as part of the series.
> > We also have to make sure that new QRTR offset is added on top of the
> > default QRTR instance ID-s that are currently used in the driver.
> >
>
> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
> So I'm not sure if this solution is going to work on all ath11k supported
> chipsets.
>
> Kalle, can you confirm?
>
> > This finally allows using AHB + PCI or multiple PCI cards on the same
> > system.
> >
> > Before:
> > root@OpenWrt:/# qrtr-lookup
> >   Service Version Instance Node  Port
> >      1054       1        0    7     1 <unknown>
> >        69       1        2    7     3 ATH10k WLAN firmware service
> >
> > After:
> > root@OpenWrt:/# qrtr-lookup
> >   Service Version Instance Node  Port
> >      1054       1        0    7     1 <unknown>
> >        69       1        2    7     3 ATH10k WLAN firmware service
> >        15       1        0    8     1 Test service
> >        69       1        8    8     2 ATH10k WLAN firmware service
> >
> > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
> >
> > Signed-off-by: Robert Marko <[e-mail]>
> > ---
> >  drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
> >  drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
> >  drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
> >  3 files changed, 38 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
> > index 86995e8dc913..23e85ea902f5 100644
> > --- a/drivers/net/wireless/ath/ath11k/mhi.c
> > +++ b/drivers/net/wireless/ath/ath11k/mhi.c
> > @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
> >  {
> >  }
> >
> > +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> > +                               void __iomem *addr,
> > +                               u32 *out)
> > +{
> > +     *out = readl(addr);
> > +
> > +     return 0;
> > +}
> > +
> > +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> > +                                 void __iomem *addr,
> > +                                 u32 val)
> > +{
> > +     writel(val, addr);
> > +}
> > +
> > +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
> > +{
> > +     struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
> > +
> > +     ath11k_mhi_op_write_reg(mhi_cntrl,
> > +                             mhi_cntrl->bhi + BHI_ERRDBG2,
> > +                             FIELD_PREP(QRTR_INSTANCE_MASK,
> > +                             ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
> > +}
> > +
> >  static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
> >  {
> >       switch (reason) {
> > @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
> >               return "MHI_CB_FATAL_ERROR";
> >       case MHI_CB_BW_REQ:
> >               return "MHI_CB_BW_REQ";
> > +     case MHI_CB_EE_SBL_MODE:
> > +             return "MHI_CB_EE_SBL_MODE";
> >       default:
> >               return "UNKNOWN";
> >       }
> > @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
> >               if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
> >                       queue_work(ab->workqueue_aux, &ab->reset_work);
> >               break;
> > +     case MHI_CB_EE_SBL_MODE:
> > +             ath11k_mhi_qrtr_instance_set(mhi_cntrl);
>
> I still don't understand how SBL could make use of this information during
> boot without waiting for an update.

Me neither, but it works reliably as long as it's updated once SBL is live.
Trying to do it earlier or later does nothing, it will just use the
default node ID then.

Regards,
Robert
>
> Thanks,
> Mani
>
> > +             break;
> >       default:
> >               break;
> >       }
> >  }
> >
> > -static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
> > -                               void __iomem *addr,
> > -                               u32 *out)
> > -{
> > -     *out = readl(addr);
> > -
> > -     return 0;
> > -}
> > -
> > -static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
> > -                                 void __iomem *addr,
> > -                                 u32 val)
> > -{
> > -     writel(val, addr);
> > -}
> > -
> >  static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
> >  {
> >       struct device_node *np;
> > diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
> > index 8d9f852da695..0db308bc3047 100644
> > --- a/drivers/net/wireless/ath/ath11k/mhi.h
> > +++ b/drivers/net/wireless/ath/ath11k/mhi.h
> > @@ -16,6 +16,9 @@
> >  #define MHICTRL                                      0x38
> >  #define MHICTRL_RESET_MASK                   0x2
> >
> > +#define BHI_ERRDBG2                          0x38
> > +#define QRTR_INSTANCE_MASK                   GENMASK(7, 0)
> > +
> >  int ath11k_mhi_start(struct ath11k_pci *ar_pci);
> >  void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
> >  int ath11k_mhi_register(struct ath11k_pci *ar_pci);
> > diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
> > index 99cf3357c66e..cd26c1567415 100644
> > --- a/drivers/net/wireless/ath/ath11k/pci.c
> > +++ b/drivers/net/wireless/ath/ath11k/pci.c
> > @@ -370,13 +370,16 @@ static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
> >  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;
> >
> >       cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
> >       cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
> > -     ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
> > +     ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
> > +     (((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
> >
> >       ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
> >                                   &cfg->shadow_reg_v2_len);
> > --
> > 2.38.1
> >
> >
>
> --
> மணிவண்ணன் சதாசிவம்

Jeffrey Hugo Nov. 7, 2022, 6:02 p.m. UTC | #5

On 11/7/2022 10:52 AM, Robert Marko wrote:
> On Mon, 7 Nov 2022 at 18:47, Manivannan Sadhasivam <[e-mail]> wrote:
>>
>> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
>>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
>>> will cause a clash in the QRTR instance node ID and prevent the driver
>>> from talking via QMI to the card and thus initializing it with:
>>> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
>>> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
>>>
>>
>> There is still an outstanding issue where you cannot connect two WLAN modules
>> with same node id.
> 
> Yes, but as far as I can understand QRTR that is never gonna be
> possible, node ID-s
> must be different, but I dont have any docs at all.
> 
>>
>>> So, in order to allow for this combination of cards, especially AHB + PCI
>>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
>>> QRTR instance ID offset by calculating a unique one based on PCI domain
>>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
>>> using the SBL state callback that is added as part of the series.
>>> We also have to make sure that new QRTR offset is added on top of the
>>> default QRTR instance ID-s that are currently used in the driver.
>>>
>>
>> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
>> So I'm not sure if this solution is going to work on all ath11k supported
>> chipsets.
>>
>> Kalle, can you confirm?
>>
>>> This finally allows using AHB + PCI or multiple PCI cards on the same
>>> system.
>>>
>>> Before:
>>> root@OpenWrt:/# qrtr-lookup
>>>    Service Version Instance Node  Port
>>>       1054       1        0    7     1 <unknown>
>>>         69       1        2    7     3 ATH10k WLAN firmware service
>>>
>>> After:
>>> root@OpenWrt:/# qrtr-lookup
>>>    Service Version Instance Node  Port
>>>       1054       1        0    7     1 <unknown>
>>>         69       1        2    7     3 ATH10k WLAN firmware service
>>>         15       1        0    8     1 Test service
>>>         69       1        8    8     2 ATH10k WLAN firmware service
>>>
>>> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
>>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01208-QCAHKSWPL_SILICONZ-1
>>>
>>> Signed-off-by: Robert Marko <[e-mail]>
>>> ---
>>>   drivers/net/wireless/ath/ath11k/mhi.c | 47 ++++++++++++++++++---------
>>>   drivers/net/wireless/ath/ath11k/mhi.h |  3 ++
>>>   drivers/net/wireless/ath/ath11k/pci.c |  5 ++-
>>>   3 files changed, 38 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
>>> index 86995e8dc913..23e85ea902f5 100644
>>> --- a/drivers/net/wireless/ath/ath11k/mhi.c
>>> +++ b/drivers/net/wireless/ath/ath11k/mhi.c
>>> @@ -294,6 +294,32 @@ static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
>>>   {
>>>   }
>>>
>>> +static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
>>> +                               void __iomem *addr,
>>> +                               u32 *out)
>>> +{
>>> +     *out = readl(addr);
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
>>> +                                 void __iomem *addr,
>>> +                                 u32 val)
>>> +{
>>> +     writel(val, addr);
>>> +}
>>> +
>>> +static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
>>> +{
>>> +     struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
>>> +
>>> +     ath11k_mhi_op_write_reg(mhi_cntrl,
>>> +                             mhi_cntrl->bhi + BHI_ERRDBG2,
>>> +                             FIELD_PREP(QRTR_INSTANCE_MASK,
>>> +                             ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
>>> +}
>>> +
>>>   static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>>>   {
>>>        switch (reason) {
>>> @@ -315,6 +341,8 @@ static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
>>>                return "MHI_CB_FATAL_ERROR";
>>>        case MHI_CB_BW_REQ:
>>>                return "MHI_CB_BW_REQ";
>>> +     case MHI_CB_EE_SBL_MODE:
>>> +             return "MHI_CB_EE_SBL_MODE";
>>>        default:
>>>                return "UNKNOWN";
>>>        }
>>> @@ -336,27 +364,14 @@ static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
>>>                if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
>>>                        queue_work(ab->workqueue_aux, &ab->reset_work);
>>>                break;
>>> +     case MHI_CB_EE_SBL_MODE:
>>> +             ath11k_mhi_qrtr_instance_set(mhi_cntrl);
>>
>> I still don't understand how SBL could make use of this information during
>> boot without waiting for an update.
> 
> Me neither, but it works reliably as long as it's updated once SBL is live.
> Trying to do it earlier or later does nothing, it will just use the
> default node ID then.

If I recall correctly, PBL will write to the register at the end of the 
BHI process, even in the success case.  So, you have a race condition 
where you need to update the register after BHI is complete, but before 
SBL reads it.

If the value is just based on the BDF, I don't see why the device 
couldn't compute it without input from the host.  This whole mechanism 
seems pretty poorly designed, but sadly I don't have any brilliant ideas 
on how to fix it for you.

Kalle Valo Nov. 8, 2022, 5:24 p.m. UTC | #6

Manivannan Sadhasivam <[e-mail]> writes:

> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
>> will cause a clash in the QRTR instance node ID and prevent the driver
>> from talking via QMI to the card and thus initializing it with:
>> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
>> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
>> 
>
> There is still an outstanding issue where you cannot connect two WLAN modules
> with same node id.
>
>> So, in order to allow for this combination of cards, especially AHB + PCI
>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
>> QRTR instance ID offset by calculating a unique one based on PCI domain
>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
>> using the SBL state callback that is added as part of the series.
>> We also have to make sure that new QRTR offset is added on top of the
>> default QRTR instance ID-s that are currently used in the driver.
>> 
>
> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
> So I'm not sure if this solution is going to work on all ath11k supported
> chipsets.
>
> Kalle, can you confirm?

I can't look at this in detail right now, but hopefully in few days.
I'll get back to you.

Kalle Valo Nov. 22, 2022, 11:26 a.m. UTC | #7

Kalle Valo <[e-mail]> writes:

> Manivannan Sadhasivam <[e-mail]> writes:
>
>> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
>>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
>>> will cause a clash in the QRTR instance node ID and prevent the driver
>>> from talking via QMI to the card and thus initializing it with:
>>> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
>>> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
>>> 
>>
>> There is still an outstanding issue where you cannot connect two WLAN modules
>> with same node id.
>>
>>> So, in order to allow for this combination of cards, especially AHB + PCI
>>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
>>> QRTR instance ID offset by calculating a unique one based on PCI domain
>>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
>>> using the SBL state callback that is added as part of the series.
>>> We also have to make sure that new QRTR offset is added on top of the
>>> default QRTR instance ID-s that are currently used in the driver.
>>> 
>>
>> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
>> So I'm not sure if this solution is going to work on all ath11k supported
>> chipsets.
>>
>> Kalle, can you confirm?
>
> I can't look at this in detail right now, but hopefully in few days.
> I'll get back to you.

The solution we have been thinking internally would not use
MHI_CB_EE_SBL_MODE at all, it's not clear for me yet why the mode was
not needed in our solution. Maybe there are firmware modifications? I
think it's best that we submit our proposal as well, then we can then
compare implementations and see what is the best course of action.

But it looks that not all ath11k hardware and firmware releases support
this feature, we would need meta data information from the firmware to
detect it. I am working on adding firmware meta data support[1] to
ath11k, will post patches for that "soon".

[1] similar to firmware-N.bin support ath10k has

Robert Marko Dec. 14, 2022, 12:02 p.m. UTC | #8

On Tue, Nov 22, 2022 at 12:26 PM Kalle Valo <[e-mail]> wrote:
>
> Kalle Valo <[e-mail]> writes:
>
> > Manivannan Sadhasivam <[e-mail]> writes:
> >
> >> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
> >>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
> >>> will cause a clash in the QRTR instance node ID and prevent the driver
> >>> from talking via QMI to the card and thus initializing it with:
> >>> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
> >>> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
> >>>
> >>
> >> There is still an outstanding issue where you cannot connect two WLAN modules
> >> with same node id.
> >>
> >>> So, in order to allow for this combination of cards, especially AHB + PCI
> >>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
> >>> QRTR instance ID offset by calculating a unique one based on PCI domain
> >>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
> >>> using the SBL state callback that is added as part of the series.
> >>> We also have to make sure that new QRTR offset is added on top of the
> >>> default QRTR instance ID-s that are currently used in the driver.
> >>>
> >>
> >> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
> >> So I'm not sure if this solution is going to work on all ath11k supported
> >> chipsets.
> >>
> >> Kalle, can you confirm?
> >
> > I can't look at this in detail right now, but hopefully in few days.
> > I'll get back to you.
>
> The solution we have been thinking internally would not use
> MHI_CB_EE_SBL_MODE at all, it's not clear for me yet why the mode was
> not needed in our solution. Maybe there are firmware modifications? I
> think it's best that we submit our proposal as well, then we can then
> compare implementations and see what is the best course of action.

Kalle, any ETA when you will post your idea?
I am constantly hitting this crazy limitation and my idea does not work on cards
like QCA6390 so it's not a viable workaround at all.

Regards,
Robert
>
> But it looks that not all ath11k hardware and firmware releases support
> this feature, we would need meta data information from the firmware to
> detect it. I am working on adding firmware meta data support[1] to
> ath11k, will post patches for that "soon".
>
> [1] similar to firmware-N.bin support ath10k has
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>
> --
> ath11k mailing list
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/ath11k

Kalle Valo Dec. 22, 2022, 1:57 p.m. UTC | #9

Robert Marko <[e-mail]> writes:

> On Tue, Nov 22, 2022 at 12:26 PM Kalle Valo <[e-mail]> wrote:
>
>>
>> Kalle Valo <[e-mail]> writes:
>>
>> > Manivannan Sadhasivam <[e-mail]> writes:
>> >
>> >> On Sat, Nov 05, 2022 at 08:49:43PM +0100, Robert Marko wrote:
>> >>> Currently, trying to use AHB + PCI/MHI cards or multiple PCI/MHI cards
>> >>> will cause a clash in the QRTR instance node ID and prevent the driver
>> >>> from talking via QMI to the card and thus initializing it with:
>> >>> [    9.836329] ath11k c000000.wifi: host capability request failed: 1 90
>> >>> [    9.842047] ath11k c000000.wifi: failed to send qmi host cap: -22
>> >>>
>> >>
>> >> There is still an outstanding issue where you cannot connect two WLAN modules
>> >> with same node id.
>> >>
>> >>> So, in order to allow for this combination of cards, especially AHB + PCI
>> >>> cards like IPQ8074 + QCN9074 (Used by me and tested on) set the desired
>> >>> QRTR instance ID offset by calculating a unique one based on PCI domain
>> >>> and bus ID-s and writing it to bits 7-0 of BHI_ERRDBG2 MHI register by
>> >>> using the SBL state callback that is added as part of the series.
>> >>> We also have to make sure that new QRTR offset is added on top of the
>> >>> default QRTR instance ID-s that are currently used in the driver.
>> >>>
>> >>
>> >> Register BHI_ERRDBG2 is listed as Read only from Host as per the BHI spec.
>> >> So I'm not sure if this solution is going to work on all ath11k supported
>> >> chipsets.
>> >>
>> >> Kalle, can you confirm?
>> >
>> > I can't look at this in detail right now, but hopefully in few days.
>> > I'll get back to you.
>>
>> The solution we have been thinking internally would not use
>> MHI_CB_EE_SBL_MODE at all, it's not clear for me yet why the mode was
>> not needed in our solution. Maybe there are firmware modifications? I
>> think it's best that we submit our proposal as well, then we can then
>> compare implementations and see what is the best course of action.
>
> Kalle, any ETA when you will post your idea? I am constantly hitting
> this crazy limitation and my idea does not work on cards like QCA6390
> so it's not a viable workaround at all.

Really sorry, I just didn't manage to get this finalised due to other
stuff and now I'm leaving for a two week vacation :(

Robert Marko Jan. 11, 2023, 9:21 a.m. UTC | #10

> Really sorry, I just didn't manage to get this finalised due to other
> stuff and now I'm leaving for a two week vacation :(

Any news regarding this, I have a PR for ipq807x support in OpenWrt
and the current workaround for supporting AHB + PCI or multiple PCI
cards is breaking cards like QCA6390 which are obviously really
popular.

Regards,
Robert

Kalle Valo Jan. 11, 2023, 5:09 p.m. UTC | #11

Robert Marko <[e-mail]> writes:

>> Really sorry, I just didn't manage to get this finalised due to other
>> stuff and now I'm leaving for a two week vacation :(
>
> Any news regarding this, I have a PR for ipq807x support in OpenWrt
> and the current workaround for supporting AHB + PCI or multiple PCI
> cards is breaking cards like QCA6390 which are obviously really
> popular.

Sorry, came back only on Monday and trying to catch up slowly. But I
submitted the RFC now:

https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/

Please take a look and let me know what you think.

Robert Marko Jan. 11, 2023, 5:10 p.m. UTC | #12

On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
>
> Robert Marko <[e-mail]> writes:
>
> >> Really sorry, I just didn't manage to get this finalised due to other
> >> stuff and now I'm leaving for a two week vacation :(
> >
> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
> > and the current workaround for supporting AHB + PCI or multiple PCI
> > cards is breaking cards like QCA6390 which are obviously really
> > popular.
>
> Sorry, came back only on Monday and trying to catch up slowly. But I
> submitted the RFC now:
>
> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/

Great, thanks for that.

Does it depend on firmware-2 being available?

Regards,
Robert
>
> Please take a look and let me know what you think.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Kalle Valo Jan. 12, 2023, 9:40 a.m. UTC | #13

Robert Marko <[e-mail]> writes:

> On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
>>
>> Robert Marko <[e-mail]> writes:
>>
>> >> Really sorry, I just didn't manage to get this finalised due to other
>> >> stuff and now I'm leaving for a two week vacation :(
>> >
>> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
>> > and the current workaround for supporting AHB + PCI or multiple PCI
>> > cards is breaking cards like QCA6390 which are obviously really
>> > popular.
>>
>> Sorry, came back only on Monday and trying to catch up slowly. But I
>> submitted the RFC now:
>>
>> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
>
> Great, thanks for that.
>
> Does it depend on firmware-2 being available?

The final solution for the users will require firmware-2.bin. But for a
quick test you can omit the feature bit test by replacing
"test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" with
"true". Just make sure that the firmware release you are using supports
this feature, I believe only recent QCN9074 releases do that.

Robert Marko Jan. 12, 2023, 9:43 a.m. UTC | #14

On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <[e-mail]> wrote:
>
> Robert Marko <[e-mail]> writes:
>
> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
> >>
> >> Robert Marko <[e-mail]> writes:
> >>
> >> >> Really sorry, I just didn't manage to get this finalised due to other
> >> >> stuff and now I'm leaving for a two week vacation :(
> >> >
> >> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
> >> > and the current workaround for supporting AHB + PCI or multiple PCI
> >> > cards is breaking cards like QCA6390 which are obviously really
> >> > popular.
> >>
> >> Sorry, came back only on Monday and trying to catch up slowly. But I
> >> submitted the RFC now:
> >>
> >> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
> >
> > Great, thanks for that.
> >
> > Does it depend on firmware-2 being available?
>
> The final solution for the users will require firmware-2.bin. But for a
> quick test you can omit the feature bit test by replacing
> "test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" with
> "true". Just make sure that the firmware release you are using supports
> this feature, I believe only recent QCN9074 releases do that.

Hi,
I was able to test on IPQ8074+QCN9074 yesterday by just bypassing the
test and it worked.
Sideffect is that until firmware-2.bin is available cards like QCA6390
wont work like with my
hack.

Regards,
Robert
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Kalle Valo Jan. 12, 2023, 9:48 a.m. UTC | #15

Robert Marko <[e-mail]> writes:

> On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <[e-mail]> wrote:
>>
>> Robert Marko <[e-mail]> writes:
>>
>> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
>> >>
>> >> Robert Marko <[e-mail]> writes:
>> >>
>> >> >> Really sorry, I just didn't manage to get this finalised due to other
>> >> >> stuff and now I'm leaving for a two week vacation :(
>> >> >
>> >> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
>> >> > and the current workaround for supporting AHB + PCI or multiple PCI
>> >> > cards is breaking cards like QCA6390 which are obviously really
>> >> > popular.
>> >>
>> >> Sorry, came back only on Monday and trying to catch up slowly. But I
>> >> submitted the RFC now:
>> >>
>> >> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
>> >
>> > Great, thanks for that.
>> >
>> > Does it depend on firmware-2 being available?
>>
>> The final solution for the users will require firmware-2.bin. But for a
>> quick test you can omit the feature bit test by replacing
>> "test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" with
>> "true". Just make sure that the firmware release you are using supports
>> this feature, I believe only recent QCN9074 releases do that.
>
> I was able to test on IPQ8074+QCN9074 yesterday by just bypassing the
> test and it worked.
>
> Sideffect is that until firmware-2.bin is available cards like QCA6390
> wont work like with my hack.

Not following here, can you elaborate what won't work with QCA6390?

Robert Marko Jan. 23, 2023, 7:21 p.m. UTC | #16

On Thu, Jan 12, 2023 at 10:49 AM Kalle Valo <[e-mail]> wrote:
>
> Robert Marko <[e-mail]> writes:
>
> > On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <[e-mail]> wrote:
> >>
> >> Robert Marko <[e-mail]> writes:
> >>
> >> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
> >> >>
> >> >> Robert Marko <[e-mail]> writes:
> >> >>
> >> >> >> Really sorry, I just didn't manage to get this finalised due to other
> >> >> >> stuff and now I'm leaving for a two week vacation :(
> >> >> >
> >> >> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
> >> >> > and the current workaround for supporting AHB + PCI or multiple PCI
> >> >> > cards is breaking cards like QCA6390 which are obviously really
> >> >> > popular.
> >> >>
> >> >> Sorry, came back only on Monday and trying to catch up slowly. But I
> >> >> submitted the RFC now:
> >> >>
> >> >> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
> >> >
> >> > Great, thanks for that.
> >> >
> >> > Does it depend on firmware-2 being available?
> >>
> >> The final solution for the users will require firmware-2.bin. But for a
> >> quick test you can omit the feature bit test by replacing
> >> "test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" with
> >> "true". Just make sure that the firmware release you are using supports
> >> this feature, I believe only recent QCN9074 releases do that.
> >
> > I was able to test on IPQ8074+QCN9074 yesterday by just bypassing the
> > test and it worked.
> >
> > Sideffect is that until firmware-2.bin is available cards like QCA6390
> > wont work like with my hack.
>
> Not following here, can you elaborate what won't work with QCA6390?


Our downstream hack does not work with QCA6390, so that is why its quite
important for OpenWrt to have a generic solution that works on all cards.

Regards,
Robert
>
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Kalle Valo March 8, 2023, 12:43 p.m. UTC | #17

Robert Marko <[e-mail]> writes:

> On Thu, Jan 12, 2023 at 10:49 AM Kalle Valo <[e-mail]> wrote:
>
>>
>> Robert Marko <[e-mail]> writes:
>>
>> > On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <[e-mail]> wrote:
>> >>
>> >> Robert Marko <[e-mail]> writes:
>> >>
>> >> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <[e-mail]> wrote:
>> >> >>
>> >> >> Robert Marko <[e-mail]> writes:
>> >> >>
>> >> >> >> Really sorry, I just didn't manage to get this finalised due to other
>> >> >> >> stuff and now I'm leaving for a two week vacation :(
>> >> >> >
>> >> >> > Any news regarding this, I have a PR for ipq807x support in OpenWrt
>> >> >> > and the current workaround for supporting AHB + PCI or multiple PCI
>> >> >> > cards is breaking cards like QCA6390 which are obviously really
>> >> >> > popular.
>> >> >>
>> >> >> Sorry, came back only on Monday and trying to catch up slowly. But I
>> >> >> submitted the RFC now:
>> >> >>
>> >> >> https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
>> >> >
>> >> > Great, thanks for that.
>> >> >
>> >> > Does it depend on firmware-2 being available?
>> >>
>> >> The final solution for the users will require firmware-2.bin. But for a
>> >> quick test you can omit the feature bit test by replacing
>> >> "test_bit(ATH11K_FW_FEATURE_MULTI_QRTR_ID, ab->fw.fw_features)" with
>> >> "true". Just make sure that the firmware release you are using supports
>> >> this feature, I believe only recent QCN9074 releases do that.
>> >
>> > I was able to test on IPQ8074+QCN9074 yesterday by just bypassing the
>> > test and it worked.
>> >
>> > Sideffect is that until firmware-2.bin is available cards like QCA6390
>> > wont work like with my hack.
>>
>> Not following here, can you elaborate what won't work with QCA6390?
>
> Our downstream hack does not work with QCA6390,

Still not sure what you mean. Are you saying that this patch under
discussion ("wifi: ath11k: use unique QRTR instance ID") also works with
QCA6390 and it's possible to connect two QCA6390 devices on the same
host?

Or are you referring to some other hack? Or have I totally
misunderstood? :)

> so that is why its quite important for OpenWrt to have a generic
> solution that works on all cards.

I fully agree on importance of having a generic solution. It's just sad
that it seems people who designed this didn't consider about having
multiple devices on the same host. It looks like there's no easy way to
implement a generic solution, we have only bad choices to choose from.
Your solution[1] is racy and writing to a register which is marked as
read-only in the spec.

Qualcomm's solution[2] needs changes in firmware and it's uncertain if
I'm able to convince all firmware teams to implement the support.
(Currently only QCN9074 firmware supports this.)

Thoughts?

[1] https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/

[2] https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/

Robert Marko April 26, 2023, 12:40 p.m. UTC | #18

> Still not sure what you mean. Are you saying that this patch under
> discussion ("wifi: ath11k: use unique QRTR instance ID") also works with
> QCA6390 and it's possible to connect two QCA6390 devices on the same
> host?
>
> Or are you referring to some other hack? Or have I totally
> misunderstood? :)

We probably have a misunderstanding, QCA6390 does not work with
("wifi: ath11k: use unique QRTR instance ID"), that is why we in OpenWrt
limited it to QCN9074 only so far.

>
> > so that is why its quite important for OpenWrt to have a generic
> > solution that works on all cards.
>
> I fully agree on importance of having a generic solution. It's just sad
> that it seems people who designed this didn't consider about having
> multiple devices on the same host. It looks like there's no easy way to
> implement a generic solution, we have only bad choices to choose from.
> Your solution[1] is racy and writing to a register which is marked as
> read-only in the spec.

I agree, this is purely a hack based on what QCA is doing downstream where
they hardcode the QRTR ID in DTS and write to the same register.

>
> Qualcomm's solution[2] needs changes in firmware and it's uncertain if
> I'm able to convince all firmware teams to implement the support.
> (Currently only QCN9074 firmware supports this.)
>
> Thoughts?

I mean, we need some kind of a solution cause trying to pitch using a QCA
AX SoC-s and PCI cards but then saying that they cannot use AHB+PCI
or multiple PCI cards at the same time are not viable.

Regards,
Robert
>
> [1] https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
>
> [2] https://patchwork.kernel.org/project/linux-wireless/patch/[e-mail]/
>
> --
> https://patchwork.kernel.org/project/linux-wireless/list/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

Patch

diff --git a/drivers/net/wireless/ath/ath11k/mhi.c b/drivers/net/wireless/ath/ath11k/mhi.c
index 86995e8dc913..23e85ea902f5 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.c
+++ b/drivers/net/wireless/ath/ath11k/mhi.c
@@ -294,6 +294,32 @@  static void ath11k_mhi_op_runtime_put(struct mhi_controller *mhi_cntrl)
 {
 }
 
+static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
+				  void __iomem *addr,
+				  u32 *out)
+{
+	*out = readl(addr);
+
+	return 0;
+}
+
+static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
+				    void __iomem *addr,
+				    u32 val)
+{
+	writel(val, addr);
+}
+
+static void ath11k_mhi_qrtr_instance_set(struct mhi_controller *mhi_cntrl)
+{
+	struct ath11k_base *ab = dev_get_drvdata(mhi_cntrl->cntrl_dev);
+
+	ath11k_mhi_op_write_reg(mhi_cntrl,
+				mhi_cntrl->bhi + BHI_ERRDBG2,
+				FIELD_PREP(QRTR_INSTANCE_MASK,
+				ab->qmi.service_ins_id - ab->hw_params.qmi_service_ins_id));
+}
+
 static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
 {
 	switch (reason) {
@@ -315,6 +341,8 @@  static char *ath11k_mhi_op_callback_to_str(enum mhi_callback reason)
 		return "MHI_CB_FATAL_ERROR";
 	case MHI_CB_BW_REQ:
 		return "MHI_CB_BW_REQ";
+	case MHI_CB_EE_SBL_MODE:
+		return "MHI_CB_EE_SBL_MODE";
 	default:
 		return "UNKNOWN";
 	}
@@ -336,27 +364,14 @@  static void ath11k_mhi_op_status_cb(struct mhi_controller *mhi_cntrl,
 		if (!(test_bit(ATH11K_FLAG_UNREGISTERING, &ab->dev_flags)))
 			queue_work(ab->workqueue_aux, &ab->reset_work);
 		break;
+	case MHI_CB_EE_SBL_MODE:
+		ath11k_mhi_qrtr_instance_set(mhi_cntrl);
+		break;
 	default:
 		break;
 	}
 }
 
-static int ath11k_mhi_op_read_reg(struct mhi_controller *mhi_cntrl,
-				  void __iomem *addr,
-				  u32 *out)
-{
-	*out = readl(addr);
-
-	return 0;
-}
-
-static void ath11k_mhi_op_write_reg(struct mhi_controller *mhi_cntrl,
-				    void __iomem *addr,
-				    u32 val)
-{
-	writel(val, addr);
-}
-
 static int ath11k_mhi_read_addr_from_dt(struct mhi_controller *mhi_ctrl)
 {
 	struct device_node *np;
diff --git a/drivers/net/wireless/ath/ath11k/mhi.h b/drivers/net/wireless/ath/ath11k/mhi.h
index 8d9f852da695..0db308bc3047 100644
--- a/drivers/net/wireless/ath/ath11k/mhi.h
+++ b/drivers/net/wireless/ath/ath11k/mhi.h
@@ -16,6 +16,9 @@ 
 #define MHICTRL					0x38
 #define MHICTRL_RESET_MASK			0x2
 
+#define BHI_ERRDBG2				0x38
+#define QRTR_INSTANCE_MASK			GENMASK(7, 0)
+
 int ath11k_mhi_start(struct ath11k_pci *ar_pci);
 void ath11k_mhi_stop(struct ath11k_pci *ar_pci);
 int ath11k_mhi_register(struct ath11k_pci *ar_pci);
diff --git a/drivers/net/wireless/ath/ath11k/pci.c b/drivers/net/wireless/ath/ath11k/pci.c
index 99cf3357c66e..cd26c1567415 100644
--- a/drivers/net/wireless/ath/ath11k/pci.c
+++ b/drivers/net/wireless/ath/ath11k/pci.c
@@ -370,13 +370,16 @@  static void ath11k_pci_sw_reset(struct ath11k_base *ab, bool power_on)
 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;
 
 	cfg->svc_to_ce_map = ab->hw_params.svc_to_ce_map;
 	cfg->svc_to_ce_map_len = ab->hw_params.svc_to_ce_map_len;
-	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id;
+	ab->qmi.service_ins_id = ab->hw_params.qmi_service_ins_id +
+	(((pci_domain_nr(bus) & 0xF) << 4) | (bus->number & 0xF));
 
 	ath11k_ce_get_shadow_config(ab, &cfg->shadow_reg_v2,
 				    &cfg->shadow_reg_v2_len);