{"action":"created","issue":{"url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/1553","repository_url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware","labels_url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/1553/labels{/name}","comments_url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/1553/comments","events_url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/1553/events","html_url":"https://github.com/ARM-software/arm-trusted-firmware/pull/1553","id":355731518,"node_id":"MDExOlB1bGxSZXF1ZXN0MjEyMTkzMTQ0","number":1553,"title":"Allow D-Cache to remain on during core power-down","user":{"login":"glneo","id":1456044,"node_id":"MDQ6VXNlcjE0NTYwNDQ=","avatar_url":"https://avatars3.githubusercontent.com/u/1456044?v=4","gravatar_id":"","url":"https://api.github.com/users/glneo","html_url":"https://github.com/glneo","followers_url":"https://api.github.com/users/glneo/followers","following_url":"https://api.github.com/users/glneo/following{/other_user}","gists_url":"https://api.github.com/users/glneo/gists{/gist_id}","starred_url":"https://api.github.com/users/glneo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/glneo/subscriptions","organizations_url":"https://api.github.com/users/glneo/orgs","repos_url":"https://api.github.com/users/glneo/repos","events_url":"https://api.github.com/users/glneo/events{/privacy}","received_events_url":"https://api.github.com/users/glneo/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2018-08-30T19:56:17Z","updated_at":"2018-10-12T13:11:51Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/pulls/1553","html_url":"https://github.com/ARM-software/arm-trusted-firmware/pull/1553","diff_url":"https://github.com/ARM-software/arm-trusted-firmware/pull/1553.diff","patch_url":"https://github.com/ARM-software/arm-trusted-firmware/pull/1553.patch"},"body":"Hello all,\r\n\r\nThe first two patches should be simple enough. The third is a bit more interesting and is kinda an RFC.\r\n\r\nFor K3, we have hardware assisted coherency but as I've argued before the flag \"HW_ASSISTED_COHERENCY\" is a misnomer and is targeted specifically for DynamIQ platforms. An example of this is in psci_do_pwrdown_sequence(), where we assume all platforms with the hardware assisted coherency flag will use a core's with a *_core_pwr_dwn function that will not disable the cache. This is not the case for K3 with A53 cores.\r\n\r\nWe introduce a new flag \"DISABLE_DCACHE_LATE\" designed as the inverse of \"WARMBOOT_ENABLE_DCACHE_EARLY\". It is for platforms that do not need (or cannot have due to hardware issues) cache disabled in the power down path.\r\n\r\nAs you can see we need to make a check for this flag in cortex_a53.S, is this the right thing to do? Should this check be added for all cores for consistency?\r\n\r\nThanks,\r\nAndrew"},"comment":{"url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/comments/429320522","html_url":"https://github.com/ARM-software/arm-trusted-firmware/pull/1553#issuecomment-429320522","issue_url":"https://api.github.com/repos/ARM-software/arm-trusted-firmware/issues/1553","id":429320522,"node_id":"MDEyOklzc3VlQ29tbWVudDQyOTMyMDUyMg==","user":{"login":"soby-mathew","id":6860836,"node_id":"MDQ6VXNlcjY4NjA4MzY=","avatar_url":"https://avatars3.githubusercontent.com/u/6860836?v=4","gravatar_id":"","url":"https://api.github.com/users/soby-mathew","html_url":"https://github.com/soby-mathew","followers_url":"https://api.github.com/users/soby-mathew/followers","following_url":"https://api.github.com/users/soby-mathew/following{/other_user}","gists_url":"https://api.github.com/users/soby-mathew/gists{/gist_id}","starred_url":"https://api.github.com/users/soby-mathew/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/soby-mathew/subscriptions","organizations_url":"https://api.github.com/users/soby-mathew/orgs","repos_url":"https://api.github.com/users/soby-mathew/repos","events_url":"https://api.github.com/users/soby-mathew/events{/privacy}","received_events_url":"https://api.github.com/users/soby-mathew/received_events","type":"User","site_admin":false},"created_at":"2018-10-12T13:11:51Z","updated_at":"2018-10-12T13:11:51Z","author_association":"CONTRIBUTOR","body":"Sorry for the delay as I had to spend some time to think about this problem.\r\n\r\n> Does the following invalidate act as a clean+invalidate?\r\n\r\nIt is not a clean invalidate which means if you are running with caches ON at that point, the cache line with the update is going to be invalidated.\r\n\r\n> Yes, but the regular Load-Store Exclusives do not work for us due to the lock being written with cache on and then data on the same cache-line as the lock being read with the cache off by the starting core.\r\n\r\nEnabling the `WARMBOOT_ENABLE_DCACHE_EARLY` flag would avoid the issue you mention. Although we could move the spin_lock to a separate cache line aligned data to solve this irregularity, this shouldn't affect you anymore since you set the option.\r\n\r\n>> The aff_info update would get invalidated for the above sequence in TI_K3.\r\n\r\n> Well it seems to work anyway, not sure how or why, but it does.\r\n\r\nI think the problem may trigger if stressed enough. Although if you enable HW_ASSISTED_COHERENCY flag, then the invalidate operation is not done. \r\n\r\nI am also not convinced about adding the `DISABLE_DCACHE_LATE` build flag to generic code and if HW_ASSISTED_COHERENCY build can do the right things for TI_K3, then that is ideal.\r\n\r\n>The only other fix I can think of is to put every single memory location accessed (write and read) after cache has been disabled into a non-cached section for all cores.\r\n\r\nIf you disable PSCI_STAT and RUNTIME_INSTRUMENTATION features, the only data that is accessed from generic code after caches are turned OFF are `psci_svc_cpu_data.aff_info_state` and the cpu/non-cpu power domain locks.\r\n\r\nAssuming you keep the caches ON, with HW_ASSISTED_COHERENCY=1, the power domain locks use spin_locks which are all accessed with caches ON which should be alright on TI_K3.\r\n\r\nYou could explicitly flush `psci_svc_cpu_data.aff_info_state` in `psci_plat_pm_ops->pwr_domain_pwr_down_wfi()`, but it would be redundant for CPU_SUSPEND which is fine.\r\n\r\nAny global data accessed within the platform plat_psci_ops_t `pwr_domain_suspend` and `pwr_domain_off` hooks need to be taken of.\r\n\r\nThe idea is to keep the caches ON throughout the power down sequence for TI_K3 and any global data which are updated after the caches flush in cpu_ops are handled explicitly by the platform. Since the caches are ON, all the cache maintenance ops can be NOP which the HW_ASSISTED_COHERENCY=1 does.\r\n\r\n> I would be fine with this, but how do I implement it? The core functions are chosen by the cores MIDR, so I would need to have the TI-53 have the same MIDR as the A53, which I can do if I do and that should work if I do not compile in the regular A53 file, but I need that so I can share the reset handler.\r\n\r\nA TI specific build flag could work as you suggest. But let us know the results after the above changes.\r\n\r\n\r\n\r\n\r\n\r\n\r\n"}}
{"actor":{"display_login":"soby-mathew"}}