On 06/08/2024 09:19, Markus Schneider-Pargmann wrote:
> On Tue, Aug 06, 2024 at 08:26:00AM GMT, Krzysztof Kozlowski wrote:
>> On 29/07/2024 10:00, Markus Schneider-Pargmann wrote:
>>> +static int tisci_sys_off_handler(struct sys_off_data *data)
>>> +{
>>> + struct ti_sci_info *info = data->cb_data;
>>> + int i;
>>> + int ret;
>>> + bool enter_partial_io = false;
>>> +
>>> + for (i = 0; i != info->nr_wakeup_sources; ++i) {
>>> + struct platform_device *pdev =
>>> + of_find_device_by_node(info->wakeup_source_nodes[i]);
>>> +
>>> + if (!pdev)
>>> + continue;
>>> +
>>> + if (device_may_wakeup(&pdev->dev)) {
>>
>> ...
>>
>>> + dev_dbg(info->dev, "%pOFp identified as wakeup source\n",
>>> + info->wakeup_source_nodes[i]);
>>> + enter_partial_io = true;
>>> + }
>>> + }
>>> +
>>> + if (!enter_partial_io)
>>> + return NOTIFY_DONE;
>>> +
>>> + ret = tisci_enter_partial_io(info);
>>> +
>>> + if (ret) {
>>> + dev_err(info->dev,
>>> + "Failed to enter Partial-IO %pe, trying to do an emergency restart\n",
>>> + ERR_PTR(ret));
>>> + emergency_restart();
>>> + }
>>> +
>>> + while (1);
>>> +
>>> + return NOTIFY_DONE;
>>> +}
>>> +
>>> /* Description for K2G */
>>> static const struct ti_sci_desc ti_sci_pmmc_k2g_desc = {
>>> .default_host_id = 2,
>>> @@ -3398,6 +3485,35 @@ static int ti_sci_probe(struct platform_device *pdev)
>>> goto out;
>>> }
>>>
>>> + if (of_property_read_bool(dev->of_node, "ti,partial-io-wakeup-sources")) {
>>> + info->nr_wakeup_sources =
>>> + of_count_phandle_with_args(dev->of_node,
>>> + "ti,partial-io-wakeup-sources",
>>> + NULL);
>>
>> I don't see the point of this. You have quite a static list of devices -
>> just look at your DTS. Then you don't even do anything useful with the
>> phandles you got here.
>
> I am gathering the list of phandles in probe.
I know what the code is doing.
>
> Then during shutdown I am resolving the phandles to devices if there
> are associated devices and check if wakeup is enabled for these devices.
I can read the code.
> If wakeup is enabled for one of the devices in the list, I put the
> SoC into Partial-IO instead of a normal poweroff. This way the
> devices which have wakeup enabled can actually wakeup the SoC as
> requested by the user by setting wakeup enable.
I see all this. I said there is no point in doing this. Instead of
repeating the code, you can say what is the point of doing it.
I said once, so repeat one more time - you have *static* list of
devices, therefore any DTS is meaningless. It is just fixed, not flexible.
The code here is still not doing anything useful with the phandles. By
useful I mean - actually enable the wakeup. No, the property is totally
misplaced just to satisfy your code. That's a "no".
Best regards,
Krzysztof
