[PATCHv3 1/2] i2c: cadence: Fix the Documentation

[PATCHv3 1/2] i2c: cadence: Fix the Documentation

[Shubhrajyoti Datta]

The runtime adaptation patch missed the documentation update.
This fixes the same.

Tested-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
---
v2 changes:
fix the dev param description

v3 changes
Fix the cdns_i2c_clear_bus_hold

 drivers/i2c/busses/i2c-cadence.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-cadence.c b/drivers/i2c/busses/i2c-cadence.c
index 6b08d16..90bbd9f 100644
--- a/drivers/i2c/busses/i2c-cadence.c
+++ b/drivers/i2c/busses/i2c-cadence.c
@@ -124,6 +124,8 @@
 
 /**
  * struct cdns_i2c - I2C device private data structure
+ *
+ * @dev:		Pointer to device structure
  * @membase:		Base address of the I2C device
  * @adap:		I2C adapter instance
  * @p_msg:		Message pointer
@@ -171,7 +173,7 @@ struct cdns_platform_data {
 					     clk_rate_change_nb)
 
 /**
- * cdns_i2c_clear_bus_hold() - Clear bus hold bit
+ * cdns_i2c_clear_bus_hold - Clear bus hold bit
  * @id:	Pointer to driver data struct
  *
  * Helper to clear the controller's bus hold bit.
@@ -815,8 +817,8 @@ static int cdns_i2c_clk_notifier_cb(struct notifier_block *nb, unsigned long
 }
 
 /**
- * cdns_i2c_suspend - Suspend method for the driver
- * @_dev:	Address of the platform_device structure
+ * cdns_i2c_runtime_suspend -  Runtime suspend method for the driver
+ * @dev:	Address of the platform_device structure
  *
  * Put the driver into low power mode.
  *
@@ -833,10 +835,10 @@ static int __maybe_unused cdns_i2c_runtime_suspend(struct device *dev)
 }
 
 /**
- * cdns_i2c_resume - Resume from suspend
- * @_dev:	Address of the platform_device structure
+ * cdns_i2c_runtime_resume - Runtime resume
+ * @dev:	Address of the platform_device structure
  *
- * Resume operation after suspend.
+ * Runtime resume callback.
  *
  * Return: 0 on success and error value on error
  */
-- 
1.7.1

[PATCHv3 2/2] i2c: cadence: Implement save restore

[Shubhrajyoti Datta]

Implement save restore for i2c module.
Since we have only a couple of registers
an unconditional restore is done.

Acked-by: Michal Simek <michal.simek@xilinx.com>
Signed-off-by: Shubhrajyoti Datta <shubhraj@xilinx.com>
---
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.

v2 changes:
add kernel doc for ctrl_reg and cdns_i2c_init

v3 changes:
Add Michal's ack

 drivers/i2c/busses/i2c-cadence.c |   41 +++++++++++++++++++++++++-------------
 1 files changed, 27 insertions(+), 14 deletions(-)

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);
-- 
1.7.1

Re: [PATCHv3 2/2] i2c: cadence: Implement save restore

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 2/2] i2c: cadence: Implement save restore

[Shubhrajyoti Datta]

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?
>

Re: [PATCHv3 2/2] i2c: cadence: Implement save restore

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2481 bytes --]


> >> 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.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 2/2] i2c: cadence: Implement save restore

[Shubhrajyoti Datta]

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

Re: [PATCHv3 1/2] i2c: cadence: Fix the Documentation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 198 bytes --]

On Thu, Mar 03, 2016 at 11:20:39AM +0530, Shubhrajyoti Datta wrote:
> The runtime adaptation patch missed the documentation update.
> This fixes the same.

The subject of the patch is too generic.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCHv3 1/2] i2c: cadence: Fix the Documentation

[Shubhrajyoti Datta]

On Fri, Mar 4, 2016 at 2:03 AM, Wolfram Sang <wsa@the-dreams.de> wrote:
> On Thu, Mar 03, 2016 at 11:20:39AM +0530, Shubhrajyoti Datta wrote:
>> The runtime adaptation patch missed the documentation update.
>> This fixes the same.
>
> The subject of the patch is too generic.
>
Will update in the next version.