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 <[email protected]>
Cc: Christophe Leroy <[email protected]>
Cc: Scott Wood <[email protected]>
Cc: Michael Ellerman <[email protected]>
Cc: [email protected]
Signed-off-by: Wang Wenhu <[email protected]>
---
Changes since v1:
* Addressed comments of Greg K-H
* Moved kfree(info->name) into uio_info_free_internal()
---
drivers/uio/Kconfig | 8 ++
drivers/uio/Makefile | 1 +
drivers/uio/uio_fsl_85xx_cache_sram.c | 182 ++++++++++++++++++++++++++
3 files changed, 191 insertions(+)
create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 202ee81cfc2b..afd38ec13de0 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig
@@ -105,6 +105,14 @@ 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_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..fb6903fdaddb
--- /dev/null
+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
@@ -0,0 +1,182 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
+ * Copyright (C) 2020 Wang Wenhu <[email protected]>
+ * 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_NAME "uio_cache_sram"
+
+static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
+ { .compatible = "uio,fsl,p2020-l2-cache-controller", },
+ { .compatible = "uio,fsl,p2010-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1020-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1011-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1013-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1022-l2-cache-controller", },
+ { .compatible = "uio,fsl,mpc8548-l2-cache-controller", },
+ { .compatible = "uio,fsl,mpc8544-l2-cache-controller", },
+ { .compatible = "uio,fsl,mpc8572-l2-cache-controller", },
+ { .compatible = "uio,fsl,mpc8536-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1021-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1012-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1025-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1016-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1024-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1015-l2-cache-controller", },
+ { .compatible = "uio,fsl,p1010-l2-cache-controller", },
+ { .compatible = "uio,fsl,bsc9131-l2-cache-controller", },
+ {},
+};
+
+static void uio_info_free_internal(struct uio_info *info)
+{
+ struct uio_mem *uiomem = &info->mem[0];
+
+ while (uiomem < &info->mem[MAX_UIO_MAPS]) {
+ if (uiomem->size) {
+ mpc85xx_cache_sram_free(uiomem->internal_addr);
+ kfree(uiomem->name);
+ }
+ uiomem++;
+ }
+
+ kfree(info->name);
+}
+
+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;
+ u32 align;
+ void *virt;
+ phys_addr_t phys;
+ int ret = -ENODEV;
+
+ /* alloc uio_info for one device */
+ info = kzalloc(sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ /* get optional uio name */
+ if (of_property_read_string(parent, "uio_name", &dt_name))
+ dt_name = UIO_NAME;
+
+ info->name = kstrdup(dt_name, GFP_KERNEL);
+ if (!info->name) {
+ ret = -ENOMEM;
+ goto err_info_free;
+ }
+
+ uiomem = &info->mem[0];
+ for_each_child_of_node(parent, node) {
+ ret = of_property_read_u32(node, "cache-mem-size", &mem_size);
+ if (ret) {
+ ret = -EINVAL;
+ goto err_info_free_internel;
+ }
+
+ if (mem_size == 0) {
+ dev_err(&pdev->dev, "cache-mem-size should not be 0\n");
+ ret = -EINVAL;
+ goto err_info_free_internel;
+ }
+
+ align = 2;
+ while (align < mem_size)
+ align *= 2;
+ virt = mpc85xx_cache_sram_alloc(mem_size, &phys, align);
+ if (!virt) {
+ /* mpc85xx_cache_sram_alloc to define the cause */
+ ret = -EINVAL;
+ goto err_info_free_internel;
+ }
+
+ 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;
+ }
+ }
+
+ while (uiomem < &info->mem[MAX_UIO_MAPS]) {
+ uiomem->size = 0;
+ ++uiomem;
+ }
+
+ if (info->mem[0].size == 0) {
+ dev_err(&pdev->dev, "error no valid uio-map configured\n");
+ ret = -EINVAL;
+ goto err_info_free_internel;
+ }
+
+ info->version = "0.1.0";
+
+ /* register uio device */
+ if (uio_register_device(&pdev->dev, info)) {
+ dev_err(&pdev->dev, "uio registration failed\n");
+ ret = -ENODEV;
+ goto err_info_free_internel;
+ }
+
+ platform_set_drvdata(pdev, info);
+
+ return 0;
+err_info_free_internel:
+ uio_info_free_internal(info);
+err_info_free:
+ kfree(info);
+err_out:
+ 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);
+
+ kfree(info);
+
+ return 0;
+}
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
+MODULE_ALIAS("platform:" DRIVER_NAME);
+MODULE_LICENSE("GPL v2");
--
2.17.1
Le 15/04/2020 à 17:24, 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 <[email protected]>
> Cc: Christophe Leroy <[email protected]>
> Cc: Scott Wood <[email protected]>
> Cc: Michael Ellerman <[email protected]>
> Cc: [email protected]
> Signed-off-by: Wang Wenhu <[email protected]>
> ---
> Changes since v1:
> * Addressed comments of Greg K-H
> * Moved kfree(info->name) into uio_info_free_internal()
> ---
> drivers/uio/Kconfig | 8 ++
> drivers/uio/Makefile | 1 +
> drivers/uio/uio_fsl_85xx_cache_sram.c | 182 ++++++++++++++++++++++++++
> 3 files changed, 191 insertions(+)
> create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
>
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 202ee81cfc2b..afd38ec13de0 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -105,6 +105,14 @@ 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_85XX_CACHE_SRAM
Is there any point having FSL_85XX_CACHE_SRAM without this ?
Should it be the other way round, leave FSL_85XX_CACHE_SRAM unselectable
by user, and this driver select FSL_85XX_CACHE_SRAM instead of depending
on it ?
> + 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..fb6903fdaddb
> --- /dev/null
> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2020 Vivo Communication Technology Co. Ltd.
> + * Copyright (C) 2020 Wang Wenhu <[email protected]>
> + * 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_NAME "uio_cache_sram"
> +
> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> + { .compatible = "uio,fsl,p2020-l2-cache-controller", },
> + { .compatible = "uio,fsl,p2010-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1020-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1011-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1013-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1022-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8548-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8544-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8572-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8536-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1021-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1012-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1025-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1016-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1024-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1015-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1010-l2-cache-controller", },
> + { .compatible = "uio,fsl,bsc9131-l2-cache-controller", },
> + {},
> +};
> +
> +static void uio_info_free_internal(struct uio_info *info)
> +{
> + struct uio_mem *uiomem = &info->mem[0];
> +
> + while (uiomem < &info->mem[MAX_UIO_MAPS]) {
> + if (uiomem->size) {
> + mpc85xx_cache_sram_free(uiomem->internal_addr);
> + kfree(uiomem->name);
> + }
> + uiomem++;
> + }
> +
> + kfree(info->name);
> +}
> +
> +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;
> + u32 align;
Align is not used outside of the for loop, it should be declared in the
loop block.
> + void *virt;
Same for virt
> + phys_addr_t phys;
Same for phys
> + int ret = -ENODEV;
It looks like this init value is unneeded, you should leave 'ret'
uninitialised (unless I missed some way out and you get a warning).
> +
> + /* alloc uio_info for one device */
> + info = kzalloc(sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + ret = -ENOMEM;
> + goto err_out;
Nothing special is done at err_out, you should instead do:
if (!info)
return -ENOMEM;
> + }
> +
> + /* get optional uio name */
> + if (of_property_read_string(parent, "uio_name", &dt_name))
> + dt_name = UIO_NAME;
> +
> + info->name = kstrdup(dt_name, GFP_KERNEL);
> + if (!info->name) {
> + ret = -ENOMEM;
> + goto err_info_free;
> + }
> +
> + uiomem = &info->mem[0];
I'd prefer
uiomem = info->mem;
> + for_each_child_of_node(parent, node) {
> + ret = of_property_read_u32(node, "cache-mem-size", &mem_size);
> + if (ret) {
> + ret = -EINVAL;
> + goto err_info_free_internel;
> + }
> +
> + if (mem_size == 0) {
> + dev_err(&pdev->dev, "cache-mem-size should not be 0\n");
> + ret = -EINVAL;
> + goto err_info_free_internel;
> + }
> +
> + align = 2;
> + while (align < mem_size)
> + align *= 2;
I think ilog2() or one of it friends should be used here, maybe
roundup_pow_of_two()
> + virt = mpc85xx_cache_sram_alloc(mem_size, &phys, align);
> + if (!virt) {
> + /* mpc85xx_cache_sram_alloc to define the cause */
> + ret = -EINVAL;
Should it be -ENOMEM like any allocation failure ?
> + goto err_info_free_internel;
> + }
> +
> + uiomem->memtype = UIO_MEM_PHYS;
> + uiomem->addr = phys;
> + uiomem->size = mem_size;
> + uiomem->name = kstrdup(node->name, GFP_KERNEL);;
> + uiomem->internal_addr = virt;
> + ++uiomem;
Usually people use
uiomem++;
> +
> + if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
I'd prefer
if (uiomem - info->mem >= MAX_UIO_MAPS) {
> + dev_warn(&pdev->dev, "more than %d uio-maps for device.\n",
> + MAX_UIO_MAPS);
> + break;
> + }
> + }
> +
> + while (uiomem < &info->mem[MAX_UIO_MAPS]) {
I'd prefer
while (uiomem - info->mem < MAX_UIO_MAPS) {
> + uiomem->size = 0;
> + ++uiomem;
> + }
> +
> + if (info->mem[0].size == 0) {
Is there any point in doing all the clearing loop above if it's to bail
out here ?
Wouldn't it be cleaner to do the test above the clearing loop, by just
checking whether uiomem is still equal to info->mem ?
> + dev_err(&pdev->dev, "error no valid uio-map configured\n");
> + ret = -EINVAL;
> + goto err_info_free_internel;
> + }
> +
> + info->version = "0.1.0";
Could you define some DRIVER_VERSION in the top of the file next to
DRIVER_NAME instead of hard coding in the middle on a function ?
> +
> + /* register uio device */
> + if (uio_register_device(&pdev->dev, info)) {
> + dev_err(&pdev->dev, "uio registration failed\n");
> + ret = -ENODEV;
> + goto err_info_free_internel;
> + }
> +
> + platform_set_drvdata(pdev, info);
> +
> + return 0;
> +err_info_free_internel:
Do you mean 'internal' instead of 'internel' ?
> + uio_info_free_internal(info);
> +err_info_free:
> + kfree(info);
> +err_out:
> + 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);
> +
> + kfree(info);
> +
> + return 0;
> +}
> +
> +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 <[email protected]>");
> +MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_LICENSE("GPL v2");
>
Christophe
On Wed, 2020-04-15 at 08:24 -0700, Wang Wenhu wrote:
> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> + { .compatible = "uio,fsl,p2020-l2-cache-controller", },
> + { .compatible = "uio,fsl,p2010-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1020-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1011-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1013-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1022-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8548-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8544-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8572-l2-cache-controller", },
> + { .compatible = "uio,fsl,mpc8536-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1021-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1012-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1025-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1016-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1024-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1015-l2-cache-controller", },
> + { .compatible = "uio,fsl,p1010-l2-cache-controller", },
> + { .compatible = "uio,fsl,bsc9131-l2-cache-controller", },
> + {},
> +};
NACK
The device tree describes the hardware, not what driver you want to bind the
hardware to, or how you want to allocate the resources. And even if defining
nodes for sram allocation were the right way to go, why do you have a separate
compatible for each chip when you're just describing software configuration?
Instead, have module parameters that take the sizes and alignments you'd like
to allocate and expose to userspace. Better still would be some sort of
dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment,
if it succeeds you can mmap it, and when the fd is closed the region is
freed).
-Scott
On Wed, 2020-04-15 at 18:52 +0200, Christophe Leroy wrote:
>
> Le 15/04/2020 à 17:24, Wang Wenhu a écrit :
> > +
> > + if (uiomem >= &info->mem[MAX_UIO_MAPS]) {
>
> I'd prefer
> if (uiomem - info->mem >= MAX_UIO_MAPS) {
>
> > + dev_warn(&pdev->dev, "more than %d uio-maps for
> > device.\n",
> > + MAX_UIO_MAPS);
> > + break;
> > + }
> > + }
> > +
> > + while (uiomem < &info->mem[MAX_UIO_MAPS]) {
>
> I'd prefer
>
> while (uiomem - info->mem < MAX_UIO_MAPS) {
>
I wouldn't. You're turning a simple comparison into a division and a
comparison (if the compiler doesn't optimize it back into the original form),
and making it less clear in the process.
Of course, working with array indices to begin with instead of incrementing a
pointer would be more idiomatic.
> > + uiomem->size = 0;
> > + ++uiomem;
> > + }
> > +
> > + if (info->mem[0].size == 0) {
>
> Is there any point in doing all the clearing loop above if it's to bail
> out here ?
>
> Wouldn't it be cleaner to do the test above the clearing loop, by just
> checking whether uiomem is still equal to info->mem ?
There's no point doing the clearing at all, since the array was allocated with
kzalloc().
> > + dev_err(&pdev->dev, "error no valid uio-map configured\n");
> > + ret = -EINVAL;
> > + goto err_info_free_internel;
> > + }
> > +
> > + info->version = "0.1.0";
>
> Could you define some DRIVER_VERSION in the top of the file next to
> DRIVER_NAME instead of hard coding in the middle on a function ?
That's what v1 had, and Greg KH said to remove it. I'm guessing that he
thought it was the common-but-pointless practice of having the driver print a
version number that never gets updated, rather than something the UIO API
(unfortunately, compared to a feature query interface) expects. That said,
I'm not sure what the value is of making it a macro since it should only be
used once, that use is self documenting, it isn't tunable, etc. Though if
this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
again, it should be UIO_VERSION, not DRIVER_VERSION).
Does this really need a three-part version scheme? What's wrong with a
version of "1", to be changed to "2" in the hopefully-unlikely event that the
userspace API changes? Assuming UIO is used for this at all, which doesn't
seem like a great fit to me.
-Scott
Yes, kzalloc() would clean the allocated areas and the init of remaining array
elements are redundant. I will remove the block in v3.
>> > + dev_err(&pdev->dev, "error no valid uio-map configured\n");
>> > + ret = -EINVAL;
>> > + goto err_info_free_internel;
>> > + }
>> > +
>> > + info->version = "0.1.0";
>>
>> Could you define some DRIVER_VERSION in the top of the file next to
>> DRIVER_NAME instead of hard coding in the middle on a function ?
>
>That's what v1 had, and Greg KH said to remove it. I'm guessing that he
>thought it was the common-but-pointless practice of having the driver print a
>version number that never gets updated, rather than something the UIO API
>(unfortunately, compared to a feature query interface) expects. That said,
>I'm not sure what the value is of making it a macro since it should only be
>used once, that use is self documenting, it isn't tunable, etc. Though if
>this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
>again, it should be UIO_VERSION, not DRIVER_VERSION).
>
>Does this really need a three-part version scheme? What's wrong with a
>version of "1", to be changed to "2" in the hopefully-unlikely event that the
>userspace API changes? Assuming UIO is used for this at all, which doesn't
>seem like a great fit to me.
>
>-Scott
>
As Scott mentioned, the version define as necessity by uio core but actually
useless for us here(and for many other type of devices I guess). So maybe the better
way is to set it optionally, but this belong first to uio core.
For the cache-sram uio driver, I will define an UIO_VERSION micro as a compromise
fit all wonders, no confusing as Greg first mentioned.
>> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>> + { .compatible = "uio,fsl,p2020-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p2010-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1020-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1011-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1013-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1022-l2-cache-controller", },
>> + { .compatible = "uio,fsl,mpc8548-l2-cache-controller", },
>> + { .compatible = "uio,fsl,mpc8544-l2-cache-controller", },
>> + { .compatible = "uio,fsl,mpc8572-l2-cache-controller", },
>> + { .compatible = "uio,fsl,mpc8536-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1021-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1012-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1025-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1016-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1024-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1015-l2-cache-controller", },
>> + { .compatible = "uio,fsl,p1010-l2-cache-controller", },
>> + { .compatible = "uio,fsl,bsc9131-l2-cache-controller", },
>> + {},
>> +};
>
>NACK
>
>The device tree describes the hardware, not what driver you want to bind the
>hardware to, or how you want to allocate the resources. And even if defining
>nodes for sram allocation were the right way to go, why do you have a separate
>compatible for each chip when you're just describing software configuration?
>
>Instead, have module parameters that take the sizes and alignments you'd like
>to allocate and expose to userspace. Better still would be some sort of
>dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment,
>if it succeeds you can mmap it, and when the fd is closed the region is
>freed).
>
>-Scott
>
Can not agree more. But what if I want to define more than one cache-sram uio devices?
How about use the device tree for pseudo uio cache-sram driver?
static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
{ .compatible = "uio,cache-sram", },
{},
};
Thanks,
Wenhu
Le 16/04/2020 à 07:22, Wang Wenhu a écrit :
> Yes, kzalloc() would clean the allocated areas and the init of remaining array
> elements are redundant. I will remove the block in v3.
>
>>>> + dev_err(&pdev->dev, "error no valid uio-map configured\n");
>>>> + ret = -EINVAL;
>>>> + goto err_info_free_internel;
>>>> + }
>>>> +
>>>> + info->version = "0.1.0";
>>>
>>> Could you define some DRIVER_VERSION in the top of the file next to
>>> DRIVER_NAME instead of hard coding in the middle on a function ?
>>
>> That's what v1 had, and Greg KH said to remove it. I'm guessing that he
>> thought it was the common-but-pointless practice of having the driver print a
>> version number that never gets updated, rather than something the UIO API
>> (unfortunately, compared to a feature query interface) expects. That said,
>> I'm not sure what the value is of making it a macro since it should only be
>> used once, that use is self documenting, it isn't tunable, etc. Though if
>> this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
>> again, it should be UIO_VERSION, not DRIVER_VERSION).
>>
>> Does this really need a three-part version scheme? What's wrong with a
>> version of "1", to be changed to "2" in the hopefully-unlikely event that the
>> userspace API changes? Assuming UIO is used for this at all, which doesn't
>> seem like a great fit to me.
>>
>> -Scott
>>
>
> As Scott mentioned, the version define as necessity by uio core but actually
> useless for us here(and for many other type of devices I guess). So maybe the better
> way is to set it optionally, but this belong first to uio core.
>
> For the cache-sram uio driver, I will define an UIO_VERSION micro as a compromise
> fit all wonders, no confusing as Greg first mentioned.
Yes I like it.
>
>>> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
>>> + { .compatible = "uio,fsl,p2020-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p2010-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1020-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1011-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1013-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1022-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,mpc8548-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,mpc8544-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,mpc8572-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,mpc8536-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1021-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1012-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1025-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1016-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1024-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1015-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,p1010-l2-cache-controller", },
>>> + { .compatible = "uio,fsl,bsc9131-l2-cache-controller", },
>>> + {},
>>> +};
>>
>> NACK
>>
>> The device tree describes the hardware, not what driver you want to bind the
>> hardware to, or how you want to allocate the resources. And even if defining
>> nodes for sram allocation were the right way to go, why do you have a separate
>> compatible for each chip when you're just describing software configuration?
>>
>> Instead, have module parameters that take the sizes and alignments you'd like
>> to allocate and expose to userspace. Better still would be some sort of
>> dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment,
>> if it succeeds you can mmap it, and when the fd is closed the region is
>> freed).
>>
>> -Scott
>>
>
> Can not agree more. But what if I want to define more than one cache-sram uio devices?
> How about use the device tree for pseudo uio cache-sram driver?
>
> static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> { .compatible = "uio,cache-sram", },
> {},
> };
>
You can still give it a name in line with your driver, ie:
"uio,mpc85xx-cache-sram"
After, it you have different behaviours depending on the compatible,
then you have to add a .data field which will tell the driver which
behaviour to implement.
Christophe
On Wed, Apr 15, 2020 at 02:26:55PM -0500, Scott Wood wrote:
> Instead, have module parameters that take the sizes and alignments you'd like
> to allocate and expose to userspace. Better still would be some sort of
> dynamic allocation (e.g. open a fd, ioctl to set the requested size/alignment,
> if it succeeds you can mmap it, and when the fd is closed the region is
> freed).
No module parameters please, this is not the 1990's.
Use device tree, that is what it is there for.
thanks,
greg k-h
On Wed, Apr 15, 2020 at 02:27:51PM -0500, Scott Wood wrote:
> > > + dev_err(&pdev->dev, "error no valid uio-map configured\n");
> > > + ret = -EINVAL;
> > > + goto err_info_free_internel;
> > > + }
> > > +
> > > + info->version = "0.1.0";
> >
> > Could you define some DRIVER_VERSION in the top of the file next to
> > DRIVER_NAME instead of hard coding in the middle on a function ?
>
> That's what v1 had, and Greg KH said to remove it. I'm guessing that he
> thought it was the common-but-pointless practice of having the driver print a
> version number that never gets updated, rather than something the UIO API
> (unfortunately, compared to a feature query interface) expects. That said,
> I'm not sure what the value is of making it a macro since it should only be
> used once, that use is self documenting, it isn't tunable, etc. Though if
> this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
> again, it should be UIO_VERSION, not DRIVER_VERSION).
>
> Does this really need a three-part version scheme? What's wrong with a
> version of "1", to be changed to "2" in the hopefully-unlikely event that the
> userspace API changes? Assuming UIO is used for this at all, which doesn't
> seem like a great fit to me.
No driver version numbers at all please, they do not make any sense when
the driver is included in the kernel tree.
greg k-h
On Thu, 2020-04-16 at 08:30 +0200, Greg KH wrote:
> On Wed, Apr 15, 2020 at 02:27:51PM -0500, Scott Wood wrote:
> > > > + dev_err(&pdev->dev, "error no valid uio-map
> > > > configured\n");
> > > > + ret = -EINVAL;
> > > > + goto err_info_free_internel;
> > > > + }
> > > > +
> > > > + info->version = "0.1.0";
> > >
> > > Could you define some DRIVER_VERSION in the top of the file next to
> > > DRIVER_NAME instead of hard coding in the middle on a function ?
> >
> > That's what v1 had, and Greg KH said to remove it. I'm guessing that he
> > thought it was the common-but-pointless practice of having the driver
> > print a
> > version number that never gets updated, rather than something the UIO API
> > (unfortunately, compared to a feature query interface) expects. That
> > said,
> > I'm not sure what the value is of making it a macro since it should only
> > be
> > used once, that use is self documenting, it isn't tunable, etc. Though if
> > this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
> > again, it should be UIO_VERSION, not DRIVER_VERSION).
> >
> > Does this really need a three-part version scheme? What's wrong with a
> > version of "1", to be changed to "2" in the hopefully-unlikely event that
> > the
> > userspace API changes? Assuming UIO is used for this at all, which
> > doesn't
> > seem like a great fit to me.
>
> No driver version numbers at all please, they do not make any sense when
> the driver is included in the kernel tree.
Again, reporting a version string is part of the UIO API. It might not be a
good API, but if it's left as NULL the registration will fail.
-Scott
On Thu, 2020-04-16 at 08:30 +0200, Greg KH wrote:
> On Wed, Apr 15, 2020 at 02:26:55PM -0500, Scott Wood wrote:
> > Instead, have module parameters that take the sizes and alignments you'd
> > like
> > to allocate and expose to userspace. Better still would be some sort of
> > dynamic allocation (e.g. open a fd, ioctl to set the requested
> > size/alignment,
> > if it succeeds you can mmap it, and when the fd is closed the region is
> > freed).
>
> No module parameters please, this is not the 1990's.
>
> Use device tree, that is what it is there for.
Since when is the device tree for indicating desired allocations? This is not
hardware description.
If module parameters are unacceptable, then I'd suggest dynamic allocation as
described above.
-Scott