Message-ID: <753e5369-0d25-b149-8862-67a691f19ba0@csgroup.eu>
Date:   Wed, 15 Jun 2022 06:48:50 +0000
From:   Christophe Leroy <christophe.leroy@...roup.eu>
To:     Wang Wenhu <wenhu.wang@...mail.com>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "arnd@...db.de" <arnd@...db.de>,
        "hao.wu@...el.com" <hao.wu@...el.com>,
        "trix@...hat.com" <trix@...hat.com>,
        "mdf@...nel.org" <mdf@...nel.org>,
        "yilun.xu@...el.com" <yilun.xu@...el.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "mpe@...erman.id.au" <mpe@...erman.id.au>
Subject: Re: [PATCHv2 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver
 implementation



Le 15/06/2022 à 07:57, Wang Wenhu a écrit :
> Freescale mpc85xx l2-cache could be optionally configured as SRAM partly
> or fully. Users can make use of it as a block of independent memory that
> offers special usage, such as for debuging or other critical status info
> storage, which keeps consistently even when the whole system crashed.
> Applications can make use of UIO driver to access the SRAM from user level.
> 
> Once there was another driver version for the l2-cache-sram for SRAM access
> in kernel space. It had been removed recently.
> See: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dc21ed2aef4150fc2fcf58227a4ff24502015c03
> 
> Signed-off-by: Wang Wenhu <wenhu.wang@...mail.com>
> ---
> v2:
>   - Use __be32 instead of u32 for big-endian data declarations;
>   - Use generic ioremap_cache instead of ioremap_coherent;
>   - Physical address support both 32 and 64 bits;
>   - Addressed some other comments from Greg.
> ---
>   drivers/uio/Kconfig                   |  14 ++
>   drivers/uio/Makefile                  |   1 +
>   drivers/uio/uio_fsl_85xx_cache_sram.c | 288 ++++++++++++++++++++++++++
>   3 files changed, 303 insertions(+)
>   create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c
> 
> diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
> index 2e16c5338e5b..f7604584a12c 100644
> --- a/drivers/uio/Kconfig
> +++ b/drivers/uio/Kconfig
> @@ -105,6 +105,20 @@ 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 L2-Cache-SRAM UIO driver"
> +	depends on FSL_SOC_BOOKE && PPC32
> +	help
> +	  Driver for user level access of freescale mpc85xx l2-cache-sram.
> +
> +	  Freescale's mpc85xx provides an option of configuring a part of
> +	  (or full) cache memory as SRAM. The driver does this configuring
> +	  work and exports SRAM to user-space for access form user level.
> +	  This is extremely helpful for user applications that require
> +	  high performance memory accesses.
> +
> +	  If you don't know what to do here, say N.
> +
>   config UIO_FSL_ELBC_GPCM
>   	tristate "eLBC/GPCM driver"
>   	depends on FSL_LBC
> diff --git a/drivers/uio/Makefile b/drivers/uio/Makefile
> index f2f416a14228..1ba07d92a1b1 100644
> --- a/drivers/uio/Makefile
> +++ b/drivers/uio/Makefile
> @@ -12,3 +12,4 @@ obj-$(CONFIG_UIO_MF624)         += uio_mf624.o
>   obj-$(CONFIG_UIO_FSL_ELBC_GPCM)	+= uio_fsl_elbc_gpcm.o
>   obj-$(CONFIG_UIO_HV_GENERIC)	+= uio_hv_generic.o
>   obj-$(CONFIG_UIO_DFL)	+= uio_dfl.o
> +obj-$(CONFIG_UIO_FSL_85XX_CACHE_SRAM)	+= uio_fsl_85xx_cache_sram.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..6f91b0aa946b
> --- /dev/null
> +++ b/drivers/uio/uio_fsl_85xx_cache_sram.c
> @@ -0,0 +1,288 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Wang Wenhu <wenhu.wang@...mail.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 <linux/of_address.h>
> +#include <linux/io.h>
> +
> +#define DRIVER_NAME	"uio_mpc85xx_cache_sram"
> +#define UIO_INFO_VER	"0.0.1"
> +#define UIO_NAME	"uio_cache_sram"
> +
> +#define L2CR_L2FI		0x40000000	/* L2 flash invalidate */
> +#define L2CR_L2IO		0x00200000	/* L2 instruction only */
> +#define L2CR_SRAM_ZERO		0x00000000	/* L2SRAM zero size */
> +#define L2CR_SRAM_FULL		0x00010000	/* L2SRAM full size */
> +#define L2CR_SRAM_HALF		0x00020000	/* L2SRAM half size */
> +#define L2CR_SRAM_TWO_HALFS	0x00030000	/* L2SRAM two half sizes */
> +#define L2CR_SRAM_QUART		0x00040000	/* L2SRAM one quarter size */
> +#define L2CR_SRAM_TWO_QUARTS	0x00050000	/* L2SRAM two quarter size */
> +#define L2CR_SRAM_EIGHTH	0x00060000	/* L2SRAM one eighth size */
> +#define L2CR_SRAM_TWO_EIGHTH	0x00070000	/* L2SRAM two eighth size */
> +
> +#define L2SRAM_OPTIMAL_SZ_SHIFT	0x00000003	/* Optimum size for L2SRAM */
> +
> +#define L2SRAM_BAR_MSK_LO18	0xFFFFC000	/* Lower 18 bits */
> +#define L2SRAM_BARE_MSK_HI4	0x0000000F	/* Upper 4 bits */
> +
> +enum cache_sram_lock_ways {
> +	LOCK_WAYS_ZERO		= 0,
> +	LOCK_WAYS_EIGHTH	= 1,
> +	LOCK_WAYS_TWO_EIGHTH	= 2,
> +	LOCK_WAYS_HALF		= 4,
> +	LOCK_WAYS_FULL		= 8,
> +};
> +
> +struct mpc85xx_l2ctlr {
> +	__be32	ctl;		/* 0x000 - L2 control */
> +	u8	res1[0xC];
> +	__be32	ewar0;		/* 0x010 - External write address 0 */
> +	__be32	ewarea0;	/* 0x014 - External write address extended 0 */
> +	__be32	ewcr0;		/* 0x018 - External write ctrl */
> +	u8	res2[4];
> +	__be32	ewar1;		/* 0x020 - External write address 1 */
> +	__be32	ewarea1;	/* 0x024 - External write address extended 1 */
> +	__be32	ewcr1;		/* 0x028 - External write ctrl 1 */
> +	u8	res3[4];
> +	__be32	ewar2;		/* 0x030 - External write address 2 */
> +	__be32	ewarea2;	/* 0x034 - External write address extended 2 */
> +	__be32	ewcr2;		/* 0x038 - External write ctrl 2 */
> +	u8	res4[4];
> +	__be32	ewar3;		/* 0x040 - External write address 3 */
> +	__be32	ewarea3;	/* 0x044 - External write address extended 3 */
> +	__be32	ewcr3;		/* 0x048 - External write ctrl 3 */
> +	u8	res5[0xB4];
> +	__be32	srbar0;		/* 0x100 - SRAM base address 0 */
> +	__be32	srbarea0;	/* 0x104 - SRAM base addr reg ext address 0 */
> +	__be32	srbar1;		/* 0x108 - SRAM base address 1 */
> +	__be32	srbarea1;	/* 0x10C - SRAM base addr reg ext address 1 */
> +	u8	res6[0xCF0];
> +	__be32	errinjhi;	/* 0xE00 - Error injection mask high */
> +	__be32	errinjlo;	/* 0xE04 - Error injection mask low */
> +	__be32	errinjctl;	/* 0xE08 - Error injection tag/ecc control */
> +	u8	res7[0x14];
> +	__be32	captdatahi;	/* 0xE20 - Error data high capture */
> +	__be32	captdatalo;	/* 0xE24 - Error data low capture */
> +	__be32	captecc;	/* 0xE28 - Error syndrome */
> +	u8	res8[0x14];
> +	__be32	errdet;		/* 0xE40 - Error detect */
> +	__be32	errdis;		/* 0xE44 - Error disable */
> +	__be32	errinten;	/* 0xE48 - Error interrupt enable */
> +	__be32	errattr;	/* 0xE4c - Error attribute capture */
> +	__be32	erradrrl;	/* 0xE50 - Error address capture low */
> +	__be32	erradrrh;	/* 0xE54 - Error address capture high */
> +	__be32	errctl;		/* 0xE58 - Error control */
> +	u8	res9[0x1A4];
> +};

Should those macros and struct go in a .h file ?


> +
> +static int uio_cache_sram_setup(struct platform_device *pdev,
> +				phys_addr_t base, u8 ways)
> +{
> +	struct mpc85xx_l2ctlr __iomem *l2ctlr = of_iomap(pdev->dev.of_node, 0);

What happens to that mapping once you leave the function ?

By the way, new drivers use platform_get_resource(ofdev, IORESOURCE_MEM, 0);

> +
> +	if (!l2ctlr) {
> +		dev_err(&pdev->dev, "can not map l2 controller\n");
> +		return -EINVAL;
> +	}
> +
> +	/* write bits[0-17] to srbar0 */
> +	out_be32(&l2ctlr->srbar0, lower_32_bits(base) & L2SRAM_BAR_MSK_LO18);

out_be32() and in_be32() are specific to powerpc, they should be avoided 
outside of arch/powerpc/

You have iowrite32be() and ioread32be() which are equivalent.

> +
> +	/* write bits[18-21] to srbare0 */
> +	if (IS_ENABLED(CONFIG_PHYS_64BIT))
> +		out_be32(&l2ctlr->srbarea0, upper_32_bits(base) & L2SRAM_BARE_MSK_HI4);
> +
> +	clrsetbits_be32(&l2ctlr->ctl, L2CR_L2E, L2CR_L2FI);
> +
> +	switch (ways) {
> +	case LOCK_WAYS_EIGHTH:
> +		setbits32(&l2ctlr->ctl, L2CR_L2E | L2CR_L2FI | L2CR_SRAM_EIGHTH);
> +		break;
> +
> +	case LOCK_WAYS_TWO_EIGHTH:
> +		setbits32(&l2ctlr->ctl, L2CR_L2E | L2CR_L2FI | L2CR_SRAM_QUART);
> +		break;
> +
> +	case LOCK_WAYS_HALF:
> +		setbits32(&l2ctlr->ctl, L2CR_L2E | L2CR_L2FI | L2CR_SRAM_HALF);
> +		break;
> +
> +	case LOCK_WAYS_FULL:
> +	default:
> +		setbits32(&l2ctlr->ctl, L2CR_L2E | L2CR_L2FI | L2CR_SRAM_FULL);
> +		break;
> +	}
> +	eieio();

Why do you need eieio() after a setbits32() which already do a full sync ?

> +
> +	return 0;
> +}
> +
> +static const struct vm_operations_struct uio_cache_sram_vm_ops = {
> +	.access = generic_access_phys,
> +};
> +
> +static int uio_cache_sram_mmap(struct uio_info *info,
> +			       struct vm_area_struct *vma)
> +{
> +	struct uio_mem *mem = info->mem;
> +
> +	if (mem->addr & ~PAGE_MASK)
> +		return -ENODEV;
> +
> +	if ((vma->vm_end - vma->vm_start > mem->size) ||
> +	    (mem->size == 0) ||
> +	    (mem->memtype != UIO_MEM_PHYS))
> +		return -EINVAL;
> +
> +	vma->vm_ops = &uio_cache_sram_vm_ops;
> +	vma->vm_page_prot = pgprot_cached(vma->vm_page_prot);
> +
> +	return remap_pfn_range(vma,
> +			       vma->vm_start,
> +			       mem->addr >> PAGE_SHIFT,
> +			       vma->vm_end - vma->vm_start,
> +			       vma->vm_page_prot);
> +}
> +
> +static int uio_cache_sram_probe(struct platform_device *pdev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct uio_info *info;
> +	struct uio_mem *uiomem;
> +	const char *dt_name;
> +	u32 mem_base_32;
> +	u64 mem_base_64;

mem_base_32 and mem_base_64 needed here, define them inside the block 
where they are used, see below

> +	phys_addr_t mem_base;
> +	u32 l2cache_size;
> +	u32 mem_size;
> +	u32 rem;
> +	u8 ways;
> +	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(node, "uio_name", &dt_name))
> +		dt_name = UIO_NAME;
> +
> +	info->name = devm_kstrdup(&pdev->dev, dt_name, GFP_KERNEL);
> +	if (!info->name)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_u32(node, "cache-mem-size", &mem_size);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing cache-mem-size\n");
> +		return -EINVAL;
> +	}
> +
> +
> +	if (IS_ENABLED(CONFIG_PHYS_64BIT)) {
		u64 mem_base_64;

> +		ret = of_property_read_u64(node, "cache-mem-base", &mem_base_64);
> +		mem_base = mem_base_64;
> +	} else {

		u32 mem_base_32;

> +		ret = of_property_read_u32(node, "cache-mem-base", &mem_base_32);
> +		mem_base = mem_base_32;
> +	}
> +
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing cache-mem-base\n");
> +		return -EINVAL;
> +	}
> +
> +	if (mem_size == 0) {
> +		dev_err(&pdev->dev, "cache-mem-size should not be 0\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = of_property_read_u32(node, "cache-size", &l2cache_size);
> +	if (ret) {
> +		dev_err(&pdev->dev, "missing l2cache-size\n");
> +		return -EINVAL;
> +	}
> +
> +	rem = l2cache_size % mem_size;
> +	ways = LOCK_WAYS_FULL * mem_size / l2cache_size;
> +	if (rem || (ways & (ways - 1))) {

Use is_power_of_2() instead ?

> +		dev_err(&pdev->dev, "illegal cache-sram-size parameter\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = uio_cache_sram_setup(pdev, mem_base, ways);
> +	if (ret)
> +		return ret;
> +
> +	if (!request_mem_region(mem_base, mem_size, "fsl_85xx_cache_sram")) {
> +		dev_err(&pdev->dev, "uio_cache_sram request memory failed\n");
> +		ret = -ENXIO;

ret is never used.

> +	}
> +
> +	info->irq = UIO_IRQ_NONE;
> +	info->version = UIO_INFO_VER;
> +	info->mmap = uio_cache_sram_mmap;
> +	uiomem = info->mem;
> +	uiomem->memtype = UIO_MEM_PHYS;
> +	uiomem->addr = mem_base;
> +	uiomem->size = mem_size;
> +	uiomem->name = devm_kstrdup(&pdev->dev, node->name, GFP_KERNEL);
> +	uiomem->internal_addr = ioremap_cache(mem_base, mem_size);
> +	if (!uiomem->internal_addr) {
> +		dev_err(&pdev->dev, "cache ioremap failed\n");
> +		ret = -ENOMEM;

ret is never used.

> +	}
> +
> +	/* 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:
> +	iounmap(info->mem[0].internal_addr);

What happends to the requested mem region ?

> +	return ret;
> +}
> +
> +static int uio_cache_sram_remove(struct platform_device *pdev)
> +{
> +	struct uio_info *info = platform_get_drvdata(pdev);
> +
> +	uio_unregister_device(info);
> +	iounmap(info->mem[0].internal_addr);

No release_mem_region() ?

> +
> +	return 0;
> +}
> +
> +static const struct of_device_id uio_cache_sram_of_match[] = {
> +	{ .compatible = "fsl,p2020-l2-cache-sram-uio", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, uio_cache_sram_of_match);
> +
> +static struct platform_driver uio_fsl_85xx_cache_sram = {
> +	.probe = uio_cache_sram_probe,
> +	.remove = uio_cache_sram_remove,
> +	.driver = {
> +		.name = DRIVER_NAME,
> +		.of_match_table	= uio_cache_sram_of_match,
> +	},
> +};
> +
> +module_platform_driver(uio_fsl_85xx_cache_sram);
> +
> +MODULE_AUTHOR("Wang Wenhu <wenhu.wang@...mail.com>");
> +MODULE_DESCRIPTION("Freescale MPC85xx Cache-SRAM UIO Platform Driver");
> +MODULE_ALIAS("platform:" DRIVER_NAME);
> +MODULE_LICENSE("GPL");

Powered by blists - more mailing lists