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
OpenPGP_0x27F4B3459F002257.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
