| Message ID | SG2PR01MB295139AA7360917B2C4846E19FA79@SG2PR01MB2951.apcprd01.prod.exchangelabs.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | uio:ppc: cache-sram driver implementation | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 10 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
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
On Tue, Jun 14, 2022 at 06:09:35AM +0000, Wenhu Wang wrote: > 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 That's not the answer to my question :) These are big-endian, right? Please mark them as such and access them properly with the correct functions. > >> + 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. Tabs in Linux are always 8 spaces wide. thanks, greg k-h
Le 14/06/2022 à 08:34, Greg KH a écrit : > On Tue, Jun 14, 2022 at 06:09:35AM +0000, Wenhu Wang wrote: >>> >>> 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. > > Tabs in Linux are always 8 spaces wide. > See https://docs.kernel.org/process/coding-style.html?highlight=coding+style#indentation Tabs are 8 characters, and thus indentations are also 8 characters. There are heretic movements that try to make indentations 4 (or even 2!) characters deep, and that is akin to trying to define the value of PI to be 3. Christophe
Le 14/06/2022 à 08:09, Wenhu Wang a écrit : >>> +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 > I looked at that patch. I don't think you can just drop the #ifdef in function __access_remote_vm() in mm/memory.c You have to replace it with something like: if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) break; Christophe
Le 14/06/2022 à 09:18, Christophe Leroy a écrit : > > > Le 14/06/2022 à 08:09, Wenhu Wang a écrit : >>>> +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 >> > > I looked at that patch. > > I don't think you can just drop the #ifdef in function > __access_remote_vm() in mm/memory.c > > You have to replace it with something like: > > if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) > break; > Another thing in that patch: By making generic_access_phys() a static inline, it means that everytime you refer to the address of that function in a vm_operations_struct struct, the compiler has to provide an outlined instance of the function. It means you'll likely have several instances of a generic_access_phys(). What you could do instead is to add the following at the start of generic_access_phys() in mm/memory.c : if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) return 0;
>> >> + >> >> +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 > >That's not the answer to my question :) > >These are big-endian, right? Please mark them as such and access them >properly with the correct functions. Yes, they are big-edian. Does it work to add comments(about order and access functions) for the structure ahead of it？ And appending like "_be", "_access_be" or "_big_endian"? (struct mpc85xx_l2ctlr_be {……}; >Tabs in Linux are always 8 spaces wide. > I will re-confirm the v2 of identation. >> >> I looked at that patch. >> >> I don't think you can just drop the #ifdef in function >> __access_remote_vm() in mm/memory.c >> >> You have to replace it with something like: >> >> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >> break; >> > > >Another thing in that patch: > >By making generic_access_phys() a static inline, it means that everytime >you refer to the address of that function in a vm_operations_struct >struct, the compiler has to provide an outlined instance of the >function. It means you'll likely have several instances of a >generic_access_phys(). > >What you could do instead is to add the following at the start of >generic_access_phys() in mm/memory.c : > > if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) > return 0; > It is really a better chmoce, thanks for the advice. Multiple instances exist as you mentioned, the block returns 0 with no-op instance which makes no difference with the function return value. I will update the patch after a re-confirming. Thanks, Wenhu
>On Tue, Jun 14, 2022 at 07:53:46AM +0000, Wenhu Wang wrote: >> >> >> + >> >> >> +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 >> > >> >That's not the answer to my question :) >> > >> >These are big-endian, right? Please mark them as such and access them >> >properly with the correct functions. >> >> Yes, they are big-edian. >> Does it work to add comments(about order and access functions) for the structure ahead of it? >> And appending like "_be", "_access_be" or "_big_endian"? (struct mpc85xx_l2ctlr_be {...}; > >No, not comments, these should be of the type __be32, right? > Yes, understand. It's clear straight forward. I will update those in patch v2. Thanks, Wenhu
>>> >>> I looked at that patch. >>> >>> I don't think you can just drop the #ifdef in function >>> __access_remote_vm() in mm/memory.c >>> >>> You have to replace it with something like: >>> >>> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >>> break; >>> >> >> >>Another thing in that patch: >> >>By making generic_access_phys() a static inline, it means that everytime >>you refer to the address of that function in a vm_operations_struct >>struct, the compiler has to provide an outlined instance of the >>function. It means you'll likely have several instances of a >>generic_access_phys(). >> >>What you could do instead is to add the following at the start of >>generic_access_phys() in mm/memory.c : >> >> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >> return 0; >> > >It is really a better chmoce, thanks for the advice. >Multiple instances exist as you mentioned, the block returns 0 with no-op >instance which makes no difference with the function return value. > >I will update the patch after a re-confirming. > I tried as adviced but when not defined, error happens on archectures such as arm64. Actually the function generic_access_phys calls a lot of functions that become undefined if we compile it with CONFIG_HAVE_IOREMAP_PROT disabled. The archectures that support CONFIG_HAVE_IOREMAP_PROT are mips, x86, sh, arc, s390, loongarch and powerpc. So we may just define the function with static inline and add IS_ENABLED condition branch in function __access_remote_vm in mm/memory.c. The executing path breaks if CONFIG_HAVE_IOREMAP_PROT is disabled, and never goes into the static no-op function. In short, the static inline no-op function would never be executed, the only difference is that there would be a lot of function code in compiled target. Thanks, Wenhu
UIO seems like the wrong kind of interface for this. Why isn't this a simple character device?
Le 14/06/2022 à 16:40, Wenhu Wang a écrit : >>>> >>>> I looked at that patch. >>>> >>>> I don't think you can just drop the #ifdef in function >>>> __access_remote_vm() in mm/memory.c >>>> >>>> You have to replace it with something like: >>>> >>>> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >>>> break; >>>> >>> >>> >>> Another thing in that patch: >>> >>> By making generic_access_phys() a static inline, it means that everytime >>> you refer to the address of that function in a vm_operations_struct >>> struct, the compiler has to provide an outlined instance of the >>> function. It means you'll likely have several instances of a >>> generic_access_phys(). >>> >>> What you could do instead is to add the following at the start of >>> generic_access_phys() in mm/memory.c : >>> >>> if (!IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT)) >>> return 0; >>> >> >> It is really a better chmoce, thanks for the advice. >> Multiple instances exist as you mentioned, the block returns 0 with no-op >> instance which makes no difference with the function return value. >> >> I will update the patch after a re-confirming. >> > > I tried as adviced but when not defined, error happens on archectures such > as arm64. Actually the function generic_access_phys calls a lot of functions > that become undefined if we compile it with CONFIG_HAVE_IOREMAP_PROT disabled. > The archectures that support CONFIG_HAVE_IOREMAP_PROT are mips, x86, sh, arc, > s390, loongarch and powerpc. > > So we may just define the function with static inline and add IS_ENABLED > condition branch in function __access_remote_vm in mm/memory.c. The executing > path breaks if CONFIG_HAVE_IOREMAP_PROT is disabled, and never goes into the > static no-op function. > > In short, the static inline no-op function would never be executed, the only > difference is that there would be a lot of function code in compiled target. > In that case all you have to do is: diff --git a/mm/memory.c b/mm/memory.c index 7a089145cad4..39b369fc77f6 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -5413,6 +5413,13 @@ int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, return ret; } EXPORT_SYMBOL_GPL(generic_access_phys); +#else +int generic_access_phys(struct vm_area_struct *vma, unsigned long addr, + void *buf, int len, int write) +{ + return 0; +} +EXPORT_SYMBOL_GPL(generic_access_phys); #endif /* Christophe
Le 14/06/2022 à 08:09, Wenhu Wang a écrit : >>> + >>> +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 >>> +}; Another solution is to do: static const struct vm_operations_struct uio_cache_sram_vm_ops = { .access = IS_ENABLED(CONFIG_HAVE_IOREMAP_PROT) ? generic_access_phys : NULL, }; Christophe
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. + 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, + LOCK_WAYS_HALF = 4, + LOCK_WAYS_FULL = 8, +}; + +struct mpc85xx_l2ctlr { + u32 ctl; /* 0x000 - L2 control */ + 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 + 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 + .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); +} + +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"); + return -EINVAL; + } + + /* 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; + } + + ret = of_property_read_u32(node, "cache-mem-base", &mem_base); + 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))) { + 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; + } + + 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_coherent(mem_base, mem_size); + if (!uiomem->internal_addr) { + dev_err(&pdev->dev, "cache ioremep_coherent failed\n"); + ret = -ENOMEM; + } + + /* 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); + 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); + + 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@hotmail.com>"); +MODULE_DESCRIPTION("Freescale MPC85xx Cache-Sram UIO Platform Driver"); +MODULE_ALIAS("platform:" DRIVER_NAME); +MODULE_LICENSE("GPL");
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