Re: [PATCH v3 8/8] mfd: Add support for Intel Sunrisepoint LPSS devices

[Lee Jones]

Mike,

Can you review the clock implementation please?  It looks fragile to
me as it relies heavily on device names constructed of MFD cell names
and IDA numbers cat'ed together!

> The new coming Intel platforms such as Skylake will contain Sunrisepoint PCH.
> The main difference to the previous platforms is that the LPSS devices are
> compound devices where usually main (SPI, HSUART, or I2C) and DMA IPs are
> present.
> 
> This patch brings the driver for such devices found on Sunrisepoint PCH.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> ---
>  drivers/mfd/Kconfig           |  23 ++
>  drivers/mfd/Makefile          |   3 +
>  drivers/mfd/intel-lpss-acpi.c |  84 +++++++
>  drivers/mfd/intel-lpss-pci.c  | 113 +++++++++
>  drivers/mfd/intel-lpss.c      | 524 ++++++++++++++++++++++++++++++++++++++++++
>  drivers/mfd/intel-lpss.h      |  62 +++++
>  6 files changed, 809 insertions(+)
>  create mode 100644 drivers/mfd/intel-lpss-acpi.c
>  create mode 100644 drivers/mfd/intel-lpss-pci.c
>  create mode 100644 drivers/mfd/intel-lpss.c
>  create mode 100644 drivers/mfd/intel-lpss.h

[...]

