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 0x  /* L2SRAM zero size */
>> @@ -81,6 +83,15 @@ struct sram_parameters {
>>  phys_addr_t sram_offset;
>>   };
>>   

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

[王文虎]

>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 
>> Cc: Christophe Leroy 
>> Cc: Scott Wood 
>> Cc: Michael Ellerman 
>> Cc: Greg Kroah-Hartman 
>> Cc: Arnd Bergmann 
>> Cc: linuxppc-dev@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)
>> +{

Re: [PATCH v3,4/5] misc: sram_dynamic for user level SRAM access

[王文虎]

>> diff --git a/include/linux/sram_dynamic.h b/include/linux/sram_dynamic.h
>> new file mode 100644
>> index ..c77e9e7b1151
>> --- /dev/null
>> +++ b/include/linux/sram_dynamic.h
>> @@ -0,0 +1,23 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __SRAM_DYNAMIC_H
>> +#define __SRAM_DYNAMIC_H
>> +
>> +struct sram_api {
>> +const char  *name;
>> +struct sram_device *sdev;
>> +void *(*alloc)(__u32 size, phys_addr_t *phys, __u32 align);
>> +void (*free)(void *ptr);
>> +};
>> +
>> +extern int __must_check
>> +__sram_register_device(struct module *owner,
>> +   struct device *parent,
>> +   struct sram_api *sa);
>
>'extern' keyword is useless here, remove it (checkpatch --strict will 
>likely tell you the same)
>
>> +
>> +/* Use a define to avoid include chaining to get THIS_MODULE */
>> +#define sram_register_device(parent, sa) \
>> +__sram_register_device(THIS_MODULE, parent, sa)
>> +
>> +extern void sram_unregister_device(struct sram_api *sa);
>
>Same, no 'extern' please.
>

Thanks, I will remove them in patch v4. And checkpatch with --strict will be 
prefered.

Wenhu

Re:Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

>Hi, Scott, Greg,
>
>Thank you for your helpful comments.
>For that Greg mentioned that the patch (or patch series) via UIO should worked 
>through,
>so I want to make it clear that if it would go upstream?(And if so, when? No 
>push, just ask)
>
>Also I have been wondering how the patches with components in different 
>subsystems
>go get upstream to the mainline? Like patch 1-3 are of linuxppc-dev, and patch 
>4 is of
>subsystem UIO, and if acceptable, how would you deal with them?
>
>Back to the devicetree thing, I make it detached from hardware compatibilities 
>which belong
>to the hardware level driver and also used module parameter for of_id 
>definition as dt-binding
>is not allowed for UIO now. So as I can see, things may go well and there is 
>no harm to anything,
>I hope you(Scott) please take a re-consideration. 
>

I mean I have get some new work done based on the comments of Arnd, Scott and 
Greg. Also a lot of tests done.
So it would be better to make it clear whether I shoud keep the work going or 
the UIO version is to be accepted
to go upstream recently in the future.

Thanks & regards,
Wenhu
>
>>On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
>>> +static void sram_uapi_res_insert(struct sram_uapi *uapi,
>>> +struct sram_resource *res)
>>> +{
>>> +   struct sram_resource *cur, *tmp;
>>> +   struct list_head *head = >res_list;
>>> +
>>> +   list_for_each_entry_safe(cur, tmp, head, list) {
>>> +   if (>list != head &&
>>> +   (cur->info.offset + cur->info.size + res->info.size <=
>>> +   tmp->info.offset)) {
>>> +   res->info.offset = cur->info.offset + cur->info.size;
>>> +   res->parent = uapi;
>>> +   list_add(>list, >list);
>>> +   return;
>>> +   }
>>> +   }
>>
>>We don't need yet another open coded allocator.  If you really need to do this
>>then use include/linux/genalloc.h, but maybe keep it simple and just have one
>>allocaton per file descriptor so you don't need to manage fd offsets?
>>
>>> +static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
>>> +   __u32 offset)
>>> +{
>>> +   struct sram_resource *res;
>>> +
>>> +   list_for_each_entry(res, >res_list, list) {
>>> +   if (res->info.offset == offset)
>>> +   return res;
>>> +   }
>>> +
>>> +   return NULL;
>>> +}
>>
>>What if the allocation is more than one page, and the user mmaps starting
>>somewhere other than the first page?
>>
>>> +   switch (cmd) {
>>> +   case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>>> +   if (uapi->sa)
>>> +   return -EEXIST;
>>> +
>>> +   get_user(type, (const __u32 __user *)arg);
>>> +   uapi->sa = get_sram_api_from_type(type);
>>> +   if (uapi->sa)
>>> +   ret = 0;
>>> +   else
>>> +   ret = -ENODEV;
>>> +
>>> +   break;
>>> +
>>
>>Just expose one device per backing SRAM, especially if the user has any reason
>>to care about where the SRAM is coming from (correlating sysfs nodes is much
>>more expressive than some vague notion of "type").
>>
>>> +   case SRAM_UAPI_IOC_ALLOC:
>>> +   if (!uapi->sa)
>>> +   return -EINVAL;
>>> +
>>> +   res = kzalloc(sizeof(*res), GFP_KERNEL);
>>> +   if (!res)
>>> +   return -ENOMEM;
>>> +
>>> +   size = copy_from_user((void *)>info,
>>> + (const void __user *)arg,
>>> + sizeof(res->info));
>>> +   if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
>>> +   return -EINVAL;
>>
>>Missing EFAULT test (here and elsewhere), and res leaks on error.
>>
>>> +
>>> +   res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>>> +>phys,
>>> +PAGE_SIZE);
>>
>>Do we really need multiple allocators, or could the backend be limited to just
>>adding regions to a generic allocator (with that allocator also serving in-
>>kernel users)?
>>
>>If sram_alloc is supposed to return a virtual address, why isn't that the
>>return type?
>>
>>> +   if (!res->virt) {
>>> +   kfree(res);
>>> +   return -ENOMEM;
>>> +   }
>>
>>ENOSPC might be more appropriate, as this isn't general-purpose RAM.
>>
>>> +
>>> +   sram_uapi_res_insert(uapi, res);
>>> +   size = copy_to_user((void __user *)arg,
>>> +   (const void *)>info,
>>> +   sizeof(res->info));
>>> +
>>> +   ret = 0;
>>> +   break;
>>> +
>>> +   case SRAM_UAPI_IOC_FREE:
>>> +   if (!uapi->sa)
>>> +   return -EINVAL;
>>> +
>>> +   size = copy_from_user((void *), (const void __user *)arg,
>>> + 

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

Hi, Scott, Greg,

Thank you for your helpful comments.
For that Greg mentioned that the patch (or patch series) via UIO should worked 
through,
so I want to make it clear that if it would go upstream?(And if so, when? No 
push, just ask)

Also I have been wondering how the patches with components in different 
subsystems
go get upstream to the mainline? Like patch 1-3 are of linuxppc-dev, and patch 
4 is of
subsystem UIO, and if acceptable, how would you deal with them?

Back to the devicetree thing, I make it detached from hardware compatibilities 
which belong
to the hardware level driver and also used module parameter for of_id 
definition as dt-binding
is not allowed for UIO now. So as I can see, things may go well and there is no 
harm to anything,
I hope you(Scott) please take a re-consideration. 

Thanks & regards,
Wenhu

>On Sun, 2020-04-19 at 20:05 -0700, Wang Wenhu wrote:
>> +static void sram_uapi_res_insert(struct sram_uapi *uapi,
>> + struct sram_resource *res)
>> +{
>> +struct sram_resource *cur, *tmp;
>> +struct list_head *head = >res_list;
>> +
>> +list_for_each_entry_safe(cur, tmp, head, list) {
>> +if (>list != head &&
>> +(cur->info.offset + cur->info.size + res->info.size <=
>> +tmp->info.offset)) {
>> +res->info.offset = cur->info.offset + cur->info.size;
>> +res->parent = uapi;
>> +list_add(>list, >list);
>> +return;
>> +}
>> +}
>
>We don't need yet another open coded allocator.  If you really need to do this
>then use include/linux/genalloc.h, but maybe keep it simple and just have one
>allocaton per file descriptor so you don't need to manage fd offsets?
>
>> +static struct sram_resource *sram_uapi_find_res(struct sram_uapi *uapi,
>> +__u32 offset)
>> +{
>> +struct sram_resource *res;
>> +
>> +list_for_each_entry(res, >res_list, list) {
>> +if (res->info.offset == offset)
>> +return res;
>> +}
>> +
>> +return NULL;
>> +}
>
>What if the allocation is more than one page, and the user mmaps starting
>somewhere other than the first page?
>
>> +switch (cmd) {
>> +case SRAM_UAPI_IOC_SET_SRAM_TYPE:
>> +if (uapi->sa)
>> +return -EEXIST;
>> +
>> +get_user(type, (const __u32 __user *)arg);
>> +uapi->sa = get_sram_api_from_type(type);
>> +if (uapi->sa)
>> +ret = 0;
>> +else
>> +ret = -ENODEV;
>> +
>> +break;
>> +
>
>Just expose one device per backing SRAM, especially if the user has any reason
>to care about where the SRAM is coming from (correlating sysfs nodes is much
>more expressive than some vague notion of "type").
>
>> +case SRAM_UAPI_IOC_ALLOC:
>> +if (!uapi->sa)
>> +return -EINVAL;
>> +
>> +res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +if (!res)
>> +return -ENOMEM;
>> +
>> +size = copy_from_user((void *)>info,
>> +  (const void __user *)arg,
>> +  sizeof(res->info));
>> +if (!PAGE_ALIGNED(res->info.size) || !res->info.size)
>> +return -EINVAL;
>
>Missing EFAULT test (here and elsewhere), and res leaks on error.
>
>> +
>> +res->virt = (void *)uapi->sa->sram_alloc(res->info.size,
>> + >phys,
>> + PAGE_SIZE);
>
>Do we really need multiple allocators, or could the backend be limited to just
>adding regions to a generic allocator (with that allocator also serving in-
>kernel users)?
>
>If sram_alloc is supposed to return a virtual address, why isn't that the
>return type?
>
>> +if (!res->virt) {
>> +kfree(res);
>> +return -ENOMEM;
>> +}
>
>ENOSPC might be more appropriate, as this isn't general-purpose RAM.
>
>> +
>> +sram_uapi_res_insert(uapi, res);
>> +size = copy_to_user((void __user *)arg,
>> +(const void *)>info,
>> +sizeof(res->info));
>> +
>> +ret = 0;
>> +break;
>> +
>> +case SRAM_UAPI_IOC_FREE:
>> +if (!uapi->sa)
>> +return -EINVAL;
>> +
>> +size = copy_from_user((void *), (const void __user *)arg,
>> +  sizeof(info));
>> +
>> +res = sram_uapi_res_delete(uapi, );
>> +if (!res) {
>> +pr_err("error no sram resource found\n");
>> +return -EINVAL;
>> +}
>> +
>> +uapi->sa->sram_free(res->virt);
>> +kfree(res);
>> +
>> +ret 

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

>On Tue, Apr 21, 2020 at 05:09:47PM +0800, 王文虎 wrote:
>> Hi, Greg, Arnd,
>> 
>> Thank you for your comments first, and then really very very very sorry
>> for driving Greg to sigh and I hope there would be chance to share Moutai
>> (rather than whisky, we drink it much, a kind of Baijiu), after the virus.
>> 
>> Back to the comments, I'd like to do a bit of documentation or explanation 
>> first,
>> which should have been done early or else there would not be so much to 
>> explain:
>> 1. What I have been trying to do is to access the Freescale Cache-SRAM 
>> device form
>> user level;
>> 2. I implemented it using UIO, which was thought of non-proper;
>
>I still think that using uio is the best way to do this, and never said
>it was "non-proper".  All we got bogged down in was the DT
>representation of stuff from what I remember.  That should be worked
>through.
>
>thanks,
>
>greg k-h

Surely, but so how would things go? Scott said not fit for him. And he was
gonna to write a new patch(Oh,  that is what I have been doing.,and I really
donot think he need to do it)

This is the final version using UIO, and even Christophe had Reviewed-by,
https://lore.kernel.org/patchwork/patch/1226225/

If no ending reaches, I have to make a step forward to keep working with
the misc device version.

Thanks, and regards,
Wenhu

Re: [PATCH v2,RESEND] misc: new driver sram_uapi for user level SRAM access

[王文虎]

Hi, Greg, Arnd,

Thank you for your comments first, and then really very very very sorry
for driving Greg to sigh and I hope there would be chance to share Moutai
(rather than whisky, we drink it much, a kind of Baijiu), after the virus.

Back to the comments, I'd like to do a bit of documentation or explanation 
first,
which should have been done early or else there would not be so much to explain:
1. What I have been trying to do is to access the Freescale Cache-SRAM device 
form
user level;
2. I implemented it using UIO, which was thought of non-proper;
3. So I implemented it here to create a misc device as a interface between 
Kernel
and user space applications, as the comments of Scott suggested;
[Link for 2 & 3: https://lore.kernel.org/patchwork/patch/1225798/]
4. This is exactly a shim of Cache-SRAM hardware level driver, and it would use
apis provided by the hardware driver to do the allocation and free work of
SRAM memory;
5. The file drivers/misc/sram.c actually has done some abstractions for SRAM 
access,
but it is used as a static way that resources are defined within devicetree;
6. This patch is to reach a way that allocate and release SRAM memory 
dynamically
which is much more convenient for user space applications, and for another 
reason,
the Cache-SRAM of Freescale is kind of not compatible with drivers/misc/sram.c;

So I implemented the sram_uapi.c as following:
1. Create a misc device as a interface that support ioctl and mmap for memory 
access;
2. For that I think this could be used for any SRAM devices that would support 
dynamic
memory allocation and release, I create sram_api_list as follow:

static DEFINE_MUTEX(sram_api_list_lock);
static LIST_HEAD(sram_api_list);

and different SRAM devices could add their apis to the sram_api_list
which could be used for the allocation from them;
a. int sram_api_register(struct sram_api *sa);
b. int sram_api_unregister(struct sram_api *sa);
c. ioctl case SRAM_UAPI_IOC_SET_SRAM_TYPE;
the register and unregister api are exported for any SRAM drivers to do this,
and maybe user could chose one with ioctl to allocate;
[maybe only one SRAM device available currently and the implementation is 
redundant]
3. For each fd that opened, a sram_uapi would be created:

struct sram_uapi {
struct list_headres_list;
struct sram_api *sa;
};

The res_list would be a list of resources that allocated attached to it:

struct sram_resource {
struct list_headlist;
struct res_info info;
phys_addr_t phys;
void*virt;
struct vm_area_struct   *vma;
struct sram_uapi*parent;
};

The sa is a pointer to the apis registered by hardware SRAM driver as descibed 
earlier,
and when a fd is attached to it, kref it once;
And for the resources, it is accessed by the process only so I did not use a 
lock here,
and as Greg commented, lock is needed for the fd sharement.
4. The exported apis are currently not reference and I would add another patch 
series
for Freescale Cache-SRAM to use the api sram_api_register to register its 
allocation
and free api, as well as sram_api_unregister for unregistering;

Actually, I implemented the api list another way in v1, but was commented by 
Arnd not a
better way.
[https://lore.kernel.org/patchwork/patch/1226689/]

Then for the comments:
1. Ioctl: I will move the block to uapi, and use static length definition of 
__be64;
2. Naming: as Greg commented, so hard, and how do you think of
sram_dynamic(as compared to drivers/misc/sram.c)?
3. API list: only one SRAM device? But however, this is a shim, and hardware 
level
SRAM driver(s) should be enabled to register or init the apis for real 
allocation;
4. Test: My team and me myself did not cover it, especially newly added kref, 
and that
is really really so wrong of us, and I will make efforts to test for every 
logical path
next time, and this would never ever happen. Really sorry for it.

>On Sun, Apr 19, 2020 at 08:05:38PM -0700, Wang Wenhu wrote:
>> A generic User-Kernel interface that allows a misc device created
>> by it to support file-operations of ioctl and mmap to access SRAM
>> memory from user level. Different kinds of SRAM alloction and free
>> APIs could be registered by specific SRAM hardware level driver to
>> the available list and then be chosen by users to allocate and map
>> SRAM memory from user level.
>> 
>> It is extremely helpful for the user space applications that require
>> high performance memory accesses, such as embedded networking devices
>> that would process data in user space, and PowerPC e500 is a case.
>> 
>> Signed-off-by: Wang Wenhu 
>> Cc: Greg Kroah-Hartman 
>> Cc: Arnd Bergmann 
>> Cc: Christophe Leroy 
>> Cc: Scott Wood 
>> Cc: Michael Ellerman 
>> Cc: Randy Dunlap 
>> Cc: 

Re: [PATCH v6,4/4] drivers: misc: new driver sram_uapi for user level SRAM access

[王文虎]

>> A generic User-Kernel interface that allows a misc device created
>> by it to support file-operations of ioctl and mmap to access SRAM
>> memory from user level. Different kinds of SRAM alloction and free
>> APIs could be added to the available array and could be configured
>> from user level.
>
>Having a generic user level interface seem reasonable, but it would
>be helpful to list one or more particular use cases.

OK, I will use the FSL_85XX_SRAM as a case to describe it.

>
>> +if SRAM_UAPI
>> +
>> +config FSL_85XX_SRAM_UAPI
>> +   bool "Freescale MPC85xx Cache-SRAM UAPI support"
>> +   depends on FSL_SOC_BOOKE && PPC32
>> +   select FSL_85XX_CACHE_SRAM
>> +   help
>> + This adds the Freescale MPC85xx Cache-SRAM memory allocation and
>> + free interfaces to the available SRAM API array, which finally 
>> could
>> + be used from user level to access the Freescale MPC85xx Cache-SRAM
>> + memory.
>
>Why do you need  a hardware specific Kconfig option here, shouldn't
>this just use the generic kernel abstraction for the sram?
>
Yes, I will add a interface for any hardware drivers to register there specific 
apis
instead of the definition here.

>> +struct sram_api {
>> +   u32 type;
>> +   long (*sram_alloc)(u32 size, phys_addr_t *phys, u32 align);
>> +   void (*sram_free)(void *ptr);
>> +};
>> +
>> +struct sram_uapi {
>> +   struct list_headres_list;
>> +   struct sram_api *sa;
>> +};
>> +
>> +enum SRAM_TYPE {
>> +#ifdef FSL_85XX_CACHE_SRAM
>> +   SRAM_TYPE_FSL_85XX_CACHE_SRAM,
>> +#endif
>> +   SRAM_TYPE_MAX,
>> +};
>> +
>> +/* keep the SRAM_TYPE value the same with array index */
>> +static struct sram_api srams[] = {
>> +#ifdef FSL_85XX_CACHE_SRAM
>> +   {
>> +   .type   = SRAM_TYPE_FSL_85XX_CACHE_SRAM,
>> +   .sram_alloc = mpc85xx_cache_sram_alloc,
>> +   .sram_free  = mpc85xx_cache_sram_free,
>> +   },
>> +#endif
>> +};
>
>If there is a indeed a requirement for hardware specific functions,
>I'd say these should be registered from the hardware specific driver
>rather than the generic driver having to know about every single
>instance.

Yes, as you mentioned upper, and the interfaces should be registered
by hardware drivers. and I'd use a set of generic abstractions of the 
definitions.

>> +static long sram_uapi_ioctl(struct file *filp, unsigned int cmd,
>> +   unsigned long arg)
>> +{
>> +   struct sram_uapi *uapi = filp->private_data;
>> +   struct sram_resource *res;
>> +   struct res_info info;
>> +   long ret = -EINVAL;
>> +   int size;
>> +   u32 type;
>> +
>> +   if (!uapi)
>> +   return ret;
>> +
>> +   switch (cmd) {
>> +   case SRAM_UAPI_IOCTL_SET_SRAM_TYPE:
>> +   size = copy_from_user((void *), (const void __user 
>> *)arg,
>> + sizeof(type));
>
>This could be a simpler get_user().

Addressed.

>
>> +static const struct file_operations sram_uapi_ops = {
>> +   .owner = THIS_MODULE,
>> +   .open = sram_uapi_open,
>> +   .unlocked_ioctl = sram_uapi_ioctl,
>> +   .mmap = sram_uapi_mmap,
>> +   .release = sram_uapi_release,
>> +};
>
>If you have a .unlocked_ioctl callback, there should also be a
>.compat_ioctl one. This can normally point to compat_ptr_ioctl().

Addressed

>> +
>> +static struct miscdevice sram_uapi_miscdev = {
>> +   MISC_DYNAMIC_MINOR,
>> +   "sram-uapi",
>> +   _uapi_ops,
>> +};
>
>The name of the character device should not contain "uapi", that
>is kind of implied here.

Addressed

>> +
>> +#define SRAM_UAPI_IOCTL_SET_SRAM_TYPE  0
>> +#define SRAM_UAPI_IOCTL_ALLOC  1
>> +#define SRAM_UAPI_IOCTL_FREE   2
>> +
>> +struct res_info {
>> +   u32 offset;
>> +   u32 size;
>> +};
>
>This is of course not a proper ioctl interface at all, please see
>Documentation/driver-api/ioctl.rst for how to define the numbers
>in a uapi header file.
>
>The offset/size arguments should probably be 64 bit wide.

OK, I will reference the ioctl.rst and make a improvement and I think
phys_addr_t would be a better choice.

Thanks,
Wenhu

Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[王文虎]

>On Fri, 2020-04-17 at 09:42 +0200, Greg KH wrote:>> On Thu, Apr 16, 2020 at 
>11:58:29PM -0500, Scott Wood wrote:
>> > On Fri, 2020-04-17 at 10:31 +0800, 王文虎 wrote:
>> > > Sounds it is. And does the modification below fit well?
>> > > ---
>> > > -static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> > > -   {   .compatible = "uio,mpc85xx-cache-sram", },
>> > > -   {},
>> > > +#ifdef CONFIG_OF
>> > > +static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
>> > > +   { /* This is filled with module_parm */ },
>> > > +   { /* Sentinel */ },
>> > >  };
>> > > +MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
>> > > +module_param_string(of_id,
>> > > uio_fsl_85xx_cache_sram_of_match[0].compatible,
>> > > +   sizeof(uio_fsl_85xx_cache_sram_of_match[0].c
>> > > ompa
>> > > tible), 0);
>> > > +MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-
>> > > sram-
>> > > uio");
>> > > +#endif
>> > 
>> > No.  The point is that you wouldn't be configuring this with the device
>> > tree
>> > at all.
>> 
>> Wait, why not?  Don't force people to use module parameters, that is
>> crazy.  DT describes the hardware involved, if someone wants to bind to
>> a specific range of memory, as described by DT, why can't they do so?
>
>Yes, DT describes the hardware, and as I've said a couple times already, this
>isn't hardware description.
>
>I'm not forcing people to use module parameters.  That was a least-effort
>suggestion to avoid abusing the DT.  I later said I'd try to come up with a
>patch that allocates regions dynamically (and most likely doesn't use UIO at
>all).
>
>> I can understand not liking the name "uio" in a dt tree, but there's no
>> reason that DT can not describe what a driver binds to here.
>
>The DT already describes this hardware, and there is already code that binds
>to it.  This patch is trying to add a second node for it with configuration.
>
Hi, Scott, Greg,
Seems like no balance here. How about I implement a driver of uio including
the l2ctrl and cache_sram related implementations?
And this way, the driver would be a hardware level driver and targeted for uio.

Regards,
Wenhu

Re:[PATCH v5,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[王文虎]

>A driver for freescale 85xx platforms to access the Cache-Sram form>user 
>level. This is extremely helpful for some user-space applications
>that require high performance memory accesses.
>
>Cc: Greg Kroah-Hartman 
>Cc: Christophe Leroy 
>Cc: Scott Wood 
>Cc: Michael Ellerman 
>Cc: linuxppc-dev@lists.ozlabs.org
>Reviewed-by: Christophe Leroy 
>Signed-off-by: Wang Wenhu 

Hi, Christophe,
I labeled you Reviewed comment.

Regards,
Wenhu

>---
>Changes since v1:
> * Addressed comments from Greg K-H
> * Moved kfree(info->name) into uio_info_free_internal()
>Changes since v2:
> * Addressed comments from Greg, Scott and Christophe
> * Use "uiomem->internal_addr" as if condition for sram memory free,
>   and memset the uiomem entry
> * of_match_table modified to be apart from HW info which belong to
>   the HW level driver fsl_85xx_cache_sram to match
> * Use roundup_pow_of_two for align calc
> * Remove useless clear block of uiomem entries.
> * Use UIO_INFO_VER micro for info->version, and define it as
>   "devicetree,pseudo", meaning this is pseudo device and probed from
>   device tree configuration
>Changes since v3:
> * Addressed comments from Christophe(use devm_xxx memory alloc interfaces)
>Changes since v4:
> * Use module_param_string for of_match_table, no binding to devicetree
>---
> drivers/uio/Kconfig   |   9 ++
> drivers/uio/Makefile  |   1 +
> drivers/uio/uio_fsl_85xx_cache_sram.c | 154 ++
> 3 files changed, 164 insertions(+)
> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>
>diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
>index 202ee81cfc2b..9c3b47461b71 100644
>--- a/drivers/uio/Kconfig
>+++ b/drivers/uio/Kconfig
>@@ -105,6 +105,15 @@ config UIO_NETX
> To compile this driver as a module, choose M here; the module
> will be called uio_netx.
> 
>+config UIO_FSL_85XX_CACHE_SRAM
>+  tristate "Freescale 85xx Cache-Sram driver"
>+  depends on FSL_SOC_BOOKE && PPC32
>+  select FSL_85XX_CACHE_SRAM
>+  help
>+Generic driver for accessing the Cache-Sram form user level. This
>+is extremely helpful for some user-space applications that require
>+high performance memory accesses.
>+
> config UIO_FSL_ELBC_GPCM
>   tristate "eLBC/GPCM driver"
>   depends on FSL_LBC
>diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
>index c285dd2a4539..be2056cffc21 100644
>--- a/drivers/uio/Makefile
>+++ b/drivers/uio/Makefile
>@@ -10,4 +10,5 @@ obj-$(CONFIG_UIO_NETX)   += uio_netx.o
> obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o
> obj-$(CONFIG_UIO_MF624) += uio_mf624.o
> obj-$(CONFIG_UIO_FSL_ELBC_GPCM)   += uio_fsl_elbc_gpcm.o
>+obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM) += uio_fsl_85xx_cache_sram.o
> obj-$(CONFIG_UIO_HV_GENERIC)  += uio_hv_generic.o
>diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c 
>b/drivers/uio/uio_fsl_85xx_cache_sram.c
>new file mode 100644
>index ..4db3648629b3
>--- /dev/null
>+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
>@@ -0,0 +1,154 @@
>+// SPDX-License-Identifier: GPL-2.0
>+/*
>+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>+ * Copyright (C) 2020 Wang Wenhu 
>+ * All rights reserved.
>+ */
>+
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+#include 
>+
>+#define DRIVER_NAME   "uio_fsl_85xx_cache_sram"
>+#define UIO_INFO_VER  "devicetree,pseudo"
>+#define UIO_NAME  "uio_cache_sram"
>+
>+static void uio_info_free_internal(struct uio_info *info)
>+{
>+  int i;
>+
>+  for (i = 0; i < MAX_UIO_MAPS; i++) {
>+  struct uio_mem *uiomem = >mem[i];
>+
>+  if (uiomem->internal_addr) {
>+  mpc85xx_cache_sram_free(uiomem->internal_addr);
>+  memset(uiomem, 0, sizeof(*uiomem));
>+  }
>+  }
>+}
>+
>+static int uio_fsl_85xx_cache_sram_probe(struct platform_device *pdev)
>+{
>+  struct device_node *parent = pdev->dev.of_node;
>+  struct device_node *node = NULL;
>+  struct uio_info *info;
>+  struct uio_mem *uiomem;
>+  const char *dt_name;
>+  u32 mem_size;
>+  int ret;
>+
>+  /* alloc uio_info for one device */
>+  info = devm_kzalloc(>dev, sizeof(*info), GFP_KERNEL);
>+  if (!info)
>+  return -ENOMEM;
>+
>+  /* get optional uio name */
>+  if (of_property_read_string(parent, "uio_name", _name))
>+  dt_name = UIO_NAME;
>+
>+  info->name = devm_kstrdup(>dev, dt_name, GFP_KERNEL);
>+  if (!info->name)
>+  return -ENOMEM;
>+
>+  uiomem = info->mem;
>+  for_each_child_of_node(parent, node) {
>+  void *virt;
>+  phys_addr_t phys;
>+
>+  ret = of_property_read_u32(node, "cache-mem-size", _size);
>+  if (ret) {
>+  ret = -EINVAL;
>+  goto err_out;
>+  }
>+
>+  if (mem_size == 0) {
>+  dev_err(>dev, 

Re:Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[王文虎]

>> > > On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
>> > > > +#define UIO_INFO_VER  "devicetree,pseudo"
>> > > 
>> > > What does this mean?  Changing a number into a non-obvious string (Why
>> > > "pseudo"?  Why does the UIO user care that the config came from the
>> > > device
>> > > tree?) just to avoid setting off Greg's version number autoresponse
>> > > isn't
>> > > really helping anything.
As I mentioned before, this is not currently meaningful for us, and maybe the 
better
way is to set it optionally for uio, but it belongs to uio core, which is a 
framework for
different kind of drivers or devices, but not only for us. So I guess this is 
not a thing
troubles and arguing about this is really helpless.

>> > > 
>> > > > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> > > > +  {   .compatible = "uio,mpc85xx-cache-sram", },
>> > 
>> > Form is , and "uio" is not a vendor (and never will be).
>> > 
>> 
>> Should have been something like "fsl,mpc85xx-cache-sram-uio", and if it is
>> to be defined with module parameters, this would be user defined.
>> Anyway, , should always be used.
>> 
>> > > > +  {},
>> > > > +};
>> > > > +
>> > > > +static struct platform_driver uio_fsl_85xx_cache_sram = {
>> > > > +  .probe = uio_fsl_85xx_cache_sram_probe,
>> > > > +  .remove = uio_fsl_85xx_cache_sram_remove,
>> > > > +  .driver = {
>> > > > +  .name = DRIVER_NAME,
>> > > > +  .owner = THIS_MODULE,
>> > > > +  .of_match_table = uio_mpc85xx_l2ctlr_of_match,
>> > > > +  },
>> > > > +};
>> > > 
>> > > Greg's comment notwithstanding, I really don't think this belongs in the
>> > > device tree (and if I do get overruled on that point, it at least needs
>> > > a
>> > > binding document).  Let me try to come up with a patch for dynamic
>> > > allocation.
>> > 
>> > Agreed. "UIO" bindings have long been rejected.
>> > 
>> 
>> Sounds it is. And does the modification below fit well?
>> ---
>> -static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> -   {   .compatible = "uio,mpc85xx-cache-sram", },
>> -   {},
>> +#ifdef CONFIG_OF
>> +static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
>> +   { /* This is filled with module_parm */ },
>> +   { /* Sentinel */ },
>>  };
>> +MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
>> +module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
>> +   sizeof(uio_fsl_85xx_cache_sram_of_match[0].compa
>> tible), 0);
>> +MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-
>> uio");
>> +#endif
>
>No.  The point is that you wouldn't be configuring this with the device tree
>at all.
>
It was to fit for the unbinding requriement. And I mentioned what if I want to 
create
more than one device and each owns multiple uiomaps?
Devicetree is definitely better choice to solve the problem and make the driver 
more
convenient for users to use. Pseudo device is a device and a device. Or else 
device
tree should be hardware-only-devicetree.

The point is why we left the better choice and write a plenty of redundant codes
to parse the module parameters?

Further more, I don't think there is enough reason for the lower driver mpc85xx 
cache-sram
to restrict other from using it in a way of module param or dynamic allocation 
with ioctl or so.

UIO is there and all these parts cooperate well to make the cache-sram more 
useful and better
resolve user requirement.

I will update the patch with the diff applied in v5.

Thanks,
Wenhu

Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[王文虎]

>> On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
>> > +#define UIO_INFO_VER  "devicetree,pseudo"
>> 
>> What does this mean?  Changing a number into a non-obvious string (Why
>> "pseudo"?  Why does the UIO user care that the config came from the device
>> tree?) just to avoid setting off Greg's version number autoresponse isn't
>> really helping anything.
>> 
>> > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> > +  {   .compatible = "uio,mpc85xx-cache-sram", },
>
>Form is , and "uio" is not a vendor (and never will be).
>
Should have been something like "fsl,mpc85xx-cache-sram-uio", and if it is
to be defined with module parameters, this would be user defined.
Anyway, , should always be used.

>> > +  {},
>> > +};
>> > +
>> > +static struct platform_driver uio_fsl_85xx_cache_sram = {
>> > +  .probe = uio_fsl_85xx_cache_sram_probe,
>> > +  .remove = uio_fsl_85xx_cache_sram_remove,
>> > +  .driver = {
>> > +  .name = DRIVER_NAME,
>> > +  .owner = THIS_MODULE,
>> > +  .of_match_table = uio_mpc85xx_l2ctlr_of_match,
>> > +  },
>> > +};
>> 
>> Greg's comment notwithstanding, I really don't think this belongs in the
>> device tree (and if I do get overruled on that point, it at least needs a
>> binding document).  Let me try to come up with a patch for dynamic 
>> allocation.
>
>Agreed. "UIO" bindings have long been rejected.
>
Sounds it is. And does the modification below fit well?
---
-static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
-   {   .compatible = "uio,mpc85xx-cache-sram", },
-   {},
+#ifdef CONFIG_OF
+static struct of_device_id uio_fsl_85xx_cache_sram_of_match[] = {
+   { /* This is filled with module_parm */ },
+   { /* Sentinel */ },
 };
+MODULE_DEVICE_TABLE(of, uio_fsl_85xx_cache_sram_of_match);
+module_param_string(of_id, uio_fsl_85xx_cache_sram_of_match[0].compatible,
+   
sizeof(uio_fsl_85xx_cache_sram_of_match[0].compatible), 0);
+MODULE_PARM_DESC(of_id, "platform device id to be handled by cache-sram-uio");
+#endif
 
 static struct platform_driver uio_fsl_85xx_cache_sram = {
.probe = uio_fsl_85xx_cache_sram_probe,
.remove = uio_fsl_85xx_cache_sram_remove,
.driver = {
.name = DRIVER_NAME,
-   .owner = THIS_MODULE,
-   .of_match_table = uio_mpc85xx_l2ctlr_of_match,
+   .of_match_table = 
of_match_ptr(uio_fsl_85xx_cache_sram_of_match),
},
 };

Regards,
Wenhu

Re:Re: [PATCH RESEND,v3,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[王文虎]

Hi, Christophe,
dev_kzalloc really looks better. I will update the patch with the comments 
addressed.

Thanks,
Wenhu

From: Christophe Leroy  Date: 2020-04-16 19:49:01
To:Wang Wenhu ,gre...@linuxfoundation.org,
linux-ker...@vger.kernel.org,o...@buserror.net,linuxppc-dev@lists.ozlabs.org
 cc: ker...@vivo.com,Michael Ellerman 
Subject: Re: [PATCH RESEND,v3,4/4] drivers: uio: new driver for 
fsl_85xx_cache_sram>
>
>Le 16/04/2020 à 13:16, Wang Wenhu a écrit :
>> A driver for freescale 85xx platforms to access the Cache-Sram form
>> user level. This is extremely helpful for some user-space applications
>> that require high performance memory accesses.
>> 
>> Cc: Greg Kroah-Hartman 
>> Cc: Christophe Leroy 
>> Cc: Scott Wood 
>> Cc: Michael Ellerman 
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Signed-off-by: Wang Wenhu 
>> ---
>> Changes since v1:
>>   * Addressed comments from Greg K-H
>>   * Moved kfree(info->name) into uio_info_free_internal()
>> Changes since v2:
>>   * Addressed comments from Greg, Scott and Christophe
>>   * Use "uiomem->internal_addr" as if condition for sram memory free,
>> and memset the uiomem entry
>>   * Modified of_match_table make the driver apart from Cache-Sram HW info
>> which belong to the HW level driver fsl_85xx_cache_sram to match
>>   * Use roundup_pow_of_two for align calculation
>>   * Remove useless clear block of uiomem entries.
>>   * Use UIO_INFO_VER micro for info->version, and define it as
>> "devicetree,pseudo", meaning this is pseudo device and probed from
>> device tree configuration
>>   * Select FSL_85XX_CACHE_SRAM rather than depends on it
>> ---
>>   drivers/uio/Kconfig   |   9 ++
>>   drivers/uio/Makefile  |   1 +
>>   drivers/uio/uio_fsl_85xx_cache_sram.c | 158 ++
>>   3 files changed, 168 insertions(+)
>>   create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>> 
>> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
>> index 202ee81cfc2b..9c3b47461b71 100644
>> --- a/drivers/uio/Kconfig
>> +++ b/drivers/uio/Kconfig
>> @@ -105,6 +105,15 @@ config UIO_NETX
>>To compile this driver as a module, choose M here; the module
>>will be called uio_netx.
>>   
>> +config UIO_FSL_85XX_CACHE_SRAM
>> +tristate "Freescale 85xx Cache-Sram driver"
>> +depends on FSL_SOC_BOOKE && PPC32
>> +select FSL_85XX_CACHE_SRAM
>> +help
>> +  Generic driver for accessing the Cache-Sram form user level. This
>> +  is extremely helpful for some user-space applications that require
>> +  high performance memory accesses.
>> +
>>   config UIO_FSL_ELBC_GPCM
>>  tristate "eLBC/GPCM driver"
>>  depends on FSL_LBC
>> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
>> index c285dd2a4539..be2056cffc21 100644
>> --- a/drivers/uio/Makefile
>> +++ b/drivers/uio/Makefile
>> @@ -10,4 +10,5 @@ obj-$(CONFIG_UIO_NETX) += uio_netx.o
>>   obj-$(CONFIG_UIO_PRUSS) += uio_pruss.o
>>   obj-$(CONFIG_UIO_MF624) += uio_mf624.o
>>   obj-$(CONFIG_UIO_FSL_ELBC_GPCM)+= uio_fsl_elbc_gpcm.o
>> +obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM)   += uio_fsl_85xx_cache_sram.o
>>   obj-$(CONFIG_UIO_HV_GENERIC)   += uio_hv_generic.o
>> diff --git a/drivers/uio/uio_fsl_85xx_cache_sram.c 
>> b/drivers/uio/uio_fsl_85xx_cache_sram.c
>> new file mode 100644
>> index ..8701df695307
>> --- /dev/null
>> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
>> @@ -0,0 +1,158 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
>> + * Copyright (C) 2020 Wang Wenhu 
>> + * All rights reserved.
>> + */
>> +
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +#include 
>> +
>> +#define DRIVER_NAME "uio_fsl_85xx_cache_sram"
>> +#define UIO_INFO_VER"devicetree,pseudo"
>> +#define UIO_NAME"uio_cache_sram"
>> +
>> +static void uio_info_free_internal(struct uio_info *info)
>> +{
>> +struct uio_mem *uiomem = info->mem;
>> +
>> +while (uiomem < >mem[MAX_UIO_MAPS]) {
>
>As suggested by Scott, maybe it would be better to use a loop with an 
>index, something like
>
>   for (i = 0; i < MAX_UIO_MAPS; i++, uiomem++) {
>   struct uio_mem *uiomem = info->mem[i];
>
>> +if (uiomem->internal_addr) {
>> +mpc85xx_cache_sram_free(uiomem->internal_addr);
>> +kfree(uiomem->name);
>
>Unneeded when using devm_kstrdup(), see in the probe function.
>
>> +memset(uiomem, 0, sizeof(*uiomem));
>> +}
>> +uiomem++;
>> +}
>> +
>> +kfree(info->name);
>
>That's a bit unbalanced. This function is handy for the things allocated 
>inside the loop, but for the info->name allocated outside the loop, it 
>should be released outside this function.
>
>At the end if you use devm_kstrdup(), it will void anyway.
>
>> +}
>> +
>> +static int uio_fsl_85xx_cache_sram_probe(struct platform_device *pdev)
>> 

Re: [PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram

[王文虎]

Hi, 
From: Christophe Leroy 
 Date: 2020-04-16 18:36:38
To:"王文虎" 
 cc: 
gre...@linuxfoundation.org,linux-ker...@vger.kernel.org,o...@buserror.net,linuxppc-dev@lists.ozlabs.org,ker...@vivo.com
Subject: Re: [PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram>
>
>Le 16/04/2020 à 11:29, 王文虎 a écrit :
>> Hi,
>> Seems there is something wrong with the server that multiple dumplications
>> of the v3 patches were sent out, please ignore the rest and take this newest
>> series as formal count.
>
>Which series ?
>
>It seems you sent 3 times, at 9:29, 9:41 and 9:49 (Paris Time)
>
> From the series of 9:29, I received patches 0 to 3
> From the series of 9:41, I received patches 0 to 3
> From the series of 9:49, I received patches 0 and 4.
>
>Looks like powerpc patchwork 
>(https://patchwork.ozlabs.org/project/linuxppc-dev/list/?submitter=78320) 
>got:
> From the series of 9:29, I received patches 0 to 4
> From the series of 9:41, I received patches 1 to 4
> From the series of 9:49, I received patches 1 to 4
>
>So this seems to be something wrong somewhere.
>
>Christophe
>

Hi Christophe,
Sorry again, and I don't know which best fit you all. I guess a RESEND tag 
might help.
So I will send another series with RESEND tag, please just drop all this three.

Hope that relly won't trouble you yelling, and hope the mail server would work 
well.

Thanks,
Wenhu
>> 
>> From: Wang Wenhu 
>> Date: 2020-04-16 15:49:14
>> To:  
>> gre...@linuxfoundation.org,linux-ker...@vger.kernel.org,o...@buserror.net,christophe.le...@c-s.fr,linuxppc-dev@lists.ozlabs.org
>> Cc:  ker...@vivo.com,Wang Wenhu 
>> Subject: [PATCH v3,0/4] drivers: uio: new driver 
>> uio_fsl_85xx_cache_sram>This series add a new uio driver for freescale 85xx 
>> platforms to
>>> access the Cache-Sram form user level. This is extremely helpful
>>> for the user-space applications that require high performance memory
>>> accesses.
>>>
>>> It fixes the compile errors and warning of the hardware level drivers
>>> and implements the uio driver in uio_fsl_85xx_cache_sram.c.
>>>
>>> Changes since v1:
>>> * Addressed comments from Greg K-H
>>> * Moved kfree(info->name) into uio_info_free_internal()
>>>
>>> Changes since v2:
>>> * Drop the patch that modifies Kconfigs of arch/powerpc/platforms
>>>and modified the sequence of patches:
>>> 01:dropped, 02->03, 03->02, 04->01, 05->04
>>> * Addressed comments from Greg, Scott and Christophe
>>> * Use "uiomem->internal_addr" as if condition for sram memory free,
>>>and memset the uiomem entry
>>> * Modified of_match_table make the driver apart from Cache-Sram HW info
>>>which belong to the HW level driver fsl_85xx_cache_sram to match
>>> * Use roundup_pow_of_two for align calc(really learned a lot from 
>>> Christophe)
>>> * Remove useless clear block of uiomem entries.
>>> * Use UIO_INFO_VER micro for info->version, and define it as
>>>"devicetree,pseudo", meaning this is pseudo device and probed from
>>>device tree configuration
>>> * Select FSL_85XX_CACHE_SRAM rather than depends on it
>>>
>>> Wang Wenhu (4):
>>>   powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
>>>   powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
>>>   powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
>>>   drivers: uio: new driver for fsl_85xx_cache_sram
>>>
>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c |   3 +-
>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c |   1 +
>>> drivers/uio/Kconfig   |   9 ++
>>> drivers/uio/Makefile  |   1 +
>>> drivers/uio/uio_fsl_85xx_cache_sram.c | 158 ++
>>> 5 files changed, 171 insertions(+), 1 deletion(-)
>>> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>>>
>>> -- 
>>> 2.17.1
>>>
>> 
>> 

Re:[PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram

[王文虎]

Hi,
Seems there is something wrong with the server that multiple dumplications
of the v3 patches were sent out, please ignore the rest and take this newest
series as formal count.

Thanks,
Wenhu

From: Wang Wenhu 
Date: 2020-04-16 15:49:14
To:  
gre...@linuxfoundation.org,linux-ker...@vger.kernel.org,o...@buserror.net,christophe.le...@c-s.fr,linuxppc-dev@lists.ozlabs.org
Cc:  ker...@vivo.com,Wang Wenhu 
Subject: [PATCH v3,0/4] drivers: uio: new driver uio_fsl_85xx_cache_sram>This 
series add a new uio driver for freescale 85xx platforms to
>access the Cache-Sram form user level. This is extremely helpful
>for the user-space applications that require high performance memory
>accesses.
>
>It fixes the compile errors and warning of the hardware level drivers
>and implements the uio driver in uio_fsl_85xx_cache_sram.c.
>
>Changes since v1:
> * Addressed comments from Greg K-H
> * Moved kfree(info->name) into uio_info_free_internal()
>
>Changes since v2:
> * Drop the patch that modifies Kconfigs of arch/powerpc/platforms
>   and modified the sequence of patches:
>01:dropped, 02->03, 03->02, 04->01, 05->04
> * Addressed comments from Greg, Scott and Christophe
> * Use "uiomem->internal_addr" as if condition for sram memory free,
>   and memset the uiomem entry
> * Modified of_match_table make the driver apart from Cache-Sram HW info
>   which belong to the HW level driver fsl_85xx_cache_sram to match
> * Use roundup_pow_of_two for align calc(really learned a lot from Christophe)
> * Remove useless clear block of uiomem entries.
> * Use UIO_INFO_VER micro for info->version, and define it as
>   "devicetree,pseudo", meaning this is pseudo device and probed from
>   device tree configuration
> * Select FSL_85XX_CACHE_SRAM rather than depends on it
>
>Wang Wenhu (4):
>  powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr
>  powerpc: sysdev: fix compile error for fsl_85xx_cache_sram
>  powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram
>  drivers: uio: new driver for fsl_85xx_cache_sram
>
> arch/powerpc/sysdev/fsl_85xx_cache_sram.c |   3 +-
> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c |   1 +
> drivers/uio/Kconfig   |   9 ++
> drivers/uio/Makefile  |   1 +
> drivers/uio/uio_fsl_85xx_cache_sram.c | 158 ++
> 5 files changed, 171 insertions(+), 1 deletion(-)
> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>
>-- 
>2.17.1
>

Re:Re: [PATCH v3] powerpc/fsl-85xx: fix compile error

[王文虎]

From: Michael Ellerman 
 Date: 2020-03-17 19:22:13
To:"王文虎" 
 cc: Benjamin Herrenschmidt ,Paul Mackerras 
,Allison Randal ,Richard Fontana 
,Greg Kroah-Hartman ,Thomas 
Gleixner 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,triv...@kernel.org,ker...@vivo.com,stable
 
Subject: Re: [PATCH v3] powerpc/fsl-85xx: fix compile error>王文虎 
 writes:
>> From: Michael Ellerman 
>>  Date: 2020-03-16 17:41:12
>> To:WANG Wenhu ,Benjamin Herrenschmidt 
>> ,Paul Mackerras ,WANG Wenhu 
>> ,Allison Randal ,Richard Fontana 
>> ,Greg Kroah-Hartman ,Thomas 
>> Gleixner 
>> ,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>>  cc: triv...@kernel.org,ker...@vivo.com,stable 
>> Subject: Re: [PATCH v3] powerpc/fsl-85xx: fix compile error>WANG Wenhu 
>>  writes:
>>>> Include "linux/of_address.h" to fix the compile error for
>>>> mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
>>>>
>>>>   CC  arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function 
>>>> ‘mpc85xx_l2ctlr_of_probe’:
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration 
>>>> of function ‘of_iomap’; did you mean ‘pci_iomap’? 
>>>> [-Werror=implicit-function-declaration]
>>>>   l2ctlr = of_iomap(dev->dev.of_node, 0);
>>>>^~~~
>>>>pci_iomap
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes 
>>>> pointer from integer without a cast [-Werror=int-conversion]
>>>>   l2ctlr = of_iomap(dev->dev.of_node, 0);
>>>>  ^
>>>> cc1: all warnings being treated as errors
>>>> scripts/Makefile.build:267: recipe for target 
>>>> 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
>>>> make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
>>>>
>>>> Fixes: commit 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>>>
>>>The syntax is:
>>>
>>>Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>>>
>>>> Cc: stable 
>>>
>>>The commit above went into v2.6.37.
>>>
>>>So no one has noticed this bug since then, how? Or did something else
>>>change to expose the problem?
>>
>> Actually a hard question to answer cause it also left me scratching for long.
>> However, I can not find right or definite answer.
>
>Oh, actually it's fairly straight forward, the code can't be built at
>all in upstream because CONFIG_FSL_85XX_CACHE_SRAM is not selectable or
>selected by anything.

Yeah, sure that is the reason, and I meant it was hard to figure out why
nobody had ever compiled the driver with FSL_85XX_CACHE_SRAM enabled
until me.
>
>You sent a patch previously to make it selectable, which Scott thought
>was a bad idea.
>
>So this whole file is dead code as far as I'm concerned, so patches for
>it definitely do not need to go to stable.
>
>If you want to add a user for it then please send a series doing that,
>and this commit can be the first.

For this, as you mentioned, it is dead and do not need to be applied to any 
stable.
And I recommand the patch as a unit itself cause our module which uses
it is still under developing, and the module itself would be taken as a
complete logical block. Also it would take some time.

Thanks, Wenhu
>
>cheers

Re: [PATCH v3] powerpc/fsl-85xx: fix compile error

[王文虎]

From: Michael Ellerman 
 Date: 2020-03-16 17:41:12
To:WANG Wenhu ,Benjamin Herrenschmidt 
,Paul Mackerras ,WANG Wenhu 
,Allison Randal ,Richard Fontana 
,Greg Kroah-Hartman ,Thomas 
Gleixner 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
 cc: triv...@kernel.org,ker...@vivo.com,stable 
Subject: Re: [PATCH v3] powerpc/fsl-85xx: fix compile error>WANG Wenhu 
 writes:
>> Include "linux/of_address.h" to fix the compile error for
>> mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
>>
>>   CC  arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of 
>> function ‘of_iomap’; did you mean ‘pci_iomap’? 
>> [-Werror=implicit-function-declaration]
>>   l2ctlr = of_iomap(dev->dev.of_node, 0);
>>^~~~
>>pci_iomap
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer 
>> from integer without a cast [-Werror=int-conversion]
>>   l2ctlr = of_iomap(dev->dev.of_node, 0);
>>  ^
>> cc1: all warnings being treated as errors
>> scripts/Makefile.build:267: recipe for target 
>> 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
>> make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
>>
>> Fixes: commit 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>
>The syntax is:
>
>Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>
>> Cc: stable 
>
>The commit above went into v2.6.37.
>
>So no one has noticed this bug since then, how? Or did something else
>change to expose the problem?

Actually a hard question to answer cause it also left me scratching for long.
However, I can not find right or definite answer.

A convincing explanation is the driver has not been in use since v2.6.37,
but somehow, we are to use it recently.
Anyway, it's better for me as well as no harm to anyone to fix it even though.

Thanks, Wenhu
>
>cheers

Re:Re: [PATCH v2] powerpc/fsl-85xx: fix compile error

[王文虎]

发件人：Christophe Leroy 
发送日期：2020-03-14 03:24:20
收件人："王文虎" 
抄送人：Benjamin Herrenschmidt ,Paul Mackerras 
,Michael Ellerman ,Richard Fontana 
,Kate Stewart ,Allison 
Randal ,Thomas Gleixner 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,ker...@vivo.com,triv...@kernel.org
主题：Re: [PATCH v2] powerpc/fsl-85xx: fix compile error>
>
>Le 13/03/2020 à 19:17, 王文虎 a écrit :
>> 发件人：Christophe Leroy 
>> 发送日期：2020-03-14 01:45:11
>> 收件人：WANG Wenhu ,Benjamin Herrenschmidt 
>> ,Paul Mackerras ,Michael 
>> Ellerman ,Richard Fontana ,Kate 
>> Stewart ,Allison Randal 
>> ,Thomas Gleixner 
>> ,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>> 抄送人：ker...@vivo.com,triv...@kernel.org
>> 主题：Re: [PATCH v2] powerpc/fsl-85xx: fix compile error>
>>>
>>> Le 13/03/2020 à 18:19, WANG Wenhu a écrit :
>>>> Include "linux/of_address.h" to fix the compile error for
>>>> mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
>>>>
>>>> CC  arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function 
>>>> ‘mpc85xx_l2ctlr_of_probe’:
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration 
>>>> of function ‘of_iomap’; did you mean ‘pci_iomap’? 
>>>> [-Werror=implicit-function-declaration]
>>>> l2ctlr = of_iomap(dev->dev.of_node, 0);
>>>>  ^~~~
>>>>  pci_iomap
>>>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes 
>>>> pointer from integer without a cast [-Werror=int-conversion]
>>>> l2ctlr = of_iomap(dev->dev.of_node, 0);
>>>>^
>>>> cc1: all warnings being treated as errors
>>>> scripts/Makefile.build:267: recipe for target 
>>>> 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
>>>> make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
>>>>
>>>> Fixes: commit 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>>>
>>> Shouldn't you Cc stable as well ?
>> Pretty sure if it makes a difference(that I did not recognize).
>> Does the inconsistency of Cc lead to a failure on classification
>> or something else which may confuse you?
>
>See 
>https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/process/submitting-patches.rst?h=v5.1.9#n299
Seen, and I 'll do a repatch of v3 with stable being added
to sign-off area as Cc list. Thanks a lot.

Wenhu

>
>>>
>>>> Signed-off-by: WANG Wenhu 
>>>> ---
>>>
>>> What's the difference between v1 and v2 ?
>> The label field modification: "Fixed" -> "Fixes", which now is
>> identified successfully. Really sorry for the fault on v1.
>
>Ok. Usually people tell here (just below the ---) what is the difference 
>between the different versions. It helps people understand what the 
>changes are.
>
>Christophe

Re:Re: [PATCH v2] powerpc/fsl-85xx: fix compile error

[王文虎]

发件人：Christophe Leroy 
发送日期：2020-03-14 01:45:11
收件人：WANG Wenhu ,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,Richard Fontana ,Kate Stewart 
,Allison Randal ,Thomas 
Gleixner 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
抄送人：ker...@vivo.com,triv...@kernel.org
主题：Re: [PATCH v2] powerpc/fsl-85xx: fix compile error>
>
>Le 13/03/2020 à 18:19, WANG Wenhu a écrit :
>> Include "linux/of_address.h" to fix the compile error for
>> mpc85xx_l2ctlr_of_probe() when compiling fsl_85xx_cache_sram.c.
>> 
>>CC  arch/powerpc/sysdev/fsl_85xx_l2ctlr.o
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c: In function ‘mpc85xx_l2ctlr_of_probe’:
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:11: error: implicit declaration of 
>> function ‘of_iomap’; did you mean ‘pci_iomap’? 
>> [-Werror=implicit-function-declaration]
>>l2ctlr = of_iomap(dev->dev.of_node, 0);
>> ^~~~
>> pci_iomap
>> arch/powerpc/sysdev/fsl_85xx_l2ctlr.c:90:9: error: assignment makes pointer 
>> from integer without a cast [-Werror=int-conversion]
>>l2ctlr = of_iomap(dev->dev.of_node, 0);
>>   ^
>> cc1: all warnings being treated as errors
>> scripts/Makefile.build:267: recipe for target 
>> 'arch/powerpc/sysdev/fsl_85xx_l2ctlr.o' failed
>> make[2]: *** [arch/powerpc/sysdev/fsl_85xx_l2ctlr.o] Error 1
>> 
>> Fixes: commit 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
>
>Shouldn't you Cc stable as well ?
Pretty sure if it makes a difference(that I did not recognize). 
Does the inconsistency of Cc lead to a failure on classification
or something else which may confuse you?
>
>> Signed-off-by: WANG Wenhu 
>> ---
>
>What's the difference between v1 and v2 ?
The label field modification: "Fixed" -> "Fixes", which now is
identified successfully. Really sorry for the fault on v1.
>
>Christophe

Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable

[王文虎]

发件人：Scott Wood 
发送日期：2020-03-02 16:58:52
收件人："王文虎" 
抄送人：wangwenhu ,Kumar Gala 
,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,triv...@kernel.org,Rai
 Harninder 
主题：Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On Mon, 
2020-03-02 at 12:42 +0800, 王文虎 wrote:
>> 发件人：Scott Wood 
>> 发送日期：2020-03-01 07:12:58
>> 收件人："王文虎" 
>> 抄送人：wangwenhu ,Kumar Gala ,B
>> enjamin Herrenschmidt ,Paul Mackerras <
>> pau...@samba.org>,Michael Ellerman ,
>> linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,
>> triv...@kernel.org,Rai Harninder 
>> 主题：Re: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On
>> Tue, 2020-01-21 at 14:38 +0800, 王文虎 wrote:
>> > > 发件人：Scott Wood 
>> > > 发送日期：2020-01-21 13:49:59
>> > > 收件人："王文虎" 
>> > > 抄送人：wangwenhu ,Kumar Gala <
>> > > ga...@kernel.crashing.org>,B
>> > > enjamin Herrenschmidt ,Paul Mackerras <
>> > > pau...@samba.org>,Michael Ellerman ,
>> > > linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,
>> > > triv...@kernel.org,Rai Harninder 
>> > > 主题：Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On
>> > > Tue, 2020-01-21 at 13:20 +0800, 王文虎 wrote:
>> > > > > From: Scott Wood 
>> > > > > Date: 2020-01-21 11:25:25
>> > > > > To:  wangwenhu ,Kumar Gala <
>> > > > > ga...@kernel.crashing.org>,
>> > > > > Benjamin Herrenschmidt ,Paul Mackerras <
>> > > > > pau...@samba.org>,Michael Ellerman ,
>> > > > > linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>> > > > > Cc:  triv...@kernel.org,wenhu.w...@vivo.com,Rai Harninder <
>> > > > > harninder@nxp.com>
>> > > > > Subject: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM
>> > > > > configurable>On Mon, 2020-01-20 at 06:43 -0800, wangwenhu wrote:
>> > > > > > > From: wangwenhu 
>> > > > > > > 
>> > > > > > > When generating .config file with menuconfig on Freescale BOOKE
>> > > > > > > SOC, FSL_85XX_CACHE_SRAM is not configurable for the lack of
>> > > > > > > description in the Kconfig field, which makes it impossible
>> > > > > > > to support L2Cache-Sram driver. Add a description to make it
>> > > > > > > configurable.
>> > > > > > > 
>> > > > > > > Signed-off-by: wangwenhu 
>> > > > > > 
>> > > > > > The intent was that drivers using the SRAM API would select the
>> > > > > > symbol.  What
>> > > > > > is the use case for selecting it manually?
>> > > > > > 
>> > > > > 
>> > > > > With a repository of multiple products(meaning different defconfigs)
>> > > > > and
>> > > > > multiple
>> > > > > developers, the Kconfigs of the Kernel Source Tree change
>> > > > > frequently. So
>> > > > > the
>> > > > > "make menuconfig"
>> > > > > process is needed for defconfigs' re-generating or updating for the
>> > > > > complexity of dependencies
>> > > > > between different features defined in the Kconfigs.
>> > > > 
>> > > > That doesn't answer my question of how the SRAM code would be useful
>> > > > other
>> > > > than to some other driver that uses the API (which would use
>> > > > "select").  There
>> > > > is no userspace API.  You could use the kernel command line to
>> > > > configure
>> > > > the
>> > > > SRAM but you need to get the address of it for it to be useful.
>> > > > 
>> > > 
>> > > Like you've asked below, via /dev/mem or direct calling within the
>> > > Kernel.
>> > > And they are not submitted yes, under development.
>> > 
>> > If they are calling within the kernel, then whatever driver that is should
>> > select FSL_85XX_CACHE_SRAM.  Directly accessing /dev/mem without any way
>> > for
>> > the kernel to advertise where it is or which parts of SRAM are available
>> > for
>> > use sounds like a bad idea.
>> > 
>> 

Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable

[王文虎]

发件人：Scott Wood 
发送日期：2020-03-01 07:12:58
收件人："王文虎" 
抄送人：wangwenhu ,Kumar Gala 
,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,triv...@kernel.org,Rai
 Harninder 
主题：Re: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On 
Tue, 2020-01-21 at 14:38 +0800, 王文虎 wrote:
>> 发件人：Scott Wood 
>> 发送日期：2020-01-21 13:49:59
>> 收件人："王文虎" 
>> 抄送人：wangwenhu ,Kumar Gala ,B
>> enjamin Herrenschmidt ,Paul Mackerras <
>> pau...@samba.org>,Michael Ellerman ,
>> linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,
>> triv...@kernel.org,Rai Harninder 
>> 主题：Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On
>> Tue, 2020-01-21 at 13:20 +0800, 王文虎 wrote:
>> > > From: Scott Wood 
>> > > Date: 2020-01-21 11:25:25
>> > > To:  wangwenhu ,Kumar Gala <
>> > > ga...@kernel.crashing.org>,
>> > > Benjamin Herrenschmidt ,Paul Mackerras <
>> > > pau...@samba.org>,Michael Ellerman ,
>> > > linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>> > > Cc:  triv...@kernel.org,wenhu.w...@vivo.com,Rai Harninder <
>> > > harninder@nxp.com>
>> > > Subject: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM
>> > > configurable>On Mon, 2020-01-20 at 06:43 -0800, wangwenhu wrote:
>> > > > > From: wangwenhu 
>> > > > > 
>> > > > > When generating .config file with menuconfig on Freescale BOOKE
>> > > > > SOC, FSL_85XX_CACHE_SRAM is not configurable for the lack of
>> > > > > description in the Kconfig field, which makes it impossible
>> > > > > to support L2Cache-Sram driver. Add a description to make it
>> > > > > configurable.
>> > > > > 
>> > > > > Signed-off-by: wangwenhu 
>> > > > 
>> > > > The intent was that drivers using the SRAM API would select the
>> > > > symbol.  What
>> > > > is the use case for selecting it manually?
>> > > > 
>> > > 
>> > > With a repository of multiple products(meaning different defconfigs) and
>> > > multiple
>> > > developers, the Kconfigs of the Kernel Source Tree change frequently. So
>> > > the
>> > > "make menuconfig"
>> > > process is needed for defconfigs' re-generating or updating for the
>> > > complexity of dependencies
>> > > between different features defined in the Kconfigs.
>> > 
>> > That doesn't answer my question of how the SRAM code would be useful other
>> > than to some other driver that uses the API (which would use
>> > "select").  There
>> > is no userspace API.  You could use the kernel command line to configure
>> > the
>> > SRAM but you need to get the address of it for it to be useful.
>> > 
>> 
>> Like you've asked below, via /dev/mem or direct calling within the Kernel.
>> And they are not submitted yes, under development.
>
>If they are calling within the kernel, then whatever driver that is should
>select FSL_85XX_CACHE_SRAM.  Directly accessing /dev/mem without any way for
>the kernel to advertise where it is or which parts of SRAM are available for
>use sounds like a bad idea.

>
Yes, definitely. So like we enable the moulde which should selet 
FSL_85XX_CACHE_SRAM to build vmlinux, FSL_85XX_CACHE_SRAM 
could not be seleted because of the Kconfig definition problem 
which I am trying to fix now.  So would you please merge the patch 
for the convenience of later works depending on the driver.

Wenhu

>-Scott
>
>

Re:Re: [PATCH] powerpc/sysdev: fix compile errors

[王文虎]

From: Christophe Leroy 
 Date: 2020-01-21 16:37:07
To:"王文虎" ,Andrew Donnellan 
 cc: Kate Stewart ,Richard Fontana 
,Greg Kroah-Hartman 
,linux-ker...@vger.kernel.org,wangwenhu 
,Paul Mackerras 
,triv...@kernel.org,Thomas Gleixner 
,linuxppc-dev@lists.ozlabs.org,loneh...@hotmail.com
Subject: Re: [PATCH] powerpc/sysdev: fix compile errors>
>
>Le 21/01/2020 à 07:59, 王文虎 a écrit :
>> 发件人：Andrew Donnellan 
>> 发送日期：2020-01-21 14:13:07
>> 收件人：wangwenhu ,Benjamin Herrenschmidt 
>> ,Paul Mackerras ,Michael 
>> Ellerman ,Kate Stewart 
>> ,Greg Kroah-Hartman 
>> ,Richard Fontana ,Thomas 
>> Gleixner 
>> ,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>> 抄送人：triv...@kernel.org,loneh...@hotmail.com,wenhu.w...@vivo.com
>> 主题：Re: [PATCH] powerpc/sysdev: fix compile errors>On 21/1/20 4:31 pm, 
>> wangwenhu wrote:
>>>> From: wangwenhu 
>>>>
>>>> Include arch/powerpc/include/asm/io.h into fsl_85xx_cache_sram.c to
>>>> fix the implicit declaration compile errors when building Cache-Sram.
>>>>
>>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function 
>>>> ‘instantiate_cache_sram’:
>>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit 
>>>> declaration of function ‘ioremap_coherent’; did you mean 
>>>> ‘bitmap_complement’? [-Werror=implicit-function-declaration]
>>>> cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
>>>> ^~~~
>>>> bitmap_complement
>>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes 
>>>> pointer from integer without a cast [-Werror=int-conversion]
>>>> cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
>>>>   ^
>>>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit 
>>>> declaration of function ‘iounmap’; did you mean ‘roundup’? 
>>>> [-Werror=implicit-function-declaration]
>>>> iounmap(cache_sram->base_virt);
>>>> ^~~
>>>> roundup
>>>> cc1: all warnings being treated as errors
>>>>
>>>> Signed-off-by: wangwenhu 
>>>
>>> How long has this code been broken for?
>> 
>> It's been broken almost 15 months since the commit below:
>> "commit aa91796ec46339f2ed53da311bd3ea77a3e4dfe1
>> Author: Christophe Leroy 
>> Date:   Tue Oct 9 13:51:41 2018 +
>> 
>>  powerpc: don't use ioremap_prot() nor __ioremap() unless really needed."
>> 
>> And we are working on it now for further development.
>
>That's pretty surprising. That commit didn't change the iounmap(). It 
>only replaced ioremap_prot() by ioremap_coherent(). Both are defined in io.h
>
>Christophe
>

The compile error exists since the uploading of the driver
Details below:
1. "ioremap_flags" defined in "asm/io.h" was used primarily(Wed Oct 13 17:30:56 
2010):
(6db92cc9d07d: powerpc/85xx: add cache-sram support);
2. "ioremap_prot" was used to replace "ioremap_flags"
(40f1ce7fb7e8: powerpc: Remove ioremap_flags);
3. "ioremap_coherent" was used to replace "ioremap_prot":
(aa91796ec463: powerpc: don't use ioremap_prot() nor __ioremap() unless really 
needed.)

So I will do the re-patch with a "Fixed" tag and "" include 
modification.
Which commit should be referenced to append to "Fixed" tag? (No.1 or No.3 ?)

Wenhu

>> 
>>>
>>>> ---
>>>>arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
>>>>1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c 
>>>> b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> index f6c665dac725..29b6868eff7d 100644
>>>> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>>>> @@ -17,6 +17,7 @@
>>>>#include 
>>>>#include 
>>>>#include 
>>>> +#include 
>>>>
>>>>#include "fsl_85xx_cache_ctlr.h"
>>>>
>>>
>>> -- 
>>> Andrew Donnellan  OzLabs, ADL Canberra
>>> a...@linux.ibm.com IBM Australia Limited
>>>
>> 
>> Wenhu
>> 

Re:Re: [PATCH] powerpc/sysdev: fix compile errors

[王文虎]

发件人：Andrew Donnellan 
发送日期：2020-01-21 14:13:07
收件人：wangwenhu ,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,Kate Stewart ,Greg 
Kroah-Hartman ,Richard Fontana 
,Thomas Gleixner 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
抄送人：triv...@kernel.org,loneh...@hotmail.com,wenhu.w...@vivo.com
主题：Re: [PATCH] powerpc/sysdev: fix compile errors>On 21/1/20 4:31 pm, wangwenhu 
wrote:
>> From: wangwenhu 
>> 
>> Include arch/powerpc/include/asm/io.h into fsl_85xx_cache_sram.c to
>> fix the implicit declaration compile errors when building Cache-Sram.
>> 
>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c: In function 
>> ‘instantiate_cache_sram’:
>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:26: error: implicit declaration 
>> of function ‘ioremap_coherent’; did you mean ‘bitmap_complement’? 
>> [-Werror=implicit-function-declaration]
>>cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
>>^~~~
>>bitmap_complement
>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:97:24: error: assignment makes 
>> pointer from integer without a cast [-Werror=int-conversion]
>>cache_sram->base_virt = ioremap_coherent(cache_sram->base_phys,
>>  ^
>> arch/powerpc/sysdev/fsl_85xx_cache_sram.c:123:2: error: implicit declaration 
>> of function ‘iounmap’; did you mean ‘roundup’? 
>> [-Werror=implicit-function-declaration]
>>iounmap(cache_sram->base_virt);
>>^~~
>>roundup
>> cc1: all warnings being treated as errors
>> 
>> Signed-off-by: wangwenhu 
>
>How long has this code been broken for?

It's been broken almost 15 months since the commit below:
"commit aa91796ec46339f2ed53da311bd3ea77a3e4dfe1
Author: Christophe Leroy 
Date:   Tue Oct 9 13:51:41 2018 +

powerpc: don't use ioremap_prot() nor __ioremap() unless really needed."

And we are working on it now for further development.

>
>> ---
>>   arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 1 +
>>   1 file changed, 1 insertion(+)
>> 
>> diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c 
>> b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>> index f6c665dac725..29b6868eff7d 100644
>> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
>> @@ -17,6 +17,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>> 
>>   #include "fsl_85xx_cache_ctlr.h"
>> 
>
>-- 
>Andrew Donnellan  OzLabs, ADL Canberra
>a...@linux.ibm.com IBM Australia Limited
>

Wenhu

Re:Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable

[王文虎]

发件人：Scott Wood 
发送日期：2020-01-21 13:49:59
收件人："王文虎" 
抄送人：wangwenhu ,Kumar Gala 
,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org,triv...@kernel.org,Rai
 Harninder 
主题：Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On Tue, 
2020-01-21 at 13:20 +0800, 王文虎 wrote:
>> From: Scott Wood 
>> Date: 2020-01-21 11:25:25
>> To:  wangwenhu ,Kumar Gala ,
>> Benjamin Herrenschmidt ,Paul Mackerras <
>> pau...@samba.org>,Michael Ellerman ,
>> linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
>> Cc:  triv...@kernel.org,wenhu.w...@vivo.com,Rai Harninder <
>> harninder@nxp.com>
>> Subject: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM
>> configurable>On Mon, 2020-01-20 at 06:43 -0800, wangwenhu wrote:
>> > > From: wangwenhu 
>> > > 
>> > > When generating .config file with menuconfig on Freescale BOOKE
>> > > SOC, FSL_85XX_CACHE_SRAM is not configurable for the lack of
>> > > description in the Kconfig field, which makes it impossible
>> > > to support L2Cache-Sram driver. Add a description to make it
>> > > configurable.
>> > > 
>> > > Signed-off-by: wangwenhu 
>> > 
>> > The intent was that drivers using the SRAM API would select the
>> > symbol.  What
>> > is the use case for selecting it manually?
>> > 
>> 
>> With a repository of multiple products(meaning different defconfigs) and
>> multiple
>> developers, the Kconfigs of the Kernel Source Tree change frequently. So the
>> "make menuconfig"
>> process is needed for defconfigs' re-generating or updating for the
>> complexity of dependencies
>> between different features defined in the Kconfigs.
>
>That doesn't answer my question of how the SRAM code would be useful other
>than to some other driver that uses the API (which would use "select").  There
>is no userspace API.  You could use the kernel command line to configure the
>SRAM but you need to get the address of it for it to be useful.
>

Like you've asked below, via /dev/mem or direct calling within the Kernel.
And they are not submitted yes, under development.

>> > Since this code was added almost ten years ago and there are still no (in-
>> > tree?) users of the API, we should just remove the sram code (unless this
>> > prods someone to submit such a user very soon).
>> > 
>> 
>> Yes, pretty long a time. But we DO really use the API now for
>> PPCE500/Freescale SoC.
>
>I do not see any users in the kernel tree.  Are you talking about out-of-tree
>code, or something that you've submitted or will submit soon?  Or are you
>accessing it via /dev/mem?
>

Both, but not submitted yet, and partly under development.

>> Like sometimes we need to reset the whole RAM, then the L2-Cache would be
>> used as
>> SRAM for backup using. Since it is useful for us now, a re-consideration is
>> recommanded.
>
>Where is the code that would do this?
>

Currently under development, and not submitted yet.

>-Scott
>> 
>

Wenhu

Re:[PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable

[王文虎]

From: Scott Wood 
Date: 2020-01-21 11:25:25
To:  wangwenhu ,Kumar Gala 
,Benjamin Herrenschmidt 
,Paul Mackerras ,Michael Ellerman 
,linuxppc-dev@lists.ozlabs.org,linux-ker...@vger.kernel.org
Cc:  triv...@kernel.org,wenhu.w...@vivo.com,Rai Harninder 

Subject: Re: [PATCH] powerpc/Kconfig: Make FSL_85XX_CACHE_SRAM configurable>On 
Mon, 2020-01-20 at 06:43 -0800, wangwenhu wrote:
>> From: wangwenhu 
>> 
>> When generating .config file with menuconfig on Freescale BOOKE
>> SOC, FSL_85XX_CACHE_SRAM is not configurable for the lack of
>> description in the Kconfig field, which makes it impossible
>> to support L2Cache-Sram driver. Add a description to make it
>> configurable.
>> 
>> Signed-off-by: wangwenhu 
>
>The intent was that drivers using the SRAM API would select the symbol.  What
>is the use case for selecting it manually?
>

With a repository of multiple products(meaning different defconfigs) and 
multiple
developers, the Kconfigs of the Kernel Source Tree change frequently. So the 
"make menuconfig"
process is needed for defconfigs' re-generating or updating for the complexity 
of dependencies
between different features defined in the Kconfigs.

>Since this code was added almost ten years ago and there are still no (in-
>tree?) users of the API, we should just remove the sram code (unless this
>prods someone to submit such a user very soon).
>

Yes, pretty long a time. But we DO really use the API now for PPCE500/Freescale 
SoC.
Like sometimes we need to reset the whole RAM, then the L2-Cache would be used 
as
SRAM for backup using. Since it is useful for us now, a re-consideration is 
recommanded.

>-Scott
>
>

--
Wenhu
vivo