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