> +static void intel_lpss_unregister_clock_tree(struct clk *clk)
> +{
> +	struct clk *parent;
> +
> +	while (clk) {
> +		parent = clk_get_parent(clk);
> +		clk_unregister(clk);
> +		clk = parent;
> +	}
> +}
> +
> +static int intel_lpss_register_clock_divider(struct intel_lpss *lpss,
> +					     const char *devname,
> +					     struct clk **clk)
> +{
> +	char name[32];
> +	struct clk *tmp = *clk;
> +
> +	snprintf(name, sizeof(name), "%s-enable", devname);
> +	tmp = clk_register_gate(NULL, name, __clk_get_name(tmp), 0,
> +				lpss->priv, 0, 0, NULL);
> +	if (IS_ERR(tmp))
> +		return PTR_ERR(tmp);
> +
> +	snprintf(name, sizeof(name), "%s-div", devname);
> +	tmp = clk_register_fractional_divider(NULL, name, __clk_get_name(tmp),
> +					      0, lpss->priv, 1, 15, 16, 15, 0,
> +					      NULL);
> +	if (IS_ERR(tmp))
> +		return PTR_ERR(tmp);
> +	*clk = tmp;
> +
> +	snprintf(name, sizeof(name), "%s-update", devname);
> +	tmp = clk_register_gate(NULL, name, __clk_get_name(tmp),
> +				CLK_SET_RATE_PARENT, lpss->priv, 31, 0, NULL);
> +	if (IS_ERR(tmp))
> +		return PTR_ERR(tmp);
> +	*clk = tmp;
> +
> +	return 0;
> +}
> +
> +static int intel_lpss_register_clock(struct intel_lpss *lpss)
> +{
> +	const struct mfd_cell *cell = lpss->cell;
> +	struct clk *clk;
> +	char devname[24];
> +	int ret;
> +
> +	if (!lpss->info->clk_rate)
> +		return 0;
> +
> +	/* Root clock */
> +	clk = clk_register_fixed_rate(NULL, dev_name(lpss->dev), NULL,
> +				      CLK_IS_ROOT, lpss->info->clk_rate);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	snprintf(devname, sizeof(devname), "%s.%d", cell->name, lpss->devid);
> +
> +	/*
> +	 * Support for clock divider only if it has some preset value.
> +	 * Otherwise we assume that the divider is not used.
> +	 */
> +	if (lpss->type != LPSS_DEV_I2C) {
> +		ret = intel_lpss_register_clock_divider(lpss, devname, &clk);
> +		if (ret)
> +			goto err_clk_register;
> +	}
> +
> +	ret = -ENOMEM;
> +
> +	/* Clock for the host controller */
> +	lpss->clock = clkdev_create(clk, lpss->info->clk_con_id, "%s", devname);
> +	if (!lpss->clock)
> +		goto err_clk_register;
> +
> +	lpss->clk = clk;
> +
> +	return 0;
> +
> +err_clk_register:
> +	intel_lpss_unregister_clock_tree(clk);
> +
> +	return ret;
> +}
> +
> +static void intel_lpss_unregister_clock(struct intel_lpss *lpss)
> +{
> +	if (IS_ERR_OR_NULL(lpss->clk))
> +		return;
> +
> +	clkdev_drop(lpss->clock);
> +	intel_lpss_unregister_clock_tree(lpss->clk);
> +}
> +
> +int intel_lpss_probe(struct device *dev,
> +		     const struct intel_lpss_platform_info *info)
> +{
> +	struct intel_lpss *lpss;
> +	int ret;
> +
> +	if (!info || !info->mem || info->irq <= 0)
> +		return -EINVAL;
> +
> +	lpss = devm_kzalloc(dev, sizeof(*lpss), GFP_KERNEL);
> +	if (!lpss)
> +		return -ENOMEM;
> +
> +	lpss->priv = devm_ioremap(dev, info->mem->start + LPSS_PRIV_OFFSET,
> +				  LPSS_PRIV_SIZE);
> +	if (!lpss->priv)
> +		return -ENOMEM;
> +
> +	lpss->info = info;
> +	lpss->dev = dev;
> +	lpss->caps = readl(lpss->priv + LPSS_PRIV_CAPS);
> +
> +	dev_set_drvdata(dev, lpss);
> +
> +	ret = intel_lpss_assign_devs(lpss);
> +	if (ret)
> +		return ret;
> +
> +	intel_lpss_init_dev(lpss);
> +
> +	lpss->devid = ida_simple_get(&intel_lpss_devid_ida, 0, 0, GFP_KERNEL);
> +	if (lpss->devid < 0)
> +		return lpss->devid;
> +
> +	ret = intel_lpss_register_clock(lpss);
> +	if (ret)
> +		goto err_clk_register;
> +
> +	intel_lpss_ltr_expose(lpss);
> +
> +	ret = intel_lpss_debugfs_add(lpss);
> +	if (ret)
> +		dev_warn(dev, "Failed to create debugfs entries\n");
> +
> +	if (intel_lpss_has_idma(lpss)) {
> +		/*
> +		 * Ensure the DMA driver is loaded before the host
> +		 * controller device appears, so that the host controller
> +		 * driver can request its DMA channels as early as
> +		 * possible.
> +		 *
> +		 * If the DMA module is not there that's OK as well.
> +		 */
> +		intel_lpss_request_dma_module(LPSS_IDMA64_DRIVER_NAME);
> +
> +		ret = mfd_add_devices(dev, lpss->devid, &intel_lpss_idma64_cell,
> +				      1, info->mem, info->irq, NULL);
> +		if (ret)
> +			dev_warn(dev, "Failed to add %s, fallback to PIO\n",
> +				 LPSS_IDMA64_DRIVER_NAME);
> +	}
> +
> +	ret = mfd_add_devices(dev, lpss->devid, lpss->cell,
> +			      1, info->mem, info->irq, NULL);
> +	if (ret)
> +		goto err_remove_ltr;
> +
> +	return 0;
> +
> +err_remove_ltr:
> +	intel_lpss_debugfs_remove(lpss);
> +	intel_lpss_ltr_hide(lpss);
> +
> +err_clk_register:
> +	ida_simple_remove(&intel_lpss_devid_ida, lpss->devid);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_probe);
> +
> +void intel_lpss_remove(struct device *dev)
> +{
> +	struct intel_lpss *lpss = dev_get_drvdata(dev);
> +
> +	mfd_remove_devices(dev);
> +	intel_lpss_debugfs_remove(lpss);
> +	intel_lpss_ltr_hide(lpss);
> +	intel_lpss_unregister_clock(lpss);
> +	ida_simple_remove(&intel_lpss_devid_ida, lpss->devid);
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_remove);
> +
> +static int resume_lpss_device(struct device *dev, void *data)
> +{
> +	pm_runtime_resume(dev);
> +	return 0;
> +}
> +
> +int intel_lpss_prepare(struct device *dev)
> +{
> +	/*
> +	 * Resume both child devices before entering system sleep. This
> +	 * ensures that they are in proper state before they get suspended.
> +	 */
> +	device_for_each_child_reverse(dev, NULL, resume_lpss_device);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_prepare);
> +
> +int intel_lpss_suspend(struct device *dev)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_suspend);
> +
> +int intel_lpss_resume(struct device *dev)
> +{
> +	struct intel_lpss *lpss = dev_get_drvdata(dev);
> +
> +	intel_lpss_init_dev(lpss);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(intel_lpss_resume);
> +
> +static int __init intel_lpss_init(void)
> +{
> +	intel_lpss_debugfs = debugfs_create_dir("intel_lpss", NULL);
> +	return 0;
> +}
> +module_init(intel_lpss_init);
> +
> +static void __exit intel_lpss_exit(void)
> +{
> +	debugfs_remove(intel_lpss_debugfs);
> +}
> +module_exit(intel_lpss_exit);
> +
> +MODULE_AUTHOR("Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>");
> +MODULE_AUTHOR("Jarkko Nikula <jarkko.nikula@xxxxxxxxxxxxxxx>");
> +MODULE_DESCRIPTION("Intel LPSS core driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mfd/intel-lpss.h b/drivers/mfd/intel-lpss.h
> new file mode 100644
> index 0000000..f28cb28a
> --- /dev/null
> +++ b/drivers/mfd/intel-lpss.h
> @@ -0,0 +1,62 @@
> +/*
> + * Intel LPSS core support.
> + *
> + * Copyright (C) 2015, Intel Corporation
> + *
> + * Authors: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
> + *          Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> + *
> + * 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.
> + */
> +
> +#ifndef __MFD_INTEL_LPSS_H
> +#define __MFD_INTEL_LPSS_H
> +
> +struct device;
> +struct resource;
> +
> +struct intel_lpss_platform_info {
> +	struct resource *mem;
> +	int irq;
> +	unsigned long clk_rate;
> +	const char *clk_con_id;
> +};
> +
> +int intel_lpss_probe(struct device *dev,
> +		     const struct intel_lpss_platform_info *info);
> +void intel_lpss_remove(struct device *dev);
> +
> +#ifdef CONFIG_PM
> +int intel_lpss_prepare(struct device *dev);
> +int intel_lpss_suspend(struct device *dev);
> +int intel_lpss_resume(struct device *dev);
> +
> +#ifdef CONFIG_PM_SLEEP
> +#define INTEL_LPSS_SLEEP_PM_OPS			\
> +	.prepare = intel_lpss_prepare,		\
> +	.suspend = intel_lpss_suspend,		\
> +	.resume = intel_lpss_resume,		\
> +	.freeze = intel_lpss_suspend,		\
> +	.thaw = intel_lpss_resume,		\
> +	.poweroff = intel_lpss_suspend,		\
> +	.restore = intel_lpss_resume,
> +#endif
> +
> +#define INTEL_LPSS_RUNTIME_PM_OPS		\
> +	.runtime_suspend = intel_lpss_suspend,	\
> +	.runtime_resume = intel_lpss_resume,
> +
> +#else /* !CONFIG_PM */
> +#define INTEL_LPSS_SLEEP_PM_OPS
> +#define INTEL_LPSS_RUNTIME_PM_OPS
> +#endif /* CONFIG_PM */
> +
> +#define INTEL_LPSS_PM_OPS(name)			\
> +const struct dev_pm_ops name = {		\
> +	INTEL_LPSS_SLEEP_PM_OPS			\
> +	INTEL_LPSS_RUNTIME_PM_OPS		\
> +}
> +
> +#endif /* __MFD_INTEL_LPSS_H */

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/