| Message ID | 1450219138-5868-1-git-send-email-Suravee.Suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
+Jarkko and Andy On Tue, Dec 15, 2015 at 04:38:58PM -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, this patch removes the clock requirement when SSCN and FFCN > are provided. Actually I think the only thing you need to change is i2c_dw_init() so that it does not call dev->get_clk_rate_khz(dev) if *CNT values are already provided. The clk framework should work fine if the returned clock is NULL (which I think is your case). The driver gates clocks when the device is suspended and on Intel LPSS there actually is a clock that gets gated. > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > > Note: This has been tested on AMD Seattle RevB for both DT and ACPI. > > drivers/i2c/busses/i2c-designware-core.c | 5 +++-- > drivers/i2c/busses/i2c-designware-platdrv.c | 30 +++++++++++++++++++++++------ > 2 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c > index 8c48b27..ec458ec 100644 > --- a/drivers/i2c/busses/i2c-designware-core.c > +++ b/drivers/i2c/busses/i2c-designware-core.c > @@ -281,7 +281,7 @@ 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 input_clock_khz = 0; > u32 hcnt, lcnt; > u32 reg; > u32 sda_falling_time, scl_falling_time; > @@ -295,7 +295,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev) > } > } > > - input_clock_khz = dev->get_clk_rate_khz(dev); > + if (dev->get_clk_rate_khz) > + 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)) { > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c > index 809579e..57f623b 100644 > --- a/drivers/i2c/busses/i2c-designware-platdrv.c > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c > @@ -127,6 +127,26 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev) > } > #endif > > +static int dw_i2c_configure_clk(struct platform_device *pdev, > + struct dw_i2c_dev *dev) > +{ > + /* For ACPI, if SSCN and FMCN is provided, we should not > + * need the clock value. > + */ > + if (has_acpi_companion(&pdev->dev) && > + dev->ss_hcnt && dev->ss_lcnt && > + dev->fs_hcnt && dev->fs_lcnt) > + return 0; > + > + 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); > + > + return 0; > +} > + > static int dw_i2c_plat_probe(struct platform_device *pdev) > { > struct dw_i2c_dev *dev; > @@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | > 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); > + r = dw_i2c_configure_clk(pdev, dev); > + if (r) > + return r; > > - if (!dev->sda_hold_time && ht) { > + if (!dev->sda_hold_time && ht && dev->get_clk_rate_khz) { > u32 ic_clk = dev->get_clk_rate_khz(dev); > > dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000, > -- > 2.5.0 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mika, On 12/16/2015 03:42 AM, Mika Westerberg wrote: > +Jarkko and Andy > > On Tue, Dec 15, 2015 at 04:38:58PM -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, this patch removes the clock requirement when SSCN and FFCN >> are provided. > > Actually I think the only thing you need to change is i2c_dw_init() so > that it does not call dev->get_clk_rate_khz(dev) if *CNT values are > already provided. Please see below. > > The clk framework should work fine if the returned clock is NULL (which > I think is your case). > > The driver gates clocks when the device is suspended and on Intel LPSS > there actually is a clock that gets gated. > >> [..] >> @@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) >> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | >> 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); Actually, if we don't provide the clock (which is the case for ACPI), this would also return and cause the probing to fail. Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 16, 2015 at 08:11:12AM -0600, Suravee Suthikulpanit wrote: > >The clk framework should work fine if the returned clock is NULL (which > >I think is your case). > > > >The driver gates clocks when the device is suspended and on Intel LPSS > >there actually is a clock that gets gated. > > > >>[..] > >>@@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) > >> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | > >> 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); > > Actually, if we don't provide the clock (which is the case for ACPI), this > would also return and cause the probing to fail. Indeed it seems that when you have CONFIG_COMMON_CLK selected the clock framework starts returning errors if the clock is not found. Since we need the clock for Intel LPSS I2C host controllers (and they may have *CNT methods), I think you just need to provide the clock for AMD I2C host controller in similar way than we do in drivers/acpi/acpi_lpss.c. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/16/2015 08:28 AM, Mika Westerberg wrote: > On Wed, Dec 16, 2015 at 08:11:12AM -0600, Suravee Suthikulpanit wrote: >>> The clk framework should work fine if the returned clock is NULL (which >>> I think is your case). >>> >>> The driver gates clocks when the device is suspended and on Intel LPSS >>> there actually is a clock that gets gated. >>> >>>> [..] >>>> @@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) >>>> dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | >>>> 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); >> >> Actually, if we don't provide the clock (which is the case for ACPI), this >> would also return and cause the probing to fail. > > Indeed it seems that when you have CONFIG_COMMON_CLK selected the clock > framework starts returning errors if the clock is not found. > > Since we need the clock for Intel LPSS I2C host controllers (and they > may have *CNT methods), I think you just need to provide the clock for > AMD I2C host controller in similar way than we do in > drivers/acpi/acpi_lpss.c. I am trying to avoid having to hard-coded clock frequency value in the driver. Would it be alright to not return w/ error, and just do the following? dev->clk = devm_clk_get(&pdev->dev, NULL); if (!IS_ERR(dev->clk)) dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; This should work for the Intel case when clock is also provided. Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Dec 16, 2015 at 08:44:34AM -0600, Suravee Suthikulpanit wrote: > I am trying to avoid having to hard-coded clock frequency value in the > driver. Would it be alright to not return w/ error, and just do the > following? > > dev->clk = devm_clk_get(&pdev->dev, NULL); > if (!IS_ERR(dev->clk)) > dev->get_clk_rate_khz = i2c_dw_get_clk_rate_khz; > > This should work for the Intel case when clock is also provided. Does it also work when clk_prepare_enable()/disable() is called for the clock (in PM callbacks for example)? If yes, then I don't see problems with what you are suggesting. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, this patch removes the clock requirement when SSCN and FFCN > are provided. > > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> Tested on X-Gene ACPI boot with updated FW and work just fine. -Loc -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c index 8c48b27..ec458ec 100644 --- a/drivers/i2c/busses/i2c-designware-core.c +++ b/drivers/i2c/busses/i2c-designware-core.c @@ -281,7 +281,7 @@ 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 input_clock_khz = 0; u32 hcnt, lcnt; u32 reg; u32 sda_falling_time, scl_falling_time; @@ -295,7 +295,8 @@ int i2c_dw_init(struct dw_i2c_dev *dev) } } - input_clock_khz = dev->get_clk_rate_khz(dev); + if (dev->get_clk_rate_khz) + 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)) { diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c index 809579e..57f623b 100644 --- a/drivers/i2c/busses/i2c-designware-platdrv.c +++ b/drivers/i2c/busses/i2c-designware-platdrv.c @@ -127,6 +127,26 @@ static inline int dw_i2c_acpi_configure(struct platform_device *pdev) } #endif +static int dw_i2c_configure_clk(struct platform_device *pdev, + struct dw_i2c_dev *dev) +{ + /* For ACPI, if SSCN and FMCN is provided, we should not + * need the clock value. + */ + if (has_acpi_companion(&pdev->dev) && + dev->ss_hcnt && dev->ss_lcnt && + dev->fs_hcnt && dev->fs_lcnt) + return 0; + + 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); + + return 0; +} + static int dw_i2c_plat_probe(struct platform_device *pdev) { struct dw_i2c_dev *dev; @@ -203,13 +223,11 @@ static int dw_i2c_plat_probe(struct platform_device *pdev) dev->master_cfg = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE | 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); + r = dw_i2c_configure_clk(pdev, dev); + if (r) + return r; - if (!dev->sda_hold_time && ht) { + if (!dev->sda_hold_time && ht && dev->get_clk_rate_khz) { u32 ic_clk = dev->get_clk_rate_khz(dev); dev->sda_hold_time = div_u64((u64)ic_clk * ht + 500000,
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, this patch removes the clock requirement when SSCN and FFCN are provided. Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> --- Note: This has been tested on AMD Seattle RevB for both DT and ACPI. drivers/i2c/busses/i2c-designware-core.c | 5 +++-- drivers/i2c/busses/i2c-designware-platdrv.c | 30 +++++++++++++++++++++++------ 2 files changed, 27 insertions(+), 8 deletions(-)