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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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@linux.intel.com>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.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@linux.intel.com>
Intel Finland Oy