Re: [PATCH] i2c: i2c-tegra: Move clk_prepare/clk_set_rate to probe

From: Stephen Warren
Date: Fri Aug 15 2014 - 14:06:50 EST


On 08/15/2014 12:02 PM, Peter De Schrijver wrote:
On Fri, Aug 15, 2014 at 06:18:15PM +0200, Stephen Warren wrote:
On 08/15/2014 03:47 AM, Mikko Perttunen wrote:
Currently the i2c-tegra bus driver prepares, enables
and set_rates its clocks separately for each transfer.
This causes locking problems when doing I2C transfers
from clock notifiers; see
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-July/268653.html

This patch moves clk_prepare/unprepare and clk_set_rate calls to
the probe function, leaving only clk_enable/disable to be
done on each transfer. This solves the locking issue.

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c

@@ -380,34 +380,33 @@ static inline int tegra_i2c_clock_enable(struct tegra_i2c_dev *i2c_dev)
{
int ret;
if (!i2c_dev->hw->has_single_clk_source) {
- ret = clk_prepare_enable(i2c_dev->fast_clk);
+ ret = clk_enable(i2c_dev->fast_clk);

Here, both the prepare and enable wrap just the I2C transfer, ...

@@ -428,9 +427,6 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
i2c_writel(i2c_dev, val, I2C_CNFG);
i2c_writel(i2c_dev, 0, I2C_INT_MASK);

- clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
- clk_set_rate(i2c_dev->div_clk, i2c_dev->bus_clk_rate * clk_multiplier);

... whereas the rate is set up when the controller is initialized, i.e.
much earlier.

@@ -777,17 +774,39 @@ static int tegra_i2c_probe(struct platform_device *pdev)

+ if (!i2c_dev->hw->has_single_clk_source) {
+ ret = clk_prepare(i2c_dev->fast_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+ return ret;
+ }
+ }
+
+ ret = clk_prepare(i2c_dev->div_clk);
+ if (ret < 0) {
+ dev_err(i2c_dev->dev, "Clock prepare failed %d\n", ret);
+ goto unprepare_fast_clk;
+ }
+
+ clk_multiplier *= (i2c_dev->hw->clk_divisor_std_fast_mode + 1);
+ ret = clk_set_rate(i2c_dev->div_clk,
+ i2c_dev->bus_clk_rate * clk_multiplier);
+ if (ret) {
+ dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
+ goto unprepare_div_clk;
+ }

However, the new code sets the clock rate after the clock is prepared. I
think the rate should be set first, then the clock prepared. While this
likely doesn't apply to the Tegra clock controller, prepare() is allowed
to enable the clock if enable() can't be implemented in an atomic
fashion (in which case enable/disable would be no-ops), and we should
make sure that the driver correctly configures the clock before
potentially enabling it.

I'm not sure if a similar change to our SPI drivers is possible; after
all, the SPI transfer rate can vary per message, so if clk_set_rate()
acquires a lock, it seems there's no way to avoid the issue there.

Even for i2c this could be the case I think if you use the highspeed (3.4Mhz)
mode? From what I remember, a highspeed i2c transaction starts with a lower
speed preamble to make sure non highspeed slaves don't get confused? Which
means you could change the bus speed depending on the slave you're addressing.

Since there's no separate chip-select for I2C, I believe all I2C devices need to be able to understand the entire transaction, so the I2C bus speed is fixed.

At least, that's my understanding between 100KHz and 400KHz I2C. I don't know if 3.4MHz I2C introduced something new, although considering that slower I2C never had anything about being compatible with fast stuff in the spec AFAIK, and such speed-switching would only be useful for backwards-compatibility, I don't see how that would work.

Luckily, we don't have any SPI-based chips that do anything related to
clocks on any of our current boards...

And we don't use SPI to talk to the PMIC, which is the usecase were actually
run into problems with the locking.

IIRC, the I2C-based clock provider (or consumer?) issue was something mentioned (later on?) in the email thread linked by the patch description.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/