clk_hw.init and -EPROBE_DEFER / driver re-load

clk_hw.init and -EPROBE_DEFER / driver re-load

[Martin Blumenstingl]

Hello Stephen et al.

I am working on a driver for the "SDHC" controller on older Amlogic Meson SoCs.
This has some clock controller bits built into.
Using CCF for the whole rate management makes things a lot easier, so
I decided to use that instead of poking register bits manually.
The code can be seen here: [0] - but don't take it too seriously
because I have to move some of that in the next patch revision.

I solved an "interesting" problem where I'm not sure if it works as
intended (which is what I assumed) or if there's an actual problem (as
suggested by Jerome).

The flow in the driver is basically:
(the clk_hws are defined as "static" variables inside the driver)
1) fetch related OF properties
2) initialize and register the built-in (into the MMC controller
driver) clock controller part
3) initialize the MMC controller

Step 3) can fail for example because one of the regulators is not ready.
In this case I'm de-registering the clock controller part again.
Finally the driver returns -EPROBE_DEFER
(so far everything is working fine)

However, once the regulator (in this example) becomes ready the the
same flow as above is being executed.
Only this time the clock controller registration fails because
hw->init is NULL (before it had the values which I defined in the
driver).
This is due to the following line in __clk_register():
  hw->init = NULL;

My way to "solve" this is to clone the clk_hws while registering the
clock controller.
Unfortunately this means that I can't easily use clk_hw references to
describe the parent/child relation between these clocks.

I'm not sure if my way of defining the clk_hws is wrong, there's a bug
in CCF, this is a new feature request or something completely
different :-)
My motivation is to understand how I should to consider this behavior
for my next version of the MMC controller patches.

Any feedback is welcome!


Thank you,
Martin


[0] https://patchwork.kernel.org/patch/11463357/

Re: clk_hw.init and -EPROBE_DEFER / driver re-load

[Stephen Boyd]

Quoting Martin Blumenstingl (2020-04-27 12:57:33)
> Hello Stephen et al.
> 
> I am working on a driver for the "SDHC" controller on older Amlogic Meson SoCs.
> This has some clock controller bits built into.
> Using CCF for the whole rate management makes things a lot easier, so
> I decided to use that instead of poking register bits manually.
> The code can be seen here: [0] - but don't take it too seriously
> because I have to move some of that in the next patch revision.
> 
> I solved an "interesting" problem where I'm not sure if it works as
> intended (which is what I assumed) or if there's an actual problem (as
> suggested by Jerome).
> 
> The flow in the driver is basically:
> (the clk_hws are defined as "static" variables inside the driver)
> 1) fetch related OF properties
> 2) initialize and register the built-in (into the MMC controller
> driver) clock controller part
> 3) initialize the MMC controller
> 
> Step 3) can fail for example because one of the regulators is not ready.
> In this case I'm de-registering the clock controller part again.
> Finally the driver returns -EPROBE_DEFER
> (so far everything is working fine)
> 
> However, once the regulator (in this example) becomes ready the the
> same flow as above is being executed.
> Only this time the clock controller registration fails because
> hw->init is NULL (before it had the values which I defined in the
> driver).
> This is due to the following line in __clk_register():
>   hw->init = NULL;
> 
> My way to "solve" this is to clone the clk_hws while registering the
> clock controller.
> Unfortunately this means that I can't easily use clk_hw references to
> describe the parent/child relation between these clocks.
> 
> I'm not sure if my way of defining the clk_hws is wrong, there's a bug
> in CCF, this is a new feature request or something completely
> different :-)
> My motivation is to understand how I should to consider this behavior
> for my next version of the MMC controller patches.
> 

Maybe the patch to make hw->init be NULL is just not useful and we
should revert it. The intent was to catch misbehaving drivers that tried
to use the init data after registration but then it causes problems like
we see here where drivers have static clk data mixed with static init
data that they unregister later and expect to be able to register again.

This is one of the failed design decisions we made early on to associate
the init data with the clk_hw struct instead of passing it as another
argument to clk_register().

Maybe a better workaround would be to always assign hw->init in the
driver before registering the clks? Basically make a clk registration
API that we should have made before.

int my_clk_register(struct clk_hw *hw, struct clk_init_data *init)
{
	hw->init = init;
	ret = clk_hw_register(dev, hw);
	...
}

Do you have some sort of array of clk_hw pointers to register? Maybe we
could make a new clk registration structure to simplify this for users,
but I'm not super interested in introducing yet another registration
API! :/

struct clk_hw_desc {
	struct clk_hw *hw;
	struct clk_init_data *init;
};

and then drivers can call clk_hw_register_bulk() or something like this:

int clk_hw_register_bulk(struct device *dev, const struct clk_hw_desc **descs, size_t num_descs)
{
	int ret;
	int i;

	for (i = 0; i < num_descs; i++) {
		ret = some_internal_clk_register_func(dev, descs[i]->hw, descs[i]->init);
		if (ret)
			goto error;
	}

	return 0;
error:
	while (--i >= 0)
		clk_hw_unregister(...)

	return ret;
}

Re: clk_hw.init and -EPROBE_DEFER / driver re-load

[Martin Blumenstingl]

Hi Stephen,

On Thu, May 14, 2020 at 10:37 PM Stephen Boyd <sboyd@kernel.org> wrote:
[...]
> Do you have some sort of array of clk_hw pointers to register? Maybe we
> could make a new clk registration structure to simplify this for users,
> but I'm not super interested in introducing yet another registration
> API! :/
>
> struct clk_hw_desc {
>         struct clk_hw *hw;
>         struct clk_init_data *init;
> };
I could make an array of clk_hw pointers but I think it will make
things more complicated then they have to be.
In another version of my MMC controller patches I had a dedicated
array of clk_init_data: [0]

compare this with the latest version (which has made it to linux-next
yesterday) where I'm initializing all clocks inside a few functions
(meson_mx_sdhc_register_clkc, rather than a static table): [1]
it's not great because it means I can't use a loop to register
everything. but it's fairly easy to read so I think it's the best I
can do for now.

With the "solution" from [1] I also don't run into the issue which I
described in my original mail because now clk_init_data only exists on
the stack.
I'm not sure if this is a recurring pattern or not, but for now I feel
like a new API is not needed.


Thank you for your time,
Martin


[0] https://patchwork.kernel.org/patch/11515631/
[1] https://patchwork.kernel.org/patch/11543939/