[PATCH 0/2] i2c: designware-pci: extra features for PCI mode LPSS I2C

[PATCH 0/2] i2c: designware-pci: extra features for PCI mode LPSS I2C

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

These two patches contains the additional changes required for BayTrail LPSS
I2C on top of the patch that Mika Westerberg has submitted previously which
is still in the pending queue:
"[PATCH v2] i2c: designware-pci: Add Baytrail PCI IDs"
http://www.spinics.net/lists/linux-i2c/msg14709.html

Basically, the changes inclusive of:
i.	enable the pci glue layer to pass in target HCNT, LCNT
	and SDA hold time values to core layer if they are known
	beforehand, eg: for BayTrail.
ii.	declare the BayTrail LPSS I2C controllers are capable of
	supporting 10-bit addressing mode functionality.

Chew, Chiau Ee (2):
  i2c: designware-pci: add 10-bit addressing mode functionality for BYT
    I2C
  i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

 drivers/i2c/busses/i2c-designware-pcidrv.c |   51 ++++++++++++++++++++++++---
 1 files changed, 45 insertions(+), 6 deletions(-)

-- 
1.7.4.4

[PATCH 1/2] i2c: designware-pci: add 10-bit addressing mode functionality for BYT I2C

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

All the I2C controllers on Intel BayTrail LPSS subsystem able
to support 10-bit addressing mode functionality.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
Signed-off-by: Ong, Boon Leong <boon.leong.ong@intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index f1dabee..87f2fc4 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -64,12 +64,19 @@ struct dw_pci_controller {
 	u32 tx_fifo_depth;
 	u32 rx_fifo_depth;
 	u32 clk_khz;
+	u32 functionality;
 };
 
 #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |			\
 				DW_IC_CON_SLAVE_DISABLE |	\
 				DW_IC_CON_RESTART_EN)
 
+#define DW_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |			\
+					I2C_FUNC_SMBUS_BYTE |		\
+					I2C_FUNC_SMBUS_BYTE_DATA |	\
+					I2C_FUNC_SMBUS_WORD_DATA |	\
+					I2C_FUNC_SMBUS_I2C_BLOCK)
+
 static struct  dw_pci_controller  dw_pci_controllers[] = {
 	[moorestown_0] = {
 		.bus_num     = 0,
@@ -140,6 +147,7 @@ static struct  dw_pci_controller  dw_pci_controllers[] = {
 		.tx_fifo_depth = 32,
 		.rx_fifo_depth = 32,
 		.clk_khz = 100000,
+		.functionality = I2C_FUNC_10BIT_ADDR,
 	},
 };
 static struct i2c_algorithm i2c_dw_algo = {
@@ -256,12 +264,9 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz;
 	dev->base = pcim_iomap_table(pdev)[0];
 	dev->dev = &pdev->dev;
-	dev->functionality =
-		I2C_FUNC_I2C |
-		I2C_FUNC_SMBUS_BYTE |
-		I2C_FUNC_SMBUS_BYTE_DATA |
-		I2C_FUNC_SMBUS_WORD_DATA |
-		I2C_FUNC_SMBUS_I2C_BLOCK;
+	dev->functionality = controller->functionality |
+				DW_DEFAULT_FUNCTIONALITY;
+
 	dev->master_cfg =  controller->bus_cfg;
 
 	pci_set_drvdata(pdev, dev);
-- 
1.7.4.4

Re: [PATCH 1/2] i2c: designware-pci: add 10-bit addressing mode functionality for BYT I2C

[Wolfram Sang]

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

On Fri, Mar 07, 2014 at 10:12:50PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> All the I2C controllers on Intel BayTrail LPSS subsystem able
> to support 10-bit addressing mode functionality.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> Signed-off-by: Ong, Boon Leong <boon.leong.ong@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-pcidrv.c |   17 +++++++++++------
>  1 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index f1dabee..87f2fc4 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -64,12 +64,19 @@ struct dw_pci_controller {
>  	u32 tx_fifo_depth;
>  	u32 rx_fifo_depth;
>  	u32 clk_khz;
> +	u32 functionality;
>  };
>  
>  #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |			\
>  				DW_IC_CON_SLAVE_DISABLE |	\
>  				DW_IC_CON_RESTART_EN)
>  
> +#define DW_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |			\
> +					I2C_FUNC_SMBUS_BYTE |		\
> +					I2C_FUNC_SMBUS_BYTE_DATA |	\
> +					I2C_FUNC_SMBUS_WORD_DATA |	\
> +					I2C_FUNC_SMBUS_I2C_BLOCK)

