[PATCH 1/2] i2c: designware: add High-speed mode support

[PATCH 1/2] i2c: designware: add High-speed mode support

[wangxfdu]

From: Xiang Wang <xiang.a.wang@intel.com>

1. Add High-speed mode support in designware core
2. Add function i2c_dw_acpi_setup_speed to determine
the bus speed from ACPI table.

Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c | 88 ++++++++++++++++++++++++++++++++
 drivers/i2c/busses/i2c-designware-core.h | 11 ++++
 2 files changed, 99 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 6f19a33..f5c0d18 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -30,6 +30,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
 #include <linux/module.h>
+#include <linux/acpi.h>
 #include "i2c-designware-core.h"
 
 /*
@@ -42,6 +43,8 @@
 #define DW_IC_SS_SCL_LCNT	0x18
 #define DW_IC_FS_SCL_HCNT	0x1c
 #define DW_IC_FS_SCL_LCNT	0x20
+#define DW_IC_HS_SCL_HCNT	0x24
+#define DW_IC_HS_SCL_LCNT	0x28
 #define DW_IC_INTR_STAT		0x2c
 #define DW_IC_INTR_MASK		0x30
 #define DW_IC_RAW_INTR_STAT	0x34
@@ -358,6 +361,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	if (dev->hs_hcnt && dev->hs_lcnt) {
+		hcnt = dev->hs_hcnt;
+		lcnt = dev->hs_lcnt;
+
+		dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
+		dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
+		dev_dbg(dev->dev, "High-speed mode HCNT:LCNT = %d:%d\n",
+					hcnt, lcnt);
+	}
+
 	/* Configure SDA Hold Time if required */
 	if (dev->sda_hold_time) {
 		reg = dw_readl(dev, DW_IC_COMP_VERSION);
@@ -381,6 +394,81 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 }
 EXPORT_SYMBOL_GPL(i2c_dw_init);
 
+#ifdef CONFIG_ACPI
+static int i2c_dw_acpi_get_freq(struct acpi_resource *ares, void *data)
+{
+	struct dw_i2c_dev *i2c = data;
+	struct acpi_resource_i2c_serialbus *sb;
+	u32 i2c_speed;
+
+	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
+		sb = &ares->data.i2c_serial_bus;
+
+		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
+			i2c_speed = sb->connection_speed;
+			if (i2c_speed == DW_STD_SPEED) {
+				i2c->master_cfg &= ~DW_IC_SPEED_MASK;
+				i2c->master_cfg |= DW_IC_CON_SPEED_STD;
+			} else if (i2c_speed == DW_FAST_SPEED) {
+				i2c->master_cfg &= ~DW_IC_SPEED_MASK;
+				i2c->master_cfg |= DW_IC_CON_SPEED_FAST;
+			} else if (i2c_speed == DW_HIGH_SPEED) {
+				i2c->master_cfg &= ~DW_IC_SPEED_MASK;
+				i2c->master_cfg |= DW_IC_CON_SPEED_HIGH;
+			} else {
+				dev_err(i2c->dev, "unsupported speed: %d\n",
+					i2c_speed);
+			}
+
+			dev_dbg(i2c->dev, "i2c device speed from acpi = %d\n",
+					i2c_speed);
+		}
+	}
+
+	return 1;
+}
+
+static acpi_status acpi_i2c_find_device_speed(acpi_handle handle, u32 level,
+					void *data, void **return_value)
+{
+	struct dw_i2c_dev *i2c = data;
+	struct list_head resource_list;
+	struct acpi_device *adev;
+
+	if (acpi_bus_get_device(handle, &adev))
+		return AE_OK;
+	if (acpi_bus_get_status(adev) || !adev->status.present)
+		return AE_OK;
+
+	INIT_LIST_HEAD(&resource_list);
+	acpi_dev_get_resources(adev, &resource_list,
+				i2c_dw_acpi_get_freq, i2c);
+	acpi_dev_free_resource_list(&resource_list);
+
+	return AE_OK;
+}
+
+void i2c_dw_acpi_setup_speed(struct device *pdev, struct dw_i2c_dev *dev)
+{
+	acpi_handle handle = ACPI_HANDLE(pdev);
+	acpi_status status;
+
+	if (handle == NULL)
+		return;
+
+	/* Find I2C adapter bus frequency */
+	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
+				     acpi_i2c_find_device_speed, NULL,
+				     dev, NULL);
+	if (ACPI_FAILURE(status))
+		dev_warn(pdev, "failed to get I2C bus freq\n");
+}
+
+#else
+void i2c_dw_acpi_setup_speed(struct device *pdev, struct dw_i2c_dev *dev) {}
+#endif
+EXPORT_SYMBOL_GPL(i2c_dw_acpi_setup_speed);
+
 /*
  * Waiting for bus not busy
  */
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9630222..16f53d8 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -24,12 +24,17 @@
 
 
 #define DW_IC_CON_MASTER		0x1
