| Message ID | 1642337235-8618-1-git-send-email-quic_mpubbise@quicinc.com |
|---|---|
| Headers | show |
| Series | add support for WCN6750 | expand |
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > WCN6750 is non-DBS 2x2 11AX chipset. Unlike QCA6390/WCN6855 which > are DBS (dual band simultaneous) solutions (2 LMACs), WCN6750 has a > single LMAC supporting 2G, 5G and 6G bands. It can be operated only > on one band at any given point. > > WCN6750 is a PCIe device. Unlike other supported ATH11K PCIe devices > which are directly attached to APSS (Application Processor SubSystem), > WCN6750 is not attached to APSS, it is attached to the WPSS > (Wireless Processor SubSystem) Q6 processor, the FW which runs on the > Q6 processor will enumerate the PCIe device. Since APSS is unaware of > such a device, it has to be registered as a platform device(AHB) to the > kernel for device probing. Like other AHB devices, remoteproc APIs are > used to boot up or shutdown of WCN6750. > > WCN6750 uses both AHB and PCIe ATH11K APIs for it's operation. > It uses AHB APIs for device probe and booting of the remote processor. > Once device is booted up, it uses ATH11K PCIe APIs for initialization > and register access. Hence, it is referred as hybrid bus device in > the rest of this series. > > Since the chip is enumerated by WPSS Q6, device information like > BAR and BAR size is not known to the APSS processor. A new QMI message > called device info QMI request will be sent to the target for fetching > these details. > > STA and AP modes are supported; Basic connectivity and ping are > verified in both the modes. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 > > Note: > * Remoteproc driver changes for WCN6750 which takes care of > downloading the FW and booting of Q6 processor are under > upstream review. > Link: https://patchwork.kernel.org/project/linux-remoteproc/list/?series=582475 This is a very good overview, thanks for that. But I think something which is not clearly mentioned here is that this only works on Qualcomm Snapdragon SoC, right? So even though WCN6750 is a PCI device, it cannot be attached to any platform. It would be good to emphasise that. > Manikanta Pubbisetty (19): > ath11k: PCI changes to support WCN6750 > ath11k: Refactor PCI code to support hybrid bus devices > ath11k: Choose MSI config based on HW revision > ath11k: Refactor MSI logic > ath11k: Remove core PCI references from PCI common code > ath11k: Add HW params for WCN6750 > ath11k: Add bus params for WCN6750 > ath11k: Add register access logic for WCN6750 > ath11k: Fetch device information via QMI for WCN6750 > ath11k: Add QMI changes for WCN6750 > ath11k: HAL changes to support WCN6750 > ath11k: Datapath changes to support WCN6750 > ath11k: Fix RX de-fragmentation issue on WCN6750 > ath11k: Do not put HW in DBS mode for WCN6750 > ath11k: WMI changes to support WCN6750 > ath11k: Update WBM idle ring HP after FW mode on > ath11k: Add support for WCN6750 device > ath11k: Add support for targets without trustzone > dt: bindings: net: add bindings of WCN6750 for ath11k 19 patches is a lot to chew on in one go, my recommendation is to have max 10-12 patches per set. In this case having three patchsets would make it a lot easier for reviewers, but not sure how to split them. Maybe you could submit these patches separate for preparing WCN6750 support, after a quick look they seem pretty independent: ath11k: Fetch device information via QMI for WCN6750 ath11k: HAL changes to support WCN6750 ath11k: Fix RX de-fragmentation issue on WCN6750 ath11k: Do not put HW in DBS mode for WCN6750 ath11k: WMI changes to support WCN6750
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > Unlike other ATH11K PCIe devices which are enumerated by APSS > processor (Application Processor SubSystem), WCN6750 gets > enumerated by the WPSS Q6 processor (Wireless Processor SubSystem); > In simple terms, though WCN6750 is PCIe device, it is not attached > to the APSS processor, APSS will not know of such a device being > present in the system and therefore WCN6750 will be registered as > a platform device to the kernel core like other supported AHB > devices. > > WCN6750 uses both AHB and PCI APIs for it's operation, it uses > AHB APIs for device probe/boot and PCI APIs for device setup > and register accesses; Because of this nature, it is referred > as a hybrid bus device. > > Refactor PCI code to support hybrid bus devices like WCN6750. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> [...] > --- a/drivers/net/wireless/ath/ath11k/Makefile > +++ b/drivers/net/wireless/ath/ath11k/Makefile > @@ -29,7 +29,7 @@ obj-$(CONFIG_ATH11K_AHB) += ath11k_ahb.o > ath11k_ahb-y += ahb.o > > obj-$(CONFIG_ATH11K_PCI) += ath11k_pci.o > -ath11k_pci-y += mhi.o pci.o > +ath11k_pci-y += mhi.o pci.o pci_cmn.o So the end result looks like this: obj-$(CONFIG_ATH11K_AHB) += ath11k_ahb.o ath11k_ahb-y += ahb.o pci_cmn.o obj-$(CONFIG_ATH11K_PCI) += ath11k_pci.o ath11k_pci-y += mhi.o pci.o pci_cmn.o Linking pci_cmn.o to both ath11k_pci.ko and ath11k_ahb.ko looks wrong. Does that even compile if ath11k is linked to the kernel, eg. with allyesconfig? One way to solve is to link pci_cmn.o to ath11k.ko. But for another approach, for a long time I have been thinking about what's the point to have separate ath11k_pci.ko and ath11k_ahb.ko modules?,They are very small anyway compared to ath11k.ko. So my ideais that should we have just one ath11k.ko module, it contains all AHB and PCI code as well, and ath11k_pci.ko and ath11k_ahb.ko would not be created anymore. It would simplify things a bit, especially here. Thoughts?
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > Add the support to attach WCN6750 and map iommu domain > for targets which do not have the support of TrustZone. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> Is this not needed in the initial WCN6750 support? If yes, please submit this patch after WCN6750 support has been commited. That way we can reduce patchset size.
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > Unlike other ATH11K PCIe devices which are enumerated by APSS > processor (Application Processor SubSystem), WCN6750 gets > enumerated by the WPSS Q6 processor (Wireless Processor SubSystem); > In simple terms, though WCN6750 is PCIe device, it is not attached > to the APSS processor, APSS will not know of such a device being > present in the system and therefore WCN6750 will be registered as > a platform device to the kernel core like other supported AHB > devices. > > WCN6750 uses both AHB and PCI APIs for it's operation, it uses > AHB APIs for device probe/boot and PCI APIs for device setup > and register accesses; Because of this nature, it is referred > as a hybrid bus device. > > Refactor PCI code to support hybrid bus devices like WCN6750. > > Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 > Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 > Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 > Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 > > Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> [...] > --- /dev/null > +++ b/drivers/net/wireless/ath/ath11k/pci_cmn.c [...] > +static inline void ath11k_pci_select_window(struct ath11k_pci *ab_pci, u32 offset) > +{ > + struct ath11k_base *ab = ab_pci->ab; > + > + u32 window = FIELD_GET(ATH11K_PCI_WINDOW_VALUE_MASK, offset); > + > + lockdep_assert_held(&ab_pci->window_lock); > + > + if (window != ab_pci->register_window) { > + iowrite32(ATH11K_PCI_WINDOW_ENABLE_BIT | window, > + ab->mem + ATH11K_PCI_WINDOW_REG_ADDRESS); > + ioread32(ab->mem + ATH11K_PCI_WINDOW_REG_ADDRESS); > + ab_pci->register_window = window; > + } > +} So the style used in ath11k is ath11k_<filename>_foo, so that a function ath11k_pci_foo() should be in pci.c. This patch is now breaking that style. Maybe pci_cmn.c should renamed to cpci.c, pcic.c or something like that? Then the function prefix could be ath11k_cpci_, ath11k_pcic_ or similar.
On 2/25/2022 11:20 AM, Manikanta Pubbisetty wrote: > On 1/28/2022 3:46 PM, Kalle Valo wrote: >> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >> >>> Unlike other ATH11K PCIe devices which are enumerated by APSS >>> processor (Application Processor SubSystem), WCN6750 gets >>> enumerated by the WPSS Q6 processor (Wireless Processor SubSystem); >>> In simple terms, though WCN6750 is PCIe device, it is not attached >>> to the APSS processor, APSS will not know of such a device being >>> present in the system and therefore WCN6750 will be registered as >>> a platform device to the kernel core like other supported AHB >>> devices. >>> >>> WCN6750 uses both AHB and PCI APIs for it's operation, it uses >>> AHB APIs for device probe/boot and PCI APIs for device setup >>> and register accesses; Because of this nature, it is referred >>> as a hybrid bus device. >>> >>> Refactor PCI code to support hybrid bus devices like WCN6750. >>> >>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 >>> Tested-on: WCN6855 hw2.0 PCI >>> WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 >>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 >>> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 >>> >>> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> >> >> [...] >> >>> --- a/drivers/net/wireless/ath/ath11k/Makefile >>> +++ b/drivers/net/wireless/ath/ath11k/Makefile >>> @@ -29,7 +29,7 @@ obj-$(CONFIG_ATH11K_AHB) += ath11k_ahb.o >>> ath11k_ahb-y += ahb.o >>> obj-$(CONFIG_ATH11K_PCI) += ath11k_pci.o >>> -ath11k_pci-y += mhi.o pci.o >>> +ath11k_pci-y += mhi.o pci.o pci_cmn.o >> >> So the end result looks like this: >> >> obj-$(CONFIG_ATH11K_AHB) += ath11k_ahb.o >> ath11k_ahb-y += ahb.o pci_cmn.o >> >> obj-$(CONFIG_ATH11K_PCI) += ath11k_pci.o >> ath11k_pci-y += mhi.o pci.o pci_cmn.o >> >> Linking pci_cmn.o to both ath11k_pci.ko and ath11k_ahb.ko looks wrong. >> Does that even compile if ath11k is linked to the kernel, eg. with >> allyesconfig? >> > > I did try compiling the kernel with allyesconfig after your comment, > compilation went through without any hiccups. > >> One way to solve is to link pci_cmn.o to ath11k.ko. But for another >> approach, for a long time I have been thinking about what's the point to >> have separate ath11k_pci.ko and ath11k_ahb.ko modules?,They are very >> small anyway compared to ath11k.ko. So my ideais that should we have >> just one ath11k.ko module, it contains all AHB and PCI code as well, and >> ath11k_pci.ko and ath11k_ahb.ko would not be created anymore. It would >> simplify things a bit, especially here. >> >> Thoughts? >> > > I see some concerns going with single module combining both AHB and PCI > modules into ath11k.ko > > 1) AHB and PCI drivers make use of completely different kernel > frameworks, for example AHB driver needs remoteproc APIs for booting and > require CONFIG_REMOTEPROC to be compiled in to the kernel. Similarly, > PCI driver needs MHI APIs and also dependent on CONFIG_PCI. Both MHI and > PCI bus frameworks need to be compiled for PCI to work. If we club all > of this into single module, I see that unnecessarily additional modules > will be compiled into the kernel which IMO is not so good idea. > > > 2) Secondly, there is high chance of writing bad code all over the > driver. For example, there are chances that developers put AHB/PCI > specific code all over the driver creating a big mess. > Though this can be avoided with stringent code review, but why to > give the chance. > > Though AHB and PCI drivers are smaller in size, IMHO let AHB and PCI be > independent drivers, code looks cleaner and properly segregated by > keeping them as it is today. > > Regarding the compilation of PCI common code, shall we move it into > ath11k.ko? What is your opinion on this. > Hi Kalle, Any thoughts about the idea proposed? Thanks, Manikanta
Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: > On 4/27/2022 11:18 AM, Kalle Valo wrote: >> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >> >>> On 4/27/2022 11:02 AM, Kalle Valo wrote: >>>> Manikanta Pubbisetty <quic_mpubbise@quicinc.com> writes: >>>> >>>>> HAL RX descriptor for WCN6750 is same as QCN9074, so use >>>>> the DP APIs of QCN9074 for WCN6750. There is one change >>>>> wrt to REO configuration though, REO configuration for >>>>> WCN6750 is same as WCN6855; Therefore, use reo_setup() >>>>> of WCN6855 for WCN6750. >>>>> >>>>> Tested-on: WCN6750 hw1.0 AHB WLAN.MSL.1.0.1-00573-QCAMSLSWPLZ-1 >>>>> Tested-on: WCN6855 hw2.0 PCI WLAN.HSP.1.1-01720.1-QCAHSPSWPL_V1_V2_SILICONZ_LITE-1 >>>>> Tested-on: QCN9074 hw1.0 PCI WLAN.HK.2.5.0.1-01100-QCAHKSWPL_SILICONZ-1 >>>>> Tested-on: IPQ8074 hw2.0 AHB WLAN.HK.2.4.0.1-00192-QCAHKSWPL_SILICONZ-1 >>>>> >>>>> Signed-off-by: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> >>>>> --- >>>>> drivers/net/wireless/ath/ath11k/core.c | 1 + >>>>> drivers/net/wireless/ath/ath11k/hw.c | 31 +++++++++++++++++++++++++++++++ >>>>> 2 files changed, 32 insertions(+) >>>>> >>>>> diff --git a/drivers/net/wireless/ath/ath11k/core.c b/drivers/net/wireless/ath/ath11k/core.c >>>>> index 347131a..f65347c 100644 >>>>> --- a/drivers/net/wireless/ath/ath11k/core.c >>>>> +++ b/drivers/net/wireless/ath/ath11k/core.c >>>>> @@ -458,6 +458,7 @@ static const struct ath11k_hw_params ath11k_hw_params[] = { >>>>> .num_vdevs = 16 + 1, >>>>> .num_peers = 512, >>>>> .supports_suspend = false, >>>>> + .hal_desc_sz = sizeof(struct hal_rx_desc_qcn9074), >>>> >>>> You mention nothing about this change in the commit log, I'll add that. >>>> >>> >>> I did mention that "HAL RX descriptor for WCN6750 is same as QCN9074", >>> you were looking at something specific? >> >> If I'm understanding correctly, by accident hal_desc_sz was not >> initialised for qcn9074. If that's the case, it would be good to mention >> that in the commit log. >> > > If I understand your comment correctly, since HAL RX descriptor is > related to data path, I thought of adding it as part of the data path > patch. Ah, now I understand better. I thought you were setting hal_desc_sz for qcn9074, but now I checked it and you were actually setting it for wcn6750. It would be better to set hal_desc_sz in patch "ath11k: Add HW params for WCN6750". Actually I would prefer that _all_ existing hw_params fields for wcn6750 would be initialised in patch "ath11k: Add HW params for WCN6750", even if just set to null or invalid values. That way it's easier to track that we are not missing any fields and makes it easier to review other patches. But this is getting more to nitpicking so definitely not a hard requirement.