Re: [RFC][PATCH 1/3] phy: phy-hi6220-usb: Wire up extconn support to hikey's phy driver

[John Youn <John.Youn@synopsys.com>]

On 12/1/2016 12:12 PM, John Stultz wrote:
> On Thu, Dec 1, 2016 at 12:23 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote:
>> Hi,
>>
>> On Wednesday 23 November 2016 09:16 AM, John Stultz wrote:
>>> This wires extconn support to hikey's phy driver, and
>>> connects it to the usb UDC layer via a usb_phy structure.
>>>
>>> Not sure if this is the right way to connect phy -> UDC,
>>> but I'm lacking a clear example.
>>>
>>> Cc: Wei Xu <xuwei5@hisilicon.com>
>>> Cc: Guodong Xu <guodong.xu@linaro.org>
>>> Cc: Amit Pundir <amit.pundir@linaro.org>
>>> Cc: Rob Herring <robh+dt@kernel.org>
>>> Cc: John Youn <johnyoun@synopsys.com>
>>> Cc: Douglas Anderson <dianders@chromium.org>
>>> Cc: Chen Yu <chenyu56@huawei.com>
>>> Cc: Kishon Vijay Abraham I <kishon@ti.com>
>>> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
>>> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>>> Cc: linux-usb@vger.kernel.org
>>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>>> ---
>>>  arch/arm64/boot/dts/hisilicon/hi6220.dtsi |  11 +++
>>>  drivers/phy/Kconfig                       |   2 +
>>>  drivers/phy/phy-hi6220-usb.c              | 139 ++++++++++++++++++++++++++++++
>>>  3 files changed, 152 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> index 17839db..171fbb2 100644
>>> --- a/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> +++ b/arch/arm64/boot/dts/hisilicon/hi6220.dtsi
>>> @@ -732,10 +732,21 @@
>>>                       regulator-always-on;
>>>               };
>>>
>>> +             usb_vbus: usb-vbus {
>>> +                     compatible = "linux,extcon-usb-gpio";
>>> +                     id-gpio = <&gpio2 6 1>;
>>> +             };
>>> +
>>> +             usb_id: usb-id {
>>> +                     compatible = "linux,extcon-usb-gpio";
>>> +                     id-gpio = <&gpio2 5 1>;
>>> +             };
>>> +
>>>               usb_phy: usbphy {
>>>                       compatible = "hisilicon,hi6220-usb-phy";
>>>                       #phy-cells = <0>;
>>>                       phy-supply = <&fixed_5v_hub>;
>>> +                     extcon = <&usb_vbus>, <&usb_id>;
>>>                       hisilicon,peripheral-syscon = <&sys_ctrl>;
>>>               };
>>>
>>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>>> index fe00f91..76f4f17 100644
>>> --- a/drivers/phy/Kconfig
>>> +++ b/drivers/phy/Kconfig
>>> @@ -254,8 +254,10 @@ config PHY_MT65XX_USB3
>>>  config PHY_HI6220_USB
>>>       tristate "hi6220 USB PHY support"
>>>       depends on (ARCH_HISI && ARM64) || COMPILE_TEST
>>> +     depends on EXTCON
>>>       select GENERIC_PHY
>>>       select MFD_SYSCON
>>> +     select USB_PHY
>>>       help
>>>         Enable this to support the HISILICON HI6220 USB PHY.
>>>
>>> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
>>> index b2141cb..89d8475 100644
>>> --- a/drivers/phy/phy-hi6220-usb.c
>>> +++ b/drivers/phy/phy-hi6220-usb.c
>>> @@ -12,7 +12,12 @@
>>>  #include <linux/module.h>
>>>  #include <linux/platform_device.h>
>>>  #include <linux/phy/phy.h>
>>> +#include <linux/usb/phy_companion.h>
>>> +#include <linux/usb/otg.h>
>>> +#include <linux/usb/gadget.h>
>>> +#include <linux/usb/phy.h>
>>>  #include <linux/regmap.h>
>>> +#include <linux/extcon.h>
>>>
>>>  #define SC_PERIPH_CTRL4                      0x00c
>>>
>>> @@ -44,9 +49,21 @@
>>>
>>>  #define EYE_PATTERN_PARA             0x7053348c
>>>
>>> +
>>> +struct hi6220_usb_cable {
>>> +     struct notifier_block           nb;
>>> +     struct extcon_dev               *extcon;
>>> +     int state;
>>> +};
>>> +
>>>  struct hi6220_priv {
>>>       struct regmap *reg;
>>>       struct device *dev;
>>> +     struct usb_phy phy;
>>> +
>>> +     struct delayed_work work;
>>> +     struct hi6220_usb_cable vbus;
>>> +     struct hi6220_usb_cable id;
>>>  };
>>>
>>>  static void hi6220_phy_init(struct hi6220_priv *priv)
>>> @@ -112,23 +129,85 @@ static int hi6220_phy_exit(struct phy *phy)
>>>       return hi6220_phy_setup(priv, false);
>>>  }
>>>
>>> +
>>>  static struct phy_ops hi6220_phy_ops = {
>>>       .init           = hi6220_phy_start,
>>>       .exit           = hi6220_phy_exit,
>>>       .owner          = THIS_MODULE,
>>>  };
>>>
>>> +static void hi6220_detect_work(struct work_struct *work)
>>> +{
>>> +     struct hi6220_priv *priv =
>>> +             container_of(to_delayed_work(work), struct hi6220_priv, work);
>>> +     struct usb_otg *otg = priv->phy.otg;
>>> +
>>> +     if (!IS_ERR(priv->vbus.extcon))
>>> +             priv->vbus.state = extcon_get_cable_state_(priv->vbus.extcon,
>>> +                                                              EXTCON_USB);
>>> +     if (!IS_ERR(priv->id.extcon))
>>> +             priv->id.state = extcon_get_cable_state_(priv->id.extcon,
>>> +                                                      EXTCON_USB_HOST);
>>> +     if (otg->gadget) {
>>> +             if (priv->id.state)
>>> +                     usb_gadget_vbus_connect(otg->gadget);
>>> +             else
>>> +                     usb_gadget_vbus_disconnect(otg->gadget);
>>> +     }
>>> +}
>>> +
>>> +static int hi6220_otg_vbus_notifier(struct notifier_block *nb,
>>> +                                 unsigned long event, void *ptr)
>>> +{
>>> +     struct hi6220_usb_cable *vbus = container_of(nb,
>>> +                                             struct hi6220_usb_cable, nb);
>>> +     struct hi6220_priv *priv = container_of(vbus,
>>> +                                             struct hi6220_priv, vbus);
>>> +
>>> +     schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>> +     return NOTIFY_DONE;
>>> +}
>>> +
>>> +static int hi6220_otg_id_notifier(struct notifier_block *nb,
>>> +                               unsigned long event, void *ptr)
>>> +{
>>> +     struct hi6220_usb_cable *id = container_of(nb,
>>> +                                             struct hi6220_usb_cable, nb);
>>> +     struct hi6220_priv *priv = container_of(id, struct hi6220_priv, id);
>>> +
>>> +     schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>> +     return NOTIFY_DONE;
>>> +}
>>> +
>>> +static int hi6220_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
>>> +{
>>> +     otg->host = host;
>>> +     return 0;
>>> +}
>>> +
>>> +static int hi6220_otg_set_peripheral(struct usb_otg *otg,
>>> +                                     struct usb_gadget *gadget)
>>> +{
>>> +     otg->gadget = gadget;
>>> +     return 0;
>>> +}
>>> +
>>>  static int hi6220_phy_probe(struct platform_device *pdev)
>>>  {
>>>       struct phy_provider *phy_provider;
>>>       struct device *dev = &pdev->dev;
>>>       struct phy *phy;
>>> +     struct usb_otg *otg;
>>>       struct hi6220_priv *priv;
>>> +     struct extcon_dev *ext_id, *ext_vbus;
>>> +     int ret;
>>>
>>>       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>       if (!priv)
>>>               return -ENOMEM;
>>>
>>> +     INIT_DELAYED_WORK(&priv->work, hi6220_detect_work);
>>> +
>>>       priv->dev = dev;
>>>       priv->reg = syscon_regmap_lookup_by_phandle(dev->of_node,
>>>                                       "hisilicon,peripheral-syscon");
>>> @@ -137,13 +216,73 @@ static int hi6220_phy_probe(struct platform_device *pdev)
>>>               return PTR_ERR(priv->reg);
>>>       }
>>>
>>> +
>>> +     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(&pdev->dev, 0);
>>> +             if (IS_ERR(ext_vbus) && PTR_ERR(ext_vbus) != -ENODEV)
>>> +                     return PTR_ERR(ext_vbus);
>>> +
>>> +             ext_id = extcon_get_edev_by_phandle(&pdev->dev, 1);
>>> +             if (IS_ERR(ext_id) && PTR_ERR(ext_id) != -ENODEV)
>>> +                     return PTR_ERR(ext_id);
>>> +     }
>>> +
>>> +     priv->vbus.extcon = ext_vbus;
>>> +     if (!IS_ERR(ext_vbus)) {
>>> +             priv->vbus.nb.notifier_call = hi6220_otg_vbus_notifier;
>>> +             ret = extcon_register_notifier(ext_vbus, EXTCON_USB,
>>> +                                             &priv->vbus.nb);
>>> +             if (ret < 0) {
>>> +                     dev_err(&pdev->dev, "register VBUS notifier failed\n");
>>> +                     return ret;
>>> +             }
>>> +
>>> +             priv->vbus.state = extcon_get_cable_state_(ext_vbus,
>>> +                                                             EXTCON_USB);
>>> +     }
>>> +
>>> +     priv->id.extcon = ext_id;
>>> +     if (!IS_ERR(ext_id)) {
>>> +             priv->id.nb.notifier_call = hi6220_otg_id_notifier;
>>> +             ret = extcon_register_notifier(ext_id, EXTCON_USB_HOST,
>>> +                                             &priv->id.nb);
>>> +             if (ret < 0) {
>>> +                     dev_err(&pdev->dev, "register ID notifier failed\n");
>>> +                     return ret;
>>> +             }
>>> +
>>> +             priv->id.state = extcon_get_cable_state_(ext_id,
>>> +                                                      EXTCON_USB_HOST);
>>> +     }
>>> +
>>>       hi6220_phy_init(priv);
>>>
>>>       phy = devm_phy_create(dev, NULL, &hi6220_phy_ops);
>>>       if (IS_ERR(phy))
>>>               return PTR_ERR(phy);
>>>
>>> +     otg = devm_kzalloc(&pdev->dev, sizeof(*otg), GFP_KERNEL);
>>> +     if (!otg)
>>> +             return -ENOMEM;
>>> +
>>> +     priv->dev = &pdev->dev;
>>> +     priv->phy.dev = priv->dev;
>>> +     priv->phy.label = "hi6220_usb_phy";
>>> +     priv->phy.otg = otg;
>>> +     priv->phy.type = USB_PHY_TYPE_USB2;
>>> +     otg->set_host = hi6220_otg_set_host;
>>> +     otg->set_peripheral = hi6220_otg_set_peripheral;
>>> +     otg->usb_phy = &priv->phy;
>>> +
>>> +     platform_set_drvdata(pdev, priv);
>>> +
>>>       phy_set_drvdata(phy, priv);
>>> +
>>> +     usb_add_phy_dev(&priv->phy);
>>
>> This would be like using two independent phy infrastructure :-( Should we just
>> handle the extcon events in USB driver?
> 
> Yes. I was told that the older hikey usb-phy driver got nacked since
> new drivers should use the generic phy infrastructure, so I agree that
> registering a usb-phy device in a generic phy driver felt improper. My
> trouble was that its not obvious what is the way it should be done.

I'm not really sure how this should be solved either. But it seems
wrong to use both PHY frameworks like that.

> 
> So as for adding extcon events to the usb driver, do you have a
> pointer to a driver that does it in a way that folks like?

I would be ok with this. Not sure if other folks might have
objections.

Take a look in the probe function in dwc2/platform.c where we handle
things similar to this.

Regards,
John