Re: [PATCH] watchdog: ixp4xx: Use devm_clk_get_enabled() helper

[Christophe JAILLET <christophe.jaillet@wanadoo.fr>]

Le 01/01/2023 à 16:07, Guenter Roeck a écrit :
> On Sun, Jan 01, 2023 at 10:35:59AM +0100, Christophe JAILLET wrote:
>> Le 01/01/2023 à 00:14, Guenter Roeck a écrit :
>>> On Sat, Dec 31, 2022 at 12:07:27PM +0100, 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 and avoids the need of a dedicated function used
>>>> with devm_add_action_or_reset().
>>>>
>>>> Signed-off-by: Christophe JAILLET <christophe.jaillet-39ZsbGIQGT5GWvitb5QawA@public.gmane.org>
>>>> ---
>>>> Note that I get a compilation error because read_cpuid_id() is not defined
>>>> on my system (x86_64).
>>>> So I think that a "depends on ARM<something>" in missing in a KConfig file.
>>>
>>> It has
>>>
>>> 	depends on ARCH_IXP4XX
>>>
>>> and CONFIG_IXP4XX_WATCHDOG is not set for me after "make allmodconfig".
>>
>> Here is what  do.
>>
>> make allmodconfig
>> make -j8 drivers/watchdog/ixp4xx_wdt.o
>>
>> And I get:
>>    DESCEND objtool
>>    CALL    scripts/checksyscalls.sh
>>    CC      drivers/watchdog/ixp4xx_wdt.o
>> drivers/watchdog/ixp4xx_wdt.c: In function ‘ixp4xx_wdt_probe’:
>> drivers/watchdog/ixp4xx_wdt.c:122:15: error: implicit declaration of
>> function ‘read_cpuid_id’ [-Werror=implicit-function-declaration]
>>    122 |         if (!(read_cpuid_id() & 0xf) && !cpu_is_ixp46x()) {
>>        |               ^~~~~~~~~~~~~
>> cc1: all warnings being treated as errors
>> make[3]: *** [scripts/Makefile.build:252 : drivers/watchdog/ixp4xx_wdt.o]
>> Erreur 1
>> make[2]: *** [scripts/Makefile.build:504 : drivers/watchdog] Erreur 2
>> make[1]: *** [scripts/Makefile.build:504 : drivers] Erreur 2
>> make: *** [Makefile:2011 : .] Erreur 2
>>
>>
>> I do agree with you that:
>>
>>     - Kconfig looks fine
>> config IXP4XX_WATCHDOG
>> 	tristate "IXP4xx Watchdog"
>> 	depends on ARCH_IXP4XX
>>
>>    - Makefile looks fine
>> obj-$(CONFIG_IXP4XX_WATCHDOG) += ixp4xx_wdt.o
>>
>>    - .config looks fine
>> IXP4XX_WATCHDOG is NOT defined
>>
>>    - make drivers/watchdog/ looks fine
>> No error and ixp4xx_wdt.o is NOT built.
>>
>>
>> However, in the past (if I recollect correctly :) ), a "make <something_that
>> depends_on_a_config_variable_that_is_not_defined>" returned an error stating
>> that no rule existed to build the specified target.
>>
> 
> This is not correct. It only applies if the target directory Makefile is
> excluded by the make flags, or possibly if the target file is a complex
> one build from various source files.
> 
>> I sometimes needed to tweak the Kconfig files to force some compilation when
>> I didn't have the right tool chain or configuration.
>> It was maybe not the best practice, but it worked most of the time.
>>
>>
>> Now, with the example above, such a compilation attempt is possible. It is
>> maybe normal (because of a change somewhere in the way the kernel is built,
>> because of an updated toolchain on my machine, ...)
>> This is just fine for me, but looked really surprising.
>>
>> That is why I first thought that something was missing in a Kconfig file.
>>
>>
>> So my comments are just a surprise to me to something that seems not to
>> behave the same as before.
>>
> I don't think anything changed. It always worked like that for me.
> I would suggest to go back to an older kernel and try it there.
> You'll see exactly the same error. Maybe you just never encountered
> a file like that.

git reset --hard next-20210111			(randomly chosen date)
make allmodconfig
make clean
make -j7 drivers/watchdog/ixp4xx_wdt.o		(too build most of the needed 
stuff to build a kernel)
touch drivers/watchdog/ixp4xx_wdt.c
make -j7 drivers/watchdog/ixp4xx_wdt.o		(too build this file only)

   DESCEND  objtool
   CALL    scripts/atomic/check-atomics.sh
   CALL    scripts/checksyscalls.sh
make[3]: *** Aucune règle pour fabriquer la cible « 
drivers/watchdog/ixp4xx_wdt.o ». Arrêt.
make[2]: *** [scripts/Makefile.build:471 : __build] Erreur 2
make[1]: *** [scripts/Makefile.build:496 : drivers/watchdog] Erreur 2
make: *** [Makefile:1805 : drivers] Erreur 2


This is what I had in mind.
(Aucune règle pour fabriquer la cible... = no rule to build...)


So something changed somewhere. Maybe in the way Makefile are now 
included or not in the build process, as you suggest above.

> 
> So, in other words, what you should have said is "not compile tested".
> Alternatively, you could install cross compilers and compile test the
> patches with those.
>

No. The patches HAVE been cross compiled after my initial attempt with 
my default x86_64 failing built.


This one was successfully built using:
   make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- allmodconfig
   make ARCH=arm CROSS_COMPILE=/usr/bin/arm-linux-gnueabi- -j7 
drivers/watchdog/ixp4xx_wdt.o


The other one was successfully built using:
   make ARCH=mips CROSS_COMPILE=mips64-linux-gnuabi64- allmodconfig
   Changing CONFIG_MACH_LOONGSON32 to y, instead of 
CONFIG_MIPS_GENERIC_KERNEL
   make ARCH=mips CROSS_COMPILE=mips64-linux-gnuabi64- -j7 
drivers/watchdog/loongson1_wdt.o

I've long been reluctant to use cross-compiler because of low disk space 
on my system. But I've changed my mind recently and now I do cross 
compile. (see [1] if needed as example also with ARCH=arm)


My comments below the --- in the patches should not be taken as "I've 
not managed to build with the patch", but "I've been surprised to get an 
issue with x86_64, then cross-compiled with the relevant toolchain. 
Then, I reported what looked like a potential issue when building with 
x86_64."

CJ

[1]: 
https://lore.kernel.org/all/dc5a2157-19c8-4498-6288-d7513ad3dde2@wanadoo.fr/

> 
> Thanks,
> Guenter
>