[PATCH] Set sda-hold-time based on ACPI *CNT value

[PATCH] Set sda-hold-time based on ACPI *CNT value

[chin.yew.tan]

From: Tan Chin Yew <chin.yew.tan@intel.com>

For I2c to operate correctly under all speed mode, sda-hold-time need to
be perfectly tuned. However, sda-hold-time is precalculated according to
circuit parameter which make it platform-specific.

In order to get accurate sda-hold-time for all platforms, pretuned
sda-hold-time for particular platform is stored in ACPI table and driver
to load the sda holding time from ACPI table.

This patch read the I2c sda-hold-time from ACPI table and assigned the
suitable hold time based on the i2c clock frequency.

Tested on Intel Apollo Lake.

Tan Chin Yew (1):
  i2c: designware: Get selected speed mode sda-hold-time via ACPI

 drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH] Set sda-hold-time based on ACPI *CNT value

[chin.yew.tan]

From: Tan Chin Yew <chin.yew.tan@intel.com>

For I2c to operate correctly under all speed mode, sda-hold-time need to
be perfectly tuned. However, sda-hold-time is precalculated according to
circuit parameter which make it platform-specific.

In order to get accurate sda-hold-time for all platforms, pretuned
sda-hold-time for particular platform is stored in ACPI table and driver
to load the sda holding time from ACPI table.

This patch read the I2c sda-hold-time from ACPI table and assigned the
suitable hold time based on the i2c clock frequency.

Tested on Intel Apollo Lake.

Tan Chin Yew (1):
  i2c: designware: Get selected speed mode sda-hold-time via ACPI

 drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

-- 
1.9.1

[PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[chin.yew.tan]

From: Tan Chin Yew <chin.yew.tan@intel.com>

Sda-hold-time is an important parameter for tuning i2c to meet the
electrical specification especially for high speed. I2C with incorrect
sda-hold-time may cause lost arbitration error. Now, the driver is able to
get sda-hold-time for all the speed supported.

Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 6ce4313..aa33088 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -101,15 +101,28 @@ static int dw_i2c_acpi_configure(struct platform_device *pdev)
 	dev->rx_fifo_depth = 32;
 
 	/*
-	 * Try to get SDA hold time and *CNT values from an ACPI method if
-	 * it exists for both supported speed modes.
+	 * Try to get SDA hold time and *CNT values from an ACPI method for
+	 * selected speed modes.
 	 */
-	dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev->ss_lcnt, NULL);
-	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt,
+	switch (dev->clk_freq) {
+	case 100000:
+		dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev->ss_lcnt,
 			   &dev->sda_hold_time);
-	dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev->fp_lcnt, NULL);
-	dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev->hs_lcnt, NULL);
-
+		break;
+	case 1000000:
+		dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev->fp_lcnt,
+			   &dev->sda_hold_time);
+		break;
+	case 3400000:
+		dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev->hs_lcnt,
+			   &dev->sda_hold_time);
+		break;
+	default:
+		dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev->fs_lcnt,
+			   &dev->sda_hold_time);
+		break;
+	}
+
 	id = acpi_match_device(pdev->dev.driver->acpi_match_table, &pdev->dev);
 	if (id && id->driver_data)
 		dev->accessor_flags |= (u32)id->driver_data;
-- 
1.9.1

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 10.02.2017 13:28, chin.yew.tan@intel.com wrote:
> From: Tan Chin Yew <chin.yew.tan@intel.com>
>
> Sda-hold-time is an important parameter for tuning i2c to meet the
> electrical specification especially for high speed. I2C with incorrect
> sda-hold-time may cause lost arbitration error. Now, the driver is able to
> get sda-hold-time for all the speed supported.
>
> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

On Fri, 2017-02-10 at 19:28 +0800, chin.yew.tan@intel.com wrote:
> From: Tan Chin Yew <chin.yew.tan@intel.com>
> 
> Sda-hold-time is an important parameter for tuning i2c to meet the
> electrical specification especially for high speed. I2C with incorrect
> sda-hold-time may cause lost arbitration error. Now, the driver is
> able to
> get sda-hold-time for all the speed supported.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Couple of nitpicks below.

> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c | 27
> ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 6ce4313..aa33088 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -101,15 +101,28 @@ static int dw_i2c_acpi_configure(struct
> platform_device *pdev)
>  	dev->rx_fifo_depth = 32;
>  
>  	/*
> -	 * Try to get SDA hold time and *CNT values from an ACPI
> method if
> -	 * it exists for both supported speed modes.
> +	 * Try to get SDA hold time and *CNT values from an ACPI
> method for
> +	 * selected speed modes.
>  	 */
> -	dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
> >ss_lcnt, NULL);
> -	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev-
> >fs_lcnt,
> +	switch (dev->clk_freq) {
> +	case 100000:
> +		dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
> >ss_lcnt,

>  			   &dev->sda_hold_time);

This indentation should go in a way that & character in the same column
as p (in "p(s" context above).

> -	dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
> >fp_lcnt, NULL);
> -	dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
> >hs_lcnt, NULL);
> -
> +		break;
> +	case 1000000:
> +		dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
> >fp_lcnt,
> +			   &dev->sda_hold_time);
> +		break;
> +	case 3400000:
> +		dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
> >hs_lcnt,
> +			   &dev->sda_hold_time);
> +		break;

Can we prepend default with

case 400000:

here?

> +	default:


> +		dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev-
> >fs_lcnt,
> +			   &dev->sda_hold_time);
> +		break;
> +	}
> +
>  	id = acpi_match_device(pdev->dev.driver->acpi_match_table,
> &pdev->dev);
>  	if (id && id->driver_data)
>  		dev->accessor_flags |= (u32)id->driver_data;

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

RE: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Tan, Chin Yew]

> On Fri, 2017-02-10 at 19:28 +0800, chin.yew.tan@intel.com wrote:
> > From: Tan Chin Yew <chin.yew.tan@intel.com>
> >
> > Sda-hold-time is an important parameter for tuning i2c to meet the
> > electrical specification especially for high speed. I2C with incorrect
> > sda-hold-time may cause lost arbitration error. Now, the driver is
> > able to get sda-hold-time for all the speed supported.
> >
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Couple of nitpicks below.
> 
> > Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
> > ---
> >  drivers/i2c/busses/i2c-designware-platdrv.c | 27
> > ++++++++++++++++++++-------
> >  1 file changed, 20 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
> > b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 6ce4313..aa33088 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -101,15 +101,28 @@ static int dw_i2c_acpi_configure(struct
> > platform_device *pdev)
> >  	dev->rx_fifo_depth = 32;
> >
> >  	/*
> > -	 * Try to get SDA hold time and *CNT values from an ACPI
> > method if
> > -	 * it exists for both supported speed modes.
> > +	 * Try to get SDA hold time and *CNT values from an ACPI
> > method for
> > +	 * selected speed modes.
> >  	 */
> > -	dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
> > >ss_lcnt, NULL);
> > -	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev-
> > >fs_lcnt,
> > +	switch (dev->clk_freq) {
> > +	case 100000:
> > +		dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
> > >ss_lcnt,
> 
> >  			   &dev->sda_hold_time);
> 
> This indentation should go in a way that & character in the same column as p
> (in "p(s" context above).
I will realign the code according to suggestion.

> 
> > -	dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
> > >fp_lcnt, NULL);
> > -	dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
> > >hs_lcnt, NULL);
> > -
> > +		break;
> > +	case 1000000:
> > +		dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
> > >fp_lcnt,
> > +			   &dev->sda_hold_time);
> > +		break;
> > +	case 3400000:
> > +		dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
> > >hs_lcnt,
> > +			   &dev->sda_hold_time);
> > +		break;
> 
> Can we prepend default with
> 
> case 400000:
> 
> here?
> 
Yes, you are right, it is best not to load settings for speed mode that is 
not supported.

> > +	default:
> 
> 
> > +		dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev-
> > >fs_lcnt,
> > +			   &dev->sda_hold_time);
> > +		break;
> > +	}
> > +
> >  	id = acpi_match_device(pdev->dev.driver->acpi_match_table,
> > &pdev->dev);
> >  	if (id && id->driver_data)
> >  		dev->accessor_flags |= (u32)id->driver_data;
> 
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Jarkko Nikula]

