Re: [PATCH v3,5/5] powerpc: sysdev: support userspace access of fsl_85xx_sram

王文虎 wenhu.wang at vivo.com
Fri Apr 24 17:05:10 AEST 2020


>Le 24/04/2020 à 04:45, Wang Wenhu a écrit :
>> New module which registers its memory allocation and free APIs to the
>> sram_dynamic module, which would create a device of struct sram_device
>> type to act as an interface for user level applications to access the
>> backend hardware device, fsl_85xx_cache_sram, which is drived by the
>> FSL_85XX_CACHE_SRAM module.
>> 
>> Signed-off-by: Wang Wenhu <wenhu.wang at vivo.com>
>> Cc: Christophe Leroy <christophe.leroy at c-s.fr>
>> Cc: Scott Wood <oss at buserror.net>
>> Cc: Michael Ellerman <mpe at ellerman.id.au>
>> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
>> Cc: Arnd Bergmann <arnd at arndb.de>
>> Cc: linuxppc-dev at lists.ozlabs.org
>> ---
>>   .../powerpc/include/asm/fsl_85xx_cache_sram.h |  4 ++
>>   arch/powerpc/platforms/85xx/Kconfig           | 10 +++++
>>   arch/powerpc/sysdev/Makefile                  |  1 +
>>   arch/powerpc/sysdev/fsl_85xx_cache_ctlr.h     |  6 +++
>>   arch/powerpc/sysdev/fsl_85xx_cache_sram.c     | 12 ++++++
>>   arch/powerpc/sysdev/fsl_85xx_sram_uapi.c      | 39 +++++++++++++++++++
>>   6 files changed, 72 insertions(+)
>>   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.
+
 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,

>> +
>>   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;
+
 	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



More information about the Linuxppc-dev mailing list