[PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Heikki Krogerus]

Changes since v1:
- now using do_div() in clk_fd_recalc_rate() as suggested by Andy
- NULL checks for clk_name allocation in acpi_lpss.c

This combines two patch sets for LPSS that I had already send for
review separately. They conflicted with each other.

The first two patches will fix a problem were the context of the
private LPSS registers is lost when entering D3. The last two will add
support for the M/N dividers on LPSS by adding a new basic clock type
for fractional dividers. The UART driver needs support for it in order
to get clock rates that suit the requested baud rates.


Heikki Krogerus (4):
  ACPI / PM: Export rest of the subsys functions
  ACPI / LPSS: custom power domain for LPSS
  clk: new basic clk type for fractional divider
  ACPI / LPSS: support for fractional divider clock

 drivers/acpi/acpi_lpss.c             | 204 ++++++++++++++++++++++++++++++-----
 drivers/acpi/device_pm.c             |   2 +
 drivers/clk/Makefile                 |   1 +
 drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++
 include/linux/acpi.h                 |   4 +
 include/linux/clk-provider.h         |  31 ++++++
 6 files changed, 349 insertions(+), 28 deletions(-)
 create mode 100644 drivers/clk/clk-fractional-divider.c

-- 
2.0.0.rc2

[PATCHv2 1/4] ACPI / PM: Export rest of the subsys functions

[Heikki Krogerus]

No reason for excluding those two.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/device_pm.c | 2 ++
 include/linux/acpi.h     | 4 ++++
 2 files changed, 6 insertions(+)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index d047739..7dae90b 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -923,6 +923,7 @@ int acpi_subsys_suspend(struct device *dev)
 	pm_runtime_resume(dev);
 	return pm_generic_suspend(dev);
 }
+EXPORT_SYMBOL_GPL(acpi_subsys_suspend);
 
 /**
  * acpi_subsys_suspend_late - Suspend device using ACPI.
@@ -968,6 +969,7 @@ int acpi_subsys_freeze(struct device *dev)
 	pm_runtime_resume(dev);
 	return pm_generic_freeze(dev);
 }
+EXPORT_SYMBOL_GPL(acpi_subsys_freeze);
 
 #endif /* CONFIG_PM_SLEEP */
 
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 7a8f2cd..47dcc0e 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -556,12 +556,16 @@ int acpi_dev_resume_early(struct device *dev);
 int acpi_subsys_prepare(struct device *dev);
 int acpi_subsys_suspend_late(struct device *dev);
 int acpi_subsys_resume_early(struct device *dev);
+int acpi_subsys_suspend(struct device *dev);
+int acpi_subsys_freeze(struct device *dev);
 #else
 static inline int acpi_dev_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_dev_resume_early(struct device *dev) { return 0; }
 static inline int acpi_subsys_prepare(struct device *dev) { return 0; }
 static inline int acpi_subsys_suspend_late(struct device *dev) { return 0; }
 static inline int acpi_subsys_resume_early(struct device *dev) { return 0; }
+static inline int acpi_subsys_suspend(struct device *dev) { return 0; }
+static inline int acpi_subsys_freeze(struct device *dev) { return 0; }
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM)
-- 
2.0.0.rc2

[PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

A power domain where we save the context of the additional
LPSS registers. We need to do this or all LPSS devices are
left in reset state when resuming from D3 on some Baytrails.
The devices with the fractional clock divider also have
zeros for N and M values after resuming unless they are
reset.

Li Aubrey found the root cause for the issue. The idea of
using power domain for LPSS came from Mika Westerberg.

Reported-by: Jin Yao <yao.jin@linux.intel.com>
Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c | 133 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 126 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 69e29f4..24e49a5 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/platform_data/clk-lpss.h>
 #include <linux/pm_runtime.h>
+#include <linux/delay.h>
 
 #include "internal.h"
 
@@ -43,6 +44,8 @@ ACPI_MODULE_NAME("acpi_lpss");
 #define LPSS_TX_INT			0x20
 #define LPSS_TX_INT_MASK		BIT(1)
 
+#define LPSS_PRV_REG_COUNT		9
+
 struct lpss_shared_clock {
 	const char *name;
 	unsigned long rate;
@@ -58,6 +61,7 @@ struct lpss_device_desc {
 	unsigned int prv_offset;
 	size_t prv_size_override;
 	bool clk_gate;
+	bool save_ctx;
 	struct lpss_shared_clock *shared_clock;
 	void (*setup)(struct lpss_private_data *pdata);
 };
@@ -72,6 +76,7 @@ struct lpss_private_data {
 	resource_size_t mmio_size;
 	struct clk *clk;
 	const struct lpss_device_desc *dev_desc;
+	u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
 };
 
 static void lpss_uart_setup(struct lpss_private_data *pdata)
@@ -116,6 +121,7 @@ static struct lpss_shared_clock pwm_clock = {
 
 static struct lpss_device_desc byt_pwm_dev_desc = {
 	.clk_required = true,
+	.save_ctx = true,
 	.shared_clock = &pwm_clock,
 };
 
@@ -128,6 +134,7 @@ static struct lpss_device_desc byt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.clk_gate = true,
+	.save_ctx = true,
 	.shared_clock = &uart_clock,
 	.setup = lpss_uart_setup,
 };
@@ -141,6 +148,7 @@ static struct lpss_device_desc byt_spi_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x400,
 	.clk_gate = true,
+	.save_ctx = true,
 	.shared_clock = &spi_clock,
 };
 
@@ -156,6 +164,7 @@ static struct lpss_shared_clock i2c_clock = {
 static struct lpss_device_desc byt_i2c_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
+	.save_ctx = true,
 	.shared_clock = &i2c_clock,
 };
 
@@ -449,6 +458,102 @@ static void acpi_lpss_set_ltr(struct device *dev, s32 val)
 	}
 }
 
