Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu, > > > > > > > > > > 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. > > > > > > > > > > I also agree with Robin's point of view. Proceeding with an initial > > > driver with minimal functionality doesn't preclude having complete > > > bindings. But that said and as I pointed out, it might be better to > > > wait for the HW to be fixed before moving forward. > > > > We checked with the HW teams. The missing features will be implemented but > > this will take time. > > > > The foundation driver as it is right now is still valuable for people > > wanting to > > know how to power control Corstone external systems in a future proof manner > > (even in the incomplete state). We prefer to address all the review comments > > made so it can be merged. This includes making the DT binding as complete as > > possible as you advised. Then, once the HW is ready, I'll implement the > > comms > > and the FW reload part. Is that OK please ? > > > > I'm in agreement with that plan as long as we agree the current > preloaded heuristic is temporary and is not a valid long term > scenario. Yes, that's the plan, no problem. Cheers, Abdellatif
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu, > > > > > > > > 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. > > > > I also agree with Robin's point of view. Proceeding with an initial > driver with minimal functionality doesn't preclude having complete > bindings. But that said and as I pointed out, it might be better to > wait for the HW to be fixed before moving forward. We checked with the HW teams. The missing features will be implemented but this will take time. The foundation driver as it is right now is still valuable for people wanting to know how to power control Corstone external systems in a future proof manner (even in the incomplete state). We prefer to address all the review comments made so it can be merged. This includes making the DT binding as complete as possible as you advised. Then, once the HW is ready, I'll implement the comms and the FW reload part. Is that OK please ? Cheers, Abdellatif
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
Hi Sudeep, On Thu, Mar 14, 2024 at 03:19:13PM +, 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
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
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
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Sudeep, On Thu, Mar 14, 2024 at 02:59:20PM +, Sudeep Holla wrote: > 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. Please have a look at my answer to Robin with clarifications [1]. Apart from mapping the register area rather than using the reg property I'll also add the mboxes property as Krzysztof confirmed. [1]: https://lore.kernel.org/all/20240314134928.ga27...@e130802.arm.com/ Cheers, Abdellatif
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
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
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu, On Wed, Mar 13, 2024 at 10:25:32AM -0600, Mathieu Poirier wrote: > On Tue, Mar 12, 2024 at 05:32:52PM +0000, 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. Cheers Abdellatif
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
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. 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 under development. This is why this patchset is relying on a preloaded firmware. And it's the step 1 of adding remoteproc support for Corstone. When the HW is ready, we will be able to avoid preloading the firmware and the user can do the following: 1) Use a default firmware filename stated in the DT (firmware-name property), that's the one remoteproc subsystem will use initially, load the firmware file and start the remote core. 2) Then, the user can choose to use another firmware file: echo stop >/sys/class/remoteproc/remoteproc0/state echo -n new_firmware.elf > /sys/class/remoteproc/remoteproc0/firmware echo start >/sys/class/remoteproc/remoteproc0/state > > 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 > > > What happens when a user wants to boot the remote processor with the > firmware provided on the file system rather than the one preloaded > into memory? We will support this scenario when the HW is upgraded and copying the firmware to the remote core memory becomes possible. > Furthermore, how do we account for scenarios where the > remote processor goes from running a firmware image on the file system > to the firmware image loaded by an external entity? Is this a valid > scenario? 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. > > Steps 2 & 3 are waiting for a HW update so the Cortex-A35 (running Linux) > > can share memory with > > the remote core. > > > > I'm happy to provide more explanation in the commit log to reflect this > > status. > > > > Is it OK that we go with step 1 as a foundation please ? > > > > First let's clarify all the scenarios that need to be supported. From > there I will advise on how to proceed and what modifications to the > subsystem's core should be made, if need be. Thanks, I hope the answers above provide the information needed. Cheers Abdellatif
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu,
On Fri, Mar 08, 2024 at 09:44:26AM -0700, Mathieu Poirier wrote:
> On Thu, 7 Mar 2024 at 12:40, Abdellatif El Khlifi
> wrote:
> >
> > Hi Mathieu,
> >
> > > > + do {
> > > > + state_reg = readl(priv->reset_cfg.state_reg);
> > > > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > > > +
> > > > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > > > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > > > + *rst_ack);
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + /* expected ACK value read */
> > > > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
> > >
> > > I'm not sure why the second condition in this if() statement is needed.
> > > As far
> > > as I can tell the first condition will trigger and the second one won't be
> > > reached.
> >
> > The second condition takes care of the following: exp_ack and *rst_ack are
> > both 0.
> > This case happens when RST_REQ bit is cleared (meaning: No reset requested)
> > and
> > we expect the RST_ACK to be 00 afterwards.
> >
>
> This is the kind of conditions that definitely deserve documentation.
> Please split the conditions in two different if() statements and add a
> comment to explain what is going on.
Thanks, I'll address that.
>
> > > > +/**
> > > > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > > > + * @rproc: pointer to the remote processor object
> > > > + * @fw: pointer to the firmware
> > > > + *
> > > > + * Does nothing currently.
> > > > + *
> > > > + * Return:
> > > > + *
> > > > + * 0 for success.
> > > > + */
> > > > +static int arm_rproc_load(struct rproc *rproc, const struct firmware
> > > > *fw)
> > > > +{
> > >
> > > What is the point of doing rproc_of_parse_firmware() if the firmware
> > > image is
> > > not loaded to memory? Does the remote processor have some kind of
> > > default ROM
> > > image to run if it doesn't find anything in memory?
> >
> > Yes, the remote processor has a default FW image already loaded by default.
> >
>
> That too would have mandated a comment - otherwise people looking at
> the code are left wondering, as I did.
>
> > rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in
> > the filesystem or a filename
> > provided.
> >
> > Please correct me if I'm wrong.
>
> You are correct, the remoteproc subsystem expects a firmware image to
> be provided _and_ loaded into memory. Providing a dummy image just to
> get the remote processor booted is a hack, but simply because the
> subsystem isn't tailored to handle this use case. So I am left
> wondering what the plans are for this driver, i.e is this a real
> scenario that needs to be addressed or just an initial patchset to get
> a foundation for the driver.
>
> In the former case we need to start talking about refactoring the
> subsystem so that it properly handles remote processors that don't
> need a firmware image. In the latter case I'd rather see a patchset
> where the firmware image is loaded into RAM.
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.
I'm happy to provide more explanation in the commit log to reflect this status.
Is it OK that we go with step 1 as a foundation please ?
Cheers
Abdellatif
Re: [PATCH 2/3] arm64: dts: Add corstone1000 external system device node
Hi Sudeep,
> > + 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.
>
> [1]
> https://developer.arm.com/documentation/101418/0100/Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
The Host Base System Control registers are not specific to the External System
processors. They are various registers with different purposes.
Only 4 registers matter for the remoteproc feature:
- The External system 0 reset control and status registers:
EXT_SYS0_RST_CTRL, EXT_SYS0_RST_ST
- Same for the the External system 1: EXT_SYS1_RST_CTRL, EXT_SYS1_RST_ST
So, mapping the whole Host Base System Control area doesn't make sense for the
remoteproc feature
and exposes registers that are not related to the External Systems to the
driver.
By the way, the latest document we are referring to is [1].
[1]:
https://developer.arm.com/documentation/102342//Programmers-model/Register-descriptions/Host-Base-System-Control-register-summary
Cheers,
Abdellatif
Re: [PATCH 3/3] dt-bindings: remoteproc: Add Arm remoteproc
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 ..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
Re: [PATCH 1/3] remoteproc: Add Arm remoteproc driver
Hi Mathieu,
> > + do {
> > + state_reg = readl(priv->reset_cfg.state_reg);
> > + *rst_ack = EXTSYS_RST_ST_RST_ACK(state_reg);
> > +
> > + if (*rst_ack == EXTSYS_RST_ACK_RESERVED) {
> > + dev_err(dev, "unexpected RST_ACK value: 0x%x\n",
> > + *rst_ack);
> > + return -EINVAL;
> > + }
> > +
> > + /* expected ACK value read */
> > + if ((*rst_ack & exp_ack) || (*rst_ack == exp_ack))
>
> I'm not sure why the second condition in this if() statement is needed. As
> far
> as I can tell the first condition will trigger and the second one won't be
> reached.
The second condition takes care of the following: exp_ack and *rst_ack are
both 0.
This case happens when RST_REQ bit is cleared (meaning: No reset requested) and
we expect the RST_ACK to be 00 afterwards.
> > +/**
> > + * arm_rproc_load() - Load firmware to memory function for rproc_ops
> > + * @rproc: pointer to the remote processor object
> > + * @fw: pointer to the firmware
> > + *
> > + * Does nothing currently.
> > + *
> > + * Return:
> > + *
> > + * 0 for success.
> > + */
> > +static int arm_rproc_load(struct rproc *rproc, const struct firmware *fw)
> > +{
>
> What is the point of doing rproc_of_parse_firmware() if the firmware image is
> not loaded to memory? Does the remote processor have some kind of default ROM
> image to run if it doesn't find anything in memory?
Yes, the remote processor has a default FW image already loaded by default.
rproc_boot() [1] and _request_firmware() [2] fail if there is no FW file in the
filesystem or a filename
provided.
Please correct me if I'm wrong.
[1]:
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/remoteproc/remoteproc_core.c#L1947
[2]:
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/base/firmware_loader/main.c#L863
> > +module_platform_driver(arm_rproc_driver);
> > +
>
> I am echoing Krzysztof view about how generic this driver name is. This has
> to
> be related to a family of processors or be made less generic in some way.
> Have
> a look at what TI did for their K3 lineup [1] - I would like to see the same
> thing done here.
Thank you, I'll take care of that and of all the other comments made.
Cheers,
Abdellatif
