| Message ID | 20241115230555.2435004-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | firmware: arm_scmi: fix i.MX build dependency | expand |
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
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
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
> 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
> 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
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>
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 > >
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.