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

[Christophe JAILLET]

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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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@xxxxxxxxxx/

> Thanks,
> Guenter