Can't we have I2C_FUNC_SMBUS_EMUL here? (Need checking with
I2C_SMBUS_QUICK)


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

RE: [PATCH 1/2] i2c: designware-pci: add 10-bit addressing mode functionality for BYT I2C

[Chew, Chiau Ee]

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Sunday, March 09, 2014 4:54 PM
> To: Chew, Chiau Ee
> Cc: Mika Westerberg; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] i2c: designware-pci: add 10-bit addressing mode
> functionality for BYT I2C
> 
> On Fri, Mar 07, 2014 at 10:12:50PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> >
> > All the I2C controllers on Intel BayTrail LPSS subsystem able to
> > support 10-bit addressing mode functionality.
> >
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > Signed-off-by: Ong, Boon Leong <boon.leong.ong@intel.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-pcidrv.c |   17 +++++++++++------
> >  1 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c
> > b/drivers/i2c/busses/i2c-designware-pcidrv.c
> > index f1dabee..87f2fc4 100644
> > --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> > @@ -64,12 +64,19 @@ struct dw_pci_controller {
> >  	u32 tx_fifo_depth;
> >  	u32 rx_fifo_depth;
> >  	u32 clk_khz;
> > +	u32 functionality;
> >  };
> >
> >  #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |
> 	\
> >  				DW_IC_CON_SLAVE_DISABLE |	\
> >  				DW_IC_CON_RESTART_EN)
> >
> > +#define DW_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |
> 	\
> > +					I2C_FUNC_SMBUS_BYTE |
> 	\
> > +					I2C_FUNC_SMBUS_BYTE_DATA |
> 	\
> > +					I2C_FUNC_SMBUS_WORD_DATA |
> 	\
> > +					I2C_FUNC_SMBUS_I2C_BLOCK)
> 
> Can't we have I2C_FUNC_SMBUS_EMUL here? (Need checking with
> I2C_SMBUS_QUICK)

Per my understanding, this I2C host is not able to support I2C_SMBUS_QUICK.

Re: [PATCH 1/2] i2c: designware-pci: add 10-bit addressing mode functionality for BYT I2C

[Wolfram Sang]

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

> > > +#define DW_DEFAULT_FUNCTIONALITY (I2C_FUNC_I2C |
> > 	\
> > > +					I2C_FUNC_SMBUS_BYTE |
> > 	\
> > > +					I2C_FUNC_SMBUS_BYTE_DATA |
> > 	\
> > > +					I2C_FUNC_SMBUS_WORD_DATA |
> > 	\
> > > +					I2C_FUNC_SMBUS_I2C_BLOCK)
> > 
> > Can't we have I2C_FUNC_SMBUS_EMUL here? (Need checking with
> > I2C_SMBUS_QUICK)
> 
> Per my understanding, this I2C host is not able to support I2C_SMBUS_QUICK.

Ok. Thanks for checking, will apply this patch.


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

[PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

[Chew Chiau Ee]

From: Chew, Chiau Ee <chiau.ee.chew@intel.com>

On Intel BayTrail, there was case whereby the resulting fast mode
bus speed becomes slower (~20% slower compared to expected speed)
if using the HCNT/LCNT calculated in the core layer. Thus, this
patch is added to allow pci glue layer to pass in optimal
HCNT/LCNT/SDA hold time values to core layer since the core
layer supports cofigurable HCNT/LCNT/SDA hold time values now.

Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c |   34 ++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index 87f2fc4..96417ca 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -58,6 +58,14 @@ enum dw_pci_ctl_id_t {
 	baytrail,
 };
 
+struct scl_sda_cfg {
+	u32 ss_hcnt;
+	u32 fs_hcnt;
+	u32 ss_lcnt;
+	u32 fs_lcnt;
+	u32 sda_hold;
+};
+
 struct dw_pci_controller {
 	u32 bus_num;
 	u32 bus_cfg;
@@ -65,6 +73,7 @@ struct dw_pci_controller {
 	u32 rx_fifo_depth;
 	u32 clk_khz;
 	u32 functionality;
+	struct scl_sda_cfg *scl_sda_cfg;
 };
 
 #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |			\
@@ -77,6 +86,21 @@ struct dw_pci_controller {
 					I2C_FUNC_SMBUS_WORD_DATA |	\
 					I2C_FUNC_SMBUS_I2C_BLOCK)
 
+/* BayTrail HCNT/LCNT/SDA_HOLD */
+#define BYT_STD_MODE_HCNT	0x200
+#define BYT_STD_MODE_LCNT	BYT_STD_MODE_HCNT
+#define BYT_FAST_MODE_HCNT	0x55
+#define BYT_FAST_MODE_LCNT	0x99
+#define BYT_SDA_HOLD		0x6
+
+static struct scl_sda_cfg byt_config = {
+	.ss_hcnt	= BYT_STD_MODE_HCNT,
+	.fs_hcnt	= BYT_FAST_MODE_HCNT,
+	.ss_lcnt	= BYT_STD_MODE_LCNT,
+	.fs_lcnt	= BYT_FAST_MODE_LCNT,
+	.sda_hold	= BYT_SDA_HOLD,
+};
+
 static struct  dw_pci_controller  dw_pci_controllers[] = {
 	[moorestown_0] = {
 		.bus_num     = 0,
@@ -148,6 +172,7 @@ static struct  dw_pci_controller  dw_pci_controllers[] = {
 		.rx_fifo_depth = 32,
 		.clk_khz = 100000,
 		.functionality = I2C_FUNC_10BIT_ADDR,
+		.scl_sda_cfg = &byt_config,
 	},
 };
 static struct i2c_algorithm i2c_dw_algo = {
@@ -231,6 +256,7 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 	struct i2c_adapter *adap;
 	int r;
 	struct  dw_pci_controller *controller;
+	struct scl_sda_cfg *cfg;
 
 	if (id->driver_data >= ARRAY_SIZE(dw_pci_controllers)) {
 		dev_err(&pdev->dev, "%s: invalid driver data %ld\n", __func__,
@@ -268,6 +294,14 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev,
 				DW_DEFAULT_FUNCTIONALITY;
 
 	dev->master_cfg =  controller->bus_cfg;
+	if (controller->scl_sda_cfg) {
+		cfg = controller->scl_sda_cfg;
+		dev->ss_hcnt = cfg->ss_hcnt;
+		dev->fs_hcnt = cfg->fs_hcnt;
+		dev->ss_lcnt = cfg->ss_lcnt;
+		dev->fs_lcnt = cfg->fs_lcnt;
+		dev->sda_hold_time = cfg->sda_hold;
+	}
 
 	pci_set_drvdata(pdev, dev);
 
-- 
1.7.4.4

Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

[Wolfram Sang]

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

On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> On Intel BayTrail, there was case whereby the resulting fast mode
> bus speed becomes slower (~20% slower compared to expected speed)
> if using the HCNT/LCNT calculated in the core layer. Thus, this
> patch is added to allow pci glue layer to pass in optimal
> HCNT/LCNT/SDA hold time values to core layer since the core
> layer supports cofigurable HCNT/LCNT/SDA hold time values now.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>

Can you make use of those instead?

        u32                     sda_falling_time;
        u32                     scl_falling_time;

This is more consistent with using sda_hold_time and lets them have a
common (and more readable) unit.


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

RE: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

[Chew, Chiau Ee]

> -----Original Message-----
> From: Wolfram Sang [mailto:wsa@the-dreams.de]
> Sent: Sunday, March 09, 2014 5:04 PM
> To: Chew, Chiau Ee
> Cc: Mika Westerberg; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA
> hold time value
> 
> On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote:
> > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> >
> > On Intel BayTrail, there was case whereby the resulting fast mode bus
> > speed becomes slower (~20% slower compared to expected speed) if using
> > the HCNT/LCNT calculated in the core layer. Thus, this patch is added
> > to allow pci glue layer to pass in optimal HCNT/LCNT/SDA hold time
> > values to core layer since the core layer supports cofigurable
> > HCNT/LCNT/SDA hold time values now.
> >
> > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> Can you make use of those instead?
> 
>         u32                     sda_falling_time;
>         u32                     scl_falling_time;
> 
> This is more consistent with using sda_hold_time and lets them have a common
> (and more readable) unit.

Would like to clarify on your statement above. So you are suggesting to change the following 
variable name in the code? As in:
"u32 ss_hcnt;"   to  "u32 ss_scl_rising_time"
"u32 fs_hcnt;"   to  "u32 fs_scl_rising_time"
"u32 ss_lcnt;"    to   "u32 ss_scl_falling_time"
"u32 fs_lcnt;"     to " u32 fs_scl_falling_time"
"u32 sda_hold;"  to  "u32 sda_hold_time"

Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

[Wolfram Sang]

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

On Mon, Mar 10, 2014 at 11:34:49AM +0000, Chew, Chiau Ee wrote:
> 
> 
> > -----Original Message-----
> > From: Wolfram Sang [mailto:wsa@the-dreams.de]
> > Sent: Sunday, March 09, 2014 5:04 PM
> > To: Chew, Chiau Ee
> > Cc: Mika Westerberg; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA
> > hold time value
> > 
> > On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote:
> > > From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > >
> > > On Intel BayTrail, there was case whereby the resulting fast mode bus
> > > speed becomes slower (~20% slower compared to expected speed) if using
> > > the HCNT/LCNT calculated in the core layer. Thus, this patch is added
> > > to allow pci glue layer to pass in optimal HCNT/LCNT/SDA hold time
> > > values to core layer since the core layer supports cofigurable
> > > HCNT/LCNT/SDA hold time values now.
> > >
> > > Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> > 
> > Can you make use of those instead?
> > 
> >         u32                     sda_falling_time;
> >         u32                     scl_falling_time;
> > 
> > This is more consistent with using sda_hold_time and lets them have a common
> > (and more readable) unit.
> 
> Would like to clarify on your statement above. So you are suggesting to change the following 
> variable name in the code? As in:

Nope. But I had a look and it doesn't make sense to use the above. So,
just forget it :) Will send a second review.


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

Re: [PATCH 2/2] i2c: designware-pci: set ideal HCNT, LCNT and SDA hold time value

[Wolfram Sang]

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

On Fri, Mar 07, 2014 at 10:12:51PM +0800, Chew Chiau Ee wrote:
> From: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> 
> On Intel BayTrail, there was case whereby the resulting fast mode
> bus speed becomes slower (~20% slower compared to expected speed)
> if using the HCNT/LCNT calculated in the core layer. Thus, this
> patch is added to allow pci glue layer to pass in optimal
> HCNT/LCNT/SDA hold time values to core layer since the core
> layer supports cofigurable HCNT/LCNT/SDA hold time values now.
> 
> Signed-off-by: Chew, Chiau Ee <chiau.ee.chew@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-pcidrv.c |   34 ++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
> index 87f2fc4..96417ca 100644
> --- a/drivers/i2c/busses/i2c-designware-pcidrv.c
> +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
> @@ -58,6 +58,14 @@ enum dw_pci_ctl_id_t {
>  	baytrail,
>  };
>  
> +struct scl_sda_cfg {
> +	u32 ss_hcnt;
> +	u32 fs_hcnt;
> +	u32 ss_lcnt;
> +	u32 fs_lcnt;
> +	u32 sda_hold;
> +};

'dw_' prefix for this struct name, please.

>  #define INTEL_MID_STD_CFG  (DW_IC_CON_MASTER |			\
> @@ -77,6 +86,21 @@ struct dw_pci_controller {
>  					I2C_FUNC_SMBUS_WORD_DATA |	\
>  					I2C_FUNC_SMBUS_I2C_BLOCK)
>  
> +/* BayTrail HCNT/LCNT/SDA_HOLD */
> +#define BYT_STD_MODE_HCNT	0x200
> +#define BYT_STD_MODE_LCNT	BYT_STD_MODE_HCNT
> +#define BYT_FAST_MODE_HCNT	0x55
> +#define BYT_FAST_MODE_LCNT	0x99
> +#define BYT_SDA_HOLD		0x6

I'd think you can use these values directly in the struct. They are not
supposed to be used outside anyhow.

> +static struct scl_sda_cfg byt_config = {
> +	.ss_hcnt	= BYT_STD_MODE_HCNT,
> +	.fs_hcnt	= BYT_FAST_MODE_HCNT,
> +	.ss_lcnt	= BYT_STD_MODE_LCNT,
> +	.fs_lcnt	= BYT_FAST_MODE_LCNT,
> +	.sda_hold	= BYT_SDA_HOLD,
> +};

I'd prefer no tabs before "=", just one space.

Thanks,

   Wolfram


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