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

[Andy Shevchenko]

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@xxxxxxxxxxxxxxx>
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@xxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxx>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/