Hi Krzysztof, Thanks for the review. Please find my response inline. On 30/11/22 20:33, Krzysztof Kozlowski wrote: > On 30/11/2022 14:40, Devarsh Thakkar wrote: >> AM62 family of devices don't have a R5F cluster, instead >> they have single core DM R5F. >> Add new compatible string ti,am62-r5fss to support this scenario. >> >> When this new compatible is used don't allow cluster-mode >> property usage in device-tree as this implies that there >> is no R5F cluster available and only single R5F core >> is present. >> >> Signed-off-by: Devarsh Thakkar <devarsht@xxxxxx> >> --- >> V2: Avoid acronyms, use "Device Manager" instead of "DM" > > Use subject prefixes matching the subsystem (git log --oneline -- ...). Agreed, I will update the prefix as dt-bindings: remoteproc: k3-r5f: in V3. > >> --- >> .../bindings/remoteproc/ti,k3-r5f-rproc.yaml | 48 +++++++++++++------ >> 1 file changed, 34 insertions(+), 14 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >> index fb9605f0655b..91357635025a 100644 >> --- a/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >> +++ b/Documentation/devicetree/bindings/remoteproc/ti,k3-r5f-rproc.yaml >> @@ -21,6 +21,9 @@ description: | >> called "Single-CPU" mode, where only Core0 is used, but with ability to use >> Core1's TCMs as well. >> >> + AM62 SoC family support a single R5F core only which runs Device Manager >> + firmware and can also be used as a remote processor with IPC communication. >> + >> Each Dual-Core R5F sub-system is represented as a single DTS node >> representing the cluster, with a pair of child DT nodes representing >> the individual R5F cores. Each node has a number of required or optional >> @@ -28,6 +31,9 @@ description: | >> the device management of the remote processor and to communicate with the >> remote processor. >> >> + Since AM62 SoC family only support a single core, there is no cluster-mode >> + property setting required for it. >> + >> properties: >> $nodename: >> pattern: "^r5fss(@.*)?" >> @@ -38,6 +44,7 @@ properties: >> - ti,j721e-r5fss >> - ti,j7200-r5fss >> - ti,am64-r5fss >> + - ti,am62-r5fss > > Some order? Alphabetical, so before am64? Same in other places. Agreed, I will update in V3 accordingly. > > >> - ti,j721s2-r5fss >> >> power-domains: >> @@ -80,7 +87,8 @@ patternProperties: >> node representing a TI instantiation of the Arm Cortex R5F core. There >> are some specific integration differences for the IP like the usage of >> a Region Address Translator (RAT) for translating the larger SoC bus >> - addresses into a 32-bit address space for the processor. >> + addresses into a 32-bit address space for the processor. For AM62x, >> + should only define one R5F child node as it has only one core available. >> >> Each R5F core has an associated 64 KB of Tightly-Coupled Memory (TCM) >> internal memories split between two banks - TCMA and TCMB (further >> @@ -104,6 +112,7 @@ patternProperties: >> - ti,j721e-r5f >> - ti,j7200-r5f >> - ti,am64-r5f >> + - ti,am62-r5f >> - ti,j721s2-r5f >> >> reg: >> @@ -207,20 +216,31 @@ patternProperties: >> - firmware-name >> >> unevaluatedProperties: false > > Blank line. Agreed, I will remove it in V3. > >> +allOf: >> + - if: >> + properties: >> + compatible: >> + enum: >> + - ti,am64-r5fss >> + then: >> + properties: >> + ti,cluster-mode: >> + enum: [0, 2] >> + >> + else: >> + properties: >> + ti,cluster-mode: > > It's not really valid anymore for ti,am62-r5fss, so this cannot be > simple "else". Instead you need to list all compatibles. I agree that the else block is not valid for am62x, but my understanding is that since all the blocks under allOf are checked for validity, I thought to add a separate if block only for am62x to set cluster-mode to false [1], which I believe would negate the effect of above else condition for am62x, so that we don't have to list all compatibles under separate if blocks. Just to verify this, I deliberately set cluster-mode=1 in am62x devicetree and then ran a dtbs-check and got below log : "linux-next/arch/arm64/boot/dts/ti/k3-am625-sk.dtb: r5fss@78000000: ti,cluster-mode: False schema does not allow [[1]]" and above warning log goes away when i remove the cluster-mode node in am62x devicetree. But please do let me know if I am missing something here or there is a better/more proper way to do this. Best Regards, Devarsh > >> + enum: [0, 1] >> + [1] >> + - if: >> + properties: >> + compatible: >> + enum: >> + - ti,am62-r5fss >> + then: >> + properties: >> + ti,cluster-mode: false >> >> -if: >> - properties: >> - compatible: >> - enum: >> - - ti,am64-r5fss >> -then: >> - properties: >> - ti,cluster-mode: >> - enum: [0, 2] >> -else: >> - properties: >> - ti,cluster-mode: >> - enum: [0, 1] >> >> required: >> - compatible > > Best regards, > Krzysztof >
