>>>> create mode 100644 arch/powerpc/sysdev/fsl_85xx_sram_uapi.c >>> >>> We shouldn't add more stuff in arch/powerpc/sysdev/ >>> >>> Either it is dedicated to 85xx, and it should go into >>> arch/powerpc/platform/85xx/ , or it is common to several >>> platforms/architectures and should be moved to drivers/soc/fsl/ >>> >> >> Sure, actually I tried to find a better place, but did not recognize >> the driver/soc. Thanks, and I will put fsl_85xx_sram_uapi there. >> >>>> >>>> diff --git a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h >>>> b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h >>>> index 0235a0447baa..99cb7e202c38 100644 >>>> --- a/arch/powerpc/include/asm/fsl_85xx_cache_sram.h >>>> +++ b/arch/powerpc/include/asm/fsl_85xx_cache_sram.h >>>> @@ -26,6 +26,10 @@ struct mpc85xx_cache_sram { >>>> unsigned int size; >>>> rh_info_t *rh; >>>> spinlock_t lock; >>>> + >>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI >>>> + struct device *dev; >>>> +#endif >>>> }; >>>> >>>> extern void mpc85xx_cache_sram_free(void *ptr); >>>> diff --git a/arch/powerpc/platforms/85xx/Kconfig >>>> b/arch/powerpc/platforms/85xx/Kconfig >>>> index fa3d29dcb57e..3a6f6af973eb 100644 >>>> --- a/arch/powerpc/platforms/85xx/Kconfig >>>> +++ b/arch/powerpc/platforms/85xx/Kconfig >>>> @@ -16,6 +16,16 @@ if FSL_SOC_BOOKE >>>> >>>> if PPC32 >>>> >>>> +config FSL_85XX_SRAM_UAPI >>>> + tristate "Freescale MPC85xx SRAM UAPI Support" >>>> + depends on FSL_SOC_BOOKE && SRAM_DYNAMIC >>> >>> Is SRAM_DYNAMIC usefull on its own, without a driver like this one ? Is >>> that worth allowing tiny selection of both drivers ? Shouldn't one of >>> them imply the other one ? >> >> Truely the module like this is the top level selection, and SRAM_DYNAMIC >> should be selected by any caller modules. As SRAM_DYNAMIC may be used by >> other drivers(in the future, but currently only us here), I think make it >> seleted by this is better? (show below) >> >> diff --git a/drivers/soc/fsl/Kconfig b/drivers/soc/fsl/Kconfig >> index 4df32bc4c7a6..ceeebb22f6d3 100644 >> --- a/drivers/soc/fsl/Kconfig >> +++ b/drivers/soc/fsl/Kconfig >> @@ -50,4 +50,16 @@ config FSL_RCPM >> tasks associated with power management, such as wakeup source control. >> Note that currently this driver will not support PowerPC based >> QorIQ processor. >> + >> +config FSL_85XX_SRAM_UAPI >> + tristate "Freescale MPC85xx SRAM UAPI Support" >> + depends on FSL_SOC_BOOKE && PPC32 >> + select FSL_85XX_CACHE_SRAM >> + select SRAM_DYNAMIC >> + help >> + This registers a device of struct sram_device type which would act as >> + an interface for user level applications to access the Freescale 85xx >> + Cache-SRAM memory dynamically, meaning allocate on demand dynamically >> + while they are running. >> + > >And then in patch 4, I'm not sure it is worth to keep SRAM_DYNAMIC as >user selectable. >
Maybe it could be used as a module probed dynamically or so, Currently seems there is not need to make it selectable for users. >> endmenu >> diff --git a/drivers/soc/fsl/Makefile b/drivers/soc/fsl/Makefile >> index 906f1cd8af01..716e38f75735 100644 >> --- a/drivers/soc/fsl/Makefile >> +++ b/drivers/soc/fsl/Makefile >> @@ -10,3 +10,4 @@ obj-$(CONFIG_FSL_RCPM) += rcpm.o >> obj-$(CONFIG_FSL_GUTS) += guts.o >> obj-$(CONFIG_FSL_MC_DPIO) += dpio/ >> obj-$(CONFIG_DPAA2_CONSOLE) += dpaa2-console.o >> +obj-$(CONFIG_FSL_85XX_SRAM_UAPI) += fsl_85xx_sram_uapi.o >> >>>> >>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI >>>> +extern struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void); >>> >>> 'extern' keywork is meaningless here, remove it. >>> >> >> I will remove it in patch v4. >> >>>> +#endif >>>> + >>>> extern int instantiate_cache_sram(struct platform_device *dev, >>>> struct sram_parameters sram_params); >>>> extern void remove_cache_sram(struct platform_device *dev); >>>> diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c >>>> b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c >>>> index 3de5ac8382c0..0156ea63a3a2 100644 >>>> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c >>>> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c >>>> @@ -23,6 +23,14 @@ >>>> >>>> struct mpc85xx_cache_sram *cache_sram; >>>> >>>> + >>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI >>>> +struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void) >>>> +{ >>>> + return cache_sram; >>>> +} >>>> +#endif >>> >>> This function is not worth the mess of an #ifdef in a .c file. >>> cache_sram is already globaly visible, so this function should go in >>> fsl_85xx_cache_ctlr.h as a 'static inline' >>> >> >> Yes, and I will change it like this, with an extern of cache_sram. >> >> #define L2CR_SRAM_ZERO 0x00000000 /* L2SRAM zero size */ >> @@ -81,6 +83,15 @@ struct sram_parameters { >> phys_addr_t sram_offset; >> }; >> >> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI >> +extern struct mpc85xx_cache_sram *cache_sram; >> + >> +static inline struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void) >> +{ >> + return cache_sram; >> +} >> +#endif >> + >> extern int instantiate_cache_sram(struct platform_device *dev, >> If the module is moved to drivers/soc/fsl, fsl_85xx_cache_ctlr.h in arch/powerpc/sysdev is not available anymore to include directly. Is it OK to just use a extern within fsl_85xx_sram_uapi.c, (Seems it is not prefered to user extern in c files, but I can find any documentation in Kernel.) Or just add a header file fsl_85xx_sram.h containing only followings: /* SPDX-License-Identifier: GPL-2.0 */ #ifndef __FSL_85XX_SRAM_H__ #define __FSL_85XX_SRAM_H__ extern struct mpc85xx_cache_sram *cache_sram; static inline struct mpc85xx_cache_sram *mpc85xx_get_cache_sram(void) { return cache_sram; } #endif /* __FSL_85XX_SRAM_H__ */ >>>> + >>>> void *mpc85xx_cache_sram_alloc(unsigned int size, >>>> phys_addr_t *phys, unsigned int align) >>>> { >>>> @@ -115,6 +123,10 @@ int instantiate_cache_sram(struct platform_device >>>> *dev, >>>> rh_attach_region(cache_sram->rh, 0, cache_sram->size); >>>> spin_lock_init(&cache_sram->lock); >>>> >>>> +#ifdef CONFIG_FSL_85XX_SRAM_UAPI >>>> + cache_sram->dev = &dev->dev; >>>> +#endif >>> >>> Can we avoid the #ifdef in .c file ? (see >>> https://www.kernel.org/doc/html/latest/process/coding-style.html#conditional-compilation) >>> >> >> Definitely, and I will change it as below in patch v4: >> >> + if (IS_ENABLED(CONFIG_FSL_85XX_SRAM_UAPI)) >> + cache_sram->dev = &dev->dev; >> + > >This will work only if is defined all the time in the .h regardless of >CONFIG_FSL_85XX_SRAM_UAPI. Otherwise you should have something like that >in the .h, that you call all the time from the .c: > >#ifdef CONFIG_FSL_85XX_SRAM_UAPI >static inline void set_cache_sram_dev(struct mpc85xx_cache_sram *sram, >struct device *dev) >{ > sram->dev = dev; >} >#else >static inline void set_cache_sram_dev(struct mpc85xx_cache_sram *sram, >struct device *dev) { } >#endif > Sure, better. > >> dev_info(&dev->dev, "[base:0x%llx, size:0x%x] configured and loaded\n", >> >> Thanks, for your suggestions, as these are minor modifications, >> I will send a new patch series v4 soon. >> >> Regards, >> Wenhu >> > >Christophe Wenhu