[PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Add APM X-Gene ACPI I2C device support. These patches follow
the same implementation as AMD I2C driver - changes in ACPI APD
and Designware I2C drivers.

v2:
* Spilt the acpi_apd change with defines for AMD and X-Gene I2C's

Signed-off-by: Loc Ho <lho@apm.com>
---
Loc Ho (2):
  acpi:apd: Add APM X-Gene ACPI I2C device support
  i2c:dw: Add APM X-Gene ACPI I2C device support

 drivers/acpi/acpi_apd.c                     |   16 +++++++++++++++-
 drivers/i2c/busses/i2c-designware-platdrv.c |    1 +
 2 files changed, 16 insertions(+), 1 deletions(-)

[PATCH v2 1/2] acpi:apd: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Add APM X-Gene ACPI I2C device support by hooks into existent
ACPI apd driver. To fully enable support, require another
patch to add the X-Gene ACPI node into the DW I2C driver.

Signed-off-by: Loc Ho <lho@apm.com>
---
 drivers/acpi/acpi_apd.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
index a450e7a..d507cf6 100644
--- a/drivers/acpi/acpi_apd.c
+++ b/drivers/acpi/acpi_apd.c
@@ -51,7 +51,7 @@ struct apd_private_data {
 	const struct apd_device_desc *dev_desc;
 };
 
-#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
+#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
 #define APD_ADDR(desc)	((unsigned long)&desc)
 
 static int acpi_apd_setup(struct apd_private_data *pdata)
@@ -71,6 +71,7 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
 	return 0;
 }
 
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
 static struct apd_device_desc cz_i2c_desc = {
 	.setup = acpi_apd_setup,
 	.fixed_clk_rate = 133000000,
@@ -80,6 +81,14 @@ static struct apd_device_desc cz_uart_desc = {
 	.setup = acpi_apd_setup,
 	.fixed_clk_rate = 48000000,
 };
+#endif
+
+#ifdef CONFIG_ARM64
+static struct apd_device_desc xgene_i2c_desc = {
+	.setup = acpi_apd_setup,
+	.fixed_clk_rate = 100000000,
+};
+#endif
 
 #else
 
@@ -132,9 +141,14 @@ static int acpi_apd_create_device(struct acpi_device *adev,
 
 static const struct acpi_device_id acpi_apd_device_ids[] = {
 	/* Generic apd devices */
+#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
 	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
 	{ "AMD0020", APD_ADDR(cz_uart_desc) },
 	{ "AMD0030", },
+#endif
+#ifdef CONFIG_ARM64
+	{ "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
+#endif
 	{ }
 };
 
-- 
1.7.1

Re: [PATCH v2 1/2] acpi:apd: Add APM X-Gene ACPI I2C device support

[Ken Xue]

On Thu, 2015-12-10 at 14:19 -0700, Loc Ho wrote:
> Add APM X-Gene ACPI I2C device support by hooks into existent
> ACPI apd driver. To fully enable support, require another
> patch to add the X-Gene ACPI node into the DW I2C driver.
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> ---
>  drivers/acpi/acpi_apd.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> index a450e7a..d507cf6 100644
> --- a/drivers/acpi/acpi_apd.c
> +++ b/drivers/acpi/acpi_apd.c
> @@ -51,7 +51,7 @@ struct apd_private_data {
>  	const struct apd_device_desc *dev_desc;
>  };
>  
> -#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +#if defined(CONFIG_X86_AMD_PLATFORM_DEVICE) || defined(CONFIG_ARM64)
>  #define APD_ADDR(desc)	((unsigned long)&desc)
>  
>  static int acpi_apd_setup(struct apd_private_data *pdata)
> @@ -71,6 +71,7 @@ static int acpi_apd_setup(struct apd_private_data *pdata)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>  static struct apd_device_desc cz_i2c_desc = {
>  	.setup = acpi_apd_setup,
>  	.fixed_clk_rate = 133000000,
> @@ -80,6 +81,14 @@ static struct apd_device_desc cz_uart_desc = {
>  	.setup = acpi_apd_setup,
>  	.fixed_clk_rate = 48000000,
>  };
> +#endif
> +
> +#ifdef CONFIG_ARM64
> +static struct apd_device_desc xgene_i2c_desc = {
> +	.setup = acpi_apd_setup,
> +	.fixed_clk_rate = 100000000,
> +};
> +#endif
>  
>  #else
>  
> @@ -132,9 +141,14 @@ static int acpi_apd_create_device(struct acpi_device *adev,
>  
>  static const struct acpi_device_id acpi_apd_device_ids[] = {
>  	/* Generic apd devices */
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
>  	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
>  	{ "AMD0020", APD_ADDR(cz_uart_desc) },
>  	{ "AMD0030", },
> +#endif
> +#ifdef CONFIG_ARM64
> +	{ "APMC0D0F", APD_ADDR(xgene_i2c_desc) },
> +#endif
>  	{ }
>  };
>  

Reviewed-by: Ken Xue <Ken.Xue@amd.com>

[PATCH v2 2/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Enable APM X-Gene ACPI I2C device support by adding the
corresponding ACPI ID. The platform ACPI APD corresponding
change is required to provide the proper clock frequency input.

Signed-off-by: Loc Ho <lho@apm.com>
Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 809579e..423371d 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -117,6 +117,7 @@ static const struct acpi_device_id dw_i2c_acpi_match[] = {
 	{ "80860F41", 0 },
 	{ "808622C1", 0 },
 	{ "AMD0010", 0 },
+	{ "APMC0D0F", 0 },
 	{ }
 };
 MODULE_DEVICE_TABLE(acpi, dw_i2c_acpi_match);
-- 
1.7.1

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Suravee Suthikulanit]

Hi,

I'm not sure if this has been discussed earlier. But after looking at 
the the acpi_apd driver, all we need is just the platform-specific input 
clock frequency value used by the 
drivers/i2c/busses/i2c-designware-core.c: i2c_dw_init() to calculate the 
values to program into the DW_IC_SS_SCL_HCNT and DW_IC_SS_SCL_LCNT 
registers.

So, instead of hard-coding this value into the driver/acpi/acpi_apd.c 
(which really has nothing to do with the I2C driver), what if we 
introduce a new ACPI key-value pair in the ACPI DSDT such as:

     Device(I2C0)    // I2C controller
     {
       Name(_HID, "AMDI0510")
       Name(_UID, 0)
       Name(_CRS, ResourceTemplate() {
         Memory32Fixed (ReadWrite, 0xE1000000, 0x1000)
         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 
389 } // GSIV
       })
       Name (_DSD, Package () {
         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
         Package () {
           Package (2) {"clock-source-frequency", 100000000 }
         }
       }) // _DSD()
     }


Then, in the drivers/i2c/busses/i2c-designware-platdrv.c, we can add the 
code to parse this information. This way, the I2C driver can just 
retrieve this value from the ACPI table of each platform, which should 
make it easier to manage. Eventually, we can free I2C driver from the 
driver/acpi/acpi_apd.c, which is currently only used by AMD x86 platform.

Also, this approach would allow the I2C drivers in other OSes to use 
this same information in the ACPI table. If this approach is okay, I can 
provide a patch to implement this.

Thanks,
Suravee

On 12/10/2015 3:19 PM, Loc Ho wrote:
> Add APM X-Gene ACPI I2C device support. These patches follow
> the same implementation as AMD I2C driver - changes in ACPI APD
> and Designware I2C drivers.
>
> v2:
> * Spilt the acpi_apd change with defines for AMD and X-Gene I2C's
>
> Signed-off-by: Loc Ho <lho@apm.com>
> ---
> Loc Ho (2):
>    acpi:apd: Add APM X-Gene ACPI I2C device support
>    i2c:dw: Add APM X-Gene ACPI I2C device support
>
>   drivers/acpi/acpi_apd.c                     |   16 +++++++++++++++-
>   drivers/i2c/busses/i2c-designware-platdrv.c |    1 +
>   2 files changed, 16 insertions(+), 1 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
> I'm not sure if this has been discussed earlier. But after looking at the
> the acpi_apd driver, all we need is just the platform-specific input clock
> frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
> i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
> and DW_IC_SS_SCL_LCNT registers.

There is a way to pass *CNT values already from ACPI to the driver -- It
looks for method called FMCN (or SSCN) and retrieves the values from
there if found.

The driver could be modified not to require clock if it already knows
*CNT values.

> So, instead of hard-coding this value into the driver/acpi/acpi_apd.c (which
> really has nothing to do with the I2C driver), what if we introduce a new
> ACPI key-value pair in the ACPI DSDT such as:
> 
>     Device(I2C0)    // I2C controller
>     {
>       Name(_HID, "AMDI0510")
>       Name(_UID, 0)
>       Name(_CRS, ResourceTemplate() {
>         Memory32Fixed (ReadWrite, 0xE1000000, 0x1000)
>         Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 389
> } // GSIV
>       })
>       Name (_DSD, Package () {
>         ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
>         Package () {
>           Package (2) {"clock-source-frequency", 100000000 }

If this is to be done then I think it is better to use Linux clk
framework to provide the clock (with corresponding properties in place).

But ACPI specification wants clocks as PowerResources and those do not
have support for reading rate of the clock.

>         }
>       }) // _DSD()
>     }

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Suravee Suthikulpanit]

On 12/15/2015 07:27 AM, Mika Westerberg wrote:
> On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
>> >I'm not sure if this has been discussed earlier. But after looking at the
>> >the acpi_apd driver, all we need is just the platform-specific input clock
>> >frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
>> >i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
>> >and DW_IC_SS_SCL_LCNT registers.
> There is a way to pass *CNT values already from ACPI to the driver -- It
> looks for method called FMCN (or SSCN) and retrieves the values from
> there if found.

Right, I also noticed this afterward. By the way, are FMCN and SSCN 
documented anywhere in the ACPI spec?  I am trying to figure out how to 
update the ACPI table to add this information for the AMD Seattle 
(ARM64) platform, and I will also submit a patch to add the new HID for 
this driver.

> The driver could be modified not to require clock if it already knows
> *CNT values.
>

Sounds good.

Thanks,
Suravee

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Tue, Dec 15, 2015 at 08:52:19AM -0600, Suravee Suthikulpanit wrote:
> 
> 
> On 12/15/2015 07:27 AM, Mika Westerberg wrote:
> >On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
> >>>I'm not sure if this has been discussed earlier. But after looking at the
> >>>the acpi_apd driver, all we need is just the platform-specific input clock
> >>>frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
> >>>i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
> >>>and DW_IC_SS_SCL_LCNT registers.
> >There is a way to pass *CNT values already from ACPI to the driver -- It
> >looks for method called FMCN (or SSCN) and retrieves the values from
> >there if found.
> 
> Right, I also noticed this afterward. By the way, are FMCN and SSCN
> documented anywhere in the ACPI spec?  I am trying to figure out how to
> update the ACPI table to add this information for the AMD Seattle (ARM64)
> platform, and I will also submit a patch to add the new HID for this driver.

No, they are Intel inventions for the Windows I2C driver.

Here is what I know about it:
  SSCN - Standard Mode CNTs
  FMCN - Fast Mode CNTs

They both return a package:

  Package() {
    HCNT,
    LCNT,
    SDA_hold_time,
  }

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Hi,

>>
>> On 12/15/2015 07:27 AM, Mika Westerberg wrote:
>> >On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
>> >>>I'm not sure if this has been discussed earlier. But after looking at the
>> >>>the acpi_apd driver, all we need is just the platform-specific input clock
>> >>>frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
>> >>>i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
>> >>>and DW_IC_SS_SCL_LCNT registers.
>> >There is a way to pass *CNT values already from ACPI to the driver -- It
>> >looks for method called FMCN (or SSCN) and retrieves the values from
>> >there if found.
>>
>> Right, I also noticed this afterward. By the way, are FMCN and SSCN
>> documented anywhere in the ACPI spec?  I am trying to figure out how to
>> update the ACPI table to add this information for the AMD Seattle (ARM64)
>> platform, and I will also submit a patch to add the new HID for this driver.
>
> No, they are Intel inventions for the Windows I2C driver.
>
> Here is what I know about it:
>   SSCN - Standard Mode CNTs
>   FMCN - Fast Mode CNTs
>
> They both return a package:
>
>   Package() {
>     HCNT,
>     LCNT,
>     SDA_hold_time,
>   }

Can we not just add an AML method that will return the operation clock
frequency that the I2C driver can use? If the method doesn't existed,
we will just bail and do nothing or assume what ever default behavior?

-Loc

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Tue, Dec 15, 2015 at 11:20:03AM -0800, Loc Ho wrote:
> Can we not just add an AML method that will return the operation clock
> frequency that the I2C driver can use? If the method doesn't existed,
> we will just bail and do nothing or assume what ever default behavior?

Why would you add yet another method? There already are existing ACPI
*CNT methods that you can use, and are used in systems out there.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Jarkko Nikula]

On 12/16/2015 11:04 AM, Mika Westerberg wrote:
> On Tue, Dec 15, 2015 at 11:20:03AM -0800, Loc Ho wrote:
>> Can we not just add an AML method that will return the operation clock
>> frequency that the I2C driver can use? If the method doesn't existed,
>> we will just bail and do nothing or assume what ever default behavior?
>
> Why would you add yet another method? There already are existing ACPI
> *CNT methods that you can use, and are used in systems out there.

One thing to remember that *CNT methods allow more precise machine and 
per bus specific tunings against different HW characteristics like 
pull-up resistance, line load, propagation delay, etc.

-- 
Jarkko

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Ken Xue]

