-
Notifications
You must be signed in to change notification settings - Fork 919
dt-bindings: soc: adi: Add SC5XX SoC component bindings #3033
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: adsp-6.12.0-y
Are you sure you want to change the base?
Conversation
Documentation/devicetree/bindings/soc/adi/adi,pads-system-config.yaml
Outdated
Show resolved
Hide resolved
Documentation/devicetree/bindings/soc/adi/adi,system-event-controller.yaml
Outdated
Show resolved
Hide resolved
|
@artursartamonovsadi don't worry about the builds succeeding but please take a look at the "checks": https://github.com/analogdevicesinc/linux/actions/runs/19863355918/job/56919188243?pr=3033 |
|
device-tree binding checks are passing |
| - Arturs Artamonovs <arturs.artamonovs@analog.com> | ||
| - Utsav Agarwal <Utsav.Agarwal@analog.com> | ||
|
|
||
| description: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In practice, no need for pre-formatting
| ... | ||
| adi,system-config = <&pads_system_config>; | ||
| ... | ||
| }; No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note (it seems this is not the case). The preferred style for indentation in the DT example are 4 spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Im checking other yaml DT examples all of them have 2 spaces, that why I followed that style
|
|
||
| adi,sharc-max: | ||
| $ref: /schemas/types.yaml#/definitions/uint32 | ||
| description: "Maximum valid SHARC core ID/count" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no constrains? Can we have the full unsigned int range?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will limit it to 2 cores
| reg: | ||
| maxItems: 1 | ||
|
|
||
| "adi,rcu": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why quoting?
|
|
||
| "adi,sharc-cores": | ||
| $ref: /schemas/types.yaml#/definitions/uint32 | ||
| description: "Number of SHARC cores available" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment about constrains. I guess we have a limited number of cores 😉
| "adi,tru-slave-id": | ||
| description: Trigger Routing Slave ID | ||
| minItems: 1 | ||
| maxItems: 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you only need minItems: 1. Then maxItems defaults to 1...
| oneOf: | ||
| - items: | ||
| - enum: | ||
| - adi,pads-system-config |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Keep the original please
| oneOf: | ||
| - items: | ||
| - enum: | ||
| - adi,system-event-controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
| adi,max-slave-id = <187>; | ||
| }; | ||
| &tru { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh I see. The fixup commits are not very helpful for reviewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I merge them then and just to have single commit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, fixup commits are helpful for development, but should be squashed before review and merge. A PR really shouldn't be approved with fixup commits. And the DCO workflow also doesn't seem to handle it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The re-review button in the Reviewers section at the top left of the PR is also super helpful after you've made a bunch of updates to notify folks
|
We discussed during the meeting yesterday, but it would be good to convert the txt to yaml. Can you also update MAINTAINERS appropriately? |
Agreed! It's work that will be needed anyways so better to do it now. |
Hi, I prefer the scope of PR to stay within devicetree bindings, there is no maintainers defined in this adsp-main-6.12 branches, so far. But MAINTAINERS defined in upstreaming patch series. Maybe better to open separate PR, with complete MAINTAINERS list, rather in single PR we can try to fix all the issues. |
e70e148 to
17ae7c8
Compare
|
@nunojsa I have left one "|" in adi,rpmsg-SC598.yaml description as if I remove it I get this error from dt binding check: |
91c267a to
ffd6b12
Compare
That's odd! I would suspect there's more into it. AFAIK, there is no mandatory usage for |
nunojsa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some more comments from me. As @pamolloy, I would expect a commit per binding
| emac0: ethernet@31040000 { | ||
| reg = <0x31040000 0x2000>; | ||
| adi,system-config = <&pads_system_config>; | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation still looks wrong... Are you using 4 spaces? I think 2 spaces is also acceptable but IIRC, 4 is the preferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I followed other yaml DT examples all of them have 2 spaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I said, IIRC 2 spaces are acceptable but nor the preferred:
| adi,sharc-max: | ||
| $ref: /schemas/types.yaml#/definitions/uint32 | ||
| description: Maximum valid SHARC core ID/count | ||
| maximum: 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm the above two is odd. Why not a simple adi,sharc-count? Or just the adi,sharc-max. IOW, why do we need both properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All cores have their ID's from system perspective. Like In case of SC598 Core0/ARM, Core1/SHARC, Core2/SHARC,
adi,sharc-min will be 1, and adi-sharc-max will be 2. For SC589 it will be Core0/ARM, Core1/SHARC adi,sharc-min 1, adi-sharc-max 2. For something like SC596 and SC570 there will be just 1 SHARC Core. so adi,sharc-min and adi,sharc-max will be 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will not fit well when core id's assigned in other order or not in continues Core id's
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I see, Still two properties seems wrong to me and hard to understand. What about something like adi,sharc-core-ids? And the property is an array of minimum 1 and maximum of what? Not sure what's the max number of shark cores we can have today :). And example:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will fix in yaml file and do new PR for fix within a driver
| adi,sharc-min: | ||
| $ref: /schemas/types.yaml#/definitions/uint32 | ||
| description: Minimum valid SHARC core ID/count | ||
| minimum: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no maximum in this property...
| - Utsav Agarwal <Utsav.Agarwal@analog.com> | ||
|
|
||
| description: | | ||
| Describes device tree binding for adi rpmsg driver |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bindings are not about drivers. It's about HW. Don't use description like the above or any driver details in bindings. Maintainers will complain about it
| 262144 | 0x00684000 | 0x08000000 | ||
| 524288 | 0x00d04000 | 0x10000000 | ||
| 1048576 | 0x01a04000 | 0x20000000 | ||
| 2097152 | 0x03404000 | 0x40000000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you're adding too much description in here. Just add a small device description
| reg: | ||
| maxItems: 1 | ||
|
|
||
| core-id: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a standard property? Otherwise it needs the adi vendor prefix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its sharc core id for rpmsg communication
| memory-region: | ||
| maxItems: 1 | ||
| description: | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the above both properties could drop |
Created MAINTAINERS entries for all other drivers #3045 |
557a43f to
e4fdef9
Compare
Document the PADS system configuration register binding. Provides voltage and endian selection controls for SC5XX peripheral drivers. Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Document the Reset Control Unit (RCU) binding for managing SHARC and ARM core start/stop/reset operations on SC5XX processors Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Document the RPMSG binding for ARM-SHARC inter-core communication on SC598 processors using TRU-based signaling and shared memory. Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
… SC5XX Document the System Event Controller (SEC) binding, which provides interrupt control for SHARC cores on SC5XX processors. Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
e4fdef9 to
1bc5ff6
Compare
Document the Trigger Routing Unit (TRU) binding for mapping trigger masters to slaves, enabling inter-core communication on SC5XX. Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
3ffb77b to
facfd8c
Compare
Document device tree bindings for ADI SC5XX SoC components including
PADS config, reset controller, system event controller, trigger routing
unit, and RPMSG support.
Ported files from 5.15.x lnxdsp-linux repo
PR Type
PR Checklist