2015-04-15 13:35:35

by Ivan T. Ivanov

[permalink] [raw]
Subject: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect

On recent Qualcomm platforms VBUS and ID lines are not routed to
USB PHY LINK controller. Use extcon framework to receive connect
and disconnect ID and VBUS notification.

Signed-off-by: Ivan T. Ivanov <[email protected]>
---

Changes since v0 [1], as per Peter Chen suggestions:

* Moved external connector parsing code to ci_get_platdata()
* Moved external connector related variables to struct ci_hdrc_platform_data
* Rename ci_host_notifier() to ci_id_notifier()
* Fixed device bindings description
* Use select EXTCON framework, instead of depends on.

[1] https://lkml.org/lkml/2015/4/9/116

.../devicetree/bindings/usb/ci-hdrc-qcom.txt | 9 +++
drivers/usb/chipidea/Kconfig | 1 +
drivers/usb/chipidea/core.c | 87 ++++++++++++++++++++++
drivers/usb/chipidea/otg.c | 26 ++++++-
include/linux/usb/chipidea.h | 17 +++++
5 files changed, 139 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
index f2899b5..c635aca 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
@@ -7,6 +7,14 @@ Required properties:
- usb-phy: phandle for the PHY device
- dr_mode: Should be "peripheral"

+Optional properties:
+- extcon: phandles to external connector devices. First phandle
+ should point to external connector, which provide "USB"
+ cable events, the second should point to external connector
+ device, which provide "USB-HOST" cable events. If one of
+ the external connector devices is not required, empty <0>
+ phandle should be specified.
+
Examples:
gadget@f9a55000 {
compatible = "qcom,ci-hdrc";
@@ -14,4 +22,5 @@ Examples:
dr_mode = "peripheral";
interrupts = <0 134 0>;
usb-phy = <&usbphy0>;
+ extcon = <0>, <&usb_id>;
};
diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index 5ce3f1d..5619b8c 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -1,6 +1,7 @@
config USB_CHIPIDEA
tristate "ChipIdea Highspeed Dual Role Controller"
depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
+ select EXTCON
help
Say Y here if your system has a dual role high speed USB
controller based on ChipIdea silicon IP. Currently, only the
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 74fea4f..e1d495d 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -47,6 +47,7 @@
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/dma-mapping.h>
+#include <linux/extcon.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/module.h>
@@ -557,9 +558,39 @@ static irqreturn_t ci_irq(int irq, void *data)
return ret;
}