On Tue, 2015-12-15 at 16:59 +0200, Mika Westerberg wrote:
> On Tue, Dec 15, 2015 at 08:52:19AM -0600, Suravee Suthikulpanit wrote:
> > 
> > 
> > On 12/15/2015 07:27 AM, Mika Westerberg wrote:
> > >On Mon, Dec 14, 2015 at 06:53:25PM -0600, Suravee Suthikulanit wrote:
> > >>>I'm not sure if this has been discussed earlier. But after looking at the
> > >>>the acpi_apd driver, all we need is just the platform-specific input clock
> > >>>frequency value used by the drivers/i2c/busses/i2c-designware-core.c:
> > >>>i2c_dw_init() to calculate the values to program into the DW_IC_SS_SCL_HCNT
> > >>>and DW_IC_SS_SCL_LCNT registers.
> > >There is a way to pass *CNT values already from ACPI to the driver -- It
> > >looks for method called FMCN (or SSCN) and retrieves the values from
> > >there if found.
> > 
> > Right, I also noticed this afterward. By the way, are FMCN and SSCN
> > documented anywhere in the ACPI spec?  I am trying to figure out how to
> > update the ACPI table to add this information for the AMD Seattle (ARM64)
> > platform, and I will also submit a patch to add the new HID for this driver.
> 
> No, they are Intel inventions for the Windows I2C driver.
> 
> Here is what I know about it:
>   SSCN - Standard Mode CNTs
>   FMCN - Fast Mode CNTs
> 
> They both return a package:
> 
>   Package() {
>     HCNT,
>     LCNT,
>     SDA_hold_time,
>   }

