Re: [PATCH v2 2/6] firmware: ti_sci: Partial-IO support

[Krzysztof Kozlowski <krzk@kernel.org>]

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