[PATCH] firmware: arm_scmi: fix i.MX build dependency

[PATCH] firmware: arm_scmi: fix i.MX build dependency

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The newly added SCMI vendor driver references functions in the
protocol driver but needs a Kconfig dependency to ensure it can link,
essentially the Kconfig dependency needs to be reversed to match the
link time dependency:

arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'

This however only works after changing the dependency in the SND_SOC_FSL_MQS
driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
driver it depends on. This is generally a bad idea, so the best solution
is to change that into a dependency.

To allow the ASoC driver to keep building with the SCMI support, this
needs to be an optional dependency that enforces the link-time
dependency if IMX_SCMI_MISC_DRV is a loadable module but not
depend on it if that is disabled.

Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I'm not sure this is the version we want to go with in the end, without
having read the code in detail, but the version in linux-next is clearly
wrong in multiple ways and this is the best I could come up with so far.

If we can agree on this approach, it needs to go through the ASoC tree
as that contains the 101c9023594a commit at the moment.
---
 drivers/firmware/arm_scmi/vendors/imx/Kconfig | 1 +
 drivers/firmware/imx/Kconfig                  | 1 -
 sound/soc/fsl/Kconfig                         | 2 +-
 3 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
index 2883ed24a84d..a01bf5e47301 100644
--- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig
+++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
@@ -15,6 +15,7 @@ config IMX_SCMI_BBM_EXT
 config IMX_SCMI_MISC_EXT
 	tristate "i.MX SCMI MISC EXTENSION"
 	depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
+	depends on IMX_SCMI_MISC_DRV
 	default y if ARCH_MXC
 	help
 	  This enables i.MX System MISC control logic such as gpio expander
diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
index 477d3f32d99a..907cd149c40a 100644
--- a/drivers/firmware/imx/Kconfig
+++ b/drivers/firmware/imx/Kconfig
@@ -25,7 +25,6 @@ config IMX_SCU
 
 config IMX_SCMI_MISC_DRV
 	tristate "IMX SCMI MISC Protocol driver"
-	depends on IMX_SCMI_MISC_EXT || COMPILE_TEST
 	default y if ARCH_MXC
 	help
 	  The System Controller Management Interface firmware (SCMI FW) is
diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
index 8e88830e8e57..678540b78280 100644
--- a/sound/soc/fsl/Kconfig
+++ b/sound/soc/fsl/Kconfig
@@ -29,8 +29,8 @@ config SND_SOC_FSL_SAI
 config SND_SOC_FSL_MQS
 	tristate "Medium Quality Sound (MQS) module support"
 	depends on SND_SOC_FSL_SAI
+	depends on IMX_SCMI_MISC_DRV || !IMX_SCMI_MISC_DRV
 	select REGMAP_MMIO
-	select IMX_SCMI_MISC_DRV if IMX_SCMI_MISC_EXT !=n
 	help
 	  Say Y if you want to add Medium Quality Sound (MQS)
 	  support for the Freescale CPUs.
-- 
2.39.5

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Shawn Guo]

On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
> 
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
> 
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
> 
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
> 
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
> 
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.

Thanks for spotting and fixing, Arnd!

Unless Peng has objection to this change, I'm good with it.

Shawn

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Cristian Marussi]

On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 

Hi Arnd,

> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
> 
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
> 

The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV, generally make
ue of the related vendor protocol like IMX_SCMI_MISC_EXT, BUT the SCMI
stack is designed in a way that NO symbols are needed to be exported by
the protocol layer (to avoid a huge and growing number of symbols
exports)...so usually the current DRV-->PROTO dependency is fine.

In this case, AFAIU, it is the SCMI driver that in turn exports a few
helpers that are used by another driver fsl_mqs, which in turn could be
compiled and work with or without the SCMI stack, so with this patch we
are artificially reversing the DRV<--PROTO dependency to solve this
scenario in all the compillation scenarios...

....BUT given that the IMX_SCMI_MISC_DRV is the one that should export
the missing symbols could NOT this solved in a cleaner way, without
adding the fake reverse dependency, by instead modifying the header of
the driver with something like the classic:

