2021-09-28 07:53:34

by Arnd Bergmann

[permalink] [raw]
Subject: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

From: Arnd Bergmann <[email protected]>

Compile-testing drivers that require access to a firmware layer
fails when that firmware symbol is unavailable. This happened
twice this week:

- My proposed to change to rework the QCOM_SCM firmware symbol
broke on ppc64 and others.

- The cs_dsp firmware patch added device specific firmware loader
into drivers/firmware, which broke on the same set of
architectures.

We should probably do the same thing for other subsystems as well,
but fix this one first as this is a dependency for other patches
getting merged.

Cc: Mark Brown <[email protected]>
Cc: Liam Girdwood <[email protected]>
Cc: Charles Keepax <[email protected]>
Cc: Simon Trimmer <[email protected]>
Cc: Arnd Bergmann <[email protected]>
Cc: Michael Ellerman <[email protected]>
Signed-off-by: Arnd Bergmann <[email protected]>
---
Not sure how we'd want to merge this patch, if two other things
need it. I'd prefer to merge it along with the QCOM_SCM change
through the soc tree, but that leaves the cirrus firmware broken
unless we also merge it the same way (rather than through ASoC
as it is now).

Alternatively, we can try to find a different home for the Cirrus
firmware to decouple the two problems. I'd argue that it's actually
misplaced here, as drivers/firmware is meant for kernel code that
interfaces with system firmware, not for device drivers to load
their own firmware blobs from user space.
---
arch/arm/Kconfig | 2 --
arch/arm64/Kconfig | 2 --
arch/ia64/Kconfig | 2 --
arch/mips/Kconfig | 2 --
arch/parisc/Kconfig | 2 --
arch/riscv/Kconfig | 2 --
arch/x86/Kconfig | 2 --
drivers/Kconfig | 2 ++
8 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index ad96f3dd7e83..194d10bbff9e 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1993,8 +1993,6 @@ config ARCH_HIBERNATION_POSSIBLE

endmenu

-source "drivers/firmware/Kconfig"
-
if CRYPTO
source "arch/arm/crypto/Kconfig"
endif
diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index ebb49585a63f..8749517482ae 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -1931,8 +1931,6 @@ source "drivers/cpufreq/Kconfig"

endmenu

-source "drivers/firmware/Kconfig"
-
source "drivers/acpi/Kconfig"

source "arch/arm64/kvm/Kconfig"
diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
index 045792cde481..1e33666fa679 100644
--- a/arch/ia64/Kconfig
+++ b/arch/ia64/Kconfig
@@ -388,8 +388,6 @@ config CRASH_DUMP
help
Generate crash dump after being started by kexec.

-source "drivers/firmware/Kconfig"
-
endmenu

menu "Power management and ACPI options"
diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
index 771ca53af06d..6b8f591c5054 100644
--- a/arch/mips/Kconfig
+++ b/arch/mips/Kconfig
@@ -3316,8 +3316,6 @@ source "drivers/cpuidle/Kconfig"

endmenu

-source "drivers/firmware/Kconfig"
-
source "arch/mips/kvm/Kconfig"

source "arch/mips/vdso/Kconfig"
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index 4742b6f169b7..27a8b49af11f 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -384,6 +384,4 @@ config KEXEC_FILE

endmenu

-source "drivers/firmware/Kconfig"
-
source "drivers/parisc/Kconfig"
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 301a54233c7e..6a6fa9e976d5 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -561,5 +561,3 @@ menu "Power management options"
source "kernel/power/Kconfig"

endmenu
-
-source "drivers/firmware/Kconfig"
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e5ba8afd29a0..5dcec5f13a82 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2834,8 +2834,6 @@ config HAVE_ATOMIC_IOMAP
def_bool y
depends on X86_32

-source "drivers/firmware/Kconfig"
-
source "arch/x86/kvm/Kconfig"

source "arch/x86/Kconfig.assembler"
diff --git a/drivers/Kconfig b/drivers/Kconfig
index 30d2db37cc87..0d399ddaa185 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -17,6 +17,8 @@ source "drivers/bus/Kconfig"

source "drivers/connector/Kconfig"

+source "drivers/firmware/Kconfig"
+
source "drivers/gnss/Kconfig"

source "drivers/mtd/Kconfig"
--
2.29.2


