Message-ID: <158948864581.215346.7236327959062539884@swboyd.mtv.corp.google.com>
Date:   Thu, 14 May 2020 13:37:25 -0700
From:   Stephen Boyd <sboyd@...nel.org>
To:     Martin Blumenstingl <martin.blumenstingl@...glemail.com>,
        linux-clk@...r.kernel.org
Cc:     mturquette@...libre.com, linux-kernel@...r.kernel.org,
        jbrunet@...libre.com
Subject: Re: clk_hw.init and -EPROBE_DEFER / driver re-load

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;
}

Powered by blists - more mailing lists