| Message ID | 20241127-clk-audio-fix-rst-missing-v1-1-9f9d0ab98fce@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| 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
Happy Thanksgiving! Quoting Arnd Bergmann (2024-11-28 07:34:46) > 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? I think the best approach is to add the reset auxilary device with a function that creates the auxiliary device directly by string name and does nothing else. Maybe we can have some helper in the auxiliary layer that does that all for us, because it's quite a bit of boiler plate that we need to write over and over again. Something like: int devm_auxiliary_device_create(struct device *parent, const char *name) that does the whole kzalloc() + ida dance that devm_meson_rst_aux_register() is doing today and wraps it all up so that the device is removed when the parent driver unbinds. Then this clk driver can register the reset device with a single call and not need to do anything besides select AUXILIARY_BUS. The regmap can be acquired from the parent device in the auxiliary driver probe with dev_get_regmap(adev->parent).
On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@kernel.org> wrote: > Happy Thanksgiving! > > Quoting Arnd Bergmann (2024-11-28 07:34:46) >> 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? > > I think the best approach is to add the reset auxilary device with a > function that creates the auxiliary device directly by string name and > does nothing else. Maybe we can have some helper in the auxiliary > layer that does that all for us, because it's quite a bit of boiler > plate that we need to write over and over again. Something like: > > int devm_auxiliary_device_create(struct device *parent, const char *name) > > that does the whole kzalloc() + ida dance that > devm_meson_rst_aux_register() is doing today and wraps it all up so that > the device is removed when the parent driver unbinds. Then this clk > driver can register the reset device with a single call and not need to > do anything besides select AUXILIARY_BUS. I think this is fairly close to what I proposed in the inital RFC, but generic instead of specific. I suspect the the generic path is likely to trigger more discussion. I'd like to be able to finish this migration, instead of leaving half finished like it is now. May I add back the boiler plate code in drivers/clk/meson, similar to what was proposed in the RFC [1] and propose the generic implementation in parallel ? It will just be a matter of switching when/if it is approved. [1] https://lore.kernel.org/r/20240516150842.705844-9-jbrunet@baylibre.com > The regmap can be acquired > from the parent device in the auxiliary driver probe with > dev_get_regmap(adev->parent). Did not think about that, I'll check, Thanks
On Tue, Dec 3, 2024, at 03:53, Stephen Boyd wrote: > Quoting Arnd Bergmann (2024-11-28 07:34:46) >> On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: >> 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? > > I think the best approach is to add the reset auxilary device with a > function that creates the auxiliary device directly by string name and > does nothing else. Maybe we can have some helper in the auxiliary > layer that does that all for us, because it's quite a bit of boiler > plate that we need to write over and over again. Something like: > > int devm_auxiliary_device_create(struct device *parent, const char *name) > > that does the whole kzalloc() + ida dance that > devm_meson_rst_aux_register() is doing today and wraps it all up so that > the device is removed when the parent driver unbinds. Then this clk > driver can register the reset device with a single call and not need to > do anything besides select AUXILIARY_BUS. The regmap can be acquired > from the parent device in the auxiliary driver probe with > dev_get_regmap(adev->parent). I like the idea. Two questions about the interface: - should there be a 'void *platform_data' argument anyway? Even if this can be looked up from the parent, it seems useful enough - What is the scope of the 'ida' number? My impression was this should be local to one parent device, but I don't know how the number is used in the end, so maybe a global number allocator is sufficient. Arnd
Quoting Jerome Brunet (2024-12-03 03:15:41) > On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@kernel.org> wrote: > > > > I think the best approach is to add the reset auxilary device with a > > function that creates the auxiliary device directly by string name and > > does nothing else. Maybe we can have some helper in the auxiliary > > layer that does that all for us, because it's quite a bit of boiler > > plate that we need to write over and over again. Something like: > > > > int devm_auxiliary_device_create(struct device *parent, const char *name) > > > > that does the whole kzalloc() + ida dance that > > devm_meson_rst_aux_register() is doing today and wraps it all up so that > > the device is removed when the parent driver unbinds. Then this clk > > driver can register the reset device with a single call and not need to > > do anything besides select AUXILIARY_BUS. > > I think this is fairly close to what I proposed in the inital RFC, but > generic instead of specific. Ok :-/ I've realized that we need this sort of approach in more places to logically split the device without making it SoC specific. It's only useful to have the registration API live in the driver when we need to call functions provided by that module from the driver registering the auxiliary device. > > I suspect the the generic path is likely to trigger more discussion. > I'd like to be able to finish this migration, instead of leaving half > finished like it is now. Is the half finished migration a problem for this cycle? I was intending to send the revert later this week and try again next cycle. > > May I add back the boiler plate code in drivers/clk/meson, similar to > what was proposed in the RFC [1] and propose the generic implementation > in parallel ? It will just be a matter of switching when/if it is approved. Sure. You can make devm_meson_clk_rst_aux_register() use the same signature as I proposed above so that it's a one line patch later. And definitely drop the imply RESET_MESON and depends on REGMAP part. Maybe you can put it in the clkc-utils file?
Quoting Arnd Bergmann (2024-12-03 04:43:03) > On Tue, Dec 3, 2024, at 03:53, Stephen Boyd wrote: > > Quoting Arnd Bergmann (2024-11-28 07:34:46) > >> On Thu, Nov 28, 2024, at 16:06, Jerome Brunet wrote: > >> 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? > > > > I think the best approach is to add the reset auxilary device with a > > function that creates the auxiliary device directly by string name and > > does nothing else. Maybe we can have some helper in the auxiliary > > layer that does that all for us, because it's quite a bit of boiler > > plate that we need to write over and over again. Something like: > > > > int devm_auxiliary_device_create(struct device *parent, const char *name) > > > > that does the whole kzalloc() + ida dance that > > devm_meson_rst_aux_register() is doing today and wraps it all up so that > > the device is removed when the parent driver unbinds. Then this clk > > driver can register the reset device with a single call and not need to > > do anything besides select AUXILIARY_BUS. The regmap can be acquired > > from the parent device in the auxiliary driver probe with > > dev_get_regmap(adev->parent). > > I like the idea. Two questions about the interface: > > - should there be a 'void *platform_data' argument anyway? > Even if this can be looked up from the parent, it seems > useful enough Sure. Probably that can be added as some variant like devm_auxiliary_device_create_pdata() when/if it's needed. > > - What is the scope of the 'ida' number? My impression was > this should be local to one parent device, but I don't > know how the number is used in the end, so maybe a global > number allocator is sufficient. > From what I can tell it's only used for the device name and not for driver matching. That's probably to make it so we don't get conflicts in sysfs with devices because they all share the same bus. I'd guess that a global allocator is sufficient.
On Tue, Dec 03, 2024 at 12:15:48PM -0800, Stephen Boyd wrote: > Quoting Jerome Brunet (2024-12-03 03:15:41) > > I suspect the the generic path is likely to trigger more discussion. > > I'd like to be able to finish this migration, instead of leaving half > > finished like it is now. > Is the half finished migration a problem for this cycle? I was intending > to send the revert later this week and try again next cycle. Yes, this patch was triggered by me reporting that multiple boards in my test farm have completely broken audio with defconfig. It's already a bit frustrating that -rc1 isn't working.
Quoting Mark Brown (2024-12-03 14:22:04) > On Tue, Dec 03, 2024 at 12:15:48PM -0800, Stephen Boyd wrote: > > Quoting Jerome Brunet (2024-12-03 03:15:41) > > > > I suspect the the generic path is likely to trigger more discussion. > > > I'd like to be able to finish this migration, instead of leaving half > > > finished like it is now. > > > Is the half finished migration a problem for this cycle? I was intending > > to send the revert later this week and try again next cycle. > > Yes, this patch was triggered by me reporting that multiple boards in my > test farm have completely broken audio with defconfig. I thought that commit 5ae1a43486fb ("clk: amlogic: axg-audio: revert reset implementation") was sufficient to fix the problem. Is that incorrect?
On Tue, Dec 03, 2024 at 02:32:56PM -0800, Stephen Boyd wrote: > Quoting Mark Brown (2024-12-03 14:22:04) > > Yes, this patch was triggered by me reporting that multiple boards in my > > test farm have completely broken audio with defconfig. > I thought that commit 5ae1a43486fb ("clk: amlogic: axg-audio: revert > reset implementation") was sufficient to fix the problem. Is that > incorrect? Yes, it should be - it didn't appear in mainline or -next yet and was a bit more buried in my mailbox than this thread but I see it's in now.
On Tue 03 Dec 2024 at 12:15, Stephen Boyd <sboyd@kernel.org> wrote: > Quoting Jerome Brunet (2024-12-03 03:15:41) >> On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@kernel.org> wrote: >> > >> > I think the best approach is to add the reset auxilary device with a >> > function that creates the auxiliary device directly by string name and >> > does nothing else. Maybe we can have some helper in the auxiliary >> > layer that does that all for us, because it's quite a bit of boiler >> > plate that we need to write over and over again. Something like: >> > >> > int devm_auxiliary_device_create(struct device *parent, const char *name) >> > >> > that does the whole kzalloc() + ida dance that >> > devm_meson_rst_aux_register() is doing today and wraps it all up so that >> > the device is removed when the parent driver unbinds. Then this clk >> > driver can register the reset device with a single call and not need to >> > do anything besides select AUXILIARY_BUS. >> >> I think this is fairly close to what I proposed in the inital RFC, but >> generic instead of specific. > > Ok :-/ I've realized that we need this sort of approach in more places > to logically split the device without making it SoC specific. It's only > useful to have the registration API live in the driver when we need to > call functions provided by that module from the driver registering the > auxiliary device. > >> >> I suspect the the generic path is likely to trigger more discussion. >> I'd like to be able to finish this migration, instead of leaving half >> finished like it is now. > > Is the half finished migration a problem for this cycle? I was intending > to send the revert later this week and try again next cycle. Not really, with the fix you applied. There is just code present in reset that should not be used in its current form. I'd prefer to sanitise the situation sooner rather than later. > >> >> May I add back the boiler plate code in drivers/clk/meson, similar to >> what was proposed in the RFC [1] and propose the generic implementation >> in parallel ? It will just be a matter of switching when/if it is approved. > > Sure. You can make devm_meson_clk_rst_aux_register() use the same > signature as I proposed above so that it's a one line patch later. And > definitely drop the imply RESET_MESON and depends on REGMAP part. Maybe > you can put it in the clkc-utils file? Sure. Few things I'd like to clarify * I tend think like Arnd, platform data will be needed eventually. Not sure having 2 functions, one with the param, one without is really worth it. We could just pass NULL when it is not needed. It is not uncommon. Would it be acceptable ? (for the generic helper, the temporary solution does not need that for sure) * You mean (meson-)clkc-utils.c ? I could but that would add dependency on the auxiliary_bus for clock controllers that don't need it. It is a minor problem really that I could just ignore. I'd rather keep this helper separate if possible. * Why drop 'imply RESET_MESON_AUX' ? I would still like the COMMON_CLK_AXG_AUDIO to 'strongly suggest' RESET_MESON_AUX, with dependency problem sorted out.
Quoting Jerome Brunet (2024-12-04 09:19:50) > On Tue 03 Dec 2024 at 12:15, Stephen Boyd <sboyd@kernel.org> wrote: > > > Quoting Jerome Brunet (2024-12-03 03:15:41) > >> On Mon 02 Dec 2024 at 18:53, Stephen Boyd <sboyd@kernel.org> wrote: > > > > Is the half finished migration a problem for this cycle? I was intending > > to send the revert later this week and try again next cycle. > > Not really, with the fix you applied. There is just code present in > reset that should not be used in its current form. I'd prefer to > sanitise the situation sooner rather than later. Alright. Let's just sort it out in the next few weeks for the next merge window then. Maybe you can just do it once then and get auxiliary bus maintainers to ack the patch so you can merge the helper locally and use it in the amlogic clk tree. > > > > Sure. You can make devm_meson_clk_rst_aux_register() use the same > > signature as I proposed above so that it's a one line patch later. And > > definitely drop the imply RESET_MESON and depends on REGMAP part. Maybe > > you can put it in the clkc-utils file? > > Sure. Few things I'd like to clarify > > * I tend think like Arnd, platform data will be needed eventually. Not > sure having 2 functions, one with the param, one without is really > worth it. We could just pass NULL when it is not needed. It is not > uncommon. Would it be acceptable ? (for the generic helper, the > temporary solution does not need that for sure) I'll defer to the maintainers there. I don't feel strongly. > > * You mean (meson-)clkc-utils.c ? I could but that would add dependency on > the auxiliary_bus for clock controllers that don't need it. It is a > minor problem really that I could just ignore. > I'd rather keep this helper separate if possible. Ok, no worries. > > * Why drop 'imply RESET_MESON_AUX' ? I would still like the > COMMON_CLK_AXG_AUDIO to 'strongly suggest' RESET_MESON_AUX, with > dependency problem sorted out. Because eventually you'll lose this Kconfig. I guess you'll want to add that to the driver Kconfig option to maintain "strongly suggested".
On Wed, Dec 4, 2024, at 18:19, Jerome Brunet wrote: > On Tue 03 Dec 2024 at 12:15, Stephen Boyd <sboyd@kernel.org> wrote: >>> >>> May I add back the boiler plate code in drivers/clk/meson, similar to >>> what was proposed in the RFC [1] and propose the generic implementation >>> in parallel ? It will just be a matter of switching when/if it is approved. >> >> Sure. You can make devm_meson_clk_rst_aux_register() use the same >> signature as I proposed above so that it's a one line patch later. And >> definitely drop the imply RESET_MESON and depends on REGMAP part. Maybe >> you can put it in the clkc-utils file? > * Why drop 'imply RESET_MESON_AUX' ? I would still like the > COMMON_CLK_AXG_AUDIO to 'strongly suggest' RESET_MESON_AUX, with > dependency problem sorted out. You can do it the other way round and use 'default COMMON_CLK_AXG_AUDIO' if you want to tie the two together with the same effect but avoid the ugly "imply" statement. I still think it's best to just leave it out. From a user perspective, the dependency isn't really that the clk driver needs the reset driver, but instead it's the audio driver that needs both. 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,