Re: [PATCH v4] clk: tegra: Add BPMP clock driver

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 5426 bytes --]

On Thu, Nov 17, 2016 at 10:58:40AM +0100, Thierry Reding wrote:
> On Wed, Nov 16, 2016 at 05:19:15PM -0800, Stephen Boyd wrote:
> > On 11/15, Thierry Reding wrote:
[...]
> > > +static int
> > > +__tegra_bpmp_clk_transfer_atomic(struct tegra_bpmp *bpmp,
> > > +				 struct tegra_bpmp_message *msg)
> > > +{
> > > +	unsigned long flags;
> > > +	int err;
> > > +
> > > +	local_irq_save(flags);
> > > +	err = tegra_bpmp_transfer_atomic(bpmp, msg);
> > > +	local_irq_restore(flags);
> > 
> > Why do we need to disable irqs? Seems like an odd thing for the
> > caller to decide given that there aren't any interrupt handlers
> > in this driver.
> 
> This is there to allow clock operations to succeed very early in boot
> when some of the infrastructure to execute the non-atomic equivalents
> hasn't been brought up yet.
> 
> That said, I'm not sure we actually need it. I'll run some tests and can
> drop this if we don't need it.

Turns out that we don't need this anymore. The need for this must have
gone away somewhere during restructuring of the code.

> > > +static int tegra_bpmp_clk_enable(struct clk_hw *hw)
> > 
> > Maybe this should be called tegra_bpmp_clk_prepare() so as to not
> > confuse the reader.
> 
> Well, the function does implement an enable of the clock. The reason why
> it is later assigned to the ->prepare() operation is merely because that
> is what the CCF requires, given that this may sleep.
> 
> Bus if you prefer I can change this and also...
> 
> > > +{
> > > +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> > > +	struct tegra_bpmp_clk_message msg;
> > > +
> > > +	memset(&msg, 0, sizeof(msg));
> > > +	msg.cmd = CMD_CLK_ENABLE;
> > > +	msg.clk = clk->id;
> > > +
> > > +	return tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> > > +}
> > > +
> > > +static void tegra_bpmp_clk_disable(struct clk_hw *hw)
> 
> ... this to tegra_bpmp_clk_unprepare().
> 
> > > +{
> > > +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> > > +	struct tegra_bpmp_clk_message msg;
> > > +	int err;
> > > +
> > > +	memset(&msg, 0, sizeof(msg));
> > > +	msg.cmd = CMD_CLK_DISABLE;
> > > +	msg.clk = clk->id;
> > > +
> > > +	err = tegra_bpmp_clk_transfer(clk->bpmp, &msg);
> > > +	if (err < 0)
> > > +		dev_err(clk->bpmp->dev, "failed to disable clock %s: %d\n",
> > > +			clk_hw_get_name(hw), err);
> > > +}
> > > +
> > > +static int tegra_bpmp_clk_is_enabled(struct clk_hw *hw)
> > 
> > Why not *_is_prepared()?
> 
> Yes, given the above that makes sense.
> 
> > > +{
> > > +	struct tegra_bpmp_clk *clk = to_tegra_bpmp_clk(hw);
> > > +	struct cmd_clk_is_enabled_response response;
> > > +	struct tegra_bpmp_clk_message msg;
> > > +	int err;
> > > +
> > > +	memset(&msg, 0, sizeof(msg));
> > > +	msg.cmd = CMD_CLK_IS_ENABLED;
> > > +	msg.clk = clk->id;
> > > +	msg.rx.data = &response;
> > > +	msg.rx.size = sizeof(response);
> > > +
> > > +	err = tegra_bpmp_clk_transfer_atomic(clk->bpmp, &msg);
> > 
> > And then I presume this wouldn't need to worry about being called
> > in atomic situations.
> 
> I'll go test if that works. If it does, I agree that this would be
> preferable.

Turns out this works out fine. This sent me on a wild goose chase for a
while because after the conversion the kernel would simply hang at some
point. This turned out to be caused by the UART's clock getting
unprepared because it wasn't properly wired up in the DT. With the
original code I wasn't seeing this because the mix of ->is_enabled()
with ->prepare() and ->unprepared() would cause all of the unused clocks
to remain on across clk_disable_unused().

Perhaps this would be worth sanity checking somewhere during clock
registration? Specifically, providing ->is_enabled() but not ->enable()
or ->disable() or ->is_prepared() but not ->prepare() or ->unprepare()
seems like a bad idea.

> > > +static struct clk_hw *tegra_bpmp_clk_of_xlate(struct of_phandle_args *clkspec,
> > > +					      void *data)
> > > +{
> > > +	unsigned int id = clkspec->args[0], i;
> > > +	struct tegra_bpmp *bpmp = data;
> > > +	struct tegra_bpmp_clk *clk;
> > > +
> > > +	for (i = 0; i < bpmp->num_clocks; i++) {
> > > +		clk = to_tegra_bpmp_clk(bpmp->clocks[i]);
> > > +		if (clk->id == id)
> > > +			return bpmp->clocks[i];
> > > +	}
> > 
> >  	for (i = 0; i < bpmp->num_clocks; i++) {
> > 		hw = bpmp->clocks[i];
> > 		clk = to_tegra_bpmp_clk(hw);
> > 		if (clk->id == id)
> > 			return hw;
> > 	}
> > 
> > Uses another local variable but is much clearer.
> 
> Really? I think it's pretty clear in the original that we upcast,
> compare and return the original pointer. Even if bpmp->clocks[i] is a
> little longer than hw, the two occurrences are close enough together to
> make this immediately obvious.
> 
> Maybe yet another alternative would be to make bpmp->clocks an array to
> struct tegra_bpmp_clk instead of struct clk_hw. That way the ->xlate()
> becomes simpler to handle at the expense of a slightly more complicated
> implementation for tegra_bpmp_unregister_clocks(). Given that
> unregistering clocks is a one-time operation but ->xlate() may be called
> very often, this might be advantageous.

So I implemented this alternative and it ends up looking better than
either of the options above, at least in my opinion.

I've sent a v2, let me know what you think.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]