Re: [PATCH v2] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Suravee Suthikulanit]

Hi Mika,

On 12/18/2015 4:13 AM, Mika Westerberg wrote:

[]
So instead of this, what if we do not assign dev->get_clk_rate_khz at
all and then do something like below in the core driver?


I like the changes below since it is clear to see within the core file 
how things are handled when get_clk_rate_khz is not assigned (i.e. 
input_clock_hz = 0), and not necessary relying on the platform driver to 
return 0 in this case.


So, at this point, I can re-submit the V3 and combine these changes, and 
we both can sign-off. How does that sound?


Thanks,
Suravee


Of course we still need the other changes you did in this patch to cope
with the missing clock.

diff --git a/drivers/i2c/busses/i2c-designware-core.c 
b/drivers/i2c/busses/i2c-designware-core.c
index 8c48b27ba059..25dccd8df772 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -271,6 +271,17 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
 enable ? "en" : "dis");
  }

+static unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev)
+{
+   /*
+* Clock is not necessary if we got LCNT/HCNT values directly from
+* the platform code.
+*/
+   if (WARN_ON_ONCE(!dev->get_clk_rate_khz))
+   return 0;
+   return dev->get_clk_rate_khz(dev);
+}
+
  /**
   * i2c_dw_init() - initialize the designware i2c master hardware
   * @dev: device private data
@@ -281,7 +292,6 @@ static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool 
enable)
   */
  int i2c_dw_init(struct dw_i2c_dev *dev)
  {
-   u32 input_clock_khz;
u32 hcnt, lcnt;
u32 reg;
u32 sda_falling_time, scl_falling_time;
@@ -295,8 +305,6 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
}
}

-   input_clock_khz = dev->get_clk_rate_khz(dev);
-
reg = dw_readl(dev, DW_IC_COMP_TYPE);
if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
/* Configure register endianess access */
@@ -325,12 +333,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->ss_hcnt;
lcnt = dev->ss_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
4000,   /* tHD;STA = tHIGH = 4.0 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
4700,   /* tLOW = 4.7 us */
scl_falling_time,
0); /* No offset */
@@ -344,12 +352,12 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
hcnt = dev->fs_hcnt;
lcnt = dev->fs_lcnt;
} else {
-   hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+   hcnt = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
600,/* tHD;STA = tHIGH = 0.6 us */
sda_falling_time,
0,  /* 0: DW default, 1: Ideal */
0); /* No offset */
-   lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+   lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
1300,   /* tLOW = 1.3 us */
scl_falling_time,
0); /* No offset */



--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] i2c: designware: Add support for AMD Seattle I2C

[Suravee Suthikulanit]

Mika,

On 12/16/2015 8:54 AM, Mika Westerberg wrote:

On Wed, Dec 16, 2015 at 08:29:38AM -0600, Suravee Suthikulpanit wrote:

>
>
>On 12/16/2015 03:16 AM, Mika Westerberg wrote:

> >On Tue, Dec 15, 2015 at 08:14:34PM -0600, Suravee Suthikulpanit wrote:

> >>Hi Mika,
> >>
> >>On 12/15/15 15:55, Suravee Suthikulpanit wrote:

> >>>Add device HID AMDI0510 to match the I2C controlers on AMD Seattle platform
> >>>
> >>>Signed-off-by: Suravee Suthikulpanit
> >>>---
> >>>  drivers/i2c/busses/i2c-designware-platdrv.c | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>>diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c 
b/drivers/i2c/busses/i2c-designware-platdrv.c
> >>>index 57f623b..a027154 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 },
> >>>+  { "AMDI0510", 0 },
> >>>   { }

> >>
> >>Since this driver seems to be used by several SOCs, and we have been adding
> >>the HID from various SOC vendors. Do you think it would be better to assign
> >>a CID so that each SOC vendor can specify in their ACPI DSDT and we can
> >>match them here?

> >
> >Sure _CID would work here.

>
>Do you know if Synopsys has already provided a CID that we can use for this?

No.


>If not, who do you think should provide this?

Why can't you make _CID for AMD part only? For Intel we are going to get
new IDs for every major SoC release no matter what.

Actually, after discussed with the team. We have decided to go with the 
AMDI0510 at this point, and we will reuse this as CID in future SOC if 
it contains compatible I2C controller.


Thanks,
Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] i2c: designware: Do not require clock when SSCN and FFCN are provided

[Suravee Suthikulanit]

On 12/16/2015 8:56 PM, Loc Ho wrote:

Hi,


The current driver uses input clock source frequency to calculate
values for [SS|FS]_[HC|LC] registers. However, when booting ACPI, we do not
currently have a good way to provide the frequency information.
Instead, we can leverage the SSCN and FFCN ACPI methods, which can be used
to directly provide these values. So, the clock information should
no longer be required during probing.

However, since clk can be invalid, additional checks must be done where
we are making use of it.

Signed-off-by: Suravee Suthikulpanit 
---

Note: This has been tested on AMD Seattle RevB for both DT and ACPI.


Tested on X-Gene hardware also.

-Loc



Thanks for quick response.
Suravee
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@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

[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, 0xE100, 0x1000)
Interrupt (ResourceConsumer, Level, ActiveHigh, Exclusive,,,) { 
389 } // GSIV

  })
  Name (_DSD, Package () {
ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
Package () {
  Package (2) {"clock-source-frequency", 1 }
}
  }) // _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 
---
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html



--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html