Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933942AbbLWS24 (ORCPT ); Wed, 23 Dec 2015 13:28:56 -0500 Received: from mga14.intel.com ([192.55.52.115]:6493 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754536AbbLWS2w (ORCPT ); Wed, 23 Dec 2015 13:28:52 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,470,1444719600"; d="scan'208";a="877610530" Message-ID: <1450895221.30729.322.camel@linux.intel.com> Subject: Re: [PATCH v3] i2c: designware: Do not require clock when SSCN and FFCN are provided From: Andy Shevchenko To: Suravee Suthikulpanit , mika.westerberg@linux.intel.com, wsa@the-dreams.de Cc: jarkko.nikula@linux.intel.com, lho@apm.com, Ken.Xue@amd.com, linux-i2c@vger.kernel.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org Date: Wed, 23 Dec 2015 20:27:01 +0200 In-Reply-To: <1450820141-1720-1-git-send-email-Suravee.Suthikulpanit@amd.com> References: <1450820141-1720-1-git-send-email-Suravee.Suthikulpanit@amd.com> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.2-1 Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6476 Lines: 209 On Tue, 2015-12-22 at 15:35 -0600, Suravee Suthikulpanit wrote: > 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: Mika Westerberg > Signed-off-by: Suravee Suthikulpanit > --- > Note: This has been tested on AMD Seattle RevB for both DT and ACPI. > > Changes from V2 (https://lkml.org/lkml/2015/12/22/584): >     * Add the i2c_dw_clk_rate from Mika. >     * Add check if get_clk_rate_khz is set before setting > sda_hold_time > > Changes from V1 (https://lkml.org/lkml/2015/12/15/792): >     In v1, I disregarded the clock if SSCN and FMCN are provided, >     assuming that it was not needed. That was incorrect assumption, >     and is now fixed in v2. > >  drivers/i2c/busses/i2c-designware-core.c    | 22 +++++++++++++++-- > ----- >  drivers/i2c/busses/i2c-designware-platdrv.c | 24 ++++++++++++------- > ----- >  2 files changed, 27 insertions(+), 19 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-core.c > b/drivers/i2c/busses/i2c-designware-core.c > index 8c48b27..25dccd8 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 */ > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c > b/drivers/i2c/busses/i2c-designware-platdrv.c > index 8ffc36b..04edd09 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c Can we introduce static int i2c_dw_plat_prepare_clk(struct dw_i2c_dev *i_dev, bool prepare) { if (IS_ERR(i_dev->clk)) return PTR_ERR(i_dev->clk); if (prepare) /* REMOVEME: Yes, you have to check return value and this is one benefit of this change */ return clk_prepare_enable(i_dev->clk); clk_disable_unprepare(i_dev->clk); return 0; } and… > @@ -205,16 +205,14 @@ static int dw_i2c_plat_probe(struct > platform_device *pdev) >   DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST; >   >   dev->clk = devm_clk_get(&pdev->dev, NULL); > - dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; > - if (IS_ERR(dev->clk)) > - return PTR_ERR(dev->clk); > - clk_prepare_enable(dev->clk); > - > - if (!dev->sda_hold_time && ht) { > - u32 ic_clk = dev->get_clk_rate_khz(dev); > - > - dev->sda_hold_time = div_u64((u64)ic_clk * ht + > 500000, > -      1000000); > + if (!IS_ERR(dev->clk)) { > + dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; > + clk_prepare_enable(dev->clk); if (!i2c_dw_plat_prepare_clk(dev, true)) { dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; … > + > + if (!dev->sda_hold_time && ht) > + dev->sda_hold_time = div_u64( > + (u64)dev->get_clk_rate_khz(dev) * ht > + 500000, > + 1000000); >   } >   >   if (!dev->tx_fifo_depth) { > @@ -297,7 +295,8 @@ static int dw_i2c_plat_suspend(struct device > *dev) >   struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); >   >   i2c_dw_disable(i_dev); > - clk_disable_unprepare(i_dev->clk); > + if (!IS_ERR(i_dev->clk)) > + clk_disable_unprepare(i_dev->clk); i2c_dw_plat_prepare_clk(i_dev, false); >   >   return 0; >  } > @@ -307,7 +306,8 @@ static int dw_i2c_plat_resume(struct device *dev) >   struct platform_device *pdev = to_platform_device(dev); >   struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev); >   > - clk_prepare_enable(i_dev->clk); > + if (!IS_ERR(i_dev->clk)) > + clk_prepare_enable(i_dev->clk); i2c_dw_plat_prepare_clk(i_dev, true); >   >   if (!i_dev->pm_runtime_disabled) >   i2c_dw_init(i_dev); -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/