On 23/05/2022 23:38, Tanmay Shah wrote: > Thanks for reviews Krzysztof. Please find my comments below. > > On 5/21/22 8:12 AM, Krzysztof Kozlowski wrote: >> On 18/05/2022 21:44, Tanmay Shah wrote: >>> +description: | >>> + The Xilinx platforms include a pair of Cortex-R5F processors (RPU) for >>> + real-time processing based on the Cortex-R5F processor core from ARM. >>> + The Cortex-R5F processor implements the Arm v7-R architecture and includes a >>> + floating-point unit that implements the Arm VFPv3 instruction set. >>> + >>> +properties: >>> + compatible: >>> + const: xlnx,zynqmp-r5fss >>> + >>> + xlnx,cluster-mode: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1, 2] >>> + description: | >>> + The RPU MPCore can operate in split mode(Dual-processor performance), Safety >>> + lock-step mode(Both RPU cores execute the same code in lock-step, >>> + clock-for-clock) or Single CPU mode (RPU core 0 can be held in reset while >>> + core 1 runs normally). The processor does not support dynamic configuration. >>> + Switching between modes is only permitted immediately after a processor reset. >>> + If set to 1 then lockstep mode and if 0 then split mode. >>> + If set to 2 then single CPU mode. When not defined, default will be lockstep mode. >>> + >>> +patternProperties: >>> + "^r5f-[a-f0-9]+$": >>> + type: object >>> + description: | >>> + The RPU is located in the Low Power Domain of the Processor Subsystem. >>> + Each processor includes separate L1 instruction and data caches and >>> + tightly coupled memories (TCM). System memory is cacheable, but the TCM >>> + memory space is non-cacheable. >>> + >>> + Each RPU contains one 64KB memory and two 32KB memories that >>> + are accessed via the TCM A and B port interfaces, for a total of 128KB >>> + per processor. In lock-step mode, the processor has access to 256KB of >>> + TCM memory. >>> + >>> + properties: >>> + compatible: >>> + const: xlnx,zynqmp-r5f >>> + >>> + power-domains: >>> + description: RPU core PM domain specifier >>> + maxItems: 1 >>> + >>> + mboxes: >>> + items: >>> + - description: mailbox channel to send data to RPU >>> + - description: mailbox channel to receive data from RPU >>> + minItems: 1 >>> + >>> + mbox-names: >>> + items: >>> + - const: tx >>> + - const: rx >>> + minItems: 1 >>> + >>> + sram: >>> + $ref: /schemas/types.yaml#/definitions/phandle-array >>> + minItems: 1 >> maxItems instead > > > Here, I am not sure how many maxItems are really needed as TCM bindings > are not > defined yet. For now, I will just keep maxItems as 8. i.e. 4 OCM banks > and 4 TCM > banks. However, that can change once bindings are defined. > Is that fine? Yes, although shrinking might not be allowed once binding is being used. > > >> >>> + description: | >>> + phandles to one or more reserved on-chip SRAM regions. Other than TCM, >>> + the RPU can execute instructions and access data from, the OCM memory, >>> + the main DDR memory, and other system memories. >>> + >>> + The regions should be defined as child nodes of the respective SRAM >>> + node, and should be defined as per the generic bindings in, >>> + Documentation/devicetree/bindings/sram/sram.yaml >>> + >>> + memory-region: >>> + description: | >>> + List of phandles to the reserved memory regions associated with the >>> + remoteproc device. This is variable and describes the memories shared with >>> + the remote processor (e.g. remoteproc firmware and carveouts, rpmsg >>> + vrings, ...). This reserved memory region will be allocated on DDR memory. >>> + minItems: 1 >>> + items: >>> + - description: region used for RPU firmware image section >>> + - description: vdev buffer >>> + - description: vring0 >>> + - description: vring1 >>> + additionalItems: true >> How did this one appear here? It does not look correct, so why do you >> need it? > > > Memory regions listed in items: field here are used for default current > OpenAMP demos. However, > other demos can be developed by user that can use more number of memory > regions. > As description says, memory-region can have variable number phandles > based on > user requirement. So, by additionalItems I just want to notify that user can > define more number of regions. We can limit memory-regions with > 'maxItems: 8'. > In that case, I will add 'maxItems:' field in next revision and even, > that can change in future. That sounds fine. > But, User should have flexibility to define more memory regions than > what is in list > of 'items:' field. I think this is similar to what is defined in > ti,k3-r5 bindings. > > Please let me know your thoughts. I see. If schema accepts such combination (listing items + maxItems + additionalItems), then it's fine. > Best regards, Krzysztof