1) Regarding
https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
, Window I2C driver should pass MITT test. There are 5 I2C devices
connect to one I2C bus for test. And those devices defined different
"ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".

During test, I2C bus should run in different "ConnectionSpeed" of
device.

That means windows driver can modify I2C bus speed to match the
"ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
can not work for WITT test cases.

It sounds like odd that multi-speed can be applied over one I2C bus, But
everything goes well.
Does this behavior(multi-speed) violate the I2C spec?

2) In theory, I2C core driver also can get ACPI resource
"ConnectionSpeed" of device during creating new I2C device. 
Is it worth to add a new feature for parsing and using "ConnectionSpeed"
of device for determining bus speed?

3) For ACPI DW adapter, current driver only support Fast mode. I would
like to support other modes after previous 2 questions are clear.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> 1) Regarding
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
> , Window I2C driver should pass MITT test. There are 5 I2C devices
> connect to one I2C bus for test. And those devices defined different
> "ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".
> 
> During test, I2C bus should run in different "ConnectionSpeed" of
> device.
> 
> That means windows driver can modify I2C bus speed to match the
> "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> can not work for WITT test cases.

That is why there are *CNT methods for all supported I2C modes:

  - SSCN() - returns for standard mode (100kHz)
  - FMCN() - returns for fast mode (400kHz)
  - FPCN() - returns for fast mode+ (1MHz)
 
