On 23/09/2024 19:19, Abdellatif El Khlifi wrote: >>> >>> The Host Base System Control [3] is the big block containing various functionalities (MMIO registers). >>> Among the functionalities, the two remote cores registers (aka External system 0 and 1). >>> The remote cores have two registers each. >>> >>> 1/ In the v1 patchset, a valid point was made by the community: >>> >>> 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 >> >> ARM is not special, neither this hardware is. Therefore: >> 1. Each register as reg: nope, for obvious reasons. >> 2. One device for entire syscon: quite common, why do you think it is >> somehow odd? >> 3. If you quote other person, please provide the lore link, so I won't >> spend useless 5 minutes to find who said that or if it was even said... > > Please have a look at this lore link [1]. The idea is to add syscon > and regmap support which I did in the v2 patchset. > > [1]: https://lore.kernel.org/all/ZfMVcQsmgQUXXcef@bogus/ There is nothing there about DT bindings. We do not talk here about drivers. MFD and regmap are Linux driver stuff, not bindings. I said nothing about not using MFD, regmap or whatever driver stuff you want. We talk *only* about bindings. Syscon is mentioned there but I am sorry - that quite a stretch to call a block syscon just because you want regmap. > >> >>> 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. >>> >>> The Exernal system registers are 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. >>> >>> 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 don't understand how this lengthy semi-quote answers my concerns. >> Please write concise points as arguments to my questions. >> >> For example, I don't care what your remote proc driver does and it >> should not matter in the terms of this binding. > > I just wanted to show why we are using syscon based on the arguments > of other reviewers. > >> >>> >>> 2/ There are many examples in the kernel that use syscon as a parent node of >>> child nodes for devices located at an offset from the syscon base address. >>> Please see these two examples [1][2]. I'm trying to follow a similar design if that >>> makes sense. >> >> Yeah, for separate devices. If you have two words without any resources, >> I claim you might not have here any separate devices or "not separate >> enough", because all this is somehow fluid. Remote core sounds like >> separate device, but all your arguments about need of extid which cannot >> be used in reg does not support this case. >> >> The example in the binding is also not complete - missing rest of >> devices - which does not help. > > Here I would like to explain the current suggestion and suggest an alternative solution. > > 1/ For more clarity, here is a complete example of use of both remote cores > at the same time: > > syscon@1a010000 { > compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon"; > reg = <0x1a010000 0x1000>; > > #address-cells = <1>; > #size-cells = <1>; > > remoteproc@310 { > compatible = "arm,sse710-extsys0"; > reg = <0x310 8>; > firmware-name = "es0_flashfw.elf"; > mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>; > mbox-names = "txes0", "rxes0"; > memory-region = <&extsys0_vring0>, <&extsys0_vring1>; > }; > > remoteproc@318 { > compatible = "arm,sse710-extsys1"; > reg = <0x318 8>; > firmware-name = "es1_flashfw.elf"; > mboxes = <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>; > mbox-names = "txes0", "rxes0"; > memory-region = <&extsys1_vring0>, <&extsys1_vring1>; > }; > }; > > Here we have 2 cores, each one have 2 registers mapped respectively > at 0x1a010310 and 0x1a010318. All this looks fine, resources are indeed reasonable, except that I still do not understand why do you need to call them 0 and 1 (now via compatible). Your driver code shows this nicely - it is entirely redundant! The 'reg' - so the base - is already there! You just duplicate it with the extsys_id, instead of relying on the base. So think what is the point of 'reg' property if you do not use it? > > Definetly these cores have seperate HW resources associated with them > which are the MHUs (mailboxes HW). There are 2 pairs of MHUs associated > with each core. These mailbox peripherals are obviously seperate. > Furthermore, the vring buffers used for communication are seperate. > > Moreover, the remote cores are independent. They are not SMP cores of one processor. > > They can have different default firmware to use. In this example es0_flashfw.elf > and es1_flashfw.elf > > The current HW implementation (Corstone-1000) provides one remote core only. > However, the second core control registers are at 0x1a010318 do exist. > > Support for a second core is taken into consideration in this work to help > end users who want to add a second core to their HW. > > 2/ Here I'm suggesting an alternative solution by using one remoteproc node for both cores as > follows: > > syscon@1a010000 { > compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon"; > reg = <0x1a010000 0x1000>; > > remoteproc { > compatible = "arm,sse710-extsys"; > firmware-name = "es0_flashfw.elf"; > mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>, <&mhu0_hes1 0 1>, <&mhu0_es1h 0 1>; > mbox-names = "txes0", "rxes0", "txes1", "rxes1"; > memory-region = <&extsys0_vring0>, <&extsys0_vring1>, <&extsys1_vring0>, <&extsys1_vring1>; > }; > }; > > Does this meet your expectations please ? > >> >>> >>> 3/ Since there are two registers for each remote core. I'm suggesting to set the >>> size in the reg property to 8. >> >> How is this related? >> >>> The driver will read the match data to get the right >>> offset to be used with regmap APIs. >> >> Sorry, no talks about driver. Don't care, at least in this context. >> >> You can completely omit address space from children in DT and everything >> will work fine and look fine from bindings point of view. >> >>> >>> Suggested nodes: >>> >>> >>> syscon@1a010000 { >>> compatible = "arm,sse710-host-base-sysctrl", "simple-mfd", "syscon"; >>> reg = <0x1a010000 0x1000>; >>> >>> #address-cells = <1>; >>> #size-cells = <1>; >>> >>> remoteproc@310 { >>> compatible = "arm,sse710-extsys0"; >>> reg = <0x310 8>; >>> firmware-name = "es_flashfw.elf"; >>> mboxes = <&mhu0_hes0 0 1>, <&mhu0_es0h 0 1>; >>> mbox-names = "txes0", "rxes0"; >>> memory-region = <&extsys0_vring0>, <&extsys0_vring1>; >>> }; >>> >>> remoteproc@318 { >>> compatible = "arm,sse710-extsys1"; >>> reg = <0x318 8>; >>> firmware-name = "es_flashfw.elf"; >> >> Same firmware? Always or only depends? > > The firmware of the second core depends on the end user choice. > Currently the second core is not implemented in the HW. > Users who want to tweak Corstone-1000 HW can add > a second core. Two cores make more sense in such case. Best regards, Krzysztof
