Message-ID: <fe7c0524-cbe9-4b02-bfec-5c5b5491cf17@linaro.org>
Date: Thu, 11 Jan 2024 10:48:06 +0100
From: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To: Philipp Zabel <p.zabel@...gutronix.de>,
 Srinivas Kandagatla <srinivas.kandagatla@...aro.org>,
 Banajit Goswami <bgoswami@...cinc.com>,
 Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konrad.dybcio@...aro.org>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Rob Herring <robh+dt@...nel.org>,
 Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
 Conor Dooley <conor+dt@...nel.org>, Jaroslav Kysela <perex@...ex.cz>,
 Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
 linux-arm-msm@...r.kernel.org, linux-sound@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Cc: Bartosz Golaszewski <brgl@...ev.pl>,
 Sean Anderson <sean.anderson@...o.com>
Subject: Re: [PATCH v2 2/4] reset: Instantiate reset GPIO controller for
 shared reset-gpios

On 09/01/2024 12:58, Philipp Zabel wrote:
>>>> +	/* Not freed in normal path, persisent subsyst data */
>>>> +	rgpio_dev = kzalloc(sizeof(*rgpio_dev), GFP_KERNEL);
>>>
>>> Since this is persistent, instead of letting the reset-gpio driver call
>>> of_parse_phandle_with_args() again, this could be passed in via
>>> platform data. Is there a reason not to do that instead?
>>
>> We can pass it as read only platform data, but we cannot pass the
>> ownership. This is associated with registered platform device, not with
>> bound one device->driver.
>>
>> Imagine case:
>> 1. modprobe reset-gpio,
>> 2. Driver is bound to the device,
>> 3. unbind  (echo > unbind)
>> 4. rmmod
>> 5. goto 1
> 
> Keeping ownership on the list is fine, the reset-gpio driver makes its
> own copy of of_phandle_args anyway. I was just wondering whether it
> could make this copy from platform data instead of from the
> of_parse_phandle_with_args() return value.

Looks like it could. This could save us few lines of code in
reset-gpio.c. I'll try it.

