| 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
On Wed 27 Nov 2024 at 22:23, "Arnd Bergmann" <arnd@arndb.de> wrote: > 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 clk, pinctrl or regulator are used by other driver yes, this one as well, sure. What special about this on is that it is an auxiliary bus driver. It is directly instantiated by another driver. That's where it differs. The axg-audio clock driver instantiate the auxiliary reset, it does not use it, which is why it makes sense for it to select the driver. I agree that in such case I should not have added prompt for that symbol. I'd be happy to fix that mistake in the coming cycle. > 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: Same consistency remark applies to the clock Kconfig patched here, which select the drivers they directly need and I'd like to keep consistency here too. Also 'default ARCH_MESON' does not accurately reflect when the driver is needed, it will turn on the driver in configuration where it is not necessarily needed, making it more difficult to trim the configuration down without intimate knowledge of the problem. ATM, RESET_MESON_AUX is only needed if COMMON_CLK_AXG_AUDIO is enabled. Isn't it what select is all about ? > > 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
On Thu, Nov 28, 2024, at 14:33, Jerome Brunet wrote: > On Wed 27 Nov 2024 at 22:23, "Arnd Bergmann" <arnd@arndb.de> wrote: >> 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 clk, pinctrl or regulator are used by other driver yes, this one as > well, sure. > > What special about this on is that it is an auxiliary bus driver. > It is directly instantiated by another driver. That's where > it differs. The axg-audio clock driver instantiate the auxiliary reset, > it does not use it, which is why it makes sense for it to select the > driver. Can you explain the logic behind this design? It seems that the entire problem here is the split into more drivers than it should be. It's common for clk drivers to also act as a reset driver, and my impression here is that you were trying too hard to split out the reset functionality into file in drivers/reset/ rather than to have it in drivers/clk/. Could you perhaps move the contents of drivers/reset/amlogic/reset-meson-aux.c into drivers/clk/meson/axg-audio.c, and change the exported symbol to a static function? This would still require a dependency on the exported meson_reset_toggle_ops, but that feels like a more natural interface here, since it's just a library module. Arnd
On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote: >> All clk, pinctrl or regulator are used by other driver yes, this one as >> well, sure. >> >> What special about this on is that it is an auxiliary bus driver. >> It is directly instantiated by another driver. That's where >> it differs. The axg-audio clock driver instantiate the auxiliary reset, >> it does not use it, which is why it makes sense for it to select the >> driver. > > Can you explain the logic behind this design? It seems that the > entire problem here is the split into more drivers than it > should be. It's common for clk drivers to also act as a > reset driver, and my impression here is that you were trying > too hard to split out the reset functionality into file > in drivers/reset/ rather than to have it in drivers/clk/. > > Could you perhaps move the contents of > drivers/reset/amlogic/reset-meson-aux.c into > drivers/clk/meson/axg-audio.c, and change the exported > symbol to a static function? This would still require > a dependency on the exported meson_reset_toggle_ops, > but that feels like a more natural interface here, > since it's just a library module. That's what we originally had. Reset implemented in clock. I was specically asked to move the reset part in reset using aux drivers. https://lore.kernel.org/r/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org Eventually that will happen for the rest of the reset implemented under drivers/clk/meson. It allows to make some code common between the platform reset drivers and the aux ones. It also ease maintainance for both Stephen and Philipp. > > Arnd
On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote: > On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote: > >>> All clk, pinctrl or regulator are used by other driver yes, this one as >>> well, sure. >>> >>> What special about this on is that it is an auxiliary bus driver. >>> It is directly instantiated by another driver. That's where >>> it differs. The axg-audio clock driver instantiate the auxiliary reset, >>> it does not use it, which is why it makes sense for it to select the >>> driver. >> >> Can you explain the logic behind this design? It seems that the >> entire problem here is the split into more drivers than it >> should be. It's common for clk drivers to also act as a >> reset driver, and my impression here is that you were trying >> too hard to split out the reset functionality into file >> in drivers/reset/ rather than to have it in drivers/clk/. >> >> Could you perhaps move the contents of >> drivers/reset/amlogic/reset-meson-aux.c into >> drivers/clk/meson/axg-audio.c, and change the exported >> symbol to a static function? This would still require >> a dependency on the exported meson_reset_toggle_ops, >> but that feels like a more natural interface here, >> since it's just a library module. > > That's what we originally had. Reset implemented in clock. > I was specically asked to move the reset part in reset using > aux drivers. > > https://lore.kernel.org/r/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org > > Eventually that will happen for the rest of the reset implemented > under drivers/clk/meson. > > It allows to make some code common between the platform reset > drivers and the aux ones. It also ease maintainance for both > Stephen and Philipp. I don't understand how this helps: the entire point of using an auxiliary device is to separate the lifetime rules of the different bits, but by doing the creation of the device in the same file as the implementation, you are not taking advantage of that at all, but instead get the complexity of a link-time dependency in addition to a lot of extra code for dealing with the additional device. Arnd
On Thu 28 Nov 2024 at 15:51, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote: >> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote: >> >>>> All clk, pinctrl or regulator are used by other driver yes, this one as >>>> well, sure. >>>> >>>> What special about this on is that it is an auxiliary bus driver. >>>> It is directly instantiated by another driver. That's where >>>> it differs. The axg-audio clock driver instantiate the auxiliary reset, >>>> it does not use it, which is why it makes sense for it to select the >>>> driver. >>> >>> Can you explain the logic behind this design? It seems that the >>> entire problem here is the split into more drivers than it >>> should be. It's common for clk drivers to also act as a >>> reset driver, and my impression here is that you were trying >>> too hard to split out the reset functionality into file >>> in drivers/reset/ rather than to have it in drivers/clk/. >>> >>> Could you perhaps move the contents of >>> drivers/reset/amlogic/reset-meson-aux.c into >>> drivers/clk/meson/axg-audio.c, and change the exported >>> symbol to a static function? This would still require >>> a dependency on the exported meson_reset_toggle_ops, >>> but that feels like a more natural interface here, >>> since it's just a library module. >> >> That's what we originally had. Reset implemented in clock. >> I was specically asked to move the reset part in reset using >> aux drivers. >> >> https://lore.kernel.org/r/e3a85852b911fdf16dd9ae158f42b3ef.sboyd@kernel.org >> >> Eventually that will happen for the rest of the reset implemented >> under drivers/clk/meson. >> >> It allows to make some code common between the platform reset >> drivers and the aux ones. It also ease maintainance for both >> Stephen and Philipp. > > I don't understand how this helps: the entire point of using > an auxiliary device is to separate the lifetime rules of > the different bits, but by doing the creation of the device > in the same file as the implementation, you are not taking > advantage of that at all, but instead get the complexity of > a link-time dependency in addition to a lot of extra code > for dealing with the additional device. My initial rework had the creation in clock (note: that is why I initially used 'imply', and forgot to update when the creation moved to reset). I was asked to move the creation in reset: https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org We are deviating a bit from the initial regression reported by Mark. Is Ok with you to proceed with that fix and then continue this discussion ? > > Arnd
On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: > On Thu 28 Nov 2024 at 15:51, "Arnd Bergmann" <arnd@arndb.de> wrote: >> On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote: >>> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote: >>> Eventually that will happen for the rest of the reset implemented >>> under drivers/clk/meson. >>> >>> It allows to make some code common between the platform reset >>> drivers and the aux ones. It also ease maintainance for both >>> Stephen and Philipp. >> >> I don't understand how this helps: the entire point of using >> an auxiliary device is to separate the lifetime rules of >> the different bits, but by doing the creation of the device >> in the same file as the implementation, you are not taking >> advantage of that at all, but instead get the complexity of >> a link-time dependency in addition to a lot of extra code >> for dealing with the additional device. > > My initial rework had the creation in clock (note: that is why I > initially used 'imply', and forgot to update when the creation moved to > reset). > > I was asked to move the creation in reset: > https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org > > We are deviating a bit from the initial regression reported by Mark. > Is Ok with you to proceed with that fix and then continue this discussion > ? I really don't want to see those stray 'select' statements in there, as that leave very little incentive for anyone to fix it properly. It sounds like Stephen gave you bad advice for how it should be structured, so my best suggestion would be to move the the problem (and the reset driver) back into his subsystem and leave only a simple 'select RESET_CONTROLLER'. From the message you cited, I think Stephen had the right intentions ("so that the clk and reset drivers are decoupled"), but the end result did not actually do what he intended even if you did what he asked for. Stephen, can you please take a look here and see if you have a better idea for either decoupling the two drivers enough to avoid the link time dependency, or to reintegrate the reset controller code into the clk driver and avoid the complexity? Arnd
On Thu, Nov 28, 2024 at 04:34:46PM +0100, Arnd Bergmann wrote: > On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: > > My initial rework had the creation in clock (note: that is why I > > initially used 'imply', and forgot to update when the creation moved to > > reset). > > > > I was asked to move the creation in reset: > > https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org > > > > We are deviating a bit from the initial regression reported by Mark. > > Is Ok with you to proceed with that fix and then continue this discussion > > ? > I really don't want to see those stray 'select' statements > in there, as that leave very little incentive for anyone to > fix it properly. One option would be to get a change in defconfig for -rc1 and then deal with moving things about later. I would very much like to have these systems working in my CI if possible.
On Thu 28 Nov 2024 at 16:34, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: >> On Thu 28 Nov 2024 at 15:51, "Arnd Bergmann" <arnd@arndb.de> wrote: >>> On Thu, Nov 28, 2024, at 15:39, Jerome Brunet wrote: >>>> On Thu 28 Nov 2024 at 15:11, "Arnd Bergmann" <arnd@arndb.de> wrote: >>>> Eventually that will happen for the rest of the reset implemented >>>> under drivers/clk/meson. >>>> >>>> It allows to make some code common between the platform reset >>>> drivers and the aux ones. It also ease maintainance for both >>>> Stephen and Philipp. >>> >>> I don't understand how this helps: the entire point of using >>> an auxiliary device is to separate the lifetime rules of >>> the different bits, but by doing the creation of the device >>> in the same file as the implementation, you are not taking >>> advantage of that at all, but instead get the complexity of >>> a link-time dependency in addition to a lot of extra code >>> for dealing with the additional device. >> >> My initial rework had the creation in clock (note: that is why I >> initially used 'imply', and forgot to update when the creation moved to >> reset). >> >> I was asked to move the creation in reset: >> https://lore.kernel.org/r/217a785212d7c1a5b504c6040b3636e6.sboyd@kernel.org >> >> We are deviating a bit from the initial regression reported by Mark. >> Is Ok with you to proceed with that fix and then continue this discussion >> ? > > I really don't want to see those stray 'select' statements > in there, as that leave very little incentive for anyone to > fix it properly. > > It sounds like Stephen gave you bad advice for how it should > be structured, so my best suggestion would be to move the > the problem (and the reset driver) back into his subsystem > and leave only a simple 'select RESET_CONTROLLER'. Okay, though I don't really understand why that select is okay and not the the proposed one. There is apparently a subtility I'm missing I'd like to avoid repeating that. > > From the message you cited, I think Stephen had the right > intentions ("so that the clk and reset drivers are decoupled"), > but the end result did not actually do what he intended > even if you did what he asked for. > > Stephen, can you please take a look here and see if you > have a better idea for either decoupling the two drivers > enough to avoid the link time dependency, or to reintegrate > the reset controller code into the clk driver and avoid > the complexity? If I may, * short term fix: revert both your fix and the initial clock change that needed fixing. That will essentially bring back the reset implementation in clock. * after that: remove the creation part from driver/reset and bring back something similar to what was proposed in the initial RFC for the creation and finally switch the clock driver back to it. That should provide the proper decoupling your are requesting I think. > > Arnd
On Thu, Nov 28, 2024, at 16:52, Mark Brown wrote: > On Thu, Nov 28, 2024 at 04:34:46PM +0100, Arnd Bergmann wrote: >> On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: >> > We are deviating a bit from the initial regression reported by Mark. >> > Is Ok with you to proceed with that fix and then continue this discussion >> > ? > >> I really don't want to see those stray 'select' statements >> in there, as that leave very little incentive for anyone to >> fix it properly. > > One option would be to get a change in defconfig for -rc1 and then deal > with moving things about later. I would very much like to have these That sounds ok to me. Arnd
On Thu 28 Nov 2024 at 16:57, "Arnd Bergmann" <arnd@arndb.de> wrote: > On Thu, Nov 28, 2024, at 16:52, Mark Brown wrote: >> On Thu, Nov 28, 2024 at 04:34:46PM +0100, Arnd Bergmann wrote: >>> On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: >>> > We are deviating a bit from the initial regression reported by Mark. >>> > Is Ok with you to proceed with that fix and then continue this discussion >>> > ? >> >>> I really don't want to see those stray 'select' statements >>> in there, as that leave very little incentive for anyone to >>> fix it properly. >> >> One option would be to get a change in defconfig for -rc1 and then deal >> with moving things about later. I would very much like to have these > > That sounds ok to me. With a change in the defconfig, the reset aux driver that needs fixing will start being used, whereas it won't be if a revert is applied Fixing the driver will be a lot simpler if it is not used yet. > > Arnd
On Thu, Nov 28, 2024, at 16:53, Jerome Brunet wrote: > On Thu 28 Nov 2024 at 16:34, "Arnd Bergmann" <arnd@arndb.de> wrote: >> On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: >>> We are deviating a bit from the initial regression reported by Mark. >>> Is Ok with you to proceed with that fix and then continue this discussion >>> ? >> >> I really don't want to see those stray 'select' statements >> in there, as that leave very little incentive for anyone to >> fix it properly. >> >> It sounds like Stephen gave you bad advice for how it should >> be structured, so my best suggestion would be to move the >> the problem (and the reset driver) back into his subsystem >> and leave only a simple 'select RESET_CONTROLLER'. > > Okay, though I don't really understand why that select is okay and not > the the proposed one. There is apparently a subtility I'm missing I'd > like to avoid repeating that. The thing with 'select' is that it really has to be used very selectively. The 'select RESET_CONTROLLER' is fine as an exception because there are already tons of clk drivers that do this consistently so they can register themselves as a reset controller. A driver selecting a driver from another subsystem is pretty much always a mistake. A single one may not cause much harm, but the problems are frequent enough that we need to have fewer of them rather than more. >> From the message you cited, I think Stephen had the right >> intentions ("so that the clk and reset drivers are decoupled"), >> but the end result did not actually do what he intended >> even if you did what he asked for. >> >> Stephen, can you please take a look here and see if you >> have a better idea for either decoupling the two drivers >> enough to avoid the link time dependency, or to reintegrate >> the reset controller code into the clk driver and avoid >> the complexity? > > If I may, > > * short term fix: revert both your fix and the initial clock > change that needed fixing. That will essentially bring back the reset > implementation in clock. > > * after that: remove the creation part from driver/reset and bring back > something similar to what was proposed in the initial RFC for the > creation and finally switch the clock driver back to it. > That should provide the proper decoupling your are requesting I think. Works for me as well, though Mark's suggestion would be simpler. 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,