Message-ID: <50F93B9C.108@ti.com>
Date:	Fri, 18 Jan 2013 17:40:04 +0530
From:	kishon <kishon@...com>
To:	<balbi@...com>
CC:	<rob.herring@...xeda.com>, <rob@...dley.net>, <tony@...mide.com>,
	<b-cousson@...com>, <linux-usb@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <devicetree-discuss@...ts.ozlabs.org>,
	<grant.likely@...retlab.ca>, <linux@....linux.org.uk>,
	<gregkh@...uxfoundation.org>
Subject: Re: [PATCH 1/4] drivers: usb: phy: add a new driver for usb part
 of control module

Hi,

On Friday 18 January 2013 05:29 PM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Jan 18, 2013 at 03:10:42PM +0530, Kishon Vijay Abraham I wrote:
>> Added a new driver for the usb part of control module. This has an API
>> to power on the USB2 phy and an API to write to the mailbox depending on
>> whether MUSB has to act in host mode or in device mode.
>>
>> Writing to control module registers for doing the above task which was
>> previously done in omap glue and in omap-usb2 phy will be removed.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@...com>
>> ---
>>   Documentation/devicetree/bindings/usb/omap-usb.txt |   26 ++-
>>   Documentation/devicetree/bindings/usb/usb-phy.txt  |    5 +
>>   drivers/usb/phy/Kconfig                            |    9 +
>>   drivers/usb/phy/Makefile                           |    1 +
>>   drivers/usb/phy/omap-control-usb.c                 |  204 ++++++++++++++++++++
>>   include/linux/usb/omap_control_usb.h               |   72 +++++++
>>   6 files changed, 316 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/usb/phy/omap-control-usb.c
>>   create mode 100644 include/linux/usb/omap_control_usb.h
>>
>> diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt b/Documentation/devicetree/bindings/usb/omap-usb.txt
>> index 29a043e..ead6ba9 100644
>> --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
>> @@ -1,4 +1,4 @@
>> -OMAP GLUE
>> +OMAP GLUE AND OTHER OMAP SPECIFIC COMPONENTS
>>
>>   OMAP MUSB GLUE
>>    - compatible : Should be "ti,omap4-musb" or "ti,omap3-musb"
>> @@ -16,6 +16,10 @@ OMAP MUSB GLUE
>>    - power : Should be "50". This signifies the controller can supply upto
>>      100mA when operating in host mode.
>>
>> +Optional properties:
>> + - ctrl_module : phandle of the control module this glue uses to write to
>> +   mailbox
>> +
>>   SOC specific device node entry
>>   usb_otg_hs: usb_otg_hs@...ab000 {
>>   	compatible = "ti,omap4-musb";
>> @@ -23,6 +27,7 @@ usb_otg_hs: usb_otg_hs@...ab000 {
>>   	multipoint = <1>;
>>   	num_eps = <16>;
>>   	ram_bits = <12>;
>> +	ctrl_module = <&omap_control_usb>;
>>   };
>>
>>   Board specific device node entry
>> @@ -31,3 +36,22 @@ Board specific device node entry
>>   	mode = <3>;
>>   	power = <50>;
>>   };
>> +
>> +OMAP CONTROL USB
>> +
>> +Required properties:
>> + - compatible: Should be "ti,omap-control-usb"
>> + - reg : Address and length of the register set for the device. It contains
>> +   the address of "control_dev_conf" and "otghs_control".
>> + - reg-names: The names of the register addresses corresponding to the registers
>> +   filled in "reg".
>> + - ti,has_mailbox: This is used to specify if the platform has mailbox in
>> +   control module.
>> +
>> +omap_control_usb: omap-control-usb@...02300 {
>> +	compatible = "ti,omap-control-usb";
>> +	reg = <0x4a002300 0x4>,
>> +	      <0x4a00233c 0x4>;
>> +	reg-names = "control_dev_conf", "otghs_control";
>> +	ti,has_mailbox;
>> +};
>> diff --git a/Documentation/devicetree/bindings/usb/usb-phy.txt b/Documentation/devicetree/bindings/usb/usb-phy.txt
>> index 80d4148..2466b6f 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-phy.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-phy.txt
>> @@ -8,10 +8,15 @@ Required properties:
>>   add the address of control module dev conf register until a driver for
>>   control module is added
>>
>> +Optional properties:
>> + - ctrl_module : phandle of the control module used by PHY driver to power on
>> +   the PHY.
>> +
>>   This is usually a subnode of ocp2scp to which it is connected.
>>
>>   usb2phy@...ad080 {
>>   	compatible = "ti,omap-usb2";
>>   	reg = <0x4a0ad080 0x58>,
>>   	      <0x4a002300 0x4>;
>> +	ctrl_module = <&omap_control_usb>;
>>   };
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index 5de6e7f..a7277ee 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -14,6 +14,15 @@ config OMAP_USB2
>>   	  The USB OTG controller communicates with the comparator using this
>>   	  driver.
>>
>> +config OMAP_CONTROL_USB
>> +	tristate "OMAP CONTROL USB Driver"
>> +	depends on ARCH_OMAP2PLUS
>> +	help
>> +	  Enable this to add support for the USB part present in the control
>> +	  module. This driver has API to power on the PHY and to write to the
>> +	  mailbox. The mailbox is present only in omap4 and the register to
>> +	  power on the PHY is present in omap4 and omap5.
>> +
>>   config USB_ISP1301
>>   	tristate "NXP ISP1301 USB transceiver support"
>>   	depends on USB || USB_GADGET
>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>> index 1a579a8..0dea4d2 100644
>> --- a/drivers/usb/phy/Makefile
>> +++ b/drivers/usb/phy/Makefile
>> @@ -5,6 +5,7 @@
>>   ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
>>
>>   obj-$(CONFIG_OMAP_USB2)			+= omap-usb2.o
>> +obj-$(CONFIG_OMAP_CONTROL_USB)		+= omap-control-usb.o
>>   obj-$(CONFIG_USB_ISP1301)		+= isp1301.o
>>   obj-$(CONFIG_MV_U3D_PHY)		+= mv_u3d_phy.o
>>   obj-$(CONFIG_USB_EHCI_TEGRA)	+= tegra_usb_phy.o
>> diff --git a/drivers/usb/phy/omap-control-usb.c b/drivers/usb/phy/omap-control-usb.c
>> new file mode 100644
>> index 0000000..7edeb41
>> --- /dev/null
>> +++ b/drivers/usb/phy/omap-control-usb.c
>> @@ -0,0 +1,204 @@
>> +/*
>> + * omap-control-usb.c - The USB part of control module.
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>> + *
>> + * 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/platform_device.h>
>> +#include <linux/slab.h>
>> +#include <linux/of.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/usb/omap_control_usb.h>
>> +
>> +static struct omap_control_usb *control_usb;
>> +
>> +/**
>> + * get_omap_control_dev - returns the device pointer for this control device
>> + *
>> + * This API should be called to get the device pointer for this control
>> + * module device. This device pointer should be passed to all other API's
>> + * in this driver.
>> + *
>> + * To be used by PHY driver and glue driver
>> + */
>> +struct device *get_omap_control_dev(void)
>
> let's do a small rename here and call it:
>
> omap_get_control_dev(), just to keep consistency with rest of the file.

