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