2021-09-28 08:40:14

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
> Compile-testing drivers that require access to a firmware layer
> fails when that firmware symbol is unavailable. This happened
> twice this week:
>
> - My proposed to change to rework the QCOM_SCM firmware symbol
> broke on ppc64 and others.
>
> - The cs_dsp firmware patch added device specific firmware loader
> into drivers/firmware, which broke on the same set of
> architectures.
>
> We should probably do the same thing for other subsystems as well,
> but fix this one first as this is a dependency for other patches
> getting merged.
>
> Not sure how we'd want to merge this patch, if two other things
> need it. I'd prefer to merge it along with the QCOM_SCM change
> through the soc tree, but that leaves the cirrus firmware broken
> unless we also merge it the same way (rather than through ASoC
> as it is now).
>
> Alternatively, we can try to find a different home for the Cirrus
> firmware to decouple the two problems. I'd argue that it's actually
> misplaced here, as drivers/firmware is meant for kernel code that
> interfaces with system firmware, not for device drivers to load
> their own firmware blobs from user space.

Thanks for looking at this for us. I don't think we are greatly
attached to drivers/firmware as a location. Essentially, what we
have is some firmware parsing code that needs to be shared across
several devices, previously this was just in the sound subsystem as
all our parts were audio. We are going to shortly be upstreaming
some non-audio parts that use the same firmware infrastructure
and it didn't seem very sensible to have them including bits of
the audio subsystem.

I guess the question might be where else would said code go?
drivers/firmware seemed most obvious, all the other locations
I can think of don't really make sense. Can't really put it a bus
like spi/i2c etc. because we have parts on many buses. Can't
really put it in a functional subsystem (audio/input etc.) since
the whole idea was to try and get some independence from that so
we don't have parts including subsystems they don't use. Could
maybe put it in MFD, but no hard guarantee every part using it
will be an MFD device and I am fairly confident Lee will feel it
isn't MFD code as it doesn't relate to managing multiple devices.
Only other option I can think of would be to make some sort of
drivers/dsp or maybe drivers/cs_dsp, but not clear to me that is
obviously better than using drivers/firmware.

Thanks,
Charles

2021-09-28 08:53:11

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 10:37 AM Charles Keepax
<[email protected]> wrote:
>
> Thanks for looking at this for us. I don't think we are greatly
> attached to drivers/firmware as a location. Essentially, what we
> have is some firmware parsing code that needs to be shared across
> several devices, previously this was just in the sound subsystem as
> all our parts were audio. We are going to shortly be upstreaming
> some non-audio parts that use the same firmware infrastructure
> and it didn't seem very sensible to have them including bits of
> the audio subsystem.
>
> I guess the question might be where else would said code go?
> drivers/firmware seemed most obvious, all the other locations
> I can think of don't really make sense. Can't really put it a bus
> like spi/i2c etc. because we have parts on many buses. Can't
> really put it in a functional subsystem (audio/input etc.) since
> the whole idea was to try and get some independence from that so
> we don't have parts including subsystems they don't use. Could
> maybe put it in MFD, but no hard guarantee every part using it
> will be an MFD device and I am fairly confident Lee will feel it
> isn't MFD code as it doesn't relate to managing multiple devices.
> Only other option I can think of would be to make some sort of
> drivers/dsp or maybe drivers/cs_dsp, but not clear to me that is
> obviously better than using drivers/firmware.

Other DSPs use the drivers/remoteproc/ subsystem, but that
is more for general-purpose DSPs that can load application
specific firmware rather than loading a single firmware blob
as you'd normally do with the request_firmware() style interface.

Not sure if that fits what you do. Can you point to a high-level
description of what this DSP does besides audio, and how
flexible it is? That might help find the right place for this.

Arnd

2021-09-28 09:27:25

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 10:51:36AM +0200, Arnd Bergmann wrote:
> On Tue, Sep 28, 2021 at 10:37 AM Charles Keepax
> <[email protected]> wrote:
> > I guess the question might be where else would said code go?
> > drivers/firmware seemed most obvious, all the other locations
> > I can think of don't really make sense. Can't really put it a bus
> > like spi/i2c etc. because we have parts on many buses. Can't
> > really put it in a functional subsystem (audio/input etc.) since
> > the whole idea was to try and get some independence from that so
> > we don't have parts including subsystems they don't use. Could
> > maybe put it in MFD, but no hard guarantee every part using it
> > will be an MFD device and I am fairly confident Lee will feel it
> > isn't MFD code as it doesn't relate to managing multiple devices.
> > Only other option I can think of would be to make some sort of
> > drivers/dsp or maybe drivers/cs_dsp, but not clear to me that is
> > obviously better than using drivers/firmware.
>
> Other DSPs use the drivers/remoteproc/ subsystem, but that
> is more for general-purpose DSPs that can load application
> specific firmware rather than loading a single firmware blob
> as you'd normally do with the request_firmware() style interface.
>
> Not sure if that fits what you do. Can you point to a high-level
> description of what this DSP does besides audio, and how
> flexible it is? That might help find the right place for this.