> 
> [...]
>>>
>>>> @@ -839,21 +960,50 @@ __of_reset_control_get(struct device_node *node, const char *id, int index,
>>>>  					 index, &args);
>>>>  	if (ret == -EINVAL)
>>>>  		return ERR_PTR(ret);
>>>> -	if (ret)
>>>> -		return optional ? NULL : ERR_PTR(ret);
>>>> +	if (ret) {
>>>> +		/*
>>>> +		 * There can be only one reset-gpio for regular devices, so
>>>> +		 * don't bother with GPIO index.
>>>> +		 */
>>>
>>> I don't understand this comment. The GPIO index should be checked as
>>> part of __reset_gpios_args_match(), or should it not?
>>
>> This and earlier comment are result of a bit hacky approach to the
>> problem: how to find reset controllers for that GPIO?
>>
>> The point is that our reset gpio controller has only 1 reset, thus
>> of_reset_n_cells=1. However args_count from of_parse_handle is >0, which
>> later is compared in reset core:
>>
>> https://elixir.bootlin.com/linux/latest/source/drivers/reset/core.c#L859
>>
>> That part we need to match.
>>
>> I could make the reset-gpio driver to have of_reset_n_cells=2, but what
>> would be the point? The rest of the cells are not really relevant,
>> because you cannot refer to this reset gpio controller with any other
>> arguments.
>>
>> To remind: my solution spawns one reset-gpio controller for one GPIO.
> 
> Thank you. I think we could also just make that check
> 
> 	if (WARN_ON(!rcdev->of_args && ...))
> 
> instead and skip the of_xlate call in that case (or implement of_xlate
> in the reset-gpio driver to make this more explicit).

Ack

> 
>>>
>>>> +		ret = of_parse_phandle_with_args(node, "reset-gpios", "#gpio-cells",
>>>> +						 0, &args);
>>>> +		if (ret)
>>>> +			return optional ? NULL : ERR_PTR(ret);
>>>>  
>>>> -	mutex_lock(&reset_list_mutex);
>>>> -	rcdev = NULL;
>>>> -	list_for_each_entry(r, &reset_controller_list, list) {
>>>> -		if (args.np == r->of_node) {
>>>> -			rcdev = r;
>>>> -			break;
>>>> -		}
>>>> +		gpio_fallback = true;
>>>
>>> Is there a reason not just call __reset_add_reset_gpio_device() here?
>>> With that, there should be no need to call __reset_find_rcdev() twice.
>>
>> Hm, could be, although not sure if code would be simpler.
>>
>> This entire function handles two cases:
>> 1. Get normal reset controller ("resets" OF property),
>> 2. If above fails, get reset-gpio controller ("reset-gpios" OF property)
>>
>> Therefore the entire solution is following approach:
>> 1. of_parse_phandle(resets)
>> 1b. error? Then of_parse_phandle(reset-gpios)
>> 2. Find reset-controller based on any of above phandles.
>> 3. error? Check if we created reset-gpios platform device. If not:
>> create new reset-gpios platform device/
>> 3b. Platform device could probe, so lookup again for reset controller or
>> defer probe.
>>
>> What type of flow do you propose?
> 
> I propose to reorder after parsing the phandles: check/create the gpio
> platform device right after parsing the gpio handle. Only then lock
> reset_list_mutex look for the rcdev.
> 
> 1. of_parse_phandle(resets)
> 1b. error? Then of_parse_phandle(reset-gpios)
> 2b. gpio? Then check if we created reset-gpios platform device. If not:
> create new reset-gpios platform device/, defer if probe failed
> 3. Lock reset_list_mutex, find reset-controller based on any of above
> phandles.

Could work, let me try. I have impression this was my first approach
which resulted in a bit more complicated code, but I don't remember the
details now.

> 
>>
>>>
>>>>  	}
>>>>  
>>>> +	mutex_lock(&reset_list_mutex);
>>>> +	rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);
>>>
>>> This gets called with args as parsed. If there is a match, this will 
>>> overwrite args (in the gpio_fallback case) and return NULL.
>>
>> Overwrite not complete. It will only overwrite args_count and return a
>> valid rcdev.
>> I do not see overwriting in case of returning NULL.
> 
> Sorry, I meant to write
> 
> "This gets called with args as parsed. If there is a match, this will 
>  overwrite args (in the gpio_fallback case) _or_ return NULL."
> 
> at least at the end, when I understood the following.
> 
>>>
>>>> +
>>>>  	if (!rcdev) {
> 
> So in this non-NULL branch there was no overwriting.
> 
>>>> -		rstc = ERR_PTR(-EPROBE_DEFER);
>>>> -		goto out;
>>>> +		if (gpio_fallback) {
>>>> +			/*
>>>> +			 * Registering reset-gpio device might cause immediate
>>>> +			 * bind, thus taking reset_list_mutex lock via
>>>> +			 * reset_controller_register().
>>>> +			 */
>>>> +			mutex_unlock(&reset_list_mutex);
>>>> +			ret = __reset_add_reset_gpio_device(node, &args);
>>>
>>> So this will also be called with args as parsed.
>>>
>>>> +			mutex_lock(&reset_list_mutex);
>>>> +			if (ret) {
>>>> +				rstc = ERR_PTR(ret);
>>>> +				goto out;
>>>> +			}
>>>> +			/*
>>>> +			 * Success: reset-gpio could probe immediately, so
>>>> +			 * re-check the lookup.
>>>> +			 */
>>>> +			rcdev = __reset_find_rcdev(&args, gpio_fallback, NULL);
>>>
>>> And this will again be called with args as parsed and overwrite args
>>> again.>
>>>> +			if (!rcdev) {
>>>> +				rstc = ERR_PTR(-EPROBE_DEFER);
>>>> +				goto out;
>>>> +			}
>>>> +			/* Success, rcdev is valid thus do not bail out */
>>>> +		} else {
>>>> +			rstc = ERR_PTR(-EPROBE_DEFER);
>>>> +			goto out;
>>>> +		}
>>>>  	}
>>>
>>> So at this point args is overwritten in the gpio_fallback case. I would
>>> find it much clearer to just overwrite args here and make the first
>>> parameter to __reset_find_rcdev() const.
>>
>> I think I get your point. Overwriting happens after we store the
>> original of_args, but the code is indeed not intuitive. I think I can
>> move it further, as you suggested.
> 
> Now I think we can skip the overwriting altogether and just adapt the
> following of_reset_n_cells check ad of_xlate call as mentioned above.


Yep!

Best regards,
Krzysztof

Powered by blists - more mailing lists