Repository navigation
Update compatability string for remoteproc - #3575
Conversation
|
Fine for me, maybe consider keeping the old compatible as a fallback (i.e. but I don't feel strong about it and intend to change the binding in an incompatible way anyhow soon (which I think is needed for upstreaming), so there isn't much use on keeping compatibility anyhow. |
ukleinek
left a comment
There was a problem hiding this comment.
Apart from a minor concern, I agree that's the right way forward.
ecb53b4 to
3ab66e0
Compare
Have added, for now |
3ab66e0 to
5410ad5
Compare
|
@sipraga renamed to adi,adsp-sc598-rproc |
Long term we also want to upstream all of this and I guess the legacy stuff won't be taken into account anyways :) |
5410ad5 to
f439089
Compare
| static const struct of_device_id adi_rproc_of_match[] = { | ||
| { .compatible = "adi,remoteproc" }, | ||
| { .compatible = "adi,adsp-sc598-rproc" }, | ||
| { .compatible = "adi,adsp-sc846-rproc" }, |
There was a problem hiding this comment.
did you not want to already wire in .data pointers?
There was a problem hiding this comment.
He had but it but I asked to drop it. These pointers are useless if we're not going to do anything with them (which was the case). When we do need to do anything different per variant, then we can add them. But until them, just noise :)
There was a problem hiding this comment.
Fair enough, but I guess by the same logic this should also be dropped:
enum adi_rproc_variant {
SC5XX_RPROC_SHARC,
SC5XX_RPROC_SHARCFX,
};
struct adi_rproc_config {
unsigned int variant;
};
There was a problem hiding this comment.
It looks like the sc846 remoteproc PR will add those: #3499. I feel like we should drop it here and then let that PR add them once they are actually being used.
There was a problem hiding this comment.
I wanted to make it in small chunks rather as big PR. So all parts can be reviewed in fast manner.
There was a problem hiding this comment.
That PR now have 2 platforms, will split it in 2, SC846 - ELF support, and SC598 ELF support PR's first one is SC846
| scb { | ||
| core1-rproc@3108C000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc","adi,remoteproc"; |
There was a problem hiding this comment.
commit subject should be ARM: not arm64:
|
|
||
| core2-rproc@3108C000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc","adi,remoteproc"; |
There was a problem hiding this comment.
Generic compatibles are frowned upon. There is some advice here:
https://docs.kernel.org/devicetree/bindings/writing-bindings.html
Relevant rules:
- DO make ‘compatible’ properties specific.
- DON’T use wildcards or device-family names in compatible strings.
- DON’T add fake fallback compatibles when software cannot use such to match and bind to a device, and still operate correctly.
- DO use a SoC-specific compatible for all SoC devices, followed by a fallback if appropriate. SoC-specific compatibles are also preferred for the fallbacks.
Ergo, I would make this:
"adi,adsp-sc573-rproc","adi,adsp-sc598-remoteproc"
if 598 is to be the fallback compatible.
There was a problem hiding this comment.
Agree, "adi,remoteproc"; as a valid compatible is not the way to go
There was a problem hiding this comment.
not sure why to add "adi,adsp-sc573-rproc" as there is no any difference in code for for whole set of SHARC+ part's
something like adi,adsp-sp-rproc" for all SHARC+ remoteproc drivers.
| scb { | ||
| core1-rproc@3108C000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc","adi,remoteproc"; |
There was a problem hiding this comment.
We should add some DT bindings for these compatibles
There was a problem hiding this comment.
there was no compatibles by default
There was a problem hiding this comment.
as usually don't want to get out of scope in singe tiny PR, can always separate PR that add DT bindings
d7b236b to
c2fb2a6
Compare
Replace the generic "adi,remoteproc" compatible with "adi,sc5xx-rproc" and "adi,sc8xx-rproc", each carrying an adi_rproc_config that tags the SHARC variant (SHARC+ vs SHARCFX) so later patches can branch on the core type. Rename the platform driver to "adi-rproc" to match. Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
c2fb2a6 to
cfdc2b2
Compare
| scb { | ||
| core1-rproc@3108C000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc"; |
There was a problem hiding this comment.
Should we add a compatible string for the non-sc598 boards, e.g adi,adsp-sc573-rproc? This could be confusing to someone who doesn't know that the underlying architecture is the same. Otherwise LGTM
nunojsa
left a comment
There was a problem hiding this comment.
At some point we also need to start renaming the filenames
|
|
||
| core2-rproc@3108C000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc"; |
There was a problem hiding this comment.
Yeah... looks a bit weird to me. I guess we could very well already add adi,adsp-sc573-rproc. I guess the plan is still to upstream first 598 so we could make that as the fallback one
| &scb { | ||
| sharc0: core1-rproc@28240000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc846-rproc"; |
|
|
||
| core2-rproc@28a40000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc"; |
|
|
||
| sharc1: core2-rproc@28a40000 { | ||
| compatible = "adi,remoteproc"; | ||
| compatible = "adi,adsp-sc598-rproc"; |
|
Also, do not ignore comments without justifying :). @sipraga made the same points about compatibles and also git subject style :) |
|
looks fine now except I would still add the more SoC-specific compatibles, like I know in theory there is no difference, but when it comes to mainlining the DTs it will be required to do this. Rationale is that one day you might find that there is a subtle difference and you need to update the driver to account for it (do X if SC573, do Y if SC598). But because the driver has to work with "old" device trees (since DTs are kind of ABI), you want to future proof things by having that more SoC-specific compatible in place from the very beginning. The fallback compatible you choose (here sc598) is usually arbitrary. In fact I think the driver itself need only list that particular compatible in its OF match table if there is no actual difference in behaviour. The So in the bindings you do like: I put 846 separately because it's not actually compatible with 598. |
|
Since there isn't any bindings yaml file right now I think you can skip that part in this PR though, no harm done |
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
Signed-off-by: Arturs Artamonovs <Arturs.Artamonovs@analog.com>
2edfc9c to
0a9eb3d
Compare
sipraga
left a comment
There was a problem hiding this comment.
LGTM now!
also, I just noticed that the remoteproc nodes are in the som/board-level DTs rather than the SoC-level DT. seems a bit odd. probably needs to be moved at some point (but that's really for another day, out of scope here)
PR Description
This comes from WIP in #3499
As new platform support is added, SC8XX memory mapping's are incompatible with SC5XX, There is need to distinguish both of them.
The renaming follows stm32 remoteproc upstream
Compatibility string changed
adi,remoteprocnow is
This is first set of tiny PR's that shouldn't brake old code but introduce support for remoteproc on SC8XX
PR Type
PR Checklist