Hm... wasn't aware of that one, we should probably investigate that
a little more at this end. From a quick look, seems a bit more like
it is designed for much larger more general purpose probably memory
mapped DSPs. I guess our code is a little more firmware parsing
and loading, and a bit less generic remote proceedure call stuff.

I am not sure there is great deal available publically on the
DSP core. It is talked about in a few of our datasheets, see
section 4.4 in [1]. But a basic description might be it is a
signal processing focused, very small DSP core. If can be loaded
with different firmwares at runtime, and indeed might be doing say
echo cancellation in one use-case, or always on voice detect in
another. Functionally it is very unlikely to be used for anything
besides signal processing inside the device it is in, since it is
typically quite integrated with that hardware and will be sitting
behind a slow bus, like I2C or SPI.

Current users are all audio, planning to upstream some haptics
parts soon, with possible other uses in the future.

[1] https://statics.cirrus.com/pubs/proDatasheet/CS48L32_DS1219F4.pdf

Thanks,
Charles

2021-09-28 11:29:57

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 09:24:00AM +0000, Charles Keepax wrote:
> On Tue, Sep 28, 2021 at 10:51:36AM +0200, Arnd Bergmann wrote:

> > Other DSPs use the drivers/remoteproc/ subsystem, but that
> > is more for general-purpose DSPs that can load application
> > specific firmware rather than loading a single firmware blob
> > as you'd normally do with the request_firmware() style interface.

> > Not sure if that fits what you do. Can you point to a high-level
> > description of what this DSP does besides audio, and how
> > flexible it is? That might help find the right place for this.

> Hm... wasn't aware of that one, we should probably investigate that
> a little more at this end. From a quick look, seems a bit more like
> it is designed for much larger more general purpose probably memory
> mapped DSPs. I guess our code is a little more firmware parsing
> and loading, and a bit less generic remote proceedure call stuff.

Right, that was why I didn't suggest remoteproc - the DSPs wm_adsp
covers seem much smaller than fits comfortably with remoteproc. You
probably could make it fit though.


Attachments:
(No filename) (1.10 kB)
signature.asc (499.00 B)
Download all attachments

2021-09-28 12:04:57

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:

> Compile-testing drivers that require access to a firmware layer
> fails when that firmware symbol is unavailable. This happened
> twice this week:

...

> We should probably do the same thing for other subsystems as well,
> but fix this one first as this is a dependency for other patches
> getting merged.

Reviwed-by: Mark Brown <[email protected]>

Regardless of what we do with the Cirrus DSP this just seems like a good
idea - surprises due to this not being generally available keep coming
up, IIRC with the i.MX firmware in the past for example.

> Not sure how we'd want to merge this patch, if two other things
> need it. I'd prefer to merge it along with the QCOM_SCM change
> through the soc tree, but that leaves the cirrus firmware broken
> unless we also merge it the same way (rather than through ASoC
> as it is now).

We could also merge a tag into both places.

> Alternatively, we can try to find a different home for the Cirrus
> firmware to decouple the two problems. I'd argue that it's actually
> misplaced here, as drivers/firmware is meant for kernel code that
> interfaces with system firmware, not for device drivers to load
> their own firmware blobs from user space.

Trying to enforce distinctions here feels like it's liable to lead to
trouble at some point...


Attachments:
(No filename) (1.36 kB)
signature.asc (499.00 B)
Download all attachments

2021-09-28 12:25:29

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 1:58 PM Mark Brown <[email protected]> wrote:
> On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
>
> > Not sure how we'd want to merge this patch, if two other things
> > need it. I'd prefer to merge it along with the QCOM_SCM change
> > through the soc tree, but that leaves the cirrus firmware broken
> > unless we also merge it the same way (rather than through ASoC
> > as it is now).
>
> We could also merge a tag into both places.

