Date:	Thu, 27 Nov 2014 13:46:37 +0200
From:	Mika Westerberg <mika.westerberg@...ux.intel.com>
To:	Ken Xue <Ken.Xue@....com>
Cc:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	"lenb@...nel.org" <lenb@...nel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Wu, Jeff" <Jeff.Wu@....com>,
	Andriy Shevchenko <andriy.shevchenko@...el.com>
Subject: Re: [PATCH] acpi:apd:add AMD ACPI2Platform device support for x86
 system.

On Wed, Nov 26, 2014 at 06:31:38PM +0800, Ken Xue wrote:
> On 一, 2014-11-24 at 02:47 +0100, Rafael J. Wysocki wrote:
> > On Monday, November 24, 2014 01:02:30 AM Xue, Ken wrote:
> > > 
> > > On Tuesday, November 18, 2014 01:58:11 PM Ken Xue wrote:
> > > > This new feature is to interpret AMD specific ACPI device to platform 
> > > > device such as I2C, UART found on AMD CZ and later chipsets. It is 
> > > > based on example INTEL LPSS. Now, it can support AMD I2C & UART.
> > > > 
> > > > Signed-off-by: Ken Xue <Ken.Xue@....com>
> > > > Signed-off-by: Jeff Wu <Jeff.Wu@....com>
> > > 
> > > Generally speaking, this seems to duplicate much code from acpi_lpss which should be re-used instead.  What about moving the code that will be common between acpi_lpss and the new driver into a new file (say acpi_soc.c)?
> > > 
> > > Also, you need to avoid automatic creation of platform devices when !X86_AMD_PLATFORM_DEVICE in analogy with what acpi_lpss does, or bad things will happen.
> > > 
> > > [ken] sounds fair enough.  Let me take action to merge drivers to acpi_soc.c ? or you have other plan?
> > 
> > I'd prefer the common code to reside in one file (or one .c file and one header
> > file), and the driver-specific code to stay in separate per-driver files.
> > 
> [Ken] I wrote a proto type for acpi_soc.c. please help to confirm if it
> can match your ideal. if yes, i will submit a new patch after do more
> test and refine codes. I think it will impact lpss driver greatly, even
> i have taken it into account. below codes is for acpi_soc.c.

In general looks good. I have few comments though.

> 
> >From fc323fb7b3b4cbb79bda05ce3b1d6d8dfe5e883b Mon Sep 17 00:00:00 2001
> From: Ken Xue <Ken.Xue@....com>
> Date: Wed, 26 Nov 2014 17:15:30 +0800
> Subject: [PATCH] This is proto type for acpi_soc.
> 
> Signed-off-by: Ken Xue <Ken.Xue@....com>
> ---
>  arch/x86/Kconfig        |  11 +++
>  drivers/acpi/Makefile   |   2 +-
>  drivers/acpi/acpi_apd.c | 123 ++++++++++++++++++++++++++++
>  drivers/acpi/acpi_soc.c | 213
> ++++++++++++++++++++++++++++++++++++++++++++++++

This is line-wrapped, please make sure when you submit the actual patch
that it is formatted properly. Also you need proper changelog etc.

