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

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

[Wang Wenhu]

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

Changes since v3:
 * Addressed comments from Christophe(use devm_xxx memory alloc interfaces)

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     | 148 ++++++++++++++++++++++
 5 files changed, 161 insertions(+), 1 deletion(-)
 create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c

-- 
2.17.1

[PATCH v4,1/4] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr

[Wang Wenhu]

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

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
Changes since v3:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
index 2d0af0c517bb..7533572492f0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
+++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
@@ -10,6 +10,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/of_platform.h>
+#include <linux/of_address.h>
 #include <asm/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
-- 
2.17.1

[PATCH v4,2/4] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram

[Wang Wenhu]

Include linux/io.h into fsl_85xx_cache_sram.c to fix the
implicit-declaration compile error 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

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
Changes since v1:
 * None
Changes since v2:
 * None
Changes since v3:
 * None
---
 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..be3aef4229d7 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -17,6 +17,7 @@
 #include <linux/of_platform.h>
 #include <asm/pgtable.h>
 #include <asm/fsl_85xx_cache_sram.h>
+#include <linux/io.h>
 
 #include "fsl_85xx_cache_ctlr.h"
 
-- 
2.17.1

[PATCH v4,3/4] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram

[Wang Wenhu]

Function instantiate_cache_sram should not be linked into the init
section for its caller mpc85xx_l2ctlr_of_probe is none-__init.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Warning information:
  MODPOST vmlinux.o
WARNING: modpost: vmlinux.o(.text+0x1e540): Section mismatch in reference from the function mpc85xx_l2ctlr_of_probe() to the function .init.text:instantiate_cache_sram()
The function mpc85xx_l2ctlr_of_probe() references
the function __init instantiate_cache_sram().
This is often because mpc85xx_l2ctlr_of_probe lacks a __init
annotation or the annotation of instantiate_cache_sram is wrong.
---
Changes since v1:
 * None
Changes since v2:
 * None
Changes since v3:
 * None
---
 arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
index be3aef4229d7..3de5ac8382c0 100644
--- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
+++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
@@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
 }
 EXPORT_SYMBOL(mpc85xx_cache_sram_free);
 
-int __init instantiate_cache_sram(struct platform_device *dev,
+int instantiate_cache_sram(struct platform_device *dev,
 		struct sram_parameters sram_params)
 {
 	int ret = 0;
-- 
2.17.1

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

[Wang Wenhu]

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 <gregkh@linuxfoundation.org>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>
Cc: Scott Wood <oss@buserror.net>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: linuxppc-dev@lists.ozlabs.org
Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>
---
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(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
Changes since v3:
 * Addressed comments from Christophe(use devm_xxx memory alloc interfaces)
---
 drivers/uio/Kconfig                   |   9 ++
 drivers/uio/Makefile                  |   1 +
 drivers/uio/uio_fsl_85xx_cache_sram.c | 148 ++++++++++++++++++++++++++
 3 files changed, 158 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 000000000000..cb339d1f9019
--- /dev/null
+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
@@ -0,0 +1,148 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
+ * All rights reserved.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/uio_driver.h>
+#include <linux/stringify.h>
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <asm/fsl_85xx_cache_sram.h>
+
+#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 = &info->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(&pdev->dev, sizeof(*info), GFP_KERNEL);
+	if (!info)
+		return -ENOMEM;
+
+	/* get optional uio name */
+	if (of_property_read_string(parent, "uio_name", &dt_name))
+		dt_name = UIO_NAME;
+
+	info->name = devm_kstrdup(&pdev->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", &mem_size);
+		if (ret) {
+			ret = -EINVAL;
+			goto err_out;
+		}
+
+		if (mem_size == 0) {
+			dev_err(&pdev->dev, "error cache-mem-size should not be 0\n");
+			ret = -EINVAL;
+			goto err_out;
+		}
+
+		virt = mpc85xx_cache_sram_alloc(mem_size, &phys,
+						roundup_pow_of_two(mem_size));
+		if (!virt) {
+			/* mpc85xx_cache_sram_alloc to define the real cause */
+			ret = -ENOMEM;
+			goto err_out;
+		}
+
+		uiomem->memtype = UIO_MEM_PHYS;
+		uiomem->addr = phys;
+		uiomem->size = mem_size;
+		uiomem->name = kstrdup(node->name, GFP_KERNEL);;
+		uiomem->internal_addr = virt;
+		uiomem++;
+
+		if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
+			dev_warn(&pdev->dev, "more than %d uio-maps for device.\n",
+				 MAX_UIO_MAPS);
+			break;
+		}
+	}
+
+	if (uiomem == info->mem) {
+		dev_err(&pdev->dev, "error no valid uio-map configuration found\n");
+		return -EINVAL;
+	}
+
+	info->version = UIO_INFO_VER;
+
+	/* register uio device */
+	if (uio_register_device(&pdev->dev, info)) {
+		dev_err(&pdev->dev, "error uio,cache-sram registration failed\n");
+		ret = -ENODEV;
+		goto err_out;
+	}
+
+	platform_set_drvdata(pdev, info);
+
+	return 0;
+err_out:
+	uio_info_free_internal(info);
+	return ret;
+}
+
+static int uio_fsl_85xx_cache_sram_remove(struct platform_device *pdev)
+{
+	struct uio_info *info = platform_get_drvdata(pdev);
+
+	uio_unregister_device(info);
+
+	uio_info_free_internal(info);
+
+	return 0;
+}
+
+static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
+	{	.compatible = "uio,mpc85xx-cache-sram",	},
+	{},
+};
+
+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,
+	},
+};
+
+module_platform_driver(uio_fsl_85xx_cache_sram);
+
+MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
+MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
-- 
2.17.1

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

