Subject: [RFC PATCH 05/11] mfd: omap: control: core system control driver

From: [email-masked] (Cousson, Benoit)
Date: Tue, 29 May 2012 15:25:31 +0200

On 5/28/2012 1:35 PM, Eduardo Valentin wrote:
> Hello,
>
> On Fri, May 25, 2012 at 02:52:08PM +0200, Cousson Benoit wrote:
>> On 5/25/2012 10:25 AM, Eduardo Valentin wrote:
>>> This patch introduces a MFD core device driver for
>>> OMAP system control module.
>>>
>>> The control module allows software control of
>>> various static modes supported by the device. It is
>>> composed of two control submodules: general control
>>> module and device (padconfiguration) control
>>> module.
>>>
>>> In this patch, the children defined are for:
>>> . USB-phy pin control
>>> . Bangap temperature sensor
>>>
>>> Device driver is probed with postcore_initcall.
>>> However, as some of the APIs exposed by this driver
>>> may be needed in very early init phase, an early init
>>> class is also available: "early_omap_control".
>>>
>>> Signed-off-by: J Keerthy<[email-masked]>
>>> Signed-off-by: Kishon Vijay Abraham I<[email-masked]>
>>> Signed-off-by: Eduardo Valentin<[email-masked]>
>>> ---
>>>   .../devicetree/bindings/mfd/omap_control.txt       |   44 ++++
>>>   arch/arm/mach-omap2/Kconfig                        |    1 +
>>>   arch/arm/plat-omap/Kconfig                         |    3 +
>>>   drivers/mfd/Kconfig                                |    9 +
>>>   drivers/mfd/Makefile                               |    1 +
>>>   drivers/mfd/omap-control-core.c                    |  211 ++++++++++++++++++++
>>>   include/linux/mfd/omap_control.h                   |   69 +++++++
>>>   7 files changed, 338 insertions(+), 0 deletions(-)
>>>   create mode 100644 Documentation/devicetree/bindings/mfd/omap_control.txt
>>>   create mode 100644 drivers/mfd/omap-control-core.c
>>>   create mode 100644 include/linux/mfd/omap_control.h
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/omap_control.txt b/Documentation/devicetree/bindings/mfd/omap_control.txt
>>> new file mode 100644
>>> index 0000000..46d5e7e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/omap_control.txt
>>> @@ -0,0 +1,44 @@
>>> +* Texas Instrument OMAP System Control Module (SCM) bindings
>>> +
>>> +The control module allows software control of various static modes supported by
>>> +the device. The control module controls the settings of various device  modules
>>> +through register configuration and internal signals. It also controls  the  pad
>>> +configuration, pin functional multiplexing, and the routing of internal signals
>>> +(such as PRCM  signals or DMA requests)  to output pins configured for hardware
>>> +observability.
>>> +
>>> +Required properties:
>>> +- compatible : Should be:
>>> +  - "ti,omap3-control" for OMAP3 support
>>> +  - "ti,omap4-control" for OMAP4 support
>>> +  - "ti,omap5-control" for OMAP5 support
>>> +
>>> +OMAP specific properties:
>>> +- ti,hwmods: Name of the hwmod associated to the control module:
>>> +  Should be "ctrl_module_core";
>>> +
>>> +Sub-nodes:
>>> +- bandgap : contains the bandgap node
>>> +
>>> +  The bindings details of individual bandgap device can be found in:
>>> +  Documentation/devicetree/bindings/thermal/omap_bandgap.txt
>>> +
>>> +- usb : contains the usb phy pin control node
>>> +
>>> +  The only required property for this child is:
>>> +    - compatible = "ti,omap4-control-usb";
>>> +
>>> +Examples:
>>> +
>>> +ctrl_module_core: ctrl_module_core at 4a002000 {
>>> +	compatible = "ti,omap4-control";
>>> +	ti,hwmods = "ctrl_module_core";
>>> +	bandgap {
>>> +		compatible = "ti,omap4460-bandgap";
>>> +		interrupts =<0 126 4>; /* talert */
>>> +		ti,tshut-gpio =<86>; /* tshut */
>>> +	};
>>> +	usb {
>>> +		compatible = "ti,omap4-usb-phy";
>>> +	};
>>> +};
>>> diff --git a/arch/arm/mach-omap2/Kconfig b/arch/arm/mach-omap2/Kconfig
>>> index a2b946d..7dfe5e1 100644
>>> --- a/arch/arm/mach-omap2/Kconfig
>>> +++ b/arch/arm/mach-omap2/Kconfig
>>> @@ -52,6 +52,7 @@ config ARCH_OMAP4
>>>   	select PL310_ERRATA_727915
>>>   	select ARM_ERRATA_720789
>>>   	select ARCH_HAS_OPP
>>> +	select ARCH_HAS_CONTROL_MODULE
>>>   	select PM_OPP if PM
>>>   	select USB_ARCH_HAS_EHCI if USB_SUPPORT
>>>   	select ARM_CPU_SUSPEND if PM
>>> diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig
>>> index ad95c7a..222dbad 100644
>>> --- a/arch/arm/plat-omap/Kconfig
>>> +++ b/arch/arm/plat-omap/Kconfig
>>> @@ -5,6 +5,9 @@ menu "TI OMAP Common Features"
>>>   config ARCH_OMAP_OTG
>>>   	bool
>>>
>>> +config ARCH_HAS_CONTROL_MODULE
>>> +	bool
>>> +
>>>   choice
>>>   	prompt "OMAP System Type"
>>>   	default ARCH_OMAP2PLUS
>>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>>> index 11e4438..25a66d8 100644
>>> --- a/drivers/mfd/Kconfig
>>> +++ b/drivers/mfd/Kconfig
>>> @@ -795,6 +795,15 @@ config MFD_WL1273_CORE
>>>   	  driver connects the radio-wl1273 V4L2 module and the wl1273
>>>   	  audio codec.
>>>
>>> +config MFD_OMAP_CONTROL
>>> +	bool "Texas Instruments OMAP System control module"
>>> +	depends on ARCH_HAS_CONTROL_MODULE
>>> +	help
>>> +	  This is the core driver for system control module. This driver
>>> +	  is responsible for creating the control module mfd child,
>>> +	  like USB-pin control, pin muxing, MMC-pbias and DDR IO dynamic
>>> +	  change for off mode.
>>> +
>>>   config MFD_OMAP_USB_HOST
>>>   	bool "Support OMAP USBHS core driver"
>>>   	depends on USB_EHCI_HCD_OMAP || USB_OHCI_HCD_OMAP3
>>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>>> index 05fa538..00f99d6 100644
>>> --- a/drivers/mfd/Makefile
>>> +++ b/drivers/mfd/Makefile
>>> @@ -106,6 +106,7 @@ obj-$(CONFIG_MFD_TPS6586X)	+= tps6586x.o
>>>   obj-$(CONFIG_MFD_VX855)		+= vx855.o
>>>   obj-$(CONFIG_MFD_WL1273_CORE)	+= wl1273-core.o
>>>   obj-$(CONFIG_MFD_CS5535)	+= cs5535-mfd.o
>>> +obj-$(CONFIG_MFD_OMAP_CONTROL)	+= omap-control-core.o
>>>   obj-$(CONFIG_MFD_OMAP_USB_HOST)	+= omap-usb-host.o
>>>   obj-$(CONFIG_MFD_PM8921_CORE) 	+= pm8921-core.o
>>>   obj-$(CONFIG_MFD_PM8XXX_IRQ) 	+= pm8xxx-irq.o
>>> diff --git a/drivers/mfd/omap-control-core.c b/drivers/mfd/omap-control-core.c
>>> new file mode 100644
>>> index 0000000..7d8d408
>>> --- /dev/null
>>> +++ b/drivers/mfd/omap-control-core.c
>>> @@ -0,0 +1,211 @@
>>> +/*
>>> + * OMAP system control module driver file
>>> + *
>>> + * Copyright (C) 2011-2012 Texas Instruments Incorporated - http://www.ti.com/
>>> + * Contacts:
>>> + * Based on original code written by:
>>> + *    J Keerthy<[email-masked]>
>>> + *    Moiz Sonasath<[email-masked]>
>>> + * MFD clean up and re-factoring:
>>> + *    Eduardo Valentin<[email-masked]>
>>> + *
>>> + * 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.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#include<linux/module.h>
>>> +#include<linux/export.h>
>>> +#include<linux/platform_device.h>
>>> +#include<linux/slab.h>
>>> +#include<linux/io.h>
>>> +#include<linux/err.h>
>>> +#include<linux/of_platform.h>
>>> +#include<linux/of_address.h>
>>> +#include<linux/mfd/core.h>
>>> +#include<linux/mfd/omap_control.h>
>>> +
>>> +static struct omap_control *omap_control_module;
>>
>> Mmm, we can have up to 4 control module instances in OMAP4.
>>
>> Well, I'm not sure it worth considering them as separate devices. Is
>> that your plan as well?
>
> At least for now I was focusing on the ctrl_module_core ...

OK, that's a good start already :-)

>> But since they all have different base address, it will be trick to
>> handle them with only a single entry.
>
> Indeed. We can always add the support latter on.
>
> I am not sure what would be the best way to handle those instances though,
> and how they are going to expose APIs. Would need to have an instance bound
> to API set?

We should not go to that path I guess. We should have an API at children 
level independent of the underlying control module partitioning.

Regards,
Benoit

>>> +
>>> +/**
>>> + * omap_control_readl: Read a single omap control module register.
>>> + *
>>> + * @dev: device to read from.
>>> + * @reg: register to read.
>>> + * @val: output with register value.
>>> + *
>>> + * returns 0 on success or -EINVAL in case struct device is invalid.
>>> + */
>>> +int omap_control_readl(struct device *dev, u32 reg, u32 *val)
>>> +{
>>> +	struct omap_control *omap_control = dev_get_drvdata(dev);
>>> +
>>> +	if (!omap_control)
>>> +		return -EINVAL;
>>> +
>>> +	*val = readl(omap_control->base + reg);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(omap_control_readl);
>>> +
>>> +/**
>>> + * omap_control_writel: Write a single omap control module register.
>>> + *
>>> + * @dev: device to read from.
>>> + * @val: value to write.
>>> + * @reg: register to write to.
>>> + *
>>> + * returns 0 on success or -EINVAL in case struct device is invalid.
>>> + */
>>> +int omap_control_writel(struct device *dev, u32 val, u32 reg)
>>> +{
>>> +	struct omap_control *omap_control = dev_get_drvdata(dev);
>>> +	unsigned long flags;
>>> +
>>> +	if (!omap_control)
>>> +		return -EINVAL;
>>> +
>>> +	spin_lock_irqsave(&omap_control->reg_lock, flags);
>>> +	writel(val, omap_control->base + reg);
>>> +	spin_unlock_irqrestore(&omap_control->reg_lock, flags);
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(omap_control_writel);
>>> +
>>> +/**
>>> + * omap_control_get: returns the control module device pinter
>>
>> typo
>
> K
>
>>
>>> + *
>>> + * The modules which has to use control module API's to read or write should
>>> + * call this API to get the control module device pointer.
>>> + */
>>> +struct device *omap_control_get(void)
>>> +{
>>> +	unsigned long flags;
>>> +
>>> +	if (!omap_control_module)
>>> +		return ERR_PTR(-ENODEV);
>>> +
>>> +	spin_lock_irqsave(&omap_control_module->reg_lock, flags);
>>> +	omap_control_module->use_count++;
>>> +	spin_unlock_irqrestore(&omap_control_module->reg_lock, flags);
>>
>> Don't we do have some better way to increment atomically a variable
>> in Linux.
>
> Yeah we have, atomic API. In general I think the SCM/bangap/phy APIs
> need to be revisited WRT locking, in general.
>
>>
>>> +
>>> +	return omap_control_module->dev;
>>> +}
>>> +EXPORT_SYMBOL_GPL(omap_control_get);
>>> +
>>> +/**
>>> + * omap_control_put: returns the control module device pinter
>>> + *
>>> + * The modules which has to use control module API's to read or write should
>>> + * call this API to get the control module device pointer.
>>> + */
>>> +void omap_control_put(struct device *dev)
>>> +{
>>> +	struct omap_control *omap_control = dev_get_drvdata(dev);
>>> +	unsigned long flags;
>>> +
>>> +	if (!omap_control)
>>> +		return;
>>> +
>>> +	spin_lock_irqsave(&omap_control->reg_lock, flags);
>>> +	omap_control->use_count--;
>>> +	spin_unlock_irqrestore(&omap_control_module->reg_lock, flags);
>>> +}
>>> +EXPORT_SYMBOL_GPL(omap_control_put);
>>> +
>>> +static const struct of_device_id of_omap_control_match[] = {
>>> +	{ .compatible = "ti,omap3-control", },
>>> +	{ .compatible = "ti,omap4-control", },
>>> +	{ .compatible = "ti,omap5-control", },
>>> +	{ },
>>> +};
>>> +
>>> +static int __devinit omap_control_probe(struct platform_device *pdev)
>>> +{
>>> +	struct resource *res;
>>> +	void __iomem *base;
>>> +	struct device *dev =&pdev->dev;
>>> +	struct device_node *np = dev->of_node;
>>> +	struct omap_control *omap_control;
>>
>> Maybe omap_control_data instead? At least if this is drvdata only.
>> If this is supposed to be the *handle* to the control module
>> instance, it should be fine.
>
> That's suppose to be the phandle :-)
>
>>
>>> +
>>> +	omap_control = devm_kzalloc(dev, sizeof(*omap_control), GFP_KERNEL);
>>> +	if (!omap_control) {
>>> +		dev_err(dev, "not enough memory for omap_control\n");
>>> +		return -ENOMEM;
>>> +	}
>>> +
>>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +	if (!res) {
>>> +		dev_err(dev, "missing memory base resource\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	base = devm_request_and_ioremap(dev, res);
>>> +	if (!base) {
>>> +		dev_err(dev, "ioremap failed\n");
>>> +		return -EADDRNOTAVAIL;
>>> +	}
>>> +
>>> +	omap_control->base = base;
>>> +	omap_control->dev = dev;
>>> +	spin_lock_init(&omap_control->reg_lock);
>>> +
>>> +	platform_set_drvdata(pdev, omap_control);
>>> +	omap_control_module = omap_control;
>>> +
>>> +	return of_platform_populate(np, of_omap_control_match, NULL, dev);
>>> +}
>>> +
>>> +static int __devexit omap_control_remove(struct platform_device *pdev)
>>> +{
>>> +	struct omap_control *omap_control = platform_get_drvdata(pdev);
>>> +
>>> +	spin_lock(&omap_control->reg_lock);
>>> +	if (omap_control->use_count>   0) {
>>> +		spin_unlock(&omap_control->reg_lock);
>>> +		dev_err(&pdev->dev, "device removed while still being used\n");
>>> +		return -EBUSY;
>>> +	}
>>> +	spin_unlock(&omap_control->reg_lock);
>>> +
>>> +	iounmap(omap_control->base);
>>> +	platform_set_drvdata(pdev, NULL);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static struct platform_driver omap_control_driver = {
>>> +	.probe			= omap_control_probe,
>>> +	.remove			= __devexit_p(omap_control_remove),
>>> +	.driver = {
>>> +		.name		= "omap-control-core",
>>> +		.owner		= THIS_MODULE,
>>> +		.of_match_table	= of_omap_control_match,
>>> +	},
>>> +};
>>> +
>>> +static int __init omap_control_init(void)
>>> +{
>>> +	return platform_driver_register(&omap_control_driver);
>>> +}
>>> +postcore_initcall_sync(omap_control_init);
>>> +
>>> +static void __exit omap_control_exit(void)
>>> +{
>>> +	platform_driver_unregister(&omap_control_driver);
>>> +}
>>> +module_exit(omap_control_exit);
>>> +early_platform_init("early_omap_control",&omap_control_driver);
>>> +
>>> +MODULE_DESCRIPTION("OMAP system control module driver");
>>> +MODULE_LICENSE("GPL v2");
>>> +MODULE_ALIAS("platform:omap-control-core");
>>> +MODULE_AUTHOR("Texas Instruments Inc.");
>>> diff --git a/include/linux/mfd/omap_control.h b/include/linux/mfd/omap_control.h
>>> new file mode 100644
>>> index 0000000..7a33eda
>>> --- /dev/null
>>> +++ b/include/linux/mfd/omap_control.h
>>> @@ -0,0 +1,69 @@
>>> +/*
>>> + * OMAP system control module header file
>>> + *
>>> + * Copyright (C) 2011-2012 Texas Instruments Incorporated - http://www.ti.com/
>>> + * Contact:
>>> + *   J Keerthy<[email-masked]>
>>> + *   Moiz Sonasath<[email-masked]>
>>> + *   Abraham, Kishon Vijay<[email-masked]>
>>> + *   Eduardo Valentin<[email-masked]>
>>> + *
>>> + * 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.
>>> + *
>>> + * This program is distributed in the hope that it will be useful, but
>>> + * WITHOUT ANY WARRANTY; without even the implied warranty of
>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
>>> + * General Public License for more details.
>>> + *
>>> + */
>>> +
>>> +#ifndef __DRIVERS_OMAP_CONTROL_H
>>> +#define __DRIVERS_OMAP_CONTROL_H
>>> +
>>> +#include<linux/err.h>
>>> +
>>> +/**
>>> + * struct system control module - scm device structure
>>> + * @dev: device pointer
>>> + * @base: Base of the temp I/O
>>> + * @reg_lock: protect omap_control structure
>>> + * @use_count: track API users
>>> + */
>>> +struct omap_control {
>>> +	struct device		*dev;
>>
>> Do you really need the dev?
>> You API is device based and not omap_control based, so it should not
>> be needed.
>>
>> I guess we should be consistent here. We can store the devices and
>> used a device based API or store the omap_control and thus expose a
>> omap_control API.
>
> Yeah, this API structure is left over of the previous driver.
>
> The omap_control_get returns the SCM device reference
> and the users of SCM use it as parameter for the SCM APIs.
>
> We need to have in mind that, for SCM, the users are:
> a. Its children (USB phy, BG, etc)
> b. Non children users (mach code)
>
> The refcounting and the locking needs to take care of both I'd say.
> The struct dev was just a way to pass the SCM phandle.
>
>>
>> Regards,
>> Benoit