PMP: initialization and experiments#545
PMP: initialization and experiments#545WhatAmISupposedToPutHere wants to merge 5 commits intoAsahiLinux:mainfrom
Conversation
|
|
||
| ADT_FOREACH_PROPERTY(adt, pmp_iop_anode, prop) | ||
| { | ||
| if (fdt_setprop(dt, tunables_node, prop->name, prop->value, prop->size)) |
There was a problem hiding this comment.
I suspect we want to filter this at least lightly. compatible = [iop-nub,rtbuddy-v2] and AAPL,phandle shouldn't be copied. There are further properties an OSS driver probably does not need.
I hope "these are the property names apple uses in the ADT" is a convincing argument during dt-bindings / driver review but don't really see a sane alternative.
There was a problem hiding this comment.
The annoying part is that firmware requests the property values from this node by name, so i don't even have a static list of properties that it needs. Can only treat them as an opaque key/value dict.
There was a problem hiding this comment.
does it request "soc-device" / "controller" / "lts-config"? looking at the larger properties.
I'd expected "region-", "segment-", "pre-loaded", "firmware-name", "dram-capacity", "coredump-enable" can be removed as well. Primarily looking at properties which could end up confusing
There was a problem hiding this comment.
the big three are requested, but the others on the list can be excluded
proxyclient/experiments/pmp_init.py
Outdated
| pio_base = u.adt["/arm-io/dart-pmp"].pio_vm_base | ||
| granularity = u.adt["/arm-io/dart-pmp"].pio_granularity | ||
| i = 0 | ||
| for (host_addr, size) in [(0x282000000, 0x1000000),(0x304000000, 0x1000000),(0x383000000, 0x1000000),(0x402000000, 0x1000000),(0x210e70000, 0x90000),(0x211e70000, 0x90000),(0x212e70000, 0x90000)]: |
There was a problem hiding this comment.
Are these address/size pairs observed by tracing? On which device? Can we derive them from the ADT?
There was a problem hiding this comment.
that is on a m1 pro, they are derived from adt (reg 4 and following, i have written them down in linux driver's dt)
There was a problem hiding this comment.
can we documents that this is only expected to run on t6000 (and maybe other t600x or t602x devices)? An compatibility check with early exit would be enough. Please mention it in the commit message as well.
973ac05 to
c2b0c93
Compare
Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
c2b0c93 to
128ade7
Compare
|
|
||
| ADT_FOREACH_PROPERTY(adt, pmp_iop_anode, prop) | ||
| { | ||
| if (fdt_setprop(dt, tunables_node, prop->name, prop->value, prop->size)) |
There was a problem hiding this comment.
does it request "soc-device" / "controller" / "lts-config"? looking at the larger properties.
I'd expected "region-", "segment-", "pre-loaded", "firmware-name", "dram-capacity", "coredump-enable" can be removed as well. Primarily looking at properties which could end up confusing
128ade7 to
5aecef7
Compare
jannau
left a comment
There was a problem hiding this comment.
The commits are missing Signed-off-by:
| bail("FDT: failed to transfer pmp tunable"); | ||
| next_adt_prop:; | ||
| } | ||
| return 0; |
There was a problem hiding this comment.
please update pmp_node's "status" to "okay". I will disable the pmp node in the linux device trees so we can merge the kernel bits before the next m1n1 release
There was a problem hiding this comment.
I just realized that this isn't possible due to the power-domains dependencies. This has to be handled in the driver. I don't remember if this is already handled.
There was a problem hiding this comment.
I think it should be fine. pmp itself will stay disabled, while the kernel harmlessly writes to pmp_reporting (a shared sram area). Just like macos does, we do not wait for pmp acknowledgements until it has booted
There was a problem hiding this comment.
yes, I think so too after looking at it. So I will disable pmp in the kernel DT.
5aecef7 to
5cf3eb3
Compare
Copy all the things that PMP needs in order to boot. Signed-off-by: Sasha Finkelstein <fnkl.kernel@gmail.com>
5cf3eb3 to
2b75771
Compare
| if (ADT_GETPROP(adt, pmp_iop_anode, "dram-capacity", &dram_capacity) < 0) | ||
| bail("ADT: failed to get dram capacity\n"); | ||
|
|
||
| int pmp_node = fdt_path_offset(dt, "pmp"); |
There was a problem hiding this comment.
please move this to the beginning of the function. The j504 (M3 macbook pro) does not have dram-capacity in /arm-io/pmp/iop-pmp-nub.
Checking the FDT first for pmp avoids to fail on new devices with different ADT properties.
| bail("ADT: failed to get board id\n"); | ||
| if (ADT_GETPROP(adt, chosen_anode, "dram-vendor-id", &dram_vendor_id) < 0) | ||
| bail("ADT: failed to get dram vendor id\n"); | ||
| if (ADT_GETPROP(adt, pmp_iop_anode, "dram-capacity", &dram_capacity) < 0) |
There was a problem hiding this comment.
can we use /chosen/dram-size instead?
There was a problem hiding this comment.
/chosen/dram-size from the ADT? Don't think so, whatever this field specifies, i haven't seen an obvious relationship with the system ram size, as both my 32gb and 8gb machines have it set to 8
There was a problem hiding this comment.
All device ADTs I could quickly grep have dram-capacity = 8. I just assumed it is the RAM size and only verified that dram-size matches the installed RAM. Leave it as it is then and we'll have to adjust once we support PMP on M3
The thingy that makes your mac use less power.