Re: [PATCH v3 4/4] clk: microchip: core: allow driver to be compiled with COMPILE_TEST

[Brian Masney <bmasney@redhat.com>]

Hi Claudiu,

Sorry about the delay writing back. I was at Linux Plumbers in Tokyo.

On Sat, Dec 06, 2025 at 04:28:31PM +0200, Claudiu Beznea wrote:
> On 12/5/25 21:46, Brian Masney wrote:
> > This driver currently only supports builds against a PIC32 target. To
> > avoid future breakage in the future, let's update the Kconfig and the
> > driver so that it can be built with CONFIG_COMPILE_TEST enabled.
> > 
> > Note that with the existing asm calls is not how I'd want to do this
> > today if this was a new driver, however I don't have access to this
> > hardware. To avoid any breakage, let's keep the existing behavior.
> > 
> > Signed-off-by: Brian Masney <bmasney@redhat.com>
> > ---
> >  drivers/clk/microchip/Kconfig    |  2 +-
> >  drivers/clk/microchip/clk-core.c | 32 +++++++++++++++++++++++---------
> >  2 files changed, 24 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/clk/microchip/Kconfig b/drivers/clk/microchip/Kconfig
> > index 1b9e43eb54976b219a0277cc971f353fd6af226a..1e56a057319d97e20440fe4e107d26fa85c95ab1 100644
> > --- a/drivers/clk/microchip/Kconfig
> > +++ b/drivers/clk/microchip/Kconfig
> > @@ -1,7 +1,7 @@
> >  # SPDX-License-Identifier: GPL-2.0
> >  
> >  config COMMON_CLK_PIC32
> > -	def_bool COMMON_CLK && MACH_PIC32
> > +	def_bool (COMMON_CLK && MACH_PIC32) || COMPILE_TEST
> >  
> >  config MCHP_CLK_MPFS
> >  	bool "Clk driver for PolarFire SoC"
> > diff --git a/drivers/clk/microchip/clk-core.c b/drivers/clk/microchip/clk-core.c
> > index f467d7bc28c87a50fb18dc527574f973c4b7e615..fad4b45d908310ffb59e4ed57c55ae4266253444 100644
> > --- a/drivers/clk/microchip/clk-core.c
> > +++ b/drivers/clk/microchip/clk-core.c
> > @@ -9,7 +9,15 @@
> >  #include <linux/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/iopoll.h>
> > +
> > +#if !defined(CONFIG_MACH_PIC32) && defined(CONFIG_COMPILE_TEST)
> 
> Can't we have something like:
> 
> #if defined(CONFIG_MATCH_PIC32)
> #include <asm/match-pic32/pic32.h>
> #else
> #define PIC32_CLR(_reg)		((_reg) + 0x04)
> #define PIC32_SET(_reg)		((_reg) + 0x08)
> #define PIC32_INV(_reg)		((_reg) + 0x0C)
> #define pic32_syskey_unlock()
> #endif

Yes, I agree that would be simpler.

> > +#define PIC32_CLR(_reg)		((_reg) + 0x04)
> > +#define PIC32_SET(_reg)		((_reg) + 0x08)
> > +#define PIC32_INV(_reg)		((_reg) + 0x0C)
> > +#define pic32_syskey_unlock()
> 
> On the other side, there are other drivers using these defines, maybe a
> unified approach would fit better? Maybe moving these to
> include/linux/platform_data ?

I agree that would be the better approach. Specifically:

- Move arch/mips/include/asm/mach-pic32/pic32.h to
  include/linux/platform_data
- Drop the unused include linux/io.h in pic32.h
- Check for CONFIG_MATCH_PIC32 for the pic32_syskey_unlock define. Make
  it a noop for all other architectures.

That would allow us to have the following in the drivers, with no #if's:

#include <linux/platform_data/pic32.h>

I initially wanted to go this route, and I feel that's the best
technical decision, however my only hesitation is that this is going to
touch at least 8 different subsystems. I could probably get the MIPS
folks to take all of this, however it's going to be a pain to collect
all of the ACKs from the different subsystems.

x1:~/src/linux/linus (master %)$ git grep pic32.h
arch/mips/pic32/common/reset.c:#include <asm/mach-pic32/pic32.h>
arch/mips/pic32/common/reset.c:static void pic32_halt(void)
arch/mips/pic32/common/reset.c: pic32_halt();
arch/mips/pic32/common/reset.c: pic32_halt();
arch/mips/pic32/pic32mzda/config.c:#include <asm/mach-pic32/pic32.h>
arch/mips/pic32/pic32mzda/early_clk.c:#include <asm/mach-pic32/pic32.h>
arch/mips/pic32/pic32mzda/early_console.c:#include <asm/mach-pic32/pic32.h>
arch/mips/pic32/pic32mzda/init.c:#include <linux/platform_data/sdhci-pic32.h>
drivers/clk/microchip/clk-core.c:#include <asm/mach-pic32/pic32.h>
drivers/irqchip/irq-pic32-evic.c:#include <asm/mach-pic32/pic32.h>
drivers/mmc/host/sdhci-pic32.c:#include <linux/platform_data/sdhci-pic32.h>
drivers/pinctrl/pinctrl-pic32.c:#include <asm/mach-pic32/pic32.h>
drivers/pinctrl/pinctrl-pic32.c:#include "pinctrl-pic32.h"
drivers/rtc/rtc-pic32.c:#include <asm/mach-pic32/pic32.h>
drivers/tty/serial/pic32_uart.c:#include <asm/mach-pic32/pic32.h>
drivers/watchdog/pic32-dmt.c:#include <asm/mach-pic32/pic32.h>
drivers/watchdog/pic32-wdt.c:#include <asm/mach-pic32/pic32.h>

Thoughts?

> > +#if !defined(CONFIG_MACH_PIC32) && defined(CONFIG_COMPILE_TEST)
> 
> Same here, can't we have:
> 
> #ifdef CONFIG_MATCH_PIC32
> #define cpu_nop5()			\
> 	do {					\
> 		__asm__ __volatile__("nop");	\
> 		__asm__ __volatile__("nop");	\
> 		__asm__ __volatile__("nop");	\
> 		__asm__ __volatile__("nop");	\
> 		__asm__ __volatile__("nop");	\
> 	} while (0)
> #else
> #define cpu_nop5()
> #endif

Yes that sounds good.

Brian