+#define DW_IC_SPEED_MASK		0x6
 #define DW_IC_CON_SPEED_STD		0x2
 #define DW_IC_CON_SPEED_FAST		0x4
+#define DW_IC_CON_SPEED_HIGH		0x6
 #define DW_IC_CON_10BITADDR_MASTER	0x10
 #define DW_IC_CON_RESTART_EN		0x20
 #define DW_IC_CON_SLAVE_DISABLE		0x40
 
+#define DW_STD_SPEED	100000
+#define DW_FAST_SPEED	400000
+#define DW_HIGH_SPEED	3400000
 
 /**
  * struct dw_i2c_dev - private i2c-designware data
@@ -61,6 +66,8 @@
  * @ss_lcnt: standard speed LCNT value
  * @fs_hcnt: fast speed HCNT value
  * @fs_lcnt: fast speed LCNT value
+ * @hs_hcnt: high speed HCNT value
+ * @hs_lcnt: high speed LCNT value
  * @acquire_lock: function to acquire a hardware lock on the bus
  * @release_lock: function to release a hardware lock on the bus
  * @pm_runtime_disabled: true if pm runtime is disabled
@@ -104,6 +111,8 @@ struct dw_i2c_dev {
 	u16			ss_lcnt;
 	u16			fs_hcnt;
 	u16			fs_lcnt;
+	u16			hs_hcnt;
+	u16			hs_lcnt;
 	int			(*acquire_lock)(struct dw_i2c_dev *dev);
 	void			(*release_lock)(struct dw_i2c_dev *dev);
 	bool			pm_runtime_disabled;
@@ -114,6 +123,8 @@ struct dw_i2c_dev {
 
 extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
 extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
+extern void i2c_dw_acpi_setup_speed(struct device *pdev,
+		struct dw_i2c_dev *dev);
 extern int i2c_dw_init(struct dw_i2c_dev *dev);
 extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
 		int num);
-- 
2.5.3

[PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[wangxfdu]

From: Xiang Wang <xiang.a.wang@intel.com>

1. Support setting hs_hcnt and hs_lcnt
2. Get bus speed mode from ACPI companion of the
i2c controller.

Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 6643d2d..0f4c0c4 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t {
 struct dw_scl_sda_cfg {
 	u32 ss_hcnt;
 	u32 fs_hcnt;
+	u32 hs_hcnt;
 	u32 ss_lcnt;
 	u32 fs_lcnt;
+	u32 hs_lcnt;
 	u32 sda_hold;
 };
 
@@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 		cfg = controller->scl_sda_cfg;
 		dev->ss_hcnt = cfg->ss_hcnt;
 		dev->fs_hcnt = cfg->fs_hcnt;
+		dev->hs_hcnt = cfg->hs_hcnt;
 		dev->ss_lcnt = cfg->ss_lcnt;
 		dev->fs_lcnt = cfg->fs_lcnt;
+		dev->hs_lcnt = cfg->hs_lcnt;
 		dev->sda_hold_time = cfg->sda_hold;
 	}
 
@@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 
 	dev->tx_fifo_depth = controller->tx_fifo_depth;
 	dev->rx_fifo_depth = controller->rx_fifo_depth;
+
+	i2c_dw_acpi_setup_speed(&pdev->dev, dev);
+
 	r = i2c_dw_init(dev);
 	if (r)
 		return r;
-- 
2.5.3

Re: [PATCH 1/2] i2c: designware: add High-speed mode support

[Andy Shevchenko]

On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote:
> From: Xiang Wang <xiang.a.wang@intel.com>
> 
> 1. Add High-speed mode support in designware core
> 2. Add function i2c_dw_acpi_setup_speed to determine
> the bus speed from ACPI table.
> 
> Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-core.c | 88 
> ++++++++++++++++++++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.h | 11 ++++
>  2 files changed, 99 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-core.c 
> b/drivers/i2c/busses/i2c-designware-core.c
> index 6f19a33..f5c0d18 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -30,6 +30,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/delay.h>
>  #include <linux/module.h>
> +#include <linux/acpi.h>

Shouldn't be a part of platform driver?

>  #include "i2c-designware-core.h"
>  
>  /*
> @@ -42,6 +43,8 @@
>  #define DW_IC_SS_SCL_LCNT	0x18
>  #define DW_IC_FS_SCL_HCNT	0x1c
>  #define DW_IC_FS_SCL_LCNT	0x20
> +#define DW_IC_HS_SCL_HCNT	0x24
> +#define DW_IC_HS_SCL_LCNT	0x28
>  #define DW_IC_INTR_STAT		0x2c
>  #define DW_IC_INTR_MASK		0x30
>  #define DW_IC_RAW_INTR_STAT	0x34
> @@ -358,6 +361,16 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, 
> lcnt);
>  
> +	if (dev->hs_hcnt && dev->hs_lcnt) {
> +		hcnt = dev->hs_hcnt;
> +		lcnt = dev->hs_lcnt;
> +
> +		dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT);
> +		dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT);
> +		dev_dbg(dev->dev, "High-speed mode HCNT:LCNT = 
> %d:%d\n",
> +					hcnt, lcnt);
> +	}
> +
>  	/* Configure SDA Hold Time if required */
>  	if (dev->sda_hold_time) {
>  		reg = dw_readl(dev, DW_IC_COMP_VERSION);
> @@ -381,6 +394,81 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(i2c_dw_init);
>  
> +#ifdef CONFIG_ACPI
> +static int i2c_dw_acpi_get_freq(struct acpi_resource *ares, void 
> *data)
> +{
> +	struct dw_i2c_dev *i2c = data;
> +	struct acpi_resource_i2c_serialbus *sb;
> +	u32 i2c_speed;
> +
> +	if (ares->type == ACPI_RESOURCE_TYPE_SERIAL_BUS) {
> +		sb = &ares->data.i2c_serial_bus;
> +
> +		if (sb->type == ACPI_RESOURCE_SERIAL_TYPE_I2C) {
> +			i2c_speed = sb->connection_speed;
> +			if (i2c_speed == DW_STD_SPEED) {
> +				i2c->master_cfg &= 
> ~DW_IC_SPEED_MASK;
> +				i2c->master_cfg |= 
> DW_IC_CON_SPEED_STD;
> +			} else if (i2c_speed == DW_FAST_SPEED) {
> +				i2c->master_cfg &= 
> ~DW_IC_SPEED_MASK;
> +				i2c->master_cfg |= 
> DW_IC_CON_SPEED_FAST;
> +			} else if (i2c_speed == DW_HIGH_SPEED) {
> +				i2c->master_cfg &= 
> ~DW_IC_SPEED_MASK;
> +				i2c->master_cfg |= 
> DW_IC_CON_SPEED_HIGH;
> +			} else {
> +				dev_err(i2c->dev, "unsupported 
> speed: %d\n",
> +					i2c_speed);
> +			}
> +
> +			dev_dbg(i2c->dev, "i2c device speed from 
> acpi = %d\n",
> +					i2c_speed);
> +		}
> +	}
> +
> +	return 1;
> +}
> +
> +static acpi_status acpi_i2c_find_device_speed(acpi_handle handle, 
> u32 level,
> +					void *data, void 
> **return_value)
> +{
> +	struct dw_i2c_dev *i2c = data;
> +	struct list_head resource_list;
> +	struct acpi_device *adev;
> +
> +	if (acpi_bus_get_device(handle, &adev))
> +		return AE_OK;
> +	if (acpi_bus_get_status(adev) || !adev->status.present)
> +		return AE_OK;
> +
> +	INIT_LIST_HEAD(&resource_list);
> +	acpi_dev_get_resources(adev, &resource_list,
> +				i2c_dw_acpi_get_freq, i2c);
> +	acpi_dev_free_resource_list(&resource_list);
> +
> +	return AE_OK;
> +}
> +
> +void i2c_dw_acpi_setup_speed(struct device *pdev, struct dw_i2c_dev 
> *dev)
> +{
> +	acpi_handle handle = ACPI_HANDLE(pdev);
> +	acpi_status status;
> +
> +	if (handle == NULL)
> +		return;
> +
> +	/* Find I2C adapter bus frequency */
> +	status = acpi_walk_namespace(ACPI_TYPE_DEVICE, handle, 1,
> +				     acpi_i2c_find_device_speed, 
> NULL,
> +				     dev, NULL);
> +	if (ACPI_FAILURE(status))
> +		dev_warn(pdev, "failed to get I2C bus freq\n");
> +}
> +
> +#else
> +void i2c_dw_acpi_setup_speed(struct device *pdev, struct dw_i2c_dev 
> *dev) {}
> +#endif
> +EXPORT_SYMBOL_GPL(i2c_dw_acpi_setup_speed);
> +
>  /*
>   * Waiting for bus not busy
>   */
> diff --git a/drivers/i2c/busses/i2c-designware-core.h 
> b/drivers/i2c/busses/i2c-designware-core.h
> index 9630222..16f53d8 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -24,12 +24,17 @@
>  
>  
>  #define DW_IC_CON_MASTER		0x1
> +#define DW_IC_SPEED_MASK		0x6
>  #define DW_IC_CON_SPEED_STD		0x2
>  #define DW_IC_CON_SPEED_FAST		0x4
> +#define DW_IC_CON_SPEED_HIGH		0x6
>  #define DW_IC_CON_10BITADDR_MASTER	0x10
>  #define DW_IC_CON_RESTART_EN		0x20
>  #define DW_IC_CON_SLAVE_DISABLE		0x40
>  
> +#define DW_STD_SPEED	100000
> +#define DW_FAST_SPEED	400000
> +#define DW_HIGH_SPEED	3400000
>  
>  /**
>   * struct dw_i2c_dev - private i2c-designware data
> @@ -61,6 +66,8 @@
>   * @ss_lcnt: standard speed LCNT value
>   * @fs_hcnt: fast speed HCNT value
>   * @fs_lcnt: fast speed LCNT value
> + * @hs_hcnt: high speed HCNT value
> + * @hs_lcnt: high speed LCNT value
>   * @acquire_lock: function to acquire a hardware lock on the bus
>   * @release_lock: function to release a hardware lock on the bus
>   * @pm_runtime_disabled: true if pm runtime is disabled
> @@ -104,6 +111,8 @@ struct dw_i2c_dev {
>  	u16			ss_lcnt;
>  	u16			fs_hcnt;
>  	u16			fs_lcnt;
> +	u16			hs_hcnt;
> +	u16			hs_lcnt;
>  	int			(*acquire_lock)(struct dw_i2c_dev 
> *dev);
>  	void			(*release_lock)(struct dw_i2c_dev 
> *dev);
>  	bool			pm_runtime_disabled;
> @@ -114,6 +123,8 @@ struct dw_i2c_dev {
>  
>  extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
>  extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
> +extern void i2c_dw_acpi_setup_speed(struct device *pdev,
> +		struct dw_i2c_dev *dev);
>  extern int i2c_dw_init(struct dw_i2c_dev *dev);
>  extern int i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg 
> msgs[],
>  		int num);

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Andy Shevchenko]

On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote:
> From: Xiang Wang <xiang.a.wang@intel.com>
> 
> 1. Support setting hs_hcnt and hs_lcnt
> 2. Get bus speed mode from ACPI companion of the
> i2c controller.
> 
> Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c 
> b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 6643d2d..0f4c0c4 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t {
>  struct dw_scl_sda_cfg {
>  	u32 ss_hcnt;
>  	u32 fs_hcnt;
> +	u32 hs_hcnt;
>  	u32 ss_lcnt;
>  	u32 fs_lcnt;
> +	u32 hs_lcnt;
>  	u32 sda_hold;
>  };
>  
> @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev 
> *pdev,
>  		cfg = controller->scl_sda_cfg;
>  		dev->ss_hcnt = cfg->ss_hcnt;
>  		dev->fs_hcnt = cfg->fs_hcnt;
> +		dev->hs_hcnt = cfg->hs_hcnt;
>  		dev->ss_lcnt = cfg->ss_lcnt;
>  		dev->fs_lcnt = cfg->fs_lcnt;
> +		dev->hs_lcnt = cfg->hs_lcnt;
>  		dev->sda_hold_time = cfg->sda_hold;
>  	}
>  
> @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>  
>  	dev->tx_fifo_depth = controller->tx_fifo_depth;
>  	dev->rx_fifo_depth = controller->rx_fifo_depth;
> +
> +	i2c_dw_acpi_setup_speed(&pdev->dev, dev);

Don't see a relationship between PCI driver and this ACPI stuff.

> +
>  	r = i2c_dw_init(dev);
>  	if (r)
>  		return r;

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Xiang Wang]

Hi, Andy
Thanks for your comments.

[Andy] Don't see a relationship between PCI driver and this ACPI stuff.
Although this is a pci driver, we may enumerate the i2c devices from
DSDT table while i2c controllers are enumerated via PCI. In this
scenario, in DSDT, there are descriptions of i2c devices as well as
i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI
devices via pci-acpi glue.
So if we want to determine the i2c devices' settings (e.g. bus speed),
we should leverage ACPI.

Above is also the reason why the ACPI stuff is put in
i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat
and pci driver. Thanks

2015-10-09 17:31 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote:
>> From: Xiang Wang <xiang.a.wang@intel.com>
>>
>> 1. Support setting hs_hcnt and hs_lcnt
>> 2. Get bus speed mode from ACPI companion of the
>> i2c controller.
>>
>> Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
>> ---
>>  drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> index 6643d2d..0f4c0c4 100644
>> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t {
>>  struct dw_scl_sda_cfg {
>>       u32 ss_hcnt;
>>       u32 fs_hcnt;
>> +     u32 hs_hcnt;
>>       u32 ss_lcnt;
>>       u32 fs_lcnt;
>> +     u32 hs_lcnt;
>>       u32 sda_hold;
>>  };
>>
>> @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev
>> *pdev,
>>               cfg = controller->scl_sda_cfg;
>>               dev->ss_hcnt = cfg->ss_hcnt;
>>               dev->fs_hcnt = cfg->fs_hcnt;
>> +             dev->hs_hcnt = cfg->hs_hcnt;
>>               dev->ss_lcnt = cfg->ss_lcnt;
>>               dev->fs_lcnt = cfg->fs_lcnt;
>> +             dev->hs_lcnt = cfg->hs_lcnt;
>>               dev->sda_hold_time = cfg->sda_hold;
>>       }
>>
>> @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
>>
>>       dev->tx_fifo_depth = controller->tx_fifo_depth;
>>       dev->rx_fifo_depth = controller->rx_fifo_depth;
>> +
>> +     i2c_dw_acpi_setup_speed(&pdev->dev, dev);
>
> Don't see a relationship between PCI driver and this ACPI stuff.
>
>> +
>>       r = i2c_dw_init(dev);
>>       if (r)
>>               return r;
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy



-- 
Regards,
Xiang

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Andy Shevchenko]

On Mon, 2015-10-12 at 15:41 +0800, Xiang Wang wrote:
> Hi, Andy
> Thanks for your comments.
> 
> [Andy] Don't see a relationship between PCI driver and this ACPI 
> stuff.
> Although this is a pci driver, we may enumerate the i2c devices from
> DSDT table while i2c controllers are enumerated via PCI. In this
> scenario, in DSDT, there are descriptions of i2c devices as well as
> i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI
> devices via pci-acpi glue.
> So if we want to determine the i2c devices' settings (e.g. bus 
> speed),
> we should leverage ACPI.
> 
> Above is also the reason why the ACPI stuff is put in
> i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat
> and pci driver. Thanks

Wait, first of all let's divide parameters to two groups: a) to be
applied to host driver, and b) to be applied to slave devices.

The drivers/i2c/busses/i2c-designware* is about host driver parameters.

Thus, PCI driver comes with hardcoded values since it's enumerated via
PCI, and platform driver utilizes ACPI values.

For slave devices everything is done in i2c-core.c.

So, what exactly you are trying to enhance?

> 
> 2015-10-09 17:31 GMT+08:00 Andy Shevchenko <
> andriy.shevchenko@linux.intel.com>:
> > On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote:
> > > From: Xiang Wang <xiang.a.wang@intel.com>
> > > 
> > > 1. Support setting hs_hcnt and hs_lcnt
> > > 2. Get bus speed mode from ACPI companion of the
> > > i2c controller.
> > > 
> > > Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
> > > ---
> > >  drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> > > b/drivers/i2c/busses/i2c-designware-pcidrv.c
> > > index 6643d2d..0f4c0c4 100644
> > > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> > > @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t {
> > >  struct dw_scl_sda_cfg {
> > >       u32 ss_hcnt;
> > >       u32 fs_hcnt;
> > > +     u32 hs_hcnt;
> > >       u32 ss_lcnt;
> > >       u32 fs_lcnt;
> > > +     u32 hs_lcnt;
> > >       u32 sda_hold;
> > >  };
> > > 
> > > @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev
> > > *pdev,
> > >               cfg = controller->scl_sda_cfg;
> > >               dev->ss_hcnt = cfg->ss_hcnt;
> > >               dev->fs_hcnt = cfg->fs_hcnt;
> > > +             dev->hs_hcnt = cfg->hs_hcnt;
> > >               dev->ss_lcnt = cfg->ss_lcnt;
> > >               dev->fs_lcnt = cfg->fs_lcnt;
> > > +             dev->hs_lcnt = cfg->hs_lcnt;
> > >               dev->sda_hold_time = cfg->sda_hold;
> > >       }
> > > 
> > > @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev 
> > > *pdev,
> > > 
> > >       dev->tx_fifo_depth = controller->tx_fifo_depth;
> > >       dev->rx_fifo_depth = controller->rx_fifo_depth;
> > > +
> > > +     i2c_dw_acpi_setup_speed(&pdev->dev, dev);
> > 
> > Don't see a relationship between PCI driver and this ACPI stuff.
> > 
> > > +
> > >       r = i2c_dw_init(dev);
> > >       if (r)
> > >               return r;
> > 
> > --
> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Intel Finland Oy
> 
> 
> 

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Xiang Wang]

2015-10-12 16:31 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Mon, 2015-10-12 at 15:41 +0800, Xiang Wang wrote:
>> Hi, Andy
>> Thanks for your comments.
>>
>> [Andy] Don't see a relationship between PCI driver and this ACPI
>> stuff.
>> Although this is a pci driver, we may enumerate the i2c devices from
>> DSDT table while i2c controllers are enumerated via PCI. In this
>> scenario, in DSDT, there are descriptions of i2c devices as well as
>> i2c controllers. The ACPI node of i2c controllers are bond to i2c PCI
>> devices via pci-acpi glue.
>> So if we want to determine the i2c devices' settings (e.g. bus
>> speed),
>> we should leverage ACPI.
>>
>> Above is also the reason why the ACPI stuff is put in
>> i2c-designware-core: i2c_dw_acpi_setup_speed can be used by both plat
>> and pci driver. Thanks
>
> Wait, first of all let's divide parameters to two groups: a) to be
> applied to host driver, and b) to be applied to slave devices.
>
> The drivers/i2c/busses/i2c-designware* is about host driver parameters.
>
> Thus, PCI driver comes with hardcoded values since it's enumerated via
> PCI, and platform driver utilizes ACPI values.
>
> For slave devices everything is done in i2c-core.c.
>
> So, what exactly you are trying to enhance?
Agree on your analysis. The bus speed mode is a "host controller"
parameter. We can hardcode it in PCI driver.
However,
1. "bus speed mode" is a bit different from other parameters. Actually
it can be determined by the speed setting of "i2c devices" in ACPI
(I2CSerialBus). E.g. If i2c device uses 3MHz, we use High-speed mode
for this i2c bus.
2. If we hardcode speed setting in pci driver, we lose the
flexibility. A high-speed device may be connected to this i2c bus on
this board, but may be connected to another i2c bus on another board
design.

In this patch, we enumerate the i2c device in ACPI table to determine
the frequency setting. Then we set corresponding speed mode for this
i2c controller. The ACPI stuff is common for pci and plat driver. If
board design changes, we only change BIOS.

In conclusion, we have 2 solutions to set the i2c controller speed
mode (pci driver):
1) use hardcode value in pci driver
2) use frequency setting of "i2c device" in ACPI table (more flexible,
but looks a bit strange)

Do you have any preference/suggestions for above solutions? Thanks
>
>>
>> 2015-10-09 17:31 GMT+08:00 Andy Shevchenko <
>> andriy.shevchenko@linux.intel.com>:
>> > On Fri, 2015-10-09 at 16:47 +0800, wangxfdu@gmail.com wrote:
>> > > From: Xiang Wang <xiang.a.wang@intel.com>
>> > >
>> > > 1. Support setting hs_hcnt and hs_lcnt
>> > > 2. Get bus speed mode from ACPI companion of the
>> > > i2c controller.
>> > >
>> > > Signed-off-by: Xiang Wang <xiang.a.wang@intel.com>
>> > > ---
>> > >  drivers/i2c/busses/i2c-designware-pcidrv.c | 7 +++++++
>> > >  1 file changed, 7 insertions(+)
>> > >
>> > > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> > > b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> > > index 6643d2d..0f4c0c4 100644
>> > > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
>> > > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
>> > > @@ -54,8 +54,10 @@ enum dw_pci_ctl_id_t {
>> > >  struct dw_scl_sda_cfg {
>> > >       u32 ss_hcnt;
>> > >       u32 fs_hcnt;
>> > > +     u32 hs_hcnt;
>> > >       u32 ss_lcnt;
>> > >       u32 fs_lcnt;
>> > > +     u32 hs_lcnt;
>> > >       u32 sda_hold;
>> > >  };
>> > >
>> > > @@ -237,8 +239,10 @@ static int i2c_dw_pci_probe(struct pci_dev
>> > > *pdev,
>> > >               cfg = controller->scl_sda_cfg;
>> > >               dev->ss_hcnt = cfg->ss_hcnt;
>> > >               dev->fs_hcnt = cfg->fs_hcnt;
>> > > +             dev->hs_hcnt = cfg->hs_hcnt;
>> > >               dev->ss_lcnt = cfg->ss_lcnt;
>> > >               dev->fs_lcnt = cfg->fs_lcnt;
>> > > +             dev->hs_lcnt = cfg->hs_lcnt;
>> > >               dev->sda_hold_time = cfg->sda_hold;
>> > >       }
>> > >
>> > > @@ -246,6 +250,9 @@ static int i2c_dw_pci_probe(struct pci_dev
>> > > *pdev,
>> > >
>> > >       dev->tx_fifo_depth = controller->tx_fifo_depth;
>> > >       dev->rx_fifo_depth = controller->rx_fifo_depth;
>> > > +
>> > > +     i2c_dw_acpi_setup_speed(&pdev->dev, dev);
>> >
>> > Don't see a relationship between PCI driver and this ACPI stuff.
>> >
>> > > +
>> > >       r = i2c_dw_init(dev);
>> > >       if (r)
>> > >               return r;
>> >
>> > --
>> > Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> > Intel Finland Oy
>>
>>
>>
>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy



-- 
Regards,
Xiang

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Jarkko Nikula]

On 10/15/2015 08:46 AM, Xiang Wang wrote:
> 1. "bus speed mode" is a bit different from other parameters. Actually
> it can be determined by the speed setting of "i2c devices" in ACPI
> (I2CSerialBus). E.g. If i2c device uses 3MHz, we use High-speed mode
> for this i2c bus.
> 2. If we hardcode speed setting in pci driver, we lose the
> flexibility. A high-speed device may be connected to this i2c bus on
> this board, but may be connected to another i2c bus on another board
> design.
>
> In this patch, we enumerate the i2c device in ACPI table to determine
> the frequency setting. Then we set corresponding speed mode for this
> i2c controller. The ACPI stuff is common for pci and plat driver. If
> board design changes, we only change BIOS.
>
> In conclusion, we have 2 solutions to set the i2c controller speed
> mode (pci driver):
> 1) use hardcode value in pci driver
> 2) use frequency setting of "i2c device" in ACPI table (more flexible,
> but looks a bit strange)
>
> Do you have any preference/suggestions for above solutions? Thanks

I don't think we can hard code especially the high-speed mode because 
most typically buses are populated with slower devices.

Things are a bit more clear when ACPI provides timing parameters for the 
bus (for standard and fast speed modes at the moment in 
i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think the 
ACPI namespace walk may be needed against potential BIOS 
misconfigurations. For instance if it provides timing parameters for all 
speeds but there are devices with lower speed on the same bus.

I'd take these timing parameters as configuration data for bus features 
but actual speed (speed bits in IC_CON register) is defined separately. 
To me it looks only way to achieve that is to pick slowest device from 
I2cSerialBus resource descriptors.

-- 
Jarkko

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Andy Shevchenko]

On Thu, 2015-10-15 at 11:32 +0300, Jarkko Nikula wrote:
> On 10/15/2015 08:46 AM, Xiang Wang wrote:
> > 
> > In conclusion, we have 2 solutions to set the i2c controller speed
> > mode (pci driver):
> > 1) use hardcode value in pci driver
> > 2) use frequency setting of "i2c device" in ACPI table (more 
> > flexible,
> > but looks a bit strange)
> > 
> > Do you have any preference/suggestions for above solutions? Thanks
> 
> I don't think we can hard code especially the high-speed mode because 
> 
> most typically buses are populated with slower devices.
> 
> Things are a bit more clear when ACPI provides timing parameters for 
> the 
> bus (for standard and fast speed modes at the moment in 
> i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think 
> the 
> ACPI namespace walk may be needed against potential BIOS 
> misconfigurations. For instance if it provides timing parameters for 
> all 
> speeds but there are devices with lower speed on the same bus.
> 
> I'd take these timing parameters as configuration data for bus 
> features 
> but actual speed (speed bits in IC_CON register) is defined 
> separately. 
> To me it looks only way to achieve that is to pick slowest device 
> from 
> I2cSerialBus resource descriptors.

Should it (ACPI walk) be done in PCI case as well? If so, then it needs
to be done up to i2c-core. There you may adjust the bus speed whenever
slave device is enumerated.

For PCI case you have still to have hardcoded values and it should be
maximum supported by the bus I think. When you have implemented above
algorithm you may do this safely. Am I missing something?

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Xiang Wang]

2015-10-15 17:40 GMT+08:00 Andy Shevchenko <andriy.shevchenko@linux.intel.com>:
> On Thu, 2015-10-15 at 11:32 +0300, Jarkko Nikula wrote:
>> On 10/15/2015 08:46 AM, Xiang Wang wrote:
>> >
>> > In conclusion, we have 2 solutions to set the i2c controller speed
>> > mode (pci driver):
>> > 1) use hardcode value in pci driver
>> > 2) use frequency setting of "i2c device" in ACPI table (more
>> > flexible,
>> > but looks a bit strange)
>> >
>> > Do you have any preference/suggestions for above solutions? Thanks
>>
>> I don't think we can hard code especially the high-speed mode because
>>
>> most typically buses are populated with slower devices.
>>
>> Things are a bit more clear when ACPI provides timing parameters for
>> the
>> bus (for standard and fast speed modes at the moment in
>> i2c-designware-platdrv.c: dw_i2c_acpi_configure()) but still I think
>> the
>> ACPI namespace walk may be needed against potential BIOS
>> misconfigurations. For instance if it provides timing parameters for
>> all
>> speeds but there are devices with lower speed on the same bus.
>>
>> I'd take these timing parameters as configuration data for bus
>> features
>> but actual speed (speed bits in IC_CON register) is defined
>> separately.
>> To me it looks only way to achieve that is to pick slowest device
>> from
>> I2cSerialBus resource descriptors.
>
> Should it (ACPI walk) be done in PCI case as well? If so, then it needs
> to be done up to i2c-core. There you may adjust the bus speed whenever
> slave device is enumerated.
>
I think the "ACPI walk for bus speed" also works for PCI case. It'll
be good to do this in i2c-core.
By doing this we can get a minimum device speed for this bus. I2C bus
drivers can use this speed to set corresponding mode into i2c
controller.
Waiting for comments from others.

> For PCI case you have still to have hardcoded values and it should be
> maximum supported by the bus I think. When you have implemented above
> algorithm you may do this safely. Am I missing something?
Agree. It'll be safer to set a hardcoded maximum supported speed of
the bus for PCI case.
I2C bus driver can use MIN(speed_get_by_ACPI_walk,
hardcoded_max_supported_speed) for bus speed.

>
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy



-- 
Regards,
Xiang

Re: [PATCH 2/2] i2c: designware: enable High-speed mode for pcidrv

[Wolfram Sang]

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


> > Should it (ACPI walk) be done in PCI case as well? If so, then it needs
> > to be done up to i2c-core. There you may adjust the bus speed whenever
> > slave device is enumerated.
> >
> I think the "ACPI walk for bus speed" also works for PCI case. It'll
> be good to do this in i2c-core.
> By doing this we can get a minimum device speed for this bus. I2C bus
> drivers can use this speed to set corresponding mode into i2c
> controller.
> Waiting for comments from others.
> 
> > For PCI case you have still to have hardcoded values and it should be
> > maximum supported by the bus I think. When you have implemented above
> > algorithm you may do this safely. Am I missing something?
> Agree. It'll be safer to set a hardcoded maximum supported speed of
> the bus for PCI case.
> I2C bus driver can use MIN(speed_get_by_ACPI_walk,
> hardcoded_max_supported_speed) for bus speed.

Do I read this correctly that you agreed on an alternative path so this
series can be discarded?


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