Hi,
On Wed, Sep 5, 2012 at 2:01 PM, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote:
> On 09/05/2012 10:13 AM, Kuninori Morimoto wrote:
>> This patch adds Renesas R-Car USB phy driver.
>> It supports R8A7779 chip at this point.
>>
>> R-Car has some USB controllers, but has only one phy-initializer.
>> So, this driver is counting users.
>>
>> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>> ---
>> v1 -> v2
>>
>> - enable usb_phy_xxx()
>> - used subsys_initcall()
>>
>> drivers/usb/phy/Kconfig | 11 ++
>> drivers/usb/phy/Makefile | 1 +
>> drivers/usb/phy/rcar-phy.c | 241 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 253 insertions(+)
>> create mode 100644 drivers/usb/phy/rcar-phy.c
>>
>> diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig
>> index 2838adb..f55b6f6 100644
>> --- a/drivers/usb/phy/Kconfig
>> +++ b/drivers/usb/phy/Kconfig
>> @@ -23,3 +23,14 @@ config MV_U3D_PHY
>> help
>> Enable this to support Marvell USB 3.0 phy controller for Marvell
>> SoC.
>> +
>> +config USB_RCAR_PHY
>> + tristate "Renesas R-Car USB phy support"
>> + depends on (USB || USB_GADGET) && ARCH_R8A7779
>> + help
>> + Say Y here to add support for the Renesas R-Car USB phy driver.
>> + This chip is typically used as USB phy for USB host, gadget.
>> + This driver supports: R8A7779
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called rcar-phy.
>> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
>> index cf38f08..9af83f0 100644
>> --- a/drivers/usb/phy/Makefile
>> +++ b/drivers/usb/phy/Makefile
>> @@ -6,3 +6,4 @@ ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
>>
>> obj-$(CONFIG_USB_ISP1301) += isp1301.o
>> obj-$(CONFIG_MV_U3D_PHY) += mv_u3d_phy.o
>> +obj-$(CONFIG_USB_RCAR_PHY) += rcar-phy.o
>> diff --git a/drivers/usb/phy/rcar-phy.c b/drivers/usb/phy/rcar-phy.c
>> new file mode 100644
>> index 0000000..2169a1a
>> --- /dev/null
>> +++ b/drivers/usb/phy/rcar-phy.c
>> @@ -0,0 +1,241 @@
>> +/*
>> + * Renesas R-Car USB phy driver
>> + *
>> + * Copyright (C) 2012 Renesas Solutions Corp.
>> + * Kuninori Morimoto <kuninori.morimoto.gx@xxxxxxxxxxx>
>> + *
>> + * 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/delay.h>
>> +#include <linux/io.h>
>> +#include <linux/usb/otg.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/module.h>
>> +
>> +/* USBH common register */
>> +#define USBPCTRL0 0x0800
>> +#define USBPCTRL1 0x0804
>> +#define USBST 0x0808
>> +#define USBEH0 0x080C
>> +#define USBOH0 0x081C
>> +#define USBCTL0 0x0858
>> +#define EIIBC1 0x0094
>> +#define EIIBC2 0x009C
>> +
>> +#ifdef CONFIG_ARCH_SUPPORTS_BIG_ENDIAN
>> +# define PHY_ENDIAN "BIG"
>> +# define PHY_NO_SWAP 0x00000003
>> +#else
>> +# define PHY_ENDIAN "LITTLE"
>> +# define PHY_NO_SWAP 0x00000000
>> +#endif
>> +
>> +/* USBPCTRL1 */
>> +#define PHY_RST (1 << 2)
>> +#define PLL_ENB (1 << 1)
>> +#define PHY_ENB (1 << 0)
>> +
>> +/* USBST */
>> +#define ACT (1 << 31)
>> +#define PLL (1 << 30)
>> +
>> +struct rcar_usb_phy_priv {
>> + struct usb_phy phy;
>> + spinlock_t lock;
>> +
>> + void __iomem *reg0;
>> + void __iomem *reg1;
>> + int counter;
>> +};
>> +
>> +#define usb_phy_to_priv(p) container_of(p, struct rcar_usb_phy_priv, phy)
>> +
>> +
>> +/*
>> + * USB initial/install operation.
>> + *
>> + * This function setup USB phy.
>> + * The used value and setting order came from
>> + * [USB :: Initial setting] on datasheet.
>> + */
>> +static int rcar_usb_phy_init(struct usb_phy *phy)
>> +{
>> + struct rcar_usb_phy_priv *priv = usb_phy_to_priv(phy);
>> + struct device *dev = phy->dev;
>> + void __iomem *reg0 = priv->reg0;
>> + void __iomem *reg1 = priv->reg1;
>> + int i;
>> + u32 val;
>> + unsigned long flags;
>> +
>> + /******************** spin lock ********************/
While at that, you can also remove the above comment and the *spin
unlocked* comment.
>> + spin_lock_irqsave(&priv->lock, flags);
>> + if (0 == priv->counter) {
>> +
>> + /*----------------------------------------*
>> + * USB phy start-up
>> + *----------------------------------------*/
>
> nitpick: unusual coding style
>
> /*
> * This is the preferred
> * multi line style.
> */
>
>> +
>> + /* (1) USB-PHY standby release */
>> + iowrite32(PHY_ENB, (reg0 + USBPCTRL1));
>> +
>> + /* (2) start USB-PHY internal PLL */
>> + iowrite32(PHY_ENB | PLL_ENB, (reg0 + USBPCTRL1));
>> +
>> + /* (3) USB module status check */
>> + for (i = 0; i < 1024; i++) {
>> + udelay(10);
>> + val = ioread32(reg0 + USBST);
>> + if ((ACT | PLL) == val)
>> + goto usb_module_is_working;
>
> Don't use goto, unless in error handling.
>
>> + }
>> + dev_err(dev, "USB phy not ready\n");
>> + return -EIO;
>
> You return with the spin_lock held.
>
>> +
>> +usb_module_is_working:
>> + /* (4) USB-PHY reset clear */
>> + iowrite32(PHY_ENB | PLL_ENB | PHY_RST, (reg0 + USBPCTRL1));
>> +
>> + /* set platform specific port settings */
>> + iowrite32(0x00000000, (reg0 + USBPCTRL0));
>> +
>> + /*----------------------------------------*
>> + * EHCI IP Internal Buffer Setting
>> + *----------------------------------------*/
>> +
>> + /*
>> + * EHCI IP internal buffer setting
>> + * EHCI IP internal buffer enable
>> + *
>> + * These are recommended value of a datasheet
>> + * see [USB :: EHCI internal buffer setting]
>> + */
>> + iowrite32(0x00ff0040, (reg0 + EIIBC1));
>> + iowrite32(0x00ff0040, (reg1 + EIIBC1));
>> +
>> + iowrite32(0x00000001, (reg0 + EIIBC2));
>> + iowrite32(0x00000001, (reg1 + EIIBC2));
>> +
>> + /*----------------------------------------*
>> + * Bus alignment settings
>> + *----------------------------------------*/
>> +
>> + /* (1) EHCI bus alignment */
>> + iowrite32(PHY_NO_SWAP, (reg0 + USBEH0));
>> +
>> + /* (1) OHCI bus alignment */
>> + iowrite32(PHY_NO_SWAP, (reg0 + USBOH0));
>> + }
>> +
>> + priv->counter++;
>> + spin_unlock_irqrestore(&priv->lock, flags);
>> + /****************** spin unlocked ******************/
>> +
>> + return 0;
>> +}
>> +
>> +void rcar_usb_phy_shutdown(struct usb_phy *phy)
>> +{
>> + struct rcar_usb_phy_priv *priv = usb_phy_to_priv(phy);
>> + void __iomem *reg0 = priv->reg0;
>> + unsigned long flags;
>> +
>> + /******************** spin lock ********************/
>> + spin_lock_irqsave(&priv->lock, flags);
>> + if (0 == priv->counter) {
>> + iowrite32(0x00000000, (reg0 + USBPCTRL0));
>> + iowrite32(0x00000000, (reg0 + USBPCTRL1));
>> + }
>> +
>> + priv->counter--;
>
> Your usage counter looks broken.
>
> - in probe counter == 0
> - in init counter == 0 -> init phy -> counter == 1
> - in shutdown counter == 1 -> nothing -> counter == 0
>
>> + spin_unlock_irqrestore(&priv->lock, flags);
>> + /****************** spin unlocked ******************/
>> +}
>> +
>> +static int __devinit rcar_usb_phy_probe(struct platform_device *pdev)
>> +{
>> + struct rcar_usb_phy_priv *priv;
>> + struct resource *res0, *res1;
>> + struct device *dev = &pdev->dev;
>> + void __iomem *reg0, *reg1;
>> + int ret;
>> +
>> + res0 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + res1 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> + if (!res0 || !res1) {
>> + dev_err(dev, "Not enough platform resources\n");
>> + return -EINVAL;
>> + }
>> +
>> + reg0 = devm_ioremap_nocache(dev, res0->start, resource_size(res0));
>> + reg1 = devm_ioremap_nocache(dev, res1->start, resource_size(res1));
>> + if (!reg0 || !reg1) {
>> + dev_err(dev, "ioremap error\n");
>> + return -ENOMEM;
>> + }
>> +
>> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> + if (!priv) {
>> + dev_err(dev, "priv data allocation error\n");
>> + return -ENOMEM;
>> + }
>> +
>> + priv->reg0 = reg0;
>> + priv->reg1 = reg1;
>> + priv->counter = 0;
>> + priv->phy.dev = dev;
>> + priv->phy.label = dev_name(dev);
>> + priv->phy.init = rcar_usb_phy_init;
>> + priv->phy.shutdown = rcar_usb_phy_shutdown;
>> + spin_lock_init(&priv->lock);
>> +
>> + ret = usb_add_phy(&priv->phy, USB_PHY_TYPE_USB2);
>> + if (ret < 0) {
>> + dev_err(dev, "usb phy addition error\n");
>> + return ret;
>> + }
>> +
>> + platform_set_drvdata(pdev, priv);
>> +
>> + dev_info(dev, "Initialized R-Car USB PHY\n");
>> +
>> + return ret;
>> +}
>> +
>> +static int __exit rcar_usb_phy_remove(struct platform_device *pdev)
>> +{
>> + struct rcar_usb_phy_priv *priv = platform_get_drvdata(pdev);
>> +
>> + usb_remove_phy(&priv->phy);
>> +
>> + return 0;
>> +}
>> +
>> +static struct platform_driver rcar_usb_phy_driver = {
>> + .driver = {
>> + .name = "rcar_usb_phy",
>> + },
>> + .probe = rcar_usb_phy_probe,
>> + .remove = __devexit_p(rcar_usb_phy_remove),
>> +};
>> +
>> +static int __init rcar_usb_phy_initcall(void)
>> +{
>> + return platform_driver_register(&rcar_usb_phy_driver);
>> +}
>> +
>> +static void __exit rcar_usb_phy_exit(void)
>> +{
>> + platform_driver_unregister(&rcar_usb_phy_driver);
>> +}
>> +
>> +subsys_initcall(rcar_usb_phy_initcall);
I really dint mean that you should use subsys_initcall. -EPROBE_DEFER
in your client driver would be better.
Thanks
Kishon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