I wonder if I should just take my two patches as bugfixes for 5.15,
after all they do address real build failures. In that case, all you need
is a merge with 5.15-rc4 or higher.

Arnd

2021-09-29 10:20:41

by Will Deacon

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Compile-testing drivers that require access to a firmware layer
> fails when that firmware symbol is unavailable. This happened
> twice this week:
>
> - My proposed to change to rework the QCOM_SCM firmware symbol
> broke on ppc64 and others.
>
> - The cs_dsp firmware patch added device specific firmware loader
> into drivers/firmware, which broke on the same set of
> architectures.
>
> We should probably do the same thing for other subsystems as well,
> but fix this one first as this is a dependency for other patches
> getting merged.
>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Charles Keepax <[email protected]>
> Cc: Simon Trimmer <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> Not sure how we'd want to merge this patch, if two other things
> need it. I'd prefer to merge it along with the QCOM_SCM change
> through the soc tree, but that leaves the cirrus firmware broken
> unless we also merge it the same way (rather than through ASoC
> as it is now).
>
> Alternatively, we can try to find a different home for the Cirrus
> firmware to decouple the two problems. I'd argue that it's actually
> misplaced here, as drivers/firmware is meant for kernel code that
> interfaces with system firmware, not for device drivers to load
> their own firmware blobs from user space.
> ---
> arch/arm/Kconfig | 2 --
> arch/arm64/Kconfig | 2 --
> arch/ia64/Kconfig | 2 --
> arch/mips/Kconfig | 2 --
> arch/parisc/Kconfig | 2 --
> arch/riscv/Kconfig | 2 --
> arch/x86/Kconfig | 2 --
> drivers/Kconfig | 2 ++
> 8 files changed, 2 insertions(+), 14 deletions(-)

For arm64:

Acked-by: Will Deacon <[email protected]>

Will

2021-09-29 15:57:13

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue 28 Sep 04:24 CDT 2021, Charles Keepax wrote:

> On Tue, Sep 28, 2021 at 10:51:36AM +0200, Arnd Bergmann wrote:
> > On Tue, Sep 28, 2021 at 10:37 AM Charles Keepax
> > <[email protected]> wrote:
> > > I guess the question might be where else would said code go?
> > > drivers/firmware seemed most obvious, all the other locations
> > > I can think of don't really make sense. Can't really put it a bus
> > > like spi/i2c etc. because we have parts on many buses. Can't
> > > really put it in a functional subsystem (audio/input etc.) since
> > > the whole idea was to try and get some independence from that so
> > > we don't have parts including subsystems they don't use. Could
> > > maybe put it in MFD, but no hard guarantee every part using it
> > > will be an MFD device and I am fairly confident Lee will feel it
> > > isn't MFD code as it doesn't relate to managing multiple devices.
> > > Only other option I can think of would be to make some sort of
> > > drivers/dsp or maybe drivers/cs_dsp, but not clear to me that is
> > > obviously better than using drivers/firmware.
> >
> > Other DSPs use the drivers/remoteproc/ subsystem, but that
> > is more for general-purpose DSPs that can load application
> > specific firmware rather than loading a single firmware blob
> > as you'd normally do with the request_firmware() style interface.
> >
> > Not sure if that fits what you do. Can you point to a high-level
> > description of what this DSP does besides audio, and how
> > flexible it is? That might help find the right place for this.
>
> Hm... wasn't aware of that one, we should probably investigate that
> a little more at this end. From a quick look, seems a bit more like
> it is designed for much larger more general purpose probably memory
> mapped DSPs. I guess our code is a little more firmware parsing
> and loading, and a bit less generic remote proceedure call stuff.
>

You're correct, remoteproc tends to situations where you have
multi-function firmware; be it at a single point in time, or due to
different firmware choices. Where you essentially boot some firmware on
the remoteproc and from that instantiate one of more functional devices
based on the loaded firmware.

> I am not sure there is great deal available publically on the
> DSP core. It is talked about in a few of our datasheets, see
> section 4.4 in [1]. But a basic description might be it is a
> signal processing focused, very small DSP core. If can be loaded
> with different firmwares at runtime, and indeed might be doing say
> echo cancellation in one use-case, or always on voice detect in
> another. Functionally it is very unlikely to be used for anything
> besides signal processing inside the device it is in, since it is
> typically quite integrated with that hardware and will be sitting
> behind a slow bus, like I2C or SPI.
>

