[PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Bjorn Andersson]

Update the firmware-name of the remoteproc nodes to mimic the firmware
structure on other 845 devices.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
index 09ad37b0dd71..fa7f4373a668 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
@@ -50,6 +50,7 @@ vreg_s4a_1p8: pm8998-smps4 {
 
 &adsp_pas {
 	status = "okay";
+	firmware-name = "qcom/sdm845/adsp.mdt";
 };
 
 &apps_rsc {
@@ -350,6 +351,7 @@ vreg_s3c_0p6: smps3 {
 
 &cdsp_pas {
 	status = "okay";
+	firmware-name = "qcom/sdm845/cdsp.mdt";
 };
 
 &gcc {
@@ -372,6 +374,11 @@ &i2c10 {
 	clock-frequency = <400000>;
 };
 
+&mss_pil {
+	status = "okay";
+	firmware-name = "qcom/sdm845/mba.mbn", "qcom/sdm845/modem.mbn";
+};
+
 &qupv3_id_1 {
 	status = "okay";
 };
-- 
2.24.0

Re: [PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Sibi Sankar]

On 2020-03-02 07:37, Bjorn Andersson wrote:
> Update the firmware-name of the remoteproc nodes to mimic the firmware
> structure on other 845 devices.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 09ad37b0dd71..fa7f4373a668 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -50,6 +50,7 @@ vreg_s4a_1p8: pm8998-smps4 {
> 
>  &adsp_pas {
>  	status = "okay";
> +	firmware-name = "qcom/sdm845/adsp.mdt";
>  };
> 
>  &apps_rsc {
> @@ -350,6 +351,7 @@ vreg_s3c_0p6: smps3 {
> 
>  &cdsp_pas {
>  	status = "okay";
> +	firmware-name = "qcom/sdm845/cdsp.mdt";
>  };
> 
>  &gcc {
> @@ -372,6 +374,11 @@ &i2c10 {
>  	clock-frequency = <400000>;
>  };
> 
> +&mss_pil {
> +	status = "okay";

status okay isn't really needed...

Reviewed-by: Sibi Sankar <sibis@codeaurora.org>

> +	firmware-name = "qcom/sdm845/mba.mbn", "qcom/sdm845/modem.mbn";
> +};
> +
>  &qupv3_id_1 {
>  	status = "okay";
>  };

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Arnd Bergmann]

On Mon, Mar 2, 2020 at 3:09 AM Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> Update the firmware-name of the remoteproc nodes to mimic the firmware
> structure on other 845 devices.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
>  1 file changed, 7 insertions(+)

Hi Bjorn,

Sorry for the late reply, I only came across this one while going
through the pull requests
that we had failed to pick up earlier.

I really dislike the idea of hardcoding a firmware name in the
devicetree, we had long
discussions about this a few years ago and basically concluded that the firmware
name needs to be generated by the driver after identifying the hardware itself.

The problem is that the firmware generally needs to match both the device driver
and the hardware, so when there is a firmware update that changes the behavior
(intentionally or not) in a way the driver needs to know about, then
the driver should
be able to request a particular firmware file based on information
that the owner
of the dtb may not have.

I'm holding off on the pull request for today, maybe there is something we can
still do about it before the merge window.

        Arnd

Re: [PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Jeffrey Hugo]

On Wed, Mar 25, 2020 at 3:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Mar 2, 2020 at 3:09 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > Update the firmware-name of the remoteproc nodes to mimic the firmware
> > structure on other 845 devices.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
> >  1 file changed, 7 insertions(+)
>
> Hi Bjorn,
>
> Sorry for the late reply, I only came across this one while going
> through the pull requests
> that we had failed to pick up earlier.
>
> I really dislike the idea of hardcoding a firmware name in the
> devicetree, we had long
> discussions about this a few years ago and basically concluded that the firmware
> name needs to be generated by the driver after identifying the hardware itself.
>
> The problem is that the firmware generally needs to match both the device driver
> and the hardware, so when there is a firmware update that changes the behavior
> (intentionally or not) in a way the driver needs to know about, then
> the driver should
> be able to request a particular firmware file based on information
> that the owner
> of the dtb may not have.

Interesting, this intersects some work I plan on doing.

What level information did this discussion assume that the device
driver had?  Do you have a reference to the discussion handy?

Please correct me if I am wrong, but this seems to assume that for
device X, there is one firmware at a specific version that the driver
is then knowledgeable about, and the driver can query the device
hardware in some way to determine what is appropriate.  It seems like
this assumption is believed to hold true, no matter what system X is
included in.

I think we have the problem where likely impossible that the driver
will know what firmware is valid.

Qualcomm, for better or worse, has a signing process for their images.
This establishes a root a trust which is enforced by hardware.  For
example, the Modem subsystem (the part of the SoC that talks to cell
towers and such) will not run an image which is not properly signed.
The valid signature is burned into the chip.

"Surely there is one signed image for a particular modem on a specific SoC?"
Sadly, no.  The OEM is allowed to provide their own key.  This may be
a key which is specific to the device (Ie the Brand XYZ Model 123
phone).  Therefore, that device will only run the firmware that
contains that OEM's signature, even if the actual code happens to be
identical to what every other OEM has.

For some SoCs which go into multiple products, there seem to be
several OEMs which are willing to allow the firmware to be included in
the linux-firmware project.  Therefore, it is likely that there will
be multiple copies of the Modem image for the 845 SoC (for example) in
/lib/firmware.  In this case, it seems like your recommendation is
that the driver should somehow detect that it is running on device 123
and not device 456, and therefore be able to request the device 123
specific firmware.

I don't know how the device driver is supposed to make that
determination, and its my opinion that the driver shouldn't be.  Other
than the need to have the correct firmware, which is tied to the
specific device, I'm not aware of an instance where a driver cares
about anything more than the hardware revision of the block it drives.

Re: [PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Bjorn Andersson]

On Wed 25 Mar 14:54 PDT 2020, Jeffrey Hugo wrote:

> On Wed, Mar 25, 2020 at 3:13 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Mon, Mar 2, 2020 at 3:09 AM Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > Update the firmware-name of the remoteproc nodes to mimic the firmware
> > > structure on other 845 devices.
> > >
> > > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> >
> > Hi Bjorn,
> >
> > Sorry for the late reply, I only came across this one while going
> > through the pull requests
> > that we had failed to pick up earlier.
> >
> > I really dislike the idea of hardcoding a firmware name in the
> > devicetree, we had long
> > discussions about this a few years ago and basically concluded that the firmware
> > name needs to be generated by the driver after identifying the hardware itself.
> >
> > The problem is that the firmware generally needs to match both the device driver
> > and the hardware, so when there is a firmware update that changes the behavior
> > (intentionally or not) in a way the driver needs to know about, then
> > the driver should
> > be able to request a particular firmware file based on information
> > that the owner
> > of the dtb may not have.
> 
> Interesting, this intersects some work I plan on doing.
> 
> What level information did this discussion assume that the device
> driver had?  Do you have a reference to the discussion handy?
> 
> Please correct me if I am wrong, but this seems to assume that for
> device X, there is one firmware at a specific version that the driver
> is then knowledgeable about, and the driver can query the device
> hardware in some way to determine what is appropriate.  It seems like
> this assumption is believed to hold true, no matter what system X is
> included in.
> 
> I think we have the problem where likely impossible that the driver
> will know what firmware is valid.
> 
> Qualcomm, for better or worse, has a signing process for their images.
> This establishes a root a trust which is enforced by hardware.  For
> example, the Modem subsystem (the part of the SoC that talks to cell
> towers and such) will not run an image which is not properly signed.
> The valid signature is burned into the chip.
> 
> "Surely there is one signed image for a particular modem on a specific SoC?"
> Sadly, no.  The OEM is allowed to provide their own key.  This may be
> a key which is specific to the device (Ie the Brand XYZ Model 123
> phone).  Therefore, that device will only run the firmware that
> contains that OEM's signature, even if the actual code happens to be
> identical to what every other OEM has.
> 

And generally your XYZ 123 might come in different SKUs that might or
might not vary in software and hardware features; so for some products
the driver should know that it can use the "generic" XYZ 123 firmware
and in others it needs to know that it should be looking for the XYZ 123
firmware for, say, the Japanese market (different hardware).

> For some SoCs which go into multiple products, there seem to be
> several OEMs which are willing to allow the firmware to be included in
> the linux-firmware project.  Therefore, it is likely that there will
> be multiple copies of the Modem image for the 845 SoC (for example) in
> /lib/firmware.  In this case, it seems like your recommendation is
> that the driver should somehow detect that it is running on device 123
> and not device 456, and therefore be able to request the device 123
> specific firmware.
> 

And in the past I've worked on products where product 123, 456 and 789
had the same firmware, but on some particular market all three used a
market-specific firmware and in some cases two of them existed in a WiFi
only variant.

> I don't know how the device driver is supposed to make that
> determination, and its my opinion that the driver shouldn't be.  Other
> than the need to have the correct firmware, which is tied to the
> specific device, I'm not aware of an instance where a driver cares
> about anything more than the hardware revision of the block it drives.

Looking at the particular problem it's not the revision of the hardware
block(s) that the remoteproc interacts with that determines any of this.

E.g. the modem subsystem is the same on Dragonboard845c with WiFi-only
as it is on the Lenovo Yoga C630 with or without LTE - but we still need
some mechanism to determine which of the 3 firmware files to pick.

Regards,
Bjorn

Re: [PATCH] arm64: dts: qcom: sdm845-mtp: Relocate remoteproc firmware

[Bjorn Andersson]

On Wed 25 Mar 14:13 PDT 2020, Arnd Bergmann wrote:

> On Mon, Mar 2, 2020 at 3:09 AM Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > Update the firmware-name of the remoteproc nodes to mimic the firmware
> > structure on other 845 devices.
> >
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 7 +++++++
> >  1 file changed, 7 insertions(+)
> 
> Hi Bjorn,
> 
> Sorry for the late reply, I only came across this one while going
> through the pull requests
> that we had failed to pick up earlier.
> 
> I really dislike the idea of hardcoding a firmware name in the
> devicetree, we had long
> discussions about this a few years ago and basically concluded that the firmware
> name needs to be generated by the driver after identifying the hardware itself.
> 

I remember this discussion and generally I share your view, but after
postponing this problem for years we've not managed to come up with a
solution for our problem.

> The problem is that the firmware generally needs to match both the device driver
> and the hardware, so when there is a firmware update that changes the behavior
> (intentionally or not) in a way the driver needs to know about, then
> the driver should
> be able to request a particular firmware file based on information
> that the owner
> of the dtb may not have.
> 

There are three variables in play here:

1) Large feature differences, e.g. does your modem Hexagon have
associated RF hardware, or is it WiFi only. Or other similar things,
which does affect DeviceTree anyways (memory maps, audio routing etc)

2) Purely software versions of the firmware. Generally no impact on
remoteproc level or the immediate layers above, bug fixes etc.

3) Vendor specific signatures. All these files are signed with vendor
specific private keys.


None of these affects how we describe the hardware, so we did choose to
use a compatible per platform and remoteproc, e.g. qcom,sdm845-mss-pil
will handle the modem core on all SDM845 devices, regardless of the
firmware implementing WiFi only or it's a devboard or a product with
strict signature validation.

We could add another property in the DT node to denote if the modem RF
hardware is present and have the sdm845-mss-pil compatible result in a
selection of qcom/sdm845/modem.mbn vs qcom/sdm845/modem_nm.mbn. This
would handle 1) above.


But this doesn't solve 3) and my Lenovo Yoga C630 will refuse to load
these files, as they are not signed by Lenovo.

For years we've toyed with the idea of building the necessary firmware
path based on e.g. information from DMI (which not all boards has) or
somehow tokenizing the machine compatible. But nothing sane has come out
of these attempts/ideas.

So after years of not being able to send these files to linux-firmware,
without breaking some other board we decided to just describe these
variations using firmware-name.

So this solves 1) and 3) in a straight forward way, and so far in all
cases we've handled 2) by upgrading (until now, our fork of)
linux-firmware.

But I don't have any suggestions for how to solve the case where kernel
version X and X+1 _needs_ different versions of the firmware.


Lastly, most variations in firmware features are discoverable by the
higher layers, but for the cases where the remoteproc driver itself is
affected we're looking at changes to the memory map, clocks, regulators,
power domains - problems that has to be resolved in DT anyways. 

Which is the reason why several companies are looking at passing
dynamically loaded DT snippets with their remoteproc firmware.

> I'm holding off on the pull request for today, maybe there is something we can
> still do about it before the merge window.
> 

The binding addition was merged in 5.1, with Rob's r-b, in 5.5 we used
these properties for the Lenovo Yoga C630 and in 5.6 we merged the
equivalent change for the Dragonboard 845c.

If there is a solution that allow us to move away from firmware-name in
DT I'm interested and would like to see us migrate towards it, but the
only thing this particular change does is to make the SDM845 MTP find
the right files in linux-firmware, using the already existing binding
and implementation.

Regards,
Bjorn