for High-speed mode I'm not sure what the method name is ;-)

Then the Windows driver switches between those based on what the
ConnectionSpeed is in the ACPI I2C connector.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Ken Xue]

On Wed, 2015-12-23 at 11:52 +0200, Mika Westerberg wrote:
> On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> > 1) Regarding
> > https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
> > , Window I2C driver should pass MITT test. There are 5 I2C devices
> > connect to one I2C bus for test. And those devices defined different
> > "ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".
> > 
> > During test, I2C bus should run in different "ConnectionSpeed" of
> > device.
> > 
> > That means windows driver can modify I2C bus speed to match the
> > "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> > can not work for WITT test cases.
> 
> That is why there are *CNT methods for all supported I2C modes:
> 
>   - SSCN() - returns for standard mode (100kHz)
>   - FMCN() - returns for fast mode (400kHz)
>   - FPCN() - returns for fast mode+ (1MHz)
>  
> for High-speed mode I'm not sure what the method name is ;-)
> 
> Then the Windows driver switches between those based on what the
> ConnectionSpeed is in the ACPI I2C connector.

Window driver can set Bus speed based on "ConnectionSpeed". But Current
Linux driver only sets Bus speed during probe. How can Linux diver
determine which Bus speed should be applied, if all *CNT methods return
non-zero? 

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 05:59:49PM +0800, Ken Xue wrote:
> On Wed, 2015-12-23 at 11:52 +0200, Mika Westerberg wrote:
> > On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> > > 1) Regarding
> > > https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=vs.85).aspx
> > > , Window I2C driver should pass MITT test. There are 5 I2C devices
> > > connect to one I2C bus for test. And those devices defined different
> > > "ConnectionSpeed" over the I2C bus by ACPI resource "I2CSerialBus".
> > > 
> > > During test, I2C bus should run in different "ConnectionSpeed" of
> > > device.
> > > 
> > > That means windows driver can modify I2C bus speed to match the
> > > "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> > > can not work for WITT test cases.
> > 
> > That is why there are *CNT methods for all supported I2C modes:
> > 
> >   - SSCN() - returns for standard mode (100kHz)
> >   - FMCN() - returns for fast mode (400kHz)
> >   - FPCN() - returns for fast mode+ (1MHz)
> >  
> > for High-speed mode I'm not sure what the method name is ;-)
> > 
> > Then the Windows driver switches between those based on what the
> > ConnectionSpeed is in the ACPI I2C connector.
> 
> Window driver can set Bus speed based on "ConnectionSpeed". But Current
> Linux driver only sets Bus speed during probe. How can Linux diver
> determine which Bus speed should be applied, if all *CNT methods return
> non-zero? 