To me it sounds like the main difference compared to above is that you
have a single function that owns and controls the DSP and implements
that function - i.e. the audio driver probes, boots the DSP, if there's
a problem the audio driver will handle it etc.


When it comes to firmware parsing, that might be a somewhat unrelated
topic. E.g. in the Qualcomm case, the same customized ELF header is used
in both for remoteproc devices and in function-specific devices. For
this we extracted the relevant functions into a library of some common
helpers, which can be found in drivers/soc/qcom/mdt_loader.c.

Regards,
Bjorn

> Current users are all audio, planning to upstream some haptics
> parts soon, with possible other uses in the future.
>
> [1] https://statics.cirrus.com/pubs/proDatasheet/CS48L32_DS1219F4.pdf
>
> Thanks,
> Charles

2021-09-29 17:24:23

by Bjorn Andersson

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue 28 Sep 07:22 CDT 2021, Arnd Bergmann wrote:

> On Tue, Sep 28, 2021 at 1:58 PM Mark Brown <[email protected]> wrote:
> > On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> >
> > > Not sure how we'd want to merge this patch, if two other things
> > > need it. I'd prefer to merge it along with the QCOM_SCM change
> > > through the soc tree, but that leaves the cirrus firmware broken
> > > unless we also merge it the same way (rather than through ASoC
> > > as it is now).
> >
> > We could also merge a tag into both places.
>
> I wonder if I should just take my two patches as bugfixes for 5.15,
> after all they do address real build failures. In that case, all you need
> is a merge with 5.15-rc4 or higher.
>

I'm in favor of this, better get it over with.
With QCOM_IOMMU fixed up as well, feel free to add my

Reviewed-by: Bjorn Andersson <[email protected]>

on both the patches, and for the qcom_scm changes you have my

Acked-by: Bjorn Andersson <[email protected]>

Thanks,
Bjorn

2021-10-06 08:32:08

by Charles Keepax

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <[email protected]>
>
> Compile-testing drivers that require access to a firmware layer
> fails when that firmware symbol is unavailable. This happened
> twice this week:
>
> - My proposed to change to rework the QCOM_SCM firmware symbol
> broke on ppc64 and others.
>
> - The cs_dsp firmware patch added device specific firmware loader
> into drivers/firmware, which broke on the same set of
> architectures.
>
> We should probably do the same thing for other subsystems as well,
> but fix this one first as this is a dependency for other patches
> getting merged.
>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Charles Keepax <[email protected]>
> Cc: Simon Trimmer <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---

Reviewed-by: Charles Keepax <[email protected]>

Thanks,
Charles

2021-10-09 09:26:31

by Paul Menzel

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

[Cc: [email protected]]

