* [PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS @ 2018-12-28 4:48 Sibi Sankar 2018-12-28 4:48 ` [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 Sibi Sankar 2018-12-28 4:48 ` [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings Sibi Sankar 0 siblings, 2 replies; 16+ messages in thread From: Sibi Sankar @ 2018-12-28 4:48 UTC (permalink / raw) To: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross Cc: briannorris, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree, Sibi Sankar Q6V5 MSS on certain SoCs like SDM845 are capable of operating under completely different configuration (like Non-Modem WLAN configuration) depending on the firmware loaded without any change in boot sequence of the Hexagon core. The patch series is ultimately aimed to avoid multiple compatibles per SoC to just specify different upstreamed firmware locations. This is achieved by using "firmware-name" binding to store the relative path of mba/modem/pas firmware images. remoteproc@4080000 { ... firmware-name = "qcom/sdm845/mss/mba.mbn", "qcom/sdm845/mss/modem.mdt"; ... } remoteproc@17300000 { ... firmware-name = "qcom/sdm845/lpass/adsp.mdt"; ... } Suggested-by: Bjorn Andersson <bjorn.andersson@linaro.org> v2: * Replace "qcom,firmware" with "firmware-name" as suggested by Rob * Include dt-bindings/parsing logic for PAS based remoteprocs Sibi Sankar (2): dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 remoteproc: qcom: Add support for parsing fw dt bindings .../bindings/remoteproc/qcom,adsp.txt | 6 +++ .../bindings/remoteproc/qcom,q6v5.txt | 7 +++ drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++---- drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++- 4 files changed, 59 insertions(+), 11 deletions(-) -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2018-12-28 4:48 [PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS Sibi Sankar @ 2018-12-28 4:48 ` Sibi Sankar 2018-12-28 22:17 ` Rob Herring 2019-01-03 23:30 ` Brian Norris 2018-12-28 4:48 ` [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings Sibi Sankar 1 sibling, 2 replies; 16+ messages in thread From: Sibi Sankar @ 2018-12-28 4:48 UTC (permalink / raw) To: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross Cc: briannorris, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree, Sibi Sankar 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. + - 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2018-12-28 4:48 ` [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 Sibi Sankar @ 2018-12-28 22:17 ` Rob Herring 2019-01-03 23:30 ` Brian Norris 1 sibling, 0 replies; 16+ messages in thread From: Rob Herring @ 2018-12-28 22:17 UTC (permalink / raw) To: Sibi Sankar Cc: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross, briannorris, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree, Sibi Sankar 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2018-12-28 4:48 ` [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 Sibi Sankar 2018-12-28 22:17 ` Rob Herring @ 2019-01-03 23:30 ` Brian Norris 2019-01-03 23:50 ` Brian Norris 1 sibling, 1 reply; 16+ messages in thread From: Brian Norris @ 2019-01-03 23:30 UTC (permalink / raw) To: Sibi Sankar Cc: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-03 23:30 ` Brian Norris @ 2019-01-03 23:50 ` Brian Norris 2019-01-04 0:01 ` Bjorn Andersson 0 siblings, 1 reply; 16+ messages in thread From: Brian Norris @ 2019-01-03 23:50 UTC (permalink / raw) To: Sibi Sankar Cc: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-03 23:50 ` Brian Norris @ 2019-01-04 0:01 ` Bjorn Andersson 2019-01-04 0:11 ` Brian Norris 0 siblings, 1 reply; 16+ messages in thread From: Bjorn Andersson @ 2019-01-04 0:01 UTC (permalink / raw) To: Brian Norris Cc: Sibi Sankar, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-04 0:01 ` Bjorn Andersson @ 2019-01-04 0:11 ` Brian Norris 2019-01-05 1:54 ` Brian Norris 0 siblings, 1 reply; 16+ messages in thread From: Brian Norris @ 2019-01-04 0:11 UTC (permalink / raw) To: Bjorn Andersson Cc: Sibi Sankar, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-04 0:11 ` Brian Norris @ 2019-01-05 1:54 ` Brian Norris 2019-01-08 10:50 ` Sibi Sankar 2019-01-08 15:22 ` Rob Herring 0 siblings, 2 replies; 16+ messages in thread From: Brian Norris @ 2019-01-05 1:54 UTC (permalink / raw) To: Bjorn Andersson Cc: Sibi Sankar, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-05 1:54 ` Brian Norris @ 2019-01-08 10:50 ` Sibi Sankar 2019-01-08 15:22 ` Rob Herring 1 sibling, 0 replies; 16+ messages in thread From: Sibi Sankar @ 2019-01-08 10:50 UTC (permalink / raw) To: Brian Norris Cc: Bjorn Andersson, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree, linux-remoteproc-owner 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 -- -- Sibi Sankar -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-05 1:54 ` Brian Norris 2019-01-08 10:50 ` Sibi Sankar @ 2019-01-08 15:22 ` Rob Herring 2019-01-09 21:55 ` Brian Norris 1 sibling, 1 reply; 16+ messages in thread From: Rob Herring @ 2019-01-08 15:22 UTC (permalink / raw) To: Brian Norris Cc: Bjorn Andersson, Sibi Sankar, David Brown, Mark Rutland, Andy Gross, Avaneesh Kumar Dwivedi, Chris Lew, linux-kernel, linux-arm-msm-owner, Ohad Ben-Cohen, open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-08 15:22 ` Rob Herring @ 2019-01-09 21:55 ` Brian Norris 2019-01-10 14:56 ` Rob Herring 0 siblings, 1 reply; 16+ messages in thread From: Brian Norris @ 2019-01-09 21:55 UTC (permalink / raw) To: Rob Herring Cc: Bjorn Andersson, Sibi Sankar, David Brown, Mark Rutland, Andy Gross, Avaneesh Kumar Dwivedi, Chris Lew, linux-kernel, linux-arm-msm-owner, Ohad Ben-Cohen, open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 2019-01-09 21:55 ` Brian Norris @ 2019-01-10 14:56 ` Rob Herring 0 siblings, 0 replies; 16+ messages in thread From: Rob Herring @ 2019-01-10 14:56 UTC (permalink / raw) To: Brian Norris Cc: Bjorn Andersson, Sibi Sankar, David Brown, Mark Rutland, Andy Gross, Avaneesh Kumar Dwivedi, Chris Lew, linux-kernel, linux-arm-msm-owner, Ohad Ben-Cohen, open list:REMOTE PROCESSOR (REMOTEPROC) SUBSYSTEM, devicetree 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings 2018-12-28 4:48 [PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS Sibi Sankar 2018-12-28 4:48 ` [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 Sibi Sankar @ 2018-12-28 4:48 ` Sibi Sankar 2019-01-03 23:09 ` Bjorn Andersson 2019-01-03 23:44 ` Brian Norris 1 sibling, 2 replies; 16+ messages in thread From: Sibi Sankar @ 2018-12-28 4:48 UTC (permalink / raw) To: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross Cc: briannorris, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree, Sibi Sankar Add support for parsing "firmware-name" dt bindings which specifies the relative paths of mba/modem/pas image as strings. Fallback to the default paths for mba/modem/pas image on -EINVAL. Signed-off-by: Sibi Sankar <sibis@codeaurora.org> --- drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++------- drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++- 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c index 01be7314e176..c75179006e24 100644 --- a/drivers/remoteproc/qcom_q6v5_mss.c +++ b/drivers/remoteproc/qcom_q6v5_mss.c @@ -188,6 +188,7 @@ struct q6v5 { bool has_alt_reset; int mpss_perm; int mba_perm; + const char *hexagon_mdt_image; int version; }; @@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc) phys_addr_t min_addr = PHYS_ADDR_MAX; phys_addr_t max_addr = 0; bool relocate = false; - char seg_name[10]; + char *fw_name; + size_t fw_name_len; ssize_t offset; size_t size = 0; void *ptr; int ret; int i; - ret = request_firmware(&fw, "modem.mdt", qproc->dev); + fw_name_len = strlen(qproc->hexagon_mdt_image); + if (fw_name_len <= 4) + return -EINVAL; + + fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL); + if (!fw_name) + return -ENOMEM; + + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); if (ret < 0) { - dev_err(qproc->dev, "unable to load modem.mdt\n"); - return ret; + dev_err(qproc->dev, "unable to load %s\n", + qproc->hexagon_mdt_image); + goto out; } /* Initialize the RMB validator */ @@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) ptr = qproc->mpss_region + offset; if (phdr->p_filesz) { - snprintf(seg_name, sizeof(seg_name), "modem.b%02d", i); - ret = request_firmware(&seg_fw, seg_name, qproc->dev); + snprintf(fw_name + fw_name_len - 3, fw_name_len, + "b%02d", i); + ret = request_firmware(&seg_fw, fw_name, qproc->dev); if (ret) { - dev_err(qproc->dev, "failed to load %s\n", seg_name); + dev_err(qproc->dev, "failed to load %s\n", + fw_name); goto release_firmware; } @@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc) release_firmware: release_firmware(fw); +out: + kfree(fw_name); return ret < 0 ? ret : 0; } @@ -1075,9 +1090,10 @@ static int qcom_q6v5_register_dump_segments(struct rproc *rproc, unsigned long i; int ret; - ret = request_firmware(&fw, "modem.mdt", qproc->dev); + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); if (ret < 0) { - dev_err(qproc->dev, "unable to load modem.mdt\n"); + dev_err(qproc->dev, "unable to load %s\n", + qproc->hexagon_mdt_image); return ret; } @@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device *pdev) const struct rproc_hexagon_res *desc; struct q6v5 *qproc; struct rproc *rproc; + const char *mba_image; + const char *fw_name[2]; int ret; desc = of_device_get_match_data(&pdev->dev); @@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device *pdev) if (desc->need_mem_protection && !qcom_scm_is_available()) return -EPROBE_DEFER; + ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name", + fw_name, 2); + if (ret != -EINVAL && ret != 2) + return ret > 0 ? -EINVAL : ret; + + mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0]; + rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, - desc->hexagon_mba_image, sizeof(*qproc)); + mba_image, sizeof(*qproc)); if (!rproc) { dev_err(&pdev->dev, "failed to allocate rproc\n"); return -ENOMEM; @@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device *pdev) qproc = (struct q6v5 *)rproc->priv; qproc->dev = &pdev->dev; qproc->rproc = rproc; + qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1]; platform_set_drvdata(pdev, qproc); ret = q6v5_init_mem(qproc, pdev); diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c index b1e63fcd5fdf..141c7da29e9a 100644 --- a/drivers/remoteproc/qcom_q6v5_pas.c +++ b/drivers/remoteproc/qcom_q6v5_pas.c @@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device *pdev) const struct adsp_data *desc; struct qcom_adsp *adsp; struct rproc *rproc; + const char *fw_name; + const char *of_fw_name; int ret; desc = of_device_get_match_data(&pdev->dev); @@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device *pdev) if (!qcom_scm_is_available()) return -EPROBE_DEFER; + ret = of_property_read_string(pdev->dev.of_node, "firmware-name", + &of_fw_name); + if (ret && ret != -EINVAL) + return ret; + + fw_name = ret ? desc->firmware_name : of_fw_name; + rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops, - desc->firmware_name, sizeof(*adsp)); + fw_name, sizeof(*adsp)); if (!rproc) { dev_err(&pdev->dev, "unable to allocate remoteproc\n"); return -ENOMEM; -- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings 2018-12-28 4:48 ` [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings Sibi Sankar @ 2019-01-03 23:09 ` Bjorn Andersson 2019-01-03 23:44 ` Brian Norris 1 sibling, 0 replies; 16+ messages in thread From: Bjorn Andersson @ 2019-01-03 23:09 UTC (permalink / raw) To: Sibi Sankar Cc: david.brown, robh+dt, mark.rutland, andy.gross, briannorris, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree On Thu 27 Dec 20:48 PST 2018, Sibi Sankar wrote: > Add support for parsing "firmware-name" dt bindings which specifies > the relative paths of mba/modem/pas image as strings. Fallback to > the default paths for mba/modem/pas image on -EINVAL. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> Thanks Sibi, just some minor style issues I would prefer to have fixed. I picked patch 1, so no need to resend that. > --- > drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++------- > drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++- > 2 files changed, 46 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 01be7314e176..c75179006e24 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -188,6 +188,7 @@ struct q6v5 { > bool has_alt_reset; > int mpss_perm; > int mba_perm; > + const char *hexagon_mdt_image; > int version; > }; > > @@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > phys_addr_t min_addr = PHYS_ADDR_MAX; > phys_addr_t max_addr = 0; > bool relocate = false; > - char seg_name[10]; > + char *fw_name; > + size_t fw_name_len; > ssize_t offset; > size_t size = 0; > void *ptr; > int ret; > int i; > > - ret = request_firmware(&fw, "modem.mdt", qproc->dev); > + fw_name_len = strlen(qproc->hexagon_mdt_image); > + if (fw_name_len <= 4) > + return -EINVAL; > + > + fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL); > + if (!fw_name) > + return -ENOMEM; > + > + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); Use fw_name here. > if (ret < 0) { > - dev_err(qproc->dev, "unable to load modem.mdt\n"); > - return ret; > + dev_err(qproc->dev, "unable to load %s\n", > + qproc->hexagon_mdt_image); > + goto out; > } > > /* Initialize the RMB validator */ > @@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > ptr = qproc->mpss_region + offset; > > if (phdr->p_filesz) { > - snprintf(seg_name, sizeof(seg_name), "modem.b%02d", i); > - ret = request_firmware(&seg_fw, seg_name, qproc->dev); > + snprintf(fw_name + fw_name_len - 3, fw_name_len, > + "b%02d", i); > + ret = request_firmware(&seg_fw, fw_name, qproc->dev); > if (ret) { > - dev_err(qproc->dev, "failed to load %s\n", seg_name); > + dev_err(qproc->dev, "failed to load %s\n", > + fw_name); Follow my lead and break the 80-char limit. > goto release_firmware; > } > > @@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > release_firmware: > release_firmware(fw); > +out: > + kfree(fw_name); > > return ret < 0 ? ret : 0; > } > @@ -1075,9 +1090,10 @@ static int qcom_q6v5_register_dump_segments(struct rproc *rproc, > unsigned long i; > int ret; > > - ret = request_firmware(&fw, "modem.mdt", qproc->dev); > + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); > if (ret < 0) { > - dev_err(qproc->dev, "unable to load modem.mdt\n"); > + dev_err(qproc->dev, "unable to load %s\n", > + qproc->hexagon_mdt_image); > return ret; > } > > @@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device *pdev) > const struct rproc_hexagon_res *desc; > struct q6v5 *qproc; > struct rproc *rproc; > + const char *mba_image; > + const char *fw_name[2]; > int ret; > > desc = of_device_get_match_data(&pdev->dev); > @@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device *pdev) > if (desc->need_mem_protection && !qcom_scm_is_available()) > return -EPROBE_DEFER; > > + ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name", > + fw_name, 2); > + if (ret != -EINVAL && ret != 2) > + return ret > 0 ? -EINVAL : ret; > + > + mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0]; Use the fact that of_property_read_string_index() leaves the output parameter untouched if it doesn't succeed in parsing the property. I.e. do: mba_image = desc->hexagon_mba_image; ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", 0, &mba_image); if (ret < 0 && ret != -EINVAL) fail(); and qproc->hexagon_mdt_image = "modem.mdt"; ret = of_property_read_string_index(pdev->dev.of_node, "firmware-name", 1, &qproc->hexagon_mdt_image); if (ret < 0 && ret != -EINVAL) fail(); > + > rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, > - desc->hexagon_mba_image, sizeof(*qproc)); > + mba_image, sizeof(*qproc)); > if (!rproc) { > dev_err(&pdev->dev, "failed to allocate rproc\n"); > return -ENOMEM; > @@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device *pdev) > qproc = (struct q6v5 *)rproc->priv; > qproc->dev = &pdev->dev; > qproc->rproc = rproc; > + qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1]; > platform_set_drvdata(pdev, qproc); > > ret = q6v5_init_mem(qproc, pdev); > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c > index b1e63fcd5fdf..141c7da29e9a 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device *pdev) > const struct adsp_data *desc; > struct qcom_adsp *adsp; > struct rproc *rproc; > + const char *fw_name; > + const char *of_fw_name; > int ret; > > desc = of_device_get_match_data(&pdev->dev); > @@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device *pdev) > if (!qcom_scm_is_available()) > return -EPROBE_DEFER; > > + ret = of_property_read_string(pdev->dev.of_node, "firmware-name", > + &of_fw_name); > + if (ret && ret != -EINVAL) > + return ret; > + > + fw_name = ret ? desc->firmware_name : of_fw_name; As above, please use the fact that the last parameter of of_property_read_string() doesn't modify the output if it fails. > + > rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops, > - desc->firmware_name, sizeof(*adsp)); > + fw_name, sizeof(*adsp)); > if (!rproc) { > dev_err(&pdev->dev, "unable to allocate remoteproc\n"); > return -ENOMEM; Regards, Bjorn ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings 2018-12-28 4:48 ` [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings Sibi Sankar 2019-01-03 23:09 ` Bjorn Andersson @ 2019-01-03 23:44 ` Brian Norris 2019-01-08 10:32 ` Sibi Sankar 1 sibling, 1 reply; 16+ messages in thread From: Brian Norris @ 2019-01-03 23:44 UTC (permalink / raw) To: Sibi Sankar Cc: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree On Fri, Dec 28, 2018 at 10:18:19AM +0530, Sibi Sankar wrote: > Add support for parsing "firmware-name" dt bindings which specifies > the relative paths of mba/modem/pas image as strings. Fallback to > the default paths for mba/modem/pas image on -EINVAL. > > Signed-off-by: Sibi Sankar <sibis@codeaurora.org> > --- > drivers/remoteproc/qcom_q6v5_mss.c | 46 +++++++++++++++++++++++------- > drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++- > 2 files changed, 46 insertions(+), 11 deletions(-) > > diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c > index 01be7314e176..c75179006e24 100644 > --- a/drivers/remoteproc/qcom_q6v5_mss.c > +++ b/drivers/remoteproc/qcom_q6v5_mss.c > @@ -188,6 +188,7 @@ struct q6v5 { > bool has_alt_reset; > int mpss_perm; > int mba_perm; > + const char *hexagon_mdt_image; > int version; > }; > > @@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > phys_addr_t min_addr = PHYS_ADDR_MAX; > phys_addr_t max_addr = 0; > bool relocate = false; > - char seg_name[10]; > + char *fw_name; > + size_t fw_name_len; > ssize_t offset; > size_t size = 0; > void *ptr; > int ret; > int i; > > - ret = request_firmware(&fw, "modem.mdt", qproc->dev); > + fw_name_len = strlen(qproc->hexagon_mdt_image); > + if (fw_name_len <= 4) > + return -EINVAL; > + > + fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL); > + if (!fw_name) > + return -ENOMEM; > + > + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); > if (ret < 0) { > - dev_err(qproc->dev, "unable to load modem.mdt\n"); > - return ret; > + dev_err(qproc->dev, "unable to load %s\n", > + qproc->hexagon_mdt_image); > + goto out; > } > > /* Initialize the RMB validator */ > @@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > ptr = qproc->mpss_region + offset; > > if (phdr->p_filesz) { > - snprintf(seg_name, sizeof(seg_name), "modem.b%02d", i); > - ret = request_firmware(&seg_fw, seg_name, qproc->dev); > + snprintf(fw_name + fw_name_len - 3, fw_name_len, > + "b%02d", i); So, you're assuming that 'fw_name' ends in '.XXX' (for some 3-char value of 'XXX')? Seems a bit odd. But if you really want this, it feels like you should enforce this, and either comment on what you're doing or else use a proper computation that makes it clear (e.g., strlen("bin")). Brian > + ret = request_firmware(&seg_fw, fw_name, qproc->dev); > if (ret) { > - dev_err(qproc->dev, "failed to load %s\n", seg_name); > + dev_err(qproc->dev, "failed to load %s\n", > + fw_name); > goto release_firmware; > } > > @@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc) > > release_firmware: > release_firmware(fw); > +out: > + kfree(fw_name); > > return ret < 0 ? ret : 0; > } > @@ -1075,9 +1090,10 @@ static int qcom_q6v5_register_dump_segments(struct rproc *rproc, > unsigned long i; > int ret; > > - ret = request_firmware(&fw, "modem.mdt", qproc->dev); > + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); > if (ret < 0) { > - dev_err(qproc->dev, "unable to load modem.mdt\n"); > + dev_err(qproc->dev, "unable to load %s\n", > + qproc->hexagon_mdt_image); > return ret; > } > > @@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device *pdev) > const struct rproc_hexagon_res *desc; > struct q6v5 *qproc; > struct rproc *rproc; > + const char *mba_image; > + const char *fw_name[2]; > int ret; > > desc = of_device_get_match_data(&pdev->dev); > @@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device *pdev) > if (desc->need_mem_protection && !qcom_scm_is_available()) > return -EPROBE_DEFER; > > + ret = of_property_read_string_array(pdev->dev.of_node, "firmware-name", > + fw_name, 2); > + if (ret != -EINVAL && ret != 2) > + return ret > 0 ? -EINVAL : ret; > + > + mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0]; > + > rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, > - desc->hexagon_mba_image, sizeof(*qproc)); > + mba_image, sizeof(*qproc)); > if (!rproc) { > dev_err(&pdev->dev, "failed to allocate rproc\n"); > return -ENOMEM; > @@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device *pdev) > qproc = (struct q6v5 *)rproc->priv; > qproc->dev = &pdev->dev; > qproc->rproc = rproc; > + qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1]; > platform_set_drvdata(pdev, qproc); > > ret = q6v5_init_mem(qproc, pdev); > diff --git a/drivers/remoteproc/qcom_q6v5_pas.c b/drivers/remoteproc/qcom_q6v5_pas.c > index b1e63fcd5fdf..141c7da29e9a 100644 > --- a/drivers/remoteproc/qcom_q6v5_pas.c > +++ b/drivers/remoteproc/qcom_q6v5_pas.c > @@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device *pdev) > const struct adsp_data *desc; > struct qcom_adsp *adsp; > struct rproc *rproc; > + const char *fw_name; > + const char *of_fw_name; > int ret; > > desc = of_device_get_match_data(&pdev->dev); > @@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device *pdev) > if (!qcom_scm_is_available()) > return -EPROBE_DEFER; > > + ret = of_property_read_string(pdev->dev.of_node, "firmware-name", > + &of_fw_name); > + if (ret && ret != -EINVAL) > + return ret; > + > + fw_name = ret ? desc->firmware_name : of_fw_name; > + > rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops, > - desc->firmware_name, sizeof(*adsp)); > + fw_name, sizeof(*adsp)); > if (!rproc) { > dev_err(&pdev->dev, "unable to allocate remoteproc\n"); > return -ENOMEM; > -- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings 2019-01-03 23:44 ` Brian Norris @ 2019-01-08 10:32 ` Sibi Sankar 0 siblings, 0 replies; 16+ messages in thread From: Sibi Sankar @ 2019-01-08 10:32 UTC (permalink / raw) To: Brian Norris Cc: bjorn.andersson, david.brown, robh+dt, mark.rutland, andy.gross, akdwived, clew, linux-kernel, linux-arm-msm-owner, ohad, linux-remoteproc, devicetree Hi Brian, Thanks for the review! On 2019-01-04 05:14, Brian Norris wrote: > On Fri, Dec 28, 2018 at 10:18:19AM +0530, Sibi Sankar wrote: >> Add support for parsing "firmware-name" dt bindings which specifies >> the relative paths of mba/modem/pas image as strings. Fallback to >> the default paths for mba/modem/pas image on -EINVAL. >> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org> >> --- >> drivers/remoteproc/qcom_q6v5_mss.c | 46 >> +++++++++++++++++++++++------- >> drivers/remoteproc/qcom_q6v5_pas.c | 11 ++++++- >> 2 files changed, 46 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c >> b/drivers/remoteproc/qcom_q6v5_mss.c >> index 01be7314e176..c75179006e24 100644 >> --- a/drivers/remoteproc/qcom_q6v5_mss.c >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c >> @@ -188,6 +188,7 @@ struct q6v5 { >> bool has_alt_reset; >> int mpss_perm; >> int mba_perm; >> + const char *hexagon_mdt_image; >> int version; >> }; >> >> @@ -860,17 +861,27 @@ static int q6v5_mpss_load(struct q6v5 *qproc) >> phys_addr_t min_addr = PHYS_ADDR_MAX; >> phys_addr_t max_addr = 0; >> bool relocate = false; >> - char seg_name[10]; >> + char *fw_name; >> + size_t fw_name_len; >> ssize_t offset; >> size_t size = 0; >> void *ptr; >> int ret; >> int i; >> >> - ret = request_firmware(&fw, "modem.mdt", qproc->dev); >> + fw_name_len = strlen(qproc->hexagon_mdt_image); >> + if (fw_name_len <= 4) >> + return -EINVAL; >> + >> + fw_name = kstrdup(qproc->hexagon_mdt_image, GFP_KERNEL); >> + if (!fw_name) >> + return -ENOMEM; >> + >> + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); >> if (ret < 0) { >> - dev_err(qproc->dev, "unable to load modem.mdt\n"); >> - return ret; >> + dev_err(qproc->dev, "unable to load %s\n", >> + qproc->hexagon_mdt_image); >> + goto out; >> } >> >> /* Initialize the RMB validator */ >> @@ -918,10 +929,12 @@ static int q6v5_mpss_load(struct q6v5 *qproc) >> ptr = qproc->mpss_region + offset; >> >> if (phdr->p_filesz) { >> - snprintf(seg_name, sizeof(seg_name), "modem.b%02d", i); >> - ret = request_firmware(&seg_fw, seg_name, qproc->dev); >> + snprintf(fw_name + fw_name_len - 3, fw_name_len, >> + "b%02d", i); > > So, you're assuming that 'fw_name' ends in '.XXX' (for some 3-char > value > of 'XXX')? Seems a bit odd. But if you really want this, it feels like > you should enforce this, and either comment on what you're doing or > else > use a proper computation that makes it clear (e.g., strlen("bin")). > > Brian we want to construct the names of the fw blobs incrementally i.e xxx.xxx to xxx.bxx. Given that we restrict the fw_name_len to be greater than 4 at the beginning, I'll probably do something like this. /* Replace "xxx.xxx" with "xxx.bxx" */ sprintf(fw_name + fw_name_len - 3, "b%02d", i); > >> + ret = request_firmware(&seg_fw, fw_name, qproc->dev); >> if (ret) { >> - dev_err(qproc->dev, "failed to load %s\n", seg_name); >> + dev_err(qproc->dev, "failed to load %s\n", >> + fw_name); >> goto release_firmware; >> } >> >> @@ -960,6 +973,8 @@ static int q6v5_mpss_load(struct q6v5 *qproc) >> >> release_firmware: >> release_firmware(fw); >> +out: >> + kfree(fw_name); >> >> return ret < 0 ? ret : 0; >> } >> @@ -1075,9 +1090,10 @@ static int >> qcom_q6v5_register_dump_segments(struct rproc *rproc, >> unsigned long i; >> int ret; >> >> - ret = request_firmware(&fw, "modem.mdt", qproc->dev); >> + ret = request_firmware(&fw, qproc->hexagon_mdt_image, qproc->dev); >> if (ret < 0) { >> - dev_err(qproc->dev, "unable to load modem.mdt\n"); >> + dev_err(qproc->dev, "unable to load %s\n", >> + qproc->hexagon_mdt_image); >> return ret; >> } >> >> @@ -1253,6 +1269,8 @@ static int q6v5_probe(struct platform_device >> *pdev) >> const struct rproc_hexagon_res *desc; >> struct q6v5 *qproc; >> struct rproc *rproc; >> + const char *mba_image; >> + const char *fw_name[2]; >> int ret; >> >> desc = of_device_get_match_data(&pdev->dev); >> @@ -1262,8 +1280,15 @@ static int q6v5_probe(struct platform_device >> *pdev) >> if (desc->need_mem_protection && !qcom_scm_is_available()) >> return -EPROBE_DEFER; >> >> + ret = of_property_read_string_array(pdev->dev.of_node, >> "firmware-name", >> + fw_name, 2); >> + if (ret != -EINVAL && ret != 2) >> + return ret > 0 ? -EINVAL : ret; >> + >> + mba_image = (ret != 2) ? desc->hexagon_mba_image : fw_name[0]; >> + >> rproc = rproc_alloc(&pdev->dev, pdev->name, &q6v5_ops, >> - desc->hexagon_mba_image, sizeof(*qproc)); >> + mba_image, sizeof(*qproc)); >> if (!rproc) { >> dev_err(&pdev->dev, "failed to allocate rproc\n"); >> return -ENOMEM; >> @@ -1272,6 +1297,7 @@ static int q6v5_probe(struct platform_device >> *pdev) >> qproc = (struct q6v5 *)rproc->priv; >> qproc->dev = &pdev->dev; >> qproc->rproc = rproc; >> + qproc->hexagon_mdt_image = (ret != 2) ? "modem.mdt" : fw_name[1]; >> platform_set_drvdata(pdev, qproc); >> >> ret = q6v5_init_mem(qproc, pdev); >> diff --git a/drivers/remoteproc/qcom_q6v5_pas.c >> b/drivers/remoteproc/qcom_q6v5_pas.c >> index b1e63fcd5fdf..141c7da29e9a 100644 >> --- a/drivers/remoteproc/qcom_q6v5_pas.c >> +++ b/drivers/remoteproc/qcom_q6v5_pas.c >> @@ -258,6 +258,8 @@ static int adsp_probe(struct platform_device >> *pdev) >> const struct adsp_data *desc; >> struct qcom_adsp *adsp; >> struct rproc *rproc; >> + const char *fw_name; >> + const char *of_fw_name; >> int ret; >> >> desc = of_device_get_match_data(&pdev->dev); >> @@ -267,8 +269,15 @@ static int adsp_probe(struct platform_device >> *pdev) >> if (!qcom_scm_is_available()) >> return -EPROBE_DEFER; >> >> + ret = of_property_read_string(pdev->dev.of_node, "firmware-name", >> + &of_fw_name); >> + if (ret && ret != -EINVAL) >> + return ret; >> + >> + fw_name = ret ? desc->firmware_name : of_fw_name; >> + >> rproc = rproc_alloc(&pdev->dev, pdev->name, &adsp_ops, >> - desc->firmware_name, sizeof(*adsp)); >> + fw_name, sizeof(*adsp)); >> if (!rproc) { >> dev_err(&pdev->dev, "unable to allocate remoteproc\n"); >> return -ENOMEM; >> -- >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora >> Forum, >> a Linux Foundation Collaborative Project >> -- -- Sibi Sankar -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-01-10 14:56 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-28 4:48 [PATCH v2 0/2] Add firmware bindings for Q6V5 MSS/PAS Sibi Sankar 2018-12-28 4:48 ` [PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add firmware bindings for Q6V5 Sibi Sankar 2018-12-28 22:17 ` Rob Herring 2019-01-03 23:30 ` Brian Norris 2019-01-03 23:50 ` Brian Norris 2019-01-04 0:01 ` Bjorn Andersson 2019-01-04 0:11 ` Brian Norris 2019-01-05 1:54 ` Brian Norris 2019-01-08 10:50 ` Sibi Sankar 2019-01-08 15:22 ` Rob Herring 2019-01-09 21:55 ` Brian Norris 2019-01-10 14:56 ` Rob Herring 2018-12-28 4:48 ` [PATCH v2 2/2] remoteproc: qcom: Add support for parsing fw dt bindings Sibi Sankar 2019-01-03 23:09 ` Bjorn Andersson 2019-01-03 23:44 ` Brian Norris 2019-01-08 10:32 ` Sibi Sankar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).