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

[Paul Barker]

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.

Thanks,
Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_0x27F4B3459F002257.asc
Type: application/pgp-keys
Size: 3520 bytes
Desc: OpenPGP public key
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231005/9a8ba78c/attachment.key>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 236 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20231005/9a8ba78c/attachment.sig>