>  drivers/acpi/acpi_soc.h |  91 +++++++++++++++++++++
>  drivers/acpi/internal.h |   6 ++
>  drivers/acpi/scan.c     |   1 +
>  7 files changed, 446 insertions(+), 1 deletion(-)
>  create mode 100755 drivers/acpi/acpi_apd.c
>  create mode 100755 drivers/acpi/acpi_soc.c
>  create mode 100755 drivers/acpi/acpi_soc.h
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index ded8a67..6402c79f 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -495,6 +495,17 @@ config X86_INTEL_LPSS
>  	  things like clock tree (common clock framework) and pincontrol
>  	  which are needed by the LPSS peripheral drivers.
>  
> +config X86_AMD_PLATFORM_DEVICE
> +	bool "AMD ACPI2Platform devices support"
> +	depends on ACPI
> +	select COMMON_CLK
> +	select PINCTRL
> +	---help---
> +	  Select to interpret AMD specific ACPI device to platform device
> +	  such as I2C, UART found on AMD CARRIZO and later chipset. Selecting
> +	  this option enables things like clock tree (common clock framework)
> +	  and pinctrl.
> +
>  config IOSF_MBI
>  	tristate "Intel SoC IOSF Sideband support for SoC platforms"
>  	depends on PCI
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index c3b2fcb..b07003a 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -40,7 +40,7 @@ acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) +=
> processor_pdc.o
>  acpi-y				+= ec.o
>  acpi-$(CONFIG_ACPI_DOCK)	+= dock.o
>  acpi-y				+= pci_root.o pci_link.o pci_irq.o
> -acpi-y				+= acpi_lpss.o
> +acpi-y				+= acpi_soc.o acpi_lpss.o acpi_apd.o
>  acpi-y				+= acpi_platform.o
>  acpi-y				+= acpi_pnp.o
>  acpi-y				+= int340x_thermal.o
> diff --git a/drivers/acpi/acpi_apd.c b/drivers/acpi/acpi_apd.c
> new file mode 100755
> index 0000000..c7e916b
> --- /dev/null
> +++ b/drivers/acpi/acpi_apd.c
> @@ -0,0 +1,123 @@
> +/*
> + * AMD ACPI support for  ACPI2platform device.
> + *
> + * Copyright (c) 2014, AMD Corporation.
> + * Authors: Ken Xue <Ken.Xue@....com>
> + *	Wu, Jeff <Jeff.Wu@....com>
> + *
> + * 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.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/clk-provider.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +#include "internal.h"
> +#include "acpi_soc.h"
> +
> +struct acpi_soc asoc;

static?

Also Is "asoc" good name? That might get confused with Alsa SoC (ASoC).

> +
> +#ifdef X86_AMD_PLATFORM_DEVICE
> +static int acpi_apd_setup(struct acpi_soc_dev_private_data *pdata)
> +{
> +	struct acpi_soc_dev_desc *dev_desc = pdata->dev_desc;
> +	struct clk *clk = ERR_PTR(-ENODEV);
> +
> +	if (dev_desc->clk)
> +		return 0;
> +	
> +	if (dev_desc->fixed_clk_rate) {
> +		clk = clk_register_fixed_rate(&pdata->adev->dev, 
> +					dev_name(&pdata->adev->dev),
> +					NULL, CLK_IS_ROOT, dev_desc->rate);
> +		dev_desc->clk = clk;
> +		clk_register_clkdev(clk, NULL, dev_name(&pdata->adev->dev));
> +	}
> +
> +	return 0;
> +}
> +
> +static struct acpi_soc_dev_desc cz_i2c_desc = {
> +	.setup = acpi_apd_setup;
> +	.fixed_clk_rate = 133000000,

Oh, good so now we can get rid the hack we did for
i2c-designware-platdrv.c with this commit:

a445900c906092f i2c: designware: Add support for AMD I2C controller

Since now you have means to pass clock to the driver.

Are you going to handle that driver as well?

> +};
> +
> +static struct acpi_soc_dev_desc cz_uart_desc = {
> +	.setup = acpi_apd_setup;
> +	.fixed_clk_rate = 48000000,
> +};
> +
> +#else
> +
> +#define APD_ADDR(desc) (0UL)
> +
> +#endif /* CONFIG_X86_INTEL_LPSS */
> +
> +static struct acpi_device_id acpi_apd_device_ids[] = {

const?

> +	/* Generic apd devices */
> +	{ "AMD0010", APD_ADDR(cz_i2c_desc) },
> +	{ "AMD0020", APD_ADDR(cz_uart_desc) },
> +	{ }
> +};
> +
> +#ifdef X86_AMD_PLATFORM_DEVICE
> +
> +static ssize_t apd_device_desc_show(struct device *dev,
> +				struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	struct acpi_device *adev;
> +	struct acpi_soc_dev_private_data *pdata;
> +	struct acpi_soc_dev_desc *dev_desc;
> +
> +	ret = acpi_bus_get_device(ACPI_HANDLE(dev), &adev);
> +	if (WARN_ON(ret))
> +		return ret;
> +
> +	pdata = acpi_driver_data(adev);
> +	if (WARN_ON(!pdata || !pdata->dev_desc))
> +		return -ENODEV;
> +	
> +	dev_desc = pdata->dev_desc;
> +	if (dev_desc->fixed_clk_rate)
> +		return sprintf(buf, "Required fix rate clk %s: %ld\n",
> +			dev_desc->clk->name,
> +			dev_desc->fixed_clk_rate);
> +	else
> +		return sprintf(buf, "No need clk\n");
> +}
> +
> +static DEVICE_ATTR(device_desc, S_IRUSR, apd_device_desc_show, NULL);
> +
> +static struct attribute *apd_attrs[] = {
> +	&dev_attr_device_desc.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group apd_attr_group = {
> +	.attrs = apd_attrs,
> +	.name = "apd",
> +};

This requires updating sysfs ABI but then again, do you really need the
above? And does it belong to sysfs in the first place?

> +
> +void __init acpi_apd_init(void)
> +{
> +	asoc.ids = acpi_apd_device_ids;
> +	asoc.attr_group = &apd_attr_group;
> +	register_acpi_soc(&asoc, true);
> +}
> +
> +#else
> +
> +void __init acpi_apd_init(void)
> +{
> +	asoc.ids = acpi_apd_device_ids;
> +	register_acpi_soc(&asoc, false);
> +}
> +#endif
> diff --git a/drivers/acpi/acpi_soc.c b/drivers/acpi/acpi_soc.c
> new file mode 100755
> index 0000000..99df1ab
> --- /dev/null
> +++ b/drivers/acpi/acpi_soc.c
> @@ -0,0 +1,213 @@
> +/*
> + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> + *
> + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> + * Authors: Ken Xue <Ken.Xue@....com>
> + *		Mika Westerberg <mika.westerberg@...ux.intel.com>
> + *		Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> + *
> + * 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.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/delay.h>
> +#include <linux/pm_domain.h>
> +
> +#include "internal.h"
> +#include "acpi_soc.h"
> +
> +

Delete the extra blank line

> +ACPI_MODULE_NAME("acpi_soc");
> +
> +/*A list for all acpi soc device*/
> +static LIST_HEAD(asoc_list);
> +
> +static int is_memory(struct acpi_resource *res, void *not_used)
> +{
> +	struct resource r;
> +	return !acpi_dev_resource_memory(res, &r);
> +}
> +
> +static int acpi_soc_create_device(struct acpi_device *adev,
> +				   const struct acpi_device_id *id)
> +{
> +	struct acpi_soc_dev_desc *dev_desc;
> +	struct acpi_soc_dev_private_data *pdata;
> +	struct resource_list_entry *rentry;
> +	struct list_head resource_list;
> +	struct platform_device *pdev;
> +	int ret;
> +
> +	dev_desc = (struct acpi_soc_dev_desc *)id->driver_data;
> +	if (!dev_desc) {
> +		pdev = acpi_create_platform_device(adev);
> +		return IS_ERR_OR_NULL(pdev) ? PTR_ERR(pdev) : 1;
> +	}
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&resource_list);
> +	ret = acpi_dev_get_resources(adev, &resource_list, is_memory, NULL);
> +	if (ret < 0)
> +		goto err_out;
> +
> +	list_for_each_entry(rentry, &resource_list, node)
> +		if (resource_type(&rentry->res) == IORESOURCE_MEM) {
> +			if (dev_desc->mem_size_override)
> +				pdata->mmio_size = dev_desc->mem_size_override;
> +			else
> +				pdata->mmio_size = resource_size(&rentry->res);
> +			pdata->mmio_base = ioremap(rentry->res.start,
> +						   pdata->mmio_size);
> +			break;
> +		}
> +
> +	acpi_dev_free_resource_list(&resource_list);
> +
> +	pdata->dev_desc = dev_desc;
> +
> +	/*setup device by a hook routine*/

The comment should look like this

	/* Setup device by hook routine */

but I think it does not provide any new information so you can just drop
it.

> +	if (dev_desc->setup)
> +		dev_desc->setup(pdata);
> +
> +	/*
> +	 * This works around a known issue in ACPI tables where acpi soc
> devices
> +	 * have _PS0 and _PS3 without _PSC (and no power resources), so
> +	 * acpi_bus_init_power() will assume that the BIOS has put them into
> D0.
> +	 */
> +	ret = acpi_device_fix_up_power(adev);
> +	if (ret) {
> +		/* Skip the device, but continue the namespace scan. */
> +		ret = 0;
> +		goto err_out;
> +	}
> +
> +	adev->driver_data = pdata;
> +	pdev = acpi_create_platform_device(adev);
> +	if (!IS_ERR_OR_NULL(pdev)) {
> +		return 1;
> +	}
> +
> +	ret = PTR_ERR(pdev);
> +	adev->driver_data = NULL;
> +
> + err_out:
> +	kfree(pdata);
> +	return ret;
> +}
> +
> +static int acpi_soc_platform_notify(struct notifier_block *nb,
> +				     unsigned long action, void *data)
> +{
> +	struct platform_device *pdev = to_platform_device(data);
> +	struct acpi_soc_dev_private_data *pdata;
> +	struct acpi_device *adev;
> +	struct acpi_soc *asoc_entry;
> +	const struct acpi_device_id *id = NULL;
> +
> +	list_for_each_entry(asoc_entry, &asoc_list, list) {
> +		id = acpi_match_device(asoc_entry->ids, &pdev->dev);
> +		if (!id)
> +			break;
> +	}
> +	
> +	if (!id || !id->driver_data)
> +		return 0;
> +
> +	if (acpi_bus_get_device(ACPI_HANDLE(&pdev->dev), &adev))
> +		return 0;
> +
> +	pdata = acpi_driver_data(adev);
> +	if (!pdata || !pdata->mmio_base)
> +		return 0;
> +
> +	switch (action) {
> +	case BUS_NOTIFY_BOUND_DRIVER:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)){
> +			if (asoc_entry->pm_domain)
> +				pdev->dev.pm_domain = asoc_entry->pm_domain;
> +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> +					dev_pm_domain_attach(&pdev->dev, true);
> +			else
> +					dev_pm_domain_attach(&pdev->dev, false);
> +		}	
> +		break;
> +	case BUS_NOTIFY_UNBOUND_DRIVER:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_PM)){
> +			if (asoc_entry->pm_domain)
> +				pdev->dev.pm_domain = asoc_entry->pm_domain;
> +			else if (pdata->dev_desc->flags & ACPI_SOC_PM_ON)
> +					dev_pm_domain_detach(&pdev->dev, true);
> +			else
> +					dev_pm_domain_detach(&pdev->dev, false);
> +		}	
> +		break;
> +	case BUS_NOTIFY_ADD_DEVICE:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS) &&
> asoc_entry->pm_domain)
> +			return sysfs_create_group(&pdev->dev.kobj,
> +						  asoc_entry->attr_group);
> +	case BUS_NOTIFY_DEL_DEVICE:
> +		if ((pdata->dev_desc->flags & ACPI_SOC_SYSFS) &&
> asoc_entry->pm_domain)
> +			sysfs_remove_group(&pdev->dev.kobj, asoc_entry->attr_group);
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct notifier_block acpi_soc_nb = {
> +	.notifier_call = acpi_soc_platform_notify,
> +};
> +
> +static void acpi_soc_bind(struct device *dev)
> +{
> +	struct acpi_soc_dev_private_data *pdata;
> +
> +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +
> +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->bind)
> +		return;
> +
> +	pdata->dev_desc->bind(pdata);
> +	return;
> +}
> +
> +static void acpi_soc_unbind(struct device *dev)
> +{
> +	struct acpi_soc_dev_private_data *pdata;
> +
> +	pdata = acpi_driver_data(ACPI_COMPANION(dev));
> +
> +	if (!pdata || !pdata->dev_desc || !pdata->dev_desc->unbind)
> +		return;
> +
> +	pdata->dev_desc->unbind(pdata);
> +	return;
> +}
> +
> +void register_acpi_soc(struct acpi_soc *asoc, bool
> disable_scan_handler)
> +{
> +	struct acpi_scan_handler *acpi_soc_handler;
> +	
> +	INIT_LIST_HEAD(&asoc->list);
> +	list_add(&asoc->list, &asoc_list);
> +
> +	acpi_soc_handler = kzalloc(sizeof(struct acpi_scan_handler),
> GFP_KERNEL);
> +	acpi_soc_handler->ids = asoc->ids;
> +	if (!disable_scan_handler) {
> +		acpi_soc_handler->attach = acpi_soc_create_device;
> +		acpi_soc_handler->bind = acpi_soc_bind;
> +		acpi_soc_handler->unbind = acpi_soc_unbind;
> +	}	
> +	acpi_scan_add_handler(acpi_soc_handler);
> +	bus_register_notifier(&platform_bus_type, &acpi_soc_nb);
> +}
> +EXPORT_SYMBOL_GPL(register_acpi_soc);

