linux-mips.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper
@ 2022-08-24  8:42 Christophe JAILLET
  2022-08-25 22:41 ` Paul Cercueil
  2022-10-13 21:31 ` [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Alexandre Belloni
  0 siblings, 2 replies; 9+ messages in thread
From: Christophe JAILLET @ 2022-08-24  8:42 UTC (permalink / raw)
  To: Paul Cercueil, Alessandro Zummo, Alexandre Belloni
  Cc: linux-kernel, kernel-janitors, Christophe JAILLET, linux-mips, linux-rtc

The devm_clk_get_enabled() helper:
   - calls devm_clk_get()
   - calls clk_prepare_enable() and registers what is needed in order to
     call clk_disable_unprepare() when needed, as a managed resource.

This simplifies the code, the error handling paths and avoid the need of
a dedicated function used with devm_add_action_or_reset().


As a side effect, some error messages are not logged anymore, so also use
dev_err_probe() instead of dev_err() in case of error.
At least the error code will be logged (and -EPROBE_DEFER will be filtered)

Based on my test with allyesconfig, this reduces the .o size from:
   text	   data	    bss	    dec	    hex	filename
   9025	   2488	    128	  11641	   2d79	drivers/rtc/rtc-jz4740.o
down to:
   8267	   2080	    128	  10475	   28eb	drivers/rtc/rtc-jz4740.o

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
devm_clk_get_enabled() is new and is part of 6.0-rc1

Some recent changed use "rtc: ingenic". This looks odd to me, so turn back
to "rtc: jz4740"
---
 drivers/rtc/rtc-jz4740.c | 25 +++----------------------
 1 file changed, 3 insertions(+), 22 deletions(-)

diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
index 6e51df72fd65..c383719292c7 100644
--- a/drivers/rtc/rtc-jz4740.c
+++ b/drivers/rtc/rtc-jz4740.c
@@ -257,11 +257,6 @@ static void jz4740_rtc_power_off(void)
 	kernel_halt();
 }
 
