| Message ID | af10570000d7e103d70bbea590ce8df4f8902b67.1661330532.git.christophe.jaillet@wanadoo.fr (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | rtc: jz4740: Use devm_clk_get_enabled() helper | expand |
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 >
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
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
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
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
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 >
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 >> >
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,
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);
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(-)