LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Stephen Boyd <sboyd@codeaurora.org>
To: Colin Cross <ccross@android.com>
Cc: linux-tegra@vger.kernel.org,
	Russell King <linux@arm.linux.org.uk>,
	konkers@android.com, linux-kernel@vger.kernel.org,
	olof@lixom.net, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 12/21] ARM: tegra: clock: Add shared bus clock type
Date: Wed, 16 Feb 2011 13:51:19 -0800	[thread overview]
Message-ID: <4D5C46D7.1040903@codeaurora.org> (raw)
In-Reply-To: <AANLkTi=knKY65xaOT85jFidy6Be8K8RT0L1XtGqq_v2V@mail.gmail.com>

On 02/16/2011 01:01 PM, Colin Cross wrote:
> On Wed, Feb 16, 2011 at 12:34 PM, Stephen Boyd <sboyd@codeaurora.org> wrote:
>>
>> What do you do if clk_set_rate() fails? Should you unwind all the state
>> such as the rate and if it's enabled/disabled? Or is it safe to say
>> clk_set_rate() can't fail unless the kernel is buggy in which case why
>> aren't all those return -E* in the set rate functions just BUG_ONs?
>
> In general, clk_set_rate can fail and return an error, but in this
> case the failure may not be directly related to the driver that called
> into tegra_clk_shared_bus_update.  For example, if clk_disable is
> called on a shared clock handle, the rate may drop to the rate
> requested by another shared clock handle.  clk_disable cannot fail, so
> there's nothing that could be done with the return code, and the
> problem was not caused by the driver that called clk_disable, so an
> error would be meaningless.
>
>
> I will modify tegra_clk_shared_bus_update to BUG on a failed
> clk_set_rate,

Yes, currently if there are any errors we can't determine which clock's
vote is causing the error since the rate only takes effect when the
clock is enabled and another clock could have updated their rate while
the list is being iterated over.

The code could be written so that we only call clk_set_rate() on the
parent when the calling clock affects the aggregate rate. That would
allow us to catch errors for the clk_enable() path and the
clk_set_rate() path when the clock is already on. In the clk_disable()
path we could just ignore the errors since we can't do anything anyways
like you say. The only path that doesn't seem possible is clk_set_rate()
when the clock is off which presumably doesn't actually matter since
when you turn the clock on it will fail the same way (delayed
clk_set_rate?).

BTW, is there a race there in the rate updating code? Say clock 1 is
enabled with rate 2 on cpu1 and clock 2 is enabled at the same time with
rate 3 (currently the greatest rate) on cpu2. clock 1 is iterating over
the list and sees that clock 2 is enabled so it calculates 3 as the max.
clock 2 then returns from the enable call and then a call to disable
clock 2 comes in. clock 1 is still iterating over the list and clock 2's
call to disable runs to completion. clock 1 finally stops iterating over
the list and has an aggregated rate of 3 (since it saw that clock 2 was
on which is no longer true). It then calls set_rate() with 3 even though
the only clock that is on is clock 1 with a rate of 2.

c1 and c2 are off initially

CPU1                    CPU2
----                    -----
clk_set_rate(c1, 2)     clk_set_rate(c2, 3)
clk_enable(c1)          clk_enable(c2)
max == 3                max == 3; clk_set_rate(parent, 3)
...                     clk_disable(c2)
                        max == 2; clk_set_rate(parent, 2)
clk_set_rate(parent, 3)

I think you need some kind of lock while iterating to stop the shared
clocks from changing underneath you.

> and modify tegra_clk_shared_bus_set_rate to
> call clk_round_rate on the parent to ensure that the requested rate is valid.
>

I would hope clk_round_rate() isn't necessary to get a valid rate.
clk_set_rate() shouldn't require exact/valid rates. clk_round_rate() is
there to help drivers determine if calling clk_set_rate() with a certain
rate is going to give them something they want. It's like saying "If I
call clk_set_rate() with 500Hz what would the clock's rate actually be
after the call returns?" If the set_rate implementation needs to round
internally to find a divider or something, it should be done in the
set_rate code and not in each driver.

