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

From: Wenhu Wang <wenhu.wang@hotmail.com>
To: Greg KH <gregkh@linuxfoundation.org>
Cc: "christophe.leroy@csgroup.eu" <christophe.leroy@csgroup.eu>,
	"mpe@ellerman.id.au" <mpe@ellerman.id.au>,
	"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: 回复: [PATCH 2/2] uio:powerpc:mpc85xx: l2-cache-sram uio driver implementation
Date: Tue, 14 Jun 2022 06:09:35 +0000	[thread overview]
Message-ID: <SG2PR01MB2951EA9ED70E5F766DD26A069FAA9@SG2PR01MB2951.apcprd01.prod.exchangelabs.com> (raw)
In-Reply-To: <YqHy1uXwCLlJmftr@kroah.com>

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 <gregkh@linuxfoundation.org>
>发送时间: 2022年6月9日 21:17
>收件人: Wang Wenhu <wenhu.wang@hotmail.com>
>抄送: christophe.leroy@csgroup.eu <christophe.leroy@csgroup.eu>; mpe@ellerman.id.au <mpe@ellerman.id.au>; linuxppc-dev@lists.ozlabs.org <linuxppc-dev@lists.ozlabs.org>; linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>
>主题: 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:
>> 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 <christophe.leroy@csgroup.eu>
>> Signed-off-by: Wang Wenhu <wenhu.wang@hotmail.com>
>> ---
>>  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

>> +
>>  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

>> +     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

>> +     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

>> +     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

>> +     .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.

>> +}
>> +
>> +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

>> +             return -EINVAL;
>> +     }
>
>thanks,
>
>greg k-h
>

Thanks,
Wenhu