Re: [RFCv2 usb-next 2/3] usb: host: add a generic platform USB roothub driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Tue, Jul 25, 2017 at 8:40 AM, Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx> wrote:
> On Thu, 2017-07-13 at 12:59 +0200, Martin Blumenstingl wrote:
>> Many SoC platforms have separate devices for the USB PHY which are
>> registered through the generic PHY framework. These PHYs have to be
>> enabled to make the USB controller actually work. They also have to be
>> disabled again on shutdown/suspend.
>>
>> Currently (at least) the following HCI platform drivers are using custom
>> code to obtain all PHYs via devicetree for the roothub/controller and
>> disable/enable them when required:
>> - ehci-platform.c has ehci_platform_power_{on,off}
>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>> - ohci-platform.c has ohci_platform_power_{on,off}
>>
>> These drivers are not using the generic devicetree USB device bindings
>> yet which were only introduced recently (documentation is available in
>> devicetree/bindings/usb/usb-device.txt).
>> With this new driver the usb2-phy and usb3-phy can be specified directly
>> in the child-node of the corresponding port of the roothub via
>> devicetree. This can be extended by not just parsing PHYs (some of the
>> other drivers listed above are for example also parsing a list of clocks
>> as well) when required.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>> ---
>>  drivers/usb/host/Kconfig            |   3 +
>>  drivers/usb/host/Makefile           |   2 +
>>  drivers/usb/host/platform-roothub.c | 146 ++++++++++++++++++++++++++++++++++++
>>  drivers/usb/host/platform-roothub.h |  14 ++++
>>  4 files changed, 165 insertions(+)
>>  create mode 100644 drivers/usb/host/platform-roothub.c
>>  create mode 100644 drivers/usb/host/platform-roothub.h
>>
>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
>> index fa5692dec832..b8b05c786b2a 100644
>> --- a/drivers/usb/host/Kconfig
>> +++ b/drivers/usb/host/Kconfig
>> @@ -805,6 +805,9 @@ config USB_HCD_SSB
>>
>>         If unsure, say N.
>>
>> +config USB_PLATFORM_ROOTHUB
>> +     bool
>> +
>>  config USB_HCD_TEST_MODE
>>       bool "HCD test mode support"
>>       ---help---
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index cf2691fffcc0..dc817f82d632 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -29,6 +29,8 @@ obj-$(CONFIG_USB_WHCI_HCD)  += whci/
>>
>>  obj-$(CONFIG_USB_PCI)        += pci-quirks.o
>>
>> +obj-$(CONFIG_USB_PLATFORM_ROOTHUB)   += platform-roothub.o
>> +
>>  obj-$(CONFIG_USB_EHCI_HCD)   += ehci-hcd.o
>>  obj-$(CONFIG_USB_EHCI_PCI)   += ehci-pci.o
>>  obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)  += ehci-platform.o
>> diff --git a/drivers/usb/host/platform-roothub.c b/drivers/usb/host/platform-roothub.c
>> new file mode 100644
>> index 000000000000..84837e42b006
>> --- /dev/null
>> +++ b/drivers/usb/host/platform-roothub.c
>> @@ -0,0 +1,146 @@
>> +/*
>> + * platform roothub driver - a virtual PHY device which passes all phy_*
>> + * function calls to multiple (actual) PHY devices. This is comes handy when
>> + * initializing all PHYs on a root-hub (to keep them all in the same state).
>> + *
>> + * Copyright (C) 2017 Martin Blumenstingl <martin.blumenstingl@xxxxxxxxxxxxxx>
>> + *
>> + * 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.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/list.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/of.h>
>> +#include <linux/usb/of.h>
>> +
>> +#include "platform-roothub.h"
>> +
>> +#define ROOTHUB_PORTNUM              0
>> +
>> +struct platform_roothub {
>> +     struct phy              *phy;
>> +     struct list_head        list;
>> +};
>> +
>> +static struct platform_roothub *platform_roothub_alloc(struct device *dev)
>> +{
>> +     struct platform_roothub *roothub_entry;
>> +
>> +     roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
>> +     if (!roothub_entry)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     INIT_LIST_HEAD(&roothub_entry->list);
>> +
>> +     return roothub_entry;
>> +}
>> +
>> +static int platform_roothub_add_phy(struct device *dev,
>> +                                 struct device_node *port_np,
>> +                                 const char *con_id, struct list_head *list)
>> +{
>> +     struct platform_roothub *roothub_entry;
>> +     struct phy *phy = devm_of_phy_get(dev, port_np, con_id);
>> +
>> +     if (IS_ERR_OR_NULL(phy)) {
>> +             if (!phy || PTR_ERR(phy) == -ENODEV)
>> +                     return 0;
>> +             else
>> +                     return PTR_ERR(phy);
>> +     }
>> +
>> +     roothub_entry = platform_roothub_alloc(dev);
>> +     if (IS_ERR(roothub_entry))
>> +             return PTR_ERR(roothub_entry);
>> +
>> +     roothub_entry->phy = phy;
>> +
>> +     list_add_tail(&roothub_entry->list, list);
>> +
>> +     return 0;
>> +}
>> +
>> +struct platform_roothub *platform_roothub_init(struct device *dev)
>> +{
>> +     struct device_node *roothub_np, *port_np;
>> +     struct platform_roothub *plat_roothub;
>> +     int err;
>> +
>> +     roothub_np = usb_of_get_child_node(dev->of_node, ROOTHUB_PORTNUM);
>> +     if (!of_device_is_available(roothub_np))
>> +             return NULL;
>> +
>> +     plat_roothub = platform_roothub_alloc(dev);
>> +     if (IS_ERR(plat_roothub))
>> +             return plat_roothub;
>> +
>> +     for_each_available_child_of_node(roothub_np, port_np) {
>> +             err = platform_roothub_add_phy(dev, port_np, "usb2-phy",
>> +                                            &plat_roothub->list);
>> +             if (err)
>> +                     return ERR_PTR(err);
>> +
>> +             err = platform_roothub_add_phy(dev, port_np, "usb3-phy",
>> +                                            &plat_roothub->list);
>> +             if (err)
>> +                     return ERR_PTR(err);
>> +     }
>> +
>> +     return plat_roothub;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_roothub_init);
> Can we process the case without phy-names property?
> as following one, phy-names may be redundant, because the phy type is
> informed in phys property.
>
> port@2 {
>     reg = <2>;
>     phys = <&u2port1 PHY_TYPE_USB2>, <&u3port1 PHY_TYPE_USB3>;
>     phy-names = "usb2-phy", "usb3-phy";
> };
according to the PHY binding documentation the phy-names property is
mandatory: [0]
this would have to be discussed with the PHY framework maintainers

>
>> +
>> +int platform_roothub_power_on(struct platform_roothub *plat_roothub)
>> +{
>> +     struct platform_roothub *roothub_entry;
>> +     struct list_head *head;
>> +     int err;
>> +
>> +     if (!plat_roothub)
>> +             return 0;
>> +
>> +     head = &plat_roothub->list;
>> +
>> +     list_for_each_entry(roothub_entry, head, list) {
>> +             err = phy_init(roothub_entry->phy);
>> +             if (err)
>> +                     goto err_out;
>> +
>> +             err = phy_power_on(roothub_entry->phy);
>> +             if (err) {
>> +                     phy_exit(roothub_entry->phy);
>> +                     goto err_out;
>> +             }
>> +     }
>> +
>> +     return 0;
>> +
>> +err_out:
>> +     list_for_each_entry_continue_reverse(roothub_entry, head, list) {
>> +             phy_power_off(roothub_entry->phy);
>> +             phy_exit(roothub_entry->phy);
>> +     }
>> +
>> +     return err;
>> +}
>> +EXPORT_SYMBOL_GPL(platform_roothub_power_on);
>> +
>> +void platform_roothub_power_off(struct platform_roothub *plat_roothub)
>> +{
>> +     struct platform_roothub *roothub_entry;
>> +
>> +     if (!plat_roothub)
>> +             return;
>> +
>> +     list_for_each_entry_reverse(roothub_entry, &plat_roothub->list, list) {
>> +             phy_power_off(roothub_entry->phy);
>> +             phy_exit(roothub_entry->phy);
>> +     }
>> +}
>> +EXPORT_SYMBOL_GPL(platform_roothub_power_off);
> I test it, and works fine on mt8173.
many thanks!

> It will be flexible enough if some more APIs are provided, such as
> porwer_on/off all phys but not init/exit them.
> e.g. In order to keep link state on mt8173, we just power off all
> phys(not exit) when system enter suspend, then power on
> them again (needn't init, otherwise device will be disconnected) when
> system resume, this can avoid re-enumerating device.
OK, that sounds reasonable - we don't have suspend support on the
Amlogic platforms yet so I couldn't test this. I will add separate
_init and _exit functions and use them in the appropriate places in
the third patch. thanks for this suggestion!

>
>> diff --git a/drivers/usb/host/platform-roothub.h b/drivers/usb/host/platform-roothub.h
>> new file mode 100644
>> index 000000000000..bde0bf299e3b
>> --- /dev/null
>> +++ b/drivers/usb/host/platform-roothub.h
>> @@ -0,0 +1,14 @@
>> +#ifndef USB_HOST_PLATFORM_ROOTHUB_H
>> +#define USB_HOST_PLATFORM_ROOTHUB_H
>> +
>> +struct phy;
>> +struct device_node;
> These two declarations are not needed, when I remove them, and still
> compile pass.
thanks for spotting - neither of these is used inside the header, so
you are right: they should be removed

>> +
>> +struct platform_roothub;
>> +
>> +struct platform_roothub *platform_roothub_init(struct device *dev);
>> +
>> +int platform_roothub_power_on(struct platform_roothub *plat_roothub);
>> +void platform_roothub_power_off(struct platform_roothub *plat_roothub);
>> +
>> +#endif /* USB_HOST_PLATFORM_ROOTHUB_H */
>
>

I will fix these issues and send an update next weekend.
it would be great if the USB maintainers (especially Mathias Nyman,
since you're the xhci-plat.c maintainer) could comment on this series


Regards,
Martin


[0] http://elixir.free-electrons.com/linux/latest/source/Documentation/devicetree/bindings/phy/phy-bindings.txt#L33
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux