| Message ID | 20241127-clk-audio-fix-rst-missing-v1-1-9f9d0ab98fce@baylibre.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Neil Armstrong |
| Headers | show |
| Series | clk: amlogic: axg-audio: select RESET_MESON_AUX | expand |
On Wed, Nov 27, 2024 at 07:47:46PM +0100, Jerome Brunet wrote: > Depending on RESET_MESON_AUX result in axg-audio support being turned > off by default for the users of arm64 defconfig, which is kind of a > regression for them. > Cc: Mark Brown <broonie@kernel.org> > Fixes: 681ed497d676 ("clk: amlogic: axg-audio: fix Kconfig dependency on RESET_MESON_AUX") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> Reviewed-by: Mark Brown <broonie@kernel.org> Reported-by: Mark Brown <broonie@kernel.org> (I reported this to Jerome on IRC) > --- > Hello Stephen, > This fixes a problem introduced in this merge window. > Could you please take it directly ? It'd be great to get this into -rc1, I've got some of the affected systems in CI.
On Wed, Nov 27, 2024, at 19:47, Jerome Brunet wrote: > Depending on RESET_MESON_AUX result in axg-audio support being turned > off by default for the users of arm64 defconfig, which is kind of a > regression for them. > > RESET_MESON_AUX is not in directly the defconfig, so depending on it turn > COMMON_CLK_AXG_AUDIO off. The clock provided by this module are > necessary for every axg audio devices. Those are now deferring. > > Select RESET_MESON_AUX rather than just depending on it. > With this, the audio subsystem of the affected platform should probe > correctly again > > Cc: Mark Brown <broonie@kernel.org> > Fixes: 681ed497d676 ("clk: amlogic: axg-audio: fix Kconfig dependency > on RESET_MESON_AUX") > Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> febb5d7348ff07c2da0cb5fd41d2ad2607e5bd5d..ea16bfde0df2d7bfebb041161f6b96bbb35003ed > 100644 > --- a/drivers/clk/meson/Kconfig > +++ b/drivers/clk/meson/Kconfig > @@ -106,7 +106,7 @@ config COMMON_CLK_AXG_AUDIO > select COMMON_CLK_MESON_SCLK_DIV > select COMMON_CLK_MESON_CLKC_UTILS > select REGMAP_MMIO > - depends on RESET_MESON_AUX > + select RESET_MESON_AUX > help > Support for the audio clock controller on AmLogic A113D devices, > aka axg, Say Y if you want audio subsystem to work. You should generally not 'select' a symbol from another subsystem, as this risks introducing dependency loops, and missing dependencies. It looks like RESET_MESON_AUX is a user-visible symbol, so you can simply ask users to turn it on, and add it to the defconfig. I also see some silliness going on in the include/soc/amlogic/reset-meson-aux.h, which has a non-working 'static inline' definition of the exported function. Before my fix, that would have caused the problem auf a non-working audio driver. Arnd
On Wed 27 Nov 2024 at 20:30, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Wed, Nov 27, 2024, at 19:47, Jerome Brunet wrote: >> Depending on RESET_MESON_AUX result in axg-audio support being turned >> off by default for the users of arm64 defconfig, which is kind of a >> regression for them. >> >> RESET_MESON_AUX is not in directly the defconfig, so depending on it turn >> COMMON_CLK_AXG_AUDIO off. The clock provided by this module are >> necessary for every axg audio devices. Those are now deferring. >> >> Select RESET_MESON_AUX rather than just depending on it. >> With this, the audio subsystem of the affected platform should probe >> correctly again >> >> Cc: Mark Brown <broonie@kernel.org> >> Fixes: 681ed497d676 ("clk: amlogic: axg-audio: fix Kconfig dependency >> on RESET_MESON_AUX") >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> > > > febb5d7348ff07c2da0cb5fd41d2ad2607e5bd5d..ea16bfde0df2d7bfebb041161f6b96bbb35003ed >> 100644 >> --- a/drivers/clk/meson/Kconfig >> +++ b/drivers/clk/meson/Kconfig >> @@ -106,7 +106,7 @@ config COMMON_CLK_AXG_AUDIO >> select COMMON_CLK_MESON_SCLK_DIV >> select COMMON_CLK_MESON_CLKC_UTILS >> select REGMAP_MMIO >> - depends on RESET_MESON_AUX >> + select RESET_MESON_AUX >> help >> Support for the audio clock controller on AmLogic A113D devices, >> aka axg, Say Y if you want audio subsystem to work. > > You should generally not 'select' a symbol from another > subsystem, as this risks introducing dependency loops, > and missing dependencies. I do understand that one needs to be careful with that sort of things but I don't think this is happening here. > > It looks like RESET_MESON_AUX is a user-visible symbol, > so you can simply ask users to turn it on, and add it to > the defconfig. That would work yes but It's really something a user should not be concerned with. I can follow-up with another change to remove the user visibilty of RESET_MESON_AUX. It is always going to be something requested by another driver. > > I also see some silliness going on in the > include/soc/amlogic/reset-meson-aux.h, which has a > non-working 'static inline' definition of the exported > function. Before my fix, that would have caused the > problem auf a non-working audio driver. If by 'silliness' you mean there is symbol definition for when RESET_MESON_AUX is disabled, indeed I guess that could go away. Thanks for pointing it out. > > Arnd
On Wed, Nov 27, 2024, at 21:56, Jerome Brunet wrote: > On Wed 27 Nov 2024 at 20:30, "Arnd Bergmann" <arnd@arndb.de> wrote: >> >> It looks like RESET_MESON_AUX is a user-visible symbol, >> so you can simply ask users to turn it on, and add it to >> the defconfig. > > That would work yes but It's really something a user should not be > concerned with. I can follow-up with another change to remove the user > visibilty of RESET_MESON_AUX. It is always going to be something > requested by another driver. But that's true for all reset drivers, each one of them is only useful because it's going to be used by another driver, same for clk, pinctrl, regulator, ... All other reset drivers are user-visible, with 'default, so for consistency I think it's best to keep it that way, and just add a 'default ARCH_MESON' the same way we have for many other reset drivers: diff --git a/drivers/reset/amlogic/Kconfig b/drivers/reset/amlogic/Kconfig index 3bee9fd60269..c02edc1b51aa 100644 --- a/drivers/reset/amlogic/Kconfig +++ b/drivers/reset/amlogic/Kconfig @@ -14,6 +14,7 @@ config RESET_MESON config RESET_MESON_AUX tristate "Meson Reset Auxiliary Driver" depends on ARCH_MESON || COMPILE_TEST + default ARCH_MESON select AUXILIARY_BUS select RESET_MESON_COMMON help The only bit that's special here is the exported symbol, but that is handled by the dependency. >> I also see some silliness going on in the >> include/soc/amlogic/reset-meson-aux.h, which has a >> non-working 'static inline' definition of the exported >> function. Before my fix, that would have caused the >> problem auf a non-working audio driver. > > If by 'silliness' you mean there is symbol definition for when > RESET_MESON_AUX is disabled, indeed I guess that could go away. Yes, that's what I meant. Arnd
diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig index febb5d7348ff07c2da0cb5fd41d2ad2607e5bd5d..ea16bfde0df2d7bfebb041161f6b96bbb35003ed 100644 --- a/drivers/clk/meson/Kconfig +++ b/drivers/clk/meson/Kconfig @@ -106,7 +106,7 @@ config COMMON_CLK_AXG_AUDIO select COMMON_CLK_MESON_SCLK_DIV select COMMON_CLK_MESON_CLKC_UTILS select REGMAP_MMIO - depends on RESET_MESON_AUX + select RESET_MESON_AUX help Support for the audio clock controller on AmLogic A113D devices, aka axg, Say Y if you want audio subsystem to work.
Depending on RESET_MESON_AUX result in axg-audio support being turned off by default for the users of arm64 defconfig, which is kind of a regression for them. RESET_MESON_AUX is not in directly the defconfig, so depending on it turn COMMON_CLK_AXG_AUDIO off. The clock provided by this module are necessary for every axg audio devices. Those are now deferring. Select RESET_MESON_AUX rather than just depending on it. With this, the audio subsystem of the affected platform should probe correctly again Cc: Mark Brown <broonie@kernel.org> Fixes: 681ed497d676 ("clk: amlogic: axg-audio: fix Kconfig dependency on RESET_MESON_AUX") Signed-off-by: Jerome Brunet <jbrunet@baylibre.com> --- Hello Stephen, This fixes a problem introduced in this merge window. Could you please take it directly ? Thanks --- drivers/clk/meson/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0 change-id: 20241127-clk-audio-fix-rst-missing-0b80628d934b Best regards,