[PATCH 09/16] pinctrl: renesas: Add RZ/G2L PFC driver

[Marek Vasut]

On 10/4/23 21:43, Paul Barker wrote:

[...]

>>>>> +/*
>>>>> + * We need to ensure that the module clock is enabled and all resets are
>>>>> + * de-asserted before using either the gpio or pinctrl functionality. Error
>>>>> + * handling can be quite simple here as if the PFC cannot be enabled then we
>>>>> + * will not be able to progress with the boot anyway.
>>>>> + */
>>>>> +static int rzg2l_pfc_enable(struct udevice *dev)
>>>>> +{
>>>>> +	struct rzg2l_pfc_data *data =
>>>>> +		(struct rzg2l_pfc_data *)dev_get_driver_data(dev);
>>>>> +	struct reset_ctl_bulk rsts;
>>>>> +	struct clk clk;
>>>>> +	int ret;
>>>>> +
>>>>> +	if (data->pfc_enabled)
>>>>
>>>> When does this get triggered ?
>>>
>>> This is initialised to false in rzg2l_pfc_bind(), then this function
>>> rzg2l_pfc_enable() sets it to true before a successful return. The
>>> effect is that the PFC is enabled just once, regardless of whether the
>>> pinctrl or gpio driver is probed first.
>>
>> Why would be call to rzg2l_pfc_enable() a problem in the first place ?
>> It just grabs and enables clock and ungates reset, the second time this is
>> called the impact on harware should be no-op , right ?
> 
> The hw impact is a no-op, but it wastes time unnecessarily re-reading
> data from the fdt and repeating the setup, e.g. in rzg2l_cpg_clk_set()
> we have to search the array of clocks each time to find the requested
> entry.

Does getting clock and enabling them have noticable overhead on this 
platform ? Look at CONFIG_OF_LIVE, that should mitigate the DT access 
overhead at least.

> I think it's worth keeping the conditional here but can drop it if
> you're really against it.

It feels like fixing a problem at the wrong place really.