| Message ID | 20181228044819.5697-2-sibis@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add firmware bindings for Q6V5 MSS/PAS | expand |
On Fri, 28 Dec 2018 10:18:18 +0530, Sibi Sankar wrote: > Add optional "firmware-name" bindings for Q6V5 MSS and PAS based > remoteprocs. For Q6V5 MSS/PAS the two/one relative firmware > paths/path are to be listed respectively. Fallback to the default > images for mba/modem for Q6V5 MSS or the default Hexagon image > for Q6V5 PAS if the "firmware-name" binding is not present. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++ > Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++ > 2 files changed, 13 insertions(+) > Reviewed-by: Rob Herring <robh@kernel.org>
Hi Sibi, On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote: > Add optional "firmware-name" bindings for Q6V5 MSS and PAS based > remoteprocs. For Q6V5 MSS/PAS the two/one relative firmware > paths/path are to be listed respectively. Fallback to the default > images for mba/modem for Q6V5 MSS or the default Hexagon image > for Q6V5 PAS if the "firmware-name" binding is not present. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++ > Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++ > 2 files changed, 13 insertions(+) > > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt > index 9c0cff3a5ed8..60ee0f73071a 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt > @@ -27,6 +27,12 @@ on the Qualcomm ADSP Hexagon core. > Value type: <stringlist> > Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack" > > +- firmware-name: > + Usage: optional > + Value type: <string> > + Definition: must list the relative firmware image path for the > + Hexagon Core. Relative to what? I still think it's a terrible idea that your driver looks for files at the top-level /lib/firmware/ directory, but now you're leaking this into the device tree. This should at a bare minimum be namespaced to something like the qcom/ sub-directory. But ideally, the driver would automatically be deriving a further sub-directory of qcom/ based on the chipset or something, and then the only thing you'd describe here is some kind of variant string -- something akin to ath10k's qcom,ath10k-calibration-variant (see Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which doesn't require a full path-name or any hierarchy. Brian > + > - clocks: > Usage: required > Value type: <prop-encoded-array> > diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > index 9ff5b0309417..3a99e7379d8c 100644 > --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt > @@ -36,6 +36,13 @@ on the Qualcomm Hexagon core. > Value type: <stringlist> > Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack" > > +- firmware-name: > + Usage: optional > + Value type: <stringlist> > + Definition: must list the relative firmware image paths for mba and > + modem. They are used for booting and authenticating the > + Hexagon core. > + > - clocks: > Usage: required > Value type: <phandle> > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project >
On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote: > On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote: > > +- firmware-name: > > + Usage: optional > > + Value type: <string> > > + Definition: must list the relative firmware image path for the > > + Hexagon Core. > > Relative to what? I still think it's a terrible idea that your driver > looks for files at the top-level /lib/firmware/ directory, but now > you're leaking this into the device tree. This should at a bare minimum > be namespaced to something like the qcom/ sub-directory. But ideally, > the driver would automatically be deriving a further sub-directory of > qcom/ based on the chipset or something, and then the only thing you'd > describe here is some kind of variant string -- something akin to > ath10k's qcom,ath10k-calibration-variant (see > Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which > doesn't require a full path-name or any hierarchy. Oh, I see Rob actually recommended this binding in v1, and it's (sort of) in use by a few other drivers. Is it really expected that we put arbitrary pathnames in device tree? None of the binding documentation seems very specific to me, and their implementations *do* allow arbitrary text. As it stands today, this is a great recipe for name collision -- e.g., how the driver today suggests "modem.XYZ" names; is Qualcomm really the only one out there making modems? :D So my natural instinct is to avoid this. But if that's what everybody wants... Brian
On Thu 03 Jan 15:50 PST 2019, Brian Norris wrote: > On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote: > > On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote: > > > +- firmware-name: > > > + Usage: optional > > > + Value type: <string> > > > + Definition: must list the relative firmware image path for the > > > + Hexagon Core. > > > > Relative to what? I still think it's a terrible idea that your driver > > looks for files at the top-level /lib/firmware/ directory, but now > > you're leaking this into the device tree. This should at a bare minimum > > be namespaced to something like the qcom/ sub-directory. But ideally, > > the driver would automatically be deriving a further sub-directory of > > qcom/ based on the chipset or something, and then the only thing you'd > > describe here is some kind of variant string -- something akin to > > ath10k's qcom,ath10k-calibration-variant (see > > Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which > > doesn't require a full path-name or any hierarchy. > > Oh, I see Rob actually recommended this binding in v1, and it's (sort > of) in use by a few other drivers. Is it really expected that we put > arbitrary pathnames in device tree? None of the binding documentation > seems very specific to me, and their implementations *do* allow > arbitrary text. As it stands today, this is a great recipe for name > collision -- e.g., how the driver today suggests "modem.XYZ" names; is > Qualcomm really the only one out there making modems? :D > > So my natural instinct is to avoid this. But if that's what everybody > wants... > I share your concern about this, but I came to suggest this as the driver cares about platforms but the firmware is (often?) device/product-specific. E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas compatible, but they are unlikely to run the same adsp firmware. This allows the individual dtb to specify which firmware the driver should use. Regards, Bjorn
Hi Bjorn, On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote: > On Thu 03 Jan 15:50 PST 2019, Brian Norris wrote: > > > On Thu, Jan 03, 2019 at 03:30:14PM -0800, Brian Norris wrote: > > > On Fri, Dec 28, 2018 at 10:18:18AM +0530, Sibi Sankar wrote: > > > > +- firmware-name: > > > > + Usage: optional > > > > + Value type: <string> > > > > + Definition: must list the relative firmware image path for the > > > > + Hexagon Core. > > > > > > Relative to what? I still think it's a terrible idea that your driver > > > looks for files at the top-level /lib/firmware/ directory, but now > > > you're leaking this into the device tree. This should at a bare minimum > > > be namespaced to something like the qcom/ sub-directory. But ideally, > > > the driver would automatically be deriving a further sub-directory of > > > qcom/ based on the chipset or something, and then the only thing you'd > > > describe here is some kind of variant string -- something akin to > > > ath10k's qcom,ath10k-calibration-variant (see > > > Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt), which > > > doesn't require a full path-name or any hierarchy. > > > > Oh, I see Rob actually recommended this binding in v1, and it's (sort > > of) in use by a few other drivers. Is it really expected that we put > > arbitrary pathnames in device tree? None of the binding documentation > > seems very specific to me, and their implementations *do* allow > > arbitrary text. As it stands today, this is a great recipe for name > > collision -- e.g., how the driver today suggests "modem.XYZ" names; is > > Qualcomm really the only one out there making modems? :D > > > > So my natural instinct is to avoid this. But if that's what everybody > > wants... > > > > I share your concern about this, but I came to suggest this as the > driver cares about platforms but the firmware is (often?) > device/product-specific. > > E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas > compatible, but they are unlikely to run the same adsp firmware. This > allows the individual dtb to specify which firmware the driver should > use. I understand this, but that still doesn't mean we should be suggesting each DTB to clutter the top-level firmware search path, especially since lazy people will probably just use "modem.mdt" and similar. That means you no longer can ship the same rootfs that supports both QCOM and <other> modems, if <other> modem also uses the same lazy format. It seems like a much better practice to at least enforce a particular prefix to things. e.g., the driver could assume: qcom/sdm845-adsp-pas/ (or if you must, just qcom/) and your DTB only gets to add .../<your-string-here> to that path. In case it isn't clear: I think it's also severely misguided that the existing driver gets away with lines like request_firmware(&fw, "modem.mdt", ...); today ;) Brian
Hi again, On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote: > On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote: > > I share your concern about this, but I came to suggest this as the > > driver cares about platforms but the firmware is (often?) > > device/product-specific. > > > > E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas > > compatible, but they are unlikely to run the same adsp firmware. This > > allows the individual dtb to specify which firmware the driver should > > use. > > I understand this, but that still doesn't mean we should be suggesting > each DTB to clutter the top-level firmware search path, especially since > lazy people will probably just use "modem.mdt" and similar. That means > you no longer can ship the same rootfs that supports both QCOM and > <other> modems, if <other> modem also uses the same lazy format. > > It seems like a much better practice to at least enforce a particular > prefix to things. e.g., the driver could assume: > > qcom/sdm845-adsp-pas/ (or if you must, just qcom/) > > and your DTB only gets to add .../<your-string-here> to that path. > > In case it isn't clear: I think it's also severely misguided that the > existing driver gets away with lines like > > request_firmware(&fw, "modem.mdt", ...); > > today ;) To add to my thoughts, since I think maybe Sibi was a little unclear of my thoughts: One of my primary concerns with the existing approach is that it's basically a complete free-for-all. We should have some minimal standards (enforced in code) such that our DTB can never point us at something like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; or lots of other bad examples). This could probably be done simply by always prefixing 'qcom/' (I don't remember -- does request_firmware() follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.) As a bonus: it would be very nice if we can provide a little more structure by default, and avoid arbitrary hierarchy in the DTS. That's where I brought up ath10k's "variant" as an example; if we can use 'compatible' to capture most of this particular Hexagon core's properties, then we only leave a single level of variability to the DTS. But I might be off-base with the "bonus" paragraph. So I'd also be somewhat happy with something much less ambitious, like just a built-in prefix ('qcom/'). And you can also just ignore my thoughts entirely (and I'll be even less happy), since Rob did already provide his Reviewed-by ;) I mostly wanted to give food for thought, in the hopes that something in here would help improve this a bit. Regards, Brian
Hi Brian/Bjorn, Thanks for the review! On 2019-01-05 07:24, Brian Norris wrote: > Hi again, > > On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote: >> On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote: >> > I share your concern about this, but I came to suggest this as the >> > driver cares about platforms but the firmware is (often?) >> > device/product-specific. >> > >> > E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas >> > compatible, but they are unlikely to run the same adsp firmware. This >> > allows the individual dtb to specify which firmware the driver should >> > use. >> >> I understand this, but that still doesn't mean we should be suggesting >> each DTB to clutter the top-level firmware search path, especially >> since >> lazy people will probably just use "modem.mdt" and similar. That means >> you no longer can ship the same rootfs that supports both QCOM and >> <other> modems, if <other> modem also uses the same lazy format. >> >> It seems like a much better practice to at least enforce a particular >> prefix to things. e.g., the driver could assume: >> >> qcom/sdm845-adsp-pas/ (or if you must, just qcom/) >> >> and your DTB only gets to add .../<your-string-here> to that path. >> >> In case it isn't clear: I think it's also severely misguided that the >> existing driver gets away with lines like >> >> request_firmware(&fw, "modem.mdt", ...); >> >> today ;) > > To add to my thoughts, since I think maybe Sibi was a little unclear of > my thoughts: > > One of my primary concerns with the existing approach is that it's > basically a complete free-for-all. We should have some minimal > standards > (enforced in code) such that our DTB can never point us at something > like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; > or lots of other bad examples). This could probably be done simply by > always prefixing 'qcom/' (I don't remember -- does request_firmware() > follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.) > > As a bonus: it would be very nice if we can provide a little more > structure by default, and avoid arbitrary hierarchy in the DTS. That's > where I brought up ath10k's "variant" as an example; if we can use > 'compatible' to capture most of this particular Hexagon core's > properties, then we only leave a single level of variability to the > DTS. > > But I might be off-base with the "bonus" paragraph. So I'd also be > somewhat happy with something much less ambitious, like just a built-in > prefix ('qcom/'). > > And you can also just ignore my thoughts entirely (and I'll be even > less > happy), since Rob did already provide his Reviewed-by ;) I mostly > wanted > to give food for thought, in the hopes that something in here would > help > improve this a bit. Bjorn, let me know how you want it implemented. I am okay with either of the following: * (variant tag based solution) or * (simply going ahead with what we have now). > > Regards, > Brian
On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote: > > Hi again, > > On Thu, Jan 03, 2019 at 04:11:58PM -0800, Brian Norris wrote: > > On Thu, Jan 03, 2019 at 04:01:45PM -0800, Bjorn Andersson wrote: > > > I share your concern about this, but I came to suggest this as the > > > driver cares about platforms but the firmware is (often?) > > > device/product-specific. > > > > > > E.g. we will serve the MTP and Pixel 3 with the qcom,sdm845-adsp-pas > > > compatible, but they are unlikely to run the same adsp firmware. This > > > allows the individual dtb to specify which firmware the driver should > > > use. > > > > I understand this, but that still doesn't mean we should be suggesting > > each DTB to clutter the top-level firmware search path, especially since > > lazy people will probably just use "modem.mdt" and similar. That means > > you no longer can ship the same rootfs that supports both QCOM and > > <other> modems, if <other> modem also uses the same lazy format. > > > > It seems like a much better practice to at least enforce a particular > > prefix to things. e.g., the driver could assume: > > > > qcom/sdm845-adsp-pas/ (or if you must, just qcom/) > > > > and your DTB only gets to add .../<your-string-here> to that path. > > > > In case it isn't clear: I think it's also severely misguided that the > > existing driver gets away with lines like > > > > request_firmware(&fw, "modem.mdt", ...); > > > > today ;) > > To add to my thoughts, since I think maybe Sibi was a little unclear of > my thoughts: > > One of my primary concerns with the existing approach is that it's > basically a complete free-for-all. We should have some minimal standards > (enforced in code) such that our DTB can never point us at something > like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; > or lots of other bad examples). This could probably be done simply by > always prefixing 'qcom/' (I don't remember -- does request_firmware() > follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.) We can write a schema to enforce some of this: firmware-name: pattern: "^\w.*" And you can have a device specific schema to enforce a subdir and/or filename(s). I tend to think we should not put part of the path in drivers. No real good reason other than we already allow that for other users of 'firmware-name'. > As a bonus: it would be very nice if we can provide a little more > structure by default, and avoid arbitrary hierarchy in the DTS. That's > where I brought up ath10k's "variant" as an example; if we can use > 'compatible' to capture most of this particular Hexagon core's > properties, then we only leave a single level of variability to the DTS. Some bindings use compatible to determine/construct the firmware name. If you want to restrict things, then that's probably how you should do it IMO. Rob
On Tue, Jan 08, 2019 at 09:22:30AM -0600, Rob Herring wrote: > On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote: > > To add to my thoughts, since I think maybe Sibi was a little unclear of > > my thoughts: > > > > One of my primary concerns with the existing approach is that it's > > basically a complete free-for-all. We should have some minimal standards > > (enforced in code) such that our DTB can never point us at something > > like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; > > or lots of other bad examples). This could probably be done simply by > > always prefixing 'qcom/' (I don't remember -- does request_firmware() > > follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.) > > We can write a schema to enforce some of this: > > firmware-name: > pattern: "^\w.*" Are DT schemas ready to use/enforce? Or would this currently just be a suggestion? I'm out of the loop. But I guess that would be interesting. > And you can have a device specific schema to enforce a subdir and/or > filename(s). > > I tend to think we should not put part of the path in drivers. No real > good reason other than we already allow that for other users of > 'firmware-name'. OK. (I was surprised you accepted paths at all. But if we're going down that road... *shrug*) > > As a bonus: it would be very nice if we can provide a little more > > structure by default, and avoid arbitrary hierarchy in the DTS. That's > > where I brought up ath10k's "variant" as an example; if we can use > > 'compatible' to capture most of this particular Hexagon core's > > properties, then we only leave a single level of variability to the DTS. > > Some bindings use compatible to determine/construct the firmware name. > If you want to restrict things, then that's probably how you should do > it IMO. We already have reasons up-thread for why we can't get this exclusively from compatible. But if we were to partially construct and/or validate paths using compatible, then the time to do that is now. As soon as this is merged without such validation, then we're stuck. Given the above, it seems like maybe the best we can do is put a recommendation into a DT schema pattern. Brian
On Wed, Jan 9, 2019 at 3:55 PM Brian Norris <briannorris@chromium.org> wrote: > > On Tue, Jan 08, 2019 at 09:22:30AM -0600, Rob Herring wrote: > > On Fri, Jan 4, 2019 at 7:54 PM Brian Norris <briannorris@chromium.org> wrote: > > > To add to my thoughts, since I think maybe Sibi was a little unclear of > > > my thoughts: > > > > > > One of my primary concerns with the existing approach is that it's > > > basically a complete free-for-all. We should have some minimal standards > > > (enforced in code) such that our DTB can never point us at something > > > like /lib/firmware/<other-vendor>/foo.bin (or /lib/firmware/modem.mdt; > > > or lots of other bad examples). This could probably be done simply by > > > always prefixing 'qcom/' (I don't remember -- does request_firmware() > > > follow '..'? e.g., 'firmware-name = "../bar/foo.bin"'.) > > > > We can write a schema to enforce some of this: > > > > firmware-name: > > pattern: "^\w.*" > > Are DT schemas ready to use/enforce? Or would this currently just be a > suggestion? I'm out of the loop. But I guess that would be interesting. Yes, please! Initial support is in 5.0-rc. I'm not requiring new bindings to be written as schemas just yet though. We need to ring out any issues with early adopters. Rob
diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt index 9c0cff3a5ed8..60ee0f73071a 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt @@ -27,6 +27,12 @@ on the Qualcomm ADSP Hexagon core. Value type: <stringlist> Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack" +- firmware-name: + Usage: optional + Value type: <string> + Definition: must list the relative firmware image path for the + Hexagon Core. + - clocks: Usage: required Value type: <prop-encoded-array> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt index 9ff5b0309417..3a99e7379d8c 100644 --- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt @@ -36,6 +36,13 @@ on the Qualcomm Hexagon core. Value type: <stringlist> Definition: must be "wdog", "fatal", "ready", "handover", "stop-ack" +- firmware-name: + Usage: optional + Value type: <stringlist> + Definition: must list the relative firmware image paths for mba and + modem. They are used for booting and authenticating the + Hexagon core. + - clocks: Usage: required Value type: <phandle>
Add optional "firmware-name" bindings for Q6V5 MSS and PAS based remoteprocs. For Q6V5 MSS/PAS the two/one relative firmware paths/path are to be listed respectively. Fallback to the default images for mba/modem for Q6V5 MSS or the default Hexagon image for Q6V5 PAS if the "firmware-name" binding is not present. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- Documentation/devicetree/bindings/remoteproc/qcom,adsp.txt | 6 ++++++ Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt | 7 +++++++ 2 files changed, 13 insertions(+)