[PATCH 0/4] i2c: designware: Fixes for Asus T100 and Haswell PCI IDs

[PATCH 0/4] i2c: designware: Fixes for Asus T100 and Haswell PCI IDs

[Mika Westerberg]

Hi,

Patches 1-3 are needed in order to get runtime PM working properly on Intel
Baytrail based tablets, like on Asus T100. We need to re-initialize the
controller on resume.

Patch 4 adds Haswell PCI IDs to the driver. It is independent of the
preceding patches but I decided to include it here as well. Haswell based
chromebooks need this.

Mika Westerberg (4):
  i2c: designware: No need to disable already disabled controller
  i2c: designware: Disable device on system suspend
  i2c: designware: Add runtime PM hooks
  i2c: designware-pci: Add Haswell PCI IDs

 drivers/i2c/busses/i2c-designware-core.c    |  4 ++++
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 22 ++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-platdrv.c | 13 ++++++-------
 3 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.0.0.rc2

[PATCH 2/4] i2c: designware: Disable device on system suspend

[Mika Westerberg]

Userspace can initiate system suspend on arbitrary times which means that
device drivers must make sure that their device gets quiesced before system
suspend is entered. Therefore disable the I2C host controller in the driver
system suspend hook.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 9c7802614342..f72102f389c5 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -253,6 +253,7 @@ static int dw_i2c_suspend(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
 
+	i2c_dw_disable(i_dev);
 	clk_disable_unprepare(i_dev->clk);
 
 	return 0;
-- 
2.0.0.rc2

[PATCH 1/4] i2c: designware: No need to disable already disabled controller

[Mika Westerberg]

If the controller is already in desired state (enabled/disabled) there is
no point in setting its state again.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index b58ecf19e767..b0792675b970 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -258,6 +258,10 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
 {
 	int timeout = 100;
 
+	/* In case the controller is already in desired state */
+	if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+		return;
+
 	do {
 		dw_writel(dev, enable, DW_IC_ENABLE);
 		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
-- 
2.0.0.rc2

Re: [PATCH 1/4] i2c: designware: No need to disable already disabled controller

[Wolfram Sang]

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

On Thu, May 15, 2014 at 05:37:21PM +0300, Mika Westerberg wrote:
> If the controller is already in desired state (enabled/disabled) there is
> no point in setting its state again.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---

Does it have a side-effect when setting then enable bit again? Otherwise
it will exit the loop immediately on the first try. Not too bad IMO
given the additional code saved.

>  drivers/i2c/busses/i2c-designware-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index b58ecf19e767..b0792675b970 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -258,6 +258,10 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
>  {
>  	int timeout = 100;
>  
> +	/* In case the controller is already in desired state */
> +	if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> +		return;
> +
>  	do {
>  		dw_writel(dev, enable, DW_IC_ENABLE);
>  		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> -- 
> 2.0.0.rc2
> 

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

Re: [PATCH 1/4] i2c: designware: No need to disable already disabled controller

[Mika Westerberg]

On Mon, Jun 02, 2014 at 06:12:34PM +0200, Wolfram Sang wrote:
> On Thu, May 15, 2014 at 05:37:21PM +0300, Mika Westerberg wrote:
> > If the controller is already in desired state (enabled/disabled) there is
> > no point in setting its state again.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > ---
> 
> Does it have a side-effect when setting then enable bit again? Otherwise
> it will exit the loop immediately on the first try. Not too bad IMO
> given the additional code saved.

AFAICT there shouldn't be any side effect. So the $subject patch just
saves one register write in the best case. You are right, maybe it's not
worth adding 3 extra lines of code just for that :)

> 
> >  drivers/i2c/busses/i2c-designware-core.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > index b58ecf19e767..b0792675b970 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -258,6 +258,10 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> >  {
> >  	int timeout = 100;
> >  
> > +	/* In case the controller is already in desired state */
> > +	if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > +		return;
> > +
> >  	do {
> >  		dw_writel(dev, enable, DW_IC_ENABLE);
> >  		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > -- 
> > 2.0.0.rc2
> > 

Re: [PATCH 1/4] i2c: designware: No need to disable already disabled controller

[Wolfram Sang]

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

On Mon, Jun 02, 2014 at 08:34:08PM +0300, Mika Westerberg wrote:
> On Mon, Jun 02, 2014 at 06:12:34PM +0200, Wolfram Sang wrote:
> > On Thu, May 15, 2014 at 05:37:21PM +0300, Mika Westerberg wrote:
> > > If the controller is already in desired state (enabled/disabled) there is
> > > no point in setting its state again.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > 
> > Does it have a side-effect when setting then enable bit again? Otherwise
> > it will exit the loop immediately on the first try. Not too bad IMO
> > given the additional code saved.
> 
> AFAICT there shouldn't be any side effect. So the $subject patch just
> saves one register write in the best case. You are right, maybe it's not
> worth adding 3 extra lines of code just for that :)

:) Okay, so I'll drop it.


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

[PATCH 3/4] i2c: designware: Add runtime PM hooks

[Mika Westerberg]

It is possible that after entering runtime PM suspend the controller
context is lost due the fact that its power is removed. This happens for
example on Asus T100, an Intel Baytrail based tablet/laptop.

In order to get the controller back to functional state, we need to
implement runtime PM hooks which will re-initialize the hardware during
runtime PM resume. We can re-use the existing system suspend hooks as the
steps to resume/suspend the controller are the same.

Signed-off-by: Mika Westerberg <mika.westerberg-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index f72102f389c5..402ec3970fed 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -247,7 +247,7 @@ static const struct of_device_id dw_i2c_of_match[] = {
 MODULE_DEVICE_TABLE(of, dw_i2c_of_match);
 #endif
 
-#ifdef CONFIG_PM_SLEEP
+#ifdef CONFIG_PM
 static int dw_i2c_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -269,13 +269,11 @@ static int dw_i2c_resume(struct device *dev)
 
 	return 0;
 }
-
-static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume);
-#define DW_I2C_DEV_PM_OPS	(&dw_i2c_dev_pm_ops)
-#else
-#define DW_I2C_DEV_PM_OPS	NULL
 #endif
 
+static UNIVERSAL_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend,
+			    dw_i2c_resume, NULL);
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_designware");
 
@@ -287,7 +285,7 @@ static struct platform_driver dw_i2c_driver = {
 		.owner	= THIS_MODULE,
 		.of_match_table = of_match_ptr(dw_i2c_of_match),
 		.acpi_match_table = ACPI_PTR(dw_i2c_acpi_match),
-		.pm	= DW_I2C_DEV_PM_OPS,
+		.pm	= &dw_i2c_dev_pm_ops,
 	},
 };
 
-- 
2.0.0.rc2

Re: [PATCH 0/4] i2c: designware: Fixes for Asus T100 and Haswell PCI IDs

[Wolfram Sang]

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

On Thu, May 15, 2014 at 05:37:20PM +0300, Mika Westerberg wrote:
> Hi,
> 
> Patches 1-3 are needed in order to get runtime PM working properly on Intel
> Baytrail based tablets, like on Asus T100. We need to re-initialize the
> controller on resume.
> 
> Patch 4 adds Haswell PCI IDs to the driver. It is independent of the
> preceding patches but I decided to include it here as well. Haswell based
> chromebooks need this.
> 
> Mika Westerberg (4):
>   i2c: designware: No need to disable already disabled controller
>   i2c: designware: Disable device on system suspend
>   i2c: designware: Add runtime PM hooks
>   i2c: designware-pci: Add Haswell PCI IDs
> 
>  drivers/i2c/busses/i2c-designware-core.c    |  4 ++++
>  drivers/i2c/busses/i2c-designware-pcidrv.c  | 22 ++++++++++++++++++++++

Patches 2-4 applied to for-next, thanks! Patch 1 leaves a question to
me.


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

[PATCH 4/4] i2c: designware-pci: Add Haswell PCI IDs

[Mika Westerberg]

Intel Haswell has the same I2C host controller than Baytrail and it can
also be enumerated as a PCI device. Add the PCI IDs to the driver list.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 85056c22d21e..3356f7ab9f79 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -56,6 +56,7 @@ enum dw_pci_ctl_id_t {
 	medfield_5,
 
 	baytrail,
+	haswell,
 };
 
 struct dw_scl_sda_cfg {
@@ -95,6 +96,15 @@ static struct dw_scl_sda_cfg byt_config = {
 	.sda_hold = 0x6,
 };
 
+/* Haswell HCNT/LCNT/SDA hold time */
+static struct dw_scl_sda_cfg hsw_config = {
+	.ss_hcnt = 0x01b0,
+	.fs_hcnt = 0x48,
+	.ss_lcnt = 0x01fb,
+	.fs_lcnt = 0xa0,
+	.sda_hold = 0x9,
+};
+
 static struct  dw_pci_controller  dw_pci_controllers[] = {
 	[moorestown_0] = {
 		.bus_num     = 0,
@@ -168,6 +178,15 @@ static struct  dw_pci_controller  dw_pci_controllers[] = {
 		.functionality = I2C_FUNC_10BIT_ADDR,
 		.scl_sda_cfg = &byt_config,
 	},
+	[haswell] = {
+		.bus_num = -1,
+		.bus_cfg = INTEL_MID_STD_CFG | DW_IC_CON_SPEED_FAST,
+		.tx_fifo_depth = 32,
+		.rx_fifo_depth = 32,
+		.clk_khz = 100000,
+		.functionality = I2C_FUNC_10BIT_ADDR,
+		.scl_sda_cfg = &hsw_config,
+	},
 };
 static struct i2c_algorithm i2c_dw_algo = {
 	.master_xfer	= i2c_dw_xfer,
@@ -328,6 +347,9 @@ static const struct pci_device_id i2_designware_pci_ids[] = {
 	{ PCI_VDEVICE(INTEL, 0x0F45), baytrail },
 	{ PCI_VDEVICE(INTEL, 0x0F46), baytrail },
 	{ PCI_VDEVICE(INTEL, 0x0F47), baytrail },
+	/* Haswell */
+	{ PCI_VDEVICE(INTEL, 0x9c61), haswell },
+	{ PCI_VDEVICE(INTEL, 0x9c62), haswell },
 	{ 0,}
 };
 MODULE_DEVICE_TABLE(pci, i2_designware_pci_ids);
-- 
2.0.0.rc2

Re: [4/4] i2c: designware-pci: Add Haswell PCI IDs

[Scot Doyle]

This patch works on a Toshiba CB35 Chromebook, applied against 3.15-rc8.

On Thu, 15 May 2014, Mika Westerberg wrote:
> Intel Haswell has the same I2C host controller than Baytrail and it can
> also be enumerated as a PCI device. Add the PCI IDs to the driver list.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> ---
> drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++++++++++++++++++++++
> 1 file changed, 22 insertions(+)