On 13.02.2017 10:41, Tan, Chin Yew wrote:
>
>> On Fri, 2017-02-10 at 19:28 +0800, chin.yew.tan@intel.com wrote:
>>> From: Tan Chin Yew <chin.yew.tan@intel.com>
>>>
>>> Sda-hold-time is an important parameter for tuning i2c to meet the
>>> electrical specification especially for high speed. I2C with incorrect
>>> sda-hold-time may cause lost arbitration error. Now, the driver is
>>> able to get sda-hold-time for all the speed supported.
>>>
>>
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>>
>> Couple of nitpicks below.
>>
>>> Signed-off-by: Tan Chin Yew <chin.yew.tan@intel.com>
>>> ---
>>>  drivers/i2c/busses/i2c-designware-platdrv.c | 27
>>> ++++++++++++++++++++-------
>>>  1 file changed, 20 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> index 6ce4313..aa33088 100644
>>> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
>>> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
>>> @@ -101,15 +101,28 @@ static int dw_i2c_acpi_configure(struct
>>> platform_device *pdev)
>>>  	dev->rx_fifo_depth = 32;
>>>
>>>  	/*
>>> -	 * Try to get SDA hold time and *CNT values from an ACPI
>>> method if
>>> -	 * it exists for both supported speed modes.
>>> +	 * Try to get SDA hold time and *CNT values from an ACPI
>>> method for
>>> +	 * selected speed modes.
>>>  	 */
>>> -	dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
>>>> ss_lcnt, NULL);
>>> -	dw_i2c_acpi_params(pdev, "FMCN", &dev->fs_hcnt, &dev-
>>>> fs_lcnt,
>>> +	switch (dev->clk_freq) {
>>> +	case 100000:
>>> +		dw_i2c_acpi_params(pdev, "SSCN", &dev->ss_hcnt, &dev-
>>>> ss_lcnt,
>>
>>>  			   &dev->sda_hold_time);
>>
>> This indentation should go in a way that & character in the same column as p
>> (in "p(s" context above).
> I will realign the code according to suggestion.
>
>>
>>> -	dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
>>>> fp_lcnt, NULL);
>>> -	dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
>>>> hs_lcnt, NULL);
>>> -
>>> +		break;
>>> +	case 1000000:
>>> +		dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, &dev-
>>>> fp_lcnt,
>>> +			   &dev->sda_hold_time);
>>> +		break;
>>> +	case 3400000:
>>> +		dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, &dev-
>>>> hs_lcnt,
>>> +			   &dev->sda_hold_time);
>>> +		break;
>>
>> Can we prepend default with
>>
>> case 400000:
>>
>> here?
>>
> Yes, you are right, it is best not to load settings for speed mode that is
> not supported.
>
Andy: I guess you were looking for adding "case 400000:" for readability 
rather than removing the default case?

I think it's best to keep fall back to 400 kHz speed that has been the 
default in this driver in case we get some not supported speed from ACPI.

-- 
Jarkko

Re: [PATCH] i2c: designware: Get selected speed mode sda-hold-time via ACPI

[Andy Shevchenko]

On Mon, 2017-02-13 at 11:33 +0200, Jarkko Nikula wrote:
> On 13.02.2017 10:41, Tan, Chin Yew wrote:
> > 
> > > On Fri, 2017-02-10 at 19:28 +0800, chin.yew.tan@intel.com wrote:
> > > > From: Tan Chin Yew <chin.yew.tan@intel.com>
> > > > 
> > > > Sda-hold-time is an important parameter for tuning i2c to meet
> > > > the
> > > > electrical specification especially for high speed. I2C with
> > > > incorrect
> > > > sda-hold-time may cause lost arbitration error. Now, the driver
> > > > is
> > > > able to get sda-hold-time for all the speed supported.

> > > > +	case 1000000:
> > > > +		dw_i2c_acpi_params(pdev, "FPCN", &dev->fp_hcnt, 
> > > > &dev-
> > > > > fp_lcnt,
> > > > 
> > > > +			   &dev->sda_hold_time);
> > > > +		break;
> > > > +	case 3400000:
> > > > +		dw_i2c_acpi_params(pdev, "HSCN", &dev->hs_hcnt, 
> > > > &dev-
> > > > > hs_lcnt,
> > > > 
> > > > +			   &dev->sda_hold_time);
> > > > +		break;
> > > 
> > > Can we prepend default with
> > > 
> > > case 400000:
> > > 
> > > here?
> > > 
> > 
> > Yes, you are right, it is best not to load settings for speed mode
> > that is
> > not supported.
> > 
> 
> Andy: I guess you were looking for adding "case 400000:" for
> readability 
> rather than removing the default case?

Correct. To explicitly show that default we rather assume 400000, but if
 it's not, still go that branch.

> 
> I think it's best to keep fall back to 400 kHz speed that has been
> the 
> default in this driver in case we get some not supported speed from
> ACPI.

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