>> A good point, I didn't thought of that during review. No time to investigate this right now, but maybe Rakesh and Govind (CCed) can comment? Yes, ar->max_num_peers needs to be assigned with ar->hw_param->num_peers. This can create mismatch for wcn3990 target if we create multiple peers( TARGET_HL_10_TLV_NUM_PEERS vs TARGET_TLV_NUM_PEERS). We will fix this and raise this as separate change. >>> Btw, what the heck is SNOC anyway? SNOC is system NOC(network on chip). WCN3990 is integrated chipset connected over SNOC and only RF part is discrete to the SoC. BR, Govind -----Original Message----- From: Kalle Valo [mailto:kvalo@xxxxxxxxxxxxxx] Sent: Monday, January 8, 2018 7:12 PM To: Erik Stromdahl <erik.stromdahl@xxxxxxxxx> Cc: linux-wireless@xxxxxxxxxxxxxxx; ath10k@xxxxxxxxxxxxxxxxxxx; Rakesh Pillai <pillair@xxxxxxxxxxxxxxxx>; Govind Singh <govinds@xxxxxxxxxxxxxxxx> Subject: Re: [RFC v3 03/11] ath10k: per target configurablity of various items Erik Stromdahl <erik.stromdahl@xxxxxxxxx> writes: > On 2017-12-22 16:19, Kalle Valo wrote: > >> I was a bit torn about this, I definitely see the need for this but >> on the other hand it creates duplicate data (for example two entries >> for >> QCA9377 chip). I guess this is the right approach, at least I cannot >> come up anything better. >> >> But this patch should be split into two: >> >> 1) add bus field to struct ath10k_hw_params >> >> 2) add max_num_peers field to struct ath10k_hw_params >> >> And it seems 2) is already implemented in commit 9f2992fea580 ("ath10k: >> wmi: get wmi init parameter values from hw params"), so hopefully we >> only need 1) anymore. >> > > Before commit 9f2992fea580a48135591873e5e3ac7e01444207, > TARGET_TLV_NUM_PEERS was used both in the WMI TLV init command and as > the value of *max_num_peers* in *struct ath10k* (ar->max_num_peers). > > commit 9f2992fea580a48135591873e5e3ac7e01444207 does not set > *ar->max_num_peers* to the value of *ar->hw_param->num_peers*. > > Is this correct? > > As I see it, there is a possible mismatch between what is written to > the device in the WMI init message and the value of *ar->max_num_peers*. > > Do we still need *max_num_peers* in *struct ath10k* now that we have > the > *num_peers* member in *struct ath10k_hw_params*? A good point, I didn't thought of that during review. No time to investigate this right now, but maybe Rakesh and Govind (CCed) can comment? > I am currently rewriting my HL patches and I was thinking about adding > a separate patch related to this. Yeah, a separate patch to sort that out is a good idea. >>> --- a/drivers/net/wireless/ath/ath10k/core.c >>> +++ b/drivers/net/wireless/ath/ath10k/core.c >>> @@ -1663,9 +1663,19 @@ static int ath10k_init_hw_params(struct ath10k *ar) >>> for (i = 0; i < ARRAY_SIZE(ath10k_hw_params_list); i++) { >>> hw_params = &ath10k_hw_params_list[i]; >>> - if (hw_params->id == ar->target_version && >>> - hw_params->dev_id == ar->dev_id) >>> - break; >>> + if (ar->is_high_latency) { >>> + /* High latency devices will use different fw depending >>> + * on if it is a USB or SDIO device. >>> + */ >>> + if (hw_params->bus == ar->hif.bus && >>> + hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } else { >>> + if (hw_params->id == ar->target_version && >>> + hw_params->dev_id == ar->dev_id) >>> + break; >>> + } >> >> I don't like the is_high_latency test here at all. The bus field >> should be checked with all entries, not just high latency ones. And >> because of this even most of the hw_param bus field entries were not initialised. >> >> So only thing to do is to initialise the bus field for all the >> entries and the ugly test here can be removed. Just remember that >> QCA4019 uses AHB, I think all the rest is PCI. Or do we have AHB devices supported? > > I noticed that there has been introduced a new bus type (SNOC). > Do you know which devices are SNOC devices? SNOC is for wcn3990. > Btw, what the heck is SNOC anyway? I have forgetten already what the acronym meant but it's basically some sort of shared memory communication method with the firmware. -- Kalle Valo