Dear Arnd,


Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> From: Arnd Bergmann <[email protected]>
>
> Compile-testing drivers that require access to a firmware layer
> fails when that firmware symbol is unavailable. This happened
> twice this week:
>
> - My proposed to change to rework the QCOM_SCM firmware symbol
> broke on ppc64 and others.
>
> - The cs_dsp firmware patch added device specific firmware loader
> into drivers/firmware, which broke on the same set of
> architectures.
>
> We should probably do the same thing for other subsystems as well,
> but fix this one first as this is a dependency for other patches
> getting merged.
>
> Cc: Mark Brown <[email protected]>
> Cc: Liam Girdwood <[email protected]>
> Cc: Charles Keepax <[email protected]>
> Cc: Simon Trimmer <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Signed-off-by: Arnd Bergmann <[email protected]>
> ---
> Not sure how we'd want to merge this patch, if two other things
> need it. I'd prefer to merge it along with the QCOM_SCM change
> through the soc tree, but that leaves the cirrus firmware broken
> unless we also merge it the same way (rather than through ASoC
> as it is now).
>
> Alternatively, we can try to find a different home for the Cirrus
> firmware to decouple the two problems. I'd argue that it's actually
> misplaced here, as drivers/firmware is meant for kernel code that
> interfaces with system firmware, not for device drivers to load
> their own firmware blobs from user space.
> ---
> arch/arm/Kconfig | 2 --
> arch/arm64/Kconfig | 2 --
> arch/ia64/Kconfig | 2 --
> arch/mips/Kconfig | 2 --
> arch/parisc/Kconfig | 2 --
> arch/riscv/Kconfig | 2 --
> arch/x86/Kconfig | 2 --
> drivers/Kconfig | 2 ++
> 8 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index ad96f3dd7e83..194d10bbff9e 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -1993,8 +1993,6 @@ config ARCH_HIBERNATION_POSSIBLE
>
> endmenu
>
> -source "drivers/firmware/Kconfig"
> -
> if CRYPTO
> source "arch/arm/crypto/Kconfig"
> endif
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index ebb49585a63f..8749517482ae 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1931,8 +1931,6 @@ source "drivers/cpufreq/Kconfig"
>
> endmenu
>
> -source "drivers/firmware/Kconfig"
> -
> source "drivers/acpi/Kconfig"
>
> source "arch/arm64/kvm/Kconfig"
> diff --git a/arch/ia64/Kconfig b/arch/ia64/Kconfig
> index 045792cde481..1e33666fa679 100644
> --- a/arch/ia64/Kconfig
> +++ b/arch/ia64/Kconfig
> @@ -388,8 +388,6 @@ config CRASH_DUMP
> help
> Generate crash dump after being started by kexec.
>
> -source "drivers/firmware/Kconfig"
> -
> endmenu
>
> menu "Power management and ACPI options"
> diff --git a/arch/mips/Kconfig b/arch/mips/Kconfig
> index 771ca53af06d..6b8f591c5054 100644
> --- a/arch/mips/Kconfig
> +++ b/arch/mips/Kconfig
> @@ -3316,8 +3316,6 @@ source "drivers/cpuidle/Kconfig"
>
> endmenu
>
> -source "drivers/firmware/Kconfig"
> -
> source "arch/mips/kvm/Kconfig"
>
> source "arch/mips/vdso/Kconfig"
> diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
> index 4742b6f169b7..27a8b49af11f 100644
> --- a/arch/parisc/Kconfig
> +++ b/arch/parisc/Kconfig
> @@ -384,6 +384,4 @@ config KEXEC_FILE
>
> endmenu
>
> -source "drivers/firmware/Kconfig"
> -
> source "drivers/parisc/Kconfig"
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 301a54233c7e..6a6fa9e976d5 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -561,5 +561,3 @@ menu "Power management options"
> source "kernel/power/Kconfig"
>
> endmenu
> -
> -source "drivers/firmware/Kconfig"
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index e5ba8afd29a0..5dcec5f13a82 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2834,8 +2834,6 @@ config HAVE_ATOMIC_IOMAP
> def_bool y
> depends on X86_32
>
> -source "drivers/firmware/Kconfig"
> -
> source "arch/x86/kvm/Kconfig"
>
> source "arch/x86/Kconfig.assembler"
> diff --git a/drivers/Kconfig b/drivers/Kconfig
> index 30d2db37cc87..0d399ddaa185 100644
> --- a/drivers/Kconfig
> +++ b/drivers/Kconfig
> @@ -17,6 +17,8 @@ source "drivers/bus/Kconfig"
>
> source "drivers/connector/Kconfig"
>
> +source "drivers/firmware/Kconfig"
> +
> source "drivers/gnss/Kconfig"
>
> source "drivers/mtd/Kconfig"
>

With this change, I have the new entries below in my .config:

```
$ diff -u .config.old .config
--- .config.old 2021-10-07 11:38:39.544000000 +0200
+++ .config 2021-10-09 10:02:03.156000000 +0200
@@ -1992,6 +1992,25 @@

CONFIG_CONNECTOR=y
CONFIG_PROC_EVENTS=y
+
+#
+# Firmware Drivers
+#
+
+#
+# ARM System Control and Management Interface Protocol
+#
+# end of ARM System Control and Management Interface Protocol
+
+# CONFIG_FIRMWARE_MEMMAP is not set
+# CONFIG_GOOGLE_FIRMWARE is not set
+
+#
+# Tegra firmware driver
+#
+# end of Tegra firmware driver
+# end of Firmware Drivers
+
# CONFIG_GNSS is not set
CONFIG_MTD=m
# CONFIG_MTD_TESTS is not set
```

No idea if the entries could be hidden for platforms not supporting them.

ARM System Control and Management Interface Protocol ----
[ ] Add firmware-provided memory map to sysfs
[ ] Google Firmware Drivers ----
Tegra firmware driver ----


