From: Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> Sent: Thursday, June 18, 2015 5:49 AM > To: Gao Pan-B54642 > Cc: wsa@xxxxxxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; Li Frank-B20596; Duan > Fugang-B38611 > Subject: Re: [Patch v1] i2c: imx: add runtime pm support to improve the > performance > > Hello, > > On Thu, Jun 11, 2015 at 09:50:04AM +0800, Gao Pan wrote: > > In our former i2c driver, i2c clk is enabled and disabled in xfer > > function, which contributes to power saving. However, the clk enable > > process brings a busy wait delay until the core is stable. As a > > result, the performance is sacrificed. > Which platform are you referring here? Looking at i.MX21 I cannot find a > delay for the i2c clock. I tested platforms are i.MX6q/i.MX7d that both use i.MX21 i2c IP. There just need one delay before disable i2c controller: static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) { ... if (is_imx1_i2c(i2c_imx)) { /* * This delay caused by an i.MXL hardware bug. * If no (or too short) delay, no "STOP" bit will be generated. */ udelay(i2c_imx->disable_delay); } ... /* Disable I2C controller */ temp = i2c_imx->hwdata->i2cr_ien_opcode ^ I2CR_IEN, imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR); } > > > To weigh the power consuption and i2c bus performance, runtime > consumption Thanks for your detail review, will change it in next version. > > pm is the good solution for it. The clk is enabled when a i2c transfer > > starts, and disabled afer a specifically defined delay. > after Thanks. > > [...] > > diff --git a/drivers/i2c/busses/i2c-imx.c > > b/drivers/i2c/busses/i2c-imx.c index 785aa67..cc4b5d6 100644 > > --- a/drivers/i2c/busses/i2c-imx.c > > +++ b/drivers/i2c/busses/i2c-imx.c > > [...] > > @@ -520,9 +523,6 @@ static int i2c_imx_start(struct imx_i2c_struct > > *i2c_imx) > > > > i2c_imx_set_clk(i2c_imx); > > > > - result = clk_prepare_enable(i2c_imx->clk); > > - if (result) > > - return result; > you remove clk_prepare_enable enable and instead rely on > clk_prepare_enable in i2c_imx_runtime_resume, right? > Yes, you are right. > If I understand correctly this results in never enabling the clock if > CONFIG_PM is disabled?! > Yes, you are right, I will change it in next version. > > [...] > > @@ -583,6 +582,9 @@ static irqreturn_t i2c_imx_isr(int irq, void > *dev_id) > > struct imx_i2c_struct *i2c_imx = dev_id; > > unsigned int temp; > > > > + if (pm_runtime_suspended(i2c_imx->adapter.dev.parent)) > > + return IRQ_NONE; > > + > I don't claim to understand the runtime pm stuff, but I agree to the > previous reviewer that this smells fishy. If this is required it needs a > comment why. > In fact, I agree you two reviewers' consideration. As I said, this is just to avoid system hang in worse case. Of course, I still don't catch the worse case. So I will remove this pm stuff. > > temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR); > > if (temp & I2SR_IIF) { > > /* save status register */ > > @@ -894,6 +896,10 @@ static int i2c_imx_xfer(struct i2c_adapter > > *adapter, > > > > dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__); > > > > + result = pm_runtime_get_sync(i2c_imx->adapter.dev.parent); > > + if (IS_ERR_VALUE(result)) > Is this better than > + if (result < 0) > ? Yes, thanks. > > > + goto out; > > [...] > > @@ -1018,11 +1028,6 @@ static int i2c_imx_probe(struct platform_device > *pdev) > > return PTR_ERR(i2c_imx->clk); > > } > > > > - ret = clk_prepare_enable(i2c_imx->clk); > > - if (ret) { > > - dev_err(&pdev->dev, "can't enable I2C clock\n"); > > - return ret; > > - } > Is there a reason the clock was enabled *here* before. With the way you > rearranged the code the irq handler might be entered before the clock is > on. But if I'm not mistaken (which might very well be the case) there is > a problem also without the patch if the bootloader jumps to linux with a > pending i2c transfer that fires when the clk_disable is done below. IMHO > "Set up chip registers to defaults" should be done before "Request IRQ"?! > Agree with your opinion. I will fix it in the next version. > > [...] > > @@ -1093,14 +1110,51 @@ static int i2c_imx_remove(struct > > platform_device *pdev) [...] > > +#ifdef CONFIG_PM > > +static int i2c_imx_runtime_suspend(struct device *dev) { > > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > s/ / / > > [...] > > +static int i2c_imx_runtime_resume(struct device *dev) { > > + struct imx_i2c_struct *i2c_imx = dev_get_drvdata(dev); > s/ / / > > + int ret; > > + > > + ret = clk_prepare_enable(i2c_imx->clk); > > + if (ret) { > > + dev_err(dev, "can't enable I2C clock\n"); > > + return ret; > > + } > > + > > + return 0; > this is equivalent to: > > if (ret) > dev_err(dev, "can't enable I2C clock\n"); > > return ret; > > Probably a matter of taste. Also I'd suggest to add the value of ret to > the message to make the report more useful. > Yes, thanks. > > +} > > + > > +static const struct dev_pm_ops i2c_imx_pm_ops = { > > + SET_RUNTIME_PM_OPS(i2c_imx_runtime_suspend, > > + i2c_imx_runtime_resume, NULL) > > +}; > > +#define I2C_IMX_PM_OPS (&i2c_imx_pm_ops) #else #define I2C_IMX_PM_OPS > > +NULL #endif /* CONFIG_PM */ > > + > > static struct platform_driver i2c_imx_driver = { > > .probe = i2c_imx_probe, > > .remove = i2c_imx_remove, > > .driver = { > > .name = DRIVER_NAME, > > + .pm = I2C_IMX_PM_OPS, > > .of_match_table = i2c_imx_dt_ids, > This increases the number of styles to place the = to three. .name uses a > tab, .of_match_table a space and you add .pm with >1 spaces. > Yes, thanks. > > }, > > .id_table = imx_i2c_devtype, > unrelated to this patch: The = in the line above is not aligned > consistently compared to the other members at the same level like .probe. > Yes, thanks. > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König > | > Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html