Ok.
>
>> +{
>> +	if (!control_usb)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	return control_usb->dev;
>> +}
>> +EXPORT_SYMBOL_GPL(get_omap_control_dev);
>> +
>> +/**
>> + * omap_control_usb_phy_power - power on/off the phy using control module reg
>> + * @dev: the control module device
>> + * @on: 0 or 1, based on powering on or off the PHY
>> + */
>> +void omap_control_usb_phy_power(struct device *dev, int on)
>> +{
>> +	u32 val;
>> +	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
>> +
>> +	if (on) {
>> +		val = readl(control_usb->dev_conf);
>> +		if (val & PHY_PD)
>> +			writel(~PHY_PD, control_usb->dev_conf);
>> +	} else {
>> +		writel(PHY_PD, control_usb->dev_conf);
>> +	}
>
> this can be better. I would rather have this:
>
> struct omap_control_usb *ctrl = dev_get_drvdata(dev);
> u32 val;
>
> val = readl(ctrl->dev_conf);
>
> if (on)
> 	val &= ~OMAP_CTRL_DEV_PHY_PD;
> else
> 	val |= OMAP_CTRL_DEV_PHY_PD;
>
> writel(val, ctrl->dev_conf);
>
>
>
>> +}
>> +EXPORT_SYMBOL_GPL(omap_control_usb_phy_power);
>> +
>> +/**
>> + * omap_control_usb_host_mode - set AVALID, VBUSVALID and ID pin in grounded
>> + * @dev: struct device *
>> + *
>> + * This is an API to write to the mailbox register to notify the usb core that
>> + * a usb device has been connected.
>> + */
>> +void omap_control_usb_host_mode(struct device *dev)
>> +{
>> +	u32 val;
>> +	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
>> +
>> +	val = AVALID | VBUSVALID;
>> +
>> +	writel(val, control_usb->otghs_control);
>
> I would like to make this future proof too:
>
> val = readl(ctrl->otghs_control);
> val |= OMAP_CTRL_DEV_AVALID | OMAP_CTRL_DEV_VBUSVALID;
> writel(val, ctrl->otghs_control);

I think we might then have to add
val &= ~(IDDIG | SESSEND) right?

Similar things should be done for below API's also so that we don't 
inadvertently carry forward the settings.
>
>> +void omap_control_usb_device_mode(struct device *dev)
>> +{
>> +	u32 val;
>> +	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
>> +
>> +	val = IDDIG | AVALID | VBUSVALID;
>
> ditto. Read it before changing it. (the idea is that in future the other
> bits might be used for something else, so you risk breaking future
> functionality).
>
>> +
>> +	writel(val, control_usb->otghs_control);
>> +}
>> +EXPORT_SYMBOL_GPL(omap_control_usb_device_mode);
>> +
>> +/**
>> + * omap_control_usb_set_sessionend - Enable SESSIONEND and IDIG to high
>> + * impedance
>> + * @dev: struct device *
>> + *
>> + * This is an API to write to the mailbox register to notify the usb core
>> + * it's now in disconnected state.
>> + */
>> +void omap_control_usb_set_sessionend(struct device *dev)
>> +{
>> +	u32 val;
>> +	struct omap_control_usb	*control_usb = dev_get_drvdata(dev);
>> +
>> +	val = SESSEND | IDDIG;
>
> ditto
>
>> +
>> +	writel(val, control_usb->otghs_control);
>> +}
>> +EXPORT_SYMBOL_GPL(omap_control_usb_set_sessionend);
>> +
>> +static int omap_control_usb_probe(struct platform_device *pdev)
>> +{
>> +	struct resource	*res;
>> +	struct device_node *np = pdev->dev.of_node;
>> +	struct omap_control_usb_platform_data *pdata = pdev->dev.platform_data;
>> +
>> +	control_usb = devm_kzalloc(&pdev->dev, sizeof(*control_usb),
>> +	    GFP_KERNEL);
>
> you're using space indentation here.
>
>> +	if (!control_usb) {
>> +		dev_err(&pdev->dev, "unable to alloc memory for control usb\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	if (np) {
>> +		control_usb->has_mailbox = of_property_read_bool(np,
>> +		    "ti,has_mailbox");
>> +	} else if (pdata) {
>> +		control_usb->has_mailbox = pdata->has_mailbox;
>> +	} else {
>> +		dev_err(&pdev->dev, "no pdata present\n");
>> +		return -EINVAL;
>
> does it make sense to default to has_mailbox as true ? I mean, seems
> like this is how it's going to be in future devices looking at the
> current trend.

No actually. omap5 doesn't have mailbox in control module ;-)
>
> Another idea is to try to get the extra resource below, if it fails you
> try to continue without it ;-)

I prefer the way it is now because mailbox is needed for MUSB to be 
functional in OMAP4 and we should fail if we don't have it. No?
>
>> +	}
>> +
>> +	control_usb->dev	= &pdev->dev;
>> +
>> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +	    "control_dev_conf");
>
> using space indentation here again.
>
>> +	control_usb->dev_conf = devm_request_and_ioremap(&pdev->dev, res);
>> +	if (control_usb->dev_conf == NULL) {
>> +		dev_err(&pdev->dev, "Failed to obtain io memory\n");
>> +		return -EADDRNOTAVAIL;
>> +	}
>> +
>> +	if (control_usb->has_mailbox) {
>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +		    "otghs_control");
>> +		control_usb->otghs_control = devm_request_and_ioremap(
>> +		    &pdev->dev, res);
>> +		if (control_usb->otghs_control == NULL) {
>> +			dev_err(&pdev->dev, "Failed to obtain io memory\n");
>> +			return -EADDRNOTAVAIL;
>> +		}
>> +	}
>> +
>> +	dev_set_drvdata(control_usb->dev, control_usb);
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id omap_control_usb_id_table[] = {
>> +	{ .compatible = "ti,omap-control-usb" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, omap_control_usb_id_table);
>> +#endif
>> +
>> +static struct platform_driver omap_control_usb_driver = {
>> +	.probe		= omap_control_usb_probe,
>> +	.driver		= {
>> +		.name	= "omap-control-usb",
>> +		.owner	= THIS_MODULE,
>
> no need to set owner here. When you register the platform_driver, it
> will be set for you.

Ok.
>
>> +		.of_match_table = of_match_ptr(omap_control_usb_id_table),
>> +	},
>> +};
>> +
>> +static int __init omap_control_usb_init(void)
>> +{
>> +	return platform_driver_register(&omap_control_usb_driver);
>> +}
>> +subsys_initcall(omap_control_usb_init);
>> +
>> +static void __exit omap_control_usb_exit(void)
>> +{
>> +	platform_driver_unregister(&omap_control_usb_driver);
>> +}
>> +module_exit(omap_control_usb_exit);
>> +
>> +MODULE_ALIAS("platform: omap_control_usb");
>> +MODULE_AUTHOR("Texas Instruments Inc.");
>> +MODULE_DESCRIPTION("OMAP CONTROL USB DRIVER");
>
> don't scream to your users like that ;-)
>
> MODULE_DESCRIPTION("OMAP Control Module USB Driver");

Ok.
>
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/include/linux/usb/omap_control_usb.h b/include/linux/usb/omap_control_usb.h
>> new file mode 100644
>> index 0000000..a16f993
>> --- /dev/null
>> +++ b/include/linux/usb/omap_control_usb.h
>> @@ -0,0 +1,72 @@
>> +/*
>> + * omap_control_usb.h - Header file for the USB part of control module.
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * Author: Kishon Vijay Abraham I <kishon@...com>
>> + *
>> + * 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 __OMAP_CONTROL_USB_H__
>> +#define __OMAP_CONTROL_USB_H__
>> +
>> +struct omap_control_usb {
>> +	struct device *dev;
>> +
>> +	u32 __iomem *dev_conf;
>> +	u32 __iomem *otghs_control;
>> +
>> +	u8 has_mailbox:1;
>> +};
>> +
>> +struct omap_control_usb_platform_data {
>> +	u8 has_mailbox:1;
>> +};
>> +
>> +#define	PHY_PD		BIT(0)
>> +
>> +#define	AVALID		BIT(0)
>> +#define	BVALID		BIT(1)
>> +#define	VBUSVALID	BIT(2)
>> +#define	SESSEND		BIT(3)
>> +#define	IDDIG		BIT(4)
>
> please prepend with OMAP_CTRL_ or something similar.

Sure.

Thanks
Kishon
--
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