diff mbox series

clk: amlogic: axg-audio: select RESET_MESON_AUX

Message ID 20241127-clk-audio-fix-rst-missing-v1-1-9f9d0ab98fce@baylibre.com (mailing list archive)
State New
Headers show
Series clk: amlogic: axg-audio: select RESET_MESON_AUX | expand

Commit Message

Jerome Brunet Nov. 27, 2024, 6:47 p.m. UTC
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,

Comments

Mark Brown Nov. 27, 2024, 7 p.m. UTC | #1
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.
Arnd Bergmann Nov. 27, 2024, 7:30 p.m. UTC | #2
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
Jerome Brunet Nov. 27, 2024, 8:56 p.m. UTC | #3
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
Arnd Bergmann Nov. 27, 2024, 9:23 p.m. UTC | #4
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
Jerome Brunet Nov. 28, 2024, 1:33 p.m. UTC | #5
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
Arnd Bergmann Nov. 28, 2024, 2:11 p.m. UTC | #6
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
Jerome Brunet Nov. 28, 2024, 2:39 p.m. UTC | #7
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
Arnd Bergmann Nov. 28, 2024, 2:51 p.m. UTC | #8
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
Jerome Brunet Nov. 28, 2024, 3:06 p.m. UTC | #9
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
Arnd Bergmann Nov. 28, 2024, 3:34 p.m. UTC | #10
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
Mark Brown Nov. 28, 2024, 3:52 p.m. UTC | #11
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.
Jerome Brunet Nov. 28, 2024, 3:53 p.m. UTC | #12
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
Arnd Bergmann Nov. 28, 2024, 3:57 p.m. UTC | #13
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
diff mbox series

Patch

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.