LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
@ 2021-09-28  7:50 Arnd Bergmann
  2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Arnd Bergmann @ 2021-09-28  7:50 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Arnd Bergmann, Mark Brown, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv

From: Arnd Bergmann <arnd@arndb.de>

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 <broonie@kernel.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: Simon Trimmer <simont@opensource.cirrus.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
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


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
@ 2021-09-28  7:50 ` Arnd Bergmann
  2021-09-28 13:29   ` Alex Elder
  2021-09-29 14:50   ` Bjorn Andersson
  2021-09-28  8:37 ` [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Charles Keepax
                   ` (4 subsequent siblings)
  5 siblings, 2 replies; 17+ messages in thread
From: Arnd Bergmann @ 2021-09-28  7:50 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Arnd Bergmann, Mark Brown, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv, Kalle Valo, Rob Clark,
	Sean Paul, David Airlie, Daniel Vetter, Joerg Roedel,
	Mauro Carvalho Chehab, Ulf Hansson, Alex Elder, David S. Miller,
	Jakub Kicinski, Andy Gross, Mark Rutland, Lorenzo Pieralisi,
	Sudeep Holla, linux-arm-msm, dri-devel, freedreno, iommu,
	linux-media, linux-mmc, netdev, ath10k, linux-wireless,
	linux-gpio

From: Arnd Bergmann <arnd@arndb.de>

Now that SCM can be a loadable module, we have to add another
dependency to avoid link failures when ipa or adreno-gpu are
built-in:

aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe':
ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available'

ld.lld: error: undefined symbol: qcom_scm_is_available
>>> referenced by adreno_gpu.c
>>>               gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in archive drivers/built-in.a

This can happen when CONFIG_ARCH_QCOM is disabled and we don't select
QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd
use a similar dependency here to what we have for QCOM_RPROC_COMMON,
but that causes dependency loops from other things selecting QCOM_SCM.

This appears to be an endless problem, so try something different this
time:

 - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on'
   but that is simply selected by all of its users

 - All the stubs in include/linux/qcom_scm.h can go away

 - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to
   allow compile-testing QCOM_SCM on all architectures.

 - To avoid a circular dependency chain involving RESET_CONTROLLER
   and PINCTRL_SUNXI, drop the 'select RESET_CONTROLLER' statement.
   According to my testing this still builds fine, and the QCOM
   platform selects this symbol already.

Acked-by: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Changes in v2:
  - drop the 'select RESET_CONTROLLER' line, rather than adding
    more of the same
---
 drivers/firmware/Kconfig                |  5 +-
 drivers/gpu/drm/msm/Kconfig             |  4 +-
 drivers/iommu/Kconfig                   |  2 +-
 drivers/media/platform/Kconfig          |  2 +-
 drivers/mmc/host/Kconfig                |  2 +-
 drivers/net/ipa/Kconfig                 |  1 +
 drivers/net/wireless/ath/ath10k/Kconfig |  2 +-
 drivers/pinctrl/qcom/Kconfig            |  3 +-
 include/linux/arm-smccc.h               | 10 ++++
 include/linux/qcom_scm.h                | 71 -------------------------
 10 files changed, 20 insertions(+), 82 deletions(-)

diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 220a58cf0a44..cda7d7162cbb 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -203,10 +203,7 @@ config INTEL_STRATIX10_RSU
 	  Say Y here if you want Intel RSU support.
 
 config QCOM_SCM
-	tristate "Qcom SCM driver"
-	depends on ARM || ARM64
-	depends on HAVE_ARM_SMCCC
-	select RESET_CONTROLLER
+	tristate
 
 config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
 	bool "Qualcomm download mode enabled by default"
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index e9c6af78b1d7..3ddf739a6f9b 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -17,7 +17,7 @@ config DRM_MSM
 	select DRM_SCHED
 	select SHMEM
 	select TMPFS
-	select QCOM_SCM if ARCH_QCOM
+	select QCOM_SCM
 	select WANT_DEV_COREDUMP
 	select SND_SOC_HDMI_CODEC if SND_SOC
 	select SYNC_FILE
@@ -55,7 +55,7 @@ config DRM_MSM_GPU_SUDO
 
 config DRM_MSM_HDMI_HDCP
 	bool "Enable HDMI HDCP support in MSM DRM driver"
-	depends on DRM_MSM && QCOM_SCM
+	depends on DRM_MSM
 	default y
 	help
 	  Choose this option to enable HDCP state machine
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 124c41adeca1..989c83acbfee 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -308,7 +308,7 @@ config APPLE_DART
 config ARM_SMMU
 	tristate "ARM Ltd. System MMU (SMMU) Support"
 	depends on ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)
-	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
+	select QCOM_SCM
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_LPAE
 	select ARM_DMA_USE_IOMMU if ARM
diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
index 157c924686e4..80321e03809a 100644
--- a/drivers/media/platform/Kconfig
+++ b/drivers/media/platform/Kconfig
@@ -565,7 +565,7 @@ config VIDEO_QCOM_VENUS
 	depends on VIDEO_DEV && VIDEO_V4L2 && QCOM_SMEM
 	depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
 	select QCOM_MDT_LOADER if ARCH_QCOM
-	select QCOM_SCM if ARCH_QCOM
+	select QCOM_SCM
 	select VIDEOBUF2_DMA_CONTIG
 	select V4L2_MEM2MEM_DEV
 	help
diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 71313961cc54..95b3511b0560 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -547,7 +547,7 @@ config MMC_SDHCI_MSM
 	depends on MMC_SDHCI_PLTFM
 	select MMC_SDHCI_IO_ACCESSORS
 	select MMC_CQHCI
-	select QCOM_SCM if MMC_CRYPTO && ARCH_QCOM
+	select QCOM_SCM if MMC_CRYPTO
 	help
 	  This selects the Secure Digital Host Controller Interface (SDHCI)
 	  support present in Qualcomm SOCs. The controller supports
diff --git a/drivers/net/ipa/Kconfig b/drivers/net/ipa/Kconfig
index 8f99cfa14680..d037682fb7ad 100644
--- a/drivers/net/ipa/Kconfig
+++ b/drivers/net/ipa/Kconfig
@@ -4,6 +4,7 @@ config QCOM_IPA
 	depends on ARCH_QCOM || COMPILE_TEST
 	depends on QCOM_RPROC_COMMON || (QCOM_RPROC_COMMON=n && COMPILE_TEST)
 	select QCOM_MDT_LOADER if ARCH_QCOM
+	select QCOM_SCM
 	select QCOM_QMI_HELPERS
 	help
 	  Choose Y or M here to include support for the Qualcomm
diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
index 741289e385d5..ca007b800f75 100644
--- a/drivers/net/wireless/ath/ath10k/Kconfig
+++ b/drivers/net/wireless/ath/ath10k/Kconfig
@@ -44,7 +44,7 @@ config ATH10K_SNOC
 	tristate "Qualcomm ath10k SNOC support"
 	depends on ATH10K
 	depends on ARCH_QCOM || COMPILE_TEST
-	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
+	select QCOM_SCM
 	select QCOM_QMI_HELPERS
 	help
 	  This module adds support for integrated WCN3990 chip connected
diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
index 32ea2a8ec02b..5ff4207df66e 100644
--- a/drivers/pinctrl/qcom/Kconfig
+++ b/drivers/pinctrl/qcom/Kconfig
@@ -3,7 +3,8 @@ if (ARCH_QCOM || COMPILE_TEST)
 
 config PINCTRL_MSM
 	tristate "Qualcomm core pin controller driver"
-	depends on GPIOLIB && (QCOM_SCM || !QCOM_SCM) #if QCOM_SCM=m this can't be =y
+	depends on GPIOLIB
+	select QCOM_SCM
 	select PINMUX
 	select PINCONF
 	select GENERIC_PINCONF
diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
index 7d1cabe15262..63ccb5252190 100644
--- a/include/linux/arm-smccc.h
+++ b/include/linux/arm-smccc.h
@@ -321,10 +321,20 @@ asmlinkage unsigned long __arm_smccc_sve_check(unsigned long x0);
  * from register 0 to 3 on return from the SMC instruction.  An optional
  * quirk structure provides vendor specific behavior.
  */
+#ifdef CONFIG_HAVE_ARM_SMCCC
 asmlinkage void __arm_smccc_smc(unsigned long a0, unsigned long a1,
 			unsigned long a2, unsigned long a3, unsigned long a4,
 			unsigned long a5, unsigned long a6, unsigned long a7,
 			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
+#else
+static inline void __arm_smccc_smc(unsigned long a0, unsigned long a1,
+			unsigned long a2, unsigned long a3, unsigned long a4,
+			unsigned long a5, unsigned long a6, unsigned long a7,
+			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk)
+{
+	*res = (struct arm_smccc_res){};
+}
+#endif
 
 /**
  * __arm_smccc_hvc() - make HVC calls
diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
index c0475d1c9885..81cad9e1e412 100644
--- a/include/linux/qcom_scm.h
+++ b/include/linux/qcom_scm.h
@@ -61,7 +61,6 @@ enum qcom_scm_ice_cipher {
 #define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
 #define QCOM_SCM_PERM_RWX (QCOM_SCM_PERM_RW | QCOM_SCM_PERM_EXEC)
 
-#if IS_ENABLED(CONFIG_QCOM_SCM)
 extern bool qcom_scm_is_available(void);
 
 extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
@@ -115,74 +114,4 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
 extern int qcom_scm_lmh_profile_change(u32 profile_id);
 extern bool qcom_scm_lmh_dcvsh_available(void);
 
-#else
-
-#include <linux/errno.h>
-
-static inline bool qcom_scm_is_available(void) { return false; }
-
-static inline int qcom_scm_set_cold_boot_addr(void *entry,
-		const cpumask_t *cpus) { return -ENODEV; }
-static inline int qcom_scm_set_warm_boot_addr(void *entry,
-		const cpumask_t *cpus) { return -ENODEV; }
-static inline void qcom_scm_cpu_power_down(u32 flags) {}
-static inline u32 qcom_scm_set_remote_state(u32 state,u32 id)
-		{ return -ENODEV; }
-
-static inline int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
-		size_t size) { return -ENODEV; }
-static inline int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
-		phys_addr_t size) { return -ENODEV; }
-static inline int qcom_scm_pas_auth_and_reset(u32 peripheral)
-		{ return -ENODEV; }
-static inline int qcom_scm_pas_shutdown(u32 peripheral) { return -ENODEV; }
-static inline bool qcom_scm_pas_supported(u32 peripheral) { return false; }
-
-static inline int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
-		{ return -ENODEV; }
-static inline int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
-		{ return -ENODEV; }
-
-static inline bool qcom_scm_restore_sec_cfg_available(void) { return false; }
-static inline int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
-		{ return -ENODEV; }
-static inline int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
-		{ return -ENODEV; }
-static inline int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
-		{ return -ENODEV; }
-extern inline int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
-						 u32 cp_nonpixel_start,
-						 u32 cp_nonpixel_size)
-		{ return -ENODEV; }
-static inline int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
-		unsigned int *src, const struct qcom_scm_vmperm *newvm,
-		unsigned int dest_cnt) { return -ENODEV; }
-
-static inline bool qcom_scm_ocmem_lock_available(void) { return false; }
-static inline int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
-		u32 size, u32 mode) { return -ENODEV; }
-static inline int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id,
-		u32 offset, u32 size) { return -ENODEV; }
-
-static inline bool qcom_scm_ice_available(void) { return false; }
-static inline int qcom_scm_ice_invalidate_key(u32 index) { return -ENODEV; }
-static inline int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
-				       enum qcom_scm_ice_cipher cipher,
-				       u32 data_unit_size) { return -ENODEV; }
-
-static inline bool qcom_scm_hdcp_available(void) { return false; }
-static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
-		u32 *resp) { return -ENODEV; }
-
-static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
-		{ return -ENODEV; }
-
-static inline int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
-				     u64 limit_node, u32 node_id, u64 version)
-		{ return -ENODEV; }
-
-static inline int qcom_scm_lmh_profile_change(u32 profile_id) { return -ENODEV; }
-
-static inline bool qcom_scm_lmh_dcvsh_available(void) { return -ENODEV; }
-#endif
 #endif
-- 
2.29.2


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
  2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
@ 2021-09-28  8:37 ` Charles Keepax
  2021-09-28  8:51   ` Arnd Bergmann
  2021-09-28 11:58 ` Mark Brown
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Charles Keepax @ 2021-09-28  8:37 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bjorn Andersson, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  8:37 ` [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Charles Keepax
@ 2021-09-28  8:51   ` Arnd Bergmann
  2021-09-28  9:24     ` Charles Keepax
  0 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2021-09-28  8:51 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Bjorn Andersson, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

On Tue, Sep 28, 2021 at 10:37 AM Charles Keepax
<ckeepax@opensource.cirrus.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  8:51   ` Arnd Bergmann
@ 2021-09-28  9:24     ` Charles Keepax
  2021-09-28 11:25       ` Mark Brown
  2021-09-29 14:15       ` Bjorn Andersson
  0 siblings, 2 replies; 17+ messages in thread
From: Charles Keepax @ 2021-09-28  9:24 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bjorn Andersson, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

On Tue, Sep 28, 2021 at 10:51:36AM +0200, Arnd Bergmann wrote:
> On Tue, Sep 28, 2021 at 10:37 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  9:24     ` Charles Keepax
@ 2021-09-28 11:25       ` Mark Brown
  2021-09-29 14:15       ` Bjorn Andersson
  1 sibling, 0 replies; 17+ messages in thread
From: Mark Brown @ 2021-09-28 11:25 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Arnd Bergmann, Bjorn Andersson, Arnd Bergmann, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

[-- Attachment #1: Type: text/plain, Size: 1106 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
  2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
  2021-09-28  8:37 ` [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Charles Keepax
@ 2021-09-28 11:58 ` Mark Brown
  2021-09-28 12:22   ` Arnd Bergmann
  2021-09-29 10:17 ` Will Deacon
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Mark Brown @ 2021-09-28 11:58 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bjorn Andersson, Arnd Bergmann, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv

[-- Attachment #1: Type: text/plain, Size: 1359 bytes --]

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 <broonie@kernel.org>

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...

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28 11:58 ` Mark Brown
@ 2021-09-28 12:22   ` Arnd Bergmann
  2021-09-29 15:00     ` Bjorn Andersson
  0 siblings, 1 reply; 17+ messages in thread
From: Arnd Bergmann @ 2021-09-28 12:22 UTC (permalink / raw)
  To: Mark Brown
  Cc: Bjorn Andersson, Arnd Bergmann, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

On Tue, Sep 28, 2021 at 1:58 PM Mark Brown <broonie@kernel.org> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol
  2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
@ 2021-09-28 13:29   ` Alex Elder
  2021-09-29 14:50   ` Bjorn Andersson
  1 sibling, 0 replies; 17+ messages in thread
From: Alex Elder @ 2021-09-28 13:29 UTC (permalink / raw)
  To: Arnd Bergmann, Bjorn Andersson
  Cc: Arnd Bergmann, Mark Brown, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv, Kalle Valo, Rob Clark,
	Sean Paul, David Airlie, Daniel Vetter, Joerg Roedel,
	Mauro Carvalho Chehab, Ulf Hansson, Alex Elder, David S. Miller,
	Jakub Kicinski, Andy Gross, Mark Rutland, Lorenzo Pieralisi,
	Sudeep Holla, linux-arm-msm, dri-devel, freedreno, iommu,
	linux-media, linux-mmc, netdev, ath10k, linux-wireless,
	linux-gpio

On 9/28/21 2:50 AM, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> Now that SCM can be a loadable module, we have to add another
> dependency to avoid link failures when ipa or adreno-gpu are
> built-in:
> 
> aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe':
> ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available'
> 
> ld.lld: error: undefined symbol: qcom_scm_is_available
>>>> referenced by adreno_gpu.c
>>>>                gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in archive drivers/built-in.a
> 
> This can happen when CONFIG_ARCH_QCOM is disabled and we don't select
> QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd
> use a similar dependency here to what we have for QCOM_RPROC_COMMON,
> but that causes dependency loops from other things selecting QCOM_SCM.
> 
> This appears to be an endless problem, so try something different this
> time:
> 
>   - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on'
>     but that is simply selected by all of its users
> 
>   - All the stubs in include/linux/qcom_scm.h can go away
> 
>   - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to
>     allow compile-testing QCOM_SCM on all architectures.
> 
>   - To avoid a circular dependency chain involving RESET_CONTROLLER
>     and PINCTRL_SUNXI, drop the 'select RESET_CONTROLLER' statement.
>     According to my testing this still builds fine, and the QCOM
>     platform selects this symbol already.
> 
> Acked-by: Kalle Valo <kvalo@codeaurora.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Changes in v2:
>    - drop the 'select RESET_CONTROLLER' line, rather than adding
>      more of the same
> ---
>   drivers/firmware/Kconfig                |  5 +-
>   drivers/gpu/drm/msm/Kconfig             |  4 +-
>   drivers/iommu/Kconfig                   |  2 +-
>   drivers/media/platform/Kconfig          |  2 +-
>   drivers/mmc/host/Kconfig                |  2 +-
>   drivers/net/ipa/Kconfig                 |  1 +

For drivers/net/ipa/Kconfig, looks good to me.
Nice simplification.

Acked-by: Alex Elder <elder@linaro.org>

>   drivers/net/wireless/ath/ath10k/Kconfig |  2 +-
>   drivers/pinctrl/qcom/Kconfig            |  3 +-
>   include/linux/arm-smccc.h               | 10 ++++
>   include/linux/qcom_scm.h                | 71 -------------------------
>   10 files changed, 20 insertions(+), 82 deletions(-)
> 

. . .

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
                   ` (2 preceding siblings ...)
  2021-09-28 11:58 ` Mark Brown
@ 2021-09-29 10:17 ` Will Deacon
  2021-10-06  8:29 ` Charles Keepax
  2021-10-09  9:24 ` Paul Menzel
  5 siblings, 0 replies; 17+ messages in thread
From: Will Deacon @ 2021-09-29 10:17 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bjorn Andersson, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Charles Keepax, Simon Trimmer, Michael Ellerman, Russell King,
	Catalin Marinas, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Simon Trimmer <simont@opensource.cirrus.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 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 <will@kernel.org>

Will

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  9:24     ` Charles Keepax
  2021-09-28 11:25       ` Mark Brown
@ 2021-09-29 14:15       ` Bjorn Andersson
  1 sibling, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-09-29 14:15 UTC (permalink / raw)
  To: Charles Keepax
  Cc: Arnd Bergmann, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

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
> > <ckeepax@opensource.cirrus.com> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol
  2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
  2021-09-28 13:29   ` Alex Elder
@ 2021-09-29 14:50   ` Bjorn Andersson
  1 sibling, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-09-29 14:50 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Arnd Bergmann, Mark Brown, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv, Kalle Valo, Rob Clark,
	Sean Paul, David Airlie, Daniel Vetter, Joerg Roedel,
	Mauro Carvalho Chehab, Ulf Hansson, Alex Elder, David S. Miller,
	Jakub Kicinski, Andy Gross, Mark Rutland, Lorenzo Pieralisi,
	Sudeep Holla, linux-arm-msm, dri-devel, freedreno, iommu,
	linux-media, linux-mmc, netdev, ath10k, linux-wireless,
	linux-gpio

On Tue 28 Sep 02:50 CDT 2021, Arnd Bergmann wrote:

> From: Arnd Bergmann <arnd@arndb.de>
> 
> Now that SCM can be a loadable module, we have to add another
> dependency to avoid link failures when ipa or adreno-gpu are
> built-in:
> 
> aarch64-linux-ld: drivers/net/ipa/ipa_main.o: in function `ipa_probe':
> ipa_main.c:(.text+0xfc4): undefined reference to `qcom_scm_is_available'
> 
> ld.lld: error: undefined symbol: qcom_scm_is_available
> >>> referenced by adreno_gpu.c
> >>>               gpu/drm/msm/adreno/adreno_gpu.o:(adreno_zap_shader_load) in archive drivers/built-in.a
> 
> This can happen when CONFIG_ARCH_QCOM is disabled and we don't select
> QCOM_MDT_LOADER, but some other module selects QCOM_SCM. Ideally we'd
> use a similar dependency here to what we have for QCOM_RPROC_COMMON,
> but that causes dependency loops from other things selecting QCOM_SCM.
> 
> This appears to be an endless problem, so try something different this
> time:
> 
>  - CONFIG_QCOM_SCM becomes a hidden symbol that nothing 'depends on'
>    but that is simply selected by all of its users
> 
>  - All the stubs in include/linux/qcom_scm.h can go away
> 
>  - arm-smccc.h needs to provide a stub for __arm_smccc_smc() to
>    allow compile-testing QCOM_SCM on all architectures.
> 
>  - To avoid a circular dependency chain involving RESET_CONTROLLER
>    and PINCTRL_SUNXI, drop the 'select RESET_CONTROLLER' statement.
>    According to my testing this still builds fine, and the QCOM
>    platform selects this symbol already.
> 
> Acked-by: Kalle Valo <kvalo@codeaurora.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Changes in v2:
>   - drop the 'select RESET_CONTROLLER' line, rather than adding
>     more of the same
> ---
>  drivers/firmware/Kconfig                |  5 +-
>  drivers/gpu/drm/msm/Kconfig             |  4 +-
>  drivers/iommu/Kconfig                   |  2 +-
>  drivers/media/platform/Kconfig          |  2 +-
>  drivers/mmc/host/Kconfig                |  2 +-
>  drivers/net/ipa/Kconfig                 |  1 +
>  drivers/net/wireless/ath/ath10k/Kconfig |  2 +-
>  drivers/pinctrl/qcom/Kconfig            |  3 +-
>  include/linux/arm-smccc.h               | 10 ++++
>  include/linux/qcom_scm.h                | 71 -------------------------
>  10 files changed, 20 insertions(+), 82 deletions(-)
> 
> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
> index 220a58cf0a44..cda7d7162cbb 100644
> --- a/drivers/firmware/Kconfig
> +++ b/drivers/firmware/Kconfig
> @@ -203,10 +203,7 @@ config INTEL_STRATIX10_RSU
>  	  Say Y here if you want Intel RSU support.
>  
>  config QCOM_SCM
> -	tristate "Qcom SCM driver"
> -	depends on ARM || ARM64
> -	depends on HAVE_ARM_SMCCC
> -	select RESET_CONTROLLER
> +	tristate
>  
>  config QCOM_SCM_DOWNLOAD_MODE_DEFAULT
>  	bool "Qualcomm download mode enabled by default"
> diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
> index e9c6af78b1d7..3ddf739a6f9b 100644
> --- a/drivers/gpu/drm/msm/Kconfig
> +++ b/drivers/gpu/drm/msm/Kconfig
> @@ -17,7 +17,7 @@ config DRM_MSM
>  	select DRM_SCHED
>  	select SHMEM
>  	select TMPFS
> -	select QCOM_SCM if ARCH_QCOM
> +	select QCOM_SCM
>  	select WANT_DEV_COREDUMP
>  	select SND_SOC_HDMI_CODEC if SND_SOC
>  	select SYNC_FILE
> @@ -55,7 +55,7 @@ config DRM_MSM_GPU_SUDO
>  
>  config DRM_MSM_HDMI_HDCP
>  	bool "Enable HDMI HDCP support in MSM DRM driver"
> -	depends on DRM_MSM && QCOM_SCM
> +	depends on DRM_MSM
>  	default y
>  	help
>  	  Choose this option to enable HDCP state machine
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 124c41adeca1..989c83acbfee 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -308,7 +308,7 @@ config APPLE_DART
>  config ARM_SMMU
>  	tristate "ARM Ltd. System MMU (SMMU) Support"
>  	depends on ARM64 || ARM || (COMPILE_TEST && !GENERIC_ATOMIC64)
> -	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> +	select QCOM_SCM
>  	select IOMMU_API
>  	select IOMMU_IO_PGTABLE_LPAE
>  	select ARM_DMA_USE_IOMMU if ARM

As noted in the RFC, I think you also need to fix QCOM_IOMMU.

In particular (iirc) since all the users of the iommu might be modules,
which would prevent QCOM_IOMMU from being selected.

The rest looks good.

Regards,
Bjorn

> diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> index 157c924686e4..80321e03809a 100644
> --- a/drivers/media/platform/Kconfig
> +++ b/drivers/media/platform/Kconfig
> @@ -565,7 +565,7 @@ config VIDEO_QCOM_VENUS
>  	depends on VIDEO_DEV && VIDEO_V4L2 && QCOM_SMEM
>  	depends on (ARCH_QCOM && IOMMU_DMA) || COMPILE_TEST
>  	select QCOM_MDT_LOADER if ARCH_QCOM
> -	select QCOM_SCM if ARCH_QCOM
> +	select QCOM_SCM
>  	select VIDEOBUF2_DMA_CONTIG
>  	select V4L2_MEM2MEM_DEV
>  	help
> diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
> index 71313961cc54..95b3511b0560 100644
> --- a/drivers/mmc/host/Kconfig
> +++ b/drivers/mmc/host/Kconfig
> @@ -547,7 +547,7 @@ config MMC_SDHCI_MSM
>  	depends on MMC_SDHCI_PLTFM
>  	select MMC_SDHCI_IO_ACCESSORS
>  	select MMC_CQHCI
> -	select QCOM_SCM if MMC_CRYPTO && ARCH_QCOM
> +	select QCOM_SCM if MMC_CRYPTO
>  	help
>  	  This selects the Secure Digital Host Controller Interface (SDHCI)
>  	  support present in Qualcomm SOCs. The controller supports
> diff --git a/drivers/net/ipa/Kconfig b/drivers/net/ipa/Kconfig
> index 8f99cfa14680..d037682fb7ad 100644
> --- a/drivers/net/ipa/Kconfig
> +++ b/drivers/net/ipa/Kconfig
> @@ -4,6 +4,7 @@ config QCOM_IPA
>  	depends on ARCH_QCOM || COMPILE_TEST
>  	depends on QCOM_RPROC_COMMON || (QCOM_RPROC_COMMON=n && COMPILE_TEST)
>  	select QCOM_MDT_LOADER if ARCH_QCOM
> +	select QCOM_SCM
>  	select QCOM_QMI_HELPERS
>  	help
>  	  Choose Y or M here to include support for the Qualcomm
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig b/drivers/net/wireless/ath/ath10k/Kconfig
> index 741289e385d5..ca007b800f75 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -44,7 +44,7 @@ config ATH10K_SNOC
>  	tristate "Qualcomm ath10k SNOC support"
>  	depends on ATH10K
>  	depends on ARCH_QCOM || COMPILE_TEST
> -	depends on QCOM_SCM || !QCOM_SCM #if QCOM_SCM=m this can't be =y
> +	select QCOM_SCM
>  	select QCOM_QMI_HELPERS
>  	help
>  	  This module adds support for integrated WCN3990 chip connected
> diff --git a/drivers/pinctrl/qcom/Kconfig b/drivers/pinctrl/qcom/Kconfig
> index 32ea2a8ec02b..5ff4207df66e 100644
> --- a/drivers/pinctrl/qcom/Kconfig
> +++ b/drivers/pinctrl/qcom/Kconfig
> @@ -3,7 +3,8 @@ if (ARCH_QCOM || COMPILE_TEST)
>  
>  config PINCTRL_MSM
>  	tristate "Qualcomm core pin controller driver"
> -	depends on GPIOLIB && (QCOM_SCM || !QCOM_SCM) #if QCOM_SCM=m this can't be =y
> +	depends on GPIOLIB
> +	select QCOM_SCM
>  	select PINMUX
>  	select PINCONF
>  	select GENERIC_PINCONF
> diff --git a/include/linux/arm-smccc.h b/include/linux/arm-smccc.h
> index 7d1cabe15262..63ccb5252190 100644
> --- a/include/linux/arm-smccc.h
> +++ b/include/linux/arm-smccc.h
> @@ -321,10 +321,20 @@ asmlinkage unsigned long __arm_smccc_sve_check(unsigned long x0);
>   * from register 0 to 3 on return from the SMC instruction.  An optional
>   * quirk structure provides vendor specific behavior.
>   */
> +#ifdef CONFIG_HAVE_ARM_SMCCC
>  asmlinkage void __arm_smccc_smc(unsigned long a0, unsigned long a1,
>  			unsigned long a2, unsigned long a3, unsigned long a4,
>  			unsigned long a5, unsigned long a6, unsigned long a7,
>  			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk);
> +#else
> +static inline void __arm_smccc_smc(unsigned long a0, unsigned long a1,
> +			unsigned long a2, unsigned long a3, unsigned long a4,
> +			unsigned long a5, unsigned long a6, unsigned long a7,
> +			struct arm_smccc_res *res, struct arm_smccc_quirk *quirk)
> +{
> +	*res = (struct arm_smccc_res){};
> +}
> +#endif
>  
>  /**
>   * __arm_smccc_hvc() - make HVC calls
> diff --git a/include/linux/qcom_scm.h b/include/linux/qcom_scm.h
> index c0475d1c9885..81cad9e1e412 100644
> --- a/include/linux/qcom_scm.h
> +++ b/include/linux/qcom_scm.h
> @@ -61,7 +61,6 @@ enum qcom_scm_ice_cipher {
>  #define QCOM_SCM_PERM_RW (QCOM_SCM_PERM_READ | QCOM_SCM_PERM_WRITE)
>  #define QCOM_SCM_PERM_RWX (QCOM_SCM_PERM_RW | QCOM_SCM_PERM_EXEC)
>  
> -#if IS_ENABLED(CONFIG_QCOM_SCM)
>  extern bool qcom_scm_is_available(void);
>  
>  extern int qcom_scm_set_cold_boot_addr(void *entry, const cpumask_t *cpus);
> @@ -115,74 +114,4 @@ extern int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
>  extern int qcom_scm_lmh_profile_change(u32 profile_id);
>  extern bool qcom_scm_lmh_dcvsh_available(void);
>  
> -#else
> -
> -#include <linux/errno.h>
> -
> -static inline bool qcom_scm_is_available(void) { return false; }
> -
> -static inline int qcom_scm_set_cold_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline int qcom_scm_set_warm_boot_addr(void *entry,
> -		const cpumask_t *cpus) { return -ENODEV; }
> -static inline void qcom_scm_cpu_power_down(u32 flags) {}
> -static inline u32 qcom_scm_set_remote_state(u32 state,u32 id)
> -		{ return -ENODEV; }
> -
> -static inline int qcom_scm_pas_init_image(u32 peripheral, const void *metadata,
> -		size_t size) { return -ENODEV; }
> -static inline int qcom_scm_pas_mem_setup(u32 peripheral, phys_addr_t addr,
> -		phys_addr_t size) { return -ENODEV; }
> -static inline int qcom_scm_pas_auth_and_reset(u32 peripheral)
> -		{ return -ENODEV; }
> -static inline int qcom_scm_pas_shutdown(u32 peripheral) { return -ENODEV; }
> -static inline bool qcom_scm_pas_supported(u32 peripheral) { return false; }
> -
> -static inline int qcom_scm_io_readl(phys_addr_t addr, unsigned int *val)
> -		{ return -ENODEV; }
> -static inline int qcom_scm_io_writel(phys_addr_t addr, unsigned int val)
> -		{ return -ENODEV; }
> -
> -static inline bool qcom_scm_restore_sec_cfg_available(void) { return false; }
> -static inline int qcom_scm_restore_sec_cfg(u32 device_id, u32 spare)
> -		{ return -ENODEV; }
> -static inline int qcom_scm_iommu_secure_ptbl_size(u32 spare, size_t *size)
> -		{ return -ENODEV; }
> -static inline int qcom_scm_iommu_secure_ptbl_init(u64 addr, u32 size, u32 spare)
> -		{ return -ENODEV; }
> -extern inline int qcom_scm_mem_protect_video_var(u32 cp_start, u32 cp_size,
> -						 u32 cp_nonpixel_start,
> -						 u32 cp_nonpixel_size)
> -		{ return -ENODEV; }
> -static inline int qcom_scm_assign_mem(phys_addr_t mem_addr, size_t mem_sz,
> -		unsigned int *src, const struct qcom_scm_vmperm *newvm,
> -		unsigned int dest_cnt) { return -ENODEV; }
> -
> -static inline bool qcom_scm_ocmem_lock_available(void) { return false; }
> -static inline int qcom_scm_ocmem_lock(enum qcom_scm_ocmem_client id, u32 offset,
> -		u32 size, u32 mode) { return -ENODEV; }
> -static inline int qcom_scm_ocmem_unlock(enum qcom_scm_ocmem_client id,
> -		u32 offset, u32 size) { return -ENODEV; }
> -
> -static inline bool qcom_scm_ice_available(void) { return false; }
> -static inline int qcom_scm_ice_invalidate_key(u32 index) { return -ENODEV; }
> -static inline int qcom_scm_ice_set_key(u32 index, const u8 *key, u32 key_size,
> -				       enum qcom_scm_ice_cipher cipher,
> -				       u32 data_unit_size) { return -ENODEV; }
> -
> -static inline bool qcom_scm_hdcp_available(void) { return false; }
> -static inline int qcom_scm_hdcp_req(struct qcom_scm_hdcp_req *req, u32 req_cnt,
> -		u32 *resp) { return -ENODEV; }
> -
> -static inline int qcom_scm_qsmmu500_wait_safe_toggle(bool en)
> -		{ return -ENODEV; }
> -
> -static inline int qcom_scm_lmh_dcvsh(u32 payload_fn, u32 payload_reg, u32 payload_val,
> -				     u64 limit_node, u32 node_id, u64 version)
> -		{ return -ENODEV; }
> -
> -static inline int qcom_scm_lmh_profile_change(u32 profile_id) { return -ENODEV; }
> -
> -static inline bool qcom_scm_lmh_dcvsh_available(void) { return -ENODEV; }
> -#endif
>  #endif
> -- 
> 2.29.2
> 

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28 12:22   ` Arnd Bergmann
@ 2021-09-29 15:00     ` Bjorn Andersson
  0 siblings, 0 replies; 17+ messages in thread
From: Bjorn Andersson @ 2021-09-29 15:00 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Mark Brown, Arnd Bergmann, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	the arch/x86 maintainers, H. Peter Anvin, Geert Uytterhoeven,
	Linus Walleij, Andrew Morton, Greg Kroah-Hartman, Linux ARM,
	Linux Kernel Mailing List, linux-ia64,
	open list:BROADCOM NVRAM DRIVER, Parisc List, linux-riscv

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

> On Tue, Sep 28, 2021 at 1:58 PM Mark Brown <broonie@kernel.org> 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 <bjorn.andersson@linaro.org>

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

Acked-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Thanks,
Bjorn

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
                   ` (3 preceding siblings ...)
  2021-09-29 10:17 ` Will Deacon
@ 2021-10-06  8:29 ` Charles Keepax
  2021-10-09  9:24 ` Paul Menzel
  5 siblings, 0 replies; 17+ messages in thread
From: Charles Keepax @ 2021-10-06  8:29 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Bjorn Andersson, Arnd Bergmann, Mark Brown, Liam Girdwood,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv

On Tue, Sep 28, 2021 at 09:50:26AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Simon Trimmer <simont@opensource.cirrus.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---

Reviewed-by: Charles Keepax <ckeepax@opensource.cirrus.com>

Thanks,
Charles

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
                   ` (4 preceding siblings ...)
  2021-10-06  8:29 ` Charles Keepax
@ 2021-10-09  9:24 ` Paul Menzel
  2021-10-11  8:42   ` Geert Uytterhoeven
  5 siblings, 1 reply; 17+ messages in thread
From: Paul Menzel @ 2021-10-09  9:24 UTC (permalink / raw)
  To: Arnd Bergmann, Bjorn Andersson
  Cc: Arnd Bergmann, Mark Brown, Liam Girdwood, Charles Keepax,
	Simon Trimmer, Michael Ellerman, Russell King, Catalin Marinas,
	Will Deacon, Thomas Bogendoerfer, James E.J. Bottomley,
	Helge Deller, Paul Walmsley, Palmer Dabbelt, Albert Ou,
	Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
	H. Peter Anvin, Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, linux-arm-kernel, linux-kernel, linux-ia64,
	linux-mips, linux-parisc, linux-riscv, linuxppc-dev

[Cc: +linuxppc-dev@lists.ozlabs.org]

Dear Arnd,


Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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 <broonie@kernel.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Simon Trimmer <simont@opensource.cirrus.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-10-09  9:24 ` Paul Menzel
@ 2021-10-11  8:42   ` Geert Uytterhoeven
  2021-10-11  9:45     ` Arnd Bergmann
  0 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2021-10-11  8:42 UTC (permalink / raw)
  To: Paul Menzel
  Cc: Arnd Bergmann, Bjorn Andersson, Arnd Bergmann, Mark Brown,
	Liam Girdwood, Charles Keepax, Simon Trimmer, Michael Ellerman,
	Russell King, Catalin Marinas, Will Deacon, Thomas Bogendoerfer,
	James E.J. Bottomley, Helge Deller, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, the arch/x86 maintainers, H. Peter Anvin,
	Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, Linux ARM, Linux Kernel Mailing List,
	linux-ia64, open list:BROADCOM NVRAM DRIVER, Parisc List,
	linux-riscv, linuxppc-dev

On Sat, Oct 9, 2021 at 11:24 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> [Cc: +linuxppc-dev@lists.ozlabs.org]
>
> Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> > 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 -- geert@linux-m68k.org

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally
  2021-10-11  8:42   ` Geert Uytterhoeven
@ 2021-10-11  9:45     ` Arnd Bergmann
  0 siblings, 0 replies; 17+ messages in thread
From: Arnd Bergmann @ 2021-10-11  9:45 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Paul Menzel, Bjorn Andersson, Arnd Bergmann, Mark Brown,
	Liam Girdwood, Charles Keepax, Simon Trimmer, Michael Ellerman,
	Russell King, Catalin Marinas, Will Deacon, Thomas Bogendoerfer,
	James E.J. Bottomley, Helge Deller, Paul Walmsley,
	Palmer Dabbelt, Albert Ou, Thomas Gleixner, Ingo Molnar,
	Borislav Petkov, the arch/x86 maintainers, H. Peter Anvin,
	Geert Uytterhoeven, Linus Walleij, Andrew Morton,
	Greg Kroah-Hartman, Linux ARM, Linux Kernel Mailing List,
	linux-ia64, open list:BROADCOM NVRAM DRIVER, Parisc List,
	linux-riscv, linuxppc-dev

On Mon, Oct 11, 2021 at 10:42 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Sat, Oct 9, 2021 at 11:24 AM Paul Menzel <pmenzel@molgen.mpg.de> wrote:
> > Am 28.09.21 um 09:50 schrieb Arnd Bergmann:
> > > From: Arnd Bergmann <arnd@arndb.de>
> > +#
> > +# 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

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2021-10-11  9:46 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28  7:50 [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Arnd Bergmann
2021-09-28  7:50 ` [PATCH 2/2] [v2] qcom_scm: hide Kconfig symbol Arnd Bergmann
2021-09-28 13:29   ` Alex Elder
2021-09-29 14:50   ` Bjorn Andersson
2021-09-28  8:37 ` [PATCH 1/2] firmware: include drivers/firmware/Kconfig unconditionally Charles Keepax
2021-09-28  8:51   ` Arnd Bergmann
2021-09-28  9:24     ` Charles Keepax
2021-09-28 11:25       ` Mark Brown
2021-09-29 14:15       ` Bjorn Andersson
2021-09-28 11:58 ` Mark Brown
2021-09-28 12:22   ` Arnd Bergmann
2021-09-29 15:00     ` Bjorn Andersson
2021-09-29 10:17 ` Will Deacon
2021-10-06  8:29 ` Charles Keepax
2021-10-09  9:24 ` Paul Menzel
2021-10-11  8:42   ` Geert Uytterhoeven
2021-10-11  9:45     ` Arnd Bergmann

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).