* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-06-19 2:17 ` [PATCH v2 0/6] *** Add support for wifi QMI client driver *** Brian Norris
@ 2018-06-19 3:54 ` Govind Singh
2018-06-19 4:06 ` Govind Singh
2018-07-03 15:33 ` Kalle Valo
2 siblings, 0 replies; 8+ messages in thread
From: Govind Singh @ 2018-06-19 3:54 UTC (permalink / raw)
To: Brian Norris
Cc: andy.gross, bjorn.andersson, david.brown, linux-wireless, ath10k,
linux-kernel
On 2018-06-19 07:47, Brian Norris wrote:
> Hi Govind,
>
> One more side note: this series is called v2, but I see an older v3.
> What's up with that?
>
Earlier was typo, it was first version.
> On Tue, Jun 05, 2018 at 06:03:04PM +0530, Govind Singh wrote:
>> Add QMI client driver for Q6 integrated WLAN connectivity subsystem.
>> This module is responsible for communicating WLAN control messages to
>> FW
>> over QMI interface.
>>
>> QUALCOMM MSM Interface(QMI) provides the control interface between
>> components running b/w remote processors with underlying transport
>> layer
>> based on integrated chipset(shared memory) or discrete
>> chipset(PCI/USB/SDIO/UART).
>
> So this seems to imply QMI would work with transports that are not
> integrated. Except, your code is directly calling SNOC (one of your
> integrated chipset interfaces) code from the QMI driver. Correct? I
> suppose that's OK for now, but it's a little misleading. If you
> actually
> intend this to support multiple transports, then you might instead want
> a callback interface for this.
>
Agree, As of now there is no road-map to support qmi on discrete
chip-set on ath10k tree.
Probably if required in future we can use some generic callback and
opaque structures to
make it independent to integrated/discrete chip set.
>> QMI client driver implementation is based on qmi frmework
>> https://lwn.net/Articles/729924/.
>>
>> Below is the sequence of qmi handshake.
>>
>> QMI CLIENT(APPS) QMI
>> SERVER(FW in Q6)
>>
>> <------wlan service discoverd----
>>
>> -----connect to wlam qmi service----->
>>
>> ------------wlan info request----->
>>
>> <------------wlan info resp------------
>>
>> ------------msa info req-------->
>>
>> <------------msa info resp------------
>>
>> ------------msa ready req-------->
>>
>> <------------msa ready resp------------
>>
>> <------------msa ready indication-------
>>
>> ------------capability req------->
>>
>> <------------capability resp------------
>>
>> ------------qmi bdf req--------->
>>
>> <------------qmi bdf resp------------
>>
>> ------------qmi cal trigger------->
>>
>> <------------ QMI FW ready indication-------
>
> Let's see if I'm interpreting this right:
>
> * The above process is just initiating a handshake with the QMI
> service and doesn't actually do any loading of firmware on its own;
> it just hands things off to the SNOC client driver (and ath10k core)
> once the firmware is magically ready (??)
Yes, it is initiating the handshakes once qmi service is up on the
target Q6.
> * The ATH10K_FW_FEATURE_NON_BMI flag you added previously basically
> provides a way for a driver (and now we see which driver; it's this
> QMI / SNOC driver) to completely sidestep the typicaly in-kernel
> firmware load implementation; in fact, the kernel only reads the
> WLAN firmware just to parse some feature flags, not to actually
> program it to the device
> * Some yet-unmentioned proprietary app is involved to handle
> sideloading the actual firmware from user space
>
WLAN fw is running in modem Q6 as user PD(protection domain).
Sequence of user PD loading is given below as per current design of
Modem Q6 fw.
1) Remote proc PIL driver loads the modem fw/ROOT PD.
2) As part of ROOT pd boot-up it queries to a daemon in apps
processor(pd_mapper)
to determine how many usre pd's are getting supported in the remote
processor(Q6).
3)
> Is this correct? If not, please correct me. But if it is:
>
> * When does the user space app actually load the WLAN firmware? I'm
> not
> sure I can place it in the above diagram.
> * Is there any open source implementation of this? How am I supposed
> to
> actually use this driver, if it relies on proprietary components
> that
> I can't review and aren't really even mentioned?
>
> I hope I'm sorely wrong on this. But if I'm not, I don't see why this
> driver should be merged at all. Linux drivers should be self-sufficient
> wherever possible, and I don't see a good reason why this driver can't
> manage actually loading the WLAN firmware on its own, similar to how
> the
> BMI component of the ath10k driver loads firmware for other ath10k
> transports. But even more importantly: I believe this driver is hiding
> the fact that it relies on undocumented proprietary components to run
> on
> the CPU [1] just to make use of it at all.
>
> Brian
>
> [1] It's an unfortunate fact of life that Wifi (and in this case,
> modem+Wifi) processors will run proprietary firmware, but it's not
> accepted that Linux relies on proprietary user space.
>
>> Changes in V2:
>> Removed qmi client driver and integrated qmi client handshakes in
>> snoc platform driver.
>> Addressed comments v1 version.
>> Switched to ath10k bdf download infra(board-2.bin)
>> Added MSA fixed region support to support unload use-case.
>> Unified logging.
>>
>> Testing:
>> Tested all qmi handshakes, driver load/unload and STA/SAP sanity
>> testing.
>> Tested HW: SDM845(WCN3990)
>> Tested FW: WLAN.HL.2.0-01192-QCAHLSWMTPLZ-1
>>
>>
>>
>> Govind Singh (5):
>> ath10k: Add qmi service helpers for wcn3990 qmi client
>> dt: bindings: add bindings for msa memory region
>> ath10k: Add debug mask for QMI layer
>> firmware: qcom: scm: Add WLAN VMID for Qualcomm SCM interface
>> ath10k: Add QMI message handshake for wcn3990 client
>>
>> Rakesh Pillai (1):
>> ath10k: Add support to create boardname for non-bmi target
>>
>> .../bindings/net/wireless/qcom,ath10k.txt | 4 +
>> drivers/net/wireless/ath/ath10k/Kconfig | 13 +-
>> drivers/net/wireless/ath/ath10k/Makefile | 4 +-
>> drivers/net/wireless/ath/ath10k/core.c | 20 +-
>> drivers/net/wireless/ath/ath10k/core.h | 4 +
>> drivers/net/wireless/ath/ath10k/debug.h | 1 +
>> drivers/net/wireless/ath/ath10k/qmi.c | 1030 ++++++++
>> drivers/net/wireless/ath/ath10k/qmi.h | 136 ++
>> .../net/wireless/ath/ath10k/qmi_wlfw_v01.c | 2072
>> +++++++++++++++++
>> .../net/wireless/ath/ath10k/qmi_wlfw_v01.h | 677 ++++++
>> drivers/net/wireless/ath/ath10k/snoc.c | 209 +-
>> drivers/net/wireless/ath/ath10k/snoc.h | 3 +
>> include/linux/qcom_scm.h | 4 +-
>> 13 files changed, 4160 insertions(+), 17 deletions(-)
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.h
>>
>> --
>> 2.17.0
>>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-06-19 2:17 ` [PATCH v2 0/6] *** Add support for wifi QMI client driver *** Brian Norris
2018-06-19 3:54 ` Govind Singh
@ 2018-06-19 4:06 ` Govind Singh
2018-06-19 7:13 ` Kalle Valo
2018-07-03 15:33 ` Kalle Valo
2 siblings, 1 reply; 8+ messages in thread
From: Govind Singh @ 2018-06-19 4:06 UTC (permalink / raw)
To: Brian Norris
Cc: andy.gross, bjorn.andersson, david.brown, linux-wireless, ath10k,
linux-kernel
Hi Brian,
On 2018-06-19 07:47, Brian Norris wrote:
> Hi Govind,
>
> One more side note: this series is called v2, but I see an older v3.
> What's up with that?
>
Earlier was typo(v3), it was first version.
Pls ignore the same.
> On Tue, Jun 05, 2018 at 06:03:04PM +0530, Govind Singh wrote:
>> Add QMI client driver for Q6 integrated WLAN connectivity subsystem.
>> This module is responsible for communicating WLAN control messages to
>> FW
>> over QMI interface.
>>
>> QUALCOMM MSM Interface(QMI) provides the control interface between
>> components running b/w remote processors with underlying transport
>> layer
>> based on integrated chipset(shared memory) or discrete
>> chipset(PCI/USB/SDIO/UART).
>
> So this seems to imply QMI would work with transports that are not
> integrated. Except, your code is directly calling SNOC (one of your
> integrated chipset interfaces) code from the QMI driver. Correct? I
> suppose that's OK for now, but it's a little misleading. If you
> actually
> intend this to support multiple transports, then you might instead want
> a callback interface for this.
>
Agree, As of now there is no road-map to support qmi on discrete
chip-set on ath10k tree.
Probably if required in future, we can use some generic callback and
opaque structures to
make it independent to integrated/discrete chip set.
>> QMI client driver implementation is based on qmi frmework
>> https://lwn.net/Articles/729924/.
>>
>> Below is the sequence of qmi handshake.
>>
>> QMI CLIENT(APPS) QMI
>> SERVER(FW in Q6)
>>
>> <------wlan service discoverd----
>>
>> -----connect to wlam qmi service----->
>>
>> ------------wlan info request----->
>>
>> <------------wlan info resp------------
>>
>> ------------msa info req-------->
>>
>> <------------msa info resp------------
>>
>> ------------msa ready req-------->
>>
>> <------------msa ready resp------------
>>
>> <------------msa ready indication-------
>>
>> ------------capability req------->
>>
>> <------------capability resp------------
>>
>> ------------qmi bdf req--------->
>>
>> <------------qmi bdf resp------------
>>
>> ------------qmi cal trigger------->
>>
>> <------------ QMI FW ready indication-------
>
> Let's see if I'm interpreting this right:
>
> * The above process is just initiating a handshake with the QMI
> service and doesn't actually do any loading of firmware on its own;
> it just hands things off to the SNOC client driver (and ath10k core)
> once the firmware is magically ready (??)
Yes, it is initiating the handshakes once qmi service is up on the
target Q6.
> * The ATH10K_FW_FEATURE_NON_BMI flag you added previously basically
> provides a way for a driver (and now we see which driver; it's this
> QMI / SNOC driver) to completely sidestep the typicaly in-kernel
> firmware load implementation; in fact, the kernel only reads the
> WLAN firmware just to parse some feature flags, not to actually
> program it to the device
> * Some yet-unmentioned proprietary app is involved to handle
> sideloading the actual firmware from user space
>
WLAN fw is running in modem Q6 as user PD(protection domain).
Sequence of user PD loading is given below as per current design of
Modem Q6 fw.
1) Remote proc PIL driver loads the modem fw/ROOT PD.
2) As part of ROOT pd boot-up it queries to a daemon(pd_mapper) running
in apps processor to determine how many
user pd's are getting supported in the remote processor(Q6).
3) once supported pd info in known to Q6, tftp client in Q6 establishes
the connection with
tftp server in Q6 for user pd loading.
> Is this correct? If not, please correct me. But if it is:
>
> * When does the user space app actually load the WLAN firmware? I'm
> not
> sure I can place it in the above diagram.
> * Is there any open source implementation of this? How am I supposed
> to
> actually use this driver, if it relies on proprietary components
> that
> I can't review and aren't really even mentioned?
>
As described above.
There are no open source implementation available for tftp server as of
now and this is
work in progress.
> I hope I'm sorely wrong on this. But if I'm not, I don't see why this
> driver should be merged at all. Linux drivers should be self-sufficient
> wherever possible, and I don't see a good reason why this driver can't
> manage actually loading the WLAN firmware on its own, similar to how
> the
> BMI component of the ath10k driver loads firmware for other ath10k
> transports. But even more importantly: I believe this driver is hiding
> the fact that it relies on undocumented proprietary components to run
> on
> the CPU [1] just to make use of it at all.
>
> Brian
>
I understand your concern, currently Q6 ROOT PD infrastructure does not
support the same.
This can be documented once opensource implementation of tftp server
daemon is available.
> [1] It's an unfortunate fact of life that Wifi (and in this case,
> modem+Wifi) processors will run proprietary firmware, but it's not
> accepted that Linux relies on proprietary user space.
>
>> Changes in V2:
>> Removed qmi client driver and integrated qmi client handshakes in
>> snoc platform driver.
>> Addressed comments v1 version.
>> Switched to ath10k bdf download infra(board-2.bin)
>> Added MSA fixed region support to support unload use-case.
>> Unified logging.
>>
>> Testing:
>> Tested all qmi handshakes, driver load/unload and STA/SAP sanity
>> testing.
>> Tested HW: SDM845(WCN3990)
>> Tested FW: WLAN.HL.2.0-01192-QCAHLSWMTPLZ-1
>>
>>
>>
>> Govind Singh (5):
>> ath10k: Add qmi service helpers for wcn3990 qmi client
>> dt: bindings: add bindings for msa memory region
>> ath10k: Add debug mask for QMI layer
>> firmware: qcom: scm: Add WLAN VMID for Qualcomm SCM interface
>> ath10k: Add QMI message handshake for wcn3990 client
>>
>> Rakesh Pillai (1):
>> ath10k: Add support to create boardname for non-bmi target
>>
>> .../bindings/net/wireless/qcom,ath10k.txt | 4 +
>> drivers/net/wireless/ath/ath10k/Kconfig | 13 +-
>> drivers/net/wireless/ath/ath10k/Makefile | 4 +-
>> drivers/net/wireless/ath/ath10k/core.c | 20 +-
>> drivers/net/wireless/ath/ath10k/core.h | 4 +
>> drivers/net/wireless/ath/ath10k/debug.h | 1 +
>> drivers/net/wireless/ath/ath10k/qmi.c | 1030 ++++++++
>> drivers/net/wireless/ath/ath10k/qmi.h | 136 ++
>> .../net/wireless/ath/ath10k/qmi_wlfw_v01.c | 2072
>> +++++++++++++++++
>> .../net/wireless/ath/ath10k/qmi_wlfw_v01.h | 677 ++++++
>> drivers/net/wireless/ath/ath10k/snoc.c | 209 +-
>> drivers/net/wireless/ath/ath10k/snoc.h | 3 +
>> include/linux/qcom_scm.h | 4 +-
>> 13 files changed, 4160 insertions(+), 17 deletions(-)
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.c
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi.h
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.c
>> create mode 100644 drivers/net/wireless/ath/ath10k/qmi_wlfw_v01.h
>>
>> --
>> 2.17.0
>>
BR,
Govind
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-06-19 4:06 ` Govind Singh
@ 2018-06-19 7:13 ` Kalle Valo
2018-06-19 7:17 ` Govind Singh
0 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-06-19 7:13 UTC (permalink / raw)
To: Govind Singh
Cc: Brian Norris, linux-wireless@vger.kernel.org,
linux-kernel@vger.kernel.org, ath10k@lists.infradead.org,
bjorn.andersson@linaro.org, david.brown@linaro.org,
andy.gross@linaro.org
Govind Singh <govinds@codeaurora.org> writes:
> 3) once supported pd info in known to Q6, tftp client in Q6
> establishes the connection with tftp server in Q6 for user pd loading.
This sentence doesn't make sense, should it be that the tftp client in
Q6 establishes the connection with the tftp server in user space?
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-06-19 7:13 ` Kalle Valo
@ 2018-06-19 7:17 ` Govind Singh
0 siblings, 0 replies; 8+ messages in thread
From: Govind Singh @ 2018-06-19 7:17 UTC (permalink / raw)
To: Kalle Valo
Cc: Brian Norris, linux-wireless, linux-kernel, ath10k,
bjorn.andersson, david.brown, andy.gross
Hi Kalle,
On 2018-06-19 12:43, Kalle Valo wrote:
> Govind Singh <govinds@codeaurora.org> writes:
>
>> 3) once supported pd info in known to Q6, tftp client in Q6
>> establishes the connection with tftp server in Q6 for user pd loading.
>
> This sentence doesn't make sense, should it be that the tftp client in
> Q6 establishes the connection with the tftp server in user space?
Yes, you are right. TFTP server is running in apps user-space and client
on Q6 root pd.
BR,
Govind
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-06-19 2:17 ` [PATCH v2 0/6] *** Add support for wifi QMI client driver *** Brian Norris
2018-06-19 3:54 ` Govind Singh
2018-06-19 4:06 ` Govind Singh
@ 2018-07-03 15:33 ` Kalle Valo
2018-07-03 21:55 ` Brian Norris
2 siblings, 1 reply; 8+ messages in thread
From: Kalle Valo @ 2018-07-03 15:33 UTC (permalink / raw)
To: Brian Norris
Cc: Govind Singh, linux-wireless, linux-kernel, ath10k,
bjorn.andersson, david.brown, andy.gross
Brian Norris <briannorris@chromium.org> writes:
> On Tue, Jun 05, 2018 at 06:03:04PM +0530, Govind Singh wrote:
>> Add QMI client driver for Q6 integrated WLAN connectivity subsystem.
>> This module is responsible for communicating WLAN control messages to FW
>> over QMI interface.
>>
>> QUALCOMM MSM Interface(QMI) provides the control interface between
>> components running b/w remote processors with underlying transport layer
>> based on integrated chipset(shared memory) or discrete chipset(PCI/USB/SDIO/UART).
>
> So this seems to imply QMI would work with transports that are not
> integrated. Except, your code is directly calling SNOC (one of your
> integrated chipset interfaces) code from the QMI driver. Correct? I
> suppose that's OK for now, but it's a little misleading. If you actually
> intend this to support multiple transports, then you might instead want
> a callback interface for this.
Sure. But do we even know that the QMI interfaces are even compatible?
AFAIK QMI is just an RPC protocol, so there's no guarantee about
interface stability. So I don't see the need to support other interfaces
until we know exactly what we need to implement.
>> QMI client driver implementation is based on qmi frmework https://lwn.net/Articles/729924/.
>>
>> Below is the sequence of qmi handshake.
>>
>> QMI CLIENT(APPS) QMI SERVER(FW in Q6)
>>
>> <------wlan service discoverd----
>>
>> -----connect to wlam qmi service----->
>>
>> ------------wlan info request----->
>>
>> <------------wlan info resp------------
>>
>> ------------msa info req-------->
>>
>> <------------msa info resp------------
>>
>> ------------msa ready req-------->
>>
>> <------------msa ready resp------------
>>
>> <------------msa ready indication-------
>>
>> ------------capability req------->
>>
>> <------------capability resp------------
>>
>> ------------qmi bdf req--------->
>>
>> <------------qmi bdf resp------------
>>
>> ------------qmi cal trigger------->
>>
>> <------------ QMI FW ready indication-------
>
> Let's see if I'm interpreting this right:
>
> * The above process is just initiating a handshake with the QMI
> service and doesn't actually do any loading of firmware on its own;
> it just hands things off to the SNOC client driver (and ath10k core)
> once the firmware is magically ready (??)
> * The ATH10K_FW_FEATURE_NON_BMI flag you added previously basically
> provides a way for a driver (and now we see which driver; it's this
> QMI / SNOC driver) to completely sidestep the typicaly in-kernel
> firmware load implementation; in fact, the kernel only reads the
> WLAN firmware just to parse some feature flags, not to actually
> program it to the device
> * Some yet-unmentioned proprietary app is involved to handle
> sideloading the actual firmware from user space
>
> Is this correct? If not, please correct me. But if it is:
>
> * When does the user space app actually load the WLAN firmware? I'm not
> sure I can place it in the above diagram.
> * Is there any open source implementation of this? How am I supposed to
> actually use this driver, if it relies on proprietary components that
> I can't review and aren't really even mentioned?
>
> I hope I'm sorely wrong on this. But if I'm not, I don't see why this
> driver should be merged at all. Linux drivers should be self-sufficient
> wherever possible, and I don't see a good reason why this driver can't
> manage actually loading the WLAN firmware on its own, similar to how the
> BMI component of the ath10k driver loads firmware for other ath10k
> transports. But even more importantly: I believe this driver is hiding
> the fact that it relies on undocumented proprietary components to run on
> the CPU [1] just to make use of it at all.
First of all, thanks for bringing this up! I was aware of the need of
user space tools to download the firmware to Q6 but I assumed they were
Open Source, which to my surprise they were not. An upstream driver
definitely needs to have open user space components so that anyone can
use it, and hence I cannot apply these until that's solved. Luckily
Bjorn has been working on that and he has done good progress on those,
though I think there were some issues still.
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-07-03 15:33 ` Kalle Valo
@ 2018-07-03 21:55 ` Brian Norris
2018-07-04 10:18 ` Kalle Valo
0 siblings, 1 reply; 8+ messages in thread
From: Brian Norris @ 2018-07-03 21:55 UTC (permalink / raw)
To: Kalle Valo
Cc: Govind Singh, linux-wireless, linux-kernel, ath10k,
bjorn.andersson, david.brown, andy.gross
Hi,
On Tue, Jul 03, 2018 at 06:33:34PM +0300, Kalle Valo wrote:
> Brian Norris <briannorris@chromium.org> writes:
> > On Tue, Jun 05, 2018 at 06:03:04PM +0530, Govind Singh wrote:
> >> Add QMI client driver for Q6 integrated WLAN connectivity subsystem.
> >> This module is responsible for communicating WLAN control messages to FW
> >> over QMI interface.
> >>
> >> QUALCOMM MSM Interface(QMI) provides the control interface between
> >> components running b/w remote processors with underlying transport layer
> >> based on integrated chipset(shared memory) or discrete chipset(PCI/USB/SDIO/UART).
> >
> > So this seems to imply QMI would work with transports that are not
> > integrated. Except, your code is directly calling SNOC (one of your
> > integrated chipset interfaces) code from the QMI driver. Correct? I
> > suppose that's OK for now, but it's a little misleading. If you actually
> > intend this to support multiple transports, then you might instead want
> > a callback interface for this.
>
> Sure. But do we even know that the QMI interfaces are even compatible?
> AFAIK QMI is just an RPC protocol, so there's no guarantee about
> interface stability. So I don't see the need to support other interfaces
> until we know exactly what we need to implement.
I think my questions were meant more of clarifying questions rather than
proper suggestions. If your explanation is correct, then I'd figure this
language should mention that we're implementing a handshake specific to
SNOC (or WCN3990), instead of appearing to be agnostic.
> >> QMI client driver implementation is based on qmi frmework https://lwn.net/Articles/729924/.
> >>
> >> Below is the sequence of qmi handshake.
> >>
> >> QMI CLIENT(APPS) QMI SERVER(FW in Q6)
> >>
> >> <------wlan service discoverd----
> >>
> >> -----connect to wlam qmi service----->
> >>
> >> ------------wlan info request----->
> >>
> >> <------------wlan info resp------------
> >>
> >> ------------msa info req-------->
> >>
> >> <------------msa info resp------------
> >>
> >> ------------msa ready req-------->
> >>
> >> <------------msa ready resp------------
> >>
> >> <------------msa ready indication-------
> >>
> >> ------------capability req------->
> >>
> >> <------------capability resp------------
> >>
> >> ------------qmi bdf req--------->
> >>
> >> <------------qmi bdf resp------------
> >>
> >> ------------qmi cal trigger------->
> >>
> >> <------------ QMI FW ready indication-------
> >
> > Let's see if I'm interpreting this right:
> >
> > * The above process is just initiating a handshake with the QMI
> > service and doesn't actually do any loading of firmware on its own;
> > it just hands things off to the SNOC client driver (and ath10k core)
> > once the firmware is magically ready (??)
> > * The ATH10K_FW_FEATURE_NON_BMI flag you added previously basically
> > provides a way for a driver (and now we see which driver; it's this
> > QMI / SNOC driver) to completely sidestep the typicaly in-kernel
> > firmware load implementation; in fact, the kernel only reads the
> > WLAN firmware just to parse some feature flags, not to actually
> > program it to the device
> > * Some yet-unmentioned proprietary app is involved to handle
> > sideloading the actual firmware from user space
> >
> > Is this correct? If not, please correct me. But if it is:
> >
> > * When does the user space app actually load the WLAN firmware? I'm not
> > sure I can place it in the above diagram.
BTW, I think Govind answered most of these questions, but IMO, those
sorts of clarifying bits should be in the documentation here. Maybe in
the cover letter here, but also in either the patch description(s) or
code comments. It's nice to retain some of this architectural
description in the git history somehow -- particularly, to note what
outside dependencies there are.
> > * Is there any open source implementation of this? How am I supposed to
> > actually use this driver, if it relies on proprietary components that
> > I can't review and aren't really even mentioned?
> >
> > I hope I'm sorely wrong on this. But if I'm not, I don't see why this
> > driver should be merged at all. Linux drivers should be self-sufficient
> > wherever possible, and I don't see a good reason why this driver can't
> > manage actually loading the WLAN firmware on its own, similar to how the
> > BMI component of the ath10k driver loads firmware for other ath10k
> > transports. But even more importantly: I believe this driver is hiding
> > the fact that it relies on undocumented proprietary components to run on
> > the CPU [1] just to make use of it at all.
>
> First of all, thanks for bringing this up! I was aware of the need of
> user space tools to download the firmware to Q6 but I assumed they were
> Open Source, which to my surprise they were not. An upstream driver
> definitely needs to have open user space components so that anyone can
> use it, and hence I cannot apply these until that's solved. Luckily
> Bjorn has been working on that and he has done good progress on those,
> though I think there were some issues still.
Good to hear Bjorn is working on this. I've been asking through other
channels too, and I don't have anything more than lip service. In fact,
I've been told the opposite at times -- that I won't get source. But
then, I'm quite aware that the right hand often doesn't know what the
left hand is doing ;)
Anything you and Bjorn can do to help push this along would be great,
because while I'd love to have this driver upstream, this is a huge
sticking point for me.
Thanks,
Brian
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2 0/6] *** Add support for wifi QMI client driver ***
2018-07-03 21:55 ` Brian Norris
@ 2018-07-04 10:18 ` Kalle Valo
0 siblings, 0 replies; 8+ messages in thread
From: Kalle Valo @ 2018-07-04 10:18 UTC (permalink / raw)
To: Brian Norris
Cc: Govind Singh, linux-wireless, linux-kernel, ath10k,
bjorn.andersson, david.brown, andy.gross
Brian Norris <briannorris@chromium.org> writes:
> On Tue, Jul 03, 2018 at 06:33:34PM +0300, Kalle Valo wrote:
>> Brian Norris <briannorris@chromium.org> writes:
>> > On Tue, Jun 05, 2018 at 06:03:04PM +0530, Govind Singh wrote:
>> >> Add QMI client driver for Q6 integrated WLAN connectivity subsystem.
>> >> This module is responsible for communicating WLAN control messages to FW
>> >> over QMI interface.
>> >>
>> >> QUALCOMM MSM Interface(QMI) provides the control interface between
>> >> components running b/w remote processors with underlying transport layer
>> >> based on integrated chipset(shared memory) or discrete
>> >> chipset(PCI/USB/SDIO/UART).
>> >
>> > So this seems to imply QMI would work with transports that are not
>> > integrated. Except, your code is directly calling SNOC (one of your
>> > integrated chipset interfaces) code from the QMI driver. Correct? I
>> > suppose that's OK for now, but it's a little misleading. If you actually
>> > intend this to support multiple transports, then you might instead want
>> > a callback interface for this.
>>
>> Sure. But do we even know that the QMI interfaces are even compatible?
>> AFAIK QMI is just an RPC protocol, so there's no guarantee about
>> interface stability. So I don't see the need to support other interfaces
>> until we know exactly what we need to implement.
>
> I think my questions were meant more of clarifying questions rather than
> proper suggestions. If your explanation is correct, then I'd figure this
> language should mention that we're implementing a handshake specific to
> SNOC (or WCN3990), instead of appearing to be agnostic.
Makes sense. I haven't fully studied QMI yet but my gut feeling is that
I should consider QMI just as a transport protocol. And if different
components use QMI it does not necessarily mean that the actual
interface is the same, just that they use the same transport protocol.
>> >> QMI client driver implementation is based on qmi frmework https://lwn.net/Articles/729924/.
>> >>
>> >> Below is the sequence of qmi handshake.
>> >>
>> >> QMI CLIENT(APPS) QMI SERVER(FW in Q6)
>> >>
>> >> <------wlan service discoverd----
>> >>
>> >> -----connect to wlam qmi service----->
>> >>
>> >> ------------wlan info request----->
>> >>
>> >> <------------wlan info resp------------
>> >>
>> >> ------------msa info req-------->
>> >>
>> >> <------------msa info resp------------
>> >>
>> >> ------------msa ready req-------->
>> >>
>> >> <------------msa ready resp------------
>> >>
>> >> <------------msa ready indication-------
>> >>
>> >> ------------capability req------->
>> >>
>> >> <------------capability resp------------
>> >>
>> >> ------------qmi bdf req--------->
>> >>
>> >> <------------qmi bdf resp------------
>> >>
>> >> ------------qmi cal trigger------->
>> >>
>> >> <------------ QMI FW ready indication-------
>> >
>> > Let's see if I'm interpreting this right:
>> >
>> > * The above process is just initiating a handshake with the QMI
>> > service and doesn't actually do any loading of firmware on its own;
>> > it just hands things off to the SNOC client driver (and ath10k core)
>> > once the firmware is magically ready (??)
>> > * The ATH10K_FW_FEATURE_NON_BMI flag you added previously basically
>> > provides a way for a driver (and now we see which driver; it's this
>> > QMI / SNOC driver) to completely sidestep the typicaly in-kernel
>> > firmware load implementation; in fact, the kernel only reads the
>> > WLAN firmware just to parse some feature flags, not to actually
>> > program it to the device
>> > * Some yet-unmentioned proprietary app is involved to handle
>> > sideloading the actual firmware from user space
>> >
>> > Is this correct? If not, please correct me. But if it is:
>> >
>> > * When does the user space app actually load the WLAN firmware? I'm not
>> > sure I can place it in the above diagram.
>
> BTW, I think Govind answered most of these questions, but IMO, those
> sorts of clarifying bits should be in the documentation here. Maybe in
> the cover letter here, but also in either the patch description(s) or
> code comments. It's nice to retain some of this architectural
> description in the git history somehow -- particularly, to note what
> outside dependencies there are.
Sounds very good to me.
BTW, in the future I hope to store the cover letter also to git. Dave
already does that but I can't as patchwork.kernel.org doesn't support
it:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=2bdea157b99903c8d344dbae44fedf033db4e2c2
Hopefully the upcoming upgrade on patchwork.kernel.org makes this
possible.
>> > * Is there any open source implementation of this? How am I supposed to
>> > actually use this driver, if it relies on proprietary components that
>> > I can't review and aren't really even mentioned?
>> >
>> > I hope I'm sorely wrong on this. But if I'm not, I don't see why this
>> > driver should be merged at all. Linux drivers should be self-sufficient
>> > wherever possible, and I don't see a good reason why this driver can't
>> > manage actually loading the WLAN firmware on its own, similar to how the
>> > BMI component of the ath10k driver loads firmware for other ath10k
>> > transports. But even more importantly: I believe this driver is hiding
>> > the fact that it relies on undocumented proprietary components to run on
>> > the CPU [1] just to make use of it at all.
>>
>> First of all, thanks for bringing this up! I was aware of the need of
>> user space tools to download the firmware to Q6 but I assumed they were
>> Open Source, which to my surprise they were not. An upstream driver
>> definitely needs to have open user space components so that anyone can
>> use it, and hence I cannot apply these until that's solved. Luckily
>> Bjorn has been working on that and he has done good progress on those,
>> though I think there were some issues still.
>
> Good to hear Bjorn is working on this. I've been asking through other
> channels too, and I don't have anything more than lip service. In fact,
> I've been told the opposite at times -- that I won't get source. But
> then, I'm quite aware that the right hand often doesn't know what the
> left hand is doing ;)
Hehe, I say the same quite often :)
> Anything you and Bjorn can do to help push this along would be great,
> because while I'd love to have this driver upstream, this is a huge
> sticking point for me.
So Bjorn's work is available here:
https://github.com/andersson/tqftpserv
https://github.com/andersson/pd-mapper
Do take into account that this is very much work-in-progress, but at
least the initial feedback I got from Govind was very positive. Big
thanks to Bjorn for all this!
--
Kalle Valo
^ permalink raw reply [flat|nested] 8+ messages in thread