[Christophe Leroy]

Le 16/04/2020 à 17:35, 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 <gregkh@linuxfoundation.org>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Scott Wood <oss@buserror.net>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> ---
> 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(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
> Changes since v3:
>   * Addressed comments from Christophe(use devm_xxx memory alloc interfaces)
> ---
>   drivers/uio/Kconfig                   |   9 ++
>   drivers/uio/Makefile                  |   1 +
>   drivers/uio/uio_fsl_85xx_cache_sram.c | 148 ++++++++++++++++++++++++++
>   3 files changed, 158 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 000000000000..cb339d1f9019
> --- /dev/null
> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
> @@ -0,0 +1,148 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
> + * Copyright (C) 2020 Wang Wenhu <wenhu.wang@vivo.com>
> + * All rights reserved.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/uio_driver.h>
> +#include <linux/stringify.h>
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <asm/fsl_85xx_cache_sram.h>
> +
> +#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 = &info->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(&pdev->dev, sizeof(*info), GFP_KERNEL);
> +	if (!info)
> +		return -ENOMEM;
> +
> +	/* get optional uio name */
> +	if (of_property_read_string(parent, "uio_name", &dt_name))
> +		dt_name = UIO_NAME;
> +
> +	info->name = devm_kstrdup(&pdev->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", &mem_size);
> +		if (ret) {
> +			ret = -EINVAL;
> +			goto err_out;
> +		}
> +
> +		if (mem_size == 0) {
> +			dev_err(&pdev->dev, "error cache-mem-size should not be 0\n");
> +			ret = -EINVAL;
> +			goto err_out;
> +		}
> +
> +		virt = mpc85xx_cache_sram_alloc(mem_size, &phys,
> +						roundup_pow_of_two(mem_size));
> +		if (!virt) {
> +			/* mpc85xx_cache_sram_alloc to define the real cause */
> +			ret = -ENOMEM;
> +			goto err_out;
> +		}
> +
> +		uiomem->memtype = UIO_MEM_PHYS;
> +		uiomem->addr = phys;
> +		uiomem->size = mem_size;
> +		uiomem->name = kstrdup(node->name, GFP_KERNEL);;
> +		uiomem->internal_addr = virt;
> +		uiomem++;
> +
> +		if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
> +			dev_warn(&pdev->dev, "more than %d uio-maps for device.\n",
> +				 MAX_UIO_MAPS);
> +			break;
> +		}
> +	}
> +
> +	if (uiomem == info->mem) {
> +		dev_err(&pdev->dev, "error no valid uio-map configuration found\n");
> +		return -EINVAL;
> +	}
> +
> +	info->version = UIO_INFO_VER;
> +
> +	/* register uio device */
> +	if (uio_register_device(&pdev->dev, info)) {
> +		dev_err(&pdev->dev, "error uio,cache-sram registration failed\n");
> +		ret = -ENODEV;
> +		goto err_out;
> +	}
> +
> +	platform_set_drvdata(pdev, info);
> +
> +	return 0;
> +err_out:
> +	uio_info_free_internal(info);
> +	return ret;
> +}
> +
> +static int uio_fsl_85xx_cache_sram_remove(struct platform_device *pdev)
> +{
> +	struct uio_info *info = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(info);
> +
> +	uio_info_free_internal(info);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> +	{	.compatible = "uio,mpc85xx-cache-sram",	},
> +	{},
> +};
> +
> +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,
> +	},
> +};
> +
> +module_platform_driver(uio_fsl_85xx_cache_sram);
> +
> +MODULE_AUTHOR("Wang Wenhu <wenhu.wang@vivo.com>");
> +MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v4,1/4] powerpc: sysdev: fix compile error for fsl_85xx_l2ctlr

[Christophe Leroy]

