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

[Marek Vasut]

On 10/5/23 11:39, Paul Barker wrote:
> On 05/10/2023 03:24, Marek Vasut wrote:
>> 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've not measured this. I was just assuming that it is sensible to only do the setup once.
> 
>>
>>> 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.
> 
> I'll drop the pfc_enabled flag and re-test.

You can stick get_timer() before and after the code to get a rough idea 
of how long it took, likely it will be 0 .

Thanks