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

[çæè]

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