From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chanwoo Choi Subject: Re: [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG Date: Thu, 11 Jun 2015 17:07:02 +0900 Message-ID: <557941A6.5000306@samsung.com> References: <1433088626-8858-1-git-send-email-hdegoede@redhat.com> <1433088626-8858-2-git-send-email-hdegoede@redhat.com> <55792122.20607@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <55792122.20607-l0cyMroinI0@public.gmane.org> Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Kishon Vijay Abraham I Cc: Hans de Goede , Felipe Balbi , Maxime Ripard , Chen-Yu Tsai , Roman Byshko , linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, devicetree , linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org List-Id: devicetree@vger.kernel.org Hi Hans, I add the comment about extcon-related code. Firstly, I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD because there are both extcon provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory. On 06/11/2015 02:48 PM, Kishon Vijay Abraham I wrote: > +Chanwoo > > Hi, > > On Sunday 31 May 2015 09:40 PM, Hans de Goede wrote: >> The usb0 phy is connected to an OTG controller, and as such needs some special >> handling: >> >> 1) It allows explicit control over the pullups, enable these on phy_init and >> disable them on phy_exit. >> >> 2) It has bits to signal id and vbus detect to the musb-core, add support for >> for monitoring id and vbus detect gpio-s for use in dual role mode, and set >> these bits to the correct values for operating in host only mode when no >> gpios are specified in the devicetree. >> >> 3) When in dual role mode the musb sunxi glue needs to know if the a host or >> device cable is plugged in, so when in dual role mode register an extcon. >> >> While updating the devicetree binding documentation also add documentation >> for the sofar undocumented usage of regulators for vbus for all 3 phys. >> >> Signed-off-by: Hans de Goede >> --- >> Changes in v2: >> -Removed the sunxi specific phy functions, instead the id / vbus gpio polling >> has been moved to the phy-sun4i-usb driver and their status is exported >> through extcon for the sunxi-musb glue >> Changes in v3: >> -No changes >> Changes in v4: >> -Do not call regulator_disable in an unbalanced manner when an external vbus >> is present >> --- >> .../devicetree/bindings/phy/sun4i-usb-phy.txt | 18 +- >> drivers/phy/Kconfig | 1 + >> drivers/phy/phy-sun4i-usb.c | 273 ++++++++++++++++++++- >> 3 files changed, 281 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> index 16528b9..557fa99 100644 >> --- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> +++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> @@ -23,6 +23,13 @@ Required properties: >> * "usb1_reset" >> * "usb2_reset" for sun4i, sun6i or sun7i >> >> +Optional properties: >> +- usb0_id_det-gpios : gpio phandle for reading the otg id pin value >> +- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus >> +- usb0_vbus-supply : regulator phandle for controller usb0 vbus >> +- usb1_vbus-supply : regulator phandle for controller usb1 vbus >> +- usb2_vbus-supply : regulator phandle for controller usb2 vbus >> + >> Example: >> usbphy: phy@0x01c13400 { >> #phy-cells = <1>; >> @@ -32,6 +39,13 @@ Example: >> reg-names = "phy_ctrl", "pmu1", "pmu2"; >> clocks = <&usb_clk 8>; >> clock-names = "usb_phy"; >> - resets = <&usb_clk 1>, <&usb_clk 2>; >> - reset-names = "usb1_reset", "usb2_reset"; >> + resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>; >> + reset-names = "usb0_reset", "usb1_reset", "usb2_reset"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>; >> + usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */ >> + usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */ >> + usb0_vbus-supply = <®_usb0_vbus>; >> + usb1_vbus-supply = <®_usb1_vbus>; >> + usb2_vbus-supply = <®_usb2_vbus>; >> }; >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index a53bd5b..4614fba 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -173,6 +173,7 @@ config PHY_SUN4I_USB >> tristate "Allwinner sunxi SoC USB PHY driver" >> depends on ARCH_SUNXI && HAS_IOMEM && OF >> depends on RESET_CONTROLLER >> + select EXTCON > > Avoid using 'select' on visible Kconfig symbols. > > Also please split the patch to make the reviewing a bit easier. > > Thanks > Kishon >> select GENERIC_PHY >> help >> Enable this to support the transceiver that is part of Allwinner >> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >> index 91c5be4..b45d707 100644 >> --- a/drivers/phy/phy-sun4i-usb.c >> +++ b/drivers/phy/phy-sun4i-usb.c >> @@ -1,7 +1,7 @@ >> /* >> * Allwinner sun4i USB phy driver >> * >> - * Copyright (C) 2014 Hans de Goede >> + * Copyright (C) 2014-2015 Hans de Goede >> * >> * Based on code from >> * Allwinner Technology Co., Ltd. >> @@ -23,17 +23,23 @@ >> >> #include >> #include >> +#include >> #include >> +#include >> #include >> #include >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> #include >> #include >> +#include >> + >> +#define DRIVER_NAME "sun4i-usb-phy" >> >> #define REG_ISCR 0x00 >> #define REG_PHYCTL 0x04 >> @@ -47,6 +53,17 @@ >> #define SUNXI_AHB_INCRX_ALIGN_EN BIT(8) >> #define SUNXI_ULPI_BYPASS_EN BIT(0) >> >> +/* ISCR, Interface Status and Control bits */ >> +#define ISCR_ID_PULLUP_EN (1 << 17) >> +#define ISCR_DPDM_PULLUP_EN (1 << 16) >> +/* sunxi has the phy id/vbus pins not connected, so we use the force bits */ >> +#define ISCR_FORCE_ID_MASK (3 << 14) >> +#define ISCR_FORCE_ID_LOW (2 << 14) >> +#define ISCR_FORCE_ID_HIGH (3 << 14) >> +#define ISCR_FORCE_VBUS_MASK (3 << 12) >> +#define ISCR_FORCE_VBUS_LOW (2 << 12) >> +#define ISCR_FORCE_VBUS_HIGH (3 << 12) >> + >> /* Common Control Bits for Both PHYs */ >> #define PHY_PLL_BW 0x03 >> #define PHY_RES45_CAL_EN 0x0c >> @@ -63,6 +80,13 @@ >> >> #define MAX_PHYS 3 >> >> +/* >> + * Note do not raise the debounce time, we must report Vusb high within 100ms >> + * otherwise we get Vbus errors >> + */ >> +#define DEBOUNCE_TIME msecs_to_jiffies(50) >> +#define POLL_TIME msecs_to_jiffies(250) >> + >> struct sun4i_usb_phy_data { >> void __iomem *base; >> struct mutex mutex; >> @@ -74,13 +98,58 @@ struct sun4i_usb_phy_data { >> struct regulator *vbus; >> struct reset_control *reset; >> struct clk *clk; >> + bool regulator_on; >> int index; >> } phys[MAX_PHYS]; >> + /* phy0 / otg related variables */ >> + struct extcon_dev extcon; >> + const char *extcon_cable_names[3]; The latest extcon use the unique id to indicate the each external connector instead of legacy string. The related patch[1] and patch[2] will be merged on v4.2-rc1. [1] extcon: Use the unique id for external connector instead of string - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae [2] extcon: Redefine the unique id of supported external connectors without 'enum extcon' type - http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next-v4.3&id=bbd8d8b4244bb2799143c959fde57b1f034ec838 For exmaple in drivers/extcon/extcon-usb-gpio.c: Maybe you can add the similiar array which include the supported external connector. static const unsigned int usb_extcon_cable[] = { EXTCON_USB, EXTCON_USB_HOST, EXTCON_NONE, }; >> + bool phy0_init; >> + bool phy0_poll; >> + struct gpio_desc *id_det_gpio; >> + struct gpio_desc *vbus_det_gpio; >> + int id_det_irq; >> + int vbus_det_irq; >> + int id_det; >> + int vbus_det; >> + struct delayed_work detect; >> }; >> >> #define to_sun4i_usb_phy_data(phy) \ >> container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index]) >> >> +static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set) >> +{ >> + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + u32 iscr; >> + >> + iscr = readl(data->base + REG_ISCR); >> + iscr &= ~clr; >> + iscr |= set; >> + writel(iscr, data->base + REG_ISCR); >> +} >> + >> +static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val) >> +{ >> + if (val) >> + val = ISCR_FORCE_ID_HIGH; >> + else >> + val = ISCR_FORCE_ID_LOW; >> + >> + sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val); >> +} >> + >> +static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val) >> +{ >> + if (val) >> + val = ISCR_FORCE_VBUS_HIGH; >> + else >> + val = ISCR_FORCE_VBUS_LOW; >> + >> + sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val); >> +} >> + >> static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data, >> int len) >> { >> @@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy) >> >> sun4i_usb_phy_passby(phy, 1); >> >> + if (phy->index == 0) { >> + data->phy0_init = true; >> + >> + /* Enable pull-ups */ >> + sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN); >> + sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN); >> + >> + if (data->id_det_gpio) { >> + /* OTG mode, force ISCR and cable state updates */ >> + data->id_det = -1; >> + data->vbus_det = -1; >> + queue_delayed_work(system_wq, &data->detect, 0); >> + } else { >> + /* Host only mode */ >> + sun4i_usb_phy0_set_id_detect(_phy, 0); >> + sun4i_usb_phy0_set_vbus_detect(_phy, 1); >> + } >> + } >> + >> return 0; >> } >> >> static int sun4i_usb_phy_exit(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + >> + if (phy->index == 0) { >> + /* Disable pull-ups */ >> + sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0); >> + sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0); >> + data->phy0_init = false; >> + } >> >> sun4i_usb_phy_passby(phy, 0); >> reset_control_assert(phy->reset); >> @@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy) >> static int sun4i_usb_phy_power_on(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> - int ret = 0; >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + int ret; >> + >> + if (!phy->vbus || phy->regulator_on) >> + return 0; >> + >> + /* For phy0 only turn on Vbus if we don't have an ext. Vbus */ >> + if (phy->index == 0 && data->vbus_det) >> + return 0; >> >> - if (phy->vbus) >> - ret = regulator_enable(phy->vbus); >> + ret = regulator_enable(phy->vbus); >> + if (ret) >> + return ret; >> >> - return ret; >> + phy->regulator_on = true; >> + >> + /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */ >> + if (phy->index == 0 && data->phy0_poll) >> + mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME); >> + >> + return 0; >> } >> >> static int sun4i_usb_phy_power_off(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + >> + if (!phy->vbus || !phy->regulator_on) >> + return 0; >> >> - if (phy->vbus) >> - regulator_disable(phy->vbus); >> + regulator_disable(phy->vbus); >> + phy->regulator_on = false; >> + >> + /* >> + * phy0 vbus typically slowly discharges, sometimes this causes the >> + * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan. >> + */ >> + if (phy->index == 0 && !data->phy0_poll) >> + mod_delayed_work(system_wq, &data->detect, POLL_TIME); >> >> return 0; >> } >> @@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = { >> .owner = THIS_MODULE, >> }; >> >> +static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work) >> +{ >> + struct sun4i_usb_phy_data *data = >> + container_of(work, struct sun4i_usb_phy_data, detect.work); >> + struct phy *phy0 = data->phys[0].phy; >> + int id_det, vbus_det, id_notify = 0, vbus_notify = 0; >> + >> + id_det = gpiod_get_value_cansleep(data->id_det_gpio); >> + vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio); >> + >> + mutex_lock(&phy0->mutex); >> + >> + if (!data->phy0_init) { >> + mutex_unlock(&phy0->mutex); >> + return; >> + } >> + >> + if (id_det != data->id_det) { >> + sun4i_usb_phy0_set_id_detect(phy0, id_det); >> + data->id_det = id_det; >> + id_notify = 1; >> + } >> + >> + if (vbus_det != data->vbus_det) { >> + sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det); >> + data->vbus_det = vbus_det; >> + vbus_notify = 1; >> + } >> + >> + mutex_unlock(&phy0->mutex); >> + >> + if (id_notify) >> + extcon_set_cable_state(&data->extcon, >> + extcon_cable_name[EXTCON_USB_HOST], >> + !id_det); >> + >> + if (vbus_notify) >> + extcon_set_cable_state(&data->extcon, >> + extcon_cable_name[EXTCON_USB], >> + vbus_det); I don't want to use the legacy extcon API with string name. Instead, you can use the recommeded extcon_set_cable_state_() API as following: - Legacy extcon API : extern int extcon_set_cable_state(struct extcon_dev *edev, const char *cable_name, bool cable_state); - Recommended extcon API : extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id, bool cable_state); [1] extcon: Use the unique id for external connector instead of string - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae >> + >> + if (data->phy0_poll) >> + queue_delayed_work(system_wq, &data->detect, POLL_TIME); >> +} >> + >> +static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id) >> +{ >> + struct sun4i_usb_phy_data *data = dev_id; >> + >> + /* vbus or id changed, let the pins settle and then scan them */ >> + mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME); >> + >> + return IRQ_HANDLED; >> +} >> + >> static struct phy *sun4i_usb_phy_xlate(struct device *dev, >> struct of_phandle_args *args) >> { >> @@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> struct phy_provider *phy_provider; >> bool dedicated_clocks; >> struct resource *res; >> - int i; >> + int i, ret; >> >> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> if (!data) >> return -ENOMEM; >> >> mutex_init(&data->mutex); >> + INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan); >> + data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST]; >> + data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB]; >> + data->extcon_cable_names[2] = NULL; >> + data->extcon.name = DRIVER_NAME; Don't need it because extcon_dev_register() set the device name automatically. You can check it on patch[3]. [3] http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=71c3ffa5d23af0554c27010cf12710da9bf85950 >> + data->extcon.supported_cable = data->extcon_cable_names; You can use the devm_extcon_dev_allocate() to set the supported external connectors. >> + data->extcon.dev.parent = dev; Don't need it because extcon_dev_register() set the parent device automatically. The field of 'struct extcon_dev' shold be filled with extcon API without direct allocation. >> >> if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy")) >> data->num_phys = 2; >> @@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> if (IS_ERR(data->base)) >> return PTR_ERR(data->base); >> >> + data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN); >> + if (IS_ERR(data->id_det_gpio)) { >> + if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + data->id_det_gpio = NULL; >> + } >> + >> + data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN); >> + if (IS_ERR(data->vbus_det_gpio)) { >> + if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + data->vbus_det_gpio = NULL; >> + } >> + >> + /* We either want both gpio pins or neither (when in host mode) */ >> + if (!data->id_det_gpio != !data->vbus_det_gpio) { >> + dev_err(dev, "failed to get id or vbus detect pin\n"); >> + return -ENODEV; >> + } >> + >> + if (data->id_det_gpio) { >> + ret = devm_extcon_dev_register(dev, &data->extcon); >> + if (ret) { >> + dev_err(dev, "failed to register extcon: %d\n", ret); >> + return ret; >> + } >> + } >> + >> for (i = 0; i < data->num_phys; i++) { >> struct sun4i_usb_phy *phy = data->phys + i; >> char name[16]; >> @@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> phy_set_drvdata(phy->phy, &data->phys[i]); >> } >> >> + data->id_det_irq = gpiod_to_irq(data->id_det_gpio); >> + data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio); >> + if (data->id_det_irq < 0 || data->vbus_det_irq < 0) >> + data->phy0_poll = true; >> + >> + if (data->id_det_irq >= 0) { >> + ret = devm_request_irq(dev, data->id_det_irq, >> + sun4i_usb_phy0_id_vbus_det_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + "usb0-id-det", data); >> + if (ret) { >> + dev_err(dev, "Err requesting id-det-irq: %d\n", ret); >> + return ret; >> + } >> + } >> + >> + if (data->vbus_det_irq >= 0) { >> + ret = devm_request_irq(dev, data->vbus_det_irq, >> + sun4i_usb_phy0_id_vbus_det_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + "usb0-vbus-det", data); >> + if (ret) { >> + dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret); >> + return ret; >> + } >> + } >> + >> dev_set_drvdata(dev, data); >> phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate); >> >> return PTR_ERR_OR_ZERO(phy_provider); >> } >> >> +static int sun4i_usb_phy_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct sun4i_usb_phy_data *data = dev_get_drvdata(dev); >> + >> + if (data->id_det_irq >= 0) >> + devm_free_irq(dev, data->id_det_irq, data); >> + if (data->vbus_det_irq >= 0) >> + devm_free_irq(dev, data->vbus_det_irq, data); >> + >> + cancel_delayed_work_sync(&data->detect); >> + >> + return 0; >> +} >> + >> static const struct of_device_id sun4i_usb_phy_of_match[] = { >> { .compatible = "allwinner,sun4i-a10-usb-phy" }, >> { .compatible = "allwinner,sun5i-a13-usb-phy" }, >> @@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); >> >> static struct platform_driver sun4i_usb_phy_driver = { >> .probe = sun4i_usb_phy_probe, >> + .remove = sun4i_usb_phy_remove, >> .driver = { >> .of_match_table = sun4i_usb_phy_of_match, >> - .name = "sun4i-usb-phy", >> + .name = DRIVER_NAME, >> } >> }; >> module_platform_driver(sun4i_usb_phy_driver); >> > Thanks, Chanwoo Choi -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 From: cw00.choi@samsung.com (Chanwoo Choi) Date: Thu, 11 Jun 2015 17:07:02 +0900 Subject: [PATCH v4 1/2] phy-sun4i-usb: Add full support for usb0 phy / OTG In-Reply-To: <55792122.20607@ti.com> References: <1433088626-8858-1-git-send-email-hdegoede@redhat.com> <1433088626-8858-2-git-send-email-hdegoede@redhat.com> <55792122.20607@ti.com> Message-ID: <557941A6.5000306@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Hans, I add the comment about extcon-related code. Firstly, I'd like you to implment the extcon driver for phy-sun4i-usb device in drivers/extcon/ directoryby using MFD because there are both extcon provider driver and extcon client driver. I think that all extcon provider driver better to be included in drivers/extcon/ directory. extcon_set_cable_state() function should be handled in extcon provider driver which is incluced in drivers/extcon/ directory. On 06/11/2015 02:48 PM, Kishon Vijay Abraham I wrote: > +Chanwoo > > Hi, > > On Sunday 31 May 2015 09:40 PM, Hans de Goede wrote: >> The usb0 phy is connected to an OTG controller, and as such needs some special >> handling: >> >> 1) It allows explicit control over the pullups, enable these on phy_init and >> disable them on phy_exit. >> >> 2) It has bits to signal id and vbus detect to the musb-core, add support for >> for monitoring id and vbus detect gpio-s for use in dual role mode, and set >> these bits to the correct values for operating in host only mode when no >> gpios are specified in the devicetree. >> >> 3) When in dual role mode the musb sunxi glue needs to know if the a host or >> device cable is plugged in, so when in dual role mode register an extcon. >> >> While updating the devicetree binding documentation also add documentation >> for the sofar undocumented usage of regulators for vbus for all 3 phys. >> >> Signed-off-by: Hans de Goede >> --- >> Changes in v2: >> -Removed the sunxi specific phy functions, instead the id / vbus gpio polling >> has been moved to the phy-sun4i-usb driver and their status is exported >> through extcon for the sunxi-musb glue >> Changes in v3: >> -No changes >> Changes in v4: >> -Do not call regulator_disable in an unbalanced manner when an external vbus >> is present >> --- >> .../devicetree/bindings/phy/sun4i-usb-phy.txt | 18 +- >> drivers/phy/Kconfig | 1 + >> drivers/phy/phy-sun4i-usb.c | 273 ++++++++++++++++++++- >> 3 files changed, 281 insertions(+), 11 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> index 16528b9..557fa99 100644 >> --- a/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> +++ b/Documentation/devicetree/bindings/phy/sun4i-usb-phy.txt >> @@ -23,6 +23,13 @@ Required properties: >> * "usb1_reset" >> * "usb2_reset" for sun4i, sun6i or sun7i >> >> +Optional properties: >> +- usb0_id_det-gpios : gpio phandle for reading the otg id pin value >> +- usb0_vbus_det-gpios : gpio phandle for detecting the presence of usb0 vbus >> +- usb0_vbus-supply : regulator phandle for controller usb0 vbus >> +- usb1_vbus-supply : regulator phandle for controller usb1 vbus >> +- usb2_vbus-supply : regulator phandle for controller usb2 vbus >> + >> Example: >> usbphy: phy at 0x01c13400 { >> #phy-cells = <1>; >> @@ -32,6 +39,13 @@ Example: >> reg-names = "phy_ctrl", "pmu1", "pmu2"; >> clocks = <&usb_clk 8>; >> clock-names = "usb_phy"; >> - resets = <&usb_clk 1>, <&usb_clk 2>; >> - reset-names = "usb1_reset", "usb2_reset"; >> + resets = <&usb_clk 0>, <&usb_clk 1>, <&usb_clk 2>; >> + reset-names = "usb0_reset", "usb1_reset", "usb2_reset"; >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb0_id_detect_pin>, <&usb0_vbus_detect_pin>; >> + usb0_id_det-gpios = <&pio 7 19 GPIO_ACTIVE_HIGH>; /* PH19 */ >> + usb0_vbus_det-gpios = <&pio 7 22 GPIO_ACTIVE_HIGH>; /* PH22 */ >> + usb0_vbus-supply = <®_usb0_vbus>; >> + usb1_vbus-supply = <®_usb1_vbus>; >> + usb2_vbus-supply = <®_usb2_vbus>; >> }; >> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig >> index a53bd5b..4614fba 100644 >> --- a/drivers/phy/Kconfig >> +++ b/drivers/phy/Kconfig >> @@ -173,6 +173,7 @@ config PHY_SUN4I_USB >> tristate "Allwinner sunxi SoC USB PHY driver" >> depends on ARCH_SUNXI && HAS_IOMEM && OF >> depends on RESET_CONTROLLER >> + select EXTCON > > Avoid using 'select' on visible Kconfig symbols. > > Also please split the patch to make the reviewing a bit easier. > > Thanks > Kishon >> select GENERIC_PHY >> help >> Enable this to support the transceiver that is part of Allwinner >> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c >> index 91c5be4..b45d707 100644 >> --- a/drivers/phy/phy-sun4i-usb.c >> +++ b/drivers/phy/phy-sun4i-usb.c >> @@ -1,7 +1,7 @@ >> /* >> * Allwinner sun4i USB phy driver >> * >> - * Copyright (C) 2014 Hans de Goede >> + * Copyright (C) 2014-2015 Hans de Goede >> * >> * Based on code from >> * Allwinner Technology Co., Ltd. >> @@ -23,17 +23,23 @@ >> >> #include >> #include >> +#include >> #include >> +#include >> #include >> #include >> #include >> #include >> #include >> +#include >> #include >> #include >> #include >> #include >> #include >> +#include >> + >> +#define DRIVER_NAME "sun4i-usb-phy" >> >> #define REG_ISCR 0x00 >> #define REG_PHYCTL 0x04 >> @@ -47,6 +53,17 @@ >> #define SUNXI_AHB_INCRX_ALIGN_EN BIT(8) >> #define SUNXI_ULPI_BYPASS_EN BIT(0) >> >> +/* ISCR, Interface Status and Control bits */ >> +#define ISCR_ID_PULLUP_EN (1 << 17) >> +#define ISCR_DPDM_PULLUP_EN (1 << 16) >> +/* sunxi has the phy id/vbus pins not connected, so we use the force bits */ >> +#define ISCR_FORCE_ID_MASK (3 << 14) >> +#define ISCR_FORCE_ID_LOW (2 << 14) >> +#define ISCR_FORCE_ID_HIGH (3 << 14) >> +#define ISCR_FORCE_VBUS_MASK (3 << 12) >> +#define ISCR_FORCE_VBUS_LOW (2 << 12) >> +#define ISCR_FORCE_VBUS_HIGH (3 << 12) >> + >> /* Common Control Bits for Both PHYs */ >> #define PHY_PLL_BW 0x03 >> #define PHY_RES45_CAL_EN 0x0c >> @@ -63,6 +80,13 @@ >> >> #define MAX_PHYS 3 >> >> +/* >> + * Note do not raise the debounce time, we must report Vusb high within 100ms >> + * otherwise we get Vbus errors >> + */ >> +#define DEBOUNCE_TIME msecs_to_jiffies(50) >> +#define POLL_TIME msecs_to_jiffies(250) >> + >> struct sun4i_usb_phy_data { >> void __iomem *base; >> struct mutex mutex; >> @@ -74,13 +98,58 @@ struct sun4i_usb_phy_data { >> struct regulator *vbus; >> struct reset_control *reset; >> struct clk *clk; >> + bool regulator_on; >> int index; >> } phys[MAX_PHYS]; >> + /* phy0 / otg related variables */ >> + struct extcon_dev extcon; >> + const char *extcon_cable_names[3]; The latest extcon use the unique id to indicate the each external connector instead of legacy string. The related patch[1] and patch[2] will be merged on v4.2-rc1. [1] extcon: Use the unique id for external connector instead of string - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae [2] extcon: Redefine the unique id of supported external connectors without 'enum extcon' type - http://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/commit/?h=extcon-next-v4.3&id=bbd8d8b4244bb2799143c959fde57b1f034ec838 For exmaple in drivers/extcon/extcon-usb-gpio.c: Maybe you can add the similiar array which include the supported external connector. static const unsigned int usb_extcon_cable[] = { EXTCON_USB, EXTCON_USB_HOST, EXTCON_NONE, }; >> + bool phy0_init; >> + bool phy0_poll; >> + struct gpio_desc *id_det_gpio; >> + struct gpio_desc *vbus_det_gpio; >> + int id_det_irq; >> + int vbus_det_irq; >> + int id_det; >> + int vbus_det; >> + struct delayed_work detect; >> }; >> >> #define to_sun4i_usb_phy_data(phy) \ >> container_of((phy), struct sun4i_usb_phy_data, phys[(phy)->index]) >> >> +static void sun4i_usb_phy0_update_iscr(struct phy *_phy, u32 clr, u32 set) >> +{ >> + struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + u32 iscr; >> + >> + iscr = readl(data->base + REG_ISCR); >> + iscr &= ~clr; >> + iscr |= set; >> + writel(iscr, data->base + REG_ISCR); >> +} >> + >> +static void sun4i_usb_phy0_set_id_detect(struct phy *phy, u32 val) >> +{ >> + if (val) >> + val = ISCR_FORCE_ID_HIGH; >> + else >> + val = ISCR_FORCE_ID_LOW; >> + >> + sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_ID_MASK, val); >> +} >> + >> +static void sun4i_usb_phy0_set_vbus_detect(struct phy *phy, u32 val) >> +{ >> + if (val) >> + val = ISCR_FORCE_VBUS_HIGH; >> + else >> + val = ISCR_FORCE_VBUS_LOW; >> + >> + sun4i_usb_phy0_update_iscr(phy, ISCR_FORCE_VBUS_MASK, val); >> +} >> + >> static void sun4i_usb_phy_write(struct sun4i_usb_phy *phy, u32 addr, u32 data, >> int len) >> { >> @@ -171,12 +240,39 @@ static int sun4i_usb_phy_init(struct phy *_phy) >> >> sun4i_usb_phy_passby(phy, 1); >> >> + if (phy->index == 0) { >> + data->phy0_init = true; >> + >> + /* Enable pull-ups */ >> + sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_DPDM_PULLUP_EN); >> + sun4i_usb_phy0_update_iscr(_phy, 0, ISCR_ID_PULLUP_EN); >> + >> + if (data->id_det_gpio) { >> + /* OTG mode, force ISCR and cable state updates */ >> + data->id_det = -1; >> + data->vbus_det = -1; >> + queue_delayed_work(system_wq, &data->detect, 0); >> + } else { >> + /* Host only mode */ >> + sun4i_usb_phy0_set_id_detect(_phy, 0); >> + sun4i_usb_phy0_set_vbus_detect(_phy, 1); >> + } >> + } >> + >> return 0; >> } >> >> static int sun4i_usb_phy_exit(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + >> + if (phy->index == 0) { >> + /* Disable pull-ups */ >> + sun4i_usb_phy0_update_iscr(_phy, ISCR_DPDM_PULLUP_EN, 0); >> + sun4i_usb_phy0_update_iscr(_phy, ISCR_ID_PULLUP_EN, 0); >> + data->phy0_init = false; >> + } >> >> sun4i_usb_phy_passby(phy, 0); >> reset_control_assert(phy->reset); >> @@ -188,20 +284,46 @@ static int sun4i_usb_phy_exit(struct phy *_phy) >> static int sun4i_usb_phy_power_on(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> - int ret = 0; >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + int ret; >> + >> + if (!phy->vbus || phy->regulator_on) >> + return 0; >> + >> + /* For phy0 only turn on Vbus if we don't have an ext. Vbus */ >> + if (phy->index == 0 && data->vbus_det) >> + return 0; >> >> - if (phy->vbus) >> - ret = regulator_enable(phy->vbus); >> + ret = regulator_enable(phy->vbus); >> + if (ret) >> + return ret; >> >> - return ret; >> + phy->regulator_on = true; >> + >> + /* We must report Vbus high within OTG_TIME_A_WAIT_VRISE msec. */ >> + if (phy->index == 0 && data->phy0_poll) >> + mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME); >> + >> + return 0; >> } >> >> static int sun4i_usb_phy_power_off(struct phy *_phy) >> { >> struct sun4i_usb_phy *phy = phy_get_drvdata(_phy); >> + struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy); >> + >> + if (!phy->vbus || !phy->regulator_on) >> + return 0; >> >> - if (phy->vbus) >> - regulator_disable(phy->vbus); >> + regulator_disable(phy->vbus); >> + phy->regulator_on = false; >> + >> + /* >> + * phy0 vbus typically slowly discharges, sometimes this causes the >> + * Vbus gpio to not trigger an edge irq on Vbus off, so force a rescan. >> + */ >> + if (phy->index == 0 && !data->phy0_poll) >> + mod_delayed_work(system_wq, &data->detect, POLL_TIME); >> >> return 0; >> } >> @@ -221,6 +343,61 @@ static struct phy_ops sun4i_usb_phy_ops = { >> .owner = THIS_MODULE, >> }; >> >> +static void sun4i_usb_phy0_id_vbus_det_scan(struct work_struct *work) >> +{ >> + struct sun4i_usb_phy_data *data = >> + container_of(work, struct sun4i_usb_phy_data, detect.work); >> + struct phy *phy0 = data->phys[0].phy; >> + int id_det, vbus_det, id_notify = 0, vbus_notify = 0; >> + >> + id_det = gpiod_get_value_cansleep(data->id_det_gpio); >> + vbus_det = gpiod_get_value_cansleep(data->vbus_det_gpio); >> + >> + mutex_lock(&phy0->mutex); >> + >> + if (!data->phy0_init) { >> + mutex_unlock(&phy0->mutex); >> + return; >> + } >> + >> + if (id_det != data->id_det) { >> + sun4i_usb_phy0_set_id_detect(phy0, id_det); >> + data->id_det = id_det; >> + id_notify = 1; >> + } >> + >> + if (vbus_det != data->vbus_det) { >> + sun4i_usb_phy0_set_vbus_detect(phy0, vbus_det); >> + data->vbus_det = vbus_det; >> + vbus_notify = 1; >> + } >> + >> + mutex_unlock(&phy0->mutex); >> + >> + if (id_notify) >> + extcon_set_cable_state(&data->extcon, >> + extcon_cable_name[EXTCON_USB_HOST], >> + !id_det); >> + >> + if (vbus_notify) >> + extcon_set_cable_state(&data->extcon, >> + extcon_cable_name[EXTCON_USB], >> + vbus_det); I don't want to use the legacy extcon API with string name. Instead, you can use the recommeded extcon_set_cable_state_() API as following: - Legacy extcon API : extern int extcon_set_cable_state(struct extcon_dev *edev, const char *cable_name, bool cable_state); - Recommended extcon API : extern int extcon_set_cable_state_(struct extcon_dev *edev, unsigned int id, bool cable_state); [1] extcon: Use the unique id for external connector instead of string - http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=2a9de9c0f08d61fbe3764a21d22d0b72df97d6ae >> + >> + if (data->phy0_poll) >> + queue_delayed_work(system_wq, &data->detect, POLL_TIME); >> +} >> + >> +static irqreturn_t sun4i_usb_phy0_id_vbus_det_irq(int irq, void *dev_id) >> +{ >> + struct sun4i_usb_phy_data *data = dev_id; >> + >> + /* vbus or id changed, let the pins settle and then scan them */ >> + mod_delayed_work(system_wq, &data->detect, DEBOUNCE_TIME); >> + >> + return IRQ_HANDLED; >> +} >> + >> static struct phy *sun4i_usb_phy_xlate(struct device *dev, >> struct of_phandle_args *args) >> { >> @@ -240,13 +417,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> struct phy_provider *phy_provider; >> bool dedicated_clocks; >> struct resource *res; >> - int i; >> + int i, ret; >> >> data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); >> if (!data) >> return -ENOMEM; >> >> mutex_init(&data->mutex); >> + INIT_DELAYED_WORK(&data->detect, sun4i_usb_phy0_id_vbus_det_scan); >> + data->extcon_cable_names[0] = extcon_cable_name[EXTCON_USB_HOST]; >> + data->extcon_cable_names[1] = extcon_cable_name[EXTCON_USB]; >> + data->extcon_cable_names[2] = NULL; >> + data->extcon.name = DRIVER_NAME; Don't need it because extcon_dev_register() set the device name automatically. You can check it on patch[3]. [3] http://git.kernel.org/cgit/linux/kernel/git/gregkh/char-misc.git/commit/?h=char-misc-next&id=71c3ffa5d23af0554c27010cf12710da9bf85950 >> + data->extcon.supported_cable = data->extcon_cable_names; You can use the devm_extcon_dev_allocate() to set the supported external connectors. >> + data->extcon.dev.parent = dev; Don't need it because extcon_dev_register() set the parent device automatically. The field of 'struct extcon_dev' shold be filled with extcon API without direct allocation. >> >> if (of_device_is_compatible(np, "allwinner,sun5i-a13-usb-phy")) >> data->num_phys = 2; >> @@ -269,6 +453,34 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> if (IS_ERR(data->base)) >> return PTR_ERR(data->base); >> >> + data->id_det_gpio = devm_gpiod_get(dev, "usb0_id_det", GPIOD_IN); >> + if (IS_ERR(data->id_det_gpio)) { >> + if (PTR_ERR(data->id_det_gpio) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + data->id_det_gpio = NULL; >> + } >> + >> + data->vbus_det_gpio = devm_gpiod_get(dev, "usb0_vbus_det", GPIOD_IN); >> + if (IS_ERR(data->vbus_det_gpio)) { >> + if (PTR_ERR(data->vbus_det_gpio) == -EPROBE_DEFER) >> + return -EPROBE_DEFER; >> + data->vbus_det_gpio = NULL; >> + } >> + >> + /* We either want both gpio pins or neither (when in host mode) */ >> + if (!data->id_det_gpio != !data->vbus_det_gpio) { >> + dev_err(dev, "failed to get id or vbus detect pin\n"); >> + return -ENODEV; >> + } >> + >> + if (data->id_det_gpio) { >> + ret = devm_extcon_dev_register(dev, &data->extcon); >> + if (ret) { >> + dev_err(dev, "failed to register extcon: %d\n", ret); >> + return ret; >> + } >> + } >> + >> for (i = 0; i < data->num_phys; i++) { >> struct sun4i_usb_phy *phy = data->phys + i; >> char name[16]; >> @@ -318,12 +530,54 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev) >> phy_set_drvdata(phy->phy, &data->phys[i]); >> } >> >> + data->id_det_irq = gpiod_to_irq(data->id_det_gpio); >> + data->vbus_det_irq = gpiod_to_irq(data->vbus_det_gpio); >> + if (data->id_det_irq < 0 || data->vbus_det_irq < 0) >> + data->phy0_poll = true; >> + >> + if (data->id_det_irq >= 0) { >> + ret = devm_request_irq(dev, data->id_det_irq, >> + sun4i_usb_phy0_id_vbus_det_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + "usb0-id-det", data); >> + if (ret) { >> + dev_err(dev, "Err requesting id-det-irq: %d\n", ret); >> + return ret; >> + } >> + } >> + >> + if (data->vbus_det_irq >= 0) { >> + ret = devm_request_irq(dev, data->vbus_det_irq, >> + sun4i_usb_phy0_id_vbus_det_irq, >> + IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING, >> + "usb0-vbus-det", data); >> + if (ret) { >> + dev_err(dev, "Err requesting vbus-det-irq: %d\n", ret); >> + return ret; >> + } >> + } >> + >> dev_set_drvdata(dev, data); >> phy_provider = devm_of_phy_provider_register(dev, sun4i_usb_phy_xlate); >> >> return PTR_ERR_OR_ZERO(phy_provider); >> } >> >> +static int sun4i_usb_phy_remove(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct sun4i_usb_phy_data *data = dev_get_drvdata(dev); >> + >> + if (data->id_det_irq >= 0) >> + devm_free_irq(dev, data->id_det_irq, data); >> + if (data->vbus_det_irq >= 0) >> + devm_free_irq(dev, data->vbus_det_irq, data); >> + >> + cancel_delayed_work_sync(&data->detect); >> + >> + return 0; >> +} >> + >> static const struct of_device_id sun4i_usb_phy_of_match[] = { >> { .compatible = "allwinner,sun4i-a10-usb-phy" }, >> { .compatible = "allwinner,sun5i-a13-usb-phy" }, >> @@ -335,9 +589,10 @@ MODULE_DEVICE_TABLE(of, sun4i_usb_phy_of_match); >> >> static struct platform_driver sun4i_usb_phy_driver = { >> .probe = sun4i_usb_phy_probe, >> + .remove = sun4i_usb_phy_remove, >> .driver = { >> .of_match_table = sun4i_usb_phy_of_match, >> - .name = "sun4i-usb-phy", >> + .name = DRIVER_NAME, >> } >> }; >> module_platform_driver(sun4i_usb_phy_driver); >> > Thanks, Chanwoo Choi