+static int ci_vbus_notifier(struct notifier_block *nb, unsigned long event,
+ void *ptr)
+{
+ struct ci_hdrc_cable *vbus = container_of(nb, struct ci_hdrc_cable, nb);
+
+ if (event)
+ vbus->state = true;
+ else
+ vbus->state = false;
+
+ return NOTIFY_DONE;
+}
+
+static int ci_id_notifier(struct notifier_block *nb, unsigned long event,
+ void *ptr)
+{
+ struct ci_hdrc_cable *id = container_of(nb, struct ci_hdrc_cable, nb);
+
+ if (event)
+ id->state = false;
+ else
+ id->state = true;
+
+ return NOTIFY_DONE;
+}
+
static int ci_get_platdata(struct device *dev,
struct ci_hdrc_platform_data *platdata)
{
+ struct extcon_dev *ext_vbus, *ext_id;
+ struct ci_hdrc_cable *cable;
+ int ret;
+
if (!platdata->phy_mode)
platdata->phy_mode = of_usb_get_phy_mode(dev->of_node);

@@ -591,6 +622,53 @@ static int ci_get_platdata(struct device *dev,
if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
platdata->flags |= CI_HDRC_FORCE_FULLSPEED;

+ ext_id = ERR_PTR(-ENODEV);
+ ext_vbus = ERR_PTR(-ENODEV);
+ if (of_property_read_bool(dev->of_node, "extcon")) {
+ /* Each one of them is not mandatory */
+ ext_vbus = extcon_get_edev_by_phandle(dev, 0);
+ if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
+ return PTR_ERR(ext_vbus);
+
+ ext_id = extcon_get_edev_by_phandle(dev, 1);
+ if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
+ return PTR_ERR(ext_id);
+ }
+
+ if (!IS_ERR(ext_vbus)) {
+ cable = &platdata->vbus_extcon;
+ cable->nb.notifier_call = ci_vbus_notifier;
+ ret = extcon_register_interest(&cable->conn, ext_vbus->name,
+ "USB", &cable->nb);
+ if (ret < 0) {
+ dev_err(dev, "register VBUS failed\n");
+ return ret;
+ }
+
+ ret = extcon_get_cable_state(ext_vbus, "USB");
+ if (ret)
+ cable->state = true;
+ else
+ cable->state = false;
+ }
+
+ if (!IS_ERR(ext_id)) {
+ cable = &platdata->id_extcon;
+ cable->nb.notifier_call = ci_id_notifier;
+ ret = extcon_register_interest(&cable->conn, ext_id->name,
+ "USB-HOST", &cable->nb);
+ if (ret < 0) {
+ dev_err(dev, "register ID failed\n");
+ return ret;
+ }
+
+ ret = extcon_get_cable_state(ext_id, "USB-HOST");
+ if (ret)
+ cable->state = false;
+ else
+ cable->state = true;
+ }
+
return 0;
}

@@ -845,6 +923,7 @@ deinit_phy:
static int ci_hdrc_remove(struct platform_device *pdev)
{
struct ci_hdrc *ci = platform_get_drvdata(pdev);
+ struct extcon_specific_cable_nb *cable_nb;

if (ci->supports_runtime_pm) {
pm_runtime_get_sync(&pdev->dev);
@@ -852,6 +931,14 @@ static int ci_hdrc_remove(struct platform_device *pdev)
pm_runtime_put_noidle(&pdev->dev);
}

+ cable_nb = &ci->platdata->id_extcon.conn;
+ if (cable_nb->edev)
+ extcon_unregister_interest(cable_nb);
+
+ cable_nb = &ci->platdata->vbus_extcon.conn;
+ if (cable_nb->edev)
+ extcon_unregister_interest(cable_nb);
+
dbg_remove_files(ci);
ci_role_destroy(ci);
ci_hdrc_enter_lpm(ci, true);
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index ad6c87a..34dd196 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -30,7 +30,31 @@
*/
u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask)
{
- return hw_read(ci, OP_OTGSC, mask);
+ struct ci_hdrc_cable *cable;
+ u32 val = hw_read(ci, OP_OTGSC, mask);
+
+ /*
+ * If using extcon framework for VBUS and/or ID signal
+ * detection overwrite OTGSC resiter value
+ */
+ cable = &ci->platdata->vbus_extcon;
+ if ((mask & OTGSC_BSV) && cable->conn.edev) {
+ if (cable->state)
+ val |= OTGSC_BSV;
+ else
+ val &= ~OTGSC_BSV;
+ }
+
+ cable = &ci->platdata->id_extcon;
+ if ((mask & OTGSC_ID) && cable->conn.edev) {
+ if (cable->state)
+ val |= OTGSC_ID;
+ else
+ val &= ~OTGSC_ID;
+ }
+
+ val &= mask;
+ return val;
}

/**
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index ab94f78..0eedaf6 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -5,8 +5,21 @@
#ifndef __LINUX_USB_CHIPIDEA_H
#define __LINUX_USB_CHIPIDEA_H

+#include <linux/extcon.h>
#include <linux/usb/otg.h>

+/**
+ * struct ci_hdrc_cable - structure for external connector cable state tracking
+ * @state: current state of the line
+ * @nb: hold event notification callback
+ * @conn: used for notification registration
+ */
+struct ci_hdrc_cable {
+ bool state;
+ struct notifier_block nb;
+ struct extcon_specific_cable_nb conn;
+};
+
struct ci_hdrc;
struct ci_hdrc_platform_data {
const char *name;
@@ -35,6 +48,10 @@ struct ci_hdrc_platform_data {
void (*notify_event) (struct ci_hdrc *ci, unsigned event);
struct regulator *reg_vbus;
bool tpl_support;
+
+ /* VBUS and ID signal state tracking, using extcon framework */
+ struct ci_hdrc_cable vbus_extcon;
+ struct ci_hdrc_cable id_extcon;
};

/* Default offset of capability registers */
--
1.9.1


2015-04-15 14:11:32

by Robert Baldyga

[permalink] [raw]
Subject: Re: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect

Hi Ivan,

On 04/15/2015 03:35 PM, Ivan T. Ivanov wrote:
> On recent Qualcomm platforms VBUS and ID lines are not routed to
> USB PHY LINK controller. Use extcon framework to receive connect
> and disconnect ID and VBUS notification.
>
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
>
> Changes since v0 [1], as per Peter Chen suggestions:
>
> * Moved external connector parsing code to ci_get_platdata()
> * Moved external connector related variables to struct ci_hdrc_platform_data
> * Rename ci_host_notifier() to ci_id_notifier()
> * Fixed device bindings description
> * Use select EXTCON framework, instead of depends on.
>
> [1] https://lkml.org/lkml/2015/4/9/116
>
> .../devicetree/bindings/usb/ci-hdrc-qcom.txt | 9 +++
> drivers/usb/chipidea/Kconfig | 1 +
> drivers/usb/chipidea/core.c | 87 ++++++++++++++++++++++
> drivers/usb/chipidea/otg.c | 26 ++++++-
> include/linux/usb/chipidea.h | 17 +++++
> 5 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> index f2899b5..c635aca 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> @@ -7,6 +7,14 @@ Required properties:
> - usb-phy: phandle for the PHY device
> - dr_mode: Should be "peripheral"
>
> +Optional properties:
> +- extcon: phandles to external connector devices. First phandle
> + should point to external connector, which provide "USB"
> + cable events, the second should point to external connector
> + device, which provide "USB-HOST" cable events. If one of
> + the external connector devices is not required, empty <0>
> + phandle should be specified.

Do you expect to have "USB" and "USB-HOST" notifiers supplied by two
different extcon drivers? It looks strange. I don't think so that we
ever will need to deal with such weird configuration.

> +
> Examples:
> gadget@f9a55000 {
> compatible = "qcom,ci-hdrc";
> @@ -14,4 +22,5 @@ Examples:
> dr_mode = "peripheral";
> interrupts = <0 134 0>;
> usb-phy = <&usbphy0>;
> + extcon = <0>, <&usb_id>;
> };
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 5ce3f1d..5619b8c 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -1,6 +1,7 @@
> config USB_CHIPIDEA
> tristate "ChipIdea Highspeed Dual Role Controller"
> depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> + select EXTCON
> help
> Say Y here if your system has a dual role high speed USB
> controller based on ChipIdea silicon IP. Currently, only the
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 74fea4f..e1d495d 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -47,6 +47,7 @@
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/dma-mapping.h>
> +#include <linux/extcon.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/module.h>
> @@ -557,9 +558,39 @@ static irqreturn_t ci_irq(int irq, void *data)
> return ret;
> }
>
> +static int ci_vbus_notifier(struct notifier_block *nb, unsigned long event,
> + void *ptr)
> +{
> + struct ci_hdrc_cable *vbus = container_of(nb, struct ci_hdrc_cable, nb);
> +
> + if (event)
> + vbus->state = true;
> + else
> + vbus->state = false;
> +
> + return NOTIFY_DONE;
> +}

Actually it's not true that "USB" cable state is equal to VBUS state.
"USB" and "USB-HOST" are mutually exclusive, and when you have ID=0,
which means "USB-HOST" is connected, "USB cable will be seen as
disconnected even when VBUS=1.

We are currently discussing how to pass VBUS state to USB OTG drivers.
You can find discussion here:
http://www.spinics.net/lists/linux-usb/msg123895.html

Best regards,
Robert Baldyga

> +
> +static int ci_id_notifier(struct notifier_block *nb, unsigned long event,
> + void *ptr)
> +{
> + struct ci_hdrc_cable *id = container_of(nb, struct ci_hdrc_cable, nb);
> +
> + if (event)
> + id->state = false;
> + else
> + id->state = true;
> +
> + return NOTIFY_DONE;
> +}
> +
> static int ci_get_platdata(struct device *dev,
> struct ci_hdrc_platform_data *platdata)
> {
> + struct extcon_dev *ext_vbus, *ext_id;
> + struct ci_hdrc_cable *cable;
> + int ret;
> +
> if (!platdata->phy_mode)
> platdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
>
> @@ -591,6 +622,53 @@ static int ci_get_platdata(struct device *dev,
> if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
> platdata->flags |= CI_HDRC_FORCE_FULLSPEED;
>
> + ext_id = ERR_PTR(-ENODEV);
> + ext_vbus = ERR_PTR(-ENODEV);
> + if (of_property_read_bool(dev->of_node, "extcon")) {
> + /* Each one of them is not mandatory */
> + ext_vbus = extcon_get_edev_by_phandle(dev, 0);
> + if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> + return PTR_ERR(ext_vbus);
> +
> + ext_id = extcon_get_edev_by_phandle(dev, 1);
> + if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> + return PTR_ERR(ext_id);
> + }
> +
> + if (!IS_ERR(ext_vbus)) {
> + cable = &platdata->vbus_extcon;
> + cable->nb.notifier_call = ci_vbus_notifier;
> + ret = extcon_register_interest(&cable->conn, ext_vbus->name,
> + "USB", &cable->nb);
> + if (ret < 0) {
> + dev_err(dev, "register VBUS failed\n");
> + return ret;
> + }
> +
> + ret = extcon_get_cable_state(ext_vbus, "USB");
> + if (ret)
> + cable->state = true;
> + else
> + cable->state = false;
> + }
> +
> + if (!IS_ERR(ext_id)) {
> + cable = &platdata->id_extcon;
> + cable->nb.notifier_call = ci_id_notifier;
> + ret = extcon_register_interest(&cable->conn, ext_id->name,
> + "USB-HOST", &cable->nb);
> + if (ret < 0) {
> + dev_err(dev, "register ID failed\n");
> + return ret;
> + }
> +
> + ret = extcon_get_cable_state(ext_id, "USB-HOST");
> + if (ret)
> + cable->state = false;
> + else
> + cable->state = true;
> + }
> +
> return 0;
> }
>
> @@ -845,6 +923,7 @@ deinit_phy:
> static int ci_hdrc_remove(struct platform_device *pdev)
> {
> struct ci_hdrc *ci = platform_get_drvdata(pdev);
> + struct extcon_specific_cable_nb *cable_nb;
>
> if (ci->supports_runtime_pm) {
> pm_runtime_get_sync(&pdev->dev);
> @@ -852,6 +931,14 @@ static int ci_hdrc_remove(struct platform_device *pdev)
> pm_runtime_put_noidle(&pdev->dev);
> }
>
> + cable_nb = &ci->platdata->id_extcon.conn;
> + if (cable_nb->edev)
> + extcon_unregister_interest(cable_nb);
> +
> + cable_nb = &ci->platdata->vbus_extcon.conn;
> + if (cable_nb->edev)
> + extcon_unregister_interest(cable_nb);
> +
> dbg_remove_files(ci);
> ci_role_destroy(ci);
> ci_hdrc_enter_lpm(ci, true);
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index ad6c87a..34dd196 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -30,7 +30,31 @@
> */
> u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask)
> {
> - return hw_read(ci, OP_OTGSC, mask);
> + struct ci_hdrc_cable *cable;
> + u32 val = hw_read(ci, OP_OTGSC, mask);
> +
> + /*
> + * If using extcon framework for VBUS and/or ID signal
> + * detection overwrite OTGSC resiter value
> + */
> + cable = &ci->platdata->vbus_extcon;
> + if ((mask & OTGSC_BSV) && cable->conn.edev) {
> + if (cable->state)
> + val |= OTGSC_BSV;
> + else
> + val &= ~OTGSC_BSV;
> + }
> +
> + cable = &ci->platdata->id_extcon;
> + if ((mask & OTGSC_ID) && cable->conn.edev) {
> + if (cable->state)
> + val |= OTGSC_ID;
> + else
> + val &= ~OTGSC_ID;
> + }
> +
> + val &= mask;
> + return val;
> }
>
> /**
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index ab94f78..0eedaf6 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -5,8 +5,21 @@
> #ifndef __LINUX_USB_CHIPIDEA_H
> #define __LINUX_USB_CHIPIDEA_H
>
> +#include <linux/extcon.h>
> #include <linux/usb/otg.h>
>
> +/**
> + * struct ci_hdrc_cable - structure for external connector cable state tracking
> + * @state: current state of the line
> + * @nb: hold event notification callback
> + * @conn: used for notification registration
> + */
> +struct ci_hdrc_cable {
> + bool state;
> + struct notifier_block nb;
> + struct extcon_specific_cable_nb conn;
> +};
> +
> struct ci_hdrc;
> struct ci_hdrc_platform_data {
> const char *name;
> @@ -35,6 +48,10 @@ struct ci_hdrc_platform_data {
> void (*notify_event) (struct ci_hdrc *ci, unsigned event);
> struct regulator *reg_vbus;
> bool tpl_support;
> +
> + /* VBUS and ID signal state tracking, using extcon framework */
> + struct ci_hdrc_cable vbus_extcon;
> + struct ci_hdrc_cable id_extcon;
> };
>
> /* Default offset of capability registers */
> --
> 1.9.1

2015-04-15 14:54:46

by Ivan T. Ivanov

[permalink] [raw]
Subject: Re: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect


Hi Robert,

On Wed, 2015-04-15 at 16:11 +0200, Robert Baldyga wrote:
> Hi Ivan,
>
> On 04/15/2015 03:35 PM, Ivan T. Ivanov wrote:
> > On recent Qualcomm platforms VBUS and ID lines are not routed to
> > USB PHY LINK controller. Use extcon framework to receive connect
> > and disconnect ID and VBUS notification.
> >
> > Signed-off-by: Ivan T. Ivanov [email protected]>
> > ---
> >
> > Changes since v0 [1], as per Peter Chen suggestions:
> >
> > * Moved external connector parsing code to ci_get_platdata()
> > * Moved external connector related variables to struct ci_hdrc_platform_data
> > * Rename ci_host_notifier() to ci_id_notifier()
> > * Fixed device bindings description
> > * Use select EXTCON framework, instead of depends on.
> >
> > [1] https://lkml.org/lkml/2015/4/9/116
> >
> > .../devicetree/bindings/usb/ci-hdrc-qcom.txt | 9 +++
> > drivers/usb/chipidea/Kconfig | 1 +
> > drivers/usb/chipidea/core.c | 87 ++++++++++++++++++++++
> > drivers/usb/chipidea/otg.c | 26 ++++++-
> > include/linux/usb/chipidea.h | 17 +++++
> > 5 files changed, 139 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> > index f2899b5..c635aca 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> > @@ -7,6 +7,14 @@ Required properties:
> > - usb-phy: phandle for the PHY device
> > - dr_mode: Should be "peripheral"
> >
> > +Optional properties:
> > +- extcon: phandles to external connector devices. First phandle
> > + should point to external connector, which provide "USB"
> > + cable events, the second should point to external connector
> > + device, which provide "USB-HOST" cable events. If one of
> > + the external connector devices is not required, empty <0>
> > + phandle should be specified.
>
> Do you expect to have "USB" and "USB-HOST" notifiers supplied by two
> different extcon drivers? It looks strange. I don't think so that we
> ever will need to deal with such weird configuration.

Yes. That is what I have today on my desk.

>
> > +
> > Examples:
> >        gadget@f9a55000 {
> > compatible = "qcom,ci-hdrc";
> > @@ -14,4 +22,5 @@ Examples:
> > dr_mode = "peripheral";
> > interrupts = <0 134 0>;
> > usb-phy = <&usbphy0>;
> > + extcon = <0>, <&usb_id>;
> > };
> > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> > index 5ce3f1d..5619b8c 100644
> > --- a/drivers/usb/chipidea/Kconfig
> > +++ b/drivers/usb/chipidea/Kconfig
> > @@ -1,6 +1,7 @@
> > config USB_CHIPIDEA
> > tristate "ChipIdea Highspeed Dual Role Controller"
> > depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) ||
> > (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> > + select EXTCON
> > help
> > Say Y here if your system has a dual role high speed USB
> > controller based on ChipIdea silicon IP. Currently, only the
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index 74fea4f..e1d495d 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -47,6 +47,7 @@
> > #include <linux/delay.h>
> > #include <linux/device.h>
> > #include <linux/dma-mapping.h>
> > +#include <linux/extcon.h>
> > #include <linux/phy/phy.h>
> > #include <linux/platform_device.h>
> > #include <linux/module.h>
> > @@ -557,9 +558,39 @@ static irqreturn_t ci_irq(int irq, void *data)
> > return ret;
> > }
> >
> > +static int ci_vbus_notifier(struct notifier_block *nb, unsigned long event,
> > + void *ptr)
> > +{
> > + struct ci_hdrc_cable *vbus = container_of(nb, struct ci_hdrc_cable, nb);
> > +
> > + if (event)
> > + vbus->state = true;
> > + else
> > + vbus->state = false;
> > +
> > + return NOTIFY_DONE;
> > +}
>
> Actually it's not true that "USB" cable state is equal to VBUS state.

But this is how it supposed to work right now, no?

I have to admit that the naming is really confusing.

> "USB" and "USB-HOST" are mutually exclusive, and when you have ID=0,
> which means "USB-HOST" is connected, "USB cable will be seen as
> disconnected even when VBUS=1.
>
> We are currently discussing how to pass VBUS state to USB OTG drivers.
> You can find discussion here:
> http://www.spinics.net/lists/linux-usb/msg123895.html

Sure, I am following this discussion. When it settles down, I can change
this as appropriate, until then this is what we will have in 4.1.

It will be messy refactoring, I think. All extcon providers and consumers
are using strings to denote cable names. Not to mention that extcon-class
is using "USB-Host", while all others are using "USB-HOST".

Regards,
Ivan

2015-04-16 07:49:33

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect

On Wed, Apr 15, 2015 at 04:35:15PM +0300, Ivan T. Ivanov wrote:
> On recent Qualcomm platforms VBUS and ID lines are not routed to
> USB PHY LINK controller. Use extcon framework to receive connect
> and disconnect ID and VBUS notification.
>
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
>
> Changes since v0 [1], as per Peter Chen suggestions:
>
> * Moved external connector parsing code to ci_get_platdata()
> * Moved external connector related variables to struct ci_hdrc_platform_data
> * Rename ci_host_notifier() to ci_id_notifier()
> * Fixed device bindings description
> * Use select EXTCON framework, instead of depends on.

Hi Chanwoo,

It seems the CONFIG_EXTCON is better as a hide config.
See below thread for GENERIC_PHY.

http://marc.info/?l=linux-kernel&m=142902133811818&w=2

Peter

2015-05-26 07:47:33

by Ivan T. Ivanov

[permalink] [raw]
Subject: Re: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect


On Wed, 2015-04-15 at 16:35 +0300, Ivan T. Ivanov wrote:
> On recent Qualcomm platforms VBUS and ID lines are not routed to
> USB PHY LINK controller. Use extcon framework to receive connect
> and disconnect ID and VBUS notification.
>
> Signed-off-by: Ivan T. Ivanov [email protected]>
> ---
>
> Changes since v0 [1], as per Peter Chen suggestions:
>
> * Moved external connector parsing code to ci_get_platdata()
> * Moved external connector related variables to struct ci_hdrc_platform_data
> * Rename ci_host_notifier() to ci_id_notifier()
> * Fixed device bindings description
> * Use select EXTCON framework, instead of depends on.
>
> [1] https://lkml.org/lkml/2015/4/9/116

Hi Peter,

Did you have any further comments on this patch
or what is your plan about it.

Regards,
Ivan

2015-05-26 09:15:41

by Peter Chen

[permalink] [raw]
Subject: Re: [PATCH v2] usb: chipidea: Use extcon framework for VBUS and ID detect

On Wed, Apr 15, 2015 at 04:35:15PM +0300, Ivan T. Ivanov wrote:
> On recent Qualcomm platforms VBUS and ID lines are not routed to
> USB PHY LINK controller. Use extcon framework to receive connect
> and disconnect ID and VBUS notification.
>
> Signed-off-by: Ivan T. Ivanov <[email protected]>
> ---
>
> Changes since v0 [1], as per Peter Chen suggestions:
>
> * Moved external connector parsing code to ci_get_platdata()
> * Moved external connector related variables to struct ci_hdrc_platform_data
> * Rename ci_host_notifier() to ci_id_notifier()
> * Fixed device bindings description
> * Use select EXTCON framework, instead of depends on.
>
> [1] https://lkml.org/lkml/2015/4/9/116
>
> .../devicetree/bindings/usb/ci-hdrc-qcom.txt | 9 +++
> drivers/usb/chipidea/Kconfig | 1 +
> drivers/usb/chipidea/core.c | 87 ++++++++++++++++++++++
> drivers/usb/chipidea/otg.c | 26 ++++++-
> include/linux/usb/chipidea.h | 17 +++++
> 5 files changed, 139 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> index f2899b5..c635aca 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qcom.txt
> @@ -7,6 +7,14 @@ Required properties:
> - usb-phy: phandle for the PHY device
> - dr_mode: Should be "peripheral"
>
> +Optional properties:
> +- extcon: phandles to external connector devices. First phandle
> + should point to external connector, which provide "USB"
> + cable events, the second should point to external connector
> + device, which provide "USB-HOST" cable events. If one of
> + the external connector devices is not required, empty <0>
> + phandle should be specified.
> +
> Examples:
> gadget@f9a55000 {
> compatible = "qcom,ci-hdrc";
> @@ -14,4 +22,5 @@ Examples:
> dr_mode = "peripheral";
> interrupts = <0 134 0>;
> usb-phy = <&usbphy0>;
> + extcon = <0>, <&usb_id>;
> };
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index 5ce3f1d..5619b8c 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -1,6 +1,7 @@
> config USB_CHIPIDEA
> tristate "ChipIdea Highspeed Dual Role Controller"
> depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> + select EXTCON
> help
> Say Y here if your system has a dual role high speed USB
> controller based on ChipIdea silicon IP. Currently, only the
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 74fea4f..e1d495d 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -47,6 +47,7 @@
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/dma-mapping.h>
> +#include <linux/extcon.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/module.h>
> @@ -557,9 +558,39 @@ static irqreturn_t ci_irq(int irq, void *data)
> return ret;
> }
>
> +static int ci_vbus_notifier(struct notifier_block *nb, unsigned long event,
> + void *ptr)
> +{
> + struct ci_hdrc_cable *vbus = container_of(nb, struct ci_hdrc_cable, nb);
> +
> + if (event)
> + vbus->state = true;
> + else
> + vbus->state = false;
> +
> + return NOTIFY_DONE;
> +}
> +
> +static int ci_id_notifier(struct notifier_block *nb, unsigned long event,
> + void *ptr)
> +{
> + struct ci_hdrc_cable *id = container_of(nb, struct ci_hdrc_cable, nb);
> +
> + if (event)
> + id->state = false;
> + else
> + id->state = true;
> +
> + return NOTIFY_DONE;
> +}
> +
> static int ci_get_platdata(struct device *dev,
> struct ci_hdrc_platform_data *platdata)
> {
> + struct extcon_dev *ext_vbus, *ext_id;
> + struct ci_hdrc_cable *cable;
> + int ret;
> +
> if (!platdata->phy_mode)
> platdata->phy_mode = of_usb_get_phy_mode(dev->of_node);
>
> @@ -591,6 +622,53 @@ static int ci_get_platdata(struct device *dev,
> if (of_usb_get_maximum_speed(dev->of_node) == USB_SPEED_FULL)
> platdata->flags |= CI_HDRC_FORCE_FULLSPEED;
>
> + ext_id = ERR_PTR(-ENODEV);
> + ext_vbus = ERR_PTR(-ENODEV);
> + if (of_property_read_bool(dev->of_node, "extcon")) {
> + /* Each one of them is not mandatory */
> + ext_vbus = extcon_get_edev_by_phandle(dev, 0);
> + if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
> + return PTR_ERR(ext_vbus);
> +
> + ext_id = extcon_get_edev_by_phandle(dev, 1);
> + if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
> + return PTR_ERR(ext_id);
> + }
> +
> + if (!IS_ERR(ext_vbus)) {
> + cable = &platdata->vbus_extcon;
> + cable->nb.notifier_call = ci_vbus_notifier;
> + ret = extcon_register_interest(&cable->conn, ext_vbus->name,
> + "USB", &cable->nb);
> + if (ret < 0) {
> + dev_err(dev, "register VBUS failed\n");
> + return ret;
> + }
> +
> + ret = extcon_get_cable_state(ext_vbus, "USB");
> + if (ret)
> + cable->state = true;
> + else
> + cable->state = false;
> + }
> +
> + if (!IS_ERR(ext_id)) {
> + cable = &platdata->id_extcon;
> + cable->nb.notifier_call = ci_id_notifier;
> + ret = extcon_register_interest(&cable->conn, ext_id->name,
> + "USB-HOST", &cable->nb);
> + if (ret < 0) {
> + dev_err(dev, "register ID failed\n");
> + return ret;
> + }
> +
> + ret = extcon_get_cable_state(ext_id, "USB-HOST");
> + if (ret)
> + cable->state = false;
> + else
> + cable->state = true;
> + }
> +
> return 0;
> }
>
> @@ -845,6 +923,7 @@ deinit_phy:
> static int ci_hdrc_remove(struct platform_device *pdev)
> {
> struct ci_hdrc *ci = platform_get_drvdata(pdev);
> + struct extcon_specific_cable_nb *cable_nb;
>
> if (ci->supports_runtime_pm) {
> pm_runtime_get_sync(&pdev->dev);
> @@ -852,6 +931,14 @@ static int ci_hdrc_remove(struct platform_device *pdev)
> pm_runtime_put_noidle(&pdev->dev);
> }
>
> + cable_nb = &ci->platdata->id_extcon.conn;
> + if (cable_nb->edev)
> + extcon_unregister_interest(cable_nb);
> +
> + cable_nb = &ci->platdata->vbus_extcon.conn;
> + if (cable_nb->edev)
> + extcon_unregister_interest(cable_nb);
> +
> dbg_remove_files(ci);
> ci_role_destroy(ci);
> ci_hdrc_enter_lpm(ci, true);
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index ad6c87a..34dd196 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -30,7 +30,31 @@
> */
> u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask)
> {
> - return hw_read(ci, OP_OTGSC, mask);
> + struct ci_hdrc_cable *cable;
> + u32 val = hw_read(ci, OP_OTGSC, mask);
> +
> + /*
> + * If using extcon framework for VBUS and/or ID signal
> + * detection overwrite OTGSC resiter value
> + */
> + cable = &ci->platdata->vbus_extcon;
> + if ((mask & OTGSC_BSV) && cable->conn.edev) {
> + if (cable->state)
> + val |= OTGSC_BSV;
> + else
> + val &= ~OTGSC_BSV;
> + }
> +
> + cable = &ci->platdata->id_extcon;
> + if ((mask & OTGSC_ID) && cable->conn.edev) {
> + if (cable->state)
> + val |= OTGSC_ID;
> + else
> + val &= ~OTGSC_ID;
> + }
> +
> + val &= mask;
> + return val;
> }
>
> /**
> diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
> index ab94f78..0eedaf6 100644
> --- a/include/linux/usb/chipidea.h
> +++ b/include/linux/usb/chipidea.h
> @@ -5,8 +5,21 @@
> #ifndef __LINUX_USB_CHIPIDEA_H
> #define __LINUX_USB_CHIPIDEA_H
>
> +#include <linux/extcon.h>
> #include <linux/usb/otg.h>
>
> +/**
> + * struct ci_hdrc_cable - structure for external connector cable state tracking
> + * @state: current state of the line
> + * @nb: hold event notification callback
> + * @conn: used for notification registration
> + */
> +struct ci_hdrc_cable {
> + bool state;
> + struct notifier_block nb;
> + struct extcon_specific_cable_nb conn;
> +};
> +
> struct ci_hdrc;
> struct ci_hdrc_platform_data {
> const char *name;
> @@ -35,6 +48,10 @@ struct ci_hdrc_platform_data {
> void (*notify_event) (struct ci_hdrc *ci, unsigned event);
> struct regulator *reg_vbus;
> bool tpl_support;
> +
> + /* VBUS and ID signal state tracking, using extcon framework */
> + struct ci_hdrc_cable vbus_extcon;
> + struct ci_hdrc_cable id_extcon;
> };
>
> /* Default offset of capability registers */
> --
> 1.9.1
>

I have not found the handler for id/vbus change, eg, when the vbus
has changed, the ci_handle_vbus_change should be called?

Chanwoo Choi has some updates for id/vbus, make sure you work well
for his new patch set[1].

[1]: https://lkml.org/lkml/2015/5/22/346
--

Best Regards,
Peter Chen