>>
>> Shouldn't you call clk_enable(c->parent)? And do you need to check for
>> errors from clk_enable()?
>
> clk_enable on the parent is handled by the clock op implementation in
> mach-tegra/clock.c
>

Oops, thanks. Time to visit the optometrist.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.


  reply	other threads:[~2011-02-16 21:51 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1297590033-15035-1-git-send-email-ccross@android.com>
2011-02-13  9:40 ` [PATCH 01/21] ARM: tegra: clock: enable clk reset for non-peripheral clocks Colin Cross
2011-02-13  9:40 ` [PATCH 02/21] ARM: tegra: clock: Don't BUG on changing an enabled PLL Colin Cross
2011-02-13  9:40 ` [PATCH 03/21] ARM: tegra: clock: Drop debugging Colin Cross
2011-02-13  9:40 ` [PATCH 04/21] ARM: tegra: clock: Don't use PLL lock bits Colin Cross
2011-02-13  9:40 ` [PATCH 05/21] ARM: tegra: clock: Disable clocks left on by bootloader Colin Cross
2011-02-13  9:40 ` [PATCH 06/21] ARM: tegra: clock: Initialize clocks that have no enable Colin Cross
2011-02-13  9:40 ` [PATCH 07/21] ARM: tegra: clock: Drop CPU dvfs Colin Cross
2011-02-13  9:40 ` [PATCH 08/21] ARM: tegra: clock: Rearrange static clock tables Colin Cross
2011-02-13  9:40 ` [PATCH 09/21] ARM: tegra: clock: Move unshared clk struct members into union Colin Cross
2011-02-13  9:40 ` [PATCH 10/21] ARM: tegra: clock: Convert global lock to a lock per clock Colin Cross
2011-02-13  9:40 ` [PATCH 11/21] ARM: tegra: cpufreq: Take an extra reference to pllx Colin Cross
2011-02-13  9:40 ` [PATCH 12/21] ARM: tegra: clock: Add shared bus clock type Colin Cross
2011-02-16 20:34   ` Stephen Boyd
2011-02-16 21:01     ` Colin Cross
2011-02-16 21:51       ` Stephen Boyd [this message]
2011-02-16 22:03         ` Colin Cross
2011-02-13  9:40 ` [PATCH 13/21] ARM: tegra: clock: Remove unnecessary uses of #ifdef CONFIG_DEBUG_FS Colin Cross
2011-02-13  9:40 ` [PATCH 14/21] ARM: tegra: clock: Refcount periph clock enables Colin Cross
2011-02-13  9:40 ` [PATCH 15/21] ARM: tegra: clock: Round rate before setting rate Colin Cross
2011-02-13  9:40 ` [PATCH 16/21] ARM: tegra: Add external memory controller driver Colin Cross
2011-02-13  9:40 ` [PATCH 17/21] ARM: tegra: clocks: Add emc scaling Colin Cross
2011-02-13  9:40 ` [PATCH 18/21] ARM: tegra: cpufreq: Adjust memory frequency with cpu frequency Colin Cross
2011-02-13  9:40 ` [PATCH 19/21] ARM: tegra: clock: Add function to set SDMMC tap delay Colin Cross
2011-02-13  9:40 ` [PATCH 20/21] ARM: tegra: clock: Fix clock issues in suspend Colin Cross
2011-02-13  9:40 ` [PATCH 21/21] ARM: tegra: clock: Miscellaneous clock updates Colin Cross

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4D5C46D7.1040903@codeaurora.org \
    --to=sboyd@codeaurora.org \
    --cc=ccross@android.com \
    --cc=konkers@android.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=olof@lixom.net \
    --subject='Re: [PATCH 12/21] ARM: tegra: clock: Add shared bus clock type' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).