-static void jz4740_rtc_clk_disable(void *data)
-{
-	clk_disable_unprepare(data);
-}
-
 static const struct of_device_id jz4740_rtc_of_match[] = {
 	{ .compatible = "ingenic,jz4740-rtc", .data = (void *)ID_JZ4740 },
 	{ .compatible = "ingenic,jz4760-rtc", .data = (void *)ID_JZ4760 },
@@ -329,23 +324,9 @@ static int jz4740_rtc_probe(struct platform_device *pdev)
 	if (IS_ERR(rtc->base))
 		return PTR_ERR(rtc->base);
 
-	clk = devm_clk_get(dev, "rtc");
-	if (IS_ERR(clk)) {
-		dev_err(dev, "Failed to get RTC clock\n");
-		return PTR_ERR(clk);
-	}
-
-	ret = clk_prepare_enable(clk);
-	if (ret) {
-		dev_err(dev, "Failed to enable clock\n");
-		return ret;
-	}
-
-	ret = devm_add_action_or_reset(dev, jz4740_rtc_clk_disable, clk);
-	if (ret) {
-		dev_err(dev, "Failed to register devm action\n");
-		return ret;
-	}
+	clk = devm_clk_get_enabled(dev, "rtc");
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "Failed to get RTC clock\n");
 
 	spin_lock_init(&rtc->lock);
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper
  2022-08-24  8:42 [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Christophe JAILLET
@ 2022-08-25 22:41 ` Paul Cercueil
  2022-09-20  6:31   ` Usefulness of CONFIG_MACH_JZ47* H. Nikolaus Schaller
  2022-10-13 21:31 ` [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Alexandre Belloni
  1 sibling, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2022-08-25 22:41 UTC (permalink / raw)
  To: Christophe JAILLET
  Cc: Alessandro Zummo, Alexandre Belloni, linux-kernel,
	kernel-janitors, linux-mips, linux-rtc

Hi Christophe,

Le mer., août 24 2022 at 10:42:29 +0200, Christophe JAILLET 
<christophe.jaillet@wanadoo.fr> a écrit :
> The devm_clk_get_enabled() helper:
>    - calls devm_clk_get()
>    - calls clk_prepare_enable() and registers what is needed in order 
> to
>      call clk_disable_unprepare() when needed, as a managed resource.
> 
> This simplifies the code, the error handling paths and avoid the need 
> of
> a dedicated function used with devm_add_action_or_reset().
> 
> 
> As a side effect, some error messages are not logged anymore, so also 
> use
> dev_err_probe() instead of dev_err() in case of error.
> At least the error code will be logged (and -EPROBE_DEFER will be 
> filtered)
> 
> Based on my test with allyesconfig, this reduces the .o size from:
>    text	   data	    bss	    dec	    hex	filename
>    9025	   2488	    128	  11641	   2d79	drivers/rtc/rtc-jz4740.o
> down to:
>    8267	   2080	    128	  10475	   28eb	drivers/rtc/rtc-jz4740.o
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Acked-by: Paul Cercueil <paul@crapouillou.net>

> ---
> devm_clk_get_enabled() is new and is part of 6.0-rc1
> 
> Some recent changed use "rtc: ingenic". This looks odd to me, so turn 
> back
> to "rtc: jz4740"

Yes, the driver basically supports everything Ingenic now, so I 
generally write "rtc: ingenic:", but "rtc: jz4740:" is absolutely fine.

Cheers,
-Paul

> ---
>  drivers/rtc/rtc-jz4740.c | 25 +++----------------------
>  1 file changed, 3 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-jz4740.c b/drivers/rtc/rtc-jz4740.c
> index 6e51df72fd65..c383719292c7 100644
> --- a/drivers/rtc/rtc-jz4740.c
> +++ b/drivers/rtc/rtc-jz4740.c
> @@ -257,11 +257,6 @@ static void jz4740_rtc_power_off(void)
>  	kernel_halt();
>  }
> 
> -static void jz4740_rtc_clk_disable(void *data)
> -{
> -	clk_disable_unprepare(data);
> -}
> -
>  static const struct of_device_id jz4740_rtc_of_match[] = {
>  	{ .compatible = "ingenic,jz4740-rtc", .data = (void *)ID_JZ4740 },
>  	{ .compatible = "ingenic,jz4760-rtc", .data = (void *)ID_JZ4760 },
> @@ -329,23 +324,9 @@ static int jz4740_rtc_probe(struct 
> platform_device *pdev)
>  	if (IS_ERR(rtc->base))
>  		return PTR_ERR(rtc->base);
> 
> -	clk = devm_clk_get(dev, "rtc");
> -	if (IS_ERR(clk)) {
> -		dev_err(dev, "Failed to get RTC clock\n");
> -		return PTR_ERR(clk);
> -	}
> -
> -	ret = clk_prepare_enable(clk);
> -	if (ret) {
> -		dev_err(dev, "Failed to enable clock\n");
> -		return ret;
> -	}
> -
> -	ret = devm_add_action_or_reset(dev, jz4740_rtc_clk_disable, clk);
> -	if (ret) {
> -		dev_err(dev, "Failed to register devm action\n");
> -		return ret;
> -	}
> +	clk = devm_clk_get_enabled(dev, "rtc");
> +	if (IS_ERR(clk))
> +		return dev_err_probe(dev, PTR_ERR(clk), "Failed to get RTC 
> clock\n");
> 
>  	spin_lock_init(&rtc->lock);
> 
> --
> 2.34.1
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Usefulness of CONFIG_MACH_JZ47*
  2022-08-25 22:41 ` Paul Cercueil
@ 2022-09-20  6:31   ` H. Nikolaus Schaller
  2022-09-20  9:09     ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: H. Nikolaus Schaller @ 2022-09-20  6:31 UTC (permalink / raw)
  To: Paul Cercueil; +Cc: linux Kernel Mailing List, linux-mips

Hi Paul,
it seems as if there aren't many places left over where the MACH_JZ47* configs are still in use:

drivers/char/hw_ramdom/Kconfig
drivers/clk/ingenic/Kconfig
drivers/gpu/drm/ingenic/Kconfig
drivers/pinctrl/pinctrl-ingenic.c

Is it possible to get rid of them and just have CONFIG_MACH_INGENIC_GENERIC?

This might simplify my defconfig for multiple machines.

BR and thanks,
Nikolaus


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Usefulness of CONFIG_MACH_JZ47*
  2022-09-20  6:31   ` Usefulness of CONFIG_MACH_JZ47* H. Nikolaus Schaller
@ 2022-09-20  9:09     ` Paul Cercueil
  2022-09-20 12:31       ` H. Nikolaus Schaller
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2022-09-20  9:09 UTC (permalink / raw)
  To: H. Nikolaus Schaller; +Cc: linux Kernel Mailing List, linux-mips

Hi Nikolaus,

Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> it seems as if there aren't many places left over where the 
> MACH_JZ47* configs are still in use:
> 
> drivers/char/hw_ramdom/Kconfig
> drivers/clk/ingenic/Kconfig
> drivers/gpu/drm/ingenic/Kconfig
> drivers/pinctrl/pinctrl-ingenic.c
> 
> Is it possible to get rid of them and just have 
> CONFIG_MACH_INGENIC_GENERIC?
> 
> This might simplify my defconfig for multiple machines.

CONFIG_MIPS_GENERIC_KERNEL=y
CONFIG_BOARD_INGENIC=y

Then you can support all Ingenic-based boards alongside other MIPS 
boards.

> BR and thanks,
> Nikolaus

Cheers,
-Paul



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Usefulness of CONFIG_MACH_JZ47*
  2022-09-20  9:09     ` Paul Cercueil
@ 2022-09-20 12:31       ` H. Nikolaus Schaller
  2022-09-20 13:33         ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: H. Nikolaus Schaller @ 2022-09-20 12:31 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: linux Kernel Mailing List, linux-mips, MIPS Creator CI20 Development

Hi Paul,

> Am 20.09.2022 um 11:09 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> Hi Nikolaus,
> 
> Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>> it seems as if there aren't many places left over where the MACH_JZ47* configs are still in use:
>> drivers/char/hw_ramdom/Kconfig
>> drivers/clk/ingenic/Kconfig
>> drivers/gpu/drm/ingenic/Kconfig
>> drivers/pinctrl/pinctrl-ingenic.c
>> Is it possible to get rid of them and just have CONFIG_MACH_INGENIC_GENERIC?
>> This might simplify my defconfig for multiple machines.
> 
> CONFIG_MIPS_GENERIC_KERNEL=y

This breaks compilation for me, e.g.

arch/mips/mm/cache.c:203:6: error: 'cpu_has_tx39_cache' undeclared (first use in this function)

> CONFIG_BOARD_INGENIC=y

This config option does not exist (at least in v6.0-rc). Probably you refer to CONFIG_INGENIC_GENERIC_BOARD.

As far as I see, this does not choose to build any device tree blob.

I tried some patch to get the .dtb built, but the resulting kernel does not show any activity.

If I e.g. switch back from CONFIG_INGENIC_GENERIC_BOARD=y to CONFIG_JZ4780_CI20=y the kernel works.

> 
> Then you can support all Ingenic-based boards alongside other MIPS boards.

Yes, I know, but why are the MACH_JZ47* not replaced by CONFIG_MACH_INGENIC_GENERIC if they are almost unused or completely removed?

BTW: there are also seems to be some board specific CONFIGs in processor specific code (e.g. CONFIG_JZ4780_CI20 in irqchip code).
So selecting a MACH is not sufficient to get these features.

All this looks a little fragile and incomplete... Maybe if I find some time (which is unfortunately quite unlikely) I can propose some fixes.

BR,
Nikolaus


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Usefulness of CONFIG_MACH_JZ47*
  2022-09-20 12:31       ` H. Nikolaus Schaller
@ 2022-09-20 13:33         ` Paul Cercueil
  2022-09-20 14:19           ` H. Nikolaus Schaller
  0 siblings, 1 reply; 9+ messages in thread
From: Paul Cercueil @ 2022-09-20 13:33 UTC (permalink / raw)
  To: H. Nikolaus Schaller
  Cc: linux Kernel Mailing List, linux-mips, MIPS Creator CI20 Development



Le mar., sept. 20 2022 at 14:31:38 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 20.09.2022 um 11:09 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>>  Hi Nikolaus,
>> 
>>  Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  Hi Paul,
>>>  it seems as if there aren't many places left over where the 
>>> MACH_JZ47* configs are still in use:
>>>  drivers/char/hw_ramdom/Kconfig
>>>  drivers/clk/ingenic/Kconfig
>>>  drivers/gpu/drm/ingenic/Kconfig
>>>  drivers/pinctrl/pinctrl-ingenic.c
>>>  Is it possible to get rid of them and just have 
>>> CONFIG_MACH_INGENIC_GENERIC?
>>>  This might simplify my defconfig for multiple machines.
>> 
>>  CONFIG_MIPS_GENERIC_KERNEL=y
> 
> This breaks compilation for me, e.g.
> 
> arch/mips/mm/cache.c:203:6: error: 'cpu_has_tx39_cache' undeclared 
> (first use in this function)

v6.0-rc does not have 'cpu_has_tx39_cache' anywhere in that file, or in 
arch/mips/ for that matter. It was removed in v5.18.

And a v5.17 kernel compiles fine here with these options enabled. So 
it's a problem on your side, I guess.

>>  CONFIG_BOARD_INGENIC=y
> 
> This config option does not exist (at least in v6.0-rc). Probably you 
> refer to CONFIG_INGENIC_GENERIC_BOARD.

No, I do not, and yes, it exists.

> As far as I see, this does not choose to build any device tree blob.
> 
> I tried some patch to get the .dtb built, but the resulting kernel 
> does not show any activity.
> 
> If I e.g. switch back from CONFIG_INGENIC_GENERIC_BOARD=y to 
> CONFIG_JZ4780_CI20=y the kernel works.

Because in the first case you build a generic kernel, which does not 
embed any .dtb, and you are responsible for providing the kernel with 
the blob at boot time; while if you build with CONFIG_JZ4780_CI20=y it 
embeds the .dtb inside the kernel.

You can embed the .dtb into the generic kernel at compile-time too, 
have a look at "Kernel type -> Kernel appended dtb support." Not sure 
why you'd want that for a generic kernel, though.

>> 
>>  Then you can support all Ingenic-based boards alongside other MIPS 
>> boards.
> 
> Yes, I know, but why are the MACH_JZ47* not replaced by 
> CONFIG_MACH_INGENIC_GENERIC if they are almost unused or completely 
> removed?

They *are* used.

> BTW: there are also seems to be some board specific CONFIGs in 
> processor specific code (e.g. CONFIG_JZ4780_CI20 in irqchip code).

rgrep CI20 drivers/irqchip/

returns nothing for me.

> So selecting a MACH is not sufficient to get these features.
> 
> All this looks a little fragile and incomplete... Maybe if I find 
> some time (which is unfortunately quite unlikely) I can propose some 
> fixes.

It is not "fragile and incomplete", it works as intended, and it's a 
feature I use often.

-Paul



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Usefulness of CONFIG_MACH_JZ47*
  2022-09-20 13:33         ` Paul Cercueil
@ 2022-09-20 14:19           ` H. Nikolaus Schaller
  2022-09-20 14:46             ` Paul Cercueil
  0 siblings, 1 reply; 9+ messages in thread
From: H. Nikolaus Schaller @ 2022-09-20 14:19 UTC (permalink / raw)
  To: Paul Cercueil
  Cc: linux Kernel Mailing List, linux-mips, MIPS Creator CI20 Development

Hi Paul,

> Am 20.09.2022 um 15:33 schrieb Paul Cercueil <paul@crapouillou.net>:
> 
> 
> 
> Le mar., sept. 20 2022 at 14:31:38 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>> Hi Paul,
>>> Am 20.09.2022 um 11:09 schrieb Paul Cercueil <paul@crapouillou.net>:
>>> Hi Nikolaus,
>>> Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller <hns@goldelico.com> a écrit :
>>>> Hi Paul,
>>>> it seems as if there aren't many places left over where the MACH_JZ47* configs are still in use:
>>>> drivers/char/hw_ramdom/Kconfig
>>>> drivers/clk/ingenic/Kconfig
>>>> drivers/gpu/drm/ingenic/Kconfig
>>>> drivers/pinctrl/pinctrl-ingenic.c
>>>> Is it possible to get rid of them and just have CONFIG_MACH_INGENIC_GENERIC?
>>>> This might simplify my defconfig for multiple machines.
>>> CONFIG_MIPS_GENERIC_KERNEL=y
>> This breaks compilation for me, e.g.
>> arch/mips/mm/cache.c:203:6: error: 'cpu_has_tx39_cache' undeclared (first use in this function)
> 
> v6.0-rc does not have 'cpu_has_tx39_cache' anywhere in that file, or in arch/mips/ for that matter. It was removed in v5.18.
> 
> And a v5.17 kernel compiles fine here with these options enabled. So it's a problem on your side, I guess.

Ah, you were right.

I have a patch included which was provided by zhouyanjie@wanyeetech.com
("MIPS: mm: Add Ingenic XBurst SoCs specific cache driver.").

It is intended to improve caching and is part of jz4780 SMP support which we wanted to have.
AFAIR it was either not posted to lkml or rejected or superseded.

> 
>>> CONFIG_BOARD_INGENIC=y
>> This config option does not exist (at least in v6.0-rc). Probably you refer to CONFIG_INGENIC_GENERIC_BOARD.
> 
> No, I do not, and yes, it exists.

Ah, I grepped for CONFIG_BOARD_INGENIC but it exists only in one Kconfig as BOARD_INGENIC.
But what is then the difference to CONFIG_INGENIC_GENERIC_BOARD and CONFIG_MACH_INGENIC_GENERIC?

> 
>> As far as I see, this does not choose to build any device tree blob.
>> I tried some patch to get the .dtb built, but the resulting kernel does not show any activity.
>> If I e.g. switch back from CONFIG_INGENIC_GENERIC_BOARD=y to CONFIG_JZ4780_CI20=y the kernel works.
> 
> Because in the first case you build a generic kernel, which does not embed any .dtb, and you are responsible for providing the kernel with the blob at boot time; while if you build with CONFIG_JZ4780_CI20=y it embeds the .dtb inside the kernel.
> 
> You can embed the .dtb into the generic kernel at compile-time too, have a look at "Kernel type -> Kernel appended dtb support." Not sure why you'd want that for a generic kernel, though.

Ah, I remember. Since I usually code 99% of my time for ARM systems with separate .dtb files chosen by the boot loader, I forgot that we have to append the .dtb on the CI20 and Alpha400. So there is no good solution for a "universal" kernel binary either.

> 
>>> Then you can support all Ingenic-based boards alongside other MIPS boards.
>> Yes, I know, but why are the MACH_JZ47* not replaced by CONFIG_MACH_INGENIC_GENERIC if they are almost unused or completely removed?
> 
> They *are* used.

Well, only in a handful of places as it looks like.

> 
>> BTW: there are also seems to be some board specific CONFIGs in processor specific code (e.g. CONFIG_JZ4780_CI20 in irqchip code).
> 
> rgrep CI20 drivers/irqchip/
> 
> returns nothing for me.

Ah, again an inofficial patch which is part of the SMP stuff ("irqchip: Ingenic: Add percpu IRQ support for X2000.").

> 
>> So selecting a MACH is not sufficient to get these features.
>> All this looks a little fragile and incomplete... Maybe if I find some time (which is unfortunately quite unlikely) I can propose some fixes.
> 
> It is not "fragile and incomplete", it works as intended, and it's a feature I use often.

Yes, seems as if you are right. We may have added too many useful patches which did not go upstreamand get in conflict with upstream features.

BR and thanks for helping to better understand,
Nikolaus

> 
> -Paul
> 


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Usefulness of CONFIG_MACH_JZ47*
  2022-09-20 14:19           ` H. Nikolaus Schaller
@ 2022-09-20 14:46             ` Paul Cercueil
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Cercueil @ 2022-09-20 14:46 UTC (permalink / raw)
  To: H. Nikolaus Schaller
  Cc: linux Kernel Mailing List, linux-mips, MIPS Creator CI20 Development



Le mar., sept. 20 2022 at 16:19:41 +0200, H. Nikolaus Schaller 
<hns@goldelico.com> a écrit :
> Hi Paul,
> 
>>  Am 20.09.2022 um 15:33 schrieb Paul Cercueil <paul@crapouillou.net>:
>> 
>> 
>> 
>>  Le mar., sept. 20 2022 at 14:31:38 +0200, H. Nikolaus Schaller 
>> <hns@goldelico.com> a écrit :
>>>  Hi Paul,
>>>>  Am 20.09.2022 um 11:09 schrieb Paul Cercueil 
>>>> <paul@crapouillou.net>:
>>>>  Hi Nikolaus,
>>>>  Le mar., sept. 20 2022 at 08:31:30 +0200, H. Nikolaus Schaller 
>>>> <hns@goldelico.com> a écrit :
>>>>>  Hi Paul,
>>>>>  it seems as if there aren't many places left over where the 
>>>>> MACH_JZ47* configs are still in use:
>>>>>  drivers/char/hw_ramdom/Kconfig
>>>>>  drivers/clk/ingenic/Kconfig
>>>>>  drivers/gpu/drm/ingenic/Kconfig
>>>>>  drivers/pinctrl/pinctrl-ingenic.c
>>>>>  Is it possible to get rid of them and just have 
>>>>> CONFIG_MACH_INGENIC_GENERIC?
>>>>>  This might simplify my defconfig for multiple machines.
>>>>  CONFIG_MIPS_GENERIC_KERNEL=y
>>>  This breaks compilation for me, e.g.
>>>  arch/mips/mm/cache.c:203:6: error: 'cpu_has_tx39_cache' undeclared 
>>> (first use in this function)
>> 
>>  v6.0-rc does not have 'cpu_has_tx39_cache' anywhere in that file, 
>> or in arch/mips/ for that matter. It was removed in v5.18.
>> 
>>  And a v5.17 kernel compiles fine here with these options enabled. 
>> So it's a problem on your side, I guess.
> 
> Ah, you were right.
> 
> I have a patch included which was provided by 
> zhouyanjie@wanyeetech.com
> ("MIPS: mm: Add Ingenic XBurst SoCs specific cache driver.").
> 
> It is intended to improve caching and is part of jz4780 SMP support 
> which we wanted to have.
> AFAIR it was either not posted to lkml or rejected or superseded.
> 
>> 
>>>>  CONFIG_BOARD_INGENIC=y
>>>  This config option does not exist (at least in v6.0-rc). Probably 
>>> you refer to CONFIG_INGENIC_GENERIC_BOARD.
>> 
>>  No, I do not, and yes, it exists.
> 
> Ah, I grepped for CONFIG_BOARD_INGENIC but it exists only in one 
> Kconfig as BOARD_INGENIC.
> But what is then the difference to CONFIG_INGENIC_GENERIC_BOARD and 
> CONFIG_MACH_INGENIC_GENERIC?

CONFIG_BOARD_INGENIC=y means you are building a generic MIPS kernel 
that happens to support Ingenic boards. In this case, the kernel has a 
lot of features enabled that might not be useful on specific Ingenic 
boards, because the kernel needs to support other SoCs. This option 
selects CONFIG_MACH_INGENIC_GENERIC=y, which in turn selects 
MACH_INGENIC=y and every MACH_JZ* and MACH_X* there is.

If you instead set MACH_INGENIC_SOC=y as your "Machine Selection -> 
System Type", this means your kernel will run specifically on Ingenic 
SoCs and isn't expected to work anywhere else. By default the "Machine 
Type" will be CONFIG_INGENIC_GENERIC_BOARD=y aka. "Generic board", 
which can be used to support any board with an Ingenic SoC. This option 
also selects CONFIG_MACH_INGENIC_GENERIC=y. If you choose a different 
"machine type", for instance JZ4780_CI20=y, then only MACH_JZ4780=y 
will be selected, and all the code unrelated to the JZ4780 SoC will be 
disabled.

Cheers,
-Paul

> 
>> 
>>>  As far as I see, this does not choose to build any device tree 
>>> blob.
>>>  I tried some patch to get the .dtb built, but the resulting kernel 
>>> does not show any activity.
>>>  If I e.g. switch back from CONFIG_INGENIC_GENERIC_BOARD=y to 
>>> CONFIG_JZ4780_CI20=y the kernel works.
>> 
>>  Because in the first case you build a generic kernel, which does 
>> not embed any .dtb, and you are responsible for providing the kernel 
>> with the blob at boot time; while if you build with 
>> CONFIG_JZ4780_CI20=y it embeds the .dtb inside the kernel.
>> 
>>  You can embed the .dtb into the generic kernel at compile-time too, 
>> have a look at "Kernel type -> Kernel appended dtb support." Not 
>> sure why you'd want that for a generic kernel, though.
> 
> Ah, I remember. Since I usually code 99% of my time for ARM systems 
> with separate .dtb files chosen by the boot loader, I forgot that we 
> have to append the .dtb on the CI20 and Alpha400. So there is no good 
> solution for a "universal" kernel binary either.
> 
>> 
>>>>  Then you can support all Ingenic-based boards alongside other 
>>>> MIPS boards.
>>>  Yes, I know, but why are the MACH_JZ47* not replaced by 
>>> CONFIG_MACH_INGENIC_GENERIC if they are almost unused or completely 
>>> removed?
>> 
>>  They *are* used.
> 
> Well, only in a handful of places as it looks like.
> 
>> 
>>>  BTW: there are also seems to be some board specific CONFIGs in 
>>> processor specific code (e.g. CONFIG_JZ4780_CI20 in irqchip code).
>> 
>>  rgrep CI20 drivers/irqchip/
>> 
>>  returns nothing for me.
> 
> Ah, again an inofficial patch which is part of the SMP stuff 
> ("irqchip: Ingenic: Add percpu IRQ support for X2000.").
> 
>> 
>>>  So selecting a MACH is not sufficient to get these features.
>>>  All this looks a little fragile and incomplete... Maybe if I find 
>>> some time (which is unfortunately quite unlikely) I can propose 
>>> some fixes.
>> 
>>  It is not "fragile and incomplete", it works as intended, and it's 
>> a feature I use often.
> 
> Yes, seems as if you are right. We may have added too many useful 
> patches which did not go upstreamand get in conflict with upstream 
> features.
> 
> BR and thanks for helping to better understand,
> Nikolaus
> 
>> 
>>  -Paul
>> 
> 



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper
  2022-08-24  8:42 [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Christophe JAILLET
  2022-08-25 22:41 ` Paul Cercueil
@ 2022-10-13 21:31 ` Alexandre Belloni
  1 sibling, 0 replies; 9+ messages in thread
From: Alexandre Belloni @ 2022-10-13 21:31 UTC (permalink / raw)
  To: Alessandro Zummo, Christophe JAILLET, Paul Cercueil
  Cc: linux-kernel, linux-rtc, kernel-janitors, linux-mips

On Wed, 24 Aug 2022 10:42:29 +0200, Christophe JAILLET wrote:
> The devm_clk_get_enabled() helper:
>    - calls devm_clk_get()
>    - calls clk_prepare_enable() and registers what is needed in order to
>      call clk_disable_unprepare() when needed, as a managed resource.
> 
> This simplifies the code, the error handling paths and avoid the need of
> a dedicated function used with devm_add_action_or_reset().
> 
> [...]

Applied, thanks!

[1/1] rtc: jz4740: Use devm_clk_get_enabled() helper
      commit: 94e4603d1a262f8d79f6186d0df0379243613b95

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-10-13 21:32 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-24  8:42 [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Christophe JAILLET
2022-08-25 22:41 ` Paul Cercueil
2022-09-20  6:31   ` Usefulness of CONFIG_MACH_JZ47* H. Nikolaus Schaller
2022-09-20  9:09     ` Paul Cercueil
2022-09-20 12:31       ` H. Nikolaus Schaller
2022-09-20 13:33         ` Paul Cercueil
2022-09-20 14:19           ` H. Nikolaus Schaller
2022-09-20 14:46             ` Paul Cercueil
2022-10-13 21:31 ` [PATCH] rtc: jz4740: Use devm_clk_get_enabled() helper Alexandre Belloni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).