[PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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(-)

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.

---
base-commit: 6f3d2b5299b0a8bcb8a9405a8d3fceb24f79c4f0
change-id: 20241127-clk-audio-fix-rst-missing-0b80628d934b

Best regards,
-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 742 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Stephen Boyd]

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).

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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

-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Stephen Boyd]

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?

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Stephen Boyd]

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?

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 551 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Jerome Brunet]

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.
  
-- 
Jerome

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Stephen Boyd]

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".

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Arnd Bergmann]

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

Re: [PATCH] clk: amlogic: axg-audio: select RESET_MESON_AUX

[Stephen Boyd]

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.