| Message ID | 20240301164227.339208-4-abdellatif.elkhlifi@arm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | remoteproc: introduce Arm remoteproc support | expand |
On 01/03/2024 17:42, abdellatif.elkhlifi@arm.com wrote: > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > > introduce the bindings for Arm remoteproc support. > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > --- > .../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 000000000000..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? > + > +maintainers: > + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > + > +description: | > + Some Arm heterogeneous System-On-Chips feature remote processors that can > + be controlled with a reset control register and a reset status register to > + start or stop the processor. > + > + This document defines the bindings for these remote processors. Drop last sentence. > + > +properties: > + compatible: > + enum: > + - arm,corstone1000-extsys > + > + reg: > + minItems: 2 > + maxItems: 2 > + description: | > + Address and size in bytes of the reset control register > + and the reset status register. > + Expects the registers to be in the order as above. > + Should contain an entry for each value in 'reg-names'. Entirely redundant sentences... instead this all just list items with description. > + > + reg-names:Do not need '|' unless you need to preserve formatting. > + description: | > + Required names for each of the reset registers defined in > + the 'reg' property. Expects the names from the following > + list, in the specified order, each representing the corresponding > + reset register. Really, drop. > + items: > + - const: reset-control > + - const: reset-status > + > + firmware-name: > + description: | Do not need '|' unless you need to preserve formatting. > + Default name of the firmware to load to the remote processor. > + > +required: > + - compatible > + - reg > + - reg-names > + - firmware-name > + > +additionalProperties: false > + > +examples: > + - | > + extsys0: remoteproc@1a010310 { Drop label, not used. > + compatible = "arm,corstone1000-extsys"; Use 4 spaces for example indentation. > + reg = <0x1a010310 0x4>, <0x1a010314 0x4>; > + reg-names = "reset-control", "reset-status"; > + firmware-name = "es0_flashfw.elf"; > + }; > + > + extsys1: remoteproc@1a010318 { > + compatible = "arm,corstone1000-extsys"; These are the same examples, so keep only one. > + reg = <0x1a010318 0x4>, <0x1a01031c 0x4>; > + reg-names = "reset-control", "reset-status"; > + firmware-name = "es1_flashfw.elf"; > + }; Best regards, Krzysztof
On Fri, Mar 01, 2024 at 08:30:43PM +0100, Krzysztof Kozlowski wrote: > On 01/03/2024 17:42, abdellatif.elkhlifi@arm.com wrote: > > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > > > > introduce the bindings for Arm remoteproc support. > > > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > > --- > > .../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 000000000000..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.
Hi Krzysztof, Sudeep, > > > diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > > > new file mode 100644 > > > index 000000000000..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. Thank you guys. The file names and the documentation will reflect that it's an Arm Corstone SoC. Work in progress. Cheers, Abdellatif
On 2024-03-01 4:42 pm, abdellatif.elkhlifi@arm.com wrote: > From: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > > introduce the bindings for Arm remoteproc support. > > Signed-off-by: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > --- > .../bindings/remoteproc/arm,rproc.yaml | 69 +++++++++++++++++++ > MAINTAINERS | 1 + > 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 000000000000..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 > + > +maintainers: > + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > + > +description: | > + Some Arm heterogeneous System-On-Chips feature remote processors that can > + be controlled with a reset control register and a reset status register to > + start or stop the processor. > + > + This document defines the bindings for these remote processors. > + > +properties: > + compatible: > + enum: > + - arm,corstone1000-extsys > + > + reg: > + minItems: 2 > + maxItems: 2 > + description: | > + Address and size in bytes of the reset control register > + and the reset status register. > + Expects the registers to be in the order as above. > + Should contain an entry for each value in 'reg-names'. > + > + reg-names: > + description: | > + Required names for each of the reset registers defined in > + the 'reg' property. Expects the names from the following > + list, in the specified order, each representing the corresponding > + reset register. > + items: > + - const: reset-control > + - const: reset-status > + > + 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. 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. Thanks, Robin. > + > +required: > + - compatible > + - reg > + - reg-names > + - firmware-name > + > +additionalProperties: false > + > +examples: > + - | > + extsys0: remoteproc@1a010310 { > + compatible = "arm,corstone1000-extsys"; > + reg = <0x1a010310 0x4>, <0x1a010314 0x4>; > + reg-names = "reset-control", "reset-status"; > + firmware-name = "es0_flashfw.elf"; > + }; > + > + extsys1: remoteproc@1a010318 { > + compatible = "arm,corstone1000-extsys"; > + reg = <0x1a010318 0x4>, <0x1a01031c 0x4>; > + reg-names = "reset-control", "reset-status"; > + firmware-name = "es1_flashfw.elf"; > + }; > diff --git a/MAINTAINERS b/MAINTAINERS > index 54d6a40feea5..eddaa3841a65 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -1768,6 +1768,7 @@ ARM REMOTEPROC DRIVER > M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> > L: linux-remoteproc@vger.kernel.org > S: Maintained > +F: Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml > F: drivers/remoteproc/arm_rproc.c > > ARM SMC WATCHDOG DRIVER
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). 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. 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 Currently the mailbox controller does exist in the HW but is not usable via virtio (no memory sharing available). Do you recommend I add the mboxes property even currently we can't do the comms ? > 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. Currently the HW provides only 2 registers to control the remote processors: The reset control and status registers. It makes sense to me to use a mapped region of 8 bytes for both registers rather than individual registers (since they are consecutive). I'll update that, thanks for the suggestion. Abdellatif, Cheers
On 14/03/2024 14:49, Abdellatif El Khlifi wrote: >> 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). > > 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. > > 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 > > Currently the mailbox controller does exist in the HW but is not > usable via virtio (no memory sharing available). > > Do you recommend I add the mboxes property even currently we can't do the comms ? Bindings should be complete, regardless whether Linux driver supports it or not. Please see writing bindings document for explanation on this and other rules. So yes: please describe as much as possible/reasonable. Best regards, Krzysztof
On Thu, Mar 14, 2024 at 01:49:28PM +0000, 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.
Hi Krzysztof, On Thu, Mar 14, 2024 at 02:56:53PM +0100, Krzysztof Kozlowski wrote: > On 14/03/2024 14:49, Abdellatif El Khlifi wrote: > >> 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). > > > > 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. > > > > 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 > > > > Currently the mailbox controller does exist in the HW but is not > > usable via virtio (no memory sharing available). > > > > Do you recommend I add the mboxes property even currently we can't do the comms ? > > Bindings should be complete, regardless whether Linux driver supports it > or not. Please see writing bindings document for explanation on this and > other rules. > > So yes: please describe as much as possible/reasonable. I'll do thanks. Cheers, Abdellatif
Hi Sudeep, On Thu, Mar 14, 2024 at 03:19:13PM +0000, Sudeep Holla wrote: > > 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. Sounds good to me. I'll make the binding as complete as possible. > 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. I like the idea of using syscon/regmap to represent the "Host Base System Control registers" area. Thank you for suggesting that. I think syscon is the way to go (rather than MFD). With syscon we can use the generic syscon driver that converts a set of MMIO registers to a regmap, allowing it to be accessed from multiple device drivers. In our case these MMIO registers will be the "Host Base System Control registers". remoteproc will be a child node under sysctrl node. Cheers, Abdellatif
diff --git a/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml b/Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml new file mode 100644 index 000000000000..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 + +maintainers: + - Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> + +description: | + Some Arm heterogeneous System-On-Chips feature remote processors that can + be controlled with a reset control register and a reset status register to + start or stop the processor. + + This document defines the bindings for these remote processors. + +properties: + compatible: + enum: + - arm,corstone1000-extsys + + reg: + minItems: 2 + maxItems: 2 + description: | + Address and size in bytes of the reset control register + and the reset status register. + Expects the registers to be in the order as above. + Should contain an entry for each value in 'reg-names'. + + reg-names: + description: | + Required names for each of the reset registers defined in + the 'reg' property. Expects the names from the following + list, in the specified order, each representing the corresponding + reset register. + items: + - const: reset-control + - const: reset-status + + firmware-name: + description: | + Default name of the firmware to load to the remote processor. + +required: + - compatible + - reg + - reg-names + - firmware-name + +additionalProperties: false + +examples: + - | + extsys0: remoteproc@1a010310 { + compatible = "arm,corstone1000-extsys"; + reg = <0x1a010310 0x4>, <0x1a010314 0x4>; + reg-names = "reset-control", "reset-status"; + firmware-name = "es0_flashfw.elf"; + }; + + extsys1: remoteproc@1a010318 { + compatible = "arm,corstone1000-extsys"; + reg = <0x1a010318 0x4>, <0x1a01031c 0x4>; + reg-names = "reset-control", "reset-status"; + firmware-name = "es1_flashfw.elf"; + }; diff --git a/MAINTAINERS b/MAINTAINERS index 54d6a40feea5..eddaa3841a65 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1768,6 +1768,7 @@ ARM REMOTEPROC DRIVER M: Abdellatif El Khlifi <abdellatif.elkhlifi@arm.com> L: linux-remoteproc@vger.kernel.org S: Maintained +F: Documentation/devicetree/bindings/remoteproc/arm,rproc.yaml F: drivers/remoteproc/arm_rproc.c ARM SMC WATCHDOG DRIVER