| Message ID | 1408096034-17270-1-git-send-email-mperttunen@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
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. Luckily, we don't have any SPI-based chips that do anything related to clocks on any of our current boards... Aside from this issue, the patch looks fine to me. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > 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. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 08:07:01PM +0200, Stephen Warren wrote: > >> 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. > Does it? I would assume the slave only needs to check if the address matches its own address after a START condition and if not can just wait until the STOP condition appears on the bus? > 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. > Looking at http://www.i2c-bus.org/highspeed/ they at least claim some form of backwards compatibility ('High-speed IC devices are downward compatible allowing for mixed bus systems. ') > >> 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. Yes, that's another usecase, but we don't have that on Tegra. I was refering to Tegra usecases here. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 09:45:46PM +0200, Peter De Schrijver wrote: > On Fri, Aug 15, 2014 at 08:07:01PM +0200, Stephen Warren wrote: > > >> 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. > > > > Does it? I would assume the slave only needs to check if the address matches > its own address after a START condition and if not can just wait until the > STOP condition appears on the bus? > http://www.nxp.com/documents/user_manual/UM10204.pdf says you can mix them by using an interconnect bridge between the highspeed and the non-highspeed capable slaves. The bridge uses the special preamble to disconnect the non- highspeed part of the bus when a highspeed transaction is ongoing. It's afaics transparent to the master. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/15/2014 03:34 PM, Peter De Schrijver wrote: > On Fri, Aug 15, 2014 at 09:45:46PM +0200, Peter De Schrijver wrote: >> On Fri, Aug 15, 2014 at 08:07:01PM +0200, Stephen Warren wrote: >>>>> 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. >>> >> >> Does it? I would assume the slave only needs to check if the address matches >> its own address after a START condition and if not can just wait until the >> STOP condition appears on the bus? >> > > http://www.nxp.com/documents/user_manual/UM10204.pdf says you can mix them by > using an interconnect bridge between the highspeed and the non-highspeed > capable slaves. The bridge uses the special preamble to disconnect the non- > highspeed part of the bus when a highspeed transaction is ongoing. It's afaics > transparent to the master. I expect that works by echoing the slow-speed pre-amble to the slow-speed bus segment, then emitting a stop and turning off the echo. For actual slow-speed transactions, the whole thing would be echo'd. That way the slow-speed devices don't ever see any high-speed pulses. That all said, that does indeed imply that a master supporting the high-speed transactions would need to emit a varying-speed signal. My assumption would be that this happens inside the I2C HW, rather than under SW control though, since the transition would need to happen mid-protocol. Still, perhaps the selection between low-speed and high-speed-with-a-slow-preamble might need SW clock programming depending on the HW though... Who knows. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 11:46:49PM +0200, Stephen Warren wrote: > On 08/15/2014 03:34 PM, Peter De Schrijver wrote: > > On Fri, Aug 15, 2014 at 09:45:46PM +0200, Peter De Schrijver wrote: > >> On Fri, Aug 15, 2014 at 08:07:01PM +0200, Stephen Warren wrote: > >>>>> 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. > >>> > >> > >> Does it? I would assume the slave only needs to check if the address matches > >> its own address after a START condition and if not can just wait until the > >> STOP condition appears on the bus? > >> > > > > http://www.nxp.com/documents/user_manual/UM10204.pdf says you can mix them by > > using an interconnect bridge between the highspeed and the non-highspeed > > capable slaves. The bridge uses the special preamble to disconnect the non- > > highspeed part of the bus when a highspeed transaction is ongoing. It's afaics > > transparent to the master. > > I expect that works by echoing the slow-speed pre-amble to the > slow-speed bus segment, then emitting a stop and turning off the echo. > For actual slow-speed transactions, the whole thing would be echo'd. > That way the slow-speed devices don't ever see any high-speed pulses. > Indeed. > That all said, that does indeed imply that a master supporting the > high-speed transactions would need to emit a varying-speed signal. My > assumption would be that this happens inside the I2C HW, rather than > under SW control though, since the transition would need to happen > mid-protocol. Still, perhaps the selection between low-speed and > high-speed-with-a-slow-preamble might need SW clock programming > depending on the HW though... Who knows. > That's true if the master wants to do a high-speed transaction. If the master wants to do a normal-speed transaction to a slave on the same bus, the master will need to select a lower speed clock under software control I think. Cheers, Peter. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 15, 2014 at 10:18:15AM -0600, 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. Luckily, we don't > have any SPI-based chips that do anything related to clocks on any of our > current boards... > > Aside from this issue, the patch looks fine to me. May I count this as an Ack?
On 09/02/2014 05:56 AM, Wolfram Sang wrote: > On Fri, Aug 15, 2014 at 10:18:15AM -0600, 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. Luckily, we don't >> have any SPI-based chips that do anything related to clocks on any of our >> current boards... >> >> Aside from this issue, the patch looks fine to me. > > May I count this as an Ack? Sure, once the afore-mentioned issue is resolved. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index 87d0371..251f172 100644 --- 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); if (ret < 0) { dev_err(i2c_dev->dev, "Enabling fast clk failed, err %d\n", ret); return ret; } } - ret = clk_prepare_enable(i2c_dev->div_clk); + ret = clk_enable(i2c_dev->div_clk); if (ret < 0) { dev_err(i2c_dev->dev, "Enabling div clk failed, err %d\n", ret); - clk_disable_unprepare(i2c_dev->fast_clk); + clk_disable(i2c_dev->fast_clk); } return ret; } static inline void tegra_i2c_clock_disable(struct tegra_i2c_dev *i2c_dev) { - clk_disable_unprepare(i2c_dev->div_clk); + clk_disable(i2c_dev->div_clk); if (!i2c_dev->hw->has_single_clk_source) - clk_disable_unprepare(i2c_dev->fast_clk); + clk_disable(i2c_dev->fast_clk); } static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev) { u32 val; int err = 0; - int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE; u32 clk_divisor; err = tegra_i2c_clock_enable(i2c_dev); @@ -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); - /* Make sure clock divisor programmed correctly */ clk_divisor = i2c_dev->hw->clk_divisor_hs_mode; clk_divisor |= i2c_dev->hw->clk_divisor_std_fast_mode << @@ -712,6 +708,7 @@ static int tegra_i2c_probe(struct platform_device *pdev) void __iomem *base; int irq; int ret = 0; + int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE; res = platform_get_resource(pdev, IORESOURCE_MEM, 0); base = devm_ioremap_resource(&pdev->dev, res); @@ -777,17 +774,39 @@ static int tegra_i2c_probe(struct platform_device *pdev) platform_set_drvdata(pdev, i2c_dev); + 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; + } + ret = tegra_i2c_init(i2c_dev); if (ret) { dev_err(&pdev->dev, "Failed to initialize i2c controller"); - return ret; + goto unprepare_div_clk; } ret = devm_request_irq(&pdev->dev, i2c_dev->irq, tegra_i2c_isr, 0, dev_name(&pdev->dev), i2c_dev); if (ret) { dev_err(&pdev->dev, "Failed to request irq %i\n", i2c_dev->irq); - return ret; + goto unprepare_div_clk; } i2c_set_adapdata(&i2c_dev->adapter, i2c_dev); @@ -803,16 +822,30 @@ static int tegra_i2c_probe(struct platform_device *pdev) ret = i2c_add_numbered_adapter(&i2c_dev->adapter); if (ret) { dev_err(&pdev->dev, "Failed to add I2C adapter\n"); - return ret; + goto unprepare_div_clk; } return 0; + +unprepare_div_clk: + clk_unprepare(i2c_dev->div_clk); + +unprepare_fast_clk: + if (!i2c_dev->hw->has_single_clk_source) + clk_unprepare(i2c_dev->fast_clk); + + return ret; } static int tegra_i2c_remove(struct platform_device *pdev) { struct tegra_i2c_dev *i2c_dev = platform_get_drvdata(pdev); i2c_del_adapter(&i2c_dev->adapter); + + clk_unprepare(i2c_dev->div_clk); + if (!i2c_dev->hw->has_single_clk_source) + clk_unprepare(i2c_dev->fast_clk); + return 0; }
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. Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com> --- drivers/i2c/busses/i2c-tegra.c | 57 +++++++++++++++++++++++++++++++++--------- 1 file changed, 45 insertions(+), 12 deletions(-)