I don't think we want to export these to modules.

> diff --git a/drivers/acpi/acpi_soc.h b/drivers/acpi/acpi_soc.h
> new file mode 100755
> index 0000000..96db30e
> --- /dev/null
> +++ b/drivers/acpi/acpi_soc.h

And all this belongs to drivers/acpi/internal.h.

> @@ -0,0 +1,91 @@
> +/*
> + * ACPI SOC support for Intel Lynxpoint LPSS and AMD APD.
> + *
> + * Copyright (C) 2015, Intel Corporation & AMD Corporation
> + * Authors: Ken Xue <Ken.Xue@....com>
> + *		Mika Westerberg <mika.westerberg@...ux.intel.com>
> + *		Rafael J. Wysocki <rafael.j.wysocki@...el.com>
> + *
> + * 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 _ACPI_SOC_H
> +#define _ACPI_SOC_H
> +
> +#include <linux/acpi.h>
> +#include <linux/clk.h>
> +#include <linux/pm.h>
> +
> +/* Flags */
> +#define ACPI_SOC_SYSFS	BIT(0)
> +#define ACPI_SOC_PM		BIT(1)
> +#define ACPI_SOC_PM_ON	BIT(2)
> +
> +struct acpi_soc_dev_private_data;
> +
> +/**
> + * struct acpi_soc - acpi soc 
> + * @list: list head
> + * @ids: all acpi device ids for acpi soc 
> + * @pm_domain: power domain for all acpi device;can be NULL
> + * @attr_group: attribute group for sysfs support of acpi soc;can be
> NULL
> + */
> +struct acpi_soc {
> +	struct list_head list;
> +	struct acpi_device_id	*ids;
> +	struct dev_pm_domain	*pm_domain;
> +	struct attribute_group 	*attr_group;
> +};
> +
> +/**
> + * struct acpi_soc_dev_desc - a descriptor for acpi device 
> + * @flags: some device feature flags
> + * @clk: clock device
> + * @fixed_clk_rate: fixed rate input clock source for acpi device;
> + *				0 means no fixed rate input clock source
> + * @mem_size_override: a workaround for override device memsize;
> + *				0 means no needs for this WA
> + * @setup: a hook routine to set device resource during create platform
> device
> + * @bind: a hook of acpi_scan_handler.bind
> + * @unbind: a hook of acpi_scan_handler.unbind
> + *
> + *device description defined as acpi_device_id.driver_data
> + */
> +struct acpi_soc_dev_desc {
> +	unsigned int flags;
> +	struct clk *clk;
> +	unsigned int fixed_clk_rate;
> +	size_t mem_size_override;
> +	int (*setup)(struct acpi_soc_dev_private_data *pdata);
> +	void (*bind)(struct acpi_soc_dev_private_data *pdata);
> +	void (*unbind)(struct acpi_soc_dev_private_data *pdata);
> +};
> +
> +/**
> + * struct acpi_soc_dev_private_data - acpi device private data
> + * @mmio_base: virtual memory base addr of the device
> + * @mmio_size: device memory size
> + * @dev_desc: device description
> + * @adev: apci device
> + *

Delete the above line.

> + */
> +struct acpi_soc_dev_private_data {
> +	void __iomem *mmio_base;
> +	resource_size_t mmio_size;
> +	
> +	const struct acpi_soc_dev_desc *dev_desc;
> +	struct acpi_device *adev;
> +};
> +
> +/**
> + * register_acpi_soc - register a new acpi soc
> + * @asoc: acpi soc
> + * @disable_scan_handler: true means remove deafult scan handle
> + *					false means use deafult scan handle
> + * 
> + * register a new acpi soc into asoc_list and install deafult scan
> handle.
> + */
> +void register_acpi_soc(struct acpi_soc *asoc, bool
> disable_scan_handler);
> +
> +#endif
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 447f6d6..c8a0e8e 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -68,6 +68,12 @@ static inline void acpi_debugfs_init(void)
> { return; }
>  #endif
>  void acpi_lpss_init(void);
>  
> +#ifdef CONFIG_X86_AMD_PLATFORM_DEVICE
> +void acpi_apd_init(void);
> +#else
> +static inline void acpi_apd_init(void) {}
> +#endif
> +
>  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>  bool acpi_queue_hotplug_work(struct work_struct *work);
>  void acpi_device_hotplug(struct acpi_device *adev, u32 src);
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0476e90..24fef2b 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -2349,6 +2349,7 @@ int __init acpi_scan_init(void)
>  	acpi_pci_link_init();
>  	acpi_processor_init();
>  	acpi_lpss_init();
> +	acpi_apd_init();
>  	acpi_cmos_rtc_init();
>  	acpi_container_init();
>  	acpi_memory_hotplug_init();
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists