| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <1450895221.30729.322.camel@linux.intel.com>
Date: Wed, 23 Dec 2015 20:27:01 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
mika.westerberg@...ux.intel.com, wsa@...-dreams.de
Cc: jarkko.nikula@...ux.intel.com, lho@....com, Ken.Xue@....com,
linux-i2c@...r.kernel.org, linux-acpi@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] i2c: designware: Do not require clock when SSCN and
FFCN are provided
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 <mika.westerberg@...ux.intel.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>
> ---
> 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 <andriy.shevchenko@...ux.intel.com>
Intel Finland Oy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists