| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
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