回复: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation

Hi Greg, thanks for the comments.
The questions are replied specifically below.
I have figured out and tested the patch v2, which is to be posted later.

发件人: Greg KH [off-list ref]
发送时间: 2022年6月9日 21:17
收件人: Wang Wenhu [off-list ref]
抄送: christophe.leroy@csgroup.eu [off-list ref]; mpe@ellerman.id.au [off-list ref]; linuxppc-dev@lists.ozlabs.org [off-list ref]; linux-kernel@vger.kernel.org [off-list ref]
主题: Re: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation 

On Thu, Jun 09, 2022 at 03:28:55AM -0700, Wang Wenhu wrote:

quoted

The 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 cratical status info
storage which keeps consistently even when the whole system crashed.

The hardware related configuration process utilized the work of the
earlier implementation, which has been removed now.
See: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git/commit/?id=dc21ed2aef4150fc2fcf58227a4ff24502015c03

Cc: Christophe Leroy <redacted>
Signed-off-by: Wang Wenhu <redacted>
---
 drivers/uio/Kconfig                   |  10 +
 drivers/uio/Makefile                  |   1 +
 drivers/uio/uio_fsl_85xx_cache_sram.c | 286 ++++++++++++++++++++++++++
 3 files changed, 297 insertions(+)
 create mode 100644 drivers/uio/uio_fsl_85xx_cache_sram.c

diff --git a/drivers/uio/Kconfig b/drivers/uio/Kconfig
index 2e16c5338e5b..9199ced03880 100644
--- a/drivers/uio/Kconfig
+++ b/drivers/uio/Kconfig

@@ -105,6 +105,16 @@ 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
+     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.
+
+       If you don't know what to do here, say N.

Module name information?

 
More detailed and clearer info in v2

quoted

+
 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..d363f9d2b179
--- /dev/null
+++ b/drivers/uio/uio_fsl_85xx_cache_sram.c

@@ -0,0 +1,286 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Wang Wenhu <wenhu.wang@hotmail.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,
+     LOCK_WAYS_EIGHTH,
+     LOCK_WAYS_TWO_EIGHTH,

Why not have values for these?

full values given in v2

quoted

+     LOCK_WAYS_HALF = 4,
+     LOCK_WAYS_FULL = 8,
+};
+
+struct mpc85xx_l2ctlr {
+     u32     ctl;            /* 0x000 - L2 control */

What is the endian of these u32 values?  You map them directly to
memory, so they must be specified some way, right?  Please make it
obvious what they are.

Surely, the values should be u32 here, modified in v2
The controller info could be found in
"QorIQ™ P2020 Integrated Processor Reference Manual"
"Chapter 6 L2 Look-Aside Cache/SRAM"
See: http://m4udit.dinauz.org/P2020RM_rev0.pdf

quoted

+     u8      res1[0xC];
+     u32     ewar0;          /* 0x010 - External write address 0 */
+     u32     ewarea0;        /* 0x014 - External write address extended 0 */
+     u32     ewcr0;          /* 0x018 - External write ctrl */
+     u8      res2[4];
+     u32     ewar1;          /* 0x020 - External write address 1 */
+     u32     ewarea1;        /* 0x024 - External write address extended 1 */
+     u32     ewcr1;          /* 0x028 - External write ctrl 1 */
+     u8      res3[4];
+     u32     ewar2;          /* 0x030 - External write address 2 */
+     u32     ewarea2;        /* 0x034 - External write address extended 2 */
+     u32     ewcr2;          /* 0x038 - External write ctrl 2 */
+     u8      res4[4];
+     u32     ewar3;          /* 0x040 - External write address 3 */
+     u32     ewarea3;        /* 0x044 - External write address extended 3 */
+     u32     ewcr3;          /* 0x048 - External write ctrl 3 */
+     u8      res5[0xB4];
+     u32     srbar0;         /* 0x100 - SRAM base address 0 */
+     u32     srbarea0;       /* 0x104 - SRAM base addr reg ext address 0 */
+     u32     srbar1;         /* 0x108 - SRAM base address 1 */
+     u32     srbarea1;       /* 0x10C - SRAM base addr reg ext address 1 */
+     u8      res6[0xCF0];
+     u32     errinjhi;       /* 0xE00 - Error injection mask high */
+     u32     errinjlo;       /* 0xE04 - Error injection mask low */
+     u32     errinjctl;      /* 0xE08 - Error injection tag/ecc control */
+     u8      res7[0x14];
+     u32     captdatahi;     /* 0xE20 - Error data high capture */
+     u32     captdatalo;     /* 0xE24 - Error data low capture */
+     u32     captecc;        /* 0xE28 - Error syndrome */
+     u8      res8[0x14];
+     u32     errdet;         /* 0xE40 - Error detect */
+     u32     errdis;         /* 0xE44 - Error disable */
+     u32     errinten;       /* 0xE48 - Error interrupt enable */
+     u32     errattr;        /* 0xE4c - Error attribute capture */
+     u32     erradrrl;       /* 0xE50 - Error address capture low */
+     u32     erradrrh;       /* 0xE54 - Error address capture high */
+     u32     errctl;         /* 0xE58 - Error control */
+     u8      res9[0x1A4];
+};
+
+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);
+
+     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);
+
+     /* write bits[18-21] to srbare0 */
+#ifdef CONFIG_PHYS_64BIT

No #ifdef in .c files please.

#ifdef eliminated in v2

quoted

+     out_be32(&l2ctlr->srbarea0, upper_32_bits(base) & L2SRAM_BARE_MSK_HI4);
+#endif
+
+     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();
+
+     return 0;
+}
+
+static const struct vm_operations_struct uio_cache_sram_vm_ops = {
+#ifdef CONFIG_HAVE_IOREMAP_PROT

Same here.

I tried to eliminate it in mainline
See: [PATCH v2] mm: eliminate ifdef of HAVE_IOREMAP_PROT in .c files
https://lkml.org/lkml/2022/6/10/695

quoted

+     .access = generic_access_phys,
+#endif
+};
+
+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);

Odd indentation, did you use checkpatch.pl on your patch?

Actually, I checked with the scripts, and there was no warning here.
I also checked in text editors and vim, if I translate tab with 4 spaces,
the "vma/mem" areas in the 5 lines were aligned.

quoted

+}
+
+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;
+     phys_addr_t mem_base;
+     u32 l2cache_size;
+     u32 mem_size;
+     u32 rem;
+     u8 ways;
+     int ret;
+
+     if (!node) {
+             dev_err(&pdev->dev, "device's of_node is null\n");

How can that happen?

Redundant and will be removed in v2

quoted

+             return -EINVAL;
+     }

thanks,

greg k-h

Thanks,
Wenhu