Message ID | 1456984240-17389-2-git-send-email-shubhraj@xilinx.com |
---|---|
State | Changes Requested |
Headers | show |
On Thu, Mar 03, 2016 at 11:20:40AM +0530, Shubhrajyoti Datta wrote: > Implement save restore for i2c module. > Since we have only a couple of registers > an unconditional restore is done. But you only save one register instead of a couple? Also, the subject could be more descriptive IMO. > zynq-mp has the capability of going off. > the current kernel does not hit off however some day it will. > since the overhead of having the support is not much may be it is better to have it in the kernel. This sounds like the patch is not tested? > +static void cdns_i2c_init(struct cdns_i2c *id) > +{ > + cdns_i2c_writereg(id->ctrl_reg, CDNS_I2C_CR_OFFSET); > + /* > + * Cadence I2C controller has a bug wherein it generates > + * invalid read transaction after HW timeout in master receiver mode. > + * HW timeout is not used by this driver and the interrupt is disabled. > + * But the feature itself cannot be disabled. Hence maximum value > + * is written to this register to reduce the chances of error. > + */ > + cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); > +} This... > + > +/** > * cdns_i2c_runtime_resume - Runtime resume > * @dev: Address of the platform_device structure > * > @@ -853,6 +874,7 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > dev_err(dev, "Cannot enable clock.\n"); > return ret; > } > + cdns_i2c_init(xi2c); and this... > - > - /* > - * Cadence I2C controller has a bug wherein it generates > - * invalid read transaction after HW timeout in master receiver mode. > - * HW timeout is not used by this driver and the interrupt is disabled. > - * But the feature itself cannot be disabled. Hence maximum value > - * is written to this register to reduce the chances of error. > - */ > - cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); > + cdns_i2c_init(id); ... and this look like a unrelated change to me?
On Fri, Mar 4, 2016 at 2:10 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > On Thu, Mar 03, 2016 at 11:20:40AM +0530, Shubhrajyoti Datta wrote: > >> Implement save restore for i2c module. >> Since we have only a couple of registers >> an unconditional restore is done. > > But you only save one register instead of a couple? Also, the subject > could be more descriptive IMO. That is because the timeout is written a fixed value always so just restore is done. > >> zynq-mp has the capability of going off. >> the current kernel does not hit off however some day it will. >> since the overhead of having the support is not much may be it is better to have it in the kernel. > > This sounds like the patch is not tested? to simulate the suspend behavior I did a reset of the module by writing to slcr register. And then did a transfer. > >> +static void cdns_i2c_init(struct cdns_i2c *id) >> +{ >> + cdns_i2c_writereg(id->ctrl_reg, CDNS_I2C_CR_OFFSET); >> + /* >> + * Cadence I2C controller has a bug wherein it generates >> + * invalid read transaction after HW timeout in master receiver mode. >> + * HW timeout is not used by this driver and the interrupt is disabled. >> + * But the feature itself cannot be disabled. Hence maximum value >> + * is written to this register to reduce the chances of error. >> + */ >> + cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); >> +} > > This... Writes the values to the register. > >> + >> +/** >> * cdns_i2c_runtime_resume - Runtime resume >> * @dev: Address of the platform_device structure >> * >> @@ -853,6 +874,7 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) >> dev_err(dev, "Cannot enable clock.\n"); >> return ret; >> } >> + cdns_i2c_init(xi2c); > > and this... At resume . (Actually this could be conditional however since we have 2 registers so can be unconditioal) > >> - >> - /* >> - * Cadence I2C controller has a bug wherein it generates >> - * invalid read transaction after HW timeout in master receiver mode. >> - * HW timeout is not used by this driver and the interrupt is disabled. >> - * But the feature itself cannot be disabled. Hence maximum value >> - * is written to this register to reduce the chances of error. >> - */ >> - cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); >> + cdns_i2c_init(id); And at init. > > ... and this look like a unrelated change 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
> >> zynq-mp has the capability of going off. > >> the current kernel does not hit off however some day it will. > >> since the overhead of having the support is not much may be it is better to have it in the kernel. > > > > This sounds like the patch is not tested? > to simulate the suspend behavior I did a reset of the module by writing to > slcr register. > And then did a transfer. I'd prefer a patch which is tested under the actual condition. So, let's wait until proper suspend support is added. > >> +static void cdns_i2c_init(struct cdns_i2c *id) > >> +{ > >> + cdns_i2c_writereg(id->ctrl_reg, CDNS_I2C_CR_OFFSET); > >> + /* > >> + * Cadence I2C controller has a bug wherein it generates > >> + * invalid read transaction after HW timeout in master receiver mode. > >> + * HW timeout is not used by this driver and the interrupt is disabled. > >> + * But the feature itself cannot be disabled. Hence maximum value > >> + * is written to this register to reduce the chances of error. > >> + */ > >> + cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); > >> +} > > > > This... > > Writes the values to the register. > > > > >> + > >> +/** > >> * cdns_i2c_runtime_resume - Runtime resume > >> * @dev: Address of the platform_device structure > >> * > >> @@ -853,6 +874,7 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) > >> dev_err(dev, "Cannot enable clock.\n"); > >> return ret; > >> } > >> + cdns_i2c_init(xi2c); > > > > and this... > At resume . > (Actually this could be conditional however since we have 2 registers > so can be unconditioal) > > > >> - > >> - /* > >> - * Cadence I2C controller has a bug wherein it generates > >> - * invalid read transaction after HW timeout in master receiver mode. > >> - * HW timeout is not used by this driver and the interrupt is disabled. > >> - * But the feature itself cannot be disabled. Hence maximum value > >> - * is written to this register to reduce the chances of error. > >> - */ > >> - cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); > >> + cdns_i2c_init(id); > > And at init. I understand what it does, but I don't see any connection to the caching mentioned in the subject. So, this is maybe a seperate patch or the commit message must be updated.
On Sat, Mar 12, 2016 at 8:32 PM, Wolfram Sang <wsa@the-dreams.de> wrote: > >> >> zynq-mp has the capability of going off. >> >> the current kernel does not hit off however some day it will. >> >> since the overhead of having the support is not much may be it is better to have it in the kernel. >> > >> > This sounds like the patch is not tested? >> to simulate the suspend behavior I did a reset of the module by writing to >> slcr register. >> And then did a transfer. > > I'd prefer a patch which is tested under the actual condition. So, let's > wait until proper suspend support is added. > Makes sense. thanks, Shubhrajyoti -- 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-cadence.c b/drivers/i2c/busses/i2c-cadence.c index 90bbd9f..5997d4c 100644 --- a/drivers/i2c/busses/i2c-cadence.c +++ b/drivers/i2c/busses/i2c-cadence.c @@ -143,6 +143,7 @@ * @clk: Pointer to struct clk * @clk_rate_change_nb: Notifier block for clock rate changes * @quirks: flag for broken hold bit usage in r1p10 + * @ctrl_reg: Cached value of the control register. */ struct cdns_i2c { struct device *dev; @@ -163,6 +164,7 @@ struct cdns_i2c { struct clk *clk; struct notifier_block clk_rate_change_nb; u32 quirks; + u32 ctrl_reg; }; struct cdns_platform_data { @@ -745,12 +747,11 @@ static int cdns_i2c_setclk(unsigned long clk_in, struct cdns_i2c *id) if (ret) return ret; - ctrl_reg = cdns_i2c_readreg(CDNS_I2C_CR_OFFSET); + ctrl_reg = id->ctrl_reg; ctrl_reg &= ~(CDNS_I2C_CR_DIVA_MASK | CDNS_I2C_CR_DIVB_MASK); ctrl_reg |= ((div_a << CDNS_I2C_CR_DIVA_SHIFT) | (div_b << CDNS_I2C_CR_DIVB_SHIFT)); - cdns_i2c_writereg(ctrl_reg, CDNS_I2C_CR_OFFSET); - + id->ctrl_reg = ctrl_reg; return 0; } @@ -835,6 +836,26 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev) } /** + * cdns_i2c_init - Controller initialisation + * @id: Device private data structure + * + * Initialise the i2c controller. + * + */ +static void cdns_i2c_init(struct cdns_i2c *id) +{ + cdns_i2c_writereg(id->ctrl_reg, CDNS_I2C_CR_OFFSET); + /* + * Cadence I2C controller has a bug wherein it generates + * invalid read transaction after HW timeout in master receiver mode. + * HW timeout is not used by this driver and the interrupt is disabled. + * But the feature itself cannot be disabled. Hence maximum value + * is written to this register to reduce the chances of error. + */ + cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); +} + +/** * cdns_i2c_runtime_resume - Runtime resume * @dev: Address of the platform_device structure * @@ -853,6 +874,7 @@ static int __maybe_unused cdns_i2c_runtime_resume(struct device *dev) dev_err(dev, "Cannot enable clock.\n"); return ret; } + cdns_i2c_init(xi2c); return 0; } @@ -945,8 +967,7 @@ static int cdns_i2c_probe(struct platform_device *pdev) if (ret || (id->i2c_clk > CDNS_I2C_SPEED_MAX)) id->i2c_clk = CDNS_I2C_SPEED_DEFAULT; - cdns_i2c_writereg(CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS, - CDNS_I2C_CR_OFFSET); + id->ctrl_reg = CDNS_I2C_CR_ACK_EN | CDNS_I2C_CR_NEA | CDNS_I2C_CR_MS; ret = cdns_i2c_setclk(id->input_clk, id); if (ret) { @@ -967,15 +988,7 @@ static int cdns_i2c_probe(struct platform_device *pdev) dev_err(&pdev->dev, "reg adap failed: %d\n", ret); goto err_clk_dis; } - - /* - * Cadence I2C controller has a bug wherein it generates - * invalid read transaction after HW timeout in master receiver mode. - * HW timeout is not used by this driver and the interrupt is disabled. - * But the feature itself cannot be disabled. Hence maximum value - * is written to this register to reduce the chances of error. - */ - cdns_i2c_writereg(CDNS_I2C_TIMEOUT_MAX, CDNS_I2C_TIME_OUT_OFFSET); + cdns_i2c_init(id); dev_info(&pdev->dev, "%u kHz mmio %08lx irq %d\n", id->i2c_clk / 1000, (unsigned long)r_mem->start, id->irq);