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

[Jeffrey Hugo <quic_jhugo@quicinc.com>]

On 11/7/2022 10:52 AM, Robert Marko wrote:
> On Mon, 7 Nov 2022 at 18:47, Manivannan Sadhasivam <mani@kernel.org> 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 <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));
>>> +}
>>> +
>>>   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.