Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
On Thu, Mar 14, 2024 at 03:16:53PM +, Abdellatif El Khlifi wrote: > Hi Sudeep, > > On Thu, Mar 14, 2024 at 02:59:20PM +0000, Sudeep Holla wrote: > > > > I think Robin has raised few points that need clarification. I think it was > > done as part of DT binding patch. I share those concerns and I wanted to > > reaching to the same concerns by starting the questions I asked on corstone > > device tree changes. > > Please have a look at my answer to Robin with clarifications [1]. > Yes I did take a look at the response but am not convinced yet. I have responded to you email with other details which I want to be explored. I don't think we need to rush to add the *foundation driver* as you call before the bindings are defined well. -- Regards, Sudeep
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
On Thu, Mar 14, 2024 at 01:49:28PM +, Abdellatif El Khlifi wrote: > Hi Robin, > > > > + firmware-name: > > > +description: | > > > + Default name of the firmware to load to the remote processor. > > > > So... is loading the firmware image achieved by somehow bitbanging it > > through the one reset register, maybe? I find it hard to believe this is a > > complete and functional binding. > > > > Frankly at the moment I'd be inclined to say it isn't even a remoteproc > > binding (or driver) at all, it's a reset controller. Bindings are a contract > > for describing the hardware, not the current state of Linux driver support - > > if this thing still needs mailboxes, shared memory, a reset vector register, > > or whatever else to actually be useful, those should be in the binding from > > day 1 so that a) people can write and deploy correct DTs now, such that > > functionality becomes available on their systems as soon as driver support > > catches up, and b) the community has any hope of being able to review > > whether the binding is appropriately designed and specified for the purpose > > it intends to serve. > > This is an initial patchset for allowing to turn on and off the remote > processor. > The FW is already loaded before the Corstone-1000 SoC is powered on and this > is done through the FPGA board bootloader in case of the FPGA target. > Or by the Corstone-1000 FVP model (emulator). > If this driver does the loading of the firmware, it will get reloaded. Do you need any issues there ? The correctness matters here in the upstream driver, it may not be optimised for you usecase now, but that is fine IMO. > The plan for the driver is as follows: > > Step 1: provide a foundation driver capable of turning the core on/off > Step 2: provide mailbox support for comms > Step 3: provide FW reload capability > > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) can > share memory with the remote core. > Honestly, I would prefer to know the overall design before pushing any partial solution. If you know the final complete solution, present the same with the complete device tree binding for better understanding and review. > So, when memory sharing becomes available in the FPGA and FVP the > DT binding will be upgraded with: > > - mboxes property specifying the RX/TX mailboxes (based on MHU v2) > - memory-region property describing the virtio vrings > Looks like you have the information now, please define the complete bindings now. > Currently the mailbox controller does exist in the HW but is not > usable via virtio (no memory sharing available). > That is fine, if the plan is to use them, then include them in the design of your DT bindings. > Do you recommend I add the mboxes property even currently we can't do the > comms ? > Yes for sure IMO. > > For instance right now it seems somewhat tenuous to describe two consecutive > > 32-bit registers as separate "reg" entries, but *maybe* it's OK if that's > > all there ever is. However if it's actually going to end up needing several > > more additional MMIO and/or memory regions for other functionality, then > > describing each register and location individually is liable to get > > unmanageable really fast, and a higher-level functional grouping (e.g. these > > reset-related registers together as a single 8-byte region) would likely be > > a better design. > I completely agree with the above and this is what I was meant(I must admit didn't put it forward with above clarity). > Currently the HW provides only 2 registers to control the remote processors: > > The reset control and status registers. > Agreed, but it is part of a bigger block with other functionality in place. MFD/syscon might be better way to use these registers. You never know in future you might want to use another set of 2-4 registers with a different functionality in another driver. > It makes sense to me to use a mapped region of 8 bytes for both registers > rather > than individual registers (since they are consecutive). Not exactly. Are you sure, Linux will not have to use another other registers in that block ? Will you keep creating such (random if I may call it so) bindings for a smaller sets of register under "Host Base System Control registers". I would see if it makes sense to put together a single binding for this "Host Base System Control" register(not sure what exactly that means). Use MFD/regmap you access parts of this block. The remoteproc driver can then be semi-generic(meaning applicable to group of similar platforms) based on the platform compatible and use this regmap to provide the functionality needed. -- Regards, Sudeep
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
On Thu, Mar 14, 2024 at 08:52:59AM -0600, Mathieu Poirier wrote: > On Wed, Mar 13, 2024 at 05:17:56PM +, Abdellatif El Khlifi wrote: > > Hi Mathieu, > > > > On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote: > > > On Tue, Mar 12, 2024 at 05:32:52PM +, Abdellatif El Khlifi wrote: > > > > Hi Mathieu, > > > > > > > > On Tue, Mar 12, 2024 at 10:29:52AM -0600, Mathieu Poirier wrote: > > > > > > This is an initial patchset for allowing to turn on and off the > > > > > > remote processor. > > > > > > The FW is already loaded before the Corstone-1000 SoC is powered on > > > > > > and this > > > > > > is done through the FPGA board bootloader in case of the FPGA > > > > > > target. Or by the Corstone-1000 FVP model > > > > > > (emulator). > > > > > > > > > > > >From the above I take it that booting with a preloaded firmware is a > > > > > scenario that needs to be supported and not just a temporary stage. > > > > > > > > The current status of the Corstone-1000 SoC requires that there is > > > > a preloaded firmware for the external core. Preloading is done > > > > externally > > > > either through the FPGA bootloader or the emulator (FVP) before powering > > > > on the SoC. > > > > > > > > > > Ok > > > > > > > Corstone-1000 will be upgraded in a way that the A core running Linux > > > > is able > > > > to share memory with the remote core and also being able to access the > > > > remote > > > > core memory so Linux can copy the firmware to. This HW changes are still > > > > This is why this patchset is relying on a preloaded firmware. And it's > > > > the step 1 > > > > of adding remoteproc support for Corstone. > > > > > > > > > > Ok, so there is a HW problem where A core and M core can't see each > > > other's > > > memory, preventing the A core from copying the firmware image to the > > > proper > > > location. > > > > > > When the HW is fixed, will there be a need to support scenarios where the > > > firmware image has been preloaded into memory? > > > > No, this scenario won't apply when we get the HW upgrade. No need for an > > external entity anymore. The firmware(s) will all be files in the linux > > filesystem. > > > > Very well. I am willing to continue with this driver but it does so little > that > I wonder if it wouldn't simply be better to move forward with upstreaming when > the HW is fixed. The choice is yours. > I think Robin has raised few points that need clarification. I think it was done as part of DT binding patch. I share those concerns and I wanted to reaching to the same concerns by starting the questions I asked on corstone device tree changes. -- Regards, Sudeep
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
On Fri, Mar 01, 2024 at 08:30:43PM +0100, Krzysztof Kozlowski wrote: > On 01/03/2024 17:42, abdellatif.elkhl...@arm.com wrote: > > From: Abdellatif El Khlifi > > > > introduce the bindings for Arm remoteproc support. > > > > Signed-off-by: Abdellatif El Khlifi > > --- > > .../bindings/remoteproc/arm,rproc.yaml| 69 +++ > > MAINTAINERS | 1 + > > Fix order of patches - bindings are always before the user (see > submitting bindings doc). > > > 2 files changed, 70 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > > > > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > > b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > > new file mode 100644 > > index ..322197158059 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > > @@ -0,0 +1,69 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/remoteproc/arm,rproc.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Arm Remoteproc Devices > > That's quite generic... does it applied to all ARM designs? > Nope, it is platform specific. It can't just generically be referred as Arm Remoteproc for sure. -- Regards, Sudeep
Re: [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
On Fri, Mar 01, 2024 at 04:42:26PM +, abdellatif.elkhl...@arm.com wrote:
> From: Abdellatif El Khlifi
>
> add device tree node for the external system core in Corstone-1000
>
> Signed-off-by: Abdellatif El Khlifi
> ---
> arch/arm64/boot/dts/arm/corstone1000.dtsi | 10 +-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/arm/corstone1000.dtsi
> b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> index 6ad7829f9e28..67df642363e9 100644
> --- a/arch/arm64/boot/dts/arm/corstone1000.dtsi
> +++ b/arch/arm64/boot/dts/arm/corstone1000.dtsi
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0 OR MIT
> /*
> - * Copyright (c) 2022, Arm Limited. All rights reserved.
> + * Copyright 2022, 2024, Arm Limited and/or its affiliates
>
> * Copyright (c) 2022, Linaro Limited. All rights reserved.
> *
> */
> @@ -157,5 +157,13 @@ mhu_seh1: mailbox@1b83 {
> secure-status = "okay"; /* secure-world-only */
> status = "disabled";
> };
> +
> + extsys0: remoteproc@1a010310 {
> + compatible = "arm,corstone1000-extsys";
> + reg = <0x1a010310 0x4>,
> + <0x1a010314 0X4>;
As per [1], this is just a few registers within the 64kB block.
Not sure if it should be represented as a whole on just couple
of registers like this for reset.
--
Regards,
Sudeep
[1]
https://developer.arm.com/documentation/101418/0100/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Re: [PATCH] efifb: Fix runtime pm calls for non PCI efifb device
On Tue, Apr 20, 2021 at 04:12:26PM +0800, Kai-Heng Feng wrote:
> Hi Sudeep,
>
> On Tue, Apr 20, 2021 at 3:53 PM Sudeep Holla wrote:
> >
> > Gentle Ping! There is boot failure because of this issue with linux-next
> > on few arm platforms with non PCIe efifb. Please review and get the fix
> > merged ASAP so the testing on these platforms can continue with linux-next.
>
> It was merged in drm-tip as d510c88cfbb2 ("efifb: Check efifb_pci_dev
> before using it").
>
Ah OK, thanks! But I don't think it is appear on -next yet.
--
Regards,
Sudeep
Re: [PATCH] efifb: Fix runtime pm calls for non PCI efifb device
Gentle Ping! There is boot failure because of this issue with linux-next
on few arm platforms with non PCIe efifb. Please review and get the fix
merged ASAP so the testing on these platforms can continue with linux-next.
On Thu, Apr 15, 2021 at 11:22:24AM +0100, Sudeep Holla wrote:
> Commit a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI
> D0")
> added runtime pm calls to probe and remove routines to ensure the PCI
> device for efifb stays in D0 state. However not ever efifb is based on
> PCI device and efifb_pci_dev can be NULL if that is the case.
>
> In such cases, we will get a boot splat like below due to NULL dereference:
> -->8
> Console: switching to colour frame buffer device 240x67
> fb0: EFI VGA frame buffer device
> Unable to handle kernel NULL pointer dereference at virtual address
> 0270
> Mem abort info:
>ESR = 0x9604
>EC = 0x25: DABT (current EL), IL = 32 bits
>SET = 0, FnV = 0
>EA = 0, S1PTW = 0
> Data abort info:
>ISV = 0, ISS = 0x0004
>CM = 0, WnR = 0
> [0270] user address but active_mm is swapper
> Internal error: Oops: 9604 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc7-next-20210413 #1
> Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development
> Platform
> pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
> pc : pm_runtime_drop_link+0x12c/0x338
> lr : efifb_probe+0x7bc/0x7f0
> Call trace:
> pm_runtime_drop_link+0x12c/0x338
> efifb_probe+0x7bc/0x7f0
> platform_probe+0x68/0xd8
> really_probe+0xe4/0x3a8
> driver_probe_device+0x64/0xc8
> device_driver_attach+0x74/0x80
> __driver_attach+0x64/0xf0
> bus_for_each_dev+0x70/0xc0
> driver_attach+0x24/0x30
> bus_add_driver+0x150/0x1f8
> driver_register+0x64/0x120
> __platform_driver_register+0x28/0x38
> efifb_driver_init+0x1c/0x28
> do_one_initcall+0x48/0x2b0
> kernel_init_freeable+0x1e8/0x258
> kernel_init+0x14/0x118
> ret_from_fork+0x10/0x30
> Code: 88027c01 35a2 17fff706 f9800051 (885f7c40)
> ---[ end trace 17d8da630bf8ff77 ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
> -->8
>
> Fix the issue by checking for non-NULL efifb_pci_dev before dereferencing
> for runtime pm calls in probe and remove routines.
>
> Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI
> D0")
> Cc: Kai-Heng Feng
> Cc: Alex Deucher
> Cc: Thomas Zimmermann
> Cc: Peter Jones
> Signed-off-by: Sudeep Holla
> ---
> drivers/video/fbdev/efifb.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
> index f58a545b3bf3..8ea8f079cde2 100644
> --- a/drivers/video/fbdev/efifb.c
> +++ b/drivers/video/fbdev/efifb.c
> @@ -575,7 +575,8 @@ static int efifb_probe(struct platform_device *dev)
> goto err_fb_dealoc;
> }
> fb_info(info, "%s frame buffer device\n", info->fix.id);
> - pm_runtime_get_sync(_pci_dev->dev);
> + if (efifb_pci_dev)
> + pm_runtime_get_sync(_pci_dev->dev);
> return 0;
>
> err_fb_dealoc:
> @@ -602,7 +603,8 @@ static int efifb_remove(struct platform_device *pdev)
> unregister_framebuffer(info);
> sysfs_remove_groups(>dev.kobj, efifb_groups);
> framebuffer_release(info);
> - pm_runtime_put(_pci_dev->dev);
> + if (efifb_pci_dev)
> + pm_runtime_put(_pci_dev->dev);
>
> return 0;
> }
> --
> 2.25.1
>
--
Regards,
Sudeep
Re: linux-next: error trying to fetch the scmi tree
Hi Stephen, On Fri, Apr 16, 2021 at 09:03:41AM +1000, Stephen Rothwell wrote: > Hi all, > > Fetching the scmi tree produces this error: > > fatal: couldn't find remote ref refs/heads/for-linux-next > Thanks for letting me know. No idea how, but I have messed up and managed to push it as tag and also deleted the branch. It must be fixed now. > I will use the scmi tree from next-20210415 for today. > Thanks. -- Regards, Sudeep
[PATCH] efifb: Fix runtime pm calls for non PCI efifb device
Commit a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0")
added runtime pm calls to probe and remove routines to ensure the PCI
device for efifb stays in D0 state. However not ever efifb is based on
PCI device and efifb_pci_dev can be NULL if that is the case.
In such cases, we will get a boot splat like below due to NULL dereference:
-->8
Console: switching to colour frame buffer device 240x67
fb0: EFI VGA frame buffer device
Unable to handle kernel NULL pointer dereference at virtual address
0270
Mem abort info:
ESR = 0x9604
EC = 0x25: DABT (current EL), IL = 32 bits
SET = 0, FnV = 0
EA = 0, S1PTW = 0
Data abort info:
ISV = 0, ISS = 0x0004
CM = 0, WnR = 0
[0270] user address but active_mm is swapper
Internal error: Oops: 9604 [#1] PREEMPT SMP
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted 5.12.0-rc7-next-20210413 #1
Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno Development
Platform
pstate: 6005 (nZCv daif -PAN -UAO -TCO BTYPE=--)
pc : pm_runtime_drop_link+0x12c/0x338
lr : efifb_probe+0x7bc/0x7f0
Call trace:
pm_runtime_drop_link+0x12c/0x338
efifb_probe+0x7bc/0x7f0
platform_probe+0x68/0xd8
really_probe+0xe4/0x3a8
driver_probe_device+0x64/0xc8
device_driver_attach+0x74/0x80
__driver_attach+0x64/0xf0
bus_for_each_dev+0x70/0xc0
driver_attach+0x24/0x30
bus_add_driver+0x150/0x1f8
driver_register+0x64/0x120
__platform_driver_register+0x28/0x38
efifb_driver_init+0x1c/0x28
do_one_initcall+0x48/0x2b0
kernel_init_freeable+0x1e8/0x258
kernel_init+0x14/0x118
ret_from_fork+0x10/0x30
Code: 88027c01 35a2 17fff706 f9800051 (885f7c40)
---[ end trace 17d8da630bf8ff77 ]---
Kernel panic - not syncing: Attempted to kill init! exitcode=0x000b
-->8
Fix the issue by checking for non-NULL efifb_pci_dev before dereferencing
for runtime pm calls in probe and remove routines.
Fixes: a6c0fd3d5a8b ("efifb: Ensure graphics device for efifb stays at PCI D0")
Cc: Kai-Heng Feng
Cc: Alex Deucher
Cc: Thomas Zimmermann
Cc: Peter Jones
Signed-off-by: Sudeep Holla
---
drivers/video/fbdev/efifb.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/fbdev/efifb.c b/drivers/video/fbdev/efifb.c
index f58a545b3bf3..8ea8f079cde2 100644
--- a/drivers/video/fbdev/efifb.c
+++ b/drivers/video/fbdev/efifb.c
@@ -575,7 +575,8 @@ static int efifb_probe(struct platform_device *dev)
goto err_fb_dealoc;
}
fb_info(info, "%s frame buffer device\n", info->fix.id);
- pm_runtime_get_sync(_pci_dev->dev);
+ if (efifb_pci_dev)
+ pm_runtime_get_sync(_pci_dev->dev);
return 0;
err_fb_dealoc:
@@ -602,7 +603,8 @@ static int efifb_remove(struct platform_device *pdev)
unregister_framebuffer(info);
sysfs_remove_groups(>dev.kobj, efifb_groups);
framebuffer_release(info);
- pm_runtime_put(_pci_dev->dev);
+ if (efifb_pci_dev)
+ pm_runtime_put(_pci_dev->dev);
return 0;
}
--
2.25.1
[PATCH v3] dt-bindings: dvfs: Add support for generic performance domains
The CLKSCREW attack [0] exposed security vulnerabilities in energy management implementations where untrusted software had direct access to clock and voltage hardware controls. In this attack, the malicious software was able to place the platform into unsafe overclocked or undervolted configurations. Such configurations then enabled the injection of predictable faults to reveal secrets. Many Arm-based systems used to or still use voltage regulator and clock frameworks in the kernel. These frameworks allow callers to independently manipulate frequency and voltage settings. Such implementations can render systems susceptible to this form of attack. Attacks such as CLKSCREW are now being mitigated by not having direct and independent control of clock and voltage in the kernel and moving that control to a trusted entity, such as the SCP firmware or secure world firmware/software which are to perform sanity checking on the requested performance levels, thereby preventing any attempted malicious programming. With the advent of such an abstraction, there is a need to replace the generic clock and regulator bindings used by such devices with a generic performance domains bindings. [0] https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang Link: https://lore.kernel.org/r/20201116181356.804590-1-sudeep.ho...@arm.com Cc: Rob Herring Acked-by: Viresh Kumar Signed-off-by: Sudeep Holla --- Hi All, Sorry for the delay, I thought I had sent this out last week and it turns out that I had dry-run in my git email command and never removed it. Just noticed now looking for response for this patch on the list to find out that I never sent it out :(. v2[2]->v3: - Dropped required properties - Added non cpu device example - Updated cpu bindings too v1[1]->v2[2]: - Changed to Dual License - Added select: true, enum for #performance-domain-cells and $ref for performance-domain - Changed the example to use real existing compatibles instead of made-up ones [1] https://lore.kernel.org/lkml/20201105173539.1426301-1-sudeep.ho...@arm.com [2] https://lore.kernel.org/lkml/20201116181356.804590-1-sudeep.ho...@arm.com .../devicetree/bindings/arm/cpus.yaml | 7 ++ .../bindings/dvfs/performance-domain.yaml | 80 +++ 2 files changed, 87 insertions(+) create mode 100644 Documentation/devicetree/bindings/dvfs/performance-domain.yaml diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml b/Documentation/devicetree/bindings/arm/cpus.yaml index 26b886b20b27..98590a2982d0 100644 --- a/Documentation/devicetree/bindings/arm/cpus.yaml +++ b/Documentation/devicetree/bindings/arm/cpus.yaml @@ -255,6 +255,13 @@ description: |+ where voltage is in V, frequency is in MHz. + performance-domains: +$ref: '/schemas/types.yaml#/definitions/phandle-array' +description: + List of phandles and performance domain specifiers, as defined by + bindings of the performance domain provider. See also + dvfs/performance-domain.yaml. + power-domains: $ref: '/schemas/types.yaml#/definitions/phandle-array' description: diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml new file mode 100644 index ..640e676ed228 --- /dev/null +++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml @@ -0,0 +1,80 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Generic performance domains + +maintainers: + - Sudeep Holla + +description: |+ + This binding is intended for performance management of groups of devices or + CPUs that run in the same performance domain. Performance domains must not + be confused with power domains. A performance domain is defined by a set + of devices that always have to run at the same performance level. For a given + performance domain, there is a single point of control that affects all the + devices in the domain, making it impossible to set the performance level of + an individual device in the domain independently from other devices in + that domain. For example, a set of CPUs that share a voltage domain, and + have a common frequency control, is said to be in the same performance + domain. + + This device tree binding can be used to bind performance domain consumer + devices with their performance domains provided by performance domain + providers. A performance domain provider can be represented by any node in + the device tree and can provide one or more performance domains. A consumer + node can refer to the provider by a phandle and a set of phandle arguments + (so called performance domain specifiers) of length specified by the + \#performance-domain-cells pr
Re: [PATCH v7 00/38] SCMI vendor protocols and modularization
On Tue, 16 Mar 2021 12:48:25 +, Cristian Marussi wrote: > The current SCMI implementation does not provide an interface to easily > develop and include a custom vendor protocol implementation as prescribed > by the SCMI standard, also because, there is not currently any custom > protocol in the upstream to justify the development of a custom interface > and its maintenance. > > Moreover the current interface exposes protocol operations to the SCMI > driver users attaching per-protocol operations directly to the handle > structure, which, in this way, tends to grow indefinitely for each new > protocol addition. > > [...] Applied to sudeep.holla/linux (for-next/scmi), thanks! [01/38] firmware: arm_scmi: review protocol registration interface https://git.kernel.org/sudeep.holla/c/48dc16e2e5 [02/38] firmware: arm_scmi: introduce protocol handle definitions https://git.kernel.org/sudeep.holla/c/d7b6cc563a [03/38] firmware: arm_scmi: introduce devres get/put protocols operations https://git.kernel.org/sudeep.holla/c/23934efe37 [04/38] firmware: arm_scmi: make notifications aware of protocols users https://git.kernel.org/sudeep.holla/c/3dd2c81475 [05/38] firmware: arm_scmi: introduce new devres notification ops https://git.kernel.org/sudeep.holla/c/5ad3d1cf7d [06/38] firmware: arm_scmi: refactor events registration https://git.kernel.org/sudeep.holla/c/533c7095b1 [07/38] firmware: arm_scmi: convert events registration to protocol handles https://git.kernel.org/sudeep.holla/c/b9f7fd907c [08/38] firmware: arm_scmi: add new protocol handle core xfer ops https://git.kernel.org/sudeep.holla/c/a4a20b0975 [09/38] firmware: arm_scmi: add helper to access revision area memory https://git.kernel.org/sudeep.holla/c/3d5d6e84ea [10/38] firmware: arm_scmi: port Base protocol to new interface https://git.kernel.org/sudeep.holla/c/8d3581c252 [11/38] firmware: arm_scmi: port Perf protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/1fec5e6b52 [12/38] cpufreq: scmi: port driver to the new scmi_perf_proto_ops interface https://git.kernel.org/sudeep.holla/c/eb1d35c6e3 [13/38] firmware: arm_scmi: remove legacy scmi_perf_ops protocol interface https://git.kernel.org/sudeep.holla/c/f58315a49c [14/38] firmware: arm_scmi: port Power protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/9bc8069c85 [15/38] firmware: arm_scmi: port GenPD driver to the new scmi_power_proto_ops interface https://git.kernel.org/sudeep.holla/c/26f19496a9 [16/38] firmware: arm_scmi: remove legacy scmi_power_ops protocol interface https://git.kernel.org/sudeep.holla/c/0f84576a62 [17/38] firmware: arm_scmi: port Clock protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/887281c751 [18/38] clk: scmi: port driver to the new scmi_clk_proto_ops interface https://git.kernel.org/sudeep.holla/c/beb076bb18 [19/38] firmware: arm_scmi: remove legacy scmi_clk_ops protocol interface https://git.kernel.org/sudeep.holla/c/137e68659e [20/38] firmware: arm_scmi: port Reset protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/7e02934422 [21/38] reset: reset-scmi: port driver to the new scmi_reset_proto_ops interface https://git.kernel.org/sudeep.holla/c/35cc263062 [22/38] firmware: arm_scmi: remove legacy scmi_reset_ops protocol interface https://git.kernel.org/sudeep.holla/c/497ef0cbc6 [23/38] firmware: arm_scmi: port Sensor protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/9694a7f623 [24/38] hwmon: (scmi) port driver to the new scmi_sensor_proto_ops interface https://git.kernel.org/sudeep.holla/c/987bae41e9 [25/38] iio/scmi: port driver to the new scmi_sensor_proto_ops interface https://git.kernel.org/sudeep.holla/c/25cbdd4609 [26/38] firmware: arm_scmi: remove legacy scmi_sensor_ops protocol interface https://git.kernel.org/sudeep.holla/c/f3690d9729 [27/38] firmware: arm_scmi: port SystemPower protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/b46d852718 [28/38] firmware: arm_scmi: port Voltage protocol to new protocols interface https://git.kernel.org/sudeep.holla/c/fe4894d968 [29/38] regulator: scmi: port driver to the new scmi_voltage_proto_ops interface https://git.kernel.org/sudeep.holla/c/59046d157d [30/38] firmware: arm_scmi: remove legacy scmi_voltage_ops protocol interface https://git.kernel.org/sudeep.holla/c/c3ed5e953e [31/38] firmware: arm_scmi: make references to handle const https://git.kernel.org/sudeep.holla/c/f0e73cee26 [32/38] firmware: arm_scmi: cleanup legacy protocol init code https://git.kernel.org/sudeep.holla/c/51fe1b154e [33/38] firmware: arm_scmi: cleanup unused core xfer wrappers https://git.kernel.org/sudeep.holla/c/9162afa2ae [34/38] firmware:
Re: [PATCH v11 2/2] dt-bindings: cpufreq: add bindings for MediaTek cpufreq HW
On Tue, Mar 30, 2021 at 08:26:43AM +0530, Viresh Kumar wrote: > On 24-03-21, 10:07, Rob Herring wrote: > > On Fri, Mar 12, 2021 at 07:40:35PM +0800, Hector Yuan wrote: > > > From: "Hector.Yuan" > > > > > > Add devicetree bindings for MediaTek HW driver. > > > > > > Signed-off-by: Hector.Yuan > > > --- > > > .../bindings/cpufreq/cpufreq-mediatek-hw.yaml | 127 > > > > > > 1 file changed, 127 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml > > > > > > diff --git > > > a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml > > > b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml > > > new file mode 100644 > > > index 000..0f3ad47 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml > > > @@ -0,0 +1,127 @@ > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek-hw.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: MediaTek's CPUFREQ Bindings > > > + > > > +maintainers: > > > + - Hector Yuan > > > + > > > +description: > > > + CPUFREQ HW is a hardware engine used by MediaTek > > > + SoCs to manage frequency in hardware. It is capable of controlling > > > frequency > > > + for multiple clusters. > > > + > > > +properties: > > > + compatible: > > > +const: mediatek,cpufreq-hw > > > + > > > + reg: > > > +minItems: 1 > > > +maxItems: 2 > > > +description: | > > > + Addresses and sizes for the memory of the > > > + HW bases in each frequency domain. > > > + > > > + "#performance-domain-cells": > > > > A common binding schema for this and 'performance-domains' needs to land > > first. > > Sudeep, what happened to the series you had on this ? This patchset > has been blocked for a long time now, can we get that merged soonish > somehow ? Sorry, it slipped through the cracks. I posted this as a fix for SCMI which we fixed it later. This got de-prioritised in my todo list. Sorry for that. I think main problem I had is to write a proper select statement in YAML scheme to check the DT nodes when it is present. I couldn't get anything similar for reference from clocks. I had "select: false" which I knew was not acceptable as it can't go throw dt_bindings_check. I am happy if someone wants to pick up and work on that to push the change or provide suggestions that I can try out. I am unable to spend more time trying to understand whole YAML schema ATM. -- Regards, Sudeep
Re: [RFC PATCH] arm64: dts: allwinner: a64/h5: Add CPU idle states
On Tue, Mar 23, 2021 at 11:44:50PM -0500, Samuel Holland wrote: > 3) Modify the Linux PSCI client to respect PSCI_FEATURES when setting >psci_ops.cpu_suspend. cpuidle-psci checks for this function before >setting up idle states. We don't call PSCI_FEATURES on mandatory functions as it may not return anything meaningful for such mandatory functions(as that is not mandated in the spec). In short NACK to add PSCI_FEATURES check for mandatory functions. -- Regards, Sudeep
Re: [PATCH] ARM: vexpress: spc: fix uniprocessor initialization
Hi Arnd,
Sorry for the delay
On Tue, Mar 23, 2021 at 01:56:38PM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> When CONFIG_SMP is disabled, topology_physical_package_id()
> returns -1 and gcc notices undefined behavior:
>
> arch/arm/mach-versatile/spc.c: In function 've_spc_clk_init':
> arch/arm/mach-versatile/spc.c:583:21: error: array subscript -1 is below
> array bounds of 'bool[2]' {aka '_Bool[2]'} [-Werror=array-bounds]
> 583 | if (init_opp_table[cluster])
> | ~~^
> arch/arm/mach-versatile/spc.c:556:7: note: while referencing 'init_opp_table'
> 556 | bool init_opp_table[MAX_CLUSTERS] = { false };
> | ^~
>
> It's not clear to me what the correct behavior should be, but
> it seems safe to just not continue with the initialization.
>
I don't have any other patches for vexpress platform or driver support.
Can you apply this directly ? Assuming that,
Acked-by: Sudeep Holla
--
Regards,
Sudeep
Re: [PATCH v7 25/38] iio/scmi: port driver to the new scmi_sensor_proto_ops interface
Hi Jonathan, On Thu, Mar 18, 2021 at 12:12:02PM +, Sudeep Holla wrote: > On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote: > > Hi Christian, > > > > Thanks for the detailed explanation. Sounds good to me. > > > > Can I get official Reviewed-by or Acked-by please if you are fine with the > change ? > I need your ack to this via arm-soc. -- Regards, Sudeep
Re: [PATCH v7 18/38] clk: scmi: port driver to the new scmi_clk_proto_ops interface
Hi Stephen/Mike, On Tue, Mar 16, 2021 at 12:48:43PM +, Cristian Marussi wrote: > Port driver to the new SCMI Clock interface based on protocol handles > and common devm_get_ops(). > > Cc: Michael Turquette > Cc: Stephen Boyd Please provide ack for this change so that I can take the series via arm-soc. -- Regards, Sudeep
Re: [PATCH 1/2] arm64: dts: juno: Describe PCI dma-ranges
On Fri, 5 Mar 2021 17:33:17 +, Robin Murphy wrote: > The PLDA root complex on Juno relies on an address-based lookup table to > generate AXI attributes for inbound PCI transactions, and as such will > not pass any transaction not matching any programmed address range. The > standard firmware configuration programs 3 entries covering the GICv2m > MSI doorbell and the 2 DRAM regions, so add a "dma-ranges" property to > describe those usable inbound windows. (New commit info after rebase to v5.12-rc2 for obvious reasons) Applied to sudeep.holla/linux (for-next/juno), thanks! [1/2] arm64: dts: juno: Describe PCI dma-ranges https://git.kernel.org/sudeep.holla/c/4ac4d146cb [2/2] arm64: dts: juno: Enable more SMMUs https://git.kernel.org/sudeep.holla/c/d9df28ba58 -- Regards, Sudeep
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
On Thu, 18 Feb 2021 22:23:23 +, Nicola Mazzucato wrote: > In this V8 I have addressed your comments: > - correct the goto in patch 1/3 > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() > > Many thanks, > Nicola > > [...] (New commit info after rebase to v5.12-rc2 for obvious reasons) Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/3] scmi-cpufreq: Remove deferred probe https://git.kernel.org/sudeep.holla/c/71a37cd6a5 [2/3] scmi-cpufreq: Get opp_shared_cpus from opp-v2 for EM https://git.kernel.org/sudeep.holla/c/80a064dbd5 -- Regards, Sudeep
Re: [PATCH v7 25/38] iio/scmi: port driver to the new scmi_sensor_proto_ops interface
On Tue, Mar 16, 2021 at 10:38:43PM -0700, Jyoti Bhayana wrote: > Hi Christian, > > Thanks for the detailed explanation. Sounds good to me. > Can I get official Reviewed-by or Acked-by please if you are fine with the change ? I definitely need one from Jonathan to merge this and one from Jyoti is added bonus . -- Regards, Sudeep
Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
On Mon, Mar 15, 2021 at 11:13:24AM -0700, Sowjanya Komatineni wrote: > Hi Sudeep, > > I see you are one of the maintainer of PSCI driver. Please add any other > right persons if you think should also agree/comment. > > Can you please comment on below 2 items based on your feedback? > > 1. Can you please suggest on proper way of generalizing to pass state > residency time run-time along with state during state enter? > > Not sure if any other drivers need this but for Tegra as MCE firmware is > in-charge of states enter and decisions, passing run-time state residency > from kernel to ATF is required and agree on not using power_state value for > this which is against PSCI spec. > Yes, I prefer you need to get this added in the PSCI specification. I have passed this thread to the author of the specification. > 2. Regarding state thresholds, although state thresholds are policy related > in Tegra cpu idle perspective these thresholds are platform specific based > on use case and mainly for MCE firmware usage to decide on state transitions > for core and core clusters as well. > >From previous emails, I gather these can be moved to firmware and need not be there in DT ? > As these are Tegra platform specific, Please comment if any other concerns > in having this configured by Tegra CPU Idle kernel driver. > I prefer not to have Tegra specific idle driver if we can get the necessary changes in PSCI spec. We must then have just one PSCI idle driver in the kernel. > Based on my understanding only above issue-1 is PSCI compliant related. > Please confirm. > Correct. -- Regards, Sudeep
Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
On Fri, Mar 12, 2021 at 02:27:30PM -0800, Sowjanya Komatineni wrote: > Hi Sudeep, > > To make our driver PSCI compliant below are few updates to be done > > 1. Standardize passing residency time run-time thru PSCI to ATF. > Yes that was my initial understanding, but your previous response was confusing. I should have read this first. > From PSCI specification I only see PSCI_STAT_RESIDENCY and > PSCI_STAT_COUNT optional functions for PSCI1.0/PSCI1.1 > Indeed, we don't have any support to pass run-time residency hints in PSCI today. > Can you please help add right people to explore on possible ways to add > PSCI function for passing corresponding state residency time to ATF? > Before we jump to implementation in TF-A we need to get agreement to get this added to the specification to support in OSPM/Linux. TF-A is just one implementation of PSCI and may not be only one. Formally you can raise any specification related queries to https://developer.arm.com/support or supp...@arm.com. I will ask the author of PSCI specification internally to take a look at this thread and chime in if they can. > 2. Tegra CPU Idle support definitely need to pass deepest cluster state and > threshold to MCE firmware from DT and looks like we can move this > implementation to ATF > Yes, I just asked the same question as response to your earlier email. Thanks for confirming that it can be moved out of OSPM/Linux and DT > With both of the above implementation changes Tegra194 driver will be > PSCI compliant. > We still need to get agreement on the specification front . -- Regards, Sudeep
Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
+Lorenzo Hi Sowjanya, Sorry for the delayed response. I am still in vacation On Thu, Mar 11, 2021 at 01:11:37PM -0800, Sowjanya Komatineni wrote: > > On 3/10/21 6:52 PM, Sudeep Holla wrote: > > On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote: > > > On 3/7/21 8:37 PM, Sudeep Holla wrote: > > > > On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote: > > > > > This patch adds cpu-idle-states and corresponding state nodes to > > > > > Tegra194 CPU in dt-binding document > > > > > > > > > I see that this platform has PSCI support. Can you care to explain why > > > > you need additional DT bindings and driver for PSCI based CPU suspend. > > > > Until the reasons are convincing, consider NACK from my side for this > > > > driver and DT bindings. You should be really using those bindings and > > > > the driver may be with minor changes there. > > > > > > > MCE firmware is in charge of state transition for Tegra194 carmel CPUs. > > > > > Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel. > > No. Tegra194 CPU idle driver works with MCE firmware running in background > so cpuidle kernel driver also talks to MCE firmware directly on state > information. If that is the case I wouldn't term this as PSCI compliant firmware and wouldn't attempt to use PSCI CPU idle driver. Now if we would what to allow non-PSCI idle driver for Arm64 is entirely different question that deserves a separate discussion IMO. > > > > > For run-time state transitions, need to provide state request along with > > > its > > > residency time to MCE firmware which is running in the background. > > > > > Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need > > to make this firmware PSCI compliant or just say it is not and implement > > completely independent implementation. I am not saying that is acceptable > > ATM but I prefer not to mix some implementation to make it look like > > PSCI compliant. > > > > > State min residency is updated into power_state value along with state id > > > that is passed to psci_cpu_suspend_enter > > > > > Sounds like a hack/workaround. I would prefer to standardise that. IIUC > > the power_state is more static and derived from DT. I don't like to > > overload that TBH. Need to check with authors of that binding. > > Passing state idle time to ATF along with state to enter is Tegra specific > as ATF firmware updates idle time to Tegra MCE firmware which will be used > for deciding on state transition along with other information and background > load. > So far we don't have any platform specific PSCI in OSPM and I prefer to keep it that way. > Not sure if this need to be standardized but will try to find alternate way > to update idle time without misusing power-state value. > Sure, we can always review and see if any alternatives are acceptable, but I am bit nervous to tie this as PSCI if it is not strictly spec compliant. > Will discuss on this internally and get back. > Thanks. > > > > > Also states cross-over idle times need to be provided to MCE firmware. > > > > > New requirements if this has to be PSCI compliant. > > Updating cross-over idle times from DT to MCE firmware directly from cpuidle > kernel driver with corresponding MCE ARI commands is again Tegra specific. > So all there are platform specific but static information you need from DT ? If so, what can't it be made part of TF-A and OSPM can avoid interfering with that info completely. My understanding was that OSPM provides runtime hints like x86 mwait. If that's not the case, I am failing to understand the need for OSPM to pass such static information from DT to the firmware. Why can't that be just part of the firmware to begin with ? > > > > > MCE firmware decides on state transition based on these inputs along with > > > its background work load. > > > What do you mean by this *"background work load"* ? -- Regards, Sudeep
Re: [PATCH v7 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
Hi Jonathan, On Sun, Mar 14, 2021 at 03:40:33PM +, Jonathan Cameron wrote: > On Sat, 13 Mar 2021 11:55:39 -0800 > Jyoti Bhayana wrote: > > > Hi Jonathan, > > > > I still see the old version 6 in ib-iio-scmi-5.12-rc2-take2 as well. > > OK. I'm completely confused as to what is going with my local tree. > I have the right patch in the history but it didn't end up in the final > pushed out version. Fat finger mess-up I guess and too many similarly named > branches and the fact I didn't check the final result closely enough. > > There is now an ib-iio-scmi-5.12-rc2-take3 branch > I have now used this for my for-next/scmi branch. It is not final yet, just pushed out for bots to build test and get into -next. Let me know if you have plans to change/rework this branch, I can update it anytime till end of this week to avoid multiple hashes. -- Regards, Sudeep
Re: [PATCH v6 37/37] firmware: arm_scmi: add dynamic scmi devices creation
Hi Cristian,
Sorry for the delay.
On Tue, Feb 02, 2021 at 10:15:55PM +, Cristian Marussi wrote:
> Having added the support for SCMI protocols as modules in order to let
> vendors extend the SCMI core with their own additions it seems odd to
> then force SCMI drivers built on top to use a static device table to
> declare their devices since this way any new SCMI drivers addition
> would need the core SCMI device table to be updated too.
>
> Remove the static core device table and let SCMI drivers to simply declare
> which device/protocol pair they need at initialization time: the core will
> then take care to generate such devices dynamically during platform
> initialization or at module loading time, as long as the requested
> underlying protocol is defined in the DT.
>
> Signed-off-by: Cristian Marussi
> ---
> v4 --> v5
> - using klist instead of custom lists
> v3 --> v4
> - add a few comments
> ---
> drivers/firmware/arm_scmi/bus.c| 30 +++
> drivers/firmware/arm_scmi/common.h | 5 +
> drivers/firmware/arm_scmi/driver.c | 309 +
> 3 files changed, 310 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 88e5057f4e85..88149a46e6d9 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -51,6 +51,31 @@ static int scmi_dev_match(struct device *dev, struct
> device_driver *drv)
> return 0;
> }
>
> +static int scmi_match_by_id_table(struct device *dev, void *data)
> +{
> + struct scmi_device *sdev = to_scmi_dev(dev);
> + struct scmi_device_id *id_table = data;
> +
> + return sdev->protocol_id == id_table->protocol_id &&
> + !strcmp(sdev->name, id_table->name);
> +}
> +
> +struct scmi_device *scmi_find_child_dev(struct device *parent,
> + int prot_id, const char *name)
> +{
> + struct scmi_device_id id_table;
> + struct device *dev;
> +
> + id_table.protocol_id = prot_id;
> + id_table.name = name;
> +
> + dev = device_find_child(parent, _table, scmi_match_by_id_table);
> + if (!dev)
> + return NULL;
> +
> + return to_scmi_dev(dev);
> +}
> +
> const struct scmi_protocol *scmi_get_protocol(int protocol_id)
> {
> const struct scmi_protocol *proto;
> @@ -114,6 +139,10 @@ int scmi_driver_register(struct scmi_driver *driver,
> struct module *owner,
> {
> int retval;
>
> + retval = scmi_request_protocol_device(driver->id_table);
> + if (retval)
> + return retval;
> +
> driver->driver.bus = _bus_type;
> driver->driver.name = driver->name;
> driver->driver.owner = owner;
> @@ -130,6 +159,7 @@ EXPORT_SYMBOL_GPL(scmi_driver_register);
> void scmi_driver_unregister(struct scmi_driver *driver)
> {
> driver_unregister(>driver);
> + scmi_unrequest_protocol_device(driver->id_table);
> }
> EXPORT_SYMBOL_GPL(scmi_driver_unregister);
>
> diff --git a/drivers/firmware/arm_scmi/common.h
> b/drivers/firmware/arm_scmi/common.h
> index 1e2046c61d43..9a0519db4865 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -307,6 +307,11 @@ struct scmi_transport_ops {
> bool (*poll_done)(struct scmi_chan_info *cinfo, struct scmi_xfer *xfer);
> };
>
> +int scmi_request_protocol_device(const struct scmi_device_id *id_table);
> +void scmi_unrequest_protocol_device(const struct scmi_device_id *id_table);
Sorry for being pedantic, I don't like these names. I would prefer
something like scmi_protocol_device_{create,destroy/delete}_request.
The action the function does needs to be at the end of the function name.
Atleast that is something I follow. I haven't checked all the previous
patches, just this function made to look at both the name style and the
name itself.
> +struct scmi_device *scmi_find_child_dev(struct device *parent,
> + int prot_id, const char *name);
> +
scmi_child_dev_find based on what I mentioned above. Please change all
other non-static functions even if I have not mentioned. Try to cover
all the new functions introduced in this series, existing ones we can
take up later.
> /**
> * struct scmi_desc - Description of SoC integration
> *
> diff --git a/drivers/firmware/arm_scmi/driver.c
> b/drivers/firmware/arm_scmi/driver.c
> index dcdfd94b47f7..9fc979e3b16f 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -56,6 +57,14 @@ static DEFINE_MUTEX(scmi_list_mutex);
> /* Track the unique id for the transfers for debug & profiling purpose */
> static atomic_t transfer_last_id;
>
> +static DEFINE_IDR(scmi_requested_devices);
> +static DEFINE_MUTEX(scmi_requested_devices_mtx);
> +
> +struct scmi_requested_dev {
> + const struct scmi_device_id *id_table;
> +
Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
On Mon, Mar 08, 2021 at 10:32:17AM -0800, Sowjanya Komatineni wrote: > > On 3/7/21 8:37 PM, Sudeep Holla wrote: > > On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote: > > > This patch adds cpu-idle-states and corresponding state nodes to > > > Tegra194 CPU in dt-binding document > > > > > I see that this platform has PSCI support. Can you care to explain why > > you need additional DT bindings and driver for PSCI based CPU suspend. > > Until the reasons are convincing, consider NACK from my side for this > > driver and DT bindings. You should be really using those bindings and > > the driver may be with minor changes there. > > > MCE firmware is in charge of state transition for Tegra194 carmel CPUs. > Sure, but I assume only TF-A talks to MCE and not any OSPM/Linux kernel. > For run-time state transitions, need to provide state request along with its > residency time to MCE firmware which is running in the background. > Sounds similar to x86 mwait, perhaps we need to extend PSCI if we need to make this firmware PSCI compliant or just say it is not and implement completely independent implementation. I am not saying that is acceptable ATM but I prefer not to mix some implementation to make it look like PSCI compliant. > State min residency is updated into power_state value along with state id > that is passed to psci_cpu_suspend_enter > Sounds like a hack/workaround. I would prefer to standardise that. IIUC the power_state is more static and derived from DT. I don't like to overload that TBH. Need to check with authors of that binding. > Also states cross-over idle times need to be provided to MCE firmware. > New requirements if this has to be PSCI compliant. > MCE firmware decides on state transition based on these inputs along with > its background work load. > > So, Tegra specific CPU idle driver is required mainly to provide cross-over > thresholds from DT and run time idle state information to MCE firmware > through Tegra MCE communication APIs. > I am worried if different vendors will come up with different custom solution for this. We need to either standardise this is Linux/DT or in PSCI. > Allowing cross-over threshold through DT allows users to vary idle time > thresholds for state transitions based on different use-cases. > Sounds like policy and not platform specific to be in DT, but I will leave that to DT maintainers. -- Regards, Sudeep
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
On Thu, 18 Feb 2021 22:23:23 +, Nicola Mazzucato wrote: > In this V8 I have addressed your comments: > - correct the goto in patch 1/3 > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() > > Many thanks, > Nicola > > [...] Applied the first 2 patches to sudeep.holla/linux (for-next/scmi), thanks! [1/3] scmi-cpufreq: Remove deferred probe https://git.kernel.org/sudeep.holla/c/2a3390d53b [2/3] scmi-cpufreq: Get opp_shared_cpus from opp-v2 for EM https://git.kernel.org/sudeep.holla/c/dac7a57d2a -- Regards, Sudeep
Re: [PATCH v6 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
On Mon, Mar 08, 2021 at 07:48:41PM +, Jonathan Cameron wrote: > On Mon, 8 Mar 2021 04:28:42 + > Sudeep Holla wrote: > > > Hi Jonathan, > > > > On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote: > > > Hi Jonathan, > > > > > > Thanks for the detailed and careful review of this patch. Good to hear > > > that v7 is not required. Please find below answers to your > > > questions. Looking forward to seeing this patch merged in the next > > > cycle. Thanks for your help in making this happen. > > > > > > > Any update on this ? Please share the branch with is patch so that I > > can pull and ask Cristian to make his changes on top. > Running a bit behind at the moment. > No worries. > Anyhow, there should now be an ib-iio-scmi-5.12-rc1 branch > on https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git > Thanks > Includes making the various long long local variables explicitly > s64 and u64 as relevant. > > Based on the rc1 that eats babies so handle with care :) > > I've also merge this into the togreg branch of iio.git. > As explained above that wasn't entirely trivial so Jyoti > please take a quick look and check that changes are fine. > Pushed out as testing to let the autobuilders poke at it. > Assuming they don't find anything, it should be fine > for Sudeep to merge that ib and everything will unwind > nicely in Linus' tree next cycle. > Hope so. > There is a bit of an ongoing discussion of an earlier patch > in the IIO tree, so I might end up redoing this merge > if I need to rebase to sort that out, but I'll make sure > the diff is the same (git ID might change). > I can wait for a week or 2 if you think things will settle down by then. We can avoid 2 different git IDs if possible. The main intention was to give some reference to Cristian to rebase/post his series. I am all fine to wait for a week or 2 for final branch. -- Regards, Sudeep
Re: [PATCH v6 36/37] firmware: arm_scmi: add protocol modularization support
On Tue, Feb 02, 2021 at 10:15:54PM +, Cristian Marussi wrote: > Extend SCMI protocols accounting mechanism to address possible module > usage and add the support to possibly define new protocols as loadable > modules. > > Keep Standard protocols built into the SCMI core. > The changes look good, however without any users I am bit hesitant to add this yet. However if you think it is hard to maintain it outside the tree until first user gets merged, we can merge provided we test this every release. Let me know your thoughts. Also any comment from users requesting this would be useful. -- Regards, Sudeep
Re: [PATCH v6 35/37] firmware: arm_scmi: make notify_priv really private
On Tue, Feb 02, 2021 at 10:15:53PM +, Cristian Marussi wrote: > Notification private data is currently accessible via handle->notify_priv; > this data was indeed meant to be private to the notification core support > and not to be accessible by SCMI drivers: make it private hiding it inside > instance descriptor struct scmi_info and accessible only via dedicated > helpers. > Patch 3 - 35 looks good to me. I need to look at last 2 patches again. So thought of replying instead of keeping you waited. You can start rebasing once we pull IIO branch from Jonathan. I will keep you posted. -- Regards, Sudeep
Re: [PATCH v6 02/37] firmware: arm_scmi: introduce protocol handle definitions
On Tue, Feb 02, 2021 at 10:15:20PM +, Cristian Marussi wrote: > Add basic protocol handles definitions and private data helpers support. > > A protocol handle identifies a protocol instance initialized against a > specific handle; it embeds all the references to the core SCMI xfer methods > that will be needed by a protocol implementation to build and send its own > protocol specific messages using common core methods. > > As such, in the interface, a protocol handle will be passed down from the > core to the protocol specific initialization callback at init time. > > Anyway at this point only definitions are introduced, all protocols > initialization code and SCMI drivers probing is still based on the old > interface, so no functional change. > > Signed-off-by: Cristian Marussi > --- > drivers/firmware/arm_scmi/common.h | 59 ++ > drivers/firmware/arm_scmi/driver.c | 45 +++ > 2 files changed, 104 insertions(+) > [...] > diff --git a/drivers/firmware/arm_scmi/driver.c > b/drivers/firmware/arm_scmi/driver.c > index ed94efbecd61..2328a468bbd1 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c [...] > /** > * scmi_get_protocol_instance - Protocol initialization helper. > * @handle: A reference to the SCMI platform instance. > @@ -588,6 +629,10 @@ scmi_get_protocol_instance(struct scmi_handle *handle, > u8 protocol_id) > > pi->gid = gid; > pi->proto = proto; > + pi->handle = handle; > + pi->ph.dev = handle->dev; > + pi->ph.set_priv = scmi_set_protocol_priv; > + pi->ph.get_priv = scmi_get_protocol_priv; Sorry missed this in earlier patch. Not a must, but I prefer if you can move all these initialisation into separate functions so that scmi_get_protocol_instance can be simplified to read. if (pi) increment refcount else scmi_get_protocol alloc and init protocol instance register events How about some thing like above ? -- Regards, Sudeep
Re: [PATCH v6 01/37] firmware: arm_scmi: review protocol registration interface
On Tue, Feb 02, 2021 at 10:15:19PM +, Cristian Marussi wrote:
> Extend common protocol registration routines and provide some new generic
> protocols get/put helpers that can track protocols usage and automatically
> perform the proper initialization and de-initialization on demand when
> required.
>
> Convert all standard protocols to use this new registration scheme while
> keeping them all still using the usual initialization logic bound to SCMI
> devices probing.
>
> Signed-off-by: Cristian Marussi
> ---
> v2 --> v3
> - removed new Base protocol initialization, it will be re-introduced
> later with all other protocols
> ---
> drivers/firmware/arm_scmi/base.c| 8 ++
> drivers/firmware/arm_scmi/bus.c | 61 ---
> drivers/firmware/arm_scmi/clock.c | 10 +-
> drivers/firmware/arm_scmi/common.h | 30 +-
> drivers/firmware/arm_scmi/driver.c | 159 +++-
> drivers/firmware/arm_scmi/perf.c| 10 +-
> drivers/firmware/arm_scmi/power.c | 10 +-
> drivers/firmware/arm_scmi/reset.c | 10 +-
> drivers/firmware/arm_scmi/sensors.c | 8 +-
> drivers/firmware/arm_scmi/system.c | 8 +-
> drivers/firmware/arm_scmi/voltage.c | 8 +-
> include/linux/scmi_protocol.h | 6 +-
> 12 files changed, 296 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c
> index 1377ec76a45d..044aa9e3ebb0 100644
> --- a/drivers/firmware/arm_scmi/bus.c
> +++ b/drivers/firmware/arm_scmi/bus.c
> @@ -16,7 +16,7 @@
> #include "common.h"
>
> static DEFINE_IDA(scmi_bus_id);
> -static DEFINE_IDR(scmi_protocols);
> +static DEFINE_IDR(scmi_available_protocols);
[nit] Any particular reason for this name change ? Does it hold refer to
something different from before ? IIRC, this is list of registered protocols ?
Available might refer to the ones advertised to be present by the platform
firmware ?
> static DEFINE_SPINLOCK(protocol_lock);
>
> static const struct scmi_device_id *
> @@ -51,13 +51,29 @@ static int scmi_dev_match(struct device *dev, struct
> device_driver *drv)
> return 0;
> }
>
> +const struct scmi_protocol *scmi_get_protocol(int protocol_id)
> +{
> + const struct scmi_protocol *proto;
> +
> + proto = idr_find(_available_protocols, protocol_id);
> + if (!proto) {
> + pr_warn("SCMI Protocol 0x%x not found!\n", protocol_id);
> + return NULL;
> + }
> +
> + pr_debug("GOT SCMI Protocol 0x%x\n", protocol_id);
> +
[nit] For sake of consistency, s/GOT/Found/
[..]
> @@ -194,26 +210,45 @@ void scmi_set_handle(struct scmi_device *scmi_dev)
> scmi_dev->handle = scmi_handle_get(_dev->dev);
> }
>
> -int scmi_protocol_register(int protocol_id, scmi_prot_init_fn_t fn)
> +int scmi_protocol_register(const struct scmi_protocol *proto)
> {
> int ret;
>
> + if (!proto) {
> + pr_err("invalid protocol\n");
> + return -EINVAL;
> + }
> +
> + if (!proto->init && !proto->init_instance) {
> + pr_err("missing .init() for protocol 0x%x\n", proto->id);
s/.init()/init as it can be init_instance too ?
> diff --git a/drivers/firmware/arm_scmi/clock.c
> b/drivers/firmware/arm_scmi/clock.c
> index 4645677d86f1..e8c84cff9922 100644
> --- a/drivers/firmware/arm_scmi/clock.c
> +++ b/drivers/firmware/arm_scmi/clock.c
> @@ -2,7 +2,7 @@
> /*
> * System Control and Management Interface (SCMI) Clock Protocol
> *
> - * Copyright (C) 2018 ARM Ltd.
> + * Copyright (C) 2018-2020 ARM Ltd.
2021 perhaps ? Few instances are not updated, I prefer to be consistent
across all modified scmi firmware driver files.
Other than these minor comments, the other changes looks good to me.
--
Regards,
Sudeep
Re: [PATCH v1 3/5] dt-bindings: arm: Add cpu-idle-states to Tegra194 CPU nodes
On Wed, Mar 03, 2021 at 10:08:10PM -0800, Sowjanya Komatineni wrote: > This patch adds cpu-idle-states and corresponding state nodes to > Tegra194 CPU in dt-binding document > I see that this platform has PSCI support. Can you care to explain why you need additional DT bindings and driver for PSCI based CPU suspend. Until the reasons are convincing, consider NACK from my side for this driver and DT bindings. You should be really using those bindings and the driver may be with minor changes there. -- Regards, Sudeep
Re: [PATCH v6 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
Hi Jonathan, On Tue, Feb 23, 2021 at 10:30:37AM -0800, Jyoti Bhayana wrote: > Hi Jonathan, > > Thanks for the detailed and careful review of this patch. Good to hear > that v7 is not required. Please find below answers to your > questions. Looking forward to seeing this patch merged in the next > cycle. Thanks for your help in making this happen. > Any update on this ? Please share the branch with is patch so that I can pull and ask Cristian to make his changes on top. -- Regards, Sudeep
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
On Mon, Feb 22, 2021 at 10:09:04AM +0530, Viresh Kumar wrote: > On 19-02-21, 19:16, Sudeep Holla wrote: > > Hi Viresh, > > > > On Fri, Feb 19, 2021 at 09:49:44AM +0530, Viresh Kumar wrote: > > > On 18-02-21, 22:23, Nicola Mazzucato wrote: > > > > Hi Viresh, > > > > > > > > In this V8 I have addressed your comments: > > > > - correct the goto in patch 1/3 > > > > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() > > > > > > LGTM. I will apply them after the merge window is over. Thanks. > > > > I am planning to merge the series on scmi[1] which changes scmi-cpufreq.c > > and will conflict with these changes I think. If possible either, > > > > 1. Share a branch with these changes that I can merge or > > 2. I can take patch 1/3 and 2/3 with other scmi changes with your Ack. > > > > I am fine either way, let me know by v5.12-rc1 > > I have applied 3/3, you can take first two and add my Ack. > > Acked-by: Viresh Kumar > Thanks Viresh, I will pick after v5.12-rc1 -- Regards, Sudeep
Re: [PATCH v8 0/3] CPUFreq: Add support for opp-sharing cpus
Hi Viresh, On Fri, Feb 19, 2021 at 09:49:44AM +0530, Viresh Kumar wrote: > On 18-02-21, 22:23, Nicola Mazzucato wrote: > > Hi Viresh, > > > > In this V8 I have addressed your comments: > > - correct the goto in patch 1/3 > > - improve comment in patch 2/3 for dev_pm_opp_get_opp_count() > > LGTM. I will apply them after the merge window is over. Thanks. I am planning to merge the series on scmi[1] which changes scmi-cpufreq.c and will conflict with these changes I think. If possible either, 1. Share a branch with these changes that I can merge or 2. I can take patch 1/3 and 2/3 with other scmi changes with your Ack. I am fine either way, let me know by v5.12-rc1 -- Regards, Sudeep [1] https://lore.kernel.org/linux-arm-kernel/20210202221555.41167-1-cristian.maru...@arm.com/
Re: [PATCH v5 1/1] iio/scmi: Adding support for IIO SCMI Based Sensors
On Mon, Feb 15, 2021 at 11:07:56AM +, Jonathan Cameron wrote: > Hi Cristian, > > So this driver will also be 5.13 material now (merge window for IIO > effectively > closes 1-2 weeks before Linus opens the main one). > I guessed so. > The way we normally handle cases like this where we likely to have > dependencies > on a patch set from two separate directions is to do what is known as an > immutable branch. This is a branch that would probably be based on 5.12-rc1 > containing just this driver. > Make sense. > Then both trees, in this case IIO and scmi merge that branch. The magic > of git then means that when Linus gets the eventual pull requests for > the two trees, the git IDs and content will be the same and the history > of that particular set of files will be cleanly maintained. > > This happens quite a lot for certain parts of the kernel because there are > a lot of cross dependencies. > > @Sudeep, that work for you? Have to wait for 5.12-rc1 though to give > us a sensible base. > Since this is just one patch, I can pull in the immutable branch if you could share and then Cristian can make appropriate changes needed for his series on top of that unless you want to merge 30+ patches that I might have with Cristain series . In short I am happy to pull in your immutable branch and I agree 5.12-rc1 is sensible base. -- Regards, Sudeep
Re: [PATCH v6 13/37] cpufreq: scmi: port driver to the new scmi_perf_proto_ops interface
On Wed, Feb 03, 2021 at 12:10:35PM +, Cristian Marussi wrote: > Hi Viresh > > > On Wed, Feb 03, 2021 at 08:33:45AM +0530, Viresh Kumar wrote: > > On 02-02-21, 22:15, Cristian Marussi wrote: > > > Port driver to the new SCMI Perf interface based on protocol handles > > > and common devm_get_ops(). > > > > > > Cc: Rafael J. Wysocki > > > Cc: Viresh Kumar > > > Signed-off-by: Cristian Marussi > > > --- > > > v4 --> v5 > > > - using renamed devm_get/put_protocol > > > > Can this go through the PM tree ? > > Just to be clear, this is targeted for v5.13 not v5.12 in case you are worried about some conflicts. > > It's part of a long series changing internal and external SCMI > interfaces, series including a bit of per-protocol transient code added > and removed around this patch to maintain bisectability along the way... > > ...I'd not recommend split this patch out and committ it out of the series, > if this is what you meant. > > Anyway up to Sudeep really. > I prefer to get the entire series merged at once too. We can see if there are any conflicts once I start merging them more v5.12-rc1 or later. -- Regards, Sudeep
Re: [PATCH] arm64: kernel: Make IPI_WAKEUP under control of CONFIG_ARM64_ACPI_PARKING_PROTOCOL
On Tue, Jan 19, 2021 at 02:06:09PM +, Chen Jun wrote:
> since commit 5e89c55e4ed81d7abb1ce8828db35fa389dc0e90
> ("arm64: kernel: implement ACPI parking protocol")
>
> On arm64, IPI 6 will be wasted without setting
> CONFIG_ARM64_ACPI_PARKING_PROTOCOL.
>
> Signed-off-by: Chen Jun
> ---
> arch/arm64/kernel/smp.c | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c
> index ad00f99ee9b0..8b494ad1c61e 100644
> --- a/arch/arm64/kernel/smp.c
> +++ b/arch/arm64/kernel/smp.c
> @@ -73,7 +73,9 @@ enum ipi_msg_type {
> IPI_CPU_CRASH_STOP,
> IPI_TIMER,
Going by that logic, IPI_TIMER is used only when
CONFIG_GENERIC_CLOCKEVENTS_BROADCAST=y and
> IPI_IRQ_WORK,
IPI_IRQ_WORK when CONFIG_IRQ_WORK=y
Not sure if we what to go down that path as it may result in confusion
about IPI number and what OS is using it for. I am not against this, just
want to know others opinion on this.
> +#ifdef CONFIG_ARM64_ACPI_PARKING_PROTOCOL
> IPI_WAKEUP,
> +#endif
Just out of curiosity, are you changing this only by code inspection or
or you running short of IPI with some downstream patches. It will be
interesting to know how you are using then if you are as it may break if
kernel decides to you them in future for its own use.
--
Regards,
Sudeep
Re: [PATCH v1 5/9] firmware: keembay: Add support for Trusted Firmware Service call
On Tue, Jan 19, 2021 at 02:38:32AM +, Zulkifli, Muhammad Husaini wrote: [...] > > I try to hook up the DT last night. Seems like the SCMI Protocol 17 is not > implemented at ATF side. I had guessed that. > Double check with ATF Team, currently we don't have SCMI voltage domain > control in ARM Trusted Firmware yet as of now, that is why even if I map the > function to scmi, my call will be fail. Correct, but if you already have this custom SMCCC for voltage already implemented in TF-A, I don't see it is a big deal to support voltage protocol there. > > [2.648989] arm-scmi firmware:scmi: SCMI Notifications - Core Enabled. > [2.656157] arm-scmi firmware:scmi: SCMI Protocol v1.0 'INTEL:KMB' > Firmware version 0x1 > [2.664513] arm-scmi firmware:scmi: SCMI protocol 23 not implemented > [2.675898] arm-scmi firmware:scmi: SCMI protocol 17 not implemented > > Any possibilities that for UHS patch we go with my current regulator driver > implementation? Sorry absolutely no. If this platform was not using SCMI, I wouldn't have pushed back hard on this custom SMCCC. Please update TF-A to add this support. There is no point in having custom interface just for this when everything else is already using SCMI on this platform. -- Regards, Sudeep
Re: [PATCH v1 5/9] firmware: keembay: Add support for Trusted Firmware Service call
On Mon, Jan 18, 2021 at 10:28:33AM +, Zulkifli, Muhammad Husaini wrote:
> Hi Sudeep and Mark,
>
> Thanks for the review. I replied inline.
>
> >-Original Message-----
> >From: Sudeep Holla
> >Sent: Saturday, January 16, 2021 2:58 AM
> >To: Mark Brown
> >Cc: Zulkifli, Muhammad Husaini ;
> >ulf.hans...@linaro.org; lgirdw...@gmail.com; robh...@kernel.org;
> >devicet...@vger.kernel.org; Hunter, Adrian ;
> >michal.si...@xilinx.com; linux-...@vger.kernel.org; linux-
> >ker...@vger.kernel.org; Shevchenko, Andriy
> >; A, Rashmi ; Vaidya,
> >Mahesh R ; Sudeep Holla
> >
> >Subject: Re: [PATCH v1 5/9] firmware: keembay: Add support for Trusted
> >Firmware Service call
> >
> >On Thu, Jan 14, 2021 at 04:48:11PM +, Mark Brown wrote:
> >> On Thu, Jan 14, 2021 at 11:26:56PM +0800, Muhammad Husaini Zulkifli
> >wrote:
> >> > Export inline function to encapsulate AON_CFG1 for controling the
> >> > I/O Rail supplied voltage levels which communicate with Trusted Firmware.
> >>
> >> Adding Sudeep for the SMCCC bits, not deleting any context for his
> >> benefit.
> >>
> >
> >Thanks Mark for cc-ing me and joining the dots. I completely forgot about
> >that
> >fact that this platform was using SCMI using SMC as transport. Sorry for
> >that and
> >it is my fault. I did review the SCMI/SMC support for this platform sometime
> >in
> >June/July last year and forgot the fact it is same platform when
> >voltage/regulator support patches for SD/MMC was posted sometime later last
> >year. I concentrated on SMCCC conventions and other details.
>
> Yes Sudeep. I redesigned the way I handle the smccc call. Previously it was
> handled directly in mmc driver.
> After few discussion, we conclude to create an abstraction using regulator
> framework to encapsulate this smccc call
> during set voltage operation. Using standard abstraction will make things
> easier for the maintainer.
>
> >
> >[...]
> >
> >> > +#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE\
> >> > +ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> >> > + ARM_SMCCC_SMC_32,\
> >> > + ARM_SMCCC_OWNER_SIP, \
> >> > + KEEMBAY_SET_SD_VOLTAGE_ID)
> >> > +
> >> > +#define ARM_SMCCC_SIP_KEEMBAY_GET_SD_VOLTAGE\
> >> > +ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> >> > + ARM_SMCCC_SMC_32,\
> >> > + ARM_SMCCC_OWNER_SIP, \
> >> > + KEEMBAY_GET_SD_VOLTAGE_ID)
> >> > +
> >> > +#define KEEMBAY_REG_NUM_CONSUMERS 2
> >> > +
> >> > +struct keembay_reg_supply {
> >> > +struct regulator *consumer;
> >> > +};
> >> > +
> >> > +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> >> > +/*
> >> > + * Voltage applied on the IO Rail is controlled from the Always On
> >> > +Register using specific
> >> > + * bits in AON_CGF1 register. This is a secure register. Keem Bay
> >> > +SOC cannot exposed this
> >> > + * register address to the outside world.
> >> > + */
> >> > +static inline int keembay_set_io_rail_supplied_voltage(int volt) {
> >> > +struct arm_smccc_res res;
> >> > +
> >> > +
> > arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTA
> >GE, volt,
> >> > +);
> >>
> >> There is a SCMI voltage domain protocol intended for just this use
> >> case of controlling regulators managed by the firmware, why are you
> >> not using that for these systems? See drivers/firmware/arm_scmi/voltage.c.
>
> From mmc maintainer's perspective, I should use the common modelling either
> using regulator framework or pinctrl to perform voltage operation. Not just
> directly invoke smccc call in the mmc driver. That is why I came up with
> this regulator driver to perform voltage operation.
>
That's correct. Since the platform uses SCMI and SCMI spec[1] supports Voltage
protocol and there is upstream driver[2] to support it, I see no point in
duplicating the support with another custom/non-standard solution.
> >>
> >
> >Indeed. Please switch to using the new voltage protocol added for this
> >without
> >any extra code. You just need to wire up DT for this.
>
> May I know even if I wir
Re: [PATCH v1 8/9] regulator: keembay: Add regulator for Keem Bay SoC
On Thu, Jan 14, 2021 at 05:14:34PM +, Mark Brown wrote: > On Thu, Jan 14, 2021 at 11:26:59PM +0800, Muhammad Husaini Zulkifli wrote: > > > Keem Bay SD regulator driver module is added to encapsulate ARM > > Secure Monitor Call Calling Convention (SMCCC) during set voltage > > operations to control I/O Rail supplied voltage levels which communicate > > with Trusted Firmware. > > Adding in Sudeep again for the SMCCC bits. I just checked and am I > right in thinking this might already be shipping in production? > OK you seem to have asked the most important question here directly. I have asked for the details of platform firmware implementation in the other email basically for the same reason(to check how feasible is it to move to new SCMI voltage protocol). Some work in the firmware, but if the implement is on the A-profile itself in TF-A or any other equivalent, it should not be too difficult. -- Regards, Sudeep
Re: [PATCH v1 5/9] firmware: keembay: Add support for Trusted Firmware Service call
On Thu, Jan 14, 2021 at 04:48:11PM +, Mark Brown wrote:
> On Thu, Jan 14, 2021 at 11:26:56PM +0800, Muhammad Husaini Zulkifli wrote:
> > Export inline function to encapsulate AON_CFG1 for controling the I/O Rail
> > supplied voltage levels which communicate with Trusted Firmware.
>
> Adding Sudeep for the SMCCC bits, not deleting any context for his
> benefit.
>
Thanks Mark for cc-ing me and joining the dots. I completely forgot about
that fact that this platform was using SCMI using SMC as transport. Sorry
for that and it is my fault. I did review the SCMI/SMC support for this
platform sometime in June/July last year and forgot the fact it is same
platform when voltage/regulator support patches for SD/MMC was posted
sometime later last year. I concentrated on SMCCC conventions and other
details.
[...]
> > +#define ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE \
> > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> > + ARM_SMCCC_SMC_32,\
> > + ARM_SMCCC_OWNER_SIP, \
> > + KEEMBAY_SET_SD_VOLTAGE_ID)
> > +
> > +#define ARM_SMCCC_SIP_KEEMBAY_GET_SD_VOLTAGE \
> > + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, \
> > + ARM_SMCCC_SMC_32,\
> > + ARM_SMCCC_OWNER_SIP, \
> > + KEEMBAY_GET_SD_VOLTAGE_ID)
> > +
> > +#define KEEMBAY_REG_NUM_CONSUMERS 2
> > +
> > +struct keembay_reg_supply {
> > + struct regulator *consumer;
> > +};
> > +
> > +#if IS_ENABLED(CONFIG_HAVE_ARM_SMCCC_DISCOVERY)
> > +/*
> > + * Voltage applied on the IO Rail is controlled from the Always On
> > Register using specific
> > + * bits in AON_CGF1 register. This is a secure register. Keem Bay SOC
> > cannot exposed this
> > + * register address to the outside world.
> > + */
> > +static inline int keembay_set_io_rail_supplied_voltage(int volt)
> > +{
> > + struct arm_smccc_res res;
> > +
> > + arm_smccc_1_1_invoke(ARM_SMCCC_SIP_KEEMBAY_SET_SD_VOLTAGE, volt, );
>
> There is a SCMI voltage domain protocol intended for just this use case
> of controlling regulators managed by the firmware, why are you not using
> that for these systems? See drivers/firmware/arm_scmi/voltage.c.
>
Indeed. Please switch to using the new voltage protocol added for this without
any extra code. You just need to wire up DT for this.
Just for curiosity, where is SCMI platform firmware implemented ? On Cortex-A,
secure side or external processor. Does this platform run TF-A ?
--
Regards,
Sudeep
Re: [PATCH] firmware: arm_scmi: fix call site of notifications exit
On Tue, Jan 12, 2021 at 07:13:26PM +, Cristian Marussi wrote: > Call scmi_notification_exit() only when SCMI platform driver instance has > been really successfully removed. > Doesn't this deserve Fixes tag BTW ? Just send the tag if required, I can fold it in. -- Regards, Sudeep
Re: [PATCH] MAINTAINERS: Update ARM SCMI entry
On Tue, 5 Jan 2021 15:19:45 +, Sudeep Holla wrote: > Cristian is actively developing new features and more involved than me. > So add Cristian as a designated reviewer. Also add the newly added scmi > regulator driver to the list. Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/1] MAINTAINERS: Update ARM SCMI entry https://git.kernel.org/sudeep.holla/c/6054d97ab5 -- Regards, Sudeep
Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Tue, 22 Dec 2020 09:56:01 -0500, Jim Quinlan wrote: > v4 -- s/message-serviced/a2p/ in the bindings commit message. >-- Changed author/s-o-b/committer email address as my company is now > appending boilerplate text to all outgoing emails. > > v3 -- Changed interrupt name from "message-serviced" to "a2p" (RobH) > > v2 -- Correct commit message, s/msg/message/, and remove extra WS on > "dt-bindings" commit (Sudeep) >-- Change interrupt name to "message-serviced", move irq assignent to end > of function. (Sudeep) > > [...] Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/2] dt-bindings: arm: Add optional interrupt to smc/hvc SCMI transport https://git.kernel.org/sudeep.holla/c/99a064fb3a [2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt https://git.kernel.org/sudeep.holla/c/dd820ee21d -- Regards, Sudeep
Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Tue, Jan 05, 2021 at 01:32:49PM -0500, Jim Quinlan wrote: [...] > > I don't think that is the case; the bottom routine, > do_wait_for_common(), decrements the x->done after a completion (which > does an increment). Regardless, I think it is prudent to add the > reinit patch you've provided below. > Ah right, but I will add that. > BTW, one thing I could have done was incorporate the timeout value but > I thought that since this is the "fast" call we shouldn't be timing > out at all. > Agreed, we can add it later. However it is not related to fast call, it is more for protection against failure of delivery of interrupt from the platform or any firmware responsible IMO. > > This electronic communication and the information and any files > > transmitted with it, or attached to it, are confidential and are intended > > solely for the use of the individual or entity to whom it is addressed and > > may contain information that is confidential, legally privileged, > > protected by privacy laws, or otherwise restricted from disclosure to > > anyone else. If you are not the intended recipient or the person > > responsible for delivering the e-mail to the intended recipient, you are > > hereby notified that any use, copying, distributing, dissemination, > > forwarding, printing, or copying of this e-mail is strictly prohibited. If > > you received this e-mail in error, please return the e-mail to the sender, > > delete it from your computer, and destroy any printed copy of it. I am assuming this was unintentional and ignoring. If not disregard my response. Otherwise you may need to fix your mail server. -- Regards, Sudeep
Re: [PATCH v4 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Tue, Dec 22, 2020 at 07:37:22PM -0800, Florian Fainelli wrote: > > > On 12/22/2020 6:56 AM, Jim Quinlan wrote: > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI > > message to be indicated by an interrupt rather than the return of the smc > > call. This accommodates the existing behavior of the BrcmSTB SCMI > > "platform" whose SW is already out in the field and cannot be changed. > > > > Signed-off-by: Jim Quinlan > > This looks good to me, just one question below: > > [snip] > > > @@ -111,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info > > *cinfo, > > shmem_tx_prepare(scmi_info->shmem, xfer); > > > > arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, ); > > + if (scmi_info->irq) > > + wait_for_completion(_info->tx_complete); > > Do we need this to have a preceding call to reinit_completion()? It does > not look like this is going to make any practical difference but there > are drivers doing that for correctness. Why do you think that might not cause any issue ? After first message is completed and ISR is executed, the completion flag remains done for ever. So practically 2nd message onwards won't block in wait_for_completion which means return from smc/hvc is actually completion too which is clearly wrong or am I missing something ? Jim, please confirm either way. If you agree I can add the below snippet, no need to repost. Regards, Sudeep -- diff --git i/drivers/firmware/arm_scmi/smc.c w/drivers/firmware/arm_scmi/smc.c index fd41d436e34b..86eac0831d3c 100644 --- i/drivers/firmware/arm_scmi/smc.c +++ w/drivers/firmware/arm_scmi/smc.c @@ -144,6 +145,8 @@ static int smc_send_message(struct scmi_chan_info *cinfo, shmem_tx_prepare(scmi_info->shmem, xfer); + if (scmi_info->irq) + reinit_completion(_info->tx_complete); arm_smccc_1_1_invoke(scmi_info->func_id, 0, 0, 0, 0, 0, 0, 0, ); if (scmi_info->irq) wait_for_completion(_info->tx_complete);
[PATCH] MAINTAINERS: Update ARM SCMI entry
Cristian is actively developing new features and more involved than me. So add Cristian as a designated reviewer. Also add the newly added scmi regulator driver to the list. Cc: Cristian Marussi Signed-off-by: Sudeep Holla --- MAINTAINERS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 6eff4f720c72..34e09b55f806 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -17208,6 +17208,7 @@ F: drivers/mfd/syscon.c SYSTEM CONTROL & POWER/MANAGEMENT INTERFACE (SCPI/SCMI) Message Protocol drivers M: Sudeep Holla +R: Cristian Marussi L: linux-arm-ker...@lists.infradead.org S: Maintained F: Documentation/devicetree/bindings/arm/arm,sc[mp]i.txt @@ -17215,6 +17216,7 @@ F: drivers/clk/clk-sc[mp]i.c F: drivers/cpufreq/sc[mp]i-cpufreq.c F: drivers/firmware/arm_scmi/ F: drivers/firmware/arm_scpi.c +F: drivers/regulator/scmi-regulator.c F: drivers/reset/reset-scmi.c F: include/linux/sc[mp]i_protocol.h F: include/trace/events/scmi.h -- 2.25.1
Re: [PATCH v4 0/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Mon, Jan 04, 2021 at 09:57:31AM -0500, Jim Quinlan wrote: > Hi Sudeep, > > Since RobH has reviewed patch 1/.2 and Florian has acked it, can you > please accept patches 1 & 2? > Sure, will start queuing patches later this week, will let you know when I apply. Thanks. -- Regards, Sudeep
Re: [PATCH v4 2/5] firmware: smccc: Introduce SMCCC TRNG framework
On Fri, Dec 11, 2020 at 04:00:02PM +, Andre Przywara wrote: > The ARM DEN0098 document describe an SMCCC based firmware service to > deliver hardware generated random numbers. Its existence is advertised > according to the SMCCC v1.1 specification. > > Add a (dummy) call to probe functions implemented in each architecture > (ARM and arm64), to determine the existence of this interface. > For now this return false, but this will be overwritten by each > architecture's support patch. > > Signed-off-by: Andre Przywara > Reviewed-by: Linus Walleij Reviewed-by: Sudeep Holla -- Regards, Sudeep
Re: [PATCH v4 1/5] firmware: smccc: Add SMCCC TRNG function call IDs
On Fri, Dec 11, 2020 at 04:00:01PM +, Andre Przywara wrote: > From: Ard Biesheuvel > > The ARM architected TRNG firmware interface, described in ARM spec > DEN0098, define an ARM SMCCC based interface to a true random number > generator, provided by firmware. > > Add the definitions of the SMCCC functions as defined by the spec. > > Signed-off-by: Ard Biesheuvel > Signed-off-by: Andre Przywara > Reviewed-by: Linus Walleij Reviewed-by: Sudeep Holla -- Regards, Sudeep
[PATCH v2] drivers: soc: atmel: Avoid calling at91_soc_init on non AT91 SoCs
Since at91_soc_init is called unconditionally from atmel_soc_device_init,
we get the following warning on all non AT91 SoCs:
" AT91: Could not find identification node"
Fix the same by filtering with allowed AT91 SoC list.
Cc: Nicolas Ferre
Cc: Alexandre Belloni
Cc: Ludovic Desroches
Signed-off-by: Sudeep Holla
---
drivers/soc/atmel/soc.c | 12
1 file changed, 12 insertions(+)
v1->v2:
- Updated the allowed list as suggested by Alexandre
diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index 55a1f57a4d8c..2dc86728b132 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -265,8 +265,20 @@ struct soc_device * __init at91_soc_init(const struct
at91_soc *socs)
return soc_dev;
}
+static const struct of_device_id at91_soc_allowed_list[] __initconst = {
+ { .compatible = "atmel,at91rm9200", },
+ { .compatible = "atmel,at91sam9", },
+ { .compatible = "atmel,sama5", },
+ { .compatible = "atmel,samv7", }
+};
+
static int __init atmel_soc_device_init(void)
{
+ struct device_node *np = of_find_node_by_path("/");
+
+ if (!of_match_node(at91_soc_allowed_list, np))
+ return 0;
+
at91_soc_init(socs);
return 0;
--
2.25.1
Re: [PATCH] drivers: soc: atmel: Avoid calling at91_soc_init on non AT91 SoCs
On Fri, Dec 11, 2020 at 12:58:00PM +0100, Alexandre Belloni wrote:
> On 11/12/2020 11:50:55+0000, Sudeep Holla wrote:
> > On Fri, Dec 11, 2020 at 12:45:15PM +0100, Alexandre Belloni wrote:
> > > Hello,
> > >
> > > On 11/12/2020 10:31:43+, Sudeep Holla wrote:
> > > > Since at91_soc_init is called unconditionally from
> > > > atmel_soc_device_init,
> > > > we get the following warning on all non AT91 SoCs:
> > > > " AT91: Could not find identification node"
> > > >
> > > > Fix the same by filtering with allowed AT91 SoC list.
> > > >
> > > > Cc: Nicolas Ferre
> > > > Cc: Alexandre Belloni
> > > > Cc: Ludovic Desroches
> > > > Signed-off-by: Sudeep Holla
> > > > ---
> > > > drivers/soc/atmel/soc.c | 11 +++
> > > > 1 file changed, 11 insertions(+)
> > > >
> > > > diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
> > > > index c4472b68b7c2..ba9fc07cd91c 100644
> > > > --- a/drivers/soc/atmel/soc.c
> > > > +++ b/drivers/soc/atmel/soc.c
> > > > @@ -271,8 +271,19 @@ struct soc_device * __init at91_soc_init(const
> > > > struct at91_soc *socs)
> > > > return soc_dev;
> > > > }
> > > >
> > > > +static const struct of_device_id at91_soc_allowed_list[] __initconst =
> > > > {
> > > > + { .compatible = "atmel,at91rm9200", },
> > > > + { .compatible = "atmel,at91sam9260", },
> > > > + { .compatible = "atmel,sama5d2", },
> > >
> > > This is a very small subset of the supported SoCs. a proper list would
> > > be:
> > >
> > > atmel,at91rm9200
> > > atmel,at91sam9
> > > atmel,sama5
> > > atmel,samv7
> > >
> >
> > Sure I can update it but the existing functions
> > at91_get_cidr_exid_from_chipid
> > and at91_get_cidr_exid_from_dbgu check for following 3 compatibles and bail
> > out if not found:
> > "atmel,at91rm9200-dbgu"
> > "atmel,at91sam9260-dbgu"
> > "atmel,sama5d2-chipid"
> >
> > Quick check on DTS upstream suggested only 3 platforms, hence the choice.
> >
>
> No, atmel,at91sam9260-dbgu is used on most platforms:
> $ git grep atmel,at91sam9260-dbgu arch/arm/boot/dts/
> arch/arm/boot/dts/at91sam9260.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9261.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9263.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9g45.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9n12.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9rl.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/at91sam9x5.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/sam9x60.dtsi: compatible =
> "microchip,sam9x60-dbgu", "microchip,sam9x60-usart",
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/sama5d3.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
> arch/arm/boot/dts/sama5d4.dtsi: compatible =
> "atmel,at91sam9260-dbgu", "atmel,at91sam9260-usart";
>
Ah, I must have messed up my grep then for sure. Also not familiar with
AT91 series of platforms. I will respin with the list you suggested. Thanks!
--
Regards,
Sudeep
Re: [PATCH] drivers: soc: atmel: Avoid calling at91_soc_init on non AT91 SoCs
On Fri, Dec 11, 2020 at 12:45:15PM +0100, Alexandre Belloni wrote:
> Hello,
>
> On 11/12/2020 10:31:43+0000, Sudeep Holla wrote:
> > Since at91_soc_init is called unconditionally from atmel_soc_device_init,
> > we get the following warning on all non AT91 SoCs:
> > " AT91: Could not find identification node"
> >
> > Fix the same by filtering with allowed AT91 SoC list.
> >
> > Cc: Nicolas Ferre
> > Cc: Alexandre Belloni
> > Cc: Ludovic Desroches
> > Signed-off-by: Sudeep Holla
> > ---
> > drivers/soc/atmel/soc.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
> > index c4472b68b7c2..ba9fc07cd91c 100644
> > --- a/drivers/soc/atmel/soc.c
> > +++ b/drivers/soc/atmel/soc.c
> > @@ -271,8 +271,19 @@ struct soc_device * __init at91_soc_init(const struct
> > at91_soc *socs)
> > return soc_dev;
> > }
> >
> > +static const struct of_device_id at91_soc_allowed_list[] __initconst = {
> > + { .compatible = "atmel,at91rm9200", },
> > + { .compatible = "atmel,at91sam9260", },
> > + { .compatible = "atmel,sama5d2", },
>
> This is a very small subset of the supported SoCs. a proper list would
> be:
>
> atmel,at91rm9200
> atmel,at91sam9
> atmel,sama5
> atmel,samv7
>
Sure I can update it but the existing functions at91_get_cidr_exid_from_chipid
and at91_get_cidr_exid_from_dbgu check for following 3 compatibles and bail
out if not found:
"atmel,at91rm9200-dbgu"
"atmel,at91sam9260-dbgu"
"atmel,sama5d2-chipid"
Quick check on DTS upstream suggested only 3 platforms, hence the choice.
--
Regards,
Sudeep
[PATCH] drivers: soc: atmel: Avoid calling at91_soc_init on non AT91 SoCs
Since at91_soc_init is called unconditionally from atmel_soc_device_init,
we get the following warning on all non AT91 SoCs:
" AT91: Could not find identification node"
Fix the same by filtering with allowed AT91 SoC list.
Cc: Nicolas Ferre
Cc: Alexandre Belloni
Cc: Ludovic Desroches
Signed-off-by: Sudeep Holla
---
drivers/soc/atmel/soc.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/drivers/soc/atmel/soc.c b/drivers/soc/atmel/soc.c
index c4472b68b7c2..ba9fc07cd91c 100644
--- a/drivers/soc/atmel/soc.c
+++ b/drivers/soc/atmel/soc.c
@@ -271,8 +271,19 @@ struct soc_device * __init at91_soc_init(const struct
at91_soc *socs)
return soc_dev;
}
+static const struct of_device_id at91_soc_allowed_list[] __initconst = {
+ { .compatible = "atmel,at91rm9200", },
+ { .compatible = "atmel,at91sam9260", },
+ { .compatible = "atmel,sama5d2", },
+};
+
static int __init atmel_soc_device_init(void)
{
+ struct device_node *np = of_find_node_by_path("/");
+
+ if (!of_match_node(at91_soc_allowed_list, np))
+ return 0;
+
at91_soc_init(socs);
return 0;
--
2.25.1
Re: [PATCH] arm64: topology: Drop the useless update to per-cpu cycles
On Thu, Dec 10, 2020 at 11:17:40AM +0530, Viresh Kumar wrote: > The previous call to update_freq_counters_refs() has already updated the > per-cpu variables, don't overwrite them with the same value again. > Good catch! Reviewed-by: Sudeep Holla -- Regards, Sudeep
Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM
On Wed, Dec 09, 2020 at 11:15:02AM +0530, Viresh Kumar wrote: > On 08-12-20, 11:20, Sudeep Holla wrote: > > It is because of per-CPU vs per domain drama here. Imagine a system with > > 4 CPUs which the firmware puts in individual domains while they all are > > in the same perf domain and hence OPP is marked shared in DT. > > > > Since this probe gets called for all the cpus, we need to skip adding > > OPPs for the last 3(add only for 1st one and mark others as shared). > > Okay and this wasn't happening before this series because the firmware > was only returning the current CPU from scmi_get_sharing_cpus() ? > > Is this driver also used for the cases where we have multiple CPUs in > a policy ? Otherwise we won't be required to call > dev_pm_opp_set_sharing_cpus(). > > So I assume that we want to support both the cases here ? > Yes indeed, completely depends on what granularity firmware provides the performance control. It could be individual CPUs, could be pair of CPUs (or all the threads in the core) or subset of CPUs in the performance domain. The subset could be full set. > > If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate > > OPP as we would have already marked it as shared table with the first cpu. > > Am I missing anything ? I suggested this as Nicola saw OPP duplicate > > warnings when he was hacking up this patch. > > The common stuff (for all the CPUs) is better moved to probe() in this > case, instead of the ->init() callback. Otherwise it will always be > messy. You can initialize the OPP and cpufreq tables in probe() > itself, save the pointer somewhere and then just use it here in > ->init(). > > Also do EM registration from there. > Makes sense. -- Regards, Sudeep
Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM
On Tue, Dec 08, 2020 at 11:34:36AM +, Lukasz Luba wrote:
>
>
> On 12/8/20 11:20 AM, Sudeep Holla wrote:
> > On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
> > > On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > > > On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > > > > On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > > > > > nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > > > > > if (nr_opp <= 0) {
> > > > > > - dev_dbg(cpu_dev, "OPP table is not ready, deferring
> > > > > > probe\n");
> > > > > > - ret = -EPROBE_DEFER;
> > > > > > - goto out_free_opp;
> > > > > > + ret = handle->perf_ops->device_opps_add(handle,
> > > > > > cpu_dev);
> > > > > > + if (ret) {
> > > > > > + dev_warn(cpu_dev, "failed to add opps to the
> > > > > > device\n");
> > > > > > + goto out_free_cpumask;
> > > > > > + }
> > > > > > +
> > > > > > + ret = dev_pm_opp_set_sharing_cpus(cpu_dev,
> > > > > > opp_shared_cpus);
> > > > > > + if (ret) {
> > > > > > + dev_err(cpu_dev, "%s: failed to mark OPPs as
> > > > > > shared: %d\n",
> > > > > > + __func__, ret);
> > > > > > + goto out_free_cpumask;
> > > > > > + }
> > > > > > +
> > > > >
> > > > > Why do we need to call above two after calling
> > > > > dev_pm_opp_get_opp_count() ?
> > > >
> > > > Sorry, I am not sure to understand your question here. If there are no
> > > > opps for
> > > > a device we want to add them to it
> > >
> > > Earlier we used to call handle->perf_ops->device_opps_add() and
> > > dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(),
> > > why is
> > > the order changed now ?
> > >
> > >
> > > I am not sure why they would be duplicated in your case. I though
> > > device_opps_add() is responsible for dynamically adding the OPPs here.
> > >
> >
> > It is because of per-CPU vs per domain drama here. Imagine a system with
> > 4 CPUs which the firmware puts in individual domains while they all are
> > in the same perf domain and hence OPP is marked shared in DT.
> >
> > Since this probe gets called for all the cpus, we need to skip adding
> > OPPs for the last 3(add only for 1st one and mark others as shared).
> > If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
> > OPP as we would have already marked it as shared table with the first cpu.
> > Am I missing anything ? I suggested this as Nicola saw OPP duplicate
> > warnings when he was hacking up this patch.
> >
> > > > otherwise no need as they would be duplicated.
> > > > > And we don't check the return value of
> > > > > the below call anymore, moreover we have to call it twice now.
> >
> > Yes, that looks wrong, we need to add the check for non zero values, but
> >
> >
> > > >
> > > > This second get_opp_count is required such that we register em with the
> > > > correct
> > > > opp number after having added them. Without this the opp_count would
> > > > not be correct.
> > >
> >
> > ... I have a question here. Why do you need to call
> >
> > em_dev_register_perf_domain(cpu_dev, nr_opp, _cb, opp_shared_cpus..)
> >
> > on each CPU ? Why can't that be done once for unique opp_shared_cpus ?
>
> It just have to be called once, for one CPU from the mask. Otherwise for
> the next CPUs you should see error:
> "EM: exists for CPU%d"
OK cool, at least it is designed and expected to be used like I thought.
Ah, I might have seen those, but never thought it was error message
> It can happen that this print is not seen when the get_cpu_device(cpu)
> failed, but that would lead to investigation why CPU devices are not
> there yet.
>
> Nicola: have you seen that print?
>
I assume you must see that and you need to pull this inside if condition
to do this once for each performance domain.
--
Regards,
Sudeep
Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM
On Tue, Dec 08, 2020 at 04:31:48PM +0530, Viresh Kumar wrote:
> On 08-12-20, 10:58, Nicola Mazzucato wrote:
> >
> >
> > On 12/8/20 7:26 AM, Viresh Kumar wrote:
> > > On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > >> On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > >>> On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > if (nr_opp <= 0) {
> > - dev_dbg(cpu_dev, "OPP table is not ready, deferring
> > probe\n");
> > - ret = -EPROBE_DEFER;
> > - goto out_free_opp;
> > + ret = handle->perf_ops->device_opps_add(handle,
> > cpu_dev);
> > + if (ret) {
> > + dev_warn(cpu_dev, "failed to add opps to the
> > device\n");
> > + goto out_free_cpumask;
> > + }
> > +
> > + ret = dev_pm_opp_set_sharing_cpus(cpu_dev,
> > opp_shared_cpus);
> > + if (ret) {
> > + dev_err(cpu_dev, "%s: failed to mark OPPs as
> > shared: %d\n",
> > + __func__, ret);
> > + goto out_free_cpumask;
> > + }
> > +
> > >>>
> > >>> Why do we need to call above two after calling
> > >>> dev_pm_opp_get_opp_count() ?
> > >>
> > >> Sorry, I am not sure to understand your question here. If there are no
> > >> opps for
> > >> a device we want to add them to it
> > >
> > > Earlier we used to call handle->perf_ops->device_opps_add() and
> > > dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(),
> > > why is
> > > the order changed now ?
> >
> > True. The order has changed to take into account the fact that when we have
> > per-cpu + opp-shared, we don't need to add opps for devices which already
> > have them.
>
> The opp-shared thing is mostly a dummy thing to get you some information here.
> What else has changed here ? I still don't understand why the OPPs would get
> added and so the duplicate OPPs messages. Does this already happen ?
>
Yes, details in my earlier response.
--
Regards,
Sudeep
Re: [PATCH v4 3/4] scmi-cpufreq: get opp_shared_cpus from opp-v2 for EM
On Tue, Dec 08, 2020 at 12:56:11PM +0530, Viresh Kumar wrote:
> On 08-12-20, 07:22, Nicola Mazzucato wrote:
> > On 12/8/20 5:50 AM, Viresh Kumar wrote:
> > > On 02-12-20, 17:23, Nicola Mazzucato wrote:
> > >> nr_opp = dev_pm_opp_get_opp_count(cpu_dev);
> > >> if (nr_opp <= 0) {
> > >> -dev_dbg(cpu_dev, "OPP table is not ready, deferring
> > >> probe\n");
> > >> -ret = -EPROBE_DEFER;
> > >> -goto out_free_opp;
> > >> +ret = handle->perf_ops->device_opps_add(handle,
> > >> cpu_dev);
> > >> +if (ret) {
> > >> +dev_warn(cpu_dev, "failed to add opps to the
> > >> device\n");
> > >> +goto out_free_cpumask;
> > >> +}
> > >> +
> > >> +ret = dev_pm_opp_set_sharing_cpus(cpu_dev,
> > >> opp_shared_cpus);
> > >> +if (ret) {
> > >> +dev_err(cpu_dev, "%s: failed to mark OPPs as
> > >> shared: %d\n",
> > >> +__func__, ret);
> > >> +goto out_free_cpumask;
> > >> +}
> > >> +
> > >
> > > Why do we need to call above two after calling
> > > dev_pm_opp_get_opp_count() ?
> >
> > Sorry, I am not sure to understand your question here. If there are no opps
> > for
> > a device we want to add them to it
>
> Earlier we used to call handle->perf_ops->device_opps_add() and
> dev_pm_opp_set_sharing_cpus() before calling dev_pm_opp_get_opp_count(), why
> is
> the order changed now ?
>
>
> I am not sure why they would be duplicated in your case. I though
> device_opps_add() is responsible for dynamically adding the OPPs here.
>
It is because of per-CPU vs per domain drama here. Imagine a system with
4 CPUs which the firmware puts in individual domains while they all are
in the same perf domain and hence OPP is marked shared in DT.
Since this probe gets called for all the cpus, we need to skip adding
OPPs for the last 3(add only for 1st one and mark others as shared).
If we attempt to add OPPs on second cpu probe, it *will* shout as duplicate
OPP as we would have already marked it as shared table with the first cpu.
Am I missing anything ? I suggested this as Nicola saw OPP duplicate
warnings when he was hacking up this patch.
> > otherwise no need as they would be duplicated.
> > > And we don't check the return value of
> > > the below call anymore, moreover we have to call it twice now.
Yes, that looks wrong, we need to add the check for non zero values, but
> >
> > This second get_opp_count is required such that we register em with the
> > correct
> > opp number after having added them. Without this the opp_count would not be
> > correct.
>
... I have a question here. Why do you need to call
em_dev_register_perf_domain(cpu_dev, nr_opp, _cb, opp_shared_cpus..)
on each CPU ? Why can't that be done once for unique opp_shared_cpus ?
The whole drama of per-CPU vs perf domain is to have energy model and
if feeding it opp_shared_cpus once is not sufficient, then something is
wrong or simply duplicated or just not necessary IMO.
> What if the count is still 0 ? What about deferred probe we were doing
> earlier ?
OK, you made me think with that question. I think the check was original
added for deferred probe but then scmi core was changed to add the cpufreq
device only after everything needed is ready. So the condition must never
occur now.
--
Regards,
Sudeep
Re: [PATCH] cpufreq: scmi: add COMMON_CLK dependency
On Fri, Dec 04, 2020 at 12:17:46AM +0100, Arnd Bergmann wrote:
> From: Arnd Bergmann
>
> Wtihout CONFIG_COMMON_CLK, the scmi driver fails to link:
>
> arm-linux-gnueabi-ld: drivers/cpufreq/scmi-cpufreq.o: in function
> `scmi_cpufreq_probe':
> scmi-cpufreq.c:(.text+0x20c): undefined reference to
> `devm_of_clk_add_hw_provider'
> arm-linux-gnueabi-ld: scmi-cpufreq.c:(.text+0x22c): undefined reference to
> `of_clk_hw_simple_get'
>
> Add a Kconfig dependency for it.
>
There is a fix already upstream in later -rc(rc6 IIRC), I assume you are
seeing this prior to that.
Commit f943849f7206 ("cpufreq: scmi: Fix build for !CONFIG_COMMON_CLK")
Since the only dependency on CONFIG_COMMON_CLK is to satisfy OPP adding
dummy clock provider, I avoided adding dependency on CLK for this driver
as this works fine for !CONFIG_COMMON_CLK.
--
Regards,
Sudeep
P.S: There are 2 copies of this patch, I chose to reply on this, other
one is @[1]
[1] https://lore.kernel.org/lkml/20201203225550.1478195-1-a...@kernel.org
Re: [RFC PATCH v2 1/2] topology: Represent clusters of CPUs within a die.
On Tue, Dec 01, 2020 at 04:03:46PM +, Valentin Schneider wrote: > > On 01/12/20 02:59, Barry Song wrote: > > Currently the ID provided is the offset of the Processor > > Hierarchy Nodes Structure within PPTT. Whilst this is unique > > it is not terribly elegant so alternative suggestions welcome. > > I had already mentioned that you need to fix the firmware/PPTT on your platform. If you fill only mandatory fields, then yes this is optional and we resort to use offset as unique number in the kernel. > > Skimming through the spec, this sounds like something the ID structure > (Type 2) could be used for. However in v1 Jonathan and Sudeep talked about > UID's / DSDT, any news on that? > FYI, type 2 is for SoC identification which is being deprecated(still need to check if that progressed and made to the official release yet) in favour of Arm SMCCC v1.2 SOC_ID. Anyways it is irrelevant in this context. They need to use UIDs and mark the corresponding flag as valid for OSPM/kernel to use it. > > Note that arm64 / ACPI does not provide any means of identifying > > a die level in the topology but that may be unrelate to the cluster > > level. > > May need spec extension if there are no ways to identify the same. -- Regards, Sudeep
[PATCH] mailbox: arm_mhu_db: Fix mhu_db_shutdown by replacing kfree with devm_kfree
The mhu_db_channel info is allocated per channel using devm_kzalloc from
mhu_db_mbox_xlate which gets called from mbox_request_channel. However
we are releasing the allocated mhu_db_channel info using plain kfree from
mhu_db_shutdown which is called from mbox_free_channel.
This leads to random crashes when the channel is freed like below one:
Unable to handle kernel paging request at virtual address 00840008
[00840008] address between user and kernel address ranges
Internal error: Oops: 9644 [#1] PREEMPT SMP
Modules linked in: scmi_module(-)
CPU: 1 PID: 2212 Comm: rmmod Not tainted 5.10.0-rc5 #31
Hardware name: ARM LTD ARM Juno Development Platform/ARM Juno
Development Platform, BIOS EDK II Nov 19 2020
pstate: 2085 (nzCv daIf -PAN -UAO -TCO BTYPE=--)
pc : release_nodes+0x74/0x230
lr : devres_release_all+0x40/0x68
Call trace:
release_nodes+0x74/0x230
devres_release_all+0x40/0x68
device_release_driver_internal+0x12c/0x1f8
driver_detach+0x58/0xe8
bus_remove_driver+0x64/0xe0
driver_unregister+0x38/0x68
platform_driver_unregister+0x1c/0x28
scmi_driver_exit+0x38/0x44 [scmi_module]
__arm64_sys_delete_module+0x188/0x260
el0_svc_common.constprop.0+0x80/0x1a8
do_el0_svc+0x2c/0x98
el0_sync_handler+0x160/0x168
el0_sync+0x174/0x180
Code: 140d eb07009f 54000460 f9400486 (f90004a6)
---[ end trace c55ffd306c140233 ]---
Fix it by replacing kfree with devm_kfree as required.
Fixes: 7002ca237b21 ("mailbox: arm_mhu: Add ARM MHU doorbell driver")
Reported-by: Cristian Marussi
Signed-off-by: Sudeep Holla
---
drivers/mailbox/arm_mhu_db.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mailbox/arm_mhu_db.c b/drivers/mailbox/arm_mhu_db.c
index 275efe4cca0c..8eb66c4ecf5b 100644
--- a/drivers/mailbox/arm_mhu_db.c
+++ b/drivers/mailbox/arm_mhu_db.c
@@ -180,7 +180,7 @@ static void mhu_db_shutdown(struct mbox_chan *chan)
/* Reset channel */
mhu_db_mbox_clear_irq(chan);
- kfree(chan->con_priv);
+ devm_kfree(mbox->dev, chan->con_priv);
chan->con_priv = NULL;
}
--
2.25.1
Re: [PATCH v3 19/23] kvm: arm64: Intercept host's CPU_ON SMCs
On Thu, Nov 26, 2020 at 03:54:17PM +, David Brazdil wrote:
> Add a handler of the CPU_ON PSCI call from host. When invoked, it looks
> up the logical CPU ID corresponding to the provided MPIDR and populates
> the state struct of the target CPU with the provided x0, pc. It then
> calls CPU_ON itself, with an entry point in hyp that initializes EL2
> state before returning ERET to the provided PC in EL1.
>
> There is a simple atomic lock around the boot args struct. If it is
> already locked, CPU_ON will return PENDING_ON error code.
>
> Signed-off-by: David Brazdil
> ---
> arch/arm64/kvm/hyp/nvhe/hyp-init.S | 30
> arch/arm64/kvm/hyp/nvhe/psci-relay.c | 109 +++
> 2 files changed, 139 insertions(+)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> index 98ce40e17b42..ea71f653af55 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> @@ -9,6 +9,7 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -161,6 +162,35 @@ alternative_else_nop_endif
> ret
> SYM_CODE_END(___kvm_hyp_init)
>
> +SYM_CODE_START(__kvm_hyp_cpu_on_entry)
> + msr SPsel, #1 // We want to use SP_EL{1,2}
> +
> + /* Check that the core was booted in EL2. */
> + mrs x1, CurrentEL
> + cmp x1, #CurrentEL_EL2
> + b.eq2f
> +
> + /* The core booted in EL1. KVM cannot be initialized on it. */
> +1: wfe
> + wfi
> + b 1b
> +
> + /* Initialize EL2 CPU state to sane values. */
> +2: mov x29, x0
> + init_el2_state nvhe
> + mov x0, x29
> +
> + /* Enable MMU, set vectors and stack. */
> + bl ___kvm_hyp_init
> +
> + /* Load address of the C handler. */
> + ldr x1, =__kvm_hyp_psci_cpu_entry
> + kimg_hyp_va x1, x2
> +
> + /* Leave idmap. */
> + br x1
> +SYM_CODE_END(__kvm_hyp_cpu_on_entry)
> +
> SYM_CODE_START(__kvm_handle_stub_hvc)
> cmp x0, #HVC_SOFT_RESTART
> b.ne1f
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index 7aa87ab7f5ce..39e507672e6e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -9,12 +9,17 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
>
> #include
>
> +extern char __kvm_hyp_cpu_on_entry[];
> +
> +void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> +
> /* Config options set by the host. */
> u32 __ro_after_init kvm_host_psci_version;
> u32 __ro_after_init kvm_host_psci_function_id[PSCI_FN_MAX];
> @@ -22,6 +27,19 @@ s64 __ro_after_init hyp_physvirt_offset;
>
> #define __hyp_pa(x) ((phys_addr_t)((x)) + hyp_physvirt_offset)
>
> +#define INVALID_CPU_ID UINT_MAX
> +
> +#define CPU_UNLOCKED 0
> +#define CPU_LOCKED 1
> +
> +struct cpu_boot_args {
> + unsigned long pc;
> + unsigned long r0;
> +};
> +
> +static DEFINE_PER_CPU(atomic_t, cpu_on_lock) = ATOMIC_INIT(0);
> +static DEFINE_PER_CPU(struct cpu_boot_args, cpu_on_args);
> +
> static u64 get_psci_func_id(struct kvm_cpu_context *host_ctxt)
> {
> DECLARE_REG(u64, func_id, host_ctxt, 0);
> @@ -78,10 +96,99 @@ static __noreturn unsigned long
> psci_forward_noreturn(struct kvm_cpu_context *ho
> hyp_panic(); /* unreachable */
> }
>
> +static unsigned int find_cpu_id(u64 mpidr)
> +{
> + unsigned int i;
> +
> + /* Reject invalid MPIDRs */
> + if (mpidr & ~MPIDR_HWID_BITMASK)
> + return INVALID_CPU_ID;
> +
> + for (i = 0; i < NR_CPUS; i++) {
I may not have understood the flow correctly, so just asking:
This is just called for secondaries on boot right ? And the cpumasks
are setup by then ? Just trying to see if we can use cpu_possible_mask
instead of running through all 256/1k/4k cpus(ofcourse based on NR_CPUS
config)
--
Regards,
Sudeep
Re: [PATCH v3 16/23] kvm: arm64: Forward safe PSCI SMCs coming from host
On Thu, Nov 26, 2020 at 03:54:14PM +, David Brazdil wrote:
> Forward the following PSCI SMCs issued by host to EL3 as they do not
> require the hypervisor's intervention. This assumes that EL3 correctly
> implements the PSCI specification.
>
> Only function IDs implemented in Linux are included.
>
> Where both 32-bit and 64-bit variants exist, it is assumed that the host
> will always use the 64-bit variant.
>
> * SMCs that only return information about the system
>* PSCI_VERSION- PSCI version implemented by EL3
>* PSCI_FEATURES - optional features supported by EL3
>* AFFINITY_INFO - power state of core/cluster
>* MIGRATE_INFO_TYPE - whether Trusted OS can be migrated
>* MIGRATE_INFO_UP_CPU - resident core of Trusted OS
> * operations which do not affect the hypervisor
>* MIGRATE - migrate Trusted OS to a different core
>* SET_SUSPEND_MODE- toggle OS-initiated mode
> * system shutdown/reset
>* SYSTEM_OFF
>* SYSTEM_RESET
>* SYSTEM_RESET2
>
> Signed-off-by: David Brazdil
> ---
> arch/arm64/kvm/hyp/nvhe/psci-relay.c | 43 +++-
> 1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> index e7091d89f0fc..7aa87ab7f5ce 100644
> --- a/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> +++ b/arch/arm64/kvm/hyp/nvhe/psci-relay.c
> @@ -57,14 +57,51 @@ static bool is_psci_call(u64 func_id)
> }
> }
>
> +static unsigned long psci_call(unsigned long fn, unsigned long arg0,
> +unsigned long arg1, unsigned long arg2)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_1_1_smc(fn, arg0, arg1, arg2, );
> + return res.a0;
> +}
> +
> +static unsigned long psci_forward(struct kvm_cpu_context *host_ctxt)
> +{
> + return psci_call(cpu_reg(host_ctxt, 0), cpu_reg(host_ctxt, 1),
> + cpu_reg(host_ctxt, 2), cpu_reg(host_ctxt, 3));
> +}
> +
> +static __noreturn unsigned long psci_forward_noreturn(struct kvm_cpu_context
> *host_ctxt)
> +{
> + psci_forward(host_ctxt);
> + hyp_panic(); /* unreachable */
> +}
> +
> static unsigned long psci_0_1_handler(u64 func_id, struct kvm_cpu_context
> *host_ctxt)
> {
> - return PSCI_RET_NOT_SUPPORTED;
> + if (func_id == kvm_host_psci_function_id[PSCI_FN_CPU_OFF])
> + return psci_forward(host_ctxt);
> + else if (func_id == kvm_host_psci_function_id[PSCI_FN_MIGRATE])
> + return psci_forward(host_ctxt);
Looks weird or I am not seeing something right ? Same action for both
right ? Can't they be combined ?
--
Regards,
Sudeep
Re: [PATCH v3 06/23] kvm: arm64: Add kvm-arm.protected early kernel parameter
On Thu, Nov 26, 2020 at 03:54:04PM +, David Brazdil wrote: > Add an early parameter that allows users to opt into protected KVM mode > when using the nVHE hypervisor. In this mode, guest state will be kept > private from the host. This will primarily involve enabling stage-2 > address translation for the host, restricting DMA to host memory, and > filtering host SMCs. > > Capability ARM64_PROTECTED_KVM is set if the param is passed, CONFIG_KVM > is enabled and the kernel was not booted with VHE. > > Signed-off-by: David Brazdil > --- > .../admin-guide/kernel-parameters.txt | 5 > arch/arm64/include/asm/cpucaps.h | 3 +- > arch/arm64/include/asm/virt.h | 8 + > arch/arm64/kernel/cpufeature.c| 29 +++ > arch/arm64/kvm/arm.c | 4 ++- > 5 files changed, 47 insertions(+), 2 deletions(-) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt > b/Documentation/admin-guide/kernel-parameters.txt > index 526d65d8573a..06c89975c29c 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -2259,6 +2259,11 @@ > for all guests. > Default is 1 (enabled) if in 64-bit or 32-bit PAE mode. > > + kvm-arm.protected= > + [KVM,ARM] Allow spawning protected guests whose state > + is kept private from the host. Only valid for non-VHE. > + Default is 0 (disabled). > + Sorry for being pedantic. Can we reword this to say valid for !CONFIG_ARM64_VHE ? I read this as valid only for non-VHE hardware, it may be just me, but if you agree please update so that it doesn't give remote idea that it is not valid on VHE enabled hardware. I was trying to run this on the hardware and was trying to understand the details on how to do that. -- Regards, Sudeep
[PATCH] dpaa2-mac: select NET_DEVLINK to fix build
When NET_DEVLINK is not selected, we get the following build error:
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.o: in function
`dpaa2_eth_rx_err':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c:554: undefined reference to
`devlink_trap_report'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_info_get':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:42: undefined
reference to `devlink_info_driver_name_put'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:47: undefined
reference to `devlink_info_version_running_put'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_register':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:199: undefined
reference to `devlink_alloc'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:207: undefined
reference to `devlink_register'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:216: undefined
reference to `devlink_free'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_unregister':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:223: undefined
reference to `devlink_unregister'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:224: undefined
reference to `devlink_free'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_port_add':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:234: undefined
reference to `devlink_port_attrs_set'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:236: undefined
reference to `devlink_port_register'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:240: undefined
reference to `devlink_port_type_eth_set'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_port_del':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:249: undefined
reference to `devlink_port_type_clear'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:250: undefined
reference to `devlink_port_unregister'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_traps_register':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:273: undefined
reference to `devlink_trap_groups_register'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:280: undefined
reference to `devlink_traps_register'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:290: undefined
reference to `devlink_trap_groups_unregister'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.o: in function
`dpaa2_eth_dl_traps_unregister':
drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:303: undefined
reference to `devlink_traps_unregister'
ld: drivers/net/ethernet/freescale/dpaa2/dpaa2-eth-devlink.c:305: undefined
reference to `devlink_trap_groups_unregister'
Commit f6b19b354d50 ("net: devlink: select NET_DEVLINK from drivers")
selected NET_DEVLINK from several drivers and rely on the functions
being there.
Replicate the same for FSL_DPAA2_ETH.
Cc: Ioana Ciornei
Cc: Ioana Radulescu
Cc: David S. Miller
Cc: Jakub Kicinski
Signed-off-by: Sudeep Holla
---
drivers/net/ethernet/freescale/dpaa2/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/ethernet/freescale/dpaa2/Kconfig
b/drivers/net/ethernet/freescale/dpaa2/Kconfig
index cfd369cf4c8c..a86f9ebcf63b 100644
--- a/drivers/net/ethernet/freescale/dpaa2/Kconfig
+++ b/drivers/net/ethernet/freescale/dpaa2/Kconfig
@@ -4,6 +4,7 @@ config FSL_DPAA2_ETH
depends on FSL_MC_BUS && FSL_MC_DPIO
select PHYLINK
select PCS_LYNX
+ select NET_DEVLINK
help
This is the DPAA2 Ethernet driver supporting Freescale SoCs
with DPAA2 (DataPath Acceleration Architecture v2).
--
2.25.1
Re: [PATCH 1/2] firmware: arm_scmi: Add power_scale_mw_get() interface
On Tue, Nov 24, 2020 at 10:43:45AM +, Lukasz Luba wrote: > Add a new interface to the existing perf_ops and export the information > about the power values scale. > > This would be used by the cpufreq driver and Energy Model framework to > set the performance domains scale: milli-Watts or abstract scale. > Looks good to me. I saw this after I sent pull request this afternoon. In case you want to take it via PM tree: Acked-by: Sudeep Holla -- Regards, Sudeep
Re: [PATCH] firmware: arm_scmi: remove residual _le structs naming
On Mon, 23 Nov 2020 16:20:08 +, Cristian Marussi wrote: > For sake of consistency, remove any residual naming based on _le suffixes > in SCMI Sensors Protocol, since little endianity is already assumed across > all of SCMI implementation and, as such, all currently existent names do > not explicitly state their endianness. > > No functional change. Applied to sudeep.holla/linux (for-linux-next), thanks! [1/1] firmware: arm_scmi: Remove residual _le structs naming https://git.kernel.org/sudeep.holla/c/e945927dc7 -- Regards, Sudeep
Re: [PATCH v4 0/6] SCMIv3.0 Sensor Extensions
On Thu, 19 Nov 2020 17:49:00 +, Cristian Marussi wrote:
> this series is meant to add support for the new SCMI Sensor Protocol
> features defined by the upcoming SCMIv3.0 specification, whose BETA
> release is available at [1].
>
> The series is currently based on for-next/scmi [2] on top of:
>
> commit b141fca08207 ("firmware: arm_scmi: Fix missing destroy_workqueue()")
>
> [...]
Applied to sudeep.holla/linux (for-next/scmi), thanks!
[1/6] firmware: arm_scmi: Rework scmi_sensors_protocol_init
https://git.kernel.org/sudeep.holla/c/f25fb6de67
[2/6] firmware: arm_scmi: Add SCMI v3.0 sensors descriptors extensions
https://git.kernel.org/sudeep.holla/c/1fe00b8b42
[3/6] hwmon: (scmi) Update hwmon internal scale data type
https://git.kernel.org/sudeep.holla/c/d7971d57d2
[4/6] firmware: arm_scmi: Add SCMI v3.0 sensors timestamped reads
https://git.kernel.org/sudeep.holla/c/e2083d3673
[5/6] firmware: arm_scmi: Add SCMI v3.0 sensor configuration support
https://git.kernel.org/sudeep.holla/c/7b83c5f410
[6/6] firmware: arm_scmi: Add SCMI v3.0 sensor notifications
https://git.kernel.org/sudeep.holla/c/e3811190ac
--
Regards,
Sudeep
[GIT PULL] firmware: arm_scmi: SCMI voltage domain support for v5.11
Hi Mark, As discussed here is the pull request for SCMI firmware part for regulator support. Please pull ! Regards, Sudeep -->8 The following changes since commit 3cea11cd5e3b00d91caf0b4730194039b45c5891: Linux 5.10-rc2 (2020-11-01 14:43:51 -0800) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/sudeep.holla/linux.git tags/scmi-voltage-5.11 for you to fetch changes up to ec88381936954a146f260a21bf8466ca07e5c71e: firmware: arm_scmi: Add support to enumerated SCMI voltage domain device (2020-11-20 14:55:48 +) SCMI voltage domain management protocol support for v5.11 SCMI v3.0 voltage domain protocol support to discover the voltage levels supported by the domains and to set/get the configuration and voltage level of any given domain. Cristian Marussi (3): dt-bindings: arm: Add support for SCMI Regulators firmware: arm_scmi: Add voltage domain management protocol support firmware: arm_scmi: Add support to enumerated SCMI voltage domain device Documentation/devicetree/bindings/arm/arm,scmi.txt | 43 +++ drivers/firmware/arm_scmi/Makefile | 2 +- drivers/firmware/arm_scmi/common.h | 1 + drivers/firmware/arm_scmi/driver.c | 3 + drivers/firmware/arm_scmi/voltage.c| 380 + include/linux/scmi_protocol.h | 64 6 files changed, 492 insertions(+), 1 deletion(-) create mode 100644 drivers/firmware/arm_scmi/voltage.c
Re: [PATCH v6 0/5] Add support for SCMIv3.0 Voltage Domain Protocol and SCMI-Regulator
On Thu, 19 Nov 2020 19:10:46 +, Cristian Marussi wrote: > this series introduces the support for the new SCMI Voltage Domain Protocol > defined by the upcoming SCMIv3.0 specification, whose BETA release is > available at [1]. > > Afterwards, a new generic SCMI Regulator driver is developed on top of the > new SCMI VD Protocol. > > [...] Applied to sudeep.holla/linux (for-next/scmi-voltage), thanks! I will soon send pull request to Mark Brown so tha he can pick by the regulator driver patches with these as agreed. [4/5] dt-bindings: arm: Add support for SCMI Regulators https://git.kernel.org/sudeep.holla/c/0f80fcec08 [1/5] firmware: arm_scmi: Add voltage domain management protocol support https://git.kernel.org/sudeep.holla/c/2add5cacff [2/5] firmware: arm_scmi: Add support to enumerated SCMI voltage domain device https://git.kernel.org/sudeep.holla/c/ec88381936 -- Regards, Sudeep
Re: [PATCH v4 3/6] hwmon: scmi: update hwmon internal scale data type
On Sat, Nov 21, 2020 at 08:59:03AM -0800, Guenter Roeck wrote: > On Thu, Nov 19, 2020 at 05:49:03PM +, Cristian Marussi wrote: > > Use an int to calculate scale values inside scmi_hwmon_scale() to match > > the updated scale data type in struct scmi_sensor_info. > > > > Cc: linux-hw...@vger.kernel.org > > Cc: Guenter Roeck > > Signed-off-by: Cristian Marussi > > Acked-by: Guenter Roeck > Thanks. > [ Assuming this will be pushed togesther with the other patches in the series > ] > Yes I am planning to take as part of the series. -- Regards, Sudeep
Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Fri, Nov 20, 2020 at 08:27:38AM -0500, Jim Quinlan wrote:
> On Fri, Nov 20, 2020 at 6:14 AM Sudeep Holla wrote:
> >
> > On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> > > On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan
> > > wrote:
> > > >
> > > > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla
> > > > wrote:
> > > > >
> > > > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > > > Hi, these are fast calls. Regards, Jim
> > > > > >
> > > > > >
> > > > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla
> > > > > > wrote:
> > > > > > >
> > > > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > > > > > > The SMC/HVC SCMI transport is modified to allow the completion
> > > > > > > > of an SCMI
> > > > > > > > message to be indicated by an interrupt rather than the return
> > > > > > > > of the smc
> > > > > > > > call. This accommodates the existing behavior of the BrcmSTB
> > > > > > > > SCMI
> > > > > > > > "platform" whose SW is already out in the field and cannot be
> > > > > > > > changed.
> > > > > > > >
> > > > > > >
> > > > > > > Sorry for missing to check with you earlier. Are these not fast
> > > > > > > smc calls ?
> > > > > > > Can we check the SMC Function IDs for the same and expect IRQ to
> > > > > > > be present
> > > > > > > if they are not fast calls ?
> > > > > > Hi, if I understand you correctly you want to do something like
> > > > > > this:
> > > > > >
> > > > > > if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > > > > /* look for irq and request it */
> > > > > > }
> > > > > >
> > > > >
> > > > > Yes.
> > > > >
> > > > > > But we do use fast calls.
> > > > >
> > > > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > > > completion ?
> > > > >
> > > > > Is it because you offload it to some other microprocessor and don't
> > > > > continue execution on secure side in whcih case you can afford fast
> > > > > call ?
> > > Hi Sudeep,
> > >
> >
> > Thanks for the details. Unfortunately more questions:
> >
> > > Here is my understanding: Some SMC calls may take a few longer to
> > > complete than others. The longer ones tie up the CPU core that is
> > > handling the SMC call, and so nothing can be scheduled on that
> > > specific core.
> >
> > So far good.
> >
> > > Unfortunately, we have a real-time OS that runs
> > > sporadically on one specific core and if that happens to be the same
> > > core that is handling the SMC, the RTOS will miss its deadline. So we
> > > need to have the SMC return immediately and use an SGI for task
> > > completion.
> > >
> >
> > So it sounds more like it can't be fast call then.
> Hi Sudeep,
>
> To be honest, I'm not sure what the big difference between fast and
> slow SMC calls are other than the latter is "yielding" and
> interruptible. We cannot tolerate them being interruptible.
>
OK
> >
> > Does that me, it will always return early and send SGI when the request
> > is complete ?
> Most calls send the SGI and return immediately. The ones that take
> longer return from the SMC and send the SGI when the operation is
> completed.
That's relief.
> >
> > 1. If yes, what happens if there are multiple requests in parallel and
> >second one completes before the first. Can we handle that with this
> >patch set. Of will the second request fails until the first one is
> >complete ? It extends to number of cpus in the system worst case.
>
> With SCMI we only have one message pending at a time; perhaps I do
> not understand your question. Having the SMC return is mostly a no-op
> as far as the SCMI driver is concerned.
>
Yes we have lock, I forgot. There are requirements to make the smc atomic
by some vendors, was thinking about that and forgot about the lock and
how what I explained can never happen. Thanks for the patience.
If you ping and get Rob's ack on DT, I can take this patch along with
DT bindings for now as is. We can always enhance if required.
> Our SCMI messages cannot fail. When we do have timeouts it indicates
> that something is wrong and needs to be fixed.
>
Good to know.
--
Regards,
Sudeep
Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Thu, Nov 19, 2020 at 01:34:18PM -0500, Jim Quinlan wrote:
> On Fri, Nov 13, 2020 at 10:12 AM Jim Quinlan
> wrote:
> >
> > On Fri, Nov 13, 2020 at 9:36 AM Sudeep Holla wrote:
> > >
> > > On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> > > > Hi, these are fast calls. Regards, Jim
> > > >
> > > >
> > > > On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla
> > > > wrote:
> > > > >
> > > > > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > > > > The SMC/HVC SCMI transport is modified to allow the completion of
> > > > > > an SCMI
> > > > > > message to be indicated by an interrupt rather than the return of
> > > > > > the smc
> > > > > > call. This accommodates the existing behavior of the BrcmSTB SCMI
> > > > > > "platform" whose SW is already out in the field and cannot be
> > > > > > changed.
> > > > > >
> > > > >
> > > > > Sorry for missing to check with you earlier. Are these not fast smc
> > > > > calls ?
> > > > > Can we check the SMC Function IDs for the same and expect IRQ to be
> > > > > present
> > > > > if they are not fast calls ?
> > > > Hi, if I understand you correctly you want to do something like this:
> > > >
> > > > if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> > > > /* look for irq and request it */
> > > > }
> > > >
> > >
> > > Yes.
> > >
> > > > But we do use fast calls.
> > >
> > > What was the rationale for retaining fast SMC calls but use IRQ for Tx
> > > completion ?
> > >
> > > Is it because you offload it to some other microprocessor and don't
> > > continue execution on secure side in whcih case you can afford fast call ?
> Hi Sudeep,
>
Thanks for the details. Unfortunately more questions:
> Here is my understanding: Some SMC calls may take a few longer to
> complete than others. The longer ones tie up the CPU core that is
> handling the SMC call, and so nothing can be scheduled on that
> specific core.
So far good.
> Unfortunately, we have a real-time OS that runs
> sporadically on one specific core and if that happens to be the same
> core that is handling the SMC, the RTOS will miss its deadline. So we
> need to have the SMC return immediately and use an SGI for task
> completion.
>
So it sounds more like it can't be fast call then.
Does that me, it will always return early and send SGI when the request
is complete ?
1. If yes, what happens if there are multiple requests in parallel and
second one completes before the first. Can we handle that with this
patch set. Of will the second request fails until the first one is
complete ? It extends to number of cpus in the system worst case.
2. If no, will this not cause issues if we unconditional wait for interrupt
every single time ?
--
Regards,
Sudeep
Re: AMU extension v1 support for cortex A76, A77, A78 CPUs
On Fri, Nov 20, 2020 at 08:56:31AM +, Marc Zyngier wrote: > On 2020-11-20 04:30, Neeraj Upadhyay wrote: > > Hi, > > > > For ARM cortex A76, A77, A78 cores (which as per TRM, support AMU) > > AA64PFR0[47:44] field is not set, and AMU does not get enabled for > > them. > > Can you please provide support for these CPUs in cpufeature.c? > > If that was the case, that'd be an erratum, and it would need to be > documented as such. It could also be that this is an optional feature > for these cores (though the TRM doesn't suggest that). > > Can someone at ARM confirm what is the expected behaviour of these CPUs? IIRC discussion with Ionela long back, we intentionally decided not to support IMPDEF(pre 8.4 non-architected so called AMUs) on the CPUs listed in $subject. -- Regards, Sudeep
Re: [PATCH v4 3/6] hwmon: scmi: update hwmon internal scale data type
Hi Guenter, On Thu, Nov 19, 2020 at 05:49:03PM +, Cristian Marussi wrote: > Use an int to calculate scale values inside scmi_hwmon_scale() to match > the updated scale data type in struct scmi_sensor_info. > You were not cc-ed in previous versions unfortunately. I plan to take this along with the rest of SCMI changes if you are fine with this change and provide Ack/Review. Sorry for the rush. -- Regards, Sudeep
Re: [PATCH v5 4/5] regulator: add SCMI driver
On Thu, Nov 19, 2020 at 04:39:49PM +, Mark Brown wrote: > On Thu, Nov 19, 2020 at 04:13:08PM +0000, Sudeep Holla wrote: > > > I was thinking about how to merge this if and when you have reviewed it > > and happy with it. Is it OK to take via ARM SoC with dependent and other > > SCMI changes ? Or we can merge the SCMI part next release and the regulator > > in the following, up to you. > > I was expecting you to send me a pull request for the firmware bits once > you've applied them. Sure, I can do that. -- Regards, Sudeep
Re: [PATCH v8 2/3] dt-bindings: arm: cpus: Document 'mediatek,freq-domain' property
On Thu, Nov 19, 2020 at 03:23:20PM +, Lukasz Luba wrote: > > > On 10/28/20 3:08 PM, Rob Herring wrote: > > On Mon, Oct 26, 2020 at 04:19:08PM +0800, Hector Yuan wrote: > > > From: "Hector.Yuan" > > > > > > Add devicetree documentation for 'mediatek,freq-domain' property specific > > > to Mediatek CPUs. This property is used to reference the CPUFREQ node > > > along with the domain id. > > > > > > Signed-off-by: Hector.Yuan > > > --- > > > Documentation/devicetree/bindings/arm/cpus.yaml |6 ++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/Documentation/devicetree/bindings/arm/cpus.yaml > > > b/Documentation/devicetree/bindings/arm/cpus.yaml > > > index 1222bf1..e995b26 100644 > > > --- a/Documentation/devicetree/bindings/arm/cpus.yaml > > > +++ b/Documentation/devicetree/bindings/arm/cpus.yaml > > > @@ -255,6 +255,12 @@ properties: > > > where voltage is in V, frequency is in MHz. > > > + mediatek,freq-domain: > > > +$ref: '/schemas/types.yaml#/definitions/phandle-array' > > > +description: > > > + CPUs supporting freq-domain must set their "mediatek,freq-domain" > > > property > > > + with phandle to a cpufreq_hw node followed by the domain id. > > > > This needs to be a common binding shared with SCMI domains. > > Would it be accurate to create a new binding file: > Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.txt > ? > Nope, Rob already asked to unify all such bindings and generalise it. Here is my attempt[1] and this must just use it or help to enhance that in order to make use of that binding. -- Regards, Sudeep [1] https://lore.kernel.org/lkml/20201116181356.804590-1-sudeep.ho...@arm.com
Re: [PATCH v5 4/5] regulator: add SCMI driver
Hi Mark, On Tue, Nov 17, 2020 at 12:34:14PM +, Cristian Marussi wrote: > Add a simple regulator based on SCMI Voltage Domain Protocol. > I was thinking about how to merge this if and when you have reviewed it and happy with it. Is it OK to take via ARM SoC with dependent and other SCMI changes ? Or we can merge the SCMI part next release and the regulator in the following, up to you. -- Regards, Sudeep
Re: [PATCH v5 1/5] firmware: arm_scmi: Add Voltage Domain Support
On Tue, Nov 17, 2020 at 12:34:11PM +, Cristian Marussi wrote:
> Add SCMI Voltage Domain protocol support.
>
> Signed-off-by: Cristian Marussi
> ---
> v4 --> v5
> - removed inline
> - moved segmented intervals defines
> - fixed some macros complaints by checkpatch
>
> v3 --> v4
> - avoid fixed sized typing in voltage_info
> - avoid coccinelle falde complaints about pointer-sized allocations
>
> v2 --> v3
> - restrict segmented voltage domain descriptors to one triplet
> - removed unneeded inline
> - free allocated resources for invalid voltage domain
> - added __must_check to info_get voltage operations
> - added a few comments
> - removed fixed size typing from struct voltage_info
>
> v1 --> v2
> - fix voltage levels query loop to reload full cmd description
> between iterations as reported by Etienne Carriere
> - ensure rx buffer is properly sized calli scmi_reset_rx_to_maxsz
> between transfers
> ---
> drivers/firmware/arm_scmi/Makefile | 2 +-
> drivers/firmware/arm_scmi/common.h | 1 +
> drivers/firmware/arm_scmi/driver.c | 2 +
> drivers/firmware/arm_scmi/voltage.c | 397
> include/linux/scmi_protocol.h | 64 +
> 5 files changed, 465 insertions(+), 1 deletion(-)
> create mode 100644 drivers/firmware/arm_scmi/voltage.c
>
> diff --git a/drivers/firmware/arm_scmi/Makefile
> b/drivers/firmware/arm_scmi/Makefile
> index bc0d54f8e861..6a2ef63306d6 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -4,7 +4,7 @@ scmi-driver-y = driver.o notify.o
> scmi-transport-y = shmem.o
> scmi-transport-$(CONFIG_MAILBOX) += mailbox.o
> scmi-transport-$(CONFIG_HAVE_ARM_SMCCC_DISCOVERY) += smc.o
> -scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o
> +scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o system.o
> voltage.o
> scmi-module-objs := $(scmi-bus-y) $(scmi-driver-y) $(scmi-protocols-y) \
> $(scmi-transport-y)
> obj-$(CONFIG_ARM_SCMI_PROTOCOL) += scmi-module.o
> diff --git a/drivers/firmware/arm_scmi/common.h
> b/drivers/firmware/arm_scmi/common.h
> index 65063fa948d4..c0fb45e7c3e8 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -169,6 +169,7 @@ DECLARE_SCMI_REGISTER_UNREGISTER(perf);
> DECLARE_SCMI_REGISTER_UNREGISTER(power);
> DECLARE_SCMI_REGISTER_UNREGISTER(reset);
> DECLARE_SCMI_REGISTER_UNREGISTER(sensors);
> +DECLARE_SCMI_REGISTER_UNREGISTER(voltage);
> DECLARE_SCMI_REGISTER_UNREGISTER(system);
>
> #define DEFINE_SCMI_PROTOCOL_REGISTER_UNREGISTER(id, name) \
> diff --git a/drivers/firmware/arm_scmi/driver.c
> b/drivers/firmware/arm_scmi/driver.c
> index 3dfd8b6a0ebf..ada35e63feae 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -946,6 +946,7 @@ static int __init scmi_driver_init(void)
> scmi_power_register();
> scmi_reset_register();
> scmi_sensors_register();
> + scmi_voltage_register();
> scmi_system_register();
>
> return platform_driver_register(_driver);
> @@ -961,6 +962,7 @@ static void __exit scmi_driver_exit(void)
> scmi_power_unregister();
> scmi_reset_unregister();
> scmi_sensors_unregister();
> + scmi_voltage_unregister();
> scmi_system_unregister();
>
> platform_driver_unregister(_driver);
> diff --git a/drivers/firmware/arm_scmi/voltage.c
> b/drivers/firmware/arm_scmi/voltage.c
> new file mode 100644
> index ..6b71589e0846
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/voltage.c
> @@ -0,0 +1,397 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Voltage Protocol
> + *
> + * Copyright (C) 2020 ARM Ltd.
> + */
> +
> +#include
> +
> +#include "common.h"
> +
> +#define VOLTAGE_DOMS_NUM_MASKGENMASK(15, 0)
> +#define REMAINING_LEVELS_MASKGENMASK(31, 16)
> +#define RETURNED_LEVELS_MASK GENMASK(11, 0)
> +
> +enum scmi_voltage_protocol_cmd {
> + VOLTAGE_DOMAIN_ATTRIBUTES = 0x3,
> + VOLTAGE_DESCRIBE_LEVELS = 0x4,
> + VOLTAGE_CONFIG_SET = 0x5,
> + VOLTAGE_CONFIG_GET = 0x6,
> + VOLTAGE_LEVEL_SET = 0x7,
> + VOLTAGE_LEVEL_GET = 0x8,
> +};
> +
> +struct scmi_msg_resp_protocol_attributes {
> + __le32 attr;
> +#define NUM_VOLTAGE_DOMAINS(x) ((u16)(FIELD_GET(VOLTAGE_DOMS_NUM_MASK,
> (x
> +};
> +
Sorry but same annoying comment again, drop one element structures.
> +struct scmi_msg_resp_domain_attributes {
> + __le32 attr;
> + u8 name[SCMI_MAX_STR_SIZE];
> +};
> +
> +struct scmi_msg_cmd_describe_levels {
> + __le32 domain_id;
> + __le32 level_index;
> +};
> +
> +struct scmi_msg_resp_describe_levels {
> + __le32 flags;
> +#define NUM_REMAINING_LEVELS(f) ((u16)(FIELD_GET(REMAINING_LEVELS_MASK,
> (f
> +#define NUM_RETURNED_LEVELS(f) ((u16)(FIELD_GET(RETURNED_LEVELS_MASK,
> (f
>
Re: [PATCH v5 5/5] dt-bindings: arm: add support for SCMI Regulators
On Tue, Nov 17, 2020 at 12:34:15PM +, Cristian Marussi wrote: > Add devicetree bindings to support regulators based on SCMI Voltage > Domain Protocol. > Ideally, the DT binding should be first one, rather before the binding is used in the code. I can move the order while applying. -- Regards, Sudeep
Re: [PATCH v2] dt-bindings: dvfs: Add support for generic performance domains
On Wed, Nov 18, 2020 at 03:20:09PM -0600, Rob Herring wrote: > On Mon, Nov 16, 2020 at 06:13:56PM +0000, Sudeep Holla wrote: > > The CLKSCREW attack [0] exposed security vulnerabilities in energy > > management > > implementations where untrusted software had direct access to clock and > > voltage hardware controls. In this attack, the malicious software was able > > to > > place the platform into unsafe overclocked or undervolted configurations. > > Such > > configurations then enabled the injection of predictable faults to reveal > > secrets. > > > > Many Arm-based systems used to or still use voltage regulator and clock > > frameworks in the kernel. These frameworks allow callers to independently > > manipulate frequency and voltage settings. Such implementations can render > > systems susceptible to this form of attack. > > > > Attacks such as CLKSCREW are now being mitigated by not having direct and > > independent control of clock and voltage in the kernel and moving that > > control to a trusted entity, such as the SCP firmware or secure world > > firmware/software which are to perform sanity checking on the requested > > performance levels, thereby preventing any attempted malicious programming. > > > > With the advent of such an abstraction, there is a need to replace the > > generic clock and regulator bindings used by such devices with a generic > > performance domains bindings. > > > > [0] > > https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang > > > > Cc: Rob Herring > > Signed-off-by: Sudeep Holla > > --- > > .../bindings/dvfs/performance-domain.yaml | 76 +++ > > 1 file changed, 76 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > > > v1[1]->v2: > > - Changed to Dual License > > - Added select: true, enum for #performance-domain-cells and > > $ref for performance-domain > > - Changed the example to use real existing compatibles instead > > of made-up ones > > > > [1] > > https://lore.kernel.org/lkml/20201105173539.1426301-1-sudeep.ho...@arm.com > > > > diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > new file mode 100644 > > index ..29fb589a5192 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > @@ -0,0 +1,76 @@ > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Generic performance domains > > + > > +maintainers: > > + - Sudeep Holla > > + > > +description: |+ > > + This binding is intended for performance management of groups of devices > > or > > + CPUs that run in the same performance domain. Performance domains must > > not > > + be confused with power domains. A performance domain is defined by a set > > + of devices that always have to run at the same performance level. For a > > given > > + performance domain, there is a single point of control that affects all > > the > > + devices in the domain, making it impossible to set the performance level > > of > > + an individual device in the domain independently from other devices in > > + that domain. For example, a set of CPUs that share a voltage domain, and > > + have a common frequency control, is said to be in the same performance > > + domain. > > + > > + This device tree binding can be used to bind performance domain consumer > > + devices with their performance domains provided by performance domain > > + providers. A performance domain provider can be represented by any node > > in > > + the device tree and can provide one or more performance domains. A > > consumer > > + node can refer to the provider by a phandle and a set of phandle > > arguments > > + (so called performance domain specifiers) of length specified by the > > + \#performance-domain-cells property in the performance domain provider > > node. > > + > > +select: true > > So apply to every node and... > New to yaml, still figuring out . >From the bot build error, I now realise that I can't take shortcut to build: $ make dt_binding_check DT_SCHEMA_FILES=Documenta
Re: [PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
On Thu, Nov 19, 2020 at 12:20:29PM +, Cristian Marussi wrote: > Hi Sudeep > [...] > > > + S32_EXT(SENSOR_RES_EXP(ares)); > > > + dsize += sizeof(adesc->resolution); > > > + > > > + scmi_parse_range_attrs(>attrs, > > > +>attrs); > > > + dsize += sizeof(adesc->attrs); > > > + } > > > + > > > + adesc = (typeof(adesc))((u8 *)adesc + dsize); > > > > Just thinking if we can avoid this my having union comprising of v1 and v2 > > structures ? > > > > Not sure to understand, axis_descr are only v3.0 and in fact this is > called only for v > 2 I think, BUT the problem is that both this and the > main sensor descriptor v3 msg payloads are runtime variable, so that it > is stated in the msg->extended_attrs itself if that particular sensor desc > response that I'm parsing has the additional extended fields or not: > so the dance with dsize to keep track of where the current response ends > and when the next starts...but maybe I've not got really what you meant. > No worries, we can always improve later if possible, you can keep it as for now. [...] > > > + * retrieved via a dedicated (optional) command. > > > + * Since the command is optional, on error carry > > > + * on without any update interval. > > > + */ > > > + if (scmi_sensor_update_intervals(handle, s)) > > > + dev_info(handle->dev, > > > + "Update Intervals not > > > available for sensor ID:%d\n", > > > + s->id); > > > > Can we drop the logging or make it _dbg ? Make flood in a system with 100s > > of > > sensors. > > > > Sure, I was wondering in fact what to do with this: because the command > is optional but it seemed odd to me that an SCMIv3.0 sensor does not > expose any update interval so I wanted to log somehow this anomaly. > (but maybe it's just not an anomaly) > Anything optional can never be anomaly, there is high chance that f/w authors will drop it as it is optional unless it is absolutely necessary. -- Regards, Sudeep
Re: [PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type
On Thu, Nov 19, 2020 at 12:22:49PM +, Cristian Marussi wrote:
> On Thu, Nov 19, 2020 at 11:40:29AM +0000, Sudeep Holla wrote:
> > On Wed, Nov 18, 2020 at 04:29:02PM +, Cristian Marussi wrote:
> > > Use an int to calculate scale values inside scmi_hwmon_scale() to match
> > > the updated scale data type in struct scmi_sensor_info.
> > >
> > > Signed-off-by: Cristian Marussi
> > > ---
> > > drivers/hwmon/scmi-hwmon.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> > > index 09ce30cba54b..17d064e58938 100644
> > > --- a/drivers/hwmon/scmi-hwmon.c
> > > +++ b/drivers/hwmon/scmi-hwmon.c
> > > @@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
> > >
> > > static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64
> > > *value)
> > > {
> > > - s8 scale = sensor->scale;
> > > + int scale = sensor->scale;
> >
> > Can this go independent of the series ? You haven't cc-ed the hwmon
> > maintainer
> > and the list. We need their ack if we have to take it as part of the series.
> >
> Sure, I did not know in fact what's better to in this case (maybe
> bouncing/fwd the single patch ? CC the whole series ? ).
> I'll split and send to the maintainer
I see this can be independent of the series. I can provide my review/ack
tag once the maintainer is in cc. No point giving that here
--
Regards,
Sudeep
Re: [PATCH v3 6/6] firmware: arm_scmi: add SCMIv3.0 Sensor notifications
On Wed, Nov 18, 2020 at 04:29:05PM +, Cristian Marussi wrote: > Add support for new SCMIv3.0 SENSOR_UPDATE notification. > > Signed-off-by: Cristian Marussi > --- > v2 --> v3 > - removed stale unused msg payload definition > - moved variable declaration inside switch block Other than few minor comments in the previous patches, this series look OK. -- Regards, Sudeep
Re: [PATCH v3 5/6] firmware: arm_scmi: add SCMIv3.0 Sensor configuration support
On Wed, Nov 18, 2020 at 04:29:04PM +, Cristian Marussi wrote:
> Add SCMIv3.0 Sensor support for CONFIG_GET/CONFIG_SET commands.
>
> Signed-off-by: Cristian Marussi
> ---
> drivers/firmware/arm_scmi/sensors.c | 75 +
> include/linux/scmi_protocol.h | 37 ++
> 2 files changed, 112 insertions(+)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c
> b/drivers/firmware/arm_scmi/sensors.c
> index 0adc545116a4..fa3385045361 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -22,6 +22,8 @@ enum scmi_sensor_protocol_cmd {
> SENSOR_READING_GET = 0x6,
> SENSOR_AXIS_DESCRIPTION_GET = 0x7,
> SENSOR_LIST_UPDATE_INTERVALS = 0x8,
> + SENSOR_CONFIG_GET = 0x9,
> + SENSOR_CONFIG_SET = 0xA,
> };
>
> struct scmi_msg_resp_sensor_attributes {
> @@ -150,6 +152,19 @@ struct scmi_msg_set_sensor_trip_point {
> __le32 value_high;
> };
>
> +struct scmi_msg_sensor_config_get {
> + __le32 id;
> +};
> +
> +struct scmi_resp_sensor_config_get {
> + __le32 sensor_config;
> +};
> +
Same comment about single element structure as in previous patch.
--
Regards,
Sudeep
Re: [PATCH v3 4/6] firmware: arm_scmi: add SCMIv3.0 Sensors timestamped reads
On Wed, Nov 18, 2020 at 04:29:03PM +, Cristian Marussi wrote:
> Add new .reading_get_timestamped() method to sensor_ops to support SCMIv3.0
> timestamped reads.
>
> Signed-off-by: Cristian Marussi
> ---
> V2 --> v3
> - setting rx_size to 0 in sensor_reading_get to allow fw to send
> both v2 and v3 replies...even if sensor_reading_get() only handles
> v2 spec and returns one single value
> - using get_unaligned_le64 in lieu of le64_to_cpu
> - removed refs to v2.1
> ---
> drivers/firmware/arm_scmi/sensors.c | 137 ++--
> include/linux/scmi_protocol.h | 22 +
> 2 files changed, 152 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c
> b/drivers/firmware/arm_scmi/sensors.c
> index 1c83aaae0012..0adc545116a4 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -156,6 +156,27 @@ struct scmi_msg_sensor_reading_get {
> #define SENSOR_READ_ASYNCBIT(0)
> };
>
> +struct scmi_resp_sensor_reading_get {
> + __le64 readings;
Generally I have avoided such single element structures so far. Any
particular reasons for having it ?
--
Regards,
Sudeep
Re: [PATCH v3 3/6] hwmon: scmi: update hwmon internal scale data type
On Wed, Nov 18, 2020 at 04:29:02PM +, Cristian Marussi wrote:
> Use an int to calculate scale values inside scmi_hwmon_scale() to match
> the updated scale data type in struct scmi_sensor_info.
>
> Signed-off-by: Cristian Marussi
> ---
> drivers/hwmon/scmi-hwmon.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 09ce30cba54b..17d064e58938 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -30,7 +30,7 @@ static inline u64 __pow10(u8 x)
>
> static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64
> *value)
> {
> - s8 scale = sensor->scale;
> + int scale = sensor->scale;
Can this go independent of the series ? You haven't cc-ed the hwmon maintainer
and the list. We need their ack if we have to take it as part of the series.
--
Regards,
Sudeep
Re: [PATCH v3 2/6] firmware: arm_scmi: add SCMIv3.0 Sensors descriptors extensions
On Wed, Nov 18, 2020 at 04:29:01PM +, Cristian Marussi wrote:
> Add support for new SCMIv3.0 Sensors extensions related to new sensors'
> features, like multiple axis and update intervals, while keeping
> compatibility with SCMIv2.0 features.
> While at that, refactor and simplify all the internal helpers macros and
> move struct scmi_sensor_info to use only non-fixed-size typing.
>
Sorry for late review.
> Signed-off-by: Cristian Marussi
> ---
> v2 --> v3
> - Fix SCMI_MAX_NUM_SENSOR_AXIS
> - added missing Dox comment in resolution
> - added common INTVL SEGMENT macros
>
> v1 --> v2
> - restrict segmented intervals descriptors to single triplet
> - add proper usage of scmi_reset_rx_to_maxsz
> ---
> drivers/firmware/arm_scmi/sensors.c | 391 ++--
> include/linux/scmi_protocol.h | 223 +++-
> 2 files changed, 588 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/firmware/arm_scmi/sensors.c
> b/drivers/firmware/arm_scmi/sensors.c
> index 6aaff478d032..1c83aaae0012 100644
> --- a/drivers/firmware/arm_scmi/sensors.c
> +++ b/drivers/firmware/arm_scmi/sensors.c
> @@ -7,16 +7,21 @@
>
> #define pr_fmt(fmt) "SCMI Notifications SENSOR - " fmt
>
> +#include
> #include
>
> #include "common.h"
> #include "notify.h"
>
> +#define SCMI_MAX_NUM_SENSOR_AXIS 63
> +
> enum scmi_sensor_protocol_cmd {
> SENSOR_DESCRIPTION_GET = 0x3,
> SENSOR_TRIP_POINT_NOTIFY = 0x4,
> SENSOR_TRIP_POINT_CONFIG = 0x5,
> SENSOR_READING_GET = 0x6,
> + SENSOR_AXIS_DESCRIPTION_GET = 0x7,
> + SENSOR_LIST_UPDATE_INTERVALS = 0x8,
> };
>
> struct scmi_msg_resp_sensor_attributes {
> @@ -28,23 +33,102 @@ struct scmi_msg_resp_sensor_attributes {
> __le32 reg_size;
> };
>
> +/* v3 attributes_low macros */
> +#define SUPPORTS_UPDATE_NOTIFY(x)FIELD_GET(BIT(30), (x))
> +#define SENSOR_TSTAMP_EXP(x) FIELD_GET(GENMASK(14, 10), (x))
> +#define SUPPORTS_TIMESTAMP(x)FIELD_GET(BIT(9), (x))
> +#define SUPPORTS_EXTEND_ATTRS(x) FIELD_GET(BIT(8), (x))
> +
> +/* v2 attributes_high macros */
> +#define SENSOR_UPDATE_BASE(x)FIELD_GET(GENMASK(31, 27), (x))
> +#define SENSOR_UPDATE_SCALE(x) FIELD_GET(GENMASK(26, 22), (x))
> +
> +/* v3 attributes_high macros */
> +#define SENSOR_AXIS_NUMBER(x)FIELD_GET(GENMASK(21, 16), (x))
> +#define SUPPORTS_AXIS(x) FIELD_GET(BIT(8), (x))
> +
> +/* v3 resolution macros */
> +#define SENSOR_RES(x)FIELD_GET(GENMASK(26, 0), (x))
> +#define SENSOR_RES_EXP(x)FIELD_GET(GENMASK(31, 27), (x))
> +
> +struct scmi_range_attrs_le {
[nit] Does "_le" above indicate little endian ? If so, please drop it here
and elsewhere in the series as it is not consistent with other structure
names. LE is assumed throughout SCMI.
> + __le32 min_range_low;
> + __le32 min_range_high;
> + __le32 max_range_low;
> + __le32 max_range_high;
> +};
> +
> struct scmi_msg_resp_sensor_description {
> __le16 num_returned;
> __le16 num_remaining;
> - struct {
> + struct scmi_sensor_descriptor {
> __le32 id;
> __le32 attributes_low;
> -#define SUPPORTS_ASYNC_READ(x) ((x) & BIT(31))
> -#define NUM_TRIP_POINTS(x) ((x) & 0xff)
> +/* Common attributes_low macros */
> +#define SUPPORTS_ASYNC_READ(x) FIELD_GET(BIT(31), (x))
> +#define NUM_TRIP_POINTS(x) FIELD_GET(GENMASK(7, 0), (x))
> __le32 attributes_high;
> -#define SENSOR_TYPE(x) ((x) & 0xff)
> -#define SENSOR_SCALE(x) (((x) >> 11) & 0x1f)
> -#define SENSOR_SCALE_SIGNBIT(4)
> -#define SENSOR_SCALE_EXTEND GENMASK(7, 5)
> -#define SENSOR_UPDATE_SCALE(x) (((x) >> 22) & 0x1f)
> -#define SENSOR_UPDATE_BASE(x)(((x) >> 27) & 0x1f)
> - u8 name[SCMI_MAX_STR_SIZE];
> - } desc[0];
> +/* Common attributes_high macros */
> +#define SENSOR_SCALE(x) FIELD_GET(GENMASK(15, 11), (x))
> +#define SENSOR_SCALE_SIGNBIT(4)
> +#define SENSOR_SCALE_EXTEND GENMASK(31, 5)
> +#define SENSOR_TYPE(x) FIELD_GET(GENMASK(7, 0), (x))
> + u8 name[SCMI_MAX_STR_SIZE];
> + /* only for version > 2.0 */
> + __le32 power;
> + __le32 resolution;
> + struct scmi_range_attrs_le scalar_attrs;
> + } desc[];
> +};
> +
> +/* Sign extend to a full s32 */
> +#define S32_EXT(v)
> \
> + ({ \
> + int __v = (v); \
> + \
> + if (__v & SENSOR_SCALE_SIGN)\
> + __v |= SENSOR_SCALE_EXTEND; \
> + __v;
[PATCH v2] dt-bindings: dvfs: Add support for generic performance domains
The CLKSCREW attack [0] exposed security vulnerabilities in energy management
implementations where untrusted software had direct access to clock and
voltage hardware controls. In this attack, the malicious software was able to
place the platform into unsafe overclocked or undervolted configurations. Such
configurations then enabled the injection of predictable faults to reveal
secrets.
Many Arm-based systems used to or still use voltage regulator and clock
frameworks in the kernel. These frameworks allow callers to independently
manipulate frequency and voltage settings. Such implementations can render
systems susceptible to this form of attack.
Attacks such as CLKSCREW are now being mitigated by not having direct and
independent control of clock and voltage in the kernel and moving that
control to a trusted entity, such as the SCP firmware or secure world
firmware/software which are to perform sanity checking on the requested
performance levels, thereby preventing any attempted malicious programming.
With the advent of such an abstraction, there is a need to replace the
generic clock and regulator bindings used by such devices with a generic
performance domains bindings.
[0]
https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang
Cc: Rob Herring
Signed-off-by: Sudeep Holla
---
.../bindings/dvfs/performance-domain.yaml | 76 +++
1 file changed, 76 insertions(+)
create mode 100644
Documentation/devicetree/bindings/dvfs/performance-domain.yaml
v1[1]->v2:
- Changed to Dual License
- Added select: true, enum for #performance-domain-cells and
$ref for performance-domain
- Changed the example to use real existing compatibles instead
of made-up ones
[1] https://lore.kernel.org/lkml/20201105173539.1426301-1-sudeep.ho...@arm.com
diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
new file mode 100644
index ..29fb589a5192
--- /dev/null
+++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Generic performance domains
+
+maintainers:
+ - Sudeep Holla
+
+description: |+
+ This binding is intended for performance management of groups of devices or
+ CPUs that run in the same performance domain. Performance domains must not
+ be confused with power domains. A performance domain is defined by a set
+ of devices that always have to run at the same performance level. For a given
+ performance domain, there is a single point of control that affects all the
+ devices in the domain, making it impossible to set the performance level of
+ an individual device in the domain independently from other devices in
+ that domain. For example, a set of CPUs that share a voltage domain, and
+ have a common frequency control, is said to be in the same performance
+ domain.
+
+ This device tree binding can be used to bind performance domain consumer
+ devices with their performance domains provided by performance domain
+ providers. A performance domain provider can be represented by any node in
+ the device tree and can provide one or more performance domains. A consumer
+ node can refer to the provider by a phandle and a set of phandle arguments
+ (so called performance domain specifiers) of length specified by the
+ \#performance-domain-cells property in the performance domain provider node.
+
+select: true
+
+properties:
+ "#performance-domain-cells":
+description:
+ Number of cells in a performance domain specifier. Typically 0 for nodes
+ representing a single performance domain and 1 for nodes providing
+ multiple performance domains (e.g. performance controllers), but can be
+ any value as specified by device tree binding documentation of particular
+ provider.
+enum: [ 0, 1 ]
+
+ performance-domains:
+$ref: '/schemas/types.yaml#/definitions/phandle-array'
+description:
+ A phandle and performance domain specifier as defined by bindings of the
+ performance controller/provider specified by phandle.
+
+required:
+ - "#performance-domain-cells"
+
+additionalProperties: true
+
+examples:
+ - |
+performance: performance-controller@1234 {
+compatible = "qcom,cpufreq-hw";
+reg = <0x1234 0x1000>;
+#performance-domain-cells = <1>;
+};
+
+// The node above defines a performance controller that is a performance
+// domain provider and expects one cell as its phandle argument.
+cpus {
+#address-cells = <2>;
+#size-cells = <0>;
+
+cpu@0 {
+device_type = "cpu";
+
Re: [PATCH] dt-bindings: dvfs: Add support for generic performance domains
On Mon, Nov 09, 2020 at 02:15:18PM -0600, Rob Herring wrote: > On Thu, Nov 05, 2020 at 05:35:39PM +0000, Sudeep Holla wrote: > > The CLKSCREW attack [0] exposed security vulnerabilities in energy > > management > > implementations where untrusted software had direct access to clock and > > voltage hardware controls. In this attack, the malicious software was able > > to > > place the platform into unsafe overclocked or undervolted configurations. > > Such > > configurations then enabled the injection of predictable faults to reveal > > secrets. > > > > Many Arm-based systems used to or still use voltage regulator and clock > > frameworks in the kernel. These frameworks allow callers to independently > > manipulate frequency and voltage settings. Such implementations can render > > systems susceptible to this form of attack. > > > > Attacks such as CLKSCREW are now being mitigated by not having direct and > > independent control of clock and voltage in the kernel and moving that > > control to a trusted entity, such as the SCP firmware or secure world > > firmware/software which are to perform sanity checking on the requested > > performance levels, thereby preventing any attempted malicious programming. > > > > With the advent of such an abstraction, there is a need to replace the > > generic clock and regulator bindings used by such devices with a generic > > performance domains bindings. > > > > [0] > > https://www.usenix.org/conference/usenixsecurity17/technical-sessions/presentation/tang > > > > Cc: Rob Herring > > Signed-off-by: Sudeep Holla > > --- > > .../bindings/dvfs/performance-domain.yaml | 67 +++ > > 1 file changed, 67 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > > > diff --git a/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > new file mode 100644 > > index ..fa0151f63ac9 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/dvfs/performance-domain.yaml > > @@ -0,0 +1,67 @@ > > +# SPDX-License-Identifier: GPL-2.0 > > Dual license new bindings. > > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/dvfs/performance-domain.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Generic performance domains > > + > > +maintainers: > > + - Sudeep Holla > > + > > +description: |+ > > + This binding is intended for performance management of groups of devices > > or > > + CPUs that run in the same performance domain. Performance domains must > > not > > + be confused with power domains. A performance domain is defined by a set > > + of devices that always have to run at the same performance level. For a > > given > > + performance domain, there is a single point of control that affects all > > the > > + devices in the domain, making it impossible to set the performance level > > of > > + an individual device in the domain independently from other devices in > > + that domain. For example, a set of CPUs that share a voltage domain, and > > + have a common frequency control, is said to be in the same performance > > + domain. > > + > > + This device tree binding can be used to bind performance domain consumer > > + devices with their performance domains provided by performance domain > > + providers. A performance domain provider can be represented by any node > > in > > + the device tree and can provide one or more performance domains. A > > consumer > > + node can refer to the provider by a phandle and a set of phandle > > arguments > > + (so called performance domain specifiers) of length specified by the > > + \#performance-domain-cells property in the performance domain provider > > node. > > select: true > > Otherwise, this schema is never used. > > > + > > +properties: > > + "#performance-domain-cells": > > +description: > > + Number of cells in a performance domain specifier. Typically 0 for > > nodes > > + representing a single performance domain and 1 for nodes providing > > + multiple performance domains (e.g. performance controllers), but can > > be > > + any value as specified by device tree binding documentation of > > particular > > + provider. > > enum: [ 0, 1 ] > > If we n
Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Fri, Nov 13, 2020 at 09:26:43AM -0500, Jim Quinlan wrote:
> Hi, these are fast calls. Regards, Jim
>
>
> On Fri, Nov 13, 2020 at 4:47 AM Sudeep Holla wrote:
> >
> > On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote:
> > > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI
> > > message to be indicated by an interrupt rather than the return of the smc
> > > call. This accommodates the existing behavior of the BrcmSTB SCMI
> > > "platform" whose SW is already out in the field and cannot be changed.
> > >
> >
> > Sorry for missing to check with you earlier. Are these not fast smc calls ?
> > Can we check the SMC Function IDs for the same and expect IRQ to be present
> > if they are not fast calls ?
> Hi, if I understand you correctly you want to do something like this:
>
> if (! ARM_SMCCC_IS_FAST_CALL(func_id)) {
> /* look for irq and request it */
> }
>
Yes.
> But we do use fast calls.
What was the rationale for retaining fast SMC calls but use IRQ for Tx
completion ?
Is it because you offload it to some other microprocessor and don't
continue execution on secure side in whcih case you can afford fast call ?
--
Regards,
Sudeep
Re: [PATCH v2] firmware: arm_scmi: fix missing destroy_workqueue()
On Tue, 10 Nov 2020 15:42:21 +0800, Qinglang Miao wrote: > destroy_workqueue seems necessary before return from > scmi_notification_init in the error handling case when > fails to do devm_kcalloc(). Fix this by simply moving > devm_kcalloc to the front. Applied to sudeep.holla/linux (for-next/scmi), thanks! [1/1] firmware: arm_scmi: Fix missing destroy_workqueue() https://git.kernel.org/sudeep.holla/c/6bbdb46c4b -- Regards, Sudeep
Re: [PATCH v4 3/3] arm64: implement CPPC FFH support using AMUs
On Fri, Nov 06, 2020 at 12:53:34PM +, Ionela Voinescu wrote:
> If Activity Monitors (AMUs) are present, two of the counters can be used
> to implement support for CPPC's (Collaborative Processor Performance
> Control) delivered and reference performance monitoring functionality
> using FFH (Functional Fixed Hardware).
>
> Given that counters for a certain CPU can only be read from that CPU,
> while FFH operations can be called from any CPU for any of the CPUs, use
> smp_call_function_single() to provide the requested values.
>
> Therefore, depending on the register addresses, the following values
> are returned:
> - 0x0 (DeliveredPerformanceCounterRegister): AMU core counter
> - 0x1 (ReferencePerformanceCounterRegister): AMU constant counter
>
> The use of Activity Monitors is hidden behind the generic
> cpu_read_{corecnt,constcnt}() functions.
>
> Read functionality for these two registers represents the only current
> FFH support for CPPC. Read operations for other register values or write
> operation for all registers are unsupported. Therefore, keep CPPC's FFH
> unsupported if no CPUs have valid AMU frequency counters. For this
> purpose, the get_cpu_with_amu_feat() is introduced.
>
> Signed-off-by: Ionela Voinescu
> Cc: Catalin Marinas
> Cc: Will Deacon
> ---
> arch/arm64/include/asm/cpufeature.h | 3 ++
> arch/arm64/kernel/cpufeature.c | 10 +
> arch/arm64/kernel/topology.c| 64 +
> 3 files changed, 77 insertions(+)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h
> b/arch/arm64/include/asm/cpufeature.h
> index 751bd9d3376b..f5b44ac354dc 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -772,6 +772,9 @@ static inline bool cpu_has_amu_feat(int cpu)
> }
> #endif
>
> +/* Get a cpu that supports the Activity Monitors Unit (AMU) */
> +extern int get_cpu_with_amu_feat(void);
> +
> static inline unsigned int get_vmid_bits(u64 mmfr1)
> {
> int vmid_bits;
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index 1142970e985b..6b08ae74ad0a 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1526,6 +1526,11 @@ bool cpu_has_amu_feat(int cpu)
> return cpumask_test_cpu(cpu, _cpus);
> }
>
> +int get_cpu_with_amu_feat(void)
> +{
> + return cpumask_any(_cpus);
> +}
> +
> static void cpu_amu_enable(struct arm64_cpu_capabilities const *cap)
> {
> if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) {
> @@ -1554,6 +1559,11 @@ static bool has_amu(const struct
> arm64_cpu_capabilities *cap,
>
> return true;
> }
> +#else
> +int get_cpu_with_amu_feat(void)
> +{
> + return nr_cpu_ids;
> +}
> #endif
>
> #ifdef CONFIG_ARM64_VHE
> diff --git a/arch/arm64/kernel/topology.c b/arch/arm64/kernel/topology.c
> index b8cb16e3a2cc..7c9b6a0ecd6a 100644
> --- a/arch/arm64/kernel/topology.c
> +++ b/arch/arm64/kernel/topology.c
> @@ -147,6 +147,9 @@ void update_freq_counters_refs(void)
>
> static inline bool freq_counters_valid(int cpu)
> {
> + if ((cpu >= nr_cpu_ids) || !cpumask_test_cpu(cpu, cpu_present_mask))
> + return false;
> +
> if (!cpu_has_amu_feat(cpu)) {
> pr_debug("CPU%d: counters are not supported.\n", cpu);
> return false;
> @@ -323,3 +326,64 @@ void topology_scale_freq_tick(void)
> this_cpu_write(arch_core_cycles_prev, core_cnt);
> this_cpu_write(arch_const_cycles_prev, const_cnt);
> }
> +
> +#ifdef CONFIG_ACPI_CPPC_LIB
> +#include
Not sure what arm64 maintainers prefer, but this code has nothing to do
with topolopy strictly speaking. I wonder if we can put it in separate
file conditionally compiled if CONFIG_ACPI_CPPC_LIB is enabled there
by eliminating #ifdef(main reason for raising this point).
Either way:
Reviewed-by: Sudeep Holla
--
Regards,
Sudeep
Re: [PATCH v4 2/3] arm64: split counter validation function
On Fri, Nov 06, 2020 at 12:53:33PM +, Ionela Voinescu wrote: > In order for the counter validation function to be reused, split > validate_cpu_freq_invariance_counters() into: > - freq_counters_valid(cpu) - check cpu for valid cycle counters > - freq_inv_set_max_ratio(int cpu, u64 max_rate, u64 ref_rate) - >generic function that sets the normalization ratio used by >topology_scale_freq_tick() > Reviewed-by: Sudeep Holla -- Regards, Sudeep
Re: [PATCH v4 1/3] arm64: wrap and generalise counter read functions
On Fri, Nov 06, 2020 at 12:53:32PM +, Ionela Voinescu wrote: > In preparation for other uses of Activity Monitors (AMU) cycle counters, > place counter read functionality in generic functions that can reused: > read_corecnt() and read_constcnt(). > > As a result, implement update_freq_counters_refs() to replace > init_cpu_freq_invariance_counters() and both initialise and update > the per-cpu reference variables. > Reviewed-by: Sudeep Holla -- Regards, Sudeep
Re: [PATCH v2 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Thu, Nov 12, 2020 at 12:56:27PM -0500, Jim Quinlan wrote: > The SMC/HVC SCMI transport is modified to allow the completion of an SCMI > message to be indicated by an interrupt rather than the return of the smc > call. This accommodates the existing behavior of the BrcmSTB SCMI > "platform" whose SW is already out in the field and cannot be changed. > Sorry for missing to check with you earlier. Are these not fast smc calls ? Can we check the SMC Function IDs for the same and expect IRQ to be present if they are not fast calls ? -- Regards, Sudeep
Re: [PATCH v2 1/1] firmware: arm_scmi: Add SCMI System Power Control driver
On Mon, Oct 26, 2020 at 08:55:31PM +, Cristian Marussi wrote: > Add an SCMI System Power control driver to handle platform's requests > carried by SYSTEM_POWER_STATE_NOTIFIER notifications: such platform > requested system power state transitions are handled accordingly, > gracefully or forcefully, depending on the notifications' message flags. > > Graceful requests are by default relayed to userspace using the same > Kernel API used to handle ACPI Shutdown bus events: alternatively, instead, > a few available module parameters can be used to tunnel instead such > requests to userspace via signals addressed to CAD pid. > > When handling graceful requests, grant userspace processes a maximum > (configurable) time to perform their duties and then revert to a forceful > transition, so avoiding completely timing out platform's maximum grace time > and hitting possible abrupt power-cuts. > > Signed-off-by: Cristian Marussi > --- > drivers/firmware/Kconfig | 12 + > drivers/firmware/arm_scmi/Makefile| 1 + > .../firmware/arm_scmi/scmi_power_control.c| 387 ++ > 3 files changed, 400 insertions(+) > create mode 100644 drivers/firmware/arm_scmi/scmi_power_control.c > [nit] Since you have just one patch, it is generally preferred to place all the extra non commit messages here after diff stat to have a single thread of discussion. Anyways, probably ccing Arnd/Greg/or anyone who has made some feature changes in this area(e.g. kernel/reboot.c) might help to get some useful feedback here. -- Regards, Sudeep
Re: [PATCH v1 2/2] firmware: arm_scmi: Augment SMC/HVC to allow optional interrupt
On Wed, Nov 11, 2020 at 05:45:46PM +, Cristian Marussi wrote:
> On Wed, Nov 11, 2020 at 11:45:24AM -0500, Jim Quinlan wrote:
> > On Wed, Nov 11, 2020 at 5:42 AM Sudeep Holla wrote:
> > >
> > > On Tue, Nov 10, 2020 at 01:38:19PM -0500, Jim Quinlan wrote:
> > > > The SMC/HVC SCMI transport is modified to allow the completion of an
> > > > SCMI
> > > > message to be indicated by an interrupt rather than the return of the
> > > > smc
> > > > call. This accommodates the existing behavior of the BrcmSTB SCMI
> > > > "platform" whose SW is already out in the field and cannot be changed.
> > > >
> > > > Signed-off-by: Jim Quinlan
> > > > ---
> > > > drivers/firmware/arm_scmi/smc.c | 31 +++
> > > > 1 file changed, 31 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/arm_scmi/smc.c
> > > > b/drivers/firmware/arm_scmi/smc.c
> > > > index 82a82a5dc86a..3bf935dbd00e 100644
> > > > --- a/drivers/firmware/arm_scmi/smc.c
> > > > +++ b/drivers/firmware/arm_scmi/smc.c
> > > > @@ -9,9 +9,11 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > > #include
> > > >
> > > > #include "common.h"
> > > > @@ -23,6 +25,8 @@
> > > > * @shmem: Transmit/Receive shared memory area
> > > > * @shmem_lock: Lock to protect access to Tx/Rx shared memory area
> > > > * @func_id: smc/hvc call function id
> > > > + * @irq: Optional; employed when platforms indicates msg completion by
> > > > intr.
> > > > + * @tx_complete: Optional, employed only when irq is valid.
> > > > */
> > > >
> > > > struct scmi_smc {
> > > > @@ -30,8 +34,19 @@ struct scmi_smc {
> > > > struct scmi_shared_mem __iomem *shmem;
> > > > struct mutex shmem_lock;
> > > > u32 func_id;
> > > > + int irq;
> > > > + struct completion tx_complete;
> > > > };
> > > >
> > > > +static irqreturn_t smc_msg_done_isr(int irq, void *data)
> > > > +{
> > > > + struct scmi_smc *scmi_info = data;
> > > > +
> > > > + complete(_info->tx_complete);
> > > > +
> > > > + return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > static bool smc_chan_available(struct device *dev, int idx)
> > > > {
> > > > struct device_node *np = of_parse_phandle(dev->of_node, "shmem",
> > > > 0);
> > > > @@ -79,6 +94,20 @@ static int smc_chan_setup(struct scmi_chan_info
> > > > *cinfo, struct device *dev,
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > > > + /* Optional feature -- signal message completion using an
> > > > interrupt */
> > > > + ret = of_irq_get_byname(cdev->of_node, "msg-serviced");
> > >
> > > So, looks like it is mandatory if "interrupts" is used. Please update the
> > > binding or if that is not the practice followed elsewhere, drop search by
> > > name.
> >
> > Well, I can certainly change the comment. I do not want it to be
> > "mandatory" if just interrupts are used.
> > The reason I prefer using "interrupt-names" is that this allows
> > unforeseen use of future additional interrupts w/o caring about order
> > in the interrupts DT node. If you are absolutely positive that there
> > will never be other interrupts used in the future for the SCMI node
> > then I will drop it.
> >
Good point, please make it required property then if "interrupts" property
is present.
>
> What about the future possibility of adding p2a notifications handling
> to SMC transport, won't that need some other IRQ (and shmem) ?
>
Indeed it needs. Since this Tx completion interrupt is optional and may not
be present, better to fix the name so that when Rx/notification interrupt
is added in future, we can identify them easily.
--
Regards,
Sudeep
