From: Arnd Bergmann <arnd@arndb.de> To: "tan.shaopeng" <tan.shaopeng@jp.fujitsu.com> Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>, SoC Team <soc@kernel.org>, Will Deacon <will@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Olof Johansson <olof@lixom.net>, Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Subject: Re: [PATCH RFC] soc: fujitsu: Add cache driver code Date: Wed, 3 Mar 2021 12:24:14 +0100 [thread overview] Message-ID: <CAK8P3a0VUoixMKg5UmF8agAZUXoMT8_SbCiWBdStbZ+_yAYNjA@mail.gmail.com> (raw) In-Reply-To: <1614764303-34903-2-git-send-email-tan.shaopeng@jp.fujitsu.com> On Wed, Mar 3, 2021 at 10:38 AM tan.shaopeng <tan.shaopeng@jp.fujitsu.com> wrote: > + > +config FUJITSU_CACHE > + tristate "FUJITSU Cache Driver" > + depends on ARM64_VHE || COMPILE_TEST > + help > + FUJITSU Cache Driver > + > + This driver offers cache functions for A64FX system. > + Loading this cache driver, control registers will be set to enable > + these functions, and advanced settings registers will be set by default > + values. After loading this driver, you can use the default values of the > + advanced settings registers or set the advanced settings registers > + from EL0. Unloading this driver, control registers will be clear to > + disable these functions. > + When built as a module, this will be called as "fujitsu_cache". My feeling is that this code should be in arch/arm64/, as the cache is generally considered part of the CPU, rather than part of the wider SoC design, or something that can be controlled separately from the core kernel and memory management code. > +module_param(tagaddr_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(tagaddr_ctrl_reg, "HPC tag address override control register"); > +module_param(hwpf_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(hwpf_ctrl_reg, "hardware prefetch assist control register"); > +module_param(sec_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_ctrl_reg, "sector cache control register"); > +module_param(sec_assign_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_assign_reg, "sector cache assign register"); > +module_param(sec_set0_l2_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_set0_l2_reg, "sector cache L2 way register(sector=0,1)"); > +module_param(sec_set1_l2_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_set1_l2_reg, "sector cache L2 way register(sector=2,3)"); My feeling is that the actual settings need to be on a higher level, not tied to the specific register-level implementation of this chip. Normally, the L2 cache is set up by the firmware according to local policy, and the settings can either be read by the kernel from registers or passed down through the device tree. It sounds like you want to control the policy at runtime in the operating system rather than at boot time, so for each setting you wish to override, there should be description of what the setting does and what the purpose of overriding the firmware setting is. Looking at the first one in the list, I see the specification mentions multiple distinct features that can be enabled or disabled, so these should probably get controlled individually. I also see that it is possible to control this for TTBR1 and TTBR0 separately, and we probably cannot allow user space (through module parameters or any other interface) to control TTBR1, which is where the kernel resides. The TTBR0 settings in turn would seem to interact with CONFIG_ARM64_MTE, and should not be controlled independently but through the same interfaces as that if we find that it does need to be controlled at all. I have not looked at any further details, but that should help get an idea of what I think would happen with the other registers. > +static int __init fujitsu_drv_init(void) > +{ > + int ret; > + > + if (read_cpuid_implementor() != ARM_CPU_IMP_FUJITSU) > + return -ENODEV; > + if (read_cpuid_part_number() != FUJITSU_CPU_PART_A64FX) > + return -ENODEV; The module_init function should not return an error when it is running on incompatible hardware, please just change this to silently return success to avoid warning about the failed initcall if the driver is built into a generic kernel. Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de> To: "tan.shaopeng" <tan.shaopeng@jp.fujitsu.com> Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>, SoC Team <soc@kernel.org>, Will Deacon <will@kernel.org>, Catalin Marinas <catalin.marinas@arm.com>, Olof Johansson <olof@lixom.net>, Misono Tomohiro <misono.tomohiro@jp.fujitsu.com> Subject: Re: [PATCH RFC] soc: fujitsu: Add cache driver code Date: Wed, 3 Mar 2021 12:24:14 +0100 [thread overview] Message-ID: <CAK8P3a0VUoixMKg5UmF8agAZUXoMT8_SbCiWBdStbZ+_yAYNjA@mail.gmail.com> (raw) Message-ID: <20210303112414.Uh932k3F28WKr7whuAufl0pXhmy1xZl3hMjeVQCxxjk@z> (raw) In-Reply-To: <1614764303-34903-2-git-send-email-tan.shaopeng@jp.fujitsu.com> On Wed, Mar 3, 2021 at 10:38 AM tan.shaopeng <tan.shaopeng@jp.fujitsu.com> wrote: > + > +config FUJITSU_CACHE > + tristate "FUJITSU Cache Driver" > + depends on ARM64_VHE || COMPILE_TEST > + help > + FUJITSU Cache Driver > + > + This driver offers cache functions for A64FX system. > + Loading this cache driver, control registers will be set to enable > + these functions, and advanced settings registers will be set by default > + values. After loading this driver, you can use the default values of the > + advanced settings registers or set the advanced settings registers > + from EL0. Unloading this driver, control registers will be clear to > + disable these functions. > + When built as a module, this will be called as "fujitsu_cache". My feeling is that this code should be in arch/arm64/, as the cache is generally considered part of the CPU, rather than part of the wider SoC design, or something that can be controlled separately from the core kernel and memory management code. > +module_param(tagaddr_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(tagaddr_ctrl_reg, "HPC tag address override control register"); > +module_param(hwpf_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(hwpf_ctrl_reg, "hardware prefetch assist control register"); > +module_param(sec_ctrl_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_ctrl_reg, "sector cache control register"); > +module_param(sec_assign_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_assign_reg, "sector cache assign register"); > +module_param(sec_set0_l2_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_set0_l2_reg, "sector cache L2 way register(sector=0,1)"); > +module_param(sec_set1_l2_reg, ulong, 0444); > +MODULE_PARM_DESC(sec_set1_l2_reg, "sector cache L2 way register(sector=2,3)"); My feeling is that the actual settings need to be on a higher level, not tied to the specific register-level implementation of this chip. Normally, the L2 cache is set up by the firmware according to local policy, and the settings can either be read by the kernel from registers or passed down through the device tree. It sounds like you want to control the policy at runtime in the operating system rather than at boot time, so for each setting you wish to override, there should be description of what the setting does and what the purpose of overriding the firmware setting is. Looking at the first one in the list, I see the specification mentions multiple distinct features that can be enabled or disabled, so these should probably get controlled individually. I also see that it is possible to control this for TTBR1 and TTBR0 separately, and we probably cannot allow user space (through module parameters or any other interface) to control TTBR1, which is where the kernel resides. The TTBR0 settings in turn would seem to interact with CONFIG_ARM64_MTE, and should not be controlled independently but through the same interfaces as that if we find that it does need to be controlled at all. I have not looked at any further details, but that should help get an idea of what I think would happen with the other registers. > +static int __init fujitsu_drv_init(void) > +{ > + int ret; > + > + if (read_cpuid_implementor() != ARM_CPU_IMP_FUJITSU) > + return -ENODEV; > + if (read_cpuid_part_number() != FUJITSU_CPU_PART_A64FX) > + return -ENODEV; The module_init function should not return an error when it is running on incompatible hardware, please just change this to silently return success to avoid warning about the failed initcall if the driver is built into a generic kernel. Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-03 11:29 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-03 9:38 [PATCH RFC] Upstream A64FX cache driver tan.shaopeng 2021-03-03 9:38 ` tan.shaopeng 2021-03-03 9:38 ` [PATCH RFC] soc: fujitsu: Add cache driver code tan.shaopeng 2021-03-03 9:38 ` tan.shaopeng 2021-03-03 11:24 ` Arnd Bergmann [this message] 2021-03-03 11:24 ` Arnd Bergmann 2021-03-04 10:34 ` tan.shaopeng 2021-03-04 10:34 ` tan.shaopeng 2021-03-04 10:46 ` Will Deacon 2021-03-04 10:46 ` Will Deacon 2021-03-04 10:58 ` Arnd Bergmann 2021-03-04 10:58 ` Arnd Bergmann 2021-03-05 7:48 ` tan.shaopeng 2021-03-05 7:48 ` tan.shaopeng 2021-03-31 8:52 ` tan.shaopeng 2021-03-31 8:52 ` tan.shaopeng 2021-04-01 16:15 ` James Morse 2021-04-01 16:15 ` James Morse 2021-04-02 8:44 ` tan.shaopeng 2021-04-02 8:44 ` tan.shaopeng 2021-03-04 10:54 ` Arnd Bergmann 2021-03-04 10:54 ` Arnd Bergmann 2021-03-05 8:10 ` tan.shaopeng 2021-03-05 8:10 ` tan.shaopeng
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CAK8P3a0VUoixMKg5UmF8agAZUXoMT8_SbCiWBdStbZ+_yAYNjA@mail.gmail.com \ --to=arnd@arndb.de \ --cc=catalin.marinas@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=misono.tomohiro@jp.fujitsu.com \ --cc=olof@lixom.net \ --cc=soc@kernel.org \ --cc=tan.shaopeng@jp.fujitsu.com \ --cc=will@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.