Le 16/04/2020 à 17:35, 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
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Scott Wood <oss@buserror.net>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> ---
> Changes since v1:
>   * None
> Changes since v2:
>   * None
> Changes since v3:
>   * None
> ---
>   arch/powerpc/sysdev/fsl_85xx_l2ctlr.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
> index 2d0af0c517bb..7533572492f0 100644
> --- a/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
> +++ b/arch/powerpc/sysdev/fsl_85xx_l2ctlr.c
> @@ -10,6 +10,7 @@
>   #include <linux/kernel.h>
>   #include <linux/module.h>
>   #include <linux/of_platform.h>
> +#include <linux/of_address.h>
>   #include <asm/io.h>
>   
>   #include "fsl_85xx_cache_ctlr.h"
> 

Re: [PATCH v4,2/4] powerpc: sysdev: fix compile error for fsl_85xx_cache_sram

[Christophe Leroy]

Le 16/04/2020 à 17:35, Wang Wenhu a écrit :
> Include linux/io.h into fsl_85xx_cache_sram.c to fix the
> implicit-declaration compile error 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
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Scott Wood <oss@buserror.net>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> ---
> Changes since v1:
>   * None
> Changes since v2:
>   * None
> Changes since v3:
>   * None
> ---
>   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..be3aef4229d7 100644
> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
> @@ -17,6 +17,7 @@
>   #include <linux/of_platform.h>
>   #include <asm/pgtable.h>
>   #include <asm/fsl_85xx_cache_sram.h>
> +#include <linux/io.h>
>   
>   #include "fsl_85xx_cache_ctlr.h"
>   
> 

Re: [PATCH v4,3/4] powerpc: sysdev: fix compile warning for fsl_85xx_cache_sram

[Christophe Leroy]

Le 16/04/2020 à 17:35, Wang Wenhu a écrit :
> Function instantiate_cache_sram should not be linked into the init
> section for its caller mpc85xx_l2ctlr_of_probe is none-__init.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Scott Wood <oss@buserror.net>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> Fixes: 6db92cc9d07d ("powerpc/85xx: add cache-sram support")
> Signed-off-by: Wang Wenhu <wenhu.wang@vivo.com>

Reviewed-by: Christophe Leroy <christophe.leroy@c-s.fr>

> 
> Warning information:
>    MODPOST vmlinux.o
> WARNING: modpost: vmlinux.o(.text+0x1e540): Section mismatch in reference from the function mpc85xx_l2ctlr_of_probe() to the function .init.text:instantiate_cache_sram()
> The function mpc85xx_l2ctlr_of_probe() references
> the function __init instantiate_cache_sram().
> This is often because mpc85xx_l2ctlr_of_probe lacks a __init
> annotation or the annotation of instantiate_cache_sram is wrong.
> ---
> Changes since v1:
>   * None
> Changes since v2:
>   * None
> Changes since v3:
>   * None
> ---
>   arch/powerpc/sysdev/fsl_85xx_cache_sram.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
> index be3aef4229d7..3de5ac8382c0 100644
> --- a/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
> +++ b/arch/powerpc/sysdev/fsl_85xx_cache_sram.c
> @@ -68,7 +68,7 @@ void mpc85xx_cache_sram_free(void *ptr)
>   }
>   EXPORT_SYMBOL(mpc85xx_cache_sram_free);
>   
> -int __init instantiate_cache_sram(struct platform_device *dev,
> +int instantiate_cache_sram(struct platform_device *dev,
>   		struct sram_parameters sram_params)
>   {
>   	int ret = 0;
> 

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

[Scott Wood]

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",	},
> +	{},
> +};
> +
> +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.

-Scott

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

[Rob Herring]

On Thu, Apr 16, 2020 at 02:59:36PM -0500, Scott Wood wrote:
> 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 <vendor>,<device> and "uio" is not a vendor (and never will be).

> > +	{},
> > +};
> > +
> > +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.

Rob

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 <vendor>,<device> 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, <vendor>,<device> 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: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[Scott Wood]

On Fri, 2020-04-17 at 10:31 +0800, 王文虎 wrote:
> > > 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 <vendor>,<device> 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, <vendor>,<device> 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.

-Scott

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 <vendor>,<device> 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, <vendor>,<device> 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

[Greg KH]

On Thu, Apr 16, 2020 at 11:58:29PM -0500, Scott Wood wrote:
> On Fri, 2020-04-17 at 10:31 +0800, 王文虎 wrote:
> > > > 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 <vendor>,<device> 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, <vendor>,<device> 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.

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?

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.

Remember, module parameters are NEVER the answer, this isn't the 1990's
anymore.

thanks,

greg k-h

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

[Scott Wood]

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.

-Scott

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 v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram

[Scott Wood]

On Fri, 2020-04-17 at 22:16 +0800, 王文虎 wrote:
> > 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.

No, duplicating the code makes no sense whatsoever.  Please just wait a bit
and I'll send a patch to have the existing driver expose a dynamic allocation
interface to userspace.

-Scott