| Message ID | 20221105194943.826847-1-robimarko@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] bus: mhi: core: add SBL state callback | expand |
On Mon, 7 Nov 2022 at 12:28, Manivannan Sadhasivam <mani@kernel.org> wrote: > > On Sat, Nov 05, 2022 at 08:49:42PM +0100, Robert Marko wrote: > > Add support for SBL state callback in MHI core. > > > > It is required for ath11k MHI devices in order to be able to set QRTR > > instance ID in the SBL state so that QRTR instance ID-s dont conflict in > > case of multiple PCI/MHI cards or AHB + PCI/MHI card. > > Setting QRTR instance ID is only possible in SBL state and there is > > currently no way to ensure that we are in that state, so provide a > > callback that the controller can trigger off. > > > > Where can I find the corresponding ath11k patch that makes use of this > callback? Hi Mani, ath11k patch was sent as part of the same series to everybody included in this patch as well under the name of "[PATCH 2/2] wifi: ath11k: use unique QRTR instance ID". If you did not receive it due to some kind of error, its available at the linux-wireless patchwork[1] or ath11k mailing list[2]. [1] https://patchwork.kernel.org/project/linux-wireless/patch/20221105194943.826847-2-robimarko@gmail.com/ [2] http://lists.infradead.org/pipermail/ath11k/2022-November/003678.html Regards, Robert > > Thanks, > Mani > > > Signed-off-by: Robert Marko <robimarko@gmail.com> > > --- > > drivers/bus/mhi/host/main.c | 1 + > > include/linux/mhi.h | 2 ++ > > 2 files changed, 3 insertions(+) > > > > diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c > > index df0fbfee7b78..8b03dd1f0cb8 100644 > > --- a/drivers/bus/mhi/host/main.c > > +++ b/drivers/bus/mhi/host/main.c > > @@ -900,6 +900,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, > > switch (event) { > > case MHI_EE_SBL: > > st = DEV_ST_TRANSITION_SBL; > > + mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_SBL_MODE); > > break; > > case MHI_EE_WFW: > > case MHI_EE_AMSS: > > diff --git a/include/linux/mhi.h b/include/linux/mhi.h > > index a5441ad33c74..beffe102dd19 100644 > > --- a/include/linux/mhi.h > > +++ b/include/linux/mhi.h > > @@ -34,6 +34,7 @@ struct mhi_buf_info; > > * @MHI_CB_SYS_ERROR: MHI device entered error state (may recover) > > * @MHI_CB_FATAL_ERROR: MHI device entered fatal error state > > * @MHI_CB_BW_REQ: Received a bandwidth switch request from device > > + * @MHI_CB_EE_SBL_MODE: MHI device entered SBL mode > > */ > > enum mhi_callback { > > MHI_CB_IDLE, > > @@ -45,6 +46,7 @@ enum mhi_callback { > > MHI_CB_SYS_ERROR, > > MHI_CB_FATAL_ERROR, > > MHI_CB_BW_REQ, > > + MHI_CB_EE_SBL_MODE, > > }; > > > > /** > > -- > > 2.38.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 <robimarko@gmail.com> > --- > 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);
On Mon, 7 Nov 2022 at 16:10, Jeffrey Hugo <quic_jhugo@quicinc.com> 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 <robimarko@gmail.com> > > --- > > 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 <mani@kernel.org> 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 <kvalo@kernel.org> writes: > Manivannan Sadhasivam <mani@kernel.org> 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
On Tue, Nov 22, 2022 at 12:26 PM Kalle Valo <kvalo@kernel.org> wrote: > > Kalle Valo <kvalo@kernel.org> writes: > > > Manivannan Sadhasivam <mani@kernel.org> 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 > ath11k@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/ath11k
Robert Marko <robert.marko@sartura.hr> writes: > On Tue, Nov 22, 2022 at 12:26 PM Kalle Valo <kvalo@kernel.org> wrote: > >> >> Kalle Valo <kvalo@kernel.org> writes: >> >> > Manivannan Sadhasivam <mani@kernel.org> 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 :(
> 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
On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <kvalo@kernel.org> wrote: > > Robert Marko <robert.marko@sartura.hr> 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/20230111170033.32454-1-kvalo@kernel.org/ 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
On Thu, Jan 12, 2023 at 10:49 AM Kalle Valo <kvalo@kernel.org> wrote: > > Robert Marko <robert.marko@sartura.hr> writes: > > > On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <kvalo@kernel.org> wrote: > >> > >> Robert Marko <robert.marko@sartura.hr> writes: > >> > >> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <kvalo@kernel.org> wrote: > >> >> > >> >> Robert Marko <robert.marko@sartura.hr> 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/20230111170033.32454-1-kvalo@kernel.org/ > >> > > >> > 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
Robert Marko <robert.marko@sartura.hr> writes: > On Thu, Jan 12, 2023 at 10:49 AM Kalle Valo <kvalo@kernel.org> wrote: > >> >> Robert Marko <robert.marko@sartura.hr> writes: >> >> > On Thu, Jan 12, 2023 at 10:40 AM Kalle Valo <kvalo@kernel.org> wrote: >> >> >> >> Robert Marko <robert.marko@sartura.hr> writes: >> >> >> >> > On Wed, Jan 11, 2023 at 6:10 PM Kalle Valo <kvalo@kernel.org> wrote: >> >> >> >> >> >> Robert Marko <robert.marko@sartura.hr> 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/20230111170033.32454-1-kvalo@kernel.org/ >> >> > >> >> > 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/20221105194943.826847-2-robimarko@gmail.com/ [2] https://patchwork.kernel.org/project/linux-wireless/patch/20230111170033.32454-1-kvalo@kernel.org/
> 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/20221105194943.826847-2-robimarko@gmail.com/ > > [2] https://patchwork.kernel.org/project/linux-wireless/patch/20230111170033.32454-1-kvalo@kernel.org/ > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
diff --git a/drivers/bus/mhi/host/main.c b/drivers/bus/mhi/host/main.c index df0fbfee7b78..8b03dd1f0cb8 100644 --- a/drivers/bus/mhi/host/main.c +++ b/drivers/bus/mhi/host/main.c @@ -900,6 +900,7 @@ int mhi_process_ctrl_ev_ring(struct mhi_controller *mhi_cntrl, switch (event) { case MHI_EE_SBL: st = DEV_ST_TRANSITION_SBL; + mhi_cntrl->status_cb(mhi_cntrl, MHI_CB_EE_SBL_MODE); break; case MHI_EE_WFW: case MHI_EE_AMSS: diff --git a/include/linux/mhi.h b/include/linux/mhi.h index a5441ad33c74..beffe102dd19 100644 --- a/include/linux/mhi.h +++ b/include/linux/mhi.h @@ -34,6 +34,7 @@ struct mhi_buf_info; * @MHI_CB_SYS_ERROR: MHI device entered error state (may recover) * @MHI_CB_FATAL_ERROR: MHI device entered fatal error state * @MHI_CB_BW_REQ: Received a bandwidth switch request from device + * @MHI_CB_EE_SBL_MODE: MHI device entered SBL mode */ enum mhi_callback { MHI_CB_IDLE, @@ -45,6 +46,7 @@ enum mhi_callback { MHI_CB_SYS_ERROR, MHI_CB_FATAL_ERROR, MHI_CB_BW_REQ, + MHI_CB_EE_SBL_MODE, }; /**
Add support for SBL state callback in MHI core. It is required for ath11k MHI devices in order to be able to set QRTR instance ID in the SBL state so that QRTR instance ID-s dont conflict in case of multiple PCI/MHI cards or AHB + PCI/MHI card. Setting QRTR instance ID is only possible in SBL state and there is currently no way to ensure that we are in that state, so provide a callback that the controller can trigger off. Signed-off-by: Robert Marko <robimarko@gmail.com> --- drivers/bus/mhi/host/main.c | 1 + include/linux/mhi.h | 2 ++ 2 files changed, 3 insertions(+)