Kind regards,

Paul

2021-10-11 12:18:15

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Sat, Oct 9, 2021 at 11:24 AM Paul Menzel <[email protected]> wrote:
> [Cc: [email protected]]
>
> Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> > From: Arnd Bergmann <[email protected]>
> >
> > Compile-testing drivers that require access to a firmware layer
> > fails when that firmware symbol is unavailable. This happened
> > twice this week:
> >
> > - My proposed to change to rework the QCOM_SCM firmware symbol
> > broke on ppc64 and others.
> >
> > - The cs_dsp firmware patch added device specific firmware loader
> > into drivers/firmware, which broke on the same set of
> > architectures.
> >
> > We should probably do the same thing for other subsystems as well,
> > but fix this one first as this is a dependency for other patches
> > getting merged.
> >

> With this change, I have the new entries below in my .config:
>
> ```
> $ diff -u .config.old .config
> --- .config.old 2021-10-07 11:38:39.544000000 +0200
> +++ .config 2021-10-09 10:02:03.156000000 +0200
> @@ -1992,6 +1992,25 @@
>
> CONFIG_CONNECTOR=y
> CONFIG_PROC_EVENTS=y
> +
> +#
> +# Firmware Drivers
> +#
> +
> +#
> +# ARM System Control and Management Interface Protocol
> +#
> +# end of ARM System Control and Management Interface Protocol
> +
> +# CONFIG_FIRMWARE_MEMMAP is not set
> +# CONFIG_GOOGLE_FIRMWARE is not set
> +
> +#
> +# Tegra firmware driver
> +#
> +# end of Tegra firmware driver
> +# end of Firmware Drivers
> +
> # CONFIG_GNSS is not set
> CONFIG_MTD=m
> # CONFIG_MTD_TESTS is not set
> ```
>
> No idea if the entries could be hidden for platforms not supporting them.
>
> ARM System Control and Management Interface Protocol ----
> [ ] Add firmware-provided memory map to sysfs
> [ ] Google Firmware Drivers ----
> Tegra firmware driver ----

GOOGLE_FIRMWARE should probably depend on something.
I highly doubt Google is running servers on e.g. h8300 and nds32.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2021-10-11 13:29:50

by Arnd Bergmann

[permalink] [raw]
Subject: Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally

On Mon, Oct 11, 2021 at 10:42 AM Geert Uytterhoeven
<[email protected]> wrote:
> On Sat, Oct 9, 2021 at 11:24 AM Paul Menzel <[email protected]> wrote:
> > Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> > > From: Arnd Bergmann <[email protected]>
> > +#
> > +# ARM System Control and Management Interface Protocol
> > +#
> > +# end of ARM System Control and Management Interface Protocol
> > +
> > +# CONFIG_FIRMWARE_MEMMAP is not set
> > +# CONFIG_GOOGLE_FIRMWARE is not set
> > +
> > +#
> > +# Tegra firmware driver
> > +#
> > +# end of Tegra firmware driver
> > +# end of Firmware Drivers
> > +
> > # CONFIG_GNSS is not set
> > CONFIG_MTD=m
> > # CONFIG_MTD_TESTS is not set
> > ```
> >
> > No idea if the entries could be hidden for platforms not supporting them.
> >
> > ARM System Control and Management Interface Protocol ----
> > [ ] Add firmware-provided memory map to sysfs
> > [ ] Google Firmware Drivers ----
> > Tegra firmware driver ----
>
> GOOGLE_FIRMWARE should probably depend on something.
> I highly doubt Google is running servers on e.g. h8300 and nds32.

GOOGLE_FIRMWARE is only the 'menuconfig' option that contains
the other options, but on architectures that have neither CONFIG_OF
nor CONFIG_ACPI, this is empty. Most architectures of course
do support or require CONFIG_OF, so it's unclear whether we should
show the options for coreboot. Since it's a software-only driver, I
would tend to keep showing it, given that coreboot can be ported
to every architecture. The DT binding [1] seems to be neither
Google nor Arm specific.

CONFIG_FIRMWARE_MEMMAP in turn can be used for
anything that has memory hotplug, and in theory additional
drivers that register with this interface.

I'd lean towards "leave as is" for both, to avoid having to
change the dependencies again whenever something else
can use these.

Arnd

[1] Documentation/devicetree/bindings/firmware/coreboot.txt