From: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> To: Kalle Valo <kvalo@kernel.org> Cc: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>, <devicetree@vger.kernel.org>, <robh@kernel.org> Subject: Re: [PATCH v2 02/19] ath11k: Refactor PCI code to support hybrid bus devices Date: Fri, 25 Feb 2022 11:20:37 +0530 [thread overview] Message-ID: <41f8fd92-70e4-def6-0bd1-c764b1445d68@quicinc.com> (raw) In-Reply-To: <87ee4sgo7l.fsf@kernel.org> 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. Thanks, Manikanta
WARNING: multiple messages have this Message-ID (diff)
From: Manikanta Pubbisetty <quic_mpubbise@quicinc.com> To: Kalle Valo <kvalo@kernel.org> Cc: <ath11k@lists.infradead.org>, <linux-wireless@vger.kernel.org>, <devicetree@vger.kernel.org>, <robh@kernel.org> Subject: Re: [PATCH v2 02/19] ath11k: Refactor PCI code to support hybrid bus devices Date: Fri, 25 Feb 2022 11:20:37 +0530 [thread overview] Message-ID: <41f8fd92-70e4-def6-0bd1-c764b1445d68@quicinc.com> (raw) In-Reply-To: <87ee4sgo7l.fsf@kernel.org> 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. Thanks, Manikanta -- ath11k mailing list ath11k@lists.infradead.org http://lists.infradead.org/mailman/listinfo/ath11k
next prev parent reply other threads:[~2022-02-25 5:50 UTC|newest] Thread overview: 98+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-01-16 12:46 [PATCH v2 00/19] add support for WCN6750 Manikanta Pubbisetty 2022-01-16 12:46 ` Manikanta Pubbisetty 2022-01-16 12:46 ` [PATCH v2 01/19] ath11k: PCI changes to support WCN6750 Manikanta Pubbisetty 2022-01-16 12:46 ` Manikanta Pubbisetty 2022-01-16 12:46 ` [PATCH v2 02/19] ath11k: Refactor PCI code to support hybrid bus devices Manikanta Pubbisetty 2022-01-16 12:46 ` Manikanta Pubbisetty 2022-01-28 10:16 ` Kalle Valo 2022-01-28 10:16 ` Kalle Valo 2022-02-25 5:50 ` Manikanta Pubbisetty [this message] 2022-02-25 5:50 ` Manikanta Pubbisetty 2022-03-08 6:00 ` Manikanta Pubbisetty 2022-03-08 6:00 ` Manikanta Pubbisetty 2022-01-28 12:13 ` Kalle Valo 2022-01-28 12:13 ` Kalle Valo 2022-02-21 8:04 ` Manikanta Pubbisetty 2022-02-21 8:04 ` Manikanta Pubbisetty 2022-02-21 9:19 ` Kalle Valo 2022-02-21 9:19 ` Kalle Valo 2022-02-21 10:19 ` Manikanta Pubbisetty 2022-02-21 10:19 ` Manikanta Pubbisetty 2022-01-16 12:46 ` [PATCH v2 03/19] ath11k: Choose MSI config based on HW revision Manikanta Pubbisetty 2022-01-16 12:46 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 04/19] ath11k: Refactor MSI logic Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 05/19] ath11k: Remove core PCI references from PCI common code Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-28 10:20 ` Kalle Valo 2022-01-28 10:20 ` Kalle Valo 2022-01-28 10:37 ` Kalle Valo 2022-01-28 10:37 ` Kalle Valo 2022-02-21 7:07 ` Manikanta Pubbisetty 2022-02-21 7:07 ` Manikanta Pubbisetty 2022-02-21 9:21 ` Kalle Valo 2022-02-21 9:21 ` Kalle Valo 2022-02-21 6:55 ` Manikanta Pubbisetty 2022-02-21 6:55 ` Manikanta Pubbisetty 2022-02-21 9:17 ` Kalle Valo 2022-02-21 9:17 ` Kalle Valo 2022-02-21 10:08 ` Manikanta Pubbisetty 2022-02-21 10:08 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 06/19] ath11k: Add HW params for WCN6750 Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 07/19] ath11k: Add bus " Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 08/19] ath11k: Add register access logic " Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 09/19] ath11k: Fetch device information via QMI " Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 10/19] ath11k: Add QMI changes " Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-28 10:50 ` Kalle Valo 2022-01-28 10:50 ` Kalle Valo 2022-02-21 8:02 ` Manikanta Pubbisetty 2022-02-21 8:02 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 11/19] ath11k: HAL changes to support WCN6750 Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 12/19] ath11k: Datapath " Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-04-27 5:32 ` Kalle Valo 2022-04-27 5:32 ` Kalle Valo 2022-04-27 5:37 ` Manikanta Pubbisetty 2022-04-27 5:37 ` Manikanta Pubbisetty 2022-04-27 5:48 ` Kalle Valo 2022-04-27 5:48 ` Kalle Valo 2022-04-27 5:56 ` Manikanta Pubbisetty 2022-04-27 5:56 ` Manikanta Pubbisetty 2022-04-29 8:56 ` Kalle Valo 2022-04-29 8:56 ` Kalle Valo 2022-01-16 12:47 ` [PATCH v2 13/19] ath11k: Fix RX de-fragmentation issue on WCN6750 Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 14/19] ath11k: Do not put HW in DBS mode for WCN6750 Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 15/19] ath11k: WMI changes to support WCN6750 Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 16/19] ath11k: Update WBM idle ring HP after FW mode on Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 17/19] ath11k: Add support for WCN6750 device Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 18/19] ath11k: Add support for targets without trustzone Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-28 10:42 ` Kalle Valo 2022-01-28 10:42 ` Kalle Valo 2022-02-21 8:00 ` Manikanta Pubbisetty 2022-02-21 8:00 ` Manikanta Pubbisetty 2022-01-16 12:47 ` [PATCH v2 19/19] dt: bindings: net: add bindings of WCN6750 for ath11k Manikanta Pubbisetty 2022-01-16 12:47 ` Manikanta Pubbisetty 2022-01-28 10:39 ` Kalle Valo 2022-01-28 10:39 ` Kalle Valo 2022-02-21 7:08 ` Manikanta Pubbisetty 2022-02-21 7:08 ` Manikanta Pubbisetty 2022-02-07 22:13 ` Rob Herring 2022-02-07 22:13 ` Rob Herring 2022-02-25 11:34 ` Manikanta Pubbisetty 2022-02-25 11:34 ` Manikanta Pubbisetty 2022-01-28 10:07 ` [PATCH v2 00/19] add support for WCN6750 Kalle Valo 2022-01-28 10:07 ` Kalle Valo 2022-02-21 6:29 ` Manikanta Pubbisetty 2022-02-21 6:29 ` Manikanta Pubbisetty
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=41f8fd92-70e4-def6-0bd1-c764b1445d68@quicinc.com \ --to=quic_mpubbise@quicinc.com \ --cc=ath11k@lists.infradead.org \ --cc=devicetree@vger.kernel.org \ --cc=kvalo@kernel.org \ --cc=linux-wireless@vger.kernel.org \ --cc=robh@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.