+#ifdef CONFIG_PM
+static void acpi_lpss_save_ctx(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	int i;
+
+	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
+		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
+		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
+			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
+	}
+}
+
+static void acpi_lpss_restore_ctx(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	int i;
+
+	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
+	msleep(10);
+
+	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
+		__lpss_reg_write(pdata->prv_reg_ctx[i], pdata, i * sizeof(u32));
+		dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02x\n",
+			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
+	}
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int acpi_lpss_suspend_late(struct device *dev)
+{
+	int ret = pm_generic_suspend_late(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_save_ctx(dev);
+	return acpi_dev_suspend_late(dev);
+}
+
+static int acpi_lpss_restore_early(struct device *dev)
+{
+	int ret = acpi_dev_resume_early(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_restore_ctx(dev);
+	return pm_generic_resume_early(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM_RUNTIME
+static int acpi_lpss_runtime_suspend(struct device *dev)
+{
+	int ret = pm_generic_runtime_suspend(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_save_ctx(dev);
+	return acpi_dev_runtime_suspend(dev);
+}
+
+static int acpi_lpss_runtime_resume(struct device *dev)
+{
+	int ret = acpi_dev_runtime_resume(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_restore_ctx(dev);
+	return pm_generic_runtime_resume(dev);
+}
+#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
+
+static struct dev_pm_domain acpi_lpss_pm_domain = {
+	.ops = {
+#ifdef CONFIG_PM_SLEEP
+		.suspend_late = acpi_lpss_suspend_late,
+		.restore_early = acpi_lpss_restore_early,
+		.prepare = acpi_subsys_prepare,
+		.suspend = acpi_subsys_suspend,
+		.resume_early = acpi_subsys_resume_early,
+		.freeze = acpi_subsys_freeze,
+		.poweroff = acpi_subsys_suspend,
+		.poweroff_late = acpi_subsys_suspend_late,
+#endif
+#ifdef CONFIG_PM_RUNTIME
+		.runtime_suspend = acpi_lpss_runtime_suspend,
+		.runtime_resume = acpi_lpss_runtime_resume,
+#endif
+	},
+};
+
 static int acpi_lpss_platform_notify(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
@@ -456,7 +561,6 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 	struct lpss_private_data *pdata;
 	struct acpi_device *adev;
 	const struct acpi_device_id *id;
-	int ret = 0;
 
 	id = acpi_match_device(acpi_lpss_device_ids, &pdev->dev);
 	if (!id || !id->driver_data)
@@ -466,7 +570,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 		return 0;
 
 	pdata = acpi_driver_data(adev);
-	if (!pdata || !pdata->mmio_base || !pdata->dev_desc->ltr_required)
+	if (!pdata || !pdata->mmio_base)
 		return 0;
 
 	if (pdata->mmio_size < pdata->dev_desc->prv_offset + LPSS_LTR_SIZE) {
@@ -474,12 +578,27 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 		return 0;
 	}
 
-	if (action == BUS_NOTIFY_ADD_DEVICE)
-		ret = sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group);
-	else if (action == BUS_NOTIFY_DEL_DEVICE)
-		sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		if (pdata->dev_desc->save_ctx)
+			pdev->dev.pm_domain = &acpi_lpss_pm_domain;
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		if (pdata->dev_desc->save_ctx)
+			pdev->dev.pm_domain = NULL;
+		break;
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (pdata->dev_desc->ltr_required)
+			return sysfs_create_group(&pdev->dev.kobj,
+						  &lpss_attr_group);
+	case BUS_NOTIFY_DEL_DEVICE:
+		if (pdata->dev_desc->ltr_required)
+			sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
+	default:
+		break;
+	}
 
-	return ret;
+	return 0;
 }
 
 static struct notifier_block acpi_lpss_nb = {
-- 
2.0.0.rc2

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Thursday, May 15, 2014 04:40:24 PM Heikki Krogerus wrote:
> A power domain where we save the context of the additional
> LPSS registers. We need to do this or all LPSS devices are
> left in reset state when resuming from D3 on some Baytrails.
> The devices with the fractional clock divider also have
> zeros for N and M values after resuming unless they are
> reset.
> 
> Li Aubrey found the root cause for the issue. The idea of
> using power domain for LPSS came from Mika Westerberg.
> 
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
>  drivers/acpi/acpi_lpss.c | 133 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 126 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f4..24e49a5 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/clk-lpss.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/delay.h>
>  
>  #include "internal.h"
>  
> @@ -43,6 +44,8 @@ ACPI_MODULE_NAME("acpi_lpss");
>  #define LPSS_TX_INT			0x20
>  #define LPSS_TX_INT_MASK		BIT(1)
>  
> +#define LPSS_PRV_REG_COUNT		9
> +
>  struct lpss_shared_clock {
>  	const char *name;
>  	unsigned long rate;
> @@ -58,6 +61,7 @@ struct lpss_device_desc {
>  	unsigned int prv_offset;
>  	size_t prv_size_override;
>  	bool clk_gate;
> +	bool save_ctx;
>  	struct lpss_shared_clock *shared_clock;
>  	void (*setup)(struct lpss_private_data *pdata);
>  };
> @@ -72,6 +76,7 @@ struct lpss_private_data {
>  	resource_size_t mmio_size;
>  	struct clk *clk;
>  	const struct lpss_device_desc *dev_desc;
> +	u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
>  };
>  
>  static void lpss_uart_setup(struct lpss_private_data *pdata)
> @@ -116,6 +121,7 @@ static struct lpss_shared_clock pwm_clock = {
>  
>  static struct lpss_device_desc byt_pwm_dev_desc = {
>  	.clk_required = true,
> +	.save_ctx = true,
>  	.shared_clock = &pwm_clock,
>  };
>  
> @@ -128,6 +134,7 @@ static struct lpss_device_desc byt_uart_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x800,
>  	.clk_gate = true,
> +	.save_ctx = true,
>  	.shared_clock = &uart_clock,
>  	.setup = lpss_uart_setup,
>  };
> @@ -141,6 +148,7 @@ static struct lpss_device_desc byt_spi_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x400,
>  	.clk_gate = true,
> +	.save_ctx = true,
>  	.shared_clock = &spi_clock,
>  };
>  
> @@ -156,6 +164,7 @@ static struct lpss_shared_clock i2c_clock = {
>  static struct lpss_device_desc byt_i2c_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x800,
> +	.save_ctx = true,
>  	.shared_clock = &i2c_clock,
>  };
>  
> @@ -449,6 +458,102 @@ static void acpi_lpss_set_ltr(struct device *dev, s32 val)
>  	}
>  }
>  
> +#ifdef CONFIG_PM

It would be good to add a kerneldoc explaining what's being saved here and why.

> +static void acpi_lpss_save_ctx(struct device *dev)
> +{
> +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +	int i;
> +
> +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
> +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> +	}
> +}
> +
> +static void acpi_lpss_restore_ctx(struct device *dev)
> +{
> +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +	int i;
> +
> +	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
> +	msleep(10);

Two questions here.

First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
such delays (this is not a PCI device formally).

And because this is not a PCI device formally, why is the comment talking about
the PCI spec?  Why is PCI relevant in any way here?

> +
> +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> +		__lpss_reg_write(pdata->prv_reg_ctx[i], pdata, i * sizeof(u32));
> +		dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02x\n",
> +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> +	}
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int acpi_lpss_suspend_late(struct device *dev)
> +{
> +	int ret = pm_generic_suspend_late(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_save_ctx(dev);
> +	return acpi_dev_suspend_late(dev);
> +}
> +
> +static int acpi_lpss_restore_early(struct device *dev)
> +{
> +	int ret = acpi_dev_resume_early(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_restore_ctx(dev);
> +	return pm_generic_resume_early(dev);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM_RUNTIME
> +static int acpi_lpss_runtime_suspend(struct device *dev)
> +{
> +	int ret = pm_generic_runtime_suspend(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_save_ctx(dev);
> +	return acpi_dev_runtime_suspend(dev);
> +}
> +
> +static int acpi_lpss_runtime_resume(struct device *dev)
> +{
> +	int ret = acpi_dev_runtime_resume(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_restore_ctx(dev);
> +	return pm_generic_runtime_resume(dev);
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +#endif /* CONFIG_PM */
> +
> +static struct dev_pm_domain acpi_lpss_pm_domain = {
> +	.ops = {
> +#ifdef CONFIG_PM_SLEEP
> +		.suspend_late = acpi_lpss_suspend_late,
> +		.restore_early = acpi_lpss_restore_early,
> +		.prepare = acpi_subsys_prepare,
> +		.suspend = acpi_subsys_suspend,
> +		.resume_early = acpi_subsys_resume_early,
> +		.freeze = acpi_subsys_freeze,
> +		.poweroff = acpi_subsys_suspend,
> +		.poweroff_late = acpi_subsys_suspend_late,
> +#endif
> +#ifdef CONFIG_PM_RUNTIME
> +		.runtime_suspend = acpi_lpss_runtime_suspend,
> +		.runtime_resume = acpi_lpss_runtime_resume,
> +#endif
> +	},
> +};
> +
>  static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  				     unsigned long action, void *data)
>  {
> @@ -456,7 +561,6 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  	struct lpss_private_data *pdata;
>  	struct acpi_device *adev;
>  	const struct acpi_device_id *id;
> -	int ret = 0;
>  
>  	id = acpi_match_device(acpi_lpss_device_ids, &pdev->dev);
>  	if (!id || !id->driver_data)
> @@ -466,7 +570,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  		return 0;
>  
>  	pdata = acpi_driver_data(adev);
> -	if (!pdata || !pdata->mmio_base || !pdata->dev_desc->ltr_required)
> +	if (!pdata || !pdata->mmio_base)
>  		return 0;
>  
>  	if (pdata->mmio_size < pdata->dev_desc->prv_offset + LPSS_LTR_SIZE) {
> @@ -474,12 +578,27 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  		return 0;
>  	}
>  
> -	if (action == BUS_NOTIFY_ADD_DEVICE)
> -		ret = sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group);
> -	else if (action == BUS_NOTIFY_DEL_DEVICE)
> -		sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> +	switch (action) {
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		if (pdata->dev_desc->save_ctx)
> +			pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		if (pdata->dev_desc->save_ctx)
> +			pdev->dev.pm_domain = NULL;
> +		break;
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if (pdata->dev_desc->ltr_required)
> +			return sysfs_create_group(&pdev->dev.kobj,
> +						  &lpss_attr_group);
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if (pdata->dev_desc->ltr_required)
> +			sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> +	default:
> +		break;
> +	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static struct notifier_block acpi_lpss_nb = {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

Hi Rafael,

On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > +#ifdef CONFIG_PM
> 
> It would be good to add a kerneldoc explaining what's being saved here and why.

OK.

> > +static void acpi_lpss_save_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> > +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> > +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
> > +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> > +	}
> > +}
> > +
> > +static void acpi_lpss_restore_ctx(struct device *dev)
> > +{
> > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > +	int i;
> > +
> > +	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
> > +	msleep(10);
> 
> Two questions here.
> 
> First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> such delays (this is not a PCI device formally).

Unfortunately that is not the case. There is nothing in the AML for
this. Mika, correct me if I'm wrong.

> And because this is not a PCI device formally, why is the comment talking about
> the PCI spec?  Why is PCI relevant in any way here?

Under the hood the devices are still PCI devices, even if they
formally aren't. Maybe I should point that out in the comment..

We put the sleep there because without it there was no guarantee if
the device was properly resumed by the time the drivers resume hooks
were called. The symptom in case of a failure was simply that the
registers could not be written, which leads into timeouts at least in
case of the I2C and UART and making them unusable until the next
suspend followed by resume.


Thanks,

-- 
heikki

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> Hi Rafael,
> 
> On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > +#ifdef CONFIG_PM
> > 
> > It would be good to add a kerneldoc explaining what's being saved here and why.
> 
> OK.
> 
> > > +static void acpi_lpss_save_ctx(struct device *dev)
> > > +{
> > > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > > +	int i;
> > > +
> > > +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> > > +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> > > +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02x\n",
> > > +			pdata->prv_reg_ctx[i], (unsigned int)(i * sizeof(u32)));
> > > +	}
> > > +}
> > > +
> > > +static void acpi_lpss_restore_ctx(struct device *dev)
> > > +{
> > > +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> > > +	int i;
> > > +
> > > +	/* PCI spec expects 10ms delay when resuming from D3 to D0 */
> > > +	msleep(10);
> > 
> > Two questions here.
> > 
> > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > such delays (this is not a PCI device formally).
> 
> Unfortunately that is not the case. There is nothing in the AML for
> this. Mika, correct me if I'm wrong.
> 
> > And because this is not a PCI device formally, why is the comment talking about
> > the PCI spec?  Why is PCI relevant in any way here?
> 
> Under the hood the devices are still PCI devices, even if they
> formally aren't. Maybe I should point that out in the comment..
> 
> We put the sleep there because without it there was no guarantee if
> the device was properly resumed by the time the drivers resume hooks
> were called. The symptom in case of a failure was simply that the
> registers could not be written, which leads into timeouts at least in
> case of the I2C and UART and making them unusable until the next
> suspend followed by resume.

OK, so the msleep() is functionally necessary.  Instead of talking about the
PCI in the comment, which will make a casual reader think "What the heck?",
please say something like "the delay is necessary for the subsequent register
writes to succeed on <example system>".

Rafael

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote:
> On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > > such delays (this is not a PCI device formally).
> > 
> > Unfortunately that is not the case. There is nothing in the AML for
> > this. Mika, correct me if I'm wrong.
> > 
> > > And because this is not a PCI device formally, why is the comment talking about
> > > the PCI spec?  Why is PCI relevant in any way here?
> > 
> > Under the hood the devices are still PCI devices, even if they
> > formally aren't. Maybe I should point that out in the comment..
> > 
> > We put the sleep there because without it there was no guarantee if
> > the device was properly resumed by the time the drivers resume hooks
> > were called. The symptom in case of a failure was simply that the
> > registers could not be written, which leads into timeouts at least in
> > case of the I2C and UART and making them unusable until the next
> > suspend followed by resume.
> 
> OK, so the msleep() is functionally necessary.  Instead of talking about the
> PCI in the comment, which will make a casual reader think "What the heck?",
> please say something like "the delay is necessary for the subsequent register
> writes to succeed on <example system>".

OK.

Thanks,

-- 
heikki

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Wednesday, May 21, 2014 01:52:58 PM Heikki Krogerus wrote:
> On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> > > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > > > such delays (this is not a PCI device formally).
> > > 
> > > Unfortunately that is not the case. There is nothing in the AML for
> > > this. Mika, correct me if I'm wrong.
> > > 
> > > > And because this is not a PCI device formally, why is the comment talking about
> > > > the PCI spec?  Why is PCI relevant in any way here?
> > > 
> > > Under the hood the devices are still PCI devices, even if they
> > > formally aren't. Maybe I should point that out in the comment..
> > > 
> > > We put the sleep there because without it there was no guarantee if
> > > the device was properly resumed by the time the drivers resume hooks
> > > were called. The symptom in case of a failure was simply that the
> > > registers could not be written, which leads into timeouts at least in
> > > case of the I2C and UART and making them unusable until the next
> > > suspend followed by resume.
> > 
> > OK, so the msleep() is functionally necessary.  Instead of talking about the
> > PCI in the comment, which will make a casual reader think "What the heck?",
> > please say something like "the delay is necessary for the subsequent register
> > writes to succeed on <example system>".
> 
> OK.

So I have one more concern.  Namely, async suspend is not enabled for the LPSS
devices, so the delays will accumulate for them and that may become a big deal
at one point.

This may be addressed either (1) by enabling async suspend for them or, which would
be more complicated, by doing the msleep() once for the whole LPSS in .resume_early()
and restoring the register values in .resume() without delaying.

For (1) I have the following untested patch (on top of my bleeding-edge branch, but
it should apply to the mainline too if I haven't overlooked anything).  Can you
please try it on boxes with LPSS and see if it doesn't break suspend/resume on them?

Rafael


---
 drivers/acpi/acpi_lpss.c     |   10 +++++++---
 drivers/acpi/acpi_platform.c |   25 ++++++++++++++-----------
 drivers/acpi/internal.h      |    3 +--
 3 files changed, 22 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/acpi/acpi_lpss.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_lpss.c
+++ linux-pm/drivers/acpi/acpi_lpss.c
@@ -276,6 +276,7 @@ static int acpi_lpss_create_device(struc
 	struct lpss_private_data *pdata;
 	struct resource_list_entry *rentry;
 	struct list_head resource_list;
+	struct platform_device *pdev;
 	int ret;
 
 	dev_desc = (struct lpss_device_desc *)id->driver_data;
@@ -331,10 +332,13 @@ static int acpi_lpss_create_device(struc
 		dev_desc->setup(pdata);
 
 	adev->driver_data = pdata;
-	ret = acpi_create_platform_device(adev, id);
-	if (ret > 0)
-		return ret;
+	pdev = acpi_create_platform_device(adev);
+	if (!IS_ERR_OR_NULL(pdev)) {
+		device_enable_async_suspend(&pdev->dev);
+		return 1;
+	}
 
+	ret = PTR_ERR(pdev);
 	adev->driver_data = NULL;
 
  err_out:
Index: linux-pm/drivers/acpi/acpi_platform.c
===================================================================
--- linux-pm.orig/drivers/acpi/acpi_platform.c
+++ linux-pm/drivers/acpi/acpi_platform.c
@@ -43,7 +43,6 @@ static const struct acpi_device_id acpi_
 /**
  * acpi_create_platform_device - Create platform device for ACPI device node
  * @adev: ACPI device node to create a platform device for.
- * @id: ACPI device ID used to match @adev.
  *
  * Check if the given @adev can be represented as a platform device and, if
  * that's the case, create and register a platform device, populate its common
@@ -51,8 +50,7 @@ static const struct acpi_device_id acpi_
  *
  * Name of the platform device will be the same as @adev's.
  */
-int acpi_create_platform_device(struct acpi_device *adev,
-				const struct acpi_device_id *id)
+struct platform_device *acpi_create_platform_device(struct acpi_device *adev)
 {
 	struct platform_device *pdev = NULL;
 	struct acpi_device *acpi_parent;
@@ -64,19 +62,19 @@ int acpi_create_platform_device(struct a
 
 	/* If the ACPI node already has a physical device attached, skip it. */
 	if (adev->physical_node_count)
-		return 0;
+		return NULL;
 
 	INIT_LIST_HEAD(&resource_list);
 	count = acpi_dev_get_resources(adev, &resource_list, NULL, NULL);
 	if (count < 0) {
-		return 0;
+		return NULL;
 	} else if (count > 0) {
 		resources = kmalloc(count * sizeof(struct resource),
 				    GFP_KERNEL);
 		if (!resources) {
 			dev_err(&adev->dev, "No memory for resources\n");
 			acpi_dev_free_resource_list(&resource_list);
-			return -ENOMEM;
+			return ERR_PTR(-ENOMEM);
 		}
 		count = 0;
 		list_for_each_entry(rentry, &resource_list, node)
@@ -113,22 +111,27 @@ int acpi_create_platform_device(struct a
 	pdevinfo.num_res = count;
 	pdevinfo.acpi_node.companion = adev;
 	pdev = platform_device_register_full(&pdevinfo);
-	if (IS_ERR(pdev)) {
+	if (IS_ERR(pdev))
 		dev_err(&adev->dev, "platform device creation failed: %ld\n",
 			PTR_ERR(pdev));
-		pdev = NULL;
-	} else {
+	else
 		dev_dbg(&adev->dev, "created platform device %s\n",
 			dev_name(&pdev->dev));
-	}
 
 	kfree(resources);
+	return pdev;
+}
+
+static int acpi_platform_attach(struct acpi_device *adev,
+				const struct acpi_device_id *id)
+{
+	acpi_create_platform_device(adev);
 	return 1;
 }
 
 static struct acpi_scan_handler platform_handler = {
 	.ids = acpi_platform_device_ids,
-	.attach = acpi_create_platform_device,
+	.attach = acpi_platform_attach,
 };
 
 void __init acpi_platform_init(void)
Index: linux-pm/drivers/acpi/internal.h
===================================================================
--- linux-pm.orig/drivers/acpi/internal.h
+++ linux-pm/drivers/acpi/internal.h
@@ -180,8 +180,7 @@ static inline void suspend_nvs_restore(v
   -------------------------------------------------------------------------- */
 struct platform_device;
 
-int acpi_create_platform_device(struct acpi_device *adev,
-				const struct acpi_device_id *id);
+struct platform_device *acpi_create_platform_device(struct acpi_device *adev);
 
 /*--------------------------------------------------------------------------
 					Video

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

On Thu, May 22, 2014 at 01:28:16AM +0200, Rafael J. Wysocki wrote:
> On Wednesday, May 21, 2014 01:52:58 PM Heikki Krogerus wrote:
> > On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> > > > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > > > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > > > > such delays (this is not a PCI device formally).
> > > > 
> > > > Unfortunately that is not the case. There is nothing in the AML for
> > > > this. Mika, correct me if I'm wrong.
> > > > 
> > > > > And because this is not a PCI device formally, why is the comment talking about
> > > > > the PCI spec?  Why is PCI relevant in any way here?
> > > > 
> > > > Under the hood the devices are still PCI devices, even if they
> > > > formally aren't. Maybe I should point that out in the comment..
> > > > 
> > > > We put the sleep there because without it there was no guarantee if
> > > > the device was properly resumed by the time the drivers resume hooks
> > > > were called. The symptom in case of a failure was simply that the
> > > > registers could not be written, which leads into timeouts at least in
> > > > case of the I2C and UART and making them unusable until the next
> > > > suspend followed by resume.
> > > 
> > > OK, so the msleep() is functionally necessary.  Instead of talking about the
> > > PCI in the comment, which will make a casual reader think "What the heck?",
> > > please say something like "the delay is necessary for the subsequent register
> > > writes to succeed on <example system>".
> > 
> > OK.
> 
> So I have one more concern.  Namely, async suspend is not enabled for the LPSS
> devices, so the delays will accumulate for them and that may become a big deal
> at one point.
> 
> This may be addressed either (1) by enabling async suspend for them or, which would
> be more complicated, by doing the msleep() once for the whole LPSS in .resume_early()
> and restoring the register values in .resume() without delaying.
> 
> For (1) I have the following untested patch (on top of my bleeding-edge branch, but
> it should apply to the mainline too if I haven't overlooked anything).  Can you
> please try it on boxes with LPSS and see if it doesn't break suspend/resume on them?

Done, and there were no problems. I tested it with HSW, BYT and also BDW.

Thanks,

-- 
heikki

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Friday, May 23, 2014 03:30:53 PM Heikki Krogerus wrote:
> On Thu, May 22, 2014 at 01:28:16AM +0200, Rafael J. Wysocki wrote:
> > On Wednesday, May 21, 2014 01:52:58 PM Heikki Krogerus wrote:
> > > On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote:
> > > > On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> > > > > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > > > > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > > > > > such delays (this is not a PCI device formally).
> > > > > 
> > > > > Unfortunately that is not the case. There is nothing in the AML for
> > > > > this. Mika, correct me if I'm wrong.
> > > > > 
> > > > > > And because this is not a PCI device formally, why is the comment talking about
> > > > > > the PCI spec?  Why is PCI relevant in any way here?
> > > > > 
> > > > > Under the hood the devices are still PCI devices, even if they
> > > > > formally aren't. Maybe I should point that out in the comment..
> > > > > 
> > > > > We put the sleep there because without it there was no guarantee if
> > > > > the device was properly resumed by the time the drivers resume hooks
> > > > > were called. The symptom in case of a failure was simply that the
> > > > > registers could not be written, which leads into timeouts at least in
> > > > > case of the I2C and UART and making them unusable until the next
> > > > > suspend followed by resume.
> > > > 
> > > > OK, so the msleep() is functionally necessary.  Instead of talking about the
> > > > PCI in the comment, which will make a casual reader think "What the heck?",
> > > > please say something like "the delay is necessary for the subsequent register
> > > > writes to succeed on <example system>".
> > > 
> > > OK.
> > 
> > So I have one more concern.  Namely, async suspend is not enabled for the LPSS
> > devices, so the delays will accumulate for them and that may become a big deal
> > at one point.
> > 
> > This may be addressed either (1) by enabling async suspend for them or, which would
> > be more complicated, by doing the msleep() once for the whole LPSS in .resume_early()
> > and restoring the register values in .resume() without delaying.
> > 
> > For (1) I have the following untested patch (on top of my bleeding-edge branch, but
> > it should apply to the mainline too if I haven't overlooked anything).  Can you
> > please try it on boxes with LPSS and see if it doesn't break suspend/resume on them?
> 
> Done, and there were no problems. I tested it with HSW, BYT and also BDW.

Great, thanks!

So I'll add a changelog and I'm going to push it along with your series.

Are you going to update the $subject patch, or send an update on top of
linux-next?

Rafael

Re: [PATCHv2 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

On Fri, May 23, 2014 at 03:10:08PM +0200, Rafael J. Wysocki wrote:
> On Friday, May 23, 2014 03:30:53 PM Heikki Krogerus wrote:
> > On Thu, May 22, 2014 at 01:28:16AM +0200, Rafael J. Wysocki wrote:
> > > On Wednesday, May 21, 2014 01:52:58 PM Heikki Krogerus wrote:
> > > > On Wed, May 21, 2014 at 01:01:31PM +0200, Rafael J. Wysocki wrote:
> > > > > On Wednesday, May 21, 2014 01:05:11 PM Heikki Krogerus wrote:
> > > > > > On Tue, May 20, 2014 at 11:33:09PM +0200, Rafael J. Wysocki wrote:
> > > > > > > First, is the 10 ms sleep really necessary?  I'd expect the AML to take care of
> > > > > > > such delays (this is not a PCI device formally).
> > > > > > 
> > > > > > Unfortunately that is not the case. There is nothing in the AML for
> > > > > > this. Mika, correct me if I'm wrong.
> > > > > > 
> > > > > > > And because this is not a PCI device formally, why is the comment talking about
> > > > > > > the PCI spec?  Why is PCI relevant in any way here?
> > > > > > 
> > > > > > Under the hood the devices are still PCI devices, even if they
> > > > > > formally aren't. Maybe I should point that out in the comment..
> > > > > > 
> > > > > > We put the sleep there because without it there was no guarantee if
> > > > > > the device was properly resumed by the time the drivers resume hooks
> > > > > > were called. The symptom in case of a failure was simply that the
> > > > > > registers could not be written, which leads into timeouts at least in
> > > > > > case of the I2C and UART and making them unusable until the next
> > > > > > suspend followed by resume.
> > > > > 
> > > > > OK, so the msleep() is functionally necessary.  Instead of talking about the
> > > > > PCI in the comment, which will make a casual reader think "What the heck?",
> > > > > please say something like "the delay is necessary for the subsequent register
> > > > > writes to succeed on <example system>".
> > > > 
> > > > OK.
> > > 
> > > So I have one more concern.  Namely, async suspend is not enabled for the LPSS
> > > devices, so the delays will accumulate for them and that may become a big deal
> > > at one point.
> > > 
> > > This may be addressed either (1) by enabling async suspend for them or, which would
> > > be more complicated, by doing the msleep() once for the whole LPSS in .resume_early()
> > > and restoring the register values in .resume() without delaying.
> > > 
> > > For (1) I have the following untested patch (on top of my bleeding-edge branch, but
> > > it should apply to the mainline too if I haven't overlooked anything).  Can you
> > > please try it on boxes with LPSS and see if it doesn't break suspend/resume on them?
> > 
> > Done, and there were no problems. I tested it with HSW, BYT and also BDW.
> 
> Great, thanks!
> 
> So I'll add a changelog and I'm going to push it along with your series.
> 
> Are you going to update the $subject patch, or send an update on top of
> linux-next?

I'll send an update right away. On top of explaining the problem if
left without the msleep(10), I still left the comment about the PCI
spec plus a mention that all LPSS devices are actually PCI devices. I
hope that's OK with you. It just felt like we need to point out that
the 10ms is not pulled out of the hat.


-- 
heikki

[PATCHv3 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

A power domain where we save the context of the additional
LPSS registers. We need to do this or all LPSS devices are
left in reset state when resuming from D3 on some Baytrails.
The devices with the fractional clock divider also have
zeros for N and M values after resuming unless they are
reset.

Li Aubrey found the root cause for the issue. The idea of
using power domain for LPSS came from Mika Westerberg.

Reported-by: Jin Yao <yao.jin@linux.intel.com>
Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c | 152 ++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 145 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 69e29f4..7fe8b6d 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -19,6 +19,7 @@
 #include <linux/platform_device.h>
 #include <linux/platform_data/clk-lpss.h>
 #include <linux/pm_runtime.h>
+#include <linux/delay.h>
 
 #include "internal.h"
 
@@ -43,6 +44,8 @@ ACPI_MODULE_NAME("acpi_lpss");
 #define LPSS_TX_INT			0x20
 #define LPSS_TX_INT_MASK		BIT(1)
 
+#define LPSS_PRV_REG_COUNT		9
+
 struct lpss_shared_clock {
 	const char *name;
 	unsigned long rate;
@@ -58,6 +61,7 @@ struct lpss_device_desc {
 	unsigned int prv_offset;
 	size_t prv_size_override;
 	bool clk_gate;
+	bool save_ctx;
 	struct lpss_shared_clock *shared_clock;
 	void (*setup)(struct lpss_private_data *pdata);
 };
@@ -72,6 +76,7 @@ struct lpss_private_data {
 	resource_size_t mmio_size;
 	struct clk *clk;
 	const struct lpss_device_desc *dev_desc;
+	u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
 };
 
 static void lpss_uart_setup(struct lpss_private_data *pdata)
@@ -116,6 +121,7 @@ static struct lpss_shared_clock pwm_clock = {
 
 static struct lpss_device_desc byt_pwm_dev_desc = {
 	.clk_required = true,
+	.save_ctx = true,
 	.shared_clock = &pwm_clock,
 };
 
@@ -128,6 +134,7 @@ static struct lpss_device_desc byt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.clk_gate = true,
+	.save_ctx = true,
 	.shared_clock = &uart_clock,
 	.setup = lpss_uart_setup,
 };
@@ -141,6 +148,7 @@ static struct lpss_device_desc byt_spi_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x400,
 	.clk_gate = true,
+	.save_ctx = true,
 	.shared_clock = &spi_clock,
 };
 
@@ -156,6 +164,7 @@ static struct lpss_shared_clock i2c_clock = {
 static struct lpss_device_desc byt_i2c_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
+	.save_ctx = true,
 	.shared_clock = &i2c_clock,
 };
 
@@ -449,6 +458,121 @@ static void acpi_lpss_set_ltr(struct device *dev, s32 val)
 	}
 }
 
+#ifdef CONFIG_PM
+/**
+ * acpi_lpss_save_ctx() - Save the private registers of LPSS device
+ * @dev: LPSS device
+ *
+ * Most LPSS devices have private registers which may loose their context when
+ * the device is powered down. acpi_lpss_save_ctx() saves those registers into
+ * prv_reg_ctx array.
+ */
+static void acpi_lpss_save_ctx(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	unsigned int i;
+
+	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
+		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
+		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02lx\n",
+			pdata->prv_reg_ctx[i], i * sizeof(u32));
+	}
+}
+
+/**
+ * acpi_lpss_restore_ctx() - Restore the private registers of LPSS device
+ * @dev: LPSS device
+ *
+ * Restores the registers that were previously stored with acpi_lpss_save_ctx().
+ */
+static void acpi_lpss_restore_ctx(struct device *dev)
+{
+	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
+	unsigned int i;
+
+	/*
+	 * The following delay is needed or the subsequent write operations may
+	 * fail. The LPSS devices are actually PCI devices and the PCI spec
+	 * expects 10ms delay before the device can be accessed after D3 to D0
+	 * transition.
+	 */
+	msleep(10);
+
+	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
+		__lpss_reg_write(pdata->prv_reg_ctx[i], pdata, i * sizeof(u32));
+		dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02lx\n",
+			pdata->prv_reg_ctx[i], i * sizeof(u32));
+	}
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int acpi_lpss_suspend_late(struct device *dev)
+{
+	int ret = pm_generic_suspend_late(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_save_ctx(dev);
+	return acpi_dev_suspend_late(dev);
+}
+
+static int acpi_lpss_restore_early(struct device *dev)
+{
+	int ret = acpi_dev_resume_early(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_restore_ctx(dev);
+	return pm_generic_resume_early(dev);
+}
+#endif /* CONFIG_PM_SLEEP */
+
+#ifdef CONFIG_PM_RUNTIME
+static int acpi_lpss_runtime_suspend(struct device *dev)
+{
+	int ret = pm_generic_runtime_suspend(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_save_ctx(dev);
+	return acpi_dev_runtime_suspend(dev);
+}
+
+static int acpi_lpss_runtime_resume(struct device *dev)
+{
+	int ret = acpi_dev_runtime_resume(dev);
+
+	if (ret)
+		return ret;
+
+	acpi_lpss_restore_ctx(dev);
+	return pm_generic_runtime_resume(dev);
+}
+#endif /* CONFIG_PM_RUNTIME */
+#endif /* CONFIG_PM */
+
+static struct dev_pm_domain acpi_lpss_pm_domain = {
+	.ops = {
+#ifdef CONFIG_PM_SLEEP
+		.suspend_late = acpi_lpss_suspend_late,
+		.restore_early = acpi_lpss_restore_early,
+		.prepare = acpi_subsys_prepare,
+		.suspend = acpi_subsys_suspend,
+		.resume_early = acpi_subsys_resume_early,
+		.freeze = acpi_subsys_freeze,
+		.poweroff = acpi_subsys_suspend,
+		.poweroff_late = acpi_subsys_suspend_late,
+#endif
+#ifdef CONFIG_PM_RUNTIME
+		.runtime_suspend = acpi_lpss_runtime_suspend,
+		.runtime_resume = acpi_lpss_runtime_resume,
+#endif
+	},
+};
+
 static int acpi_lpss_platform_notify(struct notifier_block *nb,
 				     unsigned long action, void *data)
 {
@@ -456,7 +580,6 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 	struct lpss_private_data *pdata;
 	struct acpi_device *adev;
 	const struct acpi_device_id *id;
-	int ret = 0;
 
 	id = acpi_match_device(acpi_lpss_device_ids, &pdev->dev);
 	if (!id || !id->driver_data)
@@ -466,7 +589,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 		return 0;
 
 	pdata = acpi_driver_data(adev);
-	if (!pdata || !pdata->mmio_base || !pdata->dev_desc->ltr_required)
+	if (!pdata || !pdata->mmio_base)
 		return 0;
 
 	if (pdata->mmio_size < pdata->dev_desc->prv_offset + LPSS_LTR_SIZE) {
@@ -474,12 +597,27 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
 		return 0;
 	}
 
-	if (action == BUS_NOTIFY_ADD_DEVICE)
-		ret = sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group);
-	else if (action == BUS_NOTIFY_DEL_DEVICE)
-		sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
+	switch (action) {
+	case BUS_NOTIFY_BOUND_DRIVER:
+		if (pdata->dev_desc->save_ctx)
+			pdev->dev.pm_domain = &acpi_lpss_pm_domain;
+		break;
+	case BUS_NOTIFY_UNBOUND_DRIVER:
+		if (pdata->dev_desc->save_ctx)
+			pdev->dev.pm_domain = NULL;
+		break;
+	case BUS_NOTIFY_ADD_DEVICE:
+		if (pdata->dev_desc->ltr_required)
+			return sysfs_create_group(&pdev->dev.kobj,
+						  &lpss_attr_group);
+	case BUS_NOTIFY_DEL_DEVICE:
+		if (pdata->dev_desc->ltr_required)
+			sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
+	default:
+		break;
+	}
 
-	return ret;
+	return 0;
 }
 
 static struct notifier_block acpi_lpss_nb = {
-- 
2.0.0.rc2

Re: [PATCHv3 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Friday, May 23, 2014 04:15:09 PM Heikki Krogerus wrote:
> A power domain where we save the context of the additional
> LPSS registers. We need to do this or all LPSS devices are
> left in reset state when resuming from D3 on some Baytrails.
> The devices with the fractional clock divider also have
> zeros for N and M values after resuming unless they are
> reset.
> 
> Li Aubrey found the root cause for the issue. The idea of
> using power domain for LPSS came from Mika Westerberg.
> 
> Reported-by: Jin Yao <yao.jin@linux.intel.com>
> Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Queued up for 3.16, but I needed to rebase it to take some other changes
into account.  Can you please have a look and the bleeding-edge branch in
my tree and see if everything looks good in there?

> ---
>  drivers/acpi/acpi_lpss.c | 152 ++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 145 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
> index 69e29f4..7fe8b6d 100644
> --- a/drivers/acpi/acpi_lpss.c
> +++ b/drivers/acpi/acpi_lpss.c
> @@ -19,6 +19,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/platform_data/clk-lpss.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/delay.h>
>  
>  #include "internal.h"
>  
> @@ -43,6 +44,8 @@ ACPI_MODULE_NAME("acpi_lpss");
>  #define LPSS_TX_INT			0x20
>  #define LPSS_TX_INT_MASK		BIT(1)
>  
> +#define LPSS_PRV_REG_COUNT		9
> +
>  struct lpss_shared_clock {
>  	const char *name;
>  	unsigned long rate;
> @@ -58,6 +61,7 @@ struct lpss_device_desc {
>  	unsigned int prv_offset;
>  	size_t prv_size_override;
>  	bool clk_gate;
> +	bool save_ctx;
>  	struct lpss_shared_clock *shared_clock;
>  	void (*setup)(struct lpss_private_data *pdata);
>  };
> @@ -72,6 +76,7 @@ struct lpss_private_data {
>  	resource_size_t mmio_size;
>  	struct clk *clk;
>  	const struct lpss_device_desc *dev_desc;
> +	u32 prv_reg_ctx[LPSS_PRV_REG_COUNT];
>  };
>  
>  static void lpss_uart_setup(struct lpss_private_data *pdata)
> @@ -116,6 +121,7 @@ static struct lpss_shared_clock pwm_clock = {
>  
>  static struct lpss_device_desc byt_pwm_dev_desc = {
>  	.clk_required = true,
> +	.save_ctx = true,
>  	.shared_clock = &pwm_clock,
>  };
>  
> @@ -128,6 +134,7 @@ static struct lpss_device_desc byt_uart_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x800,
>  	.clk_gate = true,
> +	.save_ctx = true,
>  	.shared_clock = &uart_clock,
>  	.setup = lpss_uart_setup,
>  };
> @@ -141,6 +148,7 @@ static struct lpss_device_desc byt_spi_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x400,
>  	.clk_gate = true,
> +	.save_ctx = true,
>  	.shared_clock = &spi_clock,
>  };
>  
> @@ -156,6 +164,7 @@ static struct lpss_shared_clock i2c_clock = {
>  static struct lpss_device_desc byt_i2c_dev_desc = {
>  	.clk_required = true,
>  	.prv_offset = 0x800,
> +	.save_ctx = true,
>  	.shared_clock = &i2c_clock,
>  };
>  
> @@ -449,6 +458,121 @@ static void acpi_lpss_set_ltr(struct device *dev, s32 val)
>  	}
>  }
>  
> +#ifdef CONFIG_PM
> +/**
> + * acpi_lpss_save_ctx() - Save the private registers of LPSS device
> + * @dev: LPSS device
> + *
> + * Most LPSS devices have private registers which may loose their context when
> + * the device is powered down. acpi_lpss_save_ctx() saves those registers into
> + * prv_reg_ctx array.
> + */
> +static void acpi_lpss_save_ctx(struct device *dev)
> +{
> +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +	unsigned int i;
> +
> +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> +		pdata->prv_reg_ctx[i] = __lpss_reg_read(pdata, i * sizeof(u32));
> +		dev_dbg(dev, "saving 0x%08x from LPSS reg at offset 0x%02lx\n",
> +			pdata->prv_reg_ctx[i], i * sizeof(u32));
> +	}
> +}
> +
> +/**
> + * acpi_lpss_restore_ctx() - Restore the private registers of LPSS device
> + * @dev: LPSS device
> + *
> + * Restores the registers that were previously stored with acpi_lpss_save_ctx().
> + */
> +static void acpi_lpss_restore_ctx(struct device *dev)
> +{
> +	struct lpss_private_data *pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +	unsigned int i;
> +
> +	/*
> +	 * The following delay is needed or the subsequent write operations may
> +	 * fail. The LPSS devices are actually PCI devices and the PCI spec
> +	 * expects 10ms delay before the device can be accessed after D3 to D0
> +	 * transition.
> +	 */
> +	msleep(10);
> +
> +	for (i = 0; i < LPSS_PRV_REG_COUNT; i++) {
> +		__lpss_reg_write(pdata->prv_reg_ctx[i], pdata, i * sizeof(u32));
> +		dev_dbg(dev, "restoring 0x%08x to LPSS reg at offset 0x%02lx\n",
> +			pdata->prv_reg_ctx[i], i * sizeof(u32));
> +	}
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int acpi_lpss_suspend_late(struct device *dev)
> +{
> +	int ret = pm_generic_suspend_late(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_save_ctx(dev);
> +	return acpi_dev_suspend_late(dev);
> +}
> +
> +static int acpi_lpss_restore_early(struct device *dev)
> +{
> +	int ret = acpi_dev_resume_early(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_restore_ctx(dev);
> +	return pm_generic_resume_early(dev);
> +}
> +#endif /* CONFIG_PM_SLEEP */
> +
> +#ifdef CONFIG_PM_RUNTIME
> +static int acpi_lpss_runtime_suspend(struct device *dev)
> +{
> +	int ret = pm_generic_runtime_suspend(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_save_ctx(dev);
> +	return acpi_dev_runtime_suspend(dev);
> +}
> +
> +static int acpi_lpss_runtime_resume(struct device *dev)
> +{
> +	int ret = acpi_dev_runtime_resume(dev);
> +
> +	if (ret)
> +		return ret;
> +
> +	acpi_lpss_restore_ctx(dev);
> +	return pm_generic_runtime_resume(dev);
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +#endif /* CONFIG_PM */
> +
> +static struct dev_pm_domain acpi_lpss_pm_domain = {
> +	.ops = {
> +#ifdef CONFIG_PM_SLEEP
> +		.suspend_late = acpi_lpss_suspend_late,
> +		.restore_early = acpi_lpss_restore_early,
> +		.prepare = acpi_subsys_prepare,
> +		.suspend = acpi_subsys_suspend,
> +		.resume_early = acpi_subsys_resume_early,
> +		.freeze = acpi_subsys_freeze,
> +		.poweroff = acpi_subsys_suspend,
> +		.poweroff_late = acpi_subsys_suspend_late,
> +#endif
> +#ifdef CONFIG_PM_RUNTIME
> +		.runtime_suspend = acpi_lpss_runtime_suspend,
> +		.runtime_resume = acpi_lpss_runtime_resume,
> +#endif
> +	},
> +};
> +
>  static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  				     unsigned long action, void *data)
>  {
> @@ -456,7 +580,6 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  	struct lpss_private_data *pdata;
>  	struct acpi_device *adev;
>  	const struct acpi_device_id *id;
> -	int ret = 0;
>  
>  	id = acpi_match_device(acpi_lpss_device_ids, &pdev->dev);
>  	if (!id || !id->driver_data)
> @@ -466,7 +589,7 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  		return 0;
>  
>  	pdata = acpi_driver_data(adev);
> -	if (!pdata || !pdata->mmio_base || !pdata->dev_desc->ltr_required)
> +	if (!pdata || !pdata->mmio_base)
>  		return 0;
>  
>  	if (pdata->mmio_size < pdata->dev_desc->prv_offset + LPSS_LTR_SIZE) {
> @@ -474,12 +597,27 @@ static int acpi_lpss_platform_notify(struct notifier_block *nb,
>  		return 0;
>  	}
>  
> -	if (action == BUS_NOTIFY_ADD_DEVICE)
> -		ret = sysfs_create_group(&pdev->dev.kobj, &lpss_attr_group);
> -	else if (action == BUS_NOTIFY_DEL_DEVICE)
> -		sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> +	switch (action) {
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		if (pdata->dev_desc->save_ctx)
> +			pdev->dev.pm_domain = &acpi_lpss_pm_domain;
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		if (pdata->dev_desc->save_ctx)
> +			pdev->dev.pm_domain = NULL;
> +		break;
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if (pdata->dev_desc->ltr_required)
> +			return sysfs_create_group(&pdev->dev.kobj,
> +						  &lpss_attr_group);
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if (pdata->dev_desc->ltr_required)
> +			sysfs_remove_group(&pdev->dev.kobj, &lpss_attr_group);
> +	default:
> +		break;
> +	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static struct notifier_block acpi_lpss_nb = {
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCHv3 2/4] ACPI / LPSS: custom power domain for LPSS

[Heikki Krogerus]

On Mon, May 26, 2014 at 03:03:27PM +0200, Rafael J. Wysocki wrote:
> On Friday, May 23, 2014 04:15:09 PM Heikki Krogerus wrote:
> > A power domain where we save the context of the additional
> > LPSS registers. We need to do this or all LPSS devices are
> > left in reset state when resuming from D3 on some Baytrails.
> > The devices with the fractional clock divider also have
> > zeros for N and M values after resuming unless they are
> > reset.
> > 
> > Li Aubrey found the root cause for the issue. The idea of
> > using power domain for LPSS came from Mika Westerberg.
> > 
> > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
> > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Queued up for 3.16, but I needed to rebase it to take some other changes
> into account.  Can you please have a look and the bleeding-edge branch in
> my tree and see if everything looks good in there?

I checked the bleeding-edge and it looks good to me. Thanks for taking
care of the issue with "%lx".

About the sparse warning from the clk-fractional-divider.c. Since
there is no real issue, let's follow the style in the other clock
types and ignore the warning. So let's keep that patch as it is. OK?



-- 
heikki

Re: [PATCHv3 2/4] ACPI / LPSS: custom power domain for LPSS

[Rafael J. Wysocki]

On Monday, May 26, 2014 04:42:34 PM Heikki Krogerus wrote:
> On Mon, May 26, 2014 at 03:03:27PM +0200, Rafael J. Wysocki wrote:
> > On Friday, May 23, 2014 04:15:09 PM Heikki Krogerus wrote:
> > > A power domain where we save the context of the additional
> > > LPSS registers. We need to do this or all LPSS devices are
> > > left in reset state when resuming from D3 on some Baytrails.
> > > The devices with the fractional clock divider also have
> > > zeros for N and M values after resuming unless they are
> > > reset.
> > > 
> > > Li Aubrey found the root cause for the issue. The idea of
> > > using power domain for LPSS came from Mika Westerberg.
> > > 
> > > Reported-by: Jin Yao <yao.jin@linux.intel.com>
> > > Suggested-by: Li Aubrey <aubrey.li@linux.intel.com>
> > > Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > 
> > Queued up for 3.16, but I needed to rebase it to take some other changes
> > into account.  Can you please have a look and the bleeding-edge branch in
> > my tree and see if everything looks good in there?
> 
> I checked the bleeding-edge and it looks good to me. Thanks for taking
> care of the issue with "%lx".
> 
> About the sparse warning from the clk-fractional-divider.c. Since
> there is no real issue, let's follow the style in the other clock
> types and ignore the warning. So let's keep that patch as it is. OK?

That's fine by me.

Rafael

[PATCHv2 3/4] clk: new basic clk type for fractional divider

[Heikki Krogerus]

Fractional divider clocks are fairly common. This adds basic
type for them.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/clk/Makefile                 |   1 +
 drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++++++++++++++
 include/linux/clk-provider.h         |  31 ++++++++
 3 files changed, 167 insertions(+)
 create mode 100644 drivers/clk/clk-fractional-divider.c

diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index f651b2a..33bc79e 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)	+= clk-fixed-rate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-gate.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-mux.o
 obj-$(CONFIG_COMMON_CLK)	+= clk-composite.o
+obj-$(CONFIG_COMMON_CLK)	+= clk-fractional-divider.o
 
 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
new file mode 100644
index 0000000..ede685c
--- /dev/null
+++ b/drivers/clk/clk-fractional-divider.c
@@ -0,0 +1,135 @@
+/*
+ * Copyright (C) 2014 Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Adjustable fractional divider clock implementation.
+ * Output rate = (m / n) * parent_rate.
+ */
+
+#include <linux/clk-provider.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/slab.h>
+#include <linux/gcd.h>
+
+#define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
+
+static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
+					unsigned long parent_rate)
+{
+	struct clk_fractional_divider *fd = to_clk_fd(hw);
+	unsigned long flags = 0;
+	u32 val, m, n;
+	u64 ret;
+
+	if (fd->lock)
+		spin_lock_irqsave(fd->lock, flags);
+
+	val = clk_readl(fd->reg);
+
+	if (fd->lock)
+		spin_unlock_irqrestore(fd->lock, flags);
+
+	m = (val & fd->mmask) >> fd->mshift;
+	n = (val & fd->nmask) >> fd->nshift;
+
+	ret = parent_rate * m;
+	do_div(ret, n);
+
+	return ret;
+}
+
+static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
+			      unsigned long *prate)
+{
+	struct clk_fractional_divider *fd = to_clk_fd(hw);
+	unsigned maxn = (fd->nmask >> fd->nshift) + 1;
+	unsigned div;
+
+	if (!rate || rate >= *prate)
+		return *prate;
+
+	div = gcd(*prate, rate);
+
+	while ((*prate / div) > maxn) {
+		div <<= 1;
+		rate <<= 1;
+	}
+
+	return rate;
+}
+
+static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
+			   unsigned long parent_rate)
+{
+	struct clk_fractional_divider *fd = to_clk_fd(hw);
+	unsigned long flags = 0;
+	unsigned long div;
+	unsigned n, m;
+	u32 val;
+
+	div = gcd(parent_rate, rate);
+	m = rate / div;
+	n = parent_rate / div;
+
+	if (fd->lock)
+		spin_lock_irqsave(fd->lock, flags);
+
+	val = clk_readl(fd->reg);
+	val &= ~(fd->mmask | fd->nmask);
+	val |= (m << fd->mshift) | (n << fd->nshift);
+	clk_writel(val, fd->reg);
+
+	if (fd->lock)
+		spin_unlock_irqrestore(fd->lock, flags);
+
+	return 0;
+}
+
+const struct clk_ops clk_fractional_divider_ops = {
+	.recalc_rate = clk_fd_recalc_rate,
+	.round_rate = clk_fd_round_rate,
+	.set_rate = clk_fd_set_rate,
+};
+EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
+
+struct clk *clk_register_fractional_divider(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock)
+{
+	struct clk_fractional_divider *fd;
+	struct clk_init_data init;
+	struct clk *clk;
+
+	fd = kzalloc(sizeof(*fd), GFP_KERNEL);
+	if (!fd) {
+		dev_err(dev, "could not allocate fractional divider clk\n");
+		return ERR_PTR(-ENOMEM);
+	}
+
+	init.name = name;
+	init.ops = &clk_fractional_divider_ops;
+	init.flags = flags | CLK_IS_BASIC;
+	init.parent_names = parent_name ? &parent_name : NULL;
+	init.num_parents = parent_name ? 1 : 0;
+
+	fd->reg = reg;
+	fd->mshift = mshift;
+	fd->mmask = (BIT(mwidth) - 1) << mshift;
+	fd->nshift = nshift;
+	fd->nmask = (BIT(nwidth) - 1) << nshift;
+	fd->flags = clk_divider_flags;
+	fd->lock = lock;
+	fd->hw.init = &init;
+
+	clk = clk_register(dev, &fd->hw);
+	if (IS_ERR(clk))
+		kfree(fd);
+
+	return clk;
+}
+EXPORT_SYMBOL_GPL(clk_register_fractional_divider);
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 4080943..d76188a 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -422,6 +422,37 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
 		const char *parent_name, unsigned long flags,
 		unsigned int mult, unsigned int div);
 
+/**
+ * struct clk_fractional_divider - adjustable fractional divider clock
+ *
+ * @hw:		handle between common and hardware-specific interfaces
+ * @reg:	register containing the divider
+ * @mshift:	shift to the numerator bit field
+ * @mwidth:	width of the numerator bit field
+ * @nshift:	shift to the denominator bit field
+ * @nwidth:	width of the denominator bit field
+ * @lock:	register lock
+ *
+ * Clock with adjustable fractional divider affecting its output frequency.
+ */
+
+struct clk_fractional_divider {
+	struct clk_hw	hw;
+	void __iomem	*reg;
+	u8		mshift;
+	u32		mmask;
+	u8		nshift;
+	u32		nmask;
+	u8		flags;
+	spinlock_t	*lock;
+};
+
+extern const struct clk_ops clk_fractional_divider_ops;
+struct clk *clk_register_fractional_divider(struct device *dev,
+		const char *name, const char *parent_name, unsigned long flags,
+		void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
+		u8 clk_divider_flags, spinlock_t *lock);
+
 /***
  * struct clk_composite - aggregate clock of mux, divider and gate clocks
  *
-- 
2.0.0.rc2

Re: [PATCHv2 3/4] clk: new basic clk type for fractional divider

[Mike Turquette]

Quoting Heikki Krogerus (2014-05-15 06:40:25)
> Fractional divider clocks are fairly common. This adds basic
> type for them.
> 
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>

Taken into clk-next.

Just FYI, there was some talk at Embedded Linux Conference on providing
a better abstraction layer for some of these "basic" clock types. This
abstraction would allow the basic clock types to implement the
machine-agnostic logic (e.g. an incoming rate is divided by a value) and
then platforms and drivers could plug in the machine-specific parts
(e.g. divider is made up of m/n, or divider is power-of-two, or divider
is a simple integer with min == 1 and max == 5).

All of that is to say that in time this fractional divider could go away
once the abstraction layer allows us to fold the m/n divider stuff into
a core divider implementation.

Nothing wrong with the patch for now, so I've taken it for 3.16.

Regards,
Mike

> ---
>  drivers/clk/Makefile                 |   1 +
>  drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++++++++++++++
>  include/linux/clk-provider.h         |  31 ++++++++
>  3 files changed, 167 insertions(+)
>  create mode 100644 drivers/clk/clk-fractional-divider.c
> 
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index f651b2a..33bc79e 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_COMMON_CLK)        += clk-fixed-rate.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-gate.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-mux.o
>  obj-$(CONFIG_COMMON_CLK)       += clk-composite.o
> +obj-$(CONFIG_COMMON_CLK)       += clk-fractional-divider.o
>  
>  # hardware specific clock types
>  # please keep this section sorted lexicographically by file/directory path name
> diff --git a/drivers/clk/clk-fractional-divider.c b/drivers/clk/clk-fractional-divider.c
> new file mode 100644
> index 0000000..ede685c
> --- /dev/null
> +++ b/drivers/clk/clk-fractional-divider.c
> @@ -0,0 +1,135 @@
> +/*
> + * Copyright (C) 2014 Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * Adjustable fractional divider clock implementation.
> + * Output rate = (m / n) * parent_rate.
> + */
> +
> +#include <linux/clk-provider.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/slab.h>
> +#include <linux/gcd.h>
> +
> +#define to_clk_fd(_hw) container_of(_hw, struct clk_fractional_divider, hw)
> +
> +static unsigned long clk_fd_recalc_rate(struct clk_hw *hw,
> +                                       unsigned long parent_rate)
> +{
> +       struct clk_fractional_divider *fd = to_clk_fd(hw);
> +       unsigned long flags = 0;
> +       u32 val, m, n;
> +       u64 ret;
> +
> +       if (fd->lock)
> +               spin_lock_irqsave(fd->lock, flags);
> +
> +       val = clk_readl(fd->reg);
> +
> +       if (fd->lock)
> +               spin_unlock_irqrestore(fd->lock, flags);
> +
> +       m = (val & fd->mmask) >> fd->mshift;
> +       n = (val & fd->nmask) >> fd->nshift;
> +
> +       ret = parent_rate * m;
> +       do_div(ret, n);
> +
> +       return ret;
> +}
> +
> +static long clk_fd_round_rate(struct clk_hw *hw, unsigned long rate,
> +                             unsigned long *prate)
> +{
> +       struct clk_fractional_divider *fd = to_clk_fd(hw);
> +       unsigned maxn = (fd->nmask >> fd->nshift) + 1;
> +       unsigned div;
> +
> +       if (!rate || rate >= *prate)
> +               return *prate;
> +
> +       div = gcd(*prate, rate);
> +
> +       while ((*prate / div) > maxn) {
> +               div <<= 1;
> +               rate <<= 1;
> +       }
> +
> +       return rate;
> +}
> +
> +static int clk_fd_set_rate(struct clk_hw *hw, unsigned long rate,
> +                          unsigned long parent_rate)
> +{
> +       struct clk_fractional_divider *fd = to_clk_fd(hw);
> +       unsigned long flags = 0;
> +       unsigned long div;
> +       unsigned n, m;
> +       u32 val;
> +
> +       div = gcd(parent_rate, rate);
> +       m = rate / div;
> +       n = parent_rate / div;
> +
> +       if (fd->lock)
> +               spin_lock_irqsave(fd->lock, flags);
> +
> +       val = clk_readl(fd->reg);
> +       val &= ~(fd->mmask | fd->nmask);
> +       val |= (m << fd->mshift) | (n << fd->nshift);
> +       clk_writel(val, fd->reg);
> +
> +       if (fd->lock)
> +               spin_unlock_irqrestore(fd->lock, flags);
> +
> +       return 0;
> +}
> +
> +const struct clk_ops clk_fractional_divider_ops = {
> +       .recalc_rate = clk_fd_recalc_rate,
> +       .round_rate = clk_fd_round_rate,
> +       .set_rate = clk_fd_set_rate,
> +};
> +EXPORT_SYMBOL_GPL(clk_fractional_divider_ops);
> +
> +struct clk *clk_register_fractional_divider(struct device *dev,
> +               const char *name, const char *parent_name, unsigned long flags,
> +               void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +               u8 clk_divider_flags, spinlock_t *lock)
> +{
> +       struct clk_fractional_divider *fd;
> +       struct clk_init_data init;
> +       struct clk *clk;
> +
> +       fd = kzalloc(sizeof(*fd), GFP_KERNEL);
> +       if (!fd) {
> +               dev_err(dev, "could not allocate fractional divider clk\n");
> +               return ERR_PTR(-ENOMEM);
> +       }
> +
> +       init.name = name;
> +       init.ops = &clk_fractional_divider_ops;
> +       init.flags = flags | CLK_IS_BASIC;
> +       init.parent_names = parent_name ? &parent_name : NULL;
> +       init.num_parents = parent_name ? 1 : 0;
> +
> +       fd->reg = reg;
> +       fd->mshift = mshift;
> +       fd->mmask = (BIT(mwidth) - 1) << mshift;
> +       fd->nshift = nshift;
> +       fd->nmask = (BIT(nwidth) - 1) << nshift;
> +       fd->flags = clk_divider_flags;
> +       fd->lock = lock;
> +       fd->hw.init = &init;
> +
> +       clk = clk_register(dev, &fd->hw);
> +       if (IS_ERR(clk))
> +               kfree(fd);
> +
> +       return clk;
> +}
> +EXPORT_SYMBOL_GPL(clk_register_fractional_divider);
> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
> index 4080943..d76188a 100644
> --- a/include/linux/clk-provider.h
> +++ b/include/linux/clk-provider.h
> @@ -422,6 +422,37 @@ struct clk *clk_register_fixed_factor(struct device *dev, const char *name,
>                 const char *parent_name, unsigned long flags,
>                 unsigned int mult, unsigned int div);
>  
> +/**
> + * struct clk_fractional_divider - adjustable fractional divider clock
> + *
> + * @hw:                handle between common and hardware-specific interfaces
> + * @reg:       register containing the divider
> + * @mshift:    shift to the numerator bit field
> + * @mwidth:    width of the numerator bit field
> + * @nshift:    shift to the denominator bit field
> + * @nwidth:    width of the denominator bit field
> + * @lock:      register lock
> + *
> + * Clock with adjustable fractional divider affecting its output frequency.
> + */
> +
> +struct clk_fractional_divider {
> +       struct clk_hw   hw;
> +       void __iomem    *reg;
> +       u8              mshift;
> +       u32             mmask;
> +       u8              nshift;
> +       u32             nmask;
> +       u8              flags;
> +       spinlock_t      *lock;
> +};
> +
> +extern const struct clk_ops clk_fractional_divider_ops;
> +struct clk *clk_register_fractional_divider(struct device *dev,
> +               const char *name, const char *parent_name, unsigned long flags,
> +               void __iomem *reg, u8 mshift, u8 mwidth, u8 nshift, u8 nwidth,
> +               u8 clk_divider_flags, spinlock_t *lock);
> +
>  /***
>   * struct clk_composite - aggregate clock of mux, divider and gate clocks
>   *
> -- 
> 2.0.0.rc2
> 

Re: [PATCHv2 3/4] clk: new basic clk type for fractional divider

[Rafael J. Wysocki]

On Thursday, May 15, 2014 09:53:49 AM Mike Turquette wrote:
> Quoting Heikki Krogerus (2014-05-15 06:40:25)
> > Fractional divider clocks are fairly common. This adds basic
> > type for them.
> > 
> > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> 
> Taken into clk-next.
> 
> Just FYI, there was some talk at Embedded Linux Conference on providing
> a better abstraction layer for some of these "basic" clock types. This
> abstraction would allow the basic clock types to implement the
> machine-agnostic logic (e.g. an incoming rate is divided by a value) and
> then platforms and drivers could plug in the machine-specific parts
> (e.g. divider is made up of m/n, or divider is power-of-two, or divider
> is a simple integer with min == 1 and max == 5).
> 
> All of that is to say that in time this fractional divider could go away
> once the abstraction layer allows us to fold the m/n divider stuff into
> a core divider implementation.
> 
> Nothing wrong with the patch for now, so I've taken it for 3.16.

Well, OK, but I guess [4/4] depends on it and [4/4] also depends on
[1-2/4], so how you're proposing to resolve this?

Rafael

Re: [PATCHv2 3/4] clk: new basic clk type for fractional divider

[Rafael J. Wysocki]

On Friday, May 16, 2014 04:09:05 PM Mike Turquette wrote:
> Quoting Rafael J. Wysocki (2014-05-16 15:38:05)
> > On Thursday, May 15, 2014 09:53:49 AM Mike Turquette wrote:
> > > Quoting Heikki Krogerus (2014-05-15 06:40:25)
> > > > Fractional divider clocks are fairly common. This adds basic
> > > > type for them.
> > > > 
> > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > 
> > > Taken into clk-next.
> > > 
> > > Just FYI, there was some talk at Embedded Linux Conference on providing
> > > a better abstraction layer for some of these "basic" clock types. This
> > > abstraction would allow the basic clock types to implement the
> > > machine-agnostic logic (e.g. an incoming rate is divided by a value) and
> > > then platforms and drivers could plug in the machine-specific parts
> > > (e.g. divider is made up of m/n, or divider is power-of-two, or divider
> > > is a simple integer with min == 1 and max == 5).
> > > 
> > > All of that is to say that in time this fractional divider could go away
> > > once the abstraction layer allows us to fold the m/n divider stuff into
> > > a core divider implementation.
> > > 
> > > Nothing wrong with the patch for now, so I've taken it for 3.16.
> > 
> > Well, OK, but I guess [4/4] depends on it and [4/4] also depends on
> > [1-2/4], so how you're proposing to resolve this?
> 
> I haven't yet published this patch to my tree, so no harm done. How
> about the following Formal Proposals?
> 
> 1) I ack the patch and don't merge it through my tree. You take it.
> 2) I provide a topic branch that we both merge.
> 
> Your choice.

Thanks!  I'd prefer 1) to be honest. :-)

Rafael

Re: [PATCHv2 3/4] clk: new basic clk type for fractional divider

[Rafael J. Wysocki]

On Friday, May 16, 2014 06:34:03 PM Mike Turquette wrote:
> Quoting Rafael J. Wysocki (2014-05-16 17:15:43)
> > On Friday, May 16, 2014 04:09:05 PM Mike Turquette wrote:
> > > Quoting Rafael J. Wysocki (2014-05-16 15:38:05)
> > > > On Thursday, May 15, 2014 09:53:49 AM Mike Turquette wrote:
> > > > > Quoting Heikki Krogerus (2014-05-15 06:40:25)
> > > > > > Fractional divider clocks are fairly common. This adds basic
> > > > > > type for them.
> > > > > > 
> > > > > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > 
> > > > > Taken into clk-next.
> > > > > 
> > > > > Just FYI, there was some talk at Embedded Linux Conference on providing
> > > > > a better abstraction layer for some of these "basic" clock types. This
> > > > > abstraction would allow the basic clock types to implement the
> > > > > machine-agnostic logic (e.g. an incoming rate is divided by a value) and
> > > > > then platforms and drivers could plug in the machine-specific parts
> > > > > (e.g. divider is made up of m/n, or divider is power-of-two, or divider
> > > > > is a simple integer with min == 1 and max == 5).
> > > > > 
> > > > > All of that is to say that in time this fractional divider could go away
> > > > > once the abstraction layer allows us to fold the m/n divider stuff into
> > > > > a core divider implementation.
> > > > > 
> > > > > Nothing wrong with the patch for now, so I've taken it for 3.16.
> > > > 
> > > > Well, OK, but I guess [4/4] depends on it and [4/4] also depends on
> > > > [1-2/4], so how you're proposing to resolve this?
> > > 
> > > I haven't yet published this patch to my tree, so no harm done. How
> > > about the following Formal Proposals?
> > > 
> > > 1) I ack the patch and don't merge it through my tree. You take it.
> > > 2) I provide a topic branch that we both merge.
> > > 
> > > Your choice.
> > 
> > Thanks!  I'd prefer 1) to be honest. :-)
> 
> Acked-by: Mike Turquette <mturquette@linaro.org>

Thanks!

[PATCHv2 4/4] ACPI / LPSS: support for fractional divider clock

[Heikki Krogerus]

This creates fractional divider type clock for the ones that
have it. It is needed by the UART driver as the clock rate must
accommodate to the requested baud rate.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c | 71 ++++++++++++++++++++++++++++++++++--------------
 1 file changed, 50 insertions(+), 21 deletions(-)

diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index 24e49a5..15eb571 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -29,6 +29,7 @@ ACPI_MODULE_NAME("acpi_lpss");
 #define LPSS_LTR_SIZE	0x18
 
 /* Offsets relative to LPSS_PRIVATE_OFFSET */
+#define LPSS_CLK_DIVIDER_DEF_MASK	(BIT(1) | BIT(16))
 #define LPSS_GENERAL			0x08
 #define LPSS_GENERAL_LTR_MODE_SW	BIT(2)
 #define LPSS_GENERAL_UART_RTS_OVRD	BIT(3)
@@ -60,6 +61,7 @@ struct lpss_device_desc {
 	bool ltr_required;
 	unsigned int prv_offset;
 	size_t prv_size_override;
+	bool clk_divider;
 	bool clk_gate;
 	bool save_ctx;
 	struct lpss_shared_clock *shared_clock;
@@ -97,6 +99,14 @@ static struct lpss_device_desc lpt_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.ltr_required = true,
+	.clk_divider = true,
+	.clk_gate = true,
+};
+
+static struct lpss_device_desc lpt_i2c_dev_desc = {
+	.clk_required = true,
+	.prv_offset = 0x800,
+	.ltr_required = true,
 	.clk_gate = true,
 };
 
@@ -104,6 +114,7 @@ static struct lpss_device_desc lpt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.ltr_required = true,
+	.clk_divider = true,
 	.clk_gate = true,
 	.setup = lpss_uart_setup,
 };
@@ -125,31 +136,21 @@ static struct lpss_device_desc byt_pwm_dev_desc = {
 	.shared_clock = &pwm_clock,
 };
 
-static struct lpss_shared_clock uart_clock = {
-	.name = "uart_clk",
-	.rate = 44236800,
-};
-
 static struct lpss_device_desc byt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
+	.clk_divider = true,
 	.clk_gate = true,
 	.save_ctx = true,
-	.shared_clock = &uart_clock,
 	.setup = lpss_uart_setup,
 };
 
-static struct lpss_shared_clock spi_clock = {
-	.name = "spi_clk",
-	.rate = 50000000,
-};
-
 static struct lpss_device_desc byt_spi_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x400,
+	.clk_divider = true,
 	.clk_gate = true,
 	.save_ctx = true,
-	.shared_clock = &spi_clock,
 };
 
 static struct lpss_device_desc byt_sdio_dev_desc = {
@@ -175,8 +176,8 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = {
 	/* Lynxpoint LPSS devices */
 	{ "INT33C0", (unsigned long)&lpt_dev_desc },
 	{ "INT33C1", (unsigned long)&lpt_dev_desc },
-	{ "INT33C2", (unsigned long)&lpt_dev_desc },
-	{ "INT33C3", (unsigned long)&lpt_dev_desc },
+	{ "INT33C2", (unsigned long)&lpt_i2c_dev_desc },
+	{ "INT33C3", (unsigned long)&lpt_i2c_dev_desc },
 	{ "INT33C4", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT33C5", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT33C6", (unsigned long)&lpt_sdio_dev_desc },
@@ -221,9 +222,11 @@ static int register_device_clock(struct acpi_device *adev,
 {
 	const struct lpss_device_desc *dev_desc = pdata->dev_desc;
 	struct lpss_shared_clock *shared_clock = dev_desc->shared_clock;
+	const char *devname = dev_name(&adev->dev);
 	struct clk *clk = ERR_PTR(-ENODEV);
 	struct lpss_clk_data *clk_data;
-	const char *parent;
+	const char *parent, *clk_name;
+	void __iomem *prv_base;
 
 	if (!lpss_clk_dev)
 		lpt_register_clock_device();
@@ -234,7 +237,7 @@ static int register_device_clock(struct acpi_device *adev,
 
 	if (dev_desc->clkdev_name) {
 		clk_register_clkdev(clk_data->clk, dev_desc->clkdev_name,
-				    dev_name(&adev->dev));
+				    devname);
 		return 0;
 	}
 
@@ -243,6 +246,7 @@ static int register_device_clock(struct acpi_device *adev,
 		return -ENODATA;
 
 	parent = clk_data->name;
+	prv_base = pdata->mmio_base + dev_desc->prv_offset;
 
 	if (shared_clock) {
 		clk = shared_clock->clk;
@@ -256,16 +260,41 @@ static int register_device_clock(struct acpi_device *adev,
 	}
 
 	if (dev_desc->clk_gate) {
-		clk = clk_register_gate(NULL, dev_name(&adev->dev), parent, 0,
-					pdata->mmio_base + dev_desc->prv_offset,
-					0, 0, NULL);
-		pdata->clk = clk;
+		clk = clk_register_gate(NULL, devname, parent, 0,
+					prv_base, 0, 0, NULL);
+		parent = devname;
+	}
+
+	if (dev_desc->clk_divider) {
+		/* Prevent division by zero */
+		if (!readl(prv_base))
+			writel(LPSS_CLK_DIVIDER_DEF_MASK, prv_base);
+
+		clk_name = kasprintf(GFP_KERNEL, "%s-div", devname);
+		if (!clk_name)
+			return -ENOMEM;
+		clk = clk_register_fractional_divider(NULL, clk_name, parent,
+						      0, prv_base,
+						      1, 15, 16, 15, 0, NULL);
+		parent = clk_name;
+
+		clk_name = kasprintf(GFP_KERNEL, "%s-update", devname);
+		if (!clk_name) {
+			kfree(parent);
+			return -ENOMEM;
+		}
+		clk = clk_register_gate(NULL, clk_name, parent,
+					CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
+					prv_base, 31, 0, NULL);
+		kfree(parent);
+		kfree(clk_name);
 	}
 
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	clk_register_clkdev(clk, NULL, dev_name(&adev->dev));
+	pdata->clk = clk;
+	clk_register_clkdev(clk, NULL, devname);
 	return 0;
 }
 
-- 
2.0.0.rc2

[PATCHv3 4/4] ACPI / LPSS: support for fractional divider clock

[Heikki Krogerus]

This creates fractional divider type clock for the ones that
have it. It is needed by the UART driver as the clock rate must
accommodate to the requested baud rate.

Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
---
 drivers/acpi/acpi_lpss.c | 75 +++++++++++++++++++++++++++++++++---------------
 1 file changed, 52 insertions(+), 23 deletions(-)

Hi Rafael,

I had to make one more version of this patch..

This changes also the Broadwells to use lpt_i2c_dev_desc with the I2C
devices. Just like on Haswells, on Broadwells the I2C does not have
the dividers, just the gate. No other changes.


diff --git a/drivers/acpi/acpi_lpss.c b/drivers/acpi/acpi_lpss.c
index c314d70..0576423 100644
--- a/drivers/acpi/acpi_lpss.c
+++ b/drivers/acpi/acpi_lpss.c
@@ -29,6 +29,7 @@ ACPI_MODULE_NAME("acpi_lpss");
 #define LPSS_LTR_SIZE	0x18
 
 /* Offsets relative to LPSS_PRIVATE_OFFSET */
+#define LPSS_CLK_DIVIDER_DEF_MASK	(BIT(1) | BIT(16))
 #define LPSS_GENERAL			0x08
 #define LPSS_GENERAL_LTR_MODE_SW	BIT(2)
 #define LPSS_GENERAL_UART_RTS_OVRD	BIT(3)
@@ -60,6 +61,7 @@ struct lpss_device_desc {
 	bool ltr_required;
 	unsigned int prv_offset;
 	size_t prv_size_override;
+	bool clk_divider;
 	bool clk_gate;
 	bool save_ctx;
 	struct lpss_shared_clock *shared_clock;
@@ -97,6 +99,14 @@ static struct lpss_device_desc lpt_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.ltr_required = true,
+	.clk_divider = true,
+	.clk_gate = true,
+};
+
+static struct lpss_device_desc lpt_i2c_dev_desc = {
+	.clk_required = true,
+	.prv_offset = 0x800,
+	.ltr_required = true,
 	.clk_gate = true,
 };
 
@@ -104,6 +114,7 @@ static struct lpss_device_desc lpt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
 	.ltr_required = true,
+	.clk_divider = true,
 	.clk_gate = true,
 	.setup = lpss_uart_setup,
 };
@@ -125,31 +136,21 @@ static struct lpss_device_desc byt_pwm_dev_desc = {
 	.shared_clock = &pwm_clock,
 };
 
-static struct lpss_shared_clock uart_clock = {
-	.name = "uart_clk",
-	.rate = 44236800,
-};
-
 static struct lpss_device_desc byt_uart_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x800,
+	.clk_divider = true,
 	.clk_gate = true,
 	.save_ctx = true,
-	.shared_clock = &uart_clock,
 	.setup = lpss_uart_setup,
 };
 
-static struct lpss_shared_clock spi_clock = {
-	.name = "spi_clk",
-	.rate = 50000000,
-};
-
 static struct lpss_device_desc byt_spi_dev_desc = {
 	.clk_required = true,
 	.prv_offset = 0x400,
+	.clk_divider = true,
 	.clk_gate = true,
 	.save_ctx = true,
-	.shared_clock = &spi_clock,
 };
 
 static struct lpss_device_desc byt_sdio_dev_desc = {
@@ -175,8 +176,8 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = {
 	/* Lynxpoint LPSS devices */
 	{ "INT33C0", (unsigned long)&lpt_dev_desc },
 	{ "INT33C1", (unsigned long)&lpt_dev_desc },
-	{ "INT33C2", (unsigned long)&lpt_dev_desc },
-	{ "INT33C3", (unsigned long)&lpt_dev_desc },
+	{ "INT33C2", (unsigned long)&lpt_i2c_dev_desc },
+	{ "INT33C3", (unsigned long)&lpt_i2c_dev_desc },
 	{ "INT33C4", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT33C5", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT33C6", (unsigned long)&lpt_sdio_dev_desc },
@@ -193,8 +194,8 @@ static const struct acpi_device_id acpi_lpss_device_ids[] = {
 
 	{ "INT3430", (unsigned long)&lpt_dev_desc },
 	{ "INT3431", (unsigned long)&lpt_dev_desc },
-	{ "INT3432", (unsigned long)&lpt_dev_desc },
-	{ "INT3433", (unsigned long)&lpt_dev_desc },
+	{ "INT3432", (unsigned long)&lpt_i2c_dev_desc },
+	{ "INT3433", (unsigned long)&lpt_i2c_dev_desc },
 	{ "INT3434", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT3435", (unsigned long)&lpt_uart_dev_desc },
 	{ "INT3436", (unsigned long)&lpt_sdio_dev_desc },
@@ -222,9 +223,11 @@ static int register_device_clock(struct acpi_device *adev,
 {
 	const struct lpss_device_desc *dev_desc = pdata->dev_desc;
 	struct lpss_shared_clock *shared_clock = dev_desc->shared_clock;
+	const char *devname = dev_name(&adev->dev);
 	struct clk *clk = ERR_PTR(-ENODEV);
 	struct lpss_clk_data *clk_data;
-	const char *parent;
+	const char *parent, *clk_name;
+	void __iomem *prv_base;
 
 	if (!lpss_clk_dev)
 		lpt_register_clock_device();
@@ -235,7 +238,7 @@ static int register_device_clock(struct acpi_device *adev,
 
 	if (dev_desc->clkdev_name) {
 		clk_register_clkdev(clk_data->clk, dev_desc->clkdev_name,
-				    dev_name(&adev->dev));
+				    devname);
 		return 0;
 	}
 
@@ -244,6 +247,7 @@ static int register_device_clock(struct acpi_device *adev,
 		return -ENODATA;
 
 	parent = clk_data->name;
+	prv_base = pdata->mmio_base + dev_desc->prv_offset;
 
 	if (shared_clock) {
 		clk = shared_clock->clk;
@@ -257,16 +261,41 @@ static int register_device_clock(struct acpi_device *adev,
 	}
 
 	if (dev_desc->clk_gate) {
-		clk = clk_register_gate(NULL, dev_name(&adev->dev), parent, 0,
-					pdata->mmio_base + dev_desc->prv_offset,
-					0, 0, NULL);
-		pdata->clk = clk;
+		clk = clk_register_gate(NULL, devname, parent, 0,
+					prv_base, 0, 0, NULL);
+		parent = devname;
+	}
+
+	if (dev_desc->clk_divider) {
+		/* Prevent division by zero */
+		if (!readl(prv_base))
+			writel(LPSS_CLK_DIVIDER_DEF_MASK, prv_base);
+
+		clk_name = kasprintf(GFP_KERNEL, "%s-div", devname);
+		if (!clk_name)
+			return -ENOMEM;
+		clk = clk_register_fractional_divider(NULL, clk_name, parent,
+						      0, prv_base,
+						      1, 15, 16, 15, 0, NULL);
+		parent = clk_name;
+
+		clk_name = kasprintf(GFP_KERNEL, "%s-update", devname);
+		if (!clk_name) {
+			kfree(parent);
+			return -ENOMEM;
+		}
+		clk = clk_register_gate(NULL, clk_name, parent,
+					CLK_SET_RATE_PARENT | CLK_SET_RATE_GATE,
+					prv_base, 31, 0, NULL);
+		kfree(parent);
+		kfree(clk_name);
 	}
 
 	if (IS_ERR(clk))
 		return PTR_ERR(clk);
 
-	clk_register_clkdev(clk, NULL, dev_name(&adev->dev));
+	pdata->clk = clk;
+	clk_register_clkdev(clk, NULL, devname);
 	return 0;
 }
 
-- 
2.0.0.rc2

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/15 21:40, Heikki Krogerus wrote:
> Changes since v1:
> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
> - NULL checks for clk_name allocation in acpi_lpss.c
> 
> This combines two patch sets for LPSS that I had already send for
> review separately. They conflicted with each other.
> 
> The first two patches will fix a problem were the context of the
> private LPSS registers is lost when entering D3. The last two will add
> support for the M/N dividers on LPSS by adding a new basic clock type
> for fractional dividers. The UART driver needs support for it in order
> to get clock rates that suit the requested baud rates.

The major issue in my mind is, this proposal makes a couple between I2C
designware, HSUART, or probably DMA driver as well with LPSS driver.
That is, currently without LPSS driver, I2C/HSUART won't work properly.
So we at least need to describe this dependency in Kconfig.

Thanks,
-Aubrey
> 
> 
> Heikki Krogerus (4):
>   ACPI / PM: Export rest of the subsys functions
>   ACPI / LPSS: custom power domain for LPSS
>   clk: new basic clk type for fractional divider
>   ACPI / LPSS: support for fractional divider clock
> 
>  drivers/acpi/acpi_lpss.c             | 204 ++++++++++++++++++++++++++++++-----
>  drivers/acpi/device_pm.c             |   2 +
>  drivers/clk/Makefile                 |   1 +
>  drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++
>  include/linux/acpi.h                 |   4 +
>  include/linux/clk-provider.h         |  31 ++++++
>  6 files changed, 349 insertions(+), 28 deletions(-)
>  create mode 100644 drivers/clk/clk-fractional-divider.c
> 

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Andy Shevchenko]

On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
> On 2014/5/15 21:40, Heikki Krogerus wrote:
> > Changes since v1:
> > - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
> > - NULL checks for clk_name allocation in acpi_lpss.c
> > 
> > This combines two patch sets for LPSS that I had already send for
> > review separately. They conflicted with each other.
> > 
> > The first two patches will fix a problem were the context of the
> > private LPSS registers is lost when entering D3. The last two will add
> > support for the M/N dividers on LPSS by adding a new basic clock type
> > for fractional dividers. The UART driver needs support for it in order
> > to get clock rates that suit the requested baud rates.
> 
> The major issue in my mind is, this proposal makes a couple between I2C
> designware, HSUART, or probably DMA driver as well with LPSS driver.

acpi_lpss driver creates platform devices for each found and enumerated
device. If there no acpi_lpss enabled the drivers work as supposed
without it. I didn't see any contradiction here.

> That is, currently without LPSS driver, I2C/HSUART won't work properly.
> So we at least need to describe this dependency in Kconfig.
> 
> Thanks,
> -Aubrey
> > 
> > 
> > Heikki Krogerus (4):
> >   ACPI / PM: Export rest of the subsys functions
> >   ACPI / LPSS: custom power domain for LPSS
> >   clk: new basic clk type for fractional divider
> >   ACPI / LPSS: support for fractional divider clock
> > 
> >  drivers/acpi/acpi_lpss.c             | 204 ++++++++++++++++++++++++++++++-----
> >  drivers/acpi/device_pm.c             |   2 +
> >  drivers/clk/Makefile                 |   1 +
> >  drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++
> >  include/linux/acpi.h                 |   4 +
> >  include/linux/clk-provider.h         |  31 ++++++
> >  6 files changed, 349 insertions(+), 28 deletions(-)
> >  create mode 100644 drivers/clk/clk-fractional-divider.c
> > 
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/15 22:53, Andy Shevchenko wrote:
> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
>> On 2014/5/15 21:40, Heikki Krogerus wrote:
>>> Changes since v1:
>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
>>> - NULL checks for clk_name allocation in acpi_lpss.c
>>>
>>> This combines two patch sets for LPSS that I had already send for
>>> review separately. They conflicted with each other.
>>>
>>> The first two patches will fix a problem were the context of the
>>> private LPSS registers is lost when entering D3. The last two will add
>>> support for the M/N dividers on LPSS by adding a new basic clock type
>>> for fractional dividers. The UART driver needs support for it in order
>>> to get clock rates that suit the requested baud rates.
>>
>> The major issue in my mind is, this proposal makes a couple between I2C
>> designware, HSUART, or probably DMA driver as well with LPSS driver.
> 
> acpi_lpss driver creates platform devices for each found and enumerated
> device.

> If there no acpi_lpss enabled the drivers work as supposed without it.

This is not true.

Thanks,
-Aubrey


> I didn't see any contradiction here.
> 
>> That is, currently without LPSS driver, I2C/HSUART won't work properly.
>> So we at least need to describe this dependency in Kconfig.
>>
>> Thanks,
>> -Aubrey
>>>
>>>
>>> Heikki Krogerus (4):
>>>   ACPI / PM: Export rest of the subsys functions
>>>   ACPI / LPSS: custom power domain for LPSS
>>>   clk: new basic clk type for fractional divider
>>>   ACPI / LPSS: support for fractional divider clock
>>>
>>>  drivers/acpi/acpi_lpss.c             | 204 ++++++++++++++++++++++++++++++-----
>>>  drivers/acpi/device_pm.c             |   2 +
>>>  drivers/clk/Makefile                 |   1 +
>>>  drivers/clk/clk-fractional-divider.c | 135 +++++++++++++++++++++++
>>>  include/linux/acpi.h                 |   4 +
>>>  include/linux/clk-provider.h         |  31 ++++++
>>>  6 files changed, 349 insertions(+), 28 deletions(-)
>>>  create mode 100644 drivers/clk/clk-fractional-divider.c
>>>
>>
> 
> 

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Mika Westerberg]

On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
> On 2014/5/15 22:53, Andy Shevchenko wrote:
> > On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
> >> On 2014/5/15 21:40, Heikki Krogerus wrote:
> >>> Changes since v1:
> >>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
> >>> - NULL checks for clk_name allocation in acpi_lpss.c
> >>>
> >>> This combines two patch sets for LPSS that I had already send for
> >>> review separately. They conflicted with each other.
> >>>
> >>> The first two patches will fix a problem were the context of the
> >>> private LPSS registers is lost when entering D3. The last two will add
> >>> support for the M/N dividers on LPSS by adding a new basic clock type
> >>> for fractional dividers. The UART driver needs support for it in order
> >>> to get clock rates that suit the requested baud rates.
> >>
> >> The major issue in my mind is, this proposal makes a couple between I2C
> >> designware, HSUART, or probably DMA driver as well with LPSS driver.
> > 
> > acpi_lpss driver creates platform devices for each found and enumerated
> > device.
> 
> > If there no acpi_lpss enabled the drivers work as supposed without it.
> 
> This is not true.

The drivers work fine on non-LPSS platform. If you make them depend on
acpi_lpss, you break that.

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/16 0:11, Mika Westerberg wrote:
> On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
>> On 2014/5/15 22:53, Andy Shevchenko wrote:
>>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
>>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
>>>>> Changes since v1:
>>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
>>>>> - NULL checks for clk_name allocation in acpi_lpss.c
>>>>>
>>>>> This combines two patch sets for LPSS that I had already send for
>>>>> review separately. They conflicted with each other.
>>>>>
>>>>> The first two patches will fix a problem were the context of the
>>>>> private LPSS registers is lost when entering D3. The last two will add
>>>>> support for the M/N dividers on LPSS by adding a new basic clock type
>>>>> for fractional dividers. The UART driver needs support for it in order
>>>>> to get clock rates that suit the requested baud rates.
>>>>
>>>> The major issue in my mind is, this proposal makes a couple between I2C
>>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
>>>
>>> acpi_lpss driver creates platform devices for each found and enumerated
>>> device.
>>
>>> If there no acpi_lpss enabled the drivers work as supposed without it.
>>
>> This is not true.
> 
> The drivers work fine on non-LPSS platform. If you make them depend on
> acpi_lpss, you break that.
> 

People don't know the relationship between LPSS driver and I2C/HSUART,
there is nowhere to describe that. If LPSS driver is unchecked, they
will encounter a weird issue which is very hard to figure out what's
going on.

Thanks,
-Aubrey
> 

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Andy Shevchenko]

On Fri, 2014-05-16 at 07:29 +0800, Li, Aubrey wrote:
> On 2014/5/16 0:11, Mika Westerberg wrote:
> > On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
> >> On 2014/5/15 22:53, Andy Shevchenko wrote:
> >>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
> >>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
> >>>>> Changes since v1:
> >>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
> >>>>> - NULL checks for clk_name allocation in acpi_lpss.c
> >>>>>
> >>>>> This combines two patch sets for LPSS that I had already send for
> >>>>> review separately. They conflicted with each other.
> >>>>>
> >>>>> The first two patches will fix a problem were the context of the
> >>>>> private LPSS registers is lost when entering D3. The last two will add
> >>>>> support for the M/N dividers on LPSS by adding a new basic clock type
> >>>>> for fractional dividers. The UART driver needs support for it in order
> >>>>> to get clock rates that suit the requested baud rates.
> >>>>
> >>>> The major issue in my mind is, this proposal makes a couple between I2C
> >>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
> >>>
> >>> acpi_lpss driver creates platform devices for each found and enumerated
> >>> device.
> >>
> >>> If there no acpi_lpss enabled the drivers work as supposed without it.
> >>
> >> This is not true.
> > 
> > The drivers work fine on non-LPSS platform. If you make them depend on
> > acpi_lpss, you break that.
> > 
> 
> People don't know the relationship between LPSS driver and I2C/HSUART,
> there is nowhere to describe that. If LPSS driver is unchecked, they
> will encounter a weird issue which is very hard to figure out what's
> going on.

Besides this discussion is off the topic, I could say that LPSS drivers
are kinda optional (we won't enforce user to use them) even on some
systems where they are present. Relationship is provided by the proper
kernel configuration.

> 
> Thanks,
> -Aubrey
> > 
> 


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/16 15:04, Andy Shevchenko wrote:
> On Fri, 2014-05-16 at 07:29 +0800, Li, Aubrey wrote:
>> On 2014/5/16 0:11, Mika Westerberg wrote:
>>> On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
>>>> On 2014/5/15 22:53, Andy Shevchenko wrote:
>>>>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
>>>>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
>>>>>>> Changes since v1:
>>>>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
>>>>>>> - NULL checks for clk_name allocation in acpi_lpss.c
>>>>>>>
>>>>>>> This combines two patch sets for LPSS that I had already send for
>>>>>>> review separately. They conflicted with each other.
>>>>>>>
>>>>>>> The first two patches will fix a problem were the context of the
>>>>>>> private LPSS registers is lost when entering D3. The last two will add
>>>>>>> support for the M/N dividers on LPSS by adding a new basic clock type
>>>>>>> for fractional dividers. The UART driver needs support for it in order
>>>>>>> to get clock rates that suit the requested baud rates.
>>>>>>
>>>>>> The major issue in my mind is, this proposal makes a couple between I2C
>>>>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
>>>>>
>>>>> acpi_lpss driver creates platform devices for each found and enumerated
>>>>> device.
>>>>
>>>>> If there no acpi_lpss enabled the drivers work as supposed without it.
>>>>
>>>> This is not true.
>>>
>>> The drivers work fine on non-LPSS platform. If you make them depend on
>>> acpi_lpss, you break that.
>>>
>>
>> People don't know the relationship between LPSS driver and I2C/HSUART,
>> there is nowhere to describe that. If LPSS driver is unchecked, they
>> will encounter a weird issue which is very hard to figure out what's
>> going on.
> 
> Besides this discussion is off the topic, I could say that LPSS drivers
> are kinda optional (we won't enforce user to use them) even on some
> systems where they are present. Relationship is provided by the proper
> kernel configuration.
> 
It is optional previously but definitely not optional after this patch.
The user who uses I2C designeware driver without LPSS now, I2C won't
work properly on Asus T100.

The proper configuration leads to a question, why a completed I2C
controller driver doesn't work properly without another LPSS driver. I
worry about this is hard to maintain in future.

Do we have a platform configuration to specify LPSS is needed?

BTW, do we have a system with I2C and HSUART but without LPSS?

Thanks,
-Aubrey

Thanks,
-Aubrey

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Andy Shevchenko]

On Fri, 2014-05-16 at 21:37 +0800, Li, Aubrey wrote:
> On 2014/5/16 15:04, Andy Shevchenko wrote:
> > On Fri, 2014-05-16 at 07:29 +0800, Li, Aubrey wrote:
> >> On 2014/5/16 0:11, Mika Westerberg wrote:
> >>> On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
> >>>> On 2014/5/15 22:53, Andy Shevchenko wrote:
> >>>>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
> >>>>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
> >>>>>>> Changes since v1:
> >>>>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
> >>>>>>> - NULL checks for clk_name allocation in acpi_lpss.c
> >>>>>>>
> >>>>>>> This combines two patch sets for LPSS that I had already send for
> >>>>>>> review separately. They conflicted with each other.
> >>>>>>>
> >>>>>>> The first two patches will fix a problem were the context of the
> >>>>>>> private LPSS registers is lost when entering D3. The last two will add
> >>>>>>> support for the M/N dividers on LPSS by adding a new basic clock type
> >>>>>>> for fractional dividers. The UART driver needs support for it in order
> >>>>>>> to get clock rates that suit the requested baud rates.
> >>>>>>
> >>>>>> The major issue in my mind is, this proposal makes a couple between I2C
> >>>>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
> >>>>>
> >>>>> acpi_lpss driver creates platform devices for each found and enumerated
> >>>>> device.
> >>>>
> >>>>> If there no acpi_lpss enabled the drivers work as supposed without it.
> >>>>
> >>>> This is not true.
> >>>
> >>> The drivers work fine on non-LPSS platform. If you make them depend on
> >>> acpi_lpss, you break that.
> >>>
> >>
> >> People don't know the relationship between LPSS driver and I2C/HSUART,
> >> there is nowhere to describe that. If LPSS driver is unchecked, they
> >> will encounter a weird issue which is very hard to figure out what's
> >> going on.
> > 
> > Besides this discussion is off the topic, I could say that LPSS drivers
> > are kinda optional (we won't enforce user to use them) even on some
> > systems where they are present. Relationship is provided by the proper
> > kernel configuration.
> > 
> It is optional previously but definitely not optional after this patch.

I'm sorry, I didn't clearly see what part in this patchset prevents
this.

> The user who uses I2C designeware driver without LPSS now, I2C won't
> work properly on Asus T100.

In that case you specifically enable this driver in the kernel
configuration. T100 may require that driver to be fully functional.

> 
> The proper configuration leads to a question, why a completed I2C
> controller driver doesn't work properly without another LPSS driver. I
> worry about this is hard to maintain in future.
> 
> Do we have a platform configuration to specify LPSS is needed?
> 
> BTW, do we have a system with I2C and HSUART but without LPSS?

I believe we have.


-- 
Andy Shevchenko <andriy.shevchenko@intel.com>
Intel Finland Oy

Re: [PATCHv2 0/4] ACPI / LPSS: Solution for two issues seen on Asus T100

[Li, Aubrey]

On 2014/5/16 22:45, Andy Shevchenko wrote:
> On Fri, 2014-05-16 at 21:37 +0800, Li, Aubrey wrote:
>> On 2014/5/16 15:04, Andy Shevchenko wrote:
>>> On Fri, 2014-05-16 at 07:29 +0800, Li, Aubrey wrote:
>>>> On 2014/5/16 0:11, Mika Westerberg wrote:
>>>>> On Thu, May 15, 2014 at 11:59:46PM +0800, Li, Aubrey wrote:
>>>>>> On 2014/5/15 22:53, Andy Shevchenko wrote:
>>>>>>> On Thu, 2014-05-15 at 22:35 +0800, Li, Aubrey wrote:
>>>>>>>> On 2014/5/15 21:40, Heikki Krogerus wrote:
>>>>>>>>> Changes since v1:
>>>>>>>>> - now using do_div() in clk_fd_recalc_rate() as suggested by Andy
>>>>>>>>> - NULL checks for clk_name allocation in acpi_lpss.c
>>>>>>>>>
>>>>>>>>> This combines two patch sets for LPSS that I had already send for
>>>>>>>>> review separately. They conflicted with each other.
>>>>>>>>>
>>>>>>>>> The first two patches will fix a problem were the context of the
>>>>>>>>> private LPSS registers is lost when entering D3. The last two will add
>>>>>>>>> support for the M/N dividers on LPSS by adding a new basic clock type
>>>>>>>>> for fractional dividers. The UART driver needs support for it in order
>>>>>>>>> to get clock rates that suit the requested baud rates.
>>>>>>>>
>>>>>>>> The major issue in my mind is, this proposal makes a couple between I2C
>>>>>>>> designware, HSUART, or probably DMA driver as well with LPSS driver.
>>>>>>>
>>>>>>> acpi_lpss driver creates platform devices for each found and enumerated
>>>>>>> device.
>>>>>>
>>>>>>> If there no acpi_lpss enabled the drivers work as supposed without it.
>>>>>>
>>>>>> This is not true.
>>>>>
>>>>> The drivers work fine on non-LPSS platform. If you make them depend on
>>>>> acpi_lpss, you break that.
>>>>>
>>>>
>>>> People don't know the relationship between LPSS driver and I2C/HSUART,
>>>> there is nowhere to describe that. If LPSS driver is unchecked, they
>>>> will encounter a weird issue which is very hard to figure out what's
>>>> going on.
>>>
>>> Besides this discussion is off the topic, I could say that LPSS drivers
>>> are kinda optional (we won't enforce user to use them) even on some
>>> systems where they are present. Relationship is provided by the proper
>>> kernel configuration.
>>>
>> It is optional previously but definitely not optional after this patch.
> 
> I'm sorry, I didn't clearly see what part in this patchset prevents
> this.

That's exactly what I worry about. It's very hard to figure it out.

> 
>> The user who uses I2C designeware driver without LPSS now, I2C won't
>> work properly on Asus T100.
> 
> In that case you specifically enable this driver in the kernel
> configuration. T100 may require that driver to be fully functional.

Is it better to rephrase the help information to give some hints to the
user?

Thanks,
-Aubrey

> 
>>
>> The proper configuration leads to a question, why a completed I2C
>> controller driver doesn't work properly without another LPSS driver. I
>> worry about this is hard to maintain in future.
>>
>> Do we have a platform configuration to specify LPSS is needed?
>>
>> BTW, do we have a system with I2C and HSUART but without LPSS?
> 
> I believe we have.
> 
>