--->8-----
diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
index 9b85a3f028d1..3a7a3ec367c5 100644
--- a/include/linux/firmware/imx/sm.h
+++ b/include/linux/firmware/imx/sm.h
@@ -17,7 +17,19 @@
 #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4 MCLK */
 #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5 MCLK */
 
+#ifdef IMX_SCMI_MISC_DRV
 int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
 int scmi_imx_misc_ctrl_set(u32 id, u32 val);
+#else
+static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
+{
+       return 0;
+}
+
+static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val)
+{
+       return 0;
+}
+#endif
 
 #endif
----->8-----------

....to just support compilation in all the scenarios.

> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
> 
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
> 

...and maybe with the above additions you could avoid also these other
dep changes...

...not sure if I am missing something and I have definitely not tested
any of my babbling above...

Thanks,
Cristian

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Arnd Bergmann]

On Sun, Nov 17, 2024, at 11:03, Cristian Marussi wrote:
> On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
>> 
>> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
>> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
>> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
>> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>> 
>
> The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV, generally make
> ue of the related vendor protocol like IMX_SCMI_MISC_EXT, BUT the SCMI
> stack is designed in a way that NO symbols are needed to be exported by
> the protocol layer (to avoid a huge and growing number of symbols
> exports)...so usually the current DRV-->PROTO dependency is fine.
>
> In this case, AFAIU, it is the SCMI driver that in turn exports a few
> helpers that are used by another driver fsl_mqs, which in turn could be
> compiled and work with or without the SCMI stack, so with this patch we
> are artificially reversing the DRV<--PROTO dependency to solve this
> scenario in all the compillation scenarios...
>
> ....BUT given that the IMX_SCMI_MISC_DRV is the one that should export
> the missing symbols could NOT this solved in a cleaner way, without
> adding the fake reverse dependency, by instead modifying the header of
> the driver with something like the classic:

> --->8-----
> diff --git a/include/linux/firmware/imx/sm.h b/include/linux/firmware/imx/sm.h
> index 9b85a3f028d1..3a7a3ec367c5 100644
> --- a/include/linux/firmware/imx/sm.h
> +++ b/include/linux/firmware/imx/sm.h
> @@ -17,7 +17,19 @@
>  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4 MCLK */
>  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5 MCLK */
> 
> +#ifdef IMX_SCMI_MISC_DRV
>  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);
>  int scmi_imx_misc_ctrl_set(u32 id, u32 val);
> +#else
> +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> +{
> +       return 0;
> +}
> +
> +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val)
> +{
> +       return 0;
> +}
> +#endif

This usually doesn't work if the provider of these interfaces
can be in a loadable module. The #ifdef above means this won't
be usable when CONFIG_IMX_SCMI_MISC_DRV=m, while changing it to
IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV) still produces a link error
when the consumer is built-in. Changing it to IS_REACHABLE()
in turn is even worse because it avoids the link failure but
makes it silently do the wrong thing in some configurations.

>  #endif
> ----->8-----------
>
> ....to just support compilation in all the scenarios.
>
>> This however only works after changing the dependency in the SND_SOC_FSL_MQS
>> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
>> driver it depends on. This is generally a bad idea, so the best solution
>> is to change that into a dependency.
>> 
>> To allow the ASoC driver to keep building with the SCMI support, this
>> needs to be an optional dependency that enforces the link-time
>> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
>> depend on it if that is disabled.
>> 
>
> ...and maybe with the above additions you could avoid also these other
> dep changes...
>
> ...not sure if I am missing something and I have definitely not tested
> any of my babbling above...

In my experience, there is no way to avoid reflecting the dependencies
correctly in Kconfig: if one driver has an EXPORT_SYMBOL that
gets picked up by another driver, you need a matching 'depends on'.

       Arnd

RE: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Peng Fan]

> Subject: Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency
> 
> On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> > From: Arnd Bergmann <arnd@arndb.de>
> >
> 
> Hi Arnd,
> 
> > The newly added SCMI vendor driver references functions in the
> > protocol driver but needs a Kconfig dependency to ensure it can link,
> > essentially the Kconfig dependency needs to be reversed to match
> the
> > link time dependency:
> >
> > arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_write':
> > fsl_mqs.c:(.text+0x1aa): undefined reference to
> `scmi_imx_misc_ctrl_set'
> > arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_read':
> > fsl_mqs.c:(.text+0x1ee): undefined reference to
> `scmi_imx_misc_ctrl_get'
> >
> 
> The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV,
> generally make ue of the related vendor protocol like
> IMX_SCMI_MISC_EXT, BUT the SCMI stack is designed in a way that NO
> symbols are needed to be exported by the protocol layer (to avoid a
> huge and growing number of symbols exports)...so usually the current
> DRV-->PROTO dependency is fine.
> 
> In this case, AFAIU, it is the SCMI driver that in turn exports a few
> helpers that are used by another driver fsl_mqs, which in turn could be
> compiled and work with or without the SCMI stack, so with this patch
> we are artificially reversing the DRV<--PROTO dependency to solve this
> scenario in all the compillation scenarios...
> 
> ....BUT given that the IMX_SCMI_MISC_DRV is the one that should
> export the missing symbols could NOT this solved in a cleaner way,
> without adding the fake reverse dependency, by instead modifying the
> header of the driver with something like the classic:
> 
> --->8-----
> diff --git a/include/linux/firmware/imx/sm.h
> b/include/linux/firmware/imx/sm.h index
> 9b85a3f028d1..3a7a3ec367c5 100644
> --- a/include/linux/firmware/imx/sm.h
> +++ b/include/linux/firmware/imx/sm.h
> @@ -17,7 +17,19 @@
>  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4
> MCLK */
>  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5
> MCLK */
> 
> +#ifdef IMX_SCMI_MISC_DRV
>  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);  int
> scmi_imx_misc_ctrl_set(u32 id, u32 val);
> +#else
> +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val) {
> +       return 0;
> +}
> +
> +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val) {
> +       return 0;
> +}
> +#endif
> 

This change is good to me. With just a difference that in downstream
we use -EOPNOTSUPP

https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/include/linux/firmware/imx/sm.h#L19

Thanks,
Peng.

>  #endif
> ----->8-----------
> 
> ....to just support compilation in all the scenarios.
> 
> > This however only works after changing the dependency in the
> > SND_SOC_FSL_MQS driver as well, which uses 'select
> IMX_SCMI_MISC_DRV'
> > to turn on a driver it depends on. This is generally a bad idea, so
> > the best solution is to change that into a dependency.
> >
> > To allow the ASoC driver to keep building with the SCMI support, this
> > needs to be an optional dependency that enforces the link-time
> > dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on
> > it if that is disabled.
> >
> 
> ...and maybe with the above additions you could avoid also these other
> dep changes...
> 
> ...not sure if I am missing something and I have definitely not tested
> any of my babbling above...
> 
> Thanks,
> Cristian

RE: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Peng Fan]

> Subject: Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency
> 
> On Sun, Nov 17, 2024, at 11:03, Cristian Marussi wrote:
> > On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> >>
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_write':
> >> fsl_mqs.c:(.text+0x1aa): undefined reference to
> `scmi_imx_misc_ctrl_set'
> >> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function
> `fsl_mqs_sm_read':
> >> fsl_mqs.c:(.text+0x1ee): undefined reference to
> `scmi_imx_misc_ctrl_get'
> >>
> >
> > The SCMI drivers, like the newly added IMX_SCMI_MISC_DRV,
> generally
> > make ue of the related vendor protocol like IMX_SCMI_MISC_EXT,
> BUT the
> > SCMI stack is designed in a way that NO symbols are needed to be
> > exported by the protocol layer (to avoid a huge and growing number
> of
> > symbols exports)...so usually the current DRV-->PROTO dependency is
> fine.
> >
> > In this case, AFAIU, it is the SCMI driver that in turn exports a few
> > helpers that are used by another driver fsl_mqs, which in turn could
> > be compiled and work with or without the SCMI stack, so with this
> > patch we are artificially reversing the DRV<--PROTO dependency to
> > solve this scenario in all the compillation scenarios...
> >
> > ....BUT given that the IMX_SCMI_MISC_DRV is the one that should
> export
> > the missing symbols could NOT this solved in a cleaner way, without
> > adding the fake reverse dependency, by instead modifying the header
> of
> > the driver with something like the classic:
> 
> > --->8-----
> > diff --git a/include/linux/firmware/imx/sm.h
> > b/include/linux/firmware/imx/sm.h index
> 9b85a3f028d1..3a7a3ec367c5
> > 100644
> > --- a/include/linux/firmware/imx/sm.h
> > +++ b/include/linux/firmware/imx/sm.h
> > @@ -17,7 +17,19 @@
> >  #define SCMI_IMX_CTRL_SAI4_MCLK                4       /* WAKE SAI4
> MCLK */
> >  #define SCMI_IMX_CTRL_SAI5_MCLK                5       /* WAKE SAI5
> MCLK */
> >
> > +#ifdef IMX_SCMI_MISC_DRV
> >  int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val);  int
> > scmi_imx_misc_ctrl_set(u32 id, u32 val);
> > +#else
> > +static inline int scmi_imx_misc_ctrl_get(u32 id, u32 *num, u32 *val)
> > +{
> > +       return 0;
> > +}
> > +
> > +static inline int scmi_imx_misc_ctrl_set(u32 id, u32 val) {
> > +       return 0;
> > +}
> > +#endif
> 
> This usually doesn't work if the provider of these interfaces can be in a
> loadable module. The #ifdef above means this won't be usable when
> CONFIG_IMX_SCMI_MISC_DRV=m, while changing it to
> IS_ENABLED(CONFIG_IMX_SCMI_MISC_DRV) still produces a link error
> when the consumer is built-in. Changing it to IS_REACHABLE() in turn is
> even worse because it avoids the link failure but makes it silently do
> the wrong thing in some configurations.
> 
> >  #endif
> > ----->8-----------
> >
> > ....to just support compilation in all the scenarios.
> >
> >> This however only works after changing the dependency in the
> >> SND_SOC_FSL_MQS driver as well, which uses 'select
> IMX_SCMI_MISC_DRV'
> >> to turn on a driver it depends on. This is generally a bad idea, so
> >> the best solution is to change that into a dependency.
> >>
> >> To allow the ASoC driver to keep building with the SCMI support,
> this
> >> needs to be an optional dependency that enforces the link-time
> >> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend
> >> on it if that is disabled.
> >>
> >
> > ...and maybe with the above additions you could avoid also these
> other
> > dep changes...
> >
> > ...not sure if I am missing something and I have definitely not tested
> > any of my babbling above...
> 
> In my experience, there is no way to avoid reflecting the dependencies
> correctly in Kconfig: if one driver has an EXPORT_SYMBOL that gets
> picked up by another driver, you need a matching 'depends on'.

Oh. Thanks for sharing the knowledge. I am ok with your change.

Thanks,
Peng.

> 
>        Arnd

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Mark Brown]

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

On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:

Acked-by: Mark Brown <broonie@kernel.org>

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

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Shengjiu Wang]

On Sat, Nov 16, 2024 at 7:08 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
>
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
>
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
>
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Acked-by: Shengjiu Wang <shengjiu.wang@gmail.com>

Best regards
Shengjiu Wang
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
>
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.
> ---
>  drivers/firmware/arm_scmi/vendors/imx/Kconfig | 1 +
>  drivers/firmware/imx/Kconfig                  | 1 -
>  sound/soc/fsl/Kconfig                         | 2 +-
>  3 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/vendors/imx/Kconfig b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> index 2883ed24a84d..a01bf5e47301 100644
> --- a/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> +++ b/drivers/firmware/arm_scmi/vendors/imx/Kconfig
> @@ -15,6 +15,7 @@ config IMX_SCMI_BBM_EXT
>  config IMX_SCMI_MISC_EXT
>         tristate "i.MX SCMI MISC EXTENSION"
>         depends on ARM_SCMI_PROTOCOL || (COMPILE_TEST && OF)
> +       depends on IMX_SCMI_MISC_DRV
>         default y if ARCH_MXC
>         help
>           This enables i.MX System MISC control logic such as gpio expander
> diff --git a/drivers/firmware/imx/Kconfig b/drivers/firmware/imx/Kconfig
> index 477d3f32d99a..907cd149c40a 100644
> --- a/drivers/firmware/imx/Kconfig
> +++ b/drivers/firmware/imx/Kconfig
> @@ -25,7 +25,6 @@ config IMX_SCU
>
>  config IMX_SCMI_MISC_DRV
>         tristate "IMX SCMI MISC Protocol driver"
> -       depends on IMX_SCMI_MISC_EXT || COMPILE_TEST
>         default y if ARCH_MXC
>         help
>           The System Controller Management Interface firmware (SCMI FW) is
> diff --git a/sound/soc/fsl/Kconfig b/sound/soc/fsl/Kconfig
> index 8e88830e8e57..678540b78280 100644
> --- a/sound/soc/fsl/Kconfig
> +++ b/sound/soc/fsl/Kconfig
> @@ -29,8 +29,8 @@ config SND_SOC_FSL_SAI
>  config SND_SOC_FSL_MQS
>         tristate "Medium Quality Sound (MQS) module support"
>         depends on SND_SOC_FSL_SAI
> +       depends on IMX_SCMI_MISC_DRV || !IMX_SCMI_MISC_DRV
>         select REGMAP_MMIO
> -       select IMX_SCMI_MISC_DRV if IMX_SCMI_MISC_EXT !=n
>         help
>           Say Y if you want to add Medium Quality Sound (MQS)
>           support for the Freescale CPUs.
> --
> 2.39.5
>
>

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Sudeep Holla]

On Sat, Nov 16, 2024 at 12:05:18AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
>
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
>
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
>
> This however only works after changing the dependency in the SND_SOC_FSL_MQS
> driver as well, which uses 'select IMX_SCMI_MISC_DRV' to turn on a
> driver it depends on. This is generally a bad idea, so the best solution
> is to change that into a dependency.
>
> To allow the ASoC driver to keep building with the SCMI support, this
> needs to be an optional dependency that enforces the link-time
> dependency if IMX_SCMI_MISC_DRV is a loadable module but not
> depend on it if that is disabled.
>
> Fixes: 61c9f03e22fc ("firmware: arm_scmi: Add initial support for i.MX MISC protocol")
> Fixes: 101c9023594a ("ASoC: fsl_mqs: Support accessing registers by scmi interface")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I'm not sure this is the version we want to go with in the end, without
> having read the code in detail, but the version in linux-next is clearly
> wrong in multiple ways and this is the best I could come up with so far.
>
> If we can agree on this approach, it needs to go through the ASoC tree
> as that contains the 101c9023594a commit at the moment.

Sorry for the late response, was away for a while. I am fine with the
change, but bit confused as Mark Brown has acked the change instead by
which I assume he is not taking it through his tree.

I can can take it at -rc1 or if Mark B is happy to pull it, here is my ack.

Acked-by: Sudeep Holla <sudeep.holla@arm.com>

--
Regards,
Sudeep

Re: [PATCH] firmware: arm_scmi: fix i.MX build dependency

[Sudeep Holla]

On Sat, 16 Nov 2024 00:05:18 +0100, Arnd Bergmann wrote:
> The newly added SCMI vendor driver references functions in the
> protocol driver but needs a Kconfig dependency to ensure it can link,
> essentially the Kconfig dependency needs to be reversed to match the
> link time dependency:
> 
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_write':
> fsl_mqs.c:(.text+0x1aa): undefined reference to `scmi_imx_misc_ctrl_set'
> arm-linux-gnueabi-ld: sound/soc/fsl/fsl_mqs.o: in function `fsl_mqs_sm_read':
> fsl_mqs.c:(.text+0x1ee): undefined reference to `scmi_imx_misc_ctrl_get'
> 
> [...]

Applied to sudeep.holla/linux (for-next/scmi/fixes), thanks!

[1/1] firmware: arm_scmi: fix i.MX build dependency
      https://git.kernel.org/sudeep.holla/c/514b2262ade4
--
Regards,
Sudeep