By default Linux driver uses 400kHz so it picks values returned from
FMCN(). You can hack clk_freq in the driver to use 100kHz which then
picks SSCN().

There is no support for reading ConnectionSpeed in Linux yet.

RE: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Zheng, Ivan]

Why/how can Linux driver make use of such non-ACPI defined methods?

Thanks,
Ivan

-----Original Message-----
From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
Sent: Wednesday, December 23, 2015 17:53
To: Xue, Ken
Cc: wsa@the-dreams.de; Suthikulpanit, Suravee; Loc Ho; rjw@rjwysocki.net; lenb@kernel.org; linux-i2c@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; jcm@redhat.com; patches@apm.com; Hurwitz, Sherry; Duran, Leo; Hanjun Guo; Al Stone; Zheng, Ivan; Yu, Xiangliang
Subject: Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

On Wed, Dec 23, 2015 at 05:34:01PM +0800, Ken Xue wrote:
> 1) Regarding
> https://msdn.microsoft.com/en-us/library/windows/hardware/dn919852(v=v
> s.85).aspx , Window I2C driver should pass MITT test. There are 5 I2C 
> devices connect to one I2C bus for test. And those devices defined 
> different "ConnectionSpeed" over the I2C bus by ACPI resource 
> "I2CSerialBus".
> 
> During test, I2C bus should run in different "ConnectionSpeed" of 
> device.
> 
> That means windows driver can modify I2C bus speed to match the 
> "ConnectionSpeed" of device on-the-fly. Static value from SSCN and 
> FMCN can not work for WITT test cases.

That is why there are *CNT methods for all supported I2C modes:

  - SSCN() - returns for standard mode (100kHz)
  - FMCN() - returns for fast mode (400kHz)
  - FPCN() - returns for fast mode+ (1MHz)
 
for High-speed mode I'm not sure what the method name is ;-)

Then the Windows driver switches between those based on what the ConnectionSpeed is in the ACPI I2C connector.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 10:02:04AM +0000, Zheng, Ivan wrote:
> Why/how can Linux driver make use of such non-ACPI defined methods?

What do you mean exactly?

I added support for these (well, SSCN and FMCN) because I found out that
some vendors include such methods with their BIOS implementation.
Windows driver uses these if present and so do we.

RE: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Zheng, Ivan]

Thanks Mika for the background.

I'm a BIOS engineer and my point is that such non-ACPI defined methods rely on BIOS implementation, so how the generic I2C driver works on a platform without such methods? Should the vendor implement their own driver in such case?

I think Windows driver use ConnectionSpeed field in I2CSerialBus declaration for a certain device and it's defined in ACPI 5.0.

Thanks,
Ivan

-----Original Message-----
From: Mika Westerberg [mailto:mika.westerberg@linux.intel.com] 
Sent: Wednesday, December 23, 2015 18:10
To: Zheng, Ivan
Cc: Xue, Ken; wsa@the-dreams.de; Suthikulpanit, Suravee; Loc Ho; rjw@rjwysocki.net; lenb@kernel.org; linux-i2c@vger.kernel.org; linux-acpi@vger.kernel.org; linux-arm-kernel@lists.infradead.org; jcm@redhat.com; patches@apm.com; Hurwitz, Sherry; Duran, Leo; Hanjun Guo; Al Stone; Yu, Xiangliang
Subject: Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

On Wed, Dec 23, 2015 at 10:02:04AM +0000, Zheng, Ivan wrote:
> Why/how can Linux driver make use of such non-ACPI defined methods?

What do you mean exactly?

I added support for these (well, SSCN and FMCN) because I found out that some vendors include such methods with their BIOS implementation.
Windows driver uses these if present and so do we.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Mika Westerberg]

On Wed, Dec 23, 2015 at 11:24:44AM +0000, Zheng, Ivan wrote:
> I'm a BIOS engineer and my point is that such non-ACPI defined methods
> rely on BIOS implementation, so how the generic I2C driver works on a
> platform without such methods? Should the vendor implement their own
> driver in such case?

Those methods are by *no* means meant for generic driver. They are
specific to DesignWare I2C driver.

If you need to specify bus speed that the device is supposed to use,
you put it to the I2cSerialBus() resource (ConnectionSpeed)...

> I think Windows driver use ConnectionSpeed field in I2CSerialBus
> declaration for a certain device and it's defined in ACPI 5.0.

..like you say here.

We just need to add support for ConnectionSpeed to Linux I2C core.

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Ben Dooks]

On 23/12/15 09:34, Ken Xue wrote:
> On Tue, 2015-12-15 at 16:59 +0200, Mika Westerberg wrote:
>> On Tue, Dec 15, 2015 at 08:52:19AM -0600, Suravee Suthikulpanit wrote:

> During test, I2C bus should run in different "ConnectionSpeed" of
> device.
> 
> That means windows driver can modify I2C bus speed to match the
> "ConnectionSpeed" of device on-the-fly. Static value from SSCN and FMCN
> can not work for WITT test cases.
> 
> It sounds like odd that multi-speed can be applied over one I2C bus, But
> everything goes well.
> Does this behavior(multi-speed) violate the I2C spec?

I can't remember if the spec says it, but I would say the bus should
run at the speed of the slowest attached peripheral as they all have
to listen for the address-byte being sent at the start of each i2c
transaction.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Rafael J. Wysocki]

On Thursday, December 10, 2015 02:19:15 PM Loc Ho wrote:
> Add APM X-Gene ACPI I2C device support. These patches follow
> the same implementation as AMD I2C driver - changes in ACPI APD
> and Designware I2C drivers.
> 
> v2:
> * Spilt the acpi_apd change with defines for AMD and X-Gene I2C's
> 
> Signed-off-by: Loc Ho <lho@apm.com>
> ---
> Loc Ho (2):
>   acpi:apd: Add APM X-Gene ACPI I2C device support
>   i2c:dw: Add APM X-Gene ACPI I2C device support

Both patches applied, thanks!

Re: [PATCH v2 0/2] i2c:dw: Add APM X-Gene ACPI I2C device support

[Loc Ho]

Hi,

On Sat, Jan 2, 2016 at 4:54 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Thursday, December 10, 2015 02:19:15 PM Loc Ho wrote:
>> Add APM X-Gene ACPI I2C device support. These patches follow
>> the same implementation as AMD I2C driver - changes in ACPI APD
>> and Designware I2C drivers.
>>
>> v2:
>> * Spilt the acpi_apd change with defines for AMD and X-Gene I2C's
>>
>> Signed-off-by: Loc Ho <lho@apm.com>
>> ---
>> Loc Ho (2):
>>   acpi:apd: Add APM X-Gene ACPI I2C device support
>>   i2c:dw: Add APM X-Gene ACPI I2C device support
>
> Both patches applied